Date:   Sun, 19 Apr 2020 21:24:57 -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 07:24:57PM +0800, Cornelia Huck wrote:
> On Fri, 17 Apr 2020 05:52:02 -0400
> Yan Zhao <yan.y.zhao@...el.com> wrote:
> 
> > 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.
> 
> Ok, creating an alternate attribute for non-mdev devices makes sense.
> However, wouldn't that rather be a case (3)? The change here only
> refers to mdev devices.
>
as you pointed below, (3) and (2) serve the same purpose. 
and I think a possible usage is to migrate between a non-mdev device and
an mdev device. so I think it's better for them both to use (2) rather
than creating (3).
> > 
> > > Is existence (and compatibility) of (1) a pre-req for possible
> > > existence (and compatibility) of (2)?
> > >  
> > no. (2) does not reply on (1).
> 
> Hm. Non-existence of (1) seems to imply "this type does not support
> migration". If an mdev created for such a type suddenly does support
> migration, it feels a bit odd.
> 
yes. but I think if the condition happens, it should be reported a bug
to vendor driver.
should I add a line in the doc like "vendor driver should ensure that the
migration compatibility from migration_version under mdev_type should be
consistent with that from migration_version under device node" ?

> (It obviously cannot be a prereq for what I called (3) above.)
> 
> > 
> > > 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. 
> 
> How useful would it be for userspace to even look at (1) in that case?
> It only knows if things have a chance of working if it actually goes
> ahead and creates devices.
>
hmm, is it useful for userspace to test the migration_version under mdev
type before it knows what mdev device to generate ?
like when the userspace wants to migrate an mdev device in src vm,
but it has not created target vm and the target mdev device.

> > 
> > > 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?
> 
> What do we actually need here, I wonder? (1) and (2) seem to serve
> slightly different purposes, while (2) and what I called (3) have the
> same purpose. Is it important to userspace that (1) and (2) have the
> same name?
so change (1) to migration_type_version and (2) to
migration_instance_version?
But as they are under different locations, could that location imply
enough information?


Thanks
Yan

Powered by blists - more mailing lists