Received-SPF: pass (google.com: domain of linux-crypto+bounces-1878-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 6 Feb 2024 08:55:00 -0400
From: Jason Gunthorpe <jgg@nvidia.com>
To: Xin Zeng <xin.zeng@intel.com>
Cc: herbert@gondor.apana.org.au, alex.williamson@redhat.com,
	yishaih@nvidia.com, shameerali.kolothum.thodi@huawei.com,
	kevin.tian@intel.com, linux-crypto@vger.kernel.org,
	kvm@vger.kernel.org, qat-linux@intel.com,
	Yahui Cao <yahui.cao@intel.com>
Subject: Re: [PATCH 10/10] vfio/qat: Add vfio_pci driver for Intel QAT VF
 devices
Message-ID: <20240206125500.GC10476@nvidia.com>
References: <20240201153337.4033490-1-xin.zeng@intel.com>
 <20240201153337.4033490-11-xin.zeng@intel.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240201153337.4033490-11-xin.zeng@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?bFvk1PsV/WOLVWQupHAVnjZcJlXDpHz3XNu3uf5COgc/CT15EmI/dvnzu56s?=
 =?us-ascii?Q?MlVwcDrBGbac64WLSiJlruWhivlxSy9Xnufu3H0xDOAqwTp7k7G2pG3iu401?=
 =?us-ascii?Q?IJokpV5dEPBx3vwL0AHqNTA083Muqbb2lfy/HAOSgst57CesNCdD7UIGGnzM?=
 =?us-ascii?Q?D+g72eHqrnnltCby21rjRzh38ibJj4RHX0+kwvgQJ3JYmcjV7TX9Mfs9MiFt?=
 =?us-ascii?Q?3aja8BwByc8pOblWJ9IDloxDeWDctReSWN5/lI4KDRFORECHIKGBQsq63cdg?=
 =?us-ascii?Q?d0Fj8mGRhIjbZ8MX6exFUh2ihVTyAKLnJ/6mfqCKwZCMonBIlyAACbdZjXCZ?=
 =?us-ascii?Q?9gIeGdj8KWcLx3jdNoMv/2TlAoISQvTsSGKqdUZ6Yxk6kVEmlpmFkkGZLOJu?=
 =?us-ascii?Q?FQe3TNW0qXTKO62vUyHvnRu2LtEKCIuRqbewFzSTYo72HIPXZQXazXR1PeHL?=
 =?us-ascii?Q?Nm5eB2sqBi4W0rA4ClyYtBcNaJifCOmrq3Skg1BmMMEiSysqgFEpQMZ++1eG?=
 =?us-ascii?Q?vfGhLWLp5t6m7I7C3qO/qOHvsAduIchz3ERvVNYi6EhwTKAx32ax3GNvp+J2?=
 =?us-ascii?Q?y8TK7uaJypFSq/1htVHJGdPvWdq8Ni6AfmdW1k8BoRzBUVoFTpatYkAFi8W4?=
 =?us-ascii?Q?/shiHCS9Gdcc9lGn7t0Z8xcGVZWdW6UA6pPV1Ufj0wJ6yIzRkB2L8YKO5zKI?=
 =?us-ascii?Q?j8VK3vtZtOk9K0T194oW/weQ8CTBvzZY6dqZFRoZkRcS8RgzUacQB8aBYoKr?=
 =?us-ascii?Q?+tHg2t+aVTlUENYon8d6oezl6+2KyvUrDXsVL05zMmlWJ85l8Yolz+pOZFcB?=
 =?us-ascii?Q?T6Cz+IuZfydtuNMZ01Fezk8Xe2QLzPfqOBejnm623p69YlANDpijVJVwD3Xl?=
 =?us-ascii?Q?kIM9HzxB+pmVju6QcWpMy66n184OBg/AFlcEVEkXFO/WLAbuLpsIhvGZ45Y5?=
 =?us-ascii?Q?sjISv3tbm2exLWIu/ECyoy87oK3GgfXX84C+8ilR5HLRwZ2U4pNZ9EVRTqcN?=
 =?us-ascii?Q?73X05f0X0psGagV3MKwh7t+QRhCRPAN8r6W+tXTcRuVVwdpMxfsmxqfOvn6F?=
 =?us-ascii?Q?G2dEGBGZejLHm5zGPTlrnZaUnwjZalwtpZ5pd22PAToIgijUF+mUAeCQRTLb?=
 =?us-ascii?Q?WELzbfA5LUdLG/IlcF/SsOin3hgv1tbDwQqhlczZlt8mn0m3QetM2Tn7l/ur?=
 =?us-ascii?Q?JUgXYRx51HKb+nuEJ1xy6v+Ltwts4lvgI6rguQbfQeqY7dxf8/QhE8m7fpUy?=
 =?us-ascii?Q?OhLNGtFzv7vBpjGuwZ9IpVY3QXQ9//GxOQYCOpHmG98pkUENVLi5istLAdCB?=
 =?us-ascii?Q?1xHgsCJKyxwGScwjotvYBIwUGlUL6gmgRKi0/BcfpU15rFhjMZy80bYDxIQ3?=
 =?us-ascii?Q?AOWJCl9UMw80edGAcoq23O1UBoJc1qz3V4qZJOfZk7bqQ0IWguTFVPD8AlZf?=
 =?us-ascii?Q?mHI8q9hnbvd+K8ogIaaiQtLV6gAekwNkHL08HRXr7EMG60gVUS8nHoZmACmH?=
 =?us-ascii?Q?IESj5nr/l97/uAQy/sZBDAskyvjNituSC48rtq3pHI4xstqipgPci4cvogNE?=
 =?us-ascii?Q?E1EepfbMlAefhUoKv/U=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bfea4604-692c-4ed8-ff9e-08dc2712d502
X-MS-Exchange-CrossTenant-AuthSource: LV2PR12MB5869.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Feb 2024 12:55:01.7496
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: S/+iUhTMSBsfLazB9e7V/9T3t3Le/kfkWKMi32CBzRxSluIAsx3vMe48vpZSpAjp
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR12MB4539

On Thu, Feb 01, 2024 at 11:33:37PM +0800, Xin Zeng wrote:

> +static int qat_vf_pci_init_dev(struct vfio_device *core_vdev)
> +{
> +	struct qat_vf_core_device *qat_vdev = container_of(core_vdev,
> +			struct qat_vf_core_device, core_device.vdev);
> +	struct qat_migdev_ops *ops;
> +	struct qat_mig_dev *mdev;
> +	struct pci_dev *parent;
> +	int ret, vf_id;
> +
> +	core_vdev->migration_flags = VFIO_MIGRATION_STOP_COPY | VFIO_MIGRATION_P2P;
> +	core_vdev->mig_ops = &qat_vf_pci_mig_ops;
> +
> +	ret = vfio_pci_core_init_dev(core_vdev);
> +	if (ret)
> +		return ret;
> +
> +	mutex_init(&qat_vdev->state_mutex);
> +	spin_lock_init(&qat_vdev->reset_lock);
> +
> +	parent = qat_vdev->core_device.pdev->physfn;
> +	vf_id = pci_iov_vf_id(qat_vdev->core_device.pdev);
> +	if (!parent || vf_id < 0) {
> +		ret = -ENODEV;
> +		goto err_rel;
> +	}
> +
> +	mdev = qat_vfmig_create(parent, vf_id);
> +	if (IS_ERR(mdev)) {
> +		ret = PTR_ERR(mdev);
> +		goto err_rel;
> +	}
> +
> +	ops = mdev->ops;
> +	if (!ops || !ops->init || !ops->cleanup ||
> +	    !ops->open || !ops->close ||
> +	    !ops->save_state || !ops->load_state ||
> +	    !ops->suspend || !ops->resume) {
> +		ret = -EIO;
> +		dev_err(&parent->dev, "Incomplete device migration ops structure!");
> +		goto err_destroy;
> +	}

Why are there ops pointers here? I would expect this should just be
direct function calls to the PF QAT driver.

> +static void qat_vf_pci_aer_reset_done(struct pci_dev *pdev)
> +{
> +	struct qat_vf_core_device *qat_vdev = qat_vf_drvdata(pdev);
> +
> +	if (!qat_vdev->core_device.vdev.mig_ops)
> +		return;
> +
> +	/*
> +	 * As the higher VFIO layers are holding locks across reset and using
> +	 * those same locks with the mm_lock we need to prevent ABBA deadlock
> +	 * with the state_mutex and mm_lock.
> +	 * In case the state_mutex was taken already we defer the cleanup work
> +	 * to the unlock flow of the other running context.
> +	 */
> +	spin_lock(&qat_vdev->reset_lock);
> +	qat_vdev->deferred_reset = true;
> +	if (!mutex_trylock(&qat_vdev->state_mutex)) {
> +		spin_unlock(&qat_vdev->reset_lock);
> +		return;
> +	}
> +	spin_unlock(&qat_vdev->reset_lock);
> +	qat_vf_state_mutex_unlock(qat_vdev);
> +}

Do you really need this? I thought this ugly thing was going to be a
uniquely mlx5 thing..

Jason