[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/2] vfio/mdev: add version attribute for mde
From: |
Yan Zhao |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/2] vfio/mdev: add version attribute for mdev device |
Date: |
Thu, 9 May 2019 22:43:15 -0400 |
User-agent: |
Mutt/1.9.4 (2018-02-28) |
On Thu, May 09, 2019 at 11:24:49PM +0800, Cornelia Huck wrote:
> On Wed, 8 May 2019 07:57:05 -0400
> Yan Zhao <address@hidden> wrote:
>
> > On Tue, May 07, 2019 at 05:19:54PM +0800, Cornelia Huck wrote:
> > > On Sun, 5 May 2019 21:49:04 -0400
> > > Yan Zhao <address@hidden> wrote:
> > >
> > > > version attribute is used to check two mdev devices' compatibility.
> > > >
> > > > The key point of this version attribute is that it's rw.
> > > > User space has no need to understand internal of device version and no
> > > > need to compare versions by itself.
> > > > Compared to reading version strings from both two mdev devices being
> > > > checked, user space only reads from one mdev device's version attribute.
> > > > After getting its version string, user space writes this string into the
> > > > other mdev device's version attribute. Vendor driver of mdev device
> > > > whose version attribute being written will check device compatibility of
> > > > the two mdev devices for user space and return success for compatibility
> > > > or errno for incompatibility.
> > >
> > > I'm still missing a bit _what_ is actually supposed to be
> > > compatible/incompatible. I'd assume some internal state descriptions
> > > (even if this is not actually limited to migration).
> > >
> > right.
> > originally, I thought this attribute should only contain a device's hardware
> > compatibility info. But seems also including vendor specific software
> > migration
> > version is more reasonable, because general VFIO migration code cannot know
> > version of vendor specific software migration code until migration data is
> > transferring to the target vm. Then renaming it to migration_version is more
> > appropriate.
> > :)
>
> Nod.
>
> (...)
>
> > > > @@ -246,6 +249,143 @@ Directories and files under the sysfs for Each
> > > > Physical Device
> > > > This attribute should show the number of devices of type <type-id>
> > > > that can be
> > > > created.
> > > >
> > > > +* version
> > > > +
> > > > + This attribute is rw, and is optional.
> > > > + It is used to check device compatibility between two mdev devices
> > > > and is
> > > > + accessed in pairs between the two mdev devices being checked.
> > > > + The intent of this attribute is to make an mdev device's version
> > > > opaque to
> > > > + user space, so instead of reading two mdev devices' version strings
> > > > and
> > > > + comparing in userspace, user space should only read one mdev
> > > > device's version
> > > > + attribute, and writes this version string into the other mdev
> > > > device's version
> > > > + attribute. Then vendor driver of mdev device whose version attribute
> > > > being
> > > > + written would check the incoming version string and tell user space
> > > > whether
> > > > + the two mdev devices are compatible via return value. That's why this
> > > > + attribute is writable.
> > >
> > > I would reword this a bit:
> > >
> > > "This attribute provides a way to check device compatibility between
> > > two mdev devices from userspace. The intended usage is for userspace to
> > > read the version attribute from one mdev device and then writing that
> > > value to the version attribute of the other mdev device. The second
> > > mdev device indicates compatibility via the return code of the write
> > > operation. This makes compatibility between mdev devices completely
> > > vendor-defined and opaque to userspace."
> > >
> > > We still should explain _what_ compatibility we're talking about here,
> > > though.
> > >
> > Thanks. It's much better than mine:)
> > Then I'll change compatibility --> migration compatibility.
>
> Ok, with that it should be clear enough.
>
> >
> > > > +
> > > > + when reading this attribute, it should show device version string of
> > > > + the device of type <type-id>.
> > > > +
> > > > + This string is private to vendor driver itself. Vendor driver is
> > > > able to
> > > > + freely define format and length of device version string.
> > > > + e.g. It can use a combination of pciid of parent device + mdev type.
> > > > +
> > > > + When writing a string to this attribute, vendor driver should
> > > > analyze this
> > > > + string and check whether the mdev device being identified by this
> > > > string is
> > > > + compatible with the mdev device for this attribute. vendor driver
> > > > should then
> > > > + return written string's length if it regards the two mdev devices are
> > > > + compatible; vendor driver should return negative errno if it regards
> > > > the two
> > > > + mdev devices are not compatible.
> > > > +
> > > > + User space should treat ANY of below conditions as two mdev devices
> > > > not
> > > > + compatible:
> > > > + (1) any one of the two mdev devices does not have a version attribute
> > > > + (2) error when read from one mdev device's version attribute
> > >
> > > s/read/reading/
> > >
> > > > + (3) error when write one mdev device's version string to the other
> > > > mdev
> > >
> > > s/write/writing/
> > >
> > > > + device's version attribute
> > > > +
> > > > + User space should regard two mdev devices compatible when ALL of
> > > > below
> > > > + conditions are met:
> > > > + (1) success when read from one mdev device's version attribute.
> > >
> > > s/read/reading/
> > >
> > > > + (2) success when write one mdev device's version string to the other
> > > > mdev
> > >
> > > s/write/writing/
> > got it. thanks for pointing them out:)
> > >
> > > > + device's version attribute
> > > > +
> > > > + Errno:
> > > > + If vendor driver wants to claim a mdev device incompatible to all
> > > > other mdev
> > >
> > > "If the vendor driver wants to designate a mdev device..."
> > >
> > ok. thanks:)
> > > > + devices, it should not register version attribute for this mdev
> > > > device. But if
> > > > + a vendor driver has already registered version attribute and it
> > > > wants to claim
> > > > + a mdev device incompatible to all other mdev devices, it needs to
> > > > return
> > > > + -ENODEV on access to this mdev device's version attribute.
> > > > + If a mdev device is only incompatible to certain mdev devices, write
> > > > of
> > > > + incompatible mdev devices's version strings to its version attribute
> > > > should
> > > > + return -EINVAL;
> > >
> > >
> > > Maybe put the defined return code into a bulleted list instead? But
> > > this looks reasonable as well.
> > >
> > as user space have no idea of those errno and only gets 0/1 as return code
> > from
> > read/write. maybe I can move this description of errno to patch 2/2 as an
> > example?
>
> Confused. They should get -EINVAL/-ENODEV/... all right, shouldn't they?
>
sorry. my previous statement is not right.
read(2)/write(2) return -1 on error, error cause is returned through errno.
So, it's also fine if we can get an agreement in this doc that
-ENODEV meaning a mdev device is not compatible to all devices,
-EINVAL meaning a mdev device is not compatible to specified device.
> >
> > > > +
> > > > + This attribute can be taken advantage of by live migration.
> > > > + If user space detects two mdev devices are compatible through version
> > > > + attribute, it can start migration between the two mdev devices,
> > > > otherwise it
> > > > + should abort its migration attempts between the two mdev devices.
> > >
> > > (...)
> > > _______________________________________________
> > > intel-gvt-dev mailing list
> > > address@hidden
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
>
> _______________________________________________
> intel-gvt-dev mailing list
> address@hidden
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
- [Qemu-devel] [PATCH v2 0/2] introduction of version attribute for VFIO live migration, Yan Zhao, 2019/05/05
- Re: [Qemu-devel] [PATCH v2 1/2] vfio/mdev: add version attribute for mdev device, Alex Williamson, 2019/05/07
- Re: [Qemu-devel] [PATCH v2 1/2] vfio/mdev: add version attribute for mdev device, Yan Zhao, 2019/05/08
- Re: [Qemu-devel] [PATCH v2 1/2] vfio/mdev: add version attribute for mdev device, Alex Williamson, 2019/05/08
- Re: [Qemu-devel] [PATCH v2 1/2] vfio/mdev: add version attribute for mdev device, Yan Zhao, 2019/05/08
- Re: [Qemu-devel] [PATCH v2 1/2] vfio/mdev: add version attribute for mdev device, Alex Williamson, 2019/05/09
- Re: [Qemu-devel] [PATCH v2 1/2] vfio/mdev: add version attribute for mdev device, Yan Zhao, 2019/05/09
- Re: [Qemu-devel] [libvirt] [PATCH v2 1/2] vfio/mdev: add version attribute for mdev device, Boris Fiuczynski, 2019/05/09
- Re: [Qemu-devel] [libvirt] [PATCH v2 1/2] vfio/mdev: add version attribute for mdev device, Yan Zhao, 2019/05/09
- Re: [Qemu-devel] [libvirt] [PATCH v2 1/2] vfio/mdev: add version attribute for mdev device, Alex Williamson, 2019/05/14
- Re: [Qemu-devel] [libvirt] [PATCH v2 1/2] vfio/mdev: add version attribute for mdev device, Boris Fiuczynski, 2019/05/29