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

[Matt Evans <mattev@meta.com>]

Hi Alex,

On 11/05/2026 21:09, Alex Williamson wrote:
> 
> On Mon, 11 May 2026 16:30:39 +0100
> Matt Evans <mattev@meta.com> wrote:
> 
>> Hi Alex, Leon,
>>
>> On 27/04/2026 15:36, Alex Williamson wrote:
>>>
>>> On Sun, 26 Apr 2026 13:52:15 +0300
>>> Leon Romanovsky <leon@kernel.org> wrote:
>>>    
>>>> On Fri, Apr 24, 2026 at 03:31:53PM -0300, Jason Gunthorpe wrote:
>>>>> On Thu, Apr 16, 2026 at 06:17:52AM -0700, Matt Evans wrote:
>>>>>> A new field is reserved in vfio_device_feature_dma_buf.flags to
>>>>>> request CPU-facing memory type attributes for mmap()s of the buffer.
>>>>>> Add a flag VFIO_DEVICE_FEATURE_DMA_BUF_ATTR_WC, which results in WC
>>>>>> PTEs for the DMABUF's BAR region.
>>>>>>
>>>>>> Signed-off-by: Matt Evans <mattev@meta.com>
>>>>>> ---
>>>>>>    drivers/vfio/pci/vfio_pci_dmabuf.c | 15 +++++++++++++--
>>>>>>    drivers/vfio/pci/vfio_pci_priv.h   |  1 +
>>>>>>    include/uapi/linux/vfio.h          | 12 +++++++++---
>>>>>>    3 files changed, 23 insertions(+), 5 deletions(-)
>>>>>
>>>>> Nice and simple
>>>>>
>>>>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>>>>>       
>>>>>> @@ -1549,8 +1551,12 @@ struct vfio_region_dma_range {
>>>>>>    struct vfio_device_feature_dma_buf {
>>>>>>    	__u32	region_index;
>>>>>>    	__u32	open_flags;
>>>>>> -	__u32   flags;
>>>>>> -	__u32   nr_ranges;
>>>>>> +	__u32	flags;
>>>>>> +	/* Flags sub-field reserved for attribute enum */
>>>>>> +#define VFIO_DEVICE_FEATURE_DMA_BUF_ATTR_MASK		(0xfU << 28)
>>>>>> +#define VFIO_DEVICE_FEATURE_DMA_BUF_ATTR_UC		(0 << 28)
>>>>>> +#define VFIO_DEVICE_FEATURE_DMA_BUF_ATTR_WC		(1 << 28)
>>>>>> +	__u32	nr_ranges;
>>>>
>>>> Alex,
>>>>
>>>> The TPH proposal extends the flags field in a similar way, but I suggested
>>>> a different approach to conserve bits. At the moment, we spend three bits
>>>> on a single feature, which feels wasteful.
>>>>
>>>> What do you think?
>>>> https://lore.kernel.org/all/20260409120415.GF86584@unreal/
>>>
>>> I already proposed a very different interface for TPH that decouples
>>> the dma-buf creation from setting the TPH values:
>>>
>>> https://lore.kernel.org/all/20260423132016.4a25e074@shazbot.org/
>>>
>>> This is overall less intrusive than the TPH change proposed, but it
>>> could still make sense to align this as an operation on the dma-buf,
>>> that can be probed as a separate feature.  Thanks,
>>
>> I'll add a VFIO_DEVICE_FEATURE_DMA_BUF_ATTRS in a v2 instead to get in
>> line with the TPH work, no worries.
>>
>> For the benefit of future hackers, how would you describe the criteria
>> for adding flags to this existing field?  What hypothetical feature
>> characteristics would be appropriate?  (Maybe it's that these attrs &
>> TPH add scalar fields in several bits rather than a simple boolean.)
>> Two of us have independently added something that's turned out to be
>> inapproriate so some guidance would be good.
> 
> 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.)


Cheers,


Matt