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

[Omar Elghoul]

On 6/9/26 6:43 PM, Alex Williamson wrote:
> On Mon,  8 Jun 2026 13:18:49 -0400
> Omar Elghoul <oelghoul@xxxxxxxxxxxxx> wrote:
>
> > diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
> > index 0990fdb146b7..09454495ee23 100644
> > --- a/drivers/vfio/pci/vfio_pci_zdev.c
> > +++ b/drivers/vfio/pci/vfio_pci_zdev.c
> > @@ -167,3 +167,60 @@ 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;
> > +
> > +	guard(mutex)(&zdev->fmb_lock);
> > +
> > +	if (copy_from_user(&fmb_enable, arg, sizeof(fmb_enable)))
> > +		return -EFAULT;
>
> The guard can drop to here, it doesn't protect anything related to the
> copy_from_user().

Acked

> > +
> > +	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_temp = {0};
>
> Unnecessary initialization, we only copy to the user what's been
> written.

Acked as well

> > +	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;
> > +
> > +	guard(mutex)(&zdev->fmb_lock);
> > +
> > +	if (!zdev->fmb)
> > +		return -ENOMSG;
> > +	if (copy_from_user(&fmb_read, arg, sizeof(fmb_read)))
> > +		return -EFAULT;
> > +	if (!fmb_read.data)
> > +		return -EINVAL;
> > +
> > +	memcpy(&fmb_temp, zdev->fmb, zdev->fmb_length);
> > +	if (copy_to_user(fmb_read.data, &fmb_temp, zdev->fmb_length))
> > +		return -EFAULT;
>
> The bounce buffer itself seems unnecessary in this usage, we could just:
>
> 	if (copy_to_user(fmb_read.data, zdev->fmb, zdev->fmb_length))
>
> But maybe there was an intention to scope the bounce buffer copy within
> the guard and perform the copy_to_user() after releasing the lock?

I did not have that intention actually. This was to avoid a particular
issue with copying from a kmem_cache object to userspace. I guess an
alternate solution could be to initialize the SLUB object itself to
allow usercopy without making this a VFIO concern.

> > +
> > +	return 0;
> > +}
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 5de618a3a5ee..3988e8690e0b 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -1534,6 +1534,35 @@ struct vfio_device_feature_dma_buf {
> >    */
> >   #define VFIO_DEVICE_FEATURE_MIG_PRECOPY_INFOv2  12
> >   
> > +/**
> > + * 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. This
> > + * feature fails on attempt to double enable/disable.
>
> Does it?  Double enable just does a re-enable.

My bad, I meant to remove the "re" from "reenable".

> > + *
> > + * Returns: 0 on success, -1 and errno set appropriately on error.
> > + */
> > +#define VFIO_DEVICE_FEATURE_ZPCI_FMB_ENABLE 13
> > +
> > +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 14
> > +
> > +struct vfio_device_feature_zpci_fmb_read {
> > +	void __user *data;
>
> We should use explicit data sizes for uAPI:
>
> 	__aligned_u64 data;

Noted, will amend in the next version. Thanks.

> Thanks,
> Alex