Re: [Qemu-arm] [PATCH 08/10] cpu: use CPUClass->parse_features() as convertor to global properties

[Eduardo Habkost]

On Thu, Jun 09, 2016 at 02:38:49PM +0200, Igor Mammedov wrote:
> 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.

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.

-- 
Eduardo