WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Keir Fraser <keir.xen@xxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Subject: RE: [Xen-devel] [PATCH] x86/mce: CPU notifiers must not be registered a second time during resume
From: "Liu, Jinsong" <jinsong.liu@xxxxxxxxx>
Date: Sat, 19 Mar 2011 23:53:23 +0800
Accept-language: en-US
Acceptlanguage: en-US
Cc:
Delivery-date: Sat, 19 Mar 2011 08:54:11 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <C9A92E38.150A6%keir.xen@xxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <4D8383340200007800037412@xxxxxxxxxxxxxxxxxx> <C9A92E38.150A6%keir.xen@xxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
Thread-index: Acvlg0fnfftgVGNkgUuqSd8PG8dvfgAx5ZXQ
Thread-topic: [Xen-devel] [PATCH] x86/mce: CPU notifiers must not be registered a second time during resume
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