Re: [PATCH v4 16/16] x86/mtrr: simplify mtrr_ops initialization

[Borislav Petkov]

On Sun, Oct 30, 2022 at 04:05:29PM +0100, Juergen Gross wrote:
> As the specific ops variables are available for X86_32 only, this
> would require to add an "#ifdef CONFIG_X86_32" around the code block
> doing the assignments. Otherwise the build would fail.

Well, it looks like my compiler is smart enough and eliminates all that
dead code, see diff below.

I've added the asm markers "#begin" and "#end" and the resulting asm
looks like this:

# arch/x86/kernel/cpu/mtrr/mtrr.c:666:          asm volatile("#begin");
        call    __sanitizer_cov_trace_pc        #
#APP
# 666 "arch/x86/kernel/cpu/mtrr/mtrr.c" 1
        #begin
# 0 "" 2
# arch/x86/kernel/cpu/mtrr/mtrr.c:693:          asm volatile("#end");
# 693 "arch/x86/kernel/cpu/mtrr/mtrr.c" 1
        #end
# 0 "" 2
# arch/x86/kernel/cpu/mtrr/mtrr.c:630:  phys_addr = 32;
#NO_APP

which basically says that all between line 666 and 693 has been
eliminated.

I have the suspicion, though, that clang might not be that smart.

Lemme test it a bit.

---

diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 7ba68356c0ff..d499c83b2ad7 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -663,25 +663,26 @@ void __init mtrr_bp_init(void)
 			phys_addr = 32;
 		}
 	} else {
+		asm volatile("#begin");
 		switch (boot_cpu_data.x86_vendor) {
 		case X86_VENDOR_AMD:
 			if (cpu_feature_enabled(X86_FEATURE_K6_MTRR)) {
 				/* Pre-Athlon (K6) AMD CPU MTRRs */
-				mtrr_if = vendor_mtrr_ops(amd_mtrr_ops);
+				mtrr_if = &amd_mtrr_ops;
 				size_or_mask = SIZE_OR_MASK_BITS(32);
 				size_and_mask = 0;
 			}
 			break;
 		case X86_VENDOR_CENTAUR:
 			if (cpu_feature_enabled(X86_FEATURE_CENTAUR_MCR)) {
-				mtrr_if = vendor_mtrr_ops(centaur_mtrr_ops);
+				mtrr_if = &centaur_mtrr_ops;
 				size_or_mask = SIZE_OR_MASK_BITS(32);
 				size_and_mask = 0;
 			}
 			break;
 		case X86_VENDOR_CYRIX:
 			if (cpu_feature_enabled(X86_FEATURE_CYRIX_ARR)) {
-				mtrr_if = vendor_mtrr_ops(cyrix_mtrr_ops);
+				mtrr_if = &cyrix_mtrr_ops;
 				size_or_mask = SIZE_OR_MASK_BITS(32);
 				size_and_mask = 0;
 			}
@@ -689,6 +690,7 @@ void __init mtrr_bp_init(void)
 		default:
 			break;
 		}
+		asm volatile("#end");
 	}
 
 	if (mtrr_if) {
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.h b/arch/x86/kernel/cpu/mtrr/mtrr.h
index 7a7387356192..02eb5871492d 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.h
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.h
@@ -68,11 +68,6 @@ void mtrr_wrmsr(unsigned, unsigned, unsigned);
 extern const struct mtrr_ops amd_mtrr_ops;
 extern const struct mtrr_ops cyrix_mtrr_ops;
 extern const struct mtrr_ops centaur_mtrr_ops;
-#ifdef CONFIG_X86_64
-#define vendor_mtrr_ops(x) NULL
-#else
-#define vendor_mtrr_ops(x) &(x)
-#endif
 
 extern int changed_by_mtrr_cleanup;
 extern int mtrr_cleanup(unsigned address_bits);


-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette