Message-ID: <20200417095202.GD16688@joy-OptiPlex-7040>
Date:   Fri, 17 Apr 2020 05:52:02 -0400
From:   Yan Zhao <yan.y.zhao@...el.com>
To:     Cornelia Huck <cohuck@...hat.com>
Cc:     "intel-gvt-dev@...ts.freedesktop.org" 
        <intel-gvt-dev@...ts.freedesktop.org>,
        "libvir-list@...hat.com" <libvir-list@...hat.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "aik@...abs.ru" <aik@...abs.ru>,
        "Zhengxiao.zx@...baba-inc.com" <Zhengxiao.zx@...baba-inc.com>,
        "shuangtai.tst@...baba-inc.com" <shuangtai.tst@...baba-inc.com>,
        "qemu-devel@...gnu.org" <qemu-devel@...gnu.org>,
        "eauger@...hat.com" <eauger@...hat.com>,
        "Liu, Yi L" <yi.l.liu@...el.com>, "Zeng, Xin" <xin.zeng@...el.com>,
        "Yang, Ziye" <ziye.yang@...el.com>,
        "mlevitsk@...hat.com" <mlevitsk@...hat.com>,
        "pasic@...ux.ibm.com" <pasic@...ux.ibm.com>,
        "felipe@...anix.com" <felipe@...anix.com>,
        "Liu, Changpeng" <changpeng.liu@...el.com>,
        "Ken.Xue@....com" <Ken.Xue@....com>,
        "jonathan.davies@...anix.com" <jonathan.davies@...anix.com>,
        "He, Shaopeng" <shaopeng.he@...el.com>,
        "alex.williamson@...hat.com" <alex.williamson@...hat.com>,
        "eskultet@...hat.com" <eskultet@...hat.com>,
        "dgilbert@...hat.com" <dgilbert@...hat.com>,
        "Tian, Kevin" <kevin.tian@...el.com>,
        "zhenyuw@...ux.intel.com" <zhenyuw@...ux.intel.com>,
        "Wang, Zhi A" <zhi.a.wang@...el.com>,
        "cjia@...dia.com" <cjia@...dia.com>,
        "kwankhede@...dia.com" <kwankhede@...dia.com>,
        "berrange@...hat.com" <berrange@...hat.com>,
        "dinechin@...hat.com" <dinechin@...hat.com>,
        "corbet@....net" <corbet@....net>
Subject: Re: [PATCH v5 0/4] introduction of migration_version attribute for
 VFIO live migration

On Fri, Apr 17, 2020 at 04:44:50PM +0800, Cornelia Huck wrote:
> On Mon, 13 Apr 2020 01:52:01 -0400
> Yan Zhao <yan.y.zhao@...el.com> wrote:
> 
> > This patchset introduces a migration_version attribute under sysfs of VFIO
> > Mediated devices.
> > 
> > This migration_version attribute is used to check migration compatibility
> > between two mdev devices.
> > 
> > Currently, it has two locations:
> > (1) under mdev_type node,
> >     which can be used even before device creation, but only for mdev
> >     devices of the same mdev type.
> > (2) under mdev device node,
> >     which can only be used after the mdev devices are created, but the src
> >     and target mdev devices are not necessarily be of the same mdev type
> > (The second location is newly added in v5, in order to keep consistent
> > with the migration_version node for migratable pass-though devices)
> 
> What is the relationship between those two attributes?
> 
(1) is for mdev devices specifically, and (2) is provided to keep the same
sysfs interface as with non-mdev cases. so (2) is for both mdev devices and
non-mdev devices.

in future, if we enable vfio-pci vendor ops, (i.e. a non-mdev device
is binding to vfio-pci, but is able to register migration region and do
migration transactions from a vendor provided affiliate driver),
the vendor driver would export (2) directly, under device node.
It is not able to provide (1) as there're no mdev devices involved.

> Is existence (and compatibility) of (1) a pre-req for possible
> existence (and compatibility) of (2)?
>
no. (2) does not reply on (1).

> Does userspace need to check (1) or can it completely rely on (2), if
> it so chooses?
>
I think it can completely reply on (2) if compatibility check before
mdev creation is not required.

> If devices with a different mdev type are indeed compatible, it seems
> userspace can only find out after the devices have actually been
> created, as (1) does not apply?
yes, I think so. 

> One of my worries is that the existence of an attribute with the same
> name in two similar locations might lead to confusion. But maybe it
> isn't a problem.
>
Yes, I have the same feeling. but as (2) is for sysfs interface
consistency, to make it transparent to userspace tools like libvirt,
I guess the same name is necessary?

Thanks
Yan
> > 
> > Patch 1 defines migration_version attribute for the first location in
> > Documentation/vfio-mediated-device.txt
> > 
> > Patch 2 uses GVT as an example for patch 1 to show how to expose
> > migration_version attribute and check migration compatibility in vendor
> > driver.
> > 
> > Patch 3 defines migration_version attribute for the second location in
> > Documentation/vfio-mediated-device.txt
> > 
> > Patch 4 uses GVT as an example for patch 3 to show how to expose
> > migration_version attribute and check migration compatibility in vendor
> > driver.
> > 
> > (The previous "Reviewed-by" and "Acked-by" for patch 1 and patch 2 are
> > kept in v5, as there are only small changes to commit messages of the two
> > patches.)
> > 
> > v5:
> > added patch 2 and 4 for mdev device part of migration_version attribute.
> > 
> > v4:
> > 1. fixed indentation/spell errors, reworded several error messages
> > 2. added a missing memory free for error handling in patch 2
> > 
> > v3:
> > 1. renamed version to migration_version
> > 2. let errno to be freely defined by vendor driver
> > 3. let checking mdev_type be prerequisite of migration compatibility check
> > 4. reworded most part of patch 1
> > 5. print detailed error log in patch 2 and generate migration_version
> > string at init time
> > 
> > v2:
> > 1. renamed patched 1
> > 2. made definition of device version string completely private to vendor
> > driver
> > 3. reverted changes to sample mdev drivers
> > 4. described intent and usage of version attribute more clearly.
> > 
> > 
> > Yan Zhao (4):
> >   vfio/mdev: add migration_version attribute for mdev (under mdev_type
> >     node)
> >   drm/i915/gvt: export migration_version to mdev sysfs (under mdev_type
> >     node)
> >   vfio/mdev: add migration_version attribute for mdev (under mdev device
> >     node)
> >   drm/i915/gvt: export migration_version to mdev sysfs (under mdev
> >     device node)
> > 
> >  .../driver-api/vfio-mediated-device.rst       | 183 ++++++++++++++++++
> >  drivers/gpu/drm/i915/gvt/Makefile             |   2 +-
> >  drivers/gpu/drm/i915/gvt/gvt.c                |  39 ++++
> >  drivers/gpu/drm/i915/gvt/gvt.h                |   7 +
> >  drivers/gpu/drm/i915/gvt/kvmgt.c              |  55 ++++++
> >  drivers/gpu/drm/i915/gvt/migration_version.c  | 170 ++++++++++++++++
> >  drivers/gpu/drm/i915/gvt/vgpu.c               |  13 +-
> >  7 files changed, 466 insertions(+), 3 deletions(-)
> >  create mode 100644 drivers/gpu/drm/i915/gvt/migration_version.c
> > 
> 

Powered by blists - more mailing lists