Re: [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 8470 bytes --]

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,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]