Re: [PATCH v2 1/3] s390/pci: Preserve FMB state in device re-enablement

[Omar Elghoul]

On 6/3/26 6:50 AM, Niklas Schnelle wrote:

> On Tue, 2026-05-19 at 18:42 -0400, Omar Elghoul wrote:
> > Introduce a function zpci_fmb_reenable_device() that checks for the state
> > of the FMB and reuses the same buffer where appropriate. If FMB was not
>                                                               ^ the
Acked
> > previously enabled, it enables it for the device. Call this function during
> > a zPCI device re-enablement, which in turn implicitly ensures that the FMB
> > is enabled for host devices during their KVM registration.
> >
> > This function also clears out the software counters, so that a program
> > resetting an FMB would see all its counters restart from zero as expected.
> > The function to clear the software counters is also separated into a static
> > function as it is now reused in both zpci_fmb_enable_device() and
> > zpci_fmb_reenable_device().
> While the commit message starts in the correct imperative voice it then
> drifts to a passive voice in the last sentence. "is also separated
> into…", "it is now reused".
>
> Better:
> "Besides re-enabling the FMB itself in zpci_fmb_reenable_device() also
> clear out the software counters, such that a program resetting an FMB
> sees all counters start from zero as expected. Separate this clearing
> of software counters out into zpci_fmb_clear_iommu_ctrs() and reuse it
> in zpci_fmb_enable_device() and zpci_fmb_reenable_device()."
Noted, I will amend this.
> > Signed-off-by: Omar Elghoul <oelghoul@xxxxxxxxxxxxx>
> > ---
> >   arch/s390/include/asm/pci.h |  1 +
> >   arch/s390/pci/pci.c         | 75 +++++++++++++++++++++++++++++--------
> >   2 files changed, 61 insertions(+), 15 deletions(-)
> --- snip ---
> > +}
> > +
> > +/* Modify PCI: Set PCI function measurement parameters */
> > +int zpci_fmb_enable_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;
> > +
> > +	if (zdev->fmb || sizeof(*zdev->fmb) < zdev->fmb_length)
> > +		return -EINVAL;
> > +
> > +	zdev->fmb = kmem_cache_zalloc(zdev_fmb_cache, GFP_KERNEL);
> > +	if (!zdev->fmb)
> > +		return -ENOMEM;
> > +	WARN_ON((u64) zdev->fmb & 0xf);
> >   
> > +	zpci_fmb_clear_iommu_ctrs(zdev);
> >   
> >   	fib.fmb_addr = virt_to_phys(zdev->fmb);
> I think there is still some potential for sharing the actual enable
> code between zpci_fmb_enable_device() and zpci_fmb_reenable_device()
> e.g. using a static zpci_fmb_do_enable(struct zpci_dev *zdev) helper.
> I think that still makes it clearer what's happening too.
Acked as well, it would definitely be easier to read. Will do in v3.
> >   	fib.gd = zdev->gisa;
> > @@ -227,6 +232,41 @@ int zpci_fmb_disable_device(struct zpci_dev *zdev)
> >   	}
> >   	return cc ? -EIO : 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);
> Sashiko correctly notes a pre-existing issue in that
> zpci_fmb_enable_device() is called without holding zdev->fmb_lock in
> pcibios_enable_device() and analogously zpci_fmb_disable_device() in
> pcibios_disable_device(). This should also be caught by lockdep if we
> had lockdep_assert_held() was in zpci_fmb_enable_device() respectively
> zpci_fmb_disable_device(). Would you mind adding a minimal fix patch
> for this pre-existing issue in your series? The fix should add the
> locking in the pcibios calls as well as add lockdep_assert_held(). Also
> don't forget a Fixes tag and Cc stable.
Sure thing, I will add a fix patch in v3.

Thanks.
> > +
> > +	if (!zdev->fmb)
> > +		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;
> > +
> > +	zpci_fmb_clear_iommu_ctrs(zdev);
> > +
> > +	fib.fmb_addr = virt_to_phys(zdev->fmb);
> > +	cc = zpci_mod_fc(req, &fib, &status); /* Re-enable function measurement */
> > +	if (cc) {
> > +		kmem_cache_free(zdev_fmb_cache, zdev->fmb);
> > +		zdev->fmb = NULL;
> > +		return -EIO;
> > +	}
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(zpci_fmb_reenable_device);
> >   
> The change itself makes sense to me.
>
> Thanks,
> Niklas