Re: [Qemu-devel] [PATCH v4] target-i386: Register QOM properties for feature flags

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 14 Apr 2015 11:08:39 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

[...]
> > > +/* Register a boolean feature-bits property.
> > > + * If mask has multiple bits, all must be set for the property to return true.
> > > + * The same property name can be registered multiple times to make it affect
> > > + * multiple bits in the same FeatureWord.
> > > + */
> > > +static void x86_cpu_register_feature_prop(X86CPU *cpu,
> > > +                                          const char *prop_name,
> > > +                                          FeatureWord w,
> > > +                                          uint32_t mask)
> > isn't used as mask by caller, s/mask/bit/ ???
> 
> There will be an use case for mask containing multiple bits, later.  My
> plan is to remove the duplicate "kvmclock" alias from kvm_feature_name,
> and call this manually:
> 
> x86_cpu_register_feature_prop(cpu, "kvmclock", FEAT_KVM,
>                               (1 << KVM_FEATURE_CLOCKSOURCE) |
>                               (1 << KVM_FEATURE_CLOCKSOURCE2));
make it mask when it starts to be used as such

> 
> I didn't do that yet because I need the existing
> x86_cpu_parse_featurestr() code to keep working until it is converted to
> use object_property_set().
> 
[...]

> > > +    op = object_property_find(OBJECT(cpu), prop_name, NULL);
> > > +    if (op) {
> > > +        fp = op->opaque;
> > > +        assert(fp->word == w);
> > > +        fp->mask |= mask;
> 
> ^^^ This is the block of code that will be removed once I add the manual
> "kvmclock" registration call I mentioned above.
> 
> > > +    } else {
> > > +        fp = g_new0(FeatureProperty, 1);
> > > +        fp->word = w;
> > > +        fp->mask = mask;
> > > +        object_property_add(OBJECT(cpu), prop_name, "bool",
> > > +                            x86_cpu_get_feature_prop,
> > > +                            x86_cpu_set_feature_prop,
> > > +                            x86_cpu_release_feature_prop, fp, &error_abort);
> > > +    }
> > > +}
> > > +
> > > +static void x86_cpu_register_feature_bit_props(X86CPU *cpu,
> > this adds 1 property and possibly aliases, _props() is confusing here.
> 
> Alias properties are still properties like any other, aren't they? The
> function is still responsible for registering multiple properties. Is
> the "_props()" suffix really that confusing?
technically aliases are properties but from user pov it's the same property
just with another name.


> 
> 
> > I'd rename it to x86_cpu_add_feature_bit_prop() and inline
> > above x86_cpu_register_feature_prop() since it's not going to be reused
> 
> I prefer to keep the single-property function separated, as it may
> become a generic bitmap property registration function inside generic
> QOM code later. With your feature_word_ptr suggestion, it would be even
> more generic and non-x86-specific.
> 
> (To be honest, I would prefer to keep the single-property function
> registration code clearly separated even if it was never going to be
> reused anywhere. 20-line functions are already too long for my taste.)
ok

[...]