| Message ID | 1466238846-21365-1-git-send-email-bharata@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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); + 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,
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(+)