On Thu, Mar 14, 2013 at 02:54:21PM +0800, Li Zhang wrote:
> From: Li Zhang <zhlcindy@xxxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Li Zhang <zhlcindy@xxxxxxxxxxxxxxxxxx>
> ---
> 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list