On Sun, 5 May 2019 21:49:04 -0400 Yan Zhao <yan.y.zhao@intel.com> 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).

So two readings of version attributes + checking in user space are now changed to one reading + one writing of version attributes + checking in vendor driver.

I'm not sure that needs to go into the patch description (sounds like it is rather a change log?)

Format and length of version strings are now private to vendor driver who can define them freely.

Same here; simply drop the 'now'?

__ user space /\ \ / \write / read \ ______/__ ___\|/___ | version | | version |-->check compatibility ----------- ----------- mdev device A mdev device B

This version attribute is optional. If a mdev device does not provide with a version attribute, this mdev device is incompatible to all other mdev devices.

Again, I'd like an explanation here what kind of compatibility we're talking about.

Live migration is able to take advantage of this version attribute. Before user space actually starts live migration, it can first check whether two mdev devices are compatible.

v2: 1. added detailed intent and usage 2. made definition of version string completely private to vendor driver (Alex Williamson) 3. abandoned changes to sample mdev drivers (Alex Williamson) 4. mandatory --> optional (Cornelia Huck) 5. added description for errno (Cornelia Huck)

This changelog should go below the ---, so that it does not actually show up in the patch description later :)

Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Erik Skultety <eskultet@redhat.com> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> Cc: Cornelia Huck <cohuck@redhat.com> Cc: "Tian, Kevin" <kevin.tian@intel.com> Cc: Zhenyu Wang <zhenyuw@linux.intel.com> Cc: "Wang, Zhi A" <zhi.a.wang@intel.com> Cc: Neo Jia <cjia@nvidia.com> Cc: Kirti Wankhede <kwankhede@nvidia.com> Cc: Daniel P. Berrangé <berrange@redhat.com> Cc: Christophe de Dinechin <dinechin@redhat.com>

Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> --- Documentation/vfio-mediated-device.txt | 140 +++++++++++++++++++++++++ 1 file changed, 140 insertions(+)

diff --git a/Documentation/vfio-mediated-device.txt b/Documentation/vfio-mediated-device.txt index c3f69bcaf96e..013a764968eb 100644 --- a/Documentation/vfio-mediated-device.txt +++ b/Documentation/vfio-mediated-device.txt @@ -202,6 +202,7 @@ Directories and files under the sysfs for Each Physical Device | | |--- available_instances | | |--- device_api | | |--- description + | | |--- version | | |--- [devices] | |--- [<type-id>] | | |--- create @@ -209,6 +210,7 @@ Directories and files under the sysfs for Each Physical Device | | |--- available_instances | | |--- device_api | | |--- description + | | |--- version | | |--- [devices] | |--- [<type-id>] | |--- create @@ -216,6 +218,7 @@ Directories and files under the sysfs for Each Physical Device | |--- available_instances | |--- device_api | |--- description + | |--- version | |--- [devices]

* [mdev_supported_types] @@ -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.

+ + 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/

+ 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..."

+ 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.

+ + 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.

(...)

On Wed, 8 May 2019 07:57:05 -0400 Yan Zhao <yan.y.zhao@intel.com> 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 <yan.y.zhao@intel.com> 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?

...

...

+ + 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 intel-gvt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

On Sun, 5 May 2019 21:49:04 -0400 Yan Zhao <yan.y.zhao@intel.com> 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. So two readings of version attributes + checking in user space are now changed to one reading + one writing of version attributes + checking in vendor driver. Format and length of version strings are now private to vendor driver who can define them freely.

__ user space /\ \ / \write / read \ ______/__ ___\|/___ | version | | version |-->check compatibility ----------- ----------- mdev device A mdev device B

This version attribute is optional. If a mdev device does not provide with a version attribute, this mdev device is incompatible to all other mdev devices.

Live migration is able to take advantage of this version attribute. Before user space actually starts live migration, it can first check whether two mdev devices are compatible.

v2: 1. added detailed intent and usage 2. made definition of version string completely private to vendor driver (Alex Williamson) 3. abandoned changes to sample mdev drivers (Alex Williamson) 4. mandatory --> optional (Cornelia Huck) 5. added description for errno (Cornelia Huck)

Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Erik Skultety <eskultet@redhat.com> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> Cc: Cornelia Huck <cohuck@redhat.com> Cc: "Tian, Kevin" <kevin.tian@intel.com> Cc: Zhenyu Wang <zhenyuw@linux.intel.com> Cc: "Wang, Zhi A" <zhi.a.wang@intel.com> Cc: Neo Jia <cjia@nvidia.com> Cc: Kirti Wankhede <kwankhede@nvidia.com> Cc: Daniel P. Berrangé <berrange@redhat.com> Cc: Christophe de Dinechin <dinechin@redhat.com>

Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> --- Documentation/vfio-mediated-device.txt | 140 +++++++++++++++++++++++++ 1 file changed, 140 insertions(+)

diff --git a/Documentation/vfio-mediated-device.txt b/Documentation/vfio-mediated-device.txt index c3f69bcaf96e..013a764968eb 100644 --- a/Documentation/vfio-mediated-device.txt +++ b/Documentation/vfio-mediated-device.txt @@ -202,6 +202,7 @@ Directories and files under the sysfs for Each Physical Device | | |--- available_instances | | |--- device_api | | |--- description + | | |--- version | | |--- [devices] | |--- [<type-id>] | | |--- create @@ -209,6 +210,7 @@ Directories and files under the sysfs for Each Physical Device | | |--- available_instances | | |--- device_api | | |--- description + | | |--- version | | |--- [devices] | |--- [<type-id>] | |--- create @@ -216,6 +218,7 @@ Directories and files under the sysfs for Each Physical Device | |--- available_instances | |--- device_api | |--- description + | |--- version | |--- [devices]

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.

* [mdev_supported_types] @@ -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

between two mdev devices of the same type.

+ accessed in pairs between the two mdev devices being checked.

"in pairs"?

+ 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

perhaps "...instead of reading the version string of two mdev devices and comparing them in userspace..."

+ 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. + + 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.

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?

+ + 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

Compatible for what purpose? I think this is where specifically calling this a migration_version potentially has value.

+ 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.

IOW, the write(2) will succeed if the version is determined to be compatible and otherwise fail with vendor specific errno.

+ + User space should treat ANY of below conditions as two mdev devices not + compatible:

(0) The mdev devices are not of the same type.

+ (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

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.

+ (3) error when write one mdev device's version string to the other mdev + device's version attribute + + User space should regard two mdev devices compatible when ALL of below + conditions are met:

(0) The mdev devices are of the same type

+ (1) success when read from one mdev device's version attribute. + (2) success when write one mdev device's version string to the other mdev + device's version attribute + + Errno: + If vendor driver wants to claim a mdev device incompatible to all other mdev + 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;

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.

+ + 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. + + Example Usage: + case 1: + source side mdev device is of uuid 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-193b. + target side mdev device is if of uuid 882cc4da-dede-11e7-9180-078a62063ab1, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-193b. + + # readlink /sys/bus/pci/devices/0000\:00\:02.0/\ + 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/mdev_type + ../mdev_supported_types/i915-GVTg_V5_4 + + # readlink /sys/bus/pci/devices/0000\:00\:02.0/\ + 882cc4da-dede-11e7-9180-078a62063ab1/mdev_type + ../mdev_supported_types/i915-GVTg_V5_4 + + (1) read source side mdev device's version. + #cat \ + /sys/bus/pci/devices/0000\:00\:02.0/5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/\ + mdev_type/version + 8086-193b-i915-GVTg_V5_4

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.

+ (2) write source side mdev device's version string into target side mdev + device's version attribute. + # echo 8086-193b-i915-GVTg_V5_4 > + /sys/bus/pci/devices/0000\:00\:02.0/882cc4da-dede-11e7-9180-078a62063ab1/\ + mdev_type/version + # echo $? + 0

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.

+ + in this case, user space's write to target side mdev device's version + attribute returns success to indicate the two mdev devices are compatible. + + case 2: + source side mdev device is of uuid 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-193b. + target side mdev device is if of uuid 882cc4da-dede-11e7-9180-078a62063ab1, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-191b. + + # readlink /sys/bus/pci/devices/0000\:00\:02.0/\ + 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/mdev_type + ../mdev_supported_types/i915-GVTg_V5_4 + + # readlink /sys/bus/pci/devices/0000\:00\:02.0/\ + 882cc4da-dede-11e7-9180-078a62063ab1/mdev_type + ../mdev_supported_types/i915-GVTg_V5_4 + + (1) read source side mdev device's version. + #cat \ + /sys/bus/pci/devices/0000\:00\:02.0/5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/\ + mdev_type/version + 8086-193b-i915-GVTg_V5_4 + + (2) write source side mdev device's version string into target side mdev + device's version attribute. + # echo 8086-193b-i915-GVTg_V5_4 > + /sys/bus/pci/devices/0000\:00\:02.0/882cc4da-dede-11e7-9180-078a62063ab1/\ + mdev_type/version + -bash: echo: write error: Invalid argument + + in this case, user space's write to target side mdev device's version + attribute returns error to indicate the two mdev devices are incompatible. + (incompatible because pci ids of the two mdev devices' parent devices are + different). + + case 3: + source side mdev device is of uuid 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-193b. + But vendor driver does not provide version attribute for this device. + + (1) read source side mdev device's version. + #cat \ + /sys/bus/pci/devices/0000\:00\:02.0/5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/\ + mdev_type/version + cat: '/sys/bus/pci/devices/0000:00:02.0/5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/\ + mdev_type/version': No such file or directory + + in this case, user space reads source side mdev device's version attribute + which does not exist however. user space regards the two mdev devices as not + compatible and will not start migration between the two mdev devices. + +

This is far too long for description and examples, it's not this complicated. Thanks, Alex

* [device]

This directory contains links to the devices of type <type-id> that have been

On Wed, 8 May 2019 07:27:40 -0400 Yan Zhao <yan.y.zhao@intel.com> wrote:

On Wed, May 08, 2019 at 05:18:26AM +0800, Alex Williamson wrote:

...

On Sun, 5 May 2019 21:49:04 -0400 Yan Zhao <yan.y.zhao@intel.com> 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. So two readings of version attributes + checking in user space are now changed to one reading + one writing of version attributes + checking in vendor driver. Format and length of version strings are now private to vendor driver who can define them freely.

__ user space /\ \ / \write / read \ ______/__ ___\|/___ | version | | version |-->check compatibility ----------- ----------- mdev device A mdev device B

This version attribute is optional. If a mdev device does not provide with a version attribute, this mdev device is incompatible to all other mdev devices.

Live migration is able to take advantage of this version attribute. Before user space actually starts live migration, it can first check whether two mdev devices are compatible.

v2: 1. added detailed intent and usage 2. made definition of version string completely private to vendor driver (Alex Williamson) 3. abandoned changes to sample mdev drivers (Alex Williamson) 4. mandatory --> optional (Cornelia Huck) 5. added description for errno (Cornelia Huck)

Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Erik Skultety <eskultet@redhat.com> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> Cc: Cornelia Huck <cohuck@redhat.com> Cc: "Tian, Kevin" <kevin.tian@intel.com> Cc: Zhenyu Wang <zhenyuw@linux.intel.com> Cc: "Wang, Zhi A" <zhi.a.wang@intel.com> Cc: Neo Jia <cjia@nvidia.com> Cc: Kirti Wankhede <kwankhede@nvidia.com> Cc: Daniel P. Berrangé <berrange@redhat.com> Cc: Christophe de Dinechin <dinechin@redhat.com>

Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> --- Documentation/vfio-mediated-device.txt | 140 +++++++++++++++++++++++++ 1 file changed, 140 insertions(+)

diff --git a/Documentation/vfio-mediated-device.txt b/Documentation/vfio-mediated-device.txt index c3f69bcaf96e..013a764968eb 100644 --- a/Documentation/vfio-mediated-device.txt +++ b/Documentation/vfio-mediated-device.txt @@ -202,6 +202,7 @@ Directories and files under the sysfs for Each Physical Device | | |--- available_instances | | |--- device_api | | |--- description + | | |--- version | | |--- [devices] | |--- [<type-id>] | | |--- create @@ -209,6 +210,7 @@ Directories and files under the sysfs for Each Physical Device | | |--- available_instances | | |--- device_api | | |--- description + | | |--- version | | |--- [devices] | |--- [<type-id>] | |--- create @@ -216,6 +218,7 @@ Directories and files under the sysfs for Each Physical Device | |--- available_instances | |--- device_api | |--- description + | |--- version | |--- [devices]

I thought there was a request to make this more specific to migration by renaming it to something like migration_version. Also, as an

so this attribute may not only include a mdev device's parent device info and mdev type, but also include numeric software version of vendor specific migration code, right?

It's a vendor defined string, it should be considered opaque to the user, the vendor can include whatever they feel is relevant.

This actually makes sense. So, do I need to add a disclaimer in this doc like: vendor driver should be responsible by itself for a mdev device's migration compatibility.

I thought that was the purpose of this attribute.

During migration setup phase, general migration code in user space VFIO only checks this version of VFIO migration region, and will not check software version of vendor specific migration code.

What is "software version of vendor specific migration code"? If you're asking whether anything will check for parent device compatibility or parent vendor driver compatibility, the answer is no, that's what this interface is meant to provide. Userspace should need to do nothing more than verify the mdev types match and then use the version attribute to confirm source to target compatibility.

It is suggested to incorporate at least parent device info and software version of vendor specific migration code into this migration_version attribute.

We can make recommendations as "best practices", but ultimately it's an opaque string defined by the vendor driver. But you never addressed my comment that previous reviews asked for the attribute to be named something more specific to migration.

...

optional attribute, it seems the example should perhaps not add it to all types to illustrate that it is not required. ok. got it.

...

...

* [mdev_supported_types] @@ -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

between two mdev devices of the same type.

ok. got it. But I have a question about aggregation proposed earlier. Do we also have to assume the two mdev devices are of the same aggregation count? However, aggregation count is not available before a mdev device is created. :(

We don't support aggregation yet, but yes, that's going to introduce issues here. Any configuration beyond the base mdev type would imply that the base type could be compatible for migration, but the specific instances might not. Resolving that would imply that our version information needs to be relative to an instance, not just the base type. How would we extend this interface to support that? We could have a version attribute on each device instance, which might report something like: 0123456789,aggregate=2 IOW the device instance of version concatenates the mdev type version with the additional create parameters for that device. Writing this to the type attribute should be parsed by the vendor driver as support for given base device with specified additional create parameters. I'm afraid this also bring us around to treacherous questions around who is responsible for creating that device on the migration target and where is this meta information about the device exposed. Maybe instead of a per instance version attribute we would instead expose the create parameters as an attribute per instance and it would be userspace's responsibility to create a version string from the mdev type and create parameters similar to above. This would also make it possible to create a compatible instance on the target without pre-knowledge of how the device was created. Also, this issue exists already, but compatibility and capacity are two separate things, I think we want to limit this interface to the former. For instance, if I want to migrate an i915-GVTg_V5_1 device to another system where available_instances for that type is zero, the version attribute should strictly report the device compatibility, it's not responsible for returning an errno due to lack of resources. Similarly if we were to do something with aggregation, the version attribute would strictly report if the target supports creating that device with those parameters, not whether it has capacity to create such as device at that instant in time.

...

...

+ accessed in pairs between the two mdev devices being checked.

"in pairs"? I meant, user space needs to access version attributes from two mdev device. but seems that it's needless to mention that... I'll remove it :)

...

...

+ 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

perhaps "...instead of reading the version string of two mdev devices and comparing them in userspace..." yes, better, thanks:)

...

...

+ 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. + + 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.

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? I think so. I'll add those restrictions in next revision.

If we start adding creation parameters, PATH_MAX may actually be the more reasonable limit.

...

...

+ + 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

Compatible for what purpose? I think this is where specifically calling this a migration_version potentially has value. yes. if it also covers version of vendor specific migration code, calling it migration_version is more appropriate.

I think we're discussing an interface that validates "I [the vendor driver] am able to import the state of this version", therefore it must include every relevant aspect of the vendor support for that.

...

...

+ 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.

IOW, the write(2) will succeed if the version is determined to be compatible and otherwise fail with vendor specific errno.

thanks:)

...

...

+ + User space should treat ANY of below conditions as two mdev devices not + compatible:

(0) The mdev devices are not of the same type.

the same as above. do we also need to take aggregation count into consideration?

...

...

+ (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

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. hmm, this is to make life easier for vendor driver to have some types that support migration and others that do not. while we can get rid of returning errno by providing different attribute groups to different devices, the way of returning errno gives a simpler choice to vendors.

Yes, I think it might be overly complicated to provide different attribute groups for different devices, we have more flexibility if the user does not make any assumptions based only on the presence of a version attribute.

...

...

+ (3) error when write one mdev device's version string to the other mdev + device's version attribute + + User space should regard two mdev devices compatible when ALL of below + conditions are met:

(0) The mdev devices are of the same type

...

+ (1) success when read from one mdev device's version attribute. + (2) success when write one mdev device's version string to the other mdev + device's version attribute + + Errno: + If vendor driver wants to claim a mdev device incompatible to all other mdev + 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;

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.

yes, user space only gets 0 or 1 as return code, not those errno. maybe I only need to describe errno in patch 2/2.

...

...

+ + 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. + + Example Usage: + case 1: + source side mdev device is of uuid 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-193b. + target side mdev device is if of uuid 882cc4da-dede-11e7-9180-078a62063ab1, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-193b. + + # readlink /sys/bus/pci/devices/0000\:00\:02.0/\ + 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/mdev_type + ../mdev_supported_types/i915-GVTg_V5_4 + + # readlink /sys/bus/pci/devices/0000\:00\:02.0/\ + 882cc4da-dede-11e7-9180-078a62063ab1/mdev_type + ../mdev_supported_types/i915-GVTg_V5_4 + + (1) read source side mdev device's version. + #cat \ + /sys/bus/pci/devices/0000\:00\:02.0/5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/\ + mdev_type/version + 8086-193b-i915-GVTg_V5_4

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.

yes, currently in 2/2, the version only includes <vendor id> + <device id> + <mdev type>. but you are right, it's better to include software migration version number. so vendor drivers have below 3 ways to designate a mdev device has no migration capability. 1. not registering migration_version attribute 2. on reading migration_version, returning errno 3. on reading migration_version, returning string indicating non-migratable.

The reason of not giving up way 2 is that maybe it can accelerate user space getting information of device incompatible. if we only keep way 3, it would not know this info until writing this string to target attribute.

do you agree?

The string is opaque to the user, so if you're asking in (3) that the user read and parse some information in the string that indicates the device is non-migratable then no, I don't agree with that policy. If reading the version attribute produces a result, the only thing the user can do with that result is to test it by writing it to another version attribute. Thanks, Alex

...

...

+ (2) write source side mdev device's version string into target side mdev + device's version attribute. + # echo 8086-193b-i915-GVTg_V5_4 > + /sys/bus/pci/devices/0000\:00\:02.0/882cc4da-dede-11e7-9180-078a62063ab1/\ + mdev_type/version + # echo $? + 0

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.

got it! great thanks! I'll update it to the next revision.

...

...

+ + in this case, user space's write to target side mdev device's version + attribute returns success to indicate the two mdev devices are compatible. + + case 2: + source side mdev device is of uuid 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-193b. + target side mdev device is if of uuid 882cc4da-dede-11e7-9180-078a62063ab1, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-191b. + + # readlink /sys/bus/pci/devices/0000\:00\:02.0/\ + 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/mdev_type + ../mdev_supported_types/i915-GVTg_V5_4 + + # readlink /sys/bus/pci/devices/0000\:00\:02.0/\ + 882cc4da-dede-11e7-9180-078a62063ab1/mdev_type + ../mdev_supported_types/i915-GVTg_V5_4 + + (1) read source side mdev device's version. + #cat \ + /sys/bus/pci/devices/0000\:00\:02.0/5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/\ + mdev_type/version + 8086-193b-i915-GVTg_V5_4 + + (2) write source side mdev device's version string into target side mdev + device's version attribute. + # echo 8086-193b-i915-GVTg_V5_4 > + /sys/bus/pci/devices/0000\:00\:02.0/882cc4da-dede-11e7-9180-078a62063ab1/\ + mdev_type/version + -bash: echo: write error: Invalid argument + + in this case, user space's write to target side mdev device's version + attribute returns error to indicate the two mdev devices are incompatible. + (incompatible because pci ids of the two mdev devices' parent devices are + different). + + case 3: + source side mdev device is of uuid 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-193b. + But vendor driver does not provide version attribute for this device. + + (1) read source side mdev device's version. + #cat \ + /sys/bus/pci/devices/0000\:00\:02.0/5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/\ + mdev_type/version + cat: '/sys/bus/pci/devices/0000:00:02.0/5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/\ + mdev_type/version': No such file or directory + + in this case, user space reads source side mdev device's version attribute + which does not exist however. user space regards the two mdev devices as not + compatible and will not start migration between the two mdev devices. + +

This is far too long for description and examples, it's not this complicated. Thanks,

got it. I'll follow your above example :)

thanks Yan

...

...

* [device]

This directory contains links to the devices of type <type-id> that have been

On Thu, May 09, 2019 at 05:22:42AM +0800, Alex Williamson wrote:

On Wed, 8 May 2019 07:27:40 -0400 Yan Zhao <yan.y.zhao@intel.com> wrote:

...

On Wed, May 08, 2019 at 05:18:26AM +0800, Alex Williamson wrote:

...

On Sun, 5 May 2019 21:49:04 -0400 Yan Zhao <yan.y.zhao@intel.com> 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. So two readings of version attributes + checking in user space are now changed to one reading + one writing of version attributes + checking in vendor driver. Format and length of version strings are now private to vendor driver who can define them freely.

__ user space /\ \ / \write / read \ ______/__ ___\|/___ | version | | version |-->check compatibility ----------- ----------- mdev device A mdev device B

This version attribute is optional. If a mdev device does not provide with a version attribute, this mdev device is incompatible to all other mdev devices.

Live migration is able to take advantage of this version attribute. Before user space actually starts live migration, it can first check whether two mdev devices are compatible.

v2: 1. added detailed intent and usage 2. made definition of version string completely private to vendor driver (Alex Williamson) 3. abandoned changes to sample mdev drivers (Alex Williamson) 4. mandatory --> optional (Cornelia Huck) 5. added description for errno (Cornelia Huck)

Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Erik Skultety <eskultet@redhat.com> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> Cc: Cornelia Huck <cohuck@redhat.com> Cc: "Tian, Kevin" <kevin.tian@intel.com> Cc: Zhenyu Wang <zhenyuw@linux.intel.com> Cc: "Wang, Zhi A" <zhi.a.wang@intel.com> Cc: Neo Jia <cjia@nvidia.com> Cc: Kirti Wankhede <kwankhede@nvidia.com> Cc: Daniel P. Berrangé <berrange@redhat.com> Cc: Christophe de Dinechin <dinechin@redhat.com>

Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> --- Documentation/vfio-mediated-device.txt | 140 +++++++++++++++++++++++++ 1 file changed, 140 insertions(+)

diff --git a/Documentation/vfio-mediated-device.txt b/Documentation/vfio-mediated-device.txt index c3f69bcaf96e..013a764968eb 100644 --- a/Documentation/vfio-mediated-device.txt +++ b/Documentation/vfio-mediated-device.txt @@ -202,6 +202,7 @@ Directories and files under the sysfs for Each Physical Device | | |--- available_instances | | |--- device_api | | |--- description + | | |--- version | | |--- [devices] | |--- [<type-id>] | | |--- create @@ -209,6 +210,7 @@ Directories and files under the sysfs for Each Physical Device | | |--- available_instances | | |--- device_api | | |--- description + | | |--- version | | |--- [devices] | |--- [<type-id>] | |--- create @@ -216,6 +218,7 @@ Directories and files under the sysfs for Each Physical Device | |--- available_instances | |--- device_api | |--- description + | |--- version | |--- [devices]

I thought there was a request to make this more specific to migration by renaming it to something like migration_version. Also, as an

so this attribute may not only include a mdev device's parent device info and mdev type, but also include numeric software version of vendor specific migration code, right?

It's a vendor defined string, it should be considered opaque to the user, the vendor can include whatever they feel is relevant.

...

This actually makes sense. So, do I need to add a disclaimer in this doc like: vendor driver should be responsible by itself for a mdev device's migration compatibility.

I thought that was the purpose of this attribute.

...

During migration setup phase, general migration code in user space VFIO only checks this version of VFIO migration region, and will not check software version of vendor specific migration code.

What is "software version of vendor specific migration code"? If you're asking whether anything will check for parent device compatibility or parent vendor driver compatibility, the answer is no, that's what this interface is meant to provide. Userspace should need to do nothing more than verify the mdev types match and then use the version attribute to confirm source to target compatibility.

...

It is suggested to incorporate at least parent device info and software version of vendor specific migration code into this migration_version attribute.

We can make recommendations as "best practices", but ultimately it's an opaque string defined by the vendor driver.

But you never addressed my comment that previous reviews asked for the attribute to be named something more specific to migration.

I aggree to rename it to migration_version.

...

...

optional attribute, it seems the example should perhaps not add it to all types to illustrate that it is not required. ok. got it.

...

...

* [mdev_supported_types] @@ -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

between two mdev devices of the same type.

ok. got it. But I have a question about aggregation proposed earlier. Do we also have to assume the two mdev devices are of the same aggregation count? However, aggregation count is not available before a mdev device is created. :(

We don't support aggregation yet, but yes, that's going to introduce issues here. Any configuration beyond the base mdev type would imply that the base type could be compatible for migration, but the specific instances might not. Resolving that would imply that our version information needs to be relative to an instance, not just the base type.

How would we extend this interface to support that? We could have a version attribute on each device instance, which might report something like:

0123456789,aggregate=2

IOW the device instance of version concatenates the mdev type version with the additional create parameters for that device. Writing this to the type attribute should be parsed by the vendor driver as support for given base device with specified additional create parameters.

I'm afraid this also bring us around to treacherous questions around who is responsible for creating that device on the migration target and where is this meta information about the device exposed. Maybe instead of a per instance version attribute we would instead expose the create parameters as an attribute per instance and it would be userspace's responsibility to create a version string from the mdev type and create parameters similar to above. This would also make it possible to create a compatible instance on the target without pre-knowledge of how the device was created.

Also, this issue exists already, but compatibility and capacity are two separate things, I think we want to limit this interface to the former. For instance, if I want to migrate an i915-GVTg_V5_1 device to another system where available_instances for that type is zero, the version attribute should strictly report the device compatibility, it's not responsible for returning an errno due to lack of resources. Similarly if we were to do something with aggregation, the version attribute would strictly report if the target supports creating that device with those parameters, not whether it has capacity to create such as device at that instant in time.

I think it's good to have a migration_version attribute under each device instance. It has two pros: 1. vendor driver can incorperate into the string things like: parent device info + mdev type + aggregate count + software version 2. even for non mdev devices, like a VF in SRIOV PF driver can export this migration_version under a VF instance. so a VF is possible to migrate with vfio-pci driver installed. (though with current VFIO live migration RFCs, with vfio-pci driver is not able to migrate. but it provides a possibility) could we maintain two migration_version attributes? "create parameter + mdev_type migraton_version" for user space software to create a migration compatible mdev device. "per device instance migration_version" for verifying migration compatibility of devices already created.

...

...

...

+ accessed in pairs between the two mdev devices being checked.

"in pairs"? I meant, user space needs to access version attributes from two mdev device. but seems that it's needless to mention that... I'll remove it :)

...

...

+ 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

perhaps "...instead of reading the version string of two mdev devices and comparing them in userspace..." yes, better, thanks:)

...

...

+ 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. + + 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.

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? I think so. I'll add those restrictions in next revision.

If we start adding creation parameters, PATH_MAX may actually be the more reasonable limit.

got it.

...

...

...

+ + 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

Compatible for what purpose? I think this is where specifically calling this a migration_version potentially has value. yes. if it also covers version of vendor specific migration code, calling it migration_version is more appropriate.

I think we're discussing an interface that validates "I [the vendor driver] am able to import the state of this version", therefore it must include every relevant aspect of the vendor support for that.

...

...

...

+ 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.

IOW, the write(2) will succeed if the version is determined to be compatible and otherwise fail with vendor specific errno.

thanks:)

...

...

+ + User space should treat ANY of below conditions as two mdev devices not + compatible:

(0) The mdev devices are not of the same type.

the same as above. do we also need to take aggregation count into consideration?

...

...

+ (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

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. hmm, this is to make life easier for vendor driver to have some types that support migration and others that do not. while we can get rid of returning errno by providing different attribute groups to different devices, the way of returning errno gives a simpler choice to vendors.

Yes, I think it might be overly complicated to provide different attribute groups for different devices, we have more flexibility if the user does not make any assumptions based only on the presence of a version attribute.

...

...

...

+ (3) error when write one mdev device's version string to the other mdev + device's version attribute + + User space should regard two mdev devices compatible when ALL of below + conditions are met:

(0) The mdev devices are of the same type

...

+ (1) success when read from one mdev device's version attribute. + (2) success when write one mdev device's version string to the other mdev + device's version attribute + + Errno: + If vendor driver wants to claim a mdev device incompatible to all other mdev + 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;

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.

yes, user space only gets 0 or 1 as return code, not those errno. maybe I only need to describe errno in patch 2/2.

...

...

+ + 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. + + Example Usage: + case 1: + source side mdev device is of uuid 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-193b. + target side mdev device is if of uuid 882cc4da-dede-11e7-9180-078a62063ab1, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-193b. + + # readlink /sys/bus/pci/devices/0000\:00\:02.0/\ + 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/mdev_type + ../mdev_supported_types/i915-GVTg_V5_4 + + # readlink /sys/bus/pci/devices/0000\:00\:02.0/\ + 882cc4da-dede-11e7-9180-078a62063ab1/mdev_type + ../mdev_supported_types/i915-GVTg_V5_4 + + (1) read source side mdev device's version. + #cat \ + /sys/bus/pci/devices/0000\:00\:02.0/5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/\ + mdev_type/version + 8086-193b-i915-GVTg_V5_4

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.

yes, currently in 2/2, the version only includes <vendor id> + <device id> + <mdev type>. but you are right, it's better to include software migration version number. so vendor drivers have below 3 ways to designate a mdev device has no migration capability. 1. not registering migration_version attribute 2. on reading migration_version, returning errno 3. on reading migration_version, returning string indicating non-migratable.

The reason of not giving up way 2 is that maybe it can accelerate user space getting information of device incompatible. if we only keep way 3, it would not know this info until writing this string to target attribute.

do you agree?

The string is opaque to the user, so if you're asking in (3) that the user read and parse some information in the string that indicates the device is non-migratable then no, I don't agree with that policy. If reading the version attribute produces a result, the only thing the user can do with that result is to test it by writing it to another version attribute. Thanks,

Alex

ok. so we will keep way 2 as a valid choice :) Thanks Yan

...

...

...

+ (2) write source side mdev device's version string into target side mdev + device's version attribute. + # echo 8086-193b-i915-GVTg_V5_4 > + /sys/bus/pci/devices/0000\:00\:02.0/882cc4da-dede-11e7-9180-078a62063ab1/\ + mdev_type/version + # echo $? + 0

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.

got it! great thanks! I'll update it to the next revision.

...

...

+ + in this case, user space's write to target side mdev device's version + attribute returns success to indicate the two mdev devices are compatible. + + case 2: + source side mdev device is of uuid 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-193b. + target side mdev device is if of uuid 882cc4da-dede-11e7-9180-078a62063ab1, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-191b. + + # readlink /sys/bus/pci/devices/0000\:00\:02.0/\ + 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/mdev_type + ../mdev_supported_types/i915-GVTg_V5_4 + + # readlink /sys/bus/pci/devices/0000\:00\:02.0/\ + 882cc4da-dede-11e7-9180-078a62063ab1/mdev_type + ../mdev_supported_types/i915-GVTg_V5_4 + + (1) read source side mdev device's version. + #cat \ + /sys/bus/pci/devices/0000\:00\:02.0/5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/\ + mdev_type/version + 8086-193b-i915-GVTg_V5_4 + + (2) write source side mdev device's version string into target side mdev + device's version attribute. + # echo 8086-193b-i915-GVTg_V5_4 > + /sys/bus/pci/devices/0000\:00\:02.0/882cc4da-dede-11e7-9180-078a62063ab1/\ + mdev_type/version + -bash: echo: write error: Invalid argument + + in this case, user space's write to target side mdev device's version + attribute returns error to indicate the two mdev devices are incompatible. + (incompatible because pci ids of the two mdev devices' parent devices are + different). + + case 3: + source side mdev device is of uuid 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-193b. + But vendor driver does not provide version attribute for this device. + + (1) read source side mdev device's version. + #cat \ + /sys/bus/pci/devices/0000\:00\:02.0/5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/\ + mdev_type/version + cat: '/sys/bus/pci/devices/0000:00:02.0/5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/\ + mdev_type/version': No such file or directory + + in this case, user space reads source side mdev device's version attribute + which does not exist however. user space regards the two mdev devices as not + compatible and will not start migration between the two mdev devices. + +

This is far too long for description and examples, it's not this complicated. Thanks,

got it. I'll follow your above example :)

thanks Yan

...

...

* [device]

This directory contains links to the devices of type <type-id> that have been

On Thu, May 09, 2019 at 11:38:34AM +0800, Alex Williamson wrote:

On Wed, 8 May 2019 23:10:55 -0400 Yan Zhao <yan.y.zhao@intel.com> wrote:

...

On Thu, May 09, 2019 at 05:22:42AM +0800, Alex Williamson wrote:

...

On Wed, 8 May 2019 07:27:40 -0400 Yan Zhao <yan.y.zhao@intel.com> wrote:

...

On Wed, May 08, 2019 at 05:18:26AM +0800, Alex Williamson wrote:

...

On Sun, 5 May 2019 21:49:04 -0400 Yan Zhao <yan.y.zhao@intel.com> 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. So two readings of version attributes + checking in user space are now changed to one reading + one writing of version attributes + checking in vendor driver. Format and length of version strings are now private to vendor driver who can define them freely.

__ user space /\ \ / \write / read \ ______/__ ___\|/___ | version | | version |-->check compatibility ----------- ----------- mdev device A mdev device B

This version attribute is optional. If a mdev device does not provide with a version attribute, this mdev device is incompatible to all other mdev devices.

Live migration is able to take advantage of this version attribute. Before user space actually starts live migration, it can first check whether two mdev devices are compatible.

v2: 1. added detailed intent and usage 2. made definition of version string completely private to vendor driver (Alex Williamson) 3. abandoned changes to sample mdev drivers (Alex Williamson) 4. mandatory --> optional (Cornelia Huck) 5. added description for errno (Cornelia Huck)

Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Erik Skultety <eskultet@redhat.com> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> Cc: Cornelia Huck <cohuck@redhat.com> Cc: "Tian, Kevin" <kevin.tian@intel.com> Cc: Zhenyu Wang <zhenyuw@linux.intel.com> Cc: "Wang, Zhi A" <zhi.a.wang@intel.com> Cc: Neo Jia <cjia@nvidia.com> Cc: Kirti Wankhede <kwankhede@nvidia.com> Cc: Daniel P. Berrangé <berrange@redhat.com> Cc: Christophe de Dinechin <dinechin@redhat.com>

Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> --- Documentation/vfio-mediated-device.txt | 140 +++++++++++++++++++++++++ 1 file changed, 140 insertions(+)

diff --git a/Documentation/vfio-mediated-device.txt b/Documentation/vfio-mediated-device.txt index c3f69bcaf96e..013a764968eb 100644 --- a/Documentation/vfio-mediated-device.txt +++ b/Documentation/vfio-mediated-device.txt @@ -202,6 +202,7 @@ Directories and files under the sysfs for Each Physical Device | | |--- available_instances | | |--- device_api | | |--- description + | | |--- version | | |--- [devices] | |--- [<type-id>] | | |--- create @@ -209,6 +210,7 @@ Directories and files under the sysfs for Each Physical Device | | |--- available_instances | | |--- device_api | | |--- description + | | |--- version | | |--- [devices] | |--- [<type-id>] | |--- create @@ -216,6 +218,7 @@ Directories and files under the sysfs for Each Physical Device | |--- available_instances | |--- device_api | |--- description + | |--- version | |--- [devices]

I thought there was a request to make this more specific to migration by renaming it to something like migration_version. Also, as an

so this attribute may not only include a mdev device's parent device info and mdev type, but also include numeric software version of vendor specific migration code, right?

It's a vendor defined string, it should be considered opaque to the user, the vendor can include whatever they feel is relevant.

...

This actually makes sense. So, do I need to add a disclaimer in this doc like: vendor driver should be responsible by itself for a mdev device's migration compatibility.

I thought that was the purpose of this attribute.

...

During migration setup phase, general migration code in user space VFIO only checks this version of VFIO migration region, and will not check software version of vendor specific migration code.

What is "software version of vendor specific migration code"? If you're asking whether anything will check for parent device compatibility or parent vendor driver compatibility, the answer is no, that's what this interface is meant to provide. Userspace should need to do nothing more than verify the mdev types match and then use the version attribute to confirm source to target compatibility.

...

It is suggested to incorporate at least parent device info and software version of vendor specific migration code into this migration_version attribute.

We can make recommendations as "best practices", but ultimately it's an opaque string defined by the vendor driver.

But you never addressed my comment that previous reviews asked for the attribute to be named something more specific to migration.

I aggree to rename it to migration_version.

...

...

...

optional attribute, it seems the example should perhaps not add it to all types to illustrate that it is not required. ok. got it.

...

...

* [mdev_supported_types] @@ -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

between two mdev devices of the same type.

ok. got it. But I have a question about aggregation proposed earlier. Do we also have to assume the two mdev devices are of the same aggregation count? However, aggregation count is not available before a mdev device is created. :(

We don't support aggregation yet, but yes, that's going to introduce issues here. Any configuration beyond the base mdev type would imply that the base type could be compatible for migration, but the specific instances might not. Resolving that would imply that our version information needs to be relative to an instance, not just the base type.

How would we extend this interface to support that? We could have a version attribute on each device instance, which might report something like:

0123456789,aggregate=2

IOW the device instance of version concatenates the mdev type version with the additional create parameters for that device. Writing this to the type attribute should be parsed by the vendor driver as support for given base device with specified additional create parameters.

I'm afraid this also bring us around to treacherous questions around who is responsible for creating that device on the migration target and where is this meta information about the device exposed. Maybe instead of a per instance version attribute we would instead expose the create parameters as an attribute per instance and it would be userspace's responsibility to create a version string from the mdev type and create parameters similar to above. This would also make it possible to create a compatible instance on the target without pre-knowledge of how the device was created.

Also, this issue exists already, but compatibility and capacity are two separate things, I think we want to limit this interface to the former. For instance, if I want to migrate an i915-GVTg_V5_1 device to another system where available_instances for that type is zero, the version attribute should strictly report the device compatibility, it's not responsible for returning an errno due to lack of resources. Similarly if we were to do something with aggregation, the version attribute would strictly report if the target supports creating that device with those parameters, not whether it has capacity to create such as device at that instant in time.

I think it's good to have a migration_version attribute under each device instance. It has two pros: 1. vendor driver can incorperate into the string things like: parent device info + mdev type + aggregate count + software version

The only thing added here is aggregate count, the rest is available for the base type.

...

2. even for non mdev devices, like a VF in SRIOV PF driver can export this migration_version under a VF instance. so a VF is possible to migrate with vfio-pci driver installed. (though with current VFIO live migration RFCs, with vfio-pci driver is not able to migrate. but it provides a possibility)

I don't follow here, migration version is only one piece of the puzzle to enable the migration of a device, if a PF driver wants to make a VF assignable and migratable, it can wrap it in an mdev layer and export it within the infrastructure we're developing.

I was considering for those devices that assigned using vfio-pci and not planned to migrate until they have to be migrated. But if you think it's not a valid worry, I'm ok with it.

...

could we maintain two migration_version attributes? "create parameter + mdev_type migraton_version" for user space software to create a migration compatible mdev device. "per device instance migration_version" for verifying migration compatibility of devices already created.

I don't think so. If it wasn't obvious in my stream of consciousness previous reply, I prefer the latter mechanism where an instance exposes the creation attributes allowing the user to concatenate the version string and creation attributes to ask the type level version about compatibility. I think it's confusing to have a version on the instance that reports something different than the version on the type but we cannot remove the version on the type because we need to be able to test compatibility before instantiating an instance. Therefore let's not put a version on the instance is the conclusion I've come to. Thanks,

Alex ok. I got your meaning now:) It is (1) keeping "migration_version" attribute under mdev_type. (2) generating a "creation parameter" attribute under each mdev device instance, which is for aggregation and other meta information. right?

so, may I keep this patchset concentrate on device migration_version, and leave create attribute later until aggregation is supported? And is it ok with below statements? User space should treat ANY of below conditions as two mdev devices not migration compatible: (1) The mdev devices are not of the same type. (2) The mdev devices are not of the same creation parameter (e.g. aggregate count, except UUID) (3) any one of the two mdev devices does not have a migration_version attribute (4) error when reading from one mdev device's version attribute (5) error when writing one mdev device's version string to the other mdev device's migration_version attribute Thanks Yan

...

...

...

...

...

+ accessed in pairs between the two mdev devices being checked.

"in pairs"? I meant, user space needs to access version attributes from two mdev device. but seems that it's needless to mention that... I'll remove it :)

...

...

+ 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

perhaps "...instead of reading the version string of two mdev devices and comparing them in userspace..." yes, better, thanks:)

...

...

+ 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. + + 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.

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? I think so. I'll add those restrictions in next revision.

If we start adding creation parameters, PATH_MAX may actually be the more reasonable limit.

got it.

...

...

...

...

+ + 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

Compatible for what purpose? I think this is where specifically calling this a migration_version potentially has value. yes. if it also covers version of vendor specific migration code, calling it migration_version is more appropriate.

I think we're discussing an interface that validates "I [the vendor driver] am able to import the state of this version", therefore it must include every relevant aspect of the vendor support for that.

...

...

...

+ 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.

IOW, the write(2) will succeed if the version is determined to be compatible and otherwise fail with vendor specific errno.

thanks:)

...

...

+ + User space should treat ANY of below conditions as two mdev devices not + compatible:

(0) The mdev devices are not of the same type.

the same as above. do we also need to take aggregation count into consideration?

...

...

+ (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

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. hmm, this is to make life easier for vendor driver to have some types that support migration and others that do not. while we can get rid of returning errno by providing different attribute groups to different devices, the way of returning errno gives a simpler choice to vendors.

Yes, I think it might be overly complicated to provide different attribute groups for different devices, we have more flexibility if the user does not make any assumptions based only on the presence of a version attribute.

...

...

...

+ (3) error when write one mdev device's version string to the other mdev + device's version attribute + + User space should regard two mdev devices compatible when ALL of below + conditions are met:

(0) The mdev devices are of the same type

...

+ (1) success when read from one mdev device's version attribute. + (2) success when write one mdev device's version string to the other mdev + device's version attribute + + Errno: + If vendor driver wants to claim a mdev device incompatible to all other mdev + 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;

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.

yes, user space only gets 0 or 1 as return code, not those errno. maybe I only need to describe errno in patch 2/2.

...

...

+ + 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. + + Example Usage: + case 1: + source side mdev device is of uuid 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-193b. + target side mdev device is if of uuid 882cc4da-dede-11e7-9180-078a62063ab1, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-193b. + + # readlink /sys/bus/pci/devices/0000\:00\:02.0/\ + 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/mdev_type + ../mdev_supported_types/i915-GVTg_V5_4 + + # readlink /sys/bus/pci/devices/0000\:00\:02.0/\ + 882cc4da-dede-11e7-9180-078a62063ab1/mdev_type + ../mdev_supported_types/i915-GVTg_V5_4 + + (1) read source side mdev device's version. + #cat \ + /sys/bus/pci/devices/0000\:00\:02.0/5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/\ + mdev_type/version + 8086-193b-i915-GVTg_V5_4

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.

yes, currently in 2/2, the version only includes <vendor id> + <device id> + <mdev type>. but you are right, it's better to include software migration version number. so vendor drivers have below 3 ways to designate a mdev device has no migration capability. 1. not registering migration_version attribute 2. on reading migration_version, returning errno 3. on reading migration_version, returning string indicating non-migratable.

The reason of not giving up way 2 is that maybe it can accelerate user space getting information of device incompatible. if we only keep way 3, it would not know this info until writing this string to target attribute.

do you agree?

The string is opaque to the user, so if you're asking in (3) that the user read and parse some information in the string that indicates the device is non-migratable then no, I don't agree with that policy. If reading the version attribute produces a result, the only thing the user can do with that result is to test it by writing it to another version attribute. Thanks,

Alex

ok. so we will keep way 2 as a valid choice :)

Thanks Yan

...

...

...

...

+ (2) write source side mdev device's version string into target side mdev + device's version attribute. + # echo 8086-193b-i915-GVTg_V5_4 > + /sys/bus/pci/devices/0000\:00\:02.0/882cc4da-dede-11e7-9180-078a62063ab1/\ + mdev_type/version + # echo $? + 0

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.

got it! great thanks! I'll update it to the next revision.

...

...

+ + in this case, user space's write to target side mdev device's version + attribute returns success to indicate the two mdev devices are compatible. + + case 2: + source side mdev device is of uuid 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-193b. + target side mdev device is if of uuid 882cc4da-dede-11e7-9180-078a62063ab1, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-191b. + + # readlink /sys/bus/pci/devices/0000\:00\:02.0/\ + 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/mdev_type + ../mdev_supported_types/i915-GVTg_V5_4 + + # readlink /sys/bus/pci/devices/0000\:00\:02.0/\ + 882cc4da-dede-11e7-9180-078a62063ab1/mdev_type + ../mdev_supported_types/i915-GVTg_V5_4 + + (1) read source side mdev device's version. + #cat \ + /sys/bus/pci/devices/0000\:00\:02.0/5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/\ + mdev_type/version + 8086-193b-i915-GVTg_V5_4 + + (2) write source side mdev device's version string into target side mdev + device's version attribute. + # echo 8086-193b-i915-GVTg_V5_4 > + /sys/bus/pci/devices/0000\:00\:02.0/882cc4da-dede-11e7-9180-078a62063ab1/\ + mdev_type/version + -bash: echo: write error: Invalid argument + + in this case, user space's write to target side mdev device's version + attribute returns error to indicate the two mdev devices are incompatible. + (incompatible because pci ids of the two mdev devices' parent devices are + different). + + case 3: + source side mdev device is of uuid 5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd, + its mdev type is i915-GVTg_V5_4. pci id of parent device is 8086-193b. + But vendor driver does not provide version attribute for this device. + + (1) read source side mdev device's version. + #cat \ + /sys/bus/pci/devices/0000\:00\:02.0/5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/\ + mdev_type/version + cat: '/sys/bus/pci/devices/0000:00:02.0/5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/\ + mdev_type/version': No such file or directory + + in this case, user space reads source side mdev device's version attribute + which does not exist however. user space regards the two mdev devices as not + compatible and will not start migration between the two mdev devices. + +

This is far too long for description and examples, it's not this complicated. Thanks,

got it. I'll follow your above example :)

thanks Yan

...

...

* [device]

This directory contains links to the devices of type <type-id> that have been

On 2019.05.06 11:20:32 +0800, Zhenyu Wang wrote:

On 2019.05.05 21:51:02 -0400, Yan Zhao wrote:

...

This feature implements the version attribute for Intel's vGPU mdev devices.

version attribute is rw. It's used to check device compatibility for two mdev devices. version string format and length are private for vendor driver. vendor driver is able to define them freely.

For Intel vGPU of gen8 and gen9, the mdev device version consists of 3 fields: "vendor id" + "device id" + "mdev type".

Reading from a vGPU's version attribute, a string is returned in below format: <vendor id>-<device id>-<mdev type>. e.g. 8086-193b-i915-GVTg_V5_2.

Writing a string to a vGPU's version attribute will trigger GVT to check whether a vGPU identified by the written string is compatible with current vGPU owning this version attribute. errno is returned if the two vGPUs are incompatible. The length of written string is returned in compatible case.

For other platforms, and for GVT not supporting vGPU live migration feature, errnos are returned when read/write of mdev devices' version attributes.

For old GVT versions where no version attributes exposed in sysfs, it is regarded as not supporting vGPU live migration.

For future platforms, besides the current 2 fields in vendor proprietary part, more fields may be added to identify Intel vGPU well for live migration purpose.

v2: 1. removed 32 common part of version string (Alex Williamson) 2. do not register version attribute for GVT not supporting live migration.(Cornelia Huck) 3. for platforms out of gen8, gen9, return -EINVAL --> -ENODEV for incompatible. (Cornelia Huck)

Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Erik Skultety <eskultet@redhat.com> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> Cc: Cornelia Huck <cohuck@redhat.com> Cc: "Tian, Kevin" <kevin.tian@intel.com> Cc: Zhenyu Wang <zhenyuw@linux.intel.com> Cc: "Wang, Zhi A" <zhi.a.wang@intel.com> c: Neo Jia <cjia@nvidia.com> Cc: Kirti Wankhede <kwankhede@nvidia.com>

Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> --- drivers/gpu/drm/i915/gvt/Makefile | 2 +- drivers/gpu/drm/i915/gvt/device_version.c | 87 +++++++++++++++++++++++ drivers/gpu/drm/i915/gvt/gvt.c | 51 +++++++++++++ drivers/gpu/drm/i915/gvt/gvt.h | 6 ++ 4 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/i915/gvt/device_version.c

diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile index 271fb46d4dd0..54e209a23899 100644 --- a/drivers/gpu/drm/i915/gvt/Makefile +++ b/drivers/gpu/drm/i915/gvt/Makefile @@ -3,7 +3,7 @@ GVT_DIR := gvt GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o firmware.o \ interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \ execlist.o scheduler.o sched_policy.o mmio_context.o cmd_parser.o debugfs.o \ - fb_decoder.o dmabuf.o page_track.o + fb_decoder.o dmabuf.o page_track.o device_version.o

ccflags-y += -I$(src) -I$(src)/$(GVT_DIR) i915-y += $(addprefix $(GVT_DIR)/, $(GVT_SOURCE)) diff --git a/drivers/gpu/drm/i915/gvt/device_version.c b/drivers/gpu/drm/i915/gvt/device_version.c new file mode 100644 index 000000000000..bd4cdcbdba95 --- /dev/null +++ b/drivers/gpu/drm/i915/gvt/device_version.c @@ -0,0 +1,87 @@ +/* + * Copyright(c) 2011-2017 Intel Corporation. All rights reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + * + * Authors: + * Yan Zhao <yan.y.zhao@intel.com> + */ +#include <linux/vfio.h> +#include "i915_drv.h" + +static bool is_compatible(const char *self, const char *remote) +{ + if (strlen(remote) != strlen(self)) + return false; + + return (strncmp(self, remote, strlen(self))) ? false : true; +} + +ssize_t intel_gvt_get_vfio_device_version_len(struct drm_i915_private *dev_priv) +{ + if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9)) + return -ENODEV; + + return PAGE_SIZE; +} + +ssize_t intel_gvt_get_vfio_device_version(struct drm_i915_private *dev_priv, + char *buf, const char *mdev_type) +{ + int cnt = 0, ret = 0; + const char *str = NULL; +

...

+ /* currently only gen8 & gen9 are supported */ + if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9)) + return -ENODEV; + + /* vendor id + device id + mdev type */ + /* vendor id */ + cnt = snprintf(buf, 5, "%04x", PCI_VENDOR_ID_INTEL); + buf += cnt; + ret += cnt; + + /* device id */ + cnt = snprintf(buf, 6, "-%04x", INTEL_DEVID(dev_priv)); + buf += cnt; + ret += cnt; + + /* mdev type */ + str = mdev_type; + cnt = snprintf(buf, strlen(str) + 3, "-%s\n", mdev_type); + buf += cnt; + ret += cnt; + + return ret; +} + +ssize_t intel_gvt_check_vfio_device_version(struct drm_i915_private *dev_priv, + const char *self, const char *remote) +{ + + /* currently only gen8 & gen9 are supported */ + if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9)) + return -ENODEV; + + if (!is_compatible(self, remote)) + return -EINVAL; + + return 0; +} diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c index 43f4242062dd..19f16eec5a4c 100644 --- a/drivers/gpu/drm/i915/gvt/gvt.c +++ b/drivers/gpu/drm/i915/gvt/gvt.c @@ -105,14 +105,65 @@ static ssize_t description_show(struct kobject *kobj, struct device *dev, type->weight); }

+#ifdef GVT_MIGRATION_VERSION

No extra define.

...

+static ssize_t version_show(struct kobject *kobj, struct device *dev, + char *buf) +{ + struct drm_i915_private *i915 = kdev_to_i915(dev); + const char *mdev_type = kobject_name(kobj); + + return intel_gvt_get_vfio_device_version(i915, buf, mdev_type); +} + +static ssize_t version_store(struct kobject *kobj, struct device *dev, + const char *buf, size_t count) +{ + char *remote = NULL, *self = NULL; + int len, ret = 0; + struct drm_i915_private *i915 = kdev_to_i915(dev); + const char *mdev_type = kobject_name(kobj); + + len = intel_gvt_get_vfio_device_version_len(i915); + if (len < 0) + return len; + + self = kmalloc(len, GFP_KERNEL); + if (!self) + return -ENOMEM; + + ret = intel_gvt_get_vfio_device_version(i915, self, mdev_type); + if (ret < 0) + goto out; + + remote = kstrndup(buf, count, GFP_KERNEL); + if (!remote) { + ret = -ENOMEM; + goto out; + }

Please make device version as attribute for vgpu instead of allocating memory everytime to generate it.

Seems this is attribute for mdev type instead of instance, I was wrong to take it as vgpu instance attribute, so we could add it for vgpu type definition for device with migration.

...

+ + ret = intel_gvt_check_vfio_device_version(i915, self, remote); + +out: + kfree(self); + kfree(remote); + return (ret < 0 ? ret : count); +} +#endif + static MDEV_TYPE_ATTR_RO(available_instances); static MDEV_TYPE_ATTR_RO(device_api); static MDEV_TYPE_ATTR_RO(description); +#ifdef GVT_MIGRATION_VERSION +static MDEV_TYPE_ATTR_RW(version); +#endif

Don't need extra define.

...

static struct attribute *gvt_type_attrs[] = { &mdev_type_attr_available_instances.attr, &mdev_type_attr_device_api.attr, &mdev_type_attr_description.attr, +#ifdef GVT_MIGRATION_VERSION + &mdev_type_attr_version.attr, +#endif NULL, };

I think you need another group of attrs for type that could support migration, it will be assigned during host init for current platform with driver support. So just add new group of attrs for like gvt_migration_type_attrs[] with version.

...

diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h index f5a328b5290a..4062f6b26acf 100644 --- a/drivers/gpu/drm/i915/gvt/gvt.h +++ b/drivers/gpu/drm/i915/gvt/gvt.h @@ -687,6 +687,12 @@ void intel_gvt_debugfs_remove_vgpu(struct intel_vgpu *vgpu); int intel_gvt_debugfs_init(struct intel_gvt *gvt); void intel_gvt_debugfs_clean(struct intel_gvt *gvt);

+ssize_t intel_gvt_get_vfio_device_version(struct drm_i915_private *i915, + char *buf, const char *mdev_type); +ssize_t intel_gvt_check_vfio_device_version(struct drm_i915_private *dev_priv, + const char *self, const char *remote); +ssize_t +intel_gvt_get_vfio_device_version_len(struct drm_i915_private *dev_priv);

#include "trace.h" #include "mpt.h" -- 2.17.1

-- Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

-- Open Source Technology Center, Intel ltd. $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

On Sun, 5 May 2019 21:51:02 -0400 Yan Zhao <yan.y.zhao@intel.com> wrote:

This feature implements the version attribute for Intel's vGPU mdev devices.

version attribute is rw. It's used to check device compatibility for two mdev devices. version string format and length are private for vendor driver. vendor driver is able to define them freely.

For Intel vGPU of gen8 and gen9, the mdev device version consists of 3 fields: "vendor id" + "device id" + "mdev type".

Reading from a vGPU's version attribute, a string is returned in below format: <vendor id>-<device id>-<mdev type>. e.g. 8086-193b-i915-GVTg_V5_2.

Writing a string to a vGPU's version attribute will trigger GVT to check whether a vGPU identified by the written string is compatible with current vGPU owning this version attribute. errno is returned if the two vGPUs are incompatible. The length of written string is returned in compatible case.

For other platforms, and for GVT not supporting vGPU live migration feature, errnos are returned when read/write of mdev devices' version attributes.

For old GVT versions where no version attributes exposed in sysfs, it is regarded as not supporting vGPU live migration.

For future platforms, besides the current 2 fields in vendor proprietary part, more fields may be added to identify Intel vGPU well for live migration purpose.

v2: 1. removed 32 common part of version string (Alex Williamson) 2. do not register version attribute for GVT not supporting live migration.(Cornelia Huck) 3. for platforms out of gen8, gen9, return -EINVAL --> -ENODEV for incompatible. (Cornelia Huck)

Should go below '---'.

Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Erik Skultety <eskultet@redhat.com> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> Cc: Cornelia Huck <cohuck@redhat.com> Cc: "Tian, Kevin" <kevin.tian@intel.com> Cc: Zhenyu Wang <zhenyuw@linux.intel.com> Cc: "Wang, Zhi A" <zhi.a.wang@intel.com> c: Neo Jia <cjia@nvidia.com> Cc: Kirti Wankhede <kwankhede@nvidia.com>

Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> --- drivers/gpu/drm/i915/gvt/Makefile | 2 +- drivers/gpu/drm/i915/gvt/device_version.c | 87 +++++++++++++++++++++++ drivers/gpu/drm/i915/gvt/gvt.c | 51 +++++++++++++ drivers/gpu/drm/i915/gvt/gvt.h | 6 ++ 4 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/i915/gvt/device_version.c

(...)

diff --git a/drivers/gpu/drm/i915/gvt/device_version.c b/drivers/gpu/drm/i915/gvt/device_version.c new file mode 100644 index 000000000000..bd4cdcbdba95 --- /dev/null +++ b/drivers/gpu/drm/i915/gvt/device_version.c @@ -0,0 +1,87 @@ +/* + * Copyright(c) 2011-2017 Intel Corporation. All rights reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + * + * Authors: + * Yan Zhao <yan.y.zhao@intel.com> + */ +#include <linux/vfio.h> +#include "i915_drv.h" + +static bool is_compatible(const char *self, const char *remote) +{ + if (strlen(remote) != strlen(self)) + return false; + + return (strncmp(self, remote, strlen(self))) ? false : true; +} + +ssize_t intel_gvt_get_vfio_device_version_len(struct drm_i915_private *dev_priv) +{ + if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9)) + return -ENODEV; + + return PAGE_SIZE; +} + +ssize_t intel_gvt_get_vfio_device_version(struct drm_i915_private *dev_priv, + char *buf, const char *mdev_type) +{ + int cnt = 0, ret = 0; + const char *str = NULL; + + /* currently only gen8 & gen9 are supported */ + if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9)) + return -ENODEV; + + /* vendor id + device id + mdev type */ + /* vendor id */ + cnt = snprintf(buf, 5, "%04x", PCI_VENDOR_ID_INTEL); + buf += cnt; + ret += cnt; + + /* device id */ + cnt = snprintf(buf, 6, "-%04x", INTEL_DEVID(dev_priv)); + buf += cnt; + ret += cnt; + + /* mdev type */ + str = mdev_type; + cnt = snprintf(buf, strlen(str) + 3, "-%s\n", mdev_type); + buf += cnt; + ret += cnt; + + return ret;

I'm not familiar with this driver; but would it make sense to pre-build the version on init? It does not look to me like the values could change dynamically.

+} + +ssize_t intel_gvt_check_vfio_device_version(struct drm_i915_private *dev_priv, + const char *self, const char *remote) +{ + + /* currently only gen8 & gen9 are supported */ + if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9)) + return -ENODEV; + + if (!is_compatible(self, remote)) + return -EINVAL; + + return 0; +}

Return values look reasonable to me. I'll leave discussions regarding where the attribute should go to folks familiar with this driver.

* Yan Zhao (yan.y.zhao@intel.com) wrote:

This feature implements the version attribute for Intel's vGPU mdev devices.

version attribute is rw. It's used to check device compatibility for two mdev devices. version string format and length are private for vendor driver. vendor driver is able to define them freely.

For Intel vGPU of gen8 and gen9, the mdev device version consists of 3 fields: "vendor id" + "device id" + "mdev type".

Reading from a vGPU's version attribute, a string is returned in below format: <vendor id>-<device id>-<mdev type>. e.g. 8086-193b-i915-GVTg_V5_2.

Writing a string to a vGPU's version attribute will trigger GVT to check whether a vGPU identified by the written string is compatible with current vGPU owning this version attribute. errno is returned if the two vGPUs are incompatible. The length of written string is returned in compatible case.

For other platforms, and for GVT not supporting vGPU live migration feature, errnos are returned when read/write of mdev devices' version attributes.

For old GVT versions where no version attributes exposed in sysfs, it is regarded as not supporting vGPU live migration.

For future platforms, besides the current 2 fields in vendor proprietary part, more fields may be added to identify Intel vGPU well for live migration purpose.

v2: 1. removed 32 common part of version string (Alex Williamson) 2. do not register version attribute for GVT not supporting live migration.(Cornelia Huck) 3. for platforms out of gen8, gen9, return -EINVAL --> -ENODEV for incompatible. (Cornelia Huck)

Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Erik Skultety <eskultet@redhat.com> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> Cc: Cornelia Huck <cohuck@redhat.com> Cc: "Tian, Kevin" <kevin.tian@intel.com> Cc: Zhenyu Wang <zhenyuw@linux.intel.com> Cc: "Wang, Zhi A" <zhi.a.wang@intel.com> c: Neo Jia <cjia@nvidia.com> Cc: Kirti Wankhede <kwankhede@nvidia.com>

Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> --- drivers/gpu/drm/i915/gvt/Makefile | 2 +- drivers/gpu/drm/i915/gvt/device_version.c | 87 +++++++++++++++++++++++ drivers/gpu/drm/i915/gvt/gvt.c | 51 +++++++++++++ drivers/gpu/drm/i915/gvt/gvt.h | 6 ++ 4 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/i915/gvt/device_version.c

diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile index 271fb46d4dd0..54e209a23899 100644 --- a/drivers/gpu/drm/i915/gvt/Makefile +++ b/drivers/gpu/drm/i915/gvt/Makefile @@ -3,7 +3,7 @@ GVT_DIR := gvt GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o firmware.o \ interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \ execlist.o scheduler.o sched_policy.o mmio_context.o cmd_parser.o debugfs.o \ - fb_decoder.o dmabuf.o page_track.o + fb_decoder.o dmabuf.o page_track.o device_version.o

ccflags-y += -I$(src) -I$(src)/$(GVT_DIR) i915-y += $(addprefix $(GVT_DIR)/, $(GVT_SOURCE)) diff --git a/drivers/gpu/drm/i915/gvt/device_version.c b/drivers/gpu/drm/i915/gvt/device_version.c new file mode 100644 index 000000000000..bd4cdcbdba95 --- /dev/null +++ b/drivers/gpu/drm/i915/gvt/device_version.c @@ -0,0 +1,87 @@ +/* + * Copyright(c) 2011-2017 Intel Corporation. All rights reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + * + * Authors: + * Yan Zhao <yan.y.zhao@intel.com> + */ +#include <linux/vfio.h> +#include "i915_drv.h" + +static bool is_compatible(const char *self, const char *remote) +{ + if (strlen(remote) != strlen(self)) + return false; + + return (strncmp(self, remote, strlen(self))) ? false : true; +} + +ssize_t intel_gvt_get_vfio_device_version_len(struct drm_i915_private *dev_priv) +{ + if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9)) + return -ENODEV; + + return PAGE_SIZE; +} + +ssize_t intel_gvt_get_vfio_device_version(struct drm_i915_private *dev_priv, + char *buf, const char *mdev_type) +{ + int cnt = 0, ret = 0; + const char *str = NULL; + + /* currently only gen8 & gen9 are supported */ + if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9)) + return -ENODEV; + + /* vendor id + device id + mdev type */ + /* vendor id */ + cnt = snprintf(buf, 5, "%04x", PCI_VENDOR_ID_INTEL); + buf += cnt; + ret += cnt; + + /* device id */ + cnt = snprintf(buf, 6, "-%04x", INTEL_DEVID(dev_priv)); + buf += cnt; + ret += cnt; + + /* mdev type */ + str = mdev_type; + cnt = snprintf(buf, strlen(str) + 3, "-%s\n", mdev_type); + buf += cnt; + ret += cnt;

Why not just one big snprintf? Dave

+ return ret; +} + +ssize_t intel_gvt_check_vfio_device_version(struct drm_i915_private *dev_priv, + const char *self, const char *remote) +{ + + /* currently only gen8 & gen9 are supported */ + if (!IS_GEN(dev_priv, 8) && !IS_GEN(dev_priv, 9)) + return -ENODEV; + + if (!is_compatible(self, remote)) + return -EINVAL; + + return 0; +} diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c index 43f4242062dd..19f16eec5a4c 100644 --- a/drivers/gpu/drm/i915/gvt/gvt.c +++ b/drivers/gpu/drm/i915/gvt/gvt.c @@ -105,14 +105,65 @@ static ssize_t description_show(struct kobject *kobj, struct device *dev, type->weight); }

+#ifdef GVT_MIGRATION_VERSION +static ssize_t version_show(struct kobject *kobj, struct device *dev, + char *buf) +{ + struct drm_i915_private *i915 = kdev_to_i915(dev); + const char *mdev_type = kobject_name(kobj); + + return intel_gvt_get_vfio_device_version(i915, buf, mdev_type); +} + +static ssize_t version_store(struct kobject *kobj, struct device *dev, + const char *buf, size_t count) +{ + char *remote = NULL, *self = NULL; + int len, ret = 0; + struct drm_i915_private *i915 = kdev_to_i915(dev); + const char *mdev_type = kobject_name(kobj); + + len = intel_gvt_get_vfio_device_version_len(i915); + if (len < 0) + return len; + + self = kmalloc(len, GFP_KERNEL); + if (!self) + return -ENOMEM; + + ret = intel_gvt_get_vfio_device_version(i915, self, mdev_type); + if (ret < 0) + goto out; + + remote = kstrndup(buf, count, GFP_KERNEL); + if (!remote) { + ret = -ENOMEM; + goto out; + } + + ret = intel_gvt_check_vfio_device_version(i915, self, remote); + +out: + kfree(self); + kfree(remote); + return (ret < 0 ? ret : count); +} +#endif + static MDEV_TYPE_ATTR_RO(available_instances); static MDEV_TYPE_ATTR_RO(device_api); static MDEV_TYPE_ATTR_RO(description); +#ifdef GVT_MIGRATION_VERSION +static MDEV_TYPE_ATTR_RW(version); +#endif

static struct attribute *gvt_type_attrs[] = { &mdev_type_attr_available_instances.attr, &mdev_type_attr_device_api.attr, &mdev_type_attr_description.attr, +#ifdef GVT_MIGRATION_VERSION + &mdev_type_attr_version.attr, +#endif NULL, };

diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h index f5a328b5290a..4062f6b26acf 100644 --- a/drivers/gpu/drm/i915/gvt/gvt.h +++ b/drivers/gpu/drm/i915/gvt/gvt.h @@ -687,6 +687,12 @@ void intel_gvt_debugfs_remove_vgpu(struct intel_vgpu *vgpu); int intel_gvt_debugfs_init(struct intel_gvt *gvt); void intel_gvt_debugfs_clean(struct intel_gvt *gvt);

+ssize_t intel_gvt_get_vfio_device_version(struct drm_i915_private *i915, + char *buf, const char *mdev_type); +ssize_t intel_gvt_check_vfio_device_version(struct drm_i915_private *dev_priv, + const char *self, const char *remote); +ssize_t +intel_gvt_get_vfio_device_version_len(struct drm_i915_private *dev_priv);

#include "trace.h" #include "mpt.h" -- 2.17.1

-- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK