| Message ID | 0-v1-7355d38b9344+17481-vfio1_jgg@nvidia.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Embed struct vfio_device in all sub-structures | expand |
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
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
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(-)