[2/2] drm/i915/gvt: export mdev device version to sysfs for Intel vGPU

Submitted by Zhao, Yan Y on April 19, 2019, 8:35 a.m.

Details

Message ID 20190419083559.19725-1-yan.y.zhao@intel.com
State New
Headers show
Series "introduction of version attribute for VFIO live migration" ( rev: 1 ) in Intel GVT devel

Not browsing as part of any series.

Commit Message

Zhao, Yan Y April 19, 2019, 8:35 a.m.
This feature implements the version attribute for Intel's vGPU mdev
devices.

version attribute is rw. It is queried by userspace software like libvirt
to check whether two vGPUs are compatible for live migration.

It consists of two parts: common part and vendor proprietary part.
common part: 32 bit. lower 16 bits is vendor id and higher 16 bits
             identifies device type. e.g., for pci device, it is
             "pci vendor id" | (VFIO_DEVICE_FLAGS_PCI << 16).
vendor proprietary part: this part is varied in length. vendor driver can
             specify any string to identify a device.

For Intel vGPU of gen8 and gen9, the vendor proprietary part currently
consists of 2 fields: "device id" + "mdev type".

Reading from a vGPU's version attribute, a string is returned in below
format: 00028086-<device id>-<mdev type>. e.g.
00028086-193b-i915-GVTg_V5_2.

Writing a string to a vGPU's version attribute will trigger GVT to check
whether a vGPU identified by the written string is compatible with
current vGPU owning this version attribute. errno is returned if the two
vGPUs are incompatible. The length of written string is returned in
compatible case.

For other platforms, and for GVT not supporting vGPU live migration
feature, errnos are returned when read/write of mdev devices' version
attributes.

For old GVT versions where no version attributes exposed in sysfs, it is
regarded as not supporting vGPU live migration.

For future platforms, besides the current 2 fields in vendor proprietary
part, more fields may be added to identify Intel vGPU well for live
migration purpose.

Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Erik Skultety <eskultet@redhat.com>
Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: "Tian, Kevin" <kevin.tian@intel.com>
Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
Cc: "Wang, Zhi A" <zhi.a.wang@intel.com>
c: Neo Jia <cjia@nvidia.com>
Cc: Kirti Wankhede <kwankhede@nvidia.com>

Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
---
 drivers/gpu/drm/i915/gvt/Makefile         |  2 +-
 drivers/gpu/drm/i915/gvt/device_version.c | 94 +++++++++++++++++++++++
 drivers/gpu/drm/i915/gvt/gvt.c            | 55 +++++++++++++
 drivers/gpu/drm/i915/gvt/gvt.h            |  6 ++
 4 files changed, 156 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/i915/gvt/device_version.c

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile
index 271fb46d4dd0..54e209a23899 100644
--- a/drivers/gpu/drm/i915/gvt/Makefile
+++ b/drivers/gpu/drm/i915/gvt/Makefile
@@ -3,7 +3,7 @@  GVT_DIR := gvt
 GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o firmware.o \
 	interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \
 	execlist.o scheduler.o sched_policy.o mmio_context.o cmd_parser.o debugfs.o \
-	fb_decoder.o dmabuf.o page_track.o
+	fb_decoder.o dmabuf.o page_track.o device_version.o
 
 ccflags-y				+= -I$(src) -I$(src)/$(GVT_DIR)
 i915-y					+= $(addprefix $(GVT_DIR)/, $(GVT_SOURCE))
diff --git a/drivers/gpu/drm/i915/gvt/device_version.c b/drivers/gpu/drm/i915/gvt/device_version.c
new file mode 100644
index 000000000000..c64010d2bc54
--- /dev/null
+++ b/drivers/gpu/drm/i915/gvt/device_version.c
@@ -0,0 +1,94 @@ 
+/*
+ * Copyright(c) 2011-2017 Intel Corporation. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+#include <linux/vfio.h>
+#include "i915_drv.h"
+
+#define GVT_VFIO_DEVICE_VENDOR_ID ((0x8086) |	\
+		((VFIO_DEVICE_FLAGS_PCI & 0xff) << 16))
+
+#define GVT_DEVICE_VERSION_COMMON_LEN 0x8
+#define GVT_DEVICE_VERSION_DEVICE_ID_LEN 0x4
+
+static bool is_compatible(const char *self, const char *remote)
+{
+	if (strlen(remote) != strlen(self))
+		return false;
+
+	return (strncmp(self, remote, strlen(self))) ? false : true;
+}
+
+ssize_t intel_gvt_get_vfio_device_version_len(struct drm_i915_private *dev_priv)
+{
+	if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9))
+		return -ENODEV;
+
+	return PAGE_SIZE;
+}
+
+ssize_t intel_gvt_get_vfio_device_version(struct drm_i915_private *dev_priv,
+		char *buf, const char *mdev_type)
+{
+	int cnt = 0, ret = 0;
+	const char *str = NULL;
+
+	/* currently only gen8 & gen9 are supported */
+	if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9))
+		return -ENODEV;
+
+	/* first 32 bit common part specifying vendor id and it's a pci
+	 * device
+	 */
+	cnt = snprintf(buf, GVT_DEVICE_VERSION_COMMON_LEN + 1,
+			"%08x", GVT_VFIO_DEVICE_VENDOR_ID);
+	buf += cnt;
+	ret += cnt;
+
+	/* vendor proprietary part: device id + mdev type */
+	/* device id */
+	cnt = snprintf(buf, GVT_DEVICE_VERSION_DEVICE_ID_LEN + 2,
+			"-%04x", INTEL_DEVID(dev_priv));
+	buf += cnt;
+	ret += cnt;
+
+	/* mdev type */
+	str = mdev_type;
+	cnt = snprintf(buf, strlen(str) + 3, "-%s\n", mdev_type);
+	buf += cnt;
+	ret += cnt;
+
+	return ret;
+}
+
+ssize_t intel_gvt_check_vfio_device_version(struct drm_i915_private *dev_priv,
+		const char *self, const char *remote)
+{
+
+	/* currently only gen8 & gen9 are supported */
+	if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9))
+		return -ENODEV;
+
+	if (!is_compatible(self, remote))
+		return -EINVAL;
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
index 43f4242062dd..e720465b93d8 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.c
+++ b/drivers/gpu/drm/i915/gvt/gvt.c
@@ -105,14 +105,69 @@  static ssize_t description_show(struct kobject *kobj, struct device *dev,
 		       type->weight);
 }
 
+static ssize_t version_show(struct kobject *kobj, struct device *dev,
+		char *buf)
+{
+#ifdef GVT_MIGRATION_VERSION
+	struct drm_i915_private *i915 = kdev_to_i915(dev);
+	const char *mdev_type = kobject_name(kobj);
+
+	return intel_gvt_get_vfio_device_version(i915, buf, mdev_type);
+#else
+	/* do not support live migration */
+	return -EINVAL;
+#endif
+}
+
+static ssize_t version_store(struct kobject *kobj, struct device *dev,
+		const char *buf, size_t count)
+{
+#ifdef GVT_MIGRATION_VERSION
+	char *remote = NULL, *self = NULL;
+	int len, ret = 0;
+	struct drm_i915_private *i915 = kdev_to_i915(dev);
+	const char *mdev_type = kobject_name(kobj);
+
+	len = intel_gvt_get_vfio_device_version_len(i915);
+	if (len < 0)
+		return len;
+
+	self = kmalloc(len, GFP_KERNEL);
+	if (!self)
+		return -ENOMEM;
+
+	ret = intel_gvt_get_vfio_device_version(i915, self, mdev_type);
+	if (ret < 0)
+		goto out;
+
+	remote = kstrndup(buf, count, GFP_KERNEL);
+	if (!remote) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	ret = intel_gvt_check_vfio_device_version(i915, self, remote);
+
+out:
+	kfree(self);
+	kfree(remote);
+	return (ret < 0 ? ret : count);
+#else
+	/* do not support live migration */
+	return -EINVAL;
+#endif
+}
+
 static MDEV_TYPE_ATTR_RO(available_instances);
 static MDEV_TYPE_ATTR_RO(device_api);
 static MDEV_TYPE_ATTR_RO(description);
+static MDEV_TYPE_ATTR_RW(version);
 
 static struct attribute *gvt_type_attrs[] = {
 	&mdev_type_attr_available_instances.attr,
 	&mdev_type_attr_device_api.attr,
 	&mdev_type_attr_description.attr,
+	&mdev_type_attr_version.attr,
 	NULL,
 };
 
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index f5a328b5290a..4062f6b26acf 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.h
+++ b/drivers/gpu/drm/i915/gvt/gvt.h
@@ -687,6 +687,12 @@  void intel_gvt_debugfs_remove_vgpu(struct intel_vgpu *vgpu);
 int intel_gvt_debugfs_init(struct intel_gvt *gvt);
 void intel_gvt_debugfs_clean(struct intel_gvt *gvt);
 
+ssize_t intel_gvt_get_vfio_device_version(struct drm_i915_private *i915,
+		char *buf, const char *mdev_type);
+ssize_t intel_gvt_check_vfio_device_version(struct drm_i915_private *dev_priv,
+		const char *self, const char *remote);
+ssize_t
+intel_gvt_get_vfio_device_version_len(struct drm_i915_private *dev_priv);
 
 #include "trace.h"
 #include "mpt.h"

Comments


On Mon, Apr 22, 2019 at 04:37:39PM +0800, Zhenyu Wang wrote:
> On 2019.04.19 04:35:59 -0400, Yan Zhao wrote:
> > This feature implements the version attribute for Intel's vGPU mdev
> > devices.
> > 
> > version attribute is rw. It is queried by userspace software like libvirt
> > to check whether two vGPUs are compatible for live migration.
> > 
> > It consists of two parts: common part and vendor proprietary part.
> > common part: 32 bit. lower 16 bits is vendor id and higher 16 bits
> >              identifies device type. e.g., for pci device, it is
> >              "pci vendor id" | (VFIO_DEVICE_FLAGS_PCI << 16).
> > vendor proprietary part: this part is varied in length. vendor driver can
> >              specify any string to identify a device.
> > 
> > For Intel vGPU of gen8 and gen9, the vendor proprietary part currently
> > consists of 2 fields: "device id" + "mdev type".
> > 
> > Reading from a vGPU's version attribute, a string is returned in below
> > format: 00028086-<device id>-<mdev type>. e.g.
> > 00028086-193b-i915-GVTg_V5_2.
> > 
> > Writing a string to a vGPU's version attribute will trigger GVT to check
> > whether a vGPU identified by the written string is compatible with
> > current vGPU owning this version attribute. errno is returned if the two
> > vGPUs are incompatible. The length of written string is returned in
> > compatible case.
> > 
> > For other platforms, and for GVT not supporting vGPU live migration
> > feature, errnos are returned when read/write of mdev devices' version
> > attributes.
> > 
> > For old GVT versions where no version attributes exposed in sysfs, it is
> > regarded as not supporting vGPU live migration.
> > 
> > For future platforms, besides the current 2 fields in vendor proprietary
> > part, more fields may be added to identify Intel vGPU well for live
> > migration purpose.
> > 
> > Cc: Alex Williamson <alex.williamson@redhat.com>
> > Cc: Erik Skultety <eskultet@redhat.com>
> > Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > Cc: Cornelia Huck <cohuck@redhat.com>
> > Cc: "Tian, Kevin" <kevin.tian@intel.com>
> > Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> > Cc: "Wang, Zhi A" <zhi.a.wang@intel.com>
> > c: Neo Jia <cjia@nvidia.com>
> > Cc: Kirti Wankhede <kwankhede@nvidia.com>
> > 
> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gvt/Makefile         |  2 +-
> >  drivers/gpu/drm/i915/gvt/device_version.c | 94 +++++++++++++++++++++++
> >  drivers/gpu/drm/i915/gvt/gvt.c            | 55 +++++++++++++
> >  drivers/gpu/drm/i915/gvt/gvt.h            |  6 ++
> >  4 files changed, 156 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/i915/gvt/device_version.c
> > 
> > diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile
> > index 271fb46d4dd0..54e209a23899 100644
> > --- a/drivers/gpu/drm/i915/gvt/Makefile
> > +++ b/drivers/gpu/drm/i915/gvt/Makefile
> > @@ -3,7 +3,7 @@ GVT_DIR := gvt
> >  GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o firmware.o \
> >  	interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \
> >  	execlist.o scheduler.o sched_policy.o mmio_context.o cmd_parser.o debugfs.o \
> > -	fb_decoder.o dmabuf.o page_track.o
> > +	fb_decoder.o dmabuf.o page_track.o device_version.o
> >  
> >  ccflags-y				+= -I$(src) -I$(src)/$(GVT_DIR)
> >  i915-y					+= $(addprefix $(GVT_DIR)/, $(GVT_SOURCE))
> > diff --git a/drivers/gpu/drm/i915/gvt/device_version.c b/drivers/gpu/drm/i915/gvt/device_version.c
> > new file mode 100644
> > index 000000000000..c64010d2bc54
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/gvt/device_version.c
> > @@ -0,0 +1,94 @@
> > +/*
> > + * Copyright(c) 2011-2017 Intel Corporation. All rights reserved.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> > + * SOFTWARE.
> > + */
> > +#include <linux/vfio.h>
> > +#include "i915_drv.h"
> > +
> > +#define GVT_VFIO_DEVICE_VENDOR_ID ((0x8086) |	\
> > +		((VFIO_DEVICE_FLAGS_PCI & 0xff) << 16))
> > +
> > +#define GVT_DEVICE_VERSION_COMMON_LEN 0x8
> > +#define GVT_DEVICE_VERSION_DEVICE_ID_LEN 0x4
> > +
> > +static bool is_compatible(const char *self, const char *remote)
> > +{
> > +	if (strlen(remote) != strlen(self))
> > +		return false;
> > +
> > +	return (strncmp(self, remote, strlen(self))) ? false : true;
> > +}
> > +
> > +ssize_t intel_gvt_get_vfio_device_version_len(struct drm_i915_private *dev_priv)
> > +{
> > +	if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9))
> > +		return -ENODEV;
> > +
> > +	return PAGE_SIZE;
> > +}
> > +
> > +ssize_t intel_gvt_get_vfio_device_version(struct drm_i915_private *dev_priv,
> > +		char *buf, const char *mdev_type)
> > +{
> > +	int cnt = 0, ret = 0;
> > +	const char *str = NULL;
> > +
> > +	/* currently only gen8 & gen9 are supported */
> > +	if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9))
> > +		return -ENODEV;
> > +
> > +	/* first 32 bit common part specifying vendor id and it's a pci
> > +	 * device
> > +	 */
> > +	cnt = snprintf(buf, GVT_DEVICE_VERSION_COMMON_LEN + 1,
> > +			"%08x", GVT_VFIO_DEVICE_VENDOR_ID);
> > +	buf += cnt;
> > +	ret += cnt;
> > +
> > +	/* vendor proprietary part: device id + mdev type */
> > +	/* device id */
> > +	cnt = snprintf(buf, GVT_DEVICE_VERSION_DEVICE_ID_LEN + 2,
> > +			"-%04x", INTEL_DEVID(dev_priv));
> > +	buf += cnt;
> > +	ret += cnt;
> > +
> > +	/* mdev type */
> > +	str = mdev_type;
> > +	cnt = snprintf(buf, strlen(str) + 3, "-%s\n", mdev_type);
> > +	buf += cnt;
> > +	ret += cnt;
> > +
> > +	return ret;
> > +}
> > +
> > +ssize_t intel_gvt_check_vfio_device_version(struct drm_i915_private *dev_priv,
> > +		const char *self, const char *remote)
> > +{
> > +
> > +	/* currently only gen8 & gen9 are supported */
> > +	if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9))
> > +		return -ENODEV;
> > +
> > +	if (!is_compatible(self, remote))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
> > index 43f4242062dd..e720465b93d8 100644
> > --- a/drivers/gpu/drm/i915/gvt/gvt.c
> > +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> > @@ -105,14 +105,69 @@ static ssize_t description_show(struct kobject *kobj, struct device *dev,
> >  		       type->weight);
> >  }
> >  
> > +static ssize_t version_show(struct kobject *kobj, struct device *dev,
> > +		char *buf)
> > +{
> > +#ifdef GVT_MIGRATION_VERSION
> > +	struct drm_i915_private *i915 = kdev_to_i915(dev);
> > +	const char *mdev_type = kobject_name(kobj);
> > +
> > +	return intel_gvt_get_vfio_device_version(i915, buf, mdev_type);
> > +#else
> > +	/* do not support live migration */
> > +	return -EINVAL;
> > +#endif
> > +}
> > +
> > +static ssize_t version_store(struct kobject *kobj, struct device *dev,
> > +		const char *buf, size_t count)
> > +{
> > +#ifdef GVT_MIGRATION_VERSION
> > +	char *remote = NULL, *self = NULL;
> > +	int len, ret = 0;
> > +	struct drm_i915_private *i915 = kdev_to_i915(dev);
> > +	const char *mdev_type = kobject_name(kobj);
> > +
> > +	len = intel_gvt_get_vfio_device_version_len(i915);
> > +	if (len < 0)
> > +		return len;
> > +
> > +	self = kmalloc(len, GFP_KERNEL);
> > +	if (!self)
> > +		return -ENOMEM;
> > +
> > +	ret = intel_gvt_get_vfio_device_version(i915, self, mdev_type);
> > +	if (ret < 0)
> > +		goto out;
> > +
> 
> device version string should be allocated and specified during device instance
> setup instead of version attribute r/w time.
> 
hi Zhenyu
thanks for this comment. But my consideration on not allocating at device
instance initialization time is that this version check is not frequent and
only for migration purpose. so we do not need to allocate and keep this memory all
the time for what may not happen at all.  Also this dynamic memory
allocation is at version check time, not in hot path.
So, I think it is better to keep version string memory allocation here.
Agree?

Thanks
Yan

> > +	remote = kstrndup(buf, count, GFP_KERNEL);
> > +	if (!remote) {
> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	ret = intel_gvt_check_vfio_device_version(i915, self, remote);
> > +
> > +out:
> > +	kfree(self);
> > +	kfree(remote);
> > +	return (ret < 0 ? ret : count);
> > +#else
> > +	/* do not support live migration */
> > +	return -EINVAL;
> > +#endif
> > +}
> > +
> >  static MDEV_TYPE_ATTR_RO(available_instances);
> >  static MDEV_TYPE_ATTR_RO(device_api);
> >  static MDEV_TYPE_ATTR_RO(description);
> > +static MDEV_TYPE_ATTR_RW(version);
> >  
> >  static struct attribute *gvt_type_attrs[] = {
> >  	&mdev_type_attr_available_instances.attr,
> >  	&mdev_type_attr_device_api.attr,
> >  	&mdev_type_attr_description.attr,
> > +	&mdev_type_attr_version.attr,
> >  	NULL,
> >  };
> >  
> > diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> > index f5a328b5290a..4062f6b26acf 100644
> > --- a/drivers/gpu/drm/i915/gvt/gvt.h
> > +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> > @@ -687,6 +687,12 @@ void intel_gvt_debugfs_remove_vgpu(struct intel_vgpu *vgpu);
> >  int intel_gvt_debugfs_init(struct intel_gvt *gvt);
> >  void intel_gvt_debugfs_clean(struct intel_gvt *gvt);
> >  
> > +ssize_t intel_gvt_get_vfio_device_version(struct drm_i915_private *i915,
> > +		char *buf, const char *mdev_type);
> > +ssize_t intel_gvt_check_vfio_device_version(struct drm_i915_private *dev_priv,
> > +		const char *self, const char *remote);
> > +ssize_t
> > +intel_gvt_get_vfio_device_version_len(struct drm_i915_private *dev_priv);
> >  
> >  #include "trace.h"
> >  #include "mpt.h"
> > -- 
> > 2.17.1
> > 
> > _______________________________________________
> > intel-gvt-dev mailing list
> > intel-gvt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> 
> -- 
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827


> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
On Fri, 19 Apr 2019 04:35:59 -0400
Yan Zhao <yan.y.zhao@intel.com> wrote:

> This feature implements the version attribute for Intel's vGPU mdev
> devices.
> 
> version attribute is rw. It is queried by userspace software like libvirt
> to check whether two vGPUs are compatible for live migration.
> 
> It consists of two parts: common part and vendor proprietary part.
> common part: 32 bit. lower 16 bits is vendor id and higher 16 bits
>              identifies device type. e.g., for pci device, it is
>              "pci vendor id" | (VFIO_DEVICE_FLAGS_PCI << 16).
> vendor proprietary part: this part is varied in length. vendor driver can
>              specify any string to identify a device.
> 
> For Intel vGPU of gen8 and gen9, the vendor proprietary part currently
> consists of 2 fields: "device id" + "mdev type".
> 
> Reading from a vGPU's version attribute, a string is returned in below
> format: 00028086-<device id>-<mdev type>. e.g.
> 00028086-193b-i915-GVTg_V5_2.
> 
> Writing a string to a vGPU's version attribute will trigger GVT to check
> whether a vGPU identified by the written string is compatible with
> current vGPU owning this version attribute. errno is returned if the two
> vGPUs are incompatible. The length of written string is returned in
> compatible case.
> 
> For other platforms, and for GVT not supporting vGPU live migration
> feature, errnos are returned when read/write of mdev devices' version
> attributes.
> 
> For old GVT versions where no version attributes exposed in sysfs, it is
> regarded as not supporting vGPU live migration.
> 
> For future platforms, besides the current 2 fields in vendor proprietary
> part, more fields may be added to identify Intel vGPU well for live
> migration purpose.
> 
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Erik Skultety <eskultet@redhat.com>
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: "Tian, Kevin" <kevin.tian@intel.com>
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> Cc: "Wang, Zhi A" <zhi.a.wang@intel.com>
> c: Neo Jia <cjia@nvidia.com>
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> 
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/Makefile         |  2 +-
>  drivers/gpu/drm/i915/gvt/device_version.c | 94 +++++++++++++++++++++++
>  drivers/gpu/drm/i915/gvt/gvt.c            | 55 +++++++++++++
>  drivers/gpu/drm/i915/gvt/gvt.h            |  6 ++
>  4 files changed, 156 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/i915/gvt/device_version.c
> 

(...)

> +static bool is_compatible(const char *self, const char *remote)
> +{
> +	if (strlen(remote) != strlen(self))
> +		return false;
> +
> +	return (strncmp(self, remote, strlen(self))) ? false : true;
> +}
> +
> +ssize_t intel_gvt_get_vfio_device_version_len(struct drm_i915_private *dev_priv)
> +{
> +	if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9))
> +		return -ENODEV;
> +
> +	return PAGE_SIZE;
> +}
> +
> +ssize_t intel_gvt_get_vfio_device_version(struct drm_i915_private *dev_priv,
> +		char *buf, const char *mdev_type)
> +{
> +	int cnt = 0, ret = 0;
> +	const char *str = NULL;
> +
> +	/* currently only gen8 & gen9 are supported */
> +	if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9))
> +		return -ENODEV;
> +
> +	/* first 32 bit common part specifying vendor id and it's a pci
> +	 * device
> +	 */
> +	cnt = snprintf(buf, GVT_DEVICE_VERSION_COMMON_LEN + 1,
> +			"%08x", GVT_VFIO_DEVICE_VENDOR_ID);
> +	buf += cnt;
> +	ret += cnt;
> +
> +	/* vendor proprietary part: device id + mdev type */
> +	/* device id */
> +	cnt = snprintf(buf, GVT_DEVICE_VERSION_DEVICE_ID_LEN + 2,
> +			"-%04x", INTEL_DEVID(dev_priv));
> +	buf += cnt;
> +	ret += cnt;
> +
> +	/* mdev type */
> +	str = mdev_type;
> +	cnt = snprintf(buf, strlen(str) + 3, "-%s\n", mdev_type);
> +	buf += cnt;
> +	ret += cnt;
> +
> +	return ret;
> +}

Looking at this handling, it seems much easier to me to simply use a
numeric value instead of a string: You don't have to build it via
sprintf, there are generic functions for parsing a string input into a
simple number, and you have more options for compatibility (e.g.
"version must be between m and n" instead of an exact match).

If you still need to encode the device id here, you should be able to
easily do something like (device_id << 16) | migration_version -- do
you think that could work?

> +
> +ssize_t intel_gvt_check_vfio_device_version(struct drm_i915_private *dev_priv,
> +		const char *self, const char *remote)
> +{
> +
> +	/* currently only gen8 & gen9 are supported */
> +	if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9))
> +		return -ENODEV;
> +
> +	if (!is_compatible(self, remote))
> +		return -EINVAL;

I think the meaning of the error codes should really be standardized
across vendor drivers, if we need a value for "this device does not
support migration at all". (Your choices look reasonable for that.)

> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
> index 43f4242062dd..e720465b93d8 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.c
> +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> @@ -105,14 +105,69 @@ static ssize_t description_show(struct kobject *kobj, struct device *dev,
>  		       type->weight);
>  }
>  
> +static ssize_t version_show(struct kobject *kobj, struct device *dev,
> +		char *buf)
> +{
> +#ifdef GVT_MIGRATION_VERSION
> +	struct drm_i915_private *i915 = kdev_to_i915(dev);
> +	const char *mdev_type = kobject_name(kobj);
> +
> +	return intel_gvt_get_vfio_device_version(i915, buf, mdev_type);
> +#else
> +	/* do not support live migration */
> +	return -EINVAL;

...but this looks inconsistent. I would expect -ENODEV here, same as
for non-gen{8,9}.

Or simply do not create the attribute at all in that case?

> +#endif
> +}
> +
> +static ssize_t version_store(struct kobject *kobj, struct device *dev,
> +		const char *buf, size_t count)
> +{
> +#ifdef GVT_MIGRATION_VERSION
> +	char *remote = NULL, *self = NULL;
> +	int len, ret = 0;
> +	struct drm_i915_private *i915 = kdev_to_i915(dev);
> +	const char *mdev_type = kobject_name(kobj);
> +
> +	len = intel_gvt_get_vfio_device_version_len(i915);
> +	if (len < 0)
> +		return len;
> +
> +	self = kmalloc(len, GFP_KERNEL);
> +	if (!self)
> +		return -ENOMEM;
> +
> +	ret = intel_gvt_get_vfio_device_version(i915, self, mdev_type);
> +	if (ret < 0)
> +		goto out;
> +
> +	remote = kstrndup(buf, count, GFP_KERNEL);
> +	if (!remote) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ret = intel_gvt_check_vfio_device_version(i915, self, remote);
> +
> +out:
> +	kfree(self);
> +	kfree(remote);
> +	return (ret < 0 ? ret : count);
> +#else
> +	/* do not support live migration */
> +	return -EINVAL;
> +#endif
> +}
> +
>  static MDEV_TYPE_ATTR_RO(available_instances);
>  static MDEV_TYPE_ATTR_RO(device_api);
>  static MDEV_TYPE_ATTR_RO(description);
> +static MDEV_TYPE_ATTR_RW(version);
>  
>  static struct attribute *gvt_type_attrs[] = {
>  	&mdev_type_attr_available_instances.attr,
>  	&mdev_type_attr_device_api.attr,
>  	&mdev_type_attr_description.attr,
> +	&mdev_type_attr_version.attr,
>  	NULL,
>  };
>  
(...)
On Tue, Apr 23, 2019 at 07:39:11PM +0800, Cornelia Huck wrote:
> On Fri, 19 Apr 2019 04:35:59 -0400
> Yan Zhao <yan.y.zhao@intel.com> wrote:
> 
> > This feature implements the version attribute for Intel's vGPU mdev
> > devices.
> >
> > version attribute is rw. It is queried by userspace software like libvirt
> > to check whether two vGPUs are compatible for live migration.
> >
> > It consists of two parts: common part and vendor proprietary part.
> > common part: 32 bit. lower 16 bits is vendor id and higher 16 bits
> >              identifies device type. e.g., for pci device, it is
> >              "pci vendor id" | (VFIO_DEVICE_FLAGS_PCI << 16).
> > vendor proprietary part: this part is varied in length. vendor driver can
> >              specify any string to identify a device.
> >
> > For Intel vGPU of gen8 and gen9, the vendor proprietary part currently
> > consists of 2 fields: "device id" + "mdev type".
> >
> > Reading from a vGPU's version attribute, a string is returned in below
> > format: 00028086-<device id>-<mdev type>. e.g.
> > 00028086-193b-i915-GVTg_V5_2.
> >
> > Writing a string to a vGPU's version attribute will trigger GVT to check
> > whether a vGPU identified by the written string is compatible with
> > current vGPU owning this version attribute. errno is returned if the two
> > vGPUs are incompatible. The length of written string is returned in
> > compatible case.
> >
> > For other platforms, and for GVT not supporting vGPU live migration
> > feature, errnos are returned when read/write of mdev devices' version
> > attributes.
> >
> > For old GVT versions where no version attributes exposed in sysfs, it is
> > regarded as not supporting vGPU live migration.
> >
> > For future platforms, besides the current 2 fields in vendor proprietary
> > part, more fields may be added to identify Intel vGPU well for live
> > migration purpose.
> >
> > Cc: Alex Williamson <alex.williamson@redhat.com>
> > Cc: Erik Skultety <eskultet@redhat.com>
> > Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > Cc: Cornelia Huck <cohuck@redhat.com>
> > Cc: "Tian, Kevin" <kevin.tian@intel.com>
> > Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> > Cc: "Wang, Zhi A" <zhi.a.wang@intel.com>
> > c: Neo Jia <cjia@nvidia.com>
> > Cc: Kirti Wankhede <kwankhede@nvidia.com>
> >
> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gvt/Makefile         |  2 +-
> >  drivers/gpu/drm/i915/gvt/device_version.c | 94 +++++++++++++++++++++++
> >  drivers/gpu/drm/i915/gvt/gvt.c            | 55 +++++++++++++
> >  drivers/gpu/drm/i915/gvt/gvt.h            |  6 ++
> >  4 files changed, 156 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/i915/gvt/device_version.c
> >
> 
> (...)
> 
> > +static bool is_compatible(const char *self, const char *remote)
> > +{
> > +     if (strlen(remote) != strlen(self))
> > +             return false;
> > +
> > +     return (strncmp(self, remote, strlen(self))) ? false : true;
> > +}
> > +
> > +ssize_t intel_gvt_get_vfio_device_version_len(struct drm_i915_private *dev_priv)
> > +{
> > +     if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9))
> > +             return -ENODEV;
> > +
> > +     return PAGE_SIZE;
> > +}
> > +
> > +ssize_t intel_gvt_get_vfio_device_version(struct drm_i915_private *dev_priv,
> > +             char *buf, const char *mdev_type)
> > +{
> > +     int cnt = 0, ret = 0;
> > +     const char *str = NULL;
> > +
> > +     /* currently only gen8 & gen9 are supported */
> > +     if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9))
> > +             return -ENODEV;
> > +
> > +     /* first 32 bit common part specifying vendor id and it's a pci
> > +      * device
> > +      */
> > +     cnt = snprintf(buf, GVT_DEVICE_VERSION_COMMON_LEN + 1,
> > +                     "%08x", GVT_VFIO_DEVICE_VENDOR_ID);
> > +     buf += cnt;
> > +     ret += cnt;
> > +
> > +     /* vendor proprietary part: device id + mdev type */
> > +     /* device id */
> > +     cnt = snprintf(buf, GVT_DEVICE_VERSION_DEVICE_ID_LEN + 2,
> > +                     "-%04x", INTEL_DEVID(dev_priv));
> > +     buf += cnt;
> > +     ret += cnt;
> > +
> > +     /* mdev type */
> > +     str = mdev_type;
> > +     cnt = snprintf(buf, strlen(str) + 3, "-%s\n", mdev_type);
> > +     buf += cnt;
> > +     ret += cnt;
> > +
> > +     return ret;
> > +}
> 
> Looking at this handling, it seems much easier to me to simply use a
> numeric value instead of a string: You don't have to build it via
> sprintf, there are generic functions for parsing a string input into a
> simple number, and you have more options for compatibility (e.g.
> "version must be between m and n" instead of an exact match).
> 
> If you still need to encode the device id here, you should be able to
> easily do something like (device_id << 16) | migration_version -- do
> you think that could work?
>
hi Cornelia,
using string is based on the consideration that we want to make this
version string a thing that can distinguish a mdev, so we incoportate
vendor id, device id to identify parent device first, then mdev type to
describe the mdev based on the parent device. And that's only for gen8 and
gen9. For future platforms, we may incorpate more information, e.g. besides
vendor id and device id, different device revision number, or even a value
in a register on the run may be needed to identify a parent device. 

I think it's cleaner than a numeric version between m and n, because in that
case we have to maintain what m's configration is and what n's is. 
whenever a mdev type is added (like changing a resolution type in mdev
type) a new version is generated. it's too complicated.

That's why we use current way: version describe parent device + mdev type
elaborately, then vendor driver checks compatibility according to this
information.

Do you think it's all right?

> > +
> > +ssize_t intel_gvt_check_vfio_device_version(struct drm_i915_private *dev_priv,
> > +             const char *self, const char *remote)
> > +{
> > +
> > +     /* currently only gen8 & gen9 are supported */
> > +     if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9))
> > +             return -ENODEV;
> > +
> > +     if (!is_compatible(self, remote))
> > +             return -EINVAL;
> 
> I think the meaning of the error codes should really be standardized
> across vendor drivers, if we need a value for "this device does not
> support migration at all". (Your choices look reasonable for that.)
>
Agree. thank you. I'll keep error codes consistently in future.

> > +
> > +     return 0;
> > +}
> > diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
> > index 43f4242062dd..e720465b93d8 100644
> > --- a/drivers/gpu/drm/i915/gvt/gvt.c
> > +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> > @@ -105,14 +105,69 @@ static ssize_t description_show(struct kobject *kobj, struct device *dev,
> >                      type->weight);
> >  }
> >
> > +static ssize_t version_show(struct kobject *kobj, struct device *dev,
> > +             char *buf)
> > +{
> > +#ifdef GVT_MIGRATION_VERSION
> > +     struct drm_i915_private *i915 = kdev_to_i915(dev);
> > +     const char *mdev_type = kobject_name(kobj);
> > +
> > +     return intel_gvt_get_vfio_device_version(i915, buf, mdev_type);
> > +#else
> > +     /* do not support live migration */
> > +     return -EINVAL;
> 
> ...but this looks inconsistent. I would expect -ENODEV here, same as
> for non-gen{8,9}.
Right, case "not suppporting live migration" should return -ENODEV.
Thanks:)


> Or simply do not create the attribute at all in that case?
That's also a good choice :) 



> > +#endif
> > +}
> > +
> > +static ssize_t version_store(struct kobject *kobj, struct device *dev,
> > +             const char *buf, size_t count)
> > +{
> > +#ifdef GVT_MIGRATION_VERSION
> > +     char *remote = NULL, *self = NULL;
> > +     int len, ret = 0;
> > +     struct drm_i915_private *i915 = kdev_to_i915(dev);
> > +     const char *mdev_type = kobject_name(kobj);
> > +
> > +     len = intel_gvt_get_vfio_device_version_len(i915);
> > +     if (len < 0)
> > +             return len;
> > +
> > +     self = kmalloc(len, GFP_KERNEL);
> > +     if (!self)
> > +             return -ENOMEM;
> > +
> > +     ret = intel_gvt_get_vfio_device_version(i915, self, mdev_type);
> > +     if (ret < 0)
> > +             goto out;
> > +
> > +     remote = kstrndup(buf, count, GFP_KERNEL);
> > +     if (!remote) {
> > +             ret = -ENOMEM;
> > +             goto out;
> > +     }
> > +
> > +     ret = intel_gvt_check_vfio_device_version(i915, self, remote);
> > +
> > +out:
> > +     kfree(self);
> > +     kfree(remote);
> > +     return (ret < 0 ? ret : count);
> > +#else
> > +     /* do not support live migration */
> > +     return -EINVAL;
> > +#endif
> > +}
> > +
> >  static MDEV_TYPE_ATTR_RO(available_instances);
> >  static MDEV_TYPE_ATTR_RO(device_api);
> >  static MDEV_TYPE_ATTR_RO(description);
> > +static MDEV_TYPE_ATTR_RW(version);
> >
> >  static struct attribute *gvt_type_attrs[] = {
> >       &mdev_type_attr_available_instances.attr,
> >       &mdev_type_attr_device_api.attr,
> >       &mdev_type_attr_description.attr,
> > +     &mdev_type_attr_version.attr,
> >       NULL,
> >  };
> >
> (...)
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev