Messages in this thread | | | Date | Wed, 28 Sep 2022 12:17:08 +0200 | From | Juergen Gross <> | Subject | Re: [PATCH v3 03/10] x86/mtrr: replace use_intel() with a local flag |
| |
On 12.09.22 11:10, Juergen Gross wrote: > On 11.09.22 12:16, Borislav Petkov wrote: >> 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 > > In the end this variable doesn't specify which caching types are available, > but the ways to select/control the caching types. > > So what about "memory_caching_select" or "memory_caching_control" instead? > >> 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}. > > Fine with me. > >>> 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... > > Okay. > >> >>> 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? > > I'll have a look.
Hmm, this might be a little bit risky.
It can be done, but then X86_FEATURE_MTRR could be set even for cpus NOT supporting it (the 32-bit special cases AMD, CENTAUR, CYRIX).
So we have the following alternatives:
- do the switch to X86_FEATURE_MTRR risking code breakage for later code changes querying X86_FEATURE_MTRR and assuming the MTRR MSRs being available
- keep the current bool
- replace the bool with mtrr_if != NULL
- add a new synthetic feature, e.g. X86_FEATURE_MTRR_ENABLED (which in fact would be just a replacement of the current bool)
My preference would be the replacement with mtrr_if != NULL.
Juergen [unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] | |