Re: [PATCH v4 13/16] x86: decouple pat and mtrr handling

From: Borislav Petkov <bp@alien8.de>
To: Juergen Gross <jgross@suse.com>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Andy Lutomirski <luto@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH v4 13/16] x86: decouple pat and mtrr handling
Date: Sat, 29 Oct 2022 14:15:03 +0200	[thread overview]
Message-ID: <Y10ZR7KtWTdzl6hm@zn.tnic> (raw)
In-Reply-To: <20221004081023.32402-14-jgross@suse.com>

On Tue, Oct 04, 2022 at 10:10:20AM +0200, Juergen Gross wrote:
> diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
> index 66a209f7eb86..ce1f3246a3e3 100644
> --- a/arch/x86/mm/pat/memtype.c
> +++ b/arch/x86/mm/pat/memtype.c
> @@ -43,6 +43,7 @@
>  #include <linux/rbtree.h>
>  
>  #include <asm/cacheflush.h>
> +#include <asm/cacheinfo.h>
>  #include <asm/processor.h>
>  #include <asm/tlbflush.h>
>  #include <asm/x86_init.h>
> @@ -60,41 +61,34 @@
>  #undef pr_fmt
>  #define pr_fmt(fmt) "" fmt
>  
> -static bool __read_mostly pat_bp_initialized;
>  static bool __read_mostly pat_disabled = !IS_ENABLED(CONFIG_X86_PAT);
> -static bool __initdata pat_force_disabled = !IS_ENABLED(CONFIG_X86_PAT);
> -static bool __read_mostly pat_bp_enabled;
> -static bool __read_mostly pat_cm_initialized;
> +static u64 __read_mostly pat_msr_val;

__ro_after_init perhaps. Looks like this thing gets set only during CPU
init anyway.

...

> +void __init pat_bp_init(void)
>  {
> +	struct cpuinfo_x86 *c = &boot_cpu_data;
>  	u64 pat = 0;

Might as well get rid of that local var and use pat_msr_val directly.

>  
> -	if (pat_cm_initialized)
> -		return;
> +#ifndef CONFIG_X86_PAT
> +	pr_info_once("x86/PAT: PAT support disabled because CONFIG_X86_PAT is disabled in the kernel.\n");
> +#endif

	if (!IS_ENABLED(CONFIG...))
		pr_info_once(..)

and one more ifdeffery is gone.

> -	if (boot_cpu_has(X86_FEATURE_PAT)) {
> -		/*
> -		 * CPU supports PAT. Set PAT table to be consistent with
> -		 * PAT MSR. This case supports "nopat" boot option, and
> -		 * virtual machine environments which support PAT without
> -		 * MTRRs. In specific, Xen has unique setup to PAT MSR.
> -		 *
> -		 * If PAT MSR returns 0, it is considered invalid and emulates
> -		 * as No PAT.
> -		 */
> +	if (!boot_cpu_has(X86_FEATURE_PAT))

check_for_deprecated_apis: Warning: arch/x86/mm/pat/memtype.c:265: Do not use boot_cpu_has() - use cpu_feature_enabled() instead.

> +		pat_disable("PAT not supported by the CPU.");
> +	else
>  		rdmsrl(MSR_IA32_CR_PAT, pat);
> -	}

...

> -void pat_init(void)
> -{
> -	u64 pat;
> -	struct cpuinfo_x86 *c = &boot_cpu_data;
> -
> -#ifndef CONFIG_X86_PAT
> -	pr_info_once("x86/PAT: PAT support disabled because CONFIG_X86_PAT is disabled in the kernel.\n");
> -#endif
> +	/* Xen PV doesn't allow to set PAT MSR, but all cache modes are fine. */

... all cache modes are supported" perhaps? "fine" sounds weird here.

> +	if (pat_disabled || cpu_feature_enabled(X86_FEATURE_XENPV)) {
> +		init_cache_modes(pat);
>  

Drop that resulting newline here.

> -	if (pat_disabled)
>  		return;
> +	}
>  
>  	if ((c->x86_vendor == X86_VENDOR_INTEL) &&
>  	    (((c->x86 == 0x6) && (c->x86_model <= 0xd)) ||

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette