Re: [PATCH v7 4/4] vfio-pci/zdev: Add VFIO FMB device features

[Alex Williamson <alex@shazbot.org>]

On Wed, 30 Sep 2026 15:40:40 -0400
Omar Elghoul <oelghoul@linux.ibm.com> wrote:

> On 9/30/26 2:57 PM, Alex Williamson wrote:
> > On Tue, 22 Sep 2026 15:51:41 -0400
> > Omar Elghoul <oelghoul@linux.ibm.com> wrote:
> >> 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);  
> > 
> > A scoped guard would be preferable for consistency with the enable
> > path.  
> 
> I thought of that too, but then we'd do copy_to_user() while holding the
> mutex for longer than is necessary, which I think was pointed out in an
> earlier version.

Depends how you use it:

	scoped_guard(mutex, &zdev->fmb_lock) {
		if (!zdev->fmb_enabled)
			return -ENOMSG;
		memcpy(&fmb_bounce, zdev->fmb, zdev->fmb_length);
	}

	if (copy_to_user(u64_to_user_ptr(fmb_read.data), &fmb_bounce, zdev->fmb_length))
		return -EFAULT;

Thanks,
Alex