[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4] target-i386: Register QOM properties for fea
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH v4] target-i386: Register QOM properties for feature flags |
Date: |
Tue, 14 Apr 2015 16:19:43 +0200 |
On Tue, 14 Apr 2015 11:08:39 -0300
Eduardo Habkost <address@hidden> 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
[...]