On Thu, Aug 01, 2024 at 12:16:57PM -0600, Alex Williamson wrote:
> On Thu, 1 Aug 2024 14:53:39 -0300
> Jason Gunthorpe <jgg@xxxxxxxx> 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@xxxxxxxx> 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?
Yes
> Isn't that just a region that doesn't have an index or supporting
> infrastructure?
No? It is a "mmap cookie" that has the requested mmap-time properties.
Today the kernel side binds the mmap offset to the index in a computed
way, but from a API perspective userspace does REGION_INFO and gets
back an opaque "mmap cookie" that it uses to pass to mmap to get back
the thing describe by REGION_INFO. Userspace has no idea about any
structure to the cookie numbers.
> > 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.
Well, again, it is not a region, it is just a record that this mmap
cookie uses X region with Y mapping flags. The number of regions don't
change. Critically from a driver perspective the number of regions and
region indexes wouldn't change.
I am not thing of making a new region, I am thinking of adjusting how
mmap works. Like today we do this:
index = vma->vm_pgoff >> (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT);
So maybe instead it is something like:
vfio_decode_mmap_cookie(vma, &index, &flags);
if (flags & VFIO_MMAP_WC)
prot = pgprot_writecombine(..)
