[libvirt] [PATCH 0/7] Improve GIC support
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
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@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@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
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
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
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
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@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@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__ */
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
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>
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
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)); } } }
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
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;
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
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
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
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>
On Sun, 2016-02-07 at 09:42 -0500, John Ferlan 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
On 02/03/2016 03:26 PM, Andrea Bolognani wrote: 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
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
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
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
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
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
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
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
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
participants (3)
-
Andrea Bolognani -
Cole Robinson -
John Ferlan