RE: [PATCH] x86/mce: CPU notifiers must not be registered a second time during resume

["Jan Beulich" <JBeulich@novell.com>]

>>> On 19.03.11 at 16:53, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
> Thanks Jan and Keir!
> Sorry for late check email at weekend.
> 
> I think a while, how about following solution (draft scheme):
> -----------------------------------------------
> 1. at mce_intel.c, keep old intel_mce_initcall() func (it has been removed 
> at c/s 22964), and do
> 
> --- a/xen/arch/x86/cpu/mcheck/mce_intel.c	Fri Feb 25 01:26:01 2011 +0800
> +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c	Mon Feb 28 19:19:20 2011 +0800
>  static int __init intel_mce_initcall(void)
>  {
> +    void *hcpu = (void *)(long)smp_processor_id();
> +    cpu_callback(&cpu_nfb, CPU_UP_PREPARE, hcpu);
>      register_cpu_notifier(&cpu_nfb);
>      return 0;
>  }

This one may be an option, though it then is unclear to me why
you removed it in 22694.

> -----------------------------------------------
> 2. at setup.c, do_presmp_initcalls() at little bit earlier
> 
> diff -r 1a364b17d66a xen/arch/x86/setup.c
> --- a/xen/arch/x86/setup.c	Fri Feb 25 01:26:01 2011 +0800
> +++ b/xen/arch/x86/setup.c	Mon Feb 28 19:19:20 2011 +0800
> @@ -1203,6 +1203,8 @@ void __init __start_xen(unsigned long mb
>  
>      arch_init_memory();
>  
> +    do_presmp_initcalls();
> +
>      identify_cpu(&boot_cpu_data);
>      if ( cpu_has_fxsr )
>          set_in_cr4(X86_CR4_OSFXSR);
> @@ -1235,8 +1237,6 @@ void __init __start_xen(unsigned long mb
>      initialize_keytable();
>  
>      console_init_postirq();
> -
> -    do_presmp_initcalls();
>  
>      for_each_present_cpu ( i )
> -----------------------------------------------

This one I don't like at all - pre-SMP init calls ought to be allowed
to assume identify_cpu() was run.

Further, I wouldn't really like to see anything sufficiently generic
being pushed ahead of (the final step of) console initialization.

> How do you think? it don't need to add bsp para to mcheck_int() as
> -void mcheck_init(struct cpuinfo_x86 *c)
> +void mcheck_init(struct cpuinfo_x86 *c, bool_t bsp)
> 
> BTW, it can go further to unify cpu0 and cpux, like:
> ----------------------------------------------
> diff -r 682880e909db xen/arch/x86/setup.c
> --- a/xen/arch/x86/setup.c	Mon Feb 28 09:17:40 2011 +0800
> +++ b/xen/arch/x86/setup.c	Mon Feb 28 09:53:54 2011 +0800
> @@ -1205,7 +1205,8 @@ void __init __start_xen(unsigned long mb
>  
>      do_presmp_initcalls();
>  
> -    identify_cpu(&boot_cpu_data);
> +    smp_prepare_cpus(max_cpus);
> +    boot_cpu_data = cpu_data[0];
>      if ( cpu_has_fxsr )
>          set_in_cr4(X86_CR4_OSFXSR);
>      if ( cpu_has_xmm )
> @@ -1221,8 +1222,6 @@ void __init __start_xen(unsigned long mb
>          max_cpus = 0;
>  
>      iommu_setup();    /* setup iommu if available */
> -
> -    smp_prepare_cpus(max_cpus);
>  
>      spin_debug_enable();
>  

Here as well I'm not sure this wouldn't have any bad side effects.

Overall, trying to also answer Keir's subsequent question, I don't
think this gets us any closer to Linux - I think it'd be more of the
opposite.

Jan