| Message ID | 1466580229-4600-1-git-send-email-bharata@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jun 22, 2016 at 12:53:49PM +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> > --- > Changes in v1: > - In the routine that extracts "compat=" from -cpu cmdline, made the parsing > generic as suggested by Thomas Huth so that it works in the presence of > any other additional features. > - Addressed review comments by David with major one being setting of > compat property directly instead of going via ->parse_features(). > > TODO: > - Reconcile with Igor's work that make cpu features as global properties. > - Find a way to export the compat infomation via query-hotpluggable-cpus. > > v0: https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg05279.html > > hw/ppc/spapr.c | 8 +++++ > hw/ppc/spapr_cpu_core.c | 78 +++++++++++++++++++++++++++++++++++++++++ > include/hw/ppc/spapr_cpu_core.h | 2 ++ > 3 files changed, 88 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..d500cd6 100644 > --- a/hw/ppc/spapr_cpu_core.c > +++ b/hw/ppc/spapr_cpu_core.c > @@ -96,6 +96,31 @@ 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 *model_str = g_strdup(model); > + char *featurestr, *compat = NULL; > + > + featurestr = model_str ? strtok(model_str, ",") : NULL; AFAICT strtok() is not thread-safe. > + while (featurestr) { > + char *val; > + if (!strncmp(featurestr, "compat=", 7)) { > + val = strchr(featurestr, '='); You don't technically need the strchr(), since from the strncmp above, you know the answer will be featurestr + 6. > + val++; > + compat = g_strdup(val); > + goto out; > + } > + featurestr = strtok(NULL, ","); > + } > + > +out: > + g_free(model_str); > + return compat; > +} Couldn't we use one of the existing opts parsing functions in qemu for the above, anyway? > static void spapr_core_release(DeviceState *dev, void *opaque) > { > sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev)); > @@ -223,12 +248,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; > + > + 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; I don't think we want this else clause, because it will forbid the use of -global core_type,compat=whatever, which Igor says is the preferred approach for the future (IIUC, using -global is equivalent to setting the property explicitly on every instance). The if clause should be ok, since it implements a fallback to using -cpu, which we do want. > + } > + > if (!smc->dr_cpu_enabled && dev->hotplugged) { > error_setg(&local_err, "CPU hotplug not supported for this machine"); > goto out; > @@ -256,6 +300,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 +334,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; > > sc->threads = g_malloc0(size * cc->nr_threads); > for (i = 0; i < cc->nr_threads; i++) { > @@ -300,11 +348,18 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) > if (local_err) { > goto err; > } > + if (compat_set) { > + object_property_set_str(obj, compat, "compat", &local_err); > + if (local_err) { > + goto err; > + } > + } > } > object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &local_err); > if (local_err) { > goto err; > } else { > + g_free(compat); > return; > } > > @@ -315,9 +370,31 @@ err: > i--; > } > g_free(sc->threads); > + g_free(compat); > 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 +465,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 Thu, Jun 23, 2016 at 04:05:14PM +1000, David Gibson wrote: > On Wed, Jun 22, 2016 at 12:53:49PM +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> > > --- > > Changes in v1: > > - In the routine that extracts "compat=" from -cpu cmdline, made the parsing > > generic as suggested by Thomas Huth so that it works in the presence of > > any other additional features. > > - Addressed review comments by David with major one being setting of > > compat property directly instead of going via ->parse_features(). > > > > TODO: > > - Reconcile with Igor's work that make cpu features as global properties. > > - Find a way to export the compat infomation via query-hotpluggable-cpus. > > > > v0: https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg05279.html > > > > hw/ppc/spapr.c | 8 +++++ > > hw/ppc/spapr_cpu_core.c | 78 +++++++++++++++++++++++++++++++++++++++++ > > include/hw/ppc/spapr_cpu_core.h | 2 ++ > > 3 files changed, 88 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..d500cd6 100644 > > --- a/hw/ppc/spapr_cpu_core.c > > +++ b/hw/ppc/spapr_cpu_core.c > > @@ -96,6 +96,31 @@ 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 *model_str = g_strdup(model); > > + char *featurestr, *compat = NULL; > > + > > + featurestr = model_str ? strtok(model_str, ",") : NULL; > > AFAICT strtok() is not thread-safe. Followed cpu_common_parse_features(). > > > + while (featurestr) { > > + char *val; > > + if (!strncmp(featurestr, "compat=", 7)) { > > + val = strchr(featurestr, '='); > > You don't technically need the strchr(), since from the strncmp above, > you know the answer will be featurestr + 6. Ok fixed. > > > + val++; > > + compat = g_strdup(val); > > + goto out; > > + } > > + featurestr = strtok(NULL, ","); > > + } > > + > > +out: > > + g_free(model_str); > > + return compat; > > +} > > Couldn't we use one of the existing opts parsing functions in qemu for > the above, anyway? Using qemu_get_opt() is one option but that would need some work of setting the opts infrastructure for parsing -cpu option, but I assume with Igor's global CPU features work, that would be unwelcome. > > > static void spapr_core_release(DeviceState *dev, void *opaque) > > { > > sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev)); > > @@ -223,12 +248,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; > > + > > + 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; > > I don't think we want this else clause, because it will forbid the use > of -global core_type,compat=whatever, which Igor says is the preferred > approach for the future (IIUC, using -global is equivalent to setting > the property explicitly on every instance). > > The if clause should be ok, since it implements a fallback to using > -cpu, which we do want. In my new version, I am not touching ->pre_plug() at all since there is nothing to validate. All we do is to apply the compat property to all CPU threads if it is specified. Regards, Bharata.
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..d500cd6 100644 --- a/hw/ppc/spapr_cpu_core.c +++ b/hw/ppc/spapr_cpu_core.c @@ -96,6 +96,31 @@ 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 *model_str = g_strdup(model); + char *featurestr, *compat = NULL; + + featurestr = model_str ? strtok(model_str, ",") : NULL; + while (featurestr) { + char *val; + if (!strncmp(featurestr, "compat=", 7)) { + val = strchr(featurestr, '='); + val++; + compat = g_strdup(val); + goto out; + } + featurestr = strtok(NULL, ","); + } + +out: + g_free(model_str); + return compat; +} + static void spapr_core_release(DeviceState *dev, void *opaque) { sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev)); @@ -223,12 +248,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; + + 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 +300,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 +334,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; sc->threads = g_malloc0(size * cc->nr_threads); for (i = 0; i < cc->nr_threads; i++) { @@ -300,11 +348,18 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) if (local_err) { goto err; } + if (compat_set) { + object_property_set_str(obj, compat, "compat", &local_err); + if (local_err) { + goto err; + } + } } object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &local_err); if (local_err) { goto err; } else { + g_free(compat); return; } @@ -315,9 +370,31 @@ err: i--; } g_free(sc->threads); + g_free(compat); 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 +465,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> --- Changes in v1: - In the routine that extracts "compat=" from -cpu cmdline, made the parsing generic as suggested by Thomas Huth so that it works in the presence of any other additional features. - Addressed review comments by David with major one being setting of compat property directly instead of going via ->parse_features(). TODO: - Reconcile with Igor's work that make cpu features as global properties. - Find a way to export the compat infomation via query-hotpluggable-cpus. v0: https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg05279.html hw/ppc/spapr.c | 8 +++++ hw/ppc/spapr_cpu_core.c | 78 +++++++++++++++++++++++++++++++++++++++++ include/hw/ppc/spapr_cpu_core.h | 2 ++ 3 files changed, 88 insertions(+)