Re: [PATCH v5 3/4] vfio/mdev: add migration_version attribute for mdev (under mdev device node)

On Mon, Apr 13, 2020 at 01:55:04AM -0400, Yan Zhao wrote: > migration_version attribute is used to check migration compatibility > between two mdev devices of the same mdev type. > The key is that it's rw and its data is opaque to userspace. > > Userspace reads migration_version of mdev device at source side and > writes the value to migration_version attribute of mdev device at target > side. It judges migration compatibility according to whether the read > and write operations succeed or fail. > > Currently, it is able to read/write migration_version attribute under two > places: > > (1) under mdev_type node > userspace is able to know whether two mdev devices are compatible before > a mdev device is created. > > userspace also needs to check whether the two mdev devices are of the same > mdev type before checking the migration_version attribute. It also needs > to check device creation parameters if aggregation is supported in future. > > (2) under mdev device node > userspace is able to know whether two mdev devices are compatible after > they are all created. But it does not need to check mdev type and device > creation parameter for aggregation as device vendor driver would have > incorporated those information into the migration_version attribute. > > __ userspace > /\ \ > / \write > / read \ > ________/__________ ___\|/_____________ > | migration_version | | migration_version |-->check migration > --------------------- --------------------- compatibility > mdev device A mdev device B > > This patch is for mdev documentation about the second place (under > mdev device node) > > Cc: Alex Williamson <alex.williamson(a)redhat.com&gt; > Cc: Erik Skultety <eskultet(a)redhat.com&gt; > Cc: "Dr. David Alan Gilbert" <dgilbert(a)redhat.com&gt; > Cc: Cornelia Huck <cohuck(a)redhat.com&gt; > Cc: "Tian, Kevin" <kevin.tian(a)intel.com&gt; > Cc: Zhenyu Wang <zhenyuw(a)linux.intel.com&gt; > Cc: "Wang, Zhi A" <zhi.a.wang(a)intel.com&gt; > Cc: Neo Jia <cjia(a)nvidia.com&gt; > Cc: Kirti Wankhede <kwankhede(a)nvidia.com&gt; > Cc: Daniel P. Berrangé <berrange(a)redhat.com&gt; > Cc: Christophe de Dinechin <dinechin(a)redhat.com&gt; > > Signed-off-by: Yan Zhao <yan.y.zhao(a)intel.com&gt; > --- > .../driver-api/vfio-mediated-device.rst | 70 +++++++++++++++++++ > 1 file changed, 70 insertions(+) > > diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst > index 2d1f3c0f3c8f..efbadfd51b7e 100644 > --- a/Documentation/driver-api/vfio-mediated-device.rst > +++ b/Documentation/driver-api/vfio-mediated-device.rst > @@ -383,6 +383,7 @@ Directories and Files Under the sysfs for Each mdev Device > |--- remove > |--- mdev_type {link to its type} > |--- vendor-specific-attributes [optional] > + |--- migration_verion [optional] > > * remove (write only) > > @@ -394,6 +395,75 @@ Example:: > > # echo 1 > /sys/bus/mdev/devices/$mdev_UUID/remove > > +* migration_version (rw, optional) Hmm, ^this is not consistent with how patch 1/5 reports this information, but looking at the existing docs we're not doing very well in terms of consistency there either. I suggest we go with "(read-write)" in both patch 1/5 and here and then start the paragraph with "This is an optional attribute."

> + It is used to check migration compatibility between two mdev devices. > + Absence of this attribute means the mdev device does not support migration. > + > + This attribute provides a way to check migration compatibility between two > + mdev devices from userspace after device created. The intended usage is after the target device has been created. side note: maybe add something like "(see the migration_version attribute of the device node if the target device already exists)" in the same section in patch 1/5.

> + for userspace to read the migration_version attribute from one mdev device and > + then writing that value to the migration_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. Userspace should do nothing more > + than use the migration_version attribute to confirm source to target > + compatibility. ... > + > + Reading/Writing Attribute Data: > + read(2) will fail if a mdev device does not support migration and otherwise > + succeed and return migration_version string of the mdev device. > + > + This migration_version string is vendor defined and opaque to the > + userspace. Vendor is free to include whatever they feel is relevant. > + e.g. <pciid of parent device>-<software version>. > + > + Restrictions on this migration_version string: > + 1. It should only contain ascii characters > + 2. MAX Length is PATH_MAX (4096) > + > + write(2) expects migration_version string of source mdev device, and will > + succeed if it is determined to be compatible and otherwise fail with > + vendor specific errno. > + > + Errno: > + -An errno on read(2) indicates the mdev devicedoes not support migration; s/devicedoes/device does/

> + -An errno on write(2) indicates the mdev devices are incompatible or the > + target doesn't support migration. > + Vendor driver is free to define specific errno and is suggested to > + print detailed error in syslog for diagnose purpose. > + > + Userspace should treat ANY of below conditions as two mdev devices not > + compatible: > + (1) any one of the two mdev devices does not have a migration_version > + attribute > + (2) error when reading from migration_version attribute of one mdev device > + (3) error when writing migration_version string of one mdev device to > + migration_version attribute of the other mdev device > + > + Userspace should regard two mdev devices compatible when ALL of below > + conditions are met: > + (1) success when reading from migration_version attribute of one mdev device. > + (2) success when writing migration_version string of one mdev device to > + migration_version attribute of the other mdev device. > + > + Example Usage: > + (1) Retrieve the mdev source migration_version: > + > + # cat /sys/bus/mdev/devices/$mdev_UUID1/migration_version > + > + If reading the source migration_version generates an error, migration is not > + possible. > + > + (2) Test source migration_version at target: > + > + Given a migration_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/$mdev_UUID2/migration_version > + > + If this write fails, the source and target migration versions are not > + compatible or the target does not support migration. > + > + > Mediated device Hot plug > ------------------------ Overall, the same comments as in 1/5 apply text-wise.

Regards, -- Erik Skultety