[libvirt] [PATCH 0/8] Fix PowerPC CPU driver
Current PowerPC driver is too complicated and mostly wrong as it was apparently created from x86 driver and not properly addapted for PowerPC architecture. More details about the issues are described in individual patches. Jiri Denemark (8): cpu: Introduce cpuModelIsAllowed internal API cpu: Make comparing PowerPC CPUs easier to read cpu: Fix PowerPCNodeData cpu: Fix loading PowerPC vendor from cpu_map.xml cpu: Reimplement PowerPCBaseline cpu: Reimplement PowerPCDecode cpu: Remove hardcoded list of PowerPC models cpu: Rename PowerPCUpdate and PowerPCDataFree functions src/cpu/cpu.c | 17 ++ src/cpu/cpu.h | 5 + src/cpu/cpu_generic.c | 19 +-- src/cpu/cpu_map.xml | 12 +- src/cpu/cpu_powerpc.c | 462 +++++++++++++++++--------------------------------- src/cpu/cpu_x86.c | 11 +- 6 files changed, 193 insertions(+), 333 deletions(-) -- 1.8.0.2
The API can be used to check if the model is on the supported models list, which needs to be done in several places. --- src/cpu/cpu.c | 17 +++++++++++++++++ src/cpu/cpu.h | 5 +++++ src/cpu/cpu_generic.c | 19 +++++-------------- src/cpu/cpu_x86.c | 11 +---------- 4 files changed, 28 insertions(+), 24 deletions(-) diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index 53c4cc3..b41fb38 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -442,3 +442,20 @@ cpuHasFeature(virArch arch, return driver->hasFeature(data, feature); } + +bool +cpuModelIsAllowed(const char *model, + const char **models, + unsigned int nmodels) +{ + unsigned int i; + + if (!models || !nmodels) + return true; + + for (i = 0; i < nmodels; i++) { + if (models[i] && STREQ(models[i], model)) + return true; + } + return false; +} diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index 5153b62..7b645f7 100644 --- a/src/cpu/cpu.h +++ b/src/cpu/cpu.h @@ -163,4 +163,9 @@ cpuHasFeature(virArch arch, const char *feature); +bool +cpuModelIsAllowed(const char *model, + const char **models, + unsigned int nmodels); + #endif /* __VIR_CPU_H__ */ diff --git a/src/cpu/cpu_generic.c b/src/cpu/cpu_generic.c index b10ff47..44440bd 100644 --- a/src/cpu/cpu_generic.c +++ b/src/cpu/cpu_generic.c @@ -123,20 +123,11 @@ genericBaseline(virCPUDefPtr *cpus, unsigned int count; unsigned int i, j; - if (models) { - bool found = false; - for (i = 0; i < nmodels; i++) { - if (STREQ(cpus[0]->model, models[i])) { - found = true; - break; - } - } - if (!found) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("CPU model '%s' is not support by hypervisor"), - cpus[0]->model); - goto error; - } + if (!cpuModelIsAllowed(cpus[0]->model, models, nmodels)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("CPU model %s is not supported by hypervisor"), + cpus[0]->model); + goto error; } if (VIR_ALLOC(cpu) < 0 || diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index cb21910..ac00b3c 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1325,16 +1325,7 @@ x86Decode(virCPUDefPtr cpu, candidate = map->models; while (candidate != NULL) { - bool allowed = (models == NULL); - - for (i = 0; i < nmodels; i++) { - if (models && models[i] && STREQ(models[i], candidate->name)) { - allowed = true; - break; - } - } - - if (!allowed) { + if (!cpuModelIsAllowed(candidate->name, models, nmodels)) { if (preferred && STREQ(candidate->name, preferred)) { if (cpu->fallback != VIR_CPU_FALLBACK_ALLOW) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, -- 1.8.0.2
On 12/20/2012 05:01 PM, Jiri Denemark wrote:
The API can be used to check if the model is on the supported models list, which needs to be done in several places. --- src/cpu/cpu.c | 17 +++++++++++++++++ src/cpu/cpu.h | 5 +++++ src/cpu/cpu_generic.c | 19 +++++-------------- src/cpu/cpu_x86.c | 11 +---------- 4 files changed, 28 insertions(+), 24 deletions(-)
diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index 53c4cc3..b41fb38 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -442,3 +442,20 @@ cpuHasFeature(virArch arch,
return driver->hasFeature(data, feature); } + +bool +cpuModelIsAllowed(const char *model, + const char **models, + unsigned int nmodels)
Is size_t any better than unsigned int for nmodels?
+{ + unsigned int i; + + if (!models || !nmodels) + return true;
Should this case be false? Or more specifically, in the old code...
+ + for (i = 0; i < nmodels; i++) { + if (models[i] && STREQ(models[i], model)) + return true; + } + return false; +}
+++ b/src/cpu/cpu_generic.c @@ -123,20 +123,11 @@ genericBaseline(virCPUDefPtr *cpus, unsigned int count; unsigned int i, j;
- if (models) {
!models skipped the error message, but allocated models with nmodels==0 errored out. You have a silent change in behavior by not erroring where you used to; meanwhile, if you return false instead of true for both branches of the ||, you would have a change in behavior of erroring where you previously did not. I think that our current mix of half-and-half erroring is not useful, but it's probably worth deciding what we meant, and documenting in the commit message that the change in error policy for (!models || !nmodels) is intentional.
- bool found = false; - for (i = 0; i < nmodels; i++) { - if (STREQ(cpus[0]->model, models[i])) { - found = true; - break; - } - } - if (!found) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("CPU model '%s' is not support by hypervisor"), - cpus[0]->model); - goto error; - } + if (!cpuModelIsAllowed(cpus[0]->model, models, nmodels)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("CPU model %s is not supported by hypervisor"), + cpus[0]->model); + goto error; }
+++ b/src/cpu/cpu_x86.c @@ -1325,16 +1325,7 @@ x86Decode(virCPUDefPtr cpu,
candidate = map->models; while (candidate != NULL) { - bool allowed = (models == NULL); - - for (i = 0; i < nmodels; i++) { - if (models && models[i] && STREQ(models[i], candidate->name)) { - allowed = true; - break; - } - }
Hmm, here the behavior was different for (!models || !nmodels).
- - if (!allowed) { + if (!cpuModelIsAllowed(candidate->name, models, nmodels)) { if (preferred && STREQ(candidate->name, preferred)) { if (cpu->fallback != VIR_CPU_FALLBACK_ALLOW) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Revert the condition to make it easier to read. The function is also renamed as ppcCompare to match other functions in PowerPC CPU driver. --- src/cpu/cpu_powerpc.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c index ac10789..832ec97 100644 --- a/src/cpu/cpu_powerpc.c +++ b/src/cpu/cpu_powerpc.c @@ -414,15 +414,14 @@ no_memory: } static virCPUCompareResult -PowerPCCompare(virCPUDefPtr host, +ppcCompare(virCPUDefPtr host, virCPUDefPtr cpu) { - if ((cpu->arch != VIR_ARCH_NONE && - (host->arch != cpu->arch)) || - STRNEQ(host->model, cpu->model)) - return VIR_CPU_COMPARE_INCOMPATIBLE; + if ((cpu->arch == VIR_ARCH_NONE || host->arch == cpu->arch) && + STREQ(host->model, cpu->model)) + return VIR_CPU_COMPARE_IDENTICAL; - return VIR_CPU_COMPARE_IDENTICAL; + return VIR_CPU_COMPARE_INCOMPATIBLE; } static int @@ -630,7 +629,7 @@ struct cpuArchDriver cpuDriverPowerPC = { .name = "ppc64", .arch = archs, .narch = ARRAY_CARDINALITY(archs), - .compare = PowerPCCompare, + .compare = ppcCompare, .decode = PowerPCDecode, .encode = NULL, .free = PowerPCDataFree, -- 1.8.0.2
On 12/20/2012 05:01 PM, Jiri Denemark wrote:
Revert the condition to make it easier to read. The function is also
s/Revert/Invert/
renamed as ppcCompare to match other functions in PowerPC CPU driver. ---
src/cpu/cpu_powerpc.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) - if ((cpu->arch != VIR_ARCH_NONE && - (host->arch != cpu->arch)) || - STRNEQ(host->model, cpu->model)) - return VIR_CPU_COMPARE_INCOMPATIBLE; + if ((cpu->arch == VIR_ARCH_NONE || host->arch == cpu->arch) && + STREQ(host->model, cpu->model)) + return VIR_CPU_COMPARE_IDENTICAL;
- return VIR_CPU_COMPARE_IDENTICAL; + return VIR_CPU_COMPARE_INCOMPATIBLE;
Took me a bit, but I agree that deMorgan's law was properly followed and the logic is unchanged. ACK. -- -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Make getting node CPU data for PowerPC unsupported on other architectures. The function is also renamed as ppcNodeData to match other functions in PowerPC CPU driver. --- src/cpu/cpu_powerpc.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c index 832ec97..d5e9dd4 100644 --- a/src/cpu/cpu_powerpc.c +++ b/src/cpu/cpu_powerpc.c @@ -525,16 +525,6 @@ out: return ret; } -#if defined(__powerpc__) || \ - defined(__powerpc64__) -static uint32_t ppc_mfpvr(void) -{ - uint32_t pvr; - asm("mfpvr %0" - : "=r"(pvr)); - return pvr; -} -#endif static void PowerPCDataFree(union cpuData *data) @@ -545,8 +535,9 @@ PowerPCDataFree(union cpuData *data) VIR_FREE(data); } +#if defined(__powerpc__) || defined(__powerpc64__) static union cpuData * -PowerPCNodeData(void) +ppcNodeData(void) { union cpuData *data; @@ -555,13 +546,12 @@ PowerPCNodeData(void) return NULL; } -#if defined(__powerpc__) || \ - defined(__powerpc64__) - data->ppc.pvr = ppc_mfpvr(); -#endif + asm("mfpvr %0" + : "=r" (data->ppc.pvr)); return data; } +#endif static int PowerPCUpdate(virCPUDefPtr guest ATTRIBUTE_UNUSED, @@ -633,7 +623,11 @@ struct cpuArchDriver cpuDriverPowerPC = { .decode = PowerPCDecode, .encode = NULL, .free = PowerPCDataFree, - .nodeData = PowerPCNodeData, +#if defined(__powerpc__) || defined(__powerpc64__) + .nodeData = ppcNodeData, +#else + .nodeData = NULL, +#endif .guestData = NULL, .baseline = PowerPCBaseline, .update = PowerPCUpdate, -- 1.8.0.2
On 12/20/2012 05:01 PM, Jiri Denemark wrote:
Make getting node CPU data for PowerPC unsupported on other architectures. The function is also renamed as ppcNodeData to match other functions in PowerPC CPU driver. --- src/cpu/cpu_powerpc.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-)
@@ -633,7 +623,11 @@ struct cpuArchDriver cpuDriverPowerPC = { .decode = PowerPCDecode, .encode = NULL, .free = PowerPCDataFree, - .nodeData = PowerPCNodeData, +#if defined(__powerpc__) || defined(__powerpc64__) + .nodeData = ppcNodeData, +#else + .nodeData = NULL, +#endif
Technically, you can omit the #else branch, as all members of C99 initialization that are not specified are left 0-initialized; but I'm okay if you don't want to change this. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
When ppcVendorLoad fails to parse the vendor element for whatever reason, it is supposed to ignore it and return 0 rather than -1. The patch also removes PowerPC vendor string from the XML as it is not actually used for anything. --- src/cpu/cpu_map.xml | 7 ++++--- src/cpu/cpu_powerpc.c | 30 +++++++++--------------------- 2 files changed, 13 insertions(+), 24 deletions(-) diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index eb69a34..ba8c17d 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -585,9 +585,10 @@ </model> </arch> <arch name='ppc64'> - <!-- vendor definitions --> - <vendor name='IBM' string='PowerPC'/> - <!-- IBM-based CPU models --> + <!-- vendor definitions --> + <vendor name='IBM'/> + + <!-- IBM-based CPU models --> <model name='POWER7'> <vendor name='IBM'/> </model> diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c index d5e9dd4..3fa98bd 100644 --- a/src/cpu/cpu_powerpc.c +++ b/src/cpu/cpu_powerpc.c @@ -203,11 +203,11 @@ ppcVendorLoad(xmlXPathContextPtr ctxt, struct ppc_map *map) { struct ppc_vendor *vendor = NULL; - char *string = NULL; - int ret = -1; - if (VIR_ALLOC(vendor) < 0) - goto no_memory; + if (VIR_ALLOC(vendor) < 0) { + virReportOOMError(); + return -1; + } vendor->name = virXPathString("string(@name)", ctxt); if (!vendor->name) { @@ -222,31 +222,19 @@ ppcVendorLoad(xmlXPathContextPtr ctxt, goto ignore; } - string = virXPathString("string(@string)", ctxt); - if (!string) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Missing vendor string for CPU vendor %s"), vendor->name); - goto ignore; - } - if (!map->vendors) + if (!map->vendors) { map->vendors = vendor; - else { + } else { vendor->next = map->vendors; map->vendors = vendor; } - ret = 0; - -out: - VIR_FREE(string); - return ret; - -no_memory: - virReportOOMError(); +cleanup: + return 0; ignore: ppcVendorFree(vendor); - goto out; + goto cleanup; } static int -- 1.8.0.2
On 12/20/2012 05:01 PM, Jiri Denemark wrote:
When ppcVendorLoad fails to parse the vendor element for whatever reason, it is supposed to ignore it and return 0 rather than -1. The patch also removes PowerPC vendor string from the XML as it is not actually used for anything. --- src/cpu/cpu_map.xml | 7 ++++--- src/cpu/cpu_powerpc.c | 30 +++++++++--------------------- 2 files changed, 13 insertions(+), 24 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Baseline API is supposed to return guest CPU definition that can be used on any of the provided host CPUs. Since PowerPC CPUs are either identical or incompatible, the API just needs to check that all provided CPUs are identical. Previous implementation was completely bogus. The function is also renamed as ppcBaseline to match other functions in PowerPC CPU driver. --- src/cpu/cpu_powerpc.c | 130 +++++++++++++++++++++++--------------------------- 1 file changed, 61 insertions(+), 69 deletions(-) diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c index 3fa98bd..3722fc0 100644 --- a/src/cpu/cpu_powerpc.c +++ b/src/cpu/cpu_powerpc.c @@ -360,47 +360,6 @@ error: return NULL; } -static struct ppc_model * -ppcModelCopy(const struct ppc_model *model) -{ - struct ppc_model *copy; - - if (VIR_ALLOC(copy) < 0 - || VIR_ALLOC(copy->data) < 0 - || !(copy->name = strdup(model->name))){ - ppcModelFree(copy); - return NULL; - } - - copy->data->ppc.pvr = model->data->ppc.pvr; - copy->vendor = model->vendor; - - return copy; -} - -static struct ppc_model * -ppcModelFromCPU(const virCPUDefPtr cpu, - const struct ppc_map *map) -{ - struct ppc_model *model = NULL; - - if ((model = ppcModelFind(map, cpu->model))) { - if ((model = ppcModelCopy(model)) == NULL) { - goto no_memory; - } - } else if (!(model = ppcModelNew())) { - goto no_memory; - } - - return model; - -no_memory: - virReportOOMError(); - ppcModelFree(model); - - return NULL; -} - static virCPUCompareResult ppcCompare(virCPUDefPtr host, virCPUDefPtr cpu) @@ -548,56 +507,89 @@ PowerPCUpdate(virCPUDefPtr guest ATTRIBUTE_UNUSED, return 0; } static virCPUDefPtr -PowerPCBaseline(virCPUDefPtr *cpus, - unsigned int ncpus ATTRIBUTE_UNUSED, - const char **models ATTRIBUTE_UNUSED, - unsigned int nmodels ATTRIBUTE_UNUSED) +ppcBaseline(virCPUDefPtr *cpus, + unsigned int ncpus, + const char **models, + unsigned int nmodels) { struct ppc_map *map = NULL; - struct ppc_model *base_model = NULL; + const struct ppc_model *model; + const struct ppc_vendor *vendor = NULL; virCPUDefPtr cpu = NULL; - struct ppc_model *model = NULL; - bool outputModel = true; + unsigned int i; + + if (!(map = ppcLoadMap())) + goto error; - if (!(map = ppcLoadMap())) { + if (!(model = ppcModelFind(map, cpus[0]->model))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown CPU model %s"), cpus[0]->model); goto error; } - if (!(base_model = ppcModelFromCPU(cpus[0], map))) { + if (!cpuModelIsAllowed(model->name, models, nmodels)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("CPU model %s is not supported by hypervisor"), + model->name); goto error; } - if (VIR_ALLOC(cpu) < 0) - goto no_memory; + for (i = 0; i < ncpus; i++) { + const struct ppc_vendor *vnd; - cpu->arch = cpus[0]->arch; - cpu->type = VIR_CPU_TYPE_GUEST; - cpu->match = VIR_CPU_MATCH_EXACT; + if (STRNEQ(cpus[i]->model, model->name)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("CPUs are incompatible")); + goto error; + } - if (!cpus[0]->model) { - outputModel = false; - } else if (!(model = ppcModelFind(map, cpus[0]->model))) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("Unknown CPU vendor %s"), cpus[0]->model); - goto error; + if (!cpus[i]->vendor) + continue; + + if (!(vnd = ppcVendorFind(map, cpus[i]->vendor))) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Unknown CPU vendor %s"), cpus[i]->vendor); + goto error; + } + + if (model->vendor) { + if (model->vendor != vnd) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("CPU vendor %s of model %s differs from " + "vendor %s"), + model->vendor->name, model->name, + vnd->name); + goto error; + } + } else if (vendor) { + if (vendor != vnd) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("CPU vendors do not match")); + goto error; + } + } else { + vendor = vnd; + } } - base_model->data->ppc.pvr = model->data->ppc.pvr; - if (PowerPCDecode(cpu, base_model->data, models, nmodels, NULL) < 0) - goto error; + if (VIR_ALLOC(cpu) < 0 || + !(cpu->model = strdup(model->name))) + goto no_memory; - if (!outputModel) - VIR_FREE(cpu->model); + if (vendor && !(cpu->vendor = strdup(vendor->name))) + goto no_memory; + + cpu->type = VIR_CPU_TYPE_GUEST; + cpu->match = VIR_CPU_MATCH_EXACT; cleanup: - ppcModelFree(base_model); ppcMapFree(map); return cpu; + no_memory: virReportOOMError(); error: - ppcModelFree(model); virCPUDefFree(cpu); cpu = NULL; goto cleanup; @@ -617,7 +609,7 @@ struct cpuArchDriver cpuDriverPowerPC = { .nodeData = NULL, #endif .guestData = NULL, - .baseline = PowerPCBaseline, + .baseline = ppcBaseline, .update = PowerPCUpdate, .hasFeature = NULL, }; -- 1.8.0.2
On 12/20/2012 05:01 PM, Jiri Denemark wrote:
Baseline API is supposed to return guest CPU definition that can be used on any of the provided host CPUs. Since PowerPC CPUs are either identical or incompatible, the API just needs to check that all provided CPUs are identical. Previous implementation was completely bogus.
The function is also renamed as ppcBaseline to match other functions in PowerPC CPU driver. --- src/cpu/cpu_powerpc.c | 130 +++++++++++++++++++++++--------------------------- 1 file changed, 61 insertions(+), 69 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PowerPC CPUs are either identical or incompatible and thus we just need to look up the right model for given PVR without pretending we have several candidates which we may choose from. The function is also renamed as ppcDecode to match other functions in PowerPC CPU driver. --- src/cpu/cpu_powerpc.c | 121 +++++++++----------------------------------------- 1 file changed, 21 insertions(+), 100 deletions(-) diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c index 3722fc0..1d63c5f 100644 --- a/src/cpu/cpu_powerpc.c +++ b/src/cpu/cpu_powerpc.c @@ -70,16 +70,16 @@ struct ppc_map { }; static int -ConvertModelVendorFromPVR(char ***model, - char ***vendor, +ConvertModelVendorFromPVR(char **model, + char **vendor, uint32_t pvr) { int i; for (i = 0; cpu_defs[i].name; i++) { if (cpu_defs[i].pvr == pvr) { - **model = strdup(cpu_defs[i].name); - **vendor = strdup(cpu_defs[i].vendor); + *model = strdup(cpu_defs[i].name); + *vendor = strdup(cpu_defs[i].vendor); return 0; } } @@ -107,24 +107,6 @@ ConvertPVRFromModel(const char *model, return -1; } -static int -cpuMatch(const union cpuData *data, - char **cpu_model, - char **cpu_vendor, - const struct ppc_model *model) -{ - int ret = 0; - - ret = ConvertModelVendorFromPVR(&cpu_model, &cpu_vendor, data->ppc.pvr); - - if (STREQ(model->name, *cpu_model) && - STREQ(model->vendor->name, *cpu_vendor)) - ret = 1; - - return ret; -} - - static struct ppc_model * ppcModelNew(void) { @@ -372,102 +354,41 @@ ppcCompare(virCPUDefPtr host, } static int -PowerPCDecode(virCPUDefPtr cpu, +ppcDecode(virCPUDefPtr cpu, const union cpuData *data, const char **models, unsigned int nmodels, - const char *preferred) + const char *preferred ATTRIBUTE_UNUSED) { int ret = -1; struct ppc_map *map; - const struct ppc_model *candidate; - virCPUDefPtr cpuCandidate; - virCPUDefPtr cpuModel = NULL; char *cpu_vendor = NULL; char *cpu_model = NULL; - unsigned int i; if (data == NULL || (map = ppcLoadMap()) == NULL) return -1; - candidate = map->models; - - while (candidate != NULL) { - bool allowed = (models == NULL); - - for (i = 0; i < nmodels; i++) { - if (models && models[i] && STREQ(models[i], candidate->name)) { - allowed = true; - break; - } - } + if (ConvertModelVendorFromPVR(&cpu_model, &cpu_vendor, data->ppc.pvr) < 0) + goto cleanup; - if (!allowed) { - if (preferred && STREQ(candidate->name, preferred)) { - if (cpu->fallback != VIR_CPU_FALLBACK_ALLOW) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("CPU model %s is not supported by hypervisor"), - preferred); - goto out; - } else { - VIR_WARN("Preferred CPU model %s not allowed by" - " hypervisor; closest supported model will be" - " used", preferred); - } - } else { - VIR_DEBUG("CPU model %s not allowed by hypervisor; ignoring", - candidate->name); - } - goto next; - } - - if (VIR_ALLOC(cpuCandidate) < 0) { - virReportOOMError(); - goto out; - } - - cpuCandidate->model = strdup(candidate->name); - cpuCandidate->vendor = strdup(candidate->vendor->name); - - if (preferred && STREQ(cpuCandidate->model, preferred)) { - virCPUDefFree(cpuModel); - cpuModel = cpuCandidate; - break; - } - - ret = cpuMatch(data, &cpu_model, &cpu_vendor, candidate); - if (ret < 0) { - VIR_FREE(cpuCandidate); - goto out; - } else if (ret == 1) { - cpuCandidate->model = cpu_model; - cpuCandidate->vendor = cpu_vendor; - virCPUDefFree(cpuModel); - cpuModel = cpuCandidate; - break; - } - - virCPUDefFree(cpuCandidate); - - next: - candidate = candidate->next; - } - - if (cpuModel == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Cannot find suitable CPU model for given data")); - goto out; + if (!cpuModelIsAllowed(cpu_model, models, nmodels)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("CPU model %s is not supported by hypervisor"), + cpu_model); + goto cleanup; } - cpu->model = cpuModel->model; - cpu->vendor = cpuModel->vendor; - VIR_FREE(cpuModel); + cpu->model = cpu_model; + cpu->vendor = cpu_vendor; + cpu_model = NULL; + cpu_vendor = NULL; ret = 0; -out: +cleanup: + VIR_FREE(cpu_model); + VIR_FREE(cpu_vendor); ppcMapFree(map); - virCPUDefFree(cpuModel); return ret; } @@ -600,7 +521,7 @@ struct cpuArchDriver cpuDriverPowerPC = { .arch = archs, .narch = ARRAY_CARDINALITY(archs), .compare = ppcCompare, - .decode = PowerPCDecode, + .decode = ppcDecode, .encode = NULL, .free = PowerPCDataFree, #if defined(__powerpc__) || defined(__powerpc64__) -- 1.8.0.2
On 12/20/2012 05:01 PM, Jiri Denemark wrote:
PowerPC CPUs are either identical or incompatible and thus we just need to look up the right model for given PVR without pretending we have several candidates which we may choose from.
The function is also renamed as ppcDecode to match other functions in PowerPC CPU driver. --- src/cpu/cpu_powerpc.c | 121 +++++++++----------------------------------------- 1 file changed, 21 insertions(+), 100 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The cpu_map.xml file is there to separate CPU model definitions from the code. Having the only interesting data for PowerPC models only in the source code. This patch moves this data to the XML file and removes the hardcoded list completely. --- src/cpu/cpu_map.xml | 5 ++ src/cpu/cpu_powerpc.c | 161 ++++++++++++++++++-------------------------------- 2 files changed, 63 insertions(+), 103 deletions(-) diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index ba8c17d..6d51283 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -591,12 +591,17 @@ <!-- IBM-based CPU models --> <model name='POWER7'> <vendor name='IBM'/> + <pvr value='0x003f0200'/> </model> + <model name='POWER7_v2.1'> <vendor name='IBM'/> + <pvr value='0x003f0201'/> </model> + <model name='POWER7_v2.3'> <vendor name='IBM'/> + <pvr value='0x003f0203'/> </model> </arch> </cpus> diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c index 1d63c5f..967fa6e 100644 --- a/src/cpu/cpu_powerpc.c +++ b/src/cpu/cpu_powerpc.c @@ -38,20 +38,6 @@ static const virArch archs[] = { VIR_ARCH_PPC64 }; -struct cpuPowerPC { - const char *name; - const char *vendor; - uint32_t pvr; -}; - -static const struct cpuPowerPC cpu_defs[] = { - {"POWER7", "IBM", 0x003f0200}, - {"POWER7_v2.1", "IBM", 0x003f0201}, - {"POWER7_v2.3", "IBM", 0x003f0203}, - {NULL, NULL, 0xffffffff} -}; - - struct ppc_vendor { char *name; struct ppc_vendor *next; @@ -60,7 +46,7 @@ struct ppc_vendor { struct ppc_model { char *name; const struct ppc_vendor *vendor; - union cpuData *data; + struct cpuPPCData data; struct ppc_model *next; }; @@ -69,59 +55,6 @@ struct ppc_map { struct ppc_model *models; }; -static int -ConvertModelVendorFromPVR(char **model, - char **vendor, - uint32_t pvr) -{ - int i; - - for (i = 0; cpu_defs[i].name; i++) { - if (cpu_defs[i].pvr == pvr) { - *model = strdup(cpu_defs[i].name); - *vendor = strdup(cpu_defs[i].vendor); - return 0; - } - } - - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Missing the definition of this model")); - return -1; -} - -static int -ConvertPVRFromModel(const char *model, - uint32_t *pvr) -{ - int i; - - for (i = 0; cpu_defs[i].name; i++) { - if (STREQ(cpu_defs[i].name, model)) { - *pvr = cpu_defs[i].pvr; - return 0; - } - } - - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Missing the definition of this model")); - return -1; -} - -static struct ppc_model * -ppcModelNew(void) -{ - struct ppc_model *model; - - if (VIR_ALLOC(model) < 0) - return NULL; - - if (VIR_ALLOC(model->data) < 0) { - VIR_FREE(model); - return NULL; - } - - return model; -} static void ppcModelFree(struct ppc_model *model) @@ -130,9 +63,6 @@ ppcModelFree(struct ppc_model *model) return; VIR_FREE(model->name); - - VIR_FREE(model->data); - VIR_FREE(model); } @@ -153,6 +83,23 @@ ppcModelFind(const struct ppc_map *map, return NULL; } +static struct ppc_model * +ppcModelFindPVR(const struct ppc_map *map, + uint32_t pvr) +{ + struct ppc_model *model; + + model = map->models; + while (model != NULL) { + if (model->data.pvr == pvr) + return model; + + model = model->next; + } + + return NULL; +} + static struct ppc_vendor * ppcVendorFind(const struct ppc_map *map, const char *name) @@ -223,25 +170,27 @@ static int ppcModelLoad(xmlXPathContextPtr ctxt, struct ppc_map *map) { - xmlNodePtr *nodes = NULL; struct ppc_model *model; char *vendor = NULL; - int ret = -1; + unsigned long pvr; - if (!(model = ppcModelNew())) - goto no_memory; + if (VIR_ALLOC(model) < 0) { + virReportOOMError(); + return -1; + } model->name = virXPathString("string(@name)", ctxt); - if (model->name == NULL) { + if (!model->name) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing CPU model name")); goto ignore; } - ret = ConvertPVRFromModel(model->name, &model->data->ppc.pvr); - if (ret < 0) - goto ignore; - + if (ppcModelFind(map, model->name)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("CPU model %s already defined"), model->name); + goto ignore; + } if (virXPathBoolean("boolean(./vendor)", ctxt)) { vendor = virXPathString("string(./vendor/@name)", ctxt); @@ -260,26 +209,29 @@ ppcModelLoad(xmlXPathContextPtr ctxt, } } - if (map->models == NULL) + if (!virXPathBoolean("boolean(./pvr)", ctxt) || + virXPathULongHex("string(./pvr/@value)", ctxt, &pvr) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing or invalid PVR value in CPU model %s"), + model->name); + goto ignore; + } + model->data.pvr = pvr; + + if (map->models == NULL) { map->models = model; - else { + } else { model->next = map->models; map->models = model; } - ret = 0; - -out: +cleanup: VIR_FREE(vendor); - VIR_FREE(nodes); - return ret; - -no_memory: - virReportOOMError(); + return 0; ignore: ppcModelFree(model); - goto out; + goto cleanup; } static int @@ -294,7 +246,8 @@ ppcMapLoadCallback(enum cpuMapElement element, return ppcVendorLoad(ctxt, map); case CPU_MAP_ELEMENT_MODEL: return ppcModelLoad(ctxt, map); - default: + case CPU_MAP_ELEMENT_FEATURE: + case CPU_MAP_ELEMENT_LAST: break; } @@ -362,32 +315,34 @@ ppcDecode(virCPUDefPtr cpu, { int ret = -1; struct ppc_map *map; - char *cpu_vendor = NULL; - char *cpu_model = NULL; + const struct ppc_model *model; if (data == NULL || (map = ppcLoadMap()) == NULL) return -1; - if (ConvertModelVendorFromPVR(&cpu_model, &cpu_vendor, data->ppc.pvr) < 0) + if (!(model = ppcModelFindPVR(map, data->ppc.pvr))) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Cannot find CPU model with PVR 0x%08x"), + data->ppc.pvr); goto cleanup; + } - if (!cpuModelIsAllowed(cpu_model, models, nmodels)) { + if (!cpuModelIsAllowed(model->name, models, nmodels)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("CPU model %s is not supported by hypervisor"), - cpu_model); + model->name); goto cleanup; } - cpu->model = cpu_model; - cpu->vendor = cpu_vendor; - cpu_model = NULL; - cpu_vendor = NULL; + if (!(cpu->model = strdup(model->name)) || + (model->vendor && !(cpu->vendor = strdup(model->vendor->name)))) { + virReportOOMError(); + goto cleanup; + } ret = 0; cleanup: - VIR_FREE(cpu_model); - VIR_FREE(cpu_vendor); ppcMapFree(map); return ret; -- 1.8.0.2
On 12/20/2012 05:01 PM, Jiri Denemark wrote:
The cpu_map.xml file is there to separate CPU model definitions from the code. Having the only interesting data for PowerPC models only in the source code. This patch moves this data to the XML file and removes the hardcoded list completely. --- src/cpu/cpu_map.xml | 5 ++ src/cpu/cpu_powerpc.c | 161 ++++++++++++++++++-------------------------------- 2 files changed, 63 insertions(+), 103 deletions(-)
diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index ba8c17d..6d51283 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -591,12 +591,17 @@ <!-- IBM-based CPU models --> <model name='POWER7'> <vendor name='IBM'/> + <pvr value='0x003f0200'/> </model>
Do we have an RNG schema for cpu_map.xml? Should we?
@@ -260,26 +209,29 @@ ppcModelLoad(xmlXPathContextPtr ctxt, } }
- if (map->models == NULL) + if (!virXPathBoolean("boolean(./pvr)", ctxt) ||
Is this call to virXPathBoolean necessary? If the element doesn't exist,
+ virXPathULongHex("string(./pvr/@value)", ctxt, &pvr) < 0) {
then isn't this call to virXPathULongHex guaranteed to fail, without wasting time on the first query?
+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing or invalid PVR value in CPU model %s"), + model->name); + goto ignore; + } + model->data.pvr = pvr; + + if (map->models == NULL) { map->models = model; - else {
Nice cleanup of style violation along the way. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
For consistency with other functions in PowerPC CPU driver, the two functions are renamed as ppcUpdate and ppcDataFree, respectively. --- src/cpu/cpu_powerpc.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c index 967fa6e..ebf4036 100644 --- a/src/cpu/cpu_powerpc.c +++ b/src/cpu/cpu_powerpc.c @@ -350,7 +350,7 @@ cleanup: static void -PowerPCDataFree(union cpuData *data) +ppcDataFree(union cpuData *data) { if (data == NULL) return; @@ -377,11 +377,12 @@ ppcNodeData(void) #endif static int -PowerPCUpdate(virCPUDefPtr guest ATTRIBUTE_UNUSED, +ppcUpdate(virCPUDefPtr guest ATTRIBUTE_UNUSED, const virCPUDefPtr host ATTRIBUTE_UNUSED) { return 0; } + static virCPUDefPtr ppcBaseline(virCPUDefPtr *cpus, unsigned int ncpus, @@ -478,7 +479,7 @@ struct cpuArchDriver cpuDriverPowerPC = { .compare = ppcCompare, .decode = ppcDecode, .encode = NULL, - .free = PowerPCDataFree, + .free = ppcDataFree, #if defined(__powerpc__) || defined(__powerpc64__) .nodeData = ppcNodeData, #else @@ -486,6 +487,6 @@ struct cpuArchDriver cpuDriverPowerPC = { #endif .guestData = NULL, .baseline = ppcBaseline, - .update = PowerPCUpdate, + .update = ppcUpdate, .hasFeature = NULL, }; -- 1.8.0.2
On 12/20/2012 05:01 PM, Jiri Denemark wrote:
For consistency with other functions in PowerPC CPU driver, the two functions are renamed as ppcUpdate and ppcDataFree, respectively. --- src/cpu/cpu_powerpc.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Hi Jiri, Thanks for fix the problems. We also have redefined POWER CPU driver and introduce features. I will send out the patch later, and I think there are some conflicts with your patches. Can you help review my patch and put it to your series? Thanks. :) On Fri, Dec 21, 2012 at 8:01 AM, Jiri Denemark <jdenemar@redhat.com> wrote:
Current PowerPC driver is too complicated and mostly wrong as it was apparently created from x86 driver and not properly addapted for PowerPC architecture. More details about the issues are described in individual patches.
Jiri Denemark (8): cpu: Introduce cpuModelIsAllowed internal API cpu: Make comparing PowerPC CPUs easier to read cpu: Fix PowerPCNodeData cpu: Fix loading PowerPC vendor from cpu_map.xml cpu: Reimplement PowerPCBaseline cpu: Reimplement PowerPCDecode cpu: Remove hardcoded list of PowerPC models cpu: Rename PowerPCUpdate and PowerPCDataFree functions
src/cpu/cpu.c | 17 ++ src/cpu/cpu.h | 5 + src/cpu/cpu_generic.c | 19 +-- src/cpu/cpu_map.xml | 12 +- src/cpu/cpu_powerpc.c | 462 +++++++++++++++++--------------------------------- src/cpu/cpu_x86.c | 11 +- 6 files changed, 193 insertions(+), 333 deletions(-)
-- 1.8.0.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Best Regards -Li
participants (3)
-
Eric Blake -
Jiri Denemark -
Li Zhang