[libvirt] [PATCH 00/18] cpu: Fix and improve the ppc64 driver
This series depends on [PATCH 0/4] cpu: Rename {powerpc,ppc} => ppc64 which has been already ACKed[1] and is pending upload. Patches 1-11 are fixes and cleanups that don't introduce any new feature in the driver and just lay the groundwork for the second part of the series. Patches 12-18 rework the driver and add some test cases to cover both existing an new features. Cheers. [1] https://www.redhat.com/archives/libvir-list/2015-July/msg01170.html Andrea Bolognani (18): cpu: Mark driver functions in ppc64 driver cpu: Simplify NULL handling in ppc64 driver cpu: Add NULL check in ppc64ModelCopy() cpu: Use a different name for the copy in ppc64ModelFromCPU() cpu: Reorder functions in the ppc64 driver cpu: Remove ISA information from CPU map XML tests: Remove unused file tests: Known failing tests should never succeed cpu: Don't skip CPU model name check in ppc64 driver cpu: CPU model names have to match on ppc64 cpu: Use ppc64Compute() to implement ppc64DriverCompare() cpu: Align ppc64 CPU data with x86 cpu: Support multiple PVRs in the ppc64 driver cpu: Simplify ppc64 part of CPU map XML cpu: Add PVR mask to CPU map XML for ppc64 models cpu: Parse and use PVR masks in the ppc64 driver cpu: Add POWER8 NVL information to CPU map XML tests: Add a bunch of ppc64 cases to the cpu test src/cpu/cpu.h | 2 +- src/cpu/cpu_map.xml | 59 +-- src/cpu/cpu_ppc64.c | 432 +++++++++++++-------- src/cpu/cpu_ppc64_data.h | 12 +- tests/cputest.c | 34 +- tests/cputestdata/ppc64-baseline-1-result.xml | 3 - .../ppc64-baseline-incompatible-models.xml | 14 + .../ppc64-baseline-incompatible-vendors.xml | 4 +- .../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 | 2 +- tests/cputestdata/ppc64-guest-nofallback.xml | 2 +- tests/cputestdata/ppc64-guest.xml | 2 +- .../ppc64-host+guest,ppc_models-result.xml | 2 +- ...st-nofallback,ppc_models,POWER7_v2.1-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 + tests/cputestdata/ppc64-host.xml | 2 +- tests/cputestdata/ppc64-strict.xml | 4 +- 23 files changed, 385 insertions(+), 238 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-same-model-result.xml create mode 100644 tests/cputestdata/ppc64-baseline-same-model.xml delete mode 100644 tests/cputestdata/ppc64-host+guest-nofallback,ppc_models,POWER7_v2.1-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 -- 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 Tue, Aug 04, 2015 at 11:37:52 +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 Tue, Aug 04, 2015 at 11:37:53 +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. --- src/cpu/cpu_ppc64.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-)
ACK Jirka
--- src/cpu/cpu_ppc64.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 05ff8f2..dd02a3f 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -115,6 +115,9 @@ ppc64ModelCopy(const struct ppc64_model *model) { struct ppc64_model *copy; + if (!model) + return NULL; + if (VIR_ALLOC(copy) < 0 || VIR_STRDUP(copy->name, model->name) < 0) { ppc64ModelFree(copy); -- 2.4.3
On Tue, Aug 04, 2015 at 11:37:54 +0200, Andrea Bolognani wrote:
--- src/cpu/cpu_ppc64.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 05ff8f2..dd02a3f 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -115,6 +115,9 @@ ppc64ModelCopy(const struct ppc64_model *model) { struct ppc64_model *copy;
+ if (!model) + return NULL; + if (VIR_ALLOC(copy) < 0 || VIR_STRDUP(copy->name, model->name) < 0) { ppc64ModelFree(copy);
This doesn't seem to be really necessary since the function is not called with model == NULL and I don't think that should change. If any caller wants to pass NULL for a model, it should rather report an error and return instead of trying to copy this NULL. However, the only called of ppc64ModelCopy is pretty confusing: static struct ppc64_model * ppc64ModelFromCPU(const virCPUDef *cpu, const struct ppc64_map *map) { struct ppc64_model *model; if (!(model = ppc64ModelFind(map, cpu->model))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown CPU model %s"), cpu->model); goto error; } if (!(model = ppc64ModelCopy(model))) goto error; return model; error: ppc64ModelFree(model); return NULL; } It uses "model" for pointing to a model which must not be freed and also to its copy which has to be freed. There's no bug here but I think changing is a good idea :-) Jirka
On Wed, 2015-08-05 at 14:19 +0200, Jiri Denemark wrote:
On Tue, Aug 04, 2015 at 11:37:54 +0200, Andrea Bolognani wrote:
--- src/cpu/cpu_ppc64.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 05ff8f2..dd02a3f 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -115,6 +115,9 @@ ppc64ModelCopy(const struct ppc64_model *model) { struct ppc64_model *copy;
+ if (!model) + return NULL; + if (VIR_ALLOC(copy) < 0 || VIR_STRDUP(copy->name, model->name) < 0) { ppc64ModelFree(copy);
This doesn't seem to be really necessary since the function is not called with model == NULL and I don't think that should change. If any caller wants to pass NULL for a model, it should rather report an error and return instead of trying to copy this NULL.
I've dropped this commit and removed a similar check from ppc64DataCopy(), which will be introduced later in the series. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
While the previous code was correct, it looked wrong at first sight because the same variable used to store the result of a map lookup is later used to store a copy of said result. The copy is deallocated on error, but due to the fact that a single variable is used, it looks like the result of the lookup is deallocated instead, which would be a bug. Using a separate variable for the copy means the code is just as correct but much less likely to result confusing. No functional changes. --- src/cpu/cpu_ppc64.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index dd02a3f..c769221 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -162,6 +162,7 @@ ppc64ModelFromCPU(const virCPUDef *cpu, const struct ppc64_map *map) { struct ppc64_model *model; + struct ppc64_model *copy; if (!(model = ppc64ModelFind(map, cpu->model))) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -169,13 +170,13 @@ ppc64ModelFromCPU(const virCPUDef *cpu, goto error; } - if (!(model = ppc64ModelCopy(model))) + if (!(copy = ppc64ModelCopy(model))) goto error; - return model; + return copy; error: - ppc64ModelFree(model); + ppc64ModelFree(copy); return NULL; } -- 2.4.3
On Tue, Aug 04, 2015 at 11:37:55 +0200, Andrea Bolognani wrote:
While the previous code was correct, it looked wrong at first sight because the same variable used to store the result of a map lookup is later used to store a copy of said result. The copy is deallocated on error, but due to the fact that a single variable is used, it looks like the result of the lookup is deallocated instead, which would be a bug.
Using a separate variable for the copy means the code is just as correct but much less likely to result confusing.
No functional changes. --- src/cpu/cpu_ppc64.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index dd02a3f..c769221 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -162,6 +162,7 @@ ppc64ModelFromCPU(const virCPUDef *cpu, const struct ppc64_map *map) { struct ppc64_model *model; + struct ppc64_model *copy;
if (!(model = ppc64ModelFind(map, cpu->model))) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -169,13 +170,13 @@ ppc64ModelFromCPU(const virCPUDef *cpu, goto error; }
- if (!(model = ppc64ModelCopy(model))) + if (!(copy = ppc64ModelCopy(model))) goto error;
- return model; + return copy;
error: - ppc64ModelFree(model); + ppc64ModelFree(copy); return NULL;
You forgot to initialize copy to NULL, but why not just return ppc64ModelCopy(model); and removing "copy" end the error label completely since it will never do anything anyway? Jirka
On Wed, 2015-08-05 at 14:28 +0200, Jiri Denemark wrote:
You forgot to initialize copy to NULL, but why not just
return ppc64ModelCopy(model);
and removing "copy" end the error label completely since it will never do anything anyway?
You're right! Done :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
Having the functions grouped together this way will avoid further shuffling around down the line. No functional changes. --- src/cpu/cpu_ppc64.c | 135 +++++++++++++++++++++++++--------------------------- 1 file changed, 66 insertions(+), 69 deletions(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index c769221..20c68ca 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,26 @@ ppc64ModelFree(struct ppc64_model *model) } static struct ppc64_model * +ppc64ModelCopy(const struct ppc64_model *model) +{ + struct ppc64_model *copy; + + if (!model) + return NULL; + + 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,53 +157,6 @@ ppc64ModelFindPVR(const struct ppc64_map *map, } static struct ppc64_model * -ppc64ModelCopy(const struct ppc64_model *model) -{ - struct ppc64_model *copy; - - if (!model) - return NULL; - - 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_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 -ppc64VendorFree(struct ppc64_vendor *vendor) -{ - if (!vendor) - return; - - VIR_FREE(vendor->name); - VIR_FREE(vendor); -} - -static struct ppc64_model * ppc64ModelFromCPU(const virCPUDef *cpu, const struct ppc64_map *map) { @@ -180,6 +179,26 @@ ppc64ModelFromCPU(const virCPUDef *cpu, return NULL; } +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 int ppc64VendorLoad(xmlXPathContextPtr ctxt, @@ -304,27 +323,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) { @@ -511,7 +509,6 @@ ppc64DriverDecode(virCPUDefPtr cpu, return ret; } - static void ppc64DriverFree(virCPUDataPtr data) { -- 2.4.3
On Tue, Aug 04, 2015 at 11:37:56 +0200, Andrea Bolognani wrote:
Having the functions grouped together this way will avoid further shuffling around down the line.
No functional changes. --- src/cpu/cpu_ppc64.c | 135 +++++++++++++++++++++++++--------------------------- 1 file changed, 66 insertions(+), 69 deletions(-)
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
On Tue, Aug 04, 2015 at 11:37:57 +0200, Andrea Bolognani wrote:
The information is not used anywhere in libvirt.
No functional changes. --- src/cpu/cpu_map.xml | 5 ----- 1 file changed, 5 deletions(-)
ACK Jirka
On Tue, Aug 04, 2015 at 11:37:57 +0200, Andrea Bolognani wrote:
The information is not used anywhere in libvirt.
No functional changes. --- src/cpu/cpu_map.xml | 5 ----- 1 file changed, 5 deletions(-)
ACK Jirka
--- 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 Tue, Aug 04, 2015 at 11:37:58 +0200, Andrea Bolognani wrote:
--- 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
Fix a test case that was wrongly expected to fail as well. --- tests/cputest.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/tests/cputest.c b/tests/cputest.c index 06b3f12..5b7de0f 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -273,13 +273,8 @@ cpuTestGuestData(const void *arg) guest->match = VIR_CPU_MATCH_EXACT; guest->fallback = cpu->fallback; if (cpuDecode(guest, guestData, data->models, - data->nmodels, data->preferred) < 0) { - if (data->result < 0) { - virResetLastError(); - ret = 0; - } + data->nmodels, data->preferred) < 0) goto cleanup; - } virBufferAsprintf(&buf, "%s+%s", data->host, data->name); if (data->nmodels) @@ -302,6 +297,19 @@ cpuTestGuestData(const void *arg) virCPUDefFree(host); virCPUDefFree(cpu); virCPUDefFree(guest); + + if (data->result < 0) { + virResetLastError(); + if (ret < 0) { + ret = 0; + } else { + VIR_TEST_VERBOSE("\n%-70s... ", + "cpuGuestData/cpuDecode was expected " + "to fail but it succeeded"); + ret = -1; + } + } + return ret; } @@ -646,7 +654,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 Tue, Aug 04, 2015 at 11:37:59 +0200, Andrea Bolognani wrote:
Fix a test case that was wrongly expected to fail as well. --- tests/cputest.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/tests/cputest.c b/tests/cputest.c index 06b3f12..5b7de0f 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -273,13 +273,8 @@ cpuTestGuestData(const void *arg) guest->match = VIR_CPU_MATCH_EXACT; guest->fallback = cpu->fallback; if (cpuDecode(guest, guestData, data->models, - data->nmodels, data->preferred) < 0) { - if (data->result < 0) { - virResetLastError(); - ret = 0; - } + data->nmodels, data->preferred) < 0) goto cleanup; - }
virBufferAsprintf(&buf, "%s+%s", data->host, data->name); if (data->nmodels) @@ -302,6 +297,19 @@ cpuTestGuestData(const void *arg) virCPUDefFree(host); virCPUDefFree(cpu); virCPUDefFree(guest); + + if (data->result < 0) { + virResetLastError(); + if (ret < 0) { + ret = 0; + } else { + VIR_TEST_VERBOSE("\n%-70s... ", + "cpuGuestData/cpuDecode was expected " + "to fail but it succeeded"); + ret = -1; + } + } +
This would apply to any failure, but when, e.g., loading the XML files or memory allocation fails, we want to fail the test even if it was expected to fail. This conditional statement should only be applied when cpuDecode or cpuGuestData fails. Jirka
On Thu, 2015-08-06 at 11:23 +0200, Jiri Denemark wrote:
@@ -302,6 +297,19 @@ cpuTestGuestData(const void *arg) virCPUDefFree(host); virCPUDefFree(cpu); virCPUDefFree(guest); + + if (data->result < 0) { + virResetLastError(); + if (ret < 0) { + ret = 0; + } else { + VIR_TEST_VERBOSE("\n%-70s... ", + "cpuGuestData/cpuDecode was expected " + "to fail but it succeeded"); + ret = -1; + } + } +
This would apply to any failure, but when, e.g., loading the XML files or memory allocation fails, we want to fail the test even if it was expected to fail. This conditional statement should only be applied when cpuDecode or cpuGuestData fails.
Right. I have changed the code so that it can tell these two kinds of failure apart and act accordingly. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
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 20c68ca..633515f 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -418,26 +418,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 Tue, Aug 04, 2015 at 11:38:00 +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(-)
diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 20c68ca..633515f 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -418,26 +418,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;
I would probably simplify this as if (guestData && !(*guestData = ppc64MakeCPUData(arch, &guest_model->data))) goto cleanup; but not a big deal. 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. --- 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 633515f..09ff386 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -361,7 +361,6 @@ ppc64Compute(virCPUDefPtr host, const virCPUDef *cpu, virCPUDataPtr *guestData, char **message) - { struct ppc64_map *map = NULL; struct ppc64_model *host_model = NULL; @@ -418,9 +417,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 5b7de0f..ec867a7 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -493,7 +493,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) @@ -654,7 +654,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 Tue, Aug 04, 2015 at 11:38:01 +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. --- 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
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 09ff386..4428a55 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -449,16 +449,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 Tue, Aug 04, 2015 at 11:38:02 +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. --- src/cpu/cpu_ppc64.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
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 | 81 ++++++++++++++++++++++++++++++++++++++---------- src/cpu/cpu_ppc64_data.h | 3 +- 3 files changed, 67 insertions(+), 19 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 4428a55..d186d4c 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,31 @@ struct ppc64_map { }; static void +ppc64DataFree(virCPUppc64Data *data) +{ + if (!data) + return; + + VIR_FREE(data); +} + +static virCPUppc64Data * +ppc64DataCopy(virCPUppc64Data *data) +{ + virCPUppc64Data *copy; + + if (!data) + return NULL; + + if (VIR_ALLOC(copy) < 0) + return NULL; + + copy->pvr = data->pvr; + + return copy; +} + +static void ppc64VendorFree(struct ppc64_vendor *vendor) { if (!vendor) @@ -90,6 +115,7 @@ ppc64ModelFree(struct ppc64_model *model) if (!model) return; + ppc64DataFree(model->data); VIR_FREE(model->name); VIR_FREE(model); } @@ -102,16 +128,22 @@ ppc64ModelCopy(const struct ppc64_model *model) if (!model) return NULL; - 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 * @@ -139,7 +171,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; @@ -248,6 +280,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, @@ -285,7 +322,7 @@ ppc64ModelLoad(xmlXPathContextPtr ctxt, model->name); goto ignore; } - model->data.pvr = pvr; + model->data->pvr = pvr; if (!map->models) { map->models = model; @@ -329,7 +366,7 @@ ppc64LoadMap(void) struct ppc64_map *map; if (VIR_ALLOC(map) < 0) - return NULL; + goto error; if (cpuMapLoad("ppc64", ppc64MapLoadCallback, map) < 0) goto error; @@ -343,7 +380,7 @@ ppc64LoadMap(void) static virCPUDataPtr ppc64MakeCPUData(virArch arch, - struct cpuPPC64Data *data) + virCPUppc64Data *data) { virCPUDataPtr cpuData; @@ -351,7 +388,9 @@ ppc64MakeCPUData(virArch arch, return NULL; cpuData->arch = arch; - cpuData->data.ppc64 = *data; + + if (!(cpuData->data.ppc64 = ppc64DataCopy(data))) + VIR_FREE(cpuData); return cpuData; } @@ -432,7 +471,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; @@ -484,10 +523,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; } @@ -517,6 +556,7 @@ ppc64DriverFree(virCPUDataPtr data) if (!data) return; + ppc64DataFree(data->data.ppc64); VIR_FREE(data); } @@ -526,16 +566,23 @@ ppc64DriverNodeData(virArch arch) virCPUDataPtr cpuData; if (VIR_ALLOC(cpuData) < 0) - return NULL; + goto error; + + if (VIR_ALLOC(cpuData->data.ppc64) < 0) + goto error; cpuData->arch = arch; #if defined(__powerpc__) || defined(__powerpc64__) asm("mfpvr %0" - : "=r" (cpuData->data.ppc64.pvr)); + : "=r" (cpuData->data.ppc64->pvr)); #endif return cpuData; + + error: + VIR_FREE(cpuData); + 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 Tue, Aug 04, 2015 at 11:38:03 +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. --- src/cpu/cpu.h | 2 +- src/cpu/cpu_ppc64.c | 81 ++++++++++++++++++++++++++++++++++++++---------- src/cpu/cpu_ppc64_data.h | 3 +- 3 files changed, 67 insertions(+), 19 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 4428a55..d186d4c 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,31 @@ struct ppc64_map { };
static void +ppc64DataFree(virCPUppc64Data *data) +{ + if (!data) + return;
This is redundant unless you want to add more fields in the structure which will need to be freed here.
+ + VIR_FREE(data); +} ...
ACK Jirka
On Thu, 2015-08-06 at 13:20 +0200, Jiri Denemark wrote:
static void +ppc64DataFree(virCPUppc64Data *data) +{ + if (!data) + return;
This is redundant unless you want to add more fields in the structure which will need to be freed here.
+ + VIR_FREE(data); +}
I'm adding the fields in the next commit, but I can just move the check to that commit as well :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
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 | 73 ++++++++++++++++++++++++++++++++++++++++-------- src/cpu/cpu_ppc64_data.h | 8 +++++- 2 files changed, 69 insertions(+), 12 deletions(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index d186d4c..9351729 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -63,6 +63,7 @@ ppc64DataFree(virCPUppc64Data *data) if (!data) return; + VIR_FREE(data->pvr); VIR_FREE(data); } @@ -70,16 +71,27 @@ static virCPUppc64Data * ppc64DataCopy(virCPUppc64Data *data) { virCPUppc64Data *copy; + size_t i; if (!data) return NULL; 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 @@ -168,11 +180,13 @@ 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; } @@ -274,8 +288,12 @@ ppc64ModelLoad(xmlXPathContextPtr ctxt, struct ppc64_map *map) { struct ppc64_model *model; + xmlNodePtr *nodes = NULL; char *vendor = NULL; + char *prop = NULL; unsigned long pvr; + size_t i; + int n; if (VIR_ALLOC(model) < 0) return -1; @@ -315,14 +333,38 @@ 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++) { + + if (!(prop = virXMLPropString(nodes[i], "value"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing PVR value in CPU model %s"), + model->name); + goto ignore; + } + + if (virStrToLong_ul(prop, NULL, 16, &pvr) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid PVR value in CPU model %s"), + model->name); + goto ignore; + } + + model->data->pvr[i].value = pvr; + + VIR_FREE(prop); + } if (!map->models) { map->models = model; @@ -332,7 +374,9 @@ ppc64ModelLoad(xmlXPathContextPtr ctxt, } cleanup: + VIR_FREE(prop); VIR_FREE(vendor); + VIR_FREE(nodes); return 0; ignore: @@ -523,10 +567,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; } @@ -571,16 +615,23 @@ ppc64DriverNodeData(virArch arch) if (VIR_ALLOC(cpuData->data.ppc64) < 0) goto error; + if (VIR_ALLOC_N(cpuData->data.ppc64->pvr, 1) < 0) + goto error; + + cpuData->data.ppc64->len = 1; + cpuData->arch = arch; #if defined(__powerpc__) || defined(__powerpc64__) asm("mfpvr %0" - : "=r" (cpuData->data.ppc64->pvr)); + : "=r" (cpuData->data.ppc64->pvr[0].value)); #endif return cpuData; error: + if (cpuData) + ppc64DataFree(cpuData->data.ppc64); VIR_FREE(cpuData); return NULL; } 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 Tue, Aug 04, 2015 at 11:38:04 +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 | 73 ++++++++++++++++++++++++++++++++++++++++-------- src/cpu/cpu_ppc64_data.h | 8 +++++- 2 files changed, 69 insertions(+), 12 deletions(-)
diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index d186d4c..9351729 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -63,6 +63,7 @@ ppc64DataFree(virCPUppc64Data *data) if (!data) return;
+ VIR_FREE(data->pvr); VIR_FREE(data); }
OK, ignore my comment on the previous patch :-)
@@ -70,16 +71,27 @@ static virCPUppc64Data * ppc64DataCopy(virCPUppc64Data *data) { virCPUppc64Data *copy; + size_t i;
if (!data) return NULL;
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 @@ -168,11 +180,13 @@ 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;
I think the for loop would deserve {} around its body.
model = model->next; } @@ -274,8 +288,12 @@ ppc64ModelLoad(xmlXPathContextPtr ctxt, struct ppc64_map *map) { struct ppc64_model *model; + xmlNodePtr *nodes = NULL; char *vendor = NULL; + char *prop = NULL; unsigned long pvr; + size_t i; + int n;
if (VIR_ALLOC(model) < 0) return -1; @@ -315,14 +333,38 @@ 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++) { +
Drop the empty line here.
+ if (!(prop = virXMLPropString(nodes[i], "value"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing PVR value in CPU model %s"), + model->name); + goto ignore; + } + + if (virStrToLong_ul(prop, NULL, 16, &pvr) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid PVR value in CPU model %s"), + model->name); + goto ignore; + }
Any particular reason to replace virXPathULongHex with the above code?
+ + model->data->pvr[i].value = pvr; + + VIR_FREE(prop); + }
if (!map->models) { map->models = model; ...
Looks OK otherwise. Jirka
On Thu, 2015-08-06 at 13:53 +0200, Jiri Denemark wrote:
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;
I think the for loop would deserve {} around its body.
Added.
+ + for (i = 0; i < n; i++) { +
Drop the empty line here.
Done.
+ if (!(prop = virXMLPropString(nodes[i], "value"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing PVR value in CPU model %s"), + model->name); + goto ignore; + } + + if (virStrToLong_ul(prop, NULL, 16, &pvr) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid PVR value in CPU model %s"), + model->name); + goto ignore; + }
Any particular reason to replace virXPathULongHex with the above code?
Because I no longer have a single <pvr> element, but a number of them, that I retrieved a few lines above using virXPathNodeSet(). I'm definitely no XPath expert, so if you're thinking of a way of making this simpler feel free to share :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On Thu, Aug 06, 2015 at 16:30:37 +0200, Andrea Bolognani wrote:
On Thu, 2015-08-06 at 13:53 +0200, Jiri Denemark wrote:
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;
I think the for loop would deserve {} around its body.
Added.
+ + for (i = 0; i < n; i++) { +
Drop the empty line here.
Done.
+ if (!(prop = virXMLPropString(nodes[i], "value"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing PVR value in CPU model %s"), + model->name); + goto ignore; + } + + if (virStrToLong_ul(prop, NULL, 16, &pvr) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid PVR value in CPU model %s"), + model->name); + goto ignore; + }
Any particular reason to replace virXPathULongHex with the above code?
Because I no longer have a single <pvr> element, but a number of them, that I retrieved a few lines above using virXPathNodeSet().
I'm definitely no XPath expert, so if you're thinking of a way of making this simpler feel free to share :)
Ah, right, you'd have to do something similar to this: orig_node = ctxt->node; for (...) { virXPathULongHex("@value", ...); } ctxt->node = orig_node; Jirka
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. --- src/cpu/cpu_map.xml | 39 ++-------------------- tests/cputest.c | 4 +-- .../ppc64-baseline-incompatible-vendors.xml | 4 +-- .../ppc64-baseline-no-vendor-result.xml | 2 +- tests/cputestdata/ppc64-baseline-no-vendor.xml | 2 +- tests/cputestdata/ppc64-exact.xml | 2 +- tests/cputestdata/ppc64-guest-nofallback.xml | 2 +- tests/cputestdata/ppc64-guest.xml | 2 +- .../ppc64-host+guest,ppc_models-result.xml | 2 +- tests/cputestdata/ppc64-host.xml | 2 +- tests/cputestdata/ppc64-strict.xml | 4 +-- 11 files changed, 16 insertions(+), 49 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> diff --git a/tests/cputest.c b/tests/cputest.c index ec867a7..82999f8 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -493,7 +493,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", "POWER7_v2.3", "POWER8_v1.0"}; +static const char *ppc_models[] = { "POWER6", "POWER7", "POWER8" }; static int mymain(void) @@ -654,7 +654,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, "POWER6", -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..7fac4b7 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='allow'>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 index c84f16a..f416a59 100644 --- a/tests/cputestdata/ppc64-exact.xml +++ b/tests/cputestdata/ppc64-exact.xml @@ -1,3 +1,3 @@ <cpu match='exact'> - <model>POWER8_v1.0</model> + <model>POWER8</model> </cpu> diff --git a/tests/cputestdata/ppc64-guest-nofallback.xml b/tests/cputestdata/ppc64-guest-nofallback.xml index 42026b4..603a3ab 100644 --- a/tests/cputestdata/ppc64-guest-nofallback.xml +++ b/tests/cputestdata/ppc64-guest-nofallback.xml @@ -1,4 +1,4 @@ <cpu match='exact'> - <model fallback='forbid'>POWER7_v2.1</model> + <model fallback='forbid'>POWER6</model> <topology sockets='2' cores='4' threads='1'/> </cpu> diff --git a/tests/cputestdata/ppc64-guest.xml b/tests/cputestdata/ppc64-guest.xml index 9e91501..210f96d 100644 --- a/tests/cputestdata/ppc64-guest.xml +++ b/tests/cputestdata/ppc64-guest.xml @@ -1,4 +1,4 @@ <cpu match='exact'> - <model>POWER7_v2.3</model> + <model>POWER7</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 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 index e91c6e7..217dfc7 100644 --- a/tests/cputestdata/ppc64-strict.xml +++ b/tests/cputestdata/ppc64-strict.xml @@ -1,3 +1,3 @@ -<cpu match='exact'> - <model>POWER7_v2.3</model> +<cpu match='strict'> + <model>POWER7</model> </cpu> -- 2.4.3
On Tue, Aug 04, 2015 at 11:38:05 +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.
So what if anyone already had a domain which uses one of the CPU models you are removing here? Don't we need to keep them for backward compatibility or at least provide some translation from the removed CPU model names to the new ones? Jirka
The code currently assumes that the mask will be 0xffff0000, which is not always the case - it is for the models listed so far, though. --- src/cpu/cpu_map.xml | 14 +++++++------- 1 file changed, 7 insertions(+), 7 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> -- 2.4.3
On Tue, Aug 04, 2015 at 11:38:06 +0200, Andrea Bolognani wrote:
The code currently assumes that the mask will be 0xffff0000, which is not always the case - it is for the models listed so far, though. --- src/cpu/cpu_map.xml | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
I think this patch should be squashed into the next one which actually makes use of the new attribute. Jirka
On Thu, 2015-08-06 at 13:59 +0200, Jiri Denemark wrote:
On Tue, Aug 04, 2015 at 11:38:06 +0200, Andrea Bolognani wrote:
The code currently assumes that the mask will be 0xffff0000, which is not always the case - it is for the models listed so far, though. --- src/cpu/cpu_map.xml | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
I think this patch should be squashed into the next one which actually makes use of the new attribute.
Makes sense. Done. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
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. Rewrite ppc64DriverNodeData() to use this feature. --- src/cpu/cpu_ppc64.c | 74 ++++++++++++++++++++++++++++-------------------- src/cpu/cpu_ppc64_data.h | 1 + 2 files changed, 44 insertions(+), 31 deletions(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 9351729..add5ede 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -84,8 +84,10 @@ ppc64DataCopy(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; @@ -185,20 +187,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; } @@ -364,6 +358,24 @@ ppc64ModelLoad(xmlXPathContextPtr ctxt, model->data->pvr[i].value = pvr; VIR_FREE(prop); + + if (!(prop = virXMLPropString(nodes[i], "mask"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing PVR mask in CPU model %s"), + model->name); + goto ignore; + } + + if (virStrToLong_ul(prop, NULL, 16, &pvr) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid PVR mask in CPU model %s"), + model->name); + goto ignore; + } + + model->data->pvr[i].mask = pvr; + + VIR_FREE(prop); } if (!map->models) { @@ -607,33 +619,33 @@ ppc64DriverFree(virCPUDataPtr data) static virCPUDataPtr ppc64DriverNodeData(virArch arch) { - virCPUDataPtr cpuData; - - if (VIR_ALLOC(cpuData) < 0) - goto error; - - if (VIR_ALLOC(cpuData->data.ppc64) < 0) - goto error; - - if (VIR_ALLOC_N(cpuData->data.ppc64->pvr, 1) < 0) - goto error; - - cpuData->data.ppc64->len = 1; - - cpuData->arch = arch; + virCPUDataPtr nodeData = NULL; + struct ppc64_map *map = NULL; + struct ppc64_model *model = NULL; + uint32_t pvr = 0; #if defined(__powerpc__) || defined(__powerpc64__) asm("mfpvr %0" - : "=r" (cpuData->data.ppc64->pvr[0].value)); + : "=r" (pvr)); #endif - return cpuData; + if (!(map = ppc64LoadMap())) + goto cleanup; - error: - if (cpuData) - ppc64DataFree(cpuData->data.ppc64); - VIR_FREE(cpuData); - return NULL; + if (!(model = ppc64ModelFindPVR(map, pvr))) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Cannot find CPU model with PVR 0x%08x"), + pvr); + goto cleanup; + } + + if (!(nodeData = ppc64MakeCPUData(arch, model->data))) + goto cleanup; + + cleanup: + ppc64MapFree(map); + + return nodeData; } static virCPUCompareResult 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 Tue, Aug 04, 2015 at 11:38:07 +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.
Rewrite ppc64DriverNodeData() to use this feature. --- src/cpu/cpu_ppc64.c | 74 ++++++++++++++++++++++++++++-------------------- src/cpu/cpu_ppc64_data.h | 1 + 2 files changed, 44 insertions(+), 31 deletions(-)
diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 9351729..add5ede 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -84,8 +84,10 @@ ppc64DataCopy(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;
@@ -185,20 +187,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; }
@@ -364,6 +358,24 @@ ppc64ModelLoad(xmlXPathContextPtr ctxt, model->data->pvr[i].value = pvr;
VIR_FREE(prop); + + if (!(prop = virXMLPropString(nodes[i], "mask"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing PVR mask in CPU model %s"), + model->name); + goto ignore; + } + + if (virStrToLong_ul(prop, NULL, 16, &pvr) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid PVR mask in CPU model %s"), + model->name); + goto ignore; + } + + model->data->pvr[i].mask = pvr;
Wouldn't virXPathULongHex be good enough?
+ + VIR_FREE(prop); }
if (!map->models) { @@ -607,33 +619,33 @@ ppc64DriverFree(virCPUDataPtr data) static virCPUDataPtr ppc64DriverNodeData(virArch arch) { - virCPUDataPtr cpuData; - - if (VIR_ALLOC(cpuData) < 0) - goto error; - - if (VIR_ALLOC(cpuData->data.ppc64) < 0) - goto error; - - if (VIR_ALLOC_N(cpuData->data.ppc64->pvr, 1) < 0) - goto error; - - cpuData->data.ppc64->len = 1; - - cpuData->arch = arch; + virCPUDataPtr nodeData = NULL; + struct ppc64_map *map = NULL; + struct ppc64_model *model = NULL; + uint32_t pvr = 0;
#if defined(__powerpc__) || defined(__powerpc64__) asm("mfpvr %0" - : "=r" (cpuData->data.ppc64->pvr[0].value)); + : "=r" (pvr)); #endif
- return cpuData; + if (!(map = ppc64LoadMap())) + goto cleanup;
- error: - if (cpuData) - ppc64DataFree(cpuData->data.ppc64); - VIR_FREE(cpuData); - return NULL; + if (!(model = ppc64ModelFindPVR(map, pvr))) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Cannot find CPU model with PVR 0x%08x"), + pvr); + goto cleanup; + } + + if (!(nodeData = ppc64MakeCPUData(arch, model->data))) + goto cleanup; + + cleanup: + ppc64MapFree(map); + + return nodeData; }
The ppc64DriverNodeData API is supposed to return raw data from the host, which is then passed to ppc64DriverDecode to be translated into a CPU model name. In other words, you copied most of the internals of ppc64DriverDecode here while you in fact only need to add a single line cpuData->data.ppc64->pvr[0].mask = 0xffffffff; Jirka
On Thu, 2015-08-06 at 16:42 +0200, Jiri Denemark wrote:
@@ -364,6 +358,24 @@ ppc64ModelLoad(xmlXPathContextPtr ctxt, model->data->pvr[i].value = pvr;
VIR_FREE(prop); + + if (!(prop = virXMLPropString(nodes[i], "mask"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing PVR mask in CPU model %s"), + model->name); + goto ignore; + } + + if (virStrToLong_ul(prop, NULL, 16, &pvr) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid PVR mask in CPU model %s"), + model->name); + goto ignore; + } + + model->data->pvr[i].mask = pvr;
Wouldn't virXPathULongHex be good enough?
See the replay to the same remark in patch 13/18.
+ + VIR_FREE(prop); }
if (!map->models) { @@ -607,33 +619,33 @@ ppc64DriverFree(virCPUDataPtr data) static virCPUDataPtr ppc64DriverNodeData(virArch arch) { - virCPUDataPtr cpuData; - - if (VIR_ALLOC(cpuData) < 0) - goto error; - - if (VIR_ALLOC(cpuData->data.ppc64) < 0) - goto error; - - if (VIR_ALLOC_N(cpuData->data.ppc64->pvr, 1) < 0) - goto error; - - cpuData->data.ppc64->len = 1; - - cpuData->arch = arch; + virCPUDataPtr nodeData = NULL; + struct ppc64_map *map = NULL; + struct ppc64_model *model = NULL; + uint32_t pvr = 0;
#if defined(__powerpc__) || defined(__powerpc64__) asm("mfpvr %0" - : "=r" (cpuData->data.ppc64->pvr[0].value)); + : "=r" (pvr)); #endif
- return cpuData; + if (!(map = ppc64LoadMap())) + goto cleanup;
- error: - if (cpuData) - ppc64DataFree(cpuData->data.ppc64); - VIR_FREE(cpuData); - return NULL; + if (!(model = ppc64ModelFindPVR(map, pvr))) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Cannot find CPU model with PVR 0x%08x"), + pvr); + goto cleanup; + } + + if (!(nodeData = ppc64MakeCPUData(arch, model->data))) + goto cleanup; + + cleanup: + ppc64MapFree(map); + + return nodeData; }
The ppc64DriverNodeData API is supposed to return raw data from the host, which is then passed to ppc64DriverDecode to be translated into a CPU model name. In other words, you copied most of the internals of ppc64DriverDecode here while you in fact only need to add a single line
cpuData->data.ppc64->pvr[0].mask = 0xffffffff;
Done. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
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 Tue, Aug 04, 2015 at 11:38:08 +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>
The CPU is called "Power8NVL" rather than "POWER8 NVL" in kernel sources so you may want to change it in the subject. ACK in any case. Jirka
New test cases cover the cpuCompare() and cpuBaseline() implementation. --- tests/cputest.c | 10 ++++++++++ tests/cputestdata/ppc64-baseline-incompatible-models.xml | 14 ++++++++++++++ tests/cputestdata/ppc64-baseline-same-model-result.xml | 3 +++ tests/cputestdata/ppc64-baseline-same-model.xml | 14 ++++++++++++++ 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 ++++++ 8 files changed, 64 insertions(+) create mode 100644 tests/cputestdata/ppc64-baseline-incompatible-models.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-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 82999f8..5f17145 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -563,6 +563,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); @@ -619,6 +626,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); + /* CPU features */ DO_TEST_HASFEATURE("x86", "host", "vmx", YES); DO_TEST_HASFEATURE("x86", "host", "lm", YES); 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-same-model-result.xml b/tests/cputestdata/ppc64-baseline-same-model-result.xml new file mode 100644 index 0000000..dc0c862 --- /dev/null +++ b/tests/cputestdata/ppc64-baseline-same-model-result.xml @@ -0,0 +1,3 @@ +<cpu mode='custom' match='exact'> + <model fallback='allow'>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-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 Tue, Aug 04, 2015 at 11:38:09 +0200, Andrea Bolognani wrote:
New test cases cover the cpuCompare() and cpuBaseline() implementation. --- tests/cputest.c | 10 ++++++++++ tests/cputestdata/ppc64-baseline-incompatible-models.xml | 14 ++++++++++++++ tests/cputestdata/ppc64-baseline-same-model-result.xml | 3 +++ tests/cputestdata/ppc64-baseline-same-model.xml | 14 ++++++++++++++ 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 ++++++ 8 files changed, 64 insertions(+) create mode 100644 tests/cputestdata/ppc64-baseline-incompatible-models.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-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 82999f8..5f17145 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -563,6 +563,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); @@ -619,6 +626,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); + /* CPU features */ DO_TEST_HASFEATURE("x86", "host", "vmx", YES); DO_TEST_HASFEATURE("x86", "host", "lm", YES); 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-same-model-result.xml b/tests/cputestdata/ppc64-baseline-same-model-result.xml new file mode 100644 index 0000000..dc0c862 --- /dev/null +++ b/tests/cputestdata/ppc64-baseline-same-model-result.xml @@ -0,0 +1,3 @@ +<cpu mode='custom' match='exact'> + <model fallback='allow'>POWER8</model> +</cpu>
According to our discussion sometime last week I think PPC64 driver should never create a CPU definition with fallback='allow' since it doesn't make any sense for PPC. In other words, this series is missing one patch that would make sure all CPU defs created by PPC driver honor this. Otherwise looks good, although you will need to add more tests making sure we stay compatible with the old versioned CPU models. Jirka
On Fri, 2015-08-07 at 10:14 +0200, Jiri Denemark wrote:
According to our discussion sometime last week I think PPC64 driver should never create a CPU definition with fallback='allow' since it doesn't make any sense for PPC. In other words, this series is missing one patch that would make sure all CPU defs created by PPC driver honor this.
Otherwise looks good, although you will need to add more tests making sure we stay compatible with the old versioned CPU models.
I've just posted a v2 that covers these and all the other points you've raised during the review / we've discussed. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
participants (2)
-
Andrea Bolognani -
Jiri Denemark