Re: [Qemu-devel] [PATCH v4 27/33] target-i386: Register X86CPU "feat-kvmclock" feature

[Michael S. Tsirkin]

On Thu, Aug 14, 2014 at 08:59:17PM -0300, Eduardo Habkost wrote:
> On Thu, Aug 14, 2014 at 11:08:30PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Aug 14, 2014 at 04:25:56PM -0300, Eduardo Habkost wrote:
> > > The "kvmclock" feature is special because it affects two bits in the KVM
> > > CPUID leaf, so it has to be handled differently from the other feature
> > > properties that will be added.
> > > 
> > > Signed-off-by: Eduardo Habkost <address@hidden>
> > > ---
> > >  target-i386/cpu.c | 61 
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 61 insertions(+)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index b005b0d..0eb401b 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -2774,6 +2774,61 @@ uint32_t x86_cpu_apic_id_from_index(unsigned int 
> > > cpu_index)
> > >      }
> > >  }
> > >  
> > > +typedef struct FeatureProperty {
> > > +    FeatureWord word;
> > > +    uint32_t mask;
> > > +} FeatureProperty;
> > > +
> > > +
> > > +static void x86_cpu_get_feature_prop(Object *obj,
> > > +                                     struct Visitor *v,
> > > +                                     void *opaque,
> > > +                                     const char *name,
> > > +                                     Error **errp)
> > > +{
> > > +    X86CPU *cpu = X86_CPU(obj);
> > > +    CPUX86State *env = &cpu->env;
> > > +    FeatureProperty *fp = opaque;
> > > +    bool value = (env->features[fp->word] & fp->mask) == fp->mask;
> > > +    visit_type_bool(v, &value, name, errp);
> > > +}
> > > +
> > > +static void x86_cpu_set_feature_prop(Object *obj,
> > > +                                     struct Visitor *v,
> > > +                                     void *opaque,
> > > +                                     const char *name,
> > > +                                     Error **errp)
> > > +{
> > > +    X86CPU *cpu = X86_CPU(obj);
> > > +    CPUX86State *env = &cpu->env;
> > > +    FeatureProperty *fp = opaque;
> > > +    bool value;
> > > +    visit_type_bool(v, &value, name, errp);
> > > +    if (value) {
> > > +        env->features[fp->word] |= fp->mask;
> > > +    } else {
> > > +        env->features[fp->word] &= ~fp->mask;
> > > +    }
> > > +}
> > > +
> > > +/* Register a boolean feature-bits property.
> > > + * If mask has multiple bits, all must be set for the property to return 
> > > true.
> > > + */
> > > +static void x86_cpu_register_feature_prop(X86CPU *cpu,
> > > +                                          const char *prop_name,
> > > +                                          FeatureWord w,
> > > +                                          uint32_t mask)
> > > +{
> > > +    FeatureProperty *fp;
> > > +    fp = g_new0(FeatureProperty, 1);
> > > +    fp->word = w;
> > > +    fp->mask = mask;
> > > +    object_property_add(OBJECT(cpu), prop_name, "bool",
> > > +                        x86_cpu_set_feature_prop,
> > > +                        x86_cpu_get_feature_prop,
> > > +                        NULL, fp, &error_abort);
> > > +}
> > > +
> > 
> > This looks similar to what what DEFINE_PROP_BIT does.
> > Can't this be reused in some way?
> 
> DEFINE_PROP_BIT is from the static property system, and I understand we
> are preferring using object_property_add*() instead (and in the X86CPU
> features case, registering the properties dynamically using the feature
> name arrays saves us a lot of work).
> 
> I will take a look at the DEFINE_PROP_BIT code to see if anything from
> that code can be reused, but I doubt so. It seems to be tightly coupled
> to the static property system.

Main point is, can't we find a way to reduce code duplication?
It doesn't seem reasonable that we basically open-code
each property from scratch.

> > 
> > 
> > >  static void x86_cpu_initfn(Object *obj)
> > >  {
> > >      CPUState *cs = CPU(obj);
> > > @@ -2819,6 +2874,12 @@ static void x86_cpu_initfn(Object *obj)
> > >                          x86_cpu_get_feature_words,
> > >                          NULL, NULL, (void *)cpu->filtered_features, 
> > > NULL);
> > >  
> > > +    /* "feat-kvmclock" will affect both kvmclock feature bits */
> > > +    x86_cpu_register_feature_prop(cpu, "feat-kvmclock", FEAT_KVM,
> > > +                                  (1UL << KVM_FEATURE_CLOCKSOURCE) |
> > > +                                  (1UL << KVM_FEATURE_CLOCKSOURCE2));
> > > +
> > > +
> > >      cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
> > >      env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
> > >  
> > > -- 
> > > 1.9.3
> 
> -- 
> Eduardo