[libvirt] [PATCH v3 00/20] cpu: Fix and improve the ppc64 driver
Patches 01-07 are cleanups, 08-10 are bug fixes and 11-20 improve the driver. Changes in v3: * Fix a bug spotted by John thanks to Coverity Changes in v2: * Implement compatibility with guests defined on older libvirt versions * Always use fallback='forbid' when emitting CPU definitions * Update all tests in one go instead of changing them several times * Handle test failure and system failure differently in cpuTestGuestData() * Simplify ppc64DriverNodeData() * Simplify XML parsing code * Remove unnecessary NULL checks * Add even more test cases * Other small changes such as shuffling chunks between commits to make the history more readable Cheers. Andrea Bolognani (20): cpu: Mark driver functions in ppc64 driver cpu: Simplify NULL handling in ppc64 driver cpu: Simplify ppc64ModelFromCPU() cpu: Reorder functions in the ppc64 driver cpu: Remove ISA information from CPU map XML tests: Remove unused file tests: Improve result handling in cpuTestGuestData() cpu: Never skip CPU model name check in ppc64 driver cpu: CPU model names have to match on ppc64 cpu: Use ppc64Compute() to implement ppc64DriverCompare() tests: Temporarily disable ppc64 cpu tests cpu: Align ppc64 CPU data with x86 cpu: Support multiple PVRs in the ppc64 driver cpu: Simplify ppc64 part of CPU map XML cpu: Parse and use PVR masks in the ppc64 driver cpu: Add POWER8NVL information to CPU map XML cpu: Implement backwards compatibility in the ppc64 driver cpu: Forbid model fallback in the ppc64 driver tests: Re-enable ppc64 cpu tests tests: Add a bunch of cpu test case for ppc64 src/cpu/cpu.h | 2 +- src/cpu/cpu_map.xml | 59 +-- src/cpu/cpu_ppc64.c | 451 +++++++++++++-------- src/cpu/cpu_ppc64_data.h | 12 +- tests/cputest.c | 55 ++- tests/cputestdata/ppc64-baseline-1-result.xml | 3 - .../ppc64-baseline-incompatible-models.xml | 14 + .../ppc64-baseline-incompatible-vendors.xml | 4 +- tests/cputestdata/ppc64-baseline-legacy.xml | 14 + .../ppc64-baseline-no-vendor-result.xml | 2 +- tests/cputestdata/ppc64-baseline-no-vendor.xml | 2 +- .../ppc64-baseline-same-model-result.xml | 3 + tests/cputestdata/ppc64-baseline-same-model.xml | 14 + tests/cputestdata/ppc64-exact.xml | 3 - tests/cputestdata/ppc64-guest-exact.xml | 3 + .../ppc64-guest-legacy-incompatible.xml | 3 + tests/cputestdata/ppc64-guest-legacy-invalid.xml | 3 + tests/cputestdata/ppc64-guest-legacy.xml | 3 + tests/cputestdata/ppc64-guest-nofallback.xml | 3 +- tests/cputestdata/ppc64-guest-strict.xml | 3 + tests/cputestdata/ppc64-guest.xml | 3 +- .../ppc64-host+guest,ppc_models-result.xml | 2 +- ... ppc64-host+guest-legacy,ppc_models-result.xml} | 2 +- tests/cputestdata/ppc64-host-better.xml | 6 + tests/cputestdata/ppc64-host-incomp-arch.xml | 6 + tests/cputestdata/ppc64-host-no-vendor.xml | 5 + tests/cputestdata/ppc64-host-worse.xml | 6 + tests/cputestdata/ppc64-host.xml | 2 +- tests/cputestdata/ppc64-strict.xml | 3 - 29 files changed, 444 insertions(+), 247 deletions(-) delete mode 100644 tests/cputestdata/ppc64-baseline-1-result.xml create mode 100644 tests/cputestdata/ppc64-baseline-incompatible-models.xml create mode 100644 tests/cputestdata/ppc64-baseline-legacy.xml create mode 100644 tests/cputestdata/ppc64-baseline-same-model-result.xml create mode 100644 tests/cputestdata/ppc64-baseline-same-model.xml delete mode 100644 tests/cputestdata/ppc64-exact.xml create mode 100644 tests/cputestdata/ppc64-guest-exact.xml create mode 100644 tests/cputestdata/ppc64-guest-legacy-incompatible.xml create mode 100644 tests/cputestdata/ppc64-guest-legacy-invalid.xml create mode 100644 tests/cputestdata/ppc64-guest-legacy.xml create mode 100644 tests/cputestdata/ppc64-guest-strict.xml rename tests/cputestdata/{ppc64-host+guest-nofallback,ppc_models,POWER7_v2.1-result.xml => ppc64-host+guest-legacy,ppc_models-result.xml} (64%) create mode 100644 tests/cputestdata/ppc64-host-better.xml create mode 100644 tests/cputestdata/ppc64-host-incomp-arch.xml create mode 100644 tests/cputestdata/ppc64-host-no-vendor.xml create mode 100644 tests/cputestdata/ppc64-host-worse.xml delete mode 100644 tests/cputestdata/ppc64-strict.xml -- 2.4.3
Use the ppc64Driver prefix for all functions that are used to fill in the cpuDriverPPC64 structure, ie. those that are going to be called by the generic CPU code. This makes it clear which functions are exported and which are implementation details; it also gets rid of the ambiguity that affected the ppc64DataFree() function which, despite what the name suggested, was not related to ppc64DataCopy() and could not be used to release the memory allocated for a virCPUppc64Data* instance. No functional changes. --- src/cpu/cpu_ppc64.c | 62 ++++++++++++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index c3a51fb..5140297 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -448,9 +448,9 @@ ppc64Compute(virCPUDefPtr host, } static virCPUCompareResult -ppc64Compare(virCPUDefPtr host, - virCPUDefPtr cpu, - bool failIncompatible) +ppc64DriverCompare(virCPUDefPtr host, + virCPUDefPtr cpu, + bool failIncompatible) { if ((cpu->arch == VIR_ARCH_NONE || host->arch == cpu->arch) && STREQ(host->model, cpu->model)) @@ -465,12 +465,12 @@ ppc64Compare(virCPUDefPtr host, } static int -ppc64Decode(virCPUDefPtr cpu, - const virCPUData *data, - const char **models, - unsigned int nmodels, - const char *preferred ATTRIBUTE_UNUSED, - unsigned int flags) +ppc64DriverDecode(virCPUDefPtr cpu, + const virCPUData *data, + const char **models, + unsigned int nmodels, + const char *preferred ATTRIBUTE_UNUSED, + unsigned int flags) { int ret = -1; struct ppc64_map *map; @@ -510,7 +510,7 @@ ppc64Decode(virCPUDefPtr cpu, static void -ppc64DataFree(virCPUDataPtr data) +ppc64DriverFree(virCPUDataPtr data) { if (data == NULL) return; @@ -519,7 +519,7 @@ ppc64DataFree(virCPUDataPtr data) } static virCPUDataPtr -ppc64NodeData(virArch arch) +ppc64DriverNodeData(virArch arch) { virCPUDataPtr cpuData; @@ -537,17 +537,17 @@ ppc64NodeData(virArch arch) } static virCPUCompareResult -ppc64GuestData(virCPUDefPtr host, - virCPUDefPtr guest, - virCPUDataPtr *data, - char **message) +ppc64DriverGuestData(virCPUDefPtr host, + virCPUDefPtr guest, + virCPUDataPtr *data, + char **message) { return ppc64Compute(host, guest, data, message); } static int -ppc64Update(virCPUDefPtr guest, - const virCPUDef *host) +ppc64DriverUpdate(virCPUDefPtr guest, + const virCPUDef *host) { switch ((virCPUMode) guest->mode) { case VIR_CPU_MODE_HOST_MODEL: @@ -569,11 +569,11 @@ ppc64Update(virCPUDefPtr guest, } static virCPUDefPtr -ppc64Baseline(virCPUDefPtr *cpus, - unsigned int ncpus, - const char **models ATTRIBUTE_UNUSED, - unsigned int nmodels ATTRIBUTE_UNUSED, - unsigned int flags) +ppc64DriverBaseline(virCPUDefPtr *cpus, + unsigned int ncpus, + const char **models ATTRIBUTE_UNUSED, + unsigned int nmodels ATTRIBUTE_UNUSED, + unsigned int flags) { struct ppc64_map *map = NULL; const struct ppc64_model *model; @@ -653,7 +653,7 @@ ppc64Baseline(virCPUDefPtr *cpus, } static int -ppc64GetModels(char ***models) +ppc64DriverGetModels(char ***models) { struct ppc64_map *map; struct ppc64_model *model; @@ -699,14 +699,14 @@ struct cpuArchDriver cpuDriverPPC64 = { .name = "ppc64", .arch = archs, .narch = ARRAY_CARDINALITY(archs), - .compare = ppc64Compare, - .decode = ppc64Decode, + .compare = ppc64DriverCompare, + .decode = ppc64DriverDecode, .encode = NULL, - .free = ppc64DataFree, - .nodeData = ppc64NodeData, - .guestData = ppc64GuestData, - .baseline = ppc64Baseline, - .update = ppc64Update, + .free = ppc64DriverFree, + .nodeData = ppc64DriverNodeData, + .guestData = ppc64DriverGuestData, + .baseline = ppc64DriverBaseline, + .update = ppc64DriverUpdate, .hasFeature = NULL, - .getModels = ppc64GetModels, + .getModels = ppc64DriverGetModels, }; -- 2.4.3
On Mon, Aug 10, 2015 at 10:55:43 +0200, Andrea Bolognani wrote:
Use the ppc64Driver prefix for all functions that are used to fill in the cpuDriverPPC64 structure, ie. those that are going to be called by the generic CPU code.
This makes it clear which functions are exported and which are implementation details; it also gets rid of the ambiguity that affected the ppc64DataFree() function which, despite what the name suggested, was not related to ppc64DataCopy() and could not be used to release the memory allocated for a virCPUppc64Data* instance.
No functional changes.
ACK Jirka
Use briefer checks, eg. (!model) instead of (model == NULL), and avoid initializing to NULL a pointer that would be assigned in the first line of the function anyway. Also remove a pointless NULL assignment. No functional changes. --- src/cpu/cpu_ppc64.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 5140297..05ff8f2 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -61,7 +61,7 @@ struct ppc64_map { static void ppc64ModelFree(struct ppc64_model *model) { - if (model == NULL) + if (!model) return; VIR_FREE(model->name); @@ -75,7 +75,7 @@ ppc64ModelFind(const struct ppc64_map *map, struct ppc64_model *model; model = map->models; - while (model != NULL) { + while (model) { if (STREQ(model->name, name)) return model; @@ -92,7 +92,7 @@ ppc64ModelFindPVR(const struct ppc64_map *map, struct ppc64_model *model; model = map->models; - while (model != NULL) { + while (model) { if (model->data.pvr == pvr) return model; @@ -158,15 +158,15 @@ static struct ppc64_model * ppc64ModelFromCPU(const virCPUDef *cpu, const struct ppc64_map *map) { - struct ppc64_model *model = NULL; + struct ppc64_model *model; - if ((model = ppc64ModelFind(map, cpu->model)) == NULL) { + if (!(model = ppc64ModelFind(map, cpu->model))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown CPU model %s"), cpu->model); goto error; } - if ((model = ppc64ModelCopy(model)) == NULL) + if (!(model = ppc64ModelCopy(model))) goto error; return model; @@ -181,7 +181,7 @@ static int ppc64VendorLoad(xmlXPathContextPtr ctxt, struct ppc64_map *map) { - struct ppc64_vendor *vendor = NULL; + struct ppc64_vendor *vendor; if (VIR_ALLOC(vendor) < 0) return -1; @@ -264,7 +264,7 @@ ppc64ModelLoad(xmlXPathContextPtr ctxt, } model->data.pvr = pvr; - if (map->models == NULL) { + if (!map->models) { map->models = model; } else { model->next = map->models; @@ -303,16 +303,16 @@ ppc64MapLoadCallback(cpuMapElement element, static void ppc64MapFree(struct ppc64_map *map) { - if (map == NULL) + if (!map) return; - while (map->models != NULL) { + while (map->models) { struct ppc64_model *model = map->models; map->models = model->next; ppc64ModelFree(model); } - while (map->vendors != NULL) { + while (map->vendors) { struct ppc64_vendor *vendor = map->vendors; map->vendors = vendor->next; ppc64VendorFree(vendor); @@ -350,7 +350,6 @@ ppc64MakeCPUData(virArch arch, cpuData->arch = arch; cpuData->data.ppc64 = *data; - data = NULL; return cpuData; } @@ -417,7 +416,7 @@ ppc64Compute(virCPUDefPtr host, !(guest_model = ppc64ModelFromCPU(cpu, map))) goto cleanup; - if (guestData != NULL) { + if (guestData) { if (cpu->type == VIR_CPU_TYPE_GUEST && cpu->match == VIR_CPU_MATCH_STRICT && STRNEQ(guest_model->name, host_model->name)) { @@ -478,7 +477,7 @@ ppc64DriverDecode(virCPUDefPtr cpu, virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES, -1); - if (data == NULL || (map = ppc64LoadMap()) == NULL) + if (!data || !(map = ppc64LoadMap())) return -1; if (!(model = ppc64ModelFindPVR(map, data->data.ppc64.pvr))) { @@ -512,7 +511,7 @@ ppc64DriverDecode(virCPUDefPtr cpu, static void ppc64DriverFree(virCPUDataPtr data) { - if (data == NULL) + if (!data) return; VIR_FREE(data); @@ -575,7 +574,7 @@ ppc64DriverBaseline(virCPUDefPtr *cpus, unsigned int nmodels ATTRIBUTE_UNUSED, unsigned int flags) { - struct ppc64_map *map = NULL; + struct ppc64_map *map; const struct ppc64_model *model; const struct ppc64_vendor *vendor = NULL; virCPUDefPtr cpu = NULL; @@ -667,7 +666,7 @@ ppc64DriverGetModels(char ***models) goto error; model = map->models; - while (model != NULL) { + while (model) { if (models) { if (VIR_STRDUP(name, model->name) < 0) goto error; -- 2.4.3
On Mon, Aug 10, 2015 at 10:55:44 +0200, Andrea Bolognani wrote:
Use briefer checks, eg. (!model) instead of (model == NULL), and avoid initializing to NULL a pointer that would be assigned in the first line of the function anyway.
Also remove a pointless NULL assignment.
No functional changes.
ACK Jirka
--- src/cpu/cpu_ppc64.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 05ff8f2..18dbf86 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -163,17 +163,10 @@ ppc64ModelFromCPU(const virCPUDef *cpu, if (!(model = ppc64ModelFind(map, cpu->model))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown CPU model %s"), cpu->model); - goto error; + return NULL; } - if (!(model = ppc64ModelCopy(model))) - goto error; - - return model; - - error: - ppc64ModelFree(model); - return NULL; + return ppc64ModelCopy(model); } -- 2.4.3
On Mon, Aug 10, 2015 at 10:55:45 +0200, Andrea Bolognani wrote:
--- src/cpu/cpu_ppc64.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 05ff8f2..18dbf86 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -163,17 +163,10 @@ ppc64ModelFromCPU(const virCPUDef *cpu, if (!(model = ppc64ModelFind(map, cpu->model))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown CPU model %s"), cpu->model); - goto error; + return NULL; }
- if (!(model = ppc64ModelCopy(model))) - goto error; - - return model; - - error: - ppc64ModelFree(model); - return NULL; + return ppc64ModelCopy(model); }
ACK Jirka
Having the functions grouped together this way will avoid further shuffling around down the line. No functional changes. --- src/cpu/cpu_ppc64.c | 127 +++++++++++++++++++++++++--------------------------- 1 file changed, 62 insertions(+), 65 deletions(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 18dbf86..5921263 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -57,6 +57,32 @@ struct ppc64_map { struct ppc64_model *models; }; +static void +ppc64VendorFree(struct ppc64_vendor *vendor) +{ + if (!vendor) + return; + + VIR_FREE(vendor->name); + VIR_FREE(vendor); +} + +static struct ppc64_vendor * +ppc64VendorFind(const struct ppc64_map *map, + const char *name) +{ + struct ppc64_vendor *vendor; + + vendor = map->vendors; + while (vendor) { + if (STREQ(vendor->name, name)) + return vendor; + + vendor = vendor->next; + } + + return NULL; +} static void ppc64ModelFree(struct ppc64_model *model) @@ -69,6 +95,23 @@ ppc64ModelFree(struct ppc64_model *model) } static struct ppc64_model * +ppc64ModelCopy(const struct ppc64_model *model) +{ + struct ppc64_model *copy; + + if (VIR_ALLOC(copy) < 0 || + VIR_STRDUP(copy->name, model->name) < 0) { + ppc64ModelFree(copy); + return NULL; + } + + copy->data.pvr = model->data.pvr; + copy->vendor = model->vendor; + + return copy; +} + +static struct ppc64_model * ppc64ModelFind(const struct ppc64_map *map, const char *name) { @@ -111,65 +154,41 @@ ppc64ModelFindPVR(const struct ppc64_map *map, } static struct ppc64_model * -ppc64ModelCopy(const struct ppc64_model *model) +ppc64ModelFromCPU(const virCPUDef *cpu, + const struct ppc64_map *map) { - struct ppc64_model *copy; + struct ppc64_model *model; - if (VIR_ALLOC(copy) < 0 || - VIR_STRDUP(copy->name, model->name) < 0) { - ppc64ModelFree(copy); + if (!(model = ppc64ModelFind(map, cpu->model))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown CPU model %s"), cpu->model); return NULL; } - copy->data.pvr = model->data.pvr; - copy->vendor = model->vendor; - - return copy; -} - -static struct ppc64_vendor * -ppc64VendorFind(const struct ppc64_map *map, - const char *name) -{ - struct ppc64_vendor *vendor; - - vendor = map->vendors; - while (vendor) { - if (STREQ(vendor->name, name)) - return vendor; - - vendor = vendor->next; - } - - return NULL; + return ppc64ModelCopy(model); } static void -ppc64VendorFree(struct ppc64_vendor *vendor) +ppc64MapFree(struct ppc64_map *map) { - if (!vendor) + if (!map) return; - VIR_FREE(vendor->name); - VIR_FREE(vendor); -} - -static struct ppc64_model * -ppc64ModelFromCPU(const virCPUDef *cpu, - const struct ppc64_map *map) -{ - struct ppc64_model *model; + while (map->models) { + struct ppc64_model *model = map->models; + map->models = model->next; + ppc64ModelFree(model); + } - if (!(model = ppc64ModelFind(map, cpu->model))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unknown CPU model %s"), cpu->model); - return NULL; + while (map->vendors) { + struct ppc64_vendor *vendor = map->vendors; + map->vendors = vendor->next; + ppc64VendorFree(vendor); } - return ppc64ModelCopy(model); + VIR_FREE(map); } - static int ppc64VendorLoad(xmlXPathContextPtr ctxt, struct ppc64_map *map) @@ -293,27 +312,6 @@ ppc64MapLoadCallback(cpuMapElement element, return 0; } -static void -ppc64MapFree(struct ppc64_map *map) -{ - if (!map) - return; - - while (map->models) { - struct ppc64_model *model = map->models; - map->models = model->next; - ppc64ModelFree(model); - } - - while (map->vendors) { - struct ppc64_vendor *vendor = map->vendors; - map->vendors = vendor->next; - ppc64VendorFree(vendor); - } - - VIR_FREE(map); -} - static struct ppc64_map * ppc64LoadMap(void) { @@ -500,7 +498,6 @@ ppc64DriverDecode(virCPUDefPtr cpu, return ret; } - static void ppc64DriverFree(virCPUDataPtr data) { -- 2.4.3
On Mon, Aug 10, 2015 at 10:55:46 +0200, Andrea Bolognani wrote:
Having the functions grouped together this way will avoid further shuffling around down the line.
No functional changes.
Still ACK Jirka
The information is not used anywhere in libvirt. No functional changes. --- src/cpu/cpu_map.xml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index b924bd3..6387ce4 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -1385,31 +1385,26 @@ <model name='power6'> <vendor name='IBM'/> - <compat isa='2.05'/> <pvr value='0x003e0000'/> </model> <model name='power7'> <vendor name='IBM'/> - <compat isa='2.06'/> <pvr value='0x003f0000'/> </model> <model name='power7+'> <vendor name='IBM'/> - <compat isa='2.06B'/> <pvr value='0x004a0000'/> </model> <model name='power8e'> <vendor name='IBM'/> - <compat isa='2.07'/> <pvr value='0x004b0000'/> </model> <model name='power8'> <vendor name='IBM'/> - <compat isa='2.07'/> <pvr value='0x004d0000'/> </model> -- 2.4.3
No functional changes. --- tests/cputestdata/ppc64-baseline-1-result.xml | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 tests/cputestdata/ppc64-baseline-1-result.xml diff --git a/tests/cputestdata/ppc64-baseline-1-result.xml b/tests/cputestdata/ppc64-baseline-1-result.xml deleted file mode 100644 index cbdd9bc..0000000 --- a/tests/cputestdata/ppc64-baseline-1-result.xml +++ /dev/null @@ -1,3 +0,0 @@ -<cpu mode='custom' match='exact'> - <model fallback='allow'>POWER7+_v2.1</model> -</cpu> -- 2.4.3
On Mon, Aug 10, 2015 at 10:55:48 +0200, Andrea Bolognani wrote:
No functional changes. --- tests/cputestdata/ppc64-baseline-1-result.xml | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 tests/cputestdata/ppc64-baseline-1-result.xml
diff --git a/tests/cputestdata/ppc64-baseline-1-result.xml b/tests/cputestdata/ppc64-baseline-1-result.xml deleted file mode 100644 index cbdd9bc..0000000 --- a/tests/cputestdata/ppc64-baseline-1-result.xml +++ /dev/null @@ -1,3 +0,0 @@ -<cpu mode='custom' match='exact'> - <model fallback='allow'>POWER7+_v2.1</model> -</cpu>
ACK Jirka
A test is considered successful if the obtained result matches the expected result: if that's not the case, whether because a test that was expected to succeed failed or because a test that was supposed to fail succeeded, then something's not right and we want the user to know about this. On the other hand, if a failure that's unrelated to the bits we're testing occurs, then the user should be notified even if the test was expected to fail. Use different values to tell these two situations apart. Fix a test case that was wrongly expected to fail as well. --- tests/cputest.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/tests/cputest.c b/tests/cputest.c index 06b3f12..93f9d2e 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -247,7 +247,7 @@ static int cpuTestGuestData(const void *arg) { const struct data *data = arg; - int ret = -1; + int ret = -2; virCPUDefPtr host = NULL; virCPUDefPtr cpu = NULL; virCPUDefPtr guest = NULL; @@ -262,8 +262,10 @@ cpuTestGuestData(const void *arg) cmpResult = cpuGuestData(host, cpu, &guestData, NULL); if (cmpResult == VIR_CPU_COMPARE_ERROR || - cmpResult == VIR_CPU_COMPARE_INCOMPATIBLE) + cmpResult == VIR_CPU_COMPARE_INCOMPATIBLE) { + ret = -1; goto cleanup; + } if (VIR_ALLOC(guest) < 0) goto cleanup; @@ -274,10 +276,7 @@ cpuTestGuestData(const void *arg) guest->fallback = cpu->fallback; if (cpuDecode(guest, guestData, data->models, data->nmodels, data->preferred) < 0) { - if (data->result < 0) { - virResetLastError(); - ret = 0; - } + ret = -1; goto cleanup; } @@ -294,7 +293,10 @@ cpuTestGuestData(const void *arg) } result = virBufferContentAndReset(&buf); - ret = cpuTestCompareXML(data->arch, guest, result, false); + if (cpuTestCompareXML(data->arch, guest, result, false) < 0) + goto cleanup; + + ret = 0; cleanup: VIR_FREE(result); @@ -302,6 +304,20 @@ cpuTestGuestData(const void *arg) virCPUDefFree(host); virCPUDefFree(cpu); virCPUDefFree(guest); + + if (ret == data->result) { + /* We got the result we expected, whether it was + * a success or a failure */ + virResetLastError(); + ret = 0; + } else { + VIR_TEST_VERBOSE("\nExpected result %d, got %d\n", + data->result, ret); + /* Pad to line up with test name ... in virTestRun */ + VIR_TEST_VERBOSE("%74s", "... "); + ret = -1; + } + return ret; } @@ -646,7 +662,7 @@ mymain(void) NULL, "Haswell-noTSX", 0); DO_TEST_GUESTDATA("ppc64", "host", "guest", ppc_models, NULL, 0); - DO_TEST_GUESTDATA("ppc64", "host", "guest-nofallback", ppc_models, "POWER7_v2.1", -1); + DO_TEST_GUESTDATA("ppc64", "host", "guest-nofallback", ppc_models, "POWER7_v2.1", 0); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.4.3
On Mon, Aug 10, 2015 at 10:55:49 +0200, Andrea Bolognani wrote:
A test is considered successful if the obtained result matches the expected result: if that's not the case, whether because a test that was expected to succeed failed or because a test that was supposed to fail succeeded, then something's not right and we want the user to know about this.
On the other hand, if a failure that's unrelated to the bits we're testing occurs, then the user should be notified even if the test was expected to fail.
Use different values to tell these two situations apart.
Fix a test case that was wrongly expected to fail as well.
ACK Jirka
ppc64Compute(), called by cpuNodeData(), is used not only to retrieve the driver-specific data associated to a guest CPU definition, but also to check whether said guest CPU is compatible with the host CPU. If the user is not interested in the CPU data, it's perfectly fine to pass a NULL pointer instead of a return location, and the compatibility data returned should not be affected by this. One of the checks, specifically the one on CPU model name, was however only performed if the return location was non-NULL. --- src/cpu/cpu_ppc64.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 5921263..53d8fb0 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -407,26 +407,25 @@ ppc64Compute(virCPUDefPtr host, !(guest_model = ppc64ModelFromCPU(cpu, map))) goto cleanup; - if (guestData) { - 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 cleanup; - - ret = VIR_CPU_COMPARE_INCOMPATIBLE; + 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 cleanup; - } + ret = VIR_CPU_COMPARE_INCOMPATIBLE; + goto cleanup; + } + + if (guestData) if (!(*guestData = ppc64MakeCPUData(arch, &guest_model->data))) goto cleanup; - } ret = VIR_CPU_COMPARE_IDENTICAL; -- 2.4.3
On Mon, Aug 10, 2015 at 10:55:50 +0200, Andrea Bolognani wrote:
ppc64Compute(), called by cpuNodeData(), is used not only to retrieve the driver-specific data associated to a guest CPU definition, but also to check whether said guest CPU is compatible with the host CPU.
If the user is not interested in the CPU data, it's perfectly fine to pass a NULL pointer instead of a return location, and the compatibility data returned should not be affected by this. One of the checks, specifically the one on CPU model name, was however only performed if the return location was non-NULL. --- src/cpu/cpu_ppc64.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-)
ACK Jirka
Limitations of the POWER architecture mean that you can't run eg. a POWER7 guest on a POWER8 host when using KVM. This applies to all guests, not just those using VIR_CPU_MATCH_STRICT in the CPU definition; in fact, exact and strict CPU matching are basically the same on ppc64. This means, of course, that hosts using different CPUs have to be considered incompatible as well. Change ppc64Compute(), called by cpuGuestData(), to reflect this fact and update test cases accordingly. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1250977 --- src/cpu/cpu_ppc64.c | 5 +---- tests/cputest.c | 4 ++-- tests/cputestdata/ppc64-guest.xml | 2 +- tests/cputestdata/ppc64-host+guest,ppc_models-result.xml | 2 +- .../ppc64-host+guest-nofallback,ppc_models,POWER7_v2.1-result.xml | 5 ----- 5 files changed, 5 insertions(+), 13 deletions(-) delete mode 100644 tests/cputestdata/ppc64-host+guest-nofallback,ppc_models,POWER7_v2.1-result.xml diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 53d8fb0..0769956 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -350,7 +350,6 @@ ppc64Compute(virCPUDefPtr host, const virCPUDef *cpu, virCPUDataPtr *guestData, char **message) - { struct ppc64_map *map = NULL; struct ppc64_model *host_model = NULL; @@ -407,9 +406,7 @@ ppc64Compute(virCPUDefPtr host, !(guest_model = ppc64ModelFromCPU(cpu, map))) goto cleanup; - if (cpu->type == VIR_CPU_TYPE_GUEST && - cpu->match == VIR_CPU_MATCH_STRICT && - STRNEQ(guest_model->name, host_model->name)) { + if (STRNEQ(guest_model->name, host_model->name)) { VIR_DEBUG("host CPU model does not match required CPU model %s", guest_model->name); if (message && diff --git a/tests/cputest.c b/tests/cputest.c index 93f9d2e..1e84fd3 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -501,7 +501,7 @@ static const char *model486[] = { "486" }; static const char *nomodel[] = { "nomodel" }; static const char *models[] = { "qemu64", "core2duo", "Nehalem" }; static const char *haswell[] = { "SandyBridge", "Haswell" }; -static const char *ppc_models[] = { "POWER7", "POWER7_v2.1", "POWER8_v1.0"}; +static const char *ppc_models[] = { "POWER7", "POWER7_v2.1", "POWER7_v2.3", "POWER8_v1.0"}; static int mymain(void) @@ -662,7 +662,7 @@ mymain(void) NULL, "Haswell-noTSX", 0); DO_TEST_GUESTDATA("ppc64", "host", "guest", ppc_models, NULL, 0); - DO_TEST_GUESTDATA("ppc64", "host", "guest-nofallback", ppc_models, "POWER7_v2.1", 0); + DO_TEST_GUESTDATA("ppc64", "host", "guest-nofallback", ppc_models, "POWER7_v2.1", -1); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/cputestdata/ppc64-guest.xml b/tests/cputestdata/ppc64-guest.xml index ac81ec0..9e91501 100644 --- a/tests/cputestdata/ppc64-guest.xml +++ b/tests/cputestdata/ppc64-guest.xml @@ -1,4 +1,4 @@ <cpu match='exact'> - <model>POWER8_v1.0</model> + <model>POWER7_v2.3</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 index 0cb0830..3e55f68 100644 --- a/tests/cputestdata/ppc64-host+guest,ppc_models-result.xml +++ b/tests/cputestdata/ppc64-host+guest,ppc_models-result.xml @@ -1,5 +1,5 @@ <cpu mode='custom' match='exact'> <arch>ppc64</arch> - <model fallback='allow'>POWER8_v1.0</model> + <model fallback='allow'>POWER7_v2.3</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 deleted file mode 100644 index 7e58361..0000000 --- a/tests/cputestdata/ppc64-host+guest-nofallback,ppc_models,POWER7_v2.1-result.xml +++ /dev/null @@ -1,5 +0,0 @@ -<cpu mode='custom' match='exact'> - <arch>ppc64</arch> - <model fallback='forbid'>POWER7_v2.1</model> - <vendor>IBM</vendor> -</cpu> -- 2.4.3
On Mon, Aug 10, 2015 at 10:55:51 +0200, Andrea Bolognani wrote:
Limitations of the POWER architecture mean that you can't run eg. a POWER7 guest on a POWER8 host when using KVM. This applies to all guests, not just those using VIR_CPU_MATCH_STRICT in the CPU definition; in fact, exact and strict CPU matching are basically the same on ppc64.
This means, of course, that hosts using different CPUs have to be considered incompatible as well.
Change ppc64Compute(), called by cpuGuestData(), to reflect this fact and update test cases accordingly.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1250977
ACK Jirka
This ensures comparison of two CPU definitions will be consistent regardless of the fact that it is performed using cpuCompare() or cpuGuestData(). The x86 driver uses the same exact code. --- src/cpu/cpu_ppc64.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 0769956..efac739 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -438,16 +438,22 @@ ppc64DriverCompare(virCPUDefPtr host, virCPUDefPtr cpu, bool failIncompatible) { - if ((cpu->arch == VIR_ARCH_NONE || host->arch == cpu->arch) && - STREQ(host->model, cpu->model)) - return VIR_CPU_COMPARE_IDENTICAL; + virCPUCompareResult ret; + char *message = NULL; - if (failIncompatible) { - virReportError(VIR_ERR_CPU_INCOMPATIBLE, NULL); - return VIR_CPU_COMPARE_ERROR; - } else { - return VIR_CPU_COMPARE_INCOMPATIBLE; + ret = ppc64Compute(host, cpu, NULL, &message); + + if (failIncompatible && ret == VIR_CPU_COMPARE_INCOMPATIBLE) { + ret = VIR_CPU_COMPARE_ERROR; + if (message) { + virReportError(VIR_ERR_CPU_INCOMPATIBLE, "%s", message); + } else { + virReportError(VIR_ERR_CPU_INCOMPATIBLE, NULL); + } } + VIR_FREE(message); + + return ret; } static int -- 2.4.3
On Mon, Aug 10, 2015 at 10:55:52 +0200, Andrea Bolognani wrote:
This ensures comparison of two CPU definitions will be consistent regardless of the fact that it is performed using cpuCompare() or cpuGuestData(). The x86 driver uses the same exact code.
ACK Jirka
The upcoming commits will make heavy modifications to the ppc64 driver, split so that it's easier to review the changes. Instead of updating the test cases so that they pass, possibly only to update them again with the following commit, disable them for the time being. Another commit will update them all in one go once all required changes are in place. --- tests/cputest.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/cputest.c b/tests/cputest.c index 1e84fd3..4dbccfd 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -501,7 +501,9 @@ static const char *model486[] = { "486" }; static const char *nomodel[] = { "nomodel" }; static const char *models[] = { "qemu64", "core2duo", "Nehalem" }; static const char *haswell[] = { "SandyBridge", "Haswell" }; +/* XXX temporarily disabled static const char *ppc_models[] = { "POWER7", "POWER7_v2.1", "POWER7_v2.3", "POWER8_v1.0"}; +*/ static int mymain(void) @@ -595,8 +597,10 @@ 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); + /* XXX temporarily disabled 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 */ @@ -625,8 +629,11 @@ mymain(void) DO_TEST_BASELINE("x86", "7", 0, 0); DO_TEST_BASELINE("x86", "8", 0, 0); + /* XXX temporarily disabled 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); @@ -661,8 +668,10 @@ mymain(void) DO_TEST_GUESTDATA("x86", "host-Haswell-noTSX", "Haswell-noTSX", NULL, "Haswell-noTSX", 0); + /* XXX temporarily disabled DO_TEST_GUESTDATA("ppc64", "host", "guest", ppc_models, NULL, 0); DO_TEST_GUESTDATA("ppc64", "host", "guest-nofallback", ppc_models, "POWER7_v2.1", -1); + */ return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.4.3
On Mon, Aug 10, 2015 at 10:55:53 +0200, Andrea Bolognani wrote:
The upcoming commits will make heavy modifications to the ppc64 driver, split so that it's easier to review the changes.
Instead of updating the test cases so that they pass, possibly only to update them again with the following commit, disable them for the time being.
Another commit will update them all in one go once all required changes are in place.
ACK Jirka
Use a typedef instead of the plain struct and heap allocation. This will make it easier to extend the ppc64 specific CPU data later on. --- src/cpu/cpu.h | 2 +- src/cpu/cpu_ppc64.c | 86 ++++++++++++++++++++++++++++++++++++------------ src/cpu/cpu_ppc64_data.h | 3 +- 3 files changed, 68 insertions(+), 23 deletions(-) diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index 49d4226..7375876 100644 --- a/src/cpu/cpu.h +++ b/src/cpu/cpu.h @@ -38,7 +38,7 @@ struct _virCPUData { virArch arch; union { virCPUx86Data *x86; - struct cpuPPC64Data ppc64; + virCPUppc64Data *ppc64; /* generic driver needs no data */ } data; }; diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index efac739..6c78ab8 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -48,7 +48,7 @@ struct ppc64_vendor { struct ppc64_model { char *name; const struct ppc64_vendor *vendor; - struct cpuPPC64Data data; + virCPUppc64Data *data; struct ppc64_model *next; }; @@ -58,6 +58,25 @@ struct ppc64_map { }; static void +ppc64DataFree(virCPUppc64Data *data) +{ + VIR_FREE(data); +} + +static virCPUppc64Data * +ppc64DataCopy(const virCPUppc64Data *data) +{ + virCPUppc64Data *copy; + + if (VIR_ALLOC(copy) < 0) + return NULL; + + copy->pvr = data->pvr; + + return copy; +} + +static void ppc64VendorFree(struct ppc64_vendor *vendor) { if (!vendor) @@ -90,6 +109,7 @@ ppc64ModelFree(struct ppc64_model *model) if (!model) return; + ppc64DataFree(model->data); VIR_FREE(model->name); VIR_FREE(model); } @@ -99,16 +119,22 @@ ppc64ModelCopy(const struct ppc64_model *model) { struct ppc64_model *copy; - if (VIR_ALLOC(copy) < 0 || - VIR_STRDUP(copy->name, model->name) < 0) { - ppc64ModelFree(copy); - return NULL; - } + if (VIR_ALLOC(copy) < 0) + goto error; + + if (VIR_STRDUP(copy->name, model->name) < 0) + goto error; + + if (!(copy->data = ppc64DataCopy(model->data))) + goto error; - copy->data.pvr = model->data.pvr; copy->vendor = model->vendor; return copy; + + error: + ppc64ModelFree(copy); + return NULL; } static struct ppc64_model * @@ -136,7 +162,7 @@ ppc64ModelFindPVR(const struct ppc64_map *map, model = map->models; while (model) { - if (model->data.pvr == pvr) + if (model->data->pvr == pvr) return model; model = model->next; @@ -237,6 +263,11 @@ ppc64ModelLoad(xmlXPathContextPtr ctxt, if (VIR_ALLOC(model) < 0) return -1; + if (VIR_ALLOC(model->data) < 0) { + ppc64ModelFree(model); + return -1; + } + model->name = virXPathString("string(@name)", ctxt); if (!model->name) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -274,7 +305,7 @@ ppc64ModelLoad(xmlXPathContextPtr ctxt, model->name); goto ignore; } - model->data.pvr = pvr; + model->data->pvr = pvr; if (!map->models) { map->models = model; @@ -318,7 +349,7 @@ ppc64LoadMap(void) struct ppc64_map *map; if (VIR_ALLOC(map) < 0) - return NULL; + goto error; if (cpuMapLoad("ppc64", ppc64MapLoadCallback, map) < 0) goto error; @@ -332,7 +363,7 @@ ppc64LoadMap(void) static virCPUDataPtr ppc64MakeCPUData(virArch arch, - struct cpuPPC64Data *data) + virCPUppc64Data *data) { virCPUDataPtr cpuData; @@ -340,7 +371,9 @@ ppc64MakeCPUData(virArch arch, return NULL; cpuData->arch = arch; - cpuData->data.ppc64 = *data; + + if (!(cpuData->data.ppc64 = ppc64DataCopy(data))) + VIR_FREE(cpuData); return cpuData; } @@ -421,7 +454,7 @@ ppc64Compute(virCPUDefPtr host, } if (guestData) - if (!(*guestData = ppc64MakeCPUData(arch, &guest_model->data))) + if (!(*guestData = ppc64MakeCPUData(arch, guest_model->data))) goto cleanup; ret = VIR_CPU_COMPARE_IDENTICAL; @@ -473,10 +506,10 @@ ppc64DriverDecode(virCPUDefPtr cpu, if (!data || !(map = ppc64LoadMap())) return -1; - if (!(model = ppc64ModelFindPVR(map, data->data.ppc64.pvr))) { + if (!(model = ppc64ModelFindPVR(map, data->data.ppc64->pvr))) { virReportError(VIR_ERR_OPERATION_FAILED, _("Cannot find CPU model with PVR 0x%08x"), - data->data.ppc64.pvr); + data->data.ppc64->pvr); goto cleanup; } @@ -506,25 +539,36 @@ ppc64DriverFree(virCPUDataPtr data) if (!data) return; + ppc64DataFree(data->data.ppc64); VIR_FREE(data); } static virCPUDataPtr ppc64DriverNodeData(virArch arch) { - virCPUDataPtr cpuData; + virCPUDataPtr nodeData; + virCPUppc64Data *data; - if (VIR_ALLOC(cpuData) < 0) - return NULL; + if (VIR_ALLOC(nodeData) < 0) + goto error; - cpuData->arch = arch; + if (VIR_ALLOC(data) < 0) + goto error; + + data = nodeData->data.ppc64; #if defined(__powerpc__) || defined(__powerpc64__) asm("mfpvr %0" - : "=r" (cpuData->data.ppc64.pvr)); + : "=r" (data->pvr)); #endif - return cpuData; + nodeData->arch = arch; + + return nodeData; + + error: + ppc64DriverFree(nodeData); + return NULL; } static virCPUCompareResult diff --git a/src/cpu/cpu_ppc64_data.h b/src/cpu/cpu_ppc64_data.h index 45152de..c18fc63 100644 --- a/src/cpu/cpu_ppc64_data.h +++ b/src/cpu/cpu_ppc64_data.h @@ -26,7 +26,8 @@ # include <stdint.h> -struct cpuPPC64Data { +typedef struct _virCPUppc64Data virCPUppc64Data; +struct _virCPUppc64Data { uint32_t pvr; }; -- 2.4.3
On Mon, Aug 10, 2015 at 10:55:54 +0200, Andrea Bolognani wrote:
Use a typedef instead of the plain struct and heap allocation. This will make it easier to extend the ppc64 specific CPU data later on.
ACK Jirka
This will allow us to perform PVR matching more broadly, eg. consider both POWER8 and POWER8E CPUs to be the same even though they have different PVR values. --- src/cpu/cpu_ppc64.c | 72 ++++++++++++++++++++++++++++++++++++++++-------- src/cpu/cpu_ppc64_data.h | 8 +++++- 2 files changed, 67 insertions(+), 13 deletions(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 6c78ab8..db25f27 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -60,6 +60,10 @@ struct ppc64_map { static void ppc64DataFree(virCPUppc64Data *data) { + if (!data) + return; + + VIR_FREE(data->pvr); VIR_FREE(data); } @@ -67,13 +71,24 @@ static virCPUppc64Data * ppc64DataCopy(const virCPUppc64Data *data) { virCPUppc64Data *copy; + size_t i; if (VIR_ALLOC(copy) < 0) - return NULL; + goto error; + + if (VIR_ALLOC_N(copy->pvr, data->len) < 0) + goto error; + + copy->len = data->len; - copy->pvr = data->pvr; + for (i = 0; i < data->len; i++) + copy->pvr[i].value = data->pvr[i].value; return copy; + + error: + ppc64DataFree(copy); + return NULL; } static void @@ -159,12 +174,14 @@ ppc64ModelFindPVR(const struct ppc64_map *map, uint32_t pvr) { struct ppc64_model *model; + size_t i; model = map->models; while (model) { - if (model->data->pvr == pvr) - return model; - + for (i = 0; i < model->data->len; i++) { + if (model->data->pvr[i].value == pvr) + return model; + } model = model->next; } @@ -257,8 +274,16 @@ ppc64ModelLoad(xmlXPathContextPtr ctxt, struct ppc64_map *map) { struct ppc64_model *model; + xmlNodePtr *nodes = NULL; + xmlNodePtr bookmark; char *vendor = NULL; unsigned long pvr; + size_t i; + int n; + + /* Save the node the context was pointing to, as we're going + * to change it later. It's going to be restored on exit */ + bookmark = ctxt->node; if (VIR_ALLOC(model) < 0) return -1; @@ -298,14 +323,30 @@ ppc64ModelLoad(xmlXPathContextPtr ctxt, } } - if (!virXPathBoolean("boolean(./pvr)", ctxt) || - virXPathULongHex("string(./pvr/@value)", ctxt, &pvr) < 0) { + if ((n = virXPathNodeSet("./pvr", ctxt, &nodes)) <= 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Missing or invalid PVR value in CPU model %s"), + _("Missing PVR information for CPU model %s"), model->name); goto ignore; } - model->data->pvr = pvr; + + if (VIR_ALLOC_N(model->data->pvr, n) < 0) + goto ignore; + + model->data->len = n; + + for (i = 0; i < n; i++) { + ctxt->node = nodes[i]; + + if (!virXPathBoolean("boolean(./@value)", ctxt) || + virXPathULongHex("string(./@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[i].value = pvr; + } if (!map->models) { map->models = model; @@ -315,7 +356,9 @@ ppc64ModelLoad(xmlXPathContextPtr ctxt, } cleanup: + ctxt->node = bookmark; VIR_FREE(vendor); + VIR_FREE(nodes); return 0; ignore: @@ -506,10 +549,10 @@ ppc64DriverDecode(virCPUDefPtr cpu, if (!data || !(map = ppc64LoadMap())) return -1; - if (!(model = ppc64ModelFindPVR(map, data->data.ppc64->pvr))) { + if (!(model = ppc64ModelFindPVR(map, data->data.ppc64->pvr[0].value))) { virReportError(VIR_ERR_OPERATION_FAILED, _("Cannot find CPU model with PVR 0x%08x"), - data->data.ppc64->pvr); + data->data.ppc64->pvr[0].value); goto cleanup; } @@ -557,9 +600,14 @@ ppc64DriverNodeData(virArch arch) data = nodeData->data.ppc64; + if (VIR_ALLOC_N(data->pvr, 1) < 0) + goto error; + + data->len = 1; + #if defined(__powerpc__) || defined(__powerpc64__) asm("mfpvr %0" - : "=r" (data->pvr)); + : "=r" (data->pvr[0].value)); #endif nodeData->arch = arch; diff --git a/src/cpu/cpu_ppc64_data.h b/src/cpu/cpu_ppc64_data.h index c18fc63..0d3cb0b 100644 --- a/src/cpu/cpu_ppc64_data.h +++ b/src/cpu/cpu_ppc64_data.h @@ -26,9 +26,15 @@ # include <stdint.h> +typedef struct _virCPUppc64PVR virCPUppc64PVR; +struct _virCPUppc64PVR { + uint32_t value; +}; + typedef struct _virCPUppc64Data virCPUppc64Data; struct _virCPUppc64Data { - uint32_t pvr; + size_t len; + virCPUppc64PVR *pvr; }; #endif /* __VIR_CPU_PPC64_DATA_H__ */ -- 2.4.3
On Mon, Aug 10, 2015 at 10:55:55 +0200, Andrea Bolognani wrote:
This will allow us to perform PVR matching more broadly, eg. consider both POWER8 and POWER8E CPUs to be the same even though they have different PVR values. --- src/cpu/cpu_ppc64.c | 72 ++++++++++++++++++++++++++++++++++++++++-------- src/cpu/cpu_ppc64_data.h | 8 +++++- 2 files changed, 67 insertions(+), 13 deletions(-)
diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 6c78ab8..db25f27 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -60,6 +60,10 @@ struct ppc64_map { static void ppc64DataFree(virCPUppc64Data *data) { + if (!data) + return; + + VIR_FREE(data->pvr); VIR_FREE(data); }
@@ -67,13 +71,24 @@ static virCPUppc64Data * ppc64DataCopy(const virCPUppc64Data *data) { virCPUppc64Data *copy; + size_t i;
if (VIR_ALLOC(copy) < 0) - return NULL; + goto error; + + if (VIR_ALLOC_N(copy->pvr, data->len) < 0) + goto error; + + copy->len = data->len;
- copy->pvr = data->pvr; + for (i = 0; i < data->len; i++) + copy->pvr[i].value = data->pvr[i].value;
return copy; + + error: + ppc64DataFree(copy); + return NULL; }
static void @@ -159,12 +174,14 @@ ppc64ModelFindPVR(const struct ppc64_map *map, uint32_t pvr) { struct ppc64_model *model; + size_t i;
model = map->models; while (model) { - if (model->data->pvr == pvr) - return model; - + for (i = 0; i < model->data->len; i++) { + if (model->data->pvr[i].value == pvr) + return model; + } model = model->next; }
@@ -257,8 +274,16 @@ ppc64ModelLoad(xmlXPathContextPtr ctxt, struct ppc64_map *map) { struct ppc64_model *model; + xmlNodePtr *nodes = NULL; + xmlNodePtr bookmark; char *vendor = NULL; unsigned long pvr; + size_t i; + int n; + + /* Save the node the context was pointing to, as we're going + * to change it later. It's going to be restored on exit */ + bookmark = ctxt->node;
if (VIR_ALLOC(model) < 0) return -1; @@ -298,14 +323,30 @@ ppc64ModelLoad(xmlXPathContextPtr ctxt, } }
- if (!virXPathBoolean("boolean(./pvr)", ctxt) || - virXPathULongHex("string(./pvr/@value)", ctxt, &pvr) < 0) { + if ((n = virXPathNodeSet("./pvr", ctxt, &nodes)) <= 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Missing or invalid PVR value in CPU model %s"), + _("Missing PVR information for CPU model %s"), model->name); goto ignore; } - model->data->pvr = pvr; + + if (VIR_ALLOC_N(model->data->pvr, n) < 0) + goto ignore; + + model->data->len = n; + + for (i = 0; i < n; i++) { + ctxt->node = nodes[i]; + + if (!virXPathBoolean("boolean(./@value)", ctxt) || + virXPathULongHex("string(./@value)", ctxt, &pvr) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing or invalid PVR value in CPU model %s"), + model->name); + goto ignore;
Wrong indentation, s/^ // in the 4 lines above. ACK once it's fixed. Jirka
On Tue, 2015-08-11 at 10:26 +0200, Jiri Denemark wrote:
+ if (!virXPathBoolean("boolean(./@value)", ctxt) || + virXPathULongHex("string(./@value)", ctxt, &pvr) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing or invalid PVR value in CPU model %s"), + model->name); + goto ignore;
Wrong indentation, s/^ // in the 4 lines above. ACK once it's fixed.
Good catch. I've fixed the indentation and removed the unnecessary call to virXPathBoolean(), as agreed, both here and in patch 15/20. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
Use multiple PVRs per CPU model to reduce the number of models we need to keep track of. Remove specific CPU models (eg. POWER7+_v2.1): the corresponding generic CPU model (eg. POWER7) should be used instead to ensure the guest can be booted on any compatible host. Get rid of all the entries that did not match any of the CPU models supported by QEMU, like power8 and power8e. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1250977 --- src/cpu/cpu_map.xml | 39 +++------------------------------------ 1 file changed, 3 insertions(+), 36 deletions(-) diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index 6387ce4..b3c4477 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -1358,53 +1358,20 @@ <vendor name='Freescale'/> <!-- 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> - - <model name='POWER7+_v2.1'> - <vendor name='IBM'/> - <pvr value='0x004a0201'/> - </model> - - <model name='POWER8_v1.0'> - <vendor name='IBM'/> - <pvr value='0x004b0100'/> - </model> - - <model name='power6'> + <model name='POWER6'> <vendor name='IBM'/> <pvr value='0x003e0000'/> </model> - <model name='power7'> + <model name='POWER7'> <vendor name='IBM'/> <pvr value='0x003f0000'/> - </model> - - <model name='power7+'> - <vendor name='IBM'/> <pvr value='0x004a0000'/> </model> - <model name='power8e'> + <model name='POWER8'> <vendor name='IBM'/> <pvr value='0x004b0000'/> - </model> - - <model name='power8'> - <vendor name='IBM'/> <pvr value='0x004d0000'/> </model> -- 2.4.3
On Mon, Aug 10, 2015 at 10:55:56 +0200, Andrea Bolognani wrote:
Use multiple PVRs per CPU model to reduce the number of models we need to keep track of.
Remove specific CPU models (eg. POWER7+_v2.1): the corresponding generic CPU model (eg. POWER7) should be used instead to ensure the guest can be booted on any compatible host.
Get rid of all the entries that did not match any of the CPU models supported by QEMU, like power8 and power8e.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1250977
ACK Jirka
Instead of relying on a hard-coded mask value, read it from the CPU map XML and use it when looking up models by PVR. --- src/cpu/cpu_map.xml | 14 +++++++------- src/cpu/cpu_ppc64.c | 24 ++++++++++++++---------- src/cpu/cpu_ppc64_data.h | 1 + 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index b3c4477..0895ada 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -1360,30 +1360,30 @@ <!-- IBM-based CPU models --> <model name='POWER6'> <vendor name='IBM'/> - <pvr value='0x003e0000'/> + <pvr value='0x003e0000' mask='0xffff0000'/> </model> <model name='POWER7'> <vendor name='IBM'/> - <pvr value='0x003f0000'/> - <pvr value='0x004a0000'/> + <pvr value='0x003f0000' mask='0xffff0000'/> + <pvr value='0x004a0000' mask='0xffff0000'/> </model> <model name='POWER8'> <vendor name='IBM'/> - <pvr value='0x004b0000'/> - <pvr value='0x004d0000'/> + <pvr value='0x004b0000' mask='0xffff0000'/> + <pvr value='0x004d0000' mask='0xffff0000'/> </model> <!-- Freescale-based CPU models --> <model name='POWERPC_e5500'> <vendor name='Freescale'/> - <pvr value='0x80240000'/> + <pvr value='0x80240000' mask='0xffff0000'/> </model> <model name='POWERPC_e6500'> <vendor name='Freescale'/> - <pvr value='0x80400000'/> + <pvr value='0x80400000' mask='0xffff0000'/> </model> </arch> </cpus> diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index db25f27..bf8f4da 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -81,8 +81,10 @@ ppc64DataCopy(const virCPUppc64Data *data) copy->len = data->len; - for (i = 0; i < data->len; i++) + for (i = 0; i < data->len; i++) { copy->pvr[i].value = data->pvr[i].value; + copy->pvr[i].mask = data->pvr[i].mask; + } return copy; @@ -179,20 +181,12 @@ ppc64ModelFindPVR(const struct ppc64_map *map, model = map->models; while (model) { for (i = 0; i < model->data->len; i++) { - if (model->data->pvr[i].value == pvr) + if ((pvr & model->data->pvr[i].mask) == model->data->pvr[i].value) return model; } model = model->next; } - /* PowerPC Processor Version Register is interpreted as follows : - * Higher order 16 bits : Power ISA generation. - * Lower order 16 bits : CPU chip version number. - * If the exact CPU isn't found, return the nearest matching CPU generation - */ - if (pvr & 0x0000FFFFul) - return ppc64ModelFindPVR(map, (pvr & 0xFFFF0000ul)); - return NULL; } @@ -346,6 +340,15 @@ ppc64ModelLoad(xmlXPathContextPtr ctxt, goto ignore; } model->data->pvr[i].value = pvr; + + if (!virXPathBoolean("boolean(./@mask)", ctxt) || + virXPathULongHex("string(./@mask)", ctxt, &pvr) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing or invalid PVR mask in CPU model %s"), + model->name); + goto ignore; + } + model->data->pvr[i].mask = pvr; } if (!map->models) { @@ -609,6 +612,7 @@ ppc64DriverNodeData(virArch arch) asm("mfpvr %0" : "=r" (data->pvr[0].value)); #endif + data->pvr[0].mask = 0xfffffffful; nodeData->arch = arch; diff --git a/src/cpu/cpu_ppc64_data.h b/src/cpu/cpu_ppc64_data.h index 0d3cb0b..c0a130e 100644 --- a/src/cpu/cpu_ppc64_data.h +++ b/src/cpu/cpu_ppc64_data.h @@ -29,6 +29,7 @@ typedef struct _virCPUppc64PVR virCPUppc64PVR; struct _virCPUppc64PVR { uint32_t value; + uint32_t mask; }; typedef struct _virCPUppc64Data virCPUppc64Data; -- 2.4.3
On Mon, Aug 10, 2015 at 10:55:57 +0200, Andrea Bolognani wrote:
Instead of relying on a hard-coded mask value, read it from the CPU map XML and use it when looking up models by PVR.
ACK Jirka
This is yet another variation of POWER8. The PVR information comes from arch/powerpc/kernel/cputable.c in the Linux kernel tree. --- src/cpu/cpu_map.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index 0895ada..d2469ee 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -1372,6 +1372,7 @@ <model name='POWER8'> <vendor name='IBM'/> <pvr value='0x004b0000' mask='0xffff0000'/> + <pvr value='0x004c0000' mask='0xffff0000'/> <pvr value='0x004d0000' mask='0xffff0000'/> </model> -- 2.4.3
On Mon, Aug 10, 2015 at 10:55:58 +0200, Andrea Bolognani wrote:
This is yet another variation of POWER8. The PVR information comes from arch/powerpc/kernel/cputable.c in the Linux kernel tree. --- src/cpu/cpu_map.xml | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index 0895ada..d2469ee 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -1372,6 +1372,7 @@ <model name='POWER8'> <vendor name='IBM'/> <pvr value='0x004b0000' mask='0xffff0000'/> + <pvr value='0x004c0000' mask='0xffff0000'/> <pvr value='0x004d0000' mask='0xffff0000'/> </model>
ACK Jirka
All previously recognized CPU models (POWER7_v2.1, POWER7_v2.3, POWER7+_v2.1 and POWER8_v1.0) are internally converted to the corrisponding generation name so that existing guests don't stop working. --- src/cpu/cpu_ppc64.c | 36 ++++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index bf8f4da..d87e0d1 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -57,6 +57,33 @@ struct ppc64_map { struct ppc64_model *models; }; +/* Convert a legacy CPU definition by transforming + * model names to generation names: + * POWER7_v2.1 => POWER7 + * POWER7_v2.3 => POWER7 + * POWER7+_v2.1 => POWER7 + * POWER8_v1.0 => POWER8 */ +static virCPUDefPtr +ppc64LegacyConvertCPUDef(const virCPUDef *legacy) +{ + virCPUDefPtr cpu; + + if (!(cpu = virCPUDefCopy(legacy))) + goto out; + + if (!(STREQ(cpu->model, "POWER7_v2.1") || + STREQ(cpu->model, "POWER7_v2.3") || + STREQ(cpu->model, "POWER7+_v2.1") || + STREQ(cpu->model, "POWER8_v1.0"))) { + goto out; + } + + cpu->model[strlen("POWERx")] = 0; + + out: + return cpu; +} + static void ppc64DataFree(virCPUppc64Data *data) { @@ -426,18 +453,22 @@ ppc64MakeCPUData(virArch arch, static virCPUCompareResult ppc64Compute(virCPUDefPtr host, - const virCPUDef *cpu, + const virCPUDef *other, virCPUDataPtr *guestData, char **message) { struct ppc64_map *map = NULL; struct ppc64_model *host_model = NULL; struct ppc64_model *guest_model = NULL; - + virCPUDefPtr cpu = NULL; virCPUCompareResult ret = VIR_CPU_COMPARE_ERROR; virArch arch; size_t i; + /* Ensure existing configurations are handled correctly */ + if (!(cpu = ppc64LegacyConvertCPUDef(other))) + goto cleanup; + if (cpu->arch != VIR_ARCH_NONE) { bool found = false; @@ -506,6 +537,7 @@ ppc64Compute(virCPUDefPtr host, ret = VIR_CPU_COMPARE_IDENTICAL; cleanup: + virCPUDefFree(cpu); ppc64MapFree(map); ppc64ModelFree(host_model); ppc64ModelFree(guest_model); -- 2.4.3
On Mon, Aug 10, 2015 at 10:55:59 +0200, Andrea Bolognani wrote:
All previously recognized CPU models (POWER7_v2.1, POWER7_v2.3, POWER7+_v2.1 and POWER8_v1.0) are internally converted to the corrisponding generation name so that existing guests don't stop working. --- src/cpu/cpu_ppc64.c | 36 ++++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-)
diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index bf8f4da..d87e0d1 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -57,6 +57,33 @@ struct ppc64_map { struct ppc64_model *models; };
+/* Convert a legacy CPU definition by transforming + * model names to generation names: + * POWER7_v2.1 => POWER7 + * POWER7_v2.3 => POWER7 + * POWER7+_v2.1 => POWER7 + * POWER8_v1.0 => POWER8 */ +static virCPUDefPtr +ppc64LegacyConvertCPUDef(const virCPUDef *legacy)
I think ppc64ConvertLegacyCPUDef would be a slightly better name.
+{ + virCPUDefPtr cpu; + + if (!(cpu = virCPUDefCopy(legacy))) + goto out; + + if (!(STREQ(cpu->model, "POWER7_v2.1") || + STREQ(cpu->model, "POWER7_v2.3") || + STREQ(cpu->model, "POWER7+_v2.1") || + STREQ(cpu->model, "POWER8_v1.0"))) { + goto out; + } + + cpu->model[strlen("POWERx")] = 0; + + out: + return cpu; +} + static void ppc64DataFree(virCPUppc64Data *data) { @@ -426,18 +453,22 @@ ppc64MakeCPUData(virArch arch,
static virCPUCompareResult ppc64Compute(virCPUDefPtr host, - const virCPUDef *cpu, + const virCPUDef *other, virCPUDataPtr *guestData, char **message) { struct ppc64_map *map = NULL; struct ppc64_model *host_model = NULL; struct ppc64_model *guest_model = NULL; - + virCPUDefPtr cpu = NULL; virCPUCompareResult ret = VIR_CPU_COMPARE_ERROR; virArch arch; size_t i;
+ /* Ensure existing configurations are handled correctly */ + if (!(cpu = ppc64LegacyConvertCPUDef(other))) + goto cleanup; + if (cpu->arch != VIR_ARCH_NONE) { bool found = false;
When reading the tests, I realized we don't do any compatibility hacks for *Baseline, which is correct because it never worked on Power, but we should explicitly document it inside ppc64Baseline to avoid confusion in the future.
@@ -506,6 +537,7 @@ ppc64Compute(virCPUDefPtr host, ret = VIR_CPU_COMPARE_IDENTICAL;
cleanup: + virCPUDefFree(cpu); ppc64MapFree(map); ppc64ModelFree(host_model); ppc64ModelFree(guest_model);
ACK with the comment added. Jirka
On Tue, 2015-08-11 at 15:17 +0200, Jiri Denemark wrote:
+/* Convert a legacy CPU definition by transforming + * model names to generation names: + * POWER7_v2.1 => POWER7 + * POWER7_v2.3 => POWER7 + * POWER7+_v2.1 => POWER7 + * POWER8_v1.0 => POWER8 */ +static virCPUDefPtr +ppc64LegacyConvertCPUDef(const virCPUDef *legacy)
I think ppc64ConvertLegacyCPUDef would be a slightly better name.
Agreed, I've changed the name as per your suggestion.
When reading the tests, I realized we don't do any compatibility hacks for *Baseline, which is correct because it never worked on Power, but we should explicitly document it inside ppc64Baseline to avoid confusion in the future.
I've added a comment explaining the reason why we can't handle that situation any better. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
Unlike what happens on x86, on ppc64 you can't mix and match CPU features to obtain the guest CPU you want regardless of the host CPU, so the concept of model fallback doesn't apply. Make sure CPU definitions emitted by the driver, eg. as output of the cpuBaseline() and cpuUpdate() calls, reflect this fact. --- src/cpu/cpu_ppc64.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index d87e0d1..0f193cd 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -672,6 +672,7 @@ ppc64DriverUpdate(virCPUDefPtr guest, case VIR_CPU_MODE_HOST_MODEL: case VIR_CPU_MODE_HOST_PASSTHROUGH: guest->match = VIR_CPU_MATCH_EXACT; + guest->fallback = VIR_CPU_FALLBACK_FORBID; virCPUDefFreeModel(guest); return virCPUDefCopyModel(guest, host, true); @@ -759,6 +760,7 @@ ppc64DriverBaseline(virCPUDefPtr *cpus, cpu->type = VIR_CPU_TYPE_GUEST; cpu->match = VIR_CPU_MATCH_EXACT; + cpu->fallback = VIR_CPU_FALLBACK_FORBID; cleanup: ppc64MapFree(map); -- 2.4.3
On Mon, Aug 10, 2015 at 10:56:00 +0200, Andrea Bolognani wrote:
Unlike what happens on x86, on ppc64 you can't mix and match CPU features to obtain the guest CPU you want regardless of the host CPU, so the concept of model fallback doesn't apply.
Make sure CPU definitions emitted by the driver, eg. as output of the cpuBaseline() and cpuUpdate() calls, reflect this fact.
ACK Jirka
Now that all the changes have been implemented we can run the test cases once again, after updating them to reflect the new behaviour. --- tests/cputest.c | 16 ++++------------ .../cputestdata/ppc64-baseline-incompatible-vendors.xml | 4 ++-- tests/cputestdata/ppc64-baseline-no-vendor-result.xml | 2 +- tests/cputestdata/ppc64-baseline-no-vendor.xml | 2 +- tests/cputestdata/ppc64-exact.xml | 3 --- tests/cputestdata/ppc64-guest-exact.xml | 3 +++ tests/cputestdata/ppc64-guest-nofallback.xml | 3 +-- tests/cputestdata/ppc64-guest-strict.xml | 3 +++ tests/cputestdata/ppc64-guest.xml | 3 +-- tests/cputestdata/ppc64-host+guest,ppc_models-result.xml | 2 +- tests/cputestdata/ppc64-host.xml | 2 +- tests/cputestdata/ppc64-strict.xml | 3 --- 12 files changed, 18 insertions(+), 28 deletions(-) delete mode 100644 tests/cputestdata/ppc64-exact.xml create mode 100644 tests/cputestdata/ppc64-guest-exact.xml create mode 100644 tests/cputestdata/ppc64-guest-strict.xml delete mode 100644 tests/cputestdata/ppc64-strict.xml diff --git a/tests/cputest.c b/tests/cputest.c index 4dbccfd..09f690a 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -501,9 +501,7 @@ static const char *model486[] = { "486" }; static const char *nomodel[] = { "nomodel" }; static const char *models[] = { "qemu64", "core2duo", "Nehalem" }; static const char *haswell[] = { "SandyBridge", "Haswell" }; -/* XXX temporarily disabled -static const char *ppc_models[] = { "POWER7", "POWER7_v2.1", "POWER7_v2.3", "POWER8_v1.0"}; -*/ +static const char *ppc_models[] = { "POWER6", "POWER7", "POWER8" }; static int mymain(void) @@ -597,10 +595,8 @@ 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); - /* XXX temporarily disabled - DO_TEST_COMPARE("ppc64", "host", "strict", VIR_CPU_COMPARE_IDENTICAL); - DO_TEST_COMPARE("ppc64", "host", "exact", VIR_CPU_COMPARE_INCOMPATIBLE); - */ + DO_TEST_COMPARE("ppc64", "host", "guest-strict", VIR_CPU_COMPARE_IDENTICAL); + DO_TEST_COMPARE("ppc64", "host", "guest-exact", VIR_CPU_COMPARE_INCOMPATIBLE); /* guest updates for migration * automatically compares host CPU with the result */ @@ -629,10 +625,8 @@ mymain(void) DO_TEST_BASELINE("x86", "7", 0, 0); DO_TEST_BASELINE("x86", "8", 0, 0); - /* XXX temporarily disabled 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); @@ -668,10 +662,8 @@ mymain(void) DO_TEST_GUESTDATA("x86", "host-Haswell-noTSX", "Haswell-noTSX", NULL, "Haswell-noTSX", 0); - /* XXX temporarily disabled DO_TEST_GUESTDATA("ppc64", "host", "guest", ppc_models, NULL, 0); - DO_TEST_GUESTDATA("ppc64", "host", "guest-nofallback", ppc_models, "POWER7_v2.1", -1); - */ + DO_TEST_GUESTDATA("ppc64", "host", "guest-nofallback", ppc_models, "POWER8", -1); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/cputestdata/ppc64-baseline-incompatible-vendors.xml b/tests/cputestdata/ppc64-baseline-incompatible-vendors.xml index 97d3c9c..9e67e9d 100644 --- a/tests/cputestdata/ppc64-baseline-incompatible-vendors.xml +++ b/tests/cputestdata/ppc64-baseline-incompatible-vendors.xml @@ -1,13 +1,13 @@ <cpuTest> <cpu> <arch>ppc64</arch> - <model>POWER7+_v2.1</model> + <model>POWER7</model> <vendor>Intel</vendor> <topology sockets='2' cores='4' threads='1'/> </cpu> <cpu> <arch>ppc64</arch> - <model>POWER8_v1.0</model> + <model>POWER8</model> <vendor>Intel</vendor> <topology sockets='1' cores='1' threads='1'/> </cpu> diff --git a/tests/cputestdata/ppc64-baseline-no-vendor-result.xml b/tests/cputestdata/ppc64-baseline-no-vendor-result.xml index 36bae52..758099c 100644 --- a/tests/cputestdata/ppc64-baseline-no-vendor-result.xml +++ b/tests/cputestdata/ppc64-baseline-no-vendor-result.xml @@ -1,3 +1,3 @@ <cpu mode='custom' match='exact'> - <model fallback='allow'>POWER7_v2.3</model> + <model fallback='forbid'>POWER7</model> </cpu> diff --git a/tests/cputestdata/ppc64-baseline-no-vendor.xml b/tests/cputestdata/ppc64-baseline-no-vendor.xml index 5e69a62..6d8dd0d 100644 --- a/tests/cputestdata/ppc64-baseline-no-vendor.xml +++ b/tests/cputestdata/ppc64-baseline-no-vendor.xml @@ -1,7 +1,7 @@ <cpuTest> <cpu> <arch>ppc64</arch> - <model>POWER7_v2.3</model> + <model>POWER7</model> <topology sockets='2' cores='4' threads='1'/> </cpu> </cpuTest> diff --git a/tests/cputestdata/ppc64-exact.xml b/tests/cputestdata/ppc64-exact.xml deleted file mode 100644 index c84f16a..0000000 --- a/tests/cputestdata/ppc64-exact.xml +++ /dev/null @@ -1,3 +0,0 @@ -<cpu match='exact'> - <model>POWER8_v1.0</model> -</cpu> diff --git a/tests/cputestdata/ppc64-guest-exact.xml b/tests/cputestdata/ppc64-guest-exact.xml new file mode 100644 index 0000000..f416a59 --- /dev/null +++ b/tests/cputestdata/ppc64-guest-exact.xml @@ -0,0 +1,3 @@ +<cpu match='exact'> + <model>POWER8</model> +</cpu> diff --git a/tests/cputestdata/ppc64-guest-nofallback.xml b/tests/cputestdata/ppc64-guest-nofallback.xml index 42026b4..070f006 100644 --- a/tests/cputestdata/ppc64-guest-nofallback.xml +++ b/tests/cputestdata/ppc64-guest-nofallback.xml @@ -1,4 +1,3 @@ <cpu match='exact'> - <model fallback='forbid'>POWER7_v2.1</model> - <topology sockets='2' cores='4' threads='1'/> + <model fallback='forbid'>POWER8</model> </cpu> diff --git a/tests/cputestdata/ppc64-guest-strict.xml b/tests/cputestdata/ppc64-guest-strict.xml new file mode 100644 index 0000000..217dfc7 --- /dev/null +++ b/tests/cputestdata/ppc64-guest-strict.xml @@ -0,0 +1,3 @@ +<cpu match='strict'> + <model>POWER7</model> +</cpu> diff --git a/tests/cputestdata/ppc64-guest.xml b/tests/cputestdata/ppc64-guest.xml index 9e91501..a60c59f 100644 --- a/tests/cputestdata/ppc64-guest.xml +++ b/tests/cputestdata/ppc64-guest.xml @@ -1,4 +1,3 @@ <cpu match='exact'> - <model>POWER7_v2.3</model> - <topology sockets='2' cores='4' threads='1'/> + <model fallback='allow'>POWER7</model> </cpu> diff --git a/tests/cputestdata/ppc64-host+guest,ppc_models-result.xml b/tests/cputestdata/ppc64-host+guest,ppc_models-result.xml index 3e55f68..3548c0e 100644 --- a/tests/cputestdata/ppc64-host+guest,ppc_models-result.xml +++ b/tests/cputestdata/ppc64-host+guest,ppc_models-result.xml @@ -1,5 +1,5 @@ <cpu mode='custom' match='exact'> <arch>ppc64</arch> - <model fallback='allow'>POWER7_v2.3</model> + <model fallback='allow'>POWER7</model> <vendor>IBM</vendor> </cpu> diff --git a/tests/cputestdata/ppc64-host.xml b/tests/cputestdata/ppc64-host.xml index 39cb741..0ac5c4e 100644 --- a/tests/cputestdata/ppc64-host.xml +++ b/tests/cputestdata/ppc64-host.xml @@ -1,6 +1,6 @@ <cpu> <arch>ppc64</arch> - <model>POWER7_v2.3</model> + <model>POWER7</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 deleted file mode 100644 index e91c6e7..0000000 --- a/tests/cputestdata/ppc64-strict.xml +++ /dev/null @@ -1,3 +0,0 @@ -<cpu match='exact'> - <model>POWER7_v2.3</model> -</cpu> -- 2.4.3
On Mon, Aug 10, 2015 at 10:56:01 +0200, Andrea Bolognani wrote:
Now that all the changes have been implemented we can run the test cases once again, after updating them to reflect the new behaviour.
ACK Jirka
The test cases cover the cpuCompare(), cpuBaseline() and cpuNodeData() implementation. --- tests/cputest.c | 16 ++++++++++++++++ tests/cputestdata/ppc64-baseline-incompatible-models.xml | 14 ++++++++++++++ tests/cputestdata/ppc64-baseline-legacy.xml | 14 ++++++++++++++ tests/cputestdata/ppc64-baseline-same-model-result.xml | 3 +++ tests/cputestdata/ppc64-baseline-same-model.xml | 14 ++++++++++++++ tests/cputestdata/ppc64-guest-legacy-incompatible.xml | 3 +++ tests/cputestdata/ppc64-guest-legacy-invalid.xml | 3 +++ tests/cputestdata/ppc64-guest-legacy.xml | 3 +++ .../ppc64-host+guest-legacy,ppc_models-result.xml | 5 +++++ tests/cputestdata/ppc64-host-better.xml | 6 ++++++ tests/cputestdata/ppc64-host-incomp-arch.xml | 6 ++++++ tests/cputestdata/ppc64-host-no-vendor.xml | 5 +++++ tests/cputestdata/ppc64-host-worse.xml | 6 ++++++ 13 files changed, 98 insertions(+) create mode 100644 tests/cputestdata/ppc64-baseline-incompatible-models.xml create mode 100644 tests/cputestdata/ppc64-baseline-legacy.xml create mode 100644 tests/cputestdata/ppc64-baseline-same-model-result.xml create mode 100644 tests/cputestdata/ppc64-baseline-same-model.xml create mode 100644 tests/cputestdata/ppc64-guest-legacy-incompatible.xml create mode 100644 tests/cputestdata/ppc64-guest-legacy-invalid.xml create mode 100644 tests/cputestdata/ppc64-guest-legacy.xml create mode 100644 tests/cputestdata/ppc64-host+guest-legacy,ppc_models-result.xml create mode 100644 tests/cputestdata/ppc64-host-better.xml create mode 100644 tests/cputestdata/ppc64-host-incomp-arch.xml create mode 100644 tests/cputestdata/ppc64-host-no-vendor.xml create mode 100644 tests/cputestdata/ppc64-host-worse.xml diff --git a/tests/cputest.c b/tests/cputest.c index 09f690a..5992dd0 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -571,6 +571,13 @@ mymain(void) DO_TEST_COMPARE("x86", "host", "host-no-vendor", VIR_CPU_COMPARE_IDENTICAL); DO_TEST_COMPARE("x86", "host-no-vendor", "host", VIR_CPU_COMPARE_INCOMPATIBLE); + DO_TEST_COMPARE("ppc64", "host", "host", VIR_CPU_COMPARE_IDENTICAL); + DO_TEST_COMPARE("ppc64", "host", "host-better", VIR_CPU_COMPARE_INCOMPATIBLE); + DO_TEST_COMPARE("ppc64", "host", "host-worse", VIR_CPU_COMPARE_INCOMPATIBLE); + DO_TEST_COMPARE("ppc64", "host", "host-incomp-arch", VIR_CPU_COMPARE_INCOMPATIBLE); + DO_TEST_COMPARE("ppc64", "host", "host-no-vendor", VIR_CPU_COMPARE_IDENTICAL); + DO_TEST_COMPARE("ppc64", "host-no-vendor", "host", VIR_CPU_COMPARE_INCOMPATIBLE); + /* guest to host comparison */ DO_TEST_COMPARE("x86", "host", "bogus-model", VIR_CPU_COMPARE_ERROR); DO_TEST_COMPARE("x86", "host", "bogus-feature", VIR_CPU_COMPARE_ERROR); @@ -597,6 +604,9 @@ mymain(void) DO_TEST_COMPARE("ppc64", "host", "guest-strict", VIR_CPU_COMPARE_IDENTICAL); DO_TEST_COMPARE("ppc64", "host", "guest-exact", VIR_CPU_COMPARE_INCOMPATIBLE); + DO_TEST_COMPARE("ppc64", "host", "guest-legacy", VIR_CPU_COMPARE_IDENTICAL); + DO_TEST_COMPARE("ppc64", "host", "guest-legacy-incompatible", VIR_CPU_COMPARE_INCOMPATIBLE); + DO_TEST_COMPARE("ppc64", "host", "guest-legacy-invalid", VIR_CPU_COMPARE_ERROR); /* guest updates for migration * automatically compares host CPU with the result */ @@ -627,6 +637,9 @@ mymain(void) DO_TEST_BASELINE("ppc64", "incompatible-vendors", 0, -1); DO_TEST_BASELINE("ppc64", "no-vendor", 0, 0); + DO_TEST_BASELINE("ppc64", "incompatible-models", 0, -1); + DO_TEST_BASELINE("ppc64", "same-model", 0, 0); + DO_TEST_BASELINE("ppc64", "legacy", 0, -1); /* CPU features */ DO_TEST_HASFEATURE("x86", "host", "vmx", YES); @@ -664,6 +677,9 @@ mymain(void) DO_TEST_GUESTDATA("ppc64", "host", "guest", ppc_models, NULL, 0); DO_TEST_GUESTDATA("ppc64", "host", "guest-nofallback", ppc_models, "POWER8", -1); + DO_TEST_GUESTDATA("ppc64", "host", "guest-legacy", ppc_models, NULL, 0); + DO_TEST_GUESTDATA("ppc64", "host", "guest-legacy-incompatible", ppc_models, NULL, -1); + DO_TEST_GUESTDATA("ppc64", "host", "guest-legacy-invalid", ppc_models, NULL, -1); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/cputestdata/ppc64-baseline-incompatible-models.xml b/tests/cputestdata/ppc64-baseline-incompatible-models.xml new file mode 100644 index 0000000..7e7b9a6 --- /dev/null +++ b/tests/cputestdata/ppc64-baseline-incompatible-models.xml @@ -0,0 +1,14 @@ +<cpuTest> +<cpu> + <arch>ppc64</arch> + <model>POWER7</model> + <vendor>IBM</vendor> + <topology sockets='2' cores='4' threads='1'/> +</cpu> +<cpu> + <arch>ppc64</arch> + <model>POWER8</model> + <vendor>IBM</vendor> + <topology sockets='1' cores='1' threads='1'/> +</cpu> +</cpuTest> diff --git a/tests/cputestdata/ppc64-baseline-legacy.xml b/tests/cputestdata/ppc64-baseline-legacy.xml new file mode 100644 index 0000000..b652497 --- /dev/null +++ b/tests/cputestdata/ppc64-baseline-legacy.xml @@ -0,0 +1,14 @@ +<cpuTest> +<cpu> + <arch>ppc64</arch> + <model>POWER8</model> + <vendor>IBM</vendor> + <topology sockets='2' cores='4' threads='1'/> +</cpu> +<cpu> + <arch>ppc64</arch> + <model>power8e</model> + <vendor>IBM</vendor> + <topology sockets='1' cores='1' threads='1'/> +</cpu> +</cpuTest> diff --git a/tests/cputestdata/ppc64-baseline-same-model-result.xml b/tests/cputestdata/ppc64-baseline-same-model-result.xml new file mode 100644 index 0000000..0223018 --- /dev/null +++ b/tests/cputestdata/ppc64-baseline-same-model-result.xml @@ -0,0 +1,3 @@ +<cpu mode='custom' match='exact'> + <model fallback='forbid'>POWER8</model> +</cpu> diff --git a/tests/cputestdata/ppc64-baseline-same-model.xml b/tests/cputestdata/ppc64-baseline-same-model.xml new file mode 100644 index 0000000..dceae83 --- /dev/null +++ b/tests/cputestdata/ppc64-baseline-same-model.xml @@ -0,0 +1,14 @@ +<cpuTest> +<cpu> + <arch>ppc64</arch> + <model>POWER8</model> + <vendor>IBM</vendor> + <topology sockets='2' cores='4' threads='1'/> +</cpu> +<cpu> + <arch>ppc64</arch> + <model>POWER8</model> + <vendor>IBM</vendor> + <topology sockets='1' cores='1' threads='1'/> +</cpu> +</cpuTest> diff --git a/tests/cputestdata/ppc64-guest-legacy-incompatible.xml b/tests/cputestdata/ppc64-guest-legacy-incompatible.xml new file mode 100644 index 0000000..f5b8384 --- /dev/null +++ b/tests/cputestdata/ppc64-guest-legacy-incompatible.xml @@ -0,0 +1,3 @@ +<cpu mode='custom' match='exact'> + <model fallback='allow'>POWER8_v1.0</model> +</cpu> diff --git a/tests/cputestdata/ppc64-guest-legacy-invalid.xml b/tests/cputestdata/ppc64-guest-legacy-invalid.xml new file mode 100644 index 0000000..be059c3 --- /dev/null +++ b/tests/cputestdata/ppc64-guest-legacy-invalid.xml @@ -0,0 +1,3 @@ +<cpu mode='custom' match='exact'> + <model fallback='allow'>POWER8E_v1.0</model> +</cpu> diff --git a/tests/cputestdata/ppc64-guest-legacy.xml b/tests/cputestdata/ppc64-guest-legacy.xml new file mode 100644 index 0000000..36bae52 --- /dev/null +++ b/tests/cputestdata/ppc64-guest-legacy.xml @@ -0,0 +1,3 @@ +<cpu mode='custom' match='exact'> + <model fallback='allow'>POWER7_v2.3</model> +</cpu> diff --git a/tests/cputestdata/ppc64-host+guest-legacy,ppc_models-result.xml b/tests/cputestdata/ppc64-host+guest-legacy,ppc_models-result.xml new file mode 100644 index 0000000..3548c0e --- /dev/null +++ b/tests/cputestdata/ppc64-host+guest-legacy,ppc_models-result.xml @@ -0,0 +1,5 @@ +<cpu mode='custom' match='exact'> + <arch>ppc64</arch> + <model fallback='allow'>POWER7</model> + <vendor>IBM</vendor> +</cpu> diff --git a/tests/cputestdata/ppc64-host-better.xml b/tests/cputestdata/ppc64-host-better.xml new file mode 100644 index 0000000..af87412 --- /dev/null +++ b/tests/cputestdata/ppc64-host-better.xml @@ -0,0 +1,6 @@ +<cpu> + <arch>ppc64</arch> + <model>POWER8</model> + <vendor>IBM</vendor> + <topology sockets='1' cores='64' threads='1'/> +</cpu> diff --git a/tests/cputestdata/ppc64-host-incomp-arch.xml b/tests/cputestdata/ppc64-host-incomp-arch.xml new file mode 100644 index 0000000..195f436 --- /dev/null +++ b/tests/cputestdata/ppc64-host-incomp-arch.xml @@ -0,0 +1,6 @@ +<cpu> + <arch>x86_64</arch> + <model>POWER7</model> + <vendor>IBM</vendor> + <topology sockets='1' cores='64' threads='1'/> +</cpu> diff --git a/tests/cputestdata/ppc64-host-no-vendor.xml b/tests/cputestdata/ppc64-host-no-vendor.xml new file mode 100644 index 0000000..de73006 --- /dev/null +++ b/tests/cputestdata/ppc64-host-no-vendor.xml @@ -0,0 +1,5 @@ +<cpu> + <arch>ppc64</arch> + <model>POWER7</model> + <topology sockets='1' cores='64' threads='1'/> +</cpu> diff --git a/tests/cputestdata/ppc64-host-worse.xml b/tests/cputestdata/ppc64-host-worse.xml new file mode 100644 index 0000000..ba1806b --- /dev/null +++ b/tests/cputestdata/ppc64-host-worse.xml @@ -0,0 +1,6 @@ +<cpu> + <arch>ppc64</arch> + <model>POWER6</model> + <vendor>IBM</vendor> + <topology sockets='1' cores='64' threads='1'/> +</cpu> -- 2.4.3
On Mon, 2015-08-10 at 10:55 +0200, Andrea Bolognani wrote:
Patches 01-07 are cleanups, 08-10 are bug fixes and 11-20 improve the driver.
Changes in v3:
* Fix a bug spotted by John thanks to Coverity
Changes in v2:
* Implement compatibility with guests defined on older libvirt versions
* Always use fallback='forbid' when emitting CPU definitions
* Update all tests in one go instead of changing them several times
* Handle test failure and system failure differently in cpuTestGuestData()
* Simplify ppc64DriverNodeData()
* Simplify XML parsing code
* Remove unnecessary NULL checks
* Add even more test cases
* Other small changes such as shuffling chunks between commits to make the history more readable
Pushed after addressing your concerns. Thanks for the review :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
participants (2)
-
Andrea Bolognani -
Jiri Denemark