Re: [PATCH 9/9] vfio/pci: Add mmap() attributes to DMABUF feature

[Matt Evans <mattev@meta.com>]

Hi Alex,

On 13/05/2026 19:27, Alex Williamson wrote:
> 
> On Tue, 12 May 2026 18:51:40 +0100
> Matt Evans <mattev@meta.com> wrote:
>> On 11/05/2026 21:09, Alex Williamson wrote:
>>> I think the question of how we actually expand an arbitrary grab bag of
>>> "ATTRS" is the central question in whether we should implement the
>>> interface.
>>
>>> If we follow the direction I suggested for TPH, maybe this
>>> is just a VFIO_DEVICE_FEATURE_DMA_BUF_WC, where it supports only PROBE
>>> and SET, with SET taking only the dma-buf fd to implement the one-way
>>> promotion from UC -> WC.
>>>
>>> If we support a generic SET ATTRS feature, we really need to map out how
>>> flag bits are indicated as supported and how a user untangles failures
>>> from trying to set various attributes.  If we end up with a feature
>>> indicating each ATTR is available, we might as well have just
>>> implemented a feature for each attribute.  Thanks,
>>
>> Agreed, that's key.  Alhough, the aim of this patch is for attrs to be a
>> memory type enum rather than a bag of possibly-concurrent and
>> possibly-conflicting boolean flags.  Maybe 'memory attributes' would be
>> a better feature name.
>>
>> I'm not sure about the feature-per-attribute.  Say we do a
>> VFIO_DEVICE_FEATURE_DMA_BUF_WC and then later support a second,
>> VFIO_DEVICE_FEATURE_DMA_BUF_UC_WEAK (like, say, Arm Device-nGRE).  Then
>> we have to specify that these two VFIO feature types actually
>> interact/override somehow.  I doubt we'll end up with a dozen but it's a
>> bit tiresome having a few features that interact.
>>
>> At least if it's a single DMA_BUF_MEMATTR feature taking an enum, we
>> just encode the N different (mutually-exclusive!) valid states and done.
>>    I don't feel having a new feature for each keeps things simpler.
>>
>> Discovery of support for a specific future attribute is OK with a single
>> ATTR too; we can take an enum attribute argument to a GET and -ENOTSUPP
>> for any we don't like.
>>
>> (We could also add orthogonal DMABUF flags (can't think of a good
>> example...) but I'd suggest _those_ as semantically-grouped different
>> features, with the same issues of specifying conflicting cases versus
>> existing features.)
> 
> I think the GET behavior you're proposing is a bit counter-intuitive, if
> not abusive of the interface, but I do agree that if the feature is
> SET'ing a single value and not a group of independent flags, that we
> can probably rely more on a try-and-fail model rather than advertising
> each supported value as a separate feature.
>
> For example, the user has some list of compatible attributes ordered
> from most to least desirable, they try each in order until one works,
> or none work and they decide whether that's ok.
> 
> For GET, if we implement it, I think it should report the current
> attribute, mirroring SET.  We could almost get away without implementing
> it, but I do worry about the case of nvgrace-gpu, where it might be
> interesting for the user to see that the default attribute could be WB
> rather than UC.

I'd come to the same conclusion yesterday when implementing it. :)

GET just returns the current value, SET gives ENOTSUPP if the provided 
value isn't supported.

I haven't done much thinking on mechanisms for overriding the default 
value, but a sub-driver could add that via some hook from 
vfio_pci_core_feature_dma_buf().

> Where does the user derive the enum value?  Are we defining our own or
> is it a system header defined enum?  I'm curious if/how we're going to
> handle architecture specific attributes.  Thanks,

Good question.  There doesn't seem to be a suitable existing enum so I 
defined a new set (mirroring existing pgprot_*() semantics), in the same 
vfio.h/UAPI place as this patch.

The set could be extended in future to add some kind of "base vs 
arch-specific" grouping if we want to support arch-specific types like 
that hypothetical example arm64 'UC_WEAK' above.  (The feature param's a 
u32, so steal top byte for extension group_id?)

For the base set of types, they should at most follow the set of 
IO-related pgprot_*() types (whose names are a bit of an awkward fit 
across architectures but they're used consistently).  I've revisited the 
names to make them consistent with pgprot_*().  For sake of keeping the 
huge enum names smaller, abbreviated slightly:

pgprot_noncached()    -> VFIO_DEVICE_FEATURE_DMA_BUF_MEMATTR_NC (*)
pgprot_writecombine() -> VFIO_DEVICE_FEATURE_DMA_BUF_MEMATTR_WC
pgprot_device()       -> VFIO_DEVICE_FEATURE_DMA_BUF_MEMATTR_DEV

  *: Was UC in the v1 patch, which makes more sense as a memory type
     name, but consistency with pgprot_* is better.

But, I was thinking to support just the NC default and WC option in this 
series.  Does anyone feel strongly about needing pgprot_device() right 
now?  For external PCIe functions it'll behave the same as the NC type 
(even on arm64) so I don't think it's critical to add yet.

At this stage feels like we should get more field experience before 
adding more values/a scheme for arch-specific values so I'm keen on NC + 
WC for now, WDYT?


Matt