Jan Beulich
2011-Mar-18 15:07 UTC
[Xen-devel] [PATCH] x86/mce: CPU notifiers must not be registered a second time during resume
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); \
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Keir Fraser
2011-Mar-18 15:43 UTC
Re: [Xen-devel] [PATCH] x86/mce: CPU notifiers must not be registered a second time during resume
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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Mar-18 16:35 UTC
Re: [Xen-devel] [PATCH] x86/mce: CPU notifiers must not be registered a second time during resume
>>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2011-Mar-18 17:11 UTC
Re: [Xen-devel] [PATCH] x86/mce: CPU notifiers must not be registered a second time during resume
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Liu, Jinsong
2011-Mar-19 15:53 UTC
RE: [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@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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Keir Fraser
2011-Mar-19 22:20 UTC
Re: [Xen-devel] [PATCH] x86/mce: CPU notifiers must not be registered a second time during resume
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 >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Mar-21 08:22 UTC
RE: [Xen-devel] [PATCH] x86/mce: CPU notifiers must not be registered a second time during resume
>>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Liu, Jinsong
2011-Mar-21 13:18 UTC
RE: [Xen-devel] [PATCH] x86/mce: CPU notifiers must not be registered a second time during resume
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. > > JanReasonable to me, move do_presmp_initcall aheah is of some risk. Let''s keep current way. Thanks, Jinsong _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel