Re: [PATCH 1/2] vfio/mdev: add version field as mandatory attribute for mdev device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 4/30/19 11:29 AM, Cornelia Huck wrote:
On Wed, 24 Apr 2019 04:15:58 -0400
Yan Zhao <yan.y.zhao@xxxxxxxxx> wrote:

On Wed, Apr 24, 2019 at 03:56:24PM +0800, Cornelia Huck wrote:
On Tue, 23 Apr 2019 23:10:37 -0400
Yan Zhao <yan.y.zhao@xxxxxxxxx> wrote:
On Tue, Apr 23, 2019 at 05:59:32PM +0800, Cornelia Huck wrote:
On Fri, 19 Apr 2019 04:35:04 -0400
Yan Zhao <yan.y.zhao@xxxxxxxxx> wrote:

@@ -225,6 +228,8 @@ Directories and files under the sysfs for Each Physical Device
    [<type-id>], device_api, and available_instances are mandatory attributes
    that should be provided by vendor driver.

+  version is a mandatory attribute if a mdev device supports live migration.

What about "An mdev device wishing to support live migration must
provide the version attribute."?
yes, I just want to keep consistent with the line above it
" [<type-id>], device_api, and available_instances are mandatory attributes
   that should be provided by vendor driver."
what about below one?
   "version is a mandatory attribute if a mdev device wishing to support live
   migration."

My point is that an attribute is not mandatory if it can be left out :)
(I'm not a native speaker, though; maybe this makes perfect sense
after all?)

Maybe "version is a required attribute if live migration is supported
for an mdev device"?

My two cents: This is the best of the suggestions

Tony Krowiak

you are right, "mandatory" may bring some confusion.
Maybe
"vendor driver must provide version attribute for an mdev device wishing to
support live migration." ?
based on your first version :)

"The vendor driver must provide the version attribute for any mdev
device it wishes to support live migration for." ?



+
  * [<type-id>]

    The [<type-id>] name is created by adding the device driver string as a prefix
@@ -246,6 +251,35 @@ 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. It is used to check whether two devices are compatible
+  for live migration. If this attribute is missing, then the corresponding mdev
+  device is regarded as not supporting live migration.
+
+  It consists of two parts: common part and vendor proprietary part.
+  common part: 32 bit. lower 16 bits is vendor id and higher 16 bits identifies
+               device type. e.g., for pci device, it is
+               "pci vendor id" | (VFIO_DEVICE_FLAGS_PCI << 16).
+  vendor proprietary part: this part is varied in length. vendor driver can
+               specify any string to identify a device.
+
+  When reading this attribute, it should show device version string of the device
+  of type <type-id>. If a device does not support live migration, it should
+  return errno.
+  When writing a string to this attribute, it returns errno for incompatibility
+  or returns written string length in compatibility case. If a device does not
+  support live migration, it always returns errno.

I'm not sure whether a device that does not support live migration
should expose this attribute in the first place. Or is that to cover
cases where a driver supports live migration only for some of the
devices it supports?
yes, driver returning error code is to cover the cases where only part of devices it
supports can be migrated.

Also, I'm not sure if a string that has to be parsed is a good idea...
is this 'version' attribute supposed to convey some human-readable
information as well? The procedure you describe for compatibility
checking does the checking within the vendor driver which I would
expect to have a table/rules for that anyway.
right. if a vendor driver has the confidence to migrate between devices of
diffent platform or mdev types, it can maintain a compatibility table for that
purpose. That's the reason why we would leave the compatibility check to vendor
driver. vendor driver can freely choose its own complicated way to decide
which device is migratable to which device.

I think there are two scenarios here:
- Migrating between different device types, which is unlikely to work,
   except in special cases.
- Migrating between different versions of the same device type, which
   may work for some drivers/devices (and at least migrating to a newer
   version looks quite reasonable).

But both should be something that is decided by the individual driver;
I hope we don't want to support migration between different drivers :-O

Can we make this a driver-defined format?
yes, this is indeed driver-defined format.
Actually we define it into two parts: common part and vendor proprietary part.
common part: 32 bit. lower 16 bits is vendor id and higher 16 bits
              identifies device type. e.g., for pci device, it is
              "pci vendor id" | (VFIO_DEVICE_FLAGS_PCI << 16).
vendor proprietary part: this part is varied in length. vendor driver can
              specify any string to identify a device.

vendor proprietary part is defined by vendor driver. vendor driver can
define any format it wishes to use. Also it is its own responsibility to
ensure backward compatibility if it wants to update format definition in this
part.

So user space only needs to get source side's version string, and asks
target side whether the two are compatible. The decision maker is the
vendor driver:)

If I followed the discussion correctly, I think you plan to drop this
format, don't you? I'd be happy if a vendor driver can use a simple
number without any prefixes if it so chooses.

I also like the idea of renaming this "migration_version" so that it is
clear we're dealing with versioning of the migration capability (and
not a version of the device or so).

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux