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

[Keir Fraser <keir.xen@gmail.com>]

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
>