lkml.org 
[lkml]   [2022]   [Sep]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2022-09-11 12:16    [W:0.215 / U:0.512 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site