Use VIR_ERR_XML_ERROR instead of VIR_ERR_INTERNAL_ERROR in XML parsing code and move "%s" formating strings right after the error code. --- src/conf/cpu_conf.c | 74 ++++++++++++++++++++++++++--------------------------- 1 file changed, 37 insertions(+), 37 deletions(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 23e7638..e313cad 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -1,7 +1,7 @@ /* - * cpu_conf.h: CPU XML handling + * cpu_conf.c: CPU XML handling * - * Copyright (C) 2009-2011 Red Hat, Inc. + * Copyright (C) 2009-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -187,8 +187,7 @@ virCPUDefParseXML(const xmlNodePtr node, char *vendor_id = NULL; if (!xmlStrEqual(node->name, BAD_CAST "cpu")) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", + virReportError(VIR_ERR_XML_ERROR, "%s", _("XML does not contain expected 'cpu' element")); return NULL; } @@ -222,7 +221,7 @@ virCPUDefParseXML(const xmlNodePtr node, def->mode = virCPUModeTypeFromString(cpuMode); if (def->mode < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, + virReportError(VIR_ERR_XML_ERROR, _("Invalid mode attribute '%s'"), cpuMode); VIR_FREE(cpuMode); @@ -250,7 +249,7 @@ virCPUDefParseXML(const xmlNodePtr node, VIR_FREE(match); if (def->match < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + virReportError(VIR_ERR_XML_ERROR, "%s", _("Invalid match attribute for CPU " "specification")); goto error; @@ -261,12 +260,12 @@ virCPUDefParseXML(const xmlNodePtr node, if (def->type == VIR_CPU_TYPE_HOST) { char *arch = virXPathString("string(./arch[1])", ctxt); if (!arch) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Missing CPU architecture")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing CPU architecture")); goto error; } if ((def->arch = virArchFromString(arch)) == VIR_ARCH_NONE) { - virReportError(VIR_ERR_INTERNAL_ERROR, + virReportError(VIR_ERR_XML_ERROR, _("Unknown architecture %s"), arch); VIR_FREE(arch); goto error; @@ -276,8 +275,8 @@ virCPUDefParseXML(const xmlNodePtr node, if (!(def->model = virXPathString("string(./model[1])", ctxt)) && def->type == VIR_CPU_TYPE_HOST) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Missing CPU model name")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing CPU model name")); goto error; } @@ -315,8 +314,8 @@ virCPUDefParseXML(const xmlNodePtr node, def->vendor = virXPathString("string(./vendor[1])", ctxt); if (def->vendor && !def->model) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("CPU vendor specified without CPU model")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("CPU vendor specified without CPU model")); goto error; } @@ -327,7 +326,7 @@ virCPUDefParseXML(const xmlNodePtr node, ret = virXPathULong("string(./topology[1]/@sockets)", ctxt, &ul); if (ret < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing 'sockets' attribute in CPU topology")); goto error; } @@ -336,7 +335,7 @@ virCPUDefParseXML(const xmlNodePtr node, ret = virXPathULong("string(./topology[1]/@cores)", ctxt, &ul); if (ret < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing 'cores' attribute in CPU topology")); goto error; } @@ -345,15 +344,15 @@ virCPUDefParseXML(const xmlNodePtr node, ret = virXPathULong("string(./topology[1]/@threads)", ctxt, &ul); if (ret < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing 'threads' attribute in CPU topology")); goto error; } def->threads = (unsigned int) ul; if (!def->sockets || !def->cores || !def->threads) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Invalid CPU topology")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid CPU topology")); goto error; } } @@ -364,7 +363,7 @@ virCPUDefParseXML(const xmlNodePtr node, if (n > 0) { if (!def->model) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + virReportError(VIR_ERR_XML_ERROR, "%s", _("Non-empty feature list specified without " "CPU model")); goto error; @@ -392,8 +391,8 @@ virCPUDefParseXML(const xmlNodePtr node, VIR_FREE(strpolicy); if (policy < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Invalid CPU feature policy")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid CPU feature policy")); goto error; } } else { @@ -402,14 +401,14 @@ virCPUDefParseXML(const xmlNodePtr node, if (!(name = virXMLPropString(nodes[i], "name")) || *name == 0) { VIR_FREE(name); - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Invalid CPU feature name")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid CPU feature name")); goto error; } for (j = 0; j < i; j++) { if (STREQ(name, def->features[j].name)) { - virReportError(VIR_ERR_INTERNAL_ERROR, + virReportError(VIR_ERR_XML_ERROR, _("CPU feature `%s' specified more than once"), name); VIR_FREE(name); @@ -425,8 +424,8 @@ virCPUDefParseXML(const xmlNodePtr node, VIR_FREE(nodes); n = virXPathNodeSet("./numa[1]/cell", ctxt, &nodes); if (n <= 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("NUMA topology defined without NUMA cells")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("NUMA topology defined without NUMA cells")); goto error; } @@ -443,8 +442,8 @@ virCPUDefParseXML(const xmlNodePtr node, def->cells[i].cellid = i; cpus = virXMLPropString(nodes[i], "cpus"); if (!cpus) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Missing 'cpus' attribute in NUMA cell")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing 'cpus' attribute in NUMA cell")); goto error; } def->cells[i].cpustr = cpus; @@ -457,15 +456,15 @@ virCPUDefParseXML(const xmlNodePtr node, memory = virXMLPropString(nodes[i], "memory"); if (!memory) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Missing 'memory' attribute in NUMA cell")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Missing 'memory' attribute in NUMA cell")); goto error; } ret = virStrToLong_ui(memory, NULL, 10, &def->cells[i].mem); if (ret == -1) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Invalid 'memory' attribute in NUMA cell")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid 'memory' attribute in NUMA cell")); VIR_FREE(memory); goto error; } @@ -576,8 +575,8 @@ virCPUDefFormatBuf(virBufferPtr buf, (def->mode == VIR_CPU_MODE_CUSTOM && def->model))); if (!def->model && def->nfeatures) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Non-empty feature list specified without CPU model")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Non-empty feature list specified without CPU model")); return -1; } @@ -620,8 +619,8 @@ virCPUDefFormatBuf(virBufferPtr buf, virCPUFeatureDefPtr feature = def->features + i; if (!feature->name) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Missing CPU feature name")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing CPU feature name")); return -1; } @@ -668,7 +667,8 @@ virCPUDefAddFeature(virCPUDefPtr def, for (i = 0; i < def->nfeatures; i++) { if (STREQ(name, def->features[i].name)) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("CPU feature `%s' specified more than once"), name); + _("CPU feature `%s' specified more than once"), + name); return -1; } } -- 1.8.3.2

This new function updates or adds a feature to a existing cpu model definition. This function will be helpful to allow tuning of "host-model" features in later patches. --- src/conf/cpu_conf.c | 38 ++++++++++++++++++++++++++++++-------- src/conf/cpu_conf.h | 5 +++++ src/libvirt_private.syms | 1 + 3 files changed, 36 insertions(+), 8 deletions(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index e313cad..40fed96 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -656,19 +656,28 @@ virCPUDefFormatBuf(virBufferPtr buf, return 0; } - -int -virCPUDefAddFeature(virCPUDefPtr def, - const char *name, - int policy) +static int +virCPUDefUpdateFeatureInternal(virCPUDefPtr def, + const char *name, + int policy, + bool update) { size_t i; + if (def->type == VIR_CPU_TYPE_HOST) + policy = -1; + for (i = 0; i < def->nfeatures; i++) { if (STREQ(name, def->features[i].name)) { + if (update) { + def->features[i].policy = policy; + return 0; + } + virReportError(VIR_ERR_INTERNAL_ERROR, _("CPU feature `%s' specified more than once"), name); + return -1; } } @@ -677,9 +686,6 @@ virCPUDefAddFeature(virCPUDefPtr def, def->nfeatures, 1) < 0) return -1; - if (def->type == VIR_CPU_TYPE_HOST) - policy = -1; - if (VIR_STRDUP(def->features[def->nfeatures].name, name) < 0) return -1; @@ -689,6 +695,22 @@ virCPUDefAddFeature(virCPUDefPtr def, return 0; } +int +virCPUDefUpdateFeature(virCPUDefPtr def, + const char *name, + int policy) +{ + return virCPUDefUpdateFeatureInternal(def, name, policy, true); +} + +int +virCPUDefAddFeature(virCPUDefPtr def, + const char *name, + int policy) +{ + return virCPUDefUpdateFeatureInternal(def, name, policy, false); +} + bool virCPUDefIsEqual(virCPUDefPtr src, virCPUDefPtr dst) diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index 23ea455..2d1854f 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -164,4 +164,9 @@ virCPUDefAddFeature(virCPUDefPtr cpu, const char *name, int policy); +int +virCPUDefUpdateFeature(virCPUDefPtr cpu, + const char *name, + int policy); + #endif /* __VIR_CPU_CONF_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0ab7632..3c27041 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -70,6 +70,7 @@ virCPUDefFormatBuf; virCPUDefFree; virCPUDefFreeModel; virCPUDefParseXML; +virCPUDefUpdateFeature; virCPUModeTypeToString; -- 1.8.3.2

--- src/conf/cpu_conf.c | 4 ++-- src/cpu/cpu_x86.c | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 40fed96..1e657ad 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -357,8 +357,7 @@ virCPUDefParseXML(const xmlNodePtr node, } } - n = virXPathNodeSet("./feature", ctxt, &nodes); - if (n < 0) + if ((n = virXPathNodeSet("./feature", ctxt, &nodes)) < 0) goto error; if (n > 0) { @@ -372,6 +371,7 @@ virCPUDefParseXML(const xmlNodePtr node, if (VIR_RESIZE_N(def->features, def->nfeatures_max, def->nfeatures, n) < 0) goto error; + def->nfeatures = n; } diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index acbdb1d..928a67b 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1725,9 +1725,9 @@ x86UpdateCustom(virCPUDefPtr guest, if (guest->match == VIR_CPU_MATCH_MINIMUM) { guest->match = VIR_CPU_MATCH_EXACT; - if (x86ModelSubtractCPU(host_model, guest, map) - || x86DataToCPUFeatures(guest, VIR_CPU_FEATURE_REQUIRE, - host_model->data, map)) + if (x86ModelSubtractCPU(host_model, guest, map) || + x86DataToCPUFeatures(guest, VIR_CPU_FEATURE_REQUIRE, + host_model->data, map)) goto cleanup; } -- 1.8.3.2

https://bugzilla.redhat.com/show_bug.cgi?id=799354 Until now, the "host-model" cpu mode couldn't be influenced. This patch allows to use the <feature> elements to either enable or disable specific CPU flags. This can be used to force flags that can be emulated even if the host CPU doesn't support them. --- docs/formatdomain.html.in | 14 ++++++++----- src/conf/cpu_conf.c | 50 +++++++++++++++++++++++------------------------ src/cpu/cpu_x86.c | 43 ++++++++++++++++++++++++++++++++++++---- 3 files changed, 73 insertions(+), 34 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 52a6353..d2dbcb8 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -901,11 +901,15 @@ copying host CPU definition from capabilities XML into domain XML. Since the CPU definition is copied just before starting a domain, exactly the same XML can be used on different hosts while still - providing the best guest CPU each host supports. Neither - <code>match</code> attribute nor any <code>feature</code> elements - can be used in this mode. Specifying CPU model is not supported - either, but <code>model</code>'s <code>fallback</code> attribute may - still be used. Libvirt does not model every aspect of each CPU so + providing the best guest CPU each host supports. The + <code>match</code> attribute can't be used in this mode. Specifying + CPU model is not supported either, but <code>model</code>'s + <code>fallback</code> attribute may still be used. Using the + <code>feature</code> element, specific flags may be enabled or + disabled specifically in addition to the host model. This may be + used to fine tune features that can be emulated. + <span class="since">(Since 1.1.1)</span>. + Libvirt does not model every aspect of each CPU so the guest CPU will not match the host CPU exactly. On the other hand, the ABI provided to the guest is reproducible. During migration, complete CPU model definition is transferred to the diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 1e657ad..d3fbeba 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -361,7 +361,7 @@ virCPUDefParseXML(const xmlNodePtr node, goto error; if (n > 0) { - if (!def->model) { + if (!def->model && def->mode != VIR_CPU_MODE_HOST_MODEL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Non-empty feature list specified without " "CPU model")); @@ -574,7 +574,9 @@ virCPUDefFormatBuf(virBufferPtr buf, (def->mode == VIR_CPU_MODE_HOST_MODEL || (def->mode == VIR_CPU_MODE_CUSTOM && def->model))); - if (!def->model && def->nfeatures) { + if (!def->model && + def->mode != VIR_CPU_MODE_HOST_MODEL && + def->nfeatures) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Non-empty feature list specified without CPU model")); return -1; @@ -614,32 +616,30 @@ virCPUDefFormatBuf(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } - if (formatModel) { - for (i = 0; i < def->nfeatures; i++) { - virCPUFeatureDefPtr feature = def->features + i; + for (i = 0; i < def->nfeatures; i++) { + virCPUFeatureDefPtr feature = def->features + i; - if (!feature->name) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing CPU feature name")); - return -1; - } + if (!feature->name) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing CPU feature name")); + return -1; + } - if (def->type == VIR_CPU_TYPE_GUEST) { - const char *policy; - - policy = virCPUFeaturePolicyTypeToString(feature->policy); - if (!policy) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected CPU feature policy %d"), - feature->policy); - return -1; - } - virBufferAsprintf(buf, "<feature policy='%s' name='%s'/>\n", - policy, feature->name); - } else { - virBufferAsprintf(buf, "<feature name='%s'/>\n", - feature->name); + if (def->type == VIR_CPU_TYPE_GUEST) { + const char *policy; + + policy = virCPUFeaturePolicyTypeToString(feature->policy); + if (!policy) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected CPU feature policy %d"), + feature->policy); + return -1; } + virBufferAsprintf(buf, "<feature policy='%s' name='%s'/>\n", + policy, feature->name); + } else { + virBufferAsprintf(buf, "<feature name='%s'/>\n", + feature->name); } } diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 928a67b..ec99106 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1739,6 +1739,41 @@ cleanup: return ret; } + +static int +x86UpdateHostModel(virCPUDefPtr guest, + const virCPUDefPtr host) +{ + virCPUDefPtr oldguest; + size_t i; + + guest->match = VIR_CPU_MATCH_EXACT; + + /* no updates are required */ + if (guest->nfeatures == 0) { + virCPUDefFreeModel(guest); + return virCPUDefCopyModel(guest, host, true); + } + + /* update the host model according to the desired configuration */ + if (!(oldguest = virCPUDefCopy(guest))) + return -1; + + virCPUDefFreeModel(guest); + if (virCPUDefCopyModel(guest, host, true) < 0) + return -1; + + for (i = 0; i < oldguest->nfeatures; i++) { + if (virCPUDefUpdateFeature(guest, + oldguest->features[i].name, + oldguest->features[i].policy) < 0) + return -1; + } + + return 0; +} + + static int x86Update(virCPUDefPtr guest, const virCPUDefPtr host) @@ -1748,11 +1783,11 @@ x86Update(virCPUDefPtr guest, return x86UpdateCustom(guest, host); case VIR_CPU_MODE_HOST_MODEL: + return x86UpdateHostModel(guest, host); + + case VIR_CPU_MODE_HOST_PASSTHROUGH: - if (guest->mode == VIR_CPU_MODE_HOST_MODEL) - guest->match = VIR_CPU_MATCH_EXACT; - else - guest->match = VIR_CPU_MATCH_MINIMUM; + guest->match = VIR_CPU_MATCH_MINIMUM; virCPUDefFreeModel(guest); return virCPUDefCopyModel(guest, host, true); -- 1.8.3.2

On 07/16/13 10:45, Jiri Denemark wrote: ... Right. I fixed this. And pushed the series. Thanks for the review. Peter