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

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

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?

Or, do you want to capture some (the outcome) of the prior discussion
about the "grace period" that architecture imposes on re-using the
memory that was ever registered as FMB with a PCI function? As
rationale why this patch changes when the FMB memory is freed.

> 
> Signed-off-by: Omar Elghoul <oelghoul@linux.ibm.com>
> ---
>  arch/s390/include/asm/pci.h |  2 +
>  arch/s390/pci/pci.c         | 77 +++++++++++++++++++++++++++++--------
>  arch/s390/pci/pci_debug.c   |  2 +-
>  3 files changed, 63 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> index 88a125b92bdd..b8162f7a8968 100644
> --- a/arch/s390/include/asm/pci.h
> +++ b/arch/s390/include/asm/pci.h
> @@ -175,6 +175,7 @@ struct zpci_dev {
>  	u8		util_str_avail	: 1;
>  	u8		tid_avail	: 1;
>  	u8		rtr_avail	: 1; /* Relaxed translation allowed */
> +	u8		fmb_enabled	: 1;
>  	unsigned int	devfn;		/* DEVFN part of the RID*/
>  
>  	u8 pfip[CLP_PFIP_NR_SEGMENTS];	/* pci function internal path */
> @@ -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);

Nothing wrong in this patch, but apparently there's no common "style"
in this file regarding whether parameters are named in the function
prototypes.

>  
>  /* Debug */
>  int zpci_debug_init(void);
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index c055a9ad0972..815257ddb6c8 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -175,13 +175,18 @@ int zpci_fmb_enable_device(struct zpci_dev *zdev)
>  
>  	lockdep_assert_held(&zdev->fmb_lock);
>  
> -	if (zdev->fmb || sizeof(*zdev->fmb) < zdev->fmb_length)
> +	if (zdev->fmb_enabled || 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);
> +	if (!zdev->fmb) {
> +		zdev->fmb = kmem_cache_zalloc(zdev_fmb_cache, GFP_KERNEL);
> +		if (!zdev->fmb)
> +			return -ENOMEM;
> +		WARN_ON((u64) zdev->fmb & 0xf);

Already before this patch: Is this WARN_ON necessary, with the
alignment on struct zpci_fmb when zdev_fmb_cache is created in
zpci_mem_init()?

> +	} else {
> +		/* FMB buffers are intentionally persistent for later reuse */

How about changing this comment to: /* reuse same FMB buffer as long a
zdev lives */

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

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?

>  
>  static int zpci_cfg_load(struct zpci_dev *zdev, int offset, u32 *val, u8 len)
>  {
> @@ -737,8 +768,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_enabled)
> +		zpci_fmb_reenable_device(zdev);
>  
>  	return rc;
>  }
> @@ -1003,6 +1040,12 @@ void zpci_release_device(struct kref *kref)
>  	if (zdev->has_resources)
>  		zpci_cleanup_bus_resources(zdev);
>  
> +	if (zdev->fmb) {
> +		zdev->fmb_enabled = 0;
> +		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..44f026ead414 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_enabled) {
>  		mutex_unlock(&zdev->fmb_lock);
>  		seq_puts(m, "FMB statistics disabled\n");
>  		return 0;

Thank you,
Gerd