Re: [RFC PATCH 0/9] vfio: Introduce mmap maple tree

[Jason Gunthorpe <jgg@ziepe.ca>]

On Thu, Aug 07, 2025 at 01:06:05PM -0600, Alex Williamson wrote:

> So to a large extent I think we're causing our own grief here and I do
> agree that if we created an API that allows new regions to be created
> as lightweight (mmap-only) aliases of other regions, then we could just
> re-use REGION_INFO with the new region index, get the offset/mmap
> cookie, return -ENOSPC when we run out of indexes, and implement a
> maple tree to get a more compact region space as a follow-on.

That still needs to dynamically create mmap cookies - which is really the
whole problem here. It is not so easy to just change the existing code
with hardwired math converting pgoff to indexes to support multiple
indexes.

> > Well, we want to be able to WC map. Introducing "more cookies" again is
> > just one way to get there. How do you create those cookies ? Upon
> > request or each region automatically gets multiple with different
> > attributes ? Do they represent entire regions or subsets ? etc... 

It doesn't matter. Fixing how mmap works internally lets you use all
of those options.

> 
> > > >  * What I originally proposed ages ago when we discussed it at LPC
> > > > which is to have an ioctl to create "subregions" of a region with
> > > > different attributes. This means creating a new index (and a new
> > > > corresponding "cookie") that represents a portion of an existing
> > > > region
> > > > with a different attribute set to be later used by mmap.  
> > > 
> > > I never liked this, and it is still creating more cookies but now
> > > with
> > > weird hacky looking uapi.  
> > 
> > "weird hacky looking" isn't a great way of understanding your
> > objections :-) Yes, it's a bit more complicated, it allows to break
> > down BARs basically into sub-regions with different attributes which is
> > fairly close to what users really want to do, but it does complexify
> > the UAPI a bit.

I don't think we need sub-regions, it is too complicated in the kernel
and pretty much useless.

> > That said I'm not married to the idea, I just don't completely
> > understand the alternative you are proposing... 
> 
> Obviously Jason can correct if the interpretation I gleaned from the
> previous thread above is incorrect, but I don't really see that new
> region indexes are weird or hacky.  They fit into the REGION_INFO
> scheme, they could be managed via DEVICE_FEATURE.

I think I said before, adding more region indexes just makes a PITA
for the driver to manage this.

Indexes should refer to the physical object, the BAR in PCI. This is
easy for the drivers to manage, any easy for userspace to understand.

If you make dynamic indexes then every driver needs its own scheme to
map them to physical indexes and we end up re-inventing the maple tree
stuff without the cleanup or generality it brings.

> > > >  * A simpler approach which is to have an ioctl to change the
> > > > attributes over a range of a region, so that *any* mapping of that
> > > > range now uses that attribute. This can be done completely
> > > > dynamically
> > > > (see below).  
> > > 
> > > And I really, really don't like this. The meaning of a memmap cookie
> > > should not be changing dynamically. That is a bad precedent.  
> > 
> > What do you mean "a cookie changing dynamically" ? The cookie doesn't
> > change. The attributes of a portion of a mapping change based on what
> > userspace requested.

Exactly. You have changed the underlying meaning of the cookie
dynamicaly.

> > The biggest advantage of that approach is that it completely precludes
> > multiple conflicting mappings for a given region (at least within a
> > given process, though it might be possible to extend it globally
> > if we

It doesn't. It just makes a messy uapi. At the time of mmap the vma
would stil have to capture the attributes (no fault by fault!) into
the VMA so we will see real users doing things like:

 set to wc(cookie)
 mmap(cookie + XXX)
 set to !wc(cookie)
 mmap(cookie + YY)

And then if you try to debug this all our file/vma debug tools will
just show cookie everywhere with no distinction that some VMAs are WC
and some VMAs are !WC.

Basically, it fundamentally breaks how pgoff is supposed to work here
by making its meaning unstable.

> > want) which has been a concern expressed last time we talked about this
> > by the folks from the ARM world.
> 
> This precludes that a user could simultaneously have mappings to the
> same device memory with different mmap attributes,

Indeed, that is required for most HW. mlx5 for example has BARs that
mix WC and non WC access modes. There are too few BARs for most HW to
be able to dedicate an entire BAR to WC only.

> > > The uAPI I prefer is a 'get region info2' which would be simplified
> > > and allow userspace to pass in some flags. One of those flags can be
> > > 'request wc'.  
> > 
> > So talking of "weird hacky looking" having something called "get_info"
> > taking "request" flags ... ahem ... :-)
> 
> Yup...

We've already confused returning the mmap cookie and the other
information about the region. If you want to have flags to customize
what mmap cookie is returned, this is the simplest answer.

> > If what you really mean is that under the hood, a given "index"
> > produces multiple "regions" (cookies) and "get_info2" allows to specify
> > which one you want to get info about ?

There is only one index. You can ask for different mmap options, and
pgoff space for them is created dynamically.

A "region" is not a mmap cookie, a region can have multiple mmap cookies.

> > I disagree ... I find it actually cumbersome but we can just agree to
> > disagree here and as I said, I don't care that much as long as there is
> > no fundamental reason why it wouldn't work in the end.
> 
> I'm not a fan of the REGION_INFO2 idea either.  A new region index that
> provides an alias to another region with different mmap semantics is
> much more intuitive in our existing UAPI, imo.

It would not be another region index. That is the whole point. It is
another pgoff for an existing index.

> > Talking of cookie space, one thing we do need to preserve is the
> > natural alignment to the BAR size. Userspace *will* do "|" instead of
> > "+" on top of a cookie when mmap'ing (beyond mmap own alignment
> > requirements).

I think that's just wrong userspace, sorry. :(

Still, we want to do this anyhow as the VMA alignment stuff Peter was
working on also requires pgoff alignment.

> > > > Now, within the context of VFIO PCI, since we use a fault handler,
> > > > it
> > > > doesn't even have to be done before mmap, we can just dynamically
> > > > fault
> > > > a given page with the right attributes (and zap mappings on
> > > > attributes
> > > > changes). I would go down that path personally and generalize the
> > > > fault
> > > > handler to all VFIO implementations.  
> > > 
> > > Even worse. fault by fault changing of attributes? No way, that's a
> > > completely crazy uAPI!  
> > 
> > Why ? first userspace doesn't know or see it happens fault by fault
> > (and it could be full established at mmap time in fact, but fault time
> > makes the implementation a lot easier).
> > 
> > Here you are conflating implementation details with "uAPI" ... uAPI is
> > what is presented to userspace, which in this case is "set the
> > attributes for this portion of a region". Then at map time, that
> > portion of the region gets the requested attributes. There is nothing
> > in the uAPI that carries the fact that it happens at fault time.
> > 
> > You keep coming up with "ugly", "crazy" etc... without every actually
> > spelling out the technical pro/cons that would actually substantiate
> > those adjectives. Basically anything that isn't your 
> > get_region2 is "crazy" or "ugly" ... NIH syndrome ?

This is not how the mm ever works *anywhere* in the main kernel, the
only reason I can see you are propsing this because it avoids doing
the cleanup work.

It makes it impossible for userspace to get both WC and non-WC
mappings. It makes it indeterminate what behavior userspace gets at
every store operation. Real HW like mlx5 would need mixed WC/!WC on
the same bar, so I view this as an entirely bad uAPI.

> > > I don't know what this is about, I don't want to see APIs to slice up
> > > regions. The vfio driver should report full regions in the pgoff
> > > space, and VMAs should be linked to single maple tree ranges only. If
> > > userspace wants something weird it can call mmap multiple times and
> > > get different VMAs.  
> > 
> > Why ? What are the pros/cons of each approach ?

Extra complexity in the kernel, no usecase that isn't already handled
by mmap directly.

> > The big advantages of that latter approach is that it's very clear and
> > simple for userspace (this bit of the BAR is meant to be WC) and
> > completely
> > avoids the multiple mapping problem.

What is the "multiple mapping problem" ? We've discussed this
extensively with ARM when we added WC support to KVM and I think we
have a general agreement that multiple mappings are not something the
kernel needs to actively prevent, in the limited case of WC and !WC.

> >The inconvenient is that we have to generalize the fault handler
> > mechanism to all backends, and it makes the multi-mapping
> > impossible (in the maybe possible case where might want to allow
> > it in some cases).
> > 
> > Overall I find a lot of putting cart before horses here. Can we first
> > agree
> > on a clear definition of the uAPI first, then we can figure out the
> > implementation details ?
> 
> +1.  Thanks,

All the uAPI proposals I've seen are all trying to work around the
current state of the code.

This is broadly what I've proposed consistently since the beginning,
adjusted for the various remarks since:

struct vfio_region_get_mmap {
	__u32	argsz;
	__u32	region_index; // only one, no aliases
	__u32   mmap_flags; // Set WC here

	__aligned_u64 region_size;
	__aligned_u64 fd_offset;
};

struct vfio_region_get_caps {
	__u32	argsz;
	__u32	region_index;

	__u32	region_flags; // READ/WRITE/etc
	__aligned_u64 region_size;
	__u32	cap_offset;	/* Offset within info struct of first cap */
};

Alex, you pointed out that the parsing of the existing
VFIO_DEVICE_GET_REGION_INFO has made it non-extendable. So the above
two are creating a new extendable version that are replacements.

To avoid the naming confusion we have a specific ioctl to get
mmap'able access, and another one for the cap list. I guess this also
gives access to read/write so maybe the name needs more bikeshedding.

Compared to the existing we add a single new concept, 'mmap_flags',
which customizes how the fd_offset will behave with mmap. The kernel
will internally de-duplicate region_index/mmap_flags -> fd_offset.

There is still one index per physical object (ie BAR) in the uAPI.

We get one cookie that describes the VMA behavior exactly and immutably.

The existing VFIO_DEVICE_GET_REGION_INFO is expressed in terms of the
above two operations with mmap_flags = 0.

No new subregion concept. No alias region indexes. No changing the
meaning of returned cookies.

We simply allow the userspace to request a mmap cookie for a region
index that will always cause mmap to create a VMA with WC pgrot.

If we someday want cachable, or ARM's DEVICE_GRE or whatever we can
trivially add more flags to mmap_flags and userspace can get
more cookies.

If we later decide we need to solve the ARM multi-device issue then we
can cleanly extend an additional start/len to vfio_region_get_mmap
which can ensure mmaps cookies are disjoint. This is not subregions,
or new regions, this is just a cookie with a restriction.

In terms of implementation once you do the maple tree work that
Mahmoud started we end up with vfio_region_get_mmap allocating a
struct vfio_mmap for each unique region_index/mmap_flags and returning
it to userspace.

All the drivers will use the struct vfio_mmap everywhere instead of
trying to decode pgoff to index with macros. Inside the vfio_mmap we
store the index for the driver to use directly. The driver has a
simple implementation, one index mapped to one physical resource. A
pgprot value in the struct vfio_mmap to specify how to create the VMA.

No dynamic driver aware regions or subregions.

Jason