On Thu, 9 Jun 2016 10:23:57 -0300
Eduardo Habkost <ehabk...@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 <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.
>
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.
>
