Re: [Xen-devel] [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table
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
>
> 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
> - * 000WB : _PAGE_CACHE_MODE_WB
> - * 011WT : _PAGE_CACHE_MODE_WT
> - * 102UC-: _PAGE_CACHE_MODE_UC_MINUS
> - * 113UC : _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
> + * 000WB : _PAGE_CACHE_MODE_WB
> + * 011WT : _PAGE_CACHE_MODE_WT
> + * 102UC-: _PAGE_CACHE_MODE_UC_MINUS
> + * 113UC :
Re: [Xen-devel] [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table
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. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table
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() on slow paths (i.e., init, bootup, > etc), where speed is not that important. static_cpu_has() > 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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table
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() on slow paths (i.e., init, bootup, etc), where speed is not that important. static_cpu_has() is an optimized version which should be used in hot paths. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table
On Tue, 2016-03-15 at 11:01 +, 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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table
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.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table
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.
Thanks,
-ToshiFrom: Toshi Kani
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
- * 000WB : _PAGE_CACHE_MODE_WB
- * 011WT : _PAGE_CACHE_MODE_WT
- * 102UC-: _PAGE_CACHE_MODE_UC_MINUS
- * 113UC : _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) {
+ /*
+ * CPU supports PAT. Initialize the PAT table to match with
+ * the PAT MSR value. This setup is used by "nopat" boot
+ * option, or by virtual machine environments which do not
+ * support MTRRs but support PAT.
+ *
+ * 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
+ * 000WB : _PAGE_CACHE_MODE_WB
+ * 011WT : _PAGE_CACHE_MODE_WT
+ * 102UC-: _PAGE_CACHE_MODE_UC_MINUS
+ * 113UC : _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);
+ }
pat_init_cache_modes(pat);
- disable_init_done = 1;
+ emu_init_done = 1;
}
void pat_init(void)
@@ -281,7 +294,7 @@ void pat_init(void)
struct cpuinfo_x86 *c = _cpu_data;
if (!pat_enabled()) {
- pat_disable_init();
+ pat_emu_init();
return;
}
From: Toshi Kani
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
Re: [Xen-devel] [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table
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.
Luis
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
