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

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

Jan Beulich wrote:
>>>> On 19.03.11 at 16:53, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx> 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


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