On 02/15/2017 11:44 AM, Jiri Denemark wrote:
> While query-cpu-model-expansion returns only boolean features on s390,
> but x86_64 reports some integer and string properties which we are
> interested in.
>
> Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> ---
>
> Notes:
> Version 2:
> - no change
>
> src/qemu/qemu_capabilities.c | 84 ++++++++++++++++--------
> src/qemu/qemu_monitor.c | 22 ++++++-
> src/qemu/qemu_monitor.h | 23 +++++--
> src/qemu/qemu_monitor_json.c | 37 ++++++++---
> tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 7 ++
> 5 files changed, 133 insertions(+), 40 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index aab336954..466852d13 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -3074,14 +3074,16 @@ virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps,
> cpu->nfeatures = 0;
>
> for (i = 0; i < modelInfo->nprops; i++) {
> - if (VIR_STRDUP(cpu->features[i].name, modelInfo->props[i].name) < 0)
> + virCPUFeatureDefPtr feature = cpu->features + cpu->nfeatures;
> + qemuMonitorCPUPropertyPtr prop = modelInfo->props + i;
> +
> + if (prop->type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN)
> + continue;
So s390 only supports "boolean" or "default" types?
> +
> + if (VIR_STRDUP(feature->name, prop->name) < 0)
> return -1;
> -
> - if (modelInfo->props[i].supported)
> - cpu->features[i].policy = VIR_CPU_FEATURE_REQUIRE;
> - else
> - cpu->features[i].policy = VIR_CPU_FEATURE_DISABLE;
> -
> + feature->policy = prop->value.boolean ? VIR_CPU_FEATURE_REQUIRE
> + : VIR_CPU_FEATURE_DISABLE;
> cpu->nfeatures++;
> }
>
> @@ -3154,7 +3156,6 @@ static int
> virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
> xmlXPathContextPtr ctxt)
> {
> - char *str = NULL;
> xmlNodePtr hostCPUNode;
> xmlNodePtr *featureNodes = NULL;
> xmlNodePtr oldnode = ctxt->node;
> @@ -3187,30 +3188,47 @@ virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
> hostCPU->nprops = n;
>
> for (i = 0; i < n; i++) {
> - hostCPU->props[i].name = virXMLPropString(featureNodes[i], "name");
> - if (!hostCPU->props[i].name) {
> + qemuMonitorCPUPropertyPtr prop = hostCPU->props + i;
> + ctxt->node = featureNodes[i];
> +
> + if (!(prop->name = virXMLPropString(ctxt->node, "name"))) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("missing 'name' attribute for a host CPU"
> " model property in QEMU capabilities cache"));
> goto cleanup;
> }
If you follow the suggestion I have in the previous patch, then if you
don't find the property named "type", then you know the 'value' is
either "yes" or "no"
If there is a type then the "value" property is either "string" or "number"
>
> - if (!(str = virXMLPropString(featureNodes[i], "boolean"))) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("missing 'boolean' attribute for a host CPU"
> - " model property in QEMU capabilities cache"));
> - goto cleanup;
> - }
> - if (STREQ(str, "yes")) {
> - hostCPU->props[i].supported = true;
> - } else if (STREQ(str, "no")) {
> - hostCPU->props[i].supported = false;
> + if (virXPathBoolean("boolean(./@boolean)", ctxt)) {
> + if (virXPathBoolean("./@boolean='yes'", ctxt))
> + prop->value.boolean = true;
> + prop->type = QEMU_MONITOR_CPU_PROPERTY_BOOLEAN;
> + } else if (virXPathBoolean("boolean(./@string)", ctxt)) {
> + prop->value.string = virXMLPropString(ctxt->node, "string");
> + if (!prop->value.string) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("invalid string value for '%s' host CPU "
> + "model property in QEMU capabilities cache"),
> + prop->name);
> + goto cleanup;
> + }
> + prop->type = QEMU_MONITOR_CPU_PROPERTY_STRING;
> + } else if (virXPathBoolean("boolean(./@ull)", ctxt)) {
ull is just an implementation detail. I think it should be number
> + if (virXPathULongLong("string(./@ull)", ctxt,
> + &prop->value.ull) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("invalid integer value for '%s' host CPU "
in this context 'integer' isn't necessarily correct since the desire is
an unsigned long long
> + "model property in QEMU capabilities cache"),
> + prop->name);
> + goto cleanup;
> + }
> + prop->type = QEMU_MONITOR_CPU_PROPERTY_ULL;
> } else {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("invalid boolean value: '%s'"), str);
> + _("missing value for '%s' host CPU model "
> + "property in QEMU capabilities cache"),
> + prop->name);
One would think that if you use 'type=%s', then this would be simplified
to be if the "value" property doesn't exist, then you have a failure.
What that value property eventually gets "read" as matters only for what
the 'type' is (or the default of boolean)
> goto cleanup;
> }
> - VIR_FREE(str);
> }
> }
>
> @@ -3220,7 +3238,6 @@ virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
>
> cleanup:
> ctxt->node = oldnode;
> - VIR_FREE(str);
> VIR_FREE(featureNodes);
> qemuMonitorCPUModelInfoFree(hostCPU);
> return ret;
> @@ -3552,9 +3569,24 @@ virQEMUCapsFormatHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
> virBufferAdjustIndent(buf, 2);
>
> for (i = 0; i < model->nprops; i++) {
> - virBufferAsprintf(buf, "<property name='%s' boolean='%s'/>\n",
> - model->props[i].name,
> - model->props[i].supported ? "yes" : "no");
> + qemuMonitorCPUPropertyPtr prop = model->props + i;
> +
> + switch (prop->type) {
> + case QEMU_MONITOR_CPU_PROPERTY_BOOLEAN:
> + virBufferAsprintf(buf, "<property name='%s' boolean='%s'/>\n",
> + prop->name, prop->value.boolean ? "yes" : "no");
> + break;
> +
> + case QEMU_MONITOR_CPU_PROPERTY_STRING:
> + virBufferAsprintf(buf, "<property name='%s' ", prop->name);
> + virBufferEscapeString(buf, "string='%s'/>\n", prop->value.string);
> + break;
> +
> + case QEMU_MONITOR_CPU_PROPERTY_ULL:
> + virBufferAsprintf(buf, "<property name='%s' ull='%llu'/>\n",
> + prop->name, prop->value.ull);
> + break;
> + }
obviously easy adjustments here especially if there's a VIR_ENUM_IMPL
for qemuMonitorCPUPropertyType that only writes out the "type" if
'string' or 'number'
> }
>
> virBufferAdjustIndent(buf, -2);
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index b15207a69..a434b234b 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3661,8 +3661,11 @@ qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info)
> if (!model_info)
> return;
>
> - for (i = 0; i < model_info->nprops; i++)
> + for (i = 0; i < model_info->nprops; i++) {
> VIR_FREE(model_info->props[i].name);
> + if (model_info->props[i].type == QEMU_MONITOR_CPU_PROPERTY_STRING)
> + VIR_FREE(model_info->props[i].value.string);
> + }
>
> VIR_FREE(model_info->props);
> VIR_FREE(model_info->name);
> @@ -3691,7 +3694,22 @@ qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig)
> if (VIR_STRDUP(copy->props[i].name, orig->props[i].name) < 0)
> goto error;
>
> - copy->props[i].supported = orig->props[i].supported;
> + copy->props[i].type = orig->props[i].type;
> + switch (orig->props[i].type) {
> + case QEMU_MONITOR_CPU_PROPERTY_BOOLEAN:
> + copy->props[i].value.boolean = orig->props[i].value.boolean;
> + break;
> +
> + case QEMU_MONITOR_CPU_PROPERTY_STRING:
> + if (VIR_STRDUP(copy->props[i].value.string,
> + orig->props[i].value.string) < 0)
> + goto error;
> + break;
> +
> + case QEMU_MONITOR_CPU_PROPERTY_ULL:
> + copy->props[i].value.ull = orig->props[i].value.ull;
> + break;
> + }
> }
>
> return copy;
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 8811d8501..112f041f1 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -921,16 +921,31 @@ int qemuMonitorGetCPUDefinitions(qemuMonitorPtr mon,
> qemuMonitorCPUDefInfoPtr **cpus);
> void qemuMonitorCPUDefInfoFree(qemuMonitorCPUDefInfoPtr cpu);
>
> +typedef enum {
> + QEMU_MONITOR_CPU_PROPERTY_BOOLEAN,
As stated in previous patch, I think should should be "DEFAULT"
> + QEMU_MONITOR_CPU_PROPERTY_STRING,
> + QEMU_MONITOR_CPU_PROPERTY_ULL,
likewise, "NUMBER" not "ULL"
John
> +} qemuMonitorCPUPropertyType;
> +
> +typedef struct _qemuMonitorCPUProperty qemuMonitorCPUProperty;
> +typedef qemuMonitorCPUProperty *qemuMonitorCPUPropertyPtr;
> +struct _qemuMonitorCPUProperty {
> + char *name;
> + qemuMonitorCPUPropertyType type;
> + union {
> + bool boolean;
> + char *string;
> + unsigned long long ull;
> + } value;
> +};
> +
> typedef struct _qemuMonitorCPUModelInfo qemuMonitorCPUModelInfo;
> typedef qemuMonitorCPUModelInfo *qemuMonitorCPUModelInfoPtr;
>
> struct _qemuMonitorCPUModelInfo {
> char *name;
> size_t nprops;
> - struct {
> - char *name;
> - bool supported;
> - } *props;
> + qemuMonitorCPUPropertyPtr props;
> };
>
> int qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon,
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 1d281af48..415761525 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -4983,18 +4983,39 @@ qemuMonitorJSONParseCPUModelProperty(const char *key,
> void *opaque)
> {
> qemuMonitorCPUModelInfoPtr machine_model = opaque;
> - size_t n = machine_model->nprops;
> - bool supported;
> + qemuMonitorCPUPropertyPtr prop;
>
> - if (virJSONValueGetBoolean(value, &supported) < 0)
> + prop = machine_model->props + machine_model->nprops;
> +
> + switch ((virJSONType) value->type) {
> + case VIR_JSON_TYPE_STRING:
> + if (VIR_STRDUP(prop->value.string, virJSONValueGetString(value)) < 0)
> + return -1;
> + prop->type = QEMU_MONITOR_CPU_PROPERTY_STRING;
> + break;
> +
> + case VIR_JSON_TYPE_NUMBER:
> + /* Ignore numbers which cannot be parsed as unsigned long long */
> + if (virJSONValueGetNumberUlong(value, &prop->value.ull) < 0)
> + return 0;
> + prop->type = QEMU_MONITOR_CPU_PROPERTY_ULL;
> + break;
> +
> + case VIR_JSON_TYPE_BOOLEAN:
> + virJSONValueGetBoolean(value, &prop->value.boolean);
> + prop->type = QEMU_MONITOR_CPU_PROPERTY_BOOLEAN;
> + break;
> +
> + case VIR_JSON_TYPE_OBJECT:
> + case VIR_JSON_TYPE_ARRAY:
> + case VIR_JSON_TYPE_NULL:
> return 0;
> -
> - if (VIR_STRDUP(machine_model->props[n].name, key) < 0)
> - return -1;
> -
> - machine_model->props[n].supported = supported;
> + }
>
> machine_model->nprops++;
> + if (VIR_STRDUP(prop->name, key) < 0)
> + return -1;
> +
> return 0;
> }
>
> diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
> index c13e8318f..32368e648 100644
> --- a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
> @@ -222,11 +222,13 @@
> <property name='avx512cd' boolean='no'/>
> <property name='decodeassists' boolean='no'/>
> <property name='sse4.1' boolean='yes'/>
> + <property name='family' ull='6'/>
> <property name='avx512f' boolean='no'/>
> <property name='msr' boolean='yes'/>
> <property name='mce' boolean='yes'/>
> <property name='mca' boolean='yes'/>
> <property name='xcrypt' boolean='no'/>
> + <property name='min-level' ull='13'/>
> <property name='xgetbv1' boolean='yes'/>
> <property name='cid' boolean='no'/>
> <property name='ds' boolean='no'/>
> @@ -242,6 +244,7 @@
> <property name='xcrypt-en' boolean='no'/>
> <property name='pn' boolean='no'/>
> <property name='dca' boolean='no'/>
> + <property name='vendor' string='GenuineIntel'/>
> <property name='pku' boolean='no'/>
> <property name='smx' boolean='no'/>
> <property name='cmp-legacy' boolean='no'/>
> @@ -287,6 +290,7 @@
> <property name='sse4.2' boolean='yes'/>
> <property name='pge' boolean='yes'/>
> <property name='pdcm' boolean='no'/>
> + <property name='model' ull='94'/>
> <property name='movbe' boolean='yes'/>
> <property name='nrip-save' boolean='no'/>
> <property name='ssse3' boolean='yes'/>
> @@ -297,6 +301,7 @@
> <property name='fma' boolean='yes'/>
> <property name='cx16' boolean='yes'/>
> <property name='de' boolean='yes'/>
> + <property name='stepping' ull='3'/>
> <property name='xsave' boolean='yes'/>
> <property name='clflush' boolean='yes'/>
> <property name='skinit' boolean='no'/>
> @@ -334,6 +339,7 @@
> <property name='sep' boolean='yes'/>
> <property name='nodeid-msr' boolean='no'/>
> <property name='misalignsse' boolean='no'/>
> + <property name='min-xlevel' ull='2147483656'/>
> <property name='bmi1' boolean='yes'/>
> <property name='bmi2' boolean='yes'/>
> <property name='kvm-pv-unhalt' boolean='yes'/>
> @@ -363,6 +369,7 @@
> <property name='pse36' boolean='yes'/>
> <property name='tbm' boolean='no'/>
> <property name='wdt' boolean='no'/>
> + <property name='model-id' string='Intel(R) Xeon(R) CPU E3-1245 v5 @ 3.50GHz'/>
> <property name='sha-ni' boolean='no'/>
> <property name='abm' boolean='yes'/>
> <property name='avx512pf' boolean='no'/>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
