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

[Omar Elghoul]

On 6/3/26 3:24 PM, Alex Williamson wrote:
> 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,

Acked, I will keep this in mind for future versions.

Thanks.

> Alex