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

[Niklas Schnelle]

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

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

> 
> Signed-off-by: Omar Elghoul <oelghoul@xxxxxxxxxxxxx>
> ---
>  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.

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

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