On Sun, Feb 20 2022, Yishai Hadas <yishaih@xxxxxxxxxx> wrote:
> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
>
> Invoke a new device op 'device_feature' to handle just the data array
> portion of the command. This lifts the ioctl validation to the core code
> and makes it simpler for either the core code, or layered drivers, to
> implement their own feature values.
>
> Provide vfio_check_feature() to consolidate checking the flags/etc against
> what the driver supports.
>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@xxxxxxxxxx>
> Signed-off-by: Yishai Hadas <yishaih@xxxxxxxxxx>
> ---
> drivers/vfio/pci/vfio_pci.c | 1 +
> drivers/vfio/pci/vfio_pci_core.c | 94 +++++++++++++-------------------
> drivers/vfio/vfio.c | 46 ++++++++++++++--
> include/linux/vfio.h | 32 +++++++++++
> include/linux/vfio_pci_core.h | 2 +
> 5 files changed, 114 insertions(+), 61 deletions(-)
>
(...)
> +static int vfio_ioctl_device_feature(struct vfio_device *device,
> + struct vfio_device_feature __user *arg)
> +{
> + size_t minsz = offsetofend(struct vfio_device_feature, flags);
> + struct vfio_device_feature feature;
> +
> + if (copy_from_user(&feature, arg, minsz))
> + return -EFAULT;
> +
> + if (feature.argsz < minsz)
> + return -EINVAL;
> +
> + /* Check unknown flags */
> + if (feature.flags &
> + ~(VFIO_DEVICE_FEATURE_MASK | VFIO_DEVICE_FEATURE_SET |
> + VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_PROBE))
> + return -EINVAL;
> +
> + /* GET & SET are mutually exclusive except with PROBE */
> + if (!(feature.flags & VFIO_DEVICE_FEATURE_PROBE) &&
> + (feature.flags & VFIO_DEVICE_FEATURE_SET) &&
> + (feature.flags & VFIO_DEVICE_FEATURE_GET))
> + return -EINVAL;
> +
> + switch (feature.flags & VFIO_DEVICE_FEATURE_MASK) {
> + default:
> + if (unlikely(!device->ops->device_feature))
> + return -EINVAL;
> + return device->ops->device_feature(device, feature.flags,
> + arg->data,
> + feature.argsz - minsz);
> + }
> +}
> +
> static long vfio_device_fops_unl_ioctl(struct file *filep,
> unsigned int cmd, unsigned long arg)
> {
> struct vfio_device *device = filep->private_data;
>
> - if (unlikely(!device->ops->ioctl))
> - return -EINVAL;
> -
> - return device->ops->ioctl(device, cmd, arg);
> + switch (cmd) {
> + case VFIO_DEVICE_FEATURE:
> + return vfio_ioctl_device_feature(device, (void __user *)arg);
> + default:
> + if (unlikely(!device->ops->ioctl))
> + return -EINVAL;
> + return device->ops->ioctl(device, cmd, arg);
> + }
> }
One not-that-obvious change this is making is how VFIO_DEVICE_* ioctls
are processed. With this patch, VFIO_DEVICE_FEATURE is handled a bit
differently to other ioctl commands that are passed directly to the
device; here we have the common handling first, then control is passed
to the device. When I read in Documentation/driver-api/vfio.rst
"The ioctl interface provides a direct pass through for VFIO_DEVICE_*
ioctls."
I would not really expect that behaviour. No objection to introducing
it, but I think that needs a note in the doc, as you only see that if
you actually read the implementation (and not just the header and the
docs).
