On Thu, Oct 11, 2012 at 11:12 PM, Michal Privoznik <mprivozn@xxxxxxxxxx> 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@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.
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list