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

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

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3671 bytes --]

While c/s 22964:f71212f712fd and 23051:93c864c16ab1 fixed issues with
CPU onlining, they introduced a problem with resume: mcheck_init() is
also being called on that path, and hence checking whether it's running
on CPU 0, which is generally not a really good thing, is particularly
inappropriate here.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -187,7 +187,7 @@ static int enter_state(u32 state)
 
     device_power_up();
 
-    mcheck_init(&boot_cpu_data);
+    mcheck_init(&boot_cpu_data, 0);
     write_cr4(cr4);
 
     printk(XENLOG_INFO "Finishing wakeup from ACPI S%d state.\n", state);
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -431,19 +431,13 @@ void __cpuinit identify_cpu(struct cpuin
 		/* AND the already accumulated flags with these */
 		for ( i = 0 ; i < NCAPINTS ; i++ )
 			boot_cpu_data.x86_capability[i] &= c->x86_capability[i];
-	}
-
-	/* Init Machine Check Exception if available. */
-	mcheck_init(c);
 
-#if 0
-	if (c == &boot_cpu_data)
-		sysenter_setup();
-	enable_sep_cpu();
-#endif
+		mcheck_init(c, 0);
+	} else {
+		mcheck_init(c, 1);
 
-	if (c == &boot_cpu_data)
 		mtrr_bp_init();
+	}
 }
 
 /* cpuid returns the value latched in the HW at reset, not the APIC ID
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -793,7 +793,7 @@ static struct notifier_block cpu_nfb = {
 };
 
 /* This has to be run for each processor */
-void mcheck_init(struct cpuinfo_x86 *c)
+void mcheck_init(struct cpuinfo_x86 *c, bool_t bsp)
 {
     enum mcheck_type inited = mcheck_none;
 
@@ -822,7 +822,7 @@ void mcheck_init(struct cpuinfo_x86 *c)
         switch (c->x86) {
         case 6:
         case 15:
-            inited = intel_mcheck_init(c);
+            inited = intel_mcheck_init(c, bsp);
             break;
         }
         break;
@@ -844,7 +844,7 @@ void mcheck_init(struct cpuinfo_x86 *c)
     /* Turn on MCE now */
     set_in_cr4(X86_CR4_MCE);
 
-    if ( smp_processor_id() == 0 )
+    if ( bsp )
     {
         /* Early MCE initialisation for BSP. */
         if ( cpu_poll_bankmask_alloc(0) )
--- a/xen/arch/x86/cpu/mcheck/mce.h
+++ b/xen/arch/x86/cpu/mcheck/mce.h
@@ -42,7 +42,7 @@ enum mcheck_type amd_k7_mcheck_init(stru
 enum mcheck_type amd_k8_mcheck_init(struct cpuinfo_x86 *c);
 enum mcheck_type amd_f10_mcheck_init(struct cpuinfo_x86 *c);
 
-enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c);
+enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c, bool_t bsp);
 
 void intel_mcheck_timer(struct cpuinfo_x86 *c);
 void mce_intel_feature_init(struct cpuinfo_x86 *c);
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -1297,9 +1297,9 @@ static struct notifier_block cpu_nfb = {
 };
 
 /* p4/p6 family have similar MCA initialization process */
-enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c)
+enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c, bool_t bsp)
 {
-    if ( smp_processor_id() == 0 )
+    if ( bsp )
     {
         /* Early MCE initialisation for BSP. */
         if ( cpu_mcabank_alloc(0) )
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -555,7 +555,7 @@ void compat_show_guest_stack(struct vcpu
 extern void mtrr_ap_init(void);
 extern void mtrr_bp_init(void);
 
-void mcheck_init(struct cpuinfo_x86 *c);
+void mcheck_init(struct cpuinfo_x86 *c, bool_t bsp);
 
 #define DECLARE_TRAP_HANDLER(_name)                     \
 asmlinkage void _name(void);                            \




[-- Attachment #2: x86-mcheck_init-bsp.patch --]
[-- Type: text/plain, Size: 3665 bytes --]

While c/s 22964:f71212f712fd and 23051:93c864c16ab1 fixed issues with
CPU onlining, they introduced a problem with resume: mcheck_init() is
also being called on that path, and hence checking whether it's running
on CPU 0, which is generally not a really good thing, is particularly
inappropriate here.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -187,7 +187,7 @@ static int enter_state(u32 state)
 
     device_power_up();
 
-    mcheck_init(&boot_cpu_data);
+    mcheck_init(&boot_cpu_data, 0);
     write_cr4(cr4);
 
     printk(XENLOG_INFO "Finishing wakeup from ACPI S%d state.\n", state);
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -431,19 +431,13 @@ void __cpuinit identify_cpu(struct cpuin
 		/* AND the already accumulated flags with these */
 		for ( i = 0 ; i < NCAPINTS ; i++ )
 			boot_cpu_data.x86_capability[i] &= c->x86_capability[i];
-	}
-
-	/* Init Machine Check Exception if available. */
-	mcheck_init(c);
 
-#if 0
-	if (c == &boot_cpu_data)
-		sysenter_setup();
-	enable_sep_cpu();
-#endif
+		mcheck_init(c, 0);
+	} else {
+		mcheck_init(c, 1);
 
-	if (c == &boot_cpu_data)
 		mtrr_bp_init();
+	}
 }
 
 /* cpuid returns the value latched in the HW at reset, not the APIC ID
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -793,7 +793,7 @@ static struct notifier_block cpu_nfb = {
 };
 
 /* This has to be run for each processor */
-void mcheck_init(struct cpuinfo_x86 *c)
+void mcheck_init(struct cpuinfo_x86 *c, bool_t bsp)
 {
     enum mcheck_type inited = mcheck_none;
 
@@ -822,7 +822,7 @@ void mcheck_init(struct cpuinfo_x86 *c)
         switch (c->x86) {
         case 6:
         case 15:
-            inited = intel_mcheck_init(c);
+            inited = intel_mcheck_init(c, bsp);
             break;
         }
         break;
@@ -844,7 +844,7 @@ void mcheck_init(struct cpuinfo_x86 *c)
     /* Turn on MCE now */
     set_in_cr4(X86_CR4_MCE);
 
-    if ( smp_processor_id() == 0 )
+    if ( bsp )
     {
         /* Early MCE initialisation for BSP. */
         if ( cpu_poll_bankmask_alloc(0) )
--- a/xen/arch/x86/cpu/mcheck/mce.h
+++ b/xen/arch/x86/cpu/mcheck/mce.h
@@ -42,7 +42,7 @@ enum mcheck_type amd_k7_mcheck_init(stru
 enum mcheck_type amd_k8_mcheck_init(struct cpuinfo_x86 *c);
 enum mcheck_type amd_f10_mcheck_init(struct cpuinfo_x86 *c);
 
-enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c);
+enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c, bool_t bsp);
 
 void intel_mcheck_timer(struct cpuinfo_x86 *c);
 void mce_intel_feature_init(struct cpuinfo_x86 *c);
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -1297,9 +1297,9 @@ static struct notifier_block cpu_nfb = {
 };
 
 /* p4/p6 family have similar MCA initialization process */
-enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c)
+enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c, bool_t bsp)
 {
-    if ( smp_processor_id() == 0 )
+    if ( bsp )
     {
         /* Early MCE initialisation for BSP. */
         if ( cpu_mcabank_alloc(0) )
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -555,7 +555,7 @@ void compat_show_guest_stack(struct vcpu
 extern void mtrr_ap_init(void);
 extern void mtrr_bp_init(void);
 
-void mcheck_init(struct cpuinfo_x86 *c);
+void mcheck_init(struct cpuinfo_x86 *c, bool_t bsp);
 
 #define DECLARE_TRAP_HANDLER(_name)                     \
 asmlinkage void _name(void);                            \

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

[Keir Fraser]

On 18/03/2011 15:07, "Jan Beulich" <JBeulich@novell.com> wrote:

> While c/s 22964:f71212f712fd and 23051:93c864c16ab1 fixed issues with
> CPU onlining, they introduced a problem with resume: mcheck_init() is
> also being called on that path, and hence checking whether it's running
> on CPU 0, which is generally not a really good thing, is particularly
> inappropriate here.

Just have a 'static bool_t early_init_done' or similar in
intel_mcheck_init().

if ( !early_init_done ) {
 BUG_ON(smp_processor_id() != 0);
 ...
 early_init_done = 1;
}

It's clearer anyway -- we're simply protecting one-time-only early-boot-time
initialisation stuff.

 -- Keir

> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> 
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -187,7 +187,7 @@ static int enter_state(u32 state)
>  
>      device_power_up();
>  
> -    mcheck_init(&boot_cpu_data);
> +    mcheck_init(&boot_cpu_data, 0);
>      write_cr4(cr4);
>  
>      printk(XENLOG_INFO "Finishing wakeup from ACPI S%d state.\n", state);
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -431,19 +431,13 @@ void __cpuinit identify_cpu(struct cpuin
> /* AND the already accumulated flags with these */
> for ( i = 0 ; i < NCAPINTS ; i++ )
> boot_cpu_data.x86_capability[i] &= c->x86_capability[i];
> - }
> -
> - /* Init Machine Check Exception if available. */
> - mcheck_init(c);
>  
> -#if 0
> - if (c == &boot_cpu_data)
> -  sysenter_setup();
> - enable_sep_cpu();
> -#endif
> +  mcheck_init(c, 0);
> + } else {
> +  mcheck_init(c, 1);
>  
> - if (c == &boot_cpu_data)
> mtrr_bp_init();
> + }
>  }
>  
>  /* cpuid returns the value latched in the HW at reset, not the APIC ID
> --- a/xen/arch/x86/cpu/mcheck/mce.c
> +++ b/xen/arch/x86/cpu/mcheck/mce.c
> @@ -793,7 +793,7 @@ static struct notifier_block cpu_nfb = {
>  };
>  
>  /* This has to be run for each processor */
> -void mcheck_init(struct cpuinfo_x86 *c)
> +void mcheck_init(struct cpuinfo_x86 *c, bool_t bsp)
>  {
>      enum mcheck_type inited = mcheck_none;
>  
> @@ -822,7 +822,7 @@ void mcheck_init(struct cpuinfo_x86 *c)
>          switch (c->x86) {
>          case 6:
>          case 15:
> -            inited = intel_mcheck_init(c);
> +            inited = intel_mcheck_init(c, bsp);
>              break;
>          }
>          break;
> @@ -844,7 +844,7 @@ void mcheck_init(struct cpuinfo_x86 *c)
>      /* Turn on MCE now */
>      set_in_cr4(X86_CR4_MCE);
>  
> -    if ( smp_processor_id() == 0 )
> +    if ( bsp )
>      {
>          /* Early MCE initialisation for BSP. */
>          if ( cpu_poll_bankmask_alloc(0) )
> --- a/xen/arch/x86/cpu/mcheck/mce.h
> +++ b/xen/arch/x86/cpu/mcheck/mce.h
> @@ -42,7 +42,7 @@ enum mcheck_type amd_k7_mcheck_init(stru
>  enum mcheck_type amd_k8_mcheck_init(struct cpuinfo_x86 *c);
>  enum mcheck_type amd_f10_mcheck_init(struct cpuinfo_x86 *c);
>  
> -enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c);
> +enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c, bool_t bsp);
>  
>  void intel_mcheck_timer(struct cpuinfo_x86 *c);
>  void mce_intel_feature_init(struct cpuinfo_x86 *c);
> --- a/xen/arch/x86/cpu/mcheck/mce_intel.c
> +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
> @@ -1297,9 +1297,9 @@ static struct notifier_block cpu_nfb = {
>  };
>  
>  /* p4/p6 family have similar MCA initialization process */
> -enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c)
> +enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c, bool_t bsp)
>  {
> -    if ( smp_processor_id() == 0 )
> +    if ( bsp )
>      {
>          /* Early MCE initialisation for BSP. */
>          if ( cpu_mcabank_alloc(0) )
> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -555,7 +555,7 @@ void compat_show_guest_stack(struct vcpu
>  extern void mtrr_ap_init(void);
>  extern void mtrr_bp_init(void);
>  
> -void mcheck_init(struct cpuinfo_x86 *c);
> +void mcheck_init(struct cpuinfo_x86 *c, bool_t bsp);
>  
>  #define DECLARE_TRAP_HANDLER(_name)                     \
>  asmlinkage void _name(void);                            \
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

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

[Jan Beulich]

>>> On 18.03.11 at 16:43, Keir Fraser <keir.xen@gmail.com> wrote:
> On 18/03/2011 15:07, "Jan Beulich" <JBeulich@novell.com> wrote:
> 
>> While c/s 22964:f71212f712fd and 23051:93c864c16ab1 fixed issues with
>> CPU onlining, they introduced a problem with resume: mcheck_init() is
>> also being called on that path, and hence checking whether it's running
>> on CPU 0, which is generally not a really good thing, is particularly
>> inappropriate here.
> 
> Just have a 'static bool_t early_init_done' or similar in
> intel_mcheck_init().

And another in mcheck_init(). If the proliferates, an alternative I
would like a little better would be to just have a global variable (e.g.
extending early_boot).

> if ( !early_init_done ) {
>  BUG_ON(smp_processor_id() != 0);
>  ...
>  early_init_done = 1;
> }
> 
> It's clearer anyway -- we're simply protecting one-time-only early-boot-time
> initialisation stuff.

What's wrong with doing the protection by passing down the
necessary information?

Jan

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

[Keir Fraser]

On 18/03/2011 16:35, "Jan Beulich" <JBeulich@novell.com> wrote:

>> if ( !early_init_done ) {
>>  BUG_ON(smp_processor_id() != 0);
>>  ...
>>  early_init_done = 1;
>> }
>> 
>> It's clearer anyway -- we're simply protecting one-time-only early-boot-time
>> initialisation stuff.
> 
> What's wrong with doing the protection by passing down the
> necessary information?

Hm, yes, fair enough, I'll apply it.

 -- Keir

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

[Liu, Jinsong]

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;
 }
-----------------------------------------------
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 )
-----------------------------------------------

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();
 
diff -r 682880e909db xen/arch/x86/smpboot.c
--- a/xen/arch/x86/smpboot.c	Mon Feb 28 09:17:40 2011 +0800
+++ b/xen/arch/x86/smpboot.c	Mon Feb 28 09:53:54 2011 +0800
@@ -88,9 +88,7 @@ static void smp_store_cpu_info(int id)
 {
     struct cpuinfo_x86 *c = cpu_data + id;
 
-    *c = boot_cpu_data;
-    if ( id != 0 )
-        identify_cpu(c);
+    identify_cpu(c);
 
     /* Mask B, Pentium, but not Pentium MMX -- remember it, as it has bugs. */
     if ( (c->x86_vendor == X86_VENDOR_INTEL) &&
-----------------------------------------


Thanks,
Jinsong



Keir Fraser wrote:
> On 18/03/2011 15:07, "Jan Beulich" <JBeulich@novell.com> wrote:
> 
>> While c/s 22964:f71212f712fd and 23051:93c864c16ab1 fixed issues with
>> CPU onlining, they introduced a problem with resume: mcheck_init() is
>> also being called on that path, and hence checking whether it's
>> running on CPU 0, which is generally not a really good thing, is
>> particularly inappropriate here.
> 
> Just have a 'static bool_t early_init_done' or similar in
> intel_mcheck_init().
> 
> if ( !early_init_done ) {
>  BUG_ON(smp_processor_id() != 0);
>  ...
>  early_init_done = 1;
> }
> 
> It's clearer anyway -- we're simply protecting one-time-only
> early-boot-time initialisation stuff.
> 
>  -- Keir
> 
>> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>> 
>> --- a/xen/arch/x86/acpi/power.c
>> +++ b/xen/arch/x86/acpi/power.c
>> @@ -187,7 +187,7 @@ static int enter_state(u32 state)
>> 
>>      device_power_up();
>> 
>> -    mcheck_init(&boot_cpu_data);
>> +    mcheck_init(&boot_cpu_data, 0);
>>      write_cr4(cr4);
>> 
>>      printk(XENLOG_INFO "Finishing wakeup from ACPI S%d state.\n",
>> state); --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -431,19 +431,13 @@ void __cpuinit identify_cpu(struct cpuin
>> /* AND the already accumulated flags with these */
>> for ( i = 0 ; i < NCAPINTS ; i++ )
>> boot_cpu_data.x86_capability[i] &= c->x86_capability[i];
>> - }
>> -
>> - /* Init Machine Check Exception if available. */
>> - mcheck_init(c);
>> 
>> -#if 0
>> - if (c == &boot_cpu_data)
>> -  sysenter_setup();
>> - enable_sep_cpu();
>> -#endif
>> +  mcheck_init(c, 0);
>> + } else {
>> +  mcheck_init(c, 1);
>> 
>> - if (c == &boot_cpu_data)
>> mtrr_bp_init();
>> + }
>>  }
>> 
>>  /* cpuid returns the value latched in the HW at reset, not the APIC
>> ID --- a/xen/arch/x86/cpu/mcheck/mce.c
>> +++ b/xen/arch/x86/cpu/mcheck/mce.c
>> @@ -793,7 +793,7 @@ static struct notifier_block cpu_nfb = {  };
>> 
>>  /* This has to be run for each processor */
>> -void mcheck_init(struct cpuinfo_x86 *c)
>> +void mcheck_init(struct cpuinfo_x86 *c, bool_t bsp)  {
>>      enum mcheck_type inited = mcheck_none;
>> 
>> @@ -822,7 +822,7 @@ void mcheck_init(struct cpuinfo_x86 *c)         
>>          switch (c->x86) { case 6:
>>          case 15:
>> -            inited = intel_mcheck_init(c);
>> +            inited = intel_mcheck_init(c, bsp);
>>              break;
>>          }
>>          break;
>> @@ -844,7 +844,7 @@ void mcheck_init(struct cpuinfo_x86 *c)      /*
>>      Turn on MCE now */ set_in_cr4(X86_CR4_MCE);
>> 
>> -    if ( smp_processor_id() == 0 )
>> +    if ( bsp )
>>      {
>>          /* Early MCE initialisation for BSP. */
>>          if ( cpu_poll_bankmask_alloc(0) )
>> --- a/xen/arch/x86/cpu/mcheck/mce.h
>> +++ b/xen/arch/x86/cpu/mcheck/mce.h
>> @@ -42,7 +42,7 @@ enum mcheck_type amd_k7_mcheck_init(stru
>>  enum mcheck_type amd_k8_mcheck_init(struct cpuinfo_x86 *c);
>>  enum mcheck_type amd_f10_mcheck_init(struct cpuinfo_x86 *c);
>> 
>> -enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c);
>> +enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c, bool_t
>> bsp); 
>> 
>>  void intel_mcheck_timer(struct cpuinfo_x86 *c);
>>  void mce_intel_feature_init(struct cpuinfo_x86 *c);
>> --- a/xen/arch/x86/cpu/mcheck/mce_intel.c
>> +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
>> @@ -1297,9 +1297,9 @@ static struct notifier_block cpu_nfb = {  };
>> 
>>  /* p4/p6 family have similar MCA initialization process */
>> -enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c)
>> +enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c, bool_t
>> bsp)  { -    if ( smp_processor_id() == 0 )
>> +    if ( bsp )
>>      {
>>          /* Early MCE initialisation for BSP. */
>>          if ( cpu_mcabank_alloc(0) )
>> --- a/xen/include/asm-x86/processor.h
>> +++ b/xen/include/asm-x86/processor.h
>> @@ -555,7 +555,7 @@ void compat_show_guest_stack(struct vcpu
>>  extern void mtrr_ap_init(void);
>>  extern void mtrr_bp_init(void);
>> 
>> -void mcheck_init(struct cpuinfo_x86 *c);
>> +void mcheck_init(struct cpuinfo_x86 *c, bool_t bsp);
>> 
>>  #define DECLARE_TRAP_HANDLER(_name)                     \
>>  asmlinkage void _name(void);                            \
>> 
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel

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

[Keir Fraser]

I checked in Jan's patch as it is more suitable for 4.1 anyway, compared
with moving do_presmp_initcalls(). We could consider replacing it with your
approach below in xen-unstable however. If it makes the ordering of setup
closer to that in Linux, that would probably be a good thing.

 -- Keir

On 19/03/2011 15: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;
>  }
> -----------------------------------------------
> 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 )
> -----------------------------------------------
> 
> 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();
>  
> diff -r 682880e909db xen/arch/x86/smpboot.c
> --- a/xen/arch/x86/smpboot.c Mon Feb 28 09:17:40 2011 +0800
> +++ b/xen/arch/x86/smpboot.c Mon Feb 28 09:53:54 2011 +0800
> @@ -88,9 +88,7 @@ static void smp_store_cpu_info(int id)
>  {
>      struct cpuinfo_x86 *c = cpu_data + id;
>  
> -    *c = boot_cpu_data;
> -    if ( id != 0 )
> -        identify_cpu(c);
> +    identify_cpu(c);
>  
>      /* Mask B, Pentium, but not Pentium MMX -- remember it, as it has bugs.
> */
>      if ( (c->x86_vendor == X86_VENDOR_INTEL) &&
> -----------------------------------------
> 
> 
> Thanks,
> Jinsong
> 
> 
> 
> Keir Fraser wrote:
>> On 18/03/2011 15:07, "Jan Beulich" <JBeulich@novell.com> wrote:
>> 
>>> While c/s 22964:f71212f712fd and 23051:93c864c16ab1 fixed issues with
>>> CPU onlining, they introduced a problem with resume: mcheck_init() is
>>> also being called on that path, and hence checking whether it's
>>> running on CPU 0, which is generally not a really good thing, is
>>> particularly inappropriate here.
>> 
>> Just have a 'static bool_t early_init_done' or similar in
>> intel_mcheck_init().
>> 
>> if ( !early_init_done ) {
>>  BUG_ON(smp_processor_id() != 0);
>>  ...
>>  early_init_done = 1;
>> }
>> 
>> It's clearer anyway -- we're simply protecting one-time-only
>> early-boot-time initialisation stuff.
>> 
>>  -- Keir
>> 
>>> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>>> 
>>> --- a/xen/arch/x86/acpi/power.c
>>> +++ b/xen/arch/x86/acpi/power.c
>>> @@ -187,7 +187,7 @@ static int enter_state(u32 state)
>>> 
>>>      device_power_up();
>>> 
>>> -    mcheck_init(&boot_cpu_data);
>>> +    mcheck_init(&boot_cpu_data, 0);
>>>      write_cr4(cr4);
>>> 
>>>      printk(XENLOG_INFO "Finishing wakeup from ACPI S%d state.\n",
>>> state); --- a/xen/arch/x86/cpu/common.c
>>> +++ b/xen/arch/x86/cpu/common.c
>>> @@ -431,19 +431,13 @@ void __cpuinit identify_cpu(struct cpuin
>>> /* AND the already accumulated flags with these */
>>> for ( i = 0 ; i < NCAPINTS ; i++ )
>>> boot_cpu_data.x86_capability[i] &= c->x86_capability[i];
>>> - }
>>> -
>>> - /* Init Machine Check Exception if available. */
>>> - mcheck_init(c);
>>> 
>>> -#if 0
>>> - if (c == &boot_cpu_data)
>>> -  sysenter_setup();
>>> - enable_sep_cpu();
>>> -#endif
>>> +  mcheck_init(c, 0);
>>> + } else {
>>> +  mcheck_init(c, 1);
>>> 
>>> - if (c == &boot_cpu_data)
>>> mtrr_bp_init();
>>> + }
>>>  }
>>> 
>>>  /* cpuid returns the value latched in the HW at reset, not the APIC
>>> ID --- a/xen/arch/x86/cpu/mcheck/mce.c
>>> +++ b/xen/arch/x86/cpu/mcheck/mce.c
>>> @@ -793,7 +793,7 @@ static struct notifier_block cpu_nfb = {  };
>>> 
>>>  /* This has to be run for each processor */
>>> -void mcheck_init(struct cpuinfo_x86 *c)
>>> +void mcheck_init(struct cpuinfo_x86 *c, bool_t bsp)  {
>>>      enum mcheck_type inited = mcheck_none;
>>> 
>>> @@ -822,7 +822,7 @@ void mcheck_init(struct cpuinfo_x86 *c)
>>>          switch (c->x86) { case 6:
>>>          case 15:
>>> -            inited = intel_mcheck_init(c);
>>> +            inited = intel_mcheck_init(c, bsp);
>>>              break;
>>>          }
>>>          break;
>>> @@ -844,7 +844,7 @@ void mcheck_init(struct cpuinfo_x86 *c)      /*
>>>      Turn on MCE now */ set_in_cr4(X86_CR4_MCE);
>>> 
>>> -    if ( smp_processor_id() == 0 )
>>> +    if ( bsp )
>>>      {
>>>          /* Early MCE initialisation for BSP. */
>>>          if ( cpu_poll_bankmask_alloc(0) )
>>> --- a/xen/arch/x86/cpu/mcheck/mce.h
>>> +++ b/xen/arch/x86/cpu/mcheck/mce.h
>>> @@ -42,7 +42,7 @@ enum mcheck_type amd_k7_mcheck_init(stru
>>>  enum mcheck_type amd_k8_mcheck_init(struct cpuinfo_x86 *c);
>>>  enum mcheck_type amd_f10_mcheck_init(struct cpuinfo_x86 *c);
>>> 
>>> -enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c);
>>> +enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c, bool_t
>>> bsp); 
>>> 
>>>  void intel_mcheck_timer(struct cpuinfo_x86 *c);
>>>  void mce_intel_feature_init(struct cpuinfo_x86 *c);
>>> --- a/xen/arch/x86/cpu/mcheck/mce_intel.c
>>> +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
>>> @@ -1297,9 +1297,9 @@ static struct notifier_block cpu_nfb = {  };
>>> 
>>>  /* p4/p6 family have similar MCA initialization process */
>>> -enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c)
>>> +enum mcheck_type intel_mcheck_init(struct cpuinfo_x86 *c, bool_t
>>> bsp)  { -    if ( smp_processor_id() == 0 )
>>> +    if ( bsp )
>>>      {
>>>          /* Early MCE initialisation for BSP. */
>>>          if ( cpu_mcabank_alloc(0) )
>>> --- a/xen/include/asm-x86/processor.h
>>> +++ b/xen/include/asm-x86/processor.h
>>> @@ -555,7 +555,7 @@ void compat_show_guest_stack(struct vcpu
>>>  extern void mtrr_ap_init(void);
>>>  extern void mtrr_bp_init(void);
>>> 
>>> -void mcheck_init(struct cpuinfo_x86 *c);
>>> +void mcheck_init(struct cpuinfo_x86 *c, bool_t bsp);
>>> 
>>>  #define DECLARE_TRAP_HANDLER(_name)                     \
>>>  asmlinkage void _name(void);                            \
>>> 
>>> 
>>> 
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xensource.com
>>> http://lists.xensource.com/xen-devel
> 

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

[Jan Beulich]

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

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

[Liu, Jinsong]

Jan Beulich wrote:
>>>> 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.

It would alloc redundant mcabanks when using AMD platform.

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

Agree.

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

Reasonable to me, move do_presmp_initcall aheah is of some risk.
Let's keep current way.

Thanks,
Jinsong