Re: [libvirt] [PATCH v1 2/3] libvirt: Implement CPU model driver for PowerPC

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(a)linux.vnet.ibm.com&gt; > --- > 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(a)au.ibm.com&gt; > * Prerna Saxena <prerna(a)linux.vnet.ibm.com&gt; > + * Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; > */ > > #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