9:25 p.m.
This series was really just supposed to enable guests to use the
special "host" GIC version, but I ended up changing a bunch of other
stuff and adding a whole lot of new test cases.
I've also made it so the GIC availability is always reflected in the
domain XML, the same way other implicit devices and features work.
The GIC-related definitions are in their own file: depending on
whether we end up probing for host GIC support ourselves or relying
on QEMU this might turn out to be a huge overkill :)
Cheers.
Andrea Bolognani (7):
gic: Introduce virGICVersion enumeration
schema: List allowed GIC versions
conf: Use virGICVersion enumeration in virDomainDef
qemu: Default to GIC v2
qemu: Always enable GIC on ARM virt machines
tests: Reorganize and simplify GIC test cases
tests: Add more GIC test cases
docs/schemas/domaincommon.rng | 6 +++-
src/Makefile.am | 1 +
src/conf/domain_conf.c | 15 ++++----
src/conf/domain_conf.h | 3 +-
src/libvirt_private.syms | 5 +++
src/qemu/qemu_command.c | 8 +++--
src/qemu/qemu_domain.c | 29 ++++++++++++++++
src/util/virgic.c | 33 ++++++++++++++++++
src/util/virgic.h | 38 ++++++++++++++++++++
.../qemuxml2argv-aarch64-aavmf-virtio-mmio.xml | 1 +
.../qemuxml2argv-aarch64-gic-default.args | 1 +
.../qemuxml2argv-aarch64-gic-default.xml | 22 ++++++++++++
...gic.args => qemuxml2argv-aarch64-gic-host.args} | 13 +++----
.../qemuxml2argv-aarch64-gic-host.xml | 22 ++++++++++++
.../qemuxml2argv-aarch64-gic-invalid.xml | 22 ++++++++++++
.../qemuxml2argv-aarch64-gic-none.args | 1 +
.../qemuxml2argv-aarch64-gic-none.xml | 19 ++++++++++
.../qemuxml2argv-aarch64-gic-not-arm.xml | 22 ++++++++++++
.../qemuxml2argv-aarch64-gic-not-virt.xml | 22 ++++++++++++
...gicv3.args => qemuxml2argv-aarch64-gic-v2.args} | 12 +++----
...h64-gic.xml => qemuxml2argv-aarch64-gic-v2.xml} | 14 ++------
.../qemuxml2argv-aarch64-gic-v3.args | 20 +++++++++++
...4-gicv3.xml => qemuxml2argv-aarch64-gic-v3.xml} | 14 ++------
tests/qemuxml2argvtest.c | 40 ++++++++++++++++++----
.../qemuxml2xmlout-aarch64-gic-default.xml | 1 +
.../qemuxml2xmlout-aarch64-gic-none.xml | 1 +
tests/qemuxml2xmltest.c | 7 ++--
27 files changed, 331 insertions(+), 61 deletions(-)
create mode 100644 src/util/virgic.c
create mode 100644 src/util/virgic.h
create mode 120000 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-default.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-default.xml
rename tests/qemuxml2argvdata/{qemuxml2argv-aarch64-gic.args =>
qemuxml2argv-aarch64-gic-host.args} (57%)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-host.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-invalid.xml
create mode 120000 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-none.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-none.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-not-arm.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-not-virt.xml
rename tests/qemuxml2argvdata/{qemuxml2argv-aarch64-gicv3.args =>
qemuxml2argv-aarch64-gic-v2.args} (55%)
rename tests/qemuxml2argvdata/{qemuxml2argv-aarch64-gic.xml =>
qemuxml2argv-aarch64-gic-v2.xml} (61%)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-v3.args
rename tests/qemuxml2argvdata/{qemuxml2argv-aarch64-gicv3.xml =>
qemuxml2argv-aarch64-gic-v3.xml} (61%)
create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-gic-default.xml
create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-gic-none.xml
--
2.5.0
9:25 p.m.
New subject: [libvirt] [PATCH 1/7] gic: Introduce virGICVersion enumeration
We currently blindly accept any numeric value as a GIC version, even
thought only GIC v2 and GIC v3 actually exist; on the other hand, we
reject "host", which is a perfectly legitimate value for QEMU guests.
This new enumeration contains all GIC versions libvirt is aware of.
---
src/Makefile.am | 1 +
src/util/virgic.c | 33 +++++++++++++++++++++++++++++++++
src/util/virgic.h | 38 ++++++++++++++++++++++++++++++++++++++
3 files changed, 72 insertions(+)
create mode 100644 src/util/virgic.c
create mode 100644 src/util/virgic.h
diff --git a/src/Makefile.am b/src/Makefile.am
index a4aef0f..a3859ae 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -113,6 +113,7 @@ UTIL_SOURCES = \
util/virfile.c util/virfile.h \
util/virfirewall.c util/virfirewall.h \
util/virfirewallpriv.h \
+ util/virgic.c util/virgic.h \
util/virhash.c util/virhash.h \
util/virhashcode.c util/virhashcode.h \
util/virhook.c util/virhook.h \
diff --git a/src/util/virgic.c b/src/util/virgic.c
new file mode 100644
index 0000000..e7326d6
--- /dev/null
+++ b/src/util/virgic.c
@@ -0,0 +1,33 @@
+/*
+ * virgic.c: ARM Generic Interrupt Controller support
+ *
+ * Copyright (C) 2016 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>;.
+ *
+ * Author: Andrea Bolognani <abologna(a)redhat.com>
+ */
+
+#include <config.h>
+#include "internal.h"
+#include "virgic.h"
+#include "virutil.h"
+
+VIR_ENUM_IMPL(virGICVersion, VIR_GIC_VERSION_LAST,
+ "none",
+ "host",
+ "2",
+ "3",
+);
diff --git a/src/util/virgic.h b/src/util/virgic.h
new file mode 100644
index 0000000..a2ba300
--- /dev/null
+++ b/src/util/virgic.h
@@ -0,0 +1,38 @@
+/*
+ * virgic.h: ARM Generic Interrupt Controller support
+ *
+ * Copyright (C) 2016 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>;.
+ *
+ * Author: Andrea Bolognani <abologna(a)redhat.com>
+ */
+
+#ifndef __VIR_GIC_H__
+# define __VIR_GIC_H__
+
+# include "virutil.h"
+
+typedef enum {
+ VIR_GIC_VERSION_NONE = 0,
+ VIR_GIC_VERSION_HOST,
+ VIR_GIC_VERSION_2,
+ VIR_GIC_VERSION_3,
+ VIR_GIC_VERSION_LAST
+} virGICVersion;
+
+VIR_ENUM_DECL(virGICVersion);
+
+#endif /* __VIR_GIC_H__ */
--
2.5.0
12:27 a.m.
New subject: [libvirt] [PATCH 1/7] gic: Introduce virGICVersion enumeration
On 02/03/2016 03:25 PM, Andrea Bolognani wrote:
We currently blindly accept any numeric value as a GIC version, even
thought only GIC v2 and GIC v3 actually exist; on the other hand, we
reject "host", which is a perfectly legitimate value for QEMU guests.
This new enumeration contains all GIC versions libvirt is aware of.
---
src/Makefile.am | 1 +
src/util/virgic.c | 33 +++++++++++++++++++++++++++++++++
src/util/virgic.h | 38 ++++++++++++++++++++++++++++++++++++++
3 files changed, 72 insertions(+)
create mode 100644 src/util/virgic.c
create mode 100644 src/util/virgic.h
Seems a bit overkill to give this its own util/ file, but I assume this may
grow extra host probing bits in the future?
Thanks,
Cole
4:15 p.m.
New subject: [libvirt] [PATCH 1/7] gic: Introduce virGICVersion enumeration
On Sat, 2016-02-06 at 18:27 -0500, Cole Robinson wrote:
On 02/03/2016 03:25 PM, Andrea Bolognani wrote:
> We currently blindly accept any numeric value as a GIC version, even
> thought only GIC v2 and GIC v3 actually exist; on the other hand, we
> reject "host", which is a perfectly legitimate value for QEMU guests.
>
> This new enumeration contains all GIC versions libvirt is aware of.
> ---
> src/Makefile.am | 1 +
> src/util/virgic.c | 33 +++++++++++++++++++++++++++++++++
> src/util/virgic.h | 38 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 72 insertions(+)
> create mode 100644 src/util/virgic.c
> create mode 100644 src/util/virgic.h
Seems a bit overkill to give this its own util/ file, but I assume this may
grow extra host probing bits in the future?
That might or might not be the case, as I mentioned in the cover
letter: it mostly depends on whether we end up probing for
supported GIC versions ourselves (in which case the code would
neatly fit here) or rely on QEMU.
I can move this to device_conf.h and take it out later if we add
more GIC-related stuff, or leave it here and move it later. Having
two files just for an enumeration does indeed look a bit silly;
then again device_conf.h is a huge beast at 3000 lines long.
Also, at some point we will need to expose information about
GIC in the capabilites or domcapabilities XML, and conf/ will
not be a great fit anymore either.
Looking forward to your input on this :)
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
5:43 p.m.
New subject: [libvirt] [PATCH 1/7] gic: Introduce virGICVersion enumeration
On 02/08/2016 10:15 AM, Andrea Bolognani wrote:
On Sat, 2016-02-06 at 18:27 -0500, Cole Robinson wrote:
> On 02/03/2016 03:25 PM, Andrea Bolognani wrote:
>> We currently blindly accept any numeric value as a GIC version, even
>> thought only GIC v2 and GIC v3 actually exist; on the other hand, we
>> reject "host", which is a perfectly legitimate value for QEMU guests.
>>
>> This new enumeration contains all GIC versions libvirt is aware of.
>> ---
>> src/Makefile.am | 1 +
>> src/util/virgic.c | 33 +++++++++++++++++++++++++++++++++
>> src/util/virgic.h | 38 ++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 72 insertions(+)
>> create mode 100644 src/util/virgic.c
>> create mode 100644 src/util/virgic.h
>
> Seems a bit overkill to give this its own util/ file, but I assume this may
> grow extra host probing bits in the future?
That might or might not be the case, as I mentioned in the cover
letter: it mostly depends on whether we end up probing for
supported GIC versions ourselves (in which case the code would
neatly fit here) or rely on QEMU.
I can move this to device_conf.h and take it out later if we add
more GIC-related stuff, or leave it here and move it later. Having
two files just for an enumeration does indeed look a bit silly;
then again device_conf.h is a huge beast at 3000 lines long.
I assume you mean domain_conf.h here? Maybe we should have a separate header
and .c file for just the enums, if only to slim down domain_conf.* lengths.
Not needed for this series though.
Also, at some point we will need to expose information about
GIC in the capabilites or domcapabilities XML, and conf/ will
not be a great fit anymore either.
hmm, do we not share enums between domain conf and capabilities? maybe that's
another argument for splitting out the domain enums to their own file so they
are easier to share.
I'm fine with the current util/ layout, or domain_conf. Whatever you think is best
- Cole
3:28 p.m.
New subject: [libvirt] [PATCH 1/7] gic: Introduce virGICVersion enumeration
On 02/03/2016 03:25 PM, Andrea Bolognani wrote:
We currently blindly accept any numeric value as a GIC version, even
thought only GIC v2 and GIC v3 actually exist; on the other hand, we
s/thought/though
reject "host", which is a perfectly legitimate value for
QEMU guests.
This new enumeration contains all GIC versions libvirt is aware of.
---
src/Makefile.am | 1 +
src/util/virgic.c | 33 +++++++++++++++++++++++++++++++++
src/util/virgic.h | 38 ++++++++++++++++++++++++++++++++++++++
3 files changed, 72 insertions(+)
create mode 100644 src/util/virgic.c
create mode 100644 src/util/virgic.h
I agree with Cole - does seem to be overkill for it's own module...
Just a thought.... If 'host' were to become the default, then 'none' is
unnecessary especially since you have a tristate on related to features
for whether <gic> is provided....
That way the version attribute could be optional and whatever is
available on the host "takes over" unless they provide a specific version.
John
diff --git a/src/Makefile.am b/src/Makefile.am
index a4aef0f..a3859ae 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -113,6 +113,7 @@ UTIL_SOURCES = \
util/virfile.c util/virfile.h \
util/virfirewall.c util/virfirewall.h \
util/virfirewallpriv.h \
+ util/virgic.c util/virgic.h \
util/virhash.c util/virhash.h \
util/virhashcode.c util/virhashcode.h \
util/virhook.c util/virhook.h \
diff --git a/src/util/virgic.c b/src/util/virgic.c
new file mode 100644
index 0000000..e7326d6
--- /dev/null
+++ b/src/util/virgic.c
@@ -0,0 +1,33 @@
+/*
+ * virgic.c: ARM Generic Interrupt Controller support
+ *
+ * Copyright (C) 2016 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>;.
+ *
+ * Author: Andrea Bolognani <abologna(a)redhat.com>
+ */
+
+#include <config.h>
+#include "internal.h"
+#include "virgic.h"
+#include "virutil.h"
+
+VIR_ENUM_IMPL(virGICVersion, VIR_GIC_VERSION_LAST,
+ "none",
+ "host",
+ "2",
+ "3",
+);
diff --git a/src/util/virgic.h b/src/util/virgic.h
new file mode 100644
index 0000000..a2ba300
--- /dev/null
+++ b/src/util/virgic.h
@@ -0,0 +1,38 @@
+/*
+ * virgic.h: ARM Generic Interrupt Controller support
+ *
+ * Copyright (C) 2016 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>;.
+ *
+ * Author: Andrea Bolognani <abologna(a)redhat.com>
+ */
+
+#ifndef __VIR_GIC_H__
+# define __VIR_GIC_H__
+
+# include "virutil.h"
+
+typedef enum {
+ VIR_GIC_VERSION_NONE = 0,
+ VIR_GIC_VERSION_HOST,
+ VIR_GIC_VERSION_2,
+ VIR_GIC_VERSION_3,
+ VIR_GIC_VERSION_LAST
+} virGICVersion;
+
+VIR_ENUM_DECL(virGICVersion);
+
+#endif /* __VIR_GIC_H__ */
9:25 p.m.
New subject: [libvirt] [PATCH 2/7] schema: List allowed GIC versions
This change allows to use "host" as a GIC version in the domain XML.
Since we'll need to update the virGICVersion enumeration to support
new GIC versions anyway, it makes sense to be a bit more strict in
the schema as well and reject values that are not in the enumeration.
---
docs/schemas/domaincommon.rng | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 5deb17b..67af93a 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4154,7 +4154,11 @@
<element name="gic">
<optional>
<attribute name="version">
- <ref name="positiveInteger"/>
+ <choice>
+ <value>host</value>
+ <value>2</value>
+ <value>3</value>
+ </choice>
</attribute>
</optional>
</element>
--
2.5.0
3:31 p.m.
New subject: [libvirt] [PATCH 2/7] schema: List allowed GIC versions
On 02/03/2016 03:25 PM, Andrea Bolognani wrote:
This change allows to use "host" as a GIC version in the
domain XML.
Since we'll need to update the virGICVersion enumeration to support
new GIC versions anyway, it makes sense to be a bit more strict in
the schema as well and reject values that are not in the enumeration.
---
docs/schemas/domaincommon.rng | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 5deb17b..67af93a 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4154,7 +4154,11 @@
<element name="gic">
<optional>
Ahhh... even more fuel to make 'host' the default... The 'version'
string is optional...
John
<attribute name="version">
- <ref name="positiveInteger"/>
+ <choice>
+ <value>host</value>
+ <value>2</value>
+ <value>3</value>
+ </choice>
</attribute>
</optional>
</element>
9:25 p.m.
New subject: [libvirt] [PATCH 3/7] conf: Use virGICVersion enumeration in virDomainDef
Instead of allowing any random positive number, restrict the possible
values to the ones that are part of the virGICVersion enumeration.
---
src/conf/domain_conf.c | 15 ++++++++-------
src/conf/domain_conf.h | 3 ++-
src/libvirt_private.syms | 5 +++++
src/qemu/qemu_command.c | 8 +++++---
4 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 55e7ed9..1785b83 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15451,8 +15451,8 @@ virDomainDefParseXML(xmlDocPtr xml,
node = ctxt->node;
ctxt->node = nodes[i];
if ((tmp = virXPathString("string(./@version)", ctxt))) {
- if (virStrToLong_uip(tmp, NULL, 10, &def->gic_version) < 0 ||
- def->gic_version == 0) {
+ if ((def->gic_version = virGICVersionTypeFromString(tmp)) < 0 ||
+ def->gic_version == VIR_GIC_VERSION_NONE) {
virReportError(VIR_ERR_XML_ERROR,
_("malformed gic version: %s"), tmp);
goto error;
@@ -17528,8 +17528,9 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
/* GIC version */
if (src->gic_version != dst->gic_version) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Source GIC version '%u' does not match destination
'%u'"),
- src->gic_version, dst->gic_version);
+ _("Source GIC version '%s' does not match destination
'%s'"),
+ virGICVersionTypeToString(src->gic_version),
+ virGICVersionTypeToString(dst->gic_version));
return false;
}
@@ -22206,9 +22207,9 @@ virDomainDefFormatInternal(virDomainDefPtr def,
case VIR_DOMAIN_FEATURE_GIC:
if (def->features[i] == VIR_TRISTATE_SWITCH_ON) {
virBufferAddLit(buf, "<gic");
- if (def->gic_version)
- virBufferAsprintf(buf, " version='%u'",
- def->gic_version);
+ if (def->gic_version != VIR_GIC_VERSION_NONE)
+ virBufferAsprintf(buf, " version='%s'",
+
virGICVersionTypeToString(def->gic_version));
virBufferAddLit(buf, "/>\n");
}
break;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 9fdfdf2..c14857a 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -50,6 +50,7 @@
# include "virstoragefile.h"
# include "virseclabel.h"
# include "virprocess.h"
+# include "virgic.h"
/* forward declarations of all device types, required by
* virDomainDeviceDef
@@ -2262,7 +2263,7 @@ struct _virDomainDef {
int hyperv_features[VIR_DOMAIN_HYPERV_LAST];
int kvm_features[VIR_DOMAIN_KVM_LAST];
unsigned int hyperv_spinlocks;
- unsigned int gic_version;
+ virGICVersion gic_version;
/* These options are of type virTristateSwitch: ON = keep, OFF = drop */
int caps_features[VIR_DOMAIN_CAPS_FEATURE_LAST];
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 69be352..8e9c986 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1515,6 +1515,11 @@ virFirewallStartRollback;
virFirewallStartTransaction;
+# util/virgic.h
+virGICVersionTypeFromString;
+virGICVersionTypeToString;
+
+
# util/virhash.h
virHashAddEntry;
virHashAtomicNew;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 8943270..1f2b142 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -56,6 +56,7 @@
#include "virtpm.h"
#include "virscsi.h"
#include "virnuma.h"
+#include "virgic.h"
#if defined(__linux__)
# include <linux/capability.h>
#endif
@@ -8007,7 +8008,7 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
}
if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) {
- if (def->gic_version) {
+ if (def->gic_version != VIR_GIC_VERSION_NONE) {
if ((def->os.arch != VIR_ARCH_ARMV7L &&
def->os.arch != VIR_ARCH_AARCH64) ||
(STRNEQ(def->os.machine, "virt") &&
@@ -8022,7 +8023,7 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
/* 2 is the default, so we don't put it as option for
* backwards compatibility
*/
- if (def->gic_version != 2) {
+ if (def->gic_version != VIR_GIC_VERSION_2) {
if (!virQEMUCapsGet(qemuCaps,
QEMU_CAPS_MACH_VIRT_GIC_VERSION)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -8032,7 +8033,8 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
return -1;
}
- virBufferAsprintf(&buf, ",gic-version=%d",
def->gic_version);
+ virBufferAsprintf(&buf, ",gic-version=%s",
+ virGICVersionTypeToString(def->gic_version));
}
}
}
--
2.5.0
3:37 p.m.
New subject: [libvirt] [PATCH 3/7] conf: Use virGICVersion enumeration in virDomainDef
On 02/03/2016 03:25 PM, Andrea Bolognani wrote:
Instead of allowing any random positive number, restrict the
possible
values to the ones that are part of the virGICVersion enumeration.
---
src/conf/domain_conf.c | 15 ++++++++-------
src/conf/domain_conf.h | 3 ++-
src/libvirt_private.syms | 5 +++++
src/qemu/qemu_command.c | 8 +++++---
4 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 55e7ed9..1785b83 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15451,8 +15451,8 @@ virDomainDefParseXML(xmlDocPtr xml,
node = ctxt->node;
ctxt->node = nodes[i];
if ((tmp = virXPathString("string(./@version)", ctxt))) {
- if (virStrToLong_uip(tmp, NULL, 10, &def->gic_version) < 0 ||
- def->gic_version == 0) {
+ if ((def->gic_version = virGICVersionTypeFromString(tmp)) < 0 ||
+ def->gic_version == VIR_GIC_VERSION_NONE) {
virReportError(VIR_ERR_XML_ERROR,
_("malformed gic version: %s"), tmp);
goto error;
@@ -17528,8 +17528,9 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
/* GIC version */
if (src->gic_version != dst->gic_version) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Source GIC version '%u' does not match
destination '%u'"),
- src->gic_version, dst->gic_version);
+ _("Source GIC version '%s' does not match
destination '%s'"),
+ virGICVersionTypeToString(src->gic_version),
+ virGICVersionTypeToString(dst->gic_version));
return false;
}
@@ -22206,9 +22207,9 @@ virDomainDefFormatInternal(virDomainDefPtr def,
case VIR_DOMAIN_FEATURE_GIC:
if (def->features[i] == VIR_TRISTATE_SWITCH_ON) {
virBufferAddLit(buf, "<gic");
- if (def->gic_version)
- virBufferAsprintf(buf, " version='%u'",
- def->gic_version);
+ if (def->gic_version != VIR_GIC_VERSION_NONE)
+ virBufferAsprintf(buf, " version='%s'",
+
virGICVersionTypeToString(def->gic_version));
If you went with 'host' being the default from 1/7, then this becomes
optional...
On input the XML would have <gic>; however, on output the XML would be
<gic version='2'> thus tying this domain to using gic v2 unless someone
changes it to '3' (or 'host' or whatever else in the future). Thus your
5/7 patch to change qemuxml2argv-aarch64-aavmf-virtio-mmio.xml
John
virBufferAddLit(buf, "/>\n");
}
break;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 9fdfdf2..c14857a 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -50,6 +50,7 @@
# include "virstoragefile.h"
# include "virseclabel.h"
# include "virprocess.h"
+# include "virgic.h"
/* forward declarations of all device types, required by
* virDomainDeviceDef
@@ -2262,7 +2263,7 @@ struct _virDomainDef {
int hyperv_features[VIR_DOMAIN_HYPERV_LAST];
int kvm_features[VIR_DOMAIN_KVM_LAST];
unsigned int hyperv_spinlocks;
- unsigned int gic_version;
+ virGICVersion gic_version;
/* These options are of type virTristateSwitch: ON = keep, OFF = drop */
int caps_features[VIR_DOMAIN_CAPS_FEATURE_LAST];
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 69be352..8e9c986 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1515,6 +1515,11 @@ virFirewallStartRollback;
virFirewallStartTransaction;
+# util/virgic.h
+virGICVersionTypeFromString;
+virGICVersionTypeToString;
+
+
# util/virhash.h
virHashAddEntry;
virHashAtomicNew;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 8943270..1f2b142 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -56,6 +56,7 @@
#include "virtpm.h"
#include "virscsi.h"
#include "virnuma.h"
+#include "virgic.h"
#if defined(__linux__)
# include <linux/capability.h>
#endif
@@ -8007,7 +8008,7 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
}
if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) {
- if (def->gic_version) {
+ if (def->gic_version != VIR_GIC_VERSION_NONE) {
if ((def->os.arch != VIR_ARCH_ARMV7L &&
def->os.arch != VIR_ARCH_AARCH64) ||
(STRNEQ(def->os.machine, "virt") &&
@@ -8022,7 +8023,7 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
/* 2 is the default, so we don't put it as option for
* backwards compatibility
*/
- if (def->gic_version != 2) {
+ if (def->gic_version != VIR_GIC_VERSION_2) {
if (!virQEMUCapsGet(qemuCaps,
QEMU_CAPS_MACH_VIRT_GIC_VERSION)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -8032,7 +8033,8 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
return -1;
}
- virBufferAsprintf(&buf, ",gic-version=%d",
def->gic_version);
+ virBufferAsprintf(&buf, ",gic-version=%s",
+ virGICVersionTypeToString(def->gic_version));
}
}
}
9:25 p.m.
New subject: [libvirt] [PATCH 4/7] qemu: Default to GIC v2
When a domain is configured to use GIC but no version has been
specified by the user, default to GIC v2.
---
src/qemu/qemu_domain.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1895520..d120e15 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1239,6 +1239,18 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
static int
+qemuDomainDefAddDefaultFeatures(virDomainDefPtr def)
+{
+ /* Default to GIC v2 if no version was specified */
+ if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON &&
+ def->gic_version == VIR_GIC_VERSION_NONE)
+ def->gic_version = VIR_GIC_VERSION_2;
+
+ return 0;
+}
+
+
+static int
qemuCanonicalizeMachine(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
{
const char *canon;
@@ -1289,6 +1301,9 @@ qemuDomainDefPostParse(virDomainDefPtr def,
if (qemuCanonicalizeMachine(def, qemuCaps) < 0)
goto cleanup;
+ if (qemuDomainDefAddDefaultFeatures(def) < 0)
+ goto cleanup;
+
if (virSecurityManagerVerify(driver->securityManager, def) < 0)
goto cleanup;
--
2.5.0
3:38 p.m.
New subject: [libvirt] [PATCH 4/7] qemu: Default to GIC v2
On 02/03/2016 03:25 PM, Andrea Bolognani wrote:
When a domain is configured to use GIC but no version has been
specified by the user, default to GIC v2.
---
src/qemu/qemu_domain.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1895520..d120e15 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1239,6 +1239,18 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
static int
+qemuDomainDefAddDefaultFeatures(virDomainDefPtr def)
?qemuDomainDefSetDefaultFeatures?
You're not adding gic, it's already added, you're just setting the
default version... although this could be unnecessary if host were the
default...
+{
+ /* Default to GIC v2 if no version was specified */
+ if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON &&
+ def->gic_version == VIR_GIC_VERSION_NONE)
+ def->gic_version = VIR_GIC_VERSION_2;
+
+ return 0;
Since there is no other return value, this should be a void
Also, consider my comment in 1/7... We could set the version=host when
we send the XML to Why not use 'host' as the default? If that's the
case, then this patch goes away.
BTW: Somewhere along the way docs/formatdomain.html.in needs an
adjustment to describe the options (host, 2, 3) and how they work.
John
+}
+
+
+static int
qemuCanonicalizeMachine(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
{
const char *canon;
@@ -1289,6 +1301,9 @@ qemuDomainDefPostParse(virDomainDefPtr def,
if (qemuCanonicalizeMachine(def, qemuCaps) < 0)
goto cleanup;
+ if (qemuDomainDefAddDefaultFeatures(def) < 0)
+ goto cleanup;
+
if (virSecurityManagerVerify(driver->securityManager, def) < 0)
goto cleanup;
2:31 p.m.
New subject: [libvirt] [PATCH 4/7] qemu: Default to GIC v2
On Sun, 2016-02-07 at 09:38 -0500, John Ferlan wrote:
> static int
> +qemuDomainDefAddDefaultFeatures(virDomainDefPtr def)
?qemuDomainDefSetDefaultFeatures?
You're not adding gic, it's already added, you're just setting the
default version... although this could be unnecessary if host were the
default...
Patch 5/7 actually enables the VIR_DOMAIN_FEATURE_GIC feature in the
same function, so the name makes more sense after that one has been
applied as well, but maybe I can keep the two steps separated and
have both AddDefaultFeatures (for turning on features that should be
turned on by default) and SetDefaultFeatures (for setting the specific
value in cases like this, where it's not a simple boolean)?
Or change the name to qemuDomainDefEnableDefaultFeatures() and still
do both things in one place?
> +{
> + /* Default to GIC v2 if no version was specified */
> + if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON
&&
> + def->gic_version == VIR_GIC_VERSION_NONE)
> + def->gic_version = VIR_GIC_VERSION_2;
> +
> + return 0;
Since there is no other return value, this should be a void
Okay, we can change the return type later if we start doing something
that might fail.
BTW: Somewhere along the way docs/formatdomain.html.in needs an
adjustment to describe the options (host, 2, 3) and how they work.
There is already some documentation for GIC in
docs/formatdomain.html.in, but yeah, some improvements would definitely
be a nice addition.
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
2:46 p.m.
New subject: [libvirt] [PATCH 4/7] qemu: Default to GIC v2
On 02/08/2016 08:31 AM, Andrea Bolognani wrote:
On Sun, 2016-02-07 at 09:38 -0500, John Ferlan wrote:
>> static int
>> +qemuDomainDefAddDefaultFeatures(virDomainDefPtr def)
>
> ?qemuDomainDefSetDefaultFeatures?
>
> You're not adding gic, it's already added, you're just setting the
> default version... although this could be unnecessary if host were the
> default...
Patch 5/7 actually enables the VIR_DOMAIN_FEATURE_GIC feature in the
same function, so the name makes more sense after that one has been
applied as well, but maybe I can keep the two steps separated and
have both AddDefaultFeatures (for turning on features that should be
turned on by default) and SetDefaultFeatures (for setting the specific
value in cases like this, where it's not a simple boolean)?
Or change the name to qemuDomainDefEnableDefaultFeatures() and still
do both things in one place?
Enable seems OK. I assume you were perhaps following the lead of
qemuDomainDefAddDefaultDevices.
>> +{
>> + /* Default to GIC v2 if no version was specified */
>> + if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON
&&
>> + def->gic_version == VIR_GIC_VERSION_NONE)
>> + def->gic_version = VIR_GIC_VERSION_2;
>> +
>> + return 0;
>
> Since there is no other return value, this should be a void
Okay, we can change the return type later if we start doing something
that might fail.
> BTW: Somewhere along the way docs/formatdomain.html.in needs an
> adjustment to describe the options (host, 2, 3) and how they work.
There is already some documentation for GIC in
docs/formatdomain.html.in, but yeah, some improvements would definitely
be a nice addition.
Well yes, always good to update the docs especially when you add or
change the valid values of a documented XML attribute.
John
9:26 p.m.
New subject: [libvirt] [PATCH 5/7] qemu: Always enable GIC on ARM virt machines
GIC is always available to ARM virt machines, and the domain XML should
reflect this fact.
---
src/qemu/qemu_domain.c | 14 ++++++++++++++
.../qemuxml2argv-aarch64-aavmf-virtio-mmio.xml | 1 +
2 files changed, 15 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index d120e15..5017cbb 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1241,6 +1241,20 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
static int
qemuDomainDefAddDefaultFeatures(virDomainDefPtr def)
{
+ switch (def->os.arch) {
+ case VIR_ARCH_ARMV7L:
+ case VIR_ARCH_AARCH64:
+ if (STREQ(def->os.machine, "virt") ||
+ STRPREFIX(def->os.machine, "virt-")) {
+ /* GIC is always available to ARM virt machines */
+ def->features[VIR_DOMAIN_FEATURE_GIC] = VIR_TRISTATE_SWITCH_ON;
+ }
+ break;
+
+ default:
+ break;
+ }
+
/* Default to GIC v2 if no version was specified */
if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON &&
def->gic_version == VIR_GIC_VERSION_NONE)
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-aavmf-virtio-mmio.xml
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-aavmf-virtio-mmio.xml
index 4a31c8b..8c7428c 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-aavmf-virtio-mmio.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-aavmf-virtio-mmio.xml
@@ -16,6 +16,7 @@
<acpi/>
<apic/>
<pae/>
+ <gic version='2'/>
</features>
<cpu mode='custom' match='exact'>
<model fallback='allow'>cortex-a53</model>
--
2.5.0
3:42 p.m.
New subject: [libvirt] [PATCH 5/7] qemu: Always enable GIC on ARM virt machines
On 02/03/2016 03:26 PM, Andrea Bolognani wrote:
GIC is always available to ARM virt machines, and the domain XML
should
reflect this fact.
---
src/qemu/qemu_domain.c | 14 ++++++++++++++
.../qemuxml2argv-aarch64-aavmf-virtio-mmio.xml | 1 +
2 files changed, 15 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index d120e15..5017cbb 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1241,6 +1241,20 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
static int
qemuDomainDefAddDefaultFeatures(virDomainDefPtr def)
{
+ switch (def->os.arch) {
+ case VIR_ARCH_ARMV7L:
+ case VIR_ARCH_AARCH64:
+ if (STREQ(def->os.machine, "virt") ||
+ STRPREFIX(def->os.machine, "virt-")) {
+ /* GIC is always available to ARM virt machines */
+ def->features[VIR_DOMAIN_FEATURE_GIC] = VIR_TRISTATE_SWITCH_ON;
See once on - we then have a version='host' and we're good to go.
Of course, as I'm typing I realize that we wouldn't print out
version='host' if it were the default... But that may not be a bad
thing - although we could.
Just throwing some ideas out. Obviously the series works as is, but you
were more involved in the qemu gic discussion so you can go with Cole's
ACK and I'm fine with that.
John
+ }
+ break;
+
+ default:
+ break;
+ }
+
/* Default to GIC v2 if no version was specified */
if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON &&
def->gic_version == VIR_GIC_VERSION_NONE)
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-aavmf-virtio-mmio.xml
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-aavmf-virtio-mmio.xml
index 4a31c8b..8c7428c 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-aavmf-virtio-mmio.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-aavmf-virtio-mmio.xml
@@ -16,6 +16,7 @@
<acpi/>
<apic/>
<pae/>
+ <gic version='2'/>
</features>
<cpu mode='custom' match='exact'>
<model fallback='allow'>cortex-a53</model>
2:41 p.m.
New subject: [libvirt] [PATCH 5/7] qemu: Always enable GIC on ARM virt machines
On Sun, 2016-02-07 at 09:42 -0500, John Ferlan wrote:
On 02/03/2016 03:26 PM, Andrea Bolognani wrote:
> GIC is always available to ARM virt machines, and the domain XML should
> reflect this fact.
> ---
> src/qemu/qemu_domain.c | 14 ++++++++++++++
> .../qemuxml2argv-aarch64-aavmf-virtio-mmio.xml | 1 +
> 2 files changed, 15 insertions(+)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index d120e15..5017cbb 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1241,6 +1241,20 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
> static int
> qemuDomainDefAddDefaultFeatures(virDomainDefPtr def)
> {
> + switch (def->os.arch) {
> + case VIR_ARCH_ARMV7L:
> + case VIR_ARCH_AARCH64:
> + if (STREQ(def->os.machine, "virt") ||
> + STRPREFIX(def->os.machine, "virt-")) {
> + /* GIC is always available to ARM virt machines */
> + def->features[VIR_DOMAIN_FEATURE_GIC] = VIR_TRISTATE_SWITCH_ON;
See once on - we then have a version='host' and we're good to go.
Of course, as I'm typing I realize that we wouldn't print out
version='host' if it were the default... But that may not be a bad
thing - although we could.
That was pretty much the issue :)
For existing guests, <gic/> means <gic version='2'/>, but we pick the
default every single time the XML is loaded instead of explicitly
writing it out in the XML. Which was probably a mistake, because now
we can't really change the default without affecting existing guests :(
On the other hand, QEMU defaults to version 2 as well, so it kinda
fits nicely I guess?
Point is, I think keeping the default to version 2 in libvirt and
have upper layers (eg. virt-install via libosinfo) pick a better
value when creating new guests is in line with how we do a lot of
other stuff in libvirt.
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
4:24 p.m.
New subject: [libvirt] [PATCH 5/7] qemu: Always enable GIC on ARM virt machines
On 02/08/2016 08:41 AM, Andrea Bolognani wrote:
On Sun, 2016-02-07 at 09:42 -0500, John Ferlan wrote:
> On 02/03/2016 03:26 PM, Andrea Bolognani wrote:
>> GIC is always available to ARM virt machines, and the domain XML should
>> reflect this fact.
>> ---
>> src/qemu/qemu_domain.c | 14 ++++++++++++++
>> .../qemuxml2argv-aarch64-aavmf-virtio-mmio.xml | 1 +
>> 2 files changed, 15 insertions(+)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index d120e15..5017cbb 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -1241,6 +1241,20 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
>> static int
>> qemuDomainDefAddDefaultFeatures(virDomainDefPtr def)
>> {
>> + switch (def->os.arch) {
>> + case VIR_ARCH_ARMV7L:
>> + case VIR_ARCH_AARCH64:
>> + if (STREQ(def->os.machine, "virt") ||
>> + STRPREFIX(def->os.machine, "virt-")) {
>> + /* GIC is always available to ARM virt machines */
>> + def->features[VIR_DOMAIN_FEATURE_GIC] = VIR_TRISTATE_SWITCH_ON;
>
> See once on - we then have a version='host' and we're good to go.
>
> Of course, as I'm typing I realize that we wouldn't print out
> version='host' if it were the default... But that may not be a bad
> thing - although we could.
That was pretty much the issue :)
For existing guests, <gic/> means <gic version='2'/>, but we pick
the
default every single time the XML is loaded instead of explicitly
writing it out in the XML. Which was probably a mistake, because now
we can't really change the default without affecting existing guests :(
So yes, 2 is the default, but how it's handled now changes I think.
If one supplies "<gic/>" or "<gic version='2'/>"
prior to these changes,
the qemu args does not list the "gic-version=%d" as output. The note in
the code is "2 is the default, so we don't put it as option for
backwards compatibility". After these changes, if gic_version=2 is
found, still we don't write "gic-version=2" in the command line
(allowing qemu to choose the default, I assume).
However, since 'gic_version' is only set if 'version' is found in the
XML, by changing or setting the default to 2 - wouldn't that cause an
ABI difference (e.g. virDomainDefFeaturesCheckABIStability failure)
because patch 4 says, if GIC is ON and gic_version == 0 (NONE), then set
gic_version == 2? Eventually that "version=2" would be written out.
On the other hand, QEMU defaults to version 2 as well, so it kinda
fits nicely I guess?
I'm not against "2" - I just wasn't keeping up with all the latest
details on that GICv3 thread... For some reason, I had an impression
that supplying "host" was something perhaps desired. If in the long run
"host" for qemu means 2, then great. If in the future qemu changes
"host" to mean 3, then we don't have to change our default value or any
of the checks in qemu_command.c.
Also since "host" would be 0 (once "none" was removed) we have the
added
benefit of a our allocation routines setting our default value.
I wish there were xml2xml output difference check tests prior to this
series... eg. something that would have ensured reading "<gic/>"
resulted in output of "<gic/>", which I don't think will happen after
these patches.
John
Point is, I think keeping the default to version 2 in libvirt and
have upper layers (eg. virt-install via libosinfo) pick a better
value when creating new guests is in line with how we do a lot of
other stuff in libvirt.
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
6:20 p.m.
New subject: [libvirt] [PATCH 5/7] qemu: Always enable GIC on ARM virt machines
On Mon, 2016-02-08 at 10:24 -0500, John Ferlan wrote:
> For existing guests, <gic/> means <gic
version='2'/>, but we pick the
> default every single time the XML is loaded instead of explicitly
> writing it out in the XML. Which was probably a mistake, because now
> we can't really change the default without affecting existing guests :(
So yes, 2 is the default, but how it's handled now changes I think.
If one supplies "<gic/>" or "<gic version='2'/>"
prior to these changes,
the qemu args does not list the "gic-version=%d" as output. The note in
the code is "2 is the default, so we don't put it as option for
backwards compatibility". After these changes, if gic_version=2 is
found, still we don't write "gic-version=2" in the command line
(allowing qemu to choose the default, I assume).
Correct and intended. Also, I've been guaranteed that QEMU's default
will remain version 2, so this is definitely the right thing to do.
However, since 'gic_version' is only set if 'version'
is found in the
XML, by changing or setting the default to 2 - wouldn't that cause an
ABI difference (e.g. virDomainDefFeaturesCheckABIStability failure)
because patch 4 says, if GIC is ON and gic_version == 0 (NONE), then set
gic_version == 2?
Right, I didn't consider that. Thanks a bunch for bringing it up,
this is exactly why code reviews are a good idea! :)
I've discussed this with Michal, since my understanding of the
whole migration business is still fuzzy at best, and he seems to agree
that the change would not cause any issue because old versions of
libvirt can parse <gic version='2'/> just fine, so the ABI check will
not fail.
Eventually that "version=2" would be written out.
That's the idea :)
In general we should really make sure that, whenever we use the
information in the domain XML to infer information that's not there,
or use default values, we write the result back to the domain XML,
so that we're free to change the defaults at a later date knowing
that existing guests will not suddenly get assigned different
hardware than what they expect.
> On the other hand, QEMU defaults to version 2 as well, so it
kinda
> fits nicely I guess?
I'm not against "2" - I just wasn't keeping up with all the latest
details on that GICv3 thread... For some reason, I had an impression
that supplying "host" was something perhaps desired. If in the long run
"host" for qemu means 2, then great. If in the future qemu changes
"host" to mean 3, then we don't have to change our default value or any
of the checks in qemu_command.c.
See above - we're stuck with having version 2 as default exactly
because we never wrote our default to the domain XML.
Higher layers are in a better position to pick optimal defaults
like "host" anyway, so I wouldn't worry about that as long as we
expose all the appropriate knobs.
(AFAIK "host" just means "whatever's available on the host",
which
of course might be 2 or 3 depending on the actual hardware.)
Also since "host" would be 0 (once "none" was
removed) we have the added
benefit of a our allocation routines setting our default value.
That would be nice... We could still achieve the same result by
shuffling the values around:
typedef enum {
VIR_GIC_VERSION_2 = 0,
VIR_GIC_VERSION_3,
VIR_GIC_VERSION_HOST,
VIR_GIC_VERSION_LAST
} virGICVersion;
but that would become a bit icky once GIC version 4 comes out,
wouldn't it? Nothing we wouldn't be able to live with, of
course.
I wish there were xml2xml output difference check tests prior to
this
series... eg. something that would have ensured reading "<gic/>"
resulted in output of "<gic/>", which I don't think will happen
after
these patches.
See above - I believe we actually *want* the value to be printed
out.
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
3:30 p.m.
New subject: [libvirt] [PATCH 5/7] qemu: Always enable GIC on ARM virt machines
Before I forget ;-) or put it off in lieu of other exciting things...
On 02/08/2016 12:20 PM, Andrea Bolognani wrote:
On Mon, 2016-02-08 at 10:24 -0500, John Ferlan wrote:
>> For existing guests, <gic/> means <gic version='2'/>, but we
pick the
>> default every single time the XML is loaded instead of explicitly
>> writing it out in the XML. Which was probably a mistake, because now
>> we can't really change the default without affecting existing guests :(
>
> So yes, 2 is the default, but how it's handled now changes I think.
>
> If one supplies "<gic/>" or "<gic
version='2'/>" prior to these changes,
> the qemu args does not list the "gic-version=%d" as output. The note in
> the code is "2 is the default, so we don't put it as option for
> backwards compatibility". After these changes, if gic_version=2 is
> found, still we don't write "gic-version=2" in the command line
> (allowing qemu to choose the default, I assume).
Correct and intended. Also, I've been guaranteed that QEMU's default
will remain version 2, so this is definitely the right thing to do.
> However, since 'gic_version' is only set if 'version' is found in
the
> XML, by changing or setting the default to 2 - wouldn't that cause an
> ABI difference (e.g. virDomainDefFeaturesCheckABIStability failure)
> because patch 4 says, if GIC is ON and gic_version == 0 (NONE), then set
> gic_version == 2?
Right, I didn't consider that. Thanks a bunch for bringing it up,
this is exactly why code reviews are a good idea! :)
I've discussed this with Michal, since my understanding of the
whole migration business is still fuzzy at best, and he seems to agree
that the change would not cause any issue because old versions of
libvirt can parse <gic version='2'/> just fine, so the ABI check will
not fail.
Ahh - the benefits of intraoffice communication... Anyway, yes it's the
migration and the save/restore code that I was thinking about... If you
want to 'test' you can save a domain, then run restore after... Consider
it the poor man's migration ;-)... Something we used to call
sneakernet... Save something on one disk in the lab, then go grab that
disk and walk it over to a different (and not connected system), install
the disk there, and restore the file ;-) - the old days!
> Eventually that "version=2" would be written out.
That's the idea :)
In general we should really make sure that, whenever we use the
information in the domain XML to infer information that's not there,
or use default values, we write the result back to the domain XML,
so that we're free to change the defaults at a later date knowing
that existing guests will not suddenly get assigned different
hardware than what they expect.
>> On the other hand, QEMU defaults to version 2 as well, so it kinda
>> fits nicely I guess?
>
> I'm not against "2" - I just wasn't keeping up with all the latest
> details on that GICv3 thread... For some reason, I had an impression
> that supplying "host" was something perhaps desired. If in the long run
> "host" for qemu means 2, then great. If in the future qemu changes
> "host" to mean 3, then we don't have to change our default value or
any
> of the checks in qemu_command.c.
See above - we're stuck with having version 2 as default exactly
because we never wrote our default to the domain XML.
Higher layers are in a better position to pick optimal defaults
like "host" anyway, so I wouldn't worry about that as long as we
expose all the appropriate knobs.
I think writing it out is fine - the one concern being the migration or
save/restore type paths. Those usually bite later in some corner case.
I guess the let's make sure radar was triggered because this was a
change in how we store things if someone had only provided <gic> before
and we didn't have the xml2xml test for the GIC version code...
(AFAIK "host" just means "whatever's available on
the host", which
of course might be 2 or 3 depending on the actual hardware.)
> Also since "host" would be 0 (once "none" was removed) we have
the added
> benefit of a our allocation routines setting our default value.
That would be nice... We could still achieve the same result by
shuffling the values around:
typedef enum {
VIR_GIC_VERSION_2 = 0,
VIR_GIC_VERSION_3,
VIR_GIC_VERSION_HOST,
VIR_GIC_VERSION_LAST
} virGICVersion;
but that would become a bit icky once GIC version 4 comes out,
wouldn't it? Nothing we wouldn't be able to live with, of
course.
Yuck - the way patch 1 has the order is best since the "2" and "3"
match
up numerically and adding "4" is then ordered properly.
So let's see that means the original patches should be fine with the
following adjustments:
patch 1 has a typo in the commit message, then ACK
patch 4 could change the function name (use Enable)
patch 4 should use void instead of int for that function, then ACK
If we can add an xml2xml test that would help ensure something doesn't
mess this up for the future.
I feel those type changes are simple enough to not need a new series posted.
John
> I wish there were xml2xml output difference check tests prior to
this
> series... eg. something that would have ensured reading "<gic/>"
> resulted in output of "<gic/>", which I don't think will happen
after
> these patches.
See above - I believe we actually *want* the value to be printed
out.
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
3:49 p.m.
New subject: [libvirt] [PATCH 5/7] qemu: Always enable GIC on ARM virt machines
On Tue, 2016-02-09 at 09:30 -0500, John Ferlan wrote:
So let's see that means the original patches should be fine with
the
following adjustments:
patch 1 has a typo in the commit message, then ACK
patch 4 could change the function name (use Enable)
patch 4 should use void instead of int for that function, then ACK
If we can add an xml2xml test that would help ensure something doesn't
mess this up for the future.
I feel those type changes are simple enough to not need a new series
posted.
I have addressed these comments and pushed the series; the
documentation update you recommended will be prepared and posted
shortly.
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
9:26 p.m.
New subject: [libvirt] [PATCH 6/7] tests: Reorganize and simplify GIC test cases
Unify the naming to prepare for new test cases that will be added
later on.
Moreover, since we're only interested in testing GIC support here,
simplify XML files by getting rid of the unrelevant bits.
---
...v-aarch64-gic.args => qemuxml2argv-aarch64-gic-v2.args} | 13 ++++---------
...rgv-aarch64-gic.xml => qemuxml2argv-aarch64-gic-v2.xml} | 14 ++------------
...aarch64-gicv3.args => qemuxml2argv-aarch64-gic-v3.args} | 12 ++++--------
...v-aarch64-gicv3.xml => qemuxml2argv-aarch64-gic-v3.xml} | 14 ++------------
tests/qemuxml2argvtest.c | 13 +++++++------
tests/qemuxml2xmltest.c | 4 ++--
6 files changed, 21 insertions(+), 49 deletions(-)
rename tests/qemuxml2argvdata/{qemuxml2argv-aarch64-gic.args =>
qemuxml2argv-aarch64-gic-v2.args} (57%)
rename tests/qemuxml2argvdata/{qemuxml2argv-aarch64-gic.xml =>
qemuxml2argv-aarch64-gic-v2.xml} (61%)
rename tests/qemuxml2argvdata/{qemuxml2argv-aarch64-gicv3.args =>
qemuxml2argv-aarch64-gic-v3.args} (55%)
rename tests/qemuxml2argvdata/{qemuxml2argv-aarch64-gicv3.xml =>
qemuxml2argv-aarch64-gic-v3.xml} (61%)
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic.args
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-v2.args
similarity index 57%
rename from tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic.args
rename to tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-v2.args
index be19ea4..d30f449 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-v2.args
@@ -7,19 +7,14 @@ QEMU_AUDIO_DRV=none \
/usr/bin/qemu-system-aarch64 \
-name aarch64test \
-S \
--M virt \
--no-kvm \
--cpu cortex-a53 \
+-machine virt,accel=kvm \
+-cpu host \
-m 1024 \
-smp 1 \
-uuid 6ba410c5-1e5c-4d57-bee7-2228e7ffa32f \
-nographic \
-nodefaults \
-monitor unix:/tmp/test-monitor,server,nowait \
+-no-acpi \
-boot c \
--kernel /aarch64.kernel \
--initrd /aarch64.initrd \
--append console=ttyAMA0 \
--usb \
--net nic,macaddr=52:54:00:09:a4:37,vlan=0,model=virtio,name=net0 \
--net user,vlan=0,name=hostnet0
+-usb
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic.xml
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-v2.xml
similarity index 61%
rename from tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic.xml
rename to tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-v2.xml
index cb595e4..9ccba99 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-v2.xml
@@ -1,4 +1,4 @@
-<domain type='qemu'>
+<domain type='kvm'>
<name>aarch64test</name>
<uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid>
<memory unit='KiB'>1048576</memory>
@@ -6,27 +6,17 @@
<vcpu placement='static'>1</vcpu>
<os>
<type arch='aarch64' machine='virt'>hvm</type>
- <kernel>/aarch64.kernel</kernel>
- <initrd>/aarch64.initrd</initrd>
- <cmdline>console=ttyAMA0</cmdline>
<boot dev='hd'/>
</os>
<features>
- <acpi/>
<gic version='2'/>
</features>
- <cpu mode='custom' match='exact'>
- <model fallback='allow'>cortex-a53</model>
- </cpu>
+ <cpu mode='host-passthrough'/>
<clock offset='utc'/>
<on_poweroff>destroy</on_poweroff>
<on_reboot>restart</on_reboot>
<on_crash>destroy</on_crash>
<devices>
<emulator>/usr/bin/qemu-system-aarch64</emulator>
- <interface type='user'>
- <mac address='52:54:00:09:a4:37'/>
- <model type='virtio'/>
- </interface>
</devices>
</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gicv3.args
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-v3.args
similarity index 55%
rename from tests/qemuxml2argvdata/qemuxml2argv-aarch64-gicv3.args
rename to tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-v3.args
index a3530bb..9cd86ac 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gicv3.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-v3.args
@@ -7,18 +7,14 @@ QEMU_AUDIO_DRV=none \
/usr/bin/qemu-system-aarch64 \
-name aarch64test \
-S \
--machine virt,accel=tcg,gic-version=3 \
--cpu cortex-a53 \
+-machine virt,accel=kvm,gic-version=3 \
+-cpu host \
-m 1024 \
-smp 1 \
-uuid 6ba410c5-1e5c-4d57-bee7-2228e7ffa32f \
-nographic \
-nodefaults \
-monitor unix:/tmp/test-monitor,server,nowait \
+-no-acpi \
-boot c \
--kernel /aarch64.kernel \
--initrd /aarch64.initrd \
--append console=ttyAMA0 \
--usb \
--net nic,macaddr=52:54:00:09:a4:37,vlan=0,model=virtio,name=net0 \
--net user,vlan=0,name=hostnet0
+-usb
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gicv3.xml
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-v3.xml
similarity index 61%
rename from tests/qemuxml2argvdata/qemuxml2argv-aarch64-gicv3.xml
rename to tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-v3.xml
index 72aaaf7..7c9ee92 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gicv3.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-v3.xml
@@ -1,4 +1,4 @@
-<domain type='qemu'>
+<domain type='kvm'>
<name>aarch64test</name>
<uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid>
<memory unit='KiB'>1048576</memory>
@@ -6,27 +6,17 @@
<vcpu placement='static'>1</vcpu>
<os>
<type arch='aarch64' machine='virt'>hvm</type>
- <kernel>/aarch64.kernel</kernel>
- <initrd>/aarch64.initrd</initrd>
- <cmdline>console=ttyAMA0</cmdline>
<boot dev='hd'/>
</os>
<features>
- <acpi/>
<gic version='3'/>
</features>
- <cpu mode='custom' match='exact'>
- <model fallback='allow'>cortex-a53</model>
- </cpu>
+ <cpu mode='host-passthrough'/>
<clock offset='utc'/>
<on_poweroff>destroy</on_poweroff>
<on_reboot>restart</on_reboot>
<on_crash>destroy</on_crash>
<devices>
<emulator>/usr/bin/qemu-system-aarch64</emulator>
- <interface type='user'>
- <mac address='52:54:00:09:a4:37'/>
- <model type='virtio'/>
- </interface>
</devices>
</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index a5d4722..2ad5f5d 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1673,13 +1673,14 @@ mymain(void)
DO_TEST("aarch64-cpu-passthrough", QEMU_CAPS_DEVICE,
QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DEVICE_VIRTIO_MMIO,
QEMU_CAPS_CPU_HOST, QEMU_CAPS_KVM);
- DO_TEST("aarch64-gic", QEMU_CAPS_DEVICE,
- QEMU_CAPS_KVM);
- DO_TEST("aarch64-gicv3", QEMU_CAPS_DEVICE,
- QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT,
+ DO_TEST("aarch64-gic-v2", QEMU_CAPS_DEVICE,
+ QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_CPU_HOST,
QEMU_CAPS_MACH_VIRT_GIC_VERSION);
- DO_TEST_FAILURE("aarch64-gicv3", QEMU_CAPS_DEVICE,
- QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT);
+ DO_TEST("aarch64-gic-v3", QEMU_CAPS_DEVICE,
+ QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_CPU_HOST,
+ QEMU_CAPS_MACH_VIRT_GIC_VERSION);
+ DO_TEST_FAILURE("aarch64-gic-v3", QEMU_CAPS_DEVICE,
+ QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_CPU_HOST);
driver.caps->host.cpu->arch = VIR_ARCH_AARCH64;
DO_TEST("aarch64-kvm-32-on-64", QEMU_CAPS_DEVICE,
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 19e6c1b..bdf96a6 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -604,8 +604,8 @@ mymain(void)
DO_TEST("smbios-multiple-type2");
DO_TEST("aarch64-aavmf-virtio-mmio");
- DO_TEST("aarch64-gic");
- DO_TEST("aarch64-gicv3");
+ DO_TEST("aarch64-gic-v2");
+ DO_TEST("aarch64-gic-v3");
DO_TEST("memory-hotplug");
DO_TEST("memory-hotplug-nonuma");
--
2.5.0
9:26 p.m.
New subject: [libvirt] [PATCH 7/7] tests: Add more GIC test cases
Test all kinds of scenarios, including guests asking for GIC but
failing to specify a version, guests specifying an invalid version
and guests trying to use GIC with non-virt or even non-ARM machines.
---
.../qemuxml2argv-aarch64-gic-default.args | 1 +
.../qemuxml2argv-aarch64-gic-default.xml | 22 ++++++++++++++++++
.../qemuxml2argv-aarch64-gic-host.args | 20 ++++++++++++++++
.../qemuxml2argv-aarch64-gic-host.xml | 22 ++++++++++++++++++
.../qemuxml2argv-aarch64-gic-invalid.xml | 22 ++++++++++++++++++
.../qemuxml2argv-aarch64-gic-none.args | 1 +
.../qemuxml2argv-aarch64-gic-none.xml | 19 +++++++++++++++
.../qemuxml2argv-aarch64-gic-not-arm.xml | 22 ++++++++++++++++++
.../qemuxml2argv-aarch64-gic-not-virt.xml | 22 ++++++++++++++++++
tests/qemuxml2argvtest.c | 27 ++++++++++++++++++++++
.../qemuxml2xmlout-aarch64-gic-default.xml | 1 +
.../qemuxml2xmlout-aarch64-gic-none.xml | 1 +
tests/qemuxml2xmltest.c | 3 +++
13 files changed, 183 insertions(+)
create mode 120000 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-default.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-default.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-host.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-host.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-invalid.xml
create mode 120000 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-none.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-none.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-not-arm.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-not-virt.xml
create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-gic-default.xml
create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-gic-none.xml
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-default.args
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-default.args
new file mode 120000
index 0000000..3234039
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-default.args
@@ -0,0 +1 @@
+qemuxml2argv-aarch64-gic-v2.args
\ No newline at end of file
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-default.xml
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-default.xml
new file mode 100644
index 0000000..b219972
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-default.xml
@@ -0,0 +1,22 @@
+<domain type='kvm'>
+ <name>aarch64test</name>
+ <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid>
+ <memory unit='KiB'>1048576</memory>
+ <currentMemory unit='KiB'>1048576</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='aarch64' machine='virt'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <features>
+ <gic/>
+ </features>
+ <cpu mode='host-passthrough'/>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-aarch64</emulator>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-host.args
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-host.args
new file mode 100644
index 0000000..56adc72
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-host.args
@@ -0,0 +1,20 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-aarch64 \
+-name aarch64test \
+-S \
+-machine virt,accel=kvm,gic-version=host \
+-cpu host \
+-m 1024 \
+-smp 1 \
+-uuid 6ba410c5-1e5c-4d57-bee7-2228e7ffa32f \
+-nographic \
+-nodefaults \
+-monitor unix:/tmp/test-monitor,server,nowait \
+-no-acpi \
+-boot c \
+-usb
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-host.xml
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-host.xml
new file mode 100644
index 0000000..445b358
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-host.xml
@@ -0,0 +1,22 @@
+<domain type='kvm'>
+ <name>aarch64test</name>
+ <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid>
+ <memory unit='KiB'>1048576</memory>
+ <currentMemory unit='KiB'>1048576</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='aarch64' machine='virt'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <features>
+ <gic version='host'/>
+ </features>
+ <cpu mode='host-passthrough'/>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-aarch64</emulator>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-invalid.xml
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-invalid.xml
new file mode 100644
index 0000000..1cf9ea8
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-invalid.xml
@@ -0,0 +1,22 @@
+<domain type='kvm'>
+ <name>aarch64test</name>
+ <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid>
+ <memory unit='KiB'>1048576</memory>
+ <currentMemory unit='KiB'>1048576</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='aarch64' machine='virt'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <features>
+ <gic version='none'/>
+ </features>
+ <cpu mode='host-passthrough'/>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-aarch64</emulator>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-none.args
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-none.args
new file mode 120000
index 0000000..3234039
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-none.args
@@ -0,0 +1 @@
+qemuxml2argv-aarch64-gic-v2.args
\ No newline at end of file
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-none.xml
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-none.xml
new file mode 100644
index 0000000..272d0c8
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-none.xml
@@ -0,0 +1,19 @@
+<domain type='kvm'>
+ <name>aarch64test</name>
+ <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid>
+ <memory unit='KiB'>1048576</memory>
+ <currentMemory unit='KiB'>1048576</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='aarch64' machine='virt'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <cpu mode='host-passthrough'/>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-aarch64</emulator>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-not-arm.xml
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-not-arm.xml
new file mode 100644
index 0000000..3b907bc
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-not-arm.xml
@@ -0,0 +1,22 @@
+<domain type='kvm'>
+ <name>aarch64test</name>
+ <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid>
+ <memory unit='KiB'>1048576</memory>
+ <currentMemory unit='KiB'>1048576</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='ppc64' machine='pseries'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <features>
+ <gic/>
+ </features>
+ <cpu mode='host-passthrough'/>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-ppc64</emulator>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-not-virt.xml
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-not-virt.xml
new file mode 100644
index 0000000..256664e
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-not-virt.xml
@@ -0,0 +1,22 @@
+<domain type='kvm'>
+ <name>aarch64test</name>
+ <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid>
+ <memory unit='KiB'>1048576</memory>
+ <currentMemory unit='KiB'>1048576</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='aarch64' machine='not-virt'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <features>
+ <gic/>
+ </features>
+ <cpu mode='host-passthrough'/>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-aarch64</emulator>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 2ad5f5d..3a402b1 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1673,14 +1673,41 @@ mymain(void)
DO_TEST("aarch64-cpu-passthrough", QEMU_CAPS_DEVICE,
QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DEVICE_VIRTIO_MMIO,
QEMU_CAPS_CPU_HOST, QEMU_CAPS_KVM);
+
+ DO_TEST("aarch64-gic-none", QEMU_CAPS_DEVICE,
+ QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_CPU_HOST,
+ QEMU_CAPS_MACH_VIRT_GIC_VERSION);
+ DO_TEST("aarch64-gic-none", QEMU_CAPS_DEVICE,
+ QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_CPU_HOST);
+ DO_TEST("aarch64-gic-default", QEMU_CAPS_DEVICE,
+ QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_CPU_HOST,
+ QEMU_CAPS_MACH_VIRT_GIC_VERSION);
+ DO_TEST("aarch64-gic-default", QEMU_CAPS_DEVICE,
+ QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_CPU_HOST);
DO_TEST("aarch64-gic-v2", QEMU_CAPS_DEVICE,
QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_CPU_HOST,
QEMU_CAPS_MACH_VIRT_GIC_VERSION);
+ DO_TEST("aarch64-gic-v2", QEMU_CAPS_DEVICE,
+ QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_CPU_HOST);
DO_TEST("aarch64-gic-v3", QEMU_CAPS_DEVICE,
QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_CPU_HOST,
QEMU_CAPS_MACH_VIRT_GIC_VERSION);
DO_TEST_FAILURE("aarch64-gic-v3", QEMU_CAPS_DEVICE,
QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_CPU_HOST);
+ DO_TEST("aarch64-gic-host", QEMU_CAPS_DEVICE,
+ QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_CPU_HOST,
+ QEMU_CAPS_MACH_VIRT_GIC_VERSION);
+ DO_TEST_FAILURE("aarch64-gic-host", QEMU_CAPS_DEVICE,
+ QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_CPU_HOST);
+ DO_TEST_PARSE_ERROR("aarch64-gic-invalid", QEMU_CAPS_DEVICE,
+ QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_CPU_HOST,
+ QEMU_CAPS_MACH_VIRT_GIC_VERSION);
+ DO_TEST_FAILURE("aarch64-gic-not-virt", QEMU_CAPS_DEVICE,
+ QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_CPU_HOST,
+ QEMU_CAPS_MACH_VIRT_GIC_VERSION);
+ DO_TEST_FAILURE("aarch64-gic-not-arm", QEMU_CAPS_DEVICE,
+ QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_CPU_HOST,
+ QEMU_CAPS_MACH_VIRT_GIC_VERSION);
driver.caps->host.cpu->arch = VIR_ARCH_AARCH64;
DO_TEST("aarch64-kvm-32-on-64", QEMU_CAPS_DEVICE,
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-gic-default.xml
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-gic-default.xml
new file mode 120000
index 0000000..80a01c2
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-gic-default.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/qemuxml2argv-aarch64-gic-v2.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-gic-none.xml
b/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-gic-none.xml
new file mode 120000
index 0000000..80a01c2
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-gic-none.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/qemuxml2argv-aarch64-gic-v2.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index bdf96a6..25002ef 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -604,8 +604,11 @@ mymain(void)
DO_TEST("smbios-multiple-type2");
DO_TEST("aarch64-aavmf-virtio-mmio");
+ DO_TEST_DIFFERENT("aarch64-gic-none");
+ DO_TEST_DIFFERENT("aarch64-gic-default");
DO_TEST("aarch64-gic-v2");
DO_TEST("aarch64-gic-v3");
+ DO_TEST("aarch64-gic-host");
DO_TEST("memory-hotplug");
DO_TEST("memory-hotplug-nonuma");
--
2.5.0
3:44 p.m.
New subject: [libvirt] [PATCH 7/7] tests: Add more GIC test cases
On 02/03/2016 03:26 PM, Andrea Bolognani wrote:
Test all kinds of scenarios, including guests asking for GIC but
failing to specify a version, guests specifying an invalid version
and guests trying to use GIC with non-virt or even non-ARM machines.
---
.../qemuxml2argv-aarch64-gic-default.args | 1 +
.../qemuxml2argv-aarch64-gic-default.xml | 22 ++++++++++++++++++
.../qemuxml2argv-aarch64-gic-host.args | 20 ++++++++++++++++
.../qemuxml2argv-aarch64-gic-host.xml | 22 ++++++++++++++++++
.../qemuxml2argv-aarch64-gic-invalid.xml | 22 ++++++++++++++++++
.../qemuxml2argv-aarch64-gic-none.args | 1 +
.../qemuxml2argv-aarch64-gic-none.xml | 19 +++++++++++++++
.../qemuxml2argv-aarch64-gic-not-arm.xml | 22 ++++++++++++++++++
.../qemuxml2argv-aarch64-gic-not-virt.xml | 22 ++++++++++++++++++
tests/qemuxml2argvtest.c | 27 ++++++++++++++++++++++
.../qemuxml2xmlout-aarch64-gic-default.xml | 1 +
.../qemuxml2xmlout-aarch64-gic-none.xml | 1 +
tests/qemuxml2xmltest.c | 3 +++
13 files changed, 183 insertions(+)
create mode 120000 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-default.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-default.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-host.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-host.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-invalid.xml
create mode 120000 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-none.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-none.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-not-arm.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-aarch64-gic-not-virt.xml
create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-gic-default.xml
create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-aarch64-gic-none.xml
Obviously adjusting the default impacts this patch...
John
12:28 a.m.
On 02/03/2016 03:25 PM, Andrea Bolognani wrote:
This series was really just supposed to enable guests to use the
special "host" GIC version, but I ended up changing a bunch of other
stuff and adding a whole lot of new test cases.
I've also made it so the GIC availability is always reflected in the
domain XML, the same way other implicit devices and features work.
The GIC-related definitions are in their own file: depending on
whether we end up probing for host GIC support ourselves or relying
on QEMU this might turn out to be a huge overkill :)
Cheers.
Andrea Bolognani (7):
gic: Introduce virGICVersion enumeration
schema: List allowed GIC versions
conf: Use virGICVersion enumeration in virDomainDef
qemu: Default to GIC v2
qemu: Always enable GIC on ARM virt machines
tests: Reorganize and simplify GIC test cases
tests: Add more GIC test cases
ACK series, but I had one question on the first patch. Will conflict with my
test reorg patches but that's okay :)
- Cole
3138
days inactive
3151
days old
24 comments
3 participants
participants (3)
-
Andrea Bolognani -
Cole Robinson -
John Ferlan