| Message ID | 1458011476.6393.327.camel@hpe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 14, 2016 at 09:11:16PM -0600, Toshi Kani wrote: > - pat = PAT(0, WB) | PAT(1, WT) | PAT(2, UC_MINUS) | PAT(3, UC) | > - PAT(4, WB) | PAT(5, WT) | PAT(6, UC_MINUS) | PAT(7, UC); > + if (cpu_has_pat) { Please use on init paths boot_cpu_has(X86_FEATURE_PAT) and on fast paths static_cpu_has(X86_FEATURE_PAT). No more of that cpu_has_XXX ugliness.
On Tue, 2016-03-15 at 11:01 +0000, Borislav Petkov wrote: > On Mon, Mar 14, 2016 at 09:11:16PM -0600, Toshi Kani wrote: > > - pat = PAT(0, WB) | PAT(1, WT) | PAT(2, UC_MINUS) | PAT(3, UC) > > | > > - PAT(4, WB) | PAT(5, WT) | PAT(6, UC_MINUS) | PAT(7, UC); > > + if (cpu_has_pat) { > > Please use on init paths boot_cpu_has(X86_FEATURE_PAT) and on fast paths > static_cpu_has(X86_FEATURE_PAT). No more of that cpu_has_XXX ugliness. 'cpu_has_pat' is defined as 'boot_cpu_has(X86_FEATURE_PAT)'. Do you mean it should explicitly use 'boot_cpu_has(X86_FEATURE_PAT)'? Thanks, -Toshi
On Tue, Mar 15, 2016 at 09:43:15AM -0600, Toshi Kani wrote: > > Please use on init paths boot_cpu_has(X86_FEATURE_PAT) and on fast paths > > static_cpu_has(X86_FEATURE_PAT). No more of that cpu_has_XXX ugliness. > > 'cpu_has_pat' is defined as 'boot_cpu_has(X86_FEATURE_PAT)'. Do you mean > it should explicitly use 'boot_cpu_has(X86_FEATURE_PAT)'? No, read what I said. We use boot_cpu_has(<feature_bit>) on slow paths (i.e., init, bootup, etc), where speed is not that important. static_cpu_has(<feature_bit>) is an optimized version which should be used in hot paths.
On Tue, Mar 15, 2016 at 11:11:23AM -0600, Toshi Kani wrote: > While cpu_has_pat is the same as boot_cpu_has(X86_FEATURE_PAT), cpu_has_XXX > should not be used. So, this code needs to be changed to use > boot_cpu_has(X86_FEATURE_PAT) directly. > > Is this right? Yes.
On Tue, 2016-03-15 at 16:47 +0100, Borislav Petkov wrote: > On Tue, Mar 15, 2016 at 09:43:15AM -0600, Toshi Kani wrote: > > > Please use on init paths boot_cpu_has(X86_FEATURE_PAT) and on fast > > > paths static_cpu_has(X86_FEATURE_PAT). No more of that cpu_has_XXX > > > ugliness. > > > > 'cpu_has_pat' is defined as 'boot_cpu_has(X86_FEATURE_PAT)'. Do you > > mean it should explicitly use 'boot_cpu_has(X86_FEATURE_PAT)'? > > No, read what I said. > > We use boot_cpu_has(<feature_bit>) on slow paths (i.e., init, bootup, > etc), where speed is not that important. static_cpu_has(<feature_bit>) > is an optimized version which should be used in hot paths. Yes, I understand that part. Let me rephrase my question. This PAT code is on init paths and speed is not that important. So, it needs to use 'boot_cpu_has()' here. 'cpu_has_pat' is defined as boot_cpu_has(X86_FEATURE_PAT), and hence it uses boot_cpu_has() already. While cpu_has_pat is the same as boot_cpu_has(X86_FEATURE_PAT), cpu_has_XXX should not be used. So, this code needs to be changed to use boot_cpu_has(X86_FEATURE_PAT) directly. Is this right? Thanks, -Toshi
On Mon, Mar 14, 2016 at 09:11:16PM -0600, Toshi Kani wrote: > On Tue, 2016-03-15 at 01:29 +0100, Luis R. Rodriguez wrote: > > I like this approach more as it stuff more PAT setup on its own type > > of calls, but: > > > > On Sat, Mar 12, 2016 at 12:55:44PM +0100, Borislav Petkov wrote: > > > diff --git a/arch/x86/kernel/cpu/mtrr/main.c > > > b/arch/x86/kernel/cpu/mtrr/main.c > > > index 10f8d4796240..5c442b4bd52a 100644 > > > --- a/arch/x86/kernel/cpu/mtrr/main.c > > > +++ b/arch/x86/kernel/cpu/mtrr/main.c > > > @@ -759,8 +761,11 @@ void __init mtrr_bp_init(void) > > > } > > > } > > > > > > - if (!mtrr_enabled()) > > > + if (!__mtrr_enabled) { > > > pr_info("MTRR: Disabled\n"); > > > + pat_disable("PAT disabled by MTRR"); > > > + pat_setup(); > > > + } > > > } > > > > This hunk would break PAT on Xen. > > Can you try the attached patches? They apply on top of my original patch- > set. With this change, PAT code generally supports Xen, and the PAT init > code in Xen is now removed. If they look OK, I will reorganize the patch > series. I don't have time to test this at this time but on a cursory review this should in theory work, a few nitpicks: > > Thanks, > -Toshi > From: Toshi Kani <toshi.kani@hpe.com> > > Add support of PAT emulation that matches with the PAT MSR. > > --- > arch/x86/mm/pat.c | 73 +++++++++++++++++++++++++++++++---------------------- > 1 file changed, 43 insertions(+), 30 deletions(-) > > diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c > index 1ff8aa9..565a478 100644 > --- a/arch/x86/mm/pat.c > +++ b/arch/x86/mm/pat.c > @@ -40,7 +40,7 @@ > static bool boot_cpu_done; > > static int __read_mostly __pat_enabled = IS_ENABLED(CONFIG_X86_PAT); > -static void pat_disable_init(void); > +static void pat_emu_init(void); > > void pat_disable(const char *reason) > { > @@ -52,7 +52,7 @@ void pat_disable(const char *reason) > __pat_enabled = 0; > pr_info("x86/PAT: %s\n", reason); > > - pat_disable_init(); > + pat_emu_init(); > } > > static int __init nopat(char *str) > @@ -239,40 +239,53 @@ static void pat_ap_init(u64 pat) > wrmsrl(MSR_IA32_CR_PAT, pat); > } > > -static void pat_disable_init(void) > +static void pat_emu_init(void) > { > - u64 pat; > - static int disable_init_done; > + u64 pat = 0; > + static int emu_init_done; > > - if (disable_init_done) > + if (emu_init_done) > return; > > - /* > - * No PAT. Emulate the PAT table that corresponds to the two > - * cache bits, PWT (Write Through) and PCD (Cache Disable). This > - * setup is the same as the BIOS default setup when the system > - * has PAT but the "nopat" boot option has been specified. This > - * emulated PAT table is used when MSR_IA32_CR_PAT returns 0. > - * > - * PTE encoding: > - * > - * PCD > - * |PWT PAT > - * || slot > - * 00 0 WB : _PAGE_CACHE_MODE_WB > - * 01 1 WT : _PAGE_CACHE_MODE_WT > - * 10 2 UC-: _PAGE_CACHE_MODE_UC_MINUS > - * 11 3 UC : _PAGE_CACHE_MODE_UC > - * > - * NOTE: When WC or WP is used, it is redirected to UC- per > - * the default setup in __cachemode2pte_tbl[]. > - */ > - pat = PAT(0, WB) | PAT(1, WT) | PAT(2, UC_MINUS) | PAT(3, UC) | > - PAT(4, WB) | PAT(5, WT) | PAT(6, UC_MINUS) | PAT(7, UC); > + if (cpu_has_pat) { First, this is not emulation, to be clear. > + /* > + * CPU supports PAT. Initialize the PAT table to match with > + * the PAT MSR value. This setup is used by "nopat" boot Did you mean "nomtrr" option ? If not why would "nopat" land you here and if "nopat" was used and you ended up here why would we want to go ahead and read the MSR to keep the PAT set up ? > + * option, or by virtual machine environments which do not > + * support MTRRs but support PAT. This might end up supporting PAT for some other virtual environments ;) > + * > + * If the MSR returns 0, it is considered invalid and emulate > + * as No PAT. > + */ > + rdmsrl(MSR_IA32_CR_PAT, pat); > + } > + > + if (!pat) { > + /* > + * No PAT. Emulate the PAT table that corresponds to the two > + * cache bits, PWT (Write Through) and PCD (Cache Disable). > + * This setup is also the same as the BIOS default setup. > + * > + * PTE encoding: > + * > + * PCD > + * |PWT PAT > + * || slot > + * 00 0 WB : _PAGE_CACHE_MODE_WB > + * 01 1 WT : _PAGE_CACHE_MODE_WT > + * 10 2 UC-: _PAGE_CACHE_MODE_UC_MINUS > + * 11 3 UC : _PAGE_CACHE_MODE_UC > + * > + * NOTE: When WC or WP is used, it is redirected to UC- per > + * the default setup in __cachemode2pte_tbl[]. > + */ > + pat = PAT(0, WB) | PAT(1, WT) | PAT(2, UC_MINUS) | PAT(3, UC) | > + PAT(4, WB) | PAT(5, WT) | PAT(6, UC_MINUS) | PAT(7, UC); > + } a) pat_emu_init() -- this should probably be renamed to something that includes not just emulation as for virtual environments that's not true and using emulation is very misleading for Xen. b) Will this mean that then you can support bare metal with no MTRR but with PAT enabled too? c) Why not just split this up to enable PAT init to be a first class citizen ? Luis
From: Toshi Kani <toshi.kani@hpe.com> Delete the PAT table initialization code from Xen as this case is now supported by PAT. --- arch/x86/include/asm/mtrr.h | 2 +- arch/x86/kernel/cpu/mtrr/main.c | 4 ++-- arch/x86/xen/enlighten.c | 9 --------- 3 files changed, 3 insertions(+), 12 deletions(-) diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h index a965e74..77420c3 100644 --- a/arch/x86/include/asm/mtrr.h +++ b/arch/x86/include/asm/mtrr.h @@ -86,7 +86,7 @@ static inline void mtrr_centaur_report_mcr(int mcr, u32 lo, u32 hi) } static inline void mtrr_bp_init(void) { - pat_disable("PAT disabled by MTRR"); + pat_disable("PAT emulation"); } #define mtrr_ap_init() do {} while (0) diff --git a/arch/x86/kernel/cpu/mtrr/main.c b/arch/x86/kernel/cpu/mtrr/main.c index d9e91f1..e10fba4 100644 --- a/arch/x86/kernel/cpu/mtrr/main.c +++ b/arch/x86/kernel/cpu/mtrr/main.c @@ -767,10 +767,10 @@ void __init mtrr_bp_init(void) /* * PAT initialization relies on MTRR's rendezvous handler. - * Disable PAT until the handler can initialize both features + * Emulate PAT until the handler can initialize both features * independently. */ - pat_disable("PAT disabled by MTRR"); + pat_disable("PAT emulation"); } } diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c index d09e4c9..c883f11 100644 --- a/arch/x86/xen/enlighten.c +++ b/arch/x86/xen/enlighten.c @@ -74,7 +74,6 @@ #include <asm/mach_traps.h> #include <asm/mwait.h> #include <asm/pci_x86.h> -#include <asm/pat.h> #include <asm/cpu.h> #ifdef CONFIG_ACPI @@ -1510,7 +1509,6 @@ asmlinkage __visible void __init xen_start_kernel(void) { struct physdev_set_iopl set_iopl; unsigned long initrd_start = 0; - u64 pat; int rc; if (!xen_start_info) @@ -1617,13 +1615,6 @@ asmlinkage __visible void __init xen_start_kernel(void) xen_start_info->nr_pages); xen_reserve_special_pages(); - /* - * Modify the cache mode translation tables to match Xen's PAT - * configuration. - */ - rdmsrl(MSR_IA32_CR_PAT, pat); - pat_init_cache_modes(pat); - /* keep using Xen gdt for now; no urgent need to change it */ #ifdef CONFIG_X86_32