| Message ID | 1458175619-32206-1-git-send-email-toshi.kani@hpe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 16, 2016 at 06:46:55PM -0600, Toshi Kani wrote: > In preparation to fix a regression caused by 'commit 9cd25aac1f44 > ("x86/mm/pat: Emulate PAT when it is disabled")', PAT needs to > provide an interface that disables the OS to initialize PAT MSR. prevents the OS from initializing the PAT MSR. > > PAT MSR initialization must be done on all CPUs with the specific s/with/using/ > sequence of operations defined in Intel SDM. This requires MTRR ^ the s/MTRR/MTRRs/ > to be enabled since pat_init() is called as part of MTRR init > from mtrr_rendezvous_handler(). > > Change pat_disable() as the interface to disable the OS to initialize > PAT MSR, and set PAT table with pat_keep_handoff_state(). This > interface can be called when PAT initialization may not be performed. This paragraph reads funky and I can't really parse what it is trying to say. > This also assures that pat_disable() called from pat_bsp_init() > to set PAT table properly when CPU does not support PAT. > > Signed-off-by: Toshi Kani <toshi.kani@hpe.com> > Cc: Borislav Petkov <bp@suse.de> > Cc: Luis R. Rodriguez <mcgrof@suse.com> > Cc: Juergen Gross <jgross@suse.com> > Cc: Robert Elliott <elliott@hpe.com> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: H. Peter Anvin <hpa@zytor.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > --- > arch/x86/include/asm/pat.h | 1 + > arch/x86/mm/pat.c | 21 ++++++++++++++++++--- > 2 files changed, 19 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/include/asm/pat.h b/arch/x86/include/asm/pat.h > index ca6c228..016142b 100644 > --- a/arch/x86/include/asm/pat.h > +++ b/arch/x86/include/asm/pat.h > @@ -5,6 +5,7 @@ > #include <asm/pgtable_types.h> > > bool pat_enabled(void); > +void pat_disable(const char *reason); > extern void pat_init(void); > void pat_init_cache_modes(u64); > > diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c > index e0a34b0..48d1619 100644 > --- a/arch/x86/mm/pat.c > +++ b/arch/x86/mm/pat.c > @@ -40,11 +40,26 @@ > static bool boot_cpu_done; > > static int __read_mostly __pat_enabled = IS_ENABLED(CONFIG_X86_PAT); > +static void pat_keep_handoff_state(void); > > -static inline void pat_disable(const char *reason) > +/** > + * pat_disable() - Disable the OS to initialize PAT MSR ^^^^ Err, what? The function name can't be more clear. > + * > + * This function disables the OS to initialize PAT MSR, and calls "prevents the OS from initializing the PAT MSR..." > + * pat_keep_handoff_state() to set PAT table to the handoff state. We can see what is calls. You're explaining *what* the code does instead of *why* again. > + */ > +void pat_disable(const char *reason) > { Why aren't you checking __pat_enabled here? if (!__pat_enabled) return; You can save yourself the other guards in that function, especially that pr_err() below. > + if (boot_cpu_done) { > + pr_err("x86/PAT: PAT cannot be disabled after initialization " > + "(attempting: %s)\n", reason); Please integrate checkpatch.pl into your patch creation workflow as it sometimes has valid complaints: WARNING: quoted string split across lines #79: FILE: arch/x86/mm/pat.c:55: + pr_err("x86/PAT: PAT cannot be disabled after initialization " + "(attempting: %s)\n", reason); More to the point: why do we need that pr_err() call? What is that supposed to tell the user? I think it is more for the programmer to catch wrong use of pat_disable() and then it should be WARN_ONCE() or so... > + return; > + } > + > __pat_enabled = 0; > pr_info("x86/PAT: %s\n", reason); > + > + pat_keep_handoff_state(); > } > > static int __init nopat(char *str) > @@ -202,7 +217,7 @@ static void pat_bsp_init(u64 pat) > { > u64 tmp_pat; > > - if (!cpu_has_pat) { > + if (!boot_cpu_has(X86_FEATURE_PAT)) { > pat_disable("PAT not supported by CPU."); > return; > } > @@ -220,7 +235,7 @@ static void pat_bsp_init(u64 pat) > > static void pat_ap_init(u64 pat) > { > - if (!cpu_has_pat) { > + if (!boot_cpu_has(X86_FEATURE_PAT)) { > /* > * If this happens we are on a secondary CPU, but switched to > * PAT on the boot CPU. We have no way to undo PAT. Those last two hunks are unrelated changes and should be a separate patch.
On Tue, 2016-03-22 at 17:59 +0100, Borislav Petkov wrote: > On Wed, Mar 16, 2016 at 06:46:55PM -0600, Toshi Kani wrote: > > In preparation to fix a regression caused by 'commit 9cd25aac1f44 > > ("x86/mm/pat: Emulate PAT when it is disabled")', PAT needs to > > provide an interface that disables the OS to initialize PAT MSR. > > prevents the OS from initializing the PAT MSR. Right. Will do. > > > > PAT MSR initialization must be done on all CPUs with the specific > > s/with/using/ Ditto. > > sequence of operations defined in Intel SDM. This requires MTRR > ^ > the > > s/MTRR/MTRRs/ Ditto. > > to be enabled since pat_init() is called as part of MTRR init > > from mtrr_rendezvous_handler(). > > > > Change pat_disable() as the interface to disable the OS to initialize > > PAT MSR, and set PAT table with pat_keep_handoff_state(). This > > interface can be called when PAT initialization may not be performed. > > This paragraph reads funky and I can't really parse what it is trying to > say. Sorry... Here is a retry: Make pat_disable() as the interface that prevents the OS from initializing the PAT MSR. MTRR will call this interface when it cannot provide the SDM- defined sequence to initialize PAT. > > This also assures that pat_disable() called from pat_bsp_init() > > to set PAT table properly when CPU does not support PAT. > > : > > > > -static inline void pat_disable(const char *reason) > > +/** > > + * pat_disable() - Disable the OS to initialize PAT MSR > > ^^^^ > > Err, what? The function name can't be more clear. Will change to "Prevent the OS from initializing the PAT MSR". I wanted to clarify that "disable" does not mean to disable PAT MSR. > > + * > > + * This function disables the OS to initialize PAT MSR, and calls > > "prevents the OS from initializing the PAT MSR..." Will do. > > + * pat_keep_handoff_state() to set PAT table to the handoff state. > > We can see what is calls. You're explaining *what* the code does instead > of *why* again. Right... > > + */ > > +void pat_disable(const char *reason) > > { > > Why aren't you checking __pat_enabled here? > > if (!__pat_enabled) > return; pat_keep_handoff_state() is a no-op after the initial call, but I agree that having this check is better. Will do. > You can save yourself the other guards in that function, especially that > pr_err() below. The pr_err() below is for a difference case -- PAT is enabled, but a call is made to disable it after pat_init() is called. We cannot allow this case. > > + if (boot_cpu_done) { > > + pr_err("x86/PAT: PAT cannot be disabled after > > initialization " > > + "(attempting: %s)\n", reason); > > Please integrate checkpatch.pl into your patch creation workflow as it > sometimes has valid complaints: > > WARNING: quoted string split across lines > #79: FILE: arch/x86/mm/pat.c:55: > + pr_err("x86/PAT: PAT cannot be disabled after > initialization " > + "(attempting: %s)\n", reason); I've run checkpatch.pl and thought it was OK to have this warning (instead of a >80 warning) since the error message part was not split. The "attempting" part is for debugging and its string is passed from the caller. > More to the point: why do we need that pr_err() call? What is that > supposed to tell the user? > > I think it is more for the programmer to catch wrong use of > pat_disable() and then it should be WARN_ONCE() or so... Yes, this case is for the programmer to catch wrong use. I will change it to use WARN_ONCE() and remove the "(attempting: %s)\n" part of the message. > > + return; > > + } > > + > > __pat_enabled = 0; > > pr_info("x86/PAT: %s\n", reason); > > + > > + pat_keep_handoff_state(); > > } > > > > static int __init nopat(char *str) > > @@ -202,7 +217,7 @@ static void pat_bsp_init(u64 pat) > > { > > u64 tmp_pat; > > > > - if (!cpu_has_pat) { > > + if (!boot_cpu_has(X86_FEATURE_PAT)) { > > pat_disable("PAT not supported by CPU."); > > return; > > } > > @@ -220,7 +235,7 @@ static void pat_bsp_init(u64 pat) > > > > static void pat_ap_init(u64 pat) > > { > > - if (!cpu_has_pat) { > > + if (!boot_cpu_has(X86_FEATURE_PAT)) { > > /* > > * If this happens we are on a secondary CPU, but > > switched to > > * PAT on the boot CPU. We have no way to undo PAT. > > Those last two hunks are unrelated changes and should be a separate > patch. Will do. Thanks, -Toshi
On Tue, Mar 22, 2016 at 03:40:45PM -0600, Toshi Kani wrote: > Will change to "Prevent the OS from initializing the PAT MSR". > > I wanted to clarify that "disable" does not mean to disable PAT MSR. How do you "disable PAT MSR" ? I think you're overdocumenting this. pat_disable() is as clear as day what it does. It doesn't need any commenting... > I've run checkpatch.pl and thought it was OK to have this warning (instead > of a >80 warning) since the error message part was not split. The > "attempting" part is for debugging and its string is passed from the > caller. We always put the quoted strings on a single line for easier grepping. Forget the 80-cols rule.
On Wed, 2016-03-23 at 09:51 +0100, Borislav Petkov wrote: > On Tue, Mar 22, 2016 at 03:40:45PM -0600, Toshi Kani wrote: > > Will change to "Prevent the OS from initializing the PAT MSR". > > > > I wanted to clarify that "disable" does not mean to disable PAT MSR. > > How do you "disable PAT MSR" ? We can't, but I thought not everyone knows how it works... > I think you're overdocumenting this. pat_disable() is as clear as day > what it does. It doesn't need any commenting... Right, maybe I am just paranoid. I will remove the comment as you suggested. > > I've run checkpatch.pl and thought it was OK to have this warning > > (instead of a >80 warning) since the error message part was not split. > > The "attempting" part is for debugging and its string is passed from > > the caller. > > We always put the quoted strings on a single line for easier grepping. > Forget the 80-cols rule. OK. Thanks, -Toshi
diff --git a/arch/x86/include/asm/pat.h b/arch/x86/include/asm/pat.h index ca6c228..016142b 100644 --- a/arch/x86/include/asm/pat.h +++ b/arch/x86/include/asm/pat.h @@ -5,6 +5,7 @@ #include <asm/pgtable_types.h> bool pat_enabled(void); +void pat_disable(const char *reason); extern void pat_init(void); void pat_init_cache_modes(u64); diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c index e0a34b0..48d1619 100644 --- a/arch/x86/mm/pat.c +++ b/arch/x86/mm/pat.c @@ -40,11 +40,26 @@ static bool boot_cpu_done; static int __read_mostly __pat_enabled = IS_ENABLED(CONFIG_X86_PAT); +static void pat_keep_handoff_state(void); -static inline void pat_disable(const char *reason) +/** + * pat_disable() - Disable the OS to initialize PAT MSR + * + * This function disables the OS to initialize PAT MSR, and calls + * pat_keep_handoff_state() to set PAT table to the handoff state. + */ +void pat_disable(const char *reason) { + if (boot_cpu_done) { + pr_err("x86/PAT: PAT cannot be disabled after initialization " + "(attempting: %s)\n", reason); + return; + } + __pat_enabled = 0; pr_info("x86/PAT: %s\n", reason); + + pat_keep_handoff_state(); } static int __init nopat(char *str) @@ -202,7 +217,7 @@ static void pat_bsp_init(u64 pat) { u64 tmp_pat; - if (!cpu_has_pat) { + if (!boot_cpu_has(X86_FEATURE_PAT)) { pat_disable("PAT not supported by CPU."); return; } @@ -220,7 +235,7 @@ static void pat_bsp_init(u64 pat) static void pat_ap_init(u64 pat) { - if (!cpu_has_pat) { + if (!boot_cpu_has(X86_FEATURE_PAT)) { /* * If this happens we are on a secondary CPU, but switched to * PAT on the boot CPU. We have no way to undo PAT.
In preparation to fix a regression caused by 'commit 9cd25aac1f44 ("x86/mm/pat: Emulate PAT when it is disabled")', PAT needs to provide an interface that disables the OS to initialize PAT MSR. PAT MSR initialization must be done on all CPUs with the specific sequence of operations defined in Intel SDM. This requires MTRR to be enabled since pat_init() is called as part of MTRR init from mtrr_rendezvous_handler(). Change pat_disable() as the interface to disable the OS to initialize PAT MSR, and set PAT table with pat_keep_handoff_state(). This interface can be called when PAT initialization may not be performed. This also assures that pat_disable() called from pat_bsp_init() to set PAT table properly when CPU does not support PAT. Signed-off-by: Toshi Kani <toshi.kani@hpe.com> Cc: Borislav Petkov <bp@suse.de> Cc: Luis R. Rodriguez <mcgrof@suse.com> Cc: Juergen Gross <jgross@suse.com> Cc: Robert Elliott <elliott@hpe.com> Cc: Ingo Molnar <mingo@kernel.org> Cc: H. Peter Anvin <hpa@zytor.com> Cc: Thomas Gleixner <tglx@linutronix.de> --- arch/x86/include/asm/pat.h | 1 + arch/x86/mm/pat.c | 21 ++++++++++++++++++--- 2 files changed, 19 insertions(+), 3 deletions(-)