Re: [Xen-devel] [PATCH 1/2] x86/mm/pat: Change pat_disable() to emulate PAT table

["Luis R. Rodriguez" <mcgrof@xxxxxxxxxx>]

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@xxxxxxx>
> 
> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel