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@xxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
> * Prerna Saxena <prerna@xxxxxxxxxxxxxxxxxx>
> + * Li Zhang <zhlcindy@xxxxxxxxxxxxxxxxxx>
> */
>
> #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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list