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

["Cao, Yahui" <yahui.cao@intel.com>]

Hi Jason,

On 6/21/2023 10:40 PM, Jason Gunthorpe wrote:
> 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
Could you explain a little bit more about "create your access by 
overloading the iommufd ops." ?

>> +/**
>> + * 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.
Will change in next version.
>
>> + * 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?
Yes. This is always called from ioctl. Will remove this calling context 
check.
>> @@ -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

Will refactor the API with proper types.

Thanks.
Yahui.

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