On Thu, 27 Apr 2023 06:59:17 +0000
"Liu, Yi L" <yi.l.liu@xxxxxxxxx> wrote:
> > From: Tian, Kevin <kevin.tian@xxxxxxxxx>
> > Sent: Thursday, April 27, 2023 2:39 PM
> >
> > > From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> > > 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.
> > 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. 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,
Alex
