| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20160315213153.GB1990@wotan.suse.de>
Date: Tue, 15 Mar 2016 22:31:53 +0100
From: "Luis R. Rodriguez" <mcgrof@...nel.org>
To: Toshi Kani <toshi.kani@....com>
Cc: "Luis R. Rodriguez" <mcgrof@...nel.org>,
Borislav Petkov <bp@...en8.de>,
"mingo@...nel.org" <mingo@...nel.org>,
"hpa@...or.com" <hpa@...or.com>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"jgross@...e.com" <jgross@...e.com>,
"paul.gortmaker@...driver.com" <paul.gortmaker@...driver.com>,
"x86@...nel.org" <x86@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
xen-devel@...ts.xenproject.org, boris.ostrovsky@...cle.com
Subject: Re: [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 <toshi.kani@....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
Powered by blists - more mailing lists