[RFC,v0,8/9] target-i386: Set apic_id during CPU initfn

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.

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.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/i386/pc.c      | 33 ---------------------------------
 target-i386/cpu.c | 37 +++++++++++++++++++++++++++++++++++--
 target-i386/cpu.h |  1 +
 3 files changed, 36 insertions(+), 35 deletions(-)

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 <bharata@linux.vnet.ibm.com>
> ---
>  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
>

On Mon, Dec 14, 2015 at 03:44:06PM -0200, Eduardo Habkost wrote:
> 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?

There is a check in x86_cpu_realizefn() to see if apic_id has been
initialized properly. Hence I thought x86 target will require apic_id
to have been initialized before CPU realization and that is what
the existing code does via pc_cpus_init() and pc_new_cpu(). i.e.,
apic_id property is set before setting the realize property to true.
However...

> 
> > 
> > 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.

... with the current patchset, I just experimented now by moving the setting
of apic_id to x86_cpu_realizefn() and things work just fine. I was in fact
pleasantly surprised to see that I could hot add a cpu core by hot plugging
the cpu-core device on x86 too.

> 
> 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.

Right, I have already moved cpu_exec_init() call to realizefn for PowerPC.

Regards,
Bharata.