Re: [PATCH v3 4/4] s390/pci: Fence FMB enable/disable via sysfs for passthrough devices

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

On 6/10/26 5:07 AM, Niklas Schnelle wrote:
> On Tue, 2026-06-09 at 20:32 -0400, Omar Elghoul wrote:
>> On 6/9/26 6:52 PM, Alex Williamson wrote:
>>> On Mon,  8 Jun 2026 13:18:50 -0400
>>> Omar Elghoul <oelghoul@linux.ibm.com> wrote:
>>>
>>>> Introduce a fence over enabling or disabling FMB via sysfs when the zPCI
>>>> device is associated with a KVM. This will allow a KVM guest to use FMB
>>>> passthrough and avoid the edge-case where the host disables FMB while the
>>>> guest is still using it, which may cause partial counter resets and
>>>> inconsistent reads which have no parallel in the architecture.
>>>>
>>>> With this patch, the userspace driver, likely QEMU, is still able to enable
>>>> or disable the FMB using the VFIO device feature introduced in the previous
>>>> patch, effectively securing what is associated with the VM state and
>>>> isolating it from other processes on the host.
>>>>
>>>> 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: Niklas Schnelle <schnelle@linux.ibm.com>
>>>> ---
>>>>    arch/s390/pci/pci_debug.c | 11 ++++++++++-
>>>>    1 file changed, 10 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/s390/pci/pci_debug.c b/arch/s390/pci/pci_debug.c
>>>> index c7ed7bf254b5..a2dc79418c21 100644
>>>> --- a/arch/s390/pci/pci_debug.c
>>>> +++ b/arch/s390/pci/pci_debug.c
>>>> @@ -149,9 +149,15 @@ static ssize_t pci_perf_seq_write(struct file *file, const char __user *ubuf,
>>>>    	if (!zdev)
>>>>    		return 0;
>>>>    
>>>> +	mutex_lock(&zdev->kzdev_lock);
>>>> +	if (zdev->kzdev) {
>>>> +		rc = -EPERM;
>>>> +		goto out_unlock_kzdev;
>>>> +	}
>>>> +
>>>>    	rc = kstrtoul_from_user(ubuf, count, 10, &val);
>>>>    	if (rc)
>>>> -		return rc;
>>>> +		goto out_unlock_kzdev;
>>>>    
>>>>    	mutex_lock(&zdev->fmb_lock);
>>>>    	switch (val) {
>>>> @@ -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;
>>>>    }
>>>>    
>>>
>>> Why not also use a guard for the mutex here and avoid the goto
>>> unlock... also moving the guard below the kstrtoul_from_user()?
>>>
>>> The fmb_lock could switch to a guard too, but that's existing.
>>
>>   From where I'm standing I don't think there is any particular reason to
>> do it one way vs the other. Thanks.
> 
> I do prefer the guard(mutex) because it does remove the need for a goto
> exit in many places including the above. That said it should also take
> the surrounding code into account and definitely needs to be applied to
> the whole function. In this case pci_perf_seq_write() uses both the
> kzdev and fmb_lock so to use guard you'd have to convert both. That
> then feels out of scope for this patch so I'd prefer you stick with
> plain mutex here but use guard in cases where the whole function ends
> up using guards. So for patch 2 you can also go with guard, it's not a
> must for me though.

Acked, thanks

> 
> Thanks,
> Niklas
>>