[PATCH v3 0/2] Remove vfio_device_get_from_dev()

[PATCH v3 0/2] Remove vfio_device_get_from_dev()

[Jason Gunthorpe]

Use drvdata instead of searching to find the struct vfio_device for the
pci_driver callbacks.

This applies on top of the gvt series and at least rc3 - there are no
conflicts with the mdev vfio_group series, or the iommu series.

v2:
 - Directly access the drvdata from vfio_pci_core by making drvdata always
   point to the core struct. This will help later patches adding PM
   callbacks as well.
v1: https://lore.kernel.org/r/0-v2-0f36bcf6ec1e+64d-vfio_get_from_dev_jgg@nvidia.com

Cc: Abhishek Sahu <abhsahu@nvidia.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Jason Gunthorpe (2):
  vfio/pci: Have all VFIO PCI drivers store the vfio_pci_core_device in
    drvdata
  vfio/pci: Remove vfio_device_get_from_dev()

 .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c    | 14 ++++++--
 drivers/vfio/pci/mlx5/main.c                  | 14 ++++++--
 drivers/vfio/pci/vfio_pci.c                   |  4 ++-
 drivers/vfio/pci/vfio_pci_core.c              | 36 ++++++-------------
 drivers/vfio/vfio.c                           | 26 +-------------
 include/linux/vfio.h                          |  2 --
 include/linux/vfio_pci_core.h                 |  3 +-
 7 files changed, 38 insertions(+), 61 deletions(-)


base-commit: 758f579843974e9603191d2d77589af98001e3b3
-- 
2.36.0

[PATCH v3 1/2] vfio/pci: Have all VFIO PCI drivers store the vfio_pci_core_device in drvdata

[Jason Gunthorpe]

Having a consistent pointer in the drvdata will allow the next patch to
make use of the drvdata from some of the core code helpers.

Use a WARN_ON inside vfio_pci_core_enable() to detect drivers that miss
this.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 14 +++++++++++---
 drivers/vfio/pci/mlx5/main.c                   | 14 +++++++++++---
 drivers/vfio/pci/vfio_pci_core.c               |  4 ++++
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
index 767b5d47631a49..665691967a030c 100644
--- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
+++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
@@ -337,6 +337,14 @@ static int vf_qm_cache_wb(struct hisi_qm *qm)
 	return 0;
 }
 
+static struct hisi_acc_vf_core_device *hssi_acc_drvdata(struct pci_dev *pdev)
+{
+	struct vfio_pci_core_device *core_device = dev_get_drvdata(&pdev->dev);
+
+	return container_of(core_device, struct hisi_acc_vf_core_device,
+			    core_device);
+}
+
 static void vf_qm_fun_reset(struct hisi_acc_vf_core_device *hisi_acc_vdev,
 			    struct hisi_qm *qm)
 {
@@ -962,7 +970,7 @@ hisi_acc_vfio_pci_get_device_state(struct vfio_device *vdev,
 
 static void hisi_acc_vf_pci_aer_reset_done(struct pci_dev *pdev)
 {
-	struct hisi_acc_vf_core_device *hisi_acc_vdev = dev_get_drvdata(&pdev->dev);
+	struct hisi_acc_vf_core_device *hisi_acc_vdev = hssi_acc_drvdata(pdev);
 
 	if (hisi_acc_vdev->core_device.vdev.migration_flags !=
 				VFIO_MIGRATION_STOP_COPY)
@@ -1278,7 +1286,7 @@ static int hisi_acc_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device
 	if (ret)
 		goto out_free;
 
-	dev_set_drvdata(&pdev->dev, hisi_acc_vdev);
+	dev_set_drvdata(&pdev->dev, &hisi_acc_vdev->core_device);
 	return 0;
 
 out_free:
@@ -1289,7 +1297,7 @@ static int hisi_acc_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device
 
 static void hisi_acc_vfio_pci_remove(struct pci_dev *pdev)
 {
-	struct hisi_acc_vf_core_device *hisi_acc_vdev = dev_get_drvdata(&pdev->dev);
+	struct hisi_acc_vf_core_device *hisi_acc_vdev = hssi_acc_drvdata(pdev);
 
 	vfio_pci_core_unregister_device(&hisi_acc_vdev->core_device);
 	vfio_pci_core_uninit_device(&hisi_acc_vdev->core_device);
diff --git a/drivers/vfio/pci/mlx5/main.c b/drivers/vfio/pci/mlx5/main.c
index bbec5d288fee97..3391f965abd9f0 100644
--- a/drivers/vfio/pci/mlx5/main.c
+++ b/drivers/vfio/pci/mlx5/main.c
@@ -39,6 +39,14 @@ struct mlx5vf_pci_core_device {
 	struct mlx5_vf_migration_file *saving_migf;
 };
 
+static struct mlx5vf_pci_core_device *mlx5vf_drvdata(struct pci_dev *pdev)
+{
+	struct vfio_pci_core_device *core_device = dev_get_drvdata(&pdev->dev);
+
+	return container_of(core_device, struct mlx5vf_pci_core_device,
+			    core_device);
+}
+
 static struct page *
 mlx5vf_get_migration_page(struct mlx5_vf_migration_file *migf,
 			  unsigned long offset)
@@ -505,7 +513,7 @@ static int mlx5vf_pci_get_device_state(struct vfio_device *vdev,
 
 static void mlx5vf_pci_aer_reset_done(struct pci_dev *pdev)
 {
-	struct mlx5vf_pci_core_device *mvdev = dev_get_drvdata(&pdev->dev);
+	struct mlx5vf_pci_core_device *mvdev = mlx5vf_drvdata(pdev);
 
 	if (!mvdev->migrate_cap)
 		return;
@@ -618,7 +626,7 @@ static int mlx5vf_pci_probe(struct pci_dev *pdev,
 	if (ret)
 		goto out_free;
 
-	dev_set_drvdata(&pdev->dev, mvdev);
+	dev_set_drvdata(&pdev->dev, &mvdev->core_device);
 	return 0;
 
 out_free:
@@ -629,7 +637,7 @@ static int mlx5vf_pci_probe(struct pci_dev *pdev,
 
 static void mlx5vf_pci_remove(struct pci_dev *pdev)
 {
-	struct mlx5vf_pci_core_device *mvdev = dev_get_drvdata(&pdev->dev);
+	struct mlx5vf_pci_core_device *mvdev = mlx5vf_drvdata(pdev);
 
 	vfio_pci_core_unregister_device(&mvdev->core_device);
 	vfio_pci_core_uninit_device(&mvdev->core_device);
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 06b6f3594a1316..53ad39d617653d 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -262,6 +262,10 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
 	u16 cmd;
 	u8 msix_pos;
 
+	/* Drivers must set the vfio_pci_core_device to their drvdata */
+	if (WARN_ON(vdev != dev_get_drvdata(&vdev->pdev->dev)))
+		return -EINVAL;
+
 	vfio_pci_set_power_state(vdev, PCI_D0);
 
 	/* Don't allow our initial saved state to include busmaster */
-- 
2.36.0

[PATCH v3 2/2] vfio/pci: Remove vfio_device_get_from_dev()

[Jason Gunthorpe]

The last user of this function is in PCI callbacks that want to convert
their struct pci_dev to a vfio_device. Instead of searching use the
vfio_device available trivially through the drvdata.

When a callback in the device_driver is called, the caller must hold the
device_lock() on dev. The purpose of the device_lock is to prevent
remove() from being called (see __device_release_driver), and allow the
driver to safely interact with its drvdata without races.

The PCI core correctly follows this and holds the device_lock() when
calling error_detected (see report_error_detected) and
sriov_configure (see sriov_numvfs_store).

Further, since the drvdata holds a positive refcount on the vfio_device
any access of the drvdata, under the device_lock(), from a driver callback
needs no further protection or refcounting.

Thus the remark in the vfio_device_get_from_dev() comment does not apply
here, VFIO PCI drivers all call vfio_unregister_group_dev() from their
remove callbacks under the device_lock() and cannot race with the
remaining callers.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Kirti Wankhede <kwankhede@nvidia.com>
Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/pci/vfio_pci.c      |  4 +++-
 drivers/vfio/pci/vfio_pci_core.c | 32 ++++++--------------------------
 drivers/vfio/vfio.c              | 26 +-------------------------
 include/linux/vfio.h             |  2 --
 include/linux/vfio_pci_core.h    |  3 ++-
 5 files changed, 12 insertions(+), 55 deletions(-)

The only change here is that vfio_pci_core_aer_err_detected() now calls
drvdata intead of taking the right type in, and the origin aer ops struct is
retained.

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 2b047469e02fee..ba60195cefb302 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -174,10 +174,12 @@ static void vfio_pci_remove(struct pci_dev *pdev)
 
 static int vfio_pci_sriov_configure(struct pci_dev *pdev, int nr_virtfn)
 {
+	struct vfio_pci_core_device *vdev = dev_get_drvdata(&pdev->dev);
+
 	if (!enable_sriov)
 		return -ENOENT;
 
-	return vfio_pci_core_sriov_configure(pdev, nr_virtfn);
+	return vfio_pci_core_sriov_configure(vdev, nr_virtfn);
 }
 
 static const struct pci_device_id vfio_pci_table[] = {
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 53ad39d617653d..728756d0a6fd87 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -1894,9 +1894,7 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_register_device);
 
 void vfio_pci_core_unregister_device(struct vfio_pci_core_device *vdev)
 {
-	struct pci_dev *pdev = vdev->pdev;
-
-	vfio_pci_core_sriov_configure(pdev, 0);
+	vfio_pci_core_sriov_configure(vdev, 0);
 
 	vfio_unregister_group_dev(&vdev->vdev);
 
@@ -1911,14 +1909,7 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_unregister_device);
 pci_ers_result_t vfio_pci_core_aer_err_detected(struct pci_dev *pdev,
 						pci_channel_state_t state)
 {
-	struct vfio_pci_core_device *vdev;
-	struct vfio_device *device;
-
-	device = vfio_device_get_from_dev(&pdev->dev);
-	if (device == NULL)
-		return PCI_ERS_RESULT_DISCONNECT;
-
-	vdev = container_of(device, struct vfio_pci_core_device, vdev);
+	struct vfio_pci_core_device *vdev = dev_get_drvdata(&pdev->dev);
 
 	mutex_lock(&vdev->igate);
 
@@ -1927,26 +1918,18 @@ pci_ers_result_t vfio_pci_core_aer_err_detected(struct pci_dev *pdev,
 
 	mutex_unlock(&vdev->igate);
 
-	vfio_device_put(device);
-
 	return PCI_ERS_RESULT_CAN_RECOVER;
 }
 EXPORT_SYMBOL_GPL(vfio_pci_core_aer_err_detected);
 
-int vfio_pci_core_sriov_configure(struct pci_dev *pdev, int nr_virtfn)
+int vfio_pci_core_sriov_configure(struct vfio_pci_core_device *vdev,
+				  int nr_virtfn)
 {
-	struct vfio_pci_core_device *vdev;
-	struct vfio_device *device;
+	struct pci_dev *pdev = vdev->pdev;
 	int ret = 0;
 
 	device_lock_assert(&pdev->dev);
 
-	device = vfio_device_get_from_dev(&pdev->dev);
-	if (!device)
-		return -ENODEV;
-
-	vdev = container_of(device, struct vfio_pci_core_device, vdev);
-
 	if (nr_virtfn) {
 		mutex_lock(&vfio_pci_sriov_pfs_mutex);
 		/*
@@ -1964,8 +1947,7 @@ int vfio_pci_core_sriov_configure(struct pci_dev *pdev, int nr_virtfn)
 		ret = pci_enable_sriov(pdev, nr_virtfn);
 		if (ret)
 			goto out_del;
-		ret = nr_virtfn;
-		goto out_put;
+		return nr_virtfn;
 	}
 
 	pci_disable_sriov(pdev);
@@ -1975,8 +1957,6 @@ int vfio_pci_core_sriov_configure(struct pci_dev *pdev, int nr_virtfn)
 	list_del_init(&vdev->sriov_pfs_item);
 out_unlock:
 	mutex_unlock(&vfio_pci_sriov_pfs_mutex);
-out_put:
-	vfio_device_put(device);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(vfio_pci_core_sriov_configure);
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index a4555014bd1e72..74e4197105904e 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -492,12 +492,11 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev)
  * Device objects - create, release, get, put, search
  */
 /* Device reference always implies a group reference */
-void vfio_device_put(struct vfio_device *device)
+static void vfio_device_put(struct vfio_device *device)
 {
 	if (refcount_dec_and_test(&device->refcount))
 		complete(&device->comp);
 }
-EXPORT_SYMBOL_GPL(vfio_device_put);
 
 static bool vfio_device_try_get(struct vfio_device *device)
 {
@@ -831,29 +830,6 @@ int vfio_register_emulated_iommu_dev(struct vfio_device *device)
 }
 EXPORT_SYMBOL_GPL(vfio_register_emulated_iommu_dev);
 
-/*
- * Get a reference to the vfio_device for a device.  Even if the
- * caller thinks they own the device, they could be racing with a
- * release call path, so we can't trust drvdata for the shortcut.
- * Go the long way around, from the iommu_group to the vfio_group
- * to the vfio_device.
- */
-struct vfio_device *vfio_device_get_from_dev(struct device *dev)
-{
-	struct vfio_group *group;
-	struct vfio_device *device;
-
-	group = vfio_group_get_from_dev(dev);
-	if (!group)
-		return NULL;
-
-	device = vfio_group_get_device(group, dev);
-	vfio_group_put(group);
-
-	return device;
-}
-EXPORT_SYMBOL_GPL(vfio_device_get_from_dev);
-
 static struct vfio_device *vfio_device_get_from_name(struct vfio_group *group,
 						     char *buf)
 {
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 66dda06ec42d1b..ae06731d947bf7 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -125,8 +125,6 @@ void vfio_uninit_group_dev(struct vfio_device *device);
 int vfio_register_group_dev(struct vfio_device *device);
 int vfio_register_emulated_iommu_dev(struct vfio_device *device);
 void vfio_unregister_group_dev(struct vfio_device *device);
-extern struct vfio_device *vfio_device_get_from_dev(struct device *dev);
-extern void vfio_device_put(struct vfio_device *device);
 
 int vfio_assign_device_set(struct vfio_device *device, void *set_id);
 
diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
index 48f2dd3c568c83..23c176d4b073f1 100644
--- a/include/linux/vfio_pci_core.h
+++ b/include/linux/vfio_pci_core.h
@@ -227,8 +227,9 @@ void vfio_pci_core_init_device(struct vfio_pci_core_device *vdev,
 int vfio_pci_core_register_device(struct vfio_pci_core_device *vdev);
 void vfio_pci_core_uninit_device(struct vfio_pci_core_device *vdev);
 void vfio_pci_core_unregister_device(struct vfio_pci_core_device *vdev);
-int vfio_pci_core_sriov_configure(struct pci_dev *pdev, int nr_virtfn);
 extern const struct pci_error_handlers vfio_pci_core_err_handlers;
+int vfio_pci_core_sriov_configure(struct vfio_pci_core_device *vdev,
+				  int nr_virtfn);
 long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd,
 		unsigned long arg);
 int vfio_pci_core_ioctl_feature(struct vfio_device *device, u32 flags,
-- 
2.36.0

Re: [PATCH v3 0/2] Remove vfio_device_get_from_dev()

[Jason Gunthorpe]

On Wed, May 04, 2022 at 04:01:46PM -0300, Jason Gunthorpe wrote:
> Use drvdata instead of searching to find the struct vfio_device for the
> pci_driver callbacks.
> 
> This applies on top of the gvt series and at least rc3 - there are no
> conflicts with the mdev vfio_group series, or the iommu series.
> 
> v2:
>  - Directly access the drvdata from vfio_pci_core by making drvdata always
>    point to the core struct. This will help later patches adding PM
>    callbacks as well.
> v1: https://lore.kernel.org/r/0-v2-0f36bcf6ec1e+64d-vfio_get_from_dev_jgg@nvidia.com

Hum, I fumbled this a bit when extracting the old version. It is
rebased on the wrong thing, and this is indeed v3.

Alex, I can resend it, or if you want to take it then this hunk from
v2 is needed to put it on top of the mdev group removal series:

@@ -444,21 +444,6 @@ static void vfio_group_get(struct vfio_group *group)
 	refcount_inc(&group->users);
 }
 
-static struct vfio_group *vfio_group_get_from_dev(struct device *dev)
-{
-	struct iommu_group *iommu_group;
-	struct vfio_group *group;
-
-	iommu_group = iommu_group_get(dev);
-	if (!iommu_group)
-		return NULL;
-
-	group = vfio_group_get_from_iommu(iommu_group);
-	iommu_group_put(iommu_group);
-
-	return group;
-}
-
 /*
  * Device objects - create, release, get, put, search
  */

Sorry,
Jason

Re: [PATCH v3 2/2] vfio/pci: Remove vfio_device_get_from_dev()

[Kirti Wankhede]

On 5/5/2022 12:31 AM, Jason Gunthorpe wrote:
> The last user of this function is in PCI callbacks that want to convert
> their struct pci_dev to a vfio_device. Instead of searching use the
> vfio_device available trivially through the drvdata.
> 
> When a callback in the device_driver is called, the caller must hold the
> device_lock() on dev. The purpose of the device_lock is to prevent
> remove() from being called (see __device_release_driver), and allow the
> driver to safely interact with its drvdata without races.
> 
> The PCI core correctly follows this and holds the device_lock() when
> calling error_detected (see report_error_detected) and
> sriov_configure (see sriov_numvfs_store).
> 
> Further, since the drvdata holds a positive refcount on the vfio_device
> any access of the drvdata, under the device_lock(), from a driver callback
> needs no further protection or refcounting.
> 
> Thus the remark in the vfio_device_get_from_dev() comment does not apply
> here, VFIO PCI drivers all call vfio_unregister_group_dev() from their
> remove callbacks under the device_lock() and cannot race with the
> remaining callers.
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Kirti Wankhede <kwankhede@nvidia.com>
> Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

My ack was on previous version where drvdata is set and used in same module.
  https://www.spinics.net/lists/kvm/msg275737.html

Its not a good practice to force vfio_pci_core vendor drivers to set a 
particular structure pointer in drvdata. drvdata set from a driver 
should be used by same driver and other driver should not assume/rely on it.

I still prefer earlier version.

Thanks,
Kirti

Re: [PATCH v3 2/2] vfio/pci: Remove vfio_device_get_from_dev()

[Jason Gunthorpe]

On Thu, May 05, 2022 at 01:50:45AM +0530, Kirti Wankhede wrote:

> Its not a good practice to force vfio_pci_core vendor drivers to set a
> particular structure pointer in drvdata. drvdata set from a driver should be
> used by same driver and other driver should not assume/rely on it.

Abhishek's series is adding quite a few more callbacks to the
pci_driver that the sub drivers have no need to override. Wrappering
everything is much worse overall, especially as we add new drivers. It
was Ok in the v1 of this series because only 1 function needed
wrappering.

Jason

Re: [PATCH v3 1/2] vfio/pci: Have all VFIO PCI drivers store the vfio_pci_core_device in drvdata

[Alex Williamson]

On Wed,  4 May 2022 16:01:47 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> Having a consistent pointer in the drvdata will allow the next patch to
> make use of the drvdata from some of the core code helpers.
> 
> Use a WARN_ON inside vfio_pci_core_enable() to detect drivers that miss
> this.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 14 +++++++++++---
>  drivers/vfio/pci/mlx5/main.c                   | 14 +++++++++++---
>  drivers/vfio/pci/vfio_pci_core.c               |  4 ++++
>  3 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> index 767b5d47631a49..665691967a030c 100644
> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> @@ -337,6 +337,14 @@ static int vf_qm_cache_wb(struct hisi_qm *qm)
>  	return 0;
>  }
>  
> +static struct hisi_acc_vf_core_device *hssi_acc_drvdata(struct pci_dev *pdev)
> +{
> +	struct vfio_pci_core_device *core_device = dev_get_drvdata(&pdev->dev);
> +
> +	return container_of(core_device, struct hisi_acc_vf_core_device,
> +			    core_device);
> +}
> +
>  static void vf_qm_fun_reset(struct hisi_acc_vf_core_device *hisi_acc_vdev,
>  			    struct hisi_qm *qm)
>  {
> @@ -962,7 +970,7 @@ hisi_acc_vfio_pci_get_device_state(struct vfio_device *vdev,
>  
>  static void hisi_acc_vf_pci_aer_reset_done(struct pci_dev *pdev)
>  {
> -	struct hisi_acc_vf_core_device *hisi_acc_vdev = dev_get_drvdata(&pdev->dev);
> +	struct hisi_acc_vf_core_device *hisi_acc_vdev = hssi_acc_drvdata(pdev);
>  
>  	if (hisi_acc_vdev->core_device.vdev.migration_flags !=
>  				VFIO_MIGRATION_STOP_COPY)
> @@ -1278,7 +1286,7 @@ static int hisi_acc_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device
>  	if (ret)
>  		goto out_free;
>  
> -	dev_set_drvdata(&pdev->dev, hisi_acc_vdev);
> +	dev_set_drvdata(&pdev->dev, &hisi_acc_vdev->core_device);
>  	return 0;
>  
>  out_free:
> @@ -1289,7 +1297,7 @@ static int hisi_acc_vfio_pci_probe(struct pci_dev *pdev, const struct pci_device
>  
>  static void hisi_acc_vfio_pci_remove(struct pci_dev *pdev)
>  {
> -	struct hisi_acc_vf_core_device *hisi_acc_vdev = dev_get_drvdata(&pdev->dev);
> +	struct hisi_acc_vf_core_device *hisi_acc_vdev = hssi_acc_drvdata(pdev);
>  
>  	vfio_pci_core_unregister_device(&hisi_acc_vdev->core_device);
>  	vfio_pci_core_uninit_device(&hisi_acc_vdev->core_device);
> diff --git a/drivers/vfio/pci/mlx5/main.c b/drivers/vfio/pci/mlx5/main.c
> index bbec5d288fee97..3391f965abd9f0 100644
> --- a/drivers/vfio/pci/mlx5/main.c
> +++ b/drivers/vfio/pci/mlx5/main.c
> @@ -39,6 +39,14 @@ struct mlx5vf_pci_core_device {
>  	struct mlx5_vf_migration_file *saving_migf;
>  };
>  
> +static struct mlx5vf_pci_core_device *mlx5vf_drvdata(struct pci_dev *pdev)
> +{
> +	struct vfio_pci_core_device *core_device = dev_get_drvdata(&pdev->dev);
> +
> +	return container_of(core_device, struct mlx5vf_pci_core_device,
> +			    core_device);
> +}
> +
>  static struct page *
>  mlx5vf_get_migration_page(struct mlx5_vf_migration_file *migf,
>  			  unsigned long offset)
> @@ -505,7 +513,7 @@ static int mlx5vf_pci_get_device_state(struct vfio_device *vdev,
>  
>  static void mlx5vf_pci_aer_reset_done(struct pci_dev *pdev)
>  {
> -	struct mlx5vf_pci_core_device *mvdev = dev_get_drvdata(&pdev->dev);
> +	struct mlx5vf_pci_core_device *mvdev = mlx5vf_drvdata(pdev);
>  
>  	if (!mvdev->migrate_cap)
>  		return;
> @@ -618,7 +626,7 @@ static int mlx5vf_pci_probe(struct pci_dev *pdev,
>  	if (ret)
>  		goto out_free;
>  
> -	dev_set_drvdata(&pdev->dev, mvdev);
> +	dev_set_drvdata(&pdev->dev, &mvdev->core_device);
>  	return 0;
>  
>  out_free:
> @@ -629,7 +637,7 @@ static int mlx5vf_pci_probe(struct pci_dev *pdev,
>  
>  static void mlx5vf_pci_remove(struct pci_dev *pdev)
>  {
> -	struct mlx5vf_pci_core_device *mvdev = dev_get_drvdata(&pdev->dev);
> +	struct mlx5vf_pci_core_device *mvdev = mlx5vf_drvdata(pdev);
>  
>  	vfio_pci_core_unregister_device(&mvdev->core_device);
>  	vfio_pci_core_uninit_device(&mvdev->core_device);
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 06b6f3594a1316..53ad39d617653d 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -262,6 +262,10 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
>  	u16 cmd;
>  	u8 msix_pos;
>  
> +	/* Drivers must set the vfio_pci_core_device to their drvdata */
> +	if (WARN_ON(vdev != dev_get_drvdata(&vdev->pdev->dev)))
> +		return -EINVAL;
> +

The ordering seems off, if we only validate in the core enable function
then we can only guarantee drvdata is correct once the user has opened
the device.  However, we start invoking power management controls,
which Abhishek proposes moving to runtime pm, from the core register
device function.  Therefore we've not validated drvdata for anything we
might do in the background, not under the direction of the user.

I'd also rather see the variant driver fail to register with the core
than to see a failure opening the device an arbitrary time later.
Thanks,

Alex

>  	vfio_pci_set_power_state(vdev, PCI_D0);
>  
>  	/* Don't allow our initial saved state to include busmaster */

Re: [PATCH v3 1/2] vfio/pci: Have all VFIO PCI drivers store the vfio_pci_core_device in drvdata

[Jason Gunthorpe]

On Thu, May 05, 2022 at 12:10:47PM -0600, Alex Williamson wrote:

> > +	/* Drivers must set the vfio_pci_core_device to their drvdata */
> > +	if (WARN_ON(vdev != dev_get_drvdata(&vdev->pdev->dev)))
> > +		return -EINVAL;
> > +
> 
> The ordering seems off, if we only validate in the core enable function
> then we can only guarantee drvdata is correct once the user has opened
> the device.  However, we start invoking power management controls,
> which Abhishek proposes moving to runtime pm, from the core register
> device function.  Therefore we've not validated drvdata for anything we
> might do in the background, not under the direction of the user.

It is just a guard to make it obvious to someone testing the driver
that something has gone wrong, ie in backporting or something. 

It is not intended to be protective against drivers that are actually
wrong and installed in the system.

I added this because I felt a driver could silently be wrong and never
hit a PM or AER callback during some basic testing to catch a crash or
whatever.

> I'd also rather see the variant driver fail to register with the core
> than to see a failure opening the device an arbitrary time later.

It still permits a driver to be wrong, eg all the drivers are like
this today:

	ret = vfio_pci_core_register_device(&hisi_acc_vdev->core_device);
	if (ret)
		goto out_free;
	dev_set_drvdata(&pdev->dev, &hisi_acc_vdev->core_device);

So a WARN_ON inside register_device will not catch the mistake, as
this is the common pattern it isn't as helpful.

Jason

Re: [PATCH v3 1/2] vfio/pci: Have all VFIO PCI drivers store the vfio_pci_core_device in drvdata

[Alex Williamson]

On Thu, 5 May 2022 15:34:21 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Thu, May 05, 2022 at 12:10:47PM -0600, Alex Williamson wrote:
> 
> > > +	/* Drivers must set the vfio_pci_core_device to their drvdata */
> > > +	if (WARN_ON(vdev != dev_get_drvdata(&vdev->pdev->dev)))
> > > +		return -EINVAL;
> > > +  
> > 
> > The ordering seems off, if we only validate in the core enable function
> > then we can only guarantee drvdata is correct once the user has opened
> > the device.  However, we start invoking power management controls,
> > which Abhishek proposes moving to runtime pm, from the core register
> > device function.  Therefore we've not validated drvdata for anything we
> > might do in the background, not under the direction of the user.  
> 
> It is just a guard to make it obvious to someone testing the driver
> that something has gone wrong, ie in backporting or something. 
> 
> It is not intended to be protective against drivers that are actually
> wrong and installed in the system.
> 
> I added this because I felt a driver could silently be wrong and never
> hit a PM or AER callback during some basic testing to catch a crash or
> whatever.

Then the earlier we do the sanity test such that it fails simply by
loading the driver, the better, right?
 
> > I'd also rather see the variant driver fail to register with the core
> > than to see a failure opening the device an arbitrary time later.  
> 
> It still permits a driver to be wrong, eg all the drivers are like
> this today:
> 
> 	ret = vfio_pci_core_register_device(&hisi_acc_vdev->core_device);
> 	if (ret)
> 		goto out_free;
> 	dev_set_drvdata(&pdev->dev, &hisi_acc_vdev->core_device);
> 
> So a WARN_ON inside register_device will not catch the mistake, as
> this is the common pattern it isn't as helpful.

In the above case the WARN_ON would trigger because drvdata isn't set
to the core device for the registration call.  Yes, a driver could
still set drvdata after the registration call, but then they get to
explain why they set drvdata twice and thought they could get away with
changing it after the core made pretty clear that it wants a specific
thing there.  Thanks,

Alex

Re: [PATCH v3 1/2] vfio/pci: Have all VFIO PCI drivers store the vfio_pci_core_device in drvdata

[Jason Gunthorpe]

On Thu, May 05, 2022 at 12:54:09PM -0600, Alex Williamson wrote:
> > > I'd also rather see the variant driver fail to register with the core
> > > than to see a failure opening the device an arbitrary time later.  
> > 
> > It still permits a driver to be wrong, eg all the drivers are like
> > this today:
> > 
> > 	ret = vfio_pci_core_register_device(&hisi_acc_vdev->core_device);
> > 	if (ret)
> > 		goto out_free;
> > 	dev_set_drvdata(&pdev->dev, &hisi_acc_vdev->core_device);
> > 
> > So a WARN_ON inside register_device will not catch the mistake, as
> > this is the common pattern it isn't as helpful.
> 
> In the above case the WARN_ON would trigger because drvdata isn't set
> to the core device for the registration call.  Yes, a driver could
> still set drvdata after the registration call, but then they get to
> explain why they set drvdata twice and thought they could get away with
> changing it after the core made pretty clear that it wants a specific
> thing there.  Thanks,

I'm OK with this idea, it is not quite a nice to set the drvdata in
the middle of the probe function, but it does work here.

I'll fix it

Jason

Re: [PATCH v3 2/2] vfio/pci: Remove vfio_device_get_from_dev()

[Alex Williamson]

On Thu, 5 May 2022 01:50:45 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 5/5/2022 12:31 AM, Jason Gunthorpe wrote:
> > The last user of this function is in PCI callbacks that want to convert
> > their struct pci_dev to a vfio_device. Instead of searching use the
> > vfio_device available trivially through the drvdata.
> > 
> > When a callback in the device_driver is called, the caller must hold the
> > device_lock() on dev. The purpose of the device_lock is to prevent
> > remove() from being called (see __device_release_driver), and allow the
> > driver to safely interact with its drvdata without races.
> > 
> > The PCI core correctly follows this and holds the device_lock() when
> > calling error_detected (see report_error_detected) and
> > sriov_configure (see sriov_numvfs_store).
> > 
> > Further, since the drvdata holds a positive refcount on the vfio_device
> > any access of the drvdata, under the device_lock(), from a driver callback
> > needs no further protection or refcounting.
> > 
> > Thus the remark in the vfio_device_get_from_dev() comment does not apply
> > here, VFIO PCI drivers all call vfio_unregister_group_dev() from their
> > remove callbacks under the device_lock() and cannot race with the
> > remaining callers.
> > 
> > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> > Reviewed-by: Kirti Wankhede <kwankhede@nvidia.com>
> > Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>  
> 
> My ack was on previous version where drvdata is set and used in same module.
>   https://www.spinics.net/lists/kvm/msg275737.html
> 
> Its not a good practice to force vfio_pci_core vendor drivers to set a 
> particular structure pointer in drvdata. drvdata set from a driver 
> should be used by same driver and other driver should not assume/rely on it.
> 
> I still prefer earlier version.

Kirti,

Do you still have an objection to your previous ack being carried
forward?  Thanks,

Alex