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