Re: [PATCH rfc] vfio-pci: Allow write combining

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 1 Aug 2024 14:53:39 -0300
Jason Gunthorpe <jgg@ziepe.ca> wrote:

> On Thu, Aug 01, 2024 at 11:33:44AM -0600, Alex Williamson wrote:
> > On Thu, 1 Aug 2024 14:13:55 -0300
> > Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >   
> > > On Thu, Aug 01, 2024 at 10:52:18AM -0600, Alex Williamson wrote:  
> > > > > > vfio_region_info.flags in not currently tested for input therefore this
> > > > > > proposal could lead to unexpected behavior for a caller that doesn't
> > > > > > currently zero this field.  It's intended as an output-only field.      
> > > > > 
> > > > > Perhaps a REGION_INFO2 then?
> > > > > 
> > > > > I still think per-request is better than a global flag    
> > > > 
> > > > I don't understand why we'd need a REGION_INFO2, we already have
> > > > support for defining new regions.    
> > > 
> > > It is not a new region, it is a modified mmap behavior for an existing
> > > region.  
> > 
> > If we're returning a different offset into the vfio device file from
> > which to get a WC mapping, what's the difference?   
> 
> I think it is a pretty big difference.. The offset is just a "mmap
> cookie", it doesn't have to be 1:1 with the idea of a region.
> 
> > A vfio "region" is
> > describing a region or range of the vfio device file descriptor.  
> 
> I'm thinking a region is describing an area of memory that is
> available in the VFIO device. The offset output is just a "mmap
> cookie" to tell userspace how to mmap it. Having N mmap cookies for 1
> region is OK.

Is an "mmap cookie" an offset into the vfio device file where mmap'ing
that offset results in a WC mapping to a specific device resource?
Isn't that just a region that doesn't have an index or supporting
infrastructure?
 
> > > > We'd populate these new regions only for BARs that support prefetch and
> > > > mmap     
> > >
> > > That's not the point, prefetch has nothing to do with write combining.  
> > 
> > I was following the original proposal in this thread that added a
> > prefetch flag to REGION_INFO and allowed enabling WC only for
> > IORESOURCE_PREFETCH.  
> 
> Oh, I didn't notice that, it shouldn't do that. Returning the
> VFIO_REGION_FLAG_WRITE_COMBINE makes sense, but it shouldn't effect
> what the kernel allows.
> 
> > > Doubling all the region indexes just for WC does not seem like a good
> > > idea to me...  
> > 
> > Is the difference you see that in the REQ_WC proposal the user is
> > effectively asking vfio to pop a WC region into existence vs here
> > they're pre-populated?   
> 
> ?? This didn't create more regions AFAICT. It created a new global
> 
> +	bool			bar_write_combine[PCI_STD_NUM_BARS];
> 
> Which controls what NC/WC the mmap creates when called:
> 
> +	if (vdev->bar_write_combine[index])
> +		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
> +	else
> +		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> 
> You get the same output from REGION_INFO, same number of regions.

It was your proposal that introduced REQ_WC, this is Keith's original
proposal.  I'm equating a REQ_WC request inventing an "mmap cookie" as
effectively the same as bringing a lightweight region into existence
because it defines a section of the vfio device file to have specific
mmap semantics.

> It was the other proposal from long ago that created more regions.
> 
> This is what I like and would prefer to stick with. REGION_INFO
> doesn't really change, we don't have two regions refering to the same
> physical memory, and we find some way to request NC/WC of a region at
> mmap time.

"At mmap time" means that something in the vma needs to describe to us
to use the WC semantics, where I think you're proposing that the "mmap
cookie" provides a specific vm_pgoff which we already use to determine
the region index.  So whether or not we want to call this a region,
it's effectively in the same address space as regions.  Therefore "mmap
cookie" ~= "region offset".

> A global is a neat trick, but it would be cleaner to request
> properties of the mmap when the "mmap cookie" is obtained.
> 
> > At the limit they're the same.  We could use a
> > DEVICE_FEATURE to ask vfio to selectively populate WC regions after
> > which the user could re-enumerate additional regions, or in fact to
> > switch on WC for a given region if we want to go that route.  Thanks,  
> 
> This is still adding more regions and reporting more stuff from
> REGION_INFO, that is what I would like to avoid.

Why?  This reminds me of hidden registers outside of capability chains
in PCI config space.  Thanks,

Alex