[libvirt] [PATCH v4] qemu: add hv_vapic and hv_spinlocks support

Add new CPU flags for HyperV: hv_vapic for virtual APIC support hv_spinlocks for spinlock support XML: <features> <hyperv> <vapic state='on'/> <spinlocks state='on'>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 --- v1: https://www.redhat.com/archives/libvir-list/2013-May/msg00173.html v2: https://www.redhat.com/archives/libvir-list/2013-May/msg00184.html uses virStringSplit instead of strsep added a missing break to virDomainDefParseXML v3: https://www.redhat.com/archives/libvir-list/2013-June/msg00228.html rebased to use VIR_STRDUP fixed a crash on parsing '-cpu qemu32,hv_' v4: Added state attribute to spinlocks. CPU command line parsing refactoring is already pushed. Changed version in docs from 1.0.6 to 1.0.7. Use 'value' instead of 'hv_tokens[1]' for readability. docs/formatdomain.html.in | 17 ++++++- docs/schemas/domaincommon.rng | 13 ++++++ src/conf/domain_conf.c | 62 ++++++++++++++++++++++++- src/conf/domain_conf.h | 3 ++ src/qemu/qemu_command.c | 48 ++++++++++++++++++- tests/qemuxml2argvdata/qemuxml2argv-hyperv.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-hyperv.xml | 2 + 7 files changed, 142 insertions(+), 5 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 755d084..525e4cd 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1147,6 +1147,8 @@ <privnet/> <hyperv> <relaxed state='on'/> + <vapic state='on'/> + <spinlocks state='on'>0xFFFF</spinlocks> </hyperv> </features> @@ -1197,14 +1199,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.7 (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.7 (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 1eb2f68..a5e69e2 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4015,6 +4015,19 @@ <ref name="hypervtristate"/> </element> </optional> + <optional> + <element name="vapic"> + <ref name="hypervtristate"/> + </element> + </optional> + <optional> + <element name="spinlocks"> + <ref name="hypervtristate"/> + <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 2373397..5618c4c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -149,7 +149,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", @@ -11076,6 +11078,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 " @@ -11096,6 +11099,48 @@ virDomainDefParseXML(xmlDocPtr xml, def->hyperv_features[feature] = value; break; + case VIR_DOMAIN_HYPERV_SPINLOCKS: + if (!(tmp = virXPathString("string(./@state)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, + _("missing 'state' attribute for " + "HyperV Enlightenment feature '%s'"), + nodes[i]->name); + goto error; + } + + if ((value = virDomainFeatureStateTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid value of state argument " + "for HyperV Enlightenment feature '%s'"), + nodes[i]->name); + goto error; + } + + VIR_FREE(tmp); + if (!(tmp = virXPathString("string(.)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing HyperV spinlock retry count")); + goto error; + } + + if (virStrToLong_ui(tmp, NULL, 0, + &def->hyperv_spinlocks) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot parse HyperV spinlock retry " + "count")); + goto error; + } + + if (def->hyperv_spinlocks < 0xFFF) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("HyperV spinlock retry count must be " + "at least 0xFFF")); + goto error; + } + VIR_FREE(tmp); + def->hyperv_features[VIR_DOMAIN_HYPERV_SPINLOCKS] = value; + break; + case VIR_DOMAIN_HYPERV_LAST: break; } @@ -16216,10 +16261,23 @@ 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])); + virDomainFeatureStateTypeToString( + def->hyperv_features[i])); + break; + + case VIR_DOMAIN_HYPERV_SPINLOCKS: + if (def->hyperv_features[i] == VIR_DOMAIN_FEATURE_STATE_ON) + virBufferAsprintf(buf, " <spinlocks state='on'>" + "0x%x</spinlocks>\n", + def->hyperv_spinlocks); + else if (def->hyperv_features[i]) + virBufferAsprintf(buf, " <spinlocks state='%s'/>", + virDomainFeatureStateTypeToString( + def->hyperv_features[i])); break; case VIR_DOMAIN_HYPERV_LAST: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 5b159ac..3817e37 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1584,6 +1584,8 @@ enum virDomainFeatureState { enum virDomainHyperv { VIR_DOMAIN_HYPERV_RELAXED = 0, + VIR_DOMAIN_HYPERV_VAPIC, + VIR_DOMAIN_HYPERV_SPINLOCKS, VIR_DOMAIN_HYPERV_LAST }; @@ -1926,6 +1928,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 486682e..c6cb833 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5786,11 +5786,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; } @@ -9627,6 +9634,7 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, { virCPUDefPtr cpu = NULL; char **tokens; + char **hv_tokens = NULL; char *model = NULL; int ret = -1; int i; @@ -9706,9 +9714,19 @@ 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 (*token == '\0') + goto syntax; + + if (!(hv_tokens = virStringSplit(token, "=", 2))) + goto cleanup; + + feature = hv_tokens[0]; + value = hv_tokens[1]; + if (*feature == '\0') goto syntax; @@ -9723,12 +9741,39 @@ 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, "%s", + _("missing HyperV spinlock retry count")); + goto cleanup; + } + + if (virStrToLong_ui(value, NULL, 0, &dom->hyperv_spinlocks) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("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); + hv_tokens = NULL; } } @@ -9756,6 +9801,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..13eec63 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 state='on'>0x2fff</spinlocks> </hyperv> </features> <clock offset='utc'/> -- 1.8.1.5

On Tue, Jun 18, 2013 at 16:59:30 +0200, Jano Tomko wrote:
Add new CPU flags for HyperV: hv_vapic for virtual APIC support hv_spinlocks for spinlock support
XML: <features> <hyperv> <vapic state='on'/> <spinlocks state='on'>0xFFFF</spinlocks> </hyperv> </features>
I think it's mostly fine but I'd make the spinlock feature look less magically: <spinlocks state='on' retries='4096'/> That is, move the retries count into an attribute so that is obvious what the number means and allow for any representation of the number rather than forcing hexadecimal one. After all, the parser would already allow for any base.
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 755d084..525e4cd 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1147,6 +1147,8 @@ <privnet/> <hyperv> <relaxed state='on'/> + <vapic state='on'/> + <spinlocks state='on'>0xFFFF</spinlocks>
Move the number to an attribute.
</hyperv>
</features> @@ -1197,14 +1199,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.7 (QEMU only)</span></td>
Likely 1.1.0 as we'll have ACL support.
+ </tr> + <tr> + <td>spinlocks</td> + <td>Enable spinlock support</td> + <td>hexadecimal number of retries, at least 0xFFF</td>
I'd say at least 4096 to make it more readable :)
+ <td><span class="since">1.0.7 (QEMU only)</span></td>
Again, 1.1.0.
</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 1eb2f68..a5e69e2 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4015,6 +4015,19 @@ <ref name="hypervtristate"/> </element> </optional> + <optional> + <element name="vapic"> + <ref name="hypervtristate"/> + </element> + </optional> + <optional> + <element name="spinlocks"> + <ref name="hypervtristate"/> + <data type="string"> + <param name="pattern">0x[0-9a-fA-F]{0,8}</param> + </data>
<data type="integer"/> would work better.
+ </element> + </optional> </interleave> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2373397..5618c4c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c ... @@ -11096,6 +11099,48 @@ virDomainDefParseXML(xmlDocPtr xml, def->hyperv_features[feature] = value; break;
+ case VIR_DOMAIN_HYPERV_SPINLOCKS: + if (!(tmp = virXPathString("string(./@state)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, + _("missing 'state' attribute for " + "HyperV Enlightenment feature '%s'"), + nodes[i]->name); + goto error; + } + + if ((value = virDomainFeatureStateTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid value of state argument " + "for HyperV Enlightenment feature '%s'"), + nodes[i]->name); + goto error; + } + + VIR_FREE(tmp); + if (!(tmp = virXPathString("string(.)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing HyperV spinlock retry count")); + goto error; + } + + if (virStrToLong_ui(tmp, NULL, 0, + &def->hyperv_spinlocks) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot parse HyperV spinlock retry " + "count")); + goto error; + } + + if (def->hyperv_spinlocks < 0xFFF) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("HyperV spinlock retry count must be " + "at least 0xFFF")); + goto error; + } + VIR_FREE(tmp); + def->hyperv_features[VIR_DOMAIN_HYPERV_SPINLOCKS] = value;
[feature] would be shorter :-)
+ break; + case VIR_DOMAIN_HYPERV_LAST: break; } @@ -16216,10 +16261,23 @@ 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])); + virDomainFeatureStateTypeToString( + def->hyperv_features[i])); + break; + + case VIR_DOMAIN_HYPERV_SPINLOCKS: + if (def->hyperv_features[i] == VIR_DOMAIN_FEATURE_STATE_ON) + virBufferAsprintf(buf, " <spinlocks state='on'>" + "0x%x</spinlocks>\n", + def->hyperv_spinlocks); + else if (def->hyperv_features[i]) + virBufferAsprintf(buf, " <spinlocks state='%s'/>", + virDomainFeatureStateTypeToString( + def->hyperv_features[i]));
I guess you could just make it more like the above features: if (def->hyperv_features[i]) { virBufferAsprintf(buf, " <spinlocks state='%s'", virDomainFeatureStateTypeToString(def->hyperv_features[i])); if (def->hyperv_features[i] == VIR_DOMAIN_FEATURE_STATE_ON) virBufferAsprintf(buf, " retries='%d'", def->hyperv_spinlocks); virBufferAddLit(buf, "/>\n"); }
break;
case VIR_DOMAIN_HYPERV_LAST:
...
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 486682e..c6cb833 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5786,11 +5786,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",
s/,",/, ",/
+ def->hyperv_spinlocks); + break; + case VIR_DOMAIN_HYPERV_LAST: break; }
Looks good otherwise. However, splitting the patch in two would be nice: the first one defining the XML and parsing and formatting it and the second one with qemu implementation and test. Jirka
participants (2)
-
Jiri Denemark
-
Ján Tomko