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

Re: [libvirt] [PATCH v2 11/33] qemu: Store more types in qemuMonitorCPUModelInfo




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 redhat com>
> ---
> 
> 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'/>
> 


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