Re: [libvirt] [PATCH v3 2/2] drm/i915/gvt: export migration_version to mdev sysfs for Intel vGPU

On Sun, 26 May 2019 23:44:37 -0400 Yan Zhao <yan.y.zhao(a)intel.com&gt; wrote: > This feature implements the migration_version attribute for Intel's vGPU > mdev devices. > > migration_version attribute is rw. > It's used to check migration compatibility for two mdev devices of the > same mdev type. > migration_version string is defined by vendor driver and opaque to > userspace. > > For Intel vGPU of gen8 and gen9, the format of migration_version string > is: > <vendor id>-<device id>-<vgpu type>-<software version>. > > For future platforms, the format of migration_version string is to be > expanded to include more meta data to identify Intel vGPUs for live > migration compatibility check > > For old platforms, and for GVT not supporting vGPU live migration > feature, -ENODEV is returned on read(2)/write(2) of migration_version > attribute. > For vGPUs running old GVT who do not expose migration_version > attribute, live migration is regarded as not supported for those vGPUs. > > Cc: Alex Williamson <alex.williamson(a)redhat.com&gt; > Cc: Erik Skultety <eskultet(a)redhat.com&gt; > Cc: "Dr. David Alan Gilbert" <dgilbert(a)redhat.com&gt; > Cc: Cornelia Huck <cohuck(a)redhat.com&gt; > Cc: "Tian, Kevin" <kevin.tian(a)intel.com&gt; > Cc: Zhenyu Wang <zhenyuw(a)linux.intel.com&gt; > Cc: "Wang, Zhi A" <zhi.a.wang(a)intel.com&gt; > c: Neo Jia <cjia(a)nvidia.com&gt; > Cc: Kirti Wankhede <kwankhede(a)nvidia.com&gt; > > Signed-off-by: Yan Zhao <yan.y.zhao(a)intel.com&gt; > > --- > v3: > 1. renamed version to migration_version > (Christophe de Dinechin, Cornelia Huck, Alex Williamson) > 2. instead of generating migration version strings each time, storing > them in vgpu types generated during initialization. > (Zhenyu Wang, Cornelia Huck) > 3. replaced multiple snprintf to one big snprintf in > intel_gvt_get_vfio_migration_version() > (Dr. David Alan Gilbert) > 4. printed detailed error log > (Alex Williamson, Erik Skultety, Cornelia Huck, Dr. David Alan Gilbert) > 5. incorporated <software version> into migration_version string > (Alex Williamson) > 6. do not use ifndef macro to switch off migration_version attribute > (Zhenyu Wang) > > v2: > 1. removed 32 common part of version string > (Alex Williamson) > 2. do not register version attribute for GVT not supporting live > migration.(Cornelia Huck) > 3. for platforms out of gen8, gen9, return -EINVAL --> -ENODEV for > incompatible. (Cornelia Huck) > --- > drivers/gpu/drm/i915/gvt/Makefile | 2 +- > drivers/gpu/drm/i915/gvt/gvt.c | 39 +++++ > drivers/gpu/drm/i915/gvt/gvt.h | 5 + > drivers/gpu/drm/i915/gvt/migration_version.c | 167 +++++++++++++++++++ > drivers/gpu/drm/i915/gvt/vgpu.c | 13 +- > 5 files changed, 223 insertions(+), 3 deletions(-) > create mode 100644 drivers/gpu/drm/i915/gvt/migration_version.c > (...) > diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c > index 43f4242062dd..be2980e8ac75 100644 > --- a/drivers/gpu/drm/i915/gvt/gvt.c > +++ b/drivers/gpu/drm/i915/gvt/gvt.c > @@ -105,14 +105,53 @@ static ssize_t description_show(struct kobject *kobj, struct device *dev, > type->weight); > } > > +static ssize_t migration_version_show(struct kobject *kobj, struct device *dev, > + char *buf) Indentation looks a bit odd? (Also below.)

> +{ > + struct intel_vgpu_type *type; > + void *gvt = kdev_to_i915(dev)->gvt; > + > + type = intel_gvt_find_vgpu_type(gvt, kobject_name(kobj)); > + if (!type || !type->migration_version) { > + gvt_err("Does not support migraion on type %s. Please search previous detailed log\n", s/migraion/migration/ (also below)

Or reword to "Migration not supported on type %s."?

> + kobject_name(kobj)); > + return -ENODEV; > + } > + > + return snprintf(buf, strlen(type->migration_version) + 2, > + "%s\n", type->migration_version); > +} > + > +static ssize_t migration_version_store(struct kobject *kobj, struct device *dev, > + const char *buf, size_t count) > +{ > + int ret = 0; > + struct intel_vgpu_type *type; > + void *gvt = kdev_to_i915(dev)->gvt; > + > + type = intel_gvt_find_vgpu_type(gvt, kobject_name(kobj)); > + if (!type || !type->migration_version) { > + gvt_err("Does not support migraion on type %s. Please search previous detailed log\n", > + kobject_name(kobj)); > + return -ENODEV; > + } > + > + ret = intel_gvt_check_vfio_migration_version(gvt, > + type->migration_version, buf); > + > + return (ret < 0 ? ret : count); > +} > + > 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(migration_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_migration_version.attr, > NULL, > }; (...) > +char * > +intel_gvt_get_vfio_migration_version(struct intel_gvt *gvt, > + const char *vgpu_type) > +{ > + int cnt = 0; > + struct drm_i915_private *dev_priv = gvt->dev_priv; > + char *version = NULL; > + > + /* currently only gen8 & gen9 are supported */ > + if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9)) { > + gvt_err("Local hardware does not support migration on %d\n", > + INTEL_INFO(dev_priv)->gen); > + return NULL; > + } > + > + if (GVT_VFIO_MIGRATION_SOFTWARE_VERSION == INV_SOFTWARE_VERSION) { > + gvt_err("Local GVT does not support migration\n"); > + return NULL; > + } > + > + version = kzalloc(MIGRATION_VERSION_TOTAL_LEN, GFP_KERNEL); > + > + if (unlikely(!version)) { > + gvt_err("memory allocation failed when get local migraiton version\n"); s/migraiton/migration/ Or "cannot allocate memory for local migration version"?

> + return NULL; > + } > + > + /* vendor id + device id + vgpu type + software version */ > + cnt = snprintf(version, MIGRATION_VERSION_TOTAL_LEN, PRINTF_FORMAT, > + PCI_VENDOR_ID_INTEL, > + INTEL_DEVID(dev_priv), > + vgpu_type, > + GVT_VFIO_MIGRATION_SOFTWARE_VERSION); > + > + if (cnt) > + return version; > + > + gvt_err("string generation failed when get local migration version\n"); > + return NULL; > +} (...) Only some nitpicks from me, but I'm not really familiar with this driver. Overall, this looks sane to me, so have an Acked-by: Cornelia Huck <cohuck(a)redhat.com&gt;

intel-gvt-dev mailing list intel-gvt-dev(a)lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev