Re: [PATCH 2/2] vfio/pci: Use unmap_mapping_range()

[Peter Xu <peterx@redhat.com>]

On Fri, May 24, 2024 at 10:22:40AM -0300, Jason Gunthorpe wrote:
> On Fri, May 24, 2024 at 08:40:26AM +0000, Tian, Kevin wrote:
> > > From: Peter Xu <peterx@redhat.com>
> > > Sent: Friday, May 24, 2024 8:49 AM
> > > 
> > > Hi, Yan,
> > > 
> > > On Fri, May 24, 2024 at 08:39:37AM +0800, Yan Zhao wrote:
> > > > On Thu, May 23, 2024 at 01:56:27PM -0600, Alex Williamson wrote:
> > > > > With the vfio device fd tied to the address space of the pseudo fs
> > > > > inode, we can use the mm to track all vmas that might be mmap'ing
> > > > > device BARs, which removes our vma_list and all the complicated lock
> > > > > ordering necessary to manually zap each related vma.
> > > > >
> > > > > Note that we can no longer store the pfn in vm_pgoff if we want to use
> > > > > unmap_mapping_range() to zap a selective portion of the device fd
> > > > > corresponding to BAR mappings.
> > > > >
> > > > > This also converts our mmap fault handler to use vmf_insert_pfn()
> > > > Looks vmf_insert_pfn() does not call memtype_reserve() to reserve
> > > memory type
> > > > for the PFN on x86 as what's done in io_remap_pfn_range().
> > > >
> > > > Instead, it just calls lookup_memtype() and determine the final prot based
> > > on
> > > > the result from this lookup, which might not prevent others from reserving
> > > the
> > > > PFN to other memory types.
> > > 
> > > I didn't worry too much on others reserving the same pfn range, as that
> > > should be the mmio region for this device, and this device should be owned
> > > by vfio driver.
> > 
> > and the earliest point doing memtype_reserve() is here:
> > 
> > vfio_pci_core_mmap()
> > 	vdev->barmap[index] = pci_iomap(pdev, index, 0);
> > 
> > > 
> > > However I share the same question, see:
> > > 
> > > https://lore.kernel.org/r/20240523223745.395337-2-peterx@redhat.com
> > > 
> > > So far I think it's not a major issue as VFIO always use UC- mem type, and
> > > that's also the default.  But I do also feel like there's something we can
> > > do better, and I'll keep you copied too if I'll resend the series.
> > > 
> > 
> > vfio-nvgrace uses WC. But it directly does remap_pfn_range() in its
> > nvgrace_gpu_mmap() so not suffering from the issue here.
> 
> People keep asking for WC on normal VFIO PCI as well, we shouldn't
> rule out, or at least provide a big warning comment what needs to be
> fixed to allow it.

Maybe we can have a comment indeed.  Or as long as that pat series can get
merged before adding WC support we should also be good, and that's also the
hope..

Thanks,

-- 
Peter Xu