Hi Alex,
On 13/05/2026 19:27, Alex Williamson wrote:
On Tue, 12 May 2026 18:51:40 +0100
Matt Evans <[email protected]> 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
