Re: [PATCH v13 00/22] Add vfio_device cdev for iommufd support

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Jun 16, 2023 at 02:39:24AM -0700, Yi Liu wrote:
> Existing VFIO provides group-centric user APIs for userspace. Userspace
> opens the /dev/vfio/$group_id first before getting device fd and hence
> getting access to device. This is not the desired model for iommufd. Per
> the conclusion of community discussion[1], iommufd provides device-centric
> kAPIs and requires its consumer (like VFIO) to be device-centric user
> APIs. Such user APIs are used to associate device with iommufd and also
> the I/O address spaces managed by the iommufd.
> 
> This series first introduces a per device file structure to be prepared
> for further enhancement and refactors the kvm-vfio code to be prepared
> for accepting device file from userspace. After this, adds a mechanism for
> blocking device access before iommufd bind. Then refactors the vfio to be
> able to handle cdev path (e.g. iommufd binding, no-iommufd, [de]attach ioas).
> This refactor includes making the device_open exclusive between the group
> and the cdev path, only allow single device open in cdev path; vfio-iommufd
> code is also refactored to support cdev. e.g. split the vfio_iommufd_bind()
> into two steps. Eventually, adds the cdev support for vfio device and the
> new ioctls, then makes group infrastructure optional as it is not needed
> when vfio device cdev is compiled.
> 
> This series is based on some preparation works done to vfio emulated devices[2]
> and vfio pci hot reset enhancements[3].
> 
> This series is a prerequisite for iommu nesting for vfio device[4] [5].
> 
> The complete code can be found in below branch, simple tests done to the
> legacy group path and the cdev path. Draft QEMU branch can be found at[6]
> However, the noiommu mode test is only done with some hacks in kernel and
> qemu to check if qemu can boot with noiommu devices.
> 
> https://github.com/yiliu1765/iommufd/tree/vfio_device_cdev_v13
> (config CONFIG_IOMMUFD=y CONFIG_VFIO_DEVICE_CDEV=y)
> 
> base-commit: dcc9d48709e6bc6ec3da97626b8768582e138326
> 
> [1] https://lore.kernel.org/kvm/BN9PR11MB5433B1E4AE5B0480369F97178C189@BN9PR11MB5433.namprd11.prod.outlook.com/
> [2] https://lore.kernel.org/kvm/20230327093351.44505-1-yi.l.liu@intel.com/ - merged
> [3] https://lore.kernel.org/kvm/20230616093042.65094-1-yi.l.liu@intel.com/
> [4] https://lore.kernel.org/linux-iommu/20230511143844.22693-1-yi.l.liu@intel.com/
> [5] https://lore.kernel.org/linux-iommu/20230511145110.27707-1-yi.l.liu@intel.com/#t
> [6] https://github.com/yiliu1765/qemu/tree/iommufd_rfcv4.mig.reset.v4_var3
> 
> Change log:
> 
> v13:
>  - vfio_device_first_open() and vfio_device_last_close() to be vfio_df_device_first_open()
>    vfio_df_device_last_close() (Alex)
>  - Define struct vfio_device_file::access_granted as u8 and also place the u32 devid to
>    be behind this flag as this structure access is hot, so needs to avoid too much hole
>    in the structure (Alex)
>  - Use u8 instead bool in the struct vfio_device for the flags (Alex)
>  - Define BIND, ATTACH, DETACH ioctl behind VFIO_DEVICE_FEATURE whose offset is 17 (Alex)
>  - Drop patch 20, 21, 22 of v12 (Alex)
>  - Per the patch drop, still needs to detect the physical devices that do not have
>    IOMMU in the cdev registration as cdev does not support such devices. Per the
>    suggestion from Jason, lift the IOMMU_CAP_CACHE_COHERENCY check to be in vfio_main.c
>    so that it can fail the registration of such devices if only cdev is compiled. (Jason, Alex)
>  - Refine the vfio.rst doc, highlight that the cdev device access is stil bound with
>    iommu group. (Alex)
>  - Reaffirm t-b from below folks:
>    Nicolin Chen - Test nesting branch which is based on cdev v12, the test is done on ARM64 (SMMUv3)
>    Matthew Rosato - vfio-pci, vfio-ap, vfio-ccw under container, compat and cdev mode, and nesting
>                     test on SMMUv3 and Intel.
>    Yanting Jiang - regression tests with NIC passthrough on Intel platform

I accendiently put my remarks on v12, but they all apply here, and I
don't have any new remarks for this version.

Thanks,
Jason