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

[Jason Gunthorpe <jgg@ziepe.ca>]

On Thu, Aug 01, 2024 at 09:41:23AM -0600, Alex Williamson wrote:
> On Thu, 1 Aug 2024 11:19:14 -0300
> Jason Gunthorpe <jgg@ziepe.ca> wrote:
> 
> > On Wed, Jul 31, 2024 at 08:53:52AM -0700, Keith Busch wrote:
> > > From: Keith Busch <kbusch@kernel.org>
> > > 
> > > Write combining can be provide performance improvement for places that
> > > can safely use this capability.
> > > 
> > > Previous discussions on the topic suggest a vfio user needs to
> > > explicitly request such a mapping, and it sounds like a new vfio
> > > specific ioctl to request this is one way recommended way to do that.
> > > This patch implements a new ioctl to achieve that so a user can request
> > > write combining on prefetchable memory. A new ioctl seems a bit much for
> > > just this purpose, so the implementation here provides a "flags" field
> > > with only the write combine option defined. The rest of the bits are
> > > reserved for future use.  
> > 
> > This is a neat hack for sure
> > 
> > But how about adding this flag to vfio_region_info ?
> > 
> > @@ -275,6 +289,7 @@ struct vfio_region_info {
> >  #define VFIO_REGION_INFO_FLAG_WRITE    (1 << 1) /* Region supports write */
> >  #define VFIO_REGION_INFO_FLAG_MMAP     (1 << 2) /* Region supports mmap */
> >  #define VFIO_REGION_INFO_FLAG_CAPS     (1 << 3) /* Info supports caps */
> > +#define VFIO_REGION_INFO_REQ_WC         (1 << 4) /* Request a write combining mapping*/
> >         __u32   index;          /* Region index */
> >         __u32   cap_offset;     /* Offset within info struct of first cap */
> >         __aligned_u64   size;   /* Region size (bytes) */
> > 
> > 
> > It specify REQ_WC when calling VFIO_DEVICE_GET_REGION_INFO
> > 
> > The kernel will then return an offset value that yields a WC
> > mapping. It doesn't displace the normal non-WC mapping?
> > 
> > Arguably we should fixup the kernel to put the mmap cookies into a
> > maple tree so they can be dynamically allocated and more densely
> > packed.
> 
> 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

Jason