[libvirt] [PATCH] Added the attribute vendor_id to the cpu model
Introducing the attribute vendor_id to force the CPUID instruction in a kvm guest to return the specified vendor. --- docs/schemas/domaincommon.rng | 7 +++++ src/conf/cpu_conf.c | 61 ++++++++++++++++++++++++++++++++--------- src/conf/cpu_conf.h | 3 ++ src/qemu/qemu_command.c | 6 +++- tests/testutilsqemu.c | 1 + 5 files changed, 64 insertions(+), 14 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 46e539d..a246e8b 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2820,6 +2820,13 @@ </choice> </attribute> </optional> + <optional> + <attribute name="vendor_id"> + <data type="string"> + <param name='pattern'>.{12}</param> + </data> + </attribute> + </optional> <choice> <text/> <empty/> diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index b520f7c..b8ca7a5 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -22,6 +22,7 @@ */ #include <config.h> +#include <ctype.h> #include "virterror_internal.h" #include "memory.h" @@ -68,6 +69,7 @@ virCPUDefFreeModel(virCPUDefPtr def) VIR_FREE(def->model); VIR_FREE(def->vendor); + VIR_FREE(def->vendor_id); for (i = 0; i < def->nfeatures; i++) VIR_FREE(def->features[i].name); @@ -104,6 +106,7 @@ virCPUDefCopyModel(virCPUDefPtr dst, if ((src->model && !(dst->model = strdup(src->model))) || (src->vendor && !(dst->vendor = strdup(src->vendor))) + || (src->vendor_id && !(dst->vendor_id = strdup(src->vendor_id))) || VIR_ALLOC_N(dst->features, src->nfeatures) < 0) goto no_memory; dst->nfeatures_max = dst->nfeatures = src->nfeatures; @@ -288,19 +291,42 @@ virCPUDefParseXML(const xmlNodePtr node, } if (def->type == VIR_CPU_TYPE_GUEST && - def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH && - virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) { - const char *fallback; - - fallback = virXPathString("string(./model[1]/@fallback)", ctxt); - if (fallback) { - def->fallback = virCPUFallbackTypeFromString(fallback); - VIR_FREE(fallback); - if (def->fallback < 0) { - virCPUReportError(VIR_ERR_XML_ERROR, "%s", - _("Invalid fallback attribute")); - goto error; - } + def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { + + if(virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) { + const char *fallback; + + fallback = virXPathString("string(./model[1]/@fallback)", ctxt); + if (fallback) { + def->fallback = virCPUFallbackTypeFromString(fallback); + VIR_FREE(fallback); + if (def->fallback < 0) { + virCPUReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid fallback attribute")); + goto error; + } + } + + if(virXPathBoolean("boolean(./model[1]/@vendor_id)", ctxt)) { + char *vendor_id; + + vendor_id = virXPathString("string(./model[1]/@vendor_id)", ctxt); + if(!vendor_id || strlen(vendor_id)!=VIR_CPU_VENDOR_ID_LENGTH) { + virCPUReportError(VIR_ERR_XML_ERROR, "%s", + _("vendor id must be 12 characters long")); + VIR_FREE(vendor_id); + goto error; + } + for(i=0; i<strlen(vendor_id); i++) { + if(!isprint(vendor_id[i]) || isspace(vendor_id[i])) { + virCPUReportError(VIR_ERR_XML_ERROR, "%s", + _("vendor id is invalid")); + VIR_FREE(vendor_id); + goto error; + } + } + def->vendor_id = vendor_id; + } } } @@ -588,6 +614,8 @@ virCPUDefFormatBuf(virBufferPtr buf, return -1; } virBufferAsprintf(buf, " fallback='%s'", fallback); + if(def->vendor_id) + virBufferAsprintf(buf, " vendor_id='%s'", def->vendor_id); } if (formatModel && def->model) { virBufferAsprintf(buf, ">%s</model>\n", def->model); @@ -738,6 +766,13 @@ virCPUDefIsEqual(virCPUDefPtr src, goto cleanup; } + if (STRNEQ_NULLABLE(src->vendor_id, dst->vendor_id)) { + virCPUReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target CPU model %s does not match source %s"), + NULLSTR(dst->vendor_id), NULLSTR(src->vendor_id)); + goto cleanup; + } + if (src->sockets != dst->sockets) { virCPUReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target CPU sockets %d does not match source %d"), diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index f8b7bf9..3daadbd 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -28,6 +28,8 @@ # include "buf.h" # include "xml.h" +#define VIR_CPU_VENDOR_ID_LENGTH 12 + enum virCPUType { VIR_CPU_TYPE_HOST, VIR_CPU_TYPE_GUEST, @@ -103,6 +105,7 @@ struct _virCPUDef { int match; /* enum virCPUMatch */ char *arch; char *model; + char *vendor_id; /* vendor id returned by CPUID in the guest */ int fallback; /* enum virCPUFallback */ char *vendor; unsigned int sockets; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bd4f96a..56ecefb 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3910,7 +3910,9 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver, } virBufferAddLit(&buf, "host"); } else { - if (VIR_ALLOC(guest) < 0 || !(guest->arch = strdup(host->arch))) + if (VIR_ALLOC(guest) < 0 + || !(guest->arch = strdup(host->arch)) + || (cpu->vendor_id && !(guest->vendor_id = strdup(cpu->vendor_id)))) goto no_memory; if (cpu->match == VIR_CPU_MATCH_MINIMUM) @@ -3924,6 +3926,8 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver, goto cleanup; virBufferAdd(&buf, guest->model, -1); + if(guest->vendor_id) + virBufferAsprintf(&buf, ",vendor=%s", guest->vendor_id); for (i = 0; i < guest->nfeatures; i++) { char sign; if (guest->features[i].policy == VIR_CPU_FEATURE_DISABLE) diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 8d5a3bf..8b7cb33 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -116,6 +116,7 @@ virCapsPtr testQemuCapsInit(void) { 0, /* match */ (char *) "x86_64", /* arch */ (char *) "core2duo", /* model */ + NULL, /* vendor_id */ 0, /* fallback */ (char *) "Intel", /* vendor */ 1, /* sockets */ -- 1.7.9.5
Hi, last week I posted a patch to fake the vendor_id of the guest cpu. I'm writing an application which depends on this feature so I'm wondering if anyone has had a look at it? Best regards Hendrik On 21.06.2012 16:58, Hendrik Schwartke wrote:
Introducing the attribute vendor_id to force the CPUID instruction in a kvm guest to return the specified vendor. --- docs/schemas/domaincommon.rng | 7 +++++ src/conf/cpu_conf.c | 61 ++++++++++++++++++++++++++++++++--------- src/conf/cpu_conf.h | 3 ++ src/qemu/qemu_command.c | 6 +++- tests/testutilsqemu.c | 1 + 5 files changed, 64 insertions(+), 14 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 46e539d..a246e8b 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2820,6 +2820,13 @@ </choice> </attribute> </optional> +<optional> +<attribute name="vendor_id"> +<data type="string"> +<param name='pattern'>.{12}</param> +</data> +</attribute> +</optional> <choice> <text/> <empty/> diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index b520f7c..b8ca7a5 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -22,6 +22,7 @@ */
#include<config.h> +#include<ctype.h>
#include "virterror_internal.h" #include "memory.h" @@ -68,6 +69,7 @@ virCPUDefFreeModel(virCPUDefPtr def)
VIR_FREE(def->model); VIR_FREE(def->vendor); + VIR_FREE(def->vendor_id);
for (i = 0; i< def->nfeatures; i++) VIR_FREE(def->features[i].name); @@ -104,6 +106,7 @@ virCPUDefCopyModel(virCPUDefPtr dst,
if ((src->model&& !(dst->model = strdup(src->model))) || (src->vendor&& !(dst->vendor = strdup(src->vendor))) + || (src->vendor_id&& !(dst->vendor_id = strdup(src->vendor_id))) || VIR_ALLOC_N(dst->features, src->nfeatures)< 0) goto no_memory; dst->nfeatures_max = dst->nfeatures = src->nfeatures; @@ -288,19 +291,42 @@ virCPUDefParseXML(const xmlNodePtr node, }
if (def->type == VIR_CPU_TYPE_GUEST&& - def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH&& - virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) { - const char *fallback; - - fallback = virXPathString("string(./model[1]/@fallback)", ctxt); - if (fallback) { - def->fallback = virCPUFallbackTypeFromString(fallback); - VIR_FREE(fallback); - if (def->fallback< 0) { - virCPUReportError(VIR_ERR_XML_ERROR, "%s", - _("Invalid fallback attribute")); - goto error; - } + def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { + + if(virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) { + const char *fallback; + + fallback = virXPathString("string(./model[1]/@fallback)", ctxt); + if (fallback) { + def->fallback = virCPUFallbackTypeFromString(fallback); + VIR_FREE(fallback); + if (def->fallback< 0) { + virCPUReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid fallback attribute")); + goto error; + } + } + + if(virXPathBoolean("boolean(./model[1]/@vendor_id)", ctxt)) { + char *vendor_id; + + vendor_id = virXPathString("string(./model[1]/@vendor_id)", ctxt); + if(!vendor_id || strlen(vendor_id)!=VIR_CPU_VENDOR_ID_LENGTH) { + virCPUReportError(VIR_ERR_XML_ERROR, "%s", + _("vendor id must be 12 characters long")); + VIR_FREE(vendor_id); + goto error; + } + for(i=0; i<strlen(vendor_id); i++) { + if(!isprint(vendor_id[i]) || isspace(vendor_id[i])) { + virCPUReportError(VIR_ERR_XML_ERROR, "%s", + _("vendor id is invalid")); + VIR_FREE(vendor_id); + goto error; + } + } + def->vendor_id = vendor_id; + } } }
@@ -588,6 +614,8 @@ virCPUDefFormatBuf(virBufferPtr buf, return -1; } virBufferAsprintf(buf, " fallback='%s'", fallback); + if(def->vendor_id) + virBufferAsprintf(buf, " vendor_id='%s'", def->vendor_id); } if (formatModel&& def->model) { virBufferAsprintf(buf, ">%s</model>\n", def->model); @@ -738,6 +766,13 @@ virCPUDefIsEqual(virCPUDefPtr src, goto cleanup; }
+ if (STRNEQ_NULLABLE(src->vendor_id, dst->vendor_id)) { + virCPUReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target CPU model %s does not match source %s"), + NULLSTR(dst->vendor_id), NULLSTR(src->vendor_id)); + goto cleanup; + } + if (src->sockets != dst->sockets) { virCPUReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target CPU sockets %d does not match source %d"), diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index f8b7bf9..3daadbd 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -28,6 +28,8 @@ # include "buf.h" # include "xml.h"
+#define VIR_CPU_VENDOR_ID_LENGTH 12 + enum virCPUType { VIR_CPU_TYPE_HOST, VIR_CPU_TYPE_GUEST, @@ -103,6 +105,7 @@ struct _virCPUDef { int match; /* enum virCPUMatch */ char *arch; char *model; + char *vendor_id; /* vendor id returned by CPUID in the guest */ int fallback; /* enum virCPUFallback */ char *vendor; unsigned int sockets; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bd4f96a..56ecefb 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3910,7 +3910,9 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver, } virBufferAddLit(&buf, "host"); } else { - if (VIR_ALLOC(guest)< 0 || !(guest->arch = strdup(host->arch))) + if (VIR_ALLOC(guest)< 0 + || !(guest->arch = strdup(host->arch)) + || (cpu->vendor_id&& !(guest->vendor_id = strdup(cpu->vendor_id)))) goto no_memory;
if (cpu->match == VIR_CPU_MATCH_MINIMUM) @@ -3924,6 +3926,8 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver, goto cleanup;
virBufferAdd(&buf, guest->model, -1); + if(guest->vendor_id) + virBufferAsprintf(&buf, ",vendor=%s", guest->vendor_id); for (i = 0; i< guest->nfeatures; i++) { char sign; if (guest->features[i].policy == VIR_CPU_FEATURE_DISABLE) diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 8d5a3bf..8b7cb33 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -116,6 +116,7 @@ virCapsPtr testQemuCapsInit(void) { 0, /* match */ (char *) "x86_64", /* arch */ (char *) "core2duo", /* model */ + NULL, /* vendor_id */ 0, /* fallback */ (char *) "Intel", /* vendor */ 1, /* sockets */
On 21.06.2012 16:58, Hendrik Schwartke wrote:
Introducing the attribute vendor_id to force the CPUID instruction in a kvm guest to return the specified vendor. --- docs/schemas/domaincommon.rng | 7 +++++ src/conf/cpu_conf.c | 61 ++++++++++++++++++++++++++++++++--------- src/conf/cpu_conf.h | 3 ++ src/qemu/qemu_command.c | 6 +++- tests/testutilsqemu.c | 1 + 5 files changed, 64 insertions(+), 14 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 46e539d..a246e8b 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2820,6 +2820,13 @@ </choice> </attribute> </optional> + <optional> + <attribute name="vendor_id"> + <data type="string"> + <param name='pattern'>.{12}</param> + </data> + </attribute> + </optional> <choice> <text/> <empty/> diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index b520f7c..b8ca7a5 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -22,6 +22,7 @@ */
#include <config.h> +#include <ctype.h>
#include "virterror_internal.h" #include "memory.h" @@ -68,6 +69,7 @@ virCPUDefFreeModel(virCPUDefPtr def)
VIR_FREE(def->model); VIR_FREE(def->vendor); + VIR_FREE(def->vendor_id);
for (i = 0; i < def->nfeatures; i++) VIR_FREE(def->features[i].name); @@ -104,6 +106,7 @@ virCPUDefCopyModel(virCPUDefPtr dst,
if ((src->model && !(dst->model = strdup(src->model))) || (src->vendor && !(dst->vendor = strdup(src->vendor))) + || (src->vendor_id && !(dst->vendor_id = strdup(src->vendor_id))) || VIR_ALLOC_N(dst->features, src->nfeatures) < 0) goto no_memory; dst->nfeatures_max = dst->nfeatures = src->nfeatures; @@ -288,19 +291,42 @@ virCPUDefParseXML(const xmlNodePtr node, }
if (def->type == VIR_CPU_TYPE_GUEST && - def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH && - virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) { - const char *fallback; - - fallback = virXPathString("string(./model[1]/@fallback)", ctxt); - if (fallback) { - def->fallback = virCPUFallbackTypeFromString(fallback); - VIR_FREE(fallback); - if (def->fallback < 0) { - virCPUReportError(VIR_ERR_XML_ERROR, "%s", - _("Invalid fallback attribute")); - goto error; - } + def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { + + if(virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) { + const char *fallback; + + fallback = virXPathString("string(./model[1]/@fallback)", ctxt); + if (fallback) { + def->fallback = virCPUFallbackTypeFromString(fallback); + VIR_FREE(fallback); + if (def->fallback < 0) { + virCPUReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid fallback attribute")); + goto error; + } + } + + if(virXPathBoolean("boolean(./model[1]/@vendor_id)", ctxt)) { + char *vendor_id; + + vendor_id = virXPathString("string(./model[1]/@vendor_id)", ctxt); + if(!vendor_id || strlen(vendor_id)!=VIR_CPU_VENDOR_ID_LENGTH) { + virCPUReportError(VIR_ERR_XML_ERROR, "%s", + _("vendor id must be 12 characters long"));
I'd reword this: virCPUReportError(VIR_ERR_XML_ERROR, _("vendor_id must be exactly %d characters long"), VIR_CPU_VENDOR_ID_LENGTH); I know vendor_id length is unlikely to change but nobody knows ...
+ VIR_FREE(vendor_id); + goto error; + } + for(i=0; i<strlen(vendor_id); i++) { + if(!isprint(vendor_id[i]) || isspace(vendor_id[i])) {
This should be c_isprint() and c_isspace(); however, I am not sure but I think vendor id can contain spaces, doesn't it? Anyway, then you should not #include <ctype.h> but #include "c-ctype.h"
+ virCPUReportError(VIR_ERR_XML_ERROR, "%s", + _("vendor id is invalid")); + VIR_FREE(vendor_id); + goto error; + } + } + def->vendor_id = vendor_id; + } } }
@@ -588,6 +614,8 @@ virCPUDefFormatBuf(virBufferPtr buf, return -1; } virBufferAsprintf(buf, " fallback='%s'", fallback); + if(def->vendor_id) + virBufferAsprintf(buf, " vendor_id='%s'", def->vendor_id); } if (formatModel && def->model) { virBufferAsprintf(buf, ">%s</model>\n", def->model); @@ -738,6 +766,13 @@ virCPUDefIsEqual(virCPUDefPtr src, goto cleanup; }
+ if (STRNEQ_NULLABLE(src->vendor_id, dst->vendor_id)) { + virCPUReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target CPU model %s does not match source %s"), + NULLSTR(dst->vendor_id), NULLSTR(src->vendor_id)); + goto cleanup; + } + if (src->sockets != dst->sockets) { virCPUReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target CPU sockets %d does not match source %d"), diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index f8b7bf9..3daadbd 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -28,6 +28,8 @@ # include "buf.h" # include "xml.h"
+#define VIR_CPU_VENDOR_ID_LENGTH 12 +
Insert one space between '#' and define so this is indented properly.
enum virCPUType { VIR_CPU_TYPE_HOST, VIR_CPU_TYPE_GUEST, @@ -103,6 +105,7 @@ struct _virCPUDef { int match; /* enum virCPUMatch */ char *arch; char *model; + char *vendor_id; /* vendor id returned by CPUID in the guest */ int fallback; /* enum virCPUFallback */ char *vendor; unsigned int sockets; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bd4f96a..56ecefb 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3910,7 +3910,9 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver, } virBufferAddLit(&buf, "host"); } else { - if (VIR_ALLOC(guest) < 0 || !(guest->arch = strdup(host->arch))) + if (VIR_ALLOC(guest) < 0 + || !(guest->arch = strdup(host->arch)) + || (cpu->vendor_id && !(guest->vendor_id = strdup(cpu->vendor_id)))) goto no_memory;
if (cpu->match == VIR_CPU_MATCH_MINIMUM) @@ -3924,6 +3926,8 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver, goto cleanup;
virBufferAdd(&buf, guest->model, -1); + if(guest->vendor_id) + virBufferAsprintf(&buf, ",vendor=%s", guest->vendor_id); for (i = 0; i < guest->nfeatures; i++) { char sign; if (guest->features[i].policy == VIR_CPU_FEATURE_DISABLE) diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 8d5a3bf..8b7cb33 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -116,6 +116,7 @@ virCapsPtr testQemuCapsInit(void) { 0, /* match */ (char *) "x86_64", /* arch */ (char *) "core2duo", /* model */ + NULL, /* vendor_id */ 0, /* fallback */ (char *) "Intel", /* vendor */ 1, /* sockets */
Otherwise looking good. Can somebody chime in and provide more light on allowed characters in vendor id? Michal
On 06/27/2012 09:14 AM, Michal Privoznik wrote:
On 21.06.2012 16:58, Hendrik Schwartke wrote:
Introducing the attribute vendor_id to force the CPUID instruction in a kvm guest to return the specified vendor. ---
+ <optional> + <attribute name="vendor_id"> + <data type="string"> + <param name='pattern'>.{12}</param>
Rather than using '.', use [a-zA-Z...] to limit the RNG to the same set of characters as the C code (I'm not sure what the official set is, though). I can understand rejecting a too-long name, but do we really also want to reject a too-short name?
+ def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { + + if(virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) {
Style: space after 'if'
+ } + + if(virXPathBoolean("boolean(./model[1]/@vendor_id)", ctxt)) { + char *vendor_id; + + vendor_id = virXPathString("string(./model[1]/@vendor_id)", ctxt); + if(!vendor_id || strlen(vendor_id)!=VIR_CPU_VENDOR_ID_LENGTH) {
two more instances. Also, space on both sides of '!='
+ } + for(i=0; i<strlen(vendor_id); i++) {
space after 'for'
+ if(!isprint(vendor_id[i]) || isspace(vendor_id[i])) {
and 'if'
+#define VIR_CPU_VENDOR_ID_LENGTH 12 +
Insert one space between '#' and define so this is indented properly.
'make syntax-check' with 'cppi' installed will flag this for you.
@@ -3924,6 +3926,8 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver, goto cleanup;
virBufferAdd(&buf, guest->model, -1); + if(guest->vendor_id)
space after 'if'
Otherwise looking good. Can somebody chime in and provide more light on allowed characters in vendor id?
Sorry, I can't help there. But I think we've pointed out enough issues to warrant a v2 submission. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Thank you Michal and Eric for fast On 27.06.2012 17:22, Eric Blake wrote:
On 21.06.2012 16:58, Hendrik Schwartke wrote:
Introducing the attribute vendor_id to force the CPUID instruction in a kvm guest to return the specified vendor. --- +<optional> +<attribute name="vendor_id"> +<data type="string"> +<param name='pattern'>.{12}</param> Rather than using '.', use [a-zA-Z...] to limit the RNG to the same set of characters as the C code (I'm not sure what the official set is,
On 06/27/2012 09:14 AM, Michal Privoznik wrote: though). I can understand rejecting a too-long name, but do we really also want to reject a too-short name? Ok, that right. I will fix the pattern. The reason for rejecting too-short names is that the CPUID instruction returns the vendor id in EBX, ECX and EDX, thus the vendor id must be exactly 12 characters long. So it would be necessary to pad short names in some way. I think the best way to handle them is to reject them.
+ def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { + + if(virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) { Style: space after 'if'
+ } + + if(virXPathBoolean("boolean(./model[1]/@vendor_id)", ctxt)) { + char *vendor_id; + + vendor_id = virXPathString("string(./model[1]/@vendor_id)", ctxt); + if(!vendor_id || strlen(vendor_id)!=VIR_CPU_VENDOR_ID_LENGTH) { two more instances. Also, space on both sides of '!='
+ } + for(i=0; i<strlen(vendor_id); i++) { space after 'for'
+ if(!isprint(vendor_id[i]) || isspace(vendor_id[i])) { and 'if'
+#define VIR_CPU_VENDOR_ID_LENGTH 12 +
Insert one space between '#' and define so this is indented properly. 'make syntax-check' with 'cppi' installed will flag this for you.
Thanks a lot, that's a very good hint!
@@ -3924,6 +3926,8 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver, goto cleanup;
virBufferAdd(&buf, guest->model, -1); + if(guest->vendor_id) space after 'if'
Otherwise looking good. Can somebody chime in and provide more light on allowed characters in vendor id?
Well, the Intel instruction manual doesn't make any statement about the allowed characters. There are two reasons why I check for printable characters and spaces. The first is that I think the CPUID instruction is intended that way. The second is that I want to prevent problems calling qemu. It's for example not possible to pass spaces in the vendor id to qemu.
Sorry, I can't help there. But I think we've pointed out enough issues to warrant a v2 submission.
I will rework the patch and post it tomorrow.
On 27.06.2012 18:09, Hendrik Schwartke wrote:
Thank you Michal and Eric for fast
On 27.06.2012 17:22, Eric Blake wrote:
On 21.06.2012 16:58, Hendrik Schwartke wrote:
Introducing the attribute vendor_id to force the CPUID instruction in a kvm guest to return the specified vendor. --- +<optional> +<attribute name="vendor_id"> +<data type="string"> +<param name='pattern'>.{12}</param> Rather than using '.', use [a-zA-Z...] to limit the RNG to the same set of characters as the C code (I'm not sure what the official set is,
On 06/27/2012 09:14 AM, Michal Privoznik wrote: though). I can understand rejecting a too-long name, but do we really also want to reject a too-short name? Ok, that right. I will fix the pattern. The reason for rejecting too-short names is that the CPUID instruction returns the vendor id in EBX, ECX and EDX, thus the vendor id must be exactly 12 characters long. So it would be necessary to pad short names in some way. I think the best way to handle them is to reject them.
+ def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { + + if(virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) { Style: space after 'if'
+ } + + if(virXPathBoolean("boolean(./model[1]/@vendor_id)", ctxt)) { + char *vendor_id; + + vendor_id = virXPathString("string(./model[1]/@vendor_id)", ctxt); + if(!vendor_id || strlen(vendor_id)!=VIR_CPU_VENDOR_ID_LENGTH) { two more instances. Also, space on both sides of '!='
+ } + for(i=0; i<strlen(vendor_id); i++) { space after 'for'
+ if(!isprint(vendor_id[i]) || isspace(vendor_id[i])) { and 'if'
+#define VIR_CPU_VENDOR_ID_LENGTH 12 +
Insert one space between '#' and define so this is indented properly. 'make syntax-check' with 'cppi' installed will flag this for you.
Thanks a lot, that's a very good hint!
@@ -3924,6 +3926,8 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver, goto cleanup;
virBufferAdd(&buf, guest->model, -1); + if(guest->vendor_id) space after 'if'
Otherwise looking good. Can somebody chime in and provide more light on allowed characters in vendor id?
Well, the Intel instruction manual doesn't make any statement about the allowed characters. There are two reasons why I check for printable characters and spaces. The first is that I think the CPUID instruction is intended that way. The second is that I want to prevent problems calling qemu. It's for example not possible to pass spaces in the vendor id to qemu.
Sorry, I can't help there. But I think we've pointed out enough issues to warrant a v2 submission.
I will rework the patch and post it tomorrow.
According to Wikipedia, it's possible to have spaces in vendor ID: http://en.wikipedia.org/wiki/CPUID#EAX.3D0:_Get_vendor_ID e.g. "VIA VIA VIA ". In that case we need to find a way of telling this to qemu. What about simple argument closing into quotation marks? Michal
On 27.06.2012 18:29, Michal Privoznik wrote:
On 27.06.2012 18:09, Hendrik Schwartke wrote:
Thank you Michal and Eric for fast
On 27.06.2012 17:22, Eric Blake wrote:
On 21.06.2012 16:58, Hendrik Schwartke wrote:
Introducing the attribute vendor_id to force the CPUID instruction in a kvm guest to return the specified vendor. --- +<optional> +<attribute name="vendor_id"> +<data type="string"> +<param name='pattern'>.{12}</param> Rather than using '.', use [a-zA-Z...] to limit the RNG to the same set of characters as the C code (I'm not sure what the official set is,
On 06/27/2012 09:14 AM, Michal Privoznik wrote: though). I can understand rejecting a too-long name, but do we really also want to reject a too-short name? Ok, that right. I will fix the pattern. The reason for rejecting too-short names is that the CPUID instruction returns the vendor id in EBX, ECX and EDX, thus the vendor id must be exactly 12 characters long. So it would be necessary to pad short names in some way. I think the best way to handle them is to reject them.
+ def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { + + if(virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) { Style: space after 'if'
+ } + + if(virXPathBoolean("boolean(./model[1]/@vendor_id)", ctxt)) { + char *vendor_id; + + vendor_id = virXPathString("string(./model[1]/@vendor_id)", ctxt); + if(!vendor_id || strlen(vendor_id)!=VIR_CPU_VENDOR_ID_LENGTH) { two more instances. Also, space on both sides of '!='
+ } + for(i=0; i<strlen(vendor_id); i++) { space after 'for'
+ if(!isprint(vendor_id[i]) || isspace(vendor_id[i])) { and 'if'
+#define VIR_CPU_VENDOR_ID_LENGTH 12 + Insert one space between '#' and define so this is indented properly. 'make syntax-check' with 'cppi' installed will flag this for you. Thanks a lot, that's a very good hint! @@ -3924,6 +3926,8 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver, goto cleanup;
virBufferAdd(&buf, guest->model, -1); + if(guest->vendor_id) space after 'if'
Otherwise looking good. Can somebody chime in and provide more light on allowed characters in vendor id? Well, the Intel instruction manual doesn't make any statement about the allowed characters. There are two reasons why I check for printable characters and spaces. The first is that I think the CPUID instruction is intended that way. The second is that I want to prevent problems calling qemu. It's for example not possible to pass spaces in the vendor id to qemu.
Sorry, I can't help there. But I think we've pointed out enough issues to warrant a v2 submission.
I will rework the patch and post it tomorrow.
According to Wikipedia, it's possible to have spaces in vendor ID:
http://en.wikipedia.org/wiki/CPUID#EAX.3D0:_Get_vendor_ID
e.g. "VIA VIA VIA ". In that case we need to find a way of telling this to qemu. What about simple argument closing into quotation marks?
Michal Yes, you're right. I tested again. The reason for my problem with spaces was the shell. I had a look at the qemu code and the only character which is not usable is ','. I will post the patch shortly.
Hendrik
Introducing the attribute vendor_id to force the CPUID instruction in a kvm guest to return the specified vendor. --- docs/schemas/domaincommon.rng | 7 +++++ src/conf/cpu_conf.c | 64 +++++++++++++++++++++++++++++++++-------- src/conf/cpu_conf.h | 3 ++ src/qemu/qemu_command.c | 6 +++- tests/testutilsqemu.c | 1 + 5 files changed, 68 insertions(+), 13 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 46e539d..5c55269 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2820,6 +2820,13 @@ </choice> </attribute> </optional> + <optional> + <attribute name="vendor_id"> + <data type="string"> + <param name='pattern'>[^,]{12}</param> + </data> + </attribute> + </optional> <choice> <text/> <empty/> diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index b520f7c..b3098d8 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -22,6 +22,7 @@ */ #include <config.h> +#include <c-ctype.h> #include "virterror_internal.h" #include "memory.h" @@ -68,6 +69,7 @@ virCPUDefFreeModel(virCPUDefPtr def) VIR_FREE(def->model); VIR_FREE(def->vendor); + VIR_FREE(def->vendor_id); for (i = 0; i < def->nfeatures; i++) VIR_FREE(def->features[i].name); @@ -104,6 +106,7 @@ virCPUDefCopyModel(virCPUDefPtr dst, if ((src->model && !(dst->model = strdup(src->model))) || (src->vendor && !(dst->vendor = strdup(src->vendor))) + || (src->vendor_id && !(dst->vendor_id = strdup(src->vendor_id))) || VIR_ALLOC_N(dst->features, src->nfeatures) < 0) goto no_memory; dst->nfeatures_max = dst->nfeatures = src->nfeatures; @@ -288,18 +291,46 @@ virCPUDefParseXML(const xmlNodePtr node, } if (def->type == VIR_CPU_TYPE_GUEST && - def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH && - virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) { - const char *fallback; - - fallback = virXPathString("string(./model[1]/@fallback)", ctxt); - if (fallback) { - def->fallback = virCPUFallbackTypeFromString(fallback); - VIR_FREE(fallback); - if (def->fallback < 0) { - virCPUReportError(VIR_ERR_XML_ERROR, "%s", - _("Invalid fallback attribute")); - goto error; + def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { + + if (virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) { + const char *fallback; + + fallback = + virXPathString("string(./model[1]/@fallback)", ctxt); + if (fallback) { + def->fallback = virCPUFallbackTypeFromString(fallback); + VIR_FREE(fallback); + if (def->fallback < 0) { + virCPUReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid fallback attribute")); + goto error; + } + } + + if (virXPathBoolean("boolean(./model[1]/@vendor_id)", ctxt)) { + char *vendor_id; + + vendor_id = + virXPathString("string(./model[1]/@vendor_id)", ctxt); + if (!vendor_id + || strlen(vendor_id) != VIR_CPU_VENDOR_ID_LENGTH) { + virCPUReportError(VIR_ERR_XML_ERROR, + _("vendor_id must be exactly %d characters long"), + VIR_CPU_VENDOR_ID_LENGTH); + VIR_FREE(vendor_id); + goto error; + } + /* ensure that the string can be passed to qemu*/ + for (i = 0; i < strlen(vendor_id); i++) { + if (vendor_id[i]==',') { + virCPUReportError(VIR_ERR_XML_ERROR, "%s", + _("vendor id is invalid")); + VIR_FREE(vendor_id); + goto error; + } + } + def->vendor_id = vendor_id; } } } @@ -588,6 +619,8 @@ virCPUDefFormatBuf(virBufferPtr buf, return -1; } virBufferAsprintf(buf, " fallback='%s'", fallback); + if (def->vendor_id) + virBufferAsprintf(buf, " vendor_id='%s'", def->vendor_id); } if (formatModel && def->model) { virBufferAsprintf(buf, ">%s</model>\n", def->model); @@ -738,6 +771,13 @@ virCPUDefIsEqual(virCPUDefPtr src, goto cleanup; } + if (STRNEQ_NULLABLE(src->vendor_id, dst->vendor_id)) { + virCPUReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target CPU model %s does not match source %s"), + NULLSTR(dst->vendor_id), NULLSTR(src->vendor_id)); + goto cleanup; + } + if (src->sockets != dst->sockets) { virCPUReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target CPU sockets %d does not match source %d"), diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index f8b7bf9..2df0a50 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -28,6 +28,8 @@ # include "buf.h" # include "xml.h" +# define VIR_CPU_VENDOR_ID_LENGTH 12 + enum virCPUType { VIR_CPU_TYPE_HOST, VIR_CPU_TYPE_GUEST, @@ -103,6 +105,7 @@ struct _virCPUDef { int match; /* enum virCPUMatch */ char *arch; char *model; + char *vendor_id; /* vendor id returned by CPUID in the guest */ int fallback; /* enum virCPUFallback */ char *vendor; unsigned int sockets; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bd4f96a..d8d0220 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3910,7 +3910,9 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver, } virBufferAddLit(&buf, "host"); } else { - if (VIR_ALLOC(guest) < 0 || !(guest->arch = strdup(host->arch))) + if (VIR_ALLOC(guest) < 0 + || !(guest->arch = strdup(host->arch)) + || (cpu->vendor_id && !(guest->vendor_id = strdup(cpu->vendor_id)))) goto no_memory; if (cpu->match == VIR_CPU_MATCH_MINIMUM) @@ -3924,6 +3926,8 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver, goto cleanup; virBufferAdd(&buf, guest->model, -1); + if (guest->vendor_id) + virBufferAsprintf(&buf, ",vendor=%s", guest->vendor_id); for (i = 0; i < guest->nfeatures; i++) { char sign; if (guest->features[i].policy == VIR_CPU_FEATURE_DISABLE) diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 8d5a3bf..8b7cb33 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -116,6 +116,7 @@ virCapsPtr testQemuCapsInit(void) { 0, /* match */ (char *) "x86_64", /* arch */ (char *) "core2duo", /* model */ + NULL, /* vendor_id */ 0, /* fallback */ (char *) "Intel", /* vendor */ 1, /* sockets */ -- 1.7.9.5
On 28.06.2012 12:21, Hendrik Schwartke wrote:
Introducing the attribute vendor_id to force the CPUID instruction in a kvm guest to return the specified vendor. --- docs/schemas/domaincommon.rng | 7 +++++ src/conf/cpu_conf.c | 64 +++++++++++++++++++++++++++++++++-------- src/conf/cpu_conf.h | 3 ++ src/qemu/qemu_command.c | 6 +++- tests/testutilsqemu.c | 1 + 5 files changed, 68 insertions(+), 13 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 46e539d..5c55269 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2820,6 +2820,13 @@ </choice> </attribute> </optional> + <optional> + <attribute name="vendor_id"> + <data type="string"> + <param name='pattern'>[^,]{12}</param> + </data> + </attribute> + </optional> <choice> <text/> <empty/> diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index b520f7c..b3098d8 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -22,6 +22,7 @@ */
#include <config.h> +#include <c-ctype.h>
This is useless.
#include "virterror_internal.h" #include "memory.h" @@ -68,6 +69,7 @@ virCPUDefFreeModel(virCPUDefPtr def)
VIR_FREE(def->model); VIR_FREE(def->vendor); + VIR_FREE(def->vendor_id);
for (i = 0; i < def->nfeatures; i++) VIR_FREE(def->features[i].name); @@ -104,6 +106,7 @@ virCPUDefCopyModel(virCPUDefPtr dst,
if ((src->model && !(dst->model = strdup(src->model))) || (src->vendor && !(dst->vendor = strdup(src->vendor))) + || (src->vendor_id && !(dst->vendor_id = strdup(src->vendor_id))) || VIR_ALLOC_N(dst->features, src->nfeatures) < 0) goto no_memory; dst->nfeatures_max = dst->nfeatures = src->nfeatures; @@ -288,18 +291,46 @@ virCPUDefParseXML(const xmlNodePtr node, }
if (def->type == VIR_CPU_TYPE_GUEST && - def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH && - virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) { - const char *fallback; - - fallback = virXPathString("string(./model[1]/@fallback)", ctxt); - if (fallback) { - def->fallback = virCPUFallbackTypeFromString(fallback); - VIR_FREE(fallback); - if (def->fallback < 0) { - virCPUReportError(VIR_ERR_XML_ERROR, "%s", - _("Invalid fallback attribute")); - goto error; + def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { + + if (virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) { + const char *fallback; + + fallback = + virXPathString("string(./model[1]/@fallback)", ctxt);
Why this change?
+ if (fallback) { + def->fallback = virCPUFallbackTypeFromString(fallback); + VIR_FREE(fallback); + if (def->fallback < 0) { + virCPUReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid fallback attribute")); + goto error; + } + } + + if (virXPathBoolean("boolean(./model[1]/@vendor_id)", ctxt)) { + char *vendor_id; + + vendor_id = + virXPathString("string(./model[1]/@vendor_id)", ctxt);
If we are dealing with long lines we tend to split them rather at ',' than this.
+ if (!vendor_id + || strlen(vendor_id) != VIR_CPU_VENDOR_ID_LENGTH) {
And || operator needs to be on the preceding line.
+ virCPUReportError(VIR_ERR_XML_ERROR, + _("vendor_id must be exactly %d characters long"),
Long line.
+ VIR_CPU_VENDOR_ID_LENGTH); + VIR_FREE(vendor_id); + goto error; + } + /* ensure that the string can be passed to qemu*/ + for (i = 0; i < strlen(vendor_id); i++) { + if (vendor_id[i]==',') { + virCPUReportError(VIR_ERR_XML_ERROR, "%s", + _("vendor id is invalid")); + VIR_FREE(vendor_id); + goto error; + } + } + def->vendor_id = vendor_id; } } } @@ -588,6 +619,8 @@ virCPUDefFormatBuf(virBufferPtr buf, return -1; } virBufferAsprintf(buf, " fallback='%s'", fallback); + if (def->vendor_id) + virBufferAsprintf(buf, " vendor_id='%s'", def->vendor_id); } if (formatModel && def->model) { virBufferAsprintf(buf, ">%s</model>\n", def->model); @@ -738,6 +771,13 @@ virCPUDefIsEqual(virCPUDefPtr src, goto cleanup; }
+ if (STRNEQ_NULLABLE(src->vendor_id, dst->vendor_id)) { + virCPUReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target CPU model %s does not match source %s"), + NULLSTR(dst->vendor_id), NULLSTR(src->vendor_id)); + goto cleanup; + } + if (src->sockets != dst->sockets) { virCPUReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target CPU sockets %d does not match source %d"), diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index f8b7bf9..2df0a50 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -28,6 +28,8 @@ # include "buf.h" # include "xml.h"
+# define VIR_CPU_VENDOR_ID_LENGTH 12 + enum virCPUType { VIR_CPU_TYPE_HOST, VIR_CPU_TYPE_GUEST, @@ -103,6 +105,7 @@ struct _virCPUDef { int match; /* enum virCPUMatch */ char *arch; char *model; + char *vendor_id; /* vendor id returned by CPUID in the guest */ int fallback; /* enum virCPUFallback */ char *vendor; unsigned int sockets; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bd4f96a..d8d0220 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3910,7 +3910,9 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver, } virBufferAddLit(&buf, "host"); } else { - if (VIR_ALLOC(guest) < 0 || !(guest->arch = strdup(host->arch))) + if (VIR_ALLOC(guest) < 0 + || !(guest->arch = strdup(host->arch)) + || (cpu->vendor_id && !(guest->vendor_id = strdup(cpu->vendor_id)))) goto no_memory;
Again operators on the begining of line.
if (cpu->match == VIR_CPU_MATCH_MINIMUM) @@ -3924,6 +3926,8 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver, goto cleanup;
virBufferAdd(&buf, guest->model, -1); + if (guest->vendor_id) + virBufferAsprintf(&buf, ",vendor=%s", guest->vendor_id); for (i = 0; i < guest->nfeatures; i++) { char sign; if (guest->features[i].policy == VIR_CPU_FEATURE_DISABLE) diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 8d5a3bf..8b7cb33 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -116,6 +116,7 @@ virCapsPtr testQemuCapsInit(void) { 0, /* match */ (char *) "x86_64", /* arch */ (char *) "core2duo", /* model */ + NULL, /* vendor_id */ 0, /* fallback */ (char *) "Intel", /* vendor */ 1, /* sockets */
So squashing this in: diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index b3098d8..7fe3c1e 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -22,7 +22,6 @@ */ #include <config.h> -#include <c-ctype.h> #include "virterror_internal.h" #include "memory.h" @@ -296,8 +295,7 @@ virCPUDefParseXML(const xmlNodePtr node, if (virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) { const char *fallback; - fallback = - virXPathString("string(./model[1]/@fallback)", ctxt); + fallback = virXPathString("string(./model[1]/@fallback)", ctxt); if (fallback) { def->fallback = virCPUFallbackTypeFromString(fallback); VIR_FREE(fallback); @@ -311,12 +309,13 @@ virCPUDefParseXML(const xmlNodePtr node, if (virXPathBoolean("boolean(./model[1]/@vendor_id)", ctxt)) { char *vendor_id; - vendor_id = - virXPathString("string(./model[1]/@vendor_id)", ctxt); - if (!vendor_id - || strlen(vendor_id) != VIR_CPU_VENDOR_ID_LENGTH) { + vendor_id = virXPathString("string(./model[1]/@vendor_id)", + ctxt); + if (!vendor_id || + strlen(vendor_id) != VIR_CPU_VENDOR_ID_LENGTH) { virCPUReportError(VIR_ERR_XML_ERROR, - _("vendor_id must be exactly %d characters long"), + _("vendor_id must be exactly" + " %d characters long"), VIR_CPU_VENDOR_ID_LENGTH); VIR_FREE(vendor_id); goto error; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0b5d894..528b189 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3913,9 +3913,9 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver, } virBufferAddLit(&buf, "host"); } else { - if (VIR_ALLOC(guest) < 0 - || !(guest->arch = strdup(host->arch)) - || (cpu->vendor_id && !(guest->vendor_id = strdup(cpu->vendor_id)))) + if (VIR_ALLOC(guest) < 0 || + !(guest->arch = strdup(host->arch)) || + (cpu->vendor_id && !(guest->vendor_id = strdup(cpu->vendor_id)))) goto no_memory; if (cpu->match == VIR_CPU_MATCH_MINIMUM) And pushed now. Michal
On 28.06.2012 12:21, Hendrik Schwartke wrote:
Introducing the attribute vendor_id to force the CPUID instruction in a kvm guest to return the specified vendor. --- docs/schemas/domaincommon.rng | 7 +++++ src/conf/cpu_conf.c | 64 +++++++++++++++++++++++++++++++++-------- src/conf/cpu_conf.h | 3 ++ src/qemu/qemu_command.c | 6 +++- tests/testutilsqemu.c | 1 + 5 files changed, 68 insertions(+), 13 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 46e539d..5c55269 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2820,6 +2820,13 @@ </choice> </attribute> </optional> +<optional> +<attribute name="vendor_id"> +<data type="string"> +<param name='pattern'>[^,]{12}</param> +</data> +</attribute> +</optional> <choice> <text/> <empty/> diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index b520f7c..b3098d8 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -22,6 +22,7 @@ */
#include<config.h> +#include<c-ctype.h> This is useless. Oops! I overlooked that.
#include "virterror_internal.h" #include "memory.h" @@ -68,6 +69,7 @@ virCPUDefFreeModel(virCPUDefPtr def)
VIR_FREE(def->model); VIR_FREE(def->vendor); + VIR_FREE(def->vendor_id);
for (i = 0; i< def->nfeatures; i++) VIR_FREE(def->features[i].name); @@ -104,6 +106,7 @@ virCPUDefCopyModel(virCPUDefPtr dst,
if ((src->model&& !(dst->model = strdup(src->model))) || (src->vendor&& !(dst->vendor = strdup(src->vendor))) + || (src->vendor_id&& !(dst->vendor_id = strdup(src->vendor_id))) || VIR_ALLOC_N(dst->features, src->nfeatures)< 0) goto no_memory; dst->nfeatures_max = dst->nfeatures = src->nfeatures; @@ -288,18 +291,46 @@ virCPUDefParseXML(const xmlNodePtr node, }
if (def->type == VIR_CPU_TYPE_GUEST&& - def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH&& - virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) { - const char *fallback; - - fallback = virXPathString("string(./model[1]/@fallback)", ctxt); - if (fallback) { - def->fallback = virCPUFallbackTypeFromString(fallback); - VIR_FREE(fallback); - if (def->fallback< 0) { - virCPUReportError(VIR_ERR_XML_ERROR, "%s", - _("Invalid fallback attribute")); - goto error; + def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { + + if (virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) { + const char *fallback; + + fallback = + virXPathString("string(./model[1]/@fallback)", ctxt); Why this change? Well, Eric suggested to use 'make syntax-check'. And it recommended to use two lines. + if (fallback) { + def->fallback = virCPUFallbackTypeFromString(fallback); + VIR_FREE(fallback); + if (def->fallback< 0) { + virCPUReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid fallback attribute")); + goto error; + } + } + + if (virXPathBoolean("boolean(./model[1]/@vendor_id)", ctxt)) { + char *vendor_id; + + vendor_id = + virXPathString("string(./model[1]/@vendor_id)", ctxt); If we are dealing with long lines we tend to split them rather at ',' than this. I think make syntax-check didn't like the long line here. + if (!vendor_id + || strlen(vendor_id) != VIR_CPU_VENDOR_ID_LENGTH) { And || operator needs to be on the preceding line. Again the syntax check recommended something else. There are also a lot of other places in the code where operators are at the beginning of a
On 03.07.2012 12:06, Michal Privoznik wrote: line, see above. (IMHO it's easier to read.)
+ virCPUReportError(VIR_ERR_XML_ERROR, + _("vendor_id must be exactly %d characters long"), Long line. Yep, your're right + VIR_CPU_VENDOR_ID_LENGTH); + VIR_FREE(vendor_id); + goto error; + } + /* ensure that the string can be passed to qemu*/ + for (i = 0; i< strlen(vendor_id); i++) { + if (vendor_id[i]==',') { + virCPUReportError(VIR_ERR_XML_ERROR, "%s", + _("vendor id is invalid")); + VIR_FREE(vendor_id); + goto error; + } + } + def->vendor_id = vendor_id; } } } @@ -588,6 +619,8 @@ virCPUDefFormatBuf(virBufferPtr buf, return -1; } virBufferAsprintf(buf, " fallback='%s'", fallback); + if (def->vendor_id) + virBufferAsprintf(buf, " vendor_id='%s'", def->vendor_id); } if (formatModel&& def->model) { virBufferAsprintf(buf, ">%s</model>\n", def->model); @@ -738,6 +771,13 @@ virCPUDefIsEqual(virCPUDefPtr src, goto cleanup; }
+ if (STRNEQ_NULLABLE(src->vendor_id, dst->vendor_id)) { + virCPUReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target CPU model %s does not match source %s"), + NULLSTR(dst->vendor_id), NULLSTR(src->vendor_id)); + goto cleanup; + } + if (src->sockets != dst->sockets) { virCPUReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target CPU sockets %d does not match source %d"), diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index f8b7bf9..2df0a50 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -28,6 +28,8 @@ # include "buf.h" # include "xml.h"
+# define VIR_CPU_VENDOR_ID_LENGTH 12 + enum virCPUType { VIR_CPU_TYPE_HOST, VIR_CPU_TYPE_GUEST, @@ -103,6 +105,7 @@ struct _virCPUDef { int match; /* enum virCPUMatch */ char *arch; char *model; + char *vendor_id; /* vendor id returned by CPUID in the guest */ int fallback; /* enum virCPUFallback */ char *vendor; unsigned int sockets; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bd4f96a..d8d0220 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3910,7 +3910,9 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver, } virBufferAddLit(&buf, "host"); } else { - if (VIR_ALLOC(guest)< 0 || !(guest->arch = strdup(host->arch))) + if (VIR_ALLOC(guest)< 0 + || !(guest->arch = strdup(host->arch)) + || (cpu->vendor_id&& !(guest->vendor_id = strdup(cpu->vendor_id)))) goto no_memory; Again operators on the begining of line. Ok, get that.
if (cpu->match == VIR_CPU_MATCH_MINIMUM) @@ -3924,6 +3926,8 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver, goto cleanup;
virBufferAdd(&buf, guest->model, -1); + if (guest->vendor_id) + virBufferAsprintf(&buf, ",vendor=%s", guest->vendor_id); for (i = 0; i< guest->nfeatures; i++) { char sign; if (guest->features[i].policy == VIR_CPU_FEATURE_DISABLE) diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 8d5a3bf..8b7cb33 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -116,6 +116,7 @@ virCapsPtr testQemuCapsInit(void) { 0, /* match */ (char *) "x86_64", /* arch */ (char *) "core2duo", /* model */ + NULL, /* vendor_id */ 0, /* fallback */ (char *) "Intel", /* vendor */ 1, /* sockets */
So squashing this in:
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index b3098d8..7fe3c1e 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -22,7 +22,6 @@ */
#include<config.h> -#include<c-ctype.h>
#include "virterror_internal.h" #include "memory.h" @@ -296,8 +295,7 @@ virCPUDefParseXML(const xmlNodePtr node, if (virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) { const char *fallback;
- fallback = - virXPathString("string(./model[1]/@fallback)", ctxt); + fallback = virXPathString("string(./model[1]/@fallback)", ctxt); if (fallback) { def->fallback = virCPUFallbackTypeFromString(fallback); VIR_FREE(fallback); @@ -311,12 +309,13 @@ virCPUDefParseXML(const xmlNodePtr node, if (virXPathBoolean("boolean(./model[1]/@vendor_id)", ctxt)) { char *vendor_id;
- vendor_id = - virXPathString("string(./model[1]/@vendor_id)", ctxt); - if (!vendor_id - || strlen(vendor_id) != VIR_CPU_VENDOR_ID_LENGTH) { + vendor_id = virXPathString("string(./model[1]/@vendor_id)", + ctxt); + if (!vendor_id || + strlen(vendor_id) != VIR_CPU_VENDOR_ID_LENGTH) { virCPUReportError(VIR_ERR_XML_ERROR, - _("vendor_id must be exactly %d characters long"), + _("vendor_id must be exactly" + " %d characters long"), VIR_CPU_VENDOR_ID_LENGTH); VIR_FREE(vendor_id); goto error; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0b5d894..528b189 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3913,9 +3913,9 @@ qemuBuildCpuArgStr(const struct qemud_driver *driver, } virBufferAddLit(&buf, "host"); } else { - if (VIR_ALLOC(guest)< 0 - || !(guest->arch = strdup(host->arch)) - || (cpu->vendor_id&& !(guest->vendor_id = strdup(cpu->vendor_id)))) + if (VIR_ALLOC(guest)< 0 || + !(guest->arch = strdup(host->arch)) || + (cpu->vendor_id&& !(guest->vendor_id = strdup(cpu->vendor_id)))) goto no_memory;
if (cpu->match == VIR_CPU_MATCH_MINIMUM)
And pushed now.
Michal Thanks a lot Michal!
Hendrik
participants (3)
-
Eric Blake -
Hendrik Schwartke -
Michal Privoznik