Re: [libvirt] [PATCH v2 1/2] vfio/mdev: add version attribute for mdev device

On Sun, 5 May 2019 21:49:04 -0400 Yan Zhao <yan.y.zhao(a)intel.com&gt; wrote: I thought there was a request to make this more specific to migration by renaming it to something like migration_version. Also, as an optional attribute, it seems the example should perhaps not add it to all types to illustrate that it is not required. between two mdev devices of the same type. "in pairs"? perhaps "...instead of reading the version string of two mdev devices and comparing them in userspace..." Can the user assume the data contents of the string is ascii characters? It's good that the vendor driver defines the format and length, but the user probably needs some expectation bounding that length. Should we define it as no larger than PATH_MAX (4096), or maybe NAME_MAX (255) might be more reasonable? Compatible for what purpose? I think this is where specifically calling this a migration_version potentially has value. IOW, the write(2) will succeed if the version is determined to be compatible and otherwise fail with vendor specific errno. (0) The mdev devices are not of the same type. Is this intended to support that the vendor driver can supply a version attribute but not support migration? TBH, this sounds like a vendor driver bug, but maybe it's necessary if the vendor driver could have some types that support migration and others that do not? IOW, we're supplying the same attribute groups to all devices from a vendor, in which case my comment above regarding an example type without a version attribute might be invalid. (0) The mdev devices are of the same type I think it's best not to define the specific errno returned for a specific situation, let the vendor driver decide, userspace simply needs to know that an errno on read indicates the device does not support migration version comparison and that an errno on write indicates the devices are incompatible or the target doesn't support migration versions. Is this really the version information exposed in 2/2? This is opaque, so of course you can add things later, but it seems short sighted not to even append a version 0 tag to account for software compatibility differences since the above only represents a parent and mdev type based version. TBH, there's a lot of superfluous information in this example that can be stripped out. For example: " (1) Compare mdev types: The mdev type of an instantiated device can be read from the mdev_type link within the device instance in sysfs, for example: # basename $(readlink -f /sys/bus/mdev/devices/$MDEV_UUID/mdev_type/) The mdev types available on a given host system can also be found through /sys/class/mdev_bus, for example: # ls /sys/class/mdev_bus/*/mdev_supported_types/ Migration is only possible between devices of the same mdev type. (2) Retrieve the mdev source version: The migration version information can either be read from the mdev_type link on an instantiated device: # cat /sys/bus/mdev/devices/$UUID1/mdev_type/version Or it can be read from the mdev type definition, for example: # cat /sys/class/mdev_bus/*/mdev_supported_types/$MDEV_TYPE/version If reading the source version generates an error, migration is not possible. NB, there might be several parent devices for a given mdev type on a host system, each may support or expose different versions. Matching the specific mdev type to a parent may become important in such configurations. (3) Test source version at target: Given a version as outlined above, its compatibility to an instantiated device of the same mdev type can be tested as: # echo $VERSION > /sys/bus/mdev/devices/$UUID2/mdev_type/version If this write fails, the source and target versions are not compatible or the target does not support migration. Compatibility can also be tested prior to target device creation using the mdev type definition for a parent device with a previously found matching mdev type, for example: # echo $VERSION > /sys/class/mdev_bus/$PARENT/mdev_supported_types/$MDEV_TYPE/version Again, an error writing the version indicates that an instance of this mdev type would not support a migration from the provided version. " In particular from the provided example, the specific UUIDs, mdev types, parent information, and contents of the version attribute do not contribute to illustrating the protocol. In fact, displaying the contents of the version attribute may tempt users to do comparison on their own, especially given how easy it is to decide the GVT-g version string. This is far too long for description and examples, it's not this complicated. Thanks, Alex