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

[Bharata B Rao]

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 <address@hidden>
> > ---
> > 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.