[4/7] vfio: Introduce VFIO_DEVICE_FEATURE ioctl and first user
diff mbox series

Message ID 158146235133.16827.7215789038918853214.stgit@gimli.home
State New
Headers show
Series
  • vfio/pci: SR-IOV support
Related show

Commit Message

Alex Williamson Feb. 11, 2020, 11:05 p.m. UTC
The VFIO_DEVICE_FEATURE ioctl is meant to be a general purpose, device
agnostic ioctl for setting, retrieving, and probing device features.
This implementation provides a 16-bit field for specifying a feature
index, where the data porition of the ioctl is determined by the
semantics for the given feature.  Additional flag bits indicate the
direction and nature of the operation; SET indicates user data is
provided into the device feature, GET indicates the device feature is
written out into user data.  The PROBE flag augments determining
whether the given feature is supported, and if provided, whether the
given operation on the feature is supported.

The first user of this ioctl is for setting the vfio-pci VF token,
where the user provides a shared secret key (UUID) on a SR-IOV PF
device, which users must provide when opening associated VF devices.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/vfio/pci/vfio_pci.c |   52 +++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/vfio.h   |   37 +++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+)

Comments

Cornelia Huck Feb. 13, 2020, 12:41 p.m. UTC | #1
On Tue, 11 Feb 2020 16:05:51 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> The VFIO_DEVICE_FEATURE ioctl is meant to be a general purpose, device
> agnostic ioctl for setting, retrieving, and probing device features.
> This implementation provides a 16-bit field for specifying a feature
> index, where the data porition of the ioctl is determined by the
> semantics for the given feature.  Additional flag bits indicate the
> direction and nature of the operation; SET indicates user data is
> provided into the device feature, GET indicates the device feature is
> written out into user data.  The PROBE flag augments determining
> whether the given feature is supported, and if provided, whether the
> given operation on the feature is supported.
> 
> The first user of this ioctl is for setting the vfio-pci VF token,
> where the user provides a shared secret key (UUID) on a SR-IOV PF
> device, which users must provide when opening associated VF devices.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  drivers/vfio/pci/vfio_pci.c |   52 +++++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/vfio.h   |   37 +++++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+)

(...)

> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 9e843a147ead..c5cbf04ce5a7 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -707,6 +707,43 @@ struct vfio_device_ioeventfd {
>  
>  #define VFIO_DEVICE_IOEVENTFD		_IO(VFIO_TYPE, VFIO_BASE + 16)
>  
> +/**
> + * VFIO_DEVICE_FEATURE - _IORW(VFIO_TYPE, VFIO_BASE + 17,
> + *			       struct vfio_device_feature

Missing ')'

> + *
> + * Get, set, or probe feature data of the device.  The feature is selected
> + * using the FEATURE_MASK portion of the flags field.  Support for a feature
> + * can be probed by setting both the FEATURE_MASK and PROBE bits.  A probe
> + * may optionally include the GET and/or SET bits to determine read vs write
> + * access of the feature respectively.  Probing a feature will return success
> + * if the feature is supported and all of the optionally indicated GET/SET
> + * methods are supported.  The format of the data portion of the structure is

If neither GET nor SET are specified, will it return success if any of
the two are supported?

> + * specific to the given feature.  The data portion is not required for
> + * probing.
> + *
> + * Return 0 on success, -errno on failure.
> + */
> +struct vfio_device_feature {
> +	__u32	argsz;
> +	__u32	flags;
> +#define VFIO_DEVICE_FEATURE_MASK	(0xffff) /* 16-bit feature index */
> +#define VFIO_DEVICE_FEATURE_GET		(1 << 16) /* Get feature into data[] */
> +#define VFIO_DEVICE_FEATURE_SET		(1 << 17) /* Set feature from data[] */
> +#define VFIO_DEVICE_FEATURE_PROBE	(1 << 18) /* Probe feature support */
> +	__u8	data[];
> +};

I'm not sure I'm a fan of cramming both feature selection and operation
selection into flags. What about:

struct vfio_device_feature {
	__u32 argsz;
	__u32 flags;
/* GET/SET/PROBE #defines */
	__u32 feature;
	__u8  data[];
};

Getting/setting more than one feature at the same time does not sound
like a common use case; you would need to specify some kind of
algorithm for that anyway, and just doing it individually seems much
easier than that.

> +
> +#define VFIO_DEVICE_FEATURE		_IO(VFIO_TYPE, VFIO_BASE + 17)
> +
> +/*
> + * Provide support for setting a PCI VF Token, which is used as a shared
> + * secret between PF and VF drivers.  This feature may only be set on a
> + * PCI SR-IOV PF when SR-IOV is enabled on the PF and there are no existing
> + * open VFs.  Data provided when setting this feature is a 16-byte array
> + * (__u8 b[16]), representing a UUID.

No objection to that.

> + */
> +#define VFIO_DEVICE_FEATURE_PCI_VF_TOKEN	(0)
> +
>  /* -------- API for Type1 VFIO IOMMU -------- */
>  
>  /**
>
Alex Williamson Feb. 13, 2020, 5:39 p.m. UTC | #2
On Thu, 13 Feb 2020 13:41:21 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 11 Feb 2020 16:05:51 -0700
> Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> > The VFIO_DEVICE_FEATURE ioctl is meant to be a general purpose, device
> > agnostic ioctl for setting, retrieving, and probing device features.
> > This implementation provides a 16-bit field for specifying a feature
> > index, where the data porition of the ioctl is determined by the
> > semantics for the given feature.  Additional flag bits indicate the
> > direction and nature of the operation; SET indicates user data is
> > provided into the device feature, GET indicates the device feature is
> > written out into user data.  The PROBE flag augments determining
> > whether the given feature is supported, and if provided, whether the
> > given operation on the feature is supported.
> > 
> > The first user of this ioctl is for setting the vfio-pci VF token,
> > where the user provides a shared secret key (UUID) on a SR-IOV PF
> > device, which users must provide when opening associated VF devices.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >  drivers/vfio/pci/vfio_pci.c |   52 +++++++++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/vfio.h   |   37 +++++++++++++++++++++++++++++++
> >  2 files changed, 89 insertions(+)  
> 
> (...)
> 
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 9e843a147ead..c5cbf04ce5a7 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -707,6 +707,43 @@ struct vfio_device_ioeventfd {
> >  
> >  #define VFIO_DEVICE_IOEVENTFD		_IO(VFIO_TYPE, VFIO_BASE + 16)
> >  
> > +/**
> > + * VFIO_DEVICE_FEATURE - _IORW(VFIO_TYPE, VFIO_BASE + 17,
> > + *			       struct vfio_device_feature  
> 
> Missing ')'

Fixed.
 
> > + *
> > + * Get, set, or probe feature data of the device.  The feature is selected
> > + * using the FEATURE_MASK portion of the flags field.  Support for a feature
> > + * can be probed by setting both the FEATURE_MASK and PROBE bits.  A probe
> > + * may optionally include the GET and/or SET bits to determine read vs write
> > + * access of the feature respectively.  Probing a feature will return success
> > + * if the feature is supported and all of the optionally indicated GET/SET
> > + * methods are supported.  The format of the data portion of the structure is  
> 
> If neither GET nor SET are specified, will it return success if any of
> the two are supported?

Yes, that's how I've implemented this first feature.

> > + * specific to the given feature.  The data portion is not required for
> > + * probing.
> > + *
> > + * Return 0 on success, -errno on failure.
> > + */
> > +struct vfio_device_feature {
> > +	__u32	argsz;
> > +	__u32	flags;
> > +#define VFIO_DEVICE_FEATURE_MASK	(0xffff) /* 16-bit feature index */
> > +#define VFIO_DEVICE_FEATURE_GET		(1 << 16) /* Get feature into data[] */
> > +#define VFIO_DEVICE_FEATURE_SET		(1 << 17) /* Set feature from data[] */
> > +#define VFIO_DEVICE_FEATURE_PROBE	(1 << 18) /* Probe feature support */
> > +	__u8	data[];
> > +};  
> 
> I'm not sure I'm a fan of cramming both feature selection and operation
> selection into flags. What about:
> 
> struct vfio_device_feature {
> 	__u32 argsz;
> 	__u32 flags;
> /* GET/SET/PROBE #defines */
> 	__u32 feature;
> 	__u8  data[];
> };

Then data is unaligned so we either need to expand feature or add
padding.  So this makes the structure at least 8 bytes bigger and buys
us...?  What's so special about the bottom half of flags that we can't
designate it as the flags that specify the feature?  We still have
another 13 bits of flags for future use.

> Getting/setting more than one feature at the same time does not sound
> like a common use case; you would need to specify some kind of
> algorithm for that anyway, and just doing it individually seems much
> easier than that.

Yup.  I just figured 2^16 features is a nice way to make use of the
structure vs 2^32 features and 4 bytes of padding or 2^64 features.  I
don't think I'm being optimistic in thinking we'll have far less than
16K features and we can always reserve feature 0xffff as an extended
feature where the first 8-bytes of data defines that extended feature
index.

> > +
> > +#define VFIO_DEVICE_FEATURE		_IO(VFIO_TYPE, VFIO_BASE + 17)
> > +
> > +/*
> > + * Provide support for setting a PCI VF Token, which is used as a shared
> > + * secret between PF and VF drivers.  This feature may only be set on a
> > + * PCI SR-IOV PF when SR-IOV is enabled on the PF and there are no existing
> > + * open VFs.  Data provided when setting this feature is a 16-byte array
> > + * (__u8 b[16]), representing a UUID.  
> 
> No objection to that.

:)  Thanks!

Alex
Cornelia Huck Feb. 13, 2020, 6:08 p.m. UTC | #3
On Thu, 13 Feb 2020 10:39:57 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Thu, 13 Feb 2020 13:41:21 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Tue, 11 Feb 2020 16:05:51 -0700
> > Alex Williamson <alex.williamson@redhat.com> wrote:

> > > +struct vfio_device_feature {
> > > +	__u32	argsz;
> > > +	__u32	flags;
> > > +#define VFIO_DEVICE_FEATURE_MASK	(0xffff) /* 16-bit feature index */
> > > +#define VFIO_DEVICE_FEATURE_GET		(1 << 16) /* Get feature into data[] */
> > > +#define VFIO_DEVICE_FEATURE_SET		(1 << 17) /* Set feature from data[] */
> > > +#define VFIO_DEVICE_FEATURE_PROBE	(1 << 18) /* Probe feature support */
> > > +	__u8	data[];
> > > +};    
> > 
> > I'm not sure I'm a fan of cramming both feature selection and operation
> > selection into flags. What about:
> > 
> > struct vfio_device_feature {
> > 	__u32 argsz;
> > 	__u32 flags;
> > /* GET/SET/PROBE #defines */
> > 	__u32 feature;
> > 	__u8  data[];
> > };  
> 
> Then data is unaligned so we either need to expand feature or add
> padding.  So this makes the structure at least 8 bytes bigger and buys
> us...?  What's so special about the bottom half of flags that we can't
> designate it as the flags that specify the feature?  We still have
> another 13 bits of flags for future use.

It is more my general dislike of bit fiddling here, no strong
objection, certainly.

> 
> > Getting/setting more than one feature at the same time does not sound
> > like a common use case; you would need to specify some kind of
> > algorithm for that anyway, and just doing it individually seems much
> > easier than that.  
> 
> Yup.  I just figured 2^16 features is a nice way to make use of the
> structure vs 2^32 features and 4 bytes of padding or 2^64 features.  I
> don't think I'm being optimistic in thinking we'll have far less than
> 16K features and we can always reserve feature 0xffff as an extended
> feature where the first 8-bytes of data defines that extended feature
> index.

Agreed, we're probably not going to end up with a flood of features
here.

Anyway, much of this seems to be a matter of personal taste, so let's
keep it as it is.

Patch
diff mbox series

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 26aea9ac4863..5414744a3ead 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1171,6 +1171,58 @@  static long vfio_pci_ioctl(void *device_data,
 
 		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;
+
+		if (feature.flags & ~(VFIO_DEVICE_FEATURE_MASK |
+				      VFIO_DEVICE_FEATURE_SET |
+				      VFIO_DEVICE_FEATURE_GET |
+				      VFIO_DEVICE_FEATURE_PROBE))
+			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;
+
+			if (feature.argsz < minsz + sizeof(uuid))
+				return -EINVAL;
+
+			if (copy_from_user(&uuid, (void __user *)(arg + minsz),
+					   sizeof(uuid)))
+				return -EFAULT;
+
+			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;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 9e843a147ead..c5cbf04ce5a7 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -707,6 +707,43 @@  struct vfio_device_ioeventfd {
 
 #define VFIO_DEVICE_IOEVENTFD		_IO(VFIO_TYPE, VFIO_BASE + 16)
 
+/**
+ * VFIO_DEVICE_FEATURE - _IORW(VFIO_TYPE, VFIO_BASE + 17,
+ *			       struct vfio_device_feature
+ *
+ * Get, set, or probe feature data of the device.  The feature is selected
+ * using the FEATURE_MASK portion of the flags field.  Support for a feature
+ * can be probed by setting both the FEATURE_MASK and PROBE bits.  A probe
+ * may optionally include the GET and/or SET bits to determine read vs write
+ * access of the feature respectively.  Probing a feature will return success
+ * if the feature is supported and all of the optionally indicated GET/SET
+ * methods are supported.  The format of the data portion of the structure is
+ * specific to the given feature.  The data portion is not required for
+ * probing.
+ *
+ * Return 0 on success, -errno on failure.
+ */
+struct vfio_device_feature {
+	__u32	argsz;
+	__u32	flags;
+#define VFIO_DEVICE_FEATURE_MASK	(0xffff) /* 16-bit feature index */
+#define VFIO_DEVICE_FEATURE_GET		(1 << 16) /* Get feature into data[] */
+#define VFIO_DEVICE_FEATURE_SET		(1 << 17) /* Set feature from data[] */
+#define VFIO_DEVICE_FEATURE_PROBE	(1 << 18) /* Probe feature support */
+	__u8	data[];
+};
+
+#define VFIO_DEVICE_FEATURE		_IO(VFIO_TYPE, VFIO_BASE + 17)
+
+/*
+ * Provide support for setting a PCI VF Token, which is used as a shared
+ * secret between PF and VF drivers.  This feature may only be set on a
+ * PCI SR-IOV PF when SR-IOV is enabled on the PF and there are no existing
+ * open VFs.  Data provided when setting this feature is a 16-byte array
+ * (__u8 b[16]), representing a UUID.
+ */
+#define VFIO_DEVICE_FEATURE_PCI_VF_TOKEN	(0)
+
 /* -------- API for Type1 VFIO IOMMU -------- */
 
 /**