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

[Niklas Schnelle <schnelle@linux.ibm.com>]

On Mon, 2026-10-05 at 18:05 -0400, Omar Elghoul wrote:
> 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). 
> 

I'm not sure reading stale data is really an issue. When this happens
the device is disabled and won't see updates anyway. Also there is a
timestamp in the FMB so it's transparent how old the data is. And just
based on the interface the same FMB can be re-read between updates so
if you don't want duplicate data you'd need to check for changed
timestamp anyway.

> 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.

This feels like an overcomplication to me and "fewer stale FMB reads"
doesn't seem worth it. Even if zpci_disable_device() clears fmb_enabled
I think it wouldn't be the true state because the plaform might have
disabled it e.g. in an error event and then we would have to litter
those places with clearing the flag too instead of just using the
existing zpci_device_reenable() which gets called when we get out of a
platform disable. I do like the fmb_needed name though or maybe even
better fmb_requested.

Thanks,
Niklas