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

[Eduardo Habkost]

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

-- 
Eduardo