Messages in this thread Patch in this message | | | Date | Sun, 11 Sep 2022 12:16:32 +0200 | From | Borislav Petkov <> | 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
| |