On Wed, 8 Jun 2016 13:47:46 -0300
Eduardo Habkost <ehabk...@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 <imamm...@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
> >
>
