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

[Omar Elghoul <oelghoul@linux.ibm.com>]

On 6/3/26 3:24 PM, Alex Williamson wrote:
> On Wed, 3 Jun 2026 14:26:30 -0400
> Omar Elghoul <oelghoul@linux.ibm.com> wrote:
> 
>> On 6/3/26 11:55 AM, Alex Williamson wrote:
>>> On Wed, 3 Jun 2026 08:35:43 -0400
>>> Omar Elghoul <oelghoul@linux.ibm.com> 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
>