Re: [PATCH v2 2/3] vfio-pci/zdev: Add VFIO FMB device feature

[Alex Williamson]

On Tue, 19 May 2026 18:42:03 -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..1e9efe2bee69 100644
> --- a/drivers/vfio/pci/vfio_pci_zdev.c
> +++ b/drivers/vfio/pci/vfio_pci_zdev.c
> @@ -167,3 +167,80 @@ 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(struct vfio_pci_core_device *vdev, u32 flags,
> +			      void __user *arg, size_t argsz)
> +{
> +	struct zpci_dev *zdev;
> +	struct vfio_device_feature_zpci_fmb fmb = {0};
> +	u32 ops = VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_SET;

Somewhat gratuitous variable usage.

> +	int ret;
> +
> +	ret = vfio_check_feature(flags, argsz, ops, sizeof(fmb));
> +	if (ret != 1)
> +		return ret;
> +
> +	zdev = to_zpci(vdev->pdev);
> +	if (!zdev)
> +		return -ENODEV;
> +
> +	mutex_lock(&zdev->fmb_lock);

Use a guard and avoid the release_lock gotos.

> +	if (flags & VFIO_DEVICE_FEATURE_SET) {
> +		if (copy_from_user(&fmb, arg, sizeof(fmb))) {
> +			ret = -EFAULT;
> +			goto release_lock;
> +		}
> +
> +		if (fmb.flags & VFIO_DEVICE_FEATURE_ZPCI_FMB_FLAGS_ENABLED)
> +			ret = zpci_fmb_reenable_device(zdev);
> +		else
> +			ret = zpci_fmb_disable_device(zdev);
> +		goto release_lock;

Remaining flag bits are not tested, breaks any future expanded use of
flags.

Why does the user need to be able to control these?

Doesn't allowing the user to disable FMB remove guaranteed host-based
monitoring?

Since this is already provided via debugfs, why not make this a
userspace problem to interact with the existing interface?

Alternatively, couldn't the existing zpci mediation be extended to
support the guest registering a fmb buffer to be written at regular
intervals (the interface here seems to drop the reporting interval).

> +	}
> +
> +	ret = 0;
> +	if (zdev->fmb) {
> +		fmb.flags |= VFIO_DEVICE_FEATURE_ZPCI_FMB_FLAGS_ENABLED;
> +	} else {
> +		fmb.flags &= ~VFIO_DEVICE_FEATURE_ZPCI_FMB_FLAGS_ENABLED;
> +		goto release_lock;
> +	}

Flag bit is cleared, goto skips copy-to-user, returns success...

Regardless of what's documented in the header, the buffer should be
assumed to be userspace garbage.  Failing to set or clear the entire
flags field precludes any future use.

Why do we need to use flags to indicate the enable state?  Couldn't we
just as easily have success indicate enabled and -ENOMSG indicate
disabled?  Thanks,

Alex