Re: [libvirt] [PATCH v2 1/2] vfio/mdev: add version attribute for mdev device
On Thu, May 09, 2019 at 11:38:34AM +0800, Alex Williamson wrote: > On Wed, 8 May 2019 23:10:55 -0400 > Yan Zhao 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 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 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 > > > > > > Cc: Erik Skultety > > > > > > Cc: "Dr. David Alan Gilbert" > > > > > > Cc: Cornelia Huck > > > > > > Cc: "Tian, Kevin" > > > > > > Cc: Zhenyu Wang > > > > > > Cc: "Wang, Zhi A" > > > > > > Cc: Neo Jia > > > > > > Cc: Kirti Wankhede > > > > > > Cc: Daniel P. Berrangé > > > > > > Cc: Christophe de Dinechin > > > > > > > > > > > > Signed-off-by: Yan Zhao > > > > > > --- > > > > > > 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] > > > > > >| |--- [] > > > > > >| | |--- create > > > > > > @@ -209,6 +210,7 @@ Directories and files under the sysfs for Each > > > > > > Physical Device > > > > > >| | |--- available_instances > > > > > >| | |--- device_api > > > > > >| | |--- description > > > > > > + | | |--- version > > > > > >| | |--- [devices] > > > > > >| |--- [] > > > > > >| |--- 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
Re: [libvirt] [PATCH v2 1/2] vfio/mdev: add version attribute for mdev device
On Wed, 8 May 2019 23:10:55 -0400 Yan Zhao 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 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 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 > > > > > Cc: Erik Skultety > > > > > Cc: "Dr. David Alan Gilbert" > > > > > Cc: Cornelia Huck > > > > > Cc: "Tian, Kevin" > > > > > Cc: Zhenyu Wang > > > > > Cc: "Wang, Zhi A" > > > > > Cc: Neo Jia > > > > > Cc: Kirti Wankhede > > > > > Cc: Daniel P. Berrangé > > > > > Cc: Christophe de Dinechin > > > > > > > > > > Signed-off-by: Yan Zhao > > > > > --- > > > > > 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] > > > > >| |--- [] > > > > >| | |--- create > > > > > @@ -209,6 +210,7 @@ Directories and files under the sysfs for Each > > > > > Physical Device > > > > >| | |--- available_instances > > > > >| | |--- device_api > > > > >| | |--- description > > > > > + | | |--- version > > > > >| | |--- [devices] > > > > >| |--- [] > > > > >| |--- 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
Re: [libvirt] [PATCH v2 1/2] vfio/mdev: add version attribute for mdev device
On Thu, May 09, 2019 at 05:22:42AM +0800, Alex Williamson wrote: > On Wed, 8 May 2019 07:27:40 -0400 > Yan Zhao 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 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 > > > > Cc: Erik Skultety > > > > Cc: "Dr. David Alan Gilbert" > > > > Cc: Cornelia Huck > > > > Cc: "Tian, Kevin" > > > > Cc: Zhenyu Wang > > > > Cc: "Wang, Zhi A" > > > > Cc: Neo Jia > > > > Cc: Kirti Wankhede > > > > Cc: Daniel P. Berrangé > > > > Cc: Christophe de Dinechin > > > > > > > > Signed-off-by: Yan Zhao > > > > --- > > > > 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] > > > >| |--- [] > > > >| | |--- create > > > > @@ -209,6 +210,7 @@ Directories and files under the sysfs for Each > > > > Physical Device > > > >| | |--- available_instances > > > >| | |--- device_api > > > >| | |--- description > > > > + | | |--- version > > > >| | |--- [devices] > > > >| |--- [] > > > >| |--- 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
[libvirt] [PATCH 4/4] snapshot: Make virDomainSnapshotDef a virObject
This brings about a couple of benefits:
- use of VIR_AUTOUNREF() simplifies several callers
- Fixes a todo about virDomainMomentObjList not being polymorphic enough
Signed-off-by: Eric Blake
---
src/conf/moment_conf.h| 5 -
src/conf/snapshot_conf.h | 1 -
cfg.mk| 2 --
src/conf/moment_conf.c| 28 -
src/conf/snapshot_conf.c | 34 ++-
src/conf/virdomainmomentobjlist.c | 3 +--
src/esx/esx_driver.c | 3 +--
src/libvirt_private.syms | 1 -
src/qemu/qemu_driver.c| 5 ++---
src/test/test_driver.c| 5 ++---
src/vbox/vbox_common.c| 6 ++
src/vz/vz_driver.c| 3 +--
tests/domainsnapshotxml2xmltest.c | 3 +--
13 files changed, 65 insertions(+), 34 deletions(-)
diff --git a/src/conf/moment_conf.h b/src/conf/moment_conf.h
index 04e0c0648b..00ec1c1904 100644
--- a/src/conf/moment_conf.h
+++ b/src/conf/moment_conf.h
@@ -25,9 +25,12 @@
# include "internal.h"
# include "virconftypes.h"
+# include "virobject.h"
/* Base class for a domain moment */
struct _virDomainMomentDef {
+virObject parent;
+
/* Common portion of public XML. */
char *name;
char *description;
@@ -37,7 +40,7 @@ struct _virDomainMomentDef {
virDomainDefPtr dom;
};
-void virDomainMomentDefClear(virDomainMomentDefPtr def);
+virClassPtr virClassForDomainMomentDef(void);
int virDomainMomentDefPostParse(virDomainMomentDefPtr def);
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index 0ce9dda355..55b7487cfb 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -115,7 +115,6 @@ virDomainSnapshotDefPtr
virDomainSnapshotDefParseNode(xmlDocPtr xml,
bool *current,
unsigned int flags);
virDomainSnapshotDefPtr virDomainSnapshotDefNew(void);
-void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def);
char *virDomainSnapshotDefFormat(const char *uuidstr,
virDomainSnapshotDefPtr def,
virCapsPtr caps,
diff --git a/cfg.mk b/cfg.mk
index b785089910..786aa6e80a 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -133,7 +133,6 @@ useless_free_options = \
--name=virDomainNetDefFree \
--name=virDomainObjFree \
--name=virDomainSmartcardDefFree \
- --name=virDomainSnapshotDefFree \
--name=virDomainSnapshotObjFree \
--name=virDomainSoundDefFree \
--name=virDomainVideoDefFree \
@@ -211,7 +210,6 @@ useless_free_options = \
# y virDomainInputDefFree
# y virDomainNetDefFree
# y virDomainObjFree
-# y virDomainSnapshotDefFree
# n virDomainSnapshotFree (returns int)
# n virDomainSnapshotFreeName (returns int)
# y virDomainSnapshotObjFree
diff --git a/src/conf/moment_conf.c b/src/conf/moment_conf.c
index 9829775b3c..fea13f0f97 100644
--- a/src/conf/moment_conf.c
+++ b/src/conf/moment_conf.c
@@ -34,8 +34,34 @@
VIR_LOG_INIT("conf.moment_conf");
-void virDomainMomentDefClear(virDomainMomentDefPtr def)
+static virClassPtr virDomainMomentDefClass;
+static void virDomainMomentDefDispose(void *obj);
+
+static int
+virDomainMomentOnceInit(void)
{
+if (!VIR_CLASS_NEW(virDomainMomentDef, virClassForObject()))
+return -1;
+
+return 0;
+}
+
+VIR_ONCE_GLOBAL_INIT(virDomainMoment);
+
+virClassPtr
+virClassForDomainMomentDef(void)
+{
+if (virDomainMomentInitialize() < 0)
+return NULL;
+
+return virDomainMomentDefClass;
+}
+
+static void
+virDomainMomentDefDispose(void *obj)
+{
+virDomainMomentDefPtr def = obj;
+
VIR_FREE(def->name);
VIR_FREE(def->description);
VIR_FREE(def->parent_name);
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index e5771ae635..c7f29360e7 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -50,6 +50,20 @@
VIR_LOG_INIT("conf.snapshot_conf");
+static virClassPtr virDomainSnapshotDefClass;
+static void virDomainSnapshotDefDispose(void *obj);
+
+static int
+virDomainSnapshotOnceInit(void)
+{
+if (!VIR_CLASS_NEW(virDomainSnapshotDef, virClassForDomainMomentDef()))
+return -1;
+
+return 0;
+}
+
+VIR_ONCE_GLOBAL_INIT(virDomainSnapshot);
+
VIR_ENUM_IMPL(virDomainSnapshotLocation,
VIR_DOMAIN_SNAPSHOT_LOCATION_LAST,
"default",
@@ -81,30 +95,30 @@ virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr
disk)
disk->src = NULL;
}
+/* Allocate a new virDomainSnapshotDef; free with virObjectUnref() */
virDomainSnapshotDefPtr
virDomainSnapshotDefNew(void)
{
virDomainSnapshotDefPtr def;
-ignore_value(VIR_ALLOC(def));
+if (virDomainSnapshotInitialize() < 0)
+return NULL;
+
+def = virObjectNew(virDomainSnapshotDefClass);
return def;
}
-void
-virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
+static void
+virDomainSnapshotDefDispose(void
[libvirt] [PATCH 0/4] virObject for snapshot def [incremental backup saga]
Peter rightly complained that my attempt to leave a todo in virdomainmomentobjlist.c about not being polymorphic enough gives no incentive to get it fixed later once incremental backups are in, so instead fix it now. My v9 backup patches will be changed similarly to the changes to snapshot shown here. Eric Blake (4): snapshot: s/parent/parent_name/ as prep for virObject snapshot: s/current/parent/ as prep for virObject snapshot: Add virDomainSnapshotDefNew snapshot: Make virDomainSnapshotDef a virObject src/conf/moment_conf.h | 7 +- src/conf/snapshot_conf.h| 4 +- cfg.mk | 2 - src/conf/moment_conf.c | 30 - src/conf/snapshot_conf.c| 164 src/conf/virdomainmomentobjlist.c | 7 +- src/conf/virdomainsnapshotobjlist.c | 2 +- src/esx/esx_driver.c| 19 ++-- src/libvirt_private.syms| 2 +- src/qemu/qemu_domain.c | 10 +- src/qemu/qemu_driver.c | 29 +++-- src/test/test_driver.c | 25 ++--- src/vbox/vbox_common.c | 95 src/vz/vz_driver.c | 5 +- src/vz/vz_sdk.c | 10 +- tests/domainsnapshotxml2xmltest.c | 3 +- 16 files changed, 230 insertions(+), 184 deletions(-) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/4] snapshot: s/parent/parent_name/ as prep for virObject
VIR_CLASS_NEW insists that descendents of virObject have 'parent' as
the name of their inherited member at offset 0. While it would be
possible to write a new class-creation macro that takes the actual
field name, and rewrite VIR_CLASS_NEW to call the new macro with the
hard-coded name 'parent', so that we could make virDomainMomentDef use
a custom name for its base class, it seems less confusing if all
object code uses similar naming. Thus, this is a mechanical rename in
preparation of making virDomainSnapshotDef a descendent of virObject,
when we can no longer use 'parent' for a different purpose than the
base class.
Signed-off-by: Eric Blake
---
src/conf/moment_conf.h| 2 +-
src/conf/moment_conf.c| 2 +-
src/conf/snapshot_conf.c | 22 --
src/conf/virdomainmomentobjlist.c | 4 ++--
src/esx/esx_driver.c | 2 +-
src/qemu/qemu_domain.c| 8
src/qemu/qemu_driver.c| 12 ++--
src/test/test_driver.c| 18 +-
src/vbox/vbox_common.c| 14 +++---
src/vz/vz_sdk.c | 2 +-
10 files changed, 44 insertions(+), 42 deletions(-)
diff --git a/src/conf/moment_conf.h b/src/conf/moment_conf.h
index e06a4a7b3c..04e0c0648b 100644
--- a/src/conf/moment_conf.h
+++ b/src/conf/moment_conf.h
@@ -31,7 +31,7 @@ struct _virDomainMomentDef {
/* Common portion of public XML. */
char *name;
char *description;
-char *parent;
+char *parent_name;
long long creationTime; /* in seconds */
virDomainDefPtr dom;
diff --git a/src/conf/moment_conf.c b/src/conf/moment_conf.c
index 0eb32eeb52..9829775b3c 100644
--- a/src/conf/moment_conf.c
+++ b/src/conf/moment_conf.c
@@ -38,7 +38,7 @@ void virDomainMomentDefClear(virDomainMomentDefPtr def)
{
VIR_FREE(def->name);
VIR_FREE(def->description);
-VIR_FREE(def->parent);
+VIR_FREE(def->parent_name);
virDomainDefFree(def->dom);
}
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index eaa9b3c5e6..b571c5cc41 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -227,7 +227,7 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
goto cleanup;
}
-def->common.parent = virXPathString("string(./parent/name)", ctxt);
+def->common.parent_name = virXPathString("string(./parent/name)",
ctxt);
state = virXPathString("string(./state)", ctxt);
if (state == NULL) {
@@ -809,10 +809,11 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf,
virBufferAsprintf(buf, "%s\n",
virDomainSnapshotStateTypeToString(def->state));
-if (def->common.parent) {
+if (def->common.parent_name) {
virBufferAddLit(buf, "\n");
virBufferAdjustIndent(buf, 2);
-virBufferEscapeString(buf, "%s\n", def->common.parent);
+virBufferEscapeString(buf, "%s\n",
+ def->common.parent_name);
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "\n");
}
@@ -932,30 +933,31 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
bool check_if_stolen;
/* Prevent circular chains */
-if (def->common.parent) {
-if (STREQ(def->common.name, def->common.parent)) {
+if (def->common.parent_name) {
+if (STREQ(def->common.name, def->common.parent_name)) {
virReportError(VIR_ERR_INVALID_ARG,
_("cannot set snapshot %s as its own parent"),
def->common.name);
return -1;
}
-other = virDomainSnapshotFindByName(vm->snapshots, def->common.parent);
+other = virDomainSnapshotFindByName(vm->snapshots,
+def->common.parent_name);
if (!other) {
virReportError(VIR_ERR_INVALID_ARG,
_("parent %s for snapshot %s not found"),
- def->common.parent, def->common.name);
+ def->common.parent_name, def->common.name);
return -1;
}
otherdef = virDomainSnapshotObjGetDef(other);
-while (otherdef->common.parent) {
-if (STREQ(otherdef->common.parent, def->common.name)) {
+while (otherdef->common.parent_name) {
+if (STREQ(otherdef->common.parent_name, def->common.name)) {
virReportError(VIR_ERR_INVALID_ARG,
_("parent %s would create cycle to %s"),
otherdef->common.name, def->common.name);
return -1;
}
other = virDomainSnapshotFindByName(vm->snapshots,
-otherdef->common.parent);
+otherdef->common.parent_name);
if (!other) {
VIR_WARN("snapshots are
[libvirt] [PATCH 2/4] snapshot: s/current/parent/ as prep for virObject
VIR_CLASS_NEW insists that descendents of virObject have 'parent' as
the name of their inherited member at offset 0. While it would be
possible to write a new class-creation macro that takes the actual
field name 'current', and rewrite VIR_CLASS_NEW to call the new macro
with the hard-coded name 'parent', it seems less confusing if all
object code uses similar naming. Thus, this is a mechanical rename in
preparation of making virDomainSnapshotDef a descendent of virObject.
Signed-off-by: Eric Blake
---
src/conf/snapshot_conf.h| 2 +-
src/conf/snapshot_conf.c| 120 ++--
src/conf/virdomainsnapshotobjlist.c | 2 +-
src/esx/esx_driver.c| 16 ++--
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_driver.c | 12 +--
src/test/test_driver.c | 2 +-
src/vbox/vbox_common.c | 88 ++--
src/vz/vz_driver.c | 2 +-
src/vz/vz_sdk.c | 10 +--
10 files changed, 128 insertions(+), 128 deletions(-)
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index 5a762ccd96..f54be11619 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -74,7 +74,7 @@ struct _virDomainSnapshotDiskDef {
/* Stores the complete snapshot metadata */
struct _virDomainSnapshotDef {
-virDomainMomentDef common;
+virDomainMomentDef parent;
/* Additional public XML. */
int state; /* virDomainSnapshotState */
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index b571c5cc41..dd281d57fe 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -88,7 +88,7 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
if (!def)
return;
-virDomainMomentDefClear(>common);
+virDomainMomentDefClear(>parent);
VIR_FREE(def->file);
for (i = 0; i < def->ndisks; i++)
virDomainSnapshotDiskDefClear(>disks[i]);
@@ -208,8 +208,8 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
if (VIR_ALLOC(def) < 0)
goto cleanup;
-def->common.name = virXPathString("string(./name)", ctxt);
-if (def->common.name == NULL) {
+def->parent.name = virXPathString("string(./name)", ctxt);
+if (def->parent.name == NULL) {
if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("a redefined snapshot must have a name"));
@@ -217,17 +217,17 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
}
}
-def->common.description = virXPathString("string(./description)", ctxt);
+def->parent.description = virXPathString("string(./description)", ctxt);
if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) {
if (virXPathLongLong("string(./creationTime)", ctxt,
- >common.creationTime) < 0) {
+ >parent.creationTime) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("missing creationTime from existing snapshot"));
goto cleanup;
}
-def->common.parent_name = virXPathString("string(./parent/name)",
ctxt);
+def->parent.parent_name = virXPathString("string(./parent/name)",
ctxt);
state = virXPathString("string(./state)", ctxt);
if (state == NULL) {
@@ -263,14 +263,14 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
_("missing domain in snapshot"));
goto cleanup;
}
-def->common.dom = virDomainDefParseNode(ctxt->node->doc,
domainNode,
+def->parent.dom = virDomainDefParseNode(ctxt->node->doc,
domainNode,
caps, xmlopt, NULL,
domainflags);
-if (!def->common.dom)
+if (!def->parent.dom)
goto cleanup;
} else {
VIR_WARN("parsing older snapshot that lacks domain");
}
-} else if (virDomainXMLOptionRunMomentPostParse(xmlopt, >common) < 0)
{
+} else if (virDomainXMLOptionRunMomentPostParse(xmlopt, >parent) < 0)
{
goto cleanup;
}
@@ -422,7 +422,7 @@ virDomainSnapshotDefParseString(const char *xmlStr,
/* Perform sanity checking on a redefined snapshot definition. If
- * @other is non-NULL, this may include swapping def->common.dom from other
+ * @other is non-NULL, this may include swapping def->parent.dom from other
* into def. */
int
virDomainSnapshotRedefineValidate(virDomainSnapshotDefPtr def,
@@ -440,17 +440,17 @@ virDomainSnapshotRedefineValidate(virDomainSnapshotDefPtr
def,
virReportError(VIR_ERR_INVALID_ARG,
_("disk-only flag for snapshot %s requires "
"disk-snapshot state"),
- def->common.name);
+ def->parent.name);
return -1;
}
-if (def->common.dom
[libvirt] [PATCH 3/4] snapshot: Add virDomainSnapshotDefNew
In preparation for making virDomainSnapshotDef a descendant of
virObject, it is time to fix all callers that allocate an object to
use virDomainSnapshotDefNew() instead of VIR_ALLOC(). Fortunately,
there aren't very many :)
Signed-off-by: Eric Blake
---
src/conf/snapshot_conf.h | 1 +
src/conf/snapshot_conf.c | 16 +---
src/libvirt_private.syms | 1 +
src/vbox/vbox_common.c | 3 ++-
4 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index f54be11619..0ce9dda355 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -114,6 +114,7 @@ virDomainSnapshotDefPtr
virDomainSnapshotDefParseNode(xmlDocPtr xml,
virDomainXMLOptionPtr
xmlopt,
bool *current,
unsigned int flags);
+virDomainSnapshotDefPtr virDomainSnapshotDefNew(void);
void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def);
char *virDomainSnapshotDefFormat(const char *uuidstr,
virDomainSnapshotDefPtr def,
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index dd281d57fe..e5771ae635 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -81,7 +81,17 @@ virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr
disk)
disk->src = NULL;
}
-void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
+virDomainSnapshotDefPtr
+virDomainSnapshotDefNew(void)
+{
+virDomainSnapshotDefPtr def;
+
+ignore_value(VIR_ALLOC(def));
+return def;
+}
+
+void
+virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
{
size_t i;
@@ -205,8 +215,8 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
bool offline = !!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE);
virSaveCookieCallbacksPtr saveCookie =
virDomainXMLOptionGetSaveCookie(xmlopt);
-if (VIR_ALLOC(def) < 0)
-goto cleanup;
+if (!(def = virDomainSnapshotDefNew()))
+return NULL;
def->parent.name = virXPathString("string(./name)", ctxt);
if (def->parent.name == NULL) {
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a03cf0b645..0474a4e44c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -894,6 +894,7 @@ virDomainSnapshotAlignDisks;
virDomainSnapshotDefFormat;
virDomainSnapshotDefFree;
virDomainSnapshotDefIsExternal;
+virDomainSnapshotDefNew;
virDomainSnapshotDefParseString;
virDomainSnapshotFormatConvertXMLFlags;
virDomainSnapshotIsExternal;
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index af557690c4..7e42f6a4fe 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -6220,7 +6220,8 @@ static char
*vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
if (!(snap = vboxDomainSnapshotGet(data, dom, machine, snapshot->name)))
goto cleanup;
-if (VIR_ALLOC(def) < 0 || !(def->parent.dom = virDomainDefNew()))
+if (!(def = virDomainSnapshotDefNew()) ||
+!(def->parent.dom = virDomainDefNew()))
goto cleanup;
defdom = def->parent.dom;
if (VIR_STRDUP(def->parent.name, snapshot->name) < 0)
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/2] vfio/mdev: add version attribute for mdev device
On Wed, 8 May 2019 07:27:40 -0400 Yan Zhao 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 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 > > > Cc: Erik Skultety > > > Cc: "Dr. David Alan Gilbert" > > > Cc: Cornelia Huck > > > Cc: "Tian, Kevin" > > > Cc: Zhenyu Wang > > > Cc: "Wang, Zhi A" > > > Cc: Neo Jia > > > Cc: Kirti Wankhede > > > Cc: Daniel P. Berrangé > > > Cc: Christophe de Dinechin > > > > > > Signed-off-by: Yan Zhao > > > --- > > > 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] > > >| |--- [] > > >| | |--- create > > > @@ -209,6 +210,7 @@ Directories and files under the sysfs for Each > > > Physical Device > > >| | |--- available_instances > > >| | |--- device_api > > >| | |--- description > > > + | | |--- version > > >| | |--- [devices] > > >| |--- [] > > >| |--- 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
Re: [libvirt] [PATCH 04/10] build: require yajl >= 2.0.3
On Wed, May 08, 2019 at 06:21:21PM +0200, Olaf Hering wrote: > On Wed, Apr 03, Ján Tomko wrote: > > > Since all our supported platforms include at least yajl 2.0.4, > > use pkg-config to detect the library and set the minimum to 2.0.3. > > In case SLE_12 is on the list of supported platforms, this kicked it > off of the list. SLE_12 (and Leap 42.3) comes with version 2.0.1. > SLE_15 has version 2.1.0. Yes, this is a mistake. https://libvirt.org/platforms.html "For distributions with long-lifetime releases, the project will aim to support the most recent major version at all times. Support for the previous major version will be dropped 2 years after the new major version is released. For the purposes of identifying supported software versions, the project will look at RHEL, Debian, Ubuntu LTS, and SLES distros. " SLE 15 was released July 2018, so we should be continuing to support SLE 12 until July 2020. So we need to fix this to allow fallback to the non-pkg-config detection for yajl. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/10] build: require yajl >= 2.0.3
On Wed, Apr 03, Ján Tomko wrote: > Since all our supported platforms include at least yajl 2.0.4, > use pkg-config to detect the library and set the minimum to 2.0.3. In case SLE_12 is on the list of supported platforms, this kicked it off of the list. SLE_12 (and Leap 42.3) comes with version 2.0.1. SLE_15 has version 2.1.0. Olaf signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] QMP; unsigned 64-bit ints; JSON standards compliance
Daniel P. Berrangé writes: > On Tue, May 07, 2019 at 10:47:06AM +0200, Markus Armbruster wrote: > >> > The Golang JSON parser decodes JSON numbers to float64 by default so >> > will have this precision limitation too, though at least they provide >> > a backdoor for custom parsing from the original serialized representation. >> > >> >> > QEMU, and indeed many applications, want to handle 64-bit integers. >> >> > The C JSON library impls have traditionally mapped integers to the >> >> > data type 'long long int' which gives a min/max of -(2^63) / 2^63-1. >> >> > >> >> > QEMU however /really/ needs 64-bit unsigned integers, ie a max 2^64-1. >> >> Correct. >> >> Support for integers 2^63..2^64-1 is relatively recent: commit >> 2bc7cfea095 (v2.10, 2017). >> >> Since we really needed these, the QObject input visitor silently casts >> negative integers to uint64_t. It still does for backward >> compatibility. Commit 5923f85fb82 (right after 2bc7cfea095) explains >> >> The input visitor will cast i64 input to u64 for compatibility >> reasons (existing json QMP client already use negative i64 for large >> u64, and expect an implicit cast in qemu). >> >> Note: before the patch, uint64_t values above INT64_MAX are sent over >> json QMP as negative values, e.g. UINT64_MAX is sent as -1. After the >> patch, they are sent unmodified. Clearly a bug fix, but we have to >> consider compatibility issues anyway. libvirt should cope fine, >> because its parsing of unsigned integers accepts negative values >> modulo 2^64. There's hope that other clients will, too. > > So QEMU reading stuff sent by libvirt in a back compatible manner is > ok. The problem was specifically when a QEMU reply sent back UINT64_MAX > value as a positive number. Yes. The commit message (tries to) explain exactly that. >> >> > Libvirt has historically used the YAJL library which uses 'long long >> >> > int' >> >> > and thus can't officially go beyond 2^63-1 values. Fortunately it lets >> >> > libvirt get at the raw json string, so libvirt can re-parse the value >> >> > to get an 'unsigned long long'. >> >> > >> >> > We recently tried to switch to Jansson because YAJL has a dead upstream >> >> > for many years and countless unanswered bugs & patches. Unfortunately we >> >> > forgot about this need for 2^64-1 max, and Jansson also uses 'long long >> >> > int' >> >> > and raises a fatal parse error for unsigned 64-bit values above 2^63-1. >> >> > It >> >> > also provides no backdoor for libvirt todo its own integer parsing. Thus >> >> > we had to abort our switch to jansson as it broke parsing QEMU's JSON: >> >> > >> >> > https://bugzilla.redhat.com/show_bug.cgi?id=1614569 >> >> > >> >> > Other JSON libraries we've investigated have similar problems. I imagine >> >> > the same may well be true of non-C based JOSN impls, though I've not >> >> > investigated in any detail. >> >> > >> >> > Essentially libvirt is stuck with either using the dead YAJL library >> >> > forever, or writing its own JSON parser (most likely copying QEMU's >> >> > JSON code into libvirt's git). >> >> > >> >> > This feels like a very unappealing situation to be in as not being >> >> > able to use a JSON library of our choice is loosing one of the key >> >> > benefits of using a standard data format. >> >> > >> >> > Thus I'd like to see a solution to this to allow QMP to be reliably >> >> > consumed by any JSON library that exists. >> >> JSON is terrible at interoperability, so good luck with that. >> >> If you reduce your order to "the commonly used JSON libraries we know", >> we can talk. > > I don't particularly want us to rely on semantics of small known set > of JSON libs. I really do want us to do something that is capable of > working with any JSON impl that exists in any programming language. I think we're disagreeing mostly on the meaning of terms, and in ways that don't matter all that much. When you say "any JSON impl that exists in any programming language", then I understand "any conceivable implementation that conforms to the specification". Trouble is the specification underspecifies numbers to a degree that you cannot know what conforming implementations will accept. An implementation that accepts just 0 would still be conforming. It would also be useless. An implementation that accepts just 64 bit signed integers would still be conforming. Useless? Depends. My point is: we cannot know what *all* implementations accept. One of "know" and "all" will have to give. My choice is to relax in accordance with the guidance from RFC 7159 and 8259 I quoted (and you snipped): This specification allows implementations to set limits on the range and precision of numbers accepted. Since software that implements IEEE 754-2008 binary64 (double precision) numbers [IEEE754] is generally available and widely used, good interoperability can be achieved by implementations that expect no
Re: [libvirt] [Qemu-devel] QMP; unsigned 64-bit ints; JSON standards compliance
* Markus Armbruster (arm...@redhat.com) wrote: > Eric Blake writes: > > > On 5/7/19 4:39 AM, Daniel P. Berrangé wrote: > > > >>> JSON is terrible at interoperability, so good luck with that. > >>> > >>> If you reduce your order to "the commonly used JSON libraries we know", > >>> we can talk. > >> > >> I don't particularly want us to rely on semantics of small known set > >> of JSON libs. I really do want us to do something that is capable of > >> working with any JSON impl that exists in any programming language. > >> > >> My suggested option 2 & 3 at least would manage that I believe, as > >> any credible JSON impl will be able to represent 32-bit integers > >> or strings without loosing data. > >> > >> Option 1 would not cope as some impls can't even cope with > >> signed 64-bit ints. > >> > >> I can think of some options: > >> > >> 1. Encode unsigned 64-bit integers as signed 64-bit integers. > >> > >> This follows the example that most C libraries map JSON ints > >> to 'long long int'. This is still relying on undefined > >> behaviour as apps don't need to support > 2^53-1. > >> > >> Apps would need to cast back to 'unsigned long long' for > >> those QMP fields they know are supposed to be unsigned. > >>> > >>> Ugly. It's also what we did until v2.10, August 2017. QMP's input > >>> direction still does it, for backward compatibility. > > > > Having qemu accept signed ints in place of large unsigned values is easy > > enough. But you are right that it loses precision when doubles are > > involved on the receiving end, and we cross the 2^53 barrier. > > > >>> > >> > >> > >> 2. Encode all 64-bit integers as a pair of 32-bit integers. > >> > >> This is fully compliant with the JSON spec as each half > >> is fully within the declared limits. App has to split or > >> assemble the 2 pieces from/to a signed/unsigned 64-bit > >> int as needed. > >>> > >>> Differently ugly. > > > > Particularly ugly as we turn 1<<55 from: > > > > "value":36028797018963968 > > > > into > > > > "value":[8388608,0] > > > > and now both qemu and the client end have to agree that an array of two > > integers is a valid replacement for any larger 64-bit quantity > > (presumably, we'd always accept the array form even for small integer > > values, but only produce the array form for large values). And while it > > manages just fine for uint64_t values, what rules would you place on > > int64_t values? That the resulting 2-integer array is combined with the > > first number as a 2's-complement signed value, and the second being a > > 32-bit unsigned value? > > There's more than one way to encode integers as a list of 53 bit signed > integers. Any of them will do, we just have to specify one. > > >> > >> > >> 3. Encode all 64-bit integers as strings > >> > >> The application has todo all parsing/formatting client > >> side. > >>> > >>> Yet another ugly. > > > > But less so than option 2. > > > > "value":36028797018963968 > > > > vs. > > > > "value":"36028797018963968" > > > > is at least tolerable. > > Yes. > > >> None of these changes are backwards compatible, so I doubt we could > >> make > >> the change transparently in QMP. Instead we would have to have a > >> QMP greeting message capability where the client can request enablement > >> of the enhanced integer handling. > >>> > >>> We might be able to do option 1 without capability negotiation. v2.10's > >>> change from option 1 to what we have now produced zero complaints. > >>> > >>> On the other hand, we made that change for a reason, so we may want a > >>> "send large integers as negative integers" capability regardless. > >>> > >> > >> Any of the three options above would likely work for libvirt, but I > >> would have a slight preference for either 2 or 3, so that we become > >> 100% standards compliant. > > > > If we're going to negotiate something, I'd lean towards option 3 > > (anywhere the introspection states that we accept 'int64' or similar, it > > is also appropriate to send a string value in its place). We'd also have > > to decide if we want to allow "0xabcd", or strictly insist on 43981, > > when stringizing an integer. And while qemu should accept a string or a > > number on input, we'd still have to decide/document whether it's > > response to the client capability negotiation is to output a string > > always, or only for values larger than the 2^53 threshold. > > Picking option 3 is no excuse for complicating matters further. QMP is > primarily for machines. So my first choice would be to keep everything > decimal. I could be persuaded to have QEMU parse integers from strings > with base 0, i.e. leading 0x gets you hex, leading 0 gets you octal. as I said in an earlier reply, my preference would also be to keep it very strict and simple; although
Re: [libvirt] [Qemu-devel] QMP; unsigned 64-bit ints; JSON standards compliance
Eric Blake writes: > On 5/7/19 4:39 AM, Daniel P. Berrangé wrote: > >>> JSON is terrible at interoperability, so good luck with that. >>> >>> If you reduce your order to "the commonly used JSON libraries we know", >>> we can talk. >> >> I don't particularly want us to rely on semantics of small known set >> of JSON libs. I really do want us to do something that is capable of >> working with any JSON impl that exists in any programming language. >> >> My suggested option 2 & 3 at least would manage that I believe, as >> any credible JSON impl will be able to represent 32-bit integers >> or strings without loosing data. >> >> Option 1 would not cope as some impls can't even cope with >> signed 64-bit ints. >> >> I can think of some options: >> >> 1. Encode unsigned 64-bit integers as signed 64-bit integers. >> >> This follows the example that most C libraries map JSON ints >> to 'long long int'. This is still relying on undefined >> behaviour as apps don't need to support > 2^53-1. >> >> Apps would need to cast back to 'unsigned long long' for >> those QMP fields they know are supposed to be unsigned. >>> >>> Ugly. It's also what we did until v2.10, August 2017. QMP's input >>> direction still does it, for backward compatibility. > > Having qemu accept signed ints in place of large unsigned values is easy > enough. But you are right that it loses precision when doubles are > involved on the receiving end, and we cross the 2^53 barrier. > >>> >> >> >> 2. Encode all 64-bit integers as a pair of 32-bit integers. >> >> This is fully compliant with the JSON spec as each half >> is fully within the declared limits. App has to split or >> assemble the 2 pieces from/to a signed/unsigned 64-bit >> int as needed. >>> >>> Differently ugly. > > Particularly ugly as we turn 1<<55 from: > > "value":36028797018963968 > > into > > "value":[8388608,0] > > and now both qemu and the client end have to agree that an array of two > integers is a valid replacement for any larger 64-bit quantity > (presumably, we'd always accept the array form even for small integer > values, but only produce the array form for large values). And while it > manages just fine for uint64_t values, what rules would you place on > int64_t values? That the resulting 2-integer array is combined with the > first number as a 2's-complement signed value, and the second being a > 32-bit unsigned value? There's more than one way to encode integers as a list of 53 bit signed integers. Any of them will do, we just have to specify one. >> >> >> 3. Encode all 64-bit integers as strings >> >> The application has todo all parsing/formatting client >> side. >>> >>> Yet another ugly. > > But less so than option 2. > > "value":36028797018963968 > > vs. > > "value":"36028797018963968" > > is at least tolerable. Yes. >> None of these changes are backwards compatible, so I doubt we could make >> the change transparently in QMP. Instead we would have to have a >> QMP greeting message capability where the client can request enablement >> of the enhanced integer handling. >>> >>> We might be able to do option 1 without capability negotiation. v2.10's >>> change from option 1 to what we have now produced zero complaints. >>> >>> On the other hand, we made that change for a reason, so we may want a >>> "send large integers as negative integers" capability regardless. >>> >> >> Any of the three options above would likely work for libvirt, but I >> would have a slight preference for either 2 or 3, so that we become >> 100% standards compliant. > > If we're going to negotiate something, I'd lean towards option 3 > (anywhere the introspection states that we accept 'int64' or similar, it > is also appropriate to send a string value in its place). We'd also have > to decide if we want to allow "0xabcd", or strictly insist on 43981, > when stringizing an integer. And while qemu should accept a string or a > number on input, we'd still have to decide/document whether it's > response to the client capability negotiation is to output a string > always, or only for values larger than the 2^53 threshold. Picking option 3 is no excuse for complicating matters further. QMP is primarily for machines. So my first choice would be to keep everything decimal. I could be persuaded to have QEMU parse integers from strings with base 0, i.e. leading 0x gets you hex, leading 0 gets you octal. >>> >>> There's no such thing. You mean "we maximize interoperability with >>> common implementations of JSON". >> >> s/common/any/ >> >>> Let's talk implementation for a bit. >>> >>> Encoding and decoding integers in funny ways should be fairly easy in >>> the QObject visitors. The generated QMP marshallers all use them. >>> Trouble is a few commands still bypass the generated marshallers,
Re: [libvirt] [PATCH v2 2/2] drm/i915/gvt: export mdev device version to sysfs for Intel vGPU
On Wed, May 08, 2019 at 06:50:33PM +0800, Dr. David Alan Gilbert wrote:
> * Yan Zhao (yan.y.z...@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: --. 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
> > Cc: Erik Skultety
> > Cc: "Dr. David Alan Gilbert"
> > Cc: Cornelia Huck
> > Cc: "Tian, Kevin"
> > Cc: Zhenyu Wang
> > Cc: "Wang, Zhi A"
> > c: Neo Jia
> > Cc: Kirti Wankhede
> >
> > Signed-off-by: Yan Zhao
> > ---
> > 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 ..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
> > + */
> > +#include
> > +#include "i915_drv.h"
> > +
> > +static bool is_compatible(const char *self, const char *remote)
> > +{
> > + if (strlen(remote) !=
Re: [libvirt] [PATCH v2 2/2] drm/i915/gvt: export mdev device version to sysfs for Intel vGPU
On Tue, May 07, 2019 at 05:27:53PM +0800, Cornelia Huck wrote:
> On Sun, 5 May 2019 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: --. 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 '---'.
>
got it. will change it in next revision.
> >
> > Cc: Alex Williamson
> > Cc: Erik Skultety
> > Cc: "Dr. David Alan Gilbert"
> > Cc: Cornelia Huck
> > Cc: "Tian, Kevin"
> > Cc: Zhenyu Wang
> > Cc: "Wang, Zhi A"
> > c: Neo Jia
> > Cc: Kirti Wankhede
> >
> > Signed-off-by: Yan Zhao
> > ---
> > 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 ..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
> > + */
> > +#include
> > +#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))
> > +
Re: [libvirt] [PATCH v2 1/2] vfio/mdev: add version attribute for mdev device
On Tue, May 07, 2019 at 05:19:54PM +0800, Cornelia Huck wrote: > On Sun, 5 May 2019 21:49:04 -0400 > Yan Zhao 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. :) > > 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?) > Indeed :) > > Format and length of version strings are now private to vendor driver > > who can define them freely. > > Same here; simply drop the 'now'? > ok. thanks:) > > > > __ 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. > as above, let me reword it to migration compatibility. > > > > 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 :) got it:) > > > > Cc: Alex Williamson > > Cc: Erik Skultety > > Cc: "Dr. David Alan Gilbert" > > Cc: Cornelia Huck > > Cc: "Tian, Kevin" > > Cc: Zhenyu Wang > > Cc: "Wang, Zhi A" > > Cc: Neo Jia > > Cc: Kirti Wankhede > > Cc: Daniel P. Berrangé > > Cc: Christophe de Dinechin > > > > Signed-off-by: Yan Zhao > > --- > > 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] > >| |--- [] > >| | |--- create > > @@ -209,6 +210,7 @@ Directories and files under the sysfs for Each Physical > > Device > >| | |--- available_instances > >| | |--- device_api > >| | |--- description > > + | | |--- version > >| | |--- [devices] > >| |--- [] > >| |--- 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 that > > can be > >
Re: [libvirt] [PATCH v2 1/2] vfio/mdev: add version attribute for mdev device
On Wed, May 08, 2019 at 05:18:26AM +0800, Alex Williamson wrote: > On Sun, 5 May 2019 21:49:04 -0400 > Yan Zhao 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 > > Cc: Erik Skultety > > Cc: "Dr. David Alan Gilbert" > > Cc: Cornelia Huck > > Cc: "Tian, Kevin" > > Cc: Zhenyu Wang > > Cc: "Wang, Zhi A" > > Cc: Neo Jia > > Cc: Kirti Wankhede > > Cc: Daniel P. Berrangé > > Cc: Christophe de Dinechin > > > > Signed-off-by: Yan Zhao > > --- > > 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] > >| |--- [] > >| | |--- create > > @@ -209,6 +210,7 @@ Directories and files under the sysfs for Each Physical > > Device > >| | |--- available_instances > >| | |--- device_api > >| | |--- description > > + | | |--- version > >| | |--- [devices] > >| |--- [] > >| |--- 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? 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. 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. It is suggested to incorporate at least parent device info and software version of vendor specific migration code into this migration_version attribute. > 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 that > > can be > >created. > > > > +* version > > + > > + This attribute is rw, and is
Re: [libvirt] [PATCH v2 2/2] drm/i915/gvt: export mdev device version to sysfs for Intel vGPU
* Yan Zhao (yan.y.z...@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: --. 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
> Cc: Erik Skultety
> Cc: "Dr. David Alan Gilbert"
> Cc: Cornelia Huck
> Cc: "Tian, Kevin"
> Cc: Zhenyu Wang
> Cc: "Wang, Zhi A"
> c: Neo Jia
> Cc: Kirti Wankhede
>
> Signed-off-by: Yan Zhao
> ---
> 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 ..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
> + */
> +#include
> +#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
