Re: [PATCH v2 2/3] vfio-pci/zdev: Add VFIO FMB device feature

[Alex Williamson]

On Wed, 3 Jun 2026 14:26:30 -0400
Omar Elghoul <oelghoul@xxxxxxxxxxxxx> wrote:

> On 6/3/26 11:55 AM, Alex Williamson wrote:
> > On Wed, 3 Jun 2026 08:35:43 -0400
> > Omar Elghoul <oelghoul@xxxxxxxxxxxxx> wrote:
> >   
> >> On 6/2/26 6:24 PM, Alex Williamson wrote:  
> >>>
> >>> Why does the user need to be able to control these?  
> >> We want the user (e.g. QEMU) to be able to control these so that when a
> >> guest enables or disables the FMB, this state gets cascaded to the host and
> >> all the way to the firmware.  
> >>>
> >>> Doesn't allowing the user to disable FMB remove guaranteed host-based
> >>> monitoring?  
> >> Yes it does, but this one isn't an oversight and is intentional behavior
> >> to achieve the functionality mentioned above. The host-based monitoring is
> >> not necessarily guaranteed and is treated as a device-specific state, so it
> >> makes sense in the case of passthrough to have that state reflect the state
> >> of the guest that is actually using the device.  
> > 
> > If we really need a SET for enable/disable, I think it should be a
> > separate feature.  It really makes no sense to pass a giant structure
> > into a SET operation to look at the state of one flag bit.
> > 
> > [...]
> > 
> > Hmm, I also see fmb_length in VFIO_DEVICE_INFO_CAP_ZPCI_BASE.  If we
> > have that, do we really need structured data in the GET feature?  Maybe
> > GET just provides a user pointer and the raw fmb data is copied to it.  
> 
> If we did this and passed just flags, a user ptr, and possibly a buffer
> length field, what would you think of leaving them in one feature? This
> way, the SET case would have possibly 8 or 16 bytes of overhead rather
> than the entire FMB structs, but would still keep the uAPI simple enough
> by avoiding multiple VFIO features for the same firmware feature.

It doesn't seem the GET needs either flags or buffer length.  The data
is opaque through vfio, so there's nothing to flag.  The buffer size is
at best a sanity check, it has no actual bearing on the copy to user
buffer.  We're not writing to a variable length ioctl buffer, we're
writing out to the user pointer.  The feature only needs to be
consistent that it copies no more than fmb_length.

The combined SET/GET that perform different actions especially stands
out because of the structure, but I don't think making the structure
size more manageable resolves that they do very different things.  I
think the implementation is also much easier if GET simply dumps the
FMB to the user pointer and SET takes only a scalar enable/disable
value, ie. a fundamental type that's handled as a bool.  Thanks,

Alex