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

Re: [libvirt] [PATCH 1/1][RESEND] ppc64 cpu features



On Thu, Mar 14, 2013 at 02:54:21PM +0800, Li Zhang wrote:
> From: Li Zhang <zhlcindy linux vnet ibm com>
> 
> This patch adds a CPU feature "powernv" identifying IBM Power
> processor that supports native hypervisor e.g. KVM. This can
> be used by virtualization management software to determine
> the CPU capabilities. PVR doesn't indicate whether it a
> host or a guest CPU. So, we use /proc/cpuinfo to get the
> platform information (PowerNV) and use that to set the
> "powernv" flag.
> 
> Signed-off-by: Dipankar Sarma <dipankar in ibm com>
> Signed-off-by: Li Zhang <zhlcindy linux vnet ibm com>
> ---
>  src/cpu/cpu_map.xml    |    9 ++
>  src/cpu/cpu_powerpc.c  |  349 ++++++++++++++++++++++++++++++++++++++----------
>  src/cpu/cpu_ppc_data.h |    4 +
>  src/util/sysinfo.c     |    2 +-
>  4 files changed, 294 insertions(+), 70 deletions(-)
> 
> diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml
> index eb69a34..6b1f9b9 100644
> --- a/src/cpu/cpu_map.xml
> +++ b/src/cpu/cpu_map.xml
> @@ -587,14 +587,23 @@
>    <arch name='ppc64'>
>     <!-- vendor definitions -->
>      <vendor name='IBM' string='PowerPC'/>
> +    <feature name='powernv'> <!-- SYSTEMID_POWERNV -->
> +      <systemid platform='0x00000001'/>
> +    </feature>
>     <!-- IBM-based CPU models -->
>      <model name='POWER7'>
> +      <feature name='powernv'/>
> +      <systemid pvr='0x003f0200'/>
>        <vendor name='IBM'/>
>      </model>
>      <model name='POWER7_v2.1'>
> +      <feature name='powernv'/>
> +      <systemid pvr='0x003f0201'/>
>        <vendor name='IBM'/>
>      </model>
>      <model name='POWER7_v2.3'>
> +      <feature name='powernv'/>
> +      <systemid pvr='0x003f0203'/>
>        <vendor name='IBM'/>
>      </model>
>    </arch>
> diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c
> index ac10789..2135944 100644
> --- a/src/cpu/cpu_powerpc.c
> +++ b/src/cpu/cpu_powerpc.c
> @@ -33,6 +33,7 @@
>  
>  #include "cpu_map.h"
>  #include "buf.h"
> +#include "sysinfo.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_CPU
>  
> @@ -44,16 +45,9 @@ struct cpuPowerPC {
>      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 ppc_vendor {
>      char *name;
> +    uint32_t pvr;
>      struct ppc_vendor *next;
>  };
>  
> @@ -64,64 +58,191 @@ struct ppc_model {
>      struct ppc_model *next;
>  };
>  
> +struct ppc_feature {
> +    char *name;
> +    union cpuData *data;
> +    struct ppc_feature *next;
> +};
> +
>  struct ppc_map {
>      struct ppc_vendor *vendors;
> +    struct ppc_feature *features;
>      struct ppc_model *models;
>  };
>  
> +static void
> +ppcDataSubtract(union cpuData *data1,
> +                const union cpuData *data2)
> +{
> +    data1->ppc.platform &= ~data2->ppc.platform;
> +}
> +
> +static bool
> +ppcDataIsSubset(const union cpuData *data,
> +                const union cpuData *subset)
> +{
> +    if (subset->ppc.platform &&
> +       (data->ppc.platform & subset->ppc.platform) == subset->ppc.platform)
> +        return true;
> +
> +    return false;
> +}
> +
> +static void
> +PowerPCDataFree(union cpuData *data)
> +{
> +    if (data == NULL)
> +        return;
> +    VIR_FREE(data);
> +}
> +
> +
> +static union cpuData *
> +ppcDataCopy(const union cpuData *data)
> +{
> +    union cpuData *copy = NULL;
> +
> +    if (VIR_ALLOC(copy) < 0) {
> +        PowerPCDataFree(copy);
> +        return NULL;
> +    }
> +    copy->ppc = data->ppc;
> +    return copy;
> +}
> +
> +
> +/* also removes all detected features from data */
>  static int
> -ConvertModelVendorFromPVR(char ***model,
> -                          char ***vendor,
> -                          uint32_t pvr)
> +ppcDataToCPUFeatures(virCPUDefPtr cpu,
> +                     int policy,
> +                     union cpuData *data,
> +                     const struct ppc_map *map)
>  {
> -    int i;
> +    const struct ppc_feature *feature = map->features;
>  
> -    for (i = 0; cpu_defs[i].name; i++) {
> -        if (cpu_defs[i].pvr == pvr) {
> -            **model = strdup(cpu_defs[i].name);
> -            **vendor = strdup(cpu_defs[i].vendor);
> -            return 0;
> +    while (feature != NULL) {
> +        if (ppcDataIsSubset(data, feature->data)) {
> +            ppcDataSubtract(data, feature->data);
> +            if (virCPUDefAddFeature(cpu, feature->name, policy) < 0)
> +                return -1;
>          }
> +        feature = feature->next;
>      }
>  
> -    virReportError(VIR_ERR_INTERNAL_ERROR,
> -                   "%s", _("Missing the definition of this model"));
> -    return -1;
> +    return 0;
>  }
>  
> -static int
> -ConvertPVRFromModel(const char *model,
> -                    uint32_t *pvr)
> +static struct ppc_feature *
> +ppcFeatureNew(void)
>  {
> -    int i;
> +    struct ppc_feature *feature;
>  
> -    for (i = 0; cpu_defs[i].name; i++) {
> -        if (STREQ(cpu_defs[i].name, model)) {
> -            *pvr = cpu_defs[i].pvr;
> -            return 0;
> -        }
> +    if (VIR_ALLOC(feature) < 0)
> +        return NULL;
> +
> +    if (VIR_ALLOC(feature->data) < 0) {
> +        VIR_FREE(feature);
> +        return NULL;
> +    }
> +
> +    return feature;
> +}
> +
> +
> +static void
> +ppcFeatureFree(struct ppc_feature *feature)
> +{
> +    if (feature == NULL)
> +        return;
> +
> +    VIR_FREE(feature->name);
> +    PowerPCDataFree(feature->data);
> +    VIR_FREE(feature);
> +}
> +
> +
> +static struct ppc_feature *
> +ppcFeatureFind(const struct ppc_map *map,
> +               const char *name)
> +{
> +    struct ppc_feature *feature;
> +
> +    feature = map->features;
> +    while (feature != NULL) {
> +        if (STREQ(feature->name, name))
> +            return feature;
> +
> +        feature = feature->next;
>      }
>  
> -    virReportError(VIR_ERR_INTERNAL_ERROR,
> -                   "%s", _("Missing the definition of this model"));
> -    return -1;
> +    return NULL;
>  }
>  
>  static int
> -cpuMatch(const union cpuData *data,
> -         char **cpu_model,
> -         char **cpu_vendor,
> -         const struct ppc_model *model)
> +ppcFeatureLoad(xmlXPathContextPtr ctxt,
> +               struct ppc_map *map)
>  {
> +    xmlNodePtr *nodes = NULL;
> +    xmlNodePtr ctxt_node = ctxt->node;
> +    struct ppc_feature *feature;
>      int ret = 0;
> +    unsigned long platform = 0;
> +    unsigned long pvr = 0;
> +    int ret_platform;
> +    int ret_pvr;
> +    int n;
> +
> +    if (!(feature = ppcFeatureNew()))
> +        goto no_memory;
> +
> +    feature->name = virXPathString("string(@name)", ctxt);
> +    if (feature->name == NULL) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("Missing CPU feature name"));
> +        goto ignore;
> +    }
> +
> +    if (ppcFeatureFind(map, feature->name)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("CPU feature %s already defined"), feature->name);
> +        goto ignore;
> +    }
> +
> +    n = virXPathNodeSet("./systemid", ctxt, &nodes);
> +    if (n < 0)
> +        goto ignore;
> +
> +    ctxt->node = nodes[0];
> +    ret_platform = virXPathULongHex("string(@platform)", ctxt, &platform);
> +    ret_pvr = virXPathULongHex("string(@pvr)", ctxt, &pvr);
> +    if (ret_platform < 0 && ret_pvr < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Invalid systemid in %s feature"), feature->name);
> +            goto ignore;
> +    }
> +    feature->data->ppc.platform = platform;
> +    feature->data->ppc.pvr = pvr;
>  
> -    ret = ConvertModelVendorFromPVR(&cpu_model, &cpu_vendor, data->ppc.pvr);
> +    if (map->features == NULL)
> +        map->features = feature;
> +    else {
> +        feature->next = map->features;
> +        map->features = feature;
> +    }
>  
> -    if (STREQ(model->name, *cpu_model) &&
> -        STREQ(model->vendor->name, *cpu_vendor))
> -        ret = 1;
> +out:
> +    ctxt->node = ctxt_node;
> +    VIR_FREE(nodes);
>  
>      return ret;
> +
> +no_memory:
> +    virReportOOMError();
> +    ret = -1;
> +
> +ignore:
> +    ppcFeatureFree(feature);
> +    goto out;
>  }
>  
>  
> @@ -171,6 +292,66 @@ ppcModelFind(const struct ppc_map *map,
>      return NULL;
>  }
>  
> +/* also removes bits corresponding to vendor string from data */
> +static const struct ppc_vendor *
> +ppcDataToVendor(union cpuData *data,
> +                const struct ppc_map *map)
> +{
> +    const struct ppc_vendor *vendor = map->vendors;
> +
> +    while (vendor) {
> +        if (data->ppc.pvr == vendor->pvr)
> +            return vendor;
> +        vendor = vendor->next;
> +    }
> +
> +    return NULL;
> +}
> +
> +
> +static virCPUDefPtr
> +ppcDataToCPU(const union cpuData *data,
> +             const struct ppc_model *model,
> +             const struct ppc_map *map)
> +{
> +    virCPUDefPtr cpu;
> +    union cpuData *copy = NULL;
> +    union cpuData *modelData = NULL;
> +    const struct ppc_vendor *vendor;
> +
> +    if (VIR_ALLOC(cpu) < 0 ||
> +        !(cpu->model = strdup(model->name)) ||
> +        !(copy = ppcDataCopy(data)) ||
> +        !(modelData = ppcDataCopy(model->data)))
> +        goto no_memory;
> +
> +    if ((vendor = ppcDataToVendor(copy, map)) &&
> +        !(cpu->vendor = strdup(vendor->name)))
> +        goto no_memory;
> +
> +    ppcDataSubtract(copy, modelData);
> +    ppcDataSubtract(modelData, data);
> +
> +    /* because feature policy is ignored for host CPU */
> +    cpu->type = VIR_CPU_TYPE_GUEST;
> +
> +    if (ppcDataToCPUFeatures(cpu, VIR_CPU_FEATURE_REQUIRE, copy, map) ||
> +        ppcDataToCPUFeatures(cpu, VIR_CPU_FEATURE_DISABLE, modelData, map))
> +        goto error;
> +
> +cleanup:
> +    PowerPCDataFree(modelData);
> +    PowerPCDataFree(copy);
> +    return cpu;
> +
> +no_memory:
> +    virReportOOMError();
> +error:
> +    virCPUDefFree(cpu);
> +    cpu = NULL;
> +    goto cleanup;
> +}
> +
>  static struct ppc_vendor *
>  ppcVendorFind(const struct ppc_map *map,
>                const char *name)
> @@ -256,6 +437,9 @@ ppcModelLoad(xmlXPathContextPtr ctxt,
>      xmlNodePtr *nodes = NULL;
>      struct ppc_model *model;
>      char *vendor = NULL;
> +    unsigned long pvr = 0, platform = 0;
> +    int ret_platform, ret_pvr;
> +    int n;
>      int ret = -1;
>  
>      if (!(model = ppcModelNew()))
> @@ -268,10 +452,20 @@ ppcModelLoad(xmlXPathContextPtr ctxt,
>          goto ignore;
>      }
>  
> -    ret = ConvertPVRFromModel(model->name, &model->data->ppc.pvr);
> -    if (ret < 0)
> -       goto ignore;
> +    n = virXPathNodeSet("./systemid", ctxt, &nodes);
> +    if (n < 0)
> +        goto ignore;
>  
> +    ctxt->node = nodes[0];
> +    ret_platform = virXPathULongHex("string(@platform)", ctxt, &platform);
> +    ret_pvr = virXPathULongHex("string(@pvr)", ctxt, &pvr);
> +    if (ret_platform < 0 && ret_pvr < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Invalid systemid in %s model"), model->name);
> +            goto ignore;
> +    }
> +    model->data->ppc.platform = platform;
> +    model->data->ppc.pvr = pvr;
>  
>      if (virXPathBoolean("boolean(./vendor)", ctxt)) {
>          vendor = virXPathString("string(./vendor/@name)", ctxt);
> @@ -324,6 +518,8 @@ ppcMapLoadCallback(enum cpuMapElement element,
>          return ppcVendorLoad(ctxt, map);
>      case CPU_MAP_ELEMENT_MODEL:
>          return ppcModelLoad(ctxt, map);
> +    case CPU_MAP_ELEMENT_FEATURE:
> +        return ppcFeatureLoad(ctxt, map);
>      default:
>          break;
>      }
> @@ -385,6 +581,7 @@ ppcModelCopy(const struct ppc_model *model)
>      }
>  
>      copy->data->ppc.pvr = model->data->ppc.pvr;
> +    copy->data->ppc.platform = model->data->ppc.platform;
>      copy->vendor = model->vendor;
>  
>      return copy;
> @@ -437,8 +634,6 @@ PowerPCDecode(virCPUDefPtr cpu,
>      const struct ppc_model *candidate;
>      virCPUDefPtr cpuCandidate;
>      virCPUDefPtr cpuModel = NULL;
> -    char *cpu_vendor = NULL;
> -    char *cpu_model = NULL;
>      unsigned int i;
>  
>      if (data == NULL || (map = ppcLoadMap()) == NULL)
> @@ -475,13 +670,30 @@ PowerPCDecode(virCPUDefPtr cpu,
>              goto next;
>          }
>  
> -        if (VIR_ALLOC(cpuCandidate) < 0) {
> -            virReportOOMError();
> +        if (!(cpuCandidate = ppcDataToCPU(data, candidate, map)))
>              goto out;
> +
> +        if (candidate->vendor && cpuCandidate->vendor &&
> +            STRNEQ(candidate->vendor->name, cpuCandidate->vendor)) {
> +            VIR_DEBUG("CPU vendor %s of model %s differs from %s; ignoring",
> +                      candidate->vendor->name, candidate->name,
> +                      cpuCandidate->vendor);
> +            virCPUDefFree(cpuCandidate);
> +            goto next;
>          }
>  
> -        cpuCandidate->model = strdup(candidate->name);
> -        cpuCandidate->vendor = strdup(candidate->vendor->name);
> +        if (cpu->type == VIR_CPU_TYPE_HOST) {
> +            cpuCandidate->type = VIR_CPU_TYPE_HOST;
> +            for (i = 0; i < cpuCandidate->nfeatures; i++) {
> +                switch (cpuCandidate->features[i].policy) {
> +                case VIR_CPU_FEATURE_DISABLE:
> +                    virCPUDefFree(cpuCandidate);
> +                    goto next;
> +                default:
> +                    cpuCandidate->features[i].policy = -1;
> +                }
> +            }
> +        }
>  
>          if (preferred && STREQ(cpuCandidate->model, preferred)) {
>              virCPUDefFree(cpuModel);
> @@ -489,19 +701,12 @@ PowerPCDecode(virCPUDefPtr cpu,
>              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;
> +        if (cpuModel == NULL
> +            || cpuModel->nfeatures > cpuCandidate->nfeatures) {
>              virCPUDefFree(cpuModel);
>              cpuModel = cpuCandidate;
> -            break;
> -        }
> -
> -        virCPUDefFree(cpuCandidate);
> +        }else
> +            virCPUDefFree(cpuCandidate);
>  
>      next:
>          candidate = candidate->next;
> @@ -515,6 +720,8 @@ PowerPCDecode(virCPUDefPtr cpu,
>  
>      cpu->model = cpuModel->model;
>      cpu->vendor = cpuModel->vendor;
> +    cpu->nfeatures = cpuModel->nfeatures;
> +    cpu->features = cpuModel->features;
>      VIR_FREE(cpuModel);
>  
>      ret = 0;
> @@ -537,19 +744,11 @@ static uint32_t ppc_mfpvr(void)
>  }
>  #endif
>  
> -static void
> -PowerPCDataFree(union cpuData *data)
> -{
> -    if (data == NULL)
> -        return;
> -
> -    VIR_FREE(data);
> -}
> -
>  static union cpuData *
>  PowerPCNodeData(void)
>  {
>      union cpuData *data;
> +    virSysinfoDefPtr hostinfo;
>  
>      if (VIR_ALLOC(data) < 0) {
>          virReportOOMError();
> @@ -561,6 +760,18 @@ PowerPCNodeData(void)
>      data->ppc.pvr = ppc_mfpvr();
>  #endif
>  
> +    hostinfo = virSysinfoRead();
> +    if (hostinfo == NULL) {
> +        VIR_FREE(data);
> +        return NULL;
> +    }
> +
> +    data->ppc.platform &= ~VIR_CPU_PPC64_POWERNV;
> +
> +    if (STREQ(hostinfo->system_family, "PowerNV"))
> +        data->ppc.platform |= VIR_CPU_PPC64_POWERNV;
> +    virSysinfoDefFree(hostinfo);
> +
>      return data;
>  }
>  
> diff --git a/src/cpu/cpu_ppc_data.h b/src/cpu/cpu_ppc_data.h
> index 685332a..0e691ce 100644
> --- a/src/cpu/cpu_ppc_data.h
> +++ b/src/cpu/cpu_ppc_data.h
> @@ -28,6 +28,10 @@
>  
>  struct cpuPPCData {
>      uint32_t pvr;
> +    uint32_t platform; /* Bitflag indicating platform features */
>  };
>  
> +#define VIR_CPU_PPC64_NONE	0x0
> +#define VIR_CPU_PPC64_POWERNV	0x1

I'm not sure what to say about this. Superficially it looks ok, but I'm
not all that familiar with this code in libvirt, nor PPC. So I think I'd
prefer Jiri to look at this at least from the libvirt POV.

> diff --git a/src/util/sysinfo.c b/src/util/sysinfo.c
> index 6a5db80..5f98986 100644
> --- a/src/util/sysinfo.c
> +++ b/src/util/sysinfo.c
> @@ -234,7 +234,7 @@ virSysinfoRead(void) {
>      if (VIR_ALLOC(ret) < 0)
>          goto no_memory;
>  
> -    if (virFileReadAll(CPUINFO, 2048, &outbuf) < 0) {
> +    if (virFileReadAll(CPUINFO, 8192, &outbuf) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Failed to open %s"), CPUINFO);
>          return NULL;

This change seems like it probably ought to be separate.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


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