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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list