[PATCH 00/10] Embed struct vfio_device in all sub-structures

[PATCH 00/10] Embed struct vfio_device in all sub-structures

[Jason Gunthorpe]

Prologue
========

This series is part of a larger work that arose from the minor remark that
the mdev_parent_ops indirection shim is useless and complicates
things.

The entire project is about 70 patches broken into 5 subseries, each on a
theme:

#1 - (this series) Add type safety to the core VFIO
#2 - Add type safety to MDEV

  The mdev transformation is involved, compiler assistance through actual
  static type checking makes the transformation much more reliable, thus
  the first two steps add most of the missing types.

#3 - Make all mdev drivers register directly with the core code,
     delete vfio_mdev.c

#4 - Various minor tidies that arise from the above three series

#5 - Complete type annotations and remove unused code

A preview of the future series's is here:
  https://github.com/jgunthorpe/linux/pull/3/commits

It turns out a bunch of stuff exists in the way it does because the
'struct vfio_device' was not obviously available in places that naturally
wanted it. Across the project the following APIs are deleted as reorg
removes all the users:

   mdev_uuid()
   mdev_dev()
   mdev_get_drvdata()
   mdev_set_drvdata()
   struct mdev_parent_ops
   vfio_iommu_group_get()
   vfio_iommu_group_put(),
   vfio_group_get_external_user_from_dev()
   vfio_group_pin_pages()
   vfio_group_unpin_pages()
   vfio_group_get()
   vfio_device_data()

The remaining vfio_device related APIs in mdev.h and vfio.h have correct,
specific, types instead of 'void *' or 'struct device *'.

This work is related to, but seperate from, Max's series to split
vfio_pci. When layered on this vfio_pci_core will use a similiar
container_of scheme and layer the ultimate end-driver with container_of
all the way back to a vfio_device. Types are explicit and natural to
understand through all the layers.

Further mdev and pci get a similiar design with a set of core code
supporting normal 'struct device_driver's that directly create
vfio_device's.

In essence vfio becomes close to a normal driver subsystem pattern with a
bunch of device drivers creating vfio_devices'

========
This series:

The main focus of this series is to make VFIO follow the normal kernel
convention of structure embedding for structure inheritance instead of
linking using a 'void *opaque'. Here we focus on moving the vfio_device to
be a member of every struct vfio_XX_device that is linked by a
vfio_add_group_dev().

In turn this allows 'struct vfio_device *' to be used everwhere, and the
public API out of vfio.c can be cleaned to remove places using 'struct
device *' and 'void *' as surrogates to refer to the device.

While this has the minor trade off of moving 'struct vfio_device' the
clarity of the design is worth it. I can speak directly to this idea, as
I've invested a fair amount of time carefully working backwards what all
the type-erased APIs are supposed to be and it is certainly not trivial or
intuitive.

When we get into mdev land things become even more inscrutable, and while
I now have a pretty clear picture, it was hard to obtain. I think this
agrees with the kernel style ideal of being explicit in typing and not
sacrificing clarity to create opaque structs.

After this series the general rules are:
 - Any vfio_XX_device * can be obtained at no cost from a vfio_device *
   using container_of(), and the reverse is possible by &XXdev->vdev

   This is similar to how 'struct pci_device' and 'struct device' are
   interrelated.

   This allows 'device_data' to be completely removed from the vfio.c API.

 - The drvdata for a struct device points at the vfio_XX_device that
   belongs to the driver that was probed. drvdata is removed from the core
   code, and only used as part of the implementation of the struct
   device_driver.

 - The lifetime of vfio_XX_device and vfio_device are identical, they are
   the same memory.

   This follows the existing model where vfio_del_group_dev() blocks until
   all vfio_device_put()'s are completed. This in turn means the struct
   device_driver remove() blocks, and thus under the driver_lock() a bound
   driver must have a valid drvdata pointing at both vfio device
   structs. A following series exploits this further.

Most vfio_XX_device structs have data that duplicates the 'struct
device *dev' member of vfio_device, a following series removes that
duplication too.

Jason

Jason Gunthorpe (10):
  vfio: Simplify the lifetime logic for vfio_device
  vfio: Split creation of a vfio_device into init and register ops
  vfio/platform: Use vfio_init/register/unregister_group_dev
  vfio/fsl-mc: Use vfio_init/register/unregister_group_dev
  vfio/pci: Use vfio_init/register/unregister_group_dev
  vfio/mdev: Use vfio_init/register/unregister_group_dev
  vfio/mdev: Make to_mdev_device() into a static inline
  vfio: Make vfio_device_ops pass a 'struct vfio_device *' instead of
    'void *'
  vfio/pci: Replace uses of vfio_device_data() with container_of
  vfio: Remove device_data from the vfio bus driver API

 Documentation/driver-api/vfio.rst             |  48 ++--
 drivers/vfio/fsl-mc/vfio_fsl_mc.c             |  69 +++---
 drivers/vfio/fsl-mc/vfio_fsl_mc_private.h     |   1 +
 drivers/vfio/mdev/mdev_private.h              |   5 +-
 drivers/vfio/mdev/vfio_mdev.c                 |  57 +++--
 drivers/vfio/pci/vfio_pci.c                   | 109 +++++----
 drivers/vfio/pci/vfio_pci_private.h           |   1 +
 drivers/vfio/platform/vfio_amba.c             |   8 +-
 drivers/vfio/platform/vfio_platform.c         |  21 +-
 drivers/vfio/platform/vfio_platform_common.c  |  56 ++---
 drivers/vfio/platform/vfio_platform_private.h |   5 +-
 drivers/vfio/vfio.c                           | 210 ++++++------------
 include/linux/vfio.h                          |  37 +--
 13 files changed, 299 insertions(+), 328 deletions(-)

-- 
2.30.1

[PATCH 01/10] vfio: Simplify the lifetime logic for vfio_device

[Jason Gunthorpe]

The vfio_device is using a 'sleep until all refs go to zero' pattern for
its lifetime, but it is indirectly coded by repeatedly scanning the group
list waiting for the device to be removed on its own.

Switch this around to be a direct representation, use a refcount to count
the number of places that are blocking destruction and sleep directly on a
completion until that counter goes to zero. kfree the device after other
accesses have been excluded in vfio_del_group_dev(). This is a fairly
common Linux idiom.

This simplifies a couple of things:

- kref_put_mutex() is very rarely used in the kernel. Here it is being
  used to prevent a zero ref device from being seen in the group
  list. Instead allow the zero ref device to continue to exist in the
  device_list and use refcount_inc_not_zero() to exclude it once refs go
  to zero.

- get/putting the group while get/putting the device. The device already
  holds a reference to the group, set during vfio_group_create_device(),
  there is no need for extra reference traffic. Cleanly have the balancing
  group put in vfio_del_group_dev() before the kfree().

Clearly communicated lifetime rules are essential before we can embed the
struct vfio_device in other structures. This patch is organized so the
next patch will be able to alter the API to allow drivers to provide the
kfree.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio.c | 93 +++++++++++++++------------------------------
 1 file changed, 30 insertions(+), 63 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 38779e6fd80cb4..04e24248e77f50 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -46,7 +46,6 @@ static struct vfio {
 	struct mutex			group_lock;
 	struct cdev			group_cdev;
 	dev_t				group_devt;
-	wait_queue_head_t		release_q;
 } vfio;
 
 struct vfio_iommu_driver {
@@ -91,7 +90,8 @@ struct vfio_group {
 };
 
 struct vfio_device {
-	struct kref			kref;
+	refcount_t			refcount;
+	struct completion		comp;
 	struct device			*dev;
 	const struct vfio_device_ops	*ops;
 	struct vfio_group		*group;
@@ -544,14 +544,18 @@ struct vfio_device *vfio_group_create_device(struct vfio_group *group,
 	if (!device)
 		return ERR_PTR(-ENOMEM);
 
-	kref_init(&device->kref);
+	refcount_set(&device->refcount, 1);
+	init_completion(&device->comp);
 	device->dev = dev;
 	device->group = group;
 	device->ops = ops;
 	device->device_data = device_data;
 	dev_set_drvdata(dev, device);
 
-	/* No need to get group_lock, caller has group reference */
+	/*
+	 * No need to get group_lock, caller has group reference, matching put
+	 * is in vfio_del_group_dev()
+	 */
 	vfio_group_get(group);
 
 	mutex_lock(&group->device_lock);
@@ -562,37 +566,17 @@ struct vfio_device *vfio_group_create_device(struct vfio_group *group,
 	return device;
 }
 
-static void vfio_device_release(struct kref *kref)
-{
-	struct vfio_device *device = container_of(kref,
-						  struct vfio_device, kref);
-	struct vfio_group *group = device->group;
-
-	list_del(&device->group_next);
-	group->dev_counter--;
-	mutex_unlock(&group->device_lock);
-
-	dev_set_drvdata(device->dev, NULL);
-
-	kfree(device);
-
-	/* vfio_del_group_dev may be waiting for this device */
-	wake_up(&vfio.release_q);
-}
-
 /* Device reference always implies a group reference */
 void vfio_device_put(struct vfio_device *device)
 {
-	struct vfio_group *group = device->group;
-	kref_put_mutex(&device->kref, vfio_device_release, &group->device_lock);
-	vfio_group_put(group);
+	if (refcount_dec_and_test(&device->refcount))
+		complete(&device->comp);
 }
 EXPORT_SYMBOL_GPL(vfio_device_put);
 
-static void vfio_device_get(struct vfio_device *device)
+static bool vfio_device_try_get(struct vfio_device *device)
 {
-	vfio_group_get(device->group);
-	kref_get(&device->kref);
+	return refcount_inc_not_zero(&device->refcount);
 }
 
 static struct vfio_device *vfio_group_get_device(struct vfio_group *group,
@@ -602,8 +586,7 @@ static struct vfio_device *vfio_group_get_device(struct vfio_group *group,
 
 	mutex_lock(&group->device_lock);
 	list_for_each_entry(device, &group->device_list, group_next) {
-		if (device->dev == dev) {
-			vfio_device_get(device);
+		if (device->dev == dev && vfio_device_try_get(device)) {
 			mutex_unlock(&group->device_lock);
 			return device;
 		}
@@ -895,9 +878,8 @@ static struct vfio_device *vfio_device_get_from_name(struct vfio_group *group,
 			ret = !strcmp(dev_name(it->dev), buf);
 		}
 
-		if (ret) {
+		if (ret && vfio_device_try_get(it)) {
 			device = it;
-			vfio_device_get(device);
 			break;
 		}
 	}
@@ -920,19 +902,13 @@ EXPORT_SYMBOL_GPL(vfio_device_data);
  * removed.  Open file descriptors for the device... */
 void *vfio_del_group_dev(struct device *dev)
 {
-	DEFINE_WAIT_FUNC(wait, woken_wake_function);
 	struct vfio_device *device = dev_get_drvdata(dev);
 	struct vfio_group *group = device->group;
 	void *device_data = device->device_data;
 	struct vfio_unbound_dev *unbound;
 	unsigned int i = 0;
 	bool interrupted = false;
-
-	/*
-	 * The group exists so long as we have a device reference.  Get
-	 * a group reference and use it to scan for the device going away.
-	 */
-	vfio_group_get(group);
+	long rc;
 
 	/*
 	 * When the device is removed from the group, the group suddenly
@@ -953,32 +929,18 @@ void *vfio_del_group_dev(struct device *dev)
 	WARN_ON(!unbound);
 
 	vfio_device_put(device);
-
-	/*
-	 * If the device is still present in the group after the above
-	 * 'put', then it is in use and we need to request it from the
-	 * bus driver.  The driver may in turn need to request the
-	 * device from the user.  We send the request on an arbitrary
-	 * interval with counter to allow the driver to take escalating
-	 * measures to release the device if it has the ability to do so.
-	 */
-	add_wait_queue(&vfio.release_q, &wait);
-
-	do {
-		device = vfio_group_get_device(group, dev);
-		if (!device)
-			break;
-
+	rc = try_wait_for_completion(&device->comp);
+	while (rc <= 0) {
 		if (device->ops->request)
 			device->ops->request(device_data, i++);
 
-		vfio_device_put(device);
-
 		if (interrupted) {
-			wait_woken(&wait, TASK_UNINTERRUPTIBLE, HZ * 10);
+			rc = wait_for_completion_timeout(&device->comp,
+							 HZ * 10);
 		} else {
-			wait_woken(&wait, TASK_INTERRUPTIBLE, HZ * 10);
-			if (signal_pending(current)) {
+			rc = wait_for_completion_interruptible_timeout(
+				&device->comp, HZ * 10);
+			if (rc < 0) {
 				interrupted = true;
 				dev_warn(dev,
 					 "Device is currently in use, task"
@@ -987,10 +949,13 @@ void *vfio_del_group_dev(struct device *dev)
 					 current->comm, task_pid_nr(current));
 			}
 		}
+	}
 
-	} while (1);
+	mutex_lock(&group->device_lock);
+	list_del(&device->group_next);
+	group->dev_counter--;
+	mutex_unlock(&group->device_lock);
 
-	remove_wait_queue(&vfio.release_q, &wait);
 	/*
 	 * In order to support multiple devices per group, devices can be
 	 * plucked from the group while other devices in the group are still
@@ -1008,7 +973,10 @@ void *vfio_del_group_dev(struct device *dev)
 	if (list_empty(&group->device_list))
 		wait_event(group->container_q, !group->container);
 
+	/* Matches the get in vfio_group_create_device() */
 	vfio_group_put(group);
+	dev_set_drvdata(dev, NULL);
+	kfree(device);
 
 	return device_data;
 }
@@ -2379,7 +2347,6 @@ static int __init vfio_init(void)
 	mutex_init(&vfio.iommu_drivers_lock);
 	INIT_LIST_HEAD(&vfio.group_list);
 	INIT_LIST_HEAD(&vfio.iommu_drivers_list);
-	init_waitqueue_head(&vfio.release_q);
 
 	ret = misc_register(&vfio_dev);
 	if (ret) {
-- 
2.30.1

Re: [PATCH 01/10] vfio: Simplify the lifetime logic for vfio_device

[Christoph Hellwig]

On Tue, Mar 09, 2021 at 05:38:43PM -0400, Jason Gunthorpe wrote:
> The vfio_device is using a 'sleep until all refs go to zero' pattern for
> its lifetime, but it is indirectly coded by repeatedly scanning the group
> list waiting for the device to be removed on its own.
> 
> Switch this around to be a direct representation, use a refcount to count
> the number of places that are blocking destruction and sleep directly on a
> completion until that counter goes to zero. kfree the device after other
> accesses have been excluded in vfio_del_group_dev(). This is a fairly
> common Linux idiom.
> 
> This simplifies a couple of things:
> 
> - kref_put_mutex() is very rarely used in the kernel. Here it is being
>   used to prevent a zero ref device from being seen in the group
>   list. Instead allow the zero ref device to continue to exist in the
>   device_list and use refcount_inc_not_zero() to exclude it once refs go
>   to zero.
> 
> - get/putting the group while get/putting the device. The device already
>   holds a reference to the group, set during vfio_group_create_device(),
>   there is no need for extra reference traffic. Cleanly have the balancing
>   group put in vfio_del_group_dev() before the kfree().

Might it be worth to split this further up into separate patches for
each of the changes?

The actual changes do look good, though:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 01/10] vfio: Simplify the lifetime logic for vfio_device

[Jason Gunthorpe]

On Wed, Mar 10, 2021 at 08:23:40AM +0100, Christoph Hellwig wrote:
> On Tue, Mar 09, 2021 at 05:38:43PM -0400, Jason Gunthorpe wrote:
> > The vfio_device is using a 'sleep until all refs go to zero' pattern for
> > its lifetime, but it is indirectly coded by repeatedly scanning the group
> > list waiting for the device to be removed on its own.
> > 
> > Switch this around to be a direct representation, use a refcount to count
> > the number of places that are blocking destruction and sleep directly on a
> > completion until that counter goes to zero. kfree the device after other
> > accesses have been excluded in vfio_del_group_dev(). This is a fairly
> > common Linux idiom.
> > 
> > This simplifies a couple of things:
> > 
> > - kref_put_mutex() is very rarely used in the kernel. Here it is being
> >   used to prevent a zero ref device from being seen in the group
> >   list. Instead allow the zero ref device to continue to exist in the
> >   device_list and use refcount_inc_not_zero() to exclude it once refs go
> >   to zero.
> > 
> > - get/putting the group while get/putting the device. The device already
> >   holds a reference to the group, set during vfio_group_create_device(),
> >   there is no need for extra reference traffic. Cleanly have the balancing
> >   group put in vfio_del_group_dev() before the kfree().
> 
> Might it be worth to split this further up into separate patches for
> each of the changes?

I split the put/get and it looks nicer. The other stuff is more
tightly coupled, doesn't look like there is much win from splitting
further

Thanks,
Jason

Re: [PATCH 01/10] vfio: Simplify the lifetime logic for vfio_device

[Christoph Hellwig]

On Fri, Mar 12, 2021 at 11:41:33AM -0400, Jason Gunthorpe wrote:
> I split the put/get and it looks nicer. The other stuff is more
> tightly coupled, doesn't look like there is much win from splitting
> further

Splitting the group put/get was all that I mean anyway.  Thanks!

[PATCH 02/10] vfio: Split creation of a vfio_device into init and register ops

[Jason Gunthorpe]

This makes the struct vfio_pci_device part of the public interface so it
can be used with container_of and so forth, as is typical for a Linux
subystem.

This is the first step to bring some type-safety to the vfio interface by
allowing the replacement of 'void *' and 'struct device *' inputs with a
simple and clear 'struct vfio_pci_device *'

For now the self-allocating vfio_add_group_dev() interface is kept so each
user can be updated as a separate patch.

The expected usage pattern is

  driver core probe() function:
     my_device = kzalloc(sizeof(*mydevice));
     vfio_init_group_dev(&my_device->vdev, dev, ops, mydevice);
     /* other driver specific prep */
     vfio_register_group_dev(&my_device->vdev);
     dev_set_drvdata(my_device);

  driver core remove() function:
     my_device = dev_get_drvdata(dev);
     vfio_unregister_group_dev(&my_device->vdev);
     /* other driver specific tear down */
     kfree(my_device);

Allowing the driver to be able to use the drvdata and vifo_device to go
to/from its own data.

The pattern also makes it clear that vfio_register_group_dev() must be
last in the sequence, as once it is called the core code can immediately
start calling ops. The init/register gap is provided to allow for the
driver to do setup before ops can be called and thus avoid races.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 Documentation/driver-api/vfio.rst |  31 ++++---
 drivers/vfio/vfio.c               | 132 ++++++++++++++----------------
 include/linux/vfio.h              |  16 ++++
 3 files changed, 96 insertions(+), 83 deletions(-)

diff --git a/Documentation/driver-api/vfio.rst b/Documentation/driver-api/vfio.rst
index f1a4d3c3ba0bb1..d3a02300913a7f 100644
--- a/Documentation/driver-api/vfio.rst
+++ b/Documentation/driver-api/vfio.rst
@@ -249,18 +249,23 @@ VFIO bus driver API
 
 VFIO bus drivers, such as vfio-pci make use of only a few interfaces
 into VFIO core.  When devices are bound and unbound to the driver,
-the driver should call vfio_add_group_dev() and vfio_del_group_dev()
-respectively::
-
-	extern int vfio_add_group_dev(struct device *dev,
-				      const struct vfio_device_ops *ops,
-				      void *device_data);
-
-	extern void *vfio_del_group_dev(struct device *dev);
-
-vfio_add_group_dev() indicates to the core to begin tracking the
-iommu_group of the specified dev and register the dev as owned by
-a VFIO bus driver.  The driver provides an ops structure for callbacks
+the driver should call vfio_register_group_dev() and
+vfio_unregister_group_dev() respectively::
+
+	void vfio_init_group_dev(struct vfio_device *device,
+				struct device *dev,
+				const struct vfio_device_ops *ops,
+				void *device_data);
+	int vfio_register_group_dev(struct vfio_device *device);
+	void vfio_unregister_group_dev(struct vfio_device *device);
+
+The driver should embed the vfio_device in its own structure and call
+vfio_init_group_dev() to pre-configure it before going to registration.
+vfio_register_group_dev() indicates to the core to begin tracking the
+iommu_group of the specified dev and register the dev as owned by a VFIO bus
+driver. Once vfio_register_group_dev() returns it is possible for userspace to
+start accessing the driver, thus the driver should ensure it is completely
+ready before calling it. The driver provides an ops structure for callbacks
 similar to a file operations structure::
 
 	struct vfio_device_ops {
@@ -276,7 +281,7 @@ similar to a file operations structure::
 	};
 
 Each function is passed the device_data that was originally registered
-in the vfio_add_group_dev() call above.  This allows the bus driver
+in the vfio_register_group_dev() call above.  This allows the bus driver
 an easy place to store its opaque, private data.  The open/release
 callbacks are issued when a new file descriptor is created for a
 device (via VFIO_GROUP_GET_DEVICE_FD).  The ioctl interface provides
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 04e24248e77f50..cfa06ae3b9018b 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -89,16 +89,6 @@ struct vfio_group {
 	struct blocking_notifier_head	notifier;
 };
 
-struct vfio_device {
-	refcount_t			refcount;
-	struct completion		comp;
-	struct device			*dev;
-	const struct vfio_device_ops	*ops;
-	struct vfio_group		*group;
-	struct list_head		group_next;
-	void				*device_data;
-};
-
 #ifdef CONFIG_VFIO_NOIOMMU
 static bool noiommu __read_mostly;
 module_param_named(enable_unsafe_noiommu_mode,
@@ -532,40 +522,6 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev)
 /**
  * Device objects - create, release, get, put, search
  */
-static
-struct vfio_device *vfio_group_create_device(struct vfio_group *group,
-					     struct device *dev,
-					     const struct vfio_device_ops *ops,
-					     void *device_data)
-{
-	struct vfio_device *device;
-
-	device = kzalloc(sizeof(*device), GFP_KERNEL);
-	if (!device)
-		return ERR_PTR(-ENOMEM);
-
-	refcount_set(&device->refcount, 1);
-	init_completion(&device->comp);
-	device->dev = dev;
-	device->group = group;
-	device->ops = ops;
-	device->device_data = device_data;
-	dev_set_drvdata(dev, device);
-
-	/*
-	 * No need to get group_lock, caller has group reference, matching put
-	 * is in vfio_del_group_dev()
-	 */
-	vfio_group_get(group);
-
-	mutex_lock(&group->device_lock);
-	list_add(&device->group_next, &group->device_list);
-	group->dev_counter++;
-	mutex_unlock(&group->device_lock);
-
-	return device;
-}
-
 /* Device reference always implies a group reference */
 void vfio_device_put(struct vfio_device *device)
 {
@@ -784,14 +740,23 @@ static int vfio_iommu_group_notifier(struct notifier_block *nb,
 /**
  * VFIO driver API
  */
-int vfio_add_group_dev(struct device *dev,
-		       const struct vfio_device_ops *ops, void *device_data)
+void vfio_init_group_dev(struct vfio_device *device, struct device *dev,
+			 const struct vfio_device_ops *ops, void *device_data)
 {
+	init_completion(&device->comp);
+	device->dev = dev;
+	device->ops = ops;
+	device->device_data = device_data;
+}
+EXPORT_SYMBOL_GPL(vfio_init_group_dev);
+
+int vfio_register_group_dev(struct vfio_device *device)
+{
+	struct vfio_device *existing_device;
 	struct iommu_group *iommu_group;
 	struct vfio_group *group;
-	struct vfio_device *device;
 
-	iommu_group = iommu_group_get(dev);
+	iommu_group = iommu_group_get(device->dev);
 	if (!iommu_group)
 		return -EINVAL;
 
@@ -810,30 +775,51 @@ int vfio_add_group_dev(struct device *dev,
 		iommu_group_put(iommu_group);
 	}
 
-	device = vfio_group_get_device(group, dev);
-	if (device) {
-		dev_WARN(dev, "Device already exists on group %d\n",
+	existing_device = vfio_group_get_device(group, device->dev);
+	if (existing_device) {
+		dev_WARN(device->dev, "Device already exists on group %d\n",
 			 iommu_group_id(iommu_group));
-		vfio_device_put(device);
+		vfio_device_put(existing_device);
 		vfio_group_put(group);
 		return -EBUSY;
 	}
 
-	device = vfio_group_create_device(group, dev, ops, device_data);
-	if (IS_ERR(device)) {
-		vfio_group_put(group);
-		return PTR_ERR(device);
-	}
+	/* Our reference on group is moved to the device */
+	device->group = group;
 
-	/*
-	 * Drop all but the vfio_device reference.  The vfio_device holds
-	 * a reference to the vfio_group, which holds a reference to the
-	 * iommu_group.
-	 */
-	vfio_group_put(group);
+	/* Refcounting can't start until the driver calls register */
+	refcount_set(&device->refcount, 1);
+
+	mutex_lock(&group->device_lock);
+	list_add(&device->group_next, &group->device_list);
+	group->dev_counter++;
+	mutex_unlock(&group->device_lock);
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(vfio_register_group_dev);
+
+int vfio_add_group_dev(struct device *dev, const struct vfio_device_ops *ops,
+		       void *device_data)
+{
+	struct vfio_device *device;
+	int ret;
+
+	device = kzalloc(sizeof(*device), GFP_KERNEL);
+	if (!device)
+		return -ENOMEM;
+
+	vfio_init_group_dev(device, dev, ops, device_data);
+	ret = vfio_register_group_dev(device);
+	if (ret)
+		goto err_kfree;
+	dev_set_drvdata(dev, device);
+	return 0;
+
+err_kfree:
+	kfree(device);
+	return ret;
+}
 EXPORT_SYMBOL_GPL(vfio_add_group_dev);
 
 /**
@@ -900,11 +886,9 @@ EXPORT_SYMBOL_GPL(vfio_device_data);
 /*
  * Decrement the device reference count and wait for the device to be
  * removed.  Open file descriptors for the device... */
-void *vfio_del_group_dev(struct device *dev)
+void vfio_unregister_group_dev(struct vfio_device *device)
 {
-	struct vfio_device *device = dev_get_drvdata(dev);
 	struct vfio_group *group = device->group;
-	void *device_data = device->device_data;
 	struct vfio_unbound_dev *unbound;
 	unsigned int i = 0;
 	bool interrupted = false;
@@ -921,7 +905,7 @@ void *vfio_del_group_dev(struct device *dev)
 	 */
 	unbound = kzalloc(sizeof(*unbound), GFP_KERNEL);
 	if (unbound) {
-		unbound->dev = dev;
+		unbound->dev = device->dev;
 		mutex_lock(&group->unbound_lock);
 		list_add(&unbound->unbound_next, &group->unbound_list);
 		mutex_unlock(&group->unbound_lock);
@@ -932,7 +916,7 @@ void *vfio_del_group_dev(struct device *dev)
 	rc = try_wait_for_completion(&device->comp);
 	while (rc <= 0) {
 		if (device->ops->request)
-			device->ops->request(device_data, i++);
+			device->ops->request(device->device_data, i++);
 
 		if (interrupted) {
 			rc = wait_for_completion_timeout(&device->comp,
@@ -942,7 +926,7 @@ void *vfio_del_group_dev(struct device *dev)
 				&device->comp, HZ * 10);
 			if (rc < 0) {
 				interrupted = true;
-				dev_warn(dev,
+				dev_warn(device->dev,
 					 "Device is currently in use, task"
 					 " \"%s\" (%d) "
 					 "blocked until device is released",
@@ -975,9 +959,17 @@ void *vfio_del_group_dev(struct device *dev)
 
 	/* Matches the get in vfio_group_create_device() */
 	vfio_group_put(group);
+}
+EXPORT_SYMBOL_GPL(vfio_unregister_group_dev);
+
+void *vfio_del_group_dev(struct device *dev)
+{
+	struct vfio_device *device = dev_get_drvdata(dev);
+	void *device_data = device->device_data;
+
+	vfio_unregister_group_dev(device);
 	dev_set_drvdata(dev, NULL);
 	kfree(device);
-
 	return device_data;
 }
 EXPORT_SYMBOL_GPL(vfio_del_group_dev);
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index b7e18bde5aa8b3..ad8b579d67d34a 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -15,6 +15,18 @@
 #include <linux/poll.h>
 #include <uapi/linux/vfio.h>
 
+struct vfio_device {
+	struct device *dev;
+	const struct vfio_device_ops *ops;
+	struct vfio_group *group;
+
+	/* Members below here are private, not for driver use */
+	refcount_t refcount;
+	struct completion comp;
+	struct list_head group_next;
+	void *device_data;
+};
+
 /**
  * struct vfio_device_ops - VFIO bus driver device callbacks
  *
@@ -48,11 +60,15 @@ struct vfio_device_ops {
 extern struct iommu_group *vfio_iommu_group_get(struct device *dev);
 extern void vfio_iommu_group_put(struct iommu_group *group, struct device *dev);
 
+void vfio_init_group_dev(struct vfio_device *device, struct device *dev,
+			 const struct vfio_device_ops *ops, void *device_data);
+int vfio_register_group_dev(struct vfio_device *device);
 extern int vfio_add_group_dev(struct device *dev,
 			      const struct vfio_device_ops *ops,
 			      void *device_data);
 
 extern void *vfio_del_group_dev(struct device *dev);
+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);
 extern void *vfio_device_data(struct vfio_device *device);
-- 
2.30.1

Re: [PATCH 02/10] vfio: Split creation of a vfio_device into init and register ops

[Christoph Hellwig]

On Tue, Mar 09, 2021 at 05:38:44PM -0400, Jason Gunthorpe wrote:
> The pattern also makes it clear that vfio_register_group_dev() must be
> last in the sequence, as once it is called the core code can immediately
> start calling ops. The init/register gap is provided to allow for the
> driver to do setup before ops can be called and thus avoid races.

Yes, APIs that init and register together are generatelly a rather
bad idea.

Reviewed-by: Christoph Hellwig <hch@lst.de>

RE: [PATCH 02/10] vfio: Split creation of a vfio_device into init and register ops

[Liu, Yi L]

Hi Jason,

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, March 10, 2021 5:39 AM
> 
[...]
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index b7e18bde5aa8b3..ad8b579d67d34a 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -15,6 +15,18 @@
>  #include <linux/poll.h>
>  #include <uapi/linux/vfio.h>
> 
> +struct vfio_device {
> +	struct device *dev;
> +	const struct vfio_device_ops *ops;
> +	struct vfio_group *group;
> +
> +	/* Members below here are private, not for driver use */
> +	refcount_t refcount;
> +	struct completion comp;
> +	struct list_head group_next;
> +	void *device_data;

A dumb question. If these fields are not supposed to be used by
"external modules" like vfio_pci driver, how about defining a private
struct vfio_dev_prive within vfio.c and embed here?

Other parts look good to me.

Reviewed-by: Liu Yi L <yi.l.liu@intel.com>

Regards,
Yi Liu

Re: [PATCH 02/10] vfio: Split creation of a vfio_device into init and register ops

[Jason Gunthorpe]

On Fri, Mar 12, 2021 at 01:04:29PM +0000, Liu, Yi L wrote:
> Hi Jason,
> 
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Wednesday, March 10, 2021 5:39 AM
> > 
> [...]
> > diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> > index b7e18bde5aa8b3..ad8b579d67d34a 100644
> > +++ b/include/linux/vfio.h
> > @@ -15,6 +15,18 @@
> >  #include <linux/poll.h>
> >  #include <uapi/linux/vfio.h>
> > 
> > +struct vfio_device {
> > +	struct device *dev;
> > +	const struct vfio_device_ops *ops;
> > +	struct vfio_group *group;
> > +
> > +	/* Members below here are private, not for driver use */
> > +	refcount_t refcount;
> > +	struct completion comp;
> > +	struct list_head group_next;
> > +	void *device_data;
> 
> A dumb question. If these fields are not supposed to be used by
> "external modules" like vfio_pci driver, how about defining a private
> struct vfio_dev_prive within vfio.c and embed here?

This is rarely done, there should be a good reason to do it, as making
a private structure in a container_of system requires another memory
allocation.

'struct device' has this for instance, look at the 'p' member.

In this case I can't see much value

Jason

Re: [PATCH 02/10] vfio: Split creation of a vfio_device into init and register ops

[Christoph Hellwig]

On Fri, Mar 12, 2021 at 10:23:26AM -0400, Jason Gunthorpe wrote:
> This is rarely done, there should be a good reason to do it, as making
> a private structure in a container_of system requires another memory
> allocation.
> 
> 'struct device' has this for instance, look at the 'p' member.
> 
> In this case I can't see much value

Agreed.  Moving this code to the normal kernel pattern really not just
helps to find bugs and reduce complexity, but also makes it much easier
for random kernel developers to actually be able to understand the code.

[PATCH 03/10] vfio/platform: Use vfio_init/register/unregister_group_dev

[Jason Gunthorpe]

platform already allocates a struct vfio_platform_device with exactly
the same lifetime as vfio_device, switch to the new API and embed
vfio_device in vfio_platform_device.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/platform/vfio_amba.c             |  8 ++++---
 drivers/vfio/platform/vfio_platform.c         | 21 ++++++++---------
 drivers/vfio/platform/vfio_platform_common.c  | 23 +++++++------------
 drivers/vfio/platform/vfio_platform_private.h |  5 ++--
 4 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/drivers/vfio/platform/vfio_amba.c b/drivers/vfio/platform/vfio_amba.c
index 3626c21501017e..f970eb2a999f29 100644
--- a/drivers/vfio/platform/vfio_amba.c
+++ b/drivers/vfio/platform/vfio_amba.c
@@ -66,16 +66,18 @@ static int vfio_amba_probe(struct amba_device *adev, const struct amba_id *id)
 	if (ret) {
 		kfree(vdev->name);
 		kfree(vdev);
+		return ret;
 	}
 
-	return ret;
+	dev_set_drvdata(&adev->dev, vdev);
+	return 0;
 }
 
 static void vfio_amba_remove(struct amba_device *adev)
 {
-	struct vfio_platform_device *vdev =
-		vfio_platform_remove_common(&adev->dev);
+	struct vfio_platform_device *vdev = dev_get_drvdata(&adev->dev);
 
+	vfio_platform_remove_common(vdev);
 	kfree(vdev->name);
 	kfree(vdev);
 }
diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
index 9fb6818cea12cb..f7b3f64ecc7f6c 100644
--- a/drivers/vfio/platform/vfio_platform.c
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -54,23 +54,22 @@ static int vfio_platform_probe(struct platform_device *pdev)
 	vdev->reset_required = reset_required;
 
 	ret = vfio_platform_probe_common(vdev, &pdev->dev);
-	if (ret)
+	if (ret) {
 		kfree(vdev);
-
-	return ret;
+		return ret;
+	}
+	dev_set_drvdata(&pdev->dev, vdev);
+	return 0;
 }
 
 static int vfio_platform_remove(struct platform_device *pdev)
 {
-	struct vfio_platform_device *vdev;
-
-	vdev = vfio_platform_remove_common(&pdev->dev);
-	if (vdev) {
-		kfree(vdev);
-		return 0;
-	}
+	struct vfio_platform_device *vdev = dev_get_drvdata(&pdev->dev);
 
-	return -EINVAL;
+	vfio_platform_remove_common(vdev);
+	kfree(vdev->name);
+	kfree(vdev);
+	return 0;
 }
 
 static struct platform_driver vfio_platform_driver = {
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index fb4b385191f288..6eb749250ee41c 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -659,8 +659,7 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 	struct iommu_group *group;
 	int ret;
 
-	if (!vdev)
-		return -EINVAL;
+	vfio_init_group_dev(&vdev->vdev, dev, &vfio_platform_ops, vdev);
 
 	ret = vfio_platform_acpi_probe(vdev, dev);
 	if (ret)
@@ -685,13 +684,13 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 		goto put_reset;
 	}
 
-	ret = vfio_add_group_dev(dev, &vfio_platform_ops, vdev);
+	ret = vfio_register_group_dev(&vdev->vdev);
 	if (ret)
 		goto put_iommu;
 
 	mutex_init(&vdev->igate);
 
-	pm_runtime_enable(vdev->device);
+	pm_runtime_enable(dev);
 	return 0;
 
 put_iommu:
@@ -702,19 +701,13 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 }
 EXPORT_SYMBOL_GPL(vfio_platform_probe_common);
 
-struct vfio_platform_device *vfio_platform_remove_common(struct device *dev)
+void vfio_platform_remove_common(struct vfio_platform_device *vdev)
 {
-	struct vfio_platform_device *vdev;
-
-	vdev = vfio_del_group_dev(dev);
+	vfio_unregister_group_dev(&vdev->vdev);
 
-	if (vdev) {
-		pm_runtime_disable(vdev->device);
-		vfio_platform_put_reset(vdev);
-		vfio_iommu_group_put(dev->iommu_group, dev);
-	}
-
-	return vdev;
+	pm_runtime_disable(vdev->device);
+	vfio_platform_put_reset(vdev);
+	vfio_iommu_group_put(vdev->vdev.dev->iommu_group, vdev->vdev.dev);
 }
 EXPORT_SYMBOL_GPL(vfio_platform_remove_common);
 
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 289089910643ac..a5ba82c8cbc354 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -9,6 +9,7 @@
 
 #include <linux/types.h>
 #include <linux/interrupt.h>
+#include <linux/vfio.h>
 
 #define VFIO_PLATFORM_OFFSET_SHIFT   40
 #define VFIO_PLATFORM_OFFSET_MASK (((u64)(1) << VFIO_PLATFORM_OFFSET_SHIFT) - 1)
@@ -42,6 +43,7 @@ struct vfio_platform_region {
 };
 
 struct vfio_platform_device {
+	struct vfio_device		vdev;
 	struct vfio_platform_region	*regions;
 	u32				num_regions;
 	struct vfio_platform_irq	*irqs;
@@ -80,8 +82,7 @@ struct vfio_platform_reset_node {
 
 extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 				      struct device *dev);
-extern struct vfio_platform_device *vfio_platform_remove_common
-				     (struct device *dev);
+void vfio_platform_remove_common(struct vfio_platform_device *vdev);
 
 extern int vfio_platform_irq_init(struct vfio_platform_device *vdev);
 extern void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev);
-- 
2.30.1

Re: [PATCH 03/10] vfio/platform: Use vfio_init/register/unregister_group_dev

[Christoph Hellwig]

On Tue, Mar 09, 2021 at 05:38:45PM -0400, Jason Gunthorpe wrote:
> platform already allocates a struct vfio_platform_device with exactly
> the same lifetime as vfio_device, switch to the new API and embed
> vfio_device in vfio_platform_device.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/platform/vfio_amba.c             |  8 ++++---
>  drivers/vfio/platform/vfio_platform.c         | 21 ++++++++---------
>  drivers/vfio/platform/vfio_platform_common.c  | 23 +++++++------------
>  drivers/vfio/platform/vfio_platform_private.h |  5 ++--
>  4 files changed, 26 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_amba.c b/drivers/vfio/platform/vfio_amba.c
> index 3626c21501017e..f970eb2a999f29 100644
> --- a/drivers/vfio/platform/vfio_amba.c
> +++ b/drivers/vfio/platform/vfio_amba.c
> @@ -66,16 +66,18 @@ static int vfio_amba_probe(struct amba_device *adev, const struct amba_id *id)
>  	if (ret) {
>  		kfree(vdev->name);
>  		kfree(vdev);
> +		return ret;
>  	}
>  
> -	return ret;
> +	dev_set_drvdata(&adev->dev, vdev);
> +	return 0;

Switching to goto based unwind here would be helpful as well..

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 03/10] vfio/platform: Use vfio_init/register/unregister_group_dev

[Jason Gunthorpe]

On Wed, Mar 10, 2021 at 08:28:50AM +0100, Christoph Hellwig wrote:
> On Tue, Mar 09, 2021 at 05:38:45PM -0400, Jason Gunthorpe wrote:
> > platform already allocates a struct vfio_platform_device with exactly
> > the same lifetime as vfio_device, switch to the new API and embed
> > vfio_device in vfio_platform_device.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> >  drivers/vfio/platform/vfio_amba.c             |  8 ++++---
> >  drivers/vfio/platform/vfio_platform.c         | 21 ++++++++---------
> >  drivers/vfio/platform/vfio_platform_common.c  | 23 +++++++------------
> >  drivers/vfio/platform/vfio_platform_private.h |  5 ++--
> >  4 files changed, 26 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/vfio/platform/vfio_amba.c b/drivers/vfio/platform/vfio_amba.c
> > index 3626c21501017e..f970eb2a999f29 100644
> > +++ b/drivers/vfio/platform/vfio_amba.c
> > @@ -66,16 +66,18 @@ static int vfio_amba_probe(struct amba_device *adev, const struct amba_id *id)
> >  	if (ret) {
> >  		kfree(vdev->name);
> >  		kfree(vdev);
> > +		return ret;
> >  	}
> >  
> > -	return ret;
> > +	dev_set_drvdata(&adev->dev, vdev);
> > +	return 0;
> 
> Switching to goto based unwind here would be helpful as well..

Hurm. Lets just delete vfio_platform_device->name.

It is only used for a few diagnostic prints and in all those cases the
print is already a dev_XX on the parent:

-               dev_err(dev, "ACPI companion device not found for %s\n",
-                       vdev->name);

Here dev == vdev->dev.dev. So on platform this prints

foo_platform_device.0: ACPI companion device not found for foo_platform_device

AMBA will print the periphid, but someone who needs that can find it
in the sysfs from the parent name. I wrote a patch and will include it
in some follow up series

Jason

[PATCH 04/10] vfio/fsl-mc: Use vfio_init/register/unregister_group_dev

[Jason Gunthorpe]

fsl-mc already allocates a struct vfio_fsl_mc_device with exactly the same
lifetime as vfio_device, switch to the new API and embed vfio_device in
vfio_fsl_mc_device. While here remove the devm usage for the vdev, this
code is clean and doesn't need devm.

Add a note the probe ordering is racy.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/fsl-mc/vfio_fsl_mc.c         | 30 +++++++++++++----------
 drivers/vfio/fsl-mc/vfio_fsl_mc_private.h |  1 +
 2 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
index f27e25112c4037..ddee6ed20c4523 100644
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
@@ -600,20 +600,28 @@ static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev)
 		return -EINVAL;
 	}
 
-	vdev = devm_kzalloc(dev, sizeof(*vdev), GFP_KERNEL);
+	vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
 	if (!vdev) {
 		ret = -ENOMEM;
 		goto out_group_put;
 	}
 
+	vfio_init_group_dev(&vdev->vdev, dev, &vfio_fsl_mc_ops, vdev);
+	mutex_init(&vdev->igate);
 	vdev->mc_dev = mc_dev;
 
-	ret = vfio_add_group_dev(dev, &vfio_fsl_mc_ops, vdev);
+	ret = vfio_register_group_dev(&vdev->vdev);
 	if (ret) {
 		dev_err(dev, "VFIO_FSL_MC: Failed to add to vfio group\n");
-		goto out_group_put;
+		goto out_kfree;
 	}
+	dev_set_drvdata(dev, vdev);
 
+	/*
+	 * FIXME: vfio_register_group_dev() allows VFIO_GROUP_GET_DEVICE_FD to
+	 * immediately return the device to userspace, but we haven't finished
+	 * setting it up yet.
+	 */
 	ret = vfio_fsl_mc_reflck_attach(vdev);
 	if (ret)
 		goto out_group_dev;
@@ -621,15 +629,14 @@ static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev)
 	ret = vfio_fsl_mc_init_device(vdev);
 	if (ret)
 		goto out_reflck;
-
-	mutex_init(&vdev->igate);
-
 	return 0;
 
 out_reflck:
 	vfio_fsl_mc_reflck_put(vdev->reflck);
 out_group_dev:
-	vfio_del_group_dev(dev);
+	vfio_unregister_group_dev(&vdev->vdev);
+out_kfree:
+	kfree(vdev);
 out_group_put:
 	vfio_iommu_group_put(group, dev);
 	return ret;
@@ -637,13 +644,10 @@ static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev)
 
 static int vfio_fsl_mc_remove(struct fsl_mc_device *mc_dev)
 {
-	struct vfio_fsl_mc_device *vdev;
 	struct device *dev = &mc_dev->dev;
+	struct vfio_fsl_mc_device *vdev = dev_get_drvdata(dev);
 
-	vdev = vfio_del_group_dev(dev);
-	if (!vdev)
-		return -EINVAL;
-
+	vfio_unregister_group_dev(&vdev->vdev);
 	mutex_destroy(&vdev->igate);
 
 	vfio_fsl_mc_reflck_put(vdev->reflck);
@@ -656,8 +660,8 @@ static int vfio_fsl_mc_remove(struct fsl_mc_device *mc_dev)
 	if (vdev->nb.notifier_call)
 		bus_unregister_notifier(&fsl_mc_bus_type, &vdev->nb);
 
+	kfree(vdev);
 	vfio_iommu_group_put(mc_dev->dev.iommu_group, dev);
-
 	return 0;
 }
 
diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
index a97ee691ed47ec..89700e00e77d10 100644
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc_private.h
@@ -36,6 +36,7 @@ struct vfio_fsl_mc_region {
 };
 
 struct vfio_fsl_mc_device {
+	struct vfio_device		vdev;
 	struct fsl_mc_device		*mc_dev;
 	struct notifier_block        nb;
 	int				refcnt;
-- 
2.30.1

Re: [PATCH 04/10] vfio/fsl-mc: Use vfio_init/register/unregister_group_dev

[Christoph Hellwig]

> +	ret = vfio_register_group_dev(&vdev->vdev);
>  	if (ret) {
>  		dev_err(dev, "VFIO_FSL_MC: Failed to add to vfio group\n");
> -		goto out_group_put;
> +		goto out_kfree;
>  	}
> +	dev_set_drvdata(dev, vdev);
>  
> +	/*
> +	 * FIXME: vfio_register_group_dev() allows VFIO_GROUP_GET_DEVICE_FD to
> +	 * immediately return the device to userspace, but we haven't finished
> +	 * setting it up yet.
> +	 */

This should be trivial to fix.  Can you throw in a patch to move the
vfio_register_group_dev later?

Re: [PATCH 04/10] vfio/fsl-mc: Use vfio_init/register/unregister_group_dev

[Jason Gunthorpe]

On Wed, Mar 10, 2021 at 08:30:01AM +0100, Christoph Hellwig wrote:
> > +	ret = vfio_register_group_dev(&vdev->vdev);
> >  	if (ret) {
> >  		dev_err(dev, "VFIO_FSL_MC: Failed to add to vfio group\n");
> > -		goto out_group_put;
> > +		goto out_kfree;
> >  	}
> > +	dev_set_drvdata(dev, vdev);
> >  
> > +	/*
> > +	 * FIXME: vfio_register_group_dev() allows VFIO_GROUP_GET_DEVICE_FD to
> > +	 * immediately return the device to userspace, but we haven't finished
> > +	 * setting it up yet.
> > +	 */
> 
> This should be trivial to fix.  Can you throw in a patch to move the
> vfio_register_group_dev later?

Hmm.. Yes I don't see an obvious reason why this couldn't be trivially
re-ordered.

I was hoping someone familiar with FSL would see this comment and say
if it is OK or not :)

I added a note to my list to make a followup in one of the later
cleaning series.

Jason

[PATCH 05/10] vfio/pci: Use vfio_init/register/unregister_group_dev

[Jason Gunthorpe]

pci already allocates a struct vfio_pci_device with exactly the same
lifetime as vfio_device, switch to the new API and embed vfio_device in
vfio_pci_device.

Add a note the probe ordering is racy.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/pci/vfio_pci.c         | 17 +++++++++++------
 drivers/vfio/pci/vfio_pci_private.h |  1 +
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 65e7e6b44578c2..fae573c6f86bdf 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1957,6 +1957,7 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto out_group_put;
 	}
 
+	vfio_init_group_dev(&vdev->vdev, &pdev->dev, &vfio_pci_ops, vdev);
 	vdev->pdev = pdev;
 	vdev->irq_type = VFIO_PCI_NUM_IRQS;
 	mutex_init(&vdev->igate);
@@ -1968,7 +1969,12 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	INIT_LIST_HEAD(&vdev->vma_list);
 	init_rwsem(&vdev->memory_lock);
 
-	ret = vfio_add_group_dev(&pdev->dev, &vfio_pci_ops, vdev);
+	/*
+	 * FIXME: vfio_register_group_dev() allows VFIO_GROUP_GET_DEVICE_FD to
+	 * immediately return the device to userspace, but we haven't finished
+	 * setting it up yet.
+	 */
+	ret = vfio_register_group_dev(&vdev->vdev);
 	if (ret)
 		goto out_free;
 
@@ -2014,6 +2020,7 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		vfio_pci_set_power_state(vdev, PCI_D3hot);
 	}
 
+	dev_set_drvdata(&pdev->dev, vdev);
 	return ret;
 
 out_vf_token:
@@ -2021,7 +2028,7 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 out_reflck:
 	vfio_pci_reflck_put(vdev->reflck);
 out_del_group_dev:
-	vfio_del_group_dev(&pdev->dev);
+	vfio_unregister_group_dev(&vdev->vdev);
 out_free:
 	kfree(vdev);
 out_group_put:
@@ -2031,13 +2038,11 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 static void vfio_pci_remove(struct pci_dev *pdev)
 {
-	struct vfio_pci_device *vdev;
+	struct vfio_pci_device *vdev = dev_get_drvdata(&pdev->dev);
 
 	pci_disable_sriov(pdev);
 
-	vdev = vfio_del_group_dev(&pdev->dev);
-	if (!vdev)
-		return;
+	vfio_unregister_group_dev(&vdev->vdev);
 
 	if (vdev->vf_token) {
 		WARN_ON(vdev->vf_token->users);
diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
index 9cd1882a05af69..8755a0febd054a 100644
--- a/drivers/vfio/pci/vfio_pci_private.h
+++ b/drivers/vfio/pci/vfio_pci_private.h
@@ -100,6 +100,7 @@ struct vfio_pci_mmap_vma {
 };
 
 struct vfio_pci_device {
+	struct vfio_device	vdev;
 	struct pci_dev		*pdev;
 	void __iomem		*barmap[PCI_STD_NUM_BARS];
 	bool			bar_mmap_supported[PCI_STD_NUM_BARS];
-- 
2.30.1

Re: [PATCH 05/10] vfio/pci: Use vfio_init/register/unregister_group_dev

[Christoph Hellwig]

> -	ret = vfio_add_group_dev(&pdev->dev, &vfio_pci_ops, vdev);
> +	/*
> +	 * FIXME: vfio_register_group_dev() allows VFIO_GROUP_GET_DEVICE_FD to
> +	 * immediately return the device to userspace, but we haven't finished
> +	 * setting it up yet.
> +	 */
> +	ret = vfio_register_group_dev(&vdev->vdev);

Again, why not include a patch in the series to fix this up as well?

The patch itself looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

RE: [PATCH 05/10] vfio/pci: Use vfio_init/register/unregister_group_dev

[Liu, Yi L]

Hi Jason,

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, March 10, 2021 5:39 AM
> 
> pci already allocates a struct vfio_pci_device with exactly the same
> lifetime as vfio_device, switch to the new API and embed vfio_device in
> vfio_pci_device.
> 
> Add a note the probe ordering is racy.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>
> ---
>  drivers/vfio/pci/vfio_pci.c         | 17 +++++++++++------
>  drivers/vfio/pci/vfio_pci_private.h |  1 +
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 65e7e6b44578c2..fae573c6f86bdf 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -1957,6 +1957,7 @@ static int vfio_pci_probe(struct pci_dev *pdev,
> const struct pci_device_id *id)
>  		goto out_group_put;
>  	}
> 
> +	vfio_init_group_dev(&vdev->vdev, &pdev->dev, &vfio_pci_ops,
> vdev);
>  	vdev->pdev = pdev;
>  	vdev->irq_type = VFIO_PCI_NUM_IRQS;
>  	mutex_init(&vdev->igate);
> @@ -1968,7 +1969,12 @@ static int vfio_pci_probe(struct pci_dev *pdev,
> const struct pci_device_id *id)
>  	INIT_LIST_HEAD(&vdev->vma_list);
>  	init_rwsem(&vdev->memory_lock);
> 
> -	ret = vfio_add_group_dev(&pdev->dev, &vfio_pci_ops, vdev);
> +	/*
> +	 * FIXME: vfio_register_group_dev() allows VFIO_GROUP_GET_DEVICE_FD to
> +	 * immediately return the device to userspace, but we haven't finished
> +	 * setting it up yet.
> +	 */

this patch looks good to me.

Reviewed-by: Liu Yi L <yi.l.liu@intel.com>

But I have a question on the FIXME comment here. I checked the code below.
Even after vfio_register_group_dev(), userspace is not able to get DEVICE_FD
until the group has been added to a container. So perhaps you can give more
details behind this FIXME.

static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
{
        struct vfio_device *device;
        struct file *filep;
        int ret;

        if (0 == atomic_read(&group->container_users) ||
            !group->container->iommu_driver || !vfio_group_viable(group))
                return -EINVAL;

...
}

Regards,
Yi Liu

> +	ret = vfio_register_group_dev(&vdev->vdev);
>  	if (ret)
>  		goto out_free;
> 
> @@ -2014,6 +2020,7 @@ static int vfio_pci_probe(struct pci_dev *pdev,
> const struct pci_device_id *id)
>  		vfio_pci_set_power_state(vdev, PCI_D3hot);
>  	}
> 
> +	dev_set_drvdata(&pdev->dev, vdev);
>  	return ret;
> 
>  out_vf_token:
> @@ -2021,7 +2028,7 @@ static int vfio_pci_probe(struct pci_dev *pdev,
> const struct pci_device_id *id)
>  out_reflck:
>  	vfio_pci_reflck_put(vdev->reflck);
>  out_del_group_dev:
> -	vfio_del_group_dev(&pdev->dev);
> +	vfio_unregister_group_dev(&vdev->vdev);
>  out_free:
>  	kfree(vdev);
>  out_group_put:
> @@ -2031,13 +2038,11 @@ static int vfio_pci_probe(struct pci_dev *pdev,
> const struct pci_device_id *id)
> 
>  static void vfio_pci_remove(struct pci_dev *pdev)
>  {
> -	struct vfio_pci_device *vdev;
> +	struct vfio_pci_device *vdev = dev_get_drvdata(&pdev->dev);
> 
>  	pci_disable_sriov(pdev);
> 
> -	vdev = vfio_del_group_dev(&pdev->dev);
> -	if (!vdev)
> -		return;
> +	vfio_unregister_group_dev(&vdev->vdev);
> 
>  	if (vdev->vf_token) {
>  		WARN_ON(vdev->vf_token->users);
> diff --git a/drivers/vfio/pci/vfio_pci_private.h
> b/drivers/vfio/pci/vfio_pci_private.h
> index 9cd1882a05af69..8755a0febd054a 100644
> --- a/drivers/vfio/pci/vfio_pci_private.h
> +++ b/drivers/vfio/pci/vfio_pci_private.h
> @@ -100,6 +100,7 @@ struct vfio_pci_mmap_vma {
>  };
> 
>  struct vfio_pci_device {
> +	struct vfio_device	vdev;
>  	struct pci_dev		*pdev;
>  	void __iomem		*barmap[PCI_STD_NUM_BARS];
>  	bool
> 	bar_mmap_supported[PCI_STD_NUM_BARS];
> --
> 2.30.1

Re: [PATCH 05/10] vfio/pci: Use vfio_init/register/unregister_group_dev

[Jason Gunthorpe]

On Fri, Mar 12, 2021 at 12:53:18PM +0000, Liu, Yi L wrote:

> But I have a question on the FIXME comment here. I checked the code below.
> Even after vfio_register_group_dev(), userspace is not able to get DEVICE_FD
> until the group has been added to a container. 

The race here is between VFIO_GROUP_GET_DEVICE_FD and bind

Userspace would have already done VFIO_GROUP_SET_CONTAINER before it
does the VFIO_GROUP_GET_DEVICE_FD.

Jason

[PATCH 06/10] vfio/mdev: Use vfio_init/register/unregister_group_dev

[Jason Gunthorpe]

mdev gets little benefit because it doesn't actually do anything, however
it is the last user, so move the code here for now.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/mdev/vfio_mdev.c | 24 +++++++++++++++++++--
 drivers/vfio/vfio.c           | 39 ++---------------------------------
 include/linux/vfio.h          |  5 -----
 3 files changed, 24 insertions(+), 44 deletions(-)

diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
index b52eea128549ee..4469aaf31b56cb 100644
--- a/drivers/vfio/mdev/vfio_mdev.c
+++ b/drivers/vfio/mdev/vfio_mdev.c
@@ -21,6 +21,10 @@
 #define DRIVER_AUTHOR   "NVIDIA Corporation"
 #define DRIVER_DESC     "VFIO based driver for Mediated device"
 
+struct mdev_vfio_device {
+	struct vfio_device vdev;
+};
+
 static int vfio_mdev_open(void *device_data)
 {
 	struct mdev_device *mdev = device_data;
@@ -124,13 +128,29 @@ static const struct vfio_device_ops vfio_mdev_dev_ops = {
 static int vfio_mdev_probe(struct device *dev)
 {
 	struct mdev_device *mdev = to_mdev_device(dev);
+	struct mdev_vfio_device *mvdev;
+	int ret;
 
-	return vfio_add_group_dev(dev, &vfio_mdev_dev_ops, mdev);
+	mvdev = kzalloc(sizeof(*mvdev), GFP_KERNEL);
+	if (!mvdev)
+		return -ENOMEM;
+
+	vfio_init_group_dev(&mvdev->vdev, &mdev->dev, &vfio_mdev_dev_ops, mdev);
+	ret = vfio_register_group_dev(&mvdev->vdev);
+	if (ret) {
+		kfree(mvdev);
+		return ret;
+	}
+	dev_set_drvdata(&mdev->dev, mvdev);
+	return 0;
 }
 
 static void vfio_mdev_remove(struct device *dev)
 {
-	vfio_del_group_dev(dev);
+	struct mdev_vfio_device *mvdev = dev_get_drvdata(dev);
+
+	vfio_unregister_group_dev(&mvdev->vdev);
+	kfree(mvdev);
 }
 
 static struct mdev_driver vfio_mdev_driver = {
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index cfa06ae3b9018b..2d6d7cc1d1ebf9 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -99,8 +99,8 @@ MODULE_PARM_DESC(enable_unsafe_noiommu_mode, "Enable UNSAFE, no-IOMMU mode.  Thi
 /*
  * vfio_iommu_group_{get,put} are only intended for VFIO bus driver probe
  * and remove functions, any use cases other than acquiring the first
- * reference for the purpose of calling vfio_add_group_dev() or removing
- * that symmetric reference after vfio_del_group_dev() should use the raw
+ * reference for the purpose of calling vfio_register_group_dev() or removing
+ * that symmetric reference after vfio_unregister_group_dev() should use the raw
  * iommu_group_{get,put} functions.  In particular, vfio_iommu_group_put()
  * removes the device from the dummy group and cannot be nested.
  */
@@ -799,29 +799,6 @@ int vfio_register_group_dev(struct vfio_device *device)
 }
 EXPORT_SYMBOL_GPL(vfio_register_group_dev);
 
-int vfio_add_group_dev(struct device *dev, const struct vfio_device_ops *ops,
-		       void *device_data)
-{
-	struct vfio_device *device;
-	int ret;
-
-	device = kzalloc(sizeof(*device), GFP_KERNEL);
-	if (!device)
-		return -ENOMEM;
-
-	vfio_init_group_dev(device, dev, ops, device_data);
-	ret = vfio_register_group_dev(device);
-	if (ret)
-		goto err_kfree;
-	dev_set_drvdata(dev, device);
-	return 0;
-
-err_kfree:
-	kfree(device);
-	return ret;
-}
-EXPORT_SYMBOL_GPL(vfio_add_group_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
@@ -962,18 +939,6 @@ void vfio_unregister_group_dev(struct vfio_device *device)
 }
 EXPORT_SYMBOL_GPL(vfio_unregister_group_dev);
 
-void *vfio_del_group_dev(struct device *dev)
-{
-	struct vfio_device *device = dev_get_drvdata(dev);
-	void *device_data = device->device_data;
-
-	vfio_unregister_group_dev(device);
-	dev_set_drvdata(dev, NULL);
-	kfree(device);
-	return device_data;
-}
-EXPORT_SYMBOL_GPL(vfio_del_group_dev);
-
 /**
  * VFIO base fd, /dev/vfio/vfio
  */
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index ad8b579d67d34a..4995faf51efeae 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -63,11 +63,6 @@ extern void vfio_iommu_group_put(struct iommu_group *group, struct device *dev);
 void vfio_init_group_dev(struct vfio_device *device, struct device *dev,
 			 const struct vfio_device_ops *ops, void *device_data);
 int vfio_register_group_dev(struct vfio_device *device);
-extern int vfio_add_group_dev(struct device *dev,
-			      const struct vfio_device_ops *ops,
-			      void *device_data);
-
-extern void *vfio_del_group_dev(struct device *dev);
 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);
-- 
2.30.1

Re: [PATCH 06/10] vfio/mdev: Use vfio_init/register/unregister_group_dev

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

RE: [PATCH 06/10] vfio/mdev: Use vfio_init/register/unregister_group_dev

[Liu, Yi L]

Hi Jason,

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, March 10, 2021 5:39 AM
> 
> mdev gets little benefit because it doesn't actually do anything, however
> it is the last user, so move the code here for now.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---

Reviewed-by: Liu Yi L <yi.l.liu@intel.com>

Regards,
Yi Liu

[PATCH 07/10] vfio/mdev: Make to_mdev_device() into a static inline

[Jason Gunthorpe]

The macro wrongly uses 'dev' as both the macro argument and the member
name, which means it fails compilation if any caller uses a word other
than 'dev' as the single argument. Fix this defect by making it into
proper static inline, which is more clear and typesafe anyhow.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/mdev/mdev_private.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
index 7d922950caaf3c..74c2e541146999 100644
--- a/drivers/vfio/mdev/mdev_private.h
+++ b/drivers/vfio/mdev/mdev_private.h
@@ -35,7 +35,10 @@ struct mdev_device {
 	bool active;
 };
 
-#define to_mdev_device(dev)	container_of(dev, struct mdev_device, dev)
+static inline struct mdev_device *to_mdev_device(struct device *dev)
+{
+	return container_of(dev, struct mdev_device, dev);
+}
 #define dev_is_mdev(d)		((d)->bus == &mdev_bus_type)
 
 struct mdev_type {
-- 
2.30.1

Re: [PATCH 07/10] vfio/mdev: Make to_mdev_device() into a static inline

[Christoph Hellwig]

On Tue, Mar 09, 2021 at 05:38:49PM -0400, Jason Gunthorpe wrote:
> The macro wrongly uses 'dev' as both the macro argument and the member
> name, which means it fails compilation if any caller uses a word other
> than 'dev' as the single argument. Fix this defect by making it into
> proper static inline, which is more clear and typesafe anyhow.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 08/10] vfio: Make vfio_device_ops pass a 'struct vfio_device *' instead of 'void *'

[Jason Gunthorpe]

This is the standard kernel pattern, the ops associated with a struct get
the struct pointer in for typesafety. The expected design is to use
container_of to cleanly go from the subsystem level type to the driver
level type without having any type erasure in a void *.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 Documentation/driver-api/vfio.rst            | 18 ++++----
 drivers/vfio/fsl-mc/vfio_fsl_mc.c            | 36 +++++++++------
 drivers/vfio/mdev/vfio_mdev.c                | 33 +++++++-------
 drivers/vfio/pci/vfio_pci.c                  | 47 ++++++++++++--------
 drivers/vfio/platform/vfio_platform_common.c | 33 ++++++++------
 drivers/vfio/vfio.c                          | 20 ++++-----
 include/linux/vfio.h                         | 16 +++----
 7 files changed, 117 insertions(+), 86 deletions(-)

diff --git a/Documentation/driver-api/vfio.rst b/Documentation/driver-api/vfio.rst
index d3a02300913a7f..3337f337293a32 100644
--- a/Documentation/driver-api/vfio.rst
+++ b/Documentation/driver-api/vfio.rst
@@ -269,20 +269,22 @@ ready before calling it. The driver provides an ops structure for callbacks
 similar to a file operations structure::
 
 	struct vfio_device_ops {
-		int	(*open)(void *device_data);
-		void	(*release)(void *device_data);
-		ssize_t	(*read)(void *device_data, char __user *buf,
+		int	(*open)(struct vfio_device *vdev);
+		void	(*release)(struct vfio_device *vdev);
+		ssize_t	(*read)(struct vfio_device *vdev, char __user *buf,
 				size_t count, loff_t *ppos);
-		ssize_t	(*write)(void *device_data, const char __user *buf,
+		ssize_t	(*write)(struct vfio_device *vdev,
+				 const char __user *buf,
 				 size_t size, loff_t *ppos);
-		long	(*ioctl)(void *device_data, unsigned int cmd,
+		long	(*ioctl)(struct vfio_device *vdev, unsigned int cmd,
 				 unsigned long arg);
-		int	(*mmap)(void *device_data, struct vm_area_struct *vma);
+		int	(*mmap)(struct vfio_device *vdev,
+				struct vm_area_struct *vma);
 	};
 
-Each function is passed the device_data that was originally registered
+Each function is passed the vdev that was originally registered
 in the vfio_register_group_dev() call above.  This allows the bus driver
-an easy place to store its opaque, private data.  The open/release
+to obtain its private data using container_of().  The open/release
 callbacks are issued when a new file descriptor is created for a
 device (via VFIO_GROUP_GET_DEVICE_FD).  The ioctl interface provides
 a direct pass through for VFIO_DEVICE_* ioctls.  The read/write/mmap
diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
index ddee6ed20c4523..74a5de1b791934 100644
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
@@ -135,9 +135,10 @@ static void vfio_fsl_mc_regions_cleanup(struct vfio_fsl_mc_device *vdev)
 	kfree(vdev->regions);
 }
 
-static int vfio_fsl_mc_open(void *device_data)
+static int vfio_fsl_mc_open(struct vfio_device *core_vdev)
 {
-	struct vfio_fsl_mc_device *vdev = device_data;
+	struct vfio_fsl_mc_device *vdev =
+		container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
 	int ret;
 
 	if (!try_module_get(THIS_MODULE))
@@ -161,9 +162,10 @@ static int vfio_fsl_mc_open(void *device_data)
 	return ret;
 }
 
-static void vfio_fsl_mc_release(void *device_data)
+static void vfio_fsl_mc_release(struct vfio_device *core_vdev)
 {
-	struct vfio_fsl_mc_device *vdev = device_data;
+	struct vfio_fsl_mc_device *vdev =
+		container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
 	int ret;
 
 	mutex_lock(&vdev->reflck->lock);
@@ -197,11 +199,12 @@ static void vfio_fsl_mc_release(void *device_data)
 	module_put(THIS_MODULE);
 }
 
-static long vfio_fsl_mc_ioctl(void *device_data, unsigned int cmd,
-			      unsigned long arg)
+static long vfio_fsl_mc_ioctl(struct vfio_device *core_vdev,
+			      unsigned int cmd, unsigned long arg)
 {
 	unsigned long minsz;
-	struct vfio_fsl_mc_device *vdev = device_data;
+	struct vfio_fsl_mc_device *vdev =
+		container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
 	struct fsl_mc_device *mc_dev = vdev->mc_dev;
 
 	switch (cmd) {
@@ -327,10 +330,11 @@ static long vfio_fsl_mc_ioctl(void *device_data, unsigned int cmd,
 	}
 }
 
-static ssize_t vfio_fsl_mc_read(void *device_data, char __user *buf,
+static ssize_t vfio_fsl_mc_read(struct vfio_device *core_vdev, char __user *buf,
 				size_t count, loff_t *ppos)
 {
-	struct vfio_fsl_mc_device *vdev = device_data;
+	struct vfio_fsl_mc_device *vdev =
+		container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
 	unsigned int index = VFIO_FSL_MC_OFFSET_TO_INDEX(*ppos);
 	loff_t off = *ppos & VFIO_FSL_MC_OFFSET_MASK;
 	struct fsl_mc_device *mc_dev = vdev->mc_dev;
@@ -404,10 +408,12 @@ static int vfio_fsl_mc_send_command(void __iomem *ioaddr, uint64_t *cmd_data)
 	return 0;
 }
 
-static ssize_t vfio_fsl_mc_write(void *device_data, const char __user *buf,
-				 size_t count, loff_t *ppos)
+static ssize_t vfio_fsl_mc_write(struct vfio_device *core_vdev,
+				 const char __user *buf, size_t count,
+				 loff_t *ppos)
 {
-	struct vfio_fsl_mc_device *vdev = device_data;
+	struct vfio_fsl_mc_device *vdev =
+		container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
 	unsigned int index = VFIO_FSL_MC_OFFSET_TO_INDEX(*ppos);
 	loff_t off = *ppos & VFIO_FSL_MC_OFFSET_MASK;
 	struct fsl_mc_device *mc_dev = vdev->mc_dev;
@@ -468,9 +474,11 @@ static int vfio_fsl_mc_mmap_mmio(struct vfio_fsl_mc_region region,
 			       size, vma->vm_page_prot);
 }
 
-static int vfio_fsl_mc_mmap(void *device_data, struct vm_area_struct *vma)
+static int vfio_fsl_mc_mmap(struct vfio_device *core_vdev,
+			    struct vm_area_struct *vma)
 {
-	struct vfio_fsl_mc_device *vdev = device_data;
+	struct vfio_fsl_mc_device *vdev =
+		container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
 	struct fsl_mc_device *mc_dev = vdev->mc_dev;
 	unsigned int index;
 
diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
index 4469aaf31b56cb..e7309caa99c71b 100644
--- a/drivers/vfio/mdev/vfio_mdev.c
+++ b/drivers/vfio/mdev/vfio_mdev.c
@@ -25,10 +25,11 @@ struct mdev_vfio_device {
 	struct vfio_device vdev;
 };
 
-static int vfio_mdev_open(void *device_data)
+static int vfio_mdev_open(struct vfio_device *core_vdev)
 {
-	struct mdev_device *mdev = device_data;
+	struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
 	struct mdev_parent *parent = mdev->parent;
+
 	int ret;
 
 	if (unlikely(!parent->ops->open))
@@ -44,9 +45,9 @@ static int vfio_mdev_open(void *device_data)
 	return ret;
 }
 
-static void vfio_mdev_release(void *device_data)
+static void vfio_mdev_release(struct vfio_device *core_vdev)
 {
-	struct mdev_device *mdev = device_data;
+	struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
 	struct mdev_parent *parent = mdev->parent;
 
 	if (likely(parent->ops->release))
@@ -55,10 +56,10 @@ static void vfio_mdev_release(void *device_data)
 	module_put(THIS_MODULE);
 }
 
-static long vfio_mdev_unlocked_ioctl(void *device_data,
+static long vfio_mdev_unlocked_ioctl(struct vfio_device *core_vdev,
 				     unsigned int cmd, unsigned long arg)
 {
-	struct mdev_device *mdev = device_data;
+	struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
 	struct mdev_parent *parent = mdev->parent;
 
 	if (unlikely(!parent->ops->ioctl))
@@ -67,10 +68,10 @@ static long vfio_mdev_unlocked_ioctl(void *device_data,
 	return parent->ops->ioctl(mdev, cmd, arg);
 }
 
-static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
+static ssize_t vfio_mdev_read(struct vfio_device *core_vdev, char __user *buf,
 			      size_t count, loff_t *ppos)
 {
-	struct mdev_device *mdev = device_data;
+	struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
 	struct mdev_parent *parent = mdev->parent;
 
 	if (unlikely(!parent->ops->read))
@@ -79,10 +80,11 @@ static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
 	return parent->ops->read(mdev, buf, count, ppos);
 }
 
-static ssize_t vfio_mdev_write(void *device_data, const char __user *buf,
-			       size_t count, loff_t *ppos)
+static ssize_t vfio_mdev_write(struct vfio_device *core_vdev,
+			       const char __user *buf, size_t count,
+			       loff_t *ppos)
 {
-	struct mdev_device *mdev = device_data;
+	struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
 	struct mdev_parent *parent = mdev->parent;
 
 	if (unlikely(!parent->ops->write))
@@ -91,9 +93,10 @@ static ssize_t vfio_mdev_write(void *device_data, const char __user *buf,
 	return parent->ops->write(mdev, buf, count, ppos);
 }
 
-static int vfio_mdev_mmap(void *device_data, struct vm_area_struct *vma)
+static int vfio_mdev_mmap(struct vfio_device *core_vdev,
+			  struct vm_area_struct *vma)
 {
-	struct mdev_device *mdev = device_data;
+	struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
 	struct mdev_parent *parent = mdev->parent;
 
 	if (unlikely(!parent->ops->mmap))
@@ -102,9 +105,9 @@ static int vfio_mdev_mmap(void *device_data, struct vm_area_struct *vma)
 	return parent->ops->mmap(mdev, vma);
 }
 
-static void vfio_mdev_request(void *device_data, unsigned int count)
+static void vfio_mdev_request(struct vfio_device *core_vdev, unsigned int count)
 {
-	struct mdev_device *mdev = device_data;
+	struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
 	struct mdev_parent *parent = mdev->parent;
 
 	if (parent->ops->request)
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index fae573c6f86bdf..af5696a96a76e0 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -553,9 +553,10 @@ static void vfio_pci_vf_token_user_add(struct vfio_pci_device *vdev, int val)
 	vfio_device_put(pf_dev);
 }
 
-static void vfio_pci_release(void *device_data)
+static void vfio_pci_release(struct vfio_device *core_vdev)
 {
-	struct vfio_pci_device *vdev = device_data;
+	struct vfio_pci_device *vdev =
+		container_of(core_vdev, struct vfio_pci_device, vdev);
 
 	mutex_lock(&vdev->reflck->lock);
 
@@ -581,9 +582,10 @@ static void vfio_pci_release(void *device_data)
 	module_put(THIS_MODULE);
 }
 
-static int vfio_pci_open(void *device_data)
+static int vfio_pci_open(struct vfio_device *core_vdev)
 {
-	struct vfio_pci_device *vdev = device_data;
+	struct vfio_pci_device *vdev =
+		container_of(core_vdev, struct vfio_pci_device, vdev);
 	int ret = 0;
 
 	if (!try_module_get(THIS_MODULE))
@@ -797,10 +799,11 @@ struct vfio_devices {
 	int max_index;
 };
 
-static long vfio_pci_ioctl(void *device_data,
+static long vfio_pci_ioctl(struct vfio_device *core_vdev,
 			   unsigned int cmd, unsigned long arg)
 {
-	struct vfio_pci_device *vdev = device_data;
+	struct vfio_pci_device *vdev =
+		container_of(core_vdev, struct vfio_pci_device, vdev);
 	unsigned long minsz;
 
 	if (cmd == VFIO_DEVICE_GET_INFO) {
@@ -1402,11 +1405,10 @@ static long vfio_pci_ioctl(void *device_data,
 	return -ENOTTY;
 }
 
-static ssize_t vfio_pci_rw(void *device_data, char __user *buf,
+static ssize_t vfio_pci_rw(struct vfio_pci_device *vdev, char __user *buf,
 			   size_t count, loff_t *ppos, bool iswrite)
 {
 	unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
-	struct vfio_pci_device *vdev = device_data;
 
 	if (index >= VFIO_PCI_NUM_REGIONS + vdev->num_regions)
 		return -EINVAL;
@@ -1434,22 +1436,28 @@ static ssize_t vfio_pci_rw(void *device_data, char __user *buf,
 	return -EINVAL;
 }
 
-static ssize_t vfio_pci_read(void *device_data, char __user *buf,
+static ssize_t vfio_pci_read(struct vfio_device *core_vdev, char __user *buf,
 			     size_t count, loff_t *ppos)
 {
+	struct vfio_pci_device *vdev =
+		container_of(core_vdev, struct vfio_pci_device, vdev);
+
 	if (!count)
 		return 0;
 
-	return vfio_pci_rw(device_data, buf, count, ppos, false);
+	return vfio_pci_rw(vdev, buf, count, ppos, false);
 }
 
-static ssize_t vfio_pci_write(void *device_data, const char __user *buf,
+static ssize_t vfio_pci_write(struct vfio_device *core_vdev, const char __user *buf,
 			      size_t count, loff_t *ppos)
 {
+	struct vfio_pci_device *vdev =
+		container_of(core_vdev, struct vfio_pci_device, vdev);
+
 	if (!count)
 		return 0;
 
-	return vfio_pci_rw(device_data, (char __user *)buf, count, ppos, true);
+	return vfio_pci_rw(vdev, (char __user *)buf, count, ppos, true);
 }
 
 /* Return 1 on zap and vma_lock acquired, 0 on contention (only with @try) */
@@ -1646,9 +1654,10 @@ static const struct vm_operations_struct vfio_pci_mmap_ops = {
 	.fault = vfio_pci_mmap_fault,
 };
 
-static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
+static int vfio_pci_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma)
 {
-	struct vfio_pci_device *vdev = device_data;
+	struct vfio_pci_device *vdev =
+		container_of(core_vdev, struct vfio_pci_device, vdev);
 	struct pci_dev *pdev = vdev->pdev;
 	unsigned int index;
 	u64 phys_len, req_len, pgoff, req_start;
@@ -1714,9 +1723,10 @@ static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
 	return 0;
 }
 
-static void vfio_pci_request(void *device_data, unsigned int count)
+static void vfio_pci_request(struct vfio_device *core_vdev, unsigned int count)
 {
-	struct vfio_pci_device *vdev = device_data;
+	struct vfio_pci_device *vdev =
+		container_of(core_vdev, struct vfio_pci_device, vdev);
 	struct pci_dev *pdev = vdev->pdev;
 
 	mutex_lock(&vdev->igate);
@@ -1830,9 +1840,10 @@ static int vfio_pci_validate_vf_token(struct vfio_pci_device *vdev,
 
 #define VF_TOKEN_ARG "vf_token="
 
-static int vfio_pci_match(void *device_data, char *buf)
+static int vfio_pci_match(struct vfio_device *core_vdev, char *buf)
 {
-	struct vfio_pci_device *vdev = device_data;
+	struct vfio_pci_device *vdev =
+		container_of(core_vdev, struct vfio_pci_device, vdev);
 	bool vf_token = false;
 	uuid_t uuid;
 	int ret;
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 6eb749250ee41c..f5f6b537084a67 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -218,9 +218,10 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
 	return -EINVAL;
 }
 
-static void vfio_platform_release(void *device_data)
+static void vfio_platform_release(struct vfio_device *core_vdev)
 {
-	struct vfio_platform_device *vdev = device_data;
+	struct vfio_platform_device *vdev =
+		container_of(core_vdev, struct vfio_platform_device, vdev);
 
 	mutex_lock(&driver_lock);
 
@@ -244,9 +245,10 @@ static void vfio_platform_release(void *device_data)
 	module_put(vdev->parent_module);
 }
 
-static int vfio_platform_open(void *device_data)
+static int vfio_platform_open(struct vfio_device *core_vdev)
 {
-	struct vfio_platform_device *vdev = device_data;
+	struct vfio_platform_device *vdev =
+		container_of(core_vdev, struct vfio_platform_device, vdev);
 	int ret;
 
 	if (!try_module_get(vdev->parent_module))
@@ -293,10 +295,12 @@ static int vfio_platform_open(void *device_data)
 	return ret;
 }
 
-static long vfio_platform_ioctl(void *device_data,
+static long vfio_platform_ioctl(struct vfio_device *core_vdev,
 				unsigned int cmd, unsigned long arg)
 {
-	struct vfio_platform_device *vdev = device_data;
+	struct vfio_platform_device *vdev =
+		container_of(core_vdev, struct vfio_platform_device, vdev);
+
 	unsigned long minsz;
 
 	if (cmd == VFIO_DEVICE_GET_INFO) {
@@ -455,10 +459,11 @@ static ssize_t vfio_platform_read_mmio(struct vfio_platform_region *reg,
 	return -EFAULT;
 }
 
-static ssize_t vfio_platform_read(void *device_data, char __user *buf,
-				  size_t count, loff_t *ppos)
+static ssize_t vfio_platform_read(struct vfio_device *core_vdev,
+				  char __user *buf, size_t count, loff_t *ppos)
 {
-	struct vfio_platform_device *vdev = device_data;
+	struct vfio_platform_device *vdev =
+		container_of(core_vdev, struct vfio_platform_device, vdev);
 	unsigned int index = VFIO_PLATFORM_OFFSET_TO_INDEX(*ppos);
 	loff_t off = *ppos & VFIO_PLATFORM_OFFSET_MASK;
 
@@ -531,10 +536,11 @@ static ssize_t vfio_platform_write_mmio(struct vfio_platform_region *reg,
 	return -EFAULT;
 }
 
-static ssize_t vfio_platform_write(void *device_data, const char __user *buf,
+static ssize_t vfio_platform_write(struct vfio_device *core_vdev, const char __user *buf,
 				   size_t count, loff_t *ppos)
 {
-	struct vfio_platform_device *vdev = device_data;
+	struct vfio_platform_device *vdev =
+		container_of(core_vdev, struct vfio_platform_device, vdev);
 	unsigned int index = VFIO_PLATFORM_OFFSET_TO_INDEX(*ppos);
 	loff_t off = *ppos & VFIO_PLATFORM_OFFSET_MASK;
 
@@ -573,9 +579,10 @@ static int vfio_platform_mmap_mmio(struct vfio_platform_region region,
 			       req_len, vma->vm_page_prot);
 }
 
-static int vfio_platform_mmap(void *device_data, struct vm_area_struct *vma)
+static int vfio_platform_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma)
 {
-	struct vfio_platform_device *vdev = device_data;
+	struct vfio_platform_device *vdev =
+		container_of(core_vdev, struct vfio_platform_device, vdev);
 	unsigned int index;
 
 	index = vma->vm_pgoff >> (VFIO_PLATFORM_OFFSET_SHIFT - PAGE_SHIFT);
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 2d6d7cc1d1ebf9..01de47d1810b6b 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -832,7 +832,7 @@ static struct vfio_device *vfio_device_get_from_name(struct vfio_group *group,
 		int ret;
 
 		if (it->ops->match) {
-			ret = it->ops->match(it->device_data, buf);
+			ret = it->ops->match(it, buf);
 			if (ret < 0) {
 				device = ERR_PTR(ret);
 				break;
@@ -893,7 +893,7 @@ void vfio_unregister_group_dev(struct vfio_device *device)
 	rc = try_wait_for_completion(&device->comp);
 	while (rc <= 0) {
 		if (device->ops->request)
-			device->ops->request(device->device_data, i++);
+			device->ops->request(device, i++);
 
 		if (interrupted) {
 			rc = wait_for_completion_timeout(&device->comp,
@@ -1379,7 +1379,7 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
 	if (IS_ERR(device))
 		return PTR_ERR(device);
 
-	ret = device->ops->open(device->device_data);
+	ret = device->ops->open(device);
 	if (ret) {
 		vfio_device_put(device);
 		return ret;
@@ -1391,7 +1391,7 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
 	 */
 	ret = get_unused_fd_flags(O_CLOEXEC);
 	if (ret < 0) {
-		device->ops->release(device->device_data);
+		device->ops->release(device);
 		vfio_device_put(device);
 		return ret;
 	}
@@ -1401,7 +1401,7 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
 	if (IS_ERR(filep)) {
 		put_unused_fd(ret);
 		ret = PTR_ERR(filep);
-		device->ops->release(device->device_data);
+		device->ops->release(device);
 		vfio_device_put(device);
 		return ret;
 	}
@@ -1558,7 +1558,7 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
 {
 	struct vfio_device *device = filep->private_data;
 
-	device->ops->release(device->device_data);
+	device->ops->release(device);
 
 	vfio_group_try_dissolve_container(device->group);
 
@@ -1575,7 +1575,7 @@ static long vfio_device_fops_unl_ioctl(struct file *filep,
 	if (unlikely(!device->ops->ioctl))
 		return -EINVAL;
 
-	return device->ops->ioctl(device->device_data, cmd, arg);
+	return device->ops->ioctl(device, cmd, arg);
 }
 
 static ssize_t vfio_device_fops_read(struct file *filep, char __user *buf,
@@ -1586,7 +1586,7 @@ static ssize_t vfio_device_fops_read(struct file *filep, char __user *buf,
 	if (unlikely(!device->ops->read))
 		return -EINVAL;
 
-	return device->ops->read(device->device_data, buf, count, ppos);
+	return device->ops->read(device, buf, count, ppos);
 }
 
 static ssize_t vfio_device_fops_write(struct file *filep,
@@ -1598,7 +1598,7 @@ static ssize_t vfio_device_fops_write(struct file *filep,
 	if (unlikely(!device->ops->write))
 		return -EINVAL;
 
-	return device->ops->write(device->device_data, buf, count, ppos);
+	return device->ops->write(device, buf, count, ppos);
 }
 
 static int vfio_device_fops_mmap(struct file *filep, struct vm_area_struct *vma)
@@ -1608,7 +1608,7 @@ static int vfio_device_fops_mmap(struct file *filep, struct vm_area_struct *vma)
 	if (unlikely(!device->ops->mmap))
 		return -EINVAL;
 
-	return device->ops->mmap(device->device_data, vma);
+	return device->ops->mmap(device, vma);
 }
 
 static const struct file_operations vfio_device_fops = {
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 4995faf51efeae..784c34c0a28763 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -44,17 +44,17 @@ struct vfio_device {
  */
 struct vfio_device_ops {
 	char	*name;
-	int	(*open)(void *device_data);
-	void	(*release)(void *device_data);
-	ssize_t	(*read)(void *device_data, char __user *buf,
+	int	(*open)(struct vfio_device *vdev);
+	void	(*release)(struct vfio_device *vdev);
+	ssize_t	(*read)(struct vfio_device *vdev, char __user *buf,
 			size_t count, loff_t *ppos);
-	ssize_t	(*write)(void *device_data, const char __user *buf,
+	ssize_t	(*write)(struct vfio_device *vdev, const char __user *buf,
 			 size_t count, loff_t *size);
-	long	(*ioctl)(void *device_data, unsigned int cmd,
+	long	(*ioctl)(struct vfio_device *vdev, unsigned int cmd,
 			 unsigned long arg);
-	int	(*mmap)(void *device_data, struct vm_area_struct *vma);
-	void	(*request)(void *device_data, unsigned int count);
-	int	(*match)(void *device_data, char *buf);
+	int	(*mmap)(struct vfio_device *vdev, struct vm_area_struct *vma);
+	void	(*request)(struct vfio_device *vdev, unsigned int count);
+	int	(*match)(struct vfio_device *vdev, char *buf);
 };
 
 extern struct iommu_group *vfio_iommu_group_get(struct device *dev);
-- 
2.30.1

Re: [PATCH 08/10] vfio: Make vfio_device_ops pass a 'struct vfio_device *' instead of 'void *'

[Dan Williams]

On Tue, Mar 9, 2021 at 1:39 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> This is the standard kernel pattern, the ops associated with a struct get
> the struct pointer in for typesafety. The expected design is to use
> container_of to cleanly go from the subsystem level type to the driver
> level type without having any type erasure in a void *.

This patch alone is worth the price of admission.

Seems like it would be worth adding
to_vfio_{pci,platform,fsl_mc}_device() helpers in this patch as well.

I've sometimes added runtime type safety to to_* helpers for early
warning of mistakes that happen when refactoring...

static inline struct vfio_pci_device *
to_vfio_pci_device(struct vfio_device *core_dev)
{
        if (dev_WARN_ONCE(core_dev->dev, core_dev->ops != &vfio_pci_ops,
                          "not a vfio_pci_device!\n"))
                return NULL;
        return container_of(core_vdev, struct vfio_pci_device, vdev);
}

...but typed ops is already a significant idiomatic improvement.


>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  Documentation/driver-api/vfio.rst            | 18 ++++----
>  drivers/vfio/fsl-mc/vfio_fsl_mc.c            | 36 +++++++++------
>  drivers/vfio/mdev/vfio_mdev.c                | 33 +++++++-------
>  drivers/vfio/pci/vfio_pci.c                  | 47 ++++++++++++--------
>  drivers/vfio/platform/vfio_platform_common.c | 33 ++++++++------
>  drivers/vfio/vfio.c                          | 20 ++++-----
>  include/linux/vfio.h                         | 16 +++----
>  7 files changed, 117 insertions(+), 86 deletions(-)
>
> diff --git a/Documentation/driver-api/vfio.rst b/Documentation/driver-api/vfio.rst
> index d3a02300913a7f..3337f337293a32 100644
> --- a/Documentation/driver-api/vfio.rst
> +++ b/Documentation/driver-api/vfio.rst
> @@ -269,20 +269,22 @@ ready before calling it. The driver provides an ops structure for callbacks
>  similar to a file operations structure::
>
>         struct vfio_device_ops {
> -               int     (*open)(void *device_data);
> -               void    (*release)(void *device_data);
> -               ssize_t (*read)(void *device_data, char __user *buf,
> +               int     (*open)(struct vfio_device *vdev);
> +               void    (*release)(struct vfio_device *vdev);
> +               ssize_t (*read)(struct vfio_device *vdev, char __user *buf,
>                                 size_t count, loff_t *ppos);
> -               ssize_t (*write)(void *device_data, const char __user *buf,
> +               ssize_t (*write)(struct vfio_device *vdev,
> +                                const char __user *buf,
>                                  size_t size, loff_t *ppos);
> -               long    (*ioctl)(void *device_data, unsigned int cmd,
> +               long    (*ioctl)(struct vfio_device *vdev, unsigned int cmd,
>                                  unsigned long arg);
> -               int     (*mmap)(void *device_data, struct vm_area_struct *vma);
> +               int     (*mmap)(struct vfio_device *vdev,
> +                               struct vm_area_struct *vma);
>         };
>
> -Each function is passed the device_data that was originally registered
> +Each function is passed the vdev that was originally registered
>  in the vfio_register_group_dev() call above.  This allows the bus driver
> -an easy place to store its opaque, private data.  The open/release
> +to obtain its private data using container_of().  The open/release
>  callbacks are issued when a new file descriptor is created for a
>  device (via VFIO_GROUP_GET_DEVICE_FD).  The ioctl interface provides
>  a direct pass through for VFIO_DEVICE_* ioctls.  The read/write/mmap
> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> index ddee6ed20c4523..74a5de1b791934 100644
> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> @@ -135,9 +135,10 @@ static void vfio_fsl_mc_regions_cleanup(struct vfio_fsl_mc_device *vdev)
>         kfree(vdev->regions);
>  }
>
> -static int vfio_fsl_mc_open(void *device_data)
> +static int vfio_fsl_mc_open(struct vfio_device *core_vdev)
>  {
> -       struct vfio_fsl_mc_device *vdev = device_data;
> +       struct vfio_fsl_mc_device *vdev =
> +               container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
>         int ret;
>
>         if (!try_module_get(THIS_MODULE))
> @@ -161,9 +162,10 @@ static int vfio_fsl_mc_open(void *device_data)
>         return ret;
>  }
>
> -static void vfio_fsl_mc_release(void *device_data)
> +static void vfio_fsl_mc_release(struct vfio_device *core_vdev)
>  {
> -       struct vfio_fsl_mc_device *vdev = device_data;
> +       struct vfio_fsl_mc_device *vdev =
> +               container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
>         int ret;
>
>         mutex_lock(&vdev->reflck->lock);
> @@ -197,11 +199,12 @@ static void vfio_fsl_mc_release(void *device_data)
>         module_put(THIS_MODULE);
>  }
>
> -static long vfio_fsl_mc_ioctl(void *device_data, unsigned int cmd,
> -                             unsigned long arg)
> +static long vfio_fsl_mc_ioctl(struct vfio_device *core_vdev,
> +                             unsigned int cmd, unsigned long arg)
>  {
>         unsigned long minsz;
> -       struct vfio_fsl_mc_device *vdev = device_data;
> +       struct vfio_fsl_mc_device *vdev =
> +               container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
>         struct fsl_mc_device *mc_dev = vdev->mc_dev;
>
>         switch (cmd) {
> @@ -327,10 +330,11 @@ static long vfio_fsl_mc_ioctl(void *device_data, unsigned int cmd,
>         }
>  }
>
> -static ssize_t vfio_fsl_mc_read(void *device_data, char __user *buf,
> +static ssize_t vfio_fsl_mc_read(struct vfio_device *core_vdev, char __user *buf,
>                                 size_t count, loff_t *ppos)
>  {
> -       struct vfio_fsl_mc_device *vdev = device_data;
> +       struct vfio_fsl_mc_device *vdev =
> +               container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
>         unsigned int index = VFIO_FSL_MC_OFFSET_TO_INDEX(*ppos);
>         loff_t off = *ppos & VFIO_FSL_MC_OFFSET_MASK;
>         struct fsl_mc_device *mc_dev = vdev->mc_dev;
> @@ -404,10 +408,12 @@ static int vfio_fsl_mc_send_command(void __iomem *ioaddr, uint64_t *cmd_data)
>         return 0;
>  }
>
> -static ssize_t vfio_fsl_mc_write(void *device_data, const char __user *buf,
> -                                size_t count, loff_t *ppos)
> +static ssize_t vfio_fsl_mc_write(struct vfio_device *core_vdev,
> +                                const char __user *buf, size_t count,
> +                                loff_t *ppos)
>  {
> -       struct vfio_fsl_mc_device *vdev = device_data;
> +       struct vfio_fsl_mc_device *vdev =
> +               container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
>         unsigned int index = VFIO_FSL_MC_OFFSET_TO_INDEX(*ppos);
>         loff_t off = *ppos & VFIO_FSL_MC_OFFSET_MASK;
>         struct fsl_mc_device *mc_dev = vdev->mc_dev;
> @@ -468,9 +474,11 @@ static int vfio_fsl_mc_mmap_mmio(struct vfio_fsl_mc_region region,
>                                size, vma->vm_page_prot);
>  }
>
> -static int vfio_fsl_mc_mmap(void *device_data, struct vm_area_struct *vma)
> +static int vfio_fsl_mc_mmap(struct vfio_device *core_vdev,
> +                           struct vm_area_struct *vma)
>  {
> -       struct vfio_fsl_mc_device *vdev = device_data;
> +       struct vfio_fsl_mc_device *vdev =
> +               container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
>         struct fsl_mc_device *mc_dev = vdev->mc_dev;
>         unsigned int index;
>
> diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
> index 4469aaf31b56cb..e7309caa99c71b 100644
> --- a/drivers/vfio/mdev/vfio_mdev.c
> +++ b/drivers/vfio/mdev/vfio_mdev.c
> @@ -25,10 +25,11 @@ struct mdev_vfio_device {
>         struct vfio_device vdev;
>  };
>
> -static int vfio_mdev_open(void *device_data)
> +static int vfio_mdev_open(struct vfio_device *core_vdev)
>  {
> -       struct mdev_device *mdev = device_data;
> +       struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
>         struct mdev_parent *parent = mdev->parent;
> +
>         int ret;
>
>         if (unlikely(!parent->ops->open))
> @@ -44,9 +45,9 @@ static int vfio_mdev_open(void *device_data)
>         return ret;
>  }
>
> -static void vfio_mdev_release(void *device_data)
> +static void vfio_mdev_release(struct vfio_device *core_vdev)
>  {
> -       struct mdev_device *mdev = device_data;
> +       struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
>         struct mdev_parent *parent = mdev->parent;
>
>         if (likely(parent->ops->release))
> @@ -55,10 +56,10 @@ static void vfio_mdev_release(void *device_data)
>         module_put(THIS_MODULE);
>  }
>
> -static long vfio_mdev_unlocked_ioctl(void *device_data,
> +static long vfio_mdev_unlocked_ioctl(struct vfio_device *core_vdev,
>                                      unsigned int cmd, unsigned long arg)
>  {
> -       struct mdev_device *mdev = device_data;
> +       struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
>         struct mdev_parent *parent = mdev->parent;
>
>         if (unlikely(!parent->ops->ioctl))
> @@ -67,10 +68,10 @@ static long vfio_mdev_unlocked_ioctl(void *device_data,
>         return parent->ops->ioctl(mdev, cmd, arg);
>  }
>
> -static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
> +static ssize_t vfio_mdev_read(struct vfio_device *core_vdev, char __user *buf,
>                               size_t count, loff_t *ppos)
>  {
> -       struct mdev_device *mdev = device_data;
> +       struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
>         struct mdev_parent *parent = mdev->parent;
>
>         if (unlikely(!parent->ops->read))
> @@ -79,10 +80,11 @@ static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
>         return parent->ops->read(mdev, buf, count, ppos);
>  }
>
> -static ssize_t vfio_mdev_write(void *device_data, const char __user *buf,
> -                              size_t count, loff_t *ppos)
> +static ssize_t vfio_mdev_write(struct vfio_device *core_vdev,
> +                              const char __user *buf, size_t count,
> +                              loff_t *ppos)
>  {
> -       struct mdev_device *mdev = device_data;
> +       struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
>         struct mdev_parent *parent = mdev->parent;
>
>         if (unlikely(!parent->ops->write))
> @@ -91,9 +93,10 @@ static ssize_t vfio_mdev_write(void *device_data, const char __user *buf,
>         return parent->ops->write(mdev, buf, count, ppos);
>  }
>
> -static int vfio_mdev_mmap(void *device_data, struct vm_area_struct *vma)
> +static int vfio_mdev_mmap(struct vfio_device *core_vdev,
> +                         struct vm_area_struct *vma)
>  {
> -       struct mdev_device *mdev = device_data;
> +       struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
>         struct mdev_parent *parent = mdev->parent;
>
>         if (unlikely(!parent->ops->mmap))
> @@ -102,9 +105,9 @@ static int vfio_mdev_mmap(void *device_data, struct vm_area_struct *vma)
>         return parent->ops->mmap(mdev, vma);
>  }
>
> -static void vfio_mdev_request(void *device_data, unsigned int count)
> +static void vfio_mdev_request(struct vfio_device *core_vdev, unsigned int count)
>  {
> -       struct mdev_device *mdev = device_data;
> +       struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
>         struct mdev_parent *parent = mdev->parent;
>
>         if (parent->ops->request)
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index fae573c6f86bdf..af5696a96a76e0 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -553,9 +553,10 @@ static void vfio_pci_vf_token_user_add(struct vfio_pci_device *vdev, int val)
>         vfio_device_put(pf_dev);
>  }
>
> -static void vfio_pci_release(void *device_data)
> +static void vfio_pci_release(struct vfio_device *core_vdev)
>  {
> -       struct vfio_pci_device *vdev = device_data;
> +       struct vfio_pci_device *vdev =
> +               container_of(core_vdev, struct vfio_pci_device, vdev);
>
>         mutex_lock(&vdev->reflck->lock);
>
> @@ -581,9 +582,10 @@ static void vfio_pci_release(void *device_data)
>         module_put(THIS_MODULE);
>  }
>
> -static int vfio_pci_open(void *device_data)
> +static int vfio_pci_open(struct vfio_device *core_vdev)
>  {
> -       struct vfio_pci_device *vdev = device_data;
> +       struct vfio_pci_device *vdev =
> +               container_of(core_vdev, struct vfio_pci_device, vdev);
>         int ret = 0;
>
>         if (!try_module_get(THIS_MODULE))
> @@ -797,10 +799,11 @@ struct vfio_devices {
>         int max_index;
>  };
>
> -static long vfio_pci_ioctl(void *device_data,
> +static long vfio_pci_ioctl(struct vfio_device *core_vdev,
>                            unsigned int cmd, unsigned long arg)
>  {
> -       struct vfio_pci_device *vdev = device_data;
> +       struct vfio_pci_device *vdev =
> +               container_of(core_vdev, struct vfio_pci_device, vdev);
>         unsigned long minsz;
>
>         if (cmd == VFIO_DEVICE_GET_INFO) {
> @@ -1402,11 +1405,10 @@ static long vfio_pci_ioctl(void *device_data,
>         return -ENOTTY;
>  }
>
> -static ssize_t vfio_pci_rw(void *device_data, char __user *buf,
> +static ssize_t vfio_pci_rw(struct vfio_pci_device *vdev, char __user *buf,
>                            size_t count, loff_t *ppos, bool iswrite)
>  {
>         unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> -       struct vfio_pci_device *vdev = device_data;
>
>         if (index >= VFIO_PCI_NUM_REGIONS + vdev->num_regions)
>                 return -EINVAL;
> @@ -1434,22 +1436,28 @@ static ssize_t vfio_pci_rw(void *device_data, char __user *buf,
>         return -EINVAL;
>  }
>
> -static ssize_t vfio_pci_read(void *device_data, char __user *buf,
> +static ssize_t vfio_pci_read(struct vfio_device *core_vdev, char __user *buf,
>                              size_t count, loff_t *ppos)
>  {
> +       struct vfio_pci_device *vdev =
> +               container_of(core_vdev, struct vfio_pci_device, vdev);
> +
>         if (!count)
>                 return 0;
>
> -       return vfio_pci_rw(device_data, buf, count, ppos, false);
> +       return vfio_pci_rw(vdev, buf, count, ppos, false);
>  }
>
> -static ssize_t vfio_pci_write(void *device_data, const char __user *buf,
> +static ssize_t vfio_pci_write(struct vfio_device *core_vdev, const char __user *buf,
>                               size_t count, loff_t *ppos)
>  {
> +       struct vfio_pci_device *vdev =
> +               container_of(core_vdev, struct vfio_pci_device, vdev);
> +
>         if (!count)
>                 return 0;
>
> -       return vfio_pci_rw(device_data, (char __user *)buf, count, ppos, true);
> +       return vfio_pci_rw(vdev, (char __user *)buf, count, ppos, true);
>  }
>
>  /* Return 1 on zap and vma_lock acquired, 0 on contention (only with @try) */
> @@ -1646,9 +1654,10 @@ static const struct vm_operations_struct vfio_pci_mmap_ops = {
>         .fault = vfio_pci_mmap_fault,
>  };
>
> -static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
> +static int vfio_pci_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma)
>  {
> -       struct vfio_pci_device *vdev = device_data;
> +       struct vfio_pci_device *vdev =
> +               container_of(core_vdev, struct vfio_pci_device, vdev);
>         struct pci_dev *pdev = vdev->pdev;
>         unsigned int index;
>         u64 phys_len, req_len, pgoff, req_start;
> @@ -1714,9 +1723,10 @@ static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
>         return 0;
>  }
>
> -static void vfio_pci_request(void *device_data, unsigned int count)
> +static void vfio_pci_request(struct vfio_device *core_vdev, unsigned int count)
>  {
> -       struct vfio_pci_device *vdev = device_data;
> +       struct vfio_pci_device *vdev =
> +               container_of(core_vdev, struct vfio_pci_device, vdev);
>         struct pci_dev *pdev = vdev->pdev;
>
>         mutex_lock(&vdev->igate);
> @@ -1830,9 +1840,10 @@ static int vfio_pci_validate_vf_token(struct vfio_pci_device *vdev,
>
>  #define VF_TOKEN_ARG "vf_token="
>
> -static int vfio_pci_match(void *device_data, char *buf)
> +static int vfio_pci_match(struct vfio_device *core_vdev, char *buf)
>  {
> -       struct vfio_pci_device *vdev = device_data;
> +       struct vfio_pci_device *vdev =
> +               container_of(core_vdev, struct vfio_pci_device, vdev);
>         bool vf_token = false;
>         uuid_t uuid;
>         int ret;
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index 6eb749250ee41c..f5f6b537084a67 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -218,9 +218,10 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
>         return -EINVAL;
>  }
>
> -static void vfio_platform_release(void *device_data)
> +static void vfio_platform_release(struct vfio_device *core_vdev)
>  {
> -       struct vfio_platform_device *vdev = device_data;
> +       struct vfio_platform_device *vdev =
> +               container_of(core_vdev, struct vfio_platform_device, vdev);
>
>         mutex_lock(&driver_lock);
>
> @@ -244,9 +245,10 @@ static void vfio_platform_release(void *device_data)
>         module_put(vdev->parent_module);
>  }
>
> -static int vfio_platform_open(void *device_data)
> +static int vfio_platform_open(struct vfio_device *core_vdev)
>  {
> -       struct vfio_platform_device *vdev = device_data;
> +       struct vfio_platform_device *vdev =
> +               container_of(core_vdev, struct vfio_platform_device, vdev);
>         int ret;
>
>         if (!try_module_get(vdev->parent_module))
> @@ -293,10 +295,12 @@ static int vfio_platform_open(void *device_data)
>         return ret;
>  }
>
> -static long vfio_platform_ioctl(void *device_data,
> +static long vfio_platform_ioctl(struct vfio_device *core_vdev,
>                                 unsigned int cmd, unsigned long arg)
>  {
> -       struct vfio_platform_device *vdev = device_data;
> +       struct vfio_platform_device *vdev =
> +               container_of(core_vdev, struct vfio_platform_device, vdev);
> +
>         unsigned long minsz;
>
>         if (cmd == VFIO_DEVICE_GET_INFO) {
> @@ -455,10 +459,11 @@ static ssize_t vfio_platform_read_mmio(struct vfio_platform_region *reg,
>         return -EFAULT;
>  }
>
> -static ssize_t vfio_platform_read(void *device_data, char __user *buf,
> -                                 size_t count, loff_t *ppos)
> +static ssize_t vfio_platform_read(struct vfio_device *core_vdev,
> +                                 char __user *buf, size_t count, loff_t *ppos)
>  {
> -       struct vfio_platform_device *vdev = device_data;
> +       struct vfio_platform_device *vdev =
> +               container_of(core_vdev, struct vfio_platform_device, vdev);
>         unsigned int index = VFIO_PLATFORM_OFFSET_TO_INDEX(*ppos);
>         loff_t off = *ppos & VFIO_PLATFORM_OFFSET_MASK;
>
> @@ -531,10 +536,11 @@ static ssize_t vfio_platform_write_mmio(struct vfio_platform_region *reg,
>         return -EFAULT;
>  }
>
> -static ssize_t vfio_platform_write(void *device_data, const char __user *buf,
> +static ssize_t vfio_platform_write(struct vfio_device *core_vdev, const char __user *buf,
>                                    size_t count, loff_t *ppos)
>  {
> -       struct vfio_platform_device *vdev = device_data;
> +       struct vfio_platform_device *vdev =
> +               container_of(core_vdev, struct vfio_platform_device, vdev);
>         unsigned int index = VFIO_PLATFORM_OFFSET_TO_INDEX(*ppos);
>         loff_t off = *ppos & VFIO_PLATFORM_OFFSET_MASK;
>
> @@ -573,9 +579,10 @@ static int vfio_platform_mmap_mmio(struct vfio_platform_region region,
>                                req_len, vma->vm_page_prot);
>  }
>
> -static int vfio_platform_mmap(void *device_data, struct vm_area_struct *vma)
> +static int vfio_platform_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma)
>  {
> -       struct vfio_platform_device *vdev = device_data;
> +       struct vfio_platform_device *vdev =
> +               container_of(core_vdev, struct vfio_platform_device, vdev);
>         unsigned int index;
>
>         index = vma->vm_pgoff >> (VFIO_PLATFORM_OFFSET_SHIFT - PAGE_SHIFT);
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 2d6d7cc1d1ebf9..01de47d1810b6b 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -832,7 +832,7 @@ static struct vfio_device *vfio_device_get_from_name(struct vfio_group *group,
>                 int ret;
>
>                 if (it->ops->match) {
> -                       ret = it->ops->match(it->device_data, buf);
> +                       ret = it->ops->match(it, buf);
>                         if (ret < 0) {
>                                 device = ERR_PTR(ret);
>                                 break;
> @@ -893,7 +893,7 @@ void vfio_unregister_group_dev(struct vfio_device *device)
>         rc = try_wait_for_completion(&device->comp);
>         while (rc <= 0) {
>                 if (device->ops->request)
> -                       device->ops->request(device->device_data, i++);
> +                       device->ops->request(device, i++);
>
>                 if (interrupted) {
>                         rc = wait_for_completion_timeout(&device->comp,
> @@ -1379,7 +1379,7 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
>         if (IS_ERR(device))
>                 return PTR_ERR(device);
>
> -       ret = device->ops->open(device->device_data);
> +       ret = device->ops->open(device);
>         if (ret) {
>                 vfio_device_put(device);
>                 return ret;
> @@ -1391,7 +1391,7 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
>          */
>         ret = get_unused_fd_flags(O_CLOEXEC);
>         if (ret < 0) {
> -               device->ops->release(device->device_data);
> +               device->ops->release(device);
>                 vfio_device_put(device);
>                 return ret;
>         }
> @@ -1401,7 +1401,7 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
>         if (IS_ERR(filep)) {
>                 put_unused_fd(ret);
>                 ret = PTR_ERR(filep);
> -               device->ops->release(device->device_data);
> +               device->ops->release(device);
>                 vfio_device_put(device);
>                 return ret;
>         }
> @@ -1558,7 +1558,7 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
>  {
>         struct vfio_device *device = filep->private_data;
>
> -       device->ops->release(device->device_data);
> +       device->ops->release(device);
>
>         vfio_group_try_dissolve_container(device->group);
>
> @@ -1575,7 +1575,7 @@ static long vfio_device_fops_unl_ioctl(struct file *filep,
>         if (unlikely(!device->ops->ioctl))
>                 return -EINVAL;
>
> -       return device->ops->ioctl(device->device_data, cmd, arg);
> +       return device->ops->ioctl(device, cmd, arg);
>  }
>
>  static ssize_t vfio_device_fops_read(struct file *filep, char __user *buf,
> @@ -1586,7 +1586,7 @@ static ssize_t vfio_device_fops_read(struct file *filep, char __user *buf,
>         if (unlikely(!device->ops->read))
>                 return -EINVAL;
>
> -       return device->ops->read(device->device_data, buf, count, ppos);
> +       return device->ops->read(device, buf, count, ppos);
>  }
>
>  static ssize_t vfio_device_fops_write(struct file *filep,
> @@ -1598,7 +1598,7 @@ static ssize_t vfio_device_fops_write(struct file *filep,
>         if (unlikely(!device->ops->write))
>                 return -EINVAL;
>
> -       return device->ops->write(device->device_data, buf, count, ppos);
> +       return device->ops->write(device, buf, count, ppos);
>  }
>
>  static int vfio_device_fops_mmap(struct file *filep, struct vm_area_struct *vma)
> @@ -1608,7 +1608,7 @@ static int vfio_device_fops_mmap(struct file *filep, struct vm_area_struct *vma)
>         if (unlikely(!device->ops->mmap))
>                 return -EINVAL;
>
> -       return device->ops->mmap(device->device_data, vma);
> +       return device->ops->mmap(device, vma);
>  }
>
>  static const struct file_operations vfio_device_fops = {
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 4995faf51efeae..784c34c0a28763 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -44,17 +44,17 @@ struct vfio_device {
>   */
>  struct vfio_device_ops {
>         char    *name;
> -       int     (*open)(void *device_data);
> -       void    (*release)(void *device_data);
> -       ssize_t (*read)(void *device_data, char __user *buf,
> +       int     (*open)(struct vfio_device *vdev);
> +       void    (*release)(struct vfio_device *vdev);
> +       ssize_t (*read)(struct vfio_device *vdev, char __user *buf,
>                         size_t count, loff_t *ppos);
> -       ssize_t (*write)(void *device_data, const char __user *buf,
> +       ssize_t (*write)(struct vfio_device *vdev, const char __user *buf,
>                          size_t count, loff_t *size);
> -       long    (*ioctl)(void *device_data, unsigned int cmd,
> +       long    (*ioctl)(struct vfio_device *vdev, unsigned int cmd,
>                          unsigned long arg);
> -       int     (*mmap)(void *device_data, struct vm_area_struct *vma);
> -       void    (*request)(void *device_data, unsigned int count);
> -       int     (*match)(void *device_data, char *buf);
> +       int     (*mmap)(struct vfio_device *vdev, struct vm_area_struct *vma);
> +       void    (*request)(struct vfio_device *vdev, unsigned int count);
> +       int     (*match)(struct vfio_device *vdev, char *buf);
>  };
>
>  extern struct iommu_group *vfio_iommu_group_get(struct device *dev);
> --
> 2.30.1
>

Re: [PATCH 08/10] vfio: Make vfio_device_ops pass a 'struct vfio_device *' instead of 'void *'

[Leon Romanovsky]

On Tue, Mar 09, 2021 at 09:52:30PM -0800, Dan Williams wrote:
> On Tue, Mar 9, 2021 at 1:39 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > This is the standard kernel pattern, the ops associated with a struct get
> > the struct pointer in for typesafety. The expected design is to use
> > container_of to cleanly go from the subsystem level type to the driver
> > level type without having any type erasure in a void *.
>
> This patch alone is worth the price of admission.
>
> Seems like it would be worth adding
> to_vfio_{pci,platform,fsl_mc}_device() helpers in this patch as well.
>
> I've sometimes added runtime type safety to to_* helpers for early
> warning of mistakes that happen when refactoring...
>
> static inline struct vfio_pci_device *
> to_vfio_pci_device(struct vfio_device *core_dev)
> {
>         if (dev_WARN_ONCE(core_dev->dev, core_dev->ops != &vfio_pci_ops,
>                           "not a vfio_pci_device!\n"))
>                 return NULL;

I personally didn't see any bug related to the problem presented by you.
For this discussion let's assume that the extra check can be sometimes useful,
however the "return NULL" construction is extremely harmful. This requires callers
to check if it is not NULL, which is wrong for container_of(..) callers. Such checks
are sort of wrong assumptions later.

Just as an example of this bad behaviour, I have series in progress that fixes it:
https://elixir.bootlin.com/linux/v5.12-rc2/source/drivers/infiniband/hw/bnxt_re/ib_verbs.c#L1100
	srq = container_of(init_attr->srq, struct bnxt_re_srq, ib_srq);
	if (!srq) {
		ibdev_err(&rdev->ibdev, "SRQ not found");
		return -EINVAL;
	}

>         return container_of(core_vdev, struct vfio_pci_device, vdev);
> }
>
> ...but typed ops is already a significant idiomatic improvement.

Everything here is an improvement, in-kernel vfio implementation is too far
from canonical kernel code and I'm glad that Jason proposes way to improve
this situation.

Thanks

>
>
> >
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >  Documentation/driver-api/vfio.rst            | 18 ++++----
> >  drivers/vfio/fsl-mc/vfio_fsl_mc.c            | 36 +++++++++------
> >  drivers/vfio/mdev/vfio_mdev.c                | 33 +++++++-------
> >  drivers/vfio/pci/vfio_pci.c                  | 47 ++++++++++++--------
> >  drivers/vfio/platform/vfio_platform_common.c | 33 ++++++++------
> >  drivers/vfio/vfio.c                          | 20 ++++-----
> >  include/linux/vfio.h                         | 16 +++----
> >  7 files changed, 117 insertions(+), 86 deletions(-)
> >
> > diff --git a/Documentation/driver-api/vfio.rst b/Documentation/driver-api/vfio.rst
> > index d3a02300913a7f..3337f337293a32 100644
> > --- a/Documentation/driver-api/vfio.rst
> > +++ b/Documentation/driver-api/vfio.rst
> > @@ -269,20 +269,22 @@ ready before calling it. The driver provides an ops structure for callbacks
> >  similar to a file operations structure::
> >
> >         struct vfio_device_ops {
> > -               int     (*open)(void *device_data);
> > -               void    (*release)(void *device_data);
> > -               ssize_t (*read)(void *device_data, char __user *buf,
> > +               int     (*open)(struct vfio_device *vdev);
> > +               void    (*release)(struct vfio_device *vdev);
> > +               ssize_t (*read)(struct vfio_device *vdev, char __user *buf,
> >                                 size_t count, loff_t *ppos);
> > -               ssize_t (*write)(void *device_data, const char __user *buf,
> > +               ssize_t (*write)(struct vfio_device *vdev,
> > +                                const char __user *buf,
> >                                  size_t size, loff_t *ppos);
> > -               long    (*ioctl)(void *device_data, unsigned int cmd,
> > +               long    (*ioctl)(struct vfio_device *vdev, unsigned int cmd,
> >                                  unsigned long arg);
> > -               int     (*mmap)(void *device_data, struct vm_area_struct *vma);
> > +               int     (*mmap)(struct vfio_device *vdev,
> > +                               struct vm_area_struct *vma);
> >         };
> >
> > -Each function is passed the device_data that was originally registered
> > +Each function is passed the vdev that was originally registered
> >  in the vfio_register_group_dev() call above.  This allows the bus driver
> > -an easy place to store its opaque, private data.  The open/release
> > +to obtain its private data using container_of().  The open/release
> >  callbacks are issued when a new file descriptor is created for a
> >  device (via VFIO_GROUP_GET_DEVICE_FD).  The ioctl interface provides
> >  a direct pass through for VFIO_DEVICE_* ioctls.  The read/write/mmap
> > diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> > index ddee6ed20c4523..74a5de1b791934 100644
> > --- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> > +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> > @@ -135,9 +135,10 @@ static void vfio_fsl_mc_regions_cleanup(struct vfio_fsl_mc_device *vdev)
> >         kfree(vdev->regions);
> >  }
> >
> > -static int vfio_fsl_mc_open(void *device_data)
> > +static int vfio_fsl_mc_open(struct vfio_device *core_vdev)
> >  {
> > -       struct vfio_fsl_mc_device *vdev = device_data;
> > +       struct vfio_fsl_mc_device *vdev =
> > +               container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
> >         int ret;
> >
> >         if (!try_module_get(THIS_MODULE))
> > @@ -161,9 +162,10 @@ static int vfio_fsl_mc_open(void *device_data)
> >         return ret;
> >  }
> >
> > -static void vfio_fsl_mc_release(void *device_data)
> > +static void vfio_fsl_mc_release(struct vfio_device *core_vdev)
> >  {
> > -       struct vfio_fsl_mc_device *vdev = device_data;
> > +       struct vfio_fsl_mc_device *vdev =
> > +               container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
> >         int ret;
> >
> >         mutex_lock(&vdev->reflck->lock);
> > @@ -197,11 +199,12 @@ static void vfio_fsl_mc_release(void *device_data)
> >         module_put(THIS_MODULE);
> >  }
> >
> > -static long vfio_fsl_mc_ioctl(void *device_data, unsigned int cmd,
> > -                             unsigned long arg)
> > +static long vfio_fsl_mc_ioctl(struct vfio_device *core_vdev,
> > +                             unsigned int cmd, unsigned long arg)
> >  {
> >         unsigned long minsz;
> > -       struct vfio_fsl_mc_device *vdev = device_data;
> > +       struct vfio_fsl_mc_device *vdev =
> > +               container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
> >         struct fsl_mc_device *mc_dev = vdev->mc_dev;
> >
> >         switch (cmd) {
> > @@ -327,10 +330,11 @@ static long vfio_fsl_mc_ioctl(void *device_data, unsigned int cmd,
> >         }
> >  }
> >
> > -static ssize_t vfio_fsl_mc_read(void *device_data, char __user *buf,
> > +static ssize_t vfio_fsl_mc_read(struct vfio_device *core_vdev, char __user *buf,
> >                                 size_t count, loff_t *ppos)
> >  {
> > -       struct vfio_fsl_mc_device *vdev = device_data;
> > +       struct vfio_fsl_mc_device *vdev =
> > +               container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
> >         unsigned int index = VFIO_FSL_MC_OFFSET_TO_INDEX(*ppos);
> >         loff_t off = *ppos & VFIO_FSL_MC_OFFSET_MASK;
> >         struct fsl_mc_device *mc_dev = vdev->mc_dev;
> > @@ -404,10 +408,12 @@ static int vfio_fsl_mc_send_command(void __iomem *ioaddr, uint64_t *cmd_data)
> >         return 0;
> >  }
> >
> > -static ssize_t vfio_fsl_mc_write(void *device_data, const char __user *buf,
> > -                                size_t count, loff_t *ppos)
> > +static ssize_t vfio_fsl_mc_write(struct vfio_device *core_vdev,
> > +                                const char __user *buf, size_t count,
> > +                                loff_t *ppos)
> >  {
> > -       struct vfio_fsl_mc_device *vdev = device_data;
> > +       struct vfio_fsl_mc_device *vdev =
> > +               container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
> >         unsigned int index = VFIO_FSL_MC_OFFSET_TO_INDEX(*ppos);
> >         loff_t off = *ppos & VFIO_FSL_MC_OFFSET_MASK;
> >         struct fsl_mc_device *mc_dev = vdev->mc_dev;
> > @@ -468,9 +474,11 @@ static int vfio_fsl_mc_mmap_mmio(struct vfio_fsl_mc_region region,
> >                                size, vma->vm_page_prot);
> >  }
> >
> > -static int vfio_fsl_mc_mmap(void *device_data, struct vm_area_struct *vma)
> > +static int vfio_fsl_mc_mmap(struct vfio_device *core_vdev,
> > +                           struct vm_area_struct *vma)
> >  {
> > -       struct vfio_fsl_mc_device *vdev = device_data;
> > +       struct vfio_fsl_mc_device *vdev =
> > +               container_of(core_vdev, struct vfio_fsl_mc_device, vdev);
> >         struct fsl_mc_device *mc_dev = vdev->mc_dev;
> >         unsigned int index;
> >
> > diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
> > index 4469aaf31b56cb..e7309caa99c71b 100644
> > --- a/drivers/vfio/mdev/vfio_mdev.c
> > +++ b/drivers/vfio/mdev/vfio_mdev.c
> > @@ -25,10 +25,11 @@ struct mdev_vfio_device {
> >         struct vfio_device vdev;
> >  };
> >
> > -static int vfio_mdev_open(void *device_data)
> > +static int vfio_mdev_open(struct vfio_device *core_vdev)
> >  {
> > -       struct mdev_device *mdev = device_data;
> > +       struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
> >         struct mdev_parent *parent = mdev->parent;
> > +
> >         int ret;
> >
> >         if (unlikely(!parent->ops->open))
> > @@ -44,9 +45,9 @@ static int vfio_mdev_open(void *device_data)
> >         return ret;
> >  }
> >
> > -static void vfio_mdev_release(void *device_data)
> > +static void vfio_mdev_release(struct vfio_device *core_vdev)
> >  {
> > -       struct mdev_device *mdev = device_data;
> > +       struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
> >         struct mdev_parent *parent = mdev->parent;
> >
> >         if (likely(parent->ops->release))
> > @@ -55,10 +56,10 @@ static void vfio_mdev_release(void *device_data)
> >         module_put(THIS_MODULE);
> >  }
> >
> > -static long vfio_mdev_unlocked_ioctl(void *device_data,
> > +static long vfio_mdev_unlocked_ioctl(struct vfio_device *core_vdev,
> >                                      unsigned int cmd, unsigned long arg)
> >  {
> > -       struct mdev_device *mdev = device_data;
> > +       struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
> >         struct mdev_parent *parent = mdev->parent;
> >
> >         if (unlikely(!parent->ops->ioctl))
> > @@ -67,10 +68,10 @@ static long vfio_mdev_unlocked_ioctl(void *device_data,
> >         return parent->ops->ioctl(mdev, cmd, arg);
> >  }
> >
> > -static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
> > +static ssize_t vfio_mdev_read(struct vfio_device *core_vdev, char __user *buf,
> >                               size_t count, loff_t *ppos)
> >  {
> > -       struct mdev_device *mdev = device_data;
> > +       struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
> >         struct mdev_parent *parent = mdev->parent;
> >
> >         if (unlikely(!parent->ops->read))
> > @@ -79,10 +80,11 @@ static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
> >         return parent->ops->read(mdev, buf, count, ppos);
> >  }
> >
> > -static ssize_t vfio_mdev_write(void *device_data, const char __user *buf,
> > -                              size_t count, loff_t *ppos)
> > +static ssize_t vfio_mdev_write(struct vfio_device *core_vdev,
> > +                              const char __user *buf, size_t count,
> > +                              loff_t *ppos)
> >  {
> > -       struct mdev_device *mdev = device_data;
> > +       struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
> >         struct mdev_parent *parent = mdev->parent;
> >
> >         if (unlikely(!parent->ops->write))
> > @@ -91,9 +93,10 @@ static ssize_t vfio_mdev_write(void *device_data, const char __user *buf,
> >         return parent->ops->write(mdev, buf, count, ppos);
> >  }
> >
> > -static int vfio_mdev_mmap(void *device_data, struct vm_area_struct *vma)
> > +static int vfio_mdev_mmap(struct vfio_device *core_vdev,
> > +                         struct vm_area_struct *vma)
> >  {
> > -       struct mdev_device *mdev = device_data;
> > +       struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
> >         struct mdev_parent *parent = mdev->parent;
> >
> >         if (unlikely(!parent->ops->mmap))
> > @@ -102,9 +105,9 @@ static int vfio_mdev_mmap(void *device_data, struct vm_area_struct *vma)
> >         return parent->ops->mmap(mdev, vma);
> >  }
> >
> > -static void vfio_mdev_request(void *device_data, unsigned int count)
> > +static void vfio_mdev_request(struct vfio_device *core_vdev, unsigned int count)
> >  {
> > -       struct mdev_device *mdev = device_data;
> > +       struct mdev_device *mdev = to_mdev_device(core_vdev->dev);
> >         struct mdev_parent *parent = mdev->parent;
> >
> >         if (parent->ops->request)
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index fae573c6f86bdf..af5696a96a76e0 100644
> > --- a/drivers/vfio/pci/vfio_pci.c
> > +++ b/drivers/vfio/pci/vfio_pci.c
> > @@ -553,9 +553,10 @@ static void vfio_pci_vf_token_user_add(struct vfio_pci_device *vdev, int val)
> >         vfio_device_put(pf_dev);
> >  }
> >
> > -static void vfio_pci_release(void *device_data)
> > +static void vfio_pci_release(struct vfio_device *core_vdev)
> >  {
> > -       struct vfio_pci_device *vdev = device_data;
> > +       struct vfio_pci_device *vdev =
> > +               container_of(core_vdev, struct vfio_pci_device, vdev);
> >
> >         mutex_lock(&vdev->reflck->lock);
> >
> > @@ -581,9 +582,10 @@ static void vfio_pci_release(void *device_data)
> >         module_put(THIS_MODULE);
> >  }
> >
> > -static int vfio_pci_open(void *device_data)
> > +static int vfio_pci_open(struct vfio_device *core_vdev)
> >  {
> > -       struct vfio_pci_device *vdev = device_data;
> > +       struct vfio_pci_device *vdev =
> > +               container_of(core_vdev, struct vfio_pci_device, vdev);
> >         int ret = 0;
> >
> >         if (!try_module_get(THIS_MODULE))
> > @@ -797,10 +799,11 @@ struct vfio_devices {
> >         int max_index;
> >  };
> >
> > -static long vfio_pci_ioctl(void *device_data,
> > +static long vfio_pci_ioctl(struct vfio_device *core_vdev,
> >                            unsigned int cmd, unsigned long arg)
> >  {
> > -       struct vfio_pci_device *vdev = device_data;
> > +       struct vfio_pci_device *vdev =
> > +               container_of(core_vdev, struct vfio_pci_device, vdev);
> >         unsigned long minsz;
> >
> >         if (cmd == VFIO_DEVICE_GET_INFO) {
> > @@ -1402,11 +1405,10 @@ static long vfio_pci_ioctl(void *device_data,
> >         return -ENOTTY;
> >  }
> >
> > -static ssize_t vfio_pci_rw(void *device_data, char __user *buf,
> > +static ssize_t vfio_pci_rw(struct vfio_pci_device *vdev, char __user *buf,
> >                            size_t count, loff_t *ppos, bool iswrite)
> >  {
> >         unsigned int index = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
> > -       struct vfio_pci_device *vdev = device_data;
> >
> >         if (index >= VFIO_PCI_NUM_REGIONS + vdev->num_regions)
> >                 return -EINVAL;
> > @@ -1434,22 +1436,28 @@ static ssize_t vfio_pci_rw(void *device_data, char __user *buf,
> >         return -EINVAL;
> >  }
> >
> > -static ssize_t vfio_pci_read(void *device_data, char __user *buf,
> > +static ssize_t vfio_pci_read(struct vfio_device *core_vdev, char __user *buf,
> >                              size_t count, loff_t *ppos)
> >  {
> > +       struct vfio_pci_device *vdev =
> > +               container_of(core_vdev, struct vfio_pci_device, vdev);
> > +
> >         if (!count)
> >                 return 0;
> >
> > -       return vfio_pci_rw(device_data, buf, count, ppos, false);
> > +       return vfio_pci_rw(vdev, buf, count, ppos, false);
> >  }
> >
> > -static ssize_t vfio_pci_write(void *device_data, const char __user *buf,
> > +static ssize_t vfio_pci_write(struct vfio_device *core_vdev, const char __user *buf,
> >                               size_t count, loff_t *ppos)
> >  {
> > +       struct vfio_pci_device *vdev =
> > +               container_of(core_vdev, struct vfio_pci_device, vdev);
> > +
> >         if (!count)
> >                 return 0;
> >
> > -       return vfio_pci_rw(device_data, (char __user *)buf, count, ppos, true);
> > +       return vfio_pci_rw(vdev, (char __user *)buf, count, ppos, true);
> >  }
> >
> >  /* Return 1 on zap and vma_lock acquired, 0 on contention (only with @try) */
> > @@ -1646,9 +1654,10 @@ static const struct vm_operations_struct vfio_pci_mmap_ops = {
> >         .fault = vfio_pci_mmap_fault,
> >  };
> >
> > -static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
> > +static int vfio_pci_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma)
> >  {
> > -       struct vfio_pci_device *vdev = device_data;
> > +       struct vfio_pci_device *vdev =
> > +               container_of(core_vdev, struct vfio_pci_device, vdev);
> >         struct pci_dev *pdev = vdev->pdev;
> >         unsigned int index;
> >         u64 phys_len, req_len, pgoff, req_start;
> > @@ -1714,9 +1723,10 @@ static int vfio_pci_mmap(void *device_data, struct vm_area_struct *vma)
> >         return 0;
> >  }
> >
> > -static void vfio_pci_request(void *device_data, unsigned int count)
> > +static void vfio_pci_request(struct vfio_device *core_vdev, unsigned int count)
> >  {
> > -       struct vfio_pci_device *vdev = device_data;
> > +       struct vfio_pci_device *vdev =
> > +               container_of(core_vdev, struct vfio_pci_device, vdev);
> >         struct pci_dev *pdev = vdev->pdev;
> >
> >         mutex_lock(&vdev->igate);
> > @@ -1830,9 +1840,10 @@ static int vfio_pci_validate_vf_token(struct vfio_pci_device *vdev,
> >
> >  #define VF_TOKEN_ARG "vf_token="
> >
> > -static int vfio_pci_match(void *device_data, char *buf)
> > +static int vfio_pci_match(struct vfio_device *core_vdev, char *buf)
> >  {
> > -       struct vfio_pci_device *vdev = device_data;
> > +       struct vfio_pci_device *vdev =
> > +               container_of(core_vdev, struct vfio_pci_device, vdev);
> >         bool vf_token = false;
> >         uuid_t uuid;
> >         int ret;
> > diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> > index 6eb749250ee41c..f5f6b537084a67 100644
> > --- a/drivers/vfio/platform/vfio_platform_common.c
> > +++ b/drivers/vfio/platform/vfio_platform_common.c
> > @@ -218,9 +218,10 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
> >         return -EINVAL;
> >  }
> >
> > -static void vfio_platform_release(void *device_data)
> > +static void vfio_platform_release(struct vfio_device *core_vdev)
> >  {
> > -       struct vfio_platform_device *vdev = device_data;
> > +       struct vfio_platform_device *vdev =
> > +               container_of(core_vdev, struct vfio_platform_device, vdev);
> >
> >         mutex_lock(&driver_lock);
> >
> > @@ -244,9 +245,10 @@ static void vfio_platform_release(void *device_data)
> >         module_put(vdev->parent_module);
> >  }
> >
> > -static int vfio_platform_open(void *device_data)
> > +static int vfio_platform_open(struct vfio_device *core_vdev)
> >  {
> > -       struct vfio_platform_device *vdev = device_data;
> > +       struct vfio_platform_device *vdev =
> > +               container_of(core_vdev, struct vfio_platform_device, vdev);
> >         int ret;
> >
> >         if (!try_module_get(vdev->parent_module))
> > @@ -293,10 +295,12 @@ static int vfio_platform_open(void *device_data)
> >         return ret;
> >  }
> >
> > -static long vfio_platform_ioctl(void *device_data,
> > +static long vfio_platform_ioctl(struct vfio_device *core_vdev,
> >                                 unsigned int cmd, unsigned long arg)
> >  {
> > -       struct vfio_platform_device *vdev = device_data;
> > +       struct vfio_platform_device *vdev =
> > +               container_of(core_vdev, struct vfio_platform_device, vdev);
> > +
> >         unsigned long minsz;
> >
> >         if (cmd == VFIO_DEVICE_GET_INFO) {
> > @@ -455,10 +459,11 @@ static ssize_t vfio_platform_read_mmio(struct vfio_platform_region *reg,
> >         return -EFAULT;
> >  }
> >
> > -static ssize_t vfio_platform_read(void *device_data, char __user *buf,
> > -                                 size_t count, loff_t *ppos)
> > +static ssize_t vfio_platform_read(struct vfio_device *core_vdev,
> > +                                 char __user *buf, size_t count, loff_t *ppos)
> >  {
> > -       struct vfio_platform_device *vdev = device_data;
> > +       struct vfio_platform_device *vdev =
> > +               container_of(core_vdev, struct vfio_platform_device, vdev);
> >         unsigned int index = VFIO_PLATFORM_OFFSET_TO_INDEX(*ppos);
> >         loff_t off = *ppos & VFIO_PLATFORM_OFFSET_MASK;
> >
> > @@ -531,10 +536,11 @@ static ssize_t vfio_platform_write_mmio(struct vfio_platform_region *reg,
> >         return -EFAULT;
> >  }
> >
> > -static ssize_t vfio_platform_write(void *device_data, const char __user *buf,
> > +static ssize_t vfio_platform_write(struct vfio_device *core_vdev, const char __user *buf,
> >                                    size_t count, loff_t *ppos)
> >  {
> > -       struct vfio_platform_device *vdev = device_data;
> > +       struct vfio_platform_device *vdev =
> > +               container_of(core_vdev, struct vfio_platform_device, vdev);
> >         unsigned int index = VFIO_PLATFORM_OFFSET_TO_INDEX(*ppos);
> >         loff_t off = *ppos & VFIO_PLATFORM_OFFSET_MASK;
> >
> > @@ -573,9 +579,10 @@ static int vfio_platform_mmap_mmio(struct vfio_platform_region region,
> >                                req_len, vma->vm_page_prot);
> >  }
> >
> > -static int vfio_platform_mmap(void *device_data, struct vm_area_struct *vma)
> > +static int vfio_platform_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma)
> >  {
> > -       struct vfio_platform_device *vdev = device_data;
> > +       struct vfio_platform_device *vdev =
> > +               container_of(core_vdev, struct vfio_platform_device, vdev);
> >         unsigned int index;
> >
> >         index = vma->vm_pgoff >> (VFIO_PLATFORM_OFFSET_SHIFT - PAGE_SHIFT);
> > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > index 2d6d7cc1d1ebf9..01de47d1810b6b 100644
> > --- a/drivers/vfio/vfio.c
> > +++ b/drivers/vfio/vfio.c
> > @@ -832,7 +832,7 @@ static struct vfio_device *vfio_device_get_from_name(struct vfio_group *group,
> >                 int ret;
> >
> >                 if (it->ops->match) {
> > -                       ret = it->ops->match(it->device_data, buf);
> > +                       ret = it->ops->match(it, buf);
> >                         if (ret < 0) {
> >                                 device = ERR_PTR(ret);
> >                                 break;
> > @@ -893,7 +893,7 @@ void vfio_unregister_group_dev(struct vfio_device *device)
> >         rc = try_wait_for_completion(&device->comp);
> >         while (rc <= 0) {
> >                 if (device->ops->request)
> > -                       device->ops->request(device->device_data, i++);
> > +                       device->ops->request(device, i++);
> >
> >                 if (interrupted) {
> >                         rc = wait_for_completion_timeout(&device->comp,
> > @@ -1379,7 +1379,7 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
> >         if (IS_ERR(device))
> >                 return PTR_ERR(device);
> >
> > -       ret = device->ops->open(device->device_data);
> > +       ret = device->ops->open(device);
> >         if (ret) {
> >                 vfio_device_put(device);
> >                 return ret;
> > @@ -1391,7 +1391,7 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
> >          */
> >         ret = get_unused_fd_flags(O_CLOEXEC);
> >         if (ret < 0) {
> > -               device->ops->release(device->device_data);
> > +               device->ops->release(device);
> >                 vfio_device_put(device);
> >                 return ret;
> >         }
> > @@ -1401,7 +1401,7 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
> >         if (IS_ERR(filep)) {
> >                 put_unused_fd(ret);
> >                 ret = PTR_ERR(filep);
> > -               device->ops->release(device->device_data);
> > +               device->ops->release(device);
> >                 vfio_device_put(device);
> >                 return ret;
> >         }
> > @@ -1558,7 +1558,7 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
> >  {
> >         struct vfio_device *device = filep->private_data;
> >
> > -       device->ops->release(device->device_data);
> > +       device->ops->release(device);
> >
> >         vfio_group_try_dissolve_container(device->group);
> >
> > @@ -1575,7 +1575,7 @@ static long vfio_device_fops_unl_ioctl(struct file *filep,
> >         if (unlikely(!device->ops->ioctl))
> >                 return -EINVAL;
> >
> > -       return device->ops->ioctl(device->device_data, cmd, arg);
> > +       return device->ops->ioctl(device, cmd, arg);
> >  }
> >
> >  static ssize_t vfio_device_fops_read(struct file *filep, char __user *buf,
> > @@ -1586,7 +1586,7 @@ static ssize_t vfio_device_fops_read(struct file *filep, char __user *buf,
> >         if (unlikely(!device->ops->read))
> >                 return -EINVAL;
> >
> > -       return device->ops->read(device->device_data, buf, count, ppos);
> > +       return device->ops->read(device, buf, count, ppos);
> >  }
> >
> >  static ssize_t vfio_device_fops_write(struct file *filep,
> > @@ -1598,7 +1598,7 @@ static ssize_t vfio_device_fops_write(struct file *filep,
> >         if (unlikely(!device->ops->write))
> >                 return -EINVAL;
> >
> > -       return device->ops->write(device->device_data, buf, count, ppos);
> > +       return device->ops->write(device, buf, count, ppos);
> >  }
> >
> >  static int vfio_device_fops_mmap(struct file *filep, struct vm_area_struct *vma)
> > @@ -1608,7 +1608,7 @@ static int vfio_device_fops_mmap(struct file *filep, struct vm_area_struct *vma)
> >         if (unlikely(!device->ops->mmap))
> >                 return -EINVAL;
> >
> > -       return device->ops->mmap(device->device_data, vma);
> > +       return device->ops->mmap(device, vma);
> >  }
> >
> >  static const struct file_operations vfio_device_fops = {
> > diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> > index 4995faf51efeae..784c34c0a28763 100644
> > --- a/include/linux/vfio.h
> > +++ b/include/linux/vfio.h
> > @@ -44,17 +44,17 @@ struct vfio_device {
> >   */
> >  struct vfio_device_ops {
> >         char    *name;
> > -       int     (*open)(void *device_data);
> > -       void    (*release)(void *device_data);
> > -       ssize_t (*read)(void *device_data, char __user *buf,
> > +       int     (*open)(struct vfio_device *vdev);
> > +       void    (*release)(struct vfio_device *vdev);
> > +       ssize_t (*read)(struct vfio_device *vdev, char __user *buf,
> >                         size_t count, loff_t *ppos);
> > -       ssize_t (*write)(void *device_data, const char __user *buf,
> > +       ssize_t (*write)(struct vfio_device *vdev, const char __user *buf,
> >                          size_t count, loff_t *size);
> > -       long    (*ioctl)(void *device_data, unsigned int cmd,
> > +       long    (*ioctl)(struct vfio_device *vdev, unsigned int cmd,
> >                          unsigned long arg);
> > -       int     (*mmap)(void *device_data, struct vm_area_struct *vma);
> > -       void    (*request)(void *device_data, unsigned int count);
> > -       int     (*match)(void *device_data, char *buf);
> > +       int     (*mmap)(struct vfio_device *vdev, struct vm_area_struct *vma);
> > +       void    (*request)(struct vfio_device *vdev, unsigned int count);
> > +       int     (*match)(struct vfio_device *vdev, char *buf);
> >  };
> >
> >  extern struct iommu_group *vfio_iommu_group_get(struct device *dev);
> > --
> > 2.30.1
> >

Re: [PATCH 08/10] vfio: Make vfio_device_ops pass a 'struct vfio_device *' instead of 'void *'

[Jason Gunthorpe]

On Tue, Mar 09, 2021 at 09:52:30PM -0800, Dan Williams wrote:
> On Tue, Mar 9, 2021 at 1:39 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > This is the standard kernel pattern, the ops associated with a struct get
> > the struct pointer in for typesafety. The expected design is to use
> > container_of to cleanly go from the subsystem level type to the driver
> > level type without having any type erasure in a void *.
> 
> This patch alone is worth the price of admission.

Right, this is how I got here as I was going to do the mdev stuff and
we end up doing transformations like this:

-static long mtty_ioctl(struct mdev_device *mdev, unsigned int cmd,
+static long mtty_ioctl(struct vfio_device *vdev, unsigned int cmd,
 			unsigned long arg)

And if the 'struct vfio_device *vdev' was left a 'void *' then the
compiler doesn't get to help any more :(

> Seems like it would be worth adding
> to_vfio_{pci,platform,fsl_mc}_device() helpers in this patch as well.

I have mixed feelings on these one-liners. If people feel they are
worthwhile I'll add them

> I've sometimes added runtime type safety to to_* helpers for early
> warning of mistakes that happen when refactoring...
> 
> static inline struct vfio_pci_device *
> to_vfio_pci_device(struct vfio_device *core_dev)
> {
>         if (dev_WARN_ONCE(core_dev->dev, core_dev->ops != &vfio_pci_ops,
>                           "not a vfio_pci_device!\n"))
>                 return NULL;
>         return container_of(core_vdev, struct vfio_pci_device, vdev);

In this case I don't think we need to worry as everything is tidy in a
single module such that the compilation units can't see other
container_of options anyhow and vfio core isn't going to accidently
call an ops with the wrong type.

Jason

Re: [PATCH 08/10] vfio: Make vfio_device_ops pass a 'struct vfio_device *' instead of 'void *'

[Dan Williams]

On Wed, Mar 10, 2021 at 4:58 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Tue, Mar 09, 2021 at 09:52:30PM -0800, Dan Williams wrote:
> > On Tue, Mar 9, 2021 at 1:39 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > >
> > > This is the standard kernel pattern, the ops associated with a struct get
> > > the struct pointer in for typesafety. The expected design is to use
> > > container_of to cleanly go from the subsystem level type to the driver
> > > level type without having any type erasure in a void *.
> >
> > This patch alone is worth the price of admission.
>
> Right, this is how I got here as I was going to do the mdev stuff and
> we end up doing transformations like this:
>
> -static long mtty_ioctl(struct mdev_device *mdev, unsigned int cmd,
> +static long mtty_ioctl(struct vfio_device *vdev, unsigned int cmd,
>                         unsigned long arg)
>
> And if the 'struct vfio_device *vdev' was left a 'void *' then the
> compiler doesn't get to help any more :(
>
> > Seems like it would be worth adding
> > to_vfio_{pci,platform,fsl_mc}_device() helpers in this patch as well.
>
> I have mixed feelings on these one-liners. If people feel they are
> worthwhile I'll add them

It's only worthwhile in my opinion if it makes the diffstat more
favorable for lines removed.

>
> > I've sometimes added runtime type safety to to_* helpers for early
> > warning of mistakes that happen when refactoring...
> >
> > static inline struct vfio_pci_device *
> > to_vfio_pci_device(struct vfio_device *core_dev)
> > {
> >         if (dev_WARN_ONCE(core_dev->dev, core_dev->ops != &vfio_pci_ops,
> >                           "not a vfio_pci_device!\n"))
> >                 return NULL;
> >         return container_of(core_vdev, struct vfio_pci_device, vdev);
>
> In this case I don't think we need to worry as everything is tidy in a
> single module such that the compilation units can't see other
> container_of options anyhow and vfio core isn't going to accidently
> call an ops with the wrong type.

Sounds good.

You can add:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

RE: [PATCH 08/10] vfio: Make vfio_device_ops pass a 'struct vfio_device *' instead of 'void *'

[Liu, Yi L]

Hi Jason,

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, March 10, 2021 5:39 AM
[...]
> 
> -Each function is passed the device_data that was originally registered
> +Each function is passed the vdev that was originally registered
>  in the vfio_register_group_dev() call above.  This allows the bus driver
> -an easy place to store its opaque, private data.  The open/release
> +to obtain its private data using container_of().  The open/release

I think the is to let VFIO device drivers (e.g. vfio_pci driver) to obtain
its private data by using container_of instead of let the bus driver.
right?

Regards,
Yi Liu

Re: [PATCH 08/10] vfio: Make vfio_device_ops pass a 'struct vfio_device *' instead of 'void *'

[Jason Gunthorpe]

On Fri, Mar 12, 2021 at 01:42:38PM +0000, Liu, Yi L wrote:
> > -Each function is passed the device_data that was originally registered
> > +Each function is passed the vdev that was originally registered
> >  in the vfio_register_group_dev() call above.  This allows the bus driver
> > -an easy place to store its opaque, private data.  The open/release
> > +to obtain its private data using container_of().  The open/release
> 
> I think the is to let VFIO device drivers (e.g. vfio_pci driver) to obtain
> its private data by using container_of instead of let the bus driver.
> right?

vfio_pci is the "bus driver" in VFIO speak

It replaces the void * vfio_device_data() with container_of

Jason

[PATCH 09/10] vfio/pci: Replace uses of vfio_device_data() with container_of

[Jason Gunthorpe]

This tidies a few confused places that think they can have a refcount on
the vfio_device but the device_data could be NULL, that isn't possible by
design.

Most of the change falls out when struct vfio_devices is updated to just
store the struct vfio_pci_device itself. This wasn't possible before
because there was no easy way to get from the 'struct vfio_pci_device' to
the 'struct vfio_device' to put back the refcount.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/pci/vfio_pci.c | 45 ++++++++++++-------------------------
 1 file changed, 14 insertions(+), 31 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index af5696a96a76e0..4b0d60f7602e40 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -534,7 +534,7 @@ static struct vfio_pci_device *get_pf_vdev(struct vfio_pci_device *vdev,
 		return NULL;
 	}
 
-	return vfio_device_data(*pf_dev);
+	return container_of(*pf_dev, struct vfio_pci_device, vdev);
 }
 
 static void vfio_pci_vf_token_user_add(struct vfio_pci_device *vdev, int val)
@@ -794,7 +794,7 @@ int vfio_pci_register_dev_region(struct vfio_pci_device *vdev,
 }
 
 struct vfio_devices {
-	struct vfio_device **devices;
+	struct vfio_pci_device **devices;
 	int cur_index;
 	int max_index;
 };
@@ -1283,9 +1283,7 @@ static long vfio_pci_ioctl(struct vfio_device *core_vdev,
 			goto hot_reset_release;
 
 		for (; mem_idx < devs.cur_index; mem_idx++) {
-			struct vfio_pci_device *tmp;
-
-			tmp = vfio_device_data(devs.devices[mem_idx]);
+			struct vfio_pci_device *tmp = devs.devices[mem_idx];
 
 			ret = down_write_trylock(&tmp->memory_lock);
 			if (!ret) {
@@ -1300,17 +1298,13 @@ static long vfio_pci_ioctl(struct vfio_device *core_vdev,
 
 hot_reset_release:
 		for (i = 0; i < devs.cur_index; i++) {
-			struct vfio_device *device;
-			struct vfio_pci_device *tmp;
-
-			device = devs.devices[i];
-			tmp = vfio_device_data(device);
+			struct vfio_pci_device *tmp = devs.devices[i];
 
 			if (i < mem_idx)
 				up_write(&tmp->memory_lock);
 			else
 				mutex_unlock(&tmp->vma_lock);
-			vfio_device_put(device);
+			vfio_device_put(&tmp->vdev);
 		}
 		kfree(devs.devices);
 
@@ -2094,11 +2088,7 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
 	if (device == NULL)
 		return PCI_ERS_RESULT_DISCONNECT;
 
-	vdev = vfio_device_data(device);
-	if (vdev == NULL) {
-		vfio_device_put(device);
-		return PCI_ERS_RESULT_DISCONNECT;
-	}
+	vdev = container_of(device, struct vfio_pci_device, vdev);
 
 	mutex_lock(&vdev->igate);
 
@@ -2114,7 +2104,6 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
 
 static int vfio_pci_sriov_configure(struct pci_dev *pdev, int nr_virtfn)
 {
-	struct vfio_pci_device *vdev;
 	struct vfio_device *device;
 	int ret = 0;
 
@@ -2127,12 +2116,6 @@ static int vfio_pci_sriov_configure(struct pci_dev *pdev, int nr_virtfn)
 	if (!device)
 		return -ENODEV;
 
-	vdev = vfio_device_data(device);
-	if (!vdev) {
-		vfio_device_put(device);
-		return -ENODEV;
-	}
-
 	if (nr_virtfn == 0)
 		pci_disable_sriov(pdev);
 	else
@@ -2192,7 +2175,7 @@ static int vfio_pci_reflck_find(struct pci_dev *pdev, void *data)
 		return 0;
 	}
 
-	vdev = vfio_device_data(device);
+	vdev = container_of(device, struct vfio_pci_device, vdev);
 
 	if (vdev->reflck) {
 		vfio_pci_reflck_get(vdev->reflck);
@@ -2254,7 +2237,7 @@ static int vfio_pci_get_unused_devs(struct pci_dev *pdev, void *data)
 		return -EBUSY;
 	}
 
-	vdev = vfio_device_data(device);
+	vdev = container_of(device, struct vfio_pci_device, vdev);
 
 	/* Fault if the device is not unused */
 	if (vdev->refcnt) {
@@ -2262,7 +2245,7 @@ static int vfio_pci_get_unused_devs(struct pci_dev *pdev, void *data)
 		return -EBUSY;
 	}
 
-	devs->devices[devs->cur_index++] = device;
+	devs->devices[devs->cur_index++] = vdev;
 	return 0;
 }
 
@@ -2284,7 +2267,7 @@ static int vfio_pci_try_zap_and_vma_lock_cb(struct pci_dev *pdev, void *data)
 		return -EBUSY;
 	}
 
-	vdev = vfio_device_data(device);
+	vdev = container_of(device, struct vfio_pci_device, vdev);
 
 	/*
 	 * Locking multiple devices is prone to deadlock, runaway and
@@ -2295,7 +2278,7 @@ static int vfio_pci_try_zap_and_vma_lock_cb(struct pci_dev *pdev, void *data)
 		return -EBUSY;
 	}
 
-	devs->devices[devs->cur_index++] = device;
+	devs->devices[devs->cur_index++] = vdev;
 	return 0;
 }
 
@@ -2343,7 +2326,7 @@ static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev)
 
 	/* Does at least one need a reset? */
 	for (i = 0; i < devs.cur_index; i++) {
-		tmp = vfio_device_data(devs.devices[i]);
+		tmp = devs.devices[i];
 		if (tmp->needs_reset) {
 			ret = pci_reset_bus(vdev->pdev);
 			break;
@@ -2352,7 +2335,7 @@ static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev)
 
 put_devs:
 	for (i = 0; i < devs.cur_index; i++) {
-		tmp = vfio_device_data(devs.devices[i]);
+		tmp = devs.devices[i];
 
 		/*
 		 * If reset was successful, affected devices no longer need
@@ -2368,7 +2351,7 @@ static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev)
 				vfio_pci_set_power_state(tmp, PCI_D3hot);
 		}
 
-		vfio_device_put(devs.devices[i]);
+		vfio_device_put(&tmp->vdev);
 	}
 
 	kfree(devs.devices);
-- 
2.30.1

Re: [PATCH 09/10] vfio/pci: Replace uses of vfio_device_data() with container_of

[Christoph Hellwig]

On Tue, Mar 09, 2021 at 05:38:51PM -0400, Jason Gunthorpe wrote:
> This tidies a few confused places that think they can have a refcount on
> the vfio_device but the device_data could be NULL, that isn't possible by
> design.
> 
> Most of the change falls out when struct vfio_devices is updated to just
> store the struct vfio_pci_device itself. This wasn't possible before
> because there was no easy way to get from the 'struct vfio_pci_device' to
> the 'struct vfio_device' to put back the refcount.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/pci/vfio_pci.c | 45 ++++++++++++-------------------------
>  1 file changed, 14 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index af5696a96a76e0..4b0d60f7602e40 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -534,7 +534,7 @@ static struct vfio_pci_device *get_pf_vdev(struct vfio_pci_device *vdev,
>  		return NULL;
>  	}
>  
> -	return vfio_device_data(*pf_dev);
> +	return container_of(*pf_dev, struct vfio_pci_device, vdev);

I think it would be useful to just return the vfio_device and let
the caller do the container_of() here, maybe as a followup.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 09/10] vfio/pci: Replace uses of vfio_device_data() with container_of

[Jason Gunthorpe]

On Wed, Mar 10, 2021 at 08:36:34AM +0100, Christoph Hellwig wrote:
> On Tue, Mar 09, 2021 at 05:38:51PM -0400, Jason Gunthorpe wrote:
> > This tidies a few confused places that think they can have a refcount on
> > the vfio_device but the device_data could be NULL, that isn't possible by
> > design.
> > 
> > Most of the change falls out when struct vfio_devices is updated to just
> > store the struct vfio_pci_device itself. This wasn't possible before
> > because there was no easy way to get from the 'struct vfio_pci_device' to
> > the 'struct vfio_device' to put back the refcount.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> >  drivers/vfio/pci/vfio_pci.c | 45 ++++++++++++-------------------------
> >  1 file changed, 14 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index af5696a96a76e0..4b0d60f7602e40 100644
> > +++ b/drivers/vfio/pci/vfio_pci.c
> > @@ -534,7 +534,7 @@ static struct vfio_pci_device *get_pf_vdev(struct vfio_pci_device *vdev,
> >  		return NULL;
> >  	}
> >  
> > -	return vfio_device_data(*pf_dev);
> > +	return container_of(*pf_dev, struct vfio_pci_device, vdev);
> 
> I think it would be useful to just return the vfio_device and let
> the caller do the container_of() here, maybe as a followup.

The callers seem to need the vfio_pci_device *?

In a later series this function gets transformed into this:

	device_lock(&physfn->dev);
	vdev = vfio_pci_get_drvdata(physfn);
	if (!vdev) {
		device_unlock(&physfn->dev);
		return NULL;
	}
	vfio_device_get(&vdev->vdev);
	device_unlock(&physfn->dev);

There is no container_of here because the drvdata now points at the
struct vfio_pci_device

Jason

Re: [PATCH 09/10] vfio/pci: Replace uses of vfio_device_data() with container_of

[Christoph Hellwig]

On Wed, Mar 10, 2021 at 03:59:41PM -0400, Jason Gunthorpe wrote:
> > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > > index af5696a96a76e0..4b0d60f7602e40 100644
> > > +++ b/drivers/vfio/pci/vfio_pci.c
> > > @@ -534,7 +534,7 @@ static struct vfio_pci_device *get_pf_vdev(struct vfio_pci_device *vdev,
> > >  		return NULL;
> > >  	}
> > >  
> > > -	return vfio_device_data(*pf_dev);
> > > +	return container_of(*pf_dev, struct vfio_pci_device, vdev);
> > 
> > I think it would be useful to just return the vfio_device and let
> > the caller do the container_of() here, maybe as a followup.
> 
> The callers seem to need the vfio_pci_device *?

Yes.  But the container_of is trivial arithmetics, no need to waste
an indirect argument for that.

> In a later series this function gets transformed into this:
> 
> 	device_lock(&physfn->dev);
> 	vdev = vfio_pci_get_drvdata(physfn);
> 	if (!vdev) {
> 		device_unlock(&physfn->dev);
> 		return NULL;
> 	}
> 	vfio_device_get(&vdev->vdev);
> 	device_unlock(&physfn->dev);
> 
> There is no container_of here because the drvdata now points at the
> struct vfio_pci_device

Ok, so discard my comment then.

RE: [PATCH 09/10] vfio/pci: Replace uses of vfio_device_data() with container_of

[Liu, Yi L]

Hi Jason,

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, March 10, 2021 5:39 AM
> 
> This tidies a few confused places that think they can have a refcount on
> the vfio_device but the device_data could be NULL, that isn't possible by
> design.

I think the purpose of this patch is to use container_of in vfio_pci driver
since patch 0008 has already let the callbacks of vfio_device_ops to pass in
struct vfio_device, which can be used to get the vfio_pci_device easily with
container_of. Just not quite get relationship between the above commit message.
and the actual purpose. But I'm not a native speaker, so this is not a strong
comment. Just FYI.

Regards,
Yi Liu

Re: [PATCH 09/10] vfio/pci: Replace uses of vfio_device_data() with container_of

[Jason Gunthorpe]

On Fri, Mar 12, 2021 at 01:42:44PM +0000, Liu, Yi L wrote:
> Hi Jason,
> 
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Wednesday, March 10, 2021 5:39 AM
> > 
> > This tidies a few confused places that think they can have a refcount on
> > the vfio_device but the device_data could be NULL, that isn't possible by
> > design.
> 
> I think the purpose of this patch is to use container_of in vfio_pci driver
> since patch 0008 has already let the callbacks of vfio_device_ops to pass in
> struct vfio_device, which can be used to get the vfio_pci_device easily with
> container_of. Just not quite get relationship between the above commit message.
> and the actual purpose. But I'm not a native speaker, so this is not a strong
> comment. Just FYI.

Patch 8,9,10 are a sequence with the goal to remove everything related
to vfio_device_data()

Patch 8 removes the device_data connected to the ops

Patch 9 removes the remaining vfio_device_data calls.

Patch 10 removes device_data completely

Jason

[PATCH 10/10] vfio: Remove device_data from the vfio bus driver API

[Jason Gunthorpe]

There are no longer any users, so it can go away. Everything is using
container_of now.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 Documentation/driver-api/vfio.rst            |  3 +--
 drivers/vfio/fsl-mc/vfio_fsl_mc.c            |  5 +++--
 drivers/vfio/mdev/vfio_mdev.c                |  2 +-
 drivers/vfio/pci/vfio_pci.c                  |  2 +-
 drivers/vfio/platform/vfio_platform_common.c |  2 +-
 drivers/vfio/vfio.c                          | 12 +-----------
 include/linux/vfio.h                         |  4 +---
 7 files changed, 9 insertions(+), 21 deletions(-)

diff --git a/Documentation/driver-api/vfio.rst b/Documentation/driver-api/vfio.rst
index 3337f337293a32..decc68cb8114ac 100644
--- a/Documentation/driver-api/vfio.rst
+++ b/Documentation/driver-api/vfio.rst
@@ -254,8 +254,7 @@ vfio_unregister_group_dev() respectively::
 
 	void vfio_init_group_dev(struct vfio_device *device,
 				struct device *dev,
-				const struct vfio_device_ops *ops,
-				void *device_data);
+				const struct vfio_device_ops *ops);
 	int vfio_register_group_dev(struct vfio_device *device);
 	void vfio_unregister_group_dev(struct vfio_device *device);
 
diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
index 74a5de1b791934..07f636b9f6b472 100644
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
@@ -75,7 +75,8 @@ static int vfio_fsl_mc_reflck_attach(struct vfio_fsl_mc_device *vdev)
 			goto unlock;
 		}
 
-		cont_vdev = vfio_device_data(device);
+		cont_vdev =
+			container_of(device, struct vfio_fsl_mc_device, vdev);
 		if (!cont_vdev || !cont_vdev->reflck) {
 			vfio_device_put(device);
 			ret = -ENODEV;
@@ -614,7 +615,7 @@ static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev)
 		goto out_group_put;
 	}
 
-	vfio_init_group_dev(&vdev->vdev, dev, &vfio_fsl_mc_ops, vdev);
+	vfio_init_group_dev(&vdev->vdev, dev, &vfio_fsl_mc_ops);
 	mutex_init(&vdev->igate);
 	vdev->mc_dev = mc_dev;
 
diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
index e7309caa99c71b..71bd28f976e5af 100644
--- a/drivers/vfio/mdev/vfio_mdev.c
+++ b/drivers/vfio/mdev/vfio_mdev.c
@@ -138,7 +138,7 @@ static int vfio_mdev_probe(struct device *dev)
 	if (!mvdev)
 		return -ENOMEM;
 
-	vfio_init_group_dev(&mvdev->vdev, &mdev->dev, &vfio_mdev_dev_ops, mdev);
+	vfio_init_group_dev(&mvdev->vdev, &mdev->dev, &vfio_mdev_dev_ops);
 	ret = vfio_register_group_dev(&mvdev->vdev);
 	if (ret) {
 		kfree(mvdev);
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 4b0d60f7602e40..3c2497ba79460c 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1962,7 +1962,7 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto out_group_put;
 	}
 
-	vfio_init_group_dev(&vdev->vdev, &pdev->dev, &vfio_pci_ops, vdev);
+	vfio_init_group_dev(&vdev->vdev, &pdev->dev, &vfio_pci_ops);
 	vdev->pdev = pdev;
 	vdev->irq_type = VFIO_PCI_NUM_IRQS;
 	mutex_init(&vdev->igate);
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index f5f6b537084a67..361e5b57e36932 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -666,7 +666,7 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 	struct iommu_group *group;
 	int ret;
 
-	vfio_init_group_dev(&vdev->vdev, dev, &vfio_platform_ops, vdev);
+	vfio_init_group_dev(&vdev->vdev, dev, &vfio_platform_ops);
 
 	ret = vfio_platform_acpi_probe(vdev, dev);
 	if (ret)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 01de47d1810b6b..39ea77557ba0c4 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -741,12 +741,11 @@ static int vfio_iommu_group_notifier(struct notifier_block *nb,
  * VFIO driver API
  */
 void vfio_init_group_dev(struct vfio_device *device, struct device *dev,
-			 const struct vfio_device_ops *ops, void *device_data)
+			 const struct vfio_device_ops *ops)
 {
 	init_completion(&device->comp);
 	device->dev = dev;
 	device->ops = ops;
-	device->device_data = device_data;
 }
 EXPORT_SYMBOL_GPL(vfio_init_group_dev);
 
@@ -851,15 +850,6 @@ static struct vfio_device *vfio_device_get_from_name(struct vfio_group *group,
 	return device;
 }
 
-/*
- * Caller must hold a reference to the vfio_device
- */
-void *vfio_device_data(struct vfio_device *device)
-{
-	return device->device_data;
-}
-EXPORT_SYMBOL_GPL(vfio_device_data);
-
 /*
  * Decrement the device reference count and wait for the device to be
  * removed.  Open file descriptors for the device... */
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 784c34c0a28763..a2c5b30e1763ba 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -24,7 +24,6 @@ struct vfio_device {
 	refcount_t refcount;
 	struct completion comp;
 	struct list_head group_next;
-	void *device_data;
 };
 
 /**
@@ -61,12 +60,11 @@ extern struct iommu_group *vfio_iommu_group_get(struct device *dev);
 extern void vfio_iommu_group_put(struct iommu_group *group, struct device *dev);
 
 void vfio_init_group_dev(struct vfio_device *device, struct device *dev,
-			 const struct vfio_device_ops *ops, void *device_data);
+			 const struct vfio_device_ops *ops);
 int vfio_register_group_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);
-extern void *vfio_device_data(struct vfio_device *device);
 
 /* events for the backend driver notify callback */
 enum vfio_iommu_notify_type {
-- 
2.30.1

Re: [PATCH 10/10] vfio: Remove device_data from the vfio bus driver API

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 00/10] Embed struct vfio_device in all sub-structures

[Alex Williamson]

On Tue,  9 Mar 2021 17:38:42 -0400
Jason Gunthorpe <jgg@nvidia.com> wrote:
> This series:
> 
> The main focus of this series is to make VFIO follow the normal kernel
> convention of structure embedding for structure inheritance instead of
> linking using a 'void *opaque'. Here we focus on moving the vfio_device to
> be a member of every struct vfio_XX_device that is linked by a
> vfio_add_group_dev().
> 
> In turn this allows 'struct vfio_device *' to be used everwhere, and the
> public API out of vfio.c can be cleaned to remove places using 'struct
> device *' and 'void *' as surrogates to refer to the device.
> 
> While this has the minor trade off of moving 'struct vfio_device' the
> clarity of the design is worth it. I can speak directly to this idea, as
> I've invested a fair amount of time carefully working backwards what all
> the type-erased APIs are supposed to be and it is certainly not trivial or
> intuitive.
> 
> When we get into mdev land things become even more inscrutable, and while
> I now have a pretty clear picture, it was hard to obtain. I think this
> agrees with the kernel style ideal of being explicit in typing and not
> sacrificing clarity to create opaque structs.
> 
> After this series the general rules are:
>  - Any vfio_XX_device * can be obtained at no cost from a vfio_device *
>    using container_of(), and the reverse is possible by &XXdev->vdev
> 
>    This is similar to how 'struct pci_device' and 'struct device' are
>    interrelated.
> 
>    This allows 'device_data' to be completely removed from the vfio.c API.
> 
>  - The drvdata for a struct device points at the vfio_XX_device that
>    belongs to the driver that was probed. drvdata is removed from the core
>    code, and only used as part of the implementation of the struct
>    device_driver.
> 
>  - The lifetime of vfio_XX_device and vfio_device are identical, they are
>    the same memory.
> 
>    This follows the existing model where vfio_del_group_dev() blocks until
>    all vfio_device_put()'s are completed. This in turn means the struct
>    device_driver remove() blocks, and thus under the driver_lock() a bound
>    driver must have a valid drvdata pointing at both vfio device
>    structs. A following series exploits this further.
> 
> Most vfio_XX_device structs have data that duplicates the 'struct
> device *dev' member of vfio_device, a following series removes that
> duplication too.
> 
> Jason
> 
> Jason Gunthorpe (10):
>   vfio: Simplify the lifetime logic for vfio_device
>   vfio: Split creation of a vfio_device into init and register ops
>   vfio/platform: Use vfio_init/register/unregister_group_dev
>   vfio/fsl-mc: Use vfio_init/register/unregister_group_dev
>   vfio/pci: Use vfio_init/register/unregister_group_dev
>   vfio/mdev: Use vfio_init/register/unregister_group_dev
>   vfio/mdev: Make to_mdev_device() into a static inline
>   vfio: Make vfio_device_ops pass a 'struct vfio_device *' instead of
>     'void *'
>   vfio/pci: Replace uses of vfio_device_data() with container_of
>   vfio: Remove device_data from the vfio bus driver API
> 
>  Documentation/driver-api/vfio.rst             |  48 ++--
>  drivers/vfio/fsl-mc/vfio_fsl_mc.c             |  69 +++---
>  drivers/vfio/fsl-mc/vfio_fsl_mc_private.h     |   1 +
>  drivers/vfio/mdev/mdev_private.h              |   5 +-
>  drivers/vfio/mdev/vfio_mdev.c                 |  57 +++--
>  drivers/vfio/pci/vfio_pci.c                   | 109 +++++----
>  drivers/vfio/pci/vfio_pci_private.h           |   1 +
>  drivers/vfio/platform/vfio_amba.c             |   8 +-
>  drivers/vfio/platform/vfio_platform.c         |  21 +-
>  drivers/vfio/platform/vfio_platform_common.c  |  56 ++---
>  drivers/vfio/platform/vfio_platform_private.h |   5 +-
>  drivers/vfio/vfio.c                           | 210 ++++++------------
>  include/linux/vfio.h                          |  37 +--
>  13 files changed, 299 insertions(+), 328 deletions(-)
> 

This looks great.  As Christoph noted, addressing those init vs
register races in the bus drivers don't seem too difficult or out of
scope for this series.  Thanks,

Alex

Re: [PATCH 00/10] Embed struct vfio_device in all sub-structures

[Jason Gunthorpe]

On Wed, Mar 10, 2021 at 04:52:47PM -0700, Alex Williamson wrote:

> This looks great.  As Christoph noted, addressing those init vs
> register races in the bus drivers don't seem too difficult or out of
> scope for this series.  Thanks,

Sure, I'm happy to add it. I need to check vfio-pci closely that there
is no hidden dependency, but fsl looked fine

I'll look at splitting patch 1 as well and send a v2.

Thanks
Jason