[PATCH v4] x86/mtrr: Check if fixed MTRRs exist before saving them

[PATCH v4] x86/mtrr: Check if fixed MTRRs exist before saving them

[Andi Kleen]

MTRRs have a obsolete fixed variant for fine grained
caching control of the 640K-1MB region. This fixed variant has a
separate capability bit in the MTRR capability MSR. Most of the MTRR code
checks this capability bit before trying to access the fixed MTRR MSRs,
except in one place. This patch fixes this place to also
check the capability.

Otherwise there will be a WARN_ON due to the #GP when the respective MSRs
are accessed to save them.

Fixes: 2b1f6278d77c ("[PATCH] x86: Save the MTRRs of the BSP before booting an AP")
Signed-off-by: Andi Kleen <ak@linux.intel.com>

---

v2: Add Fixes tag and expand description.
v3: Expand description
v4: Expand description
---
 arch/x86/kernel/cpu/mtrr/mtrr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 767bf1c71aad..2a2fc14955cd 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -609,7 +609,7 @@ void mtrr_save_state(void)
 {
 	int first_cpu;
 
-	if (!mtrr_enabled())
+	if (!mtrr_enabled() || !mtrr_state.have_fixed)
 		return;
 
 	first_cpu = cpumask_first(cpu_online_mask);
-- 
2.45.1

Re: [PATCH v4] x86/mtrr: Check if fixed MTRRs exist before saving them

[Thomas Gleixner]

On Fri, Jun 07 2024 at 12:44, Andi Kleen wrote:
> MTRRs have a obsolete fixed variant for fine grained
> caching control of the 640K-1MB region. This fixed variant has a
> separate capability bit in the MTRR capability MSR. Most of the MTRR code
> checks this capability bit before trying to access the fixed MTRR MSRs,
> except in one place. This patch fixes this place to also
> check the capability.

I don't know how many times I have told you that "This patch ..." has no
place in change logs. Just in case you can't find the reference:

  git grep 'This patch' Documentation/process/

While at it you might also hit the format shortcut in your favourite
editor to actually make the above paragraph properly justified.

Also the content of this "changelog" is close to word salad. It's not
that hard to follow the documented expectations of change logs:

  "It’s also useful to structure the changelog into several paragraphs
   and not lump everything together into a single one. A good structure
   is to explain the context, the problem and the solution in separate
   paragraphs and this order."

It's irrelevant whether most code checks the bit, what's relevant is
that a particular piece of code does NOT check it. It's also interesting
what the resulting problem is, i.e. reading gunk, #GP or whatever and
why this suddenly matters and did not explode in decades, but that's
nowhere explained. So why does this patch matter?

It's really not rocket science to write a coherent changelog.

Thanks,

        tglx