Re: [Qemu-devel] [PATCH 05/11] target-i386: cpu: replace custom apic-id setter/getter with static property

[Eduardo Habkost]

On Tue, Jun 28, 2016 at 08:43:59AM +0200, Igor Mammedov wrote:
> On Mon, 27 Jun 2016 14:55:24 -0300
> Eduardo Habkost <address@hidden> wrote:
> 
> > On Thu, Jun 23, 2016 at 04:54:23PM +0200, Igor Mammedov wrote:
> > > custom apic-id setter/getter doesn't do any property specific
> > > checks anymorer, so clean it up and use more compact static
> > > property DEFINE_PROP_UINT32 instead.
> > > 
> > > Signed-off-by: Igor Mammedov <address@hidden>
> > > ---
> > >  target-i386/cpu.c | 45 ++++++---------------------------------------
> > >  1 file changed, 6 insertions(+), 39 deletions(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 9511474..9294b3d 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -1824,37 +1824,6 @@ static void x86_cpuid_set_tsc_freq(Object *obj, 
> > > Visitor *v, const char *name,
> > >      cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
> > >  }
> > >  
> > > -static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, const char 
> > > *name,
> > > -                                  void *opaque, Error **errp)
> > > -{
> > > -    X86CPU *cpu = X86_CPU(obj);
> > > -    int64_t value = cpu->apic_id;
> > > -
> > > -    visit_type_int(v, name, &value, errp);
> > > -}
> > > -
> > > -static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, const char 
> > > *name,
> > > -                                  void *opaque, Error **errp)
> > > -{
> > > -    X86CPU *cpu = X86_CPU(obj);
> > > -    DeviceState *dev = DEVICE(obj);
> > > -    Error *error = NULL;
> > > -    int64_t value;
> > > -
> > > -    if (dev->realized) {
> > > -        error_setg(errp, "Attempt to set property '%s' on '%s' after "
> > > -                   "it was realized", name, object_get_typename(obj));
> > > -        return;
> > > -    }
> > > -
> > > -    visit_type_int(v, name, &value, &error);
> > > -    if (error) {
> > > -        error_propagate(errp, error);
> > > -        return;
> > > -    }
> > > -    cpu->apic_id = value;
> > > -}
> > > -
> > >  /* Generic getter for "feature-words" and "filtered-features" properties 
> > > */
> > >  static void x86_cpu_get_feature_words(Object *obj, Visitor *v,
> > >                                        const char *name, void *opaque,
> > > @@ -3127,9 +3096,6 @@ static void x86_cpu_initfn(Object *obj)
> > >      object_property_add(obj, "tsc-frequency", "int",
> > >                          x86_cpuid_get_tsc_freq,
> > >                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> > > -    object_property_add(obj, "apic-id", "int",
> > > -                        x86_cpuid_get_apic_id,
> > > -                        x86_cpuid_set_apic_id, NULL, NULL, NULL);
> > >      object_property_add(obj, "feature-words", "X86CPUFeatureWordInfo",
> > >                          x86_cpu_get_feature_words,
> > >                          NULL, NULL, (void *)env->features, NULL);
> > > @@ -3139,11 +3105,6 @@ static void x86_cpu_initfn(Object *obj)
> > >  
> > >      cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
> > >  
> > > -#ifndef CONFIG_USER_ONLY
> > > -    /* Any code creating new X86CPU objects have to set apic-id 
> > > explicitly */
> > > -    cpu->apic_id = UNASSIGNED_APIC_ID;
> > > -#endif
> > > -
> > >      for (w = 0; w < FEATURE_WORDS; w++) {
> > >          int bitnr;
> > >  
> > > @@ -3200,6 +3161,12 @@ static bool x86_cpu_has_work(CPUState *cs)
> > >  }
> > >  
> > >  static Property x86_cpu_properties[] = {
> > > +#ifdef CONFIG_USER_ONLY
> > > +    /* apic_id = 0 by default for *-user, see commit 9886e834 */
> > > +    DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, 0),
> > > +#else
> > > +    DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, UNASSIGNED_APIC_ID),
> > > +#endif  
> > 
> > Suggestion for a follow-up patch: setting the default to
> > UNASSIGNED_APIC_ID unconditionally, and just adding this to
> > x86_cpu_realizefn().
> > 
> >   #ifdef CONFIG_USER_ONLY
> >       if (cpu->apic_id == UNASSIGNED_APIC_ID) {
> >           cpu->apic_id = 0;
> >       }
> >   #endif
> Putting default along with property definition seemed cleaner to me,
> considering that we want do similar thing for socket/core/thread-ids
> it still seems a better way.

Agreed that consistency is more important.

It bothers me that we don't have a better mechanism to declare
different defaults depending on CONFIG_USER_ONLY, but that's
something to be discussed later.

Reviewed-by: Eduardo Habkost <address@hidden>

-- 
Eduardo