[libvirt] [PATCH v2 0/3] Improve PPC CPU driver
From: Li Zhang <zhlcindy@linux.vnet.ibm.com> Currently, PPC CPU driver doesn't support to parse guest data. It can't pass CPU parameters to Qemu command line. This patchset is to implement .guestData to support to parse guest CPU configuration and .update to support host-model and host-passthrough. The guest CPU configuration is as the following: <cpu match='exact'> <model>POWER7_v2.3</model> <vendor>IBM</vendor> </cpu> v2 -> v1: * Remove features functions calling for non-x86 platform (Doug Goldstein) * Improve update code. * Merge update code with guestData. Li Zhang (3): Remove CPU features check for non-x86 platform. cpu: Implement guestData and update for PPC cpu: Add cpu test cases for PPC CPU driver. src/cpu/cpu_powerpc.c | 178 ++++++++++++++++++++- src/qemu/qemu_command.c | 16 +- tests/cputest.c | 9 ++ tests/cputestdata/ppc64-baseline-1-result.xml | 3 + .../ppc64-baseline-incompatible-vendors.xml | 14 ++ .../ppc64-baseline-no-vendor-result.xml | 3 + tests/cputestdata/ppc64-baseline-no-vendor.xml | 7 + tests/cputestdata/ppc64-exact.xml | 3 + tests/cputestdata/ppc64-guest-nofallback.xml | 4 + tests/cputestdata/ppc64-guest.xml | 4 + .../ppc64-host+guest,ppc_models-result.xml | 5 + ...st-nofallback,ppc_models,POWER7_v2.1-result.xml | 5 + tests/cputestdata/ppc64-host.xml | 6 + tests/cputestdata/ppc64-strict.xml | 3 + .../qemuxml2argv-pseries-cpu-exact.args | 7 + .../qemuxml2argv-pseries-cpu-exact.xml | 21 +++ tests/qemuxml2argvtest.c | 1 + 17 files changed, 279 insertions(+), 10 deletions(-) create mode 100644 tests/cputestdata/ppc64-baseline-1-result.xml create mode 100644 tests/cputestdata/ppc64-baseline-incompatible-vendors.xml create mode 100644 tests/cputestdata/ppc64-baseline-no-vendor-result.xml create mode 100644 tests/cputestdata/ppc64-baseline-no-vendor.xml create mode 100644 tests/cputestdata/ppc64-exact.xml create mode 100644 tests/cputestdata/ppc64-guest-nofallback.xml create mode 100644 tests/cputestdata/ppc64-guest.xml create mode 100644 tests/cputestdata/ppc64-host+guest,ppc_models-result.xml create mode 100644 tests/cputestdata/ppc64-host+guest-nofallback,ppc_models,POWER7_v2.1-result.xml create mode 100644 tests/cputestdata/ppc64-host.xml create mode 100644 tests/cputestdata/ppc64-strict.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-exact.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-exact.xml -- 1.8.1.4
From: Li Zhang <zhlcindy@linux.vnet.ibm.com> CPU features are not supported on non-x86 and hasFeatures will be NULL. This patch is to remove CPU features functions calling to avoid errors. Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f8fccea..3b6ba7a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6349,7 +6349,6 @@ qemuBuildCpuArgStr(const virQEMUDriverPtr driver, (def->cpu->mode != VIR_CPU_MODE_CUSTOM || def->cpu->model)) { virCPUCompareResult cmp; const char *preferred; - int hasSVM; if (!host || !host->model || @@ -6389,10 +6388,13 @@ qemuBuildCpuArgStr(const virQEMUDriverPtr driver, /* Only 'svm' requires --enable-nesting. The nested * 'vmx' patches now simply hook off the CPU features */ - hasSVM = cpuHasFeature(data, "svm"); - if (hasSVM < 0) - goto cleanup; - *hasHwVirt = hasSVM > 0 ? true : false; + if (def->os.arch == VIR_ARCH_X86_64 || + def->os.arch == VIR_ARCH_I686) { + int hasSVM = cpuHasFeature(data, "svm"); + if (hasSVM < 0) + goto cleanup; + *hasHwVirt = hasSVM > 0 ? true : false; + } if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) { const char *mode = virCPUModeTypeToString(cpu->mode); @@ -10575,7 +10577,9 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, model = NULL; } - if (virCPUDefAddFeature(cpu, feature, policy) < 0) + if ((dom->os.arch == VIR_ARCH_X86_64 || + dom->os.arch == VIR_ARCH_I686) && + virCPUDefAddFeature(cpu, feature, policy) < 0) goto cleanup; } } else if (STRPREFIX(tokens[i], "hv_")) { -- 1.8.1.4
On Tue, Sep 03, 2013 at 02:28:23PM +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
CPU features are not supported on non-x86 and hasFeatures will be NULL.
This patch is to remove CPU features functions calling to avoid errors.
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f8fccea..3b6ba7a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6349,7 +6349,6 @@ qemuBuildCpuArgStr(const virQEMUDriverPtr driver, (def->cpu->mode != VIR_CPU_MODE_CUSTOM || def->cpu->model)) { virCPUCompareResult cmp; const char *preferred; - int hasSVM;
if (!host || !host->model || @@ -6389,10 +6388,13 @@ qemuBuildCpuArgStr(const virQEMUDriverPtr driver, /* Only 'svm' requires --enable-nesting. The nested * 'vmx' patches now simply hook off the CPU features */ - hasSVM = cpuHasFeature(data, "svm"); - if (hasSVM < 0) - goto cleanup; - *hasHwVirt = hasSVM > 0 ? true : false; + if (def->os.arch == VIR_ARCH_X86_64 || + def->os.arch == VIR_ARCH_I686) { + int hasSVM = cpuHasFeature(data, "svm"); + if (hasSVM < 0) + goto cleanup; + *hasHwVirt = hasSVM > 0 ? true : false; + }
if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) { const char *mode = virCPUModeTypeToString(cpu->mode);
This bit looks good.
@@ -10575,7 +10577,9 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, model = NULL; }
- if (virCPUDefAddFeature(cpu, feature, policy) < 0) + if ((dom->os.arch == VIR_ARCH_X86_64 || + dom->os.arch == VIR_ARCH_I686) && + virCPUDefAddFeature(cpu, feature, policy) < 0) goto cleanup; } } else if (STRPREFIX(tokens[i], "hv_")) {
For this, I think we should virReportError() if we find a CPU flag on the command line for non-x86 arches, instead of silently ignoring it. I think I'd also like to split this patch in two. I'll apply the first chunk of it now. 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 :|
On 2013年09月05日 19:08, Daniel P. Berrange wrote:
On Tue, Sep 03, 2013 at 02:28:23PM +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
CPU features are not supported on non-x86 and hasFeatures will be NULL.
This patch is to remove CPU features functions calling to avoid errors.
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f8fccea..3b6ba7a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6349,7 +6349,6 @@ qemuBuildCpuArgStr(const virQEMUDriverPtr driver, (def->cpu->mode != VIR_CPU_MODE_CUSTOM || def->cpu->model)) { virCPUCompareResult cmp; const char *preferred; - int hasSVM;
if (!host || !host->model || @@ -6389,10 +6388,13 @@ qemuBuildCpuArgStr(const virQEMUDriverPtr driver, /* Only 'svm' requires --enable-nesting. The nested * 'vmx' patches now simply hook off the CPU features */ - hasSVM = cpuHasFeature(data, "svm"); - if (hasSVM < 0) - goto cleanup; - *hasHwVirt = hasSVM > 0 ? true : false; + if (def->os.arch == VIR_ARCH_X86_64 || + def->os.arch == VIR_ARCH_I686) { + int hasSVM = cpuHasFeature(data, "svm"); + if (hasSVM < 0) + goto cleanup; + *hasHwVirt = hasSVM > 0 ? true : false; + }
if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) { const char *mode = virCPUModeTypeToString(cpu->mode); This bit looks good.
@@ -10575,7 +10577,9 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, model = NULL; }
- if (virCPUDefAddFeature(cpu, feature, policy) < 0) + if ((dom->os.arch == VIR_ARCH_X86_64 || + dom->os.arch == VIR_ARCH_I686) && + virCPUDefAddFeature(cpu, feature, policy) < 0) goto cleanup; } } else if (STRPREFIX(tokens[i], "hv_")) { For this, I think we should virReportError() if we find a CPU flag on the command line for non-x86 arches, instead of silently ignoring it.
Okay, I will change it. Thanks.
I think I'd also like to split this patch in two. I'll apply the first chunk of it now.
Daniel
From: Li Zhang <zhlcindy@linux.vnet.ibm.com> On Power platform, Power7+ can support Power7 guest. It needs to define XML configuration to specify guest's CPU model. For exmaple: <cpu match='exact'> <model>POWER7_v2.1</model> <vendor>IBM</vendor> </cpu> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- src/cpu/cpu_powerpc.c | 178 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 174 insertions(+), 4 deletions(-) diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c index 647a8a1..62a789e 100644 --- a/src/cpu/cpu_powerpc.c +++ b/src/cpu/cpu_powerpc.c @@ -99,6 +99,23 @@ ppcModelFindPVR(const struct ppc_map *map, return NULL; } +static struct ppc_model * +ppcModelCopy(const struct ppc_model *model) +{ + struct ppc_model *copy; + + if (VIR_ALLOC(copy) < 0 || + VIR_STRDUP(copy->name, model->name) < 0) { + ppcModelFree(copy); + return NULL; + } + + copy->data.pvr = model->data.pvr; + copy->vendor = model->vendor; + + return copy; +} + static struct ppc_vendor * ppcVendorFind(const struct ppc_map *map, const char *name) @@ -126,6 +143,29 @@ ppcVendorFree(struct ppc_vendor *vendor) VIR_FREE(vendor); } +static struct ppc_model * +ppcModelFromCPU(const virCPUDefPtr cpu, + const struct ppc_map *map) +{ + struct ppc_model *model = NULL; + + if ((model = ppcModelFind(map, cpu->model)) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown CPU model %s"), cpu->model); + goto error; + } + + if ((model = ppcModelCopy(model)) == NULL) + goto error; + + return model; + +error: + ppcModelFree(model); + return NULL; +} + + static int ppcVendorLoad(xmlXPathContextPtr ctxt, struct ppc_map *map) @@ -288,6 +328,111 @@ error: return NULL; } +static virCPUDataPtr +ppcMakeCPUData(virArch arch, struct cpuPPCData *data) +{ + virCPUDataPtr cpuData; + + if (VIR_ALLOC(cpuData) < 0) + return NULL; + + cpuData->arch = arch; + cpuData->data.ppc = *data; + data = NULL; + + return cpuData; +} + +static virCPUCompareResult +ppcCompute(virCPUDefPtr host, + const virCPUDefPtr cpu, + virCPUDataPtr *guestData, + char **message) + +{ + struct ppc_map *map = NULL; + struct ppc_model *host_model = NULL; + struct ppc_model *guest_model = NULL; + + int ret = 0; + virArch arch; + size_t i; + + if (cpu->arch != VIR_ARCH_NONE) { + bool found = false; + + for (i = 0; i < ARRAY_CARDINALITY(archs); i++) { + if (archs[i] == cpu->arch) { + found = true; + break; + } + } + + if (!found) { + VIR_DEBUG("CPU arch %s does not match host arch", + virArchToString(cpu->arch)); + if (message && + virAsprintf(message, + _("CPU arch %s does not match host arch"), + virArchToString(cpu->arch)) < 0) + goto error; + return VIR_CPU_COMPARE_INCOMPATIBLE; + } + arch = cpu->arch; + } else { + arch = host->arch; + } + + if (cpu->vendor && + (!host->vendor || STRNEQ(cpu->vendor, host->vendor))) { + VIR_DEBUG("host CPU vendor does not match required CPU vendor %s", + cpu->vendor); + if (message && + virAsprintf(message, + _("host CPU vendor does not match required " + "CPU vendor %s"), + cpu->vendor) < 0) + goto error; + return VIR_CPU_COMPARE_INCOMPATIBLE; + } + + if (!(map = ppcLoadMap()) || + !(host_model = ppcModelFromCPU(host, map)) || + !(guest_model = ppcModelFromCPU(cpu, map))) + goto error; + + if (guestData != NULL) { + if (cpu->type == VIR_CPU_TYPE_GUEST && + cpu->match == VIR_CPU_MATCH_STRICT && + STRNEQ(guest_model->name, host_model->name)) { + VIR_DEBUG("host CPU model does not match required CPU model %s", + guest_model->name); + if (message && + virAsprintf(message, + _("host CPU model does not match required " + "CPU model %s"), + guest_model->name) < 0) + goto error; + return VIR_CPU_COMPARE_INCOMPATIBLE; + } + + if (!(*guestData = ppcMakeCPUData(arch, &guest_model->data))) + goto error; + } + + ret = VIR_CPU_COMPARE_IDENTICAL; + +out: + ppcMapFree(map); + ppcModelFree(host_model); + ppcModelFree(guest_model); + return ret; + +error: + ret = VIR_CPU_COMPARE_ERROR; + goto out; +} + static virCPUCompareResult ppcCompare(virCPUDefPtr host, virCPUDefPtr cpu) @@ -369,11 +514,36 @@ ppcNodeData(void) } #endif +static virCPUCompareResult +ppcGuestData(virCPUDefPtr host, + virCPUDefPtr guest, + virCPUDataPtr *data, + char **message) +{ + return ppcCompute(host, guest, data, message); +} + static int -ppcUpdate(virCPUDefPtr guest ATTRIBUTE_UNUSED, - const virCPUDefPtr host ATTRIBUTE_UNUSED) +ppcUpdate(virCPUDefPtr guest, + const virCPUDefPtr host) { - return 0; + switch ((enum virCPUMode) guest->mode) { + case VIR_CPU_MODE_HOST_MODEL: + case VIR_CPU_MODE_HOST_PASSTHROUGH: + guest->match = VIR_CPU_MATCH_EXACT; + virCPUDefFreeModel(guest); + return virCPUDefCopyModel(guest, host, true); + + case VIR_CPU_MODE_CUSTOM: + return 0; + + case VIR_CPU_MODE_LAST: + break; + } + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected CPU mode: %d"), guest->mode); + return -1; } static virCPUDefPtr @@ -479,7 +649,7 @@ struct cpuArchDriver cpuDriverPowerPC = { #else .nodeData = NULL, #endif - .guestData = NULL, + .guestData = ppcGuestData, .baseline = ppcBaseline, .update = ppcUpdate, .hasFeature = NULL, -- 1.8.1.4
On Tue, Sep 03, 2013 at 02:28:24PM +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
On Power platform, Power7+ can support Power7 guest. It needs to define XML configuration to specify guest's CPU model.
For exmaple: <cpu match='exact'> <model>POWER7_v2.1</model> <vendor>IBM</vendor> </cpu>
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- src/cpu/cpu_powerpc.c | 178 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 174 insertions(+), 4 deletions(-)
ACK 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 :|
On 09/03/2013 02:28 AM, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
On Power platform, Power7+ can support Power7 guest. It needs to define XML configuration to specify guest's CPU model.
For exmaple: <cpu match='exact'> <model>POWER7_v2.1</model> <vendor>IBM</vendor> </cpu>
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- src/cpu/cpu_powerpc.c | 178 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 174 insertions(+), 4 deletions(-)
...<snip>... There were Coverity RESOURCE_LEAK's detected in this code - so while looking at those I looked at the rest of this function and had a few comments...
+ +static virCPUCompareResult +ppcCompute(virCPUDefPtr host, + const virCPUDefPtr cpu, + virCPUDataPtr *guestData, + char **message) + +{ + struct ppc_map *map = NULL; + struct ppc_model *host_model = NULL; + struct ppc_model *guest_model = NULL; + + int ret = 0;
NOTE: To be consistent should this be 'VIR_CPU_COMPARE_INCOMPATIBLE' (e.g. 0)? or better yet 'VIR_CPU_COMPARE_ERROR' (-1).
+ virArch arch; + size_t i; + + if (cpu->arch != VIR_ARCH_NONE) { + bool found = false; + + for (i = 0; i < ARRAY_CARDINALITY(archs); i++) { + if (archs[i] == cpu->arch) { + found = true; + break; + } + } + + if (!found) { + VIR_DEBUG("CPU arch %s does not match host arch", + virArchToString(cpu->arch)); + if (message && + virAsprintf(message, + _("CPU arch %s does not match host arch"), + virArchToString(cpu->arch)) < 0) + goto error; + return VIR_CPU_COMPARE_INCOMPATIBLE;
Why on a "message" do you go to error which changes the return value to ERROR, while if message isn't defined you return INCOMPATIBLE? Seems you'd want to set "ret" to INCOMPATIBLE, then goto out (or cleanup). Does the caller differentiate ERROR and INCOMPATIBLE? Does it do something different if it passed a message, but you got different return values?
+ } + arch = cpu->arch; + } else { + arch = host->arch; + } + + if (cpu->vendor && + (!host->vendor || STRNEQ(cpu->vendor, host->vendor))) { + VIR_DEBUG("host CPU vendor does not match required CPU vendor %s", + cpu->vendor); + if (message && + virAsprintf(message, + _("host CPU vendor does not match required " + "CPU vendor %s"), + cpu->vendor) < 0) + goto error; + return VIR_CPU_COMPARE_INCOMPATIBLE;
Same comment as above
+ } + + if (!(map = ppcLoadMap()) || + !(host_model = ppcModelFromCPU(host, map)) || + !(guest_model = ppcModelFromCPU(cpu, map))) + goto error;
If you initialize ret to COMPARE_ERROR, then it's a goto out (or cleanup)... Also the only way these are ever used is if (guestData != NULL), thus why not put them inside the below if condition?
+ + if (guestData != NULL) { + if (cpu->type == VIR_CPU_TYPE_GUEST && + cpu->match == VIR_CPU_MATCH_STRICT && + STRNEQ(guest_model->name, host_model->name)) { + VIR_DEBUG("host CPU model does not match required CPU model %s", + guest_model->name); + if (message && + virAsprintf(message, + _("host CPU model does not match required " + "CPU model %s"), + guest_model->name) < 0) + goto error; + return VIR_CPU_COMPARE_INCOMPATIBLE;
Coverity found a RESOURCE_LEAK here on 'host_model' and 'guest_model'. Coverity missed that 'map' is also leaked... And this would have the same comments as above regarding the goto/return
+ } + + if (!(*guestData = ppcMakeCPUData(arch, &guest_model->data))) + goto error;
Initializing ret to ERROR would mean this would be a goto out (or cleanup).
+ } + + ret = VIR_CPU_COMPARE_IDENTICAL; + +out:
I think by convention this is usually "cleanup:" John
+ ppcMapFree(map); + ppcModelFree(host_model); + ppcModelFree(guest_model); + return ret; + +error: + ret = VIR_CPU_COMPARE_ERROR; + goto out; +} +
On Fri, Sep 6, 2013 at 6:32 AM, John Ferlan <jferlan@redhat.com> wrote:
On 09/03/2013 02:28 AM, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
On Power platform, Power7+ can support Power7 guest. It needs to define XML configuration to specify guest's CPU model.
For exmaple: <cpu match='exact'> <model>POWER7_v2.1</model> <vendor>IBM</vendor> </cpu>
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- src/cpu/cpu_powerpc.c | 178 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 174 insertions(+), 4 deletions(-)
...<snip>...
There were Coverity RESOURCE_LEAK's detected in this code - so while looking at those I looked at the rest of this function and had a few comments...
+ +static virCPUCompareResult +ppcCompute(virCPUDefPtr host, + const virCPUDefPtr cpu, + virCPUDataPtr *guestData, + char **message) + +{ + struct ppc_map *map = NULL; + struct ppc_model *host_model = NULL; + struct ppc_model *guest_model = NULL; + + int ret = 0;
NOTE: To be consistent should this be 'VIR_CPU_COMPARE_INCOMPATIBLE' (e.g. 0)? or better yet 'VIR_CPU_COMPARE_ERROR' (-1).
I agree here. It should likely be VIR_CPU_COMPARE_ERROR as a good initializer. On top of that using "int" is wrong, the correct type is virCPUCompareResult.
+ virArch arch; + size_t i; + + if (cpu->arch != VIR_ARCH_NONE) { + bool found = false; + + for (i = 0; i < ARRAY_CARDINALITY(archs); i++) { + if (archs[i] == cpu->arch) { + found = true; + break; + } + } + + if (!found) { + VIR_DEBUG("CPU arch %s does not match host arch", + virArchToString(cpu->arch)); + if (message && + virAsprintf(message, + _("CPU arch %s does not match host arch"), + virArchToString(cpu->arch)) < 0) + goto error; + return VIR_CPU_COMPARE_INCOMPATIBLE;
Why on a "message" do you go to error which changes the return value to ERROR, while if message isn't defined you return INCOMPATIBLE? Seems you'd want to set "ret" to INCOMPATIBLE, then goto out (or cleanup). Does the caller differentiate ERROR and INCOMPATIBLE? Does it do something different if it passed a message, but you got different return values?
This is something copied and pasted from x86Compute() from the x86 side. Only x86 and PPC now define this function and both use the same code. If its always going to be the same behavior I suggest we life it up into cpu.c in cpuGuestData() and improve the semantics as you (John) noted.
+ } + arch = cpu->arch; + } else { + arch = host->arch; + } + + if (cpu->vendor && + (!host->vendor || STRNEQ(cpu->vendor, host->vendor))) { + VIR_DEBUG("host CPU vendor does not match required CPU vendor %s", + cpu->vendor); + if (message && + virAsprintf(message, + _("host CPU vendor does not match required " + "CPU vendor %s"), + cpu->vendor) < 0) + goto error; + return VIR_CPU_COMPARE_INCOMPATIBLE;
Same comment as above
Same for me as well. Comes from x86 again.
+ } + + if (!(map = ppcLoadMap()) || + !(host_model = ppcModelFromCPU(host, map)) || + !(guest_model = ppcModelFromCPU(cpu, map))) + goto error;
If you initialize ret to COMPARE_ERROR, then it's a goto out (or cleanup)... Also the only way these are ever used is if (guestData != NULL), thus why not put them inside the below if condition?
+ + if (guestData != NULL) { + if (cpu->type == VIR_CPU_TYPE_GUEST && + cpu->match == VIR_CPU_MATCH_STRICT && + STRNEQ(guest_model->name, host_model->name)) { + VIR_DEBUG("host CPU model does not match required CPU model %s", + guest_model->name); + if (message && + virAsprintf(message, + _("host CPU model does not match required " + "CPU model %s"), + guest_model->name) < 0) + goto error; + return VIR_CPU_COMPARE_INCOMPATIBLE;
Coverity found a RESOURCE_LEAK here on 'host_model' and 'guest_model'. Coverity missed that 'map' is also leaked...
And this would have the same comments as above regarding the goto/return
+ } + + if (!(*guestData = ppcMakeCPUData(arch, &guest_model->data))) + goto error;
Initializing ret to ERROR would mean this would be a goto out (or cleanup).
+ } + + ret = VIR_CPU_COMPARE_IDENTICAL; + +out:
I think by convention this is usually "cleanup:"
John
+ ppcMapFree(map); + ppcModelFree(host_model); + ppcModelFree(guest_model); + return ret; + +error: + ret = VIR_CPU_COMPARE_ERROR; + goto out; +} +
I agree with all of John's reviews here as well. -- Doug Goldstein
On 2013年09月06日 19:32, John Ferlan wrote:
On 09/03/2013 02:28 AM, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
On Power platform, Power7+ can support Power7 guest. It needs to define XML configuration to specify guest's CPU model.
For exmaple: <cpu match='exact'> <model>POWER7_v2.1</model> <vendor>IBM</vendor> </cpu>
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- src/cpu/cpu_powerpc.c | 178 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 174 insertions(+), 4 deletions(-)
...<snip>...
There were Coverity RESOURCE_LEAK's detected in this code - so while looking at those I looked at the rest of this function and had a few comments...
+ +static virCPUCompareResult +ppcCompute(virCPUDefPtr host, + const virCPUDefPtr cpu, + virCPUDataPtr *guestData, + char **message) + +{ + struct ppc_map *map = NULL; + struct ppc_model *host_model = NULL; + struct ppc_model *guest_model = NULL; + + int ret = 0; NOTE: To be consistent should this be 'VIR_CPU_COMPARE_INCOMPATIBLE' (e.g. 0)? or better yet 'VIR_CPU_COMPARE_ERROR' (-1). OK, I will change it.
+ virArch arch; + size_t i; + + if (cpu->arch != VIR_ARCH_NONE) { + bool found = false; + + for (i = 0; i < ARRAY_CARDINALITY(archs); i++) { + if (archs[i] == cpu->arch) { + found = true; + break; + } + } + + if (!found) { + VIR_DEBUG("CPU arch %s does not match host arch", + virArchToString(cpu->arch)); + if (message && + virAsprintf(message, + _("CPU arch %s does not match host arch"), + virArchToString(cpu->arch)) < 0) + goto error; + return VIR_CPU_COMPARE_INCOMPATIBLE; Why on a "message" do you go to error which changes the return value to ERROR, while if message isn't defined you return INCOMPATIBLE? Seems you'd want to set "ret" to INCOMPATIBLE, then goto out (or cleanup). Does the caller differentiate ERROR and INCOMPATIBLE? Does it do something different if it passed a message, but you got different return values?
On a message, if virAsprintf return value < 0, it will goto error which means some error happens. This place return INCOMPATIBLE only if (!found) not if (!message). It doesn't belong to the block of 'if(message & ...)'. This means that we can't find out any archs we support matched. So we need to report INCOMPATIBLE. We don't alloc any memory here, map, host_model and guest_model are all NULL.
+ } + arch = cpu->arch; + } else { + arch = host->arch; + } + + if (cpu->vendor && + (!host->vendor || STRNEQ(cpu->vendor, host->vendor))) { + VIR_DEBUG("host CPU vendor does not match required CPU vendor %s", + cpu->vendor); + if (message && + virAsprintf(message, + _("host CPU vendor does not match required " + "CPU vendor %s"), + cpu->vendor) < 0) + goto error; + return VIR_CPU_COMPARE_INCOMPATIBLE; Same comment as above
+ } + + if (!(map = ppcLoadMap()) || + !(host_model = ppcModelFromCPU(host, map)) || + !(guest_model = ppcModelFromCPU(cpu, map))) + goto error; If you initialize ret to COMPARE_ERROR, then it's a goto out (or cleanup)... Also the only way these are ever used is if (guestData != NULL), thus why not put them inside the below if condition?
OK, let me clean these goto clause.
+ + if (guestData != NULL) { + if (cpu->type == VIR_CPU_TYPE_GUEST && + cpu->match == VIR_CPU_MATCH_STRICT && + STRNEQ(guest_model->name, host_model->name)) { + VIR_DEBUG("host CPU model does not match required CPU model %s", + guest_model->name); + if (message && + virAsprintf(message, + _("host CPU model does not match required " + "CPU model %s"), + guest_model->name) < 0) + goto error; + return VIR_CPU_COMPARE_INCOMPATIBLE; Coverity found a RESOURCE_LEAK here on 'host_model' and 'guest_model'. Coverity missed that 'map' is also leaked...
Ah, right. There is memory leak here. Will clean this.
And this would have the same comments as above regarding the goto/return
+ } + + if (!(*guestData = ppcMakeCPUData(arch, &guest_model->data))) + goto error; Initializing ret to ERROR would mean this would be a goto out (or cleanup).
+ } + + ret = VIR_CPU_COMPARE_IDENTICAL; + +out: I think by convention this is usually "cleanup:"
John
+ ppcMapFree(map); + ppcModelFree(host_model); + ppcModelFree(guest_model); + return ret; + +error: + ret = VIR_CPU_COMPARE_ERROR; + goto out; +} +
From: Li Zhang <zhlcindy@linux.vnet.ibm.com> This patch is to add cpu test casses for PPC CPU driver. Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- tests/cputest.c | 9 +++++++++ tests/cputestdata/ppc64-baseline-1-result.xml | 3 +++ .../ppc64-baseline-incompatible-vendors.xml | 14 ++++++++++++++ .../cputestdata/ppc64-baseline-no-vendor-result.xml | 3 +++ tests/cputestdata/ppc64-baseline-no-vendor.xml | 7 +++++++ tests/cputestdata/ppc64-exact.xml | 3 +++ tests/cputestdata/ppc64-guest-nofallback.xml | 4 ++++ tests/cputestdata/ppc64-guest.xml | 4 ++++ .../ppc64-host+guest,ppc_models-result.xml | 5 +++++ ...est-nofallback,ppc_models,POWER7_v2.1-result.xml | 5 +++++ tests/cputestdata/ppc64-host.xml | 6 ++++++ tests/cputestdata/ppc64-strict.xml | 3 +++ .../qemuxml2argv-pseries-cpu-exact.args | 7 +++++++ .../qemuxml2argv-pseries-cpu-exact.xml | 21 +++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 15 files changed, 95 insertions(+) create mode 100644 tests/cputestdata/ppc64-baseline-1-result.xml create mode 100644 tests/cputestdata/ppc64-baseline-incompatible-vendors.xml create mode 100644 tests/cputestdata/ppc64-baseline-no-vendor-result.xml create mode 100644 tests/cputestdata/ppc64-baseline-no-vendor.xml create mode 100644 tests/cputestdata/ppc64-exact.xml create mode 100644 tests/cputestdata/ppc64-guest-nofallback.xml create mode 100644 tests/cputestdata/ppc64-guest.xml create mode 100644 tests/cputestdata/ppc64-host+guest,ppc_models-result.xml create mode 100644 tests/cputestdata/ppc64-host+guest-nofallback,ppc_models,POWER7_v2.1-result.xml create mode 100644 tests/cputestdata/ppc64-host.xml create mode 100644 tests/cputestdata/ppc64-strict.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-exact.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-exact.xml diff --git a/tests/cputest.c b/tests/cputest.c index 959cb9f..408a510 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -493,6 +493,7 @@ cpuTestRun(const char *name, const struct data *data) static const char *model486[] = { "486" }; static const char *nomodel[] = { "nomodel" }; static const char *models[] = { "qemu64", "core2duo", "Nehalem" }; +static const char *ppc_models[] = { "POWER7", "POWER7_v2.1", "POWER8_v1.0"}; static int mymain(void) @@ -584,6 +585,9 @@ mymain(void) DO_TEST_COMPARE("x86", "host-worse", "nehalem-force", VIR_CPU_COMPARE_IDENTICAL); DO_TEST_COMPARE("x86", "host-SandyBridge", "exact-force-Haswell", VIR_CPU_COMPARE_IDENTICAL); + DO_TEST_COMPARE("ppc64", "host", "strict", VIR_CPU_COMPARE_IDENTICAL); + DO_TEST_COMPARE("ppc64", "host", "exact", VIR_CPU_COMPARE_INCOMPATIBLE); + /* guest updates for migration * automatically compares host CPU with the result */ DO_TEST_UPDATE("x86", "host", "min", VIR_CPU_COMPARE_IDENTICAL); @@ -601,6 +605,8 @@ mymain(void) DO_TEST_BASELINE("x86", "2", 0, 0); DO_TEST_BASELINE("x86", "3", VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES, 0); + DO_TEST_BASELINE("ppc64", "incompatible-vendors", 0, -1); + DO_TEST_BASELINE("ppc64", "no-vendor", 0, 0); /* CPU features */ DO_TEST_HASFEATURE("x86", "host", "vmx", YES); DO_TEST_HASFEATURE("x86", "host", "lm", YES); @@ -627,6 +633,9 @@ mymain(void) DO_TEST_GUESTDATA("x86", "host", "host+host-model-nofallback", models, "Penryn", -1); + DO_TEST_GUESTDATA("ppc64", "host", "guest", ppc_models, NULL, 0); + DO_TEST_GUESTDATA("ppc64", "host", "guest-nofallback", ppc_models, "POWER7_v2.1", -1); + VIR_FREE(map); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/cputestdata/ppc64-baseline-1-result.xml b/tests/cputestdata/ppc64-baseline-1-result.xml new file mode 100644 index 0000000..cbdd9bc --- /dev/null +++ b/tests/cputestdata/ppc64-baseline-1-result.xml @@ -0,0 +1,3 @@ +<cpu mode='custom' match='exact'> + <model fallback='allow'>POWER7+_v2.1</model> +</cpu> diff --git a/tests/cputestdata/ppc64-baseline-incompatible-vendors.xml b/tests/cputestdata/ppc64-baseline-incompatible-vendors.xml new file mode 100644 index 0000000..97d3c9c --- /dev/null +++ b/tests/cputestdata/ppc64-baseline-incompatible-vendors.xml @@ -0,0 +1,14 @@ +<cpuTest> +<cpu> + <arch>ppc64</arch> + <model>POWER7+_v2.1</model> + <vendor>Intel</vendor> + <topology sockets='2' cores='4' threads='1'/> +</cpu> +<cpu> + <arch>ppc64</arch> + <model>POWER8_v1.0</model> + <vendor>Intel</vendor> + <topology sockets='1' cores='1' threads='1'/> +</cpu> +</cpuTest> diff --git a/tests/cputestdata/ppc64-baseline-no-vendor-result.xml b/tests/cputestdata/ppc64-baseline-no-vendor-result.xml new file mode 100644 index 0000000..36bae52 --- /dev/null +++ b/tests/cputestdata/ppc64-baseline-no-vendor-result.xml @@ -0,0 +1,3 @@ +<cpu mode='custom' match='exact'> + <model fallback='allow'>POWER7_v2.3</model> +</cpu> diff --git a/tests/cputestdata/ppc64-baseline-no-vendor.xml b/tests/cputestdata/ppc64-baseline-no-vendor.xml new file mode 100644 index 0000000..5e69a62 --- /dev/null +++ b/tests/cputestdata/ppc64-baseline-no-vendor.xml @@ -0,0 +1,7 @@ +<cpuTest> +<cpu> + <arch>ppc64</arch> + <model>POWER7_v2.3</model> + <topology sockets='2' cores='4' threads='1'/> +</cpu> +</cpuTest> diff --git a/tests/cputestdata/ppc64-exact.xml b/tests/cputestdata/ppc64-exact.xml new file mode 100644 index 0000000..c84f16a --- /dev/null +++ b/tests/cputestdata/ppc64-exact.xml @@ -0,0 +1,3 @@ +<cpu match='exact'> + <model>POWER8_v1.0</model> +</cpu> diff --git a/tests/cputestdata/ppc64-guest-nofallback.xml b/tests/cputestdata/ppc64-guest-nofallback.xml new file mode 100644 index 0000000..42026b4 --- /dev/null +++ b/tests/cputestdata/ppc64-guest-nofallback.xml @@ -0,0 +1,4 @@ +<cpu match='exact'> + <model fallback='forbid'>POWER7_v2.1</model> + <topology sockets='2' cores='4' threads='1'/> +</cpu> diff --git a/tests/cputestdata/ppc64-guest.xml b/tests/cputestdata/ppc64-guest.xml new file mode 100644 index 0000000..ac81ec0 --- /dev/null +++ b/tests/cputestdata/ppc64-guest.xml @@ -0,0 +1,4 @@ +<cpu match='exact'> + <model>POWER8_v1.0</model> + <topology sockets='2' cores='4' threads='1'/> +</cpu> diff --git a/tests/cputestdata/ppc64-host+guest,ppc_models-result.xml b/tests/cputestdata/ppc64-host+guest,ppc_models-result.xml new file mode 100644 index 0000000..0cb0830 --- /dev/null +++ b/tests/cputestdata/ppc64-host+guest,ppc_models-result.xml @@ -0,0 +1,5 @@ +<cpu mode='custom' match='exact'> + <arch>ppc64</arch> + <model fallback='allow'>POWER8_v1.0</model> + <vendor>IBM</vendor> +</cpu> diff --git a/tests/cputestdata/ppc64-host+guest-nofallback,ppc_models,POWER7_v2.1-result.xml b/tests/cputestdata/ppc64-host+guest-nofallback,ppc_models,POWER7_v2.1-result.xml new file mode 100644 index 0000000..7e58361 --- /dev/null +++ b/tests/cputestdata/ppc64-host+guest-nofallback,ppc_models,POWER7_v2.1-result.xml @@ -0,0 +1,5 @@ +<cpu mode='custom' match='exact'> + <arch>ppc64</arch> + <model fallback='forbid'>POWER7_v2.1</model> + <vendor>IBM</vendor> +</cpu> diff --git a/tests/cputestdata/ppc64-host.xml b/tests/cputestdata/ppc64-host.xml new file mode 100644 index 0000000..39cb741 --- /dev/null +++ b/tests/cputestdata/ppc64-host.xml @@ -0,0 +1,6 @@ +<cpu> + <arch>ppc64</arch> + <model>POWER7_v2.3</model> + <vendor>IBM</vendor> + <topology sockets='1' cores='64' threads='1'/> +</cpu> diff --git a/tests/cputestdata/ppc64-strict.xml b/tests/cputestdata/ppc64-strict.xml new file mode 100644 index 0000000..e91c6e7 --- /dev/null +++ b/tests/cputestdata/ppc64-strict.xml @@ -0,0 +1,3 @@ +<cpu match='exact'> + <model>POWER7_v2.3</model> +</cpu> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-exact.args b/tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-exact.args new file mode 100644 index 0000000..1e09680 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-exact.args @@ -0,0 +1,7 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ +/usr/bin/qemu-system-ppc64 -S -M pseries -cpu POWER7_v2.3 -m 512 -smp 1 -nographic \ +-nodefconfig -nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -usb \ +-chardev pty,id=charserial0 \ +-device spapr-vty,chardev=charserial0,reg=0x30000000 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-exact.xml b/tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-exact.xml new file mode 100644 index 0000000..b54dae2 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-exact.xml @@ -0,0 +1,21 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + </os> + <cpu match='exact'> + <model>POWER7_v2.3</model> + <vendor>IBM</vendor> + </cpu> + <clock offset='utc'/> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <console type='pty'> + <address type="spapr-vio"/> + </console> + <memballoon model="none"/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 4e3508b..89ebabb 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -950,6 +950,7 @@ mymain(void) DO_TEST_ERROR("pseries-vio-address-clash", QEMU_CAPS_DRIVE, QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); DO_TEST("pseries-nvram", QEMU_CAPS_DEVICE_NVRAM); + DO_TEST_FAILURE("pseries-cpu-exact", QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); DO_TEST("disk-ide-drive-split", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_IDE_CD); -- 1.8.1.4
On Tue, Sep 03, 2013 at 02:28:25PM +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
This patch is to add cpu test casses for PPC CPU driver.
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- tests/cputest.c | 9 +++++++++ tests/cputestdata/ppc64-baseline-1-result.xml | 3 +++ .../ppc64-baseline-incompatible-vendors.xml | 14 ++++++++++++++ .../cputestdata/ppc64-baseline-no-vendor-result.xml | 3 +++ tests/cputestdata/ppc64-baseline-no-vendor.xml | 7 +++++++ tests/cputestdata/ppc64-exact.xml | 3 +++ tests/cputestdata/ppc64-guest-nofallback.xml | 4 ++++ tests/cputestdata/ppc64-guest.xml | 4 ++++ .../ppc64-host+guest,ppc_models-result.xml | 5 +++++ ...est-nofallback,ppc_models,POWER7_v2.1-result.xml | 5 +++++ tests/cputestdata/ppc64-host.xml | 6 ++++++ tests/cputestdata/ppc64-strict.xml | 3 +++ .../qemuxml2argv-pseries-cpu-exact.args | 7 +++++++ .../qemuxml2argv-pseries-cpu-exact.xml | 21 +++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 15 files changed, 95 insertions(+) create mode 100644 tests/cputestdata/ppc64-baseline-1-result.xml create mode 100644 tests/cputestdata/ppc64-baseline-incompatible-vendors.xml create mode 100644 tests/cputestdata/ppc64-baseline-no-vendor-result.xml create mode 100644 tests/cputestdata/ppc64-baseline-no-vendor.xml create mode 100644 tests/cputestdata/ppc64-exact.xml create mode 100644 tests/cputestdata/ppc64-guest-nofallback.xml create mode 100644 tests/cputestdata/ppc64-guest.xml create mode 100644 tests/cputestdata/ppc64-host+guest,ppc_models-result.xml create mode 100644 tests/cputestdata/ppc64-host+guest-nofallback,ppc_models,POWER7_v2.1-result.xml create mode 100644 tests/cputestdata/ppc64-host.xml create mode 100644 tests/cputestdata/ppc64-strict.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-exact.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-exact.xml
ACK 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 :|
Verified-by: Shivaprasad The cpu model to pvr value mappings are POWER7 -> '0x003f0200' POWER7_v2.1 -> '0x003f0201' POWER7_v2.3 -> '0x003f0203' POWER7+_v2.1 -> '0x004a0201' POWER8_v1.0 -> '0x004b0100' Verified on, libvirt version: 1.1.2 Host : Fedora 19 - 3.11.0-rc7+ Guest : Fedora 19 - 3.9.2-301.fc19.ppc64p7 qemu - version 1.6.0 Test Results : On Host : [root@ltcfbl9cb libvirt]# cat ../usr/etc/libvirt/qemu/shiv.xml | grep -e cpu -e model <vcpu placement='static'>16</vcpu> <cpu mode='custom' match='exact'> <model fallback='allow'>POWER7_v2.1</model> </cpu> [root@ltcfbl9cb libvirt]# ./run ./tools/virsh list --all Id Name State ---------------------------------------------------- 13 shiv running The qemu CLI has the -cpu option [root@ltcfbl9cb libvirt]# ps -aef | grep qemu root 38478 1 3 05:38 ? 00:01:45 /usr/bin/qemu-kvm -name shiv -S -machine pseries,accel=kvm,usb=off -cpu POWER7_v2.1 -m 1024 -realtime mlock=off -smp 16,sockets=16,cores=1,threads=1 -uuid 938df102-5f9a-1d53-60f8-... On Guest : [root@localhost ~]# dtc -I fs /proc/device-tree/ | grep POWER PowerPC,POWER7_V2.1@3c { [root@localhost ~]# dtc -I fs /proc/device-tree/ | grep cpu-versio[ cpu-version = <0x3f0201>; ================== On Host : [root@ltcfbl9cb libvirt]# ./run ./tools/virsh list Id Name State ---------------------------------------------------- 5 shiv running [root@ltcfbl9cb libvirt]# cat ../usr/etc/libvirt/qemu/shiv.xml | grep -e cpu -e model <vcpu placement='static'>16</vcpu> <cpu mode='custom' match='exact'> <model fallback='allow'>POWER7+_v2.1</model> </cpu> [root@localhost ~]# dtc -I fs /proc/device-tree/ | grep cpu-version cpu-version = <0x4a0201>; [root@localhost ~]# dtc -I fs /proc/device-tree/ | grep POWER PowerPC,POWER7+_V2.1@3c { ==================================
-------- Original Message -------- Subject: [libvirt][PATCH v2 0/3] Improve PPC CPU driver Date: Tue, 3 Sep 2013 14:28:22 +0800 From: Li Zhang <zhlcindy@gmail.com> To: libvir-list@redhat.com, jdenemar@redhat.com CC: bpradip@in.ibm.com, Li Zhang <zhlcindy@linux.vnet.ibm.com>
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
Currently, PPC CPU driver doesn't support to parse guest data. It can't pass CPU parameters to Qemu command line.
This patchset is to implement .guestData to support to parse guest CPU configuration and .update to support host-model and host-passthrough.
The guest CPU configuration is as the following: <cpu match='exact'> <model>POWER7_v2.3</model> <vendor>IBM</vendor> </cpu>
v2 -> v1: * Remove features functions calling for non-x86 platform (Doug Goldstein) * Improve update code. * Merge update code with guestData.
Li Zhang (3): Remove CPU features check for non-x86 platform. cpu: Implement guestData and update for PPC cpu: Add cpu test cases for PPC CPU driver.
src/cpu/cpu_powerpc.c | 178 ++++++++++++++++++++- src/qemu/qemu_command.c | 16 +- tests/cputest.c | 9 ++ tests/cputestdata/ppc64-baseline-1-result.xml | 3 + .../ppc64-baseline-incompatible-vendors.xml | 14 ++ .../ppc64-baseline-no-vendor-result.xml | 3 + tests/cputestdata/ppc64-baseline-no-vendor.xml | 7 + tests/cputestdata/ppc64-exact.xml | 3 + tests/cputestdata/ppc64-guest-nofallback.xml | 4 + tests/cputestdata/ppc64-guest.xml | 4 + .../ppc64-host+guest,ppc_models-result.xml | 5 + ...st-nofallback,ppc_models,POWER7_v2.1-result.xml | 5 + tests/cputestdata/ppc64-host.xml | 6 + tests/cputestdata/ppc64-strict.xml | 3 + .../qemuxml2argv-pseries-cpu-exact.args | 7 + .../qemuxml2argv-pseries-cpu-exact.xml | 21 +++ tests/qemuxml2argvtest.c | 1 + 17 files changed, 279 insertions(+), 10 deletions(-) create mode 100644 tests/cputestdata/ppc64-baseline-1-result.xml create mode 100644 tests/cputestdata/ppc64-baseline-incompatible-vendors.xml create mode 100644 tests/cputestdata/ppc64-baseline-no-vendor-result.xml create mode 100644 tests/cputestdata/ppc64-baseline-no-vendor.xml create mode 100644 tests/cputestdata/ppc64-exact.xml create mode 100644 tests/cputestdata/ppc64-guest-nofallback.xml create mode 100644 tests/cputestdata/ppc64-guest.xml create mode 100644 tests/cputestdata/ppc64-host +guest,ppc_models-result.xml create mode 100644 tests/cputestdata/ppc64-host +guest-nofallback,ppc_models,POWER7_v2.1-result.xml create mode 100644 tests/cputestdata/ppc64-host.xml create mode 100644 tests/cputestdata/ppc64-strict.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-exact.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-exact.xml
Verified-by: Shivaprasad The cpu model to pvr value mappings are POWER7 -> '0x003f0200' POWER7_v2.1 -> '0x003f0201' POWER7_v2.3 -> '0x003f0203' POWER7+_v2.1 -> '0x004a0201' POWER8_v1.0 -> '0x004b0100' Verified on, libvirt version: 1.1.2 Host : Fedora 19 - 3.11.0-rc7+ Guest : Fedora 19 - 3.9.2-301.fc19.ppc64p7 qemu - version 1.6.0 Test Results : On Host : [root@ltcfbl9cb libvirt]# cat ../usr/etc/libvirt/qemu/shiv.xml | grep -e cpu -e model <vcpu placement='static'>16</vcpu> <cpu mode='custom' match='exact'> <model fallback='allow'>POWER7_v2.1</model> </cpu> [root@ltcfbl9cb libvirt]# ./run ./tools/virsh list --all Id Name State ---------------------------------------------------- 13 shiv running The qemu CLI has the -cpu option [root@ltcfbl9cb libvirt]# ps -aef | grep qemu root 38478 1 3 05:38 ? 00:01:45 /usr/bin/qemu-kvm -name shiv -S -machine pseries,accel=kvm,usb=off -cpu POWER7_v2.1 -m 1024 -realtime mlock=off -smp 16,sockets=16,cores=1,threads=1 -uuid 938df102-5f9a-1d53-60f8-... On Guest : [root@localhost ~]# dtc -I fs /proc/device-tree/ | grep POWER PowerPC,POWER7_V2.1@3c { [root@localhost ~]# dtc -I fs /proc/device-tree/ | grep cpu-versio[ cpu-version = <0x3f0201>; ================== On Host : [root@ltcfbl9cb libvirt]# ./run ./tools/virsh list Id Name State ---------------------------------------------------- 5 shiv running [root@ltcfbl9cb libvirt]# cat ../usr/etc/libvirt/qemu/shiv.xml | grep -e cpu -e model <vcpu placement='static'>16</vcpu> <cpu mode='custom' match='exact'> <model fallback='allow'>POWER7+_v2.1</model> </cpu> [root@localhost ~]# dtc -I fs /proc/device-tree/ | grep cpu-version cpu-version = <0x4a0201>; [root@localhost ~]# dtc -I fs /proc/device-tree/ | grep POWER PowerPC,POWER7+_V2.1@3c { ==================================
-------- Original Message -------- Subject: [libvirt][PATCH v2 0/3] Improve PPC CPU driver Date: Tue, 3 Sep 2013 14:28:22 +0800 From: Li Zhang <zhlcindy@gmail.com> To: libvir-list@redhat.com, jdenemar@redhat.com CC: bpradip@in.ibm.com, Li Zhang <zhlcindy@linux.vnet.ibm.com>
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
Currently, PPC CPU driver doesn't support to parse guest data. It can't pass CPU parameters to Qemu command line.
This patchset is to implement .guestData to support to parse guest CPU configuration and .update to support host-model and host-passthrough.
The guest CPU configuration is as the following: <cpu match='exact'> <model>POWER7_v2.3</model> <vendor>IBM</vendor> </cpu>
v2 -> v1: * Remove features functions calling for non-x86 platform (Doug Goldstein) * Improve update code. * Merge update code with guestData.
Li Zhang (3): Remove CPU features check for non-x86 platform. cpu: Implement guestData and update for PPC cpu: Add cpu test cases for PPC CPU driver.
src/cpu/cpu_powerpc.c | 178 ++++++++++++++++++++- src/qemu/qemu_command.c | 16 +- tests/cputest.c | 9 ++ tests/cputestdata/ppc64-baseline-1-result.xml | 3 + .../ppc64-baseline-incompatible-vendors.xml | 14 ++ .../ppc64-baseline-no-vendor-result.xml | 3 + tests/cputestdata/ppc64-baseline-no-vendor.xml | 7 + tests/cputestdata/ppc64-exact.xml | 3 + tests/cputestdata/ppc64-guest-nofallback.xml | 4 + tests/cputestdata/ppc64-guest.xml | 4 + .../ppc64-host+guest,ppc_models-result.xml | 5 + ...st-nofallback,ppc_models,POWER7_v2.1-result.xml | 5 + tests/cputestdata/ppc64-host.xml | 6 + tests/cputestdata/ppc64-strict.xml | 3 + .../qemuxml2argv-pseries-cpu-exact.args | 7 + .../qemuxml2argv-pseries-cpu-exact.xml | 21 +++ tests/qemuxml2argvtest.c | 1 + 17 files changed, 279 insertions(+), 10 deletions(-) create mode 100644 tests/cputestdata/ppc64-baseline-1-result.xml create mode 100644 tests/cputestdata/ppc64-baseline-incompatible-vendors.xml create mode 100644 tests/cputestdata/ppc64-baseline-no-vendor-result.xml create mode 100644 tests/cputestdata/ppc64-baseline-no-vendor.xml create mode 100644 tests/cputestdata/ppc64-exact.xml create mode 100644 tests/cputestdata/ppc64-guest-nofallback.xml create mode 100644 tests/cputestdata/ppc64-guest.xml create mode 100644 tests/cputestdata/ppc64-host +guest,ppc_models-result.xml create mode 100644 tests/cputestdata/ppc64-host +guest-nofallback,ppc_models,POWER7_v2.1-result.xml create mode 100644 tests/cputestdata/ppc64-host.xml create mode 100644 tests/cputestdata/ppc64-strict.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-exact.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-exact.xml
ping ? On 2013年09月03日 14:28, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
Currently, PPC CPU driver doesn't support to parse guest data. It can't pass CPU parameters to Qemu command line.
This patchset is to implement .guestData to support to parse guest CPU configuration and .update to support host-model and host-passthrough.
The guest CPU configuration is as the following: <cpu match='exact'> <model>POWER7_v2.3</model> <vendor>IBM</vendor> </cpu>
v2 -> v1: * Remove features functions calling for non-x86 platform (Doug Goldstein) * Improve update code. * Merge update code with guestData.
Li Zhang (3): Remove CPU features check for non-x86 platform. cpu: Implement guestData and update for PPC cpu: Add cpu test cases for PPC CPU driver.
src/cpu/cpu_powerpc.c | 178 ++++++++++++++++++++- src/qemu/qemu_command.c | 16 +- tests/cputest.c | 9 ++ tests/cputestdata/ppc64-baseline-1-result.xml | 3 + .../ppc64-baseline-incompatible-vendors.xml | 14 ++ .../ppc64-baseline-no-vendor-result.xml | 3 + tests/cputestdata/ppc64-baseline-no-vendor.xml | 7 + tests/cputestdata/ppc64-exact.xml | 3 + tests/cputestdata/ppc64-guest-nofallback.xml | 4 + tests/cputestdata/ppc64-guest.xml | 4 + .../ppc64-host+guest,ppc_models-result.xml | 5 + ...st-nofallback,ppc_models,POWER7_v2.1-result.xml | 5 + tests/cputestdata/ppc64-host.xml | 6 + tests/cputestdata/ppc64-strict.xml | 3 + .../qemuxml2argv-pseries-cpu-exact.args | 7 + .../qemuxml2argv-pseries-cpu-exact.xml | 21 +++ tests/qemuxml2argvtest.c | 1 + 17 files changed, 279 insertions(+), 10 deletions(-) create mode 100644 tests/cputestdata/ppc64-baseline-1-result.xml create mode 100644 tests/cputestdata/ppc64-baseline-incompatible-vendors.xml create mode 100644 tests/cputestdata/ppc64-baseline-no-vendor-result.xml create mode 100644 tests/cputestdata/ppc64-baseline-no-vendor.xml create mode 100644 tests/cputestdata/ppc64-exact.xml create mode 100644 tests/cputestdata/ppc64-guest-nofallback.xml create mode 100644 tests/cputestdata/ppc64-guest.xml create mode 100644 tests/cputestdata/ppc64-host+guest,ppc_models-result.xml create mode 100644 tests/cputestdata/ppc64-host+guest-nofallback,ppc_models,POWER7_v2.1-result.xml create mode 100644 tests/cputestdata/ppc64-host.xml create mode 100644 tests/cputestdata/ppc64-strict.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-exact.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-exact.xml
participants (5)
-
Daniel P. Berrange -
Doug Goldstein -
John Ferlan -
Li Zhang -
Shivaprasad G Bhat