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

[Borislav Petkov <bp@alien8.de>]

On Thu, Mar 10, 2016 at 09:45:45PM -0700, Toshi Kani wrote:
> Since 'commit 9cd25aac1f44 ("x86/mm/pat: Emulate PAT when it
> is disabled")', we emulate a PAT table when PAT is disabled.
> This requires pat_init() be called even if PAT is disabled,
> which revealed a long standing issue that PAT is left enabled
> without calling pat_init() at all.
> 
> pat_init() is called from MTRR code since it relies on MTRR's
> rendezvous handler to initialize PAT for all APs .  However,
> when CPU does not support MTRR, ex. qemu32's virtual CPU, MTRR
> is set disabled and does not call pat_init().  There is no
> interface available for MTRR to disable PAT, either.
> 
> Change pat_disable() to a regular function (from an inline func)
> so that MTRR can call it to disable PAT when MTRR is disabled.
> pat_disable() sets PAT disabled, and calls pat_disable_init()
> to emulate the PAT table.
> 
> link: https://lkml.org/lkml/2016/3/10/402
> Reported-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> 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: 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          |   84 +++++++++++++++++++++++++++-----------------
>  2 files changed, 52 insertions(+), 33 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 f4ae536..1ff8aa9 100644
> --- a/arch/x86/mm/pat.c
> +++ b/arch/x86/mm/pat.c
> @@ -40,11 +40,19 @@
>  static bool boot_cpu_done;
>  
>  static int __read_mostly __pat_enabled = IS_ENABLED(CONFIG_X86_PAT);
> +static void pat_disable_init(void);
>  
> -static inline void pat_disable(const char *reason)
> +void pat_disable(const char *reason)
>  {
> +	if (boot_cpu_done) {
> +		pr_info("x86/PAT: PAT cannot be disabled after initialized\n");

		pr_err()

> +		return;
> +	}
> +
>  	__pat_enabled = 0;
>  	pr_info("x86/PAT: %s\n", reason);
> +
> +	pat_disable_init();

Why can't you call pat_init() here simply? It checks pat_enabled(). You
can call it pat_setup() or so if it looks confusing to call an init
function in a disable function...

Then you don't have to add yet another static disable_init_done but rely on
boot_cpu_done which gets set in pat_init().

Also, I don't see the static_cpu_has() check I suggested yesterday - we need to
check the feature bits if PAT gets disabled early on some old Intels.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.