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

[Gerd Bayer]

On Wed, 2026-09-23 at 12:37 -0400, Omar Elghoul wrote:
> On 9/23/26 11:09 AM, Gerd Bayer wrote:
> > On Tue, 2026-09-22 at 15:51 -0400, Omar Elghoul wrote:
> > > Introduce the function zpci_fmb_reenable_device() that checks the state
> > > of function measurement and ensures it is enabled. Reset the counters to
> > > zero, disable, and re-enable the FMB if it was already enabled. Call
> > > this function from zpci_reenable_device().
> > > 
> > > Don't free the FMB buffer during disabling and reuse it when re-enabling
> > > measurement. Instead, free the buffer upon device teardown, allowing the
> > > same buffer to be reused in the enable path and add the bit fmb_enabled
> > > to struct zpci_dev. Audit the only consumer of zdev->fmb and update it
> > > to reflect the change in semantics.
> > 
> > While I understand how this evolved, this commit message reads upside-
> > down for me. Shouldn't we consider the changes described in this second
> > part of the commit message as a preparatory step for the introduction
> > of zpci_fmb_reenable_device() and put this paragraph first - or even
> > into a separate commit of its own?
> 
> That's a fair point, the commit message can be restructured to describe
> exactly what the function does first, and then afterwards describe where
> we're calling it and why.

Sounds good.

> I also think having everything in one commit is necessary because this
> commit changes the semantics of zdev->fmb, where the old code used it
> as both the buffer and also as an FMB enablement check. The latter check
> is no longer valid after this commit, unless we want to add a separate
> commit that just adds the fmb_enabled bool, which I thought was a little
> overkill.

That's fine with me, no hard feelings.

[...]

> > 
> > > +		memset(zdev->fmb, 0, sizeof(*zdev->fmb));
> > > +	}
> > >   
> > >   	/* reset software counters */
> > >   	spin_lock_irqsave(&zdev->dom_lock, flags);
> > > @@ -199,11 +204,11 @@ int zpci_fmb_enable_device(struct zpci_dev *zdev)
> > >   	fib.fmb_addr = virt_to_phys(zdev->fmb);
> > >   	fib.gd = zdev->gisa;
> > >   	cc = zpci_mod_fc(req, &fib, &status);
> > > -	if (cc) {
> > > -		kmem_cache_free(zdev_fmb_cache, zdev->fmb);
> > > -		zdev->fmb = NULL;
> > > -	}
> > > -	return cc ? -EIO : 0;
> > > +	if (cc)
> > > +		return -EIO;
> > > +
> > > +	zdev->fmb_enabled = 1;
> > > +	return 0;
> > >   }
> > >   
> > >   /* Modify PCI: Disable PCI function measurement */
> > > @@ -215,7 +220,7 @@ int zpci_fmb_disable_device(struct zpci_dev *zdev)
> > >   
> > >   	lockdep_assert_held(&zdev->fmb_lock);
> > >   
> > > -	if (!zdev->fmb)
> > > +	if (!zdev->fmb_enabled)
> > >   		return -EINVAL;
> > >   
> > >   	fib.gd = zdev->gisa;
> > > @@ -224,13 +229,39 @@ int zpci_fmb_disable_device(struct zpci_dev *zdev)
> > >   	cc = zpci_mod_fc(req, &fib, &status);
> > >   	if (cc == 3) /* Function already gone. */
> > >   		cc = 0;
> > > +	if (cc)
> > > +		return -EIO;
> > >   
> > > -	if (!cc) {
> > > -		kmem_cache_free(zdev_fmb_cache, zdev->fmb);
> > > -		zdev->fmb = NULL;
> > > -	}
> > > -	return cc ? -EIO : 0;
> > > +	zdev->fmb_enabled = 0;
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(zpci_fmb_disable_device);
> > > +
> > > +int zpci_fmb_reenable_device(struct zpci_dev *zdev)
> > > +{
> > > +	u64 req = ZPCI_CREATE_REQ(zdev->fh, 0, ZPCI_MOD_FC_SET_MEASURE);
> > > +	struct zpci_fib fib = {0};
> > > +	u8 cc, status;
> > > +
> > > +	lockdep_assert_held(&zdev->fmb_lock);
> > > +
> > > +	if (!zdev->fmb_enabled)
> > > +		return zpci_fmb_enable_device(zdev);
> > > +
> > > +	fib.gd = zdev->gisa;
> > > +	cc = zpci_mod_fc(req, &fib, &status); /* Disable function measurement */
> > > +
> > > +	/* Unlike in zpci_fmb_disable_device(), cc == 3 is not a valid state here
> > > +	 * because we are re-enabling function measurement for the same function
> > > +	 * handle.
> > > +	 */
> > > +	if (cc)
> > > +		return -EIO;
> > > +
> > > +	zdev->fmb_enabled = 0;
> > > +	return zpci_fmb_enable_device(zdev);
> > >   }
> > > +EXPORT_SYMBOL_GPL(zpci_fmb_reenable_device);
> > 
> > I see a little imbalance of the semantics of "reenable" in
> > zpci_fmb_reenable_device() vs. zpci_reenable_device():
> > zpci_reenable_device() "just" enables + registers existing data
> > structures with the underlying system - while
> > zpci_fmb_reenable_device() does both the disablement + the enablement.
> 
> Strictly speaking, the disablement step may not be necessary, provided
> firmware starts the counters at zero upon changing an FMB address, which
> does seem to be true in practice. The architecture doesn't explicitly
> require that though, so I thought it's a reasonable safeguard to use it
> as an intermediate step that signals firmware to stop counting before we
> immediately restart measurement after.

Initially, my point was purely "semantics":
If it is enough for zpci_reenable_device() to do only "enabling"-kind
of steps, why is zpci_fmb_reenable_device() then also doing some
"disabling" (under certain conditions). In my eyes the "reenable" was
actually a "conditional-toggling-on".

And this then led me to checking the paths leading into
zpci_reenable_device(). I found that all paths would call
zpci_disable_device() before and that led me to the next question:

> > Since zpci_disable_device() includes the disablement of FMB per
> > architecture, I wonder if it would suffice to set zdev->fmb_enabled = 0
> > in that function, and drop the explicit disable FMB there?
> 
> The semantics of the FMB re-enable function were intended to allow us
> to re-enable the FMB when we re-enable the device after FMB was
> implicitly disabled via zpci_disable_device(), like you said. Prior to
> this patch, there was a sort of "limbo" state where firmware thinks FMB
> is disabled, but the kernel is unaware of it because it was implicit.
> 
> For that same reason, I would prefer to not touch zpci_disable_device()
> at all, neither explicitly disabling FMB nor setting fmb_enabled to 0.
> The purpose of fmb_enabled variable is to allow us to restore the
> original FMB enablement when we re-enable the device, and so we want to
> preserve it here [1].

OK, I see. The whole point was to preserve the FMB enabled state over
disable/enable sequences on a zdev: Re-enable if (and only if) it was
enabled before the sequence. So I agree, you must not set zdev-
>fmb_enabled = 0 in zpci_disable_device(). But you can trust firmware
to stop updating the FMB buffer (after the ominous grace-period) after
zpci_disable_device() ran - the zpci_mod_fc() to set FMB to 0 is done
implicitly in clp_disable_fh().

And you wanted to reuse the zdev->fmb buffer if there ever was one
allocated for the zdev. It just occurred to me, what good is the whole
zdev_fmb_cache if the life-time of struct zpci_fmb buffers becomes
almost as static as struct zpci_dev (short of those that never leave
"STANDBY"). Couldn't we just kzalloc() a struct zpci_fmb right in
zpci_create_device()?


> 
> Thanks
> 
> [1] 
> https://lore.kernel.org/all/dae3c7cd-21aa-4263-bec5-792b018d21e2@xxxxxxxxxxxxx/
> 
> 

[...]

Thank you,
Gerd