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