[libvirt] [PATCHv4 0/6] Hyper-v crash feature support

A new Hyper-V cpu feature 'hv_crash' was added to QEMU. The feature will become available in v2.5.0. What is changed in v4: * panic model attribute is added even if it wasn't specified by the user [5/6], tests updated. * configuration with multiple panic defices of same model is rejected [6/6] 1-4 commits have no changes. The previous version: https://www.redhat.com/archives/libvir-list/2015-November/msg00457.html Dmitry Andreev (6): conf: add 'model' attribute for panic device with values isa, pseries, hyperv qemu: add support for hv_crash feature as a panic device schema: allow multiple panic devices tests: add tests for panic models and multiple panic devices qemu: set panic device model if not specified by the user conf: reject multiple panic devices of same model docs/formatdomain.html.in | 19 ++- docs/schemas/domaincommon.rng | 13 ++- src/conf/domain_conf.c | 128 +++++++++++++-------- src/conf/domain_conf.h | 15 ++- src/qemu/qemu_command.c | 74 ++++++++++-- src/qemu/qemu_domain.c | 30 ++++- tests/qemuargv2xmltest.c | 1 + .../qemuxml2argv-hyperv-panic.args | 21 ++++ .../qemuxml2argvdata/qemuxml2argv-hyperv-panic.xml | 25 ++++ .../qemuxml2argv-panic-double.args | 21 ++++ .../qemuxml2argvdata/qemuxml2argv-panic-double.xml | 28 +++++ tests/qemuxml2argvdata/qemuxml2argv-panic-isa.xml | 31 +++++ .../qemuxml2argv-panic-no-address.xml | 2 +- .../qemuxml2argv-panic-pseries.xml | 30 +++++ .../qemuxml2argvdata/qemuxml2argv-pseries-disk.xml | 2 +- .../qemuxml2argv-pseries-nvram.xml | 2 +- tests/qemuxml2argvtest.c | 3 + tests/qemuxml2xmloutdata/qemuxml2xmlout-panic.xml | 31 +++++ .../qemuxml2xmlout-pseries-panic-missing.xml | 2 +- .../qemuxml2xmlout-pseries-panic-no-address.xml | 30 +++++ tests/qemuxml2xmltest.c | 8 +- 21 files changed, 446 insertions(+), 70 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hyperv-panic.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hyperv-panic.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-panic-double.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-panic-double.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-panic-isa.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-panic-pseries.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-panic.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-no-address.xml -- 1.8.3.1

Libvirt already has two types of panic devices - pvpanic and pSeries firmware. This patch introduces the 'model' attribute and a new type of panic device. 'isa' model is for ISA pvpanic device. 'pseries' model is a default value for pSeries guests. 'hyperv' model is the new type. It's used for Hyper-V crash. Schema and docs are updated for the new attribute. --- docs/formatdomain.html.in | 18 ++++++++++++++++-- docs/schemas/domaincommon.rng | 9 +++++++++ src/conf/domain_conf.c | 28 ++++++++++++++++++++++++++-- src/conf/domain_conf.h | 11 +++++++++++ 4 files changed, 62 insertions(+), 4 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c88b032..ef30624 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6152,19 +6152,33 @@ qemu-kvm -net nic,model=? /dev/null <pre> ... <devices> - <panic> + <panic model='isa'> <address type='isa' iobase='0x505'/> </panic> </devices> ... </pre> <dl> + <dt><code>model</code></dt> + <dd> + <p> + The optional <code>model</code> attribute specifies what type + of panic device is provided. The panic model used when this attribute + is missing depends on the hypervisor and guest arch. + </p> + <ul> + <li>'isa' — for ISA pvpanic device</li> + <li>'pseries' — default and valid only for pSeries guests.</li> + <li>'hyperv' — for Hyper-V crash CPU feature. + <span class="since">Since 2.5.0, QEMU and KVM only</span></li> + </ul> + </dd> <dt><code>address</code></dt> <dd> <p> address of panic. The default ioport is 0x505. Most users don't need to specify an address, and doing so is forbidden - altogether for pSeries guests. + altogether for pseries and hyperv models. </p> </dd> </dl> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f196177..d67b1bf 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -5359,6 +5359,15 @@ <define name="panic"> <element name="panic"> <optional> + <attribute name="model"> + <choice> + <value>isa</value> + <value>pseries</value> + <value>hyperv</value> + </choice> + </attribute> + </optional> + <optional> <ref name="address"/> </optional> </element> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index eb00444..8339280 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -525,6 +525,12 @@ VIR_ENUM_IMPL(virDomainWatchdogAction, VIR_DOMAIN_WATCHDOG_ACTION_LAST, "none", "inject-nmi") +VIR_ENUM_IMPL(virDomainPanicModel, VIR_DOMAIN_PANIC_MODEL_LAST, + "default", + "isa", + "pseries", + "hyperv") + VIR_ENUM_IMPL(virDomainVideo, VIR_DOMAIN_VIDEO_TYPE_LAST, "vga", "cirrus", @@ -10197,6 +10203,7 @@ static virDomainPanicDefPtr virDomainPanicDefParseXML(xmlNodePtr node) { virDomainPanicDefPtr panic; + char *model = NULL; if (VIR_ALLOC(panic) < 0) return NULL; @@ -10204,10 +10211,22 @@ virDomainPanicDefParseXML(xmlNodePtr node) if (virDomainDeviceInfoParseXML(node, NULL, &panic->info, 0) < 0) goto error; + model = virXMLPropString(node, "model"); + if (model != NULL && + (int)(panic->model = virDomainPanicModelTypeFromString(model)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown panic model '%s'"), model); + goto error; + } + + cleanup: + VIR_FREE(model); return panic; + error: virDomainPanicDefFree(panic); - return NULL; + panic = NULL; + goto cleanup; } /* Parse the XML definition for an input device */ @@ -20629,8 +20648,13 @@ static int virDomainPanicDefFormat(virBufferPtr buf, { virBuffer childrenBuf = VIR_BUFFER_INITIALIZER; int indent = virBufferGetIndent(buf, false); + const char *model = virDomainPanicModelTypeToString(def->model); + + if (def->model == VIR_DOMAIN_PANIC_MODEL_DEFAULT) + virBufferAddLit(buf, "<panic"); + else + virBufferAsprintf(buf, "<panic model='%s'", model); - virBufferAddLit(buf, "<panic"); virBufferAdjustIndent(&childrenBuf, indent + 2); if (virDomainDeviceInfoFormat(&childrenBuf, &def->info, 0) < 0) return -1; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f10b534..be18994 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2045,7 +2045,17 @@ struct _virDomainIdMapDef { }; +typedef enum { + VIR_DOMAIN_PANIC_MODEL_DEFAULT, + VIR_DOMAIN_PANIC_MODEL_ISA, + VIR_DOMAIN_PANIC_MODEL_PSERIES, + VIR_DOMAIN_PANIC_MODEL_HYPERV, + + VIR_DOMAIN_PANIC_MODEL_LAST +} virDomainPanicModel; + struct _virDomainPanicDef { + int model; virDomainDeviceInfo info; }; @@ -3060,6 +3070,7 @@ VIR_ENUM_DECL(virDomainMemballoonModel) VIR_ENUM_DECL(virDomainSmbiosMode) VIR_ENUM_DECL(virDomainWatchdogModel) VIR_ENUM_DECL(virDomainWatchdogAction) +VIR_ENUM_DECL(virDomainPanicModel) VIR_ENUM_DECL(virDomainVideo) VIR_ENUM_DECL(virDomainHostdevMode) VIR_ENUM_DECL(virDomainHostdevSubsys) -- 1.8.3.1

On Fri, Nov 13, 2015 at 20:16:35 +0300, Dmitry Andreev wrote:
Libvirt already has two types of panic devices - pvpanic and pSeries firmware. This patch introduces the 'model' attribute and a new type of panic device.
'isa' model is for ISA pvpanic device. 'pseries' model is a default value for pSeries guests. 'hyperv' model is the new type. It's used for Hyper-V crash.
Schema and docs are updated for the new attribute. --- docs/formatdomain.html.in | 18 ++++++++++++++++-- docs/schemas/domaincommon.rng | 9 +++++++++ src/conf/domain_conf.c | 28 ++++++++++++++++++++++++++-- src/conf/domain_conf.h | 11 +++++++++++ 4 files changed, 62 insertions(+), 4 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c88b032..ef30624 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6152,19 +6152,33 @@ qemu-kvm -net nic,model=? /dev/null <pre> ... <devices> - <panic> + <panic model='isa'> <address type='isa' iobase='0x505'/> </panic> </devices> ... </pre> <dl> + <dt><code>model</code></dt> + <dd> + <p> + The optional <code>model</code> attribute specifies what type + of panic device is provided. The panic model used when this attribute + is missing depends on the hypervisor and guest arch. + </p> + <ul> + <li>'isa' — for ISA pvpanic device</li> + <li>'pseries' — default and valid only for pSeries guests.</li> + <li>'hyperv' — for Hyper-V crash CPU feature. + <span class="since">Since 2.5.0, QEMU and KVM only</span></li>
"Since 1.3.0". It refers to the libvirt version which supports this 'hyperv' model rather than QEMU version.
+ </ul> + </dd> <dt><code>address</code></dt> <dd> <p> address of panic. The default ioport is 0x505. Most users don't need to specify an address, and doing so is forbidden - altogether for pSeries guests. + altogether for pseries and hyperv models. </p> </dd> </dl> ... diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index eb00444..8339280 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c ... @@ -10204,10 +10211,22 @@ virDomainPanicDefParseXML(xmlNodePtr node) if (virDomainDeviceInfoParseXML(node, NULL, &panic->info, 0) < 0) goto error;
+ model = virXMLPropString(node, "model"); + if (model != NULL && + (int)(panic->model = virDomainPanicModelTypeFromString(model)) < 0) {
panic->model is declared as int, there's no need to typecast it here.
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown panic model '%s'"), model); + goto error; + } + + cleanup: + VIR_FREE(model); return panic; + error: virDomainPanicDefFree(panic); - return NULL; + panic = NULL; + goto cleanup; }
/* Parse the XML definition for an input device */ @@ -20629,8 +20648,13 @@ static int virDomainPanicDefFormat(virBufferPtr buf, { virBuffer childrenBuf = VIR_BUFFER_INITIALIZER; int indent = virBufferGetIndent(buf, false); + const char *model = virDomainPanicModelTypeToString(def->model); + + if (def->model == VIR_DOMAIN_PANIC_MODEL_DEFAULT) + virBufferAddLit(buf, "<panic"); + else + virBufferAsprintf(buf, "<panic model='%s'", model);
- virBufferAddLit(buf, "<panic");
I think the following code would be better (esp. in case we need to add another attribute in the future): virBufferAddLit(buf, "<panic"); if (def->model) virBufferAsprintf(buf, " model='%s'", virDomainPanicModelTypeToString(def->model));
virBufferAdjustIndent(&childrenBuf, indent + 2); if (virDomainDeviceInfoFormat(&childrenBuf, &def->info, 0) < 0) return -1; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f10b534..be18994 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2045,7 +2045,17 @@ struct _virDomainIdMapDef { };
+typedef enum { + VIR_DOMAIN_PANIC_MODEL_DEFAULT, + VIR_DOMAIN_PANIC_MODEL_ISA, + VIR_DOMAIN_PANIC_MODEL_PSERIES, + VIR_DOMAIN_PANIC_MODEL_HYPERV, + + VIR_DOMAIN_PANIC_MODEL_LAST +} virDomainPanicModel; + struct _virDomainPanicDef { + int model;
Add /* virDomainPanicModel */ comment at the end of this line.
virDomainDeviceInfo info; };
... Jirka

Panic device type used depends on 'model' attribute. If no model is specified then device type depends on hypervisor and guest arch. 'pseries' model is used for pSeries guest and 'isa' model is used in other cases. XML: <devices> <panic model='hyperv'/> </devices> QEMU command line: qemu -cpu <cpu_model>,hv_crash --- src/qemu/qemu_command.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 59 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 792ada3..2d0cec0 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7623,6 +7623,16 @@ qemuBuildCpuArgStr(virQEMUDriverPtr driver, } } + if (def->panic && + def->panic->model == VIR_DOMAIN_PANIC_MODEL_HYPERV) { + if (!have_cpu) { + virBufferAdd(&buf, default_model, -1); + have_cpu = true; + } + + virBufferAddLit(&buf, ",hv_crash"); + } + if (def->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON) { if (!have_cpu) { virBufferAdd(&buf, default_model, -1); @@ -11150,17 +11160,49 @@ qemuBuildCommandLine(virConnectPtr conn, } if (def->panic) { - if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { - /* For pSeries guests, the firmware provides the same - * functionality as the pvpanic device. The address + virDomainPanicModel model = def->panic->model; + + if (model == VIR_DOMAIN_PANIC_MODEL_DEFAULT) { + if (ARCH_IS_PPC64(def->os.arch) && + STRPREFIX(def->os.machine, "pseries")) + model = VIR_DOMAIN_PANIC_MODEL_PSERIES; + else + model = VIR_DOMAIN_PANIC_MODEL_ISA; + } + + switch (model) { + case VIR_DOMAIN_PANIC_MODEL_HYPERV: + /* Panic with model 'hyperv' is not a device, it should + * be configured in cpu commandline. The address * cannot be configured by the user */ if (def->panic->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("setting the panic device address is not " - "supported for pSeries guests")); + "supported for model 'hyperv'")); goto error; } - } else { + break; + + case VIR_DOMAIN_PANIC_MODEL_PSERIES: + if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { + /* For pSeries guests, the firmware provides the same + * functionality as the pvpanic device. The address + * cannot be configured by the user */ + if (def->panic->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("setting the panic device address is not " + "supported for model 'pseries'")); + goto error; + } + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only pSeries guests support panic device " + "with model 'pseries'")); + goto error; + } + break; + + case VIR_DOMAIN_PANIC_MODEL_ISA: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PANIC)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the QEMU binary does not support the " @@ -11185,6 +11227,11 @@ qemuBuildCommandLine(virConnectPtr conn, "with ISA address type")); goto error; } + + /* default model value was changed before switch */ + case VIR_DOMAIN_PANIC_MODEL_DEFAULT: + case VIR_DOMAIN_PANIC_MODEL_LAST: + break; } } @@ -12534,6 +12581,13 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, if (virCPUDefAddFeature(cpu, feature, policy) < 0) goto cleanup; } + } else if (STREQ(tokens[i], "hv_crash")) { + virDomainPanicDefPtr panic; + if (VIR_ALLOC(panic) < 0) + goto cleanup; + + panic->model = VIR_DOMAIN_PANIC_MODEL_HYPERV; + dom->panic = panic; } else if (STRPREFIX(tokens[i], "hv_")) { const char *token = tokens[i] + 3; /* "hv_" */ const char *feature, *value; -- 1.8.3.1

On Fri, Nov 13, 2015 at 20:16:36 +0300, Dmitry Andreev wrote:
Panic device type used depends on 'model' attribute.
If no model is specified then device type depends on hypervisor and guest arch. 'pseries' model is used for pSeries guest and 'isa' model is used in other cases.
XML: <devices> <panic model='hyperv'/> </devices>
QEMU command line: qemu -cpu <cpu_model>,hv_crash --- src/qemu/qemu_command.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 59 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 792ada3..2d0cec0 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c ... @@ -11150,17 +11160,49 @@ qemuBuildCommandLine(virConnectPtr conn, }
if (def->panic) { - if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { - /* For pSeries guests, the firmware provides the same - * functionality as the pvpanic device. The address + virDomainPanicModel model = def->panic->model; + + if (model == VIR_DOMAIN_PANIC_MODEL_DEFAULT) { + if (ARCH_IS_PPC64(def->os.arch) && + STRPREFIX(def->os.machine, "pseries")) + model = VIR_DOMAIN_PANIC_MODEL_PSERIES; + else + model = VIR_DOMAIN_PANIC_MODEL_ISA; + }
This code should go in the post parse callback.
+ + switch (model) { + case VIR_DOMAIN_PANIC_MODEL_HYPERV: + /* Panic with model 'hyperv' is not a device, it should + * be configured in cpu commandline. The address * cannot be configured by the user */ if (def->panic->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("setting the panic device address is not " - "supported for pSeries guests")); + "supported for model 'hyperv'")); goto error; }
Should we also reject hyperv model if !ARCH_IS_X86 similarly to what we do for pseries model? Another big question is what happens if you try <panic model='hyperv'/> with QEMU < 2.5.0? Will it fail (that would be OK) or does QEMU just ignore the unknown CPU feature and starts anyway (not OK)?
- } else { + break; + + case VIR_DOMAIN_PANIC_MODEL_PSERIES: + if (ARCH_IS_PPC64(def->os.arch) && STRPREFIX(def->os.machine, "pseries")) { + /* For pSeries guests, the firmware provides the same + * functionality as the pvpanic device. The address + * cannot be configured by the user */ + if (def->panic->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("setting the panic device address is not " + "supported for model 'pseries'")); + goto error; + } + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only pSeries guests support panic device " + "with model 'pseries'")); + goto error; + } + break; + + case VIR_DOMAIN_PANIC_MODEL_ISA: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PANIC)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("the QEMU binary does not support the " ...
Jirka

'model' attribute was added to a panic device but only one panic device is allowed. This patch changes panic device presence from 'optional' to 'zeroOrMore'. --- docs/formatdomain.html.in | 1 + docs/schemas/domaincommon.rng | 4 +-- src/conf/domain_conf.c | 73 ++++++++++++++++++------------------------- src/conf/domain_conf.h | 4 ++- src/qemu/qemu_command.c | 50 ++++++++++++++++++----------- src/qemu/qemu_domain.c | 21 ++++++++++--- 6 files changed, 83 insertions(+), 70 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ef30624..7a14473 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6152,6 +6152,7 @@ qemu-kvm -net nic,model=? /dev/null <pre> ... <devices> + <panic model='hyperv'/> <panic model='isa'> <address type='isa' iobase='0x505'/> </panic> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index d67b1bf..8965976 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4044,9 +4044,9 @@ <optional> <ref name="nvram"/> </optional> - <optional> + <zeroOrMore> <ref name="panic"/> - </optional> + </zeroOrMore> </interleave> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8339280..2f17675 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2538,7 +2538,9 @@ void virDomainDefFree(virDomainDefPtr def) virDomainTPMDefFree(def->tpm); - virDomainPanicDefFree(def->panic); + for (i = 0; i < def->npanics; i++) + virDomainPanicDefFree(def->panics[i]); + VIR_FREE(def->panics); VIR_FREE(def->idmap.uidmap); VIR_FREE(def->idmap.gidmap); @@ -3617,10 +3619,10 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, if (cb(def, &device, &def->tpm->info, opaque) < 0) return -1; } - if (def->panic) { - device.type = VIR_DOMAIN_DEVICE_PANIC; - device.data.panic = def->panic; - if (cb(def, &device, &def->panic->info, opaque) < 0) + device.type = VIR_DOMAIN_DEVICE_PANIC; + for (i = 0; i < def->npanics; i++) { + device.data.panic = def->panics[i]; + if (cb(def, &device, &def->panics[i]->info, opaque) < 0) return -1; } @@ -16407,23 +16409,20 @@ virDomainDefParseXML(xmlDocPtr xml, VIR_FREE(nodes); /* analysis of the panic devices */ - def->panic = NULL; if ((n = virXPathNodeSet("./devices/panic", ctxt, &nodes)) < 0) goto error; - if (n > 1) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("only a single panic device is supported")); + if (n && VIR_ALLOC_N(def->panics, n) < 0) goto error; - } - if (n > 0) { + for (i = 0; i < n; i++) { virDomainPanicDefPtr panic = - virDomainPanicDefParseXML(nodes[0]); + virDomainPanicDefParseXML(nodes[i]); + if (!panic) goto error; - def->panic = panic; - VIR_FREE(nodes); + def->panics[def->npanics++] = panic; } + VIR_FREE(nodes); /* analysis of the shmem devices */ if ((n = virXPathNodeSet("./devices/shmem", ctxt, &nodes)) < 0) @@ -17633,17 +17632,14 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, } static bool -virDomainPanicCheckABIStability(virDomainPanicDefPtr src, +virDomainPanicDefCheckABIStability(virDomainPanicDefPtr src, virDomainPanicDefPtr dst) { - if (!src && !dst) - return true; - - if (!src || !dst) { + if (src->model != dst->model) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain panic device count '%d' " - "does not match source count '%d'"), - src ? 1 : 0, dst ? 1 : 0); + _("Target panic model '%s' does not match source '%s'"), + virDomainPanicModelTypeToString(dst->model), + virDomainPanicModelTypeToString(src->model)); return false; } @@ -17709,13 +17705,6 @@ virDomainTPMDefCheckABIStability(virDomainTPMDefPtr src, } static bool -virDomainPanicDefCheckABIStability(virDomainPanicDefPtr src, - virDomainPanicDefPtr dst) -{ - return virDomainDeviceInfoCheckABIStability(&src->info, &dst->info); -} - -static bool virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src, virDomainMemoryDefPtr dst) { @@ -18132,8 +18121,16 @@ virDomainDefCheckABIStability(virDomainDefPtr src, if (!virDomainRNGDefCheckABIStability(src->rngs[i], dst->rngs[i])) goto error; - if (!virDomainPanicCheckABIStability(src->panic, dst->panic)) + if (src->npanics != dst->npanics) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target domain panic device count %zu " + "does not match source %zu"), dst->npanics, src->npanics); goto error; + } + + for (i = 0; i < src->npanics; i++) + if (!virDomainPanicDefCheckABIStability(src->panics[i], dst->panics[i])) + goto error; if (src->nshmems != dst->nshmems) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -18157,16 +18154,6 @@ virDomainDefCheckABIStability(virDomainDefPtr src, goto error; } - if (src->panic && dst->panic) { - if (!virDomainPanicDefCheckABIStability(src->panic, dst->panic)) - goto error; - } else if (src->panic || dst->panic) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Either both target and source domains or none of " - "them must have PANIC device present")); - goto error; - } - if (src->nmems != dst->nmems) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain memory device count %zu " @@ -22460,9 +22447,9 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (def->nvram) virDomainNVRAMDefFormat(buf, def->nvram, flags); - if (def->panic && - virDomainPanicDefFormat(buf, def->panic) < 0) - goto error; + for (n = 0; n < def->npanics; n++) + if (virDomainPanicDefFormat(buf, def->panics[n]) < 0) + goto error; for (n = 0; n < def->nshmems; n++) if (virDomainShmemDefFormat(buf, def->shmems[n], flags) < 0) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index be18994..43f99ac 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2316,6 +2316,9 @@ struct _virDomainDef { size_t nmems; virDomainMemoryDefPtr *mems; + size_t npanics; + virDomainPanicDefPtr *panics; + /* Only 1 */ virDomainWatchdogDefPtr watchdog; virDomainMemballoonDefPtr memballoon; @@ -2324,7 +2327,6 @@ struct _virDomainDef { virCPUDefPtr cpu; virSysinfoDefPtr sysinfo; virDomainRedirFilterDefPtr redirfilter; - virDomainPanicDefPtr panic; void *namespaceData; virDomainXMLNamespace ns; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2d0cec0..965b68e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7623,14 +7623,16 @@ qemuBuildCpuArgStr(virQEMUDriverPtr driver, } } - if (def->panic && - def->panic->model == VIR_DOMAIN_PANIC_MODEL_HYPERV) { - if (!have_cpu) { - virBufferAdd(&buf, default_model, -1); - have_cpu = true; - } + for (i = 0; i < def->npanics; i++) { + if (def->panics[i]->model == VIR_DOMAIN_PANIC_MODEL_HYPERV) { + if (!have_cpu) { + virBufferAdd(&buf, default_model, -1); + have_cpu = true; + } - virBufferAddLit(&buf, ",hv_crash"); + virBufferAddLit(&buf, ",hv_crash"); + break; + } } if (def->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON) { @@ -11159,8 +11161,8 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } - if (def->panic) { - virDomainPanicModel model = def->panic->model; + for (i = 0; i < def->npanics; i++) { + virDomainPanicModel model = def->panics[i]->model; if (model == VIR_DOMAIN_PANIC_MODEL_DEFAULT) { if (ARCH_IS_PPC64(def->os.arch) && @@ -11175,7 +11177,7 @@ qemuBuildCommandLine(virConnectPtr conn, /* Panic with model 'hyperv' is not a device, it should * be configured in cpu commandline. The address * cannot be configured by the user */ - if (def->panic->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (def->panics[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("setting the panic device address is not " "supported for model 'hyperv'")); @@ -11188,7 +11190,7 @@ qemuBuildCommandLine(virConnectPtr conn, /* For pSeries guests, the firmware provides the same * functionality as the pvpanic device. The address * cannot be configured by the user */ - if (def->panic->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (def->panics[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("setting the panic device address is not " "supported for model 'pseries'")); @@ -11210,11 +11212,11 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } - switch (def->panic->info.type) { + switch (def->panics[i]->info.type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA: virCommandAddArg(cmd, "-device"); virCommandAddArgFormat(cmd, "pvpanic,ioport=%d", - def->panic->info.addr.isa.iobase); + def->panics[i]->info.addr.isa.iobase); break; case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE: @@ -12582,12 +12584,22 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, goto cleanup; } } else if (STREQ(tokens[i], "hv_crash")) { - virDomainPanicDefPtr panic; - if (VIR_ALLOC(panic) < 0) - goto cleanup; - - panic->model = VIR_DOMAIN_PANIC_MODEL_HYPERV; - dom->panic = panic; + size_t j; + for (j = 0; j < dom->npanics; j++) { + if (dom->panics[j]->model == VIR_DOMAIN_PANIC_MODEL_HYPERV) + break; + } + + if (j == dom->npanics) { + virDomainPanicDefPtr panic; + if (VIR_ALLOC(panic) < 0 || + VIR_APPEND_ELEMENT_COPY(dom->panics, + dom->npanics, panic) < 0) { + VIR_FREE(panic); + goto cleanup; + } + panic->model = VIR_DOMAIN_PANIC_MODEL_HYPERV; + } } else if (STRPREFIX(tokens[i], "hv_")) { const char *token = tokens[i] + 3; /* "hv_" */ const char *feature, *value; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3d2b7a0..19ae5f7 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1175,12 +1175,23 @@ qemuDomainDefPostParse(virDomainDefPtr def, VIR_DOMAIN_INPUT_BUS_USB) < 0) goto cleanup; - if (addPanicDevice && !def->panic) { - virDomainPanicDefPtr panic; - if (VIR_ALLOC(panic) < 0) - goto cleanup; + if (addPanicDevice) { + size_t j; + for (j = 0; j < def->npanics; j++) { + if (def->panics[j]->model == VIR_DOMAIN_PANIC_MODEL_DEFAULT || + def->panics[j]->model == VIR_DOMAIN_PANIC_MODEL_PSERIES) + break; + } - def->panic = panic; + if (j == def->npanics) { + virDomainPanicDefPtr panic; + if (VIR_ALLOC(panic) < 0 || + VIR_APPEND_ELEMENT_COPY(def->panics, + def->npanics, panic) < 0) { + VIR_FREE(panic); + goto cleanup; + } + } } ret = 0; -- 1.8.3.1

On Fri, Nov 13, 2015 at 20:16:37 +0300, Dmitry Andreev wrote:
'model' attribute was added to a panic device but only one panic device is allowed. This patch changes panic device presence from 'optional' to 'zeroOrMore'.
I'd remove the "schema: " prefix from the subject since this patch touches more than just schema.
--- docs/formatdomain.html.in | 1 + docs/schemas/domaincommon.rng | 4 +-- src/conf/domain_conf.c | 73 ++++++++++++++++++------------------------- src/conf/domain_conf.h | 4 ++- src/qemu/qemu_command.c | 50 ++++++++++++++++++----------- src/qemu/qemu_domain.c | 21 ++++++++++--- 6 files changed, 83 insertions(+), 70 deletions(-)
...
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8339280..2f17675 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c ... @@ -16407,23 +16409,20 @@ virDomainDefParseXML(xmlDocPtr xml, VIR_FREE(nodes);
/* analysis of the panic devices */ - def->panic = NULL; if ((n = virXPathNodeSet("./devices/panic", ctxt, &nodes)) < 0) goto error; - if (n > 1) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("only a single panic device is supported")); + if (n && VIR_ALLOC_N(def->panics, n) < 0) goto error; - } - if (n > 0) { + for (i = 0; i < n; i++) { virDomainPanicDefPtr panic = - virDomainPanicDefParseXML(nodes[0]); + virDomainPanicDefParseXML(nodes[i]); +
Extra empty line.
if (!panic) goto error;
- def->panic = panic; - VIR_FREE(nodes); + def->panics[def->npanics++] = panic; } + VIR_FREE(nodes);
/* analysis of the shmem devices */ if ((n = virXPathNodeSet("./devices/shmem", ctxt, &nodes)) < 0)
...
@@ -18157,16 +18154,6 @@ virDomainDefCheckABIStability(virDomainDefPtr src, goto error; }
- if (src->panic && dst->panic) { - if (!virDomainPanicDefCheckABIStability(src->panic, dst->panic)) - goto error; - } else if (src->panic || dst->panic) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Either both target and source domains or none of " - "them must have PANIC device present")); - goto error; - } - if (src->nmems != dst->nmems) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain memory device count %zu "
Ouch, our code for checking ABI stability of panic devices was rather disgusting. Your patch seems to make it better, but it would be cool if you could refactor the code in a separate patch (which would be the first one in this series), add support for panic->model in the same patch which introduces it elsewhere, and update the check to work with multiple panic devices in this patch. ...
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3d2b7a0..19ae5f7 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1175,12 +1175,23 @@ qemuDomainDefPostParse(virDomainDefPtr def, VIR_DOMAIN_INPUT_BUS_USB) < 0) goto cleanup;
- if (addPanicDevice && !def->panic) { - virDomainPanicDefPtr panic; - if (VIR_ALLOC(panic) < 0) - goto cleanup; + if (addPanicDevice) { + size_t j; + for (j = 0; j < def->npanics; j++) { + if (def->panics[j]->model == VIR_DOMAIN_PANIC_MODEL_DEFAULT || + def->panics[j]->model == VIR_DOMAIN_PANIC_MODEL_PSERIES) + break; + }
- def->panic = panic; + if (j == def->npanics) { + virDomainPanicDefPtr panic;
Wrong indentation.
+ if (VIR_ALLOC(panic) < 0 || + VIR_APPEND_ELEMENT_COPY(def->panics, + def->npanics, panic) < 0) { + VIR_FREE(panic); + goto cleanup; + } + } }
ret = 0;
Jirka

--- tests/qemuargv2xmltest.c | 1 + .../qemuxml2argv-hyperv-panic.args | 21 +++++++++++++++ .../qemuxml2argvdata/qemuxml2argv-hyperv-panic.xml | 25 +++++++++++++++++ .../qemuxml2argv-panic-double.args | 21 +++++++++++++++ .../qemuxml2argvdata/qemuxml2argv-panic-double.xml | 28 +++++++++++++++++++ tests/qemuxml2argvdata/qemuxml2argv-panic-isa.xml | 31 ++++++++++++++++++++++ .../qemuxml2argv-panic-pseries.xml | 30 +++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 +++ tests/qemuxml2xmltest.c | 4 +++ 9 files changed, 164 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hyperv-panic.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hyperv-panic.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-panic-double.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-panic-double.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-panic-isa.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-panic-pseries.xml diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 34f3e5c..7759a09 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -261,6 +261,7 @@ mymain(void) DO_TEST("smp"); DO_TEST("hyperv"); + DO_TEST("hyperv-panic"); DO_TEST("kvm-features"); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hyperv-panic.args b/tests/qemuxml2argvdata/qemuxml2argv-hyperv-panic.args new file mode 100644 index 0000000..a9f13e0 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hyperv-panic.args @@ -0,0 +1,21 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-cpu qemu32,hv_crash \ +-m 214 \ +-smp 6 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-monitor unix:/tmp/test-monitor,server,nowait \ +-boot n \ +-usb \ +-net none \ +-serial none \ +-parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hyperv-panic.xml b/tests/qemuxml2argvdata/qemuxml2argv-hyperv-panic.xml new file mode 100644 index 0000000..9f0edbb --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hyperv-panic.xml @@ -0,0 +1,25 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>6</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='network'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='none'/> + <panic model='hyperv'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-panic-double.args b/tests/qemuxml2argvdata/qemuxml2argv-panic-double.args new file mode 100644 index 0000000..0d3a385 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-panic-double.args @@ -0,0 +1,21 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-cpu qemu32,hv_crash \ +-m 214 \ +-smp 6 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefconfig \ +-nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait \ +-boot n \ +-usb \ +-device pvpanic,ioport=1285 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-panic-double.xml b/tests/qemuxml2argvdata/qemuxml2argv-panic-double.xml new file mode 100644 index 0000000..63009d9 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-panic-double.xml @@ -0,0 +1,28 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>6</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='network'/> + </os> + <features> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='none'/> + <panic model='hyperv'/> + <panic> + <address type='isa' iobase='0x505'/> + </panic> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-panic-isa.xml b/tests/qemuxml2argvdata/qemuxml2argv-panic-isa.xml new file mode 100644 index 0000000..b9595a8 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-panic-isa.xml @@ -0,0 +1,31 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='fdc' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + <panic model='isa'> + <address type='isa' iobase='0x505'/> + </panic> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-panic-pseries.xml b/tests/qemuxml2argvdata/qemuxml2argv-panic-pseries.xml new file mode 100644 index 0000000..8fcd644 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-panic-pseries.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + <boot dev='hd'/> + </os> + <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> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target port='0'/> + <address type='spapr-vio'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + <address type='spapr-vio'/> + </console> + <memballoon model='none'/> + <panic model='pseries'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 1c52828..b82ba95 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -686,6 +686,7 @@ mymain(void) DO_TEST("hyperv", NONE); DO_TEST("hyperv-off", NONE); + DO_TEST("hyperv-panic", NONE); DO_TEST("kvm-features", NONE); DO_TEST("kvm-features-off", NONE); @@ -1626,6 +1627,8 @@ mymain(void) DO_TEST("panic", QEMU_CAPS_DEVICE_PANIC, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); + DO_TEST("panic-double", QEMU_CAPS_DEVICE_PANIC, + QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); DO_TEST("panic-no-address", QEMU_CAPS_DEVICE_PANIC, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index cbd4d0d..ea75aff 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -375,6 +375,7 @@ mymain(void) DO_TEST("hyperv"); DO_TEST("hyperv-off"); + DO_TEST("hyperv-panic"); DO_TEST("kvm-features"); DO_TEST("kvm-features-off"); @@ -591,6 +592,9 @@ mymain(void) DO_TEST("pcihole64-q35"); DO_TEST("panic"); + DO_TEST("panic-isa"); + DO_TEST("panic-pseries"); + DO_TEST("panic-double"); DO_TEST("panic-no-address"); DO_TEST_DIFFERENT("disk-backing-chains"); -- 1.8.3.1

Choose 'ise' or 'pseries' model for panic device 'default' model value. Fixed tests and add two new outputs for xml-2-xml tests. Set value --- src/qemu/qemu_command.c | 14 ++-------- src/qemu/qemu_domain.c | 9 +++++++ .../qemuxml2argvdata/qemuxml2argv-panic-double.xml | 2 +- .../qemuxml2argv-panic-no-address.xml | 2 +- .../qemuxml2argvdata/qemuxml2argv-pseries-disk.xml | 2 +- .../qemuxml2argv-pseries-nvram.xml | 2 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-panic.xml | 31 ++++++++++++++++++++++ .../qemuxml2xmlout-pseries-panic-missing.xml | 2 +- .../qemuxml2xmlout-pseries-panic-no-address.xml | 30 +++++++++++++++++++++ tests/qemuxml2xmltest.c | 4 +-- 10 files changed, 79 insertions(+), 19 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-panic.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-no-address.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 965b68e..2a44c25 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -11162,17 +11162,7 @@ qemuBuildCommandLine(virConnectPtr conn, } for (i = 0; i < def->npanics; i++) { - virDomainPanicModel model = def->panics[i]->model; - - if (model == VIR_DOMAIN_PANIC_MODEL_DEFAULT) { - if (ARCH_IS_PPC64(def->os.arch) && - STRPREFIX(def->os.machine, "pseries")) - model = VIR_DOMAIN_PANIC_MODEL_PSERIES; - else - model = VIR_DOMAIN_PANIC_MODEL_ISA; - } - - switch (model) { + switch ((virDomainPanicModel) def->panics[i]->model) { case VIR_DOMAIN_PANIC_MODEL_HYPERV: /* Panic with model 'hyperv' is not a device, it should * be configured in cpu commandline. The address @@ -11230,7 +11220,7 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } - /* default model value was changed before switch */ + /* default model value was changed is PostParse */ case VIR_DOMAIN_PANIC_MODEL_DEFAULT: case VIR_DOMAIN_PANIC_MODEL_LAST: break; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 19ae5f7..27f9146 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1366,6 +1366,15 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, goto cleanup; } + if (dev->type == VIR_DOMAIN_DEVICE_PANIC && + dev->data.panic->model == VIR_DOMAIN_PANIC_MODEL_DEFAULT) { + if (ARCH_IS_PPC64(def->os.arch) && + STRPREFIX(def->os.machine, "pseries")) + dev->data.panic->model = VIR_DOMAIN_PANIC_MODEL_PSERIES; + else + dev->data.panic->model = VIR_DOMAIN_PANIC_MODEL_ISA; + } + ret = 0; cleanup: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-panic-double.xml b/tests/qemuxml2argvdata/qemuxml2argv-panic-double.xml index 63009d9..aadb758 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-panic-double.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-panic-double.xml @@ -21,7 +21,7 @@ <controller type='pci' index='0' model='pci-root'/> <memballoon model='none'/> <panic model='hyperv'/> - <panic> + <panic model='isa'> <address type='isa' iobase='0x505'/> </panic> </devices> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-panic-no-address.xml b/tests/qemuxml2argvdata/qemuxml2argv-panic-no-address.xml index 79f8a1e..f3f3fbb 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-panic-no-address.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-panic-no-address.xml @@ -24,6 +24,6 @@ <controller type='ide' index='0'/> <controller type='pci' index='0' model='pci-root'/> <memballoon model='virtio'/> - <panic/> + <panic model='isa'/> </devices> </domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml index 3a96209..39f4a1f 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-disk.xml @@ -37,6 +37,6 @@ <model type='cirrus' vram='16384' heads='1'/> </video> <memballoon model='none'/> - <panic/> + <panic model='pseries'/> </devices> </domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml index 619186a..2da2832 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-nvram.xml @@ -20,6 +20,6 @@ <nvram> <address type='spapr-vio' reg='0x4000'/> </nvram> - <panic/> + <panic model='pseries'/> </devices> </domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-panic.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-panic.xml new file mode 100644 index 0000000..b9595a8 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-panic.xml @@ -0,0 +1,31 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='fdc' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + <panic model='isa'> + <address type='isa' iobase='0x505'/> + </panic> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml index 9312975..8fcd644 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-missing.xml @@ -25,6 +25,6 @@ <address type='spapr-vio'/> </console> <memballoon model='none'/> - <panic/> + <panic model='pseries'/> </devices> </domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-no-address.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-no-address.xml new file mode 100644 index 0000000..8fcd644 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-no-address.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + <boot dev='hd'/> + </os> + <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> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <serial type='pty'> + <target port='0'/> + <address type='spapr-vio'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + <address type='spapr-vio'/> + </console> + <memballoon model='none'/> + <panic model='pseries'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index ea75aff..fd58331 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -535,7 +535,7 @@ mymain(void) DO_TEST("pseries-nvram"); DO_TEST_DIFFERENT("pseries-panic-missing"); - DO_TEST("pseries-panic-no-address"); + DO_TEST_DIFFERENT("pseries-panic-no-address"); /* These tests generate different XML */ DO_TEST_DIFFERENT("balloon-device-auto"); @@ -591,7 +591,7 @@ mymain(void) DO_TEST("pcihole64-none"); DO_TEST("pcihole64-q35"); - DO_TEST("panic"); + DO_TEST_DIFFERENT("panic"); DO_TEST("panic-isa"); DO_TEST("panic-pseries"); DO_TEST("panic-double"); -- 1.8.3.1

On Fri, Nov 13, 2015 at 20:16:39 +0300, Dmitry Andreev wrote:
Choose 'ise' or 'pseries' model for panic device 'default' model
s/ise/isa/
value.
Fixed tests and add two new outputs for xml-2-xml tests.
Set value
Did you want to say something more here? Anyway, most of this patch should either go before 2/6 qemu: add support for hv_crash feature as a panic device or it can alternatively be squashed into it.
--- src/qemu/qemu_command.c | 14 ++-------- src/qemu/qemu_domain.c | 9 +++++++ .../qemuxml2argvdata/qemuxml2argv-panic-double.xml | 2 +- .../qemuxml2argv-panic-no-address.xml | 2 +- .../qemuxml2argvdata/qemuxml2argv-pseries-disk.xml | 2 +- .../qemuxml2argv-pseries-nvram.xml | 2 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-panic.xml | 31 ++++++++++++++++++++++ .../qemuxml2xmlout-pseries-panic-missing.xml | 2 +- .../qemuxml2xmlout-pseries-panic-no-address.xml | 30 +++++++++++++++++++++ tests/qemuxml2xmltest.c | 4 +-- 10 files changed, 79 insertions(+), 19 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-panic.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-panic-no-address.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 965b68e..2a44c25 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -11162,17 +11162,7 @@ qemuBuildCommandLine(virConnectPtr conn, }
for (i = 0; i < def->npanics; i++) { - virDomainPanicModel model = def->panics[i]->model; - - if (model == VIR_DOMAIN_PANIC_MODEL_DEFAULT) { - if (ARCH_IS_PPC64(def->os.arch) && - STRPREFIX(def->os.machine, "pseries")) - model = VIR_DOMAIN_PANIC_MODEL_PSERIES; - else - model = VIR_DOMAIN_PANIC_MODEL_ISA; - } - - switch (model) { + switch ((virDomainPanicModel) def->panics[i]->model) { case VIR_DOMAIN_PANIC_MODEL_HYPERV: /* Panic with model 'hyperv' is not a device, it should * be configured in cpu commandline. The address @@ -11230,7 +11220,7 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; }
- /* default model value was changed before switch */ + /* default model value was changed is PostParse */
s/is/in/
case VIR_DOMAIN_PANIC_MODEL_DEFAULT: case VIR_DOMAIN_PANIC_MODEL_LAST: break;
... Jirka

Only one panic device per model is allowed. --- src/conf/domain_conf.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2f17675..b4a46ad 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3747,6 +3747,30 @@ virDomainDefRejectDuplicateControllers(virDomainDefPtr def) return ret; } +static int +virDomainDefRejectDuplicatePanics(virDomainDefPtr def) +{ + bool exists[VIR_DOMAIN_PANIC_MODEL_LAST]; + size_t i; + + for (i = 0; i < VIR_DOMAIN_PANIC_MODEL_LAST; i++) + exists[i] = false; + + for (i = 0; i < def->npanics; i++) { + virDomainPanicModel model = def->panics[i]->model; + if (!exists[model]) { + exists[model] = true; + } else { + virReportError(VIR_ERR_XML_ERROR, + _("Multiple panic devices with model '%s'"), + virDomainPanicModelTypeToString(model)); + return -1; + } + + } + + return 0; +} /** * virDomainDefMetadataSanitize: @@ -3976,6 +4000,9 @@ virDomainDefPostParseInternal(virDomainDefPtr def, if (virDomainDefRejectDuplicateControllers(def) < 0) return -1; + if (virDomainDefRejectDuplicatePanics(def) < 0) + return -1; + /* verify settings of guest timers */ for (i = 0; i < def->clock.ntimers; i++) { virDomainTimerDefPtr timer = def->clock.timers[i]; -- 1.8.3.1

On Fri, Nov 13, 2015 at 20:16:40 +0300, Dmitry Andreev wrote:
Only one panic device per model is allowed. --- src/conf/domain_conf.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2f17675..b4a46ad 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3747,6 +3747,30 @@ virDomainDefRejectDuplicateControllers(virDomainDefPtr def) return ret; }
+static int +virDomainDefRejectDuplicatePanics(virDomainDefPtr def) +{ + bool exists[VIR_DOMAIN_PANIC_MODEL_LAST]; + size_t i; + + for (i = 0; i < VIR_DOMAIN_PANIC_MODEL_LAST; i++) + exists[i] = false; + + for (i = 0; i < def->npanics; i++) { + virDomainPanicModel model = def->panics[i]->model; + if (!exists[model]) { + exists[model] = true;
Wrong indentation.
+ } else { + virReportError(VIR_ERR_XML_ERROR, + _("Multiple panic devices with model '%s'"), + virDomainPanicModelTypeToString(model)); + return -1; + }
But I think avoiding negative test would make the code a bit more readable: if (exists[model]) { virReportError(...); return -1; } exists[model] = true;
+
Extra empty line.
+ } + + return 0; +}
/** * virDomainDefMetadataSanitize: @@ -3976,6 +4000,9 @@ virDomainDefPostParseInternal(virDomainDefPtr def, if (virDomainDefRejectDuplicateControllers(def) < 0) return -1;
+ if (virDomainDefRejectDuplicatePanics(def) < 0) + return -1; + /* verify settings of guest timers */ for (i = 0; i < def->clock.ntimers; i++) { virDomainTimerDefPtr timer = def->clock.timers[i];
Jirka

On Fri, Nov 13, 2015 at 20:16:34 +0300, Dmitry Andreev wrote:
A new Hyper-V cpu feature 'hv_crash' was added to QEMU. The feature will become available in v2.5.0.
What is changed in v4: * panic model attribute is added even if it wasn't specified by the user [5/6], tests updated. * configuration with multiple panic defices of same model is rejected [6/6]
1-4 commits have no changes.
Thanks for the updated patches. Overall looking at the cumulated diff of this series, it is pretty good. Just it's sometimes split weirdly into individual patches. See my replies to individual patches for a few comments (I did not reply to 4/6). Jirka
participants (2)
-
Dmitry Andreev
-
Jiri Denemark