On 2023/4/28 02:32, Alex Williamson wrote:
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.
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@xxxxxxxxxx/#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
