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

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

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