qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 08/10] cpu: use CPUClass->parse_features() as co


From: Igor Mammedov
Subject: Re: [Qemu-devel] [PATCH 08/10] cpu: use CPUClass->parse_features() as convertor to global properties
Date: Thu, 9 Jun 2016 14:38:49 +0200

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]