[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 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


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