Re: [Qemu-devel] [RFC PATCH v0 8/9] target-i386: Set apic_id during CPU initfn

[Eduardo Habkost]

On Thu, Dec 10, 2015 at 11:45:43AM +0530, Bharata B Rao wrote:
> Move back the setting of apic_id to instance_init routine (x86_cpu_initfn)
> This is needed to initialize X86 CPUs using generic cpu-package device.

Could you explain where exactly apic_id will be used, to make it
necessary to initialize it earlier?

> 
> TODO: I am not fully aware of the general direction in which apic_id
> changes in X86 have evolved and hence not sure if this is indeed aligned with
> the X86 way of doing things. This is just to help the PoC implementation
> that I have in this patchset to convert PC CPUs initialization into
> cpu-package device based initialization.

You shouldn't initialize apic_id on initfn. APIC ID depends (and
will depend) on different CPU properties related to topology,
including (but not limited to) CPU index and CPU topology
properties we may introduce in the future, so it should be done
later (at realize time), not on initfn.

Also, cpu_index is initialized by cpu_exec_init(), and
cpu_exec_init() must not be called by initfn. The cpu_exec_init()
call should (and will) be moved to realize in x86 and all other
architectures.

> 
> Signed-off-by: Bharata B Rao <address@hidden>
> ---
>  hw/i386/pc.c      | 33 ---------------------------------
>  target-i386/cpu.c | 37 +++++++++++++++++++++++++++++++++++--
>  target-i386/cpu.h |  1 +
>  3 files changed, 36 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index ffcd645..80a4d98 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -670,39 +670,6 @@ bool e820_get_entry(int idx, uint32_t type, uint64_t 
> *address, uint64_t *length)
>      return false;
>  }
>  
> -/* Enables contiguous-apic-ID mode, for compatibility */
> -static bool compat_apic_id_mode;
> -
> -void enable_compat_apic_id_mode(void)
> -{
> -    compat_apic_id_mode = true;
> -}
> -
> -/* Calculates initial APIC ID for a specific CPU index
> - *
> - * Currently we need to be able to calculate the APIC ID from the CPU index
> - * alone (without requiring a CPU object), as the QEMU<->Seabios interfaces 
> have
> - * no concept of "CPU index", and the NUMA tables on fw_cfg need the APIC ID 
> of
> - * all CPUs up to max_cpus.
> - */
> -static uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
> -{
> -    uint32_t correct_id;
> -    static bool warned;
> -
> -    correct_id = x86_apicid_from_cpu_idx(smp_cores, smp_threads, cpu_index);
> -    if (compat_apic_id_mode) {
> -        if (cpu_index != correct_id && !warned && !qtest_enabled()) {
> -            error_report("APIC IDs set in compatibility mode, "
> -                         "CPU topology won't match the configuration");
> -            warned = true;
> -        }
> -        return cpu_index;
> -    } else {
> -        return correct_id;
> -    }
> -}
> -
>  /* Calculates the limit to CPU APIC ID values
>   *
>   * This function returns the limit for the APIC ID value, so that all
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 11e5e39..c97a646 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -25,6 +25,7 @@
>  #include "sysemu/kvm.h"
>  #include "sysemu/cpus.h"
>  #include "kvm_i386.h"
> +#include "hw/i386/topology.h"
>  
>  #include "qemu/error-report.h"
>  #include "qemu/option.h"
> @@ -3028,6 +3029,39 @@ static void x86_cpu_register_feature_bit_props(X86CPU 
> *cpu,
>      g_strfreev(names);
>  }
>  
> +/* Enables contiguous-apic-ID mode, for compatibility */
> +static bool compat_apic_id_mode;
> +
> +void enable_compat_apic_id_mode(void)
> +{
> +    compat_apic_id_mode = true;
> +}
> +
> +/* Calculates initial APIC ID for a specific CPU index
> + *
> + * Currently we need to be able to calculate the APIC ID from the CPU index
> + * alone (without requiring a CPU object), as the QEMU<->Seabios interfaces 
> have
> + * no concept of "CPU index", and the NUMA tables on fw_cfg need the APIC ID 
> of
> + * all CPUs up to max_cpus.
> + */
> +uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
> +{
> +    uint32_t correct_id;
> +    static bool warned;
> +
> +    correct_id = x86_apicid_from_cpu_idx(smp_cores, smp_threads, cpu_index);
> +    if (compat_apic_id_mode) {
> +        if (cpu_index != correct_id && !warned) {
> +            error_report("APIC IDs set in compatibility mode, "
> +                         "CPU topology won't match the configuration");
> +            warned = true;
> +        }
> +        return cpu_index;
> +    } else {
> +        return correct_id;
> +    }
> +}
> +
>  static void x86_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -3071,8 +3105,7 @@ 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 = -1;
> +    cpu->apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
>  #endif
>  
>      for (w = 0; w < FEATURE_WORDS; w++) {
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index fc4a605..a5368cf 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1333,6 +1333,7 @@ void x86_cpu_change_kvm_default(const char *prop, const 
> char *value);
>  /* Return name of 32-bit register, from a R_* constant */
>  const char *get_register_name_32(unsigned int reg);
>  
> +uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index);
>  void enable_compat_apic_id_mode(void);
>  
>  #define APIC_DEFAULT_ADDRESS 0xfee00000
> -- 
> 2.1.0
> 

-- 
Eduardo