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

[Igor Mammedov <imammedo@redhat.com>]

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.
>