Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1560858 > unrolled thread

[PATCH 02/13] x86/microcode: Use own MSR accessors

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 02/13] x86/microcode: Use own MSR accessors Borislav Petkov <bp@alien8.de> - 2017-01-17 18:50 +0100
    Re: [PATCH 02/13] x86/microcode: Use own MSR accessors Thomas Gleixner <tglx@linutronix.de> - 2017-01-17 19:20 +0100
      Re: [PATCH 02/13] x86/microcode: Use own MSR accessors Borislav Petkov <bp@alien8.de> - 2017-01-17 19:30 +0100
        Re: [PATCH 02/13] x86/microcode: Use own MSR accessors Thomas Gleixner <tglx@linutronix.de> - 2017-01-17 20:20 +0100
          Re: [PATCH 02/13] x86/microcode: Use own MSR accessors Borislav Petkov <bp@alien8.de> - 2017-01-17 23:50 +0100
            Re: [PATCH 02/13] x86/microcode: Use own MSR accessors Thomas Gleixner <tglx@linutronix.de> - 2017-01-18 11:00 +0100

#1560858 — [PATCH 02/13] x86/microcode: Use own MSR accessors

From: Borislav Petkov <bp@suse.de>

Having tracepoints to the MSR accessors makes them unsuitable for early
microcode loading: think 32-bit before paging is enabled and us chasing
pointers to test whether a tracepoint is enabled or not. Results in a
reliable triple fault.

Thus, define microcode loader-specific MSR accessors which do the bare
minimum of reading or writing an MSR and nothing more.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/microcode.h       | 29 +++++++++++++++++++++--------
 arch/x86/include/asm/microcode_intel.h |  4 ++--
 arch/x86/kernel/cpu/microcode/amd.c    |  6 +++---
 arch/x86/kernel/cpu/microcode/intel.c  |  4 ++--
 4 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 38711df3bcb5..fbecea6e46e2 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -5,20 +5,33 @@
 #include <linux/earlycpio.h>
 #include <linux/initrd.h>
 
-#define native_rdmsr(msr, val1, val2)			\
+static inline unsigned long long __rdmsr(unsigned int msr)
+{
+	DECLARE_ARGS(val, low, high);
+
+	asm volatile("1: rdmsr\n"
+		     "2:\n"
+		     : EAX_EDX_RET(val, low, high) : "c" (msr));
+	return EAX_EDX_VAL(val, low, high);
+}
+
+#define microcode_rdmsr(msr, val1, val2)		\
 do {							\
-	u64 __val = native_read_msr((msr));		\
+	u64 __val = __rdmsr((msr));			\
 	(void)((val1) = (u32)__val);			\
 	(void)((val2) = (u32)(__val >> 32));		\
 } while (0)
 
-#define native_wrmsr(msr, low, high)			\
-	native_write_msr(msr, low, high)
+static inline void microcode_wrmsr(unsigned int msr, u64 val)
+{
+	u32 low, high;
+
+	low  = (u32)val;
+	high = (u32)(val >> 32);
 
-#define native_wrmsrl(msr, val)				\
-	native_write_msr((msr),				\
-			 (u32)((u64)(val)),		\
-			 (u32)((u64)(val) >> 32))
+	asm volatile("wrmsr\n"
+		     :: "c" (msr), "a"(low), "d" (high) : "memory");
+}
 
 struct ucode_patch {
 	struct list_head plist;
diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index e793fc9a9b20..7330b2b79484 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -56,13 +56,13 @@ static inline u32 intel_get_microcode_revision(void)
 {
 	u32 rev, dummy;
 
-	native_wrmsrl(MSR_IA32_UCODE_REV, 0);
+	microcode_wrmsr(MSR_IA32_UCODE_REV, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
 	native_cpuid_eax(1);
 
 	/* get the current revision from MSR 0x8B */
-	native_rdmsr(MSR_IA32_UCODE_REV, dummy, rev);
+	microcode_rdmsr(MSR_IA32_UCODE_REV, dummy, rev);
 
 	return rev;
 }
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 6a31e2691f3a..3f89e6712afe 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -198,10 +198,10 @@ static int __apply_microcode_amd(struct microcode_amd *mc_amd)
 {
 	u32 rev, dummy;
 
-	native_wrmsrl(MSR_AMD64_PATCH_LOADER, (u64)(long)&mc_amd->hdr.data_code);
+	microcode_wrmsr(MSR_AMD64_PATCH_LOADER, (u64)(long)&mc_amd->hdr.data_code);
 
 	/* verify patch application was successful */
-	native_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
+	microcode_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
 	if (rev != mc_amd->hdr.patch_id)
 		return -1;
 
@@ -656,7 +656,7 @@ bool check_current_patch_level(u32 *rev, bool early)
 	bool ret = false;
 	u32 *levels;
 
-	native_rdmsr(MSR_AMD64_PATCH_LEVEL, lvl, dummy);
+	microcode_rdmsr(MSR_AMD64_PATCH_LEVEL, lvl, dummy);
 
 	if (IS_ENABLED(CONFIG_X86_32) && early)
 		levels = (u32 *)__pa_nodebug(&final_levels);
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 8325d8a09ab0..1c6f12fa3108 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -387,7 +387,7 @@ static int collect_cpu_info_early(struct ucode_cpu_info *uci)
 
 	if ((model >= 5) || (family > 6)) {
 		/* get processor flags from MSR 0x17 */
-		native_rdmsr(MSR_IA32_PLATFORM_ID, val[0], val[1]);
+		microcode_rdmsr(MSR_IA32_PLATFORM_ID, val[0], val[1]);
 		csig.pf = 1 << ((val[1] >> 18) & 7);
 	}
 
@@ -582,7 +582,7 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 		return 0;
 
 	/* write microcode via MSR 0x79 */
-	native_wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
+	microcode_wrmsr(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
 
 	rev = intel_get_microcode_revision();
 	if (rev != mc->hdr.rev)
-- 
2.11.0

[toc] | [next] | [standalone]

#1560884

On Tue, 17 Jan 2017, Borislav Petkov wrote:
> diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
> index 38711df3bcb5..fbecea6e46e2 100644
> --- a/arch/x86/include/asm/microcode.h
> +++ b/arch/x86/include/asm/microcode.h
> @@ -5,20 +5,33 @@
>  #include <linux/earlycpio.h>
>  #include <linux/initrd.h>
>  
> -#define native_rdmsr(msr, val1, val2)			\
> +static inline unsigned long long __rdmsr(unsigned int msr)
> +{
> +	DECLARE_ARGS(val, low, high);
> +
> +	asm volatile("1: rdmsr\n"
> +		     "2:\n"
> +		     : EAX_EDX_RET(val, low, high) : "c" (msr));
> +	return EAX_EDX_VAL(val, low, high);
> +}
> +
> +#define microcode_rdmsr(msr, val1, val2)		\
>  do {							\
> -	u64 __val = native_read_msr((msr));		\
> +	u64 __val = __rdmsr((msr));			\
>  	(void)((val1) = (u32)__val);			\
>  	(void)((val2) = (u32)(__val >> 32));		\
>  } while (0)
>  
> -#define native_wrmsr(msr, low, high)			\
> -	native_write_msr(msr, low, high)
> +static inline void microcode_wrmsr(unsigned int msr, u64 val)
> +{
> +	u32 low, high;
> +
> +	low  = (u32)val;
> +	high = (u32)(val >> 32);
>  
> -#define native_wrmsrl(msr, val)				\
> -	native_write_msr((msr),				\
> -			 (u32)((u64)(val)),		\
> -			 (u32)((u64)(val) >> 32))
> +	asm volatile("wrmsr\n"
> +		     :: "c" (msr), "a"(low), "d" (high) : "memory");
> +}

msr.h already has:

wrmsr_notrace() so adding wrmsrl_notrace() to it should be a nobrainer.

Providing rdmsr_notrace() in msr.h is a natural extension of the existing
interfaces.

That would get rid of all the extra microcode specific MSR accessors which
are just yet another copy of stuff in msr.h.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1560894

On Tue, Jan 17, 2017 at 06:51:06PM +0100, Thomas Gleixner wrote:
> That would get rid of all the extra microcode specific MSR accessors which
> are just yet another copy of stuff in msr.h.

Well, I did think about reusing those but last time I did, they received
those tracepoints (apparently, we're sprinkling dumb tracepoints left
and right because good ol' staring at the code is just too hard) which
simply doesn't work on 32-bit before paging is enabled.

Then, __native_write_msr_notrace() has exception handling which doesn't
work before paging has been enabled on 32-bit - this is when the 32-bit
microcode update path happens due to paging hardware bugs in CPUs which
are fixed in microcode. So we must run that early on 32-bit.

So before someone decides to add more "functionality" to the generic MSR
accessors and break the microcode loader once more, I'd really really
prefer to have private accessors. They're small enough so shouldn't be
that much of a bloat.

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1560949

On Tue, 17 Jan 2017, Borislav Petkov wrote:

> On Tue, Jan 17, 2017 at 06:51:06PM +0100, Thomas Gleixner wrote:
> > That would get rid of all the extra microcode specific MSR accessors which
> > are just yet another copy of stuff in msr.h.
> 
> Well, I did think about reusing those but last time I did, they received
> those tracepoints (apparently, we're sprinkling dumb tracepoints left
> and right because good ol' staring at the code is just too hard) which
> simply doesn't work on 32-bit before paging is enabled.
>
> Then, __native_write_msr_notrace() has exception handling which doesn't
> work before paging has been enabled on 32-bit - this is when the 32-bit
> microcode update path happens due to paging hardware bugs in CPUs which
> are fixed in microcode. So we must run that early on 32-bit.

Well, the exception handling is irrelevant in that case. If the MSR access
succeeds then the exception handling is not invoked. If it fails then it
does not matter much whether you die from the MSR #GP or from the exception
handling attempt #GP.

> So before someone decides to add more "functionality" to the generic MSR
> accessors and break the microcode loader once more, I'd really really
> prefer to have private accessors. They're small enough so shouldn't be
> that much of a bloat.

I can understand that, but we better have real native functions in msr.h
which do exactly what they are supposed to do: read or write the MSR,
nothing else. Add a comment to those function which should be immutable and
threaten that offenders will be slapped with stinking trouts or frozen
sharks.

We really want to have a single place for all of this MSR stuff, because
following your argumentation will be a perfect precedence for more private
implementations.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1561071

On Tue, Jan 17, 2017 at 08:12:24PM +0100, Thomas Gleixner wrote:
> We really want to have a single place for all of this MSR stuff, because
> following your argumentation will be a perfect precedence for more private
> implementations.

Yes, you're so right. Ok, how about this untested thing. The warning
should be saucy enough :-)

---
From: Borislav Petkov <bp@suse.de>
Date: Tue, 17 Jan 2017 23:29:10 +0100
Subject: [PATCH] x86/MSR: Carve out bare minimum accessors

Add __rdmsr() and __wrmsr() which *only* read and write an MSR with
exception handling. Those are going to be used in early code, like the
microcode loader, which cannot stomach tracing code piggybacking on the
MSR operation.

While at it, get rid of __native_write_msr_notrace().

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/apic.h |  2 +-
 arch/x86/include/asm/msr.h  | 51 +++++++++++++++++++++++++++------------------
 2 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 0c5fbc68e82d..eff8e36aaf72 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -195,7 +195,7 @@ static inline void native_apic_msr_write(u32 reg, u32 v)
 
 static inline void native_apic_msr_eoi_write(u32 reg, u32 v)
 {
-	wrmsr_notrace(APIC_BASE_MSR + (APIC_EOI >> 4), APIC_EOI_ACK, 0);
+	__wrmsr(APIC_BASE_MSR + (APIC_EOI >> 4), APIC_EOI_ACK, 0);
 }
 
 static inline u32 native_apic_msr_read(u32 reg)
diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index db0b90c3b03e..898dba2e2e2c 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -80,7 +80,14 @@ static inline void do_trace_read_msr(unsigned int msr, u64 val, int failed) {}
 static inline void do_trace_rdpmc(unsigned int msr, u64 val, int failed) {}
 #endif
 
-static inline unsigned long long native_read_msr(unsigned int msr)
+/*
+ * __rdmsr() and __wrmsr() are the two primitives which are the bare minimum MSR
+ * accessors and should not have any tracing or other functionality piggybacking
+ * on them - those are *purely* for accessing MSRs and nothing more. So don't even
+ * think of extending them - you will be slapped with a stinking trout or a frozen
+ * shark will reach you, wherever you are! You've been warned.
+ */
+static inline unsigned long long notrace __rdmsr(unsigned int msr)
 {
 	DECLARE_ARGS(val, low, high);
 
@@ -88,11 +95,30 @@ static inline unsigned long long native_read_msr(unsigned int msr)
 		     "2:\n"
 		     _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_rdmsr_unsafe)
 		     : EAX_EDX_RET(val, low, high) : "c" (msr));
-	if (msr_tracepoint_active(__tracepoint_read_msr))
-		do_trace_read_msr(msr, EAX_EDX_VAL(val, low, high), 0);
+
 	return EAX_EDX_VAL(val, low, high);
 }
 
+static inline void notrace __wrmsr(unsigned int msr, u32 low, u32 high)
+{
+	asm volatile("1: wrmsr\n"
+		     "2:\n"
+		     _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe)
+		     : : "c" (msr), "a"(low), "d" (high) : "memory");
+}
+
+static inline unsigned long long native_read_msr(unsigned int msr)
+{
+	unsigned long long val;
+
+	val = __rdmsr(msr);
+
+	if (msr_tracepoint_active(__tracepoint_read_msr))
+		do_trace_read_msr(msr, val, 0);
+
+	return val;
+}
+
 static inline unsigned long long native_read_msr_safe(unsigned int msr,
 						      int *err)
 {
@@ -116,29 +142,14 @@ static inline unsigned long long native_read_msr_safe(unsigned int msr,
 
 /* Can be uninlined because referenced by paravirt */
 static inline void notrace
-__native_write_msr_notrace(unsigned int msr, u32 low, u32 high)
-{
-	asm volatile("1: wrmsr\n"
-		     "2:\n"
-		     _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe)
-		     : : "c" (msr), "a"(low), "d" (high) : "memory");
-}
-
-/* Can be uninlined because referenced by paravirt */
-static inline void notrace
 native_write_msr(unsigned int msr, u32 low, u32 high)
 {
-	__native_write_msr_notrace(msr, low, high);
+	__wrmsr(msr, low, high);
+
 	if (msr_tracepoint_active(__tracepoint_write_msr))
 		do_trace_write_msr(msr, ((u64)high << 32 | low), 0);
 }
 
-static inline void
-wrmsr_notrace(unsigned int msr, u32 low, u32 high)
-{
-	__native_write_msr_notrace(msr, low, high);
-}
-
 /* Can be uninlined because referenced by paravirt */
 static inline int notrace
 native_write_msr_safe(unsigned int msr, u32 low, u32 high)
-- 
2.11.0

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1561421

On Tue, 17 Jan 2017, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> Date: Tue, 17 Jan 2017 23:29:10 +0100
> Subject: [PATCH] x86/MSR: Carve out bare minimum accessors
> 
> Add __rdmsr() and __wrmsr() which *only* read and write an MSR with
> exception handling. Those are going to be used in early code, like the
> microcode loader, which cannot stomach tracing code piggybacking on the
> MSR operation.
> 
> While at it, get rid of __native_write_msr_notrace().

Looks good.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web