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

["Liu, Jinsong" <jinsong.liu@xxxxxxxxx>]

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@xxxxxxxxxx> 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@xxxxxxxxxx>
>> 
>> --- 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@xxxxxxxxxxxxxxxxxxx
>> http://lists.xensource.com/xen-devel


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