5:37 p.m.
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
5:37 p.m.
New subject: [libvirt] [PATCH 01/18] cpu: Mark driver functions in ppc64 driver
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
8:09 p.m.
New subject: [libvirt] [PATCH 01/18] cpu: Mark driver functions in ppc64 driver
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
5:37 p.m.
New subject: [libvirt] [PATCH 02/18] cpu: Simplify NULL handling in ppc64 driver
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
8:12 p.m.
New subject: [libvirt] [PATCH 02/18] cpu: Simplify NULL handling in ppc64 driver
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
5:37 p.m.
New subject: [libvirt] [PATCH 03/18] cpu: Add NULL check in ppc64ModelCopy()
---
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
8:19 p.m.
New subject: [libvirt] [PATCH 03/18] cpu: Add NULL check in ppc64ModelCopy()
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
12:58 a.m.
New subject: [libvirt] [PATCH 03/18] cpu: Add NULL check in ppc64ModelCopy()
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
5:37 p.m.
New subject: [libvirt] [PATCH 04/18] cpu: Use a different name for the copy in ppc64ModelFromCPU()
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
8:28 p.m.
New subject: [libvirt] [PATCH 04/18] cpu: Use a different name for the copy in ppc64ModelFromCPU()
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
11:48 p.m.
New subject: [libvirt] [PATCH 04/18] cpu: Use a different name for the copy in ppc64ModelFromCPU()
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
5:37 p.m.
New subject: [libvirt] [PATCH 05/18] cpu: Reorder functions in the ppc64 driver
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
3:10 p.m.
New subject: [libvirt] [PATCH 05/18] cpu: Reorder functions in the ppc64 driver
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
5:37 p.m.
New subject: [libvirt] [PATCH 06/18] cpu: Remove ISA information from CPU map XML
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
3:37 p.m.
New subject: [libvirt] [PATCH 06/18] cpu: Remove ISA information from CPU map XML
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
4:04 p.m.
New subject: [libvirt] [PATCH 06/18] cpu: Remove ISA information from CPU map XML
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
5:37 p.m.
New subject: [libvirt] [PATCH 07/18] tests: Remove unused file
---
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
4:28 p.m.
New subject: [libvirt] [PATCH 07/18] tests: Remove unused file
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
5:37 p.m.
New subject: [libvirt] [PATCH 08/18] tests: Known failing tests should never succeed
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
5:23 p.m.
New subject: [libvirt] [PATCH 08/18] tests: Known failing tests should never succeed
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
10:20 p.m.
New subject: [libvirt] [PATCH 08/18] tests: Known failing tests should never succeed
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
5:38 p.m.
New subject: [libvirt] [PATCH 09/18] cpu: Don't skip CPU model name check in ppc64 driver
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
3:48 p.m.
New subject: [libvirt] [PATCH 09/18] cpu: Don't skip CPU model name check in ppc64 driver
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
5:38 p.m.
New subject: [libvirt] [PATCH 10/18] cpu: CPU model names have to match on ppc64
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
5:25 p.m.
New subject: [libvirt] [PATCH 10/18] cpu: CPU model names have to match on ppc64
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
5:38 p.m.
New subject: [libvirt] [PATCH 11/18] cpu: Use ppc64Compute() to implement ppc64DriverCompare()
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
7:03 p.m.
New subject: [libvirt] [PATCH 11/18] cpu: Use ppc64Compute() to implement ppc64DriverCompare()
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
5:38 p.m.
New subject: [libvirt] [PATCH 12/18] cpu: Align ppc64 CPU data with x86
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
7:20 p.m.
New subject: [libvirt] [PATCH 12/18] cpu: Align ppc64 CPU data with x86
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
7:48 p.m.
New subject: [libvirt] [PATCH 12/18] cpu: Align ppc64 CPU data with x86
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
5:38 p.m.
New subject: [libvirt] [PATCH 13/18] cpu: Support multiple PVRs in the ppc64 driver
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
7:53 p.m.
New subject: [libvirt] [PATCH 13/18] cpu: Support multiple PVRs in the ppc64 driver
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
10:30 p.m.
New subject: [libvirt] [PATCH 13/18] cpu: Support multiple PVRs in the ppc64 driver
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
10:51 p.m.
New subject: [libvirt] [PATCH 13/18] cpu: Support multiple PVRs in the ppc64 driver
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
5:38 p.m.
New subject: [libvirt] [PATCH 14/18] cpu: Simplify ppc64 part of CPU map XML
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
7:57 p.m.
New subject: [libvirt] [PATCH 14/18] cpu: Simplify ppc64 part of CPU map XML
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
5:38 p.m.
New subject: [libvirt] [PATCH 15/18] cpu: Add PVR mask to CPU map XML for ppc64 models
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
7:59 p.m.
New subject: [libvirt] [PATCH 15/18] cpu: Add PVR mask to CPU map XML for ppc64 models
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
8:19 p.m.
New subject: [libvirt] [PATCH 15/18] cpu: Add PVR mask to CPU map XML for ppc64 models
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
5:38 p.m.
New subject: [libvirt] [PATCH 16/18] cpu: Parse and use PVR masks in the ppc64 driver
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
10:42 p.m.
New subject: [libvirt] [PATCH 16/18] cpu: Parse and use PVR masks in the ppc64 driver
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
10:53 p.m.
New subject: [libvirt] [PATCH 16/18] cpu: Parse and use PVR masks in the ppc64 driver
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
5:38 p.m.
New subject: [libvirt] [PATCH 17/18] cpu: Add POWER8 NVL information to CPU map XML
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
3:45 p.m.
New subject: [libvirt] [PATCH 17/18] cpu: Add POWER8 NVL information to CPU map XML
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
5:38 p.m.
New subject: [libvirt] [PATCH 18/18] tests: Add a bunch of ppc64 cases to the cpu test
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
4:14 p.m.
New subject: [libvirt] [PATCH 18/18] tests: Add a bunch of ppc64 cases to the cpu test
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
11:42 p.m.
New subject: [libvirt] [PATCH 18/18] tests: Add a bunch of ppc64 cases to the cpu test
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
3373
days inactive
3376
days old
46 comments
2 participants
participants (2)
-
Andrea Bolognani -
Jiri Denemark