[libvirt] [PATCH v1 0/3] libvirt: Implement CPU model driver for PowerPC
CPU model driver is to get host's CPU information and it also provides one mechanism to expose host's CPU information to guests during migration. When migrating one guest from one machine to another machine, it will compare the CPU information. If it is incomptible, it won't start the guest. On x86, it uses CPUID instruction to get information. When migrating the guest, if the CPU defined in guest XML file is incompatible with host CPU, cpu-baseline will find out best features to expose to guest. Refer to [http://berrange.com/posts/2010/02/15 /guest-cpu-model-configuration-in-libvirt-with-qemukvm/]. On PowerPC, it can get CPU version by mfpvr instruction. So, if PVR is different from definition in guest XML file by cpu-compare, it will fail to start guest. The CPU infromation can be got by cpu-baseline. But only CPU vendor and model can be shown, there are no features as x86. So on PowrePC, it assumes that migration only occurs between the machines with the same type CPUs on PowerPC. In this driver, there are definitions of models and PVRs for CPUs supported on PowerPC. The relationship between models and PVRs are as the following: For one specific CPU model, its PVR is unique. And also for one PVR code, the PVR code is also unique. So from the PVR code of CPU data, it's easy to get model and vendor information. It is tested on my Power machine which CPU PVR is Power7_v2.3. * cap.xml has the same definition as the host. [root@ltckvmopal2 kvm-test]# /bin/virsh cpu-compare cap.xml CPU described in cap.xml is identical to host CPU [root@ltckvmopal2 kvm-test]# /bin/virsh cpu-baseline cap.xml <cpu mode='custom' match='exact'> <model fallback='allow'>POWER7_v2.3</model> <vendor>IBM</vendor> </cpu> * cap.xml has differnt model as the host. [root@ltckvmopal2 kvm-test]# /bin/virsh cpu-compare cap.xml CPU described in cap.xml is incompatible with host CPU [root@ltckvmopal2 kvm-test]# /bin/virsh cpu-baseline cap.xml <cpu mode='custom' match='exact'> <model fallback='allow'>POWER7_v2.1</model> <vendor>IBM</vendor> </cpu> *v1 -> v2: * Fix coding style's problems in several places. * Seperate doc-fix from the coding Li Zhang (3): libvirt: Add one file cpu_ppc_data.h to define CPU data for PPC libvirt: Implement CPU model driver for PowerPC Doc-fix for PowerPC CPU model driver src/cpu/cpu.h | 3 + src/cpu/cpu_map.xml | 14 ++ src/cpu/cpu_powerpc.c | 589 ++++++++++++++++++++++++++++++++++++++++++++++-- src/cpu/cpu_ppc_data.h | 33 +++ 4 files changed, 621 insertions(+), 18 deletions(-) create mode 100644 src/cpu/cpu_ppc_data.h -- 1.7.7.6
CPU version can be got by PVR on PowerPC. So this PVR is defined in the CPU data in cpuData structure. Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- src/cpu/cpu.h | 3 +++ src/cpu/cpu_ppc_data.h | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 0 deletions(-) create mode 100644 src/cpu/cpu_ppc_data.h diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index 5ba742d..01c732c 100644 --- a/src/cpu/cpu.h +++ b/src/cpu/cpu.h @@ -28,11 +28,14 @@ # include "datatypes.h" # include "conf/cpu_conf.h" # include "cpu_x86_data.h" +# include "cpu_ppc_data.h" union cpuData { struct cpuX86Data x86; /* generic driver needs no data */ + /* PowerPC driver need data*/ + struct cpuPPCData ppc; }; diff --git a/src/cpu/cpu_ppc_data.h b/src/cpu/cpu_ppc_data.h new file mode 100644 index 0000000..37199f7 --- /dev/null +++ b/src/cpu/cpu_ppc_data.h @@ -0,0 +1,33 @@ +/* + * cpu_ppc_data.h: PowerPC specific CPU data + * + * Copyright (C) 2012 IBM Corporation. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Li Zhang <zhlcindy@linux.vnet.ibm.com> + */ + +#ifndef __VIR_CPU_PPC_DATA_H__ +#define __VIR_CPU_PPC_DATA_H__ + +#include <stdint.h> + +struct cpuPPCData { + uint32_t pvr; +}; + +#endif /* __VIR_CPU_PPC_DATA_H__ */ -- 1.7.7.6
On 09.10.2012 09:58, Li Zhang wrote:
CPU version can be got by PVR on PowerPC. So this PVR is defined in the CPU data in cpuData structure.
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- src/cpu/cpu.h | 3 +++ src/cpu/cpu_ppc_data.h | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 0 deletions(-) create mode 100644 src/cpu/cpu_ppc_data.h
diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index 5ba742d..01c732c 100644 --- a/src/cpu/cpu.h +++ b/src/cpu/cpu.h @@ -28,11 +28,14 @@ # include "datatypes.h" # include "conf/cpu_conf.h" # include "cpu_x86_data.h" +# include "cpu_ppc_data.h"
union cpuData { struct cpuX86Data x86; /* generic driver needs no data */ + /* PowerPC driver need data*/ + struct cpuPPCData ppc; };
diff --git a/src/cpu/cpu_ppc_data.h b/src/cpu/cpu_ppc_data.h new file mode 100644 index 0000000..37199f7 --- /dev/null +++ b/src/cpu/cpu_ppc_data.h @@ -0,0 +1,33 @@ +/* + * cpu_ppc_data.h: PowerPC specific CPU data + * + * Copyright (C) 2012 IBM Corporation. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Li Zhang <zhlcindy@linux.vnet.ibm.com> + */ + +#ifndef __VIR_CPU_PPC_DATA_H__ +#define __VIR_CPU_PPC_DATA_H__
s/#define/# define/
+ +#include <stdint.h>
s/#include/# include/
+ +struct cpuPPCData { + uint32_t pvr; +}; + +#endif /* __VIR_CPU_PPC_DATA_H__ */
ACK Michal
On Thu, Oct 11, 2012 at 11:12 PM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 09.10.2012 09:58, Li Zhang wrote:
CPU version can be got by PVR on PowerPC. So this PVR is defined in the CPU data in cpuData structure.
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- src/cpu/cpu.h | 3 +++ src/cpu/cpu_ppc_data.h | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 0 deletions(-) create mode 100644 src/cpu/cpu_ppc_data.h
diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index 5ba742d..01c732c 100644 --- a/src/cpu/cpu.h +++ b/src/cpu/cpu.h @@ -28,11 +28,14 @@ # include "datatypes.h" # include "conf/cpu_conf.h" # include "cpu_x86_data.h" +# include "cpu_ppc_data.h"
union cpuData { struct cpuX86Data x86; /* generic driver needs no data */ + /* PowerPC driver need data*/ + struct cpuPPCData ppc; };
diff --git a/src/cpu/cpu_ppc_data.h b/src/cpu/cpu_ppc_data.h new file mode 100644 index 0000000..37199f7 --- /dev/null +++ b/src/cpu/cpu_ppc_data.h @@ -0,0 +1,33 @@ +/* + * cpu_ppc_data.h: PowerPC specific CPU data + * + * Copyright (C) 2012 IBM Corporation. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Li Zhang <zhlcindy@linux.vnet.ibm.com> + */ + +#ifndef __VIR_CPU_PPC_DATA_H__ +#define __VIR_CPU_PPC_DATA_H__
s/#define/# define/
+ +#include <stdint.h>
s/#include/# include/
Thanks for your comments, Michal. This is the coding style in libvirt,right? I didn't use space before. :-)
+ +struct cpuPPCData { + uint32_t pvr; +}; + +#endif /* __VIR_CPU_PPC_DATA_H__ */
ACK
Michal
-- Best Regards -Li
On 12.10.2012 09:31, Li Zhang wrote:
On Thu, Oct 11, 2012 at 11:12 PM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 09.10.2012 09:58, Li Zhang wrote:
CPU version can be got by PVR on PowerPC. So this PVR is defined in the CPU data in cpuData structure.
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- src/cpu/cpu.h | 3 +++ src/cpu/cpu_ppc_data.h | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 0 deletions(-) create mode 100644 src/cpu/cpu_ppc_data.h
diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index 5ba742d..01c732c 100644 --- a/src/cpu/cpu.h +++ b/src/cpu/cpu.h @@ -28,11 +28,14 @@ # include "datatypes.h" # include "conf/cpu_conf.h" # include "cpu_x86_data.h" +# include "cpu_ppc_data.h"
union cpuData { struct cpuX86Data x86; /* generic driver needs no data */ + /* PowerPC driver need data*/ + struct cpuPPCData ppc; };
diff --git a/src/cpu/cpu_ppc_data.h b/src/cpu/cpu_ppc_data.h new file mode 100644 index 0000000..37199f7 --- /dev/null +++ b/src/cpu/cpu_ppc_data.h @@ -0,0 +1,33 @@ +/* + * cpu_ppc_data.h: PowerPC specific CPU data + * + * Copyright (C) 2012 IBM Corporation. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Li Zhang <zhlcindy@linux.vnet.ibm.com> + */ + +#ifndef __VIR_CPU_PPC_DATA_H__ +#define __VIR_CPU_PPC_DATA_H__
s/#define/# define/
+ +#include <stdint.h>
s/#include/# include/
Thanks for your comments, Michal.
This is the coding style in libvirt,right? I didn't use space before. :-)
Yeah. You can find the whole set of rules online: http://libvirt.org/hacking.html Michal
On Fri, Oct 12, 2012 at 4:02 PM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 12.10.2012 09:31, Li Zhang wrote:
On Thu, Oct 11, 2012 at 11:12 PM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 09.10.2012 09:58, Li Zhang wrote:
CPU version can be got by PVR on PowerPC. So this PVR is defined in the CPU data in cpuData structure.
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- src/cpu/cpu.h | 3 +++ src/cpu/cpu_ppc_data.h | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 0 deletions(-) create mode 100644 src/cpu/cpu_ppc_data.h
diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index 5ba742d..01c732c 100644 --- a/src/cpu/cpu.h +++ b/src/cpu/cpu.h @@ -28,11 +28,14 @@ # include "datatypes.h" # include "conf/cpu_conf.h" # include "cpu_x86_data.h" +# include "cpu_ppc_data.h"
union cpuData { struct cpuX86Data x86; /* generic driver needs no data */ + /* PowerPC driver need data*/ + struct cpuPPCData ppc; };
diff --git a/src/cpu/cpu_ppc_data.h b/src/cpu/cpu_ppc_data.h new file mode 100644 index 0000000..37199f7 --- /dev/null +++ b/src/cpu/cpu_ppc_data.h @@ -0,0 +1,33 @@ +/* + * cpu_ppc_data.h: PowerPC specific CPU data + * + * Copyright (C) 2012 IBM Corporation. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Li Zhang <zhlcindy@linux.vnet.ibm.com> + */ + +#ifndef __VIR_CPU_PPC_DATA_H__ +#define __VIR_CPU_PPC_DATA_H__
s/#define/# define/
+ +#include <stdint.h>
s/#include/# include/
Thanks for your comments, Michal.
This is the coding style in libvirt,right? I didn't use space before. :-)
Yeah. You can find the whole set of rules online:
Got it, thanks a lot. I will study it.
Michal
-- Best Regards -Li
Currently, the CPU model driver is not implemented for PowerPC. Host's CPU information is needed to exposed to guests' XML file some time. This patch is to implement the callback functions of CPU model driver. Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- src/cpu/cpu_map.xml | 14 ++ src/cpu/cpu_powerpc.c | 585 +++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 583 insertions(+), 16 deletions(-) diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index 9a89e2e..affcce3 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -495,4 +495,18 @@ <feature name='fma4'/> </model> </arch> + <arch name='ppc64'> + <!-- vendor definitions --> + <vendor name='IBM' string='PowerPC'/> + <!-- IBM-based CPU models --> + <model name='POWER7'> + <vendor name='IBM'/> + </model> + <model name='POWER7_v2.1'> + <vendor name='IBM'/> + </model> + <model name='POWER7_v2.3'> + <vendor name='IBM'/> + </model> + </arch> </cpus> diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c index 1b77caf..3a88e68 100644 --- a/src/cpu/cpu_powerpc.c +++ b/src/cpu/cpu_powerpc.c @@ -20,18 +20,526 @@ * Authors: * Anton Blanchard <anton@au.ibm.com> * Prerna Saxena <prerna@linux.vnet.ibm.com> + * Li Zhang <zhlcindy@linux.vnet.ibm.com> */ #include <config.h> +#include <stdint.h> +#include "logging.h" #include "memory.h" +#include "util.h" #include "cpu.h" +#include "cpu_map.h" +#include "buf.h" #define VIR_FROM_THIS VIR_FROM_CPU +#define VENDOR_STRING_LENGTH 7 + static const char *archs[] = { "ppc64" }; +struct cpuPowerPC { + const char *name; + const char *vendor; + uint32_t pvr; +}; + +static const struct cpuPowerPC cpu_defs[] = { + {"POWER7", "IBM", 0x003f0200}, + {"POWER7_v2.1", "IBM", 0x003f0201}, + {"POWER7_v2.3", "IBM", 0x003f0203}, + {NULL, NULL, 0xffffffff} +}; + + +struct ppcVendor { + char *name; + struct ppcVendor *next; +}; + +struct ppcModel { + char *name; + const struct ppcVendor *vendor; + union cpuData *data; + struct ppcModel *next; +}; + +struct ppcMap { + struct ppcVendor *vendors; + struct ppcModel *models; +}; + +static int ConvertModelVendorFromPVR(char ***model, char ***vendor, uint32_t pvr) +{ + int i = 0; + while (cpu_defs[i].name != NULL) { + if (cpu_defs[i].pvr == pvr) { + **model = strdup(cpu_defs[i].name); + **vendor = strdup(cpu_defs[i].vendor); + return 0; + } + i ++; + } + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing the definition of this model")); + return -1; +} + +static int ConvertPVRFromModel(const char *model, uint32_t *pvr) +{ + int i = 0; + while (cpu_defs[i].name !=NULL) { + if (STREQ(cpu_defs[i].name, model)) { + *pvr = cpu_defs[i].pvr; + return 0; + } + i ++; + } + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing the definition of this model")); + return -1; +} + +static int cpuMatch(const union cpuData *data, char **cpu_model, + char **cpu_vendor, const struct ppcModel *model) +{ + int ret = 0; + + ret = ConvertModelVendorFromPVR(&cpu_model, &cpu_vendor, data->ppc.pvr); + + if (STREQ(model->name, *cpu_model) && + STREQ(model->vendor->name, *cpu_vendor)) + ret = 1; + + return ret; +} + + +static struct ppcModel * +ppcModelNew(void) +{ + struct ppcModel *model; + + if (VIR_ALLOC(model) < 0) + return NULL; + + if (VIR_ALLOC(model->data) < 0) { + VIR_FREE(model); + return NULL; + } + + return model; +} + +static void +ppcModelFree(struct ppcModel *model) +{ + if (model == NULL) + return; + + VIR_FREE(model->name); + + if (model->data) + VIR_FREE(model->data); + + VIR_FREE(model); +} + +static struct ppcModel * +ppcModelFind(const struct ppcMap *map, + const char *name) +{ + struct ppcModel *model; + + model = map->models; + while (model != NULL) { + if (STREQ(model->name, name)) + return model; + + model = model->next; + } + + return NULL; +} + +static struct ppcVendor * +ppcVendorFind(const struct ppcMap *map, + const char *name) +{ + struct ppcVendor *vendor; + + vendor = map->vendors; + while (vendor) { + if (STREQ(vendor->name, name)) + return vendor; + + vendor = vendor->next; + } + + return NULL; +} + +static void +ppcVendorFree(struct ppcVendor *vendor) +{ + if (!vendor) + return; + + VIR_FREE(vendor->name); + VIR_FREE(vendor); +} + +static int +ppcVendorLoad(xmlXPathContextPtr ctxt, + struct ppcMap *map) +{ + struct ppcVendor *vendor = NULL; + char *string = NULL; + int ret = 0; + + if (VIR_ALLOC(vendor) < 0) + goto no_memory; + + vendor->name = virXPathString("string(@name)", ctxt); + if (!vendor->name) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Missing CPU vendor name")); + goto ignore; + } + + if (ppcVendorFind(map, vendor->name)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("CPU vendor %s already defined"), vendor->name); + goto ignore; + } + + string = virXPathString("string(@string)", ctxt); + if (!string) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing vendor string for CPU vendor %s"), vendor->name); + goto ignore; + } + if (strlen(string) != VENDOR_STRING_LENGTH) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid CPU vendor string '%s'"), string); + goto ignore; + } + if (!map->vendors) + map->vendors = vendor; + else { + vendor->next = map->vendors; + map->vendors = vendor; + } + +out: + VIR_FREE(string); + + return ret; + +no_memory: + virReportOOMError(); + ret = -1; +ignore: + ppcVendorFree(vendor); + goto out; +} + +static int +ppcModelLoad(xmlXPathContextPtr ctxt, + struct ppcMap *map) +{ + xmlNodePtr *nodes = NULL; + struct ppcModel *model; + char *vendor = NULL; + int ret = 0; + + if (!(model = ppcModelNew())) + goto no_memory; + + model->name = virXPathString("string(@name)", ctxt); + if (model->name == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Missing CPU model name")); + goto ignore; + } + + ret = ConvertPVRFromModel(model->name, &model->data->ppc.pvr); + if (ret < 0) + goto ignore; + + + if (virXPathBoolean("boolean(./vendor)", ctxt)) { + vendor = virXPathString("string(./vendor/@name)", ctxt); + if (!vendor) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid vendor element in CPU model %s"), + model->name); + goto ignore; + } + + if (!(model->vendor = ppcVendorFind(map, vendor))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown vendor %s referenced by CPU model %s"), + vendor, model->name); + goto ignore; + } + } + + if (map->models == NULL) + map->models = model; + else { + model->next = map->models; + map->models = model; + } + +out: + VIR_FREE(vendor); + VIR_FREE(nodes); + return ret; + +no_memory: + virReportOOMError(); + ret = -1; + +ignore: + ppcModelFree(model); + goto out; +} + +static int +ppcMapLoadCallback(enum cpuMapElement element, + xmlXPathContextPtr ctxt, + void *data) +{ + struct ppcMap *map = data; + + switch (element) { + case CPU_MAP_ELEMENT_VENDOR: + return ppcVendorLoad(ctxt, map); + case CPU_MAP_ELEMENT_MODEL: + return ppcModelLoad(ctxt, map); + default: + break; + } + + return 0; +} + +static void +ppcMapFree(struct ppcMap *map) +{ + if (map == NULL) + return; + + while (map->models != NULL) { + struct ppcModel *model = map->models; + map->models = model->next; + ppcModelFree(model); + } + + while (map->vendors != NULL) { + struct ppcVendor *vendor = map->vendors; + map->vendors = vendor->next; + ppcVendorFree(vendor); + } + + VIR_FREE(map); +} + +static struct ppcMap * +ppcLoadMap(void) +{ + struct ppcMap *map; + + if (VIR_ALLOC(map) < 0) { + virReportOOMError(); + return NULL; + } + + if (cpuMapLoad("ppc64", ppcMapLoadCallback, map) < 0) + goto error; + + return map; + +error: + ppcMapFree(map); + return NULL; +} + +static struct ppcModel * +ppcModelCopy(const struct ppcModel *model) +{ + struct ppcModel *copy; + + if (VIR_ALLOC(copy) < 0 + || VIR_ALLOC(copy->data) < 0 + || !(copy->name = strdup(model->name))){ + ppcModelFree(copy); + return NULL; + } + + copy->data->ppc.pvr = model->data->ppc.pvr; + copy->vendor = model->vendor; + + return copy; +} + +static struct ppcModel * +ppcModelFromCPU(const virCPUDefPtr cpu, + const struct ppcMap *map) +{ + struct ppcModel *model = NULL; + + if ((model = ppcModelFind(map, cpu->model))) { + if ((model = ppcModelCopy(model)) == NULL) { + goto no_memory; + } + } else if (!(model = ppcModelNew())) { + goto no_memory; + } + + return model; + +no_memory: + virReportOOMError(); + ppcModelFree(model); + + return NULL; +} + +static virCPUCompareResult +PowerPCCompare(virCPUDefPtr host, + virCPUDefPtr cpu) +{ + if ((cpu->arch && STRNEQ(host->arch, cpu->arch)) || + STRNEQ(host->model, cpu->model)) + return VIR_CPU_COMPARE_INCOMPATIBLE; + + return VIR_CPU_COMPARE_IDENTICAL; +} + +static int +PowerPCDecode(virCPUDefPtr cpu, + const union cpuData *data, + const char **models, + unsigned int nmodels, + const char *preferred) +{ + int ret = -1; + struct ppcMap *map; + const struct ppcModel *candidate; + virCPUDefPtr cpuCandidate; + virCPUDefPtr cpuModel = NULL; + char *cpu_vendor = NULL; + char *cpu_model = NULL; + unsigned int i; + + if (data == NULL || (map = ppcLoadMap()) == NULL) + return -1; + + candidate = map->models; + + while (candidate != NULL) { + bool allowed = (models == NULL); + + for (i = 0; i < nmodels; i++) { + if (models && models[i] && STREQ(models[i], candidate->name)) { + allowed = true; + break; + } + } + + if (!allowed) { + if (preferred && STREQ(candidate->name, preferred)) { + if (cpu->fallback != VIR_CPU_FALLBACK_ALLOW) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("CPU model %s is not supported by hypervisor"), + preferred); + goto out; + } else { + VIR_WARN("Preferred CPU model %s not allowed by" + " hypervisor; closest supported model will be" + " used", preferred); + } + } else { + VIR_DEBUG("CPU model %s not allowed by hypervisor; ignoring", + candidate->name); + } + goto next; + } + + if (VIR_ALLOC(cpuCandidate) < 0) { + virReportOOMError(); + goto out; + } + + cpuCandidate->model = strdup(candidate->name); + cpuCandidate->vendor = strdup(candidate->vendor->name); + + if (preferred && STREQ(cpuCandidate->model, preferred)) { + virCPUDefFree(cpuModel); + cpuModel = cpuCandidate; + break; + } + + ret = cpuMatch(data, &cpu_model, &cpu_vendor, candidate); + if (ret < 0) { + VIR_FREE(cpuCandidate); + goto out; + }else if(ret == 1) { + cpuCandidate->model = cpu_model; + cpuCandidate->vendor = cpu_vendor; + virCPUDefFree(cpuModel); + cpuModel = cpuCandidate; + break; + } + + virCPUDefFree(cpuCandidate); + + next: + candidate = candidate->next; + } + + if (cpuModel == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Cannot find suitable CPU model for given data")); + goto out; + } + + cpu->model = cpuModel->model; + cpu->vendor = cpuModel->vendor; + VIR_FREE(cpuModel); + + ret = 0; + +out: + ppcMapFree(map); + virCPUDefFree(cpuModel); + + return ret; +} + +static uint32_t ppc_mfpvr(void) +{ + uint32_t pvr; + asm ("mfpvr %0" + : "=r"(pvr)); + return pvr; +} + +static void +PowerPCDataFree(union cpuData *data) +{ + if (data == NULL) + return; + + VIR_FREE(data); +} + static union cpuData * PowerPCNodeData(void) { @@ -42,40 +550,85 @@ PowerPCNodeData(void) return NULL; } + data->ppc.pvr = ppc_mfpvr(); + return data; } - static int -PowerPCDecode(virCPUDefPtr cpu ATTRIBUTE_UNUSED, - const union cpuData *data ATTRIBUTE_UNUSED, - const char **models ATTRIBUTE_UNUSED, - unsigned int nmodels ATTRIBUTE_UNUSED, - const char *preferred ATTRIBUTE_UNUSED) +PowerPCUpdate(virCPUDefPtr guest ATTRIBUTE_UNUSED, + const virCPUDefPtr host ATTRIBUTE_UNUSED) { - return 0; + return 0; } - -static void -PowerPCDataFree(union cpuData *data) +static virCPUDefPtr +PowerPCBaseline(virCPUDefPtr *cpus, + unsigned int ncpus ATTRIBUTE_UNUSED, + const char **models ATTRIBUTE_UNUSED, + unsigned int nmodels ATTRIBUTE_UNUSED) { - if (data == NULL) - return; + struct ppcMap *map = NULL; + struct ppcModel *base_model = NULL; + virCPUDefPtr cpu = NULL; + struct ppcModel *model = NULL; + bool outputModel = true; + + if (!(map = ppcLoadMap())) { + goto error; + } + + if (!(base_model = ppcModelFromCPU(cpus[0], map))) { + goto error; + } + + if (VIR_ALLOC(cpu) < 0 || + !(cpu->arch = strdup(cpus[0]->arch))) + goto no_memory; + cpu->type = VIR_CPU_TYPE_GUEST; + cpu->match = VIR_CPU_MATCH_EXACT; + + if (!cpus[0]->model) { + outputModel = false; + } else if (!(model = ppcModelFind(map, cpus[0]->model))) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Unknown CPU vendor %s"), cpus[0]->model); + goto error; + } + + base_model->data->ppc.pvr = model->data->ppc.pvr; + if (PowerPCDecode(cpu, base_model->data, models, nmodels, NULL) < 0) + goto error; + + if (!outputModel) + VIR_FREE(cpu->model); + + VIR_FREE(cpu->arch); + +cleanup: + ppcModelFree(base_model); + ppcMapFree(map); - VIR_FREE(data); + return cpu; +no_memory: + virReportOOMError(); +error: + ppcModelFree(model); + virCPUDefFree(cpu); + cpu = NULL; + goto cleanup; } struct cpuArchDriver cpuDriverPowerPC = { .name = "ppc64", .arch = archs, .narch = ARRAY_CARDINALITY(archs), - .compare = NULL, + .compare = PowerPCCompare, .decode = PowerPCDecode, .encode = NULL, .free = PowerPCDataFree, .nodeData = PowerPCNodeData, .guestData = NULL, - .baseline = NULL, - .update = NULL, + .baseline = PowerPCBaseline, + .update = PowerPCUpdate, .hasFeature = NULL, }; -- 1.7.7.6
On 09.10.2012 09:58, Li Zhang wrote:
Currently, the CPU model driver is not implemented for PowerPC. Host's CPU information is needed to exposed to guests' XML file some time.
This patch is to implement the callback functions of CPU model driver.
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- src/cpu/cpu_map.xml | 14 ++ src/cpu/cpu_powerpc.c | 585 +++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 583 insertions(+), 16 deletions(-)
diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index 9a89e2e..affcce3 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -495,4 +495,18 @@ <feature name='fma4'/> </model> </arch> + <arch name='ppc64'> + <!-- vendor definitions --> + <vendor name='IBM' string='PowerPC'/> + <!-- IBM-based CPU models --> + <model name='POWER7'> + <vendor name='IBM'/> + </model> + <model name='POWER7_v2.1'> + <vendor name='IBM'/> + </model> + <model name='POWER7_v2.3'> + <vendor name='IBM'/> + </model> + </arch> </cpus> diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c index 1b77caf..3a88e68 100644 --- a/src/cpu/cpu_powerpc.c +++ b/src/cpu/cpu_powerpc.c @@ -20,18 +20,526 @@ * Authors: * Anton Blanchard <anton@au.ibm.com> * Prerna Saxena <prerna@linux.vnet.ibm.com> + * Li Zhang <zhlcindy@linux.vnet.ibm.com> */
#include <config.h> +#include <stdint.h>
+#include "logging.h" #include "memory.h" +#include "util.h" #include "cpu.h"
+#include "cpu_map.h" +#include "buf.h"
#define VIR_FROM_THIS VIR_FROM_CPU
+#define VENDOR_STRING_LENGTH 7 + static const char *archs[] = { "ppc64" };
+struct cpuPowerPC { + const char *name; + const char *vendor; + uint32_t pvr; +}; + +static const struct cpuPowerPC cpu_defs[] = { + {"POWER7", "IBM", 0x003f0200}, + {"POWER7_v2.1", "IBM", 0x003f0201}, + {"POWER7_v2.3", "IBM", 0x003f0203}, + {NULL, NULL, 0xffffffff} +}; + + +struct ppcVendor { + char *name; + struct ppcVendor *next; +}; + +struct ppcModel { + char *name; + const struct ppcVendor *vendor; + union cpuData *data; + struct ppcModel *next; +}; + +struct ppcMap { + struct ppcVendor *vendors; + struct ppcModel *models; +}; + +static int ConvertModelVendorFromPVR(char ***model, char ***vendor, uint32_t pvr)
This line is too long. Moreover, we tend to prefix functions with the part of libvirt they live in. So this function should be static int cpuConvertModelVendorFromPVR(char ***model, char ***vendor, uint32_t pvr); or something like that.
+{ + int i = 0; + while (cpu_defs[i].name != NULL) { + if (cpu_defs[i].pvr == pvr) { + **model = strdup(cpu_defs[i].name); + **vendor = strdup(cpu_defs[i].vendor); + return 0; + } + i ++; + }
Yeah, we've used 'while' in this way many times. But for() loop - while being totally exchangeable in this case is better.
+ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing the definition of this model"));
s/VIR_ERR_INTERNAL_ERROR,/VIR_ERR_INTERNAL_ERROR, "%s"/
+ return -1; +} + +static int ConvertPVRFromModel(const char *model, uint32_t *pvr) +{ + int i = 0; + while (cpu_defs[i].name !=NULL) { + if (STREQ(cpu_defs[i].name, model)) { + *pvr = cpu_defs[i].pvr; + return 0; + } + i ++; + } + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing the definition of this model"));
ditto
+ return -1; +}
Same applies for this function.
+ +static int cpuMatch(const union cpuData *data, char **cpu_model, + char **cpu_vendor, const struct ppcModel *model) +{ + int ret = 0; + + ret = ConvertModelVendorFromPVR(&cpu_model, &cpu_vendor, data->ppc.pvr); + + if (STREQ(model->name, *cpu_model) && + STREQ(model->vendor->name, *cpu_vendor)) + ret = 1; + + return ret; +} + + +static struct ppcModel * +ppcModelNew(void) +{ + struct ppcModel *model; + + if (VIR_ALLOC(model) < 0) + return NULL; + + if (VIR_ALLOC(model->data) < 0) { + VIR_FREE(model); + return NULL; + } + + return model; +} + +static void +ppcModelFree(struct ppcModel *model) +{ + if (model == NULL) + return; + + VIR_FREE(model->name); + + if (model->data) + VIR_FREE(model->data);
VIR_FREE(NULL) is just fine. so you don't need to test model->data.
+ + VIR_FREE(model); +} + +static struct ppcModel * +ppcModelFind(const struct ppcMap *map, + const char *name) +{ + struct ppcModel *model; + + model = map->models; + while (model != NULL) { + if (STREQ(model->name, name)) + return model; + + model = model->next; + } + + return NULL; +} + +static struct ppcVendor * +ppcVendorFind(const struct ppcMap *map, + const char *name) +{ + struct ppcVendor *vendor; + + vendor = map->vendors; + while (vendor) { + if (STREQ(vendor->name, name)) + return vendor; + + vendor = vendor->next; + } + + return NULL; +} + +static void +ppcVendorFree(struct ppcVendor *vendor) +{ + if (!vendor) + return; + + VIR_FREE(vendor->name); + VIR_FREE(vendor); +} + +static int +ppcVendorLoad(xmlXPathContextPtr ctxt, + struct ppcMap *map) +{ + struct ppcVendor *vendor = NULL; + char *string = NULL; + int ret = 0; + + if (VIR_ALLOC(vendor) < 0) + goto no_memory; + + vendor->name = virXPathString("string(@name)", ctxt);
I think attributes are always string, aren't they? But this doesn't hurt.
+ if (!vendor->name) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Missing CPU vendor name")); + goto ignore; + } + + if (ppcVendorFind(map, vendor->name)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("CPU vendor %s already defined"), vendor->name); + goto ignore; + } + + string = virXPathString("string(@string)", ctxt); + if (!string) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing vendor string for CPU vendor %s"), vendor->name); + goto ignore; + } + if (strlen(string) != VENDOR_STRING_LENGTH) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid CPU vendor string '%s'"), string); + goto ignore; + }
Does PowerPC equivalent of CPUID requires 7 characters at most?
+ if (!map->vendors) + map->vendors = vendor; + else { + vendor->next = map->vendors; + map->vendors = vendor; + } + +out: + VIR_FREE(string); + + return ret; + +no_memory: + virReportOOMError(); + ret = -1;
we often initialize ret = -1; and when everything went okay, we set ret = 0; in this case that would be just before out: label. However, in this case out can be dropped and we can have the only error label. Having VIR_FREE(string) twice there is not a problem as it is single line.
+ignore: + ppcVendorFree(vendor); + goto out; +} + +static int +ppcModelLoad(xmlXPathContextPtr ctxt, + struct ppcMap *map) +{ + xmlNodePtr *nodes = NULL; + struct ppcModel *model; + char *vendor = NULL; + int ret = 0; + + if (!(model = ppcModelNew())) + goto no_memory; + + model->name = virXPathString("string(@name)", ctxt); + if (model->name == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Missing CPU model name")); + goto ignore; + } + + ret = ConvertPVRFromModel(model->name, &model->data->ppc.pvr); + if (ret < 0) + goto ignore; + + + if (virXPathBoolean("boolean(./vendor)", ctxt)) { + vendor = virXPathString("string(./vendor/@name)", ctxt); + if (!vendor) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid vendor element in CPU model %s"), + model->name); + goto ignore; + } + + if (!(model->vendor = ppcVendorFind(map, vendor))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown vendor %s referenced by CPU model %s"), + vendor, model->name); + goto ignore; + } + } + + if (map->models == NULL) + map->models = model; + else { + model->next = map->models; + map->models = model; + } + +out: + VIR_FREE(vendor); + VIR_FREE(nodes); + return ret; + +no_memory: + virReportOOMError(); + ret = -1; + +ignore: + ppcModelFree(model); + goto out; +}
see my comments to previous function.
+ +static int +ppcMapLoadCallback(enum cpuMapElement element, + xmlXPathContextPtr ctxt, + void *data) +{ + struct ppcMap *map = data; + + switch (element) { + case CPU_MAP_ELEMENT_VENDOR: + return ppcVendorLoad(ctxt, map); + case CPU_MAP_ELEMENT_MODEL: + return ppcModelLoad(ctxt, map); + default: + break; + } + + return 0; +} + +static void +ppcMapFree(struct ppcMap *map) +{ + if (map == NULL) + return; + + while (map->models != NULL) { + struct ppcModel *model = map->models; + map->models = model->next; + ppcModelFree(model); + } + + while (map->vendors != NULL) { + struct ppcVendor *vendor = map->vendors; + map->vendors = vendor->next; + ppcVendorFree(vendor); + } + + VIR_FREE(map); +} + +static struct ppcMap * +ppcLoadMap(void) +{ + struct ppcMap *map; + + if (VIR_ALLOC(map) < 0) { + virReportOOMError(); + return NULL; + } + + if (cpuMapLoad("ppc64", ppcMapLoadCallback, map) < 0) + goto error; + + return map; + +error: + ppcMapFree(map); + return NULL; +} + +static struct ppcModel * +ppcModelCopy(const struct ppcModel *model) +{ + struct ppcModel *copy; + + if (VIR_ALLOC(copy) < 0 + || VIR_ALLOC(copy->data) < 0 + || !(copy->name = strdup(model->name))){ + ppcModelFree(copy); + return NULL; + } + + copy->data->ppc.pvr = model->data->ppc.pvr; + copy->vendor = model->vendor; + + return copy; +} + +static struct ppcModel * +ppcModelFromCPU(const virCPUDefPtr cpu, + const struct ppcMap *map) +{ + struct ppcModel *model = NULL; + + if ((model = ppcModelFind(map, cpu->model))) { + if ((model = ppcModelCopy(model)) == NULL) { + goto no_memory; + } + } else if (!(model = ppcModelNew())) { + goto no_memory; + } + + return model; + +no_memory: + virReportOOMError(); + ppcModelFree(model); + + return NULL; +} + +static virCPUCompareResult +PowerPCCompare(virCPUDefPtr host,
whitespace at the EOL
+ virCPUDefPtr cpu) +{ + if ((cpu->arch && STRNEQ(host->arch, cpu->arch)) || + STRNEQ(host->model, cpu->model)) + return VIR_CPU_COMPARE_INCOMPATIBLE; + + return VIR_CPU_COMPARE_IDENTICAL; +} + +static int +PowerPCDecode(virCPUDefPtr cpu, + const union cpuData *data, + const char **models, + unsigned int nmodels, + const char *preferred) +{ + int ret = -1; + struct ppcMap *map; + const struct ppcModel *candidate; + virCPUDefPtr cpuCandidate; + virCPUDefPtr cpuModel = NULL; + char *cpu_vendor = NULL; + char *cpu_model = NULL; + unsigned int i; + + if (data == NULL || (map = ppcLoadMap()) == NULL) + return -1; + + candidate = map->models; + + while (candidate != NULL) { + bool allowed = (models == NULL); + + for (i = 0; i < nmodels; i++) { + if (models && models[i] && STREQ(models[i], candidate->name)) { + allowed = true; + break; + } + } + + if (!allowed) { + if (preferred && STREQ(candidate->name, preferred)) { + if (cpu->fallback != VIR_CPU_FALLBACK_ALLOW) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("CPU model %s is not supported by hypervisor"), + preferred); + goto out; + } else { + VIR_WARN("Preferred CPU model %s not allowed by" + " hypervisor; closest supported model will be" + " used", preferred); + } + } else { + VIR_DEBUG("CPU model %s not allowed by hypervisor; ignoring", + candidate->name); + } + goto next; + } + + if (VIR_ALLOC(cpuCandidate) < 0) { + virReportOOMError(); + goto out; + } + + cpuCandidate->model = strdup(candidate->name); + cpuCandidate->vendor = strdup(candidate->vendor->name); + + if (preferred && STREQ(cpuCandidate->model, preferred)) { + virCPUDefFree(cpuModel); + cpuModel = cpuCandidate; + break; + } + + ret = cpuMatch(data, &cpu_model, &cpu_vendor, candidate); + if (ret < 0) { + VIR_FREE(cpuCandidate); + goto out; + }else if(ret == 1) { + cpuCandidate->model = cpu_model; + cpuCandidate->vendor = cpu_vendor; + virCPUDefFree(cpuModel); + cpuModel = cpuCandidate; + break; + } + + virCPUDefFree(cpuCandidate); + + next: + candidate = candidate->next; + } + + if (cpuModel == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Cannot find suitable CPU model for given data")); + goto out; + } + + cpu->model = cpuModel->model; + cpu->vendor = cpuModel->vendor; + VIR_FREE(cpuModel); + + ret = 0; + +out: + ppcMapFree(map); + virCPUDefFree(cpuModel); + + return ret; +} + +static uint32_t ppc_mfpvr(void) +{ + uint32_t pvr; + asm ("mfpvr %0" + : "=r"(pvr)); + return pvr; +}
There is no such instruction on x86. So we need to make this function ppc only and introduce stub for other architectures.
+ +static void +PowerPCDataFree(union cpuData *data) +{ + if (data == NULL) + return; + + VIR_FREE(data); +} + static union cpuData * PowerPCNodeData(void) { @@ -42,40 +550,85 @@ PowerPCNodeData(void) return NULL; }
+ data->ppc.pvr = ppc_mfpvr(); + return data; }
- static int -PowerPCDecode(virCPUDefPtr cpu ATTRIBUTE_UNUSED, - const union cpuData *data ATTRIBUTE_UNUSED, - const char **models ATTRIBUTE_UNUSED, - unsigned int nmodels ATTRIBUTE_UNUSED, - const char *preferred ATTRIBUTE_UNUSED) +PowerPCUpdate(virCPUDefPtr guest ATTRIBUTE_UNUSED, + const virCPUDefPtr host ATTRIBUTE_UNUSED) { - return 0; + return 0; } - -static void -PowerPCDataFree(union cpuData *data) +static virCPUDefPtr +PowerPCBaseline(virCPUDefPtr *cpus, + unsigned int ncpus ATTRIBUTE_UNUSED, + const char **models ATTRIBUTE_UNUSED, + unsigned int nmodels ATTRIBUTE_UNUSED) { - if (data == NULL) - return; + struct ppcMap *map = NULL; + struct ppcModel *base_model = NULL; + virCPUDefPtr cpu = NULL; + struct ppcModel *model = NULL; + bool outputModel = true; + + if (!(map = ppcLoadMap())) { + goto error; + } + + if (!(base_model = ppcModelFromCPU(cpus[0], map))) { + goto error; + } + + if (VIR_ALLOC(cpu) < 0 || + !(cpu->arch = strdup(cpus[0]->arch))) + goto no_memory; + cpu->type = VIR_CPU_TYPE_GUEST; + cpu->match = VIR_CPU_MATCH_EXACT; + + if (!cpus[0]->model) { + outputModel = false; + } else if (!(model = ppcModelFind(map, cpus[0]->model))) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Unknown CPU vendor %s"), cpus[0]->model); + goto error; + } + + base_model->data->ppc.pvr = model->data->ppc.pvr; + if (PowerPCDecode(cpu, base_model->data, models, nmodels, NULL) < 0) + goto error; + + if (!outputModel) + VIR_FREE(cpu->model); + + VIR_FREE(cpu->arch); + +cleanup: + ppcModelFree(base_model); + ppcMapFree(map);
- VIR_FREE(data); + return cpu; +no_memory: + virReportOOMError(); +error: + ppcModelFree(model); + virCPUDefFree(cpu); + cpu = NULL; + goto cleanup; }
struct cpuArchDriver cpuDriverPowerPC = { .name = "ppc64", .arch = archs, .narch = ARRAY_CARDINALITY(archs), - .compare = NULL, + .compare = PowerPCCompare, .decode = PowerPCDecode, .encode = NULL, .free = PowerPCDataFree, .nodeData = PowerPCNodeData, .guestData = NULL, - .baseline = NULL, - .update = NULL, + .baseline = PowerPCBaseline, + .update = PowerPCUpdate, .hasFeature = NULL, };
Moreover, you need to add src/cpu/cpu_powerpc.c into po/POTFILES.in make syntax-check should have warned you about this. Michal
On Thu, Oct 11, 2012 at 11:12 PM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 09.10.2012 09:58, Li Zhang wrote:
Currently, the CPU model driver is not implemented for PowerPC. Host's CPU information is needed to exposed to guests' XML file some time.
This patch is to implement the callback functions of CPU model driver.
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- src/cpu/cpu_map.xml | 14 ++ src/cpu/cpu_powerpc.c | 585 +++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 583 insertions(+), 16 deletions(-)
diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index 9a89e2e..affcce3 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -495,4 +495,18 @@ <feature name='fma4'/> </model> </arch> + <arch name='ppc64'> + <!-- vendor definitions --> + <vendor name='IBM' string='PowerPC'/> + <!-- IBM-based CPU models --> + <model name='POWER7'> + <vendor name='IBM'/> + </model> + <model name='POWER7_v2.1'> + <vendor name='IBM'/> + </model> + <model name='POWER7_v2.3'> + <vendor name='IBM'/> + </model> + </arch> </cpus> diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c index 1b77caf..3a88e68 100644 --- a/src/cpu/cpu_powerpc.c +++ b/src/cpu/cpu_powerpc.c @@ -20,18 +20,526 @@ * Authors: * Anton Blanchard <anton@au.ibm.com> * Prerna Saxena <prerna@linux.vnet.ibm.com> + * Li Zhang <zhlcindy@linux.vnet.ibm.com> */
#include <config.h> +#include <stdint.h>
+#include "logging.h" #include "memory.h" +#include "util.h" #include "cpu.h"
+#include "cpu_map.h" +#include "buf.h"
#define VIR_FROM_THIS VIR_FROM_CPU
+#define VENDOR_STRING_LENGTH 7 + static const char *archs[] = { "ppc64" };
+struct cpuPowerPC { + const char *name; + const char *vendor; + uint32_t pvr; +}; + +static const struct cpuPowerPC cpu_defs[] = { + {"POWER7", "IBM", 0x003f0200}, + {"POWER7_v2.1", "IBM", 0x003f0201}, + {"POWER7_v2.3", "IBM", 0x003f0203}, + {NULL, NULL, 0xffffffff} +}; + + +struct ppcVendor { + char *name; + struct ppcVendor *next; +}; + +struct ppcModel { + char *name; + const struct ppcVendor *vendor; + union cpuData *data; + struct ppcModel *next; +}; + +struct ppcMap { + struct ppcVendor *vendors; + struct ppcModel *models; +}; + +static int ConvertModelVendorFromPVR(char ***model, char ***vendor, uint32_t pvr)
This line is too long. Moreover, we tend to prefix functions with the part of libvirt they live in. So this function should be
static int cpuConvertModelVendorFromPVR(char ***model, char ***vendor, uint32_t pvr);
or something like that. Got it, I will modify it in next version. Thanks.
+{ + int i = 0; + while (cpu_defs[i].name != NULL) { + if (cpu_defs[i].pvr == pvr) { + **model = strdup(cpu_defs[i].name); + **vendor = strdup(cpu_defs[i].vendor); + return 0; + } + i ++; + }
Yeah, we've used 'while' in this way many times. But for() loop - while being totally exchangeable in this case is better.
Thanks, I will modify it.
+ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing the definition of this model"));
s/VIR_ERR_INTERNAL_ERROR,/VIR_ERR_INTERNAL_ERROR, "%s"/
Sorry for my mistake, I will fix this.
+ return -1; +} + +static int ConvertPVRFromModel(const char *model, uint32_t *pvr) +{ + int i = 0; + while (cpu_defs[i].name !=NULL) { + if (STREQ(cpu_defs[i].name, model)) { + *pvr = cpu_defs[i].pvr; + return 0; + } + i ++; + } + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing the definition of this model"));
ditto
+ return -1; +}
Same applies for this function.
I see.
+ +static int cpuMatch(const union cpuData *data, char **cpu_model, + char **cpu_vendor, const struct ppcModel *model) +{ + int ret = 0; + + ret = ConvertModelVendorFromPVR(&cpu_model, &cpu_vendor, data->ppc.pvr); + + if (STREQ(model->name, *cpu_model) && + STREQ(model->vendor->name, *cpu_vendor)) + ret = 1; + + return ret; +} + + +static struct ppcModel * +ppcModelNew(void) +{ + struct ppcModel *model; + + if (VIR_ALLOC(model) < 0) + return NULL; + + if (VIR_ALLOC(model->data) < 0) { + VIR_FREE(model); + return NULL; + } + + return model; +} + +static void +ppcModelFree(struct ppcModel *model) +{ + if (model == NULL) + return; + + VIR_FREE(model->name); + + if (model->data) + VIR_FREE(model->data);
VIR_FREE(NULL) is just fine. so you don't need to test model->data.
I see, I will remove this assertion.
+ + VIR_FREE(model); +} + +static struct ppcModel * +ppcModelFind(const struct ppcMap *map, + const char *name) +{ + struct ppcModel *model; + + model = map->models; + while (model != NULL) { + if (STREQ(model->name, name)) + return model; + + model = model->next; + } + + return NULL; +} + +static struct ppcVendor * +ppcVendorFind(const struct ppcMap *map, + const char *name) +{ + struct ppcVendor *vendor; + + vendor = map->vendors; + while (vendor) { + if (STREQ(vendor->name, name)) + return vendor; + + vendor = vendor->next; + } + + return NULL; +} + +static void +ppcVendorFree(struct ppcVendor *vendor) +{ + if (!vendor) + return; + + VIR_FREE(vendor->name); + VIR_FREE(vendor); +} + +static int +ppcVendorLoad(xmlXPathContextPtr ctxt, + struct ppcMap *map) +{ + struct ppcVendor *vendor = NULL; + char *string = NULL; + int ret = 0; + + if (VIR_ALLOC(vendor) < 0) + goto no_memory; + + vendor->name = virXPathString("string(@name)", ctxt);
I think attributes are always string, aren't they? But this doesn't hurt.
Yes, it should be string. I think this way can make sure that this is a string.
+ if (!vendor->name) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Missing CPU vendor name")); + goto ignore; + } + + if (ppcVendorFind(map, vendor->name)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("CPU vendor %s already defined"), vendor->name); + goto ignore; + } + + string = virXPathString("string(@string)", ctxt); + if (!string) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing vendor string for CPU vendor %s"), vendor->name); + goto ignore; + } + if (strlen(string) != VENDOR_STRING_LENGTH) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid CPU vendor string '%s'"), string); + goto ignore; + }
Does PowerPC equivalent of CPUID requires 7 characters at most?
There shouldn't be this limitation. I will make this clear, and clean this code in next version.
+ if (!map->vendors) + map->vendors = vendor; + else { + vendor->next = map->vendors; + map->vendors = vendor; + } + +out: + VIR_FREE(string); + + return ret; + +no_memory: + virReportOOMError(); + ret = -1;
we often initialize ret = -1; and when everything went okay, we set ret = 0; in this case that would be just before out: label. However, in this case out can be dropped and we can have the only error label. Having VIR_FREE(string) twice there is not a problem as it is single line.
Got it, thanks for your suggestions. I will clean this.
+ignore: + ppcVendorFree(vendor); + goto out; +} + +static int +ppcModelLoad(xmlXPathContextPtr ctxt, + struct ppcMap *map) +{ + xmlNodePtr *nodes = NULL; + struct ppcModel *model; + char *vendor = NULL; + int ret = 0; + + if (!(model = ppcModelNew())) + goto no_memory; + + model->name = virXPathString("string(@name)", ctxt); + if (model->name == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Missing CPU model name")); + goto ignore; + } + + ret = ConvertPVRFromModel(model->name, &model->data->ppc.pvr); + if (ret < 0) + goto ignore; + + + if (virXPathBoolean("boolean(./vendor)", ctxt)) { + vendor = virXPathString("string(./vendor/@name)", ctxt); + if (!vendor) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid vendor element in CPU model %s"), + model->name); + goto ignore; + } + + if (!(model->vendor = ppcVendorFind(map, vendor))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown vendor %s referenced by CPU model %s"), + vendor, model->name); + goto ignore; + } + } + + if (map->models == NULL) + map->models = model; + else { + model->next = map->models; + map->models = model; + } + +out: + VIR_FREE(vendor); + VIR_FREE(nodes); + return ret; + +no_memory: + virReportOOMError(); + ret = -1; + +ignore: + ppcModelFree(model); + goto out; +}
see my comments to previous function.
Thanks.
+ +static int +ppcMapLoadCallback(enum cpuMapElement element, + xmlXPathContextPtr ctxt, + void *data) +{ + struct ppcMap *map = data; + + switch (element) { + case CPU_MAP_ELEMENT_VENDOR: + return ppcVendorLoad(ctxt, map); + case CPU_MAP_ELEMENT_MODEL: + return ppcModelLoad(ctxt, map); + default: + break; + } + + return 0; +} + +static void +ppcMapFree(struct ppcMap *map) +{ + if (map == NULL) + return; + + while (map->models != NULL) { + struct ppcModel *model = map->models; + map->models = model->next; + ppcModelFree(model); + } + + while (map->vendors != NULL) { + struct ppcVendor *vendor = map->vendors; + map->vendors = vendor->next; + ppcVendorFree(vendor); + } + + VIR_FREE(map); +} + +static struct ppcMap * +ppcLoadMap(void) +{ + struct ppcMap *map; + + if (VIR_ALLOC(map) < 0) { + virReportOOMError(); + return NULL; + } + + if (cpuMapLoad("ppc64", ppcMapLoadCallback, map) < 0) + goto error; + + return map; + +error: + ppcMapFree(map); + return NULL; +} + +static struct ppcModel * +ppcModelCopy(const struct ppcModel *model) +{ + struct ppcModel *copy; + + if (VIR_ALLOC(copy) < 0 + || VIR_ALLOC(copy->data) < 0 + || !(copy->name = strdup(model->name))){ + ppcModelFree(copy); + return NULL; + } + + copy->data->ppc.pvr = model->data->ppc.pvr; + copy->vendor = model->vendor; + + return copy; +} + +static struct ppcModel * +ppcModelFromCPU(const virCPUDefPtr cpu, + const struct ppcMap *map) +{ + struct ppcModel *model = NULL; + + if ((model = ppcModelFind(map, cpu->model))) { + if ((model = ppcModelCopy(model)) == NULL) { + goto no_memory; + } + } else if (!(model = ppcModelNew())) { + goto no_memory; + } + + return model; + +no_memory: + virReportOOMError(); + ppcModelFree(model); + + return NULL; +} + +static virCPUCompareResult +PowerPCCompare(virCPUDefPtr host,
whitespace at the EOL OK.
+ virCPUDefPtr cpu) +{ + if ((cpu->arch && STRNEQ(host->arch, cpu->arch)) || + STRNEQ(host->model, cpu->model)) + return VIR_CPU_COMPARE_INCOMPATIBLE; + + return VIR_CPU_COMPARE_IDENTICAL; +} + +static int +PowerPCDecode(virCPUDefPtr cpu, + const union cpuData *data, + const char **models, + unsigned int nmodels, + const char *preferred) +{ + int ret = -1; + struct ppcMap *map; + const struct ppcModel *candidate; + virCPUDefPtr cpuCandidate; + virCPUDefPtr cpuModel = NULL; + char *cpu_vendor = NULL; + char *cpu_model = NULL; + unsigned int i; + + if (data == NULL || (map = ppcLoadMap()) == NULL) + return -1; + + candidate = map->models; + + while (candidate != NULL) { + bool allowed = (models == NULL); + + for (i = 0; i < nmodels; i++) { + if (models && models[i] && STREQ(models[i], candidate->name)) { + allowed = true; + break; + } + } + + if (!allowed) { + if (preferred && STREQ(candidate->name, preferred)) { + if (cpu->fallback != VIR_CPU_FALLBACK_ALLOW) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("CPU model %s is not supported by hypervisor"), + preferred); + goto out; + } else { + VIR_WARN("Preferred CPU model %s not allowed by" + " hypervisor; closest supported model will be" + " used", preferred); + } + } else { + VIR_DEBUG("CPU model %s not allowed by hypervisor; ignoring", + candidate->name); + } + goto next; + } + + if (VIR_ALLOC(cpuCandidate) < 0) { + virReportOOMError(); + goto out; + } + + cpuCandidate->model = strdup(candidate->name); + cpuCandidate->vendor = strdup(candidate->vendor->name); + + if (preferred && STREQ(cpuCandidate->model, preferred)) { + virCPUDefFree(cpuModel); + cpuModel = cpuCandidate; + break; + } + + ret = cpuMatch(data, &cpu_model, &cpu_vendor, candidate); + if (ret < 0) { + VIR_FREE(cpuCandidate); + goto out; + }else if(ret == 1) { + cpuCandidate->model = cpu_model; + cpuCandidate->vendor = cpu_vendor; + virCPUDefFree(cpuModel); + cpuModel = cpuCandidate; + break; + } + + virCPUDefFree(cpuCandidate); + + next: + candidate = candidate->next; + } + + if (cpuModel == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Cannot find suitable CPU model for given data")); + goto out; + } + + cpu->model = cpuModel->model; + cpu->vendor = cpuModel->vendor; + VIR_FREE(cpuModel); + + ret = 0; + +out: + ppcMapFree(map); + virCPUDefFree(cpuModel); + + return ret; +} + +static uint32_t ppc_mfpvr(void) +{ + uint32_t pvr; + asm ("mfpvr %0" + : "=r"(pvr)); + return pvr; +}
There is no such instruction on x86. So we need to make this function ppc only and introduce stub for other architectures.
OK, I think this driver is for ppc64. For other architectures, it shouldn't be called.
+ +static void +PowerPCDataFree(union cpuData *data) +{ + if (data == NULL) + return; + + VIR_FREE(data); +} + static union cpuData * PowerPCNodeData(void) { @@ -42,40 +550,85 @@ PowerPCNodeData(void) return NULL; }
+ data->ppc.pvr = ppc_mfpvr(); + return data; }
- static int -PowerPCDecode(virCPUDefPtr cpu ATTRIBUTE_UNUSED, - const union cpuData *data ATTRIBUTE_UNUSED, - const char **models ATTRIBUTE_UNUSED, - unsigned int nmodels ATTRIBUTE_UNUSED, - const char *preferred ATTRIBUTE_UNUSED) +PowerPCUpdate(virCPUDefPtr guest ATTRIBUTE_UNUSED, + const virCPUDefPtr host ATTRIBUTE_UNUSED) { - return 0; + return 0; } - -static void -PowerPCDataFree(union cpuData *data) +static virCPUDefPtr +PowerPCBaseline(virCPUDefPtr *cpus, + unsigned int ncpus ATTRIBUTE_UNUSED, + const char **models ATTRIBUTE_UNUSED, + unsigned int nmodels ATTRIBUTE_UNUSED) { - if (data == NULL) - return; + struct ppcMap *map = NULL; + struct ppcModel *base_model = NULL; + virCPUDefPtr cpu = NULL; + struct ppcModel *model = NULL; + bool outputModel = true; + + if (!(map = ppcLoadMap())) { + goto error; + } + + if (!(base_model = ppcModelFromCPU(cpus[0], map))) { + goto error; + } + + if (VIR_ALLOC(cpu) < 0 || + !(cpu->arch = strdup(cpus[0]->arch))) + goto no_memory; + cpu->type = VIR_CPU_TYPE_GUEST; + cpu->match = VIR_CPU_MATCH_EXACT; + + if (!cpus[0]->model) { + outputModel = false; + } else if (!(model = ppcModelFind(map, cpus[0]->model))) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Unknown CPU vendor %s"), cpus[0]->model); + goto error; + } + + base_model->data->ppc.pvr = model->data->ppc.pvr; + if (PowerPCDecode(cpu, base_model->data, models, nmodels, NULL) < 0) + goto error; + + if (!outputModel) + VIR_FREE(cpu->model); + + VIR_FREE(cpu->arch); + +cleanup: + ppcModelFree(base_model); + ppcMapFree(map);
- VIR_FREE(data); + return cpu; +no_memory: + virReportOOMError(); +error: + ppcModelFree(model); + virCPUDefFree(cpu); + cpu = NULL; + goto cleanup; }
struct cpuArchDriver cpuDriverPowerPC = { .name = "ppc64", .arch = archs, .narch = ARRAY_CARDINALITY(archs), - .compare = NULL, + .compare = PowerPCCompare, .decode = PowerPCDecode, .encode = NULL, .free = PowerPCDataFree, .nodeData = PowerPCNodeData, .guestData = NULL, - .baseline = NULL, - .update = NULL, + .baseline = PowerPCBaseline, + .update = PowerPCUpdate, .hasFeature = NULL, };
Moreover, you need to add src/cpu/cpu_powerpc.c into po/POTFILES.in
make syntax-check should have warned you about this.
Got it, I will add it in next version. Thanks for your great comments.
Michal
-- Best Regards -Li
There are some descriptions not right in PowerPC CPU model driver. This patch is to fix them. Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- src/cpu/cpu_powerpc.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c index 3a88e68..48b216a 100644 --- a/src/cpu/cpu_powerpc.c +++ b/src/cpu/cpu_powerpc.c @@ -1,7 +1,7 @@ /* - * cpu_powerpc.h: CPU driver for PowerPC CPUs + * cpu_powerpc.c: CPU driver for PowerPC CPUs * - * Copyright (C) Copyright (C) IBM Corporation, 2010 + * Copyright (C) IBM Corporation, 2010 * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public -- 1.7.7.6
On 09.10.2012 09:58, Li Zhang wrote:
There are some descriptions not right in PowerPC CPU model driver. This patch is to fix them.
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- src/cpu/cpu_powerpc.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c index 3a88e68..48b216a 100644 --- a/src/cpu/cpu_powerpc.c +++ b/src/cpu/cpu_powerpc.c @@ -1,7 +1,7 @@ /* - * cpu_powerpc.h: CPU driver for PowerPC CPUs + * cpu_powerpc.c: CPU driver for PowerPC CPUs * - * Copyright (C) Copyright (C) IBM Corporation, 2010 + * Copyright (C) IBM Corporation, 2010 * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public
ACK Michal
Hi, I read x86 CPU model driver and have some understanding. Then do some work on Power as x86. Maybe there some problems. Can someone help review these patches? Thanks in advance. :-) On Tue, Oct 9, 2012 at 3:58 PM, Li Zhang <zhlcindy@gmail.com> wrote:
CPU model driver is to get host's CPU information and it also provides one mechanism to expose host's CPU information to guests during migration.
When migrating one guest from one machine to another machine, it will compare the CPU information. If it is incomptible, it won't start the guest.
On x86, it uses CPUID instruction to get information. When migrating the guest, if the CPU defined in guest XML file is incompatible with host CPU, cpu-baseline will find out best features to expose to guest. Refer to [http://berrange.com/posts/2010/02/15 /guest-cpu-model-configuration-in-libvirt-with-qemukvm/].
On PowerPC, it can get CPU version by mfpvr instruction. So, if PVR is different from definition in guest XML file by cpu-compare, it will fail to start guest. The CPU infromation can be got by cpu-baseline. But only CPU vendor and model can be shown, there are no features as x86. So on PowrePC, it assumes that migration only occurs between the machines with the same type CPUs on PowerPC.
In this driver, there are definitions of models and PVRs for CPUs supported on PowerPC. The relationship between models and PVRs are as the following: For one specific CPU model, its PVR is unique. And also for one PVR code, the PVR code is also unique.
So from the PVR code of CPU data, it's easy to get model and vendor information.
It is tested on my Power machine which CPU PVR is Power7_v2.3. * cap.xml has the same definition as the host. [root@ltckvmopal2 kvm-test]# /bin/virsh cpu-compare cap.xml CPU described in cap.xml is identical to host CPU
[root@ltckvmopal2 kvm-test]# /bin/virsh cpu-baseline cap.xml <cpu mode='custom' match='exact'> <model fallback='allow'>POWER7_v2.3</model> <vendor>IBM</vendor> </cpu>
* cap.xml has differnt model as the host. [root@ltckvmopal2 kvm-test]# /bin/virsh cpu-compare cap.xml CPU described in cap.xml is incompatible with host CPU
[root@ltckvmopal2 kvm-test]# /bin/virsh cpu-baseline cap.xml <cpu mode='custom' match='exact'> <model fallback='allow'>POWER7_v2.1</model> <vendor>IBM</vendor> </cpu>
*v1 -> v2: * Fix coding style's problems in several places. * Seperate doc-fix from the coding
Li Zhang (3): libvirt: Add one file cpu_ppc_data.h to define CPU data for PPC libvirt: Implement CPU model driver for PowerPC Doc-fix for PowerPC CPU model driver
src/cpu/cpu.h | 3 + src/cpu/cpu_map.xml | 14 ++ src/cpu/cpu_powerpc.c | 589 ++++++++++++++++++++++++++++++++++++++++++++++-- src/cpu/cpu_ppc_data.h | 33 +++ 4 files changed, 621 insertions(+), 18 deletions(-) create mode 100644 src/cpu/cpu_ppc_data.h
-- 1.7.7.6
-- Best Regards -Li
participants (2)
-
Li Zhang -
Michal Privoznik