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

[Alex Williamson <alex.williamson@redhat.com>]

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