MTRR suspend/resume cleanup
PATCH 1/1 MTRR suspend/resume cleanup
Hi,
There has been some discuss about solving the SMP MTRR suspend/resume breakage, but I didn't find a patch for it. This is an intent for it. The basic idea is moving mtrr initializing into cpu_identify for all APs (so it works for cpu hotplug). For BP, restore_processor_state is responsible for restoring MTRR.
Thanks,
Shaohua
---
linux-2.6.12-mm1-root/arch/i386/kernel/cpu/common.c | 5 linux-2.6.12-mm1-root/arch/i386/kernel/cpu/mtrr/generic.c | 22 ++-- linux-2.6.12-mm1-root/arch/i386/kernel/cpu/mtrr/main.c | 72 +++++++++----- linux-2.6.12-mm1-root/arch/i386/kernel/cpu/mtrr/mtrr.h | 1 linux-2.6.12-mm1-root/arch/i386/power/cpu.c | 1 linux-2.6.12-mm1-root/arch/x86_64/kernel/setup.c | 4 linux-2.6.12-mm1-root/arch/x86_64/kernel/suspend.c | 1 linux-2.6.12-mm1-root/include/asm-i386/processor.h | 8 + linux-2.6.12-mm1-root/include/asm-x86_64/proto.h | 7 + 9 files changed, 87 insertions(+), 34 deletions(-)
arch/i386/kernel/cpu/mtrr/main.c renamed from arch/i386/kernel/cpu/mtrr/main.c~mtrr_cpuhotplug_cleanup +50 -22
@@ -332,6 +332,7 @@ int mtrr_add_page(unsigned long base, un error = -EINVAL ; +lock_cpu_hotplug ();/* Search for existing MTRR */ down (&main_lock);for (i =0 ; i < num_var_ranges; ++i) {
@@ -372,6 +373,7 @@ int mtrr_add_page(unsigned long base, un error = i; out:up (&main_lock); +unlock_cpu_hotplug ();return error; }
@@ -461,6 +463,7 @@ int mtrr_del_page(int reg, unsigned longreturn -ENXIO ; max = num_var_ranges; +lock_cpu_hotplug ();down (&main_lock);if (reg <0 ) {/* Search for existing MTRR */
@@ -501,6 +504,7 @@ int mtrr_del_page(int reg, unsigned long error = reg; out:up (&main_lock); +unlock_cpu_hotplug ();return error; }/**
@@ -544,21 +548,9 @@ static void __init init_ifs(void)centaur_init_mtrr (); } -static void __init init_other_cpus (void ) -{ -if (use_intel ()) -get_mtrr_state (); - -/* bring up the other processors */ -set_mtrr (~0U ,0 ,0 ,0 ); - -if (use_intel ()) { -finalize_mtrr_state (); -mtrr_state_warn (); - } -} - - +/* The suspend/resume methods are only for CPU without MTRR. CPU using generic +* MTRR driver doesn't require this +*/ struct mtrr_value {mtrr_type ltype;unsigned long lbase;
@@ -611,13 +603,13 @@ static struct sysdev_driver mtrr_sysdev_/** -* mtrr_init - initialize mtrrs on the boot CPU +* mtrr_bp_init - initialize mtrrs on the boot CPU * * This needs to be called early; before any of the other CPUs are * initialized (i.e. before smp_init()). * */ -static int __init mtrr_init (void ) +int __init mtrr_bp_init (void ) {init_ifs ();
@@ -674,12 +666,48 @@ static int __init mtrr_init(void)if (mtrr_if) {set_num_var_ranges ();init_table (); -init_other_cpus (); - -return sysdev_driver_register (&cpu_sysdev_class, - &mtrr_sysdev_driver); +if (use_intel ()) +get_mtrr_state (); }return -ENXIO ; } -subsys_initcall (mtrr_init); +int mtrr_ap_init (void ) +{ +unsigned long flags; + +if (!mtrr_if || !use_intel ()) +return 0 ; +/* +* Ideally we should hold main_lock here to avoid mtrr entries changed, +* but this routine will be called in cpu boot time, holding the lock +* breaks it. This routine is called in two cases: 1.very earily time +* of software resume, when there absolutely isn't mtrr entry changes; +* 2.cpu hotadd time. We let mtrr_add/del_page hold cpuhotplug lock to +* prevent mtrr entry changes +*/ +local_irq_save (flags); + + mtrr_if->set_all (); + +local_irq_restore (flags); +return 0 ; +} + +static int __init mtrr_init_finialize (void ) +{ +if (!mtrr_if) +return 0 ; +if (use_intel ()) +mtrr_state_warn (); +else +/* The CPUs haven't MTRR and seemes not support SMP. They have specific +* drivers, we use a tricky method to support suspend/resume for them +* TBD: is there any system with such CPU which supports suspend/resume? +* if no, we should remove the code. +*/ +sysdev_driver_register (&cpu_sysdev_class, + &mtrr_sysdev_driver); +return 0 ; +} +subsys_initcall (mtrr_init_finialize);
arch/i386/kernel/cpu/mtrr/generic.c renamed from arch/i386/kernel/cpu/mtrr/generic.c~mtrr_cpuhotplug_cleanup +11 -11
@@ -67,13 +67,6 @@ void __init get_mtrr_state(void) mtrr_state.enabled = (lo &0xc00 ) >>10 ; } -/* Free resources associated with a struct mtrr_state */ -void __init finalize_mtrr_state (void ) -{ -kfree (mtrr_state.var_ranges); - mtrr_state.var_ranges =NULL ; -} -/* Some BIOS's are fucked and don't set all MTRRs the same! */ void __init mtrr_state_warn (void ) {
@@ -334,6 +327,9 @@ static void generic_set_mtrr(unsigned in*/ {unsigned long flags; +struct mtrr_var_range *vr; + + vr = &mtrr_state.var_ranges[reg];local_irq_save (flags);prepare_set ();
@@ -342,11 +338,15 @@ static void generic_set_mtrr(unsigned in/* The invalid bit is kept in the mask, so we simply clear the relevant mask register to disable a range. */ mtrr_wrmsr (MTRRphysMask_MSR (reg),0 ,0 ); +memset (vr,0 ,sizeof (struct mtrr_var_range )); }else { -mtrr_wrmsr (MTRRphysBase_MSR (reg), base <<PAGE_SHIFT | type, - (base & size_and_mask) >> (32 -PAGE_SHIFT )); -mtrr_wrmsr (MTRRphysMask_MSR (reg), -size <<PAGE_SHIFT |0x800 , - (-size & size_and_mask) >> (32 -PAGE_SHIFT )); + vr->base_lo = base <<PAGE_SHIFT | type; + vr->base_hi = (base & size_and_mask) >> (32 -PAGE_SHIFT ); + vr->mask_lo = -size <<PAGE_SHIFT |0x800 ; + vr->mask_hi = (-size & size_and_mask) >> (32 -PAGE_SHIFT ); + +mtrr_wrmsr (MTRRphysBase_MSR (reg), vr->base_lo, vr->base_hi); +mtrr_wrmsr (MTRRphysMask_MSR (reg), vr->mask_lo, vr->mask_hi); }post_set ();
arch/i386/kernel/cpu/mtrr/mtrr.h renamed from arch/i386/kernel/cpu/mtrr/mtrr.h~mtrr_cpuhotplug_cleanup +0 -1
@@ -91,7 +91,6 @@ extern struct mtrr_ops * mtrr_if;extern unsigned int num_var_ranges; -void finalize_mtrr_state (void );void mtrr_state_warn (void );char *mtrr_attrib_to_str (int x);void mtrr_wrmsr (unsigned ,unsigned ,unsigned );
arch/i386/kernel/cpu/common.c renamed from arch/i386/kernel/cpu/common.c~mtrr_cpuhotplug_cleanup +5 -0
@@ -435,6 +435,11 @@ void __devinit identify_cpu(struct cpuinif (c == &boot_cpu_data)sysenter_setup ();enable_sep_cpu (); + +if (c == &boot_cpu_data) +mtrr_bp_init (); +else +mtrr_ap_init (); }#ifdef CONFIG_X86_HT
include/asm-i386/processor.h renamed from include/asm-i386/processor.h~mtrr_cpuhotplug_cleanup +8 -0
@@ -696,4 +696,12 @@ extern unsigned long boot_option_idle_ovextern void enable_sep_cpu (void );extern int sysenter_setup (void ); +#ifdef CONFIG_MTRR +extern int mtrr_ap_init (void ); +extern int mtrr_bp_init (void ); +#else +#define mtrr_ap_init () +#define mtrr_bp_init () +#endif +#endif /* __ASM_I386_PROCESSOR_H */
arch/i386/power/cpu.c renamed from arch/i386/power/cpu.c~mtrr_cpuhotplug_cleanup +1 -0
@@ -137,6 +137,7 @@ void __restore_processor_state(struct safix_processor_context ();do_fpu_end (); +mtrr_ap_init (); }void restore_processor_state (void )
include/asm-x86_64/proto.h renamed from include/asm-x86_64/proto.h~mtrr_cpuhotplug_cleanup +7 -0
@@ -15,6 +15,13 @@ extern void pda_init(int);extern void early_idt_handler (void );extern void mcheck_init (struct cpuinfo_x86 *c); +#ifdef CONFIG_MTRR +extern int mtrr_ap_init (void ); +extern int mtrr_bp_init (void ); +#else +#define mtrr_ap_init () +#define mtrr_bp_init (); +#endif extern void init_memory_mapping (unsigned long start,unsigned long end);extern void system_call (void );
arch/x86_64/kernel/suspend.c renamed from arch/x86_64/kernel/suspend.c~mtrr_cpuhotplug_cleanup +1 -0
@@ -119,6 +119,7 @@ void __restore_processor_state(struct safix_processor_context ();do_fpu_end (); +mtrr_ap_init (); }void restore_processor_state (void )
arch/x86_64/kernel/setup.c renamed from arch/x86_64/kernel/setup.c~mtrr_cpuhotplug_cleanup +4 -0
@@ -1076,6 +1076,10 @@ void __cpuinit identify_cpu(struct cpuin#ifdef CONFIG_X86_MCE mcheck_init (c);#endif +if (c == &boot_cpu_data) +mtrr_bp_init (); +else +mtrr_ap_init ();#ifdef CONFIG_NUMA if (c != &boot_cpu_data)numa_add_cpu (c - cpu_data);
_
Quoted 9 lines · Shaohua Li
Hi,
There has been some discuss about solving the SMP MTRR suspend/resume breakage, but I didn't find a patch for it. This is an intent for it. The basic idea is moving mtrr initializing intocpu_identifyfor all APs (so it works for cpu hotplug). For BP,restore_processor_stateis responsible for restoring MTRR.
arch/i386/kernel/cpu/mtrr/main.c int mtrr_add_page(unsigned long base, un · line 338
Quoted 7 lines
arch/i386/kernel/cpu/mtrr/main.c renamed from arch/i386/kernel/cpu/mtrr/main.c~mtrr_cpuhotplug_cleanup +1
-0@@ -332,6 +332,7 @@ int mtrr_add_page(unsigned long base, un error = -EINVAL ; +lock_cpu_hotplug ();/* Search for existing MTRR */ down (&main_lock);for (i =0 ; i < num_var_ranges; ++i) {
Is this new locking? If so, please describe it.
Quoted 7 lines
arch/i386/kernel/cpu/mtrr/main.c renamed from arch/i386/kernel/cpu/mtrr/main.c~mtrr_cpuhotplug_cleanup +7
-0+ +static int __init mtrr_init_finialize (void ) +{ +if (!mtrr_if) +return 0 ; +if (use_intel ()) +mtrr_state_warn ();
arch/i386/kernel/cpu/mtrr/main.c static int __init mtrr_init(void) · line 712
Quoted 10 lines
arch/i386/kernel/cpu/mtrr/main.c renamed from arch/i386/kernel/cpu/mtrr/main.c~mtrr_cpuhotplug_cleanup +10
-0+else +/* The CPUs haven't MTRR and seemes not support SMP. They have specific +* drivers, we use a tricky method to support suspend/resume for them +* TBD: is there any system with such CPU which supports suspend/resume? +* if no, we should remove the code. +*/ +sysdev_driver_register (&cpu_sysdev_class, + &mtrr_sysdev_driver); +return 0 ; +}
That's pretty hard to read. Braces will improve it:
if (use_intel()) {
mtrr_state_warn();
} else {
/*
* The CPUs haven't MTRR and seemes not support SMP. They have
* specific drivers, we use a tricky method to support
* suspend/resume for them.
* TBD: is there any system with such CPU which supports
* suspend/resume? If no, we should remove the code.
*/
sysdev_driver_register(&cpu_sysdev_class,
&mtrr_sysdev_driver);
}include/asm-i386/processor.h extern unsigned long boot_option_idle_ov · line 705
Quoted 7 lines
include/asm-i386/processor.h renamed from include/asm-i386/processor.h~mtrr_cpuhotplug_cleanup +7
-0+#ifdef CONFIG_MTRR +extern int mtrr_ap_init (void ); +extern int mtrr_bp_init (void ); +#else +#define mtrr_ap_init () +#define mtrr_bp_init () +#endif
If this works, it means we're always ignoring the return values and these things should return void.
Otherwise the !CONFIG_MTRR stubs should be returning an integer. So we need either:
#ifdef CONFIG_MTRR
extern void mtrr_ap_init(void);
extern void mtrr_bp_init(void);
#else
#define mtrr_ap_init() do {} while (0) (or static inlines)
#define mtrr_bp_init() do {} while (0)
#endif
or
#ifdef CONFIG_MTRR
extern int mtrr_ap_init(void);
extern int mtrr_bp_init(void);
#else
static inline int mtrr_ap_init(void) { return 0; } static inline int mtrr_bp_init(void) { return 0; } #endif
include/asm-x86_64/proto.h extern void pda_init(int); · line 24
Quoted 7 lines
include/asm-x86_64/proto.h renamed from include/asm-x86_64/proto.h~mtrr_cpuhotplug_cleanup +7
-0+#ifdef CONFIG_MTRR +extern int mtrr_ap_init (void ); +extern int mtrr_bp_init (void ); +#else +#define mtrr_ap_init () +#define mtrr_bp_init (); +#endif
Ditto.
Quoted 82 lines · Andrew Morton
arch/i386/kernel/cpu/mtrr/main.c int mtrr_add_page(unsigned long base, un · line 338
Quoted 8 lines
arch/i386/kernel/cpu/mtrr/main.c renamed from arch/i386/kernel/cpu/mtrr/main.c~mtrr_cpuhotplug_cleanup +1
-0@@ -332,6 +332,7 @@ int mtrr_add_page(unsigned long base, un error = -EINVAL ; +lock_cpu_hotplug ();/* Search for existing MTRR */ down (&main_lock);for (i =0 ; i < num_var_ranges; ++i) {Is this new locking? If so, please describe it.
arch/i386/kernel/cpu/mtrr/main.c static int __init mtrr_init(void)
Quoted 9 lines
arch/i386/kernel/cpu/mtrr/main.c renamed from arch/i386/kernel/cpu/mtrr/main.c~mtrr_cpuhotplug_cleanup +9
-0+ +static int __init mtrr_init_finialize (void ) +{ +if (!mtrr_if) +return 0 ; +if (use_intel ()) +mtrr_state_warn (); +else +/* The CPUs haven't MTRR and seemes not support SMP. They have specific
Quoted 1 line
+ * drivers, we use a tricky method to support suspend/resume
for them
Quoted 1 line
+ * TBD: is there any system with such CPU which supports
suspend/resume?
arch/i386/kernel/cpu/mtrr/main.c static int __init mtrr_init(void) · line 712
Quoted 6 lines
arch/i386/kernel/cpu/mtrr/main.c renamed from arch/i386/kernel/cpu/mtrr/main.c~mtrr_cpuhotplug_cleanup +6
-0+* if no, we should remove the code. +*/ +sysdev_driver_register (&cpu_sysdev_class, + &mtrr_sysdev_driver); +return 0 ; +}That's pretty hard to read. Braces will improve it:
if (use_intel()) { mtrr_state_warn(); } else { /* * The CPUs haven't MTRR and seemes not support SMP.They have
* specific drivers, we use a tricky method to support
* suspend/resume for them.
* TBD: is there any system with such CPU which
supports* suspend/resume? If no, we should remove the code. */ sysdev_driver_register(&cpu_sysdev_class, &mtrr_sysdev_driver); }include/asm-i386/processor.h extern unsigned long boot_option_idle_ov · line 705
Quoted 7 lines
include/asm-i386/processor.h renamed from include/asm-i386/processor.h~mtrr_cpuhotplug_cleanup +7
-0+#ifdef CONFIG_MTRR +extern int mtrr_ap_init (void ); +extern int mtrr_bp_init (void ); +#else +#define mtrr_ap_init () +#define mtrr_bp_init () +#endif If this works, it means we're always ignoring the return values and these
things should return void.Otherwise the !CONFIG_MTRR stubs should be returning an integer. So we
need either:#ifdef
CONFIG_MTRR
extern voidmtrr_ap_init(void);
extern voidmtrr_bp_init(void);
#else
#definemtrr_ap_init()do {} while (0) (or static inlines)
#definemtrr_bp_init()do {} while (0)
#endifor
#ifdef
CONFIG_MTRR
extern intmtrr_ap_init(void);
extern intmtrr_bp_init(void);
#else
static inline intmtrr_ap_init(void){ return 0; } static inline intmtrr_bp_init(void){ return 0; } #endif
Quoted 8 lines
include/asm-x86_64/proto.h extern void pda_init(int); · line 24
Quoted 7 lines
include/asm-x86_64/proto.h renamed from include/asm-x86_64/proto.h~mtrr_cpuhotplug_cleanup +7
-0+#ifdef CONFIG_MTRR +extern int mtrr_ap_init (void ); +extern int mtrr_bp_init (void ); +#else +#define mtrr_ap_init () +#define mtrr_bp_init (); +#endif Ditto.
Thanks for your feedback. I will update the patch when I collect all comments.
Thanks,
Shaohua
Quoted 10 lines · Andrew Morton
Review int mtrr_add_page(unsigned long base, un · line 338
Quoted 8 lines
(quoted diff) +1
-0@@ -332,6 +332,7 @@ int mtrr_add_page(unsigned long base, un error = -EINVAL; + lock_cpu_hotplug(); /* Search for existing MTRR */ down(&main_lock); for (i = 0; i < num_var_ranges; ++i) {Is this new locking? If so, please describe it.
<Snip>
Ok, this is an updated patch based on your suggestion. Could this be put into next -mm tree?
Thanks,
Shaohua
---
linux-2.6.12-mm1-root/arch/i386/kernel/cpu/common.c | 5 linux-2.6.12-mm1-root/arch/i386/kernel/cpu/mtrr/generic.c | 22 ++-- linux-2.6.12-mm1-root/arch/i386/kernel/cpu/mtrr/main.c | 76 +++++++++----- linux-2.6.12-mm1-root/arch/i386/kernel/cpu/mtrr/mtrr.h | 1 linux-2.6.12-mm1-root/arch/i386/power/cpu.c | 1 linux-2.6.12-mm1-root/arch/x86_64/kernel/setup.c | 4 linux-2.6.12-mm1-root/arch/x86_64/kernel/suspend.c | 1 linux-2.6.12-mm1-root/include/asm-i386/processor.h | 8 + linux-2.6.12-mm1-root/include/asm-x86_64/proto.h | 7 + 9 files changed, 90 insertions(+), 35 deletions(-)
arch/i386/kernel/cpu/mtrr/main.c renamed from arch/i386/kernel/cpu/mtrr/main.c~mtrr_cpuhotplug_cleanup +53 -23
@@ -332,6 +332,8 @@ int mtrr_add_page(unsigned long base, un error = -EINVAL ; +/* No CPU hotplug when we change MTRR entries */ +lock_cpu_hotplug ();/* Search for existing MTRR */ down (&main_lock);for (i =0 ; i < num_var_ranges; ++i) {
@@ -372,6 +374,7 @@ int mtrr_add_page(unsigned long base, un error = i; out:up (&main_lock); +unlock_cpu_hotplug ();return error; }
@@ -461,6 +464,8 @@ int mtrr_del_page(int reg, unsigned longreturn -ENXIO ; max = num_var_ranges; +/* No CPU hotplug when we change MTRR entries */ +lock_cpu_hotplug ();down (&main_lock);if (reg <0 ) {/* Search for existing MTRR */
@@ -501,6 +506,7 @@ int mtrr_del_page(int reg, unsigned long error = reg; out:up (&main_lock); +unlock_cpu_hotplug ();return error; }/**
@@ -544,21 +550,9 @@ static void __init init_ifs(void)centaur_init_mtrr (); } -static void __init init_other_cpus (void ) -{ -if (use_intel ()) -get_mtrr_state (); - -/* bring up the other processors */ -set_mtrr (~0U ,0 ,0 ,0 ); - -if (use_intel ()) { -finalize_mtrr_state (); -mtrr_state_warn (); - } -} - - +/* The suspend/resume methods are only for CPU without MTRR. CPU using generic +* MTRR driver doesn't require this +*/ struct mtrr_value {mtrr_type ltype;unsigned long lbase;
@@ -611,13 +605,13 @@ static struct sysdev_driver mtrr_sysdev_/** -* mtrr_init - initialize mtrrs on the boot CPU +* mtrr_bp_init - initialize mtrrs on the boot CPU * * This needs to be called early; before any of the other CPUs are * initialized (i.e. before smp_init()). * */ -static int __init mtrr_init (void ) +void __init mtrr_bp_init (void ) {init_ifs ();
@@ -674,12 +668,48 @@ static int __init mtrr_init(void)if (mtrr_if) {set_num_var_ranges ();init_table (); -init_other_cpus (); - -return sysdev_driver_register (&cpu_sysdev_class, - &mtrr_sysdev_driver); +if (use_intel ()) +get_mtrr_state (); } -return -ENXIO ; } -subsys_initcall (mtrr_init); +void mtrr_ap_init (void ) +{ +unsigned long flags; + +if (!mtrr_if || !use_intel ()) +return ; +/* +* Ideally we should hold main_lock here to avoid mtrr entries changed, +* but this routine will be called in cpu boot time, holding the lock +* breaks it. This routine is called in two cases: 1.very earily time +* of software resume, when there absolutely isn't mtrr entry changes; +* 2.cpu hotadd time. We let mtrr_add/del_page hold cpuhotplug lock to +* prevent mtrr entry changes +*/ +local_irq_save (flags); + + mtrr_if->set_all (); + +local_irq_restore (flags); +} + +static int __init mtrr_init_finialize (void ) +{ +if (!mtrr_if) +return 0 ; +if (use_intel ()) +mtrr_state_warn (); +else { +/* The CPUs haven't MTRR and seemes not support SMP. They have +* specific drivers, we use a tricky method to support +* suspend/resume for them. +* TBD: is there any system with such CPU which supports +* suspend/resume? if no, we should remove the code. +*/ +sysdev_driver_register (&cpu_sysdev_class, + &mtrr_sysdev_driver); + } +return 0 ; +} +subsys_initcall (mtrr_init_finialize);
arch/i386/kernel/cpu/mtrr/generic.c renamed from arch/i386/kernel/cpu/mtrr/generic.c~mtrr_cpuhotplug_cleanup +11 -11
@@ -67,13 +67,6 @@ void __init get_mtrr_state(void) mtrr_state.enabled = (lo &0xc00 ) >>10 ; } -/* Free resources associated with a struct mtrr_state */ -void __init finalize_mtrr_state (void ) -{ -kfree (mtrr_state.var_ranges); - mtrr_state.var_ranges =NULL ; -} -/* Some BIOS's are fucked and don't set all MTRRs the same! */ void __init mtrr_state_warn (void ) {
@@ -334,6 +327,9 @@ static void generic_set_mtrr(unsigned in*/ {unsigned long flags; +struct mtrr_var_range *vr; + + vr = &mtrr_state.var_ranges[reg];local_irq_save (flags);prepare_set ();
@@ -342,11 +338,15 @@ static void generic_set_mtrr(unsigned in/* The invalid bit is kept in the mask, so we simply clear the relevant mask register to disable a range. */ mtrr_wrmsr (MTRRphysMask_MSR (reg),0 ,0 ); +memset (vr,0 ,sizeof (struct mtrr_var_range )); }else { -mtrr_wrmsr (MTRRphysBase_MSR (reg), base <<PAGE_SHIFT | type, - (base & size_and_mask) >> (32 -PAGE_SHIFT )); -mtrr_wrmsr (MTRRphysMask_MSR (reg), -size <<PAGE_SHIFT |0x800 , - (-size & size_and_mask) >> (32 -PAGE_SHIFT )); + vr->base_lo = base <<PAGE_SHIFT | type; + vr->base_hi = (base & size_and_mask) >> (32 -PAGE_SHIFT ); + vr->mask_lo = -size <<PAGE_SHIFT |0x800 ; + vr->mask_hi = (-size & size_and_mask) >> (32 -PAGE_SHIFT ); + +mtrr_wrmsr (MTRRphysBase_MSR (reg), vr->base_lo, vr->base_hi); +mtrr_wrmsr (MTRRphysMask_MSR (reg), vr->mask_lo, vr->mask_hi); }post_set ();
arch/i386/kernel/cpu/mtrr/mtrr.h renamed from arch/i386/kernel/cpu/mtrr/mtrr.h~mtrr_cpuhotplug_cleanup +0 -1
@@ -91,7 +91,6 @@ extern struct mtrr_ops * mtrr_if;extern unsigned int num_var_ranges; -void finalize_mtrr_state (void );void mtrr_state_warn (void );char *mtrr_attrib_to_str (int x);void mtrr_wrmsr (unsigned ,unsigned ,unsigned );
arch/i386/kernel/cpu/common.c renamed from arch/i386/kernel/cpu/common.c~mtrr_cpuhotplug_cleanup +5 -0
@@ -435,6 +435,11 @@ void __devinit identify_cpu(struct cpuinif (c == &boot_cpu_data)sysenter_setup ();enable_sep_cpu (); + +if (c == &boot_cpu_data) +mtrr_bp_init (); +else +mtrr_ap_init (); }#ifdef CONFIG_X86_HT
include/asm-i386/processor.h renamed from include/asm-i386/processor.h~mtrr_cpuhotplug_cleanup +8 -0
@@ -696,4 +696,12 @@ extern unsigned long boot_option_idle_ovextern void enable_sep_cpu (void );extern int sysenter_setup (void ); +#ifdef CONFIG_MTRR +extern void mtrr_ap_init (void ); +extern void mtrr_bp_init (void ); +#else +#define mtrr_ap_init () do {} while (0) +#define mtrr_bp_init () do {} while (0) +#endif +#endif /* __ASM_I386_PROCESSOR_H */
arch/i386/power/cpu.c renamed from arch/i386/power/cpu.c~mtrr_cpuhotplug_cleanup +1 -0
@@ -137,6 +137,7 @@ void __restore_processor_state(struct safix_processor_context ();do_fpu_end (); +mtrr_ap_init (); }void restore_processor_state (void )
include/asm-x86_64/proto.h renamed from include/asm-x86_64/proto.h~mtrr_cpuhotplug_cleanup +7 -0
@@ -15,6 +15,13 @@ extern void pda_init(int);extern void early_idt_handler (void );extern void mcheck_init (struct cpuinfo_x86 *c); +#ifdef CONFIG_MTRR +extern void mtrr_ap_init (void ); +extern void mtrr_bp_init (void ); +#else +#define mtrr_ap_init () do {} while (0) +#define mtrr_bp_init () do {} while (0) +#endif extern void init_memory_mapping (unsigned long start,unsigned long end);extern void system_call (void );
arch/x86_64/kernel/suspend.c renamed from arch/x86_64/kernel/suspend.c~mtrr_cpuhotplug_cleanup +1 -0
@@ -119,6 +119,7 @@ void __restore_processor_state(struct safix_processor_context ();do_fpu_end (); +mtrr_ap_init (); }void restore_processor_state (void )
arch/x86_64/kernel/setup.c renamed from arch/x86_64/kernel/setup.c~mtrr_cpuhotplug_cleanup +4 -0
@@ -1076,6 +1076,10 @@ void __cpuinit identify_cpu(struct cpuin#ifdef CONFIG_X86_MCE mcheck_init (c);#endif +if (c == &boot_cpu_data) +mtrr_bp_init (); +else +mtrr_ap_init ();#ifdef CONFIG_NUMA if (c != &boot_cpu_data)numa_add_cpu (c - cpu_data);
_
Quoted 3 lines · Shaohua Li
Ok, this is an updated patch based on your suggestion. Could this be put
into next -mm tree?
Sure.
You seem to have lost the changelog. Please send.
Quoted 8 lines · Andrew Morton
Quoted 3 lines · Shaohua Li
Ok, this is an updated patch based on your suggestion. Could this be put
into next -mm tree?Sure.
You seem to have lost the changelog. Please send.
Here it is.
There has been some discuss about solving the SMP MTRR suspend/resume breakage, but I didn't find a patch for it. This is an intent for it. The basic idea is moving mtrr initializing into cpu_identify for all APs (so it works for cpu hotplug). For BP, restore_processor_state is responsible for restoring MTRR.
---
linux-2.6.12-mm1-root/arch/i386/kernel/cpu/common.c | 5 linux-2.6.12-mm1-root/arch/i386/kernel/cpu/mtrr/generic.c | 22 ++-- linux-2.6.12-mm1-root/arch/i386/kernel/cpu/mtrr/main.c | 76 +++++++++----- linux-2.6.12-mm1-root/arch/i386/kernel/cpu/mtrr/mtrr.h | 1 linux-2.6.12-mm1-root/arch/i386/power/cpu.c | 1 linux-2.6.12-mm1-root/arch/x86_64/kernel/setup.c | 4 linux-2.6.12-mm1-root/arch/x86_64/kernel/suspend.c | 1 linux-2.6.12-mm1-root/include/asm-i386/processor.h | 8 + linux-2.6.12-mm1-root/include/asm-x86_64/proto.h | 7 + 9 files changed, 90 insertions(+), 35 deletions(-)
arch/i386/kernel/cpu/mtrr/main.c renamed from arch/i386/kernel/cpu/mtrr/main.c~mtrr_cpuhotplug_cleanup +53 -23
@@ -332,6 +332,8 @@ int mtrr_add_page(unsigned long base, un error = -EINVAL ; +/* No CPU hotplug when we change MTRR entries */ +lock_cpu_hotplug ();/* Search for existing MTRR */ down (&main_lock);for (i =0 ; i < num_var_ranges; ++i) {
@@ -372,6 +374,7 @@ int mtrr_add_page(unsigned long base, un error = i; out:up (&main_lock); +unlock_cpu_hotplug ();return error; }
@@ -461,6 +464,8 @@ int mtrr_del_page(int reg, unsigned longreturn -ENXIO ; max = num_var_ranges; +/* No CPU hotplug when we change MTRR entries */ +lock_cpu_hotplug ();down (&main_lock);if (reg <0 ) {/* Search for existing MTRR */
@@ -501,6 +506,7 @@ int mtrr_del_page(int reg, unsigned long error = reg; out:up (&main_lock); +unlock_cpu_hotplug ();return error; }/**
@@ -544,21 +550,9 @@ static void __init init_ifs(void)centaur_init_mtrr (); } -static void __init init_other_cpus (void ) -{ -if (use_intel ()) -get_mtrr_state (); - -/* bring up the other processors */ -set_mtrr (~0U ,0 ,0 ,0 ); - -if (use_intel ()) { -finalize_mtrr_state (); -mtrr_state_warn (); - } -} - - +/* The suspend/resume methods are only for CPU without MTRR. CPU using generic +* MTRR driver doesn't require this +*/ struct mtrr_value {mtrr_type ltype;unsigned long lbase;
@@ -611,13 +605,13 @@ static struct sysdev_driver mtrr_sysdev_/** -* mtrr_init - initialize mtrrs on the boot CPU +* mtrr_bp_init - initialize mtrrs on the boot CPU * * This needs to be called early; before any of the other CPUs are * initialized (i.e. before smp_init()). * */ -static int __init mtrr_init (void ) +void __init mtrr_bp_init (void ) {init_ifs ();
@@ -674,12 +668,48 @@ static int __init mtrr_init(void)if (mtrr_if) {set_num_var_ranges ();init_table (); -init_other_cpus (); - -return sysdev_driver_register (&cpu_sysdev_class, - &mtrr_sysdev_driver); +if (use_intel ()) +get_mtrr_state (); } -return -ENXIO ; } -subsys_initcall (mtrr_init); +void mtrr_ap_init (void ) +{ +unsigned long flags; + +if (!mtrr_if || !use_intel ()) +return ; +/* +* Ideally we should hold main_lock here to avoid mtrr entries changed, +* but this routine will be called in cpu boot time, holding the lock +* breaks it. This routine is called in two cases: 1.very earily time +* of software resume, when there absolutely isn't mtrr entry changes; +* 2.cpu hotadd time. We let mtrr_add/del_page hold cpuhotplug lock to +* prevent mtrr entry changes +*/ +local_irq_save (flags); + + mtrr_if->set_all (); + +local_irq_restore (flags); +} + +static int __init mtrr_init_finialize (void ) +{ +if (!mtrr_if) +return 0 ; +if (use_intel ()) +mtrr_state_warn (); +else { +/* The CPUs haven't MTRR and seemes not support SMP. They have +* specific drivers, we use a tricky method to support +* suspend/resume for them. +* TBD: is there any system with such CPU which supports +* suspend/resume? if no, we should remove the code. +*/ +sysdev_driver_register (&cpu_sysdev_class, + &mtrr_sysdev_driver); + } +return 0 ; +} +subsys_initcall (mtrr_init_finialize);
arch/i386/kernel/cpu/mtrr/generic.c renamed from arch/i386/kernel/cpu/mtrr/generic.c~mtrr_cpuhotplug_cleanup +11 -11
@@ -67,13 +67,6 @@ void __init get_mtrr_state(void) mtrr_state.enabled = (lo &0xc00 ) >>10 ; } -/* Free resources associated with a struct mtrr_state */ -void __init finalize_mtrr_state (void ) -{ -kfree (mtrr_state.var_ranges); - mtrr_state.var_ranges =NULL ; -} -/* Some BIOS's are fucked and don't set all MTRRs the same! */ void __init mtrr_state_warn (void ) {
@@ -334,6 +327,9 @@ static void generic_set_mtrr(unsigned in*/ {unsigned long flags; +struct mtrr_var_range *vr; + + vr = &mtrr_state.var_ranges[reg];local_irq_save (flags);prepare_set ();
@@ -342,11 +338,15 @@ static void generic_set_mtrr(unsigned in/* The invalid bit is kept in the mask, so we simply clear the relevant mask register to disable a range. */ mtrr_wrmsr (MTRRphysMask_MSR (reg),0 ,0 ); +memset (vr,0 ,sizeof (struct mtrr_var_range )); }else { -mtrr_wrmsr (MTRRphysBase_MSR (reg), base <<PAGE_SHIFT | type, - (base & size_and_mask) >> (32 -PAGE_SHIFT )); -mtrr_wrmsr (MTRRphysMask_MSR (reg), -size <<PAGE_SHIFT |0x800 , - (-size & size_and_mask) >> (32 -PAGE_SHIFT )); + vr->base_lo = base <<PAGE_SHIFT | type; + vr->base_hi = (base & size_and_mask) >> (32 -PAGE_SHIFT ); + vr->mask_lo = -size <<PAGE_SHIFT |0x800 ; + vr->mask_hi = (-size & size_and_mask) >> (32 -PAGE_SHIFT ); + +mtrr_wrmsr (MTRRphysBase_MSR (reg), vr->base_lo, vr->base_hi); +mtrr_wrmsr (MTRRphysMask_MSR (reg), vr->mask_lo, vr->mask_hi); }post_set ();
arch/i386/kernel/cpu/mtrr/mtrr.h renamed from arch/i386/kernel/cpu/mtrr/mtrr.h~mtrr_cpuhotplug_cleanup +0 -1
@@ -91,7 +91,6 @@ extern struct mtrr_ops * mtrr_if;extern unsigned int num_var_ranges; -void finalize_mtrr_state (void );void mtrr_state_warn (void );char *mtrr_attrib_to_str (int x);void mtrr_wrmsr (unsigned ,unsigned ,unsigned );
arch/i386/kernel/cpu/common.c renamed from arch/i386/kernel/cpu/common.c~mtrr_cpuhotplug_cleanup +5 -0
@@ -435,6 +435,11 @@ void __devinit identify_cpu(struct cpuinif (c == &boot_cpu_data)sysenter_setup ();enable_sep_cpu (); + +if (c == &boot_cpu_data) +mtrr_bp_init (); +else +mtrr_ap_init (); }#ifdef CONFIG_X86_HT
include/asm-i386/processor.h renamed from include/asm-i386/processor.h~mtrr_cpuhotplug_cleanup +8 -0
@@ -696,4 +696,12 @@ extern unsigned long boot_option_idle_ovextern void enable_sep_cpu (void );extern int sysenter_setup (void ); +#ifdef CONFIG_MTRR +extern void mtrr_ap_init (void ); +extern void mtrr_bp_init (void ); +#else +#define mtrr_ap_init () do {} while (0) +#define mtrr_bp_init () do {} while (0) +#endif +#endif /* __ASM_I386_PROCESSOR_H */
arch/i386/power/cpu.c renamed from arch/i386/power/cpu.c~mtrr_cpuhotplug_cleanup +1 -0
@@ -137,6 +137,7 @@ void __restore_processor_state(struct safix_processor_context ();do_fpu_end (); +mtrr_ap_init (); }void restore_processor_state (void )
include/asm-x86_64/proto.h renamed from include/asm-x86_64/proto.h~mtrr_cpuhotplug_cleanup +7 -0
@@ -15,6 +15,13 @@ extern void pda_init(int);extern void early_idt_handler (void );extern void mcheck_init (struct cpuinfo_x86 *c); +#ifdef CONFIG_MTRR +extern void mtrr_ap_init (void ); +extern void mtrr_bp_init (void ); +#else +#define mtrr_ap_init () do {} while (0) +#define mtrr_bp_init () do {} while (0) +#endif extern void init_memory_mapping (unsigned long start,unsigned long end);extern void system_call (void );
arch/x86_64/kernel/suspend.c renamed from arch/x86_64/kernel/suspend.c~mtrr_cpuhotplug_cleanup +1 -0
@@ -119,6 +119,7 @@ void __restore_processor_state(struct safix_processor_context ();do_fpu_end (); +mtrr_ap_init (); }void restore_processor_state (void )
arch/x86_64/kernel/setup.c renamed from arch/x86_64/kernel/setup.c~mtrr_cpuhotplug_cleanup +4 -0
@@ -1076,6 +1076,10 @@ void __cpuinit identify_cpu(struct cpuin#ifdef CONFIG_X86_MCE mcheck_init (c);#endif +if (c == &boot_cpu_data) +mtrr_bp_init (); +else +mtrr_ap_init ();#ifdef CONFIG_NUMA if (c != &boot_cpu_data)numa_add_cpu (c - cpu_data);
_
Quoted 5 lines
There has been some discuss about solving the SMP MTRR suspend/resume breakage, but I didn't find a patch for it. This is an intent for it. The basic idea is moving mtrr initializing into
cpu_identifyfor all APs (so it works for cpu hotplug). For BP,restore_processor_stateis responsible for restoring MTRR.
Looks good to me. Thanks.
-Andi
Hi.
Quoted 6 lines · Shaohua Li
Hi,
There has been some discuss about solving the SMP MTRR suspend/resume breakage, but I didn't find a patch for it. This is an intent for it. The basic idea is moving mtrr initializing intocpu_identifyfor all APs (so it works for cpu hotplug). For BP,restore_processor_stateis responsible for restoring MTRR.
I have a patch in Suspend2 that removes the sysdev drivers and implements mtrr save & restore as part of the cpu state save/restore. I'm not sure if that's what you were thinking of.
I'm not using hotplug CPU at the moment, so won't be trying your patch at the moment sorry.
Regards,
Nigel