On Thu, 17 Jan 2013 15:44:45 -0200
Eduardo Habkost <ehabk...@redhat.com> wrote:
> On Thu, Jan 17, 2013 at 04:16:33PM +0100, Igor Mammedov wrote:
> > Move custom features parsing after built-in cpu_model defaults are set
> > and set custom features directly on CPU instance. That allows to make
> > clear distinction between built-in cpu model defaults that eventually
> > should go into clas_init() and extra property setting which is done
> > after defaults are set on CPU instance.
> >
> > Impl. details:
> > - features that are already properties, are converted to normalized
> > (name, values) list. And after featurestr has been parsed,
> > properties from the list are applied directly to CPU instance.
> > * For now it provides uniform handling of properties with single
> > object_property_parse() property setter.
> > * And after current features/properties are converted into static
> > properties, it will take a trivial patch to switch to global
> > properties. Which will allow to:
> > * get CPU instance initialized with all parameters passed on -cpu ...
> > cmd. line from object_new() call.
> > * call cpu_model/featurestr parsing only once before CPUs are created
> > * open a road for removing CPUxxxState.cpu_model_str field, when
> > other CPUs are similarly converted to subclasses and static properties.
> > - re-factor error handling, to use Error instead of fprintf()s, since
> > it is anyway passed in for property setter.
> >
> > Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> > ---
> > target-i386/cpu.c | 144
> > ++++++++++++++++++++++++++++------------------------- 1 files changed, 77
> > insertions(+), 67 deletions(-)
> >
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 5cd7917..50e10b1 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1302,9 +1302,24 @@ static int cpu_x86_find_by_name(x86_def_t
> > *x86_cpu_def, const char *name) return 0;
> > }
> >
> > +typedef struct NameValuePair {
> > + char *name;
> > + char *value;
> > + QTAILQ_ENTRY(NameValuePair) next;
> > +} NameValuePair;
> > +typedef QTAILQ_HEAD(NVList, NameValuePair) NVList;
> > +
> > +static void x86_cpu_add_nv_pair(NVList *list, const char *name,
> > + const char *value) {
> > + NameValuePair *p = g_malloc0(sizeof(*p));
> > + p->name = g_strdup(name);
> > + p->value = g_strdup(value);
> > + QTAILQ_INSERT_TAIL(list, p, next);
> > +}
>
> I am not sure I like this extra complexity. We don't need it if we
> simply set/register the properties directly.
>
> I have a different proposal:
>
> 1) By now, use:
> object_property_parse(OBJECT(cpu), P, V, errp)
> in the places you are using:
> x86_cpu_add_nv_pair(&props, P, V)
> below.
>
> 2) The day we move to global properties, we just need to mechanically
> replace the occurrences of:
> object_property_parse(OBJECT(cpu), P, V, errp)
> with:
> qdev_prop_register_global("X86CPU", P, V)
>
> What do you think?
Agreed, changes, when switching to global properties, will be mechanical
and not in many places, so it makes sense to drop list approach.
I'll resubmit series.
>
>
> > + object_property_parse(OBJECT(cpu), p->value, p->name, errp);
> > +
> > /* Parse "+feature,-feature,feature=foo" CPU feature string
> > */
[...]
