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

[Gerd Bayer <gbayer@linux.ibm.com>]

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?

> 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@linux.ibm.com>
> ---
>  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?

>  
>  	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()


>  }
>  
>  /* 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?

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

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);