linux-kernel - Re: [PATCH v3 03/10] x86/mtrr: replace use

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yx21cizZHNzD38z7@nazgul.tnic>
Date:   Sun, 11 Sep 2022 12:16:32 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Juergen Gross <jgross@...e.com>
Cc:     xen-devel@...ts.xenproject.org, x86@...nel.org,
        linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH v3 03/10] x86/mtrr: replace use_intel() with a local flag

On Thu, Sep 08, 2022 at 10:49:07AM +0200, Juergen Gross wrote:
> diff --git a/arch/x86/include/asm/cacheinfo.h b/arch/x86/include/asm/cacheinfo.h
> index 86b2e0dcc4bf..1aeafa9888f7 100644
> --- a/arch/x86/include/asm/cacheinfo.h
> +++ b/arch/x86/include/asm/cacheinfo.h
> @@ -2,6 +2,11 @@
>  #ifndef _ASM_X86_CACHEINFO_H
>  #define _ASM_X86_CACHEINFO_H
>  
> +/* Kernel controls MTRR and/or PAT MSRs. */
> +extern unsigned int cache_generic;

So this should be called something more descriptive like

	memory_caching_types

or so to denote that this is a bitfield of supported memory caching
technologies. The code then would read as

	if (memory_caching_types & CACHE_MTRR)

The name's still not optimal tho - needs more brooding over.

> +#define CACHE_GENERIC_MTRR 0x01
> +#define CACHE_GENERIC_PAT  0x02

And those should be CACHE_{MTRR,PAT}.

>  void cacheinfo_amd_init_llc_id(struct cpuinfo_x86 *c, int cpu);
>  void cacheinfo_hygon_init_llc_id(struct cpuinfo_x86 *c, int cpu);
>  
> diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
> index 66556833d7af..3b05d3ade7a6 100644
> --- a/arch/x86/kernel/cpu/cacheinfo.c
> +++ b/arch/x86/kernel/cpu/cacheinfo.c
> @@ -35,6 +35,9 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_llc_shared_map);
>  /* Shared L2 cache maps */
>  DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_l2c_shared_map);
>  
> +/* Kernel controls MTRR and/or PAT MSRs. */
> +unsigned int cache_generic;

This should either be __ro_after_init and initialized to 0 or you need
accessors...

>  u32 num_var_ranges;
> -static bool __mtrr_enabled;
> -
> -static bool mtrr_enabled(void)
> -{
> -	return __mtrr_enabled;
> -}
> +static bool mtrr_enabled;

Hmm, I don't like this. There's way too many boolean flags in the mtrr
code. There's mtrr_state.enabled too. ;-\

Can we set (or clear) X86_FEATURE_MTRR to denote MTRR enablement status
and get rid of one more boolean flag?

...

>  void __init mtrr_bp_init(void)
>  {
> +	bool use_generic = false;
>  	u32 phys_addr;
>  
>  	init_ifs();
> @@ -694,6 +691,7 @@ void __init mtrr_bp_init(void)
>  
>  	if (boot_cpu_has(X86_FEATURE_MTRR)) {
>  		mtrr_if = &generic_mtrr_ops;
> +		use_generic = true;
>  		size_or_mask = SIZE_OR_MASK_BITS(36);
>  		size_and_mask = 0x00f00000;
>  		phys_addr = 36;
> @@ -755,15 +753,18 @@ void __init mtrr_bp_init(void)
>  	}
>  
>  	if (mtrr_if) {
> -		__mtrr_enabled = true;
> -		set_num_var_ranges();
> +		mtrr_enabled = true;
> +		set_num_var_ranges(use_generic);

You don't need use_generic either:

		set_num_var_ranges(mtrr_if == generic_mtrr_ops);

(The reason being I wanna get rid of that nasty minefield of boolean
vars all round that code).

-- 
Regards/Gruss,
    Boris.

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