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

[Omar Elghoul]

On 10/12/26 5:22 AM, Gerd Bayer wrote:
> On Wed, 2026-10-07 at 09:59 -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().
>
> I lost track if this was discussed before: Would it make sense to break
> this out into a separate patch, so that memory allocation/life-time
> questions are decoupled from function-measurement preservation?

I think we did discuss that before and I expressed a preference for
leaving them in together, but since this patch seems to be growing I
guess splitting it would make review easier.

> > Introduce the bool fmb_requested to struct zpci_dev to track whether FMB
> > enablement has been requested. This decouples tracking the enablement
> > from whether the buffer had been previously allocated, allowing us to
> > account for implicit disablement by firmware in zpci_disable_device().
> > Audit the only consumer of zdev->fmb and update its liveness check to
> > reflect this change.
> >
> > Separate the buffer allocation and measurement enablement step into the
> > new function zpci_fmb_reenable_device(), which allocates the FMB buffer
> > on first use, zeroes it on reuse, resets the software IOMMU counters,
> > and enables measurement. Make zpci_fmb_enable_device() delegate to this
> > function, and call the latter from zpci_reenable_device() when
> > fmb_requested is true.
> >
> > Signed-off-by: Omar Elghoul <oelghoul@xxxxxxxxxxxxx>
> > ---
> >   arch/s390/include/asm/pci.h |   2 +
> >   arch/s390/pci/pci.c         | 106 +++++++++++++++++++++++-------------
> >   arch/s390/pci/pci_debug.c   |   2 +-
> >   3 files changed, 70 insertions(+), 40 deletions(-)
> >
> > diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> > index 88a125b92bdd..77c8c49ba6c8 100644
> > --- a/arch/s390/include/asm/pci.h
> > +++ b/arch/s390/include/asm/pci.h
> > @@ -208,6 +208,7 @@ struct zpci_dev {
> >   	struct zpci_fmb *fmb;
> >   	u16		fmb_update;	/* update interval */
> >   	u16		fmb_length;
> > +	bool		fmb_requested;	/* desired FMB enablement state */
>
> How about calling this "statistics_on" - derived from the sysfs
> attribute, to decouple this "user intent" even more from the actual ISA
> level enablement?

Sure.

> >   
> >   	u8		version;
> >   	enum pci_bus_speed max_bus_speed;
> > @@ -351,6 +352,7 @@ void zpci_remove_parent_msi_domain(struct zpci_bus *zbus);
> >   /* FMB */
> >   int zpci_fmb_enable_device(struct zpci_dev *);
> >   int zpci_fmb_disable_device(struct zpci_dev *);
> > +int zpci_fmb_reenable_device(struct zpci_dev *zdev);
> >   
> >   /* Debug */
> >   int zpci_debug_init(void);
> > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> > index c055a9ad0972..bda91d79bafd 100644
> > --- a/arch/s390/pci/pci.c
> > +++ b/arch/s390/pci/pci.c
> > @@ -167,43 +167,13 @@ int zpci_unregister_ioat(struct zpci_dev *zdev, u8 dmaas)
> >   /* 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_iommu_ctrs *ctrs;
> > -	struct zpci_fib fib = {0};
> > -	unsigned long flags;
> > -	u8 cc, status;
> > -
> >   	lockdep_assert_held(&zdev->fmb_lock);
> >   
> > -	if (zdev->fmb || sizeof(*zdev->fmb) < zdev->fmb_length)
> > +	if (zdev->fmb_requested || 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);
> > -
> > -	/* reset software counters */
> > -	spin_lock_irqsave(&zdev->dom_lock, flags);
> > -	ctrs = zpci_get_iommu_ctrs(zdev);
> > -	if (ctrs) {
> > -		atomic64_set(&ctrs->mapped_pages, 0);
> > -		atomic64_set(&ctrs->unmapped_pages, 0);
> > -		atomic64_set(&ctrs->global_rpcits, 0);
> > -		atomic64_set(&ctrs->sync_map_rpcits, 0);
> > -		atomic64_set(&ctrs->sync_rpcits, 0);
> > -	}
> > -	spin_unlock_irqrestore(&zdev->dom_lock, flags);
> > -
> > -
> > -	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;
> > +	zdev->fmb_requested = true;
> > +	return zpci_fmb_reenable_device(zdev);
>
> I'm not quite happy with "reenable" now becoming the "work horse" for
> enabling function measurement at the pci device. I append a suggested
> change at the end (since it's lengthy). My train of thought was:
> Introduce symmetric zpci_fmb_req_{dis|en}able_device() that toggle the
> "statistics_on" boolean, then call zpci_fmb_{dis|en}able_device() that
> does the actual zpci_mod_fc(). This allowed zpci_reenable_device() to
> call zpci_fmb_enable_device() if case of "statics_on", while the writes
> into sysfs would call zpci_fmb_req_{dis|en}able_device().
>
> And another after-thought, since the whole _device postfix on these
> function names seems redundant, how about the following names:
>
> zpci_request_fm_on()
> zpci_request_fm_off()
> zpci_enable_fm()
> zpci_disable_fm()

That's fine by me. It makes sense and as an added bonus, it makes it
easier to fix the issue you report where the state isn't correctly
preserved after zpcictl reset (please see below.)

> >   }
> >   
> >   /* Modify PCI: Disable PCI function measurement */
> > @@ -215,22 +185,68 @@ int zpci_fmb_disable_device(struct zpci_dev *zdev)
> >   
> >   	lockdep_assert_held(&zdev->fmb_lock);
> >   
> > -	if (!zdev->fmb)
> > +	if (!zdev->fmb_requested)
> >   		return -EINVAL;
> >   
> > +	zdev->fmb_requested = false;
> >   	fib.gd = zdev->gisa;
> >   
> >   	/* Function measurement is disabled if fmb address is zero */
> >   	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 0;
> > +}
> > +EXPORT_SYMBOL_GPL(zpci_fmb_disable_device);
> > +
> > +/* Re-enable PCI function measurement: clear the software IOMMU counters
> > + * and (re-)enable the FMB. This function is used to restore the desired
> > + * FMB enablement state when measurement may be implicitly disabled by
> > + * firmware (i.e., via CLP in zpci_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_iommu_ctrs *ctrs;
> > +	struct zpci_fib fib = {0};
> > +	unsigned long flags;
> > +	u8 cc, status;
> > +
> > +	lockdep_assert_held(&zdev->fmb_lock);
> > +
> > +	if (!zdev->fmb) {
> > +		zdev->fmb = kmem_cache_zalloc(zdev_fmb_cache, GFP_KERNEL);
> > +		if (!zdev->fmb)
> > +			return -ENOMEM;
> > +	} else {
> > +		/* reuse the same FMB buffer for as long the zdev lives */
> > +		memset(zdev->fmb, 0, sizeof(*zdev->fmb));
> >   	}
> > -	return cc ? -EIO : 0;
> > +
> > +	/* reset software counters */
> > +	spin_lock_irqsave(&zdev->dom_lock, flags);
> > +	ctrs = zpci_get_iommu_ctrs(zdev);
> > +	if (ctrs) {
> > +		atomic64_set(&ctrs->mapped_pages, 0);
> > +		atomic64_set(&ctrs->unmapped_pages, 0);
> > +		atomic64_set(&ctrs->global_rpcits, 0);
> > +		atomic64_set(&ctrs->sync_map_rpcits, 0);
> > +		atomic64_set(&ctrs->sync_rpcits, 0);
> > +	}
> > +	spin_unlock_irqrestore(&zdev->dom_lock, flags);
>
> I know this is pre-existing, but how about breaking out the buffer and
> counter preparation/clearing into a helper function?

I believe it has been done in a previous version of this series at which
point we had two callers for it. Since we don't have another caller now
I didn't think of splitting it up.

> > +
> > +	fib.fmb_addr = virt_to_phys(zdev->fmb);
> > +	fib.gd = zdev->gisa;
> > +	cc = zpci_mod_fc(req, &fib, &status);
> > +	if (cc)
> > +		return -EIO;
> > +
> > +	return 0;
> >   }
> > +EXPORT_SYMBOL_GPL(zpci_fmb_reenable_device);
> >   
> >   static int zpci_cfg_load(struct zpci_dev *zdev, int offset, u32 *val, u8 len)
> >   {
> > @@ -737,8 +753,14 @@ int zpci_reenable_device(struct zpci_dev *zdev)
> >   	}
> >   
> >   	rc = zpci_iommu_register_ioat(zdev, &status);
> > -	if (rc)
> > +	if (rc) {
> >   		zpci_disable_device(zdev);
> > +		return rc;
> > +	}
> > +
> > +	guard(mutex)(&zdev->fmb_lock);
> > +	if (zdev->fmb_requested)
> > +		zpci_fmb_reenable_device(zdev);
> >   
> >   	return rc;
> >   }
> > @@ -1003,6 +1025,12 @@ void zpci_release_device(struct kref *kref)
> >   	if (zdev->has_resources)
> >   		zpci_cleanup_bus_resources(zdev);
> >   
> > +	if (zdev->fmb) {
> > +		zdev->fmb_requested = false;
> > +		kmem_cache_free(zdev_fmb_cache, zdev->fmb);
> > +		zdev->fmb = NULL;
> > +	}
> > +
> >   	zpci_bus_device_unregister(zdev);
> >   	zpci_destroy_iommu(zdev);
> >   	zpci_dbg(3, "rem fid:%x\n", zdev->fid);
> > diff --git a/arch/s390/pci/pci_debug.c b/arch/s390/pci/pci_debug.c
> > index c7ed7bf254b5..9a960310d4dd 100644
> > --- a/arch/s390/pci/pci_debug.c
> > +++ b/arch/s390/pci/pci_debug.c
> > @@ -97,7 +97,7 @@ static int pci_perf_show(struct seq_file *m, void *v)
> >   		return 0;
> >   
> >   	mutex_lock(&zdev->fmb_lock);
> > -	if (!zdev->fmb) {
> > +	if (!zdev->fmb_requested) {
> >   		mutex_unlock(&zdev->fmb_lock);
> >   		seq_puts(m, "FMB statistics disabled\n");
> >   		return 0;
>
> Oh, and while playing around with this patch (with and without my
> additional modifications), I realized that statistics/function
> measurement gets re-enabled when I run `zpcictl --reset-fw <bdf>` to a
> function - despite it being switched off before the reset.

Great catch -- the FMB re-enablement in zpci_reenable_device() was just
one of the two places that needed it, I missed to add it in
pcibios_enable_device() instead of the existing unconditional enable. I
will fix this in the next version.

re: splitting up the functions into a pair of request functions and a
pair that actually do the MPCIFC: it will make resolving this issue more
straightforward, because preserving the state in pcibios_enable_device()
will require us to disable FMB without clearing its flag in
pcibios_disable_device() as well, which is cleanly possible with the
changes you suggested.

Thanks!

> Thanks,
> Gerd
>
>
> [ here's the patch to avoid zpci_fmb_reenable_device(): ]
>
>
> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> index 77c8c49ba6c8..26c6b616cc25 100644
> --- a/arch/s390/include/asm/pci.h
> +++ b/arch/s390/include/asm/pci.h
> @@ -350,9 +350,10 @@ int zpci_create_parent_msi_domain(struct zpci_bus
> *zbus);
>   void zpci_remove_parent_msi_domain(struct zpci_bus *zbus);
>   
>   /* FMB */
> -int zpci_fmb_enable_device(struct zpci_dev *);
> -int zpci_fmb_disable_device(struct zpci_dev *);
> -int zpci_fmb_reenable_device(struct zpci_dev *zdev);
> +int zpci_fmb_req_enable_device(struct zpci_dev *);
> +int zpci_fmb_req_disable_device(struct zpci_dev *);
> +int zpci_fmb_enable_device(struct zpci_dev *zdev);
> +int zpci_fmb_disable_device(struct zpci_dev *zdev);
>   
>   /* Debug */
>   int zpci_debug_init(void);
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index bda91d79bafd..c0bd76eac717 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -165,7 +165,7 @@ int zpci_unregister_ioat(struct zpci_dev *zdev, u8
> dmaas)
>   }
>   
>   /* Modify PCI: Set PCI function measurement parameters */
> -int zpci_fmb_enable_device(struct zpci_dev *zdev)
> +int zpci_fmb_req_enable_device(struct zpci_dev *zdev)
>   {
>   	lockdep_assert_held(&zdev->fmb_lock);
>   
> @@ -173,20 +173,25 @@ int zpci_fmb_enable_device(struct zpci_dev *zdev)
>   		return -EINVAL;
>   
>   	zdev->fmb_requested = true;
> -	return zpci_fmb_reenable_device(zdev);
> +	return zpci_fmb_enable_device(zdev);
>   }
>   
>   /* Modify PCI: Disable PCI function measurement */
> -int zpci_fmb_disable_device(struct zpci_dev *zdev)
> +int zpci_fmb_req_disable_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_requested)
>   		return -EINVAL;
> +	return zpci_fmb_disable_device(zdev);
> +}
> +EXPORT_SYMBOL_GPL(zpci_fmb_req_disable_device);
> +
> +int zpci_fmb_disable_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;
>   
>   	zdev->fmb_requested = false;
>   	fib.gd = zdev->gisa;
> @@ -200,14 +205,13 @@ int zpci_fmb_disable_device(struct zpci_dev
> *zdev)
>   
>   	return 0;
>   }
> -EXPORT_SYMBOL_GPL(zpci_fmb_disable_device);
>   
>   /* Re-enable PCI function measurement: clear the software IOMMU
> counters
>    * and (re-)enable the FMB. This function is used to restore the
> desired
>    * FMB enablement state when measurement may be implicitly disabled by
>    * firmware (i.e., via CLP in zpci_disable_device())
>    */
> -int zpci_fmb_reenable_device(struct zpci_dev *zdev)
> +int zpci_fmb_enable_device(struct zpci_dev *zdev)
>   {
>   	u64 req = ZPCI_CREATE_REQ(zdev->fh, 0,
> ZPCI_MOD_FC_SET_MEASURE);
>   	struct zpci_iommu_ctrs *ctrs;
> @@ -246,7 +250,7 @@ int zpci_fmb_reenable_device(struct zpci_dev *zdev)
>   
>   	return 0;
>   }
> -EXPORT_SYMBOL_GPL(zpci_fmb_reenable_device);
> +EXPORT_SYMBOL_GPL(zpci_fmb_enable_device);
>   
>   static int zpci_cfg_load(struct zpci_dev *zdev, int offset, u32 *val,
> u8 len)
>   {
> @@ -660,7 +664,7 @@ int pcibios_enable_device(struct pci_dev *pdev, int
> mask)
>   
>   	zpci_debug_init_device(zdev, dev_name(&pdev->dev));
>   	mutex_lock(&zdev->fmb_lock);
> -	zpci_fmb_enable_device(zdev);
> +	zpci_fmb_req_enable_device(zdev);
>   	mutex_unlock(&zdev->fmb_lock);
>   
>   	return pci_enable_resources(pdev, mask);
> @@ -671,7 +675,7 @@ void pcibios_disable_device(struct pci_dev *pdev)
>   	struct zpci_dev *zdev = to_zpci(pdev);
>   
>   	mutex_lock(&zdev->fmb_lock);
> -	zpci_fmb_disable_device(zdev);
> +	zpci_fmb_req_disable_device(zdev);
>   	mutex_unlock(&zdev->fmb_lock);
>   	zpci_debug_exit_device(zdev);
>   }
> @@ -760,7 +764,7 @@ int zpci_reenable_device(struct zpci_dev *zdev)
>   
>   	guard(mutex)(&zdev->fmb_lock);
>   	if (zdev->fmb_requested)
> -		zpci_fmb_reenable_device(zdev);
> +		zpci_fmb_enable_device(zdev);
>   
>   	return rc;
>   }
> diff --git a/arch/s390/pci/pci_debug.c b/arch/s390/pci/pci_debug.c
> index 9a960310d4dd..f2f0e7f44842 100644
> --- a/arch/s390/pci/pci_debug.c
> +++ b/arch/s390/pci/pci_debug.c
> @@ -156,10 +156,10 @@ static ssize_t pci_perf_seq_write(struct file
> *file, const char __user *ubuf,
>   	mutex_lock(&zdev->fmb_lock);
>   	switch (val) {
>   	case 0:
> -		rc = zpci_fmb_disable_device(zdev);
> +		rc = zpci_fmb_req_disable_device(zdev);
>   		break;
>   	case 1:
> -		rc = zpci_fmb_enable_device(zdev);
> +		rc = zpci_fmb_req_enable_device(zdev);
>   		break;
>   	}
>   	mutex_unlock(&zdev->fmb_lock);