[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 03/10] x86/mtrr: replace use_intel() with a local flag



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.


...

  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).

Fine with me.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.