[Intel-gfx] [PATCH v4 2/9] vfio-iommufd: Create iommufd_access for noiommu devices

[Yi Liu]

On 2023/4/28 02:32, Alex Williamson wrote:
> On Thu, 27 Apr 2023 06:59:17 +0000
> "Liu, Yi L" <yi.l.liu at intel.com> wrote:
> 
>>> From: Tian, Kevin <kevin.tian at intel.com>
>>> Sent: Thursday, April 27, 2023 2:39 PM
>>>    
>>>> From: Liu, Yi L <yi.l.liu at intel.com>
>>>> Sent: Wednesday, April 26, 2023 10:54 PM
>>>> @@ -121,7 +128,8 @@ static void vfio_emulated_unmap(void *data,
>>>> unsigned long iova,
>>>>   {
>>>>   	struct vfio_device *vdev = data;
>>>>
>>>> -	if (vdev->ops->dma_unmap)
>>>> +	/* noiommu devices cannot do map/unmap */
>>>> +	if (vdev->noiommu && vdev->ops->dma_unmap)
>>>>   		vdev->ops->dma_unmap(vdev, iova, length);
>>>
>>> Is it necessary? All mdev devices implementing @dma_unmap won't
>>> set noiommu flag.
>>
>> Hmmm. Yes, and all the devices set noiommu is not implementing @dma_unmap
>> as far as I see. Maybe this noiommu check can be removed.
> 
> Not to mention that the polarity of the noiommu test is backwards here!
> This also seems to be the only performance path where noiommu is tested
> and therefore I believe the only actual justification of the previous
> patch.

but this patch needs to use vfio_iommufd_emulated_bind() and
vfio_iommufd_emulated_unbind() for the noiommu devices when binding
to iommufd. So needs to check noiommu in the vfio_iommufd_bind()
and vfio_iommu_unbind() as well.

>>> Instead in the future if we allow noiommu userspace to pin pages
>>> we'd need similar logic too.
>>
>> I'm not quite sure about it so far. For mdev devices, the device driver
>> may use vfio_pin_pages/vfio_dma_rw () to pin page. Hence such drivers
>> need to listen to dma_unmap() event. But for noiommu users, does the
>> device driver also participate in the page pin? At least for vfio-pci driver,
>> it does not, or maybe it will in the future when enabling noiommu
>> userspace to pin pages. It looks to me such userspace should order
>> the DMA before calling ioctl to unpin page instead of letting device
>> driver listen to unmap.
> 
> Whoa, noiommu is inherently unsafe an only meant to expose the vfio
> device interface for userspace drivers that are going to do unsafe
> things regardless.  Enabling noiommu to work with mdev, pin pages, or
> anything else should not be on our agenda.

One clarification. I think the idea from Jason is to make noiommu
userspace to be able to pin page. [1]. But this is just a potential
benefit of creating iommufd_access for noiommu devices. There is no
intention to make noiommu devices to work with mdev.

[1] https://lore.kernel.org/kvm/ZD1MCc6fD+oisjki@nvidia.com/#t

> Userspaces relying on niommu
> get the minimum viable interface and must impose a minuscule
> incremental maintenance burden.  The only reason we're spending so much
> effort on it here is to make iommufd noiommu support equivalent to
> group/container noiommu support.  We should stop at that.  Thanks,

yes. This is why this patch is to bind noiommu devices to iommufd
and create iommufd_access. Otherwise, noiommu devices would have
trouble in the hot-reset path when iommufd-based ownership check
model is applied, and this is the only model for cdev. So binding
noiommu devices to iommufd is necessary for support noiommu in the
cdev interface.

If this above makes sense. Then back to the question of noiommu
check in vfio_emulated_unmap(). At first, I intend to have such
a check to avoid calling dma_unmap callback for noiommu devices.
But per Kevin's comment and your above statement on mdev and noiommu,
so in reality, noiommu device drivers won't implement dma_unmap
callback. So it is probably fine to remove the noiommu check in
vfio_emulated_unmap().

-- 
Regards,
Yi Liu