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

[Yan Zhao]

On Tue, May 28, 2019 at 05:01:35PM +0800, Cornelia Huck wrote:
> On Sun, 26 May 2019 23:44:37 -0400
> Yan Zhao <yan.y.zhao@xxxxxxxxx> 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@xxxxxxxxxx>
> > Cc: Erik Skultety <eskultet@xxxxxxxxxx>
> > Cc: "Dr. David Alan Gilbert" <dgilbert@xxxxxxxxxx>
> > Cc: Cornelia Huck <cohuck@xxxxxxxxxx>
> > Cc: "Tian, Kevin" <kevin.tian@xxxxxxxxx>
> > Cc: Zhenyu Wang <zhenyuw@xxxxxxxxxxxxxxx>
> > Cc: "Wang, Zhi A" <zhi.a.wang@xxxxxxxxx>
> > c: Neo Jia <cjia@xxxxxxxxxx>
> > Cc: Kirti Wankhede <kwankhede@xxxxxxxxxx>
> > 
> > Signed-off-by: Yan Zhao <yan.y.zhao@xxxxxxxxx>
> > 
> > ---
> > 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.)
>
yes. Let me correct it in next revision.

> > +{
> > +	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)
>
Sorry for typos again. I'll be more careful next time. thank you:)

> Or reword to "Migration not supported on type %s."?
>
Yes, better :)

> > +				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"?
>
Ok.

> > +		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@xxxxxxxxxx>


Thank you so much Cornelia :)



______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev