diff mbox series

[V6,mlx5-next,07/15] vfio: Have the core code decode the VFIO_DEVICE_FEATURE ioctl

Message ID 20220130160826.32449-8-yishaih@nvidia.com (mailing list archive)
State Superseded
Headers show
Series Add mlx5 live migration driver and v2 migration protocol | expand

Commit Message

Yishai Hadas Jan. 30, 2022, 4:08 p.m. UTC 
From: Jason Gunthorpe <jgg@nvidia.com>

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@nvidia.com>
Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
---
 drivers/vfio/pci/vfio_pci.c      |  1 +
 drivers/vfio/pci/vfio_pci_core.c | 90 ++++++++++++--------------------
 drivers/vfio/vfio.c              | 46 ++++++++++++++--
 include/linux/vfio.h             | 32 ++++++++++++
 include/linux/vfio_pci_core.h    |  2 +
 5 files changed, 109 insertions(+), 62 deletions(-)

Comments

Alex Williamson Jan. 31, 2022, 11:41 p.m. UTC | #1 
On Sun, 30 Jan 2022 18:08:18 +0200
Yishai Hadas <yishaih@nvidia.com> wrote:

> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> 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@nvidia.com>
> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
> ---
>  drivers/vfio/pci/vfio_pci.c      |  1 +
>  drivers/vfio/pci/vfio_pci_core.c | 90 ++++++++++++--------------------
>  drivers/vfio/vfio.c              | 46 ++++++++++++++--
>  include/linux/vfio.h             | 32 ++++++++++++
>  include/linux/vfio_pci_core.h    |  2 +
>  5 files changed, 109 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index a5ce92beb655..2b047469e02f 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -130,6 +130,7 @@ static const struct vfio_device_ops vfio_pci_ops = {
>  	.open_device	= vfio_pci_open_device,
>  	.close_device	= vfio_pci_core_close_device,
>  	.ioctl		= vfio_pci_core_ioctl,
> +	.device_feature = vfio_pci_core_ioctl_feature,
>  	.read		= vfio_pci_core_read,
>  	.write		= vfio_pci_core_write,
>  	.mmap		= vfio_pci_core_mmap,
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index f948e6cd2993..14a22ff20ef8 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -1114,70 +1114,44 @@ long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd,
>  
>  		return vfio_pci_ioeventfd(vdev, ioeventfd.offset,
>  					  ioeventfd.data, count, ioeventfd.fd);
> -	} else if (cmd == VFIO_DEVICE_FEATURE) {
> -		struct vfio_device_feature feature;
> -		uuid_t uuid;
> -
> -		minsz = offsetofend(struct vfio_device_feature, flags);
> -
> -		if (copy_from_user(&feature, (void __user *)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) {
> -		case VFIO_DEVICE_FEATURE_PCI_VF_TOKEN:
> -			if (!vdev->vf_token)
> -				return -ENOTTY;
> -
> -			/*
> -			 * We do not support GET of the VF Token UUID as this
> -			 * could expose the token of the previous device user.
> -			 */
> -			if (feature.flags & VFIO_DEVICE_FEATURE_GET)
> -				return -EINVAL;
> -
> -			if (feature.flags & VFIO_DEVICE_FEATURE_PROBE)
> -				return 0;
> -
> -			/* Don't SET unless told to do so */
> -			if (!(feature.flags & VFIO_DEVICE_FEATURE_SET))
> -				return -EINVAL;
> +	}
> +	return -ENOTTY;
> +}
> +EXPORT_SYMBOL_GPL(vfio_pci_core_ioctl);
>  
> -			if (feature.argsz < minsz + sizeof(uuid))
> -				return -EINVAL;
> +int vfio_pci_core_ioctl_feature(struct vfio_device *device, u32 flags,
> +				void __user *arg, size_t argsz)
> +{
> +	struct vfio_pci_core_device *vdev =
> +		container_of(device, struct vfio_pci_core_device, vdev);
> +	uuid_t uuid;
> +	int ret;

Nit, should uuid at least be scoped within the token code?  Or token
code pushed to a separate function?

>  
> -			if (copy_from_user(&uuid, (void __user *)(arg + minsz),
> -					   sizeof(uuid)))
> -				return -EFAULT;
> +	switch (flags & VFIO_DEVICE_FEATURE_MASK) {
> +	case VFIO_DEVICE_FEATURE_PCI_VF_TOKEN:
> +		if (!vdev->vf_token)
> +			return -ENOTTY;
> +		/*
> +		 * We do not support GET of the VF Token UUID as this could
> +		 * expose the token of the previous device user.
> +		 */
> +		ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET,
> +					sizeof(uuid));
> +		if (ret != 1)
> +			return ret;
>  
> -			mutex_lock(&vdev->vf_token->lock);
> -			uuid_copy(&vdev->vf_token->uuid, &uuid);
> -			mutex_unlock(&vdev->vf_token->lock);
> +		if (copy_from_user(&uuid, arg, sizeof(uuid)))
> +			return -EFAULT;
>  
> -			return 0;
> -		default:
> -			return -ENOTTY;
> -		}
> +		mutex_lock(&vdev->vf_token->lock);
> +		uuid_copy(&vdev->vf_token->uuid, &uuid);
> +		mutex_unlock(&vdev->vf_token->lock);
> +		return 0;
> +	default:
> +		return -ENOTTY;
>  	}
> -
> -	return -ENOTTY;
>  }
> -EXPORT_SYMBOL_GPL(vfio_pci_core_ioctl);
> +EXPORT_SYMBOL_GPL(vfio_pci_core_ioctl_feature);
...
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 76191d7abed1..ca69516f869d 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -55,6 +55,7 @@ struct vfio_device {
>   * @match: Optional device name match callback (return: 0 for no-match, >0 for
>   *         match, -errno for abort (ex. match with insufficient or incorrect
>   *         additional args)
> + * @device_feature: Fill in the VFIO_DEVICE_FEATURE ioctl
>   */
>  struct vfio_device_ops {
>  	char	*name;
> @@ -69,8 +70,39 @@ struct vfio_device_ops {
>  	int	(*mmap)(struct vfio_device *vdev, struct vm_area_struct *vma);
>  	void	(*request)(struct vfio_device *vdev, unsigned int count);
>  	int	(*match)(struct vfio_device *vdev, char *buf);
> +	int	(*device_feature)(struct vfio_device *device, u32 flags,
> +				  void __user *arg, size_t argsz);
>  };
>  
> +/**
> + * vfio_check_feature - Validate user input for the VFIO_DEVICE_FEATURE ioctl
> + * @flags: Arg from the device_feature op
> + * @argsz: Arg from the device_feature op
> + * @supported_ops: Combination of VFIO_DEVICE_FEATURE_GET and SET the driver
> + *                 supports
> + * @minsz: Minimum data size the driver accepts
> + *
> + * For use in a driver's device_feature op. Checks that the inputs to the
> + * VFIO_DEVICE_FEATURE ioctl are correct for the driver's feature. Returns 1 if
> + * the driver should execute the get or set, otherwise the relevant
> + * value should be returned.
> + */
> +static inline int vfio_check_feature(u32 flags, size_t argsz, u32 supported_ops,
> +				    size_t minsz)
> +{
> +	if ((flags & (VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_SET)) &
> +	    ~supported_ops)
> +		return -EINVAL;

These look like cases where it would be useful for userspace debugging
to differentiate errnos.

-EOPNOTSUPP?

> +	if (flags & VFIO_DEVICE_FEATURE_PROBE)
> +		return 0;
> +	/* Without PROBE one of GET or SET must be requested */
> +	if (!(flags & (VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_SET)))
> +		return -EINVAL;
> +	if (argsz < minsz)
> +		return -EINVAL;

-ENOSPC?

Thanks,
Alex
Jason Gunthorpe Feb. 1, 2022, 12:11 a.m. UTC | #2 
On Mon, Jan 31, 2022 at 04:41:43PM -0700, Alex Williamson wrote:
> > +int vfio_pci_core_ioctl_feature(struct vfio_device *device, u32 flags,
> > +				void __user *arg, size_t argsz)
> > +{
> > +	struct vfio_pci_core_device *vdev =
> > +		container_of(device, struct vfio_pci_core_device, vdev);
> > +	uuid_t uuid;
> > +	int ret;
> 
> Nit, should uuid at least be scoped within the token code?  Or token
> code pushed to a separate function?

Sure, it wasn't done before, but it would be nicer,.

> > +static inline int vfio_check_feature(u32 flags, size_t argsz, u32 supported_ops,
> > +				    size_t minsz)
> > +{
> > +	if ((flags & (VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_SET)) &
> > +	    ~supported_ops)
> > +		return -EINVAL;
> 
> These look like cases where it would be useful for userspace debugging
> to differentiate errnos.

I tried to keep it unchanged from what it was today.

> -EOPNOTSUPP?

This would be my preference, but it would also be the first use in
vfio

> > +	if (flags & VFIO_DEVICE_FEATURE_PROBE)
> > +		return 0;
> > +	/* Without PROBE one of GET or SET must be requested */
> > +	if (!(flags & (VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_SET)))
> > +		return -EINVAL;
> > +	if (argsz < minsz)
> > +		return -EINVAL;
>
> -ENOSPC?

Do you want to do all of these minsz then? There are lots..

Jason
Alex Williamson Feb. 1, 2022, 3:47 p.m. UTC | #3 
On Mon, 31 Jan 2022 20:11:48 -0400
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Mon, Jan 31, 2022 at 04:41:43PM -0700, Alex Williamson wrote:
> > > +int vfio_pci_core_ioctl_feature(struct vfio_device *device, u32 flags,
> > > +				void __user *arg, size_t argsz)
> > > +{
> > > +	struct vfio_pci_core_device *vdev =
> > > +		container_of(device, struct vfio_pci_core_device, vdev);
> > > +	uuid_t uuid;
> > > +	int ret;  
> > 
> > Nit, should uuid at least be scoped within the token code?  Or token
> > code pushed to a separate function?  
> 
> Sure, it wasn't done before, but it would be nicer,.
> 
> > > +static inline int vfio_check_feature(u32 flags, size_t argsz, u32 supported_ops,
> > > +				    size_t minsz)
> > > +{
> > > +	if ((flags & (VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_SET)) &
> > > +	    ~supported_ops)
> > > +		return -EINVAL;  
> > 
> > These look like cases where it would be useful for userspace debugging
> > to differentiate errnos.  
> 
> I tried to keep it unchanged from what it was today.
> 
> > -EOPNOTSUPP?  
> 
> This would be my preference, but it would also be the first use in
> vfio
> 
> > > +	if (flags & VFIO_DEVICE_FEATURE_PROBE)
> > > +		return 0;
> > > +	/* Without PROBE one of GET or SET must be requested */
> > > +	if (!(flags & (VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_SET)))
> > > +		return -EINVAL;
> > > +	if (argsz < minsz)
> > > +		return -EINVAL;  
> >
> > -ENOSPC?  
> 
> Do you want to do all of these minsz then? There are lots..

Hmm, maybe this one is more correct as EINVAL.  In the existing use
cases the structure associated with the feature is a fixed size, so
it's not a matter that we down have space for a return like
HOT_RESET_INFO, it's simply invalid arguments by the caller.  I guess
keep this one as EINVAL, but EOPNOTSUPP seems useful for the previous.
Thanks,

Alex
Jason Gunthorpe Feb. 1, 2022, 3:49 p.m. UTC | #4 
On Tue, Feb 01, 2022 at 08:47:58AM -0700, Alex Williamson wrote:
> On Mon, 31 Jan 2022 20:11:48 -0400
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > On Mon, Jan 31, 2022 at 04:41:43PM -0700, Alex Williamson wrote:
> > > > +int vfio_pci_core_ioctl_feature(struct vfio_device *device, u32 flags,
> > > > +				void __user *arg, size_t argsz)
> > > > +{
> > > > +	struct vfio_pci_core_device *vdev =
> > > > +		container_of(device, struct vfio_pci_core_device, vdev);
> > > > +	uuid_t uuid;
> > > > +	int ret;  
> > > 
> > > Nit, should uuid at least be scoped within the token code?  Or token
> > > code pushed to a separate function?  
> > 
> > Sure, it wasn't done before, but it would be nicer,.
> > 
> > > > +static inline int vfio_check_feature(u32 flags, size_t argsz, u32 supported_ops,
> > > > +				    size_t minsz)
> > > > +{
> > > > +	if ((flags & (VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_SET)) &
> > > > +	    ~supported_ops)
> > > > +		return -EINVAL;  
> > > 
> > > These look like cases where it would be useful for userspace debugging
> > > to differentiate errnos.  
> > 
> > I tried to keep it unchanged from what it was today.
> > 
> > > -EOPNOTSUPP?  
> > 
> > This would be my preference, but it would also be the first use in
> > vfio
> > 
> > > > +	if (flags & VFIO_DEVICE_FEATURE_PROBE)
> > > > +		return 0;
> > > > +	/* Without PROBE one of GET or SET must be requested */
> > > > +	if (!(flags & (VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_SET)))
> > > > +		return -EINVAL;
> > > > +	if (argsz < minsz)
> > > > +		return -EINVAL;  
> > >
> > > -ENOSPC?  
> > 
> > Do you want to do all of these minsz then? There are lots..
> 
> Hmm, maybe this one is more correct as EINVAL.  In the existing use
> cases the structure associated with the feature is a fixed size, so
> it's not a matter that we down have space for a return like
> HOT_RESET_INFO, it's simply invalid arguments by the caller.  I guess
> keep this one as EINVAL, but EOPNOTSUPP seems useful for the
> previous.

Do you want EOPNOTSUPP or ENOTTY like most other places in vfio?

Jason
diff mbox series

Patch

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index a5ce92beb655..2b047469e02f 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -130,6 +130,7 @@  static const struct vfio_device_ops vfio_pci_ops = {
 	.open_device	= vfio_pci_open_device,
 	.close_device	= vfio_pci_core_close_device,
 	.ioctl		= vfio_pci_core_ioctl,
+	.device_feature = vfio_pci_core_ioctl_feature,
 	.read		= vfio_pci_core_read,
 	.write		= vfio_pci_core_write,
 	.mmap		= vfio_pci_core_mmap,
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index f948e6cd2993..14a22ff20ef8 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -1114,70 +1114,44 @@  long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd,
 
 		return vfio_pci_ioeventfd(vdev, ioeventfd.offset,
 					  ioeventfd.data, count, ioeventfd.fd);
-	} else if (cmd == VFIO_DEVICE_FEATURE) {
-		struct vfio_device_feature feature;
-		uuid_t uuid;
-
-		minsz = offsetofend(struct vfio_device_feature, flags);
-
-		if (copy_from_user(&feature, (void __user *)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) {
-		case VFIO_DEVICE_FEATURE_PCI_VF_TOKEN:
-			if (!vdev->vf_token)
-				return -ENOTTY;
-
-			/*
-			 * We do not support GET of the VF Token UUID as this
-			 * could expose the token of the previous device user.
-			 */
-			if (feature.flags & VFIO_DEVICE_FEATURE_GET)
-				return -EINVAL;
-
-			if (feature.flags & VFIO_DEVICE_FEATURE_PROBE)
-				return 0;
-
-			/* Don't SET unless told to do so */
-			if (!(feature.flags & VFIO_DEVICE_FEATURE_SET))
-				return -EINVAL;
+	}
+	return -ENOTTY;
+}
+EXPORT_SYMBOL_GPL(vfio_pci_core_ioctl);
 
-			if (feature.argsz < minsz + sizeof(uuid))
-				return -EINVAL;
+int vfio_pci_core_ioctl_feature(struct vfio_device *device, u32 flags,
+				void __user *arg, size_t argsz)
+{
+	struct vfio_pci_core_device *vdev =
+		container_of(device, struct vfio_pci_core_device, vdev);
+	uuid_t uuid;
+	int ret;
 
-			if (copy_from_user(&uuid, (void __user *)(arg + minsz),
-					   sizeof(uuid)))
-				return -EFAULT;
+	switch (flags & VFIO_DEVICE_FEATURE_MASK) {
+	case VFIO_DEVICE_FEATURE_PCI_VF_TOKEN:
+		if (!vdev->vf_token)
+			return -ENOTTY;
+		/*
+		 * We do not support GET of the VF Token UUID as this could
+		 * expose the token of the previous device user.
+		 */
+		ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET,
+					sizeof(uuid));
+		if (ret != 1)
+			return ret;
 
-			mutex_lock(&vdev->vf_token->lock);
-			uuid_copy(&vdev->vf_token->uuid, &uuid);
-			mutex_unlock(&vdev->vf_token->lock);
+		if (copy_from_user(&uuid, arg, sizeof(uuid)))
+			return -EFAULT;
 
-			return 0;
-		default:
-			return -ENOTTY;
-		}
+		mutex_lock(&vdev->vf_token->lock);
+		uuid_copy(&vdev->vf_token->uuid, &uuid);
+		mutex_unlock(&vdev->vf_token->lock);
+		return 0;
+	default:
+		return -ENOTTY;
 	}
-
-	return -ENOTTY;
 }
-EXPORT_SYMBOL_GPL(vfio_pci_core_ioctl);
+EXPORT_SYMBOL_GPL(vfio_pci_core_ioctl_feature);
 
 static ssize_t vfio_pci_rw(struct vfio_pci_core_device *vdev, char __user *buf,
 			   size_t count, loff_t *ppos, bool iswrite)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 735d1d344af9..71763e2ac561 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1557,15 +1557,53 @@  static int vfio_device_fops_release(struct inode *inode, struct file *filep)
 	return 0;
 }
 
+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);
+	}
 }
 
 static ssize_t vfio_device_fops_read(struct file *filep, char __user *buf,
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 76191d7abed1..ca69516f869d 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -55,6 +55,7 @@  struct vfio_device {
  * @match: Optional device name match callback (return: 0 for no-match, >0 for
  *         match, -errno for abort (ex. match with insufficient or incorrect
  *         additional args)
+ * @device_feature: Fill in the VFIO_DEVICE_FEATURE ioctl
  */
 struct vfio_device_ops {
 	char	*name;
@@ -69,8 +70,39 @@  struct vfio_device_ops {
 	int	(*mmap)(struct vfio_device *vdev, struct vm_area_struct *vma);
 	void	(*request)(struct vfio_device *vdev, unsigned int count);
 	int	(*match)(struct vfio_device *vdev, char *buf);
+	int	(*device_feature)(struct vfio_device *device, u32 flags,
+				  void __user *arg, size_t argsz);
 };
 
+/**
+ * vfio_check_feature - Validate user input for the VFIO_DEVICE_FEATURE ioctl
+ * @flags: Arg from the device_feature op
+ * @argsz: Arg from the device_feature op
+ * @supported_ops: Combination of VFIO_DEVICE_FEATURE_GET and SET the driver
+ *                 supports
+ * @minsz: Minimum data size the driver accepts
+ *
+ * For use in a driver's device_feature op. Checks that the inputs to the
+ * VFIO_DEVICE_FEATURE ioctl are correct for the driver's feature. Returns 1 if
+ * the driver should execute the get or set, otherwise the relevant
+ * value should be returned.
+ */
+static inline int vfio_check_feature(u32 flags, size_t argsz, u32 supported_ops,
+				    size_t minsz)
+{
+	if ((flags & (VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_SET)) &
+	    ~supported_ops)
+		return -EINVAL;
+	if (flags & VFIO_DEVICE_FEATURE_PROBE)
+		return 0;
+	/* Without PROBE one of GET or SET must be requested */
+	if (!(flags & (VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_SET)))
+		return -EINVAL;
+	if (argsz < minsz)
+		return -EINVAL;
+	return 1;
+}
+
 void vfio_init_group_dev(struct vfio_device *device, struct device *dev,
 			 const struct vfio_device_ops *ops);
 void vfio_uninit_group_dev(struct vfio_device *device);
diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
index ef9a44b6cf5d..beba0b2ed87d 100644
--- a/include/linux/vfio_pci_core.h
+++ b/include/linux/vfio_pci_core.h
@@ -220,6 +220,8 @@  int vfio_pci_core_sriov_configure(struct pci_dev *pdev, int nr_virtfn);
 extern const struct pci_error_handlers vfio_pci_core_err_handlers;
 long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd,
 		unsigned long arg);
+int vfio_pci_core_ioctl_feature(struct vfio_device *device, u32 flags,
+				void __user *arg, size_t argsz);
 ssize_t vfio_pci_core_read(struct vfio_device *core_vdev, char __user *buf,
 		size_t count, loff_t *ppos);
 ssize_t vfio_pci_core_write(struct vfio_device *core_vdev, const char __user *buf,