[libvirt] [PATCH v2 0/2] qemu: support new HyperV-related features
1/2 prevents memory leaks and hair loss in qemuParseCommandLineCPU 2/2 adds support for the new cpu flags v1: https://www.redhat.com/archives/libvir-list/2013-May/msg00173.html v2: uses virStringSplit instead of strsep added a missing break to virDomainDefParseXML Ján Tomko (2): qemu: simplify CPU command line parsing qemu: add hv_vapic and hv_spinlocks support docs/formatdomain.html.in | 17 ++- docs/schemas/domaincommon.rng | 12 ++ src/conf/domain_conf.c | 40 +++++- src/conf/domain_conf.h | 3 + src/qemu/qemu_command.c | 168 +++++++++++++----------- tests/qemuxml2argvdata/qemuxml2argv-hyperv.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml | 2 + 7 files changed, 164 insertions(+), 80 deletions(-) -- 1.8.1.5
Use virStringSplit. Change the 'error' label to 'cleanup' to prevent memory leaks on error. --- src/qemu/qemu_command.c | 128 +++++++++++++++++++----------------------------- 1 file changed, 51 insertions(+), 77 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 575dce1..7784bdd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9306,83 +9306,68 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, const char *val) { virCPUDefPtr cpu = NULL; - const char *p = val; - const char *next; + char **tokens; char *model = NULL; + int ret = -1; + int i; - do { - if (*p == '\0' || *p == ',') - goto syntax; + if (!(tokens = virStringSplit(val, ",", 0))) + goto cleanup; - if ((next = strchr(p, ','))) - next++; + if (tokens[0] == NULL) + goto syntax; - if (p == val) { - if (next) - model = strndup(p, next - p - 1); - else - model = strdup(p); + for (i = 0; tokens[i] != NULL; i++) { + if (*tokens[i] == '\0') + goto syntax; - if (!model) + if (i == 0) { + if (!(model = strdup(tokens[i]))) goto no_memory; if (!STREQ(model, "qemu32") && !STREQ(model, "qemu64")) { if (!(cpu = qemuInitGuestCPU(dom))) - goto error; + goto cleanup; cpu->model = model; model = NULL; } - } else if (*p == '+' || *p == '-') { - char *feature; + } else if (*tokens[i] == '+' || *tokens[i] == '-') { + const char *feature = tokens[i] + 1; /* '+' or '-' */ int policy; - int ret = 0; - if (*p == '+') + if (*tokens[i] == '+') policy = VIR_CPU_FEATURE_REQUIRE; else policy = VIR_CPU_FEATURE_DISABLE; - p++; - if (*p == '\0' || *p == ',') + if (*feature == '\0') goto syntax; - if (next) - feature = strndup(p, next - p - 1); - else - feature = strdup(p); - - if (!feature) - goto no_memory; - if (STREQ(feature, "kvmclock")) { bool present = (policy == VIR_CPU_FEATURE_REQUIRE); - int i; + int j; - for (i = 0; i < dom->clock.ntimers; i++) { - if (dom->clock.timers[i]->name == VIR_DOMAIN_TIMER_NAME_KVMCLOCK) { + for (j = 0; j < dom->clock.ntimers; j++) { + if (dom->clock.timers[j]->name == VIR_DOMAIN_TIMER_NAME_KVMCLOCK) break; - } } - if (i == dom->clock.ntimers) { - if (VIR_REALLOC_N(dom->clock.timers, i+1) < 0 || - VIR_ALLOC(dom->clock.timers[i]) < 0) + if (j == dom->clock.ntimers) { + if (VIR_REALLOC_N(dom->clock.timers, j + 1) < 0 || + VIR_ALLOC(dom->clock.timers[j]) < 0) goto no_memory; - dom->clock.timers[i]->name = VIR_DOMAIN_TIMER_NAME_KVMCLOCK; - dom->clock.timers[i]->present = -1; - dom->clock.timers[i]->tickpolicy = -1; - dom->clock.timers[i]->track = -1; + dom->clock.timers[j]->name = VIR_DOMAIN_TIMER_NAME_KVMCLOCK; + dom->clock.timers[j]->present = present; + dom->clock.timers[j]->tickpolicy = -1; + dom->clock.timers[j]->track = -1; dom->clock.ntimers++; - } - - if (dom->clock.timers[i]->present != -1 && - dom->clock.timers[i]->present != present) { + } else if (dom->clock.timers[j]->present != -1 && + dom->clock.timers[j]->present != present) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("conflicting occurrences of kvmclock feature")); - goto error; + goto cleanup; } - dom->clock.timers[i]->present = present; } else if (STREQ(feature, "kvm_pv_eoi")) { if (policy == VIR_CPU_FEATURE_REQUIRE) dom->apic_eoi = VIR_DOMAIN_FEATURE_STATE_ON; @@ -9391,33 +9376,23 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, } else { if (!cpu) { if (!(cpu = qemuInitGuestCPU(dom))) - goto error; + goto cleanup; cpu->model = model; model = NULL; } - ret = virCPUDefAddFeature(cpu, feature, policy); + if (virCPUDefAddFeature(cpu, feature, policy) < 0) + goto cleanup; } - - VIR_FREE(feature); - if (ret < 0) - goto error; - } else if (STRPREFIX(p, "hv_")) { - char *feature; + } else if (STRPREFIX(tokens[i], "hv_")) { + const char *feature = tokens[i] + 3; /* "hv_" */ int f; - p += 3; /* "hv_" */ - if (*p == '\0' || *p == ',') - goto syntax; - if (next) - feature = strndup(p, next - p - 1); - else - feature = strdup(p); - if (!feature) - goto no_memory; + if (*feature == '\0') + goto syntax; dom->features |= (1 << VIR_DOMAIN_FEATURE_HYPERV); @@ -9425,7 +9400,7 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported HyperV Enlightenment feature " "'%s'"), feature); - goto error; + goto cleanup; } switch ((enum virDomainHyperv) f) { @@ -9436,21 +9411,17 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, case VIR_DOMAIN_HYPERV_LAST: break; } - - VIR_FREE(feature); } - } while ((p = next)); + } if (dom->os.arch == VIR_ARCH_X86_64) { bool is_32bit = false; if (cpu) { union cpuData *cpuData = NULL; - int ret; - ret = cpuEncode(VIR_ARCH_X86_64, cpu, NULL, &cpuData, - NULL, NULL, NULL, NULL); - if (ret < 0) - goto error; + if (cpuEncode(VIR_ARCH_X86_64, cpu, NULL, &cpuData, + NULL, NULL, NULL, NULL) < 0) + goto cleanup; is_32bit = (cpuHasFeature(VIR_ARCH_X86_64, cpuData, "lm") != 1); cpuDataFree(VIR_ARCH_X86_64, cpuData); @@ -9458,22 +9429,25 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, is_32bit = STREQ(model, "qemu32"); } - if (is_32bit) { + if (is_32bit) dom->os.arch = VIR_ARCH_I686; - } } + + ret = 0; + +cleanup: VIR_FREE(model); - return 0; + virStringFreeList(tokens); + return ret; syntax: virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown CPU syntax '%s'"), val); - goto error; + goto cleanup; no_memory: virReportOOMError(); -error: - return -1; + goto cleanup; } -- 1.8.1.5
Add new CPU flags for HyperV: hv_vapic for virtual APIC support hv_spinlocks for spinlock support XML: <features> <hyperv> <vapic state='on'/> <spinlocks>0xFFFF</spinlocks> </hyperv> </features> results in the following QEMU command line: qemu -cpu <cpu_model>,hv_vapic,hv_spinlocks=0xffff https://bugzilla.redhat.com/show_bug.cgi?id=784836 --- docs/formatdomain.html.in | 17 +++++++++- docs/schemas/domaincommon.rng | 12 +++++++ src/conf/domain_conf.c | 40 ++++++++++++++++++++++- src/conf/domain_conf.h | 3 ++ src/qemu/qemu_command.c | 42 ++++++++++++++++++++++++- tests/qemuxml2argvdata/qemuxml2argv-hyperv.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml | 2 ++ 7 files changed, 114 insertions(+), 4 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8d4edfb..cac2080 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1083,6 +1083,8 @@ <privnet/> <hyperv> <relaxed state='on'/> + <vapic state='on'/> + <spinlocks>0xFFFF</spinlocks> </hyperv> </features> @@ -1133,14 +1135,27 @@ <th>Feature</th> <th>Description</th> <th>Value</th> + <th>Since</th> </tr> <tr> <td>relaxed</td> <td>Relax contstraints on timers</td> <td> on, off</td> + <td><span class="since">1.0.0 (QEMU only)</span></td> + </tr> + <tr> + <td>vapic</td> + <td>Enable virtual APIC</td> + <td>on, off</td> + <td><span class="since">1.0.6 (QEMU only)</span></td> + </tr> + <tr> + <td>spinlocks</td> + <td>Enable spinlock support</td> + <td>hexadecimal number of retries, at least 0xFFF</td> + <td><span class="since">1.0.6 (QEMU only)</span></td> </tr> </table> - <span class="since">Since 1.0.0 (QEMU only)</span> </dd> </dl> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 10596dc..1105d15 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3897,6 +3897,18 @@ <ref name="hypervtristate"/> </element> </optional> + <optional> + <element name="vapic"> + <ref name="hypervtristate"/> + </element> + </optional> + <optional> + <element name="spinlocks"> + <data type="string"> + <param name="pattern">0x[0-9a-fA-F]{0,8}</param> + </data> + </element> + </optional> </interleave> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fe97c02..d1d92b9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -148,7 +148,9 @@ VIR_ENUM_IMPL(virDomainFeatureState, VIR_DOMAIN_FEATURE_STATE_LAST, "off") VIR_ENUM_IMPL(virDomainHyperv, VIR_DOMAIN_HYPERV_LAST, - "relaxed") + "relaxed", + "vapic", + "spinlocks") VIR_ENUM_IMPL(virDomainLifecycle, VIR_DOMAIN_LIFECYCLE_LAST, "destroy", @@ -10628,6 +10630,7 @@ virDomainDefParseXML(xmlDocPtr xml, switch ((enum virDomainHyperv) feature) { case VIR_DOMAIN_HYPERV_RELAXED: + case VIR_DOMAIN_HYPERV_VAPIC: if (!(tmp = virXPathString("string(./@state)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, _("missing 'state' attribute for " @@ -10648,6 +10651,32 @@ virDomainDefParseXML(xmlDocPtr xml, def->hyperv_features[feature] = value; break; + case VIR_DOMAIN_HYPERV_SPINLOCKS: + if (!(tmp = virXPathString("string(.)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, + _("missing HyperV spinlock retry count")); + goto error; + } + + if (virStrToLong_ui(tmp, NULL, 0, + &def->hyperv_spinlocks) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Cannot parse HyperV spinlock retry " + "count")); + goto error; + } + + if (def->hyperv_spinlocks < 0xFFF) { + virReportError(VIR_ERR_XML_ERROR, + _("HyperV spinlock retry count must be " + "at least 0xFFF")); + goto error; + } + VIR_FREE(tmp); + def->hyperv_features[VIR_DOMAIN_HYPERV_SPINLOCKS] = + VIR_DOMAIN_FEATURE_STATE_ON; + break; + case VIR_DOMAIN_HYPERV_LAST: break; } @@ -15705,12 +15734,21 @@ virDomainDefFormatInternal(virDomainDefPtr def, for (i = 0; i < VIR_DOMAIN_HYPERV_LAST; i++) { switch ((enum virDomainHyperv) i) { case VIR_DOMAIN_HYPERV_RELAXED: + case VIR_DOMAIN_HYPERV_VAPIC: if (def->hyperv_features[i]) virBufferAsprintf(buf, " <%s state='%s'/>\n", virDomainHypervTypeToString(i), virDomainFeatureStateTypeToString(def->hyperv_features[i])); break; + case VIR_DOMAIN_HYPERV_SPINLOCKS: + if (def->hyperv_features[i]) + virBufferAsprintf(buf, " <%s>0x%x</%s>\n", + virDomainHypervTypeToString(i), + def->hyperv_spinlocks, + virDomainHypervTypeToString(i)); + break; + case VIR_DOMAIN_HYPERV_LAST: break; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 21f7ce2..c841c76 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1549,6 +1549,8 @@ enum virDomainFeatureState { enum virDomainHyperv { VIR_DOMAIN_HYPERV_RELAXED = 0, + VIR_DOMAIN_HYPERV_VAPIC, + VIR_DOMAIN_HYPERV_SPINLOCKS, VIR_DOMAIN_HYPERV_LAST }; @@ -1884,6 +1886,7 @@ struct _virDomainDef { int apic_eoi; /* These options are of type virDomainFeatureState */ int hyperv_features[VIR_DOMAIN_HYPERV_LAST]; + unsigned int hyperv_spinlocks; virDomainClockDef clock; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7784bdd..7a03cda 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5546,11 +5546,18 @@ qemuBuildCpuArgStr(const virQEMUDriverPtr driver, for (i = 0; i < VIR_DOMAIN_HYPERV_LAST; i++) { switch ((enum virDomainHyperv) i) { case VIR_DOMAIN_HYPERV_RELAXED: + case VIR_DOMAIN_HYPERV_VAPIC: if (def->hyperv_features[i] == VIR_DOMAIN_FEATURE_STATE_ON) virBufferAsprintf(&buf, ",hv_%s", virDomainHypervTypeToString(i)); break; + case VIR_DOMAIN_HYPERV_SPINLOCKS: + if (def->hyperv_features[i] == VIR_DOMAIN_FEATURE_STATE_ON) + virBufferAsprintf(&buf,",hv_spinlocks=0x%x", + def->hyperv_spinlocks); + break; + case VIR_DOMAIN_HYPERV_LAST: break; } @@ -9307,6 +9314,7 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, { virCPUDefPtr cpu = NULL; char **tokens; + char **hv_tokens = NULL; char *model = NULL; int ret = -1; int i; @@ -9386,10 +9394,15 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, goto cleanup; } } else if (STRPREFIX(tokens[i], "hv_")) { - const char *feature = tokens[i] + 3; /* "hv_" */ + const char *token = tokens[i] + 3; /* "hv_" */ + const char *feature, *value; int f; + if (!(hv_tokens = virStringSplit(token, "=", 2))) + goto cleanup; + feature = hv_tokens[0]; + value = hv_tokens[1]; if (*feature == '\0') goto syntax; @@ -9405,12 +9418,38 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, switch ((enum virDomainHyperv) f) { case VIR_DOMAIN_HYPERV_RELAXED: + case VIR_DOMAIN_HYPERV_VAPIC: + if (value) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("HyperV feature '%s' should not " + "have a value"), feature); + goto cleanup; + } + dom->hyperv_features[f] = VIR_DOMAIN_FEATURE_STATE_ON; + break; + + case VIR_DOMAIN_HYPERV_SPINLOCKS: dom->hyperv_features[f] = VIR_DOMAIN_FEATURE_STATE_ON; + if (!hv_tokens[1]) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("missing HyperV spinlock retry count")); + goto cleanup; + } + + if (virStrToLong_ui(value, NULL, 0, &dom->hyperv_spinlocks) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot parse HyperV spinlock retry count")); + goto cleanup; + } + + if (dom->hyperv_spinlocks < 0xFFF) + dom->hyperv_spinlocks = 0xFFF; break; case VIR_DOMAIN_HYPERV_LAST: break; } + virStringFreeList(hv_tokens); } } @@ -9438,6 +9477,7 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, cleanup: VIR_FREE(model); virStringFreeList(tokens); + virStringFreeList(hv_tokens); return ret; syntax: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hyperv.args b/tests/qemuxml2argvdata/qemuxml2argv-hyperv.args index fac4d5f..df6b207 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hyperv.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hyperv.args @@ -1,4 +1,4 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc \ --cpu qemu32,hv_relaxed -m 214 -smp 6 -nographic -monitor \ +-cpu qemu32,hv_relaxed,hv_vapic,hv_spinlocks=0x2fff -m 214 -smp 6 -nographic -monitor \ unix:/tmp/test-monitor,server,nowait -boot n -usb -net none -serial none \ -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml b/tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml index 0d5d0c7..9601645 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml @@ -12,6 +12,8 @@ <acpi/> <hyperv> <relaxed state='on'/> + <vapic state='on'/> + <spinlocks>0x2fff</spinlocks> </hyperv> </features> <clock offset='utc'/> -- 1.8.1.5
participants (1)
-
Ján Tomko