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

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 27 Jun 2016 14:55:24 -0300
Eduardo Habkost <ehabkost@redhat.com> 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 <imammedo@redhat.com>
> > ---
> >  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.

BTW:
there is a v2 on list already, in case you missed it.

 [PATCH v2 00/18] pc: add CPU hot-add/hot-remove with device_add/device_del