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 <[email protected]> > > > --- > > > 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.
--
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
signature.asc
Description: PGP signature
