Re: [PATCH 1/2] x86: identifying the boot cpu

[Andrew Cooper <andrew.cooper3@citrix.com>]

Please use git-send-email and thread your patch series properly.  You
are still submitting 3 independent emails.

On 13/03/15 18:07, Uma Sharma wrote:
> Provide helpers to access the socket and core IDs, resulting from
> identification phase.
> Initialize socket and core ID to -1 i.e invalid instead of 0. Having
> that field in all elements set to 0 would induce credit2 to think that
> the pCPU have already been initialized, and that all are on socket 0
> in case of credit2 socket scheduler and on core 0 in case of credit2
> core scheduler.
>
> Signed-off-by: Uma Sharma <uma.sharma523@gmail.com>

You have not addressed the cpu onlining/offlining problem which Jan
asked you about.

Furthermore, you don't make any justification as to why it is safe to
change the defaults under all the other users of cpu_data.

> ---
>  xen/arch/x86/setup.c            | 7 +++++--
>  xen/arch/x86/smpboot.c          | 3 ++-
>  xen/include/asm-x86/processor.h | 2 ++
>  3 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 7593533..251840f 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1259,6 +1259,11 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>      microcode_grab_module(module_map, mbi, bootstrap_map);
>  
>      timer_init();
> +    /*
> +     * Identify the boot CPU, in case the scheduler initialization
> +     * needs to know about it (e.g., topology, etc.)
> +     */

I wouldn't bother with the comment here.  This is a long line of
initialisation, and everyone has to pay close attention to the order of
things.

~Andrew

> +    identify_cpu(&boot_cpu_data);
>  
>      init_idle_domain();
>  
> @@ -1270,8 +1275,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      arch_init_memory();
>  
> -    identify_cpu(&boot_cpu_data);
> -
>      if ( cpu_has_fxsr )
>          set_in_cr4(X86_CR4_OSFXSR);
>      if ( cpu_has_xmm )
> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> index 314e253..ac8c4c8 100644
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -59,7 +59,8 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_core_mask);
>  cpumask_t cpu_online_map __read_mostly;
>  EXPORT_SYMBOL(cpu_online_map);
>  
> -struct cpuinfo_x86 cpu_data[NR_CPUS];
> +struct cpuinfo_x86 cpu_data[NR_CPUS] =
> +        { [0 ... NR_CPUS-1] = { .phys_proc_id = -1, .cpu_core_id = -1 } };
>  
>  u32 x86_cpu_to_apicid[NR_CPUS] __read_mostly =
>  	{ [0 ... NR_CPUS-1] = BAD_APICID };
> diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
> index 87d80ff..6ec9588 100644
> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -214,7 +214,9 @@ extern void detect_extended_topology(struct cpuinfo_x86 *c);
>  
>  extern void detect_ht(struct cpuinfo_x86 *c);
>  
> +#define boot_cpu_to_core()   (boot_cpu_data.cpu_core_id)
>  #define cpu_to_core(_cpu)   (cpu_data[_cpu].cpu_core_id)
> +#define boot_cpu_to_socket() (boot_cpu_data.phys_proc_id)
>  #define cpu_to_socket(_cpu) (cpu_data[_cpu].phys_proc_id)
>  
>  unsigned int apicid_to_socket(unsigned int);