[RFC,v0,1/1] spapr: Support setting of compat CPU type for CPU cores

Compat CPU type is typically specified on -cpu cmdline option like:
-cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
With the introduction of sPAPR CPU core devices, we need to support
the same for core devices too.

Support the specification of CPU compat type on device_add command for
sPAPRCPUCore devices like:
(qemu) device_add POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
Applies on ppc-for-2.7 branch of David Gibson's tree.

 hw/ppc/spapr.c                  |  8 +++++
 hw/ppc/spapr_cpu_core.c         | 73 +++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr_cpu_core.h |  2 ++
 3 files changed, 83 insertions(+)

On Sat, Jun 18, 2016 at 02:04:06PM +0530, Bharata B Rao wrote:
> Compat CPU type is typically specified on -cpu cmdline option like:
> -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> With the introduction of sPAPR CPU core devices, we need to support
> the same for core devices too.
> 
> Support the specification of CPU compat type on device_add command for
> sPAPRCPUCore devices like:
> (qemu) device_add POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
> Applies on ppc-for-2.7 branch of David Gibson's tree.

The implementation looks ok apart from a few nits noted below.

There's a larger problem here, though, in that this doesn't advertise
the necessary compat= property via query-hotpluggable-cpus qmp and hmp
interfaces.  Which means that management has no good way of knowing
it's necessary.

> 
>  hw/ppc/spapr.c                  |  8 +++++
>  hw/ppc/spapr_cpu_core.c         | 73 +++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr_cpu_core.h |  2 ++
>  3 files changed, 83 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 778fa25..2049d7d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState *machine)
>              if (i < spapr_cores) {
>                  char *type = spapr_get_cpu_core_type(machine->cpu_model);
>                  Object *core;
> +                char *compat;
>  
>                  if (!object_class_by_name(type)) {
>                      error_report("Unable to find sPAPR CPU Core definition");
> @@ -1818,6 +1819,13 @@ static void ppc_spapr_init(MachineState *machine)
>                                          &error_fatal);
>                  object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID,
>                                          &error_fatal);
> +                compat = spapr_get_cpu_compat_type(machine->cpu_model);
> +                if (compat) {
> +                    object_property_set_str(core, compat, "compat",
> +                                            &error_fatal);
> +                    g_free(compat);
> +                }
> +
>                  object_property_set_bool(core, true, "realized", &error_fatal);
>              }
>          }
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 3a5da09..9eb63cc 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char *model)
>      return core_type;
>  }
>  
> +/*
> + * Returns the CPU compat type specified in -cpu @model.
> + */
> +char *spapr_get_cpu_compat_type(const char *model)
> +{
> +    char *compat_type = NULL;
> +    gchar **model_pieces = g_strsplit(model, ",", 2);
> +
> +    if (model_pieces[1]) {
> +        gchar **compat_pieces = g_strsplit(model_pieces[1], "=", 2);
> +
> +        compat_type = g_strdup_printf("%s", compat_pieces[1]);
> +    }
> +
> +    g_strfreev(model_pieces);
> +    return compat_type;
> +}
> +
>  static void spapr_core_release(DeviceState *dev, void *opaque)
>  {
>      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> @@ -223,12 +241,31 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      CPUCore *cc = CPU_CORE(dev);
>      char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
>      const char *type = object_get_typename(OBJECT(dev));
> +    char *base_compat_type = NULL;
> +    char *compat = NULL;
> +    bool compat_set;
>  
>      if (strcmp(base_core_type, type)) {
>          error_setg(&local_err, "CPU core type should be %s", base_core_type);
>          goto out;
>      }
>  
> +    base_compat_type = spapr_get_cpu_compat_type(machine->cpu_model);

This can go in the initializer to match the base_core_type.

> +    compat = object_property_get_str(OBJECT(dev), "compat", NULL);
> +    compat_set = (compat && *compat) ? true : false;

You don't need the ?:, the condition is already a boolean.

> +
> +    if (base_compat_type) {
> +        if ((compat_set && strcmp(base_compat_type, compat)) ||
> +            !compat_set) {
> +            error_setg(&local_err, "CPU compat type should be %s",
> +                       base_compat_type);
> +            goto out;
> +        }
> +    } else if (compat_set) {
> +        error_setg(&local_err, "CPU compat type shouldn't be set");
> +        goto out;
> +    }
> +
>      if (!smc->dr_cpu_enabled && dev->hotplugged) {
>          error_setg(&local_err, "CPU hotplug not supported for this machine");
>          goto out;
> @@ -256,6 +293,8 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      }
>  
>  out:
> +    g_free(compat);
> +    g_free(base_compat_type);
>      g_free(base_core_type);
>      error_propagate(errp, local_err);
>  }
> @@ -288,6 +327,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>      Error *local_err = NULL;
>      Object *obj;
>      int i;
> +    char *compat = object_property_get_str(OBJECT(sc), "compat", NULL);
> +    bool compat_set = (compat && *compat) ? true : false;

Again, don't need ?: here.

>  
>      sc->threads = g_malloc0(size * cc->nr_threads);
>      for (i = 0; i < cc->nr_threads; i++) {
> @@ -298,9 +339,19 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>          snprintf(id, sizeof(id), "thread[%d]", i);
>          object_property_add_child(OBJECT(sc), id, obj, &local_err);
>          if (local_err) {
> +            g_free(compat);
>              goto err;
>          }
> +        if (compat_set) {
> +            CPUClass *cc = CPU_GET_CLASS(CPU(obj));
> +            char *featurestr = g_strdup_printf("compat=%s", compat);
> +
> +            cc->parse_features(CPU(obj), featurestr, &local_err);

Hmm.. would it make more sense to just do an object_property_set()
rather than calling into parse_features?

> +            g_free(featurestr);
> +        }
>      }
> +
> +    g_free(compat);
>      object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &local_err);
>      if (local_err) {
>          goto err;
> @@ -318,6 +369,27 @@ err:
>      error_propagate(errp, local_err);
>  }
>  
> +static char *spapr_cpu_core_prop_get_compat(Object *obj, Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +
> +    return g_strdup(core->cpu_compat);
> +}
> +
> +static void spapr_cpu_core_prop_set_compat(Object *obj, const char *val,
> +                                           Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +
> +    core->cpu_compat = g_strdup(val);
> +}
> +
> +static void spapr_cpu_core_instance_init(Object *obj)
> +{
> +    object_property_add_str(obj, "compat", spapr_cpu_core_prop_get_compat,
> +                            spapr_cpu_core_prop_set_compat, NULL);
> +}
> +
>  static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
> @@ -388,6 +460,7 @@ static const TypeInfo spapr_cpu_core_type_info = {
>      .parent = TYPE_CPU_CORE,
>      .abstract = true,
>      .instance_size = sizeof(sPAPRCPUCore),
> +    .instance_init = spapr_cpu_core_instance_init,
>      .class_init = spapr_cpu_core_class_init,
>  };
>  
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index 1c9b319..e697dab 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -24,11 +24,13 @@ typedef struct sPAPRCPUCore {
>      /*< public >*/
>      void *threads;
>      ObjectClass *cpu_class;
> +    char *cpu_compat;
>  } sPAPRCPUCore;
>  
>  void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                           Error **errp);
>  char *spapr_get_cpu_core_type(const char *model);
> +char *spapr_get_cpu_compat_type(const char *model);
>  void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                       Error **errp);
>  void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,

On 18.06.2016 10:34, Bharata B Rao wrote:
> Compat CPU type is typically specified on -cpu cmdline option like:
> -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> With the introduction of sPAPR CPU core devices, we need to support
> the same for core devices too.
> 
> Support the specification of CPU compat type on device_add command for
> sPAPRCPUCore devices like:
> (qemu) device_add POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
>
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
> Applies on ppc-for-2.7 branch of David Gibson's tree.
> 
>  hw/ppc/spapr.c                  |  8 +++++
>  hw/ppc/spapr_cpu_core.c         | 73 +++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr_cpu_core.h |  2 ++
>  3 files changed, 83 insertions(+)
[...]
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 3a5da09..9eb63cc 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char *model)
>      return core_type;
>  }
>  
> +/*
> + * Returns the CPU compat type specified in -cpu @model.
> + */
> +char *spapr_get_cpu_compat_type(const char *model)
> +{
> +    char *compat_type = NULL;
> +    gchar **model_pieces = g_strsplit(model, ",", 2);
> +
> +    if (model_pieces[1]) {
> +        gchar **compat_pieces = g_strsplit(model_pieces[1], "=", 2);
> +
> +        compat_type = g_strdup_printf("%s", compat_pieces[1]);
> +    }
> +
> +    g_strfreev(model_pieces);
> +    return compat_type;
> +}

Shouldn't you check for "compat=" somewhere in this function (and
properly iterate over all parts of the comma separated string)? In case
we support more properties than "compat=" some day, this will go wrong
otherwise...

 Thomas

On Sat, 18 Jun 2016 14:04:06 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> Compat CPU type is typically specified on -cpu cmdline option like:
> -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> With the introduction of sPAPR CPU core devices, we need to support
> the same for core devices too.
> 
> Support the specification of CPU compat type on device_add command for
> sPAPRCPUCore devices like:
> (qemu) device_add
> POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
> Applies on ppc-for-2.7 branch of David Gibson's tree.
> 
>  hw/ppc/spapr.c                  |  8 +++++
>  hw/ppc/spapr_cpu_core.c         | 73
> +++++++++++++++++++++++++++++++++++++++++
> include/hw/ppc/spapr_cpu_core.h |  2 ++ 3 files changed, 83
> insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 778fa25..2049d7d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState
> *machine) if (i < spapr_cores) {
>                  char *type =
> spapr_get_cpu_core_type(machine->cpu_model); Object *core;
> +                char *compat;
>  
>                  if (!object_class_by_name(type)) {
>                      error_report("Unable to find sPAPR CPU Core
> definition"); @@ -1818,6 +1819,13 @@ static void
> ppc_spapr_init(MachineState *machine) &error_fatal);
>                  object_property_set_int(core, core_dt_id,
> CPU_CORE_PROP_CORE_ID, &error_fatal);
> +                compat =
> spapr_get_cpu_compat_type(machine->cpu_model);
> +                if (compat) {
> +                    object_property_set_str(core, compat, "compat",
> +                                            &error_fatal);
> +                    g_free(compat);
> +                }
> +
>                  object_property_set_bool(core, true, "realized",
> &error_fatal); }
>          }
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 3a5da09..9eb63cc 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char *model)
>      return core_type;
>  }
>  
> +/*
> + * Returns the CPU compat type specified in -cpu @model.
> + */
> +char *spapr_get_cpu_compat_type(const char *model)
CPUClass already has such parser and hook to override it in case of
target does legacy parsing, see CPUClass->parse_features.
Maybe extending generic core in similar way and reusing generic parser
will do the job.

However we are in progress [1] of converting legacy
 -cpu cpuname,feat1=x,feat2=y,...
into a set of global properties
 -global cputype.feat1=x ...

it would be better if you wouldn't start using -cpu for features
but use directly -global mechanism,
for that you need only have a corresponding property in spapr_core type.



> +{
> +    char *compat_type = NULL;
> +    gchar **model_pieces = g_strsplit(model, ",", 2);
> +
> +    if (model_pieces[1]) {
> +        gchar **compat_pieces = g_strsplit(model_pieces[1], "=", 2);
> +
> +        compat_type = g_strdup_printf("%s", compat_pieces[1]);
> +    }
> +
> +    g_strfreev(model_pieces);
> +    return compat_type;
> +}
> +
>  static void spapr_core_release(DeviceState *dev, void *opaque)
>  {
>      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> @@ -223,12 +241,31 @@ void spapr_core_pre_plug(HotplugHandler
> *hotplug_dev, DeviceState *dev, CPUCore *cc = CPU_CORE(dev);
>      char *base_core_type =
> spapr_get_cpu_core_type(machine->cpu_model); const char *type =
> object_get_typename(OBJECT(dev));
> +    char *base_compat_type = NULL;
> +    char *compat = NULL;
> +    bool compat_set;
>  
>      if (strcmp(base_core_type, type)) {
>          error_setg(&local_err, "CPU core type should be %s",
> base_core_type); goto out;
>      }
>  
> +    base_compat_type = spapr_get_cpu_compat_type(machine->cpu_model);
> +    compat = object_property_get_str(OBJECT(dev), "compat", NULL);
> +    compat_set = (compat && *compat) ? true : false;
> +
> +    if (base_compat_type) {
> +        if ((compat_set && strcmp(base_compat_type, compat)) ||
> +            !compat_set) {
> +            error_setg(&local_err, "CPU compat type should be %s",
> +                       base_compat_type);
> +            goto out;
> +        }
> +    } else if (compat_set) {
> +        error_setg(&local_err, "CPU compat type shouldn't be set");
> +        goto out;
> +    }
> +
>      if (!smc->dr_cpu_enabled && dev->hotplugged) {
>          error_setg(&local_err, "CPU hotplug not supported for this
> machine"); goto out;
> @@ -256,6 +293,8 @@ void spapr_core_pre_plug(HotplugHandler
> *hotplug_dev, DeviceState *dev, }
>  
>  out:
> +    g_free(compat);
> +    g_free(base_compat_type);
>      g_free(base_core_type);
>      error_propagate(errp, local_err);
>  }
> @@ -288,6 +327,8 @@ static void spapr_cpu_core_realize(DeviceState
> *dev, Error **errp) Error *local_err = NULL;
>      Object *obj;
>      int i;
> +    char *compat = object_property_get_str(OBJECT(sc), "compat",
> NULL);
> +    bool compat_set = (compat && *compat) ? true : false;
>  
>      sc->threads = g_malloc0(size * cc->nr_threads);
>      for (i = 0; i < cc->nr_threads; i++) {
> @@ -298,9 +339,19 @@ static void spapr_cpu_core_realize(DeviceState
> *dev, Error **errp) snprintf(id, sizeof(id), "thread[%d]", i);
>          object_property_add_child(OBJECT(sc), id, obj, &local_err);
>          if (local_err) {
> +            g_free(compat);
>              goto err;
>          }
> +        if (compat_set) {
> +            CPUClass *cc = CPU_GET_CLASS(CPU(obj));
> +            char *featurestr = g_strdup_printf("compat=%s", compat);
> +
> +            cc->parse_features(CPU(obj), featurestr, &local_err);
> +            g_free(featurestr);
> +        }
>      }
> +
> +    g_free(compat);
>      object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child,
> &local_err); if (local_err) {
>          goto err;
> @@ -318,6 +369,27 @@ err:
>      error_propagate(errp, local_err);
>  }
>  
> +static char *spapr_cpu_core_prop_get_compat(Object *obj, Error
> **errp) +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +
> +    return g_strdup(core->cpu_compat);
> +}
> +
> +static void spapr_cpu_core_prop_set_compat(Object *obj, const char
> *val,
> +                                           Error **errp)
> +{
> +    sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> +
> +    core->cpu_compat = g_strdup(val);
> +}
> +
> +static void spapr_cpu_core_instance_init(Object *obj)
> +{
> +    object_property_add_str(obj, "compat",
> spapr_cpu_core_prop_get_compat,
> +                            spapr_cpu_core_prop_set_compat, NULL);
> +}
> +
>  static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
> @@ -388,6 +460,7 @@ static const TypeInfo spapr_cpu_core_type_info = {
>      .parent = TYPE_CPU_CORE,
>      .abstract = true,
>      .instance_size = sizeof(sPAPRCPUCore),
> +    .instance_init = spapr_cpu_core_instance_init,
>      .class_init = spapr_cpu_core_class_init,
>  };
>  
> diff --git a/include/hw/ppc/spapr_cpu_core.h
> b/include/hw/ppc/spapr_cpu_core.h index 1c9b319..e697dab 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -24,11 +24,13 @@ typedef struct sPAPRCPUCore {
>      /*< public >*/
>      void *threads;
>      ObjectClass *cpu_class;
> +    char *cpu_compat;
>  } sPAPRCPUCore;
>  
>  void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState
> *dev, Error **errp);
>  char *spapr_get_cpu_core_type(const char *model);
> +char *spapr_get_cpu_compat_type(const char *model);
>  void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                       Error **errp);
>  void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,

On Tue, 21 Jun 2016 09:09:57 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

[...]
> 
> However we are in progress [1] of converting legacy
1) https://www.mail-archive.com/qemu-devel@nongnu.org/msg378123.html

>  -cpu cpuname,feat1=x,feat2=y,...
> into a set of global properties
>  -global cputype.feat1=x ...
> 
> it would be better if you wouldn't start using -cpu for features
> but use directly -global mechanism,
> for that you need only have a corresponding property in spapr_core
> type.
[...]

On Tue, Jun 21, 2016 at 09:09:57AM +0200, Igor Mammedov wrote:
> On Sat, 18 Jun 2016 14:04:06 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > Compat CPU type is typically specified on -cpu cmdline option like:
> > -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> > With the introduction of sPAPR CPU core devices, we need to support
> > the same for core devices too.
> > 
> > Support the specification of CPU compat type on device_add command for
> > sPAPRCPUCore devices like:
> > (qemu) device_add
> > POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> > Applies on ppc-for-2.7 branch of David Gibson's tree.
> > 
> >  hw/ppc/spapr.c                  |  8 +++++
> >  hw/ppc/spapr_cpu_core.c         | 73
> > +++++++++++++++++++++++++++++++++++++++++
> > include/hw/ppc/spapr_cpu_core.h |  2 ++ 3 files changed, 83
> > insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 778fa25..2049d7d 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState
> > *machine) if (i < spapr_cores) {
> >                  char *type =
> > spapr_get_cpu_core_type(machine->cpu_model); Object *core;
> > +                char *compat;
> >  
> >                  if (!object_class_by_name(type)) {
> >                      error_report("Unable to find sPAPR CPU Core
> > definition"); @@ -1818,6 +1819,13 @@ static void
> > ppc_spapr_init(MachineState *machine) &error_fatal);
> >                  object_property_set_int(core, core_dt_id,
> > CPU_CORE_PROP_CORE_ID, &error_fatal);
> > +                compat =
> > spapr_get_cpu_compat_type(machine->cpu_model);
> > +                if (compat) {
> > +                    object_property_set_str(core, compat, "compat",
> > +                                            &error_fatal);
> > +                    g_free(compat);
> > +                }
> > +
> >                  object_property_set_bool(core, true, "realized",
> > &error_fatal); }
> >          }
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 3a5da09..9eb63cc 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char *model)
> >      return core_type;
> >  }
> >  
> > +/*
> > + * Returns the CPU compat type specified in -cpu @model.
> > + */
> > +char *spapr_get_cpu_compat_type(const char *model)
> CPUClass already has such parser and hook to override it in case of
> target does legacy parsing, see CPUClass->parse_features.
> Maybe extending generic core in similar way and reusing generic parser
> will do the job.

In fact, I am already using CPUClass->parse_features() to set the
feature properties (which right now is only compat= for us) for core device.

What I need in the above routine spapr_get_cpu_compat_type() is to
extract "compat=", so that I can verify that the compat type specified
with -device matches with what was specified with -cpu.

> 
> However we are in progress [1] of converting legacy
>  -cpu cpuname,feat1=x,feat2=y,...
> into a set of global properties
>  -global cputype.feat1=x ...
> 
> it would be better if you wouldn't start using -cpu for features
> but use directly -global mechanism,
> for that you need only have a corresponding property in spapr_core type.

Will this work be part of 2.7 ? If not, we will have to use -cpu to extract
the compat type since powerpc core hotplug is already upstream.

Regards,
Bharata.

On Tue, Jun 21, 2016 at 08:04:12AM +0200, Thomas Huth wrote:
> On 18.06.2016 10:34, Bharata B Rao wrote:
> > Compat CPU type is typically specified on -cpu cmdline option like:
> > -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> > With the introduction of sPAPR CPU core devices, we need to support
> > the same for core devices too.
> > 
> > Support the specification of CPU compat type on device_add command for
> > sPAPRCPUCore devices like:
> > (qemu) device_add POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> >
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> > Applies on ppc-for-2.7 branch of David Gibson's tree.
> > 
> >  hw/ppc/spapr.c                  |  8 +++++
> >  hw/ppc/spapr_cpu_core.c         | 73 +++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr_cpu_core.h |  2 ++
> >  3 files changed, 83 insertions(+)
> [...]
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 3a5da09..9eb63cc 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char *model)
> >      return core_type;
> >  }
> >  
> > +/*
> > + * Returns the CPU compat type specified in -cpu @model.
> > + */
> > +char *spapr_get_cpu_compat_type(const char *model)
> > +{
> > +    char *compat_type = NULL;
> > +    gchar **model_pieces = g_strsplit(model, ",", 2);
> > +
> > +    if (model_pieces[1]) {
> > +        gchar **compat_pieces = g_strsplit(model_pieces[1], "=", 2);
> > +
> > +        compat_type = g_strdup_printf("%s", compat_pieces[1]);
> > +    }
> > +
> > +    g_strfreev(model_pieces);
> > +    return compat_type;
> > +}
> 
> Shouldn't you check for "compat=" somewhere in this function (and
> properly iterate over all parts of the comma separated string)? In case
> we support more properties than "compat=" some day, this will go wrong
> otherwise...

Currently since it is only "compat=", anything else would fail gracefully
when the property gets set. But obviously that is not future-safe as you
note. Let me rework on this.

Regards,
Bharata

On Tue, Jun 21, 2016 at 03:10:00PM +1000, David Gibson wrote:
> On Sat, Jun 18, 2016 at 02:04:06PM +0530, Bharata B Rao wrote:
> > Compat CPU type is typically specified on -cpu cmdline option like:
> > -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> > With the introduction of sPAPR CPU core devices, we need to support
> > the same for core devices too.
> > 
> > Support the specification of CPU compat type on device_add command for
> > sPAPRCPUCore devices like:
> > (qemu) device_add POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> > Applies on ppc-for-2.7 branch of David Gibson's tree.
> 
> The implementation looks ok apart from a few nits noted below.
> 
> There's a larger problem here, though, in that this doesn't advertise
> the necessary compat= property via query-hotpluggable-cpus qmp and hmp
> interfaces.  Which means that management has no good way of knowing
> it's necessary.
> 
> > 
> >  hw/ppc/spapr.c                  |  8 +++++
> >  hw/ppc/spapr_cpu_core.c         | 73 +++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr_cpu_core.h |  2 ++
> >  3 files changed, 83 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 778fa25..2049d7d 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState *machine)
> >              if (i < spapr_cores) {
> >                  char *type = spapr_get_cpu_core_type(machine->cpu_model);
> >                  Object *core;
> > +                char *compat;
> >  
> >                  if (!object_class_by_name(type)) {
> >                      error_report("Unable to find sPAPR CPU Core definition");
> > @@ -1818,6 +1819,13 @@ static void ppc_spapr_init(MachineState *machine)
> >                                          &error_fatal);
> >                  object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID,
> >                                          &error_fatal);
> > +                compat = spapr_get_cpu_compat_type(machine->cpu_model);
> > +                if (compat) {
> > +                    object_property_set_str(core, compat, "compat",
> > +                                            &error_fatal);
> > +                    g_free(compat);
> > +                }
> > +
> >                  object_property_set_bool(core, true, "realized", &error_fatal);
> >              }
> >          }
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 3a5da09..9eb63cc 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char *model)
> >      return core_type;
> >  }
> >  
> > +/*
> > + * Returns the CPU compat type specified in -cpu @model.
> > + */
> > +char *spapr_get_cpu_compat_type(const char *model)
> > +{
> > +    char *compat_type = NULL;
> > +    gchar **model_pieces = g_strsplit(model, ",", 2);
> > +
> > +    if (model_pieces[1]) {
> > +        gchar **compat_pieces = g_strsplit(model_pieces[1], "=", 2);
> > +
> > +        compat_type = g_strdup_printf("%s", compat_pieces[1]);
> > +    }
> > +
> > +    g_strfreev(model_pieces);
> > +    return compat_type;
> > +}
> > +
> >  static void spapr_core_release(DeviceState *dev, void *opaque)
> >  {
> >      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > @@ -223,12 +241,31 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      CPUCore *cc = CPU_CORE(dev);
> >      char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
> >      const char *type = object_get_typename(OBJECT(dev));
> > +    char *base_compat_type = NULL;
> > +    char *compat = NULL;
> > +    bool compat_set;
> >  
> >      if (strcmp(base_core_type, type)) {
> >          error_setg(&local_err, "CPU core type should be %s", base_core_type);
> >          goto out;
> >      }
> >  
> > +    base_compat_type = spapr_get_cpu_compat_type(machine->cpu_model);
> 
> This can go in the initializer to match the base_core_type.

Had it that way, but since there was an error exit possibility when
base_core_type is not matching, I thought better to initialize base_compat_type
after that check.

> 
> > +    compat = object_property_get_str(OBJECT(dev), "compat", NULL);
> > +    compat_set = (compat && *compat) ? true : false;
> 
> You don't need the ?:, the condition is already a boolean.

Yeah.

> 
> > +
> > +    if (base_compat_type) {
> > +        if ((compat_set && strcmp(base_compat_type, compat)) ||
> > +            !compat_set) {
> > +            error_setg(&local_err, "CPU compat type should be %s",
> > +                       base_compat_type);
> > +            goto out;
> > +        }
> > +    } else if (compat_set) {
> > +        error_setg(&local_err, "CPU compat type shouldn't be set");
> > +        goto out;
> > +    }
> > +
> >      if (!smc->dr_cpu_enabled && dev->hotplugged) {
> >          error_setg(&local_err, "CPU hotplug not supported for this machine");
> >          goto out;
> > @@ -256,6 +293,8 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      }
> >  
> >  out:
> > +    g_free(compat);
> > +    g_free(base_compat_type);
> >      g_free(base_core_type);
> >      error_propagate(errp, local_err);
> >  }
> > @@ -288,6 +327,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >      Error *local_err = NULL;
> >      Object *obj;
> >      int i;
> > +    char *compat = object_property_get_str(OBJECT(sc), "compat", NULL);
> > +    bool compat_set = (compat && *compat) ? true : false;
> 
> Again, don't need ?: here.
> 
> >  
> >      sc->threads = g_malloc0(size * cc->nr_threads);
> >      for (i = 0; i < cc->nr_threads; i++) {
> > @@ -298,9 +339,19 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >          snprintf(id, sizeof(id), "thread[%d]", i);
> >          object_property_add_child(OBJECT(sc), id, obj, &local_err);
> >          if (local_err) {
> > +            g_free(compat);
> >              goto err;
> >          }
> > +        if (compat_set) {
> > +            CPUClass *cc = CPU_GET_CLASS(CPU(obj));
> > +            char *featurestr = g_strdup_printf("compat=%s", compat);
> > +
> > +            cc->parse_features(CPU(obj), featurestr, &local_err);
> 
> Hmm.. would it make more sense to just do an object_property_set()
> rather than calling into parse_features?

It would work, but I guess better to use ->parse_features() to ensure
future additional properties would work seamlessly.

Regards,
Bharata.

On Wed, Jun 22, 2016 at 08:06:50AM +0530, Bharata B Rao wrote:
> On Tue, Jun 21, 2016 at 03:10:00PM +1000, David Gibson wrote:
> > On Sat, Jun 18, 2016 at 02:04:06PM +0530, Bharata B Rao wrote:
> > > Compat CPU type is typically specified on -cpu cmdline option like:
> > > -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> > > With the introduction of sPAPR CPU core devices, we need to support
> > > the same for core devices too.
> > > 
> > > Support the specification of CPU compat type on device_add command for
> > > sPAPRCPUCore devices like:
> > > (qemu) device_add POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > > Applies on ppc-for-2.7 branch of David Gibson's tree.
> > 
> > The implementation looks ok apart from a few nits noted below.
> > 
> > There's a larger problem here, though, in that this doesn't advertise
> > the necessary compat= property via query-hotpluggable-cpus qmp and hmp
> > interfaces.  Which means that management has no good way of knowing
> > it's necessary.
> > 
> > > 
> > >  hw/ppc/spapr.c                  |  8 +++++
> > >  hw/ppc/spapr_cpu_core.c         | 73 +++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/ppc/spapr_cpu_core.h |  2 ++
> > >  3 files changed, 83 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 778fa25..2049d7d 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState *machine)
> > >              if (i < spapr_cores) {
> > >                  char *type = spapr_get_cpu_core_type(machine->cpu_model);
> > >                  Object *core;
> > > +                char *compat;
> > >  
> > >                  if (!object_class_by_name(type)) {
> > >                      error_report("Unable to find sPAPR CPU Core definition");
> > > @@ -1818,6 +1819,13 @@ static void ppc_spapr_init(MachineState *machine)
> > >                                          &error_fatal);
> > >                  object_property_set_int(core, core_dt_id, CPU_CORE_PROP_CORE_ID,
> > >                                          &error_fatal);
> > > +                compat = spapr_get_cpu_compat_type(machine->cpu_model);
> > > +                if (compat) {
> > > +                    object_property_set_str(core, compat, "compat",
> > > +                                            &error_fatal);
> > > +                    g_free(compat);
> > > +                }
> > > +
> > >                  object_property_set_bool(core, true, "realized", &error_fatal);
> > >              }
> > >          }
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index 3a5da09..9eb63cc 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char *model)
> > >      return core_type;
> > >  }
> > >  
> > > +/*
> > > + * Returns the CPU compat type specified in -cpu @model.
> > > + */
> > > +char *spapr_get_cpu_compat_type(const char *model)
> > > +{
> > > +    char *compat_type = NULL;
> > > +    gchar **model_pieces = g_strsplit(model, ",", 2);
> > > +
> > > +    if (model_pieces[1]) {
> > > +        gchar **compat_pieces = g_strsplit(model_pieces[1], "=", 2);
> > > +
> > > +        compat_type = g_strdup_printf("%s", compat_pieces[1]);
> > > +    }
> > > +
> > > +    g_strfreev(model_pieces);
> > > +    return compat_type;
> > > +}
> > > +
> > >  static void spapr_core_release(DeviceState *dev, void *opaque)
> > >  {
> > >      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > > @@ -223,12 +241,31 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >      CPUCore *cc = CPU_CORE(dev);
> > >      char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
> > >      const char *type = object_get_typename(OBJECT(dev));
> > > +    char *base_compat_type = NULL;
> > > +    char *compat = NULL;
> > > +    bool compat_set;
> > >  
> > >      if (strcmp(base_core_type, type)) {
> > >          error_setg(&local_err, "CPU core type should be %s", base_core_type);
> > >          goto out;
> > >      }
> > >  
> > > +    base_compat_type = spapr_get_cpu_compat_type(machine->cpu_model);
> > 
> > This can go in the initializer to match the base_core_type.
> 
> Had it that way, but since there was an error exit possibility when
> base_core_type is not matching, I thought better to initialize base_compat_type
> after that check.

Ah, good point.  I forgot that get_cpu_compat_type() had an implicit
allocation that needs cleanup.

> > > @@ -298,9 +339,19 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > >          snprintf(id, sizeof(id), "thread[%d]", i);
> > >          object_property_add_child(OBJECT(sc), id, obj, &local_err);
> > >          if (local_err) {
> > > +            g_free(compat);
> > >              goto err;
> > >          }
> > > +        if (compat_set) {
> > > +            CPUClass *cc = CPU_GET_CLASS(CPU(obj));
> > > +            char *featurestr = g_strdup_printf("compat=%s", compat);
> > > +
> > > +            cc->parse_features(CPU(obj), featurestr, &local_err);
> > 
> > Hmm.. would it make more sense to just do an object_property_set()
> > rather than calling into parse_features?
> 
> It would work, but I guess better to use ->parse_features() to ensure
> future additional properties would work seamlessly.

Hmm.. except that the string you're forming to pass to parse_features
is always just "compat=%s".  So you'd have to change that bit anyway,
so I don't see that using parse_features really buys you anything.

On Wed, 22 Jun 2016 08:00:28 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Tue, Jun 21, 2016 at 09:09:57AM +0200, Igor Mammedov wrote:
> > On Sat, 18 Jun 2016 14:04:06 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > Compat CPU type is typically specified on -cpu cmdline option
> > > like: -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> > > With the introduction of sPAPR CPU core devices, we need to
> > > support the same for core devices too.
> > > 
> > > Support the specification of CPU compat type on device_add
> > > command for sPAPRCPUCore devices like:
> > > (qemu) device_add
> > > POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > > Applies on ppc-for-2.7 branch of David Gibson's tree.
> > > 
> > >  hw/ppc/spapr.c                  |  8 +++++
> > >  hw/ppc/spapr_cpu_core.c         | 73
> > > +++++++++++++++++++++++++++++++++++++++++
> > > include/hw/ppc/spapr_cpu_core.h |  2 ++ 3 files changed, 83
> > > insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 778fa25..2049d7d 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState
> > > *machine) if (i < spapr_cores) {
> > >                  char *type =
> > > spapr_get_cpu_core_type(machine->cpu_model); Object *core;
> > > +                char *compat;
> > >  
> > >                  if (!object_class_by_name(type)) {
> > >                      error_report("Unable to find sPAPR CPU Core
> > > definition"); @@ -1818,6 +1819,13 @@ static void
> > > ppc_spapr_init(MachineState *machine) &error_fatal);
> > >                  object_property_set_int(core, core_dt_id,
> > > CPU_CORE_PROP_CORE_ID, &error_fatal);
> > > +                compat =
> > > spapr_get_cpu_compat_type(machine->cpu_model);
> > > +                if (compat) {
> > > +                    object_property_set_str(core, compat,
> > > "compat",
> > > +                                            &error_fatal);
> > > +                    g_free(compat);
> > > +                }
> > > +
> > >                  object_property_set_bool(core, true, "realized",
> > > &error_fatal); }
> > >          }
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index 3a5da09..9eb63cc 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char
> > > *model) return core_type;
> > >  }
> > >  
> > > +/*
> > > + * Returns the CPU compat type specified in -cpu @model.
> > > + */
> > > +char *spapr_get_cpu_compat_type(const char *model)
> > CPUClass already has such parser and hook to override it in case of
> > target does legacy parsing, see CPUClass->parse_features.
> > Maybe extending generic core in similar way and reusing generic
> > parser will do the job.
> 
> In fact, I am already using CPUClass->parse_features() to set the
> feature properties (which right now is only compat= for us) for core
> device.
> 
> What I need in the above routine spapr_get_cpu_compat_type() is to
> extract "compat=", so that I can verify that the compat type specified
> with -device matches with what was specified with -cpu.
> 
> > 
> > However we are in progress [1] of converting legacy
> >  -cpu cpuname,feat1=x,feat2=y,...
> > into a set of global properties
> >  -global cputype.feat1=x ...
> > 
> > it would be better if you wouldn't start using -cpu for features
> > but use directly -global mechanism,
> > for that you need only have a corresponding property in spapr_core
> > type.
> 
> Will this work be part of 2.7 ? If not, we will have to use -cpu to
> extract the compat type since powerpc core hotplug is already
> upstream.
You can use -global without that work but so far it looks like it will
make into 2.7.

CCed Eduardo so he could say it for sure as it goes via his tree.

> 
> Regards,
> Bharata.
> 
>

On Wed, Jun 22, 2016 at 09:09:46AM +0200, Igor Mammedov wrote:
> On Wed, 22 Jun 2016 08:00:28 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > On Tue, Jun 21, 2016 at 09:09:57AM +0200, Igor Mammedov wrote:
> > > On Sat, 18 Jun 2016 14:04:06 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > 
> > > > Compat CPU type is typically specified on -cpu cmdline option
> > > > like: -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> > > > With the introduction of sPAPR CPU core devices, we need to
> > > > support the same for core devices too.
> > > > 
> > > > Support the specification of CPU compat type on device_add
> > > > command for sPAPRCPUCore devices like:
> > > > (qemu) device_add
> > > > POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> > > > 
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > ---
> > > > Applies on ppc-for-2.7 branch of David Gibson's tree.
> > > > 
> > > >  hw/ppc/spapr.c                  |  8 +++++
> > > >  hw/ppc/spapr_cpu_core.c         | 73
> > > > +++++++++++++++++++++++++++++++++++++++++
> > > > include/hw/ppc/spapr_cpu_core.h |  2 ++ 3 files changed, 83
> > > > insertions(+)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 778fa25..2049d7d 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState
> > > > *machine) if (i < spapr_cores) {
> > > >                  char *type =
> > > > spapr_get_cpu_core_type(machine->cpu_model); Object *core;
> > > > +                char *compat;
> > > >  
> > > >                  if (!object_class_by_name(type)) {
> > > >                      error_report("Unable to find sPAPR CPU Core
> > > > definition"); @@ -1818,6 +1819,13 @@ static void
> > > > ppc_spapr_init(MachineState *machine) &error_fatal);
> > > >                  object_property_set_int(core, core_dt_id,
> > > > CPU_CORE_PROP_CORE_ID, &error_fatal);
> > > > +                compat =
> > > > spapr_get_cpu_compat_type(machine->cpu_model);
> > > > +                if (compat) {
> > > > +                    object_property_set_str(core, compat,
> > > > "compat",
> > > > +                                            &error_fatal);
> > > > +                    g_free(compat);
> > > > +                }
> > > > +
> > > >                  object_property_set_bool(core, true, "realized",
> > > > &error_fatal); }
> > > >          }
> > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > index 3a5da09..9eb63cc 100644
> > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char
> > > > *model) return core_type;
> > > >  }
> > > >  
> > > > +/*
> > > > + * Returns the CPU compat type specified in -cpu @model.
> > > > + */
> > > > +char *spapr_get_cpu_compat_type(const char *model)
> > > CPUClass already has such parser and hook to override it in case of
> > > target does legacy parsing, see CPUClass->parse_features.
> > > Maybe extending generic core in similar way and reusing generic
> > > parser will do the job.
> > 
> > In fact, I am already using CPUClass->parse_features() to set the
> > feature properties (which right now is only compat= for us) for core
> > device.
> > 
> > What I need in the above routine spapr_get_cpu_compat_type() is to
> > extract "compat=", so that I can verify that the compat type specified
> > with -device matches with what was specified with -cpu.
> > 
> > > 
> > > However we are in progress [1] of converting legacy
> > >  -cpu cpuname,feat1=x,feat2=y,...
> > > into a set of global properties
> > >  -global cputype.feat1=x ...
> > > 
> > > it would be better if you wouldn't start using -cpu for features
> > > but use directly -global mechanism,
> > > for that you need only have a corresponding property in spapr_core
> > > type.
> > 
> > Will this work be part of 2.7 ? If not, we will have to use -cpu to
> > extract the compat type since powerpc core hotplug is already
> > upstream.
> You can use -global without that work but so far it looks like it will
> make into 2.7.

I expect the -cpu => globals series to be included soon, unless
there are objections by others.

But as Igor said, globals are already supported. If you want to
set a property in the CPU core objects, you just need to use:

  -global spapr-cpu-core.compat=XXX

The conflict here seems to be that you want the "-cpu" option to
affect the cpu-core objects, not just the VCPU/thread objects.
But we have been assuming that "-cpu" would only affect the
VCPU/thread objects.

What if you just call the cpu_common_parse_features() function
from Igor's series using "spapr-cpu-core" as the typename? This
way all the "-cpu" options would be simply translated to
spapr-cpu-core global properties.

Is "core" the only property in TYPE_POWERPC_CPU, currently?

On Thu, Jun 23, 2016 at 12:55:36PM -0300, Eduardo Habkost wrote:
> On Wed, Jun 22, 2016 at 09:09:46AM +0200, Igor Mammedov wrote:
> > On Wed, 22 Jun 2016 08:00:28 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > On Tue, Jun 21, 2016 at 09:09:57AM +0200, Igor Mammedov wrote:
> > > > On Sat, 18 Jun 2016 14:04:06 +0530
> > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > 
> > > > > Compat CPU type is typically specified on -cpu cmdline option
> > > > > like: -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> > > > > With the introduction of sPAPR CPU core devices, we need to
> > > > > support the same for core devices too.
> > > > > 
> > > > > Support the specification of CPU compat type on device_add
> > > > > command for sPAPRCPUCore devices like:
> > > > > (qemu) device_add
> > > > > POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> > > > > 
> > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > ---
> > > > > Applies on ppc-for-2.7 branch of David Gibson's tree.
> > > > > 
> > > > >  hw/ppc/spapr.c                  |  8 +++++
> > > > >  hw/ppc/spapr_cpu_core.c         | 73
> > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > > include/hw/ppc/spapr_cpu_core.h |  2 ++ 3 files changed, 83
> > > > > insertions(+)
> > > > > 
> > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > index 778fa25..2049d7d 100644
> > > > > --- a/hw/ppc/spapr.c
> > > > > +++ b/hw/ppc/spapr.c
> > > > > @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState
> > > > > *machine) if (i < spapr_cores) {
> > > > >                  char *type =
> > > > > spapr_get_cpu_core_type(machine->cpu_model); Object *core;
> > > > > +                char *compat;
> > > > >  
> > > > >                  if (!object_class_by_name(type)) {
> > > > >                      error_report("Unable to find sPAPR CPU Core
> > > > > definition"); @@ -1818,6 +1819,13 @@ static void
> > > > > ppc_spapr_init(MachineState *machine) &error_fatal);
> > > > >                  object_property_set_int(core, core_dt_id,
> > > > > CPU_CORE_PROP_CORE_ID, &error_fatal);
> > > > > +                compat =
> > > > > spapr_get_cpu_compat_type(machine->cpu_model);
> > > > > +                if (compat) {
> > > > > +                    object_property_set_str(core, compat,
> > > > > "compat",
> > > > > +                                            &error_fatal);
> > > > > +                    g_free(compat);
> > > > > +                }
> > > > > +
> > > > >                  object_property_set_bool(core, true, "realized",
> > > > > &error_fatal); }
> > > > >          }
> > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > index 3a5da09..9eb63cc 100644
> > > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char
> > > > > *model) return core_type;
> > > > >  }
> > > > >  
> > > > > +/*
> > > > > + * Returns the CPU compat type specified in -cpu @model.
> > > > > + */
> > > > > +char *spapr_get_cpu_compat_type(const char *model)
> > > > CPUClass already has such parser and hook to override it in case of
> > > > target does legacy parsing, see CPUClass->parse_features.
> > > > Maybe extending generic core in similar way and reusing generic
> > > > parser will do the job.
> > > 
> > > In fact, I am already using CPUClass->parse_features() to set the
> > > feature properties (which right now is only compat= for us) for core
> > > device.
> > > 
> > > What I need in the above routine spapr_get_cpu_compat_type() is to
> > > extract "compat=", so that I can verify that the compat type specified
> > > with -device matches with what was specified with -cpu.
> > > 
> > > > 
> > > > However we are in progress [1] of converting legacy
> > > >  -cpu cpuname,feat1=x,feat2=y,...
> > > > into a set of global properties
> > > >  -global cputype.feat1=x ...
> > > > 
> > > > it would be better if you wouldn't start using -cpu for features
> > > > but use directly -global mechanism,
> > > > for that you need only have a corresponding property in spapr_core
> > > > type.
> > > 
> > > Will this work be part of 2.7 ? If not, we will have to use -cpu to
> > > extract the compat type since powerpc core hotplug is already
> > > upstream.
> > You can use -global without that work but so far it looks like it will
> > make into 2.7.
> 
> I expect the -cpu => globals series to be included soon, unless
> there are objections by others.
> 
> But as Igor said, globals are already supported. If you want to
> set a property in the CPU core objects, you just need to use:
> 
>   -global spapr-cpu-core.compat=XXX
> 
> The conflict here seems to be that you want the "-cpu" option to
> affect the cpu-core objects, not just the VCPU/thread objects.
> But we have been assuming that "-cpu" would only affect the
> VCPU/thread objects.

-cpu property affects the CPU thread objects in our case too. Since those
CPU thread objects are aggregated under a core object, I was attempting
to add compat= with core device and validate that with -cpu type,compat=
from cmdline.

What we really need is this:

If -global cputype.compat= or -cpu cpuname,compat= is set, we need to
apply the same to all the CPU thread objects of cpu core object. Hence
I am thinking we don't need a separate property for this in the core object.

Regards,
Bharata.

On Fri, Jun 24, 2016 at 07:10:06AM +0530, Bharata B Rao wrote:
> On Thu, Jun 23, 2016 at 12:55:36PM -0300, Eduardo Habkost wrote:
> > On Wed, Jun 22, 2016 at 09:09:46AM +0200, Igor Mammedov wrote:
> > > On Wed, 22 Jun 2016 08:00:28 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > 
> > > > On Tue, Jun 21, 2016 at 09:09:57AM +0200, Igor Mammedov wrote:
> > > > > On Sat, 18 Jun 2016 14:04:06 +0530
> > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > > 
> > > > > > Compat CPU type is typically specified on -cpu cmdline option
> > > > > > like: -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> > > > > > With the introduction of sPAPR CPU core devices, we need to
> > > > > > support the same for core devices too.
> > > > > > 
> > > > > > Support the specification of CPU compat type on device_add
> > > > > > command for sPAPRCPUCore devices like:
> > > > > > (qemu) device_add
> > > > > > POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> > > > > > 
> > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > > ---
> > > > > > Applies on ppc-for-2.7 branch of David Gibson's tree.
> > > > > > 
> > > > > >  hw/ppc/spapr.c                  |  8 +++++
> > > > > >  hw/ppc/spapr_cpu_core.c         | 73
> > > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > > > include/hw/ppc/spapr_cpu_core.h |  2 ++ 3 files changed, 83
> > > > > > insertions(+)
> > > > > > 
> > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > > index 778fa25..2049d7d 100644
> > > > > > --- a/hw/ppc/spapr.c
> > > > > > +++ b/hw/ppc/spapr.c
> > > > > > @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState
> > > > > > *machine) if (i < spapr_cores) {
> > > > > >                  char *type =
> > > > > > spapr_get_cpu_core_type(machine->cpu_model); Object *core;
> > > > > > +                char *compat;
> > > > > >  
> > > > > >                  if (!object_class_by_name(type)) {
> > > > > >                      error_report("Unable to find sPAPR CPU Core
> > > > > > definition"); @@ -1818,6 +1819,13 @@ static void
> > > > > > ppc_spapr_init(MachineState *machine) &error_fatal);
> > > > > >                  object_property_set_int(core, core_dt_id,
> > > > > > CPU_CORE_PROP_CORE_ID, &error_fatal);
> > > > > > +                compat =
> > > > > > spapr_get_cpu_compat_type(machine->cpu_model);
> > > > > > +                if (compat) {
> > > > > > +                    object_property_set_str(core, compat,
> > > > > > "compat",
> > > > > > +                                            &error_fatal);
> > > > > > +                    g_free(compat);
> > > > > > +                }
> > > > > > +
> > > > > >                  object_property_set_bool(core, true, "realized",
> > > > > > &error_fatal); }
> > > > > >          }
> > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > > index 3a5da09..9eb63cc 100644
> > > > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char
> > > > > > *model) return core_type;
> > > > > >  }
> > > > > >  
> > > > > > +/*
> > > > > > + * Returns the CPU compat type specified in -cpu @model.
> > > > > > + */
> > > > > > +char *spapr_get_cpu_compat_type(const char *model)
> > > > > CPUClass already has such parser and hook to override it in case of
> > > > > target does legacy parsing, see CPUClass->parse_features.
> > > > > Maybe extending generic core in similar way and reusing generic
> > > > > parser will do the job.
> > > > 
> > > > In fact, I am already using CPUClass->parse_features() to set the
> > > > feature properties (which right now is only compat= for us) for core
> > > > device.
> > > > 
> > > > What I need in the above routine spapr_get_cpu_compat_type() is to
> > > > extract "compat=", so that I can verify that the compat type specified
> > > > with -device matches with what was specified with -cpu.
> > > > 
> > > > > 
> > > > > However we are in progress [1] of converting legacy
> > > > >  -cpu cpuname,feat1=x,feat2=y,...
> > > > > into a set of global properties
> > > > >  -global cputype.feat1=x ...
> > > > > 
> > > > > it would be better if you wouldn't start using -cpu for features
> > > > > but use directly -global mechanism,
> > > > > for that you need only have a corresponding property in spapr_core
> > > > > type.
> > > > 
> > > > Will this work be part of 2.7 ? If not, we will have to use -cpu to
> > > > extract the compat type since powerpc core hotplug is already
> > > > upstream.
> > > You can use -global without that work but so far it looks like it will
> > > make into 2.7.
> > 
> > I expect the -cpu => globals series to be included soon, unless
> > there are objections by others.
> > 
> > But as Igor said, globals are already supported. If you want to
> > set a property in the CPU core objects, you just need to use:
> > 
> >   -global spapr-cpu-core.compat=XXX
> > 
> > The conflict here seems to be that you want the "-cpu" option to
> > affect the cpu-core objects, not just the VCPU/thread objects.
> > But we have been assuming that "-cpu" would only affect the
> > VCPU/thread objects.
> 
> -cpu property affects the CPU thread objects in our case too. Since those
> CPU thread objects are aggregated under a core object, I was attempting
> to add compat= with core device and validate that with -cpu type,compat=
> from cmdline.
> 
> What we really need is this:
> 
> If -global cputype.compat= or -cpu cpuname,compat= is set, we need to
> apply the same to all the CPU thread objects of cpu core object. Hence
> I am thinking we don't need a separate property for this in the core object.

Right, I was just coming to the same conclusion.  Since we always need
to set the compat property uniformly, I don't think we actually need
to bounce this information via the core object.  Instead, the user can
either use -global or -cpu to set it directly on all thread objects.

In the unlikely event we ever have a setup where we need different
cores in different compat modes we can add a property to the core
object then.

On Fri, Jun 24, 2016 at 07:10:06AM +0530, Bharata B Rao wrote:
> On Thu, Jun 23, 2016 at 12:55:36PM -0300, Eduardo Habkost wrote:
> > On Wed, Jun 22, 2016 at 09:09:46AM +0200, Igor Mammedov wrote:
> > > On Wed, 22 Jun 2016 08:00:28 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > 
> > > > On Tue, Jun 21, 2016 at 09:09:57AM +0200, Igor Mammedov wrote:
> > > > > On Sat, 18 Jun 2016 14:04:06 +0530
> > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > > > 
> > > > > > Compat CPU type is typically specified on -cpu cmdline option
> > > > > > like: -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> > > > > > With the introduction of sPAPR CPU core devices, we need to
> > > > > > support the same for core devices too.
> > > > > > 
> > > > > > Support the specification of CPU compat type on device_add
> > > > > > command for sPAPRCPUCore devices like:
> > > > > > (qemu) device_add
> > > > > > POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> > > > > > 
> > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > > > ---
> > > > > > Applies on ppc-for-2.7 branch of David Gibson's tree.
> > > > > > 
> > > > > >  hw/ppc/spapr.c                  |  8 +++++
> > > > > >  hw/ppc/spapr_cpu_core.c         | 73
> > > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > > > include/hw/ppc/spapr_cpu_core.h |  2 ++ 3 files changed, 83
> > > > > > insertions(+)
> > > > > > 
> > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > > index 778fa25..2049d7d 100644
> > > > > > --- a/hw/ppc/spapr.c
> > > > > > +++ b/hw/ppc/spapr.c
> > > > > > @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState
> > > > > > *machine) if (i < spapr_cores) {
> > > > > >                  char *type =
> > > > > > spapr_get_cpu_core_type(machine->cpu_model); Object *core;
> > > > > > +                char *compat;
> > > > > >  
> > > > > >                  if (!object_class_by_name(type)) {
> > > > > >                      error_report("Unable to find sPAPR CPU Core
> > > > > > definition"); @@ -1818,6 +1819,13 @@ static void
> > > > > > ppc_spapr_init(MachineState *machine) &error_fatal);
> > > > > >                  object_property_set_int(core, core_dt_id,
> > > > > > CPU_CORE_PROP_CORE_ID, &error_fatal);
> > > > > > +                compat =
> > > > > > spapr_get_cpu_compat_type(machine->cpu_model);
> > > > > > +                if (compat) {
> > > > > > +                    object_property_set_str(core, compat,
> > > > > > "compat",
> > > > > > +                                            &error_fatal);
> > > > > > +                    g_free(compat);
> > > > > > +                }
> > > > > > +
> > > > > >                  object_property_set_bool(core, true, "realized",
> > > > > > &error_fatal); }
> > > > > >          }
> > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > > index 3a5da09..9eb63cc 100644
> > > > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char
> > > > > > *model) return core_type;
> > > > > >  }
> > > > > >  
> > > > > > +/*
> > > > > > + * Returns the CPU compat type specified in -cpu @model.
> > > > > > + */
> > > > > > +char *spapr_get_cpu_compat_type(const char *model)
> > > > > CPUClass already has such parser and hook to override it in case of
> > > > > target does legacy parsing, see CPUClass->parse_features.
> > > > > Maybe extending generic core in similar way and reusing generic
> > > > > parser will do the job.
> > > > 
> > > > In fact, I am already using CPUClass->parse_features() to set the
> > > > feature properties (which right now is only compat= for us) for core
> > > > device.
> > > > 
> > > > What I need in the above routine spapr_get_cpu_compat_type() is to
> > > > extract "compat=", so that I can verify that the compat type specified
> > > > with -device matches with what was specified with -cpu.
> > > > 
> > > > > 
> > > > > However we are in progress [1] of converting legacy
> > > > >  -cpu cpuname,feat1=x,feat2=y,...
> > > > > into a set of global properties
> > > > >  -global cputype.feat1=x ...
> > > > > 
> > > > > it would be better if you wouldn't start using -cpu for features
> > > > > but use directly -global mechanism,
> > > > > for that you need only have a corresponding property in spapr_core
> > > > > type.
> > > > 
> > > > Will this work be part of 2.7 ? If not, we will have to use -cpu to
> > > > extract the compat type since powerpc core hotplug is already
> > > > upstream.
> > > You can use -global without that work but so far it looks like it will
> > > make into 2.7.
> > 
> > I expect the -cpu => globals series to be included soon, unless
> > there are objections by others.
> > 
> > But as Igor said, globals are already supported. If you want to
> > set a property in the CPU core objects, you just need to use:
> > 
> >   -global spapr-cpu-core.compat=XXX
> > 
> > The conflict here seems to be that you want the "-cpu" option to
> > affect the cpu-core objects, not just the VCPU/thread objects.
> > But we have been assuming that "-cpu" would only affect the
> > VCPU/thread objects.
> 
> -cpu property affects the CPU thread objects in our case too. Since those
> CPU thread objects are aggregated under a core object, I was attempting
> to add compat= with core device and validate that with -cpu type,compat=
> from cmdline.
> 
> What we really need is this:
> 
> If -global cputype.compat= or -cpu cpuname,compat= is set, we need to
> apply the same to all the CPU thread objects of cpu core object. Hence
> I am thinking we don't need a separate property for this in the core object.

I assumed you really needed a spapr-cpu-core property for some
other reason. If all you need is to affect the VCPU thread
objects, then you only need cpu_common_parse_features() (the
default implementation of CPUClass::parse_features()).

Note that Igor's series is changing the
CPUClass::parse_features() interface. The current version needs
to be called once for each CPU, the version by Igor will call it
only once, before CPU creation. I suggest using his series as
base, so you don't need to redo your work later.

If everything goes well, I plan to submit a pull request
including his series on Monday.