Re: [Qemu-devel] [RFC 11/18] target-i386: cpu_x86_init: allow APIC ID to be set by caller

[Igor Mammedov]

On Wed,  3 Oct 2012 10:29:07 -0300
Eduardo Habkost <address@hidden> wrote:

> This allows callers of cpu_x86_init() to override the APIC ID, in case
> it needs to build a specific cores/threads topology.
> 
> Because *-user doesn't have any concept of CPU topology, we do not
> require every caller to specify an APIC ID. So a negative value will
> indicate that the CPU index can be used as APIC ID, and *-user will use
> that mode.
> 
> By now, all the callers use the default behavior, that's using the CPU
> index as APIC ID, but later the PC code will be changed to calculate the
> APIC IDs according to CPU topology.
> 
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
>  hw/pc.c              |  2 +-
>  target-i386/cpu.h    |  4 ++--
>  target-i386/helper.c | 15 +++++++++++++--
>  3 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 3cf56de..0915a9b 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -908,7 +908,7 @@ static X86CPU *pc_new_cpu(PC *pc, const char *cpu_model)
>      X86CPU *cpu;
>      CPUX86State *env;
>  
> -    cpu = cpu_x86_init(cpu_model);
> +    cpu = cpu_x86_init(cpu_model, -1);
>      if (cpu == NULL) {
>          fprintf(stderr, "Unable to find x86 CPU definition\n");
>          exit(1);
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index f37e80b..3b6445c 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -874,7 +874,7 @@ typedef struct CPUX86State {
>  
>  #include "cpu-qom.h"
>  
> -X86CPU *cpu_x86_init(const char *cpu_model);
> +X86CPU *cpu_x86_init(const char *cpu_model, long apic_id);
>  int cpu_x86_exec(CPUX86State *s);
>  void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf);
>  void x86_cpudef_setup(void);
> @@ -1050,7 +1050,7 @@ uint64_t cpu_get_tsc(CPUX86State *env);
>  
>  static inline CPUX86State *cpu_init(const char *cpu_model)
>  {
> -    X86CPU *cpu = cpu_x86_init(cpu_model);
> +    X86CPU *cpu = cpu_x86_init(cpu_model, -1);
>      if (cpu == NULL) {
>          return NULL;
>      }
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 70a9f72..423d8da 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1239,7 +1239,15 @@ int cpu_x86_get_descr_debug(CPUX86State *env,
> unsigned int selector, return 1;
>  }
>  
> -X86CPU *cpu_x86_init(const char *cpu_model)
> +/**
> + * cpu_x86_init:
> + *
> + * Creates and initializes a X86CPU object.
> + *
> + * @apic_id: sets a specific APIC ID for the CPU. If negative, the CPU
> index
> + *           will be used as APIC ID.
> + */
> +X86CPU *cpu_x86_init(const char *cpu_model, long apic_id)
Risking to being slapped again:

We are trying to move from cpu_model and probably cpu_init itself,
once CPU could be created & initialized just as any QOM object. So
maybe cpu_x86_init() could be left alone just as it is now for
using in *-linux-user and softmmu part could done in pc_cpus_init().

That will allow to play with machine specific properties only for softmmu
instead of additional ifdef-enery in cpu_x86_init() since apic is not part
of *-linux-user.
Additionally it would allow to set parent of CPU at board level right
after CPU is created and at realize time set back-link<> from apic to
CPU.

>  {
>      X86CPU *cpu;
>      CPUX86State *env;
> @@ -1249,7 +1257,10 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>      env = &cpu->env;
>      env->cpu_model_str = cpu_model;
>  
> -    if (cpu_x86_register(cpu, cpu_model, env->cpu_index) < 0) {
> +    if (apic_id < 0) {
> +        apic_id = env->cpu_index;
> +    }
> +    if (cpu_x86_register(cpu, cpu_model, apic_id) < 0) {
>          object_delete(OBJECT(cpu));
>          return NULL;
>      }