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

[Omar Elghoul <oelghoul@linux.ibm.com>]

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.

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.

> 
> 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.
> [...]
> 
> 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()?

It's not necessary and it was just preserved from the old code. You're
right that the cache already guarantees the alignment.

> 
>> +	} else {
>> +		/* FMB buffers are intentionally persistent for later reuse */
> 
> How about changing this comment to: /* reuse same FMB buffer as long a
> zdev lives */

Sure

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

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

Thanks

[1] 
https://lore.kernel.org/all/dae3c7cd-21aa-4263-bec5-792b018d21e2@linux.ibm.com/

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