Re: [RFC PATCH 05/10] vfio: Create a vfio_device from vma lookup

From: Jason Gunthorpe <jgg@nvidia.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: <cohuck@redhat.com>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <peterx@redhat.com>
Subject: Re: [RFC PATCH 05/10] vfio: Create a vfio_device from vma lookup
Date: Wed, 24 Feb 2021 20:06:10 -0400	[thread overview]
Message-ID: <20210225000610.GP4247@nvidia.com> (raw)
In-Reply-To: <20210224145506.48f6e0b4@omen.home.shazbot.org>

On Wed, Feb 24, 2021 at 02:55:06PM -0700, Alex Williamson wrote:

> > The only use of the special ops would be if there are multiple types
> > of mmap's going on, but for this narrow use case those would be safely
> > distinguished by the vm_pgoff instead
> 
> We potentially do have device specific regions which can support mmap,
> for example the migration region.  We'll need to think about how we
> could even know if portions of those regions map to a device.  We could
> use the notifier to announce it and require the code supporting those
> device specific regions manage it.

So, the above basically says any VFIO VMA is allowed for VFIO to map
to the IOMMU.

If there are places creating mmaps for VFIO that should not go to the
IOMMU then they need to return NULL from this function.

> I'm not really clear what you're getting at with vm_pgoff though, could
> you explain further?

Ah, so I have to take a side discussion to explain what I ment.

The vm_pgoff is a bit confused because we change it here in vfio_pci:

    vma->vm_pgoff = (pci_resource_start(pdev, index) >> PAGE_SHIFT) + pgoff;

But the address_space invalidation assumes it still has the region
based encoding:

+	vfio_device_unmap_mapping_range(vdev->device,
+			VFIO_PCI_INDEX_TO_OFFSET(VFIO_PCI_BAR0_REGION_INDEX),
+			VFIO_PCI_INDEX_TO_OFFSET(VFIO_PCI_ROM_REGION_INDEX) -
+			VFIO_PCI_INDEX_TO_OFFSET(VFIO_PCI_BAR0_REGION_INDEX));

Those three indexes are in the vm_pgoff numberspace and so vm_pgoff
must always be set to the same thing - either the
VFIO_PCI_INDEX_TO_OFFSET() coding or the physical pfn. 

Since you say we need a limited invalidation this looks like a bug to
me - and it must always be the VFIO_PCI_INDEX_TO_OFFSET coding.

So, the PCI vma needs to get switched to use the
VFIO_PCI_INDEX_TO_OFFSET coding and then we can always extract the
region number from the vm_pgoff and thus access any additional data,
such as the base pfn or a flag saying this cannot be mapped to the
IOMMU. Do the reverse of VFIO_PCI_INDEX_TO_OFFSET and consult
information attached to that region ID.

All places creating vfio mmaps have to set the vm_pgoff to
VFIO_PCI_INDEX_TO_OFFSET().

But we have these violations that need fixing:

drivers/vfio/fsl-mc/vfio_fsl_mc.c:      vma->vm_pgoff = (region.addr >> PAGE_SHIFT) + pgoff;
drivers/vfio/platform/vfio_platform_common.c:   vma->vm_pgoff = (region.addr >> PAGE_SHIFT) + pgoff;

Couldn't see any purpose to this code, cargo cult copy? Just delete
it.

drivers/vfio/pci/vfio_pci.c:    vma->vm_pgoff = (pci_resource_start(pdev, index) >> PAGE_SHIFT) + pgoff;

Used to implement fault() but we could get the region number and
extract the pfn from the vfio_pci_device's data easy enough.

I manually checked that other parts of VFIO not under drivers/vfio are
doing it OK, looks fine.

Jason