[libvirt] [PATCH 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 https://bugzilla.redhat.com/show_bug.cgi?id=784836 Ján Tomko (2): qemu: use strsep for 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 | 39 ++++++- src/conf/domain_conf.h | 3 + src/qemu/qemu_command.c | 145 +++++++++++++----------- tests/qemuxml2argvdata/qemuxml2argv-hyperv.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml | 2 + 7 files changed, 148 insertions(+), 72 deletions(-) -- 1.8.1.5
Copy the command line to allow modifying it, then get the tokens by strsep. This removes the need to strdup every feature separately, and simplifies the code. Change the error label to a cleanup label to prevent memory leaks. --- src/qemu/qemu_command.c | 110 ++++++++++++++++++------------------------------ 1 file changed, 41 insertions(+), 69 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 575dce1..3a27512 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9306,63 +9306,51 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, const char *val) { virCPUDefPtr cpu = NULL; - const char *p = val; - const char *next; + char *val_copy; + char *next; char *model = NULL; + char *token = NULL; + int ret = -1; - do { - if (*p == '\0' || *p == ',') - goto syntax; + if (!(val_copy = strdup(val))) + goto no_memory; - if ((next = strchr(p, ','))) - next++; + next = val_copy; - if (p == val) { - if (next) - model = strndup(p, next - p - 1); - else - model = strdup(p); + while ((token = strsep(&next, ","))) { + if (*token == '\0') + goto syntax; - if (!model) + if (token == val_copy) { + if (!(model = strdup(token))) 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 (*token == '+' || *token == '-') { + const char *feature = token + 1; /* '+' or '-' */ int policy; - int ret = 0; - if (*p == '+') + if (*token == '+') 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; for (i = 0; i < dom->clock.ntimers; i++) { - if (dom->clock.timers[i]->name == VIR_DOMAIN_TIMER_NAME_KVMCLOCK) { + if (dom->clock.timers[i]->name == VIR_DOMAIN_TIMER_NAME_KVMCLOCK) break; - } } if (i == dom->clock.ntimers) { @@ -9370,19 +9358,16 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, VIR_ALLOC(dom->clock.timers[i]) < 0) goto no_memory; dom->clock.timers[i]->name = VIR_DOMAIN_TIMER_NAME_KVMCLOCK; - dom->clock.timers[i]->present = -1; + dom->clock.timers[i]->present = present; dom->clock.timers[i]->tickpolicy = -1; dom->clock.timers[i]->track = -1; dom->clock.ntimers++; - } - - if (dom->clock.timers[i]->present != -1 && + } else if (dom->clock.timers[i]->present != -1 && dom->clock.timers[i]->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,41 +9376,29 @@ 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(token, "hv_")) { + const char *feature = token + 3; /* "hv_" */ int f; - p += 3; /* "hv_" */ - 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; - dom->features |= (1 << VIR_DOMAIN_FEATURE_HYPERV); if ((f = virDomainHypervTypeFromString(feature)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported HyperV Enlightenment feature " "'%s'"), feature); - goto error; + goto cleanup; } switch ((enum virDomainHyperv) f) { @@ -9436,21 +9409,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 +9427,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; + VIR_FREE(val_copy); + 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
On Thu, May 02, 2013 at 07:07:05PM +0200, Ján Tomko wrote:
Copy the command line to allow modifying it, then get the tokens by strsep. This removes the need to strdup every feature separately, and simplifies the code.
Please use the virStringSplit() function instead of strsep() Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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 | 39 ++++++++++++++++++++++++- src/conf/domain_conf.h | 3 ++ src/qemu/qemu_command.c | 37 ++++++++++++++++++++++- tests/qemuxml2argvdata/qemuxml2argv-hyperv.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml | 2 ++ 7 files changed, 108 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..63ded12 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,31 @@ 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; + case VIR_DOMAIN_HYPERV_LAST: break; } @@ -15705,12 +15733,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 3a27512..b0b1978 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; } @@ -9386,9 +9393,12 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, goto cleanup; } } else if (STRPREFIX(token, "hv_")) { - const char *feature = token + 3; /* "hv_" */ + char *value = token + 3; /* "hv_" */ + const char *feature; int f; + feature = strsep(&value, "="); + if (*feature == '\0') goto syntax; @@ -9403,7 +9413,32 @@ 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 (!value) { + 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: 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 (2)
-
Daniel P. Berrange -
Ján Tomko