Re: [PATCH v8 2/4] s390/pci: Reuse FMB buffer and preserve state in device re-enablement

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

On 10/5/26 3:53 PM, Niklas Schnelle wrote:
> On Mon, 2026-10-05 at 11:45 -0400, Omar Elghoul wrote:
>> Don't free the FMB buffer when disabling measurement in
>> zpci_fmb_disable_device(). Instead, make the buffer persistent for the
>> lifetime of the device and reuse it across enable/disable cycles. Defer
>> freeing the buffer until teardown in zpci_release_device().
>>
>> To support the persistent buffers, add the fmb_enabled bool to struct
>> zpci_dev to decouple whether FMB is enabled from whether the buffer has
>> been allocated. Audit the only consumer of zdev->fmb as a liveness check
>> and update it to reflect this change.
>>
>> Introduce the function zpci_fmb_reenable_device() to ensure that the FMB
>> is enabled. If it was already enabled, disable it, zero the counters,
>> and re-enable it. This allows the function to be used in both first-time
>> enabling and re-enabling measurement. Call it in zpci_reenable_device()
>> to preserve the FMB enablement if it had been implicitly disabled by
>> firmware in zpci_disable_device().
> 
> I think this causes a sequencing error in zpci_hot_reset_device().
> First the device gets disabled via zpci_disable_device(). This
> implicitly disables the FMB but keeps zdev->fmb_enabled set. Then we
> call zpci_fmb_reenable_device() in zpci_reenable_device(). Since zdev-
>> fmb_enabled is set we don't first enable the FMB and instead go
> directly to disabling it but that is wrong since the FMB is already
> disabled as a side effect of the CLP Set PCI Function (Disable) in
> zpci_disable_device().
> 
> Also, and I think Gerd mentioned this before, there is a disconnect in
> semantics between zpci_fmb_reenable_device() and zpci_reenable_device()
> that is quite confusing. While zpci_reenable_device() re-enables the
> device with existing interrupts and I/O address translations, after it
> was disabled, zpci_fmb_reenable_device() on the other hand does a
> disable and then enable cycle.
> 
> I think the idea here is that zdev->fmb_enabled tries to track whether
> the FMB is supposed to be enabled rather than if it is enabled.  This
> makes some sense since the FMB can get disabled by the device entering
> the error state or a zpci_disable_device() and we want to know if we
> need to re-enable it at the re-enable of the device.
> 
> Importantly, unlike the disablement of a device we always initiate the
> enablement. But then we can't try to disable the FMB without knowing if
> it was already disabled. I think a possible solution for this would be
> to have zpci_fmb_reenable_device() mean that we know that the FMB is
> disabled but should be enabled, which we know when we re-enable the
> device and zdev->fmb_enabled is set. Of course then it doesn't do a
> disable but only an enable despite zdev->fmb_enabled already being set,
> Then zpci_fmb_enable_device() on the other hand sets the flag initially
> and then uses zpci_fmb_reenable_device() or a shared helper. Of course
> we would then have to properly document zdev->fmb_enabled as being a
> the target rather than current state.

I agree with your insight and I'd be happy to follow this approach, but
I think this can cause FMB consumers to read stale snapshots (e.g. if
the device was disabled due to an error state or similar but fmb_enabled
is true). What would you think of leaving fmb_enabled as-is to indicate
whether FMB is actually enabled, and then introducing a second bool,
maybe something like fmb_needed, to track the user's intent and whether
we should call zpci_fmb_reenable_device() from zpci_reenable_device()?

This way, a successful zpci_fmb_enable_device() sets both flags, and
zpci_fmb_disable_device() clears both. zpci_disable_device() should
only clear fmb_enabled and leave fmb_needed as-is, allowing us to track
the implicit disablement by the firmware. This will make fmb_enabled
represent the actual firmware truth, and it becomes a reliable liveness
check for the FMB consumers (debugfs and vfio, for now.)

As for zpci_reenable_device(), it would check fmb_needed and if set,
call zpci_fmb_reenable_device(), since we'd already know by that point
that the FMB was implicitly disabled by firmware.

Thanks

> 
> Thanks,
> Niklas