Re: [Intel-wired-lan] [PATCH iwl-next V2 15/15] vfio/ice: support iommufd vfio compat mode

[Jason Gunthorpe <jgg@ziepe.ca>]

On Wed, Jun 21, 2023 at 09:11:12AM +0000, Lingyu Liu wrote:
> From: Yahui Cao <yahui.cao@intel.com>
> 
> In iommufd vfio compat mode, vfio_dma_rw() will return failure, since
> vfio_device_has_container() returns false and device->iommufd_access is
> NULL.
> 
> Currently device->iommufd_access will not be created if vfio device is
> backed by pci device. To support IOVA access, manually create
> iommufd_access context by iommufd_access_create/attach() and access IOVA
> by iommufd_access_rw(). And in order to minimize the iommufd_access's
> impact, store the iommufd_access context in driver data, create it only
> before loading the device state and destroy it once finishing loading
> the device state.
> 
> To be compatible with legacy vfio, use vfio_device_has_container() to
> check the vfio uAPI. If in legacy vfio mode, call vfio_dma_rw()
> directly, otherwise call iommufd_access_rw().

This is not the right approach, you should create your access by
overloading the iommufd ops. Nak on exposing vfio_device_has_container

> +/**
> + * ice_vfio_pci_emulated_unmap - callback to unmap IOVA
> + * @data: function handler data
> + * @iova: I/O virtuall address
> + * @len: IOVA length
> + *
> + * This function is called when application are doing DMA unmap and in some
> + * cases driver needs to explicitly do some unmap ops if this device does not
> + * have backed iommu. Nothing is required here since this is pci baseed vfio
> + * device, which has backed iommu.
> + */
> +static void
> +ice_vfio_pci_emulated_unmap(void *data, unsigned long iova, unsigned long len)
> +{
> +}
> +
> +static const struct iommufd_access_ops ice_vfio_user_ops = {
> +	.needs_pin_pages = 1,
> +	.unmap = ice_vfio_pci_emulated_unmap,
> +};

If you don't call pin pages then you shouldn't set needs_pin_pages?

An empty unmap op is unconditionally wrong.

> + * ice_vfio_dma_rw - read/write function for device IOVA address space
> + * @data: function handler data
> + * @iova: I/O virtuall address
> + * @buf: buffer for read/write access
> + * @len: buffer length
> + * @write: true for write, false for read
> + *
> + * Read/write function for device IOVA access. Since vfio_dma_rw() may fail
> + * at iommufd vfio compatiable mode, we need runtime check what uAPI it is
> + * using and use corresponding access method for IOVA access.
> + *
> + * Return 0 for success, negative value for failure.
> + */
> +static int ice_vfio_dma_rw(void *data, dma_addr_t iova,
> +			   void *buf, size_t len, bool write)
> +{
> +	struct ice_vfio_pci_core_device *ice_vdev =
> +			(struct ice_vfio_pci_core_device *)data;
> +	struct vfio_device *vdev = &ice_vdev->core_device.vdev;
> +	unsigned int flags = 0;
> +
> +	if (vfio_device_has_container(vdev))
> +		return vfio_dma_rw(vdev, iova, buf, len, write);
> +
> +	if (!current->mm)
> +		flags |= IOMMUFD_ACCESS_RW_KTHREAD;

No, you need to know your own calling context, you can't guess like this.

I suppose this is always called from an ioctl?

> @@ -19,7 +21,7 @@ void ice_migration_uninit_vf(void *opaque);
>  int ice_migration_suspend_vf(void *opaque, bool mig_dst);
>  int ice_migration_save_devstate(void *opaque, u8 *buf, u64 buf_sz);
>  int ice_migration_restore_devstate(void *opaque, const u8 *buf, u64 buf_sz,
> -				   struct vfio_device *vdev);
> +				   dma_rw_handler_t handler, void *data);

Please remove all the wild function pointers and void * opaques I see
in this driver. Use proper types and get your layering right so you
dont't have to fake up improper cross-layer calls like this.

Jason
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan