* [PATCH v7 1/4] s390/pci: Hold fmb_lock when enabling or disabling PCI devices
2026-09-22 19:51 [PATCH v7 0/4] vfio-pci/zdev: Improved zPCI Function Measurement Support Omar Elghoul
@ 2026-09-22 19:51 ` Omar Elghoul
2026-09-22 19:51 ` [PATCH v7 2/4] s390/pci: Reuse FMB buffer and preserve state in device re-enablement Omar Elghoul
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Omar Elghoul @ 2026-09-22 19:51 UTC (permalink / raw)
To: linux-s390, linux-kernel, kvm
Cc: oelghoul, hca, gor, agordeev, borntraeger, svens, schnelle,
mjrosato, alifm, farman, gbayer, pasic, alex, stable
Ensure that fmb_lock is held by pcibios_enable_device() and
pcibios_disable_device() when calling zpci_fmb_enable_device() or
zpci_fmb_disable_device(), respectively. Additionally, assert that the
fmb_lock is held within the latter two functions to prevent future race
conditions regarding new callers.
Fixes: af0a8a8453f7 ("s390/pci: implement pcibios_add_device")
Fixes: 944239c59e93 ("s390/pci: implement pcibios_release_device")
Cc: stable@vger.kernel.org
Signed-off-by: Omar Elghoul <oelghoul@linux.ibm.com>
Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
arch/s390/pci/pci.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index d64c544b32b0..c055a9ad0972 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -173,6 +173,8 @@ int zpci_fmb_enable_device(struct zpci_dev *zdev)
unsigned long flags;
u8 cc, status;
+ lockdep_assert_held(&zdev->fmb_lock);
+
if (zdev->fmb || sizeof(*zdev->fmb) < zdev->fmb_length)
return -EINVAL;
@@ -211,6 +213,8 @@ int zpci_fmb_disable_device(struct zpci_dev *zdev)
struct zpci_fib fib = {0};
u8 cc, status;
+ lockdep_assert_held(&zdev->fmb_lock);
+
if (!zdev->fmb)
return -EINVAL;
@@ -639,7 +643,9 @@ int pcibios_enable_device(struct pci_dev *pdev, int mask)
struct zpci_dev *zdev = to_zpci(pdev);
zpci_debug_init_device(zdev, dev_name(&pdev->dev));
+ mutex_lock(&zdev->fmb_lock);
zpci_fmb_enable_device(zdev);
+ mutex_unlock(&zdev->fmb_lock);
return pci_enable_resources(pdev, mask);
}
@@ -648,7 +654,9 @@ 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);
+ mutex_unlock(&zdev->fmb_lock);
zpci_debug_exit_device(zdev);
}
--
2.55.0
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v7 2/4] s390/pci: Reuse FMB buffer and preserve state in device re-enablement
2026-09-22 19:51 [PATCH v7 0/4] vfio-pci/zdev: Improved zPCI Function Measurement Support Omar Elghoul
2026-09-22 19:51 ` [PATCH v7 1/4] s390/pci: Hold fmb_lock when enabling or disabling PCI devices Omar Elghoul
@ 2026-09-22 19:51 ` Omar Elghoul
2026-09-23 15:09 ` Gerd Bayer
2026-09-23 22:20 ` Matthew Rosato
2026-09-22 19:51 ` [PATCH v7 3/4] s390/pci: Fence FMB enable/disable via debugfs for passthrough devices Omar Elghoul
2026-09-22 19:51 ` [PATCH v7 4/4] vfio-pci/zdev: Add VFIO FMB device features Omar Elghoul
3 siblings, 2 replies; 8+ messages in thread
From: Omar Elghoul @ 2026-09-22 19:51 UTC (permalink / raw)
To: linux-s390, linux-kernel, kvm
Cc: oelghoul, hca, gor, agordeev, borntraeger, svens, schnelle,
mjrosato, alifm, farman, gbayer, pasic, alex
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.
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);
/* 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);
+ } else {
+ /* FMB buffers are intentionally persistent for later reuse */
+ 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);
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;
--
2.55.0
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v7 2/4] s390/pci: Reuse FMB buffer and preserve state in device re-enablement
2026-09-22 19:51 ` [PATCH v7 2/4] s390/pci: Reuse FMB buffer and preserve state in device re-enablement Omar Elghoul
@ 2026-09-23 15:09 ` Gerd Bayer
2026-09-23 16:37 ` Omar Elghoul
2026-09-23 22:20 ` Matthew Rosato
1 sibling, 1 reply; 8+ messages in thread
From: Gerd Bayer @ 2026-09-23 15:09 UTC (permalink / raw)
To: Omar Elghoul, linux-s390, linux-kernel, kvm
Cc: hca, gor, agordeev, borntraeger, svens, schnelle, mjrosato,
alifm, farman, pasic, alex
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v7 2/4] s390/pci: Reuse FMB buffer and preserve state in device re-enablement
2026-09-23 15:09 ` Gerd Bayer
@ 2026-09-23 16:37 ` Omar Elghoul
0 siblings, 0 replies; 8+ messages in thread
From: Omar Elghoul @ 2026-09-23 16:37 UTC (permalink / raw)
To: Gerd Bayer, linux-s390, linux-kernel, kvm
Cc: hca, gor, agordeev, borntraeger, svens, schnelle, mjrosato,
alifm, farman, pasic, alex
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v7 2/4] s390/pci: Reuse FMB buffer and preserve state in device re-enablement
2026-09-22 19:51 ` [PATCH v7 2/4] s390/pci: Reuse FMB buffer and preserve state in device re-enablement Omar Elghoul
2026-09-23 15:09 ` Gerd Bayer
@ 2026-09-23 22:20 ` Matthew Rosato
1 sibling, 0 replies; 8+ messages in thread
From: Matthew Rosato @ 2026-09-23 22:20 UTC (permalink / raw)
To: Omar Elghoul, linux-s390, linux-kernel, kvm
Cc: hca, gor, agordeev, borntraeger, svens, schnelle, alifm, farman,
gbayer, pasic, alex
On 9/22/26 3:51 PM, 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.
>
> Signed-off-by: Omar Elghoul <oelghoul@linux.ibm.com>
[...]
> +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);
[...]
> + if (zdev->fmb_enabled)
> + zpci_fmb_reenable_device(zdev);
>
Besides Gerd's comments, I was looking at this patch in isolation and
this combination made me wonder why you were adding what appears to be
dead code (of course, patch 4 adds another caller that doesn't check
zdev->fmb_enabled before calling)
Maybe you could add a bit to the commit message besides 'Call
> this function from zpci_reenable_device().' to indicate that this
function is also being setup for future re-use where we might be going
disabled->enabled rather than enabled->disabled->enabled.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v7 3/4] s390/pci: Fence FMB enable/disable via debugfs for passthrough devices
2026-09-22 19:51 [PATCH v7 0/4] vfio-pci/zdev: Improved zPCI Function Measurement Support Omar Elghoul
2026-09-22 19:51 ` [PATCH v7 1/4] s390/pci: Hold fmb_lock when enabling or disabling PCI devices Omar Elghoul
2026-09-22 19:51 ` [PATCH v7 2/4] s390/pci: Reuse FMB buffer and preserve state in device re-enablement Omar Elghoul
@ 2026-09-22 19:51 ` Omar Elghoul
2026-09-22 19:51 ` [PATCH v7 4/4] vfio-pci/zdev: Add VFIO FMB device features Omar Elghoul
3 siblings, 0 replies; 8+ messages in thread
From: Omar Elghoul @ 2026-09-22 19:51 UTC (permalink / raw)
To: linux-s390, linux-kernel, kvm
Cc: oelghoul, hca, gor, agordeev, borntraeger, svens, schnelle,
mjrosato, alifm, farman, gbayer, pasic, alex
Introduce a fence over enabling or disabling FMB via debugfs when the zPCI
device is associated with a KVM. This will prevent processes on the host
from tampering with the FMB while the guest is still using it, which may
cause partial counter resets and inconsistent reads which have no parallel
in the architecture.
For VFIO devices that are not associated with a KVM (i.e., for userspace
drivers other than QEMU), this fence does not take effect.
Signed-off-by: Omar Elghoul <oelghoul@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
arch/s390/pci/pci_debug.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/s390/pci/pci_debug.c b/arch/s390/pci/pci_debug.c
index 44f026ead414..afc52bd5271a 100644
--- a/arch/s390/pci/pci_debug.c
+++ b/arch/s390/pci/pci_debug.c
@@ -153,6 +153,12 @@ static ssize_t pci_perf_seq_write(struct file *file, const char __user *ubuf,
if (rc)
return rc;
+ mutex_lock(&zdev->kzdev_lock);
+ if (zdev->kzdev) {
+ rc = -EPERM;
+ goto out_unlock_kzdev;
+ }
+
mutex_lock(&zdev->fmb_lock);
switch (val) {
case 0:
@@ -163,6 +169,9 @@ static ssize_t pci_perf_seq_write(struct file *file, const char __user *ubuf,
break;
}
mutex_unlock(&zdev->fmb_lock);
+
+out_unlock_kzdev:
+ mutex_unlock(&zdev->kzdev_lock);
return rc ? rc : count;
}
--
2.55.0
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v7 4/4] vfio-pci/zdev: Add VFIO FMB device features
2026-09-22 19:51 [PATCH v7 0/4] vfio-pci/zdev: Improved zPCI Function Measurement Support Omar Elghoul
` (2 preceding siblings ...)
2026-09-22 19:51 ` [PATCH v7 3/4] s390/pci: Fence FMB enable/disable via debugfs for passthrough devices Omar Elghoul
@ 2026-09-22 19:51 ` Omar Elghoul
3 siblings, 0 replies; 8+ messages in thread
From: Omar Elghoul @ 2026-09-22 19:51 UTC (permalink / raw)
To: linux-s390, linux-kernel, kvm
Cc: oelghoul, hca, gor, agordeev, borntraeger, svens, schnelle,
mjrosato, alifm, farman, gbayer, pasic, alex
Introduce new VFIO features for zPCI devices to provide FMB passthrough to
userspace.
Allow the user to enable or disable the FMB using the SET-only feature
VFIO_DEVICE_FEATURE_ZPCI_FMB_ENABLE. Likewise allow the user to read the
latest FMB using the GET-only feature VFIO_DEVICE_FEATURE_ZPCI_FMB_READ
in the case when the FMB is enabled.
Signed-off-by: Omar Elghoul <oelghoul@linux.ibm.com>
---
drivers/vfio/pci/vfio_pci_core.c | 4 +++
drivers/vfio/pci/vfio_pci_priv.h | 18 ++++++++++
drivers/vfio/pci/vfio_pci_zdev.c | 60 ++++++++++++++++++++++++++++++++
include/uapi/linux/vfio.h | 29 +++++++++++++++
4 files changed, 111 insertions(+)
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 6757054e9d87..3c827a77725b 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -1627,6 +1627,10 @@ int vfio_pci_core_ioctl_feature(struct vfio_device *device, u32 flags,
return vfio_pci_core_feature_dma_buf(vdev, flags, arg, argsz);
case VFIO_DEVICE_FEATURE_ZPCI_ERROR:
return vfio_pci_zdev_feature_err(device, flags, arg, argsz);
+ case VFIO_DEVICE_FEATURE_ZPCI_FMB_ENABLE:
+ return vfio_pci_zdev_feature_fmb_enable(vdev, flags, arg, argsz);
+ case VFIO_DEVICE_FEATURE_ZPCI_FMB_READ:
+ return vfio_pci_zdev_feature_fmb_read(vdev, flags, arg, argsz);
default:
return -ENOTTY;
}
diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h
index 4e7162234a2e..e04d7e9d0c30 100644
--- a/drivers/vfio/pci/vfio_pci_priv.h
+++ b/drivers/vfio/pci/vfio_pci_priv.h
@@ -95,6 +95,10 @@ int vfio_pci_zdev_open_device(struct vfio_pci_core_device *vdev);
void vfio_pci_zdev_close_device(struct vfio_pci_core_device *vdev);
int vfio_pci_zdev_feature_err(struct vfio_device *device, u32 flags,
void __user *arg, size_t argsz);
+int vfio_pci_zdev_feature_fmb_enable(struct vfio_pci_core_device *vdev, u32 flags,
+ void __user *arg, size_t argsz);
+int vfio_pci_zdev_feature_fmb_read(struct vfio_pci_core_device *vdev, u32 flags,
+ void __user *arg, size_t argsz);
#else
static inline int vfio_pci_info_zdev_add_caps(struct vfio_pci_core_device *vdev,
struct vfio_info_cap *caps)
@@ -116,6 +120,20 @@ static inline int vfio_pci_zdev_feature_err(struct vfio_device *device,
{
return -ENOTTY;
}
+
+static inline int vfio_pci_zdev_feature_fmb_enable(struct vfio_pci_core_device *vdev,
+ u32 flags, void __user *arg,
+ size_t argsz)
+{
+ return -ENOTTY;
+}
+
+static inline int vfio_pci_zdev_feature_fmb_read(struct vfio_pci_core_device *vdev,
+ u32 flags, void __user *arg,
+ size_t argsz)
+{
+ return -ENOTTY;
+}
#endif
static inline bool vfio_pci_is_vga(struct pci_dev *pdev)
diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
index f47f36314a1c..b028de8e08a4 100644
--- a/drivers/vfio/pci/vfio_pci_zdev.c
+++ b/drivers/vfio/pci/vfio_pci_zdev.c
@@ -219,3 +219,63 @@ void vfio_pci_zdev_close_device(struct vfio_pci_core_device *vdev)
if (zpci_kvm_hook.kvm_unregister)
zpci_kvm_hook.kvm_unregister(zdev);
}
+
+int vfio_pci_zdev_feature_fmb_enable(struct vfio_pci_core_device *vdev, u32 flags,
+ void __user *arg, size_t argsz)
+{
+ struct zpci_dev *zdev;
+ struct vfio_device_feature_zpci_fmb_enable fmb_enable;
+ int ret;
+
+ ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET, sizeof(fmb_enable));
+ if (ret != 1)
+ return ret;
+
+ zdev = to_zpci(vdev->pdev);
+ if (!zdev)
+ return -ENODEV;
+
+ if (copy_from_user(&fmb_enable, arg, sizeof(fmb_enable)))
+ return -EFAULT;
+
+ guard(mutex)(&zdev->fmb_lock);
+
+ if (fmb_enable.enabled)
+ return zpci_fmb_reenable_device(zdev);
+ return zpci_fmb_disable_device(zdev);
+}
+
+int vfio_pci_zdev_feature_fmb_read(struct vfio_pci_core_device *vdev, u32 flags,
+ void __user *arg, size_t argsz)
+{
+ struct zpci_dev *zdev;
+ struct vfio_device_feature_zpci_fmb_read fmb_read;
+ struct zpci_fmb fmb_bounce;
+ int ret;
+
+ ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_GET, sizeof(fmb_read));
+ if (ret != 1)
+ return ret;
+
+ zdev = to_zpci(vdev->pdev);
+ if (!zdev)
+ return -ENODEV;
+
+ if (copy_from_user(&fmb_read, arg, sizeof(fmb_read)))
+ return -EFAULT;
+ if (!fmb_read.data)
+ return -EINVAL;
+
+ mutex_lock(&zdev->fmb_lock);
+ if (!zdev->fmb_enabled) {
+ mutex_unlock(&zdev->fmb_lock);
+ return -ENOMSG;
+ }
+
+ memcpy(&fmb_bounce, zdev->fmb, zdev->fmb_length);
+ mutex_unlock(&zdev->fmb_lock);
+
+ if (copy_to_user(u64_to_user_ptr(fmb_read.data), &fmb_bounce, zdev->fmb_length))
+ return -EFAULT;
+ return 0;
+}
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index e41437fa17ad..a8d501e42ef9 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -1555,6 +1555,35 @@ struct vfio_device_feature_zpci_err {
#define VFIO_DEVICE_FEATURE_ZPCI_ERROR 13
+/**
+ * Upon VFIO_DEVICE_FEATURE_SET, enable or disable FMB for the VFIO zPCI device.
+ *
+ * enabled is treated as a bool, so any non-zero value evaluates to true. Nested
+ * enabling resets the FMB counters to zero, and nested disabling fails.
+ *
+ * Returns: 0 on success, -1 and errno set appropriately on error.
+ */
+#define VFIO_DEVICE_FEATURE_ZPCI_FMB_ENABLE 14
+
+struct vfio_device_feature_zpci_fmb_enable {
+ __u8 enabled;
+};
+
+/**
+ * Upon VFIO_DEVICE_FEATURE_GET, provide FMB passthrough for VFIO zPCI devices.
+ *
+ * The user-provided buffer must be at least fmb_length large, where fmb_length
+ * is reported in VFIO_DEVICE_INFO_CAP_ZPCI_BASE.
+ *
+ * Returns: 0 on success, -1 and errno set appropriately on error. errno==ENOMSG
+ * when the FMB is not enabled.
+ */
+#define VFIO_DEVICE_FEATURE_ZPCI_FMB_READ 15
+
+struct vfio_device_feature_zpci_fmb_read {
+ __aligned_u64 data;
+};
+
/* -------- API for Type1 VFIO IOMMU -------- */
/**
--
2.55.0
^ permalink raw reply [flat|nested] 8+ messages in thread