[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]