| Message ID | 1465226212-254093-9-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 06, 2016 at 05:16:50PM +0200, Igor Mammedov wrote: > Currently CPUClass->parse_features() is used to parse > -cpu features string and set properties on created CPU > instances. > > But considering that features specified by -cpu apply to > every created CPU instance, it doesn't make sence to > parse the same features string for every CPU created. > It also makes every target that cares about parsing > features string explicitly call CPUClass->parse_features() > parser, which gets in a way if we consider using > generic device_add for CPU hotplug as device_add > has not a clue about CPU specific hooks. > > Turns out we can use global properties mechanism to set > properties on every created CPU instance for a given > type. That way it's possible to convert CPU features > into a set of global properties for CPU type specified > by -cpu cpu_model and common Device.device_post_init() > will apply them to CPU of given type automatically > regardless whether it's manually created CPU or CPU > created with help of device_add. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > This patch only make CPUClass->parse_features() > a global properties convertor and follow up patches > will switch individual users to new behaviour Considering that we won't fix all callers to not call it multiple times in the same series, can we add TODO notes to the ->parse_features() callers that are still need to be fixed? Additional comments (and TODO notes suggestions) below: > --- > hw/arm/virt.c | 7 ++++--- > include/qom/cpu.h | 2 +- > qom/cpu.c | 29 +++++++++++++++++------------ > target-i386/cpu.c | 30 ++++++++++++++++++++---------- > 4 files changed, 42 insertions(+), 26 deletions(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index e77ed88..473e439 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1193,6 +1193,7 @@ static void machvirt_init(MachineState *machine) > > for (n = 0; n < smp_cpus; n++) { > ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]); > + const char *typename = object_class_get_name(oc); > CPUClass *cc = CPU_CLASS(oc); > Object *cpuobj; > Error *err = NULL; > @@ -1202,10 +1203,10 @@ static void machvirt_init(MachineState *machine) > error_report("Unable to find CPU definition"); > exit(1); > } > - cpuobj = object_new(object_class_get_name(oc)); > + /* convert -smp CPU options specified by the user into global props */ > + cc->parse_features(typename, cpuopts, &err); /*TODO: call cc->parse_features() only once */ > + cpuobj = object_new(typename); > > - /* Handle any CPU options specified by the user */ > - cc->parse_features(CPU(cpuobj), cpuopts, &err); > g_free(cpuopts); > if (err) { > error_report_err(err); > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 32f3af3..cacb100 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -134,7 +134,7 @@ typedef struct CPUClass { > /*< public >*/ > > ObjectClass *(*class_by_name)(const char *cpu_model); > - void (*parse_features)(CPUState *cpu, char *str, Error **errp); > + void (*parse_features)(const char *typename, char *str, Error **errp); > > void (*reset)(CPUState *cpu); > int reset_dump_flags; > diff --git a/qom/cpu.c b/qom/cpu.c > index 751e992..f3e3c02 100644 > --- a/qom/cpu.c > +++ b/qom/cpu.c > @@ -28,6 +28,7 @@ > #include "exec/log.h" > #include "qemu/error-report.h" > #include "sysemu/sysemu.h" > +#include "hw/qdev-properties.h" > > bool cpu_exists(int64_t id) > { > @@ -46,7 +47,7 @@ bool cpu_exists(int64_t id) > CPUState *cpu_generic_init(const char *typename, const char *cpu_model) > { > char *str, *name, *featurestr; > - CPUState *cpu; > + CPUState *cpu = NULL; > ObjectClass *oc; > CPUClass *cc; > Error *err = NULL; > @@ -60,16 +61,15 @@ CPUState *cpu_generic_init(const char *typename, const char *cpu_model) > return NULL; > } > > - cpu = CPU(object_new(object_class_get_name(oc))); > - cc = CPU_GET_CLASS(cpu); > - > + cc = CPU_CLASS(oc); > featurestr = strtok(NULL, ","); > - cc->parse_features(cpu, featurestr, &err); > + cc->parse_features(object_class_get_name(oc), featurestr, &err); /*TODO: all callers of cpu_generic_init() need to be converted to * call parse_features() only once, before calling cpu_generic_init(). */ > g_free(str); > if (err != NULL) { > goto out; > } > > + cpu = CPU(object_new(object_class_get_name(oc))); > object_property_set_bool(OBJECT(cpu), true, "realized", &err); > > out: > @@ -282,25 +282,29 @@ static ObjectClass *cpu_common_class_by_name(const char *cpu_model) > return NULL; > } > > -static void cpu_common_parse_features(CPUState *cpu, char *features, > +static void cpu_common_parse_features(const char *typename, char *features, > Error **errp) > { > char *featurestr; /* Single "key=value" string being parsed */ > char *val; > - Error *err = NULL; > + static bool cpu_globals_initialized; > + /*TODO: all callers of ->parse_features() need to be changed to * call it only once, so we can remove this check (or change it * to assert(!cpu_globals_initialized). * Current callers of ->parse_features() are: * - machvirt_init() * - cpu_generic_init() * - cpu_x86_create() */ As far as I can see, after applying the whole series, only cpu_x86_create() will remain. > + if (cpu_globals_initialized) { > + return; > + } > > featurestr = features ? strtok(features, ",") : NULL; > > while (featurestr) { > val = strchr(featurestr, '='); > if (val) { > + GlobalProperty *prop = g_new0(typeof(*prop), 1); > *val = 0; > val++; > - object_property_parse(OBJECT(cpu), val, featurestr, &err); > - if (err) { > - error_propagate(errp, err); > - return; > - } > + prop->driver = typename; > + prop->property = g_strdup(featurestr); > + prop->value = g_strdup(val); > + qdev_prop_register_global(prop); > } else { > error_setg(errp, "Expected key=value format, found %s.", > featurestr); > @@ -308,6 +312,7 @@ static void cpu_common_parse_features(CPUState *cpu, char *features, > } > featurestr = strtok(NULL, ","); > } > + cpu_globals_initialized = true; This will register globals multiple times if called with "foo=x,bar". Easier to just set cpu_globals_initialized=true earlier, and report errors only on the first ->parse_features() call? (The same comment applies to x86_cpu_parse_featurestr() below) > } > > static void cpu_common_realizefn(DeviceState *dev, Error **errp) > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 31e5e6f..43b22e6 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1950,12 +1950,16 @@ static FeatureWordArray minus_features = { 0 }; > > /* Parse "+feature,-feature,feature=foo" CPU feature string > */ > -static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > +static void x86_cpu_parse_featurestr(const char *typename, char *features, > Error **errp) > { > - X86CPU *cpu = X86_CPU(cs); > char *featurestr; /* Single 'key=value" string being parsed */ > Error *local_err = NULL; > + static bool cpu_globals_initialized; > + > + if (cpu_globals_initialized) { > + return; > + } > > if (!features) { > return; > @@ -1967,6 +1971,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > const char *name; > const char *val = NULL; > char *eq = NULL; > + GlobalProperty *prop; > > /* Compatibility syntax: */ > if (featurestr[0] == '+') { > @@ -2019,12 +2024,14 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > name = "tsc-frequency"; > } > > - object_property_parse(OBJECT(cpu), val, name, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > - return; > - } > + prop = g_new0(typeof(*prop), 1); > + prop->driver = typename; > + prop->property = g_strdup(name); > + prop->value = g_strdup(val); > + qdev_prop_register_global(prop); > } > + > + cpu_globals_initialized = true; > } > > /* Print all cpuid feature names in featureset > @@ -2208,9 +2215,11 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp) > { > X86CPU *cpu = NULL; > ObjectClass *oc; > + CPUClass *cc; > gchar **model_pieces; > char *name, *features; > Error *error = NULL; > + const char *typename; > > model_pieces = g_strsplit(cpu_model, ",", 2); > if (!model_pieces[0]) { > @@ -2225,10 +2234,11 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp) > error_setg(&error, "Unable to find CPU definition: %s", name); > goto out; > } > + cc = CPU_CLASS(oc); > + typename = object_class_get_name(oc); > > - cpu = X86_CPU(object_new(object_class_get_name(oc))); > - > - x86_cpu_parse_featurestr(CPU(cpu), features, &error); > + cc->parse_features(typename, features, &error); /*TODO: call cc->parse_features() only once, before calling cpu_x86_create() */ > + cpu = X86_CPU(object_new(typename)); > if (error) { > goto out; > } > -- > 1.8.3.1 >
On Wed, 8 Jun 2016 13:47:46 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Jun 06, 2016 at 05:16:50PM +0200, Igor Mammedov wrote: > > Currently CPUClass->parse_features() is used to parse > > -cpu features string and set properties on created CPU > > instances. > > > > But considering that features specified by -cpu apply to > > every created CPU instance, it doesn't make sence to > > parse the same features string for every CPU created. > > It also makes every target that cares about parsing > > features string explicitly call CPUClass->parse_features() > > parser, which gets in a way if we consider using > > generic device_add for CPU hotplug as device_add > > has not a clue about CPU specific hooks. > > > > Turns out we can use global properties mechanism to set > > properties on every created CPU instance for a given > > type. That way it's possible to convert CPU features > > into a set of global properties for CPU type specified > > by -cpu cpu_model and common Device.device_post_init() > > will apply them to CPU of given type automatically > > regardless whether it's manually created CPU or CPU > > created with help of device_add. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > This patch only make CPUClass->parse_features() > > a global properties convertor and follow up patches > > will switch individual users to new behaviour > > Considering that we won't fix all callers to not call it multiple > times in the same series, can we add TODO notes to the > ->parse_features() callers that are still need to be fixed? the only callers left that aren't fixed after this series are cpu_init() callers. The rest are taken care of by the last 2 patches. > > Additional comments (and TODO notes suggestions) below: > > > --- > > hw/arm/virt.c | 7 ++++--- > > include/qom/cpu.h | 2 +- > > qom/cpu.c | 29 +++++++++++++++++------------ > > target-i386/cpu.c | 30 ++++++++++++++++++++---------- > > 4 files changed, 42 insertions(+), 26 deletions(-) > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > index e77ed88..473e439 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -1193,6 +1193,7 @@ static void machvirt_init(MachineState > > *machine) > > for (n = 0; n < smp_cpus; n++) { > > ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, > > cpustr[0]); > > + const char *typename = object_class_get_name(oc); > > CPUClass *cc = CPU_CLASS(oc); > > Object *cpuobj; > > Error *err = NULL; > > @@ -1202,10 +1203,10 @@ static void machvirt_init(MachineState > > *machine) error_report("Unable to find CPU definition"); > > exit(1); > > } > > - cpuobj = object_new(object_class_get_name(oc)); > > + /* convert -smp CPU options specified by the user into > > global props */ > > + cc->parse_features(typename, cpuopts, &err); > > /*TODO: call cc->parse_features() only once */ I'd not add TODO here as it's done by the next patch. > > > + cpuobj = object_new(typename); > > > > - /* Handle any CPU options specified by the user */ > > - cc->parse_features(CPU(cpuobj), cpuopts, &err); > > g_free(cpuopts); > > if (err) { > > error_report_err(err); > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > > index 32f3af3..cacb100 100644 > > --- a/include/qom/cpu.h > > +++ b/include/qom/cpu.h > > @@ -134,7 +134,7 @@ typedef struct CPUClass { > > /*< public >*/ > > > > ObjectClass *(*class_by_name)(const char *cpu_model); > > - void (*parse_features)(CPUState *cpu, char *str, Error **errp); > > + void (*parse_features)(const char *typename, char *str, Error > > **errp); > > void (*reset)(CPUState *cpu); > > int reset_dump_flags; > > diff --git a/qom/cpu.c b/qom/cpu.c > > index 751e992..f3e3c02 100644 > > --- a/qom/cpu.c > > +++ b/qom/cpu.c > > @@ -28,6 +28,7 @@ > > #include "exec/log.h" > > #include "qemu/error-report.h" > > #include "sysemu/sysemu.h" > > +#include "hw/qdev-properties.h" > > > > bool cpu_exists(int64_t id) > > { > > @@ -46,7 +47,7 @@ bool cpu_exists(int64_t id) > > CPUState *cpu_generic_init(const char *typename, const char > > *cpu_model) { > > char *str, *name, *featurestr; > > - CPUState *cpu; > > + CPUState *cpu = NULL; > > ObjectClass *oc; > > CPUClass *cc; > > Error *err = NULL; > > @@ -60,16 +61,15 @@ CPUState *cpu_generic_init(const char > > *typename, const char *cpu_model) return NULL; > > } > > > > - cpu = CPU(object_new(object_class_get_name(oc))); > > - cc = CPU_GET_CLASS(cpu); > > - > > + cc = CPU_CLASS(oc); > > featurestr = strtok(NULL, ","); > > - cc->parse_features(cpu, featurestr, &err); > > + cc->parse_features(object_class_get_name(oc), featurestr, > > &err); > > /*TODO: all callers of cpu_generic_init() need to be converted to > * call parse_features() only once, before calling cpu_generic_init(). > */ > > > g_free(str); > > if (err != NULL) { > > goto out; > > } > > > > + cpu = CPU(object_new(object_class_get_name(oc))); > > object_property_set_bool(OBJECT(cpu), true, "realized", &err); > > > > out: > > @@ -282,25 +282,29 @@ static ObjectClass > > *cpu_common_class_by_name(const char *cpu_model) return NULL; > > } > > > > -static void cpu_common_parse_features(CPUState *cpu, char > > *features, +static void cpu_common_parse_features(const char > > *typename, char *features, Error **errp) > > { > > char *featurestr; /* Single "key=value" string being parsed */ > > char *val; > > - Error *err = NULL; > > + static bool cpu_globals_initialized; > > + > > /*TODO: all callers of ->parse_features() need to be changed to > * call it only once, so we can remove this check (or change it > * to assert(!cpu_globals_initialized). > * Current callers of ->parse_features() are: > * - machvirt_init() > * - cpu_generic_init() > * - cpu_x86_create() > */ > > As far as I can see, after applying the whole series, only > cpu_x86_create() will remain. Have you meant cpu_generic_init() ? cpu_x86_create is removed in the last patch. So I'd drop cpu_x86_create() and machvirt_init() from suggested comment. > > > + if (cpu_globals_initialized) { > > + return; > > + } > > > > featurestr = features ? strtok(features, ",") : NULL; > > > > while (featurestr) { > > val = strchr(featurestr, '='); > > if (val) { > > + GlobalProperty *prop = g_new0(typeof(*prop), 1); > > *val = 0; > > val++; > > - object_property_parse(OBJECT(cpu), val, featurestr, > > &err); > > - if (err) { > > - error_propagate(errp, err); > > - return; > > - } > > + prop->driver = typename; > > + prop->property = g_strdup(featurestr); > > + prop->value = g_strdup(val); > > + qdev_prop_register_global(prop); > > } else { > > error_setg(errp, "Expected key=value format, found > > %s.", featurestr); > > @@ -308,6 +312,7 @@ static void cpu_common_parse_features(CPUState > > *cpu, char *features, } > > featurestr = strtok(NULL, ","); > > } > > + cpu_globals_initialized = true; > > This will register globals multiple times if called with > "foo=x,bar". I fail to see how it could happen here. > Easier to just set cpu_globals_initialized=true > earlier, and report errors only on the first ->parse_features() > call? Agreed, I'll make it like this: if (cpu_globals_initialized) { return; } cpu_globals_initialized = true; > (The same comment applies to x86_cpu_parse_featurestr() below) > > > } > > > > static void cpu_common_realizefn(DeviceState *dev, Error **errp) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 31e5e6f..43b22e6 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -1950,12 +1950,16 @@ static FeatureWordArray minus_features = > > { 0 }; > > /* Parse "+feature,-feature,feature=foo" CPU feature string > > */ > > -static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > > +static void x86_cpu_parse_featurestr(const char *typename, char > > *features, Error **errp) > > { > > - X86CPU *cpu = X86_CPU(cs); > > char *featurestr; /* Single 'key=value" string being parsed */ > > Error *local_err = NULL; > > + static bool cpu_globals_initialized; > > + > > + if (cpu_globals_initialized) { > > + return; > > + } > > > > if (!features) { > > return; > > @@ -1967,6 +1971,7 @@ static void x86_cpu_parse_featurestr(CPUState > > *cs, char *features, const char *name; > > const char *val = NULL; > > char *eq = NULL; > > + GlobalProperty *prop; > > > > /* Compatibility syntax: */ > > if (featurestr[0] == '+') { > > @@ -2019,12 +2024,14 @@ static void > > x86_cpu_parse_featurestr(CPUState *cs, char *features, name = > > "tsc-frequency"; } > > > > - object_property_parse(OBJECT(cpu), val, name, &local_err); > > - if (local_err) { > > - error_propagate(errp, local_err); > > - return; > > - } > > + prop = g_new0(typeof(*prop), 1); > > + prop->driver = typename; > > + prop->property = g_strdup(name); > > + prop->value = g_strdup(val); > > + qdev_prop_register_global(prop); > > } > > + > > + cpu_globals_initialized = true; > > } > > > > /* Print all cpuid feature names in featureset > > @@ -2208,9 +2215,11 @@ X86CPU *cpu_x86_create(const char > > *cpu_model, Error **errp) { > > X86CPU *cpu = NULL; > > ObjectClass *oc; > > + CPUClass *cc; > > gchar **model_pieces; > > char *name, *features; > > Error *error = NULL; > > + const char *typename; > > > > model_pieces = g_strsplit(cpu_model, ",", 2); > > if (!model_pieces[0]) { > > @@ -2225,10 +2234,11 @@ X86CPU *cpu_x86_create(const char > > *cpu_model, Error **errp) error_setg(&error, "Unable to find CPU > > definition: %s", name); goto out; > > } > > + cc = CPU_CLASS(oc); > > + typename = object_class_get_name(oc); > > > > - cpu = X86_CPU(object_new(object_class_get_name(oc))); > > - > > - x86_cpu_parse_featurestr(CPU(cpu), features, &error); > > + cc->parse_features(typename, features, &error); > > /*TODO: call cc->parse_features() only once, before calling > cpu_x86_create() */ ditto, I'd not add TODO here as it's done by a following patch. > > > + cpu = X86_CPU(object_new(typename)); > > if (error) { > > goto out; > > } > > -- > > 1.8.3.1 > > >
On Thu, Jun 09, 2016 at 02:38:49PM +0200, Igor Mammedov wrote: > On Wed, 8 Jun 2016 13:47:46 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Mon, Jun 06, 2016 at 05:16:50PM +0200, Igor Mammedov wrote: > > > Currently CPUClass->parse_features() is used to parse > > > -cpu features string and set properties on created CPU > > > instances. > > > > > > But considering that features specified by -cpu apply to > > > every created CPU instance, it doesn't make sence to > > > parse the same features string for every CPU created. > > > It also makes every target that cares about parsing > > > features string explicitly call CPUClass->parse_features() > > > parser, which gets in a way if we consider using > > > generic device_add for CPU hotplug as device_add > > > has not a clue about CPU specific hooks. > > > > > > Turns out we can use global properties mechanism to set > > > properties on every created CPU instance for a given > > > type. That way it's possible to convert CPU features > > > into a set of global properties for CPU type specified > > > by -cpu cpu_model and common Device.device_post_init() > > > will apply them to CPU of given type automatically > > > regardless whether it's manually created CPU or CPU > > > created with help of device_add. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > This patch only make CPUClass->parse_features() > > > a global properties convertor and follow up patches > > > will switch individual users to new behaviour > > > > Considering that we won't fix all callers to not call it multiple > > times in the same series, can we add TODO notes to the > > ->parse_features() callers that are still need to be fixed? > the only callers left that aren't fixed after this series are > cpu_init() callers. > The rest are taken care of by the last 2 patches. I just miss some documentation in the patch saying why exactly we still need cpu_globals_initialized. I like to keep the comments consistent in the intermediate steps, as in case this patch is considered good for inclusion but the other two need a respin for some reason. But if you want to add a comment just for cpu_init()/cpu_generic_init(), that's OK. > > > > > Additional comments (and TODO notes suggestions) below: > > [...] > > > > /*TODO: all callers of ->parse_features() need to be changed to > > * call it only once, so we can remove this check (or change it > > * to assert(!cpu_globals_initialized). > > * Current callers of ->parse_features() are: I guess this needs to be changed to "current callers of ->parse_features() that may call it multiple times". > > * - machvirt_init() > > * - cpu_generic_init() > > * - cpu_x86_create() > > */ > > > > As far as I can see, after applying the whole series, only > > cpu_x86_create() will remain. > Have you meant cpu_generic_init() ? cpu_x86_create is removed > in the last patch. Oops, yes, I meant cpu_generic_init(). > > So I'd drop cpu_x86_create() and machvirt_init() from suggested > comment. Works for me. Although I prefer when patches can be reviewed/applied on their own, without depending on the patches that come after them. > > > > > > + if (cpu_globals_initialized) { > > > + return; > > > + } > > > > > > featurestr = features ? strtok(features, ",") : NULL; > > > > > > while (featurestr) { > > > val = strchr(featurestr, '='); > > > if (val) { > > > + GlobalProperty *prop = g_new0(typeof(*prop), 1); > > > *val = 0; > > > val++; > > > - object_property_parse(OBJECT(cpu), val, featurestr, > > > &err); > > > - if (err) { > > > - error_propagate(errp, err); > > > - return; > > > - } > > > + prop->driver = typename; > > > + prop->property = g_strdup(featurestr); > > > + prop->value = g_strdup(val); > > > + qdev_prop_register_global(prop); > > > } else { > > > error_setg(errp, "Expected key=value format, found > > > %s.", featurestr); > > > @@ -308,6 +312,7 @@ static void cpu_common_parse_features(CPUState > > > *cpu, char *features, } > > > featurestr = strtok(NULL, ","); > > > } > > > + cpu_globals_initialized = true; > > > > This will register globals multiple times if called with > > "foo=x,bar". > I fail to see how it could happen here. I mean it will register globals multiple times if the function is called multiple times. "foo=x" will be registered before the error at "bar" is detected and reported. > > > Easier to just set cpu_globals_initialized=true > > earlier, and report errors only on the first ->parse_features() > > call? > Agreed, I'll make it like this: > > if (cpu_globals_initialized) { > return; > } > cpu_globals_initialized = true; OK.
On Thu, 9 Jun 2016 10:23:57 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Jun 09, 2016 at 02:38:49PM +0200, Igor Mammedov wrote: > > On Wed, 8 Jun 2016 13:47:46 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Mon, Jun 06, 2016 at 05:16:50PM +0200, Igor Mammedov wrote: > > > > Currently CPUClass->parse_features() is used to parse > > > > -cpu features string and set properties on created CPU > > > > instances. > > > > > > > > But considering that features specified by -cpu apply to > > > > every created CPU instance, it doesn't make sence to > > > > parse the same features string for every CPU created. > > > > It also makes every target that cares about parsing > > > > features string explicitly call CPUClass->parse_features() > > > > parser, which gets in a way if we consider using > > > > generic device_add for CPU hotplug as device_add > > > > has not a clue about CPU specific hooks. > > > > > > > > Turns out we can use global properties mechanism to set > > > > properties on every created CPU instance for a given > > > > type. That way it's possible to convert CPU features > > > > into a set of global properties for CPU type specified > > > > by -cpu cpu_model and common Device.device_post_init() > > > > will apply them to CPU of given type automatically > > > > regardless whether it's manually created CPU or CPU > > > > created with help of device_add. > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > --- > > > > This patch only make CPUClass->parse_features() > > > > a global properties convertor and follow up patches > > > > will switch individual users to new behaviour > > > > > > Considering that we won't fix all callers to not call it multiple > > > times in the same series, can we add TODO notes to the > > > ->parse_features() callers that are still need to be fixed? > > the only callers left that aren't fixed after this series are > > cpu_init() callers. > > The rest are taken care of by the last 2 patches. > > I just miss some documentation in the patch saying why exactly we > still need cpu_globals_initialized. > > I like to keep the comments consistent in the intermediate steps, > as in case this patch is considered good for inclusion but the > other two need a respin for some reason. But if you want to add a > comment just for cpu_init()/cpu_generic_init(), that's OK. > Ok, I'll post v2 for this patch as reply here as 2 following patches look ok and don't need respining. > > > > > > > > Additional comments (and TODO notes suggestions) below: > > > > [...] > > > > > > /*TODO: all callers of ->parse_features() need to be changed to > > > * call it only once, so we can remove this check (or change it > > > * to assert(!cpu_globals_initialized). > > > * Current callers of ->parse_features() are: > > I guess this needs to be changed to "current callers of > ->parse_features() that may call it multiple times". > > > > * - machvirt_init() > > > * - cpu_generic_init() > > > * - cpu_x86_create() > > > */ > > > > > > As far as I can see, after applying the whole series, only > > > cpu_x86_create() will remain. > > Have you meant cpu_generic_init() ? cpu_x86_create is removed > > in the last patch. > > Oops, yes, I meant cpu_generic_init(). > > > > > So I'd drop cpu_x86_create() and machvirt_init() from suggested > > comment. > > Works for me. Although I prefer when patches can be > reviewed/applied on their own, without depending on the patches > that come after them. > > > > > > > > > > + if (cpu_globals_initialized) { > > > > + return; > > > > + } > > > > > > > > featurestr = features ? strtok(features, ",") : NULL; > > > > > > > > while (featurestr) { > > > > val = strchr(featurestr, '='); > > > > if (val) { > > > > + GlobalProperty *prop = g_new0(typeof(*prop), 1); > > > > *val = 0; > > > > val++; > > > > - object_property_parse(OBJECT(cpu), val, featurestr, > > > > &err); > > > > - if (err) { > > > > - error_propagate(errp, err); > > > > - return; > > > > - } > > > > + prop->driver = typename; > > > > + prop->property = g_strdup(featurestr); > > > > + prop->value = g_strdup(val); > > > > + qdev_prop_register_global(prop); > > > > } else { > > > > error_setg(errp, "Expected key=value format, found > > > > %s.", featurestr); > > > > @@ -308,6 +312,7 @@ static void > > > > cpu_common_parse_features(CPUState *cpu, char *features, } > > > > featurestr = strtok(NULL, ","); > > > > } > > > > + cpu_globals_initialized = true; > > > > > > This will register globals multiple times if called with > > > "foo=x,bar". > > I fail to see how it could happen here. > > I mean it will register globals multiple times if the function is > called multiple times. "foo=x" will be registered before the > error at "bar" is detected and reported. That's true, however I haven't considered it as a caller of parse_features() will not call it second time if error occurred. > > > > > > Easier to just set cpu_globals_initialized=true > > > earlier, and report errors only on the first ->parse_features() > > > call? > > Agreed, I'll make it like this: > > > > if (cpu_globals_initialized) { > > return; > > } > > cpu_globals_initialized = true; > > OK. >
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index e77ed88..473e439 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1193,6 +1193,7 @@ static void machvirt_init(MachineState *machine) for (n = 0; n < smp_cpus; n++) { ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]); + const char *typename = object_class_get_name(oc); CPUClass *cc = CPU_CLASS(oc); Object *cpuobj; Error *err = NULL; @@ -1202,10 +1203,10 @@ static void machvirt_init(MachineState *machine) error_report("Unable to find CPU definition"); exit(1); } - cpuobj = object_new(object_class_get_name(oc)); + /* convert -smp CPU options specified by the user into global props */ + cc->parse_features(typename, cpuopts, &err); + cpuobj = object_new(typename); - /* Handle any CPU options specified by the user */ - cc->parse_features(CPU(cpuobj), cpuopts, &err); g_free(cpuopts); if (err) { error_report_err(err); diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 32f3af3..cacb100 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -134,7 +134,7 @@ typedef struct CPUClass { /*< public >*/ ObjectClass *(*class_by_name)(const char *cpu_model); - void (*parse_features)(CPUState *cpu, char *str, Error **errp); + void (*parse_features)(const char *typename, char *str, Error **errp); void (*reset)(CPUState *cpu); int reset_dump_flags; diff --git a/qom/cpu.c b/qom/cpu.c index 751e992..f3e3c02 100644 --- a/qom/cpu.c +++ b/qom/cpu.c @@ -28,6 +28,7 @@ #include "exec/log.h" #include "qemu/error-report.h" #include "sysemu/sysemu.h" +#include "hw/qdev-properties.h" bool cpu_exists(int64_t id) { @@ -46,7 +47,7 @@ bool cpu_exists(int64_t id) CPUState *cpu_generic_init(const char *typename, const char *cpu_model) { char *str, *name, *featurestr; - CPUState *cpu; + CPUState *cpu = NULL; ObjectClass *oc; CPUClass *cc; Error *err = NULL; @@ -60,16 +61,15 @@ CPUState *cpu_generic_init(const char *typename, const char *cpu_model) return NULL; } - cpu = CPU(object_new(object_class_get_name(oc))); - cc = CPU_GET_CLASS(cpu); - + cc = CPU_CLASS(oc); featurestr = strtok(NULL, ","); - cc->parse_features(cpu, featurestr, &err); + cc->parse_features(object_class_get_name(oc), featurestr, &err); g_free(str); if (err != NULL) { goto out; } + cpu = CPU(object_new(object_class_get_name(oc))); object_property_set_bool(OBJECT(cpu), true, "realized", &err); out: @@ -282,25 +282,29 @@ static ObjectClass *cpu_common_class_by_name(const char *cpu_model) return NULL; } -static void cpu_common_parse_features(CPUState *cpu, char *features, +static void cpu_common_parse_features(const char *typename, char *features, Error **errp) { char *featurestr; /* Single "key=value" string being parsed */ char *val; - Error *err = NULL; + static bool cpu_globals_initialized; + + if (cpu_globals_initialized) { + return; + } featurestr = features ? strtok(features, ",") : NULL; while (featurestr) { val = strchr(featurestr, '='); if (val) { + GlobalProperty *prop = g_new0(typeof(*prop), 1); *val = 0; val++; - object_property_parse(OBJECT(cpu), val, featurestr, &err); - if (err) { - error_propagate(errp, err); - return; - } + prop->driver = typename; + prop->property = g_strdup(featurestr); + prop->value = g_strdup(val); + qdev_prop_register_global(prop); } else { error_setg(errp, "Expected key=value format, found %s.", featurestr); @@ -308,6 +312,7 @@ static void cpu_common_parse_features(CPUState *cpu, char *features, } featurestr = strtok(NULL, ","); } + cpu_globals_initialized = true; } static void cpu_common_realizefn(DeviceState *dev, Error **errp) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 31e5e6f..43b22e6 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1950,12 +1950,16 @@ static FeatureWordArray minus_features = { 0 }; /* Parse "+feature,-feature,feature=foo" CPU feature string */ -static void x86_cpu_parse_featurestr(CPUState *cs, char *features, +static void x86_cpu_parse_featurestr(const char *typename, char *features, Error **errp) { - X86CPU *cpu = X86_CPU(cs); char *featurestr; /* Single 'key=value" string being parsed */ Error *local_err = NULL; + static bool cpu_globals_initialized; + + if (cpu_globals_initialized) { + return; + } if (!features) { return; @@ -1967,6 +1971,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, const char *name; const char *val = NULL; char *eq = NULL; + GlobalProperty *prop; /* Compatibility syntax: */ if (featurestr[0] == '+') { @@ -2019,12 +2024,14 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, name = "tsc-frequency"; } - object_property_parse(OBJECT(cpu), val, name, &local_err); - if (local_err) { - error_propagate(errp, local_err); - return; - } + prop = g_new0(typeof(*prop), 1); + prop->driver = typename; + prop->property = g_strdup(name); + prop->value = g_strdup(val); + qdev_prop_register_global(prop); } + + cpu_globals_initialized = true; } /* Print all cpuid feature names in featureset @@ -2208,9 +2215,11 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp) { X86CPU *cpu = NULL; ObjectClass *oc; + CPUClass *cc; gchar **model_pieces; char *name, *features; Error *error = NULL; + const char *typename; model_pieces = g_strsplit(cpu_model, ",", 2); if (!model_pieces[0]) { @@ -2225,10 +2234,11 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp) error_setg(&error, "Unable to find CPU definition: %s", name); goto out; } + cc = CPU_CLASS(oc); + typename = object_class_get_name(oc); - cpu = X86_CPU(object_new(object_class_get_name(oc))); - - x86_cpu_parse_featurestr(CPU(cpu), features, &error); + cc->parse_features(typename, features, &error); + cpu = X86_CPU(object_new(typename)); if (error) { goto out; }
Currently CPUClass->parse_features() is used to parse -cpu features string and set properties on created CPU instances. But considering that features specified by -cpu apply to every created CPU instance, it doesn't make sence to parse the same features string for every CPU created. It also makes every target that cares about parsing features string explicitly call CPUClass->parse_features() parser, which gets in a way if we consider using generic device_add for CPU hotplug as device_add has not a clue about CPU specific hooks. Turns out we can use global properties mechanism to set properties on every created CPU instance for a given type. That way it's possible to convert CPU features into a set of global properties for CPU type specified by -cpu cpu_model and common Device.device_post_init() will apply them to CPU of given type automatically regardless whether it's manually created CPU or CPU created with help of device_add. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- This patch only make CPUClass->parse_features() a global properties convertor and follow up patches will switch individual users to new behaviour --- hw/arm/virt.c | 7 ++++--- include/qom/cpu.h | 2 +- qom/cpu.c | 29 +++++++++++++++++------------ target-i386/cpu.c | 30 ++++++++++++++++++++---------- 4 files changed, 42 insertions(+), 26 deletions(-)