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

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:
> From: Borislav Petkov 
> 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.

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

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:
> From: Borislav Petkov 
> 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.

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

[Borislav Petkov]

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

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

[Borislav Petkov]

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

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

[Thomas Gleixner]

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

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

[Thomas Gleixner]

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

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

[Borislav Petkov]

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.

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

[Borislav Petkov]

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.

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

[Thomas Gleixner]

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 
>  #include 
>  
> -#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

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

[Thomas Gleixner]

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 
>  #include 
>  
> -#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