On Thu, Mar 16, 2023 at 05:41:52AM -0700, Yi Liu wrote:
> as an alternative method for ownership check when iommufd is used. In
> this case all opened devices in the affected dev_set are verified to
> be bound to a same valid iommufd value to allow reset. It's simpler
> and faster as user does not need to pass a set of fds and kernel no
> need to search the device within the given fds.
>
> a device in noiommu mode doesn't have a valid iommufd, so this method
> should not be used in a dev_set which contains multiple devices and one
> of them is in noiommu. The only allowed noiommu scenario is that the
> calling device is noiommu and it's in a singleton dev_set.
>
> Suggested-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> ---
> drivers/iommu/iommufd/device.c | 6 ++
> drivers/vfio/iommufd.c | 8 +++
> drivers/vfio/pci/vfio_pci_core.c | 94 +++++++++++++++++++++++---------
> include/linux/iommufd.h | 1 +
> include/linux/vfio.h | 3 +
> include/uapi/linux/vfio.h | 9 ++-
> 6 files changed, 93 insertions(+), 28 deletions(-)
This could probably be split to two or three patches
> -static int vfio_pci_ioctl_pci_hot_reset(struct vfio_pci_core_device *vdev,
> - struct vfio_pci_hot_reset __user *arg)
> +static int
> +vfio_pci_ioctl_pci_hot_reset_groups(struct vfio_pci_core_device *vdev,
> + struct vfio_pci_hot_reset *hdr,
> + bool slot,
> + struct vfio_pci_hot_reset __user *arg)
> {
At least this mechanical re-organization should be in its own patch
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 3188d8a374bd..f0a5ff317b20 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -116,6 +116,7 @@ struct vfio_device_ops {
> int vfio_iommufd_physical_bind(struct vfio_device *vdev,
> struct iommufd_ctx *ictx, u32 *out_device_id);
> void vfio_iommufd_physical_unbind(struct vfio_device *vdev);
> +struct iommufd_ctx *vfio_iommufd_physical_ictx(struct vfio_device *vdev);
> int vfio_iommufd_physical_attach_ioas(struct vfio_device *vdev, u32 *pt_id);
> int vfio_iommufd_emulated_bind(struct vfio_device *vdev,
> struct iommufd_ctx *ictx, u32 *out_device_id);
> @@ -127,6 +128,8 @@ int vfio_iommufd_emulated_attach_ioas(struct vfio_device *vdev, u32 *pt_id);
> u32 *out_device_id)) NULL)
> #define vfio_iommufd_physical_unbind \
> ((void (*)(struct vfio_device *vdev)) NULL)
> +#define vfio_iommufd_physical_ictx \
> + ((struct iommufd_ctx * (*)(struct vfio_device *vdev)) NULL)
??
This should just be a normal static inline?? It won't compile like
this.
It would also be a nice touch to include a new vfio_pci_hot_reset_info
that returns the dev_id's of the other devices in the reset group
instead of a BDF. It would be alot easier for userspace to work with.
Otherwise this looks basically OK.
Jason
