Improve non-"safe" MSR access failure handling

These hooks match the _safe variants, so name them accordingly. This will make room for unsafe PV hooks.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/paravirt.h       | 33 +++++++++++++++++----------------
 arch/x86/include/asm/paravirt_types.h |  8 ++++----
 arch/x86/kernel/paravirt.c            |  4 ++--
 arch/x86/xen/enlighten.c              |  4 ++--
 4 files changed, 25 insertions(+), 24 deletions(-)

arch/x86/include/asm/paravirt.h +17 -16

index f6192502149e..2e49228ed9a3 100644
@@ -129,34 +129,35 @@ static inline void wbinvd(void)
 
 #define get_kernel_rpl()  (pv_info.kernel_rpl)
 
-static inline u64 paravirt_read_msr(unsigned msr, int *err)
+static inline u64 paravirt_read_msr_safe(unsigned msr, int *err)
 {
-	return PVOP_CALL2(u64, pv_cpu_ops.read_msr, msr, err);
+	return PVOP_CALL2(u64, pv_cpu_ops.read_msr_safe, msr, err);
 }
 
-static inline int paravirt_write_msr(unsigned msr, unsigned low, unsigned high)
+static inline int paravirt_write_msr_safe(unsigned msr,
+					  unsigned low, unsigned high)
 {
-	return PVOP_CALL3(int, pv_cpu_ops.write_msr, msr, low, high);
+	return PVOP_CALL3(int, pv_cpu_ops.write_msr_safe, msr, low, high);
 }
 
 /* These should all do BUG_ON(_err), but our headers are too tangled. */
 #define rdmsr(msr, val1, val2)			\
 do {						\
 	int _err;				\
-	u64 _l = paravirt_read_msr(msr, &_err);	\
+	u64 _l = paravirt_read_msr_safe(msr, &_err);	\
 	val1 = (u32)_l;				\
 	val2 = _l >> 32;			\
 } while (0)
 
 #define wrmsr(msr, val1, val2)			\
 do {						\
-	paravirt_write_msr(msr, val1, val2);	\
+	paravirt_write_msr_safe(msr, val1, val2);	\
 } while (0)
 
 #define rdmsrl(msr, val)			\
 do {						\
 	int _err;				\
-	val = paravirt_read_msr(msr, &_err);	\
+	val = paravirt_read_msr_safe(msr, &_err);	\
 } while (0)
 
 static inline void wrmsrl(unsigned msr, u64 val)

@@ -164,23 +165,23 @@ static inline void wrmsrl(unsigned msr, u64 val)
 	wrmsr(msr, (u32)val, (u32)(val>>32));
 }
 
-#define wrmsr_safe(msr, a, b)	paravirt_write_msr(msr, a, b)
+#define wrmsr_safe(msr, a, b)	paravirt_write_msr_safe(msr, a, b)
 
 /* rdmsr with exception handling */
-#define rdmsr_safe(msr, a, b)			\
-({						\
-	int _err;				\
-	u64 _l = paravirt_read_msr(msr, &_err);	\
-	(*a) = (u32)_l;				\
-	(*b) = _l >> 32;			\
-	_err;					\
+#define rdmsr_safe(msr, a, b)				\
+({							\
+	int _err;					\
+	u64 _l = paravirt_read_msr_safe(msr, &_err);	\
+	(*a) = (u32)_l;					\
+	(*b) = _l >> 32;				\
+	_err;						\
 })
 
 static inline int rdmsrl_safe(unsigned msr, unsigned long long *p)
 {
 	int err;
 
-	*p = paravirt_read_msr(msr, &err);
+	*p = paravirt_read_msr_safe(msr, &err);
 	return err;
 }
 

arch/x86/include/asm/paravirt_types.h +0 -0

index 77db5616a473..5a06cccd36f0 100644

These hooks match the _safe variants, so name them accordingly. This will make room for unsafe PV hooks.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/paravirt.h       | 33 +++++++++++++++++----------------
 arch/x86/include/asm/paravirt_types.h |  8 ++++----
 arch/x86/kernel/paravirt.c            |  4 ++--
 arch/x86/xen/enlighten.c              |  4 ++--
 4 files changed, 25 insertions(+), 24 deletions(-)

arch/x86/include/asm/paravirt.h +17 -16

index f6192502149e..2e49228ed9a3 100644
@@ -129,34 +129,35 @@ static inline void wbinvd(void)
 
 #define get_kernel_rpl()  (pv_info.kernel_rpl)
 
-static inline u64 paravirt_read_msr(unsigned msr, int *err)
+static inline u64 paravirt_read_msr_safe(unsigned msr, int *err)
 {
-	return PVOP_CALL2(u64, pv_cpu_ops.read_msr, msr, err);
+	return PVOP_CALL2(u64, pv_cpu_ops.read_msr_safe, msr, err);
 }
 
-static inline int paravirt_write_msr(unsigned msr, unsigned low, unsigned high)
+static inline int paravirt_write_msr_safe(unsigned msr,
+					  unsigned low, unsigned high)
 {
-	return PVOP_CALL3(int, pv_cpu_ops.write_msr, msr, low, high);
+	return PVOP_CALL3(int, pv_cpu_ops.write_msr_safe, msr, low, high);
 }
 
 /* These should all do BUG_ON(_err), but our headers are too tangled. */
 #define rdmsr(msr, val1, val2)			\
 do {						\
 	int _err;				\
-	u64 _l = paravirt_read_msr(msr, &_err);	\
+	u64 _l = paravirt_read_msr_safe(msr, &_err);	\
 	val1 = (u32)_l;				\
 	val2 = _l >> 32;			\
 } while (0)
 
 #define wrmsr(msr, val1, val2)			\
 do {						\
-	paravirt_write_msr(msr, val1, val2);	\
+	paravirt_write_msr_safe(msr, val1, val2);	\
 } while (0)
 
 #define rdmsrl(msr, val)			\
 do {						\
 	int _err;				\
-	val = paravirt_read_msr(msr, &_err);	\
+	val = paravirt_read_msr_safe(msr, &_err);	\
 } while (0)
 
 static inline void wrmsrl(unsigned msr, u64 val)

@@ -164,23 +165,23 @@ static inline void wrmsrl(unsigned msr, u64 val)
 	wrmsr(msr, (u32)val, (u32)(val>>32));
 }
 
-#define wrmsr_safe(msr, a, b)	paravirt_write_msr(msr, a, b)
+#define wrmsr_safe(msr, a, b)	paravirt_write_msr_safe(msr, a, b)
 
 /* rdmsr with exception handling */
-#define rdmsr_safe(msr, a, b)			\
-({						\
-	int _err;				\
-	u64 _l = paravirt_read_msr(msr, &_err);	\
-	(*a) = (u32)_l;				\
-	(*b) = _l >> 32;			\
-	_err;					\
+#define rdmsr_safe(msr, a, b)				\
+({							\
+	int _err;					\
+	u64 _l = paravirt_read_msr_safe(msr, &_err);	\
+	(*a) = (u32)_l;					\
+	(*b) = _l >> 32;				\
+	_err;						\
 })
 
 static inline int rdmsrl_safe(unsigned msr, unsigned long long *p)
 {
 	int err;
 
-	*p = paravirt_read_msr(msr, &err);
+	*p = paravirt_read_msr_safe(msr, &err);
 	return err;
 }
 

arch/x86/include/asm/paravirt_types.h +0 -0

index 77db5616a473..5a06cccd36f0 100644

Quoted 96 lines · Andy Lutomirski

These hooks match the _safe variants, so name them accordingly. This will make room for unsafe PV hooks.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/paravirt.h       | 33 +++++++++++++++++----------------
 arch/x86/include/asm/paravirt_types.h |  8 ++++----
 arch/x86/kernel/paravirt.c            |  4 ++--
 arch/x86/xen/enlighten.c              |  4 ++--
 4 files changed, 25 insertions(+), 24 deletions(-)

arch/x86/include/asm/paravirt.h +17 -16

index f6192502149e..2e49228ed9a3 100644
@@ -129,34 +129,35 @@ static inline void wbinvd(void)
 
 #define get_kernel_rpl()  (pv_info.kernel_rpl)
 
-static inline u64 paravirt_read_msr(unsigned msr, int *err)
+static inline u64 paravirt_read_msr_safe(unsigned msr, int *err)
 {
-     return PVOP_CALL2(u64, pv_cpu_ops.read_msr, msr, err);
+     return PVOP_CALL2(u64, pv_cpu_ops.read_msr_safe, msr, err);
 }
 
-static inline int paravirt_write_msr(unsigned msr, unsigned low, unsigned high)
+static inline int paravirt_write_msr_safe(unsigned msr,
+                                       unsigned low, unsigned high)
 {
-     return PVOP_CALL3(int, pv_cpu_ops.write_msr, msr, low, high);
+     return PVOP_CALL3(int, pv_cpu_ops.write_msr_safe, msr, low, high);
 }
 
 /* These should all do BUG_ON(_err), but our headers are too tangled. */
 #define rdmsr(msr, val1, val2)                       \
 do {                                         \
      int _err;                               \
-     u64 _l = paravirt_read_msr(msr, &_err); \
+     u64 _l = paravirt_read_msr_safe(msr, &_err);    \
      val1 = (u32)_l;                         \
      val2 = _l >> 32;                        \
 } while (0)
 
 #define wrmsr(msr, val1, val2)                       \
 do {                                         \
-     paravirt_write_msr(msr, val1, val2);    \
+     paravirt_write_msr_safe(msr, val1, val2);       \
 } while (0)
 
 #define rdmsrl(msr, val)                     \
 do {                                         \
      int _err;                               \
-     val = paravirt_read_msr(msr, &_err);    \
+     val = paravirt_read_msr_safe(msr, &_err);       \
 } while (0)
 
 static inline void wrmsrl(unsigned msr, u64 val)

@@ -164,23 +165,23 @@ static inline void wrmsrl(unsigned msr, u64 val)
      wrmsr(msr, (u32)val, (u32)(val>>32));
 }
 
-#define wrmsr_safe(msr, a, b)        paravirt_write_msr(msr, a, b)
+#define wrmsr_safe(msr, a, b)        paravirt_write_msr_safe(msr, a, b)
 
 /* rdmsr with exception handling */
-#define rdmsr_safe(msr, a, b)                        \
-({                                           \
-     int _err;                               \
-     u64 _l = paravirt_read_msr(msr, &_err); \
-     (*a) = (u32)_l;                         \
-     (*b) = _l >> 32;                        \
-     _err;                                   \
+#define rdmsr_safe(msr, a, b)                                \
+({                                                   \
+     int _err;                                       \
+     u64 _l = paravirt_read_msr_safe(msr, &_err);    \
+     (*a) = (u32)_l;                                 \
+     (*b) = _l >> 32;                                \
+     _err;                                           \
 })
 
 static inline int rdmsrl_safe(unsigned msr, unsigned long long *p)
 {
      int err;
 
-     *p = paravirt_read_msr(msr, &err);
+     *p = paravirt_read_msr_safe(msr, &err);
      return err;
 }
 

arch/x86/include/asm/paravirt_types.h +0 -0

index 77db5616a473..5a06cccd36f0 100644

arch/x86/include/asm/paravirt_types.h struct pv_cpu_ops { · line 159

Quoted 9 lines

arch/x86/include/asm/paravirt_types.h +2 -4

@@ -155,10 +155,10 @@ struct pv_cpu_ops {
      void (*cpuid)(unsigned int *eax, unsigned int *ebx,
                    unsigned int *ecx, unsigned int *edx);
 
-     /* MSR, PMC and TSR operations.
-        err = 0/-EFAULT.  wrmsr returns 0/-EFAULT. */
-     u64 (*read_msr)(unsigned int msr, int *err);
-     int (*write_msr)(unsigned int msr, unsigned low, unsigned high);
+     /* MSR operations.
+        err = 0/-EIO.  wrmsr returns 0/-EIO. */

Please fix the comment format while you're touching this :

/*
* A sentence.
* Another sentence.
*/

Other than that:

Acked-by Borislav Petkov <[email protected]>

Quoted 96 lines · Andy Lutomirski

These hooks match the _safe variants, so name them accordingly. This will make room for unsafe PV hooks.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/paravirt.h       | 33 +++++++++++++++++----------------
 arch/x86/include/asm/paravirt_types.h |  8 ++++----
 arch/x86/kernel/paravirt.c            |  4 ++--
 arch/x86/xen/enlighten.c              |  4 ++--
 4 files changed, 25 insertions(+), 24 deletions(-)

arch/x86/include/asm/paravirt.h +17 -16

index f6192502149e..2e49228ed9a3 100644
@@ -129,34 +129,35 @@ static inline void wbinvd(void)
 
 #define get_kernel_rpl()  (pv_info.kernel_rpl)
 
-static inline u64 paravirt_read_msr(unsigned msr, int *err)
+static inline u64 paravirt_read_msr_safe(unsigned msr, int *err)
 {
-     return PVOP_CALL2(u64, pv_cpu_ops.read_msr, msr, err);
+     return PVOP_CALL2(u64, pv_cpu_ops.read_msr_safe, msr, err);
 }
 
-static inline int paravirt_write_msr(unsigned msr, unsigned low, unsigned high)
+static inline int paravirt_write_msr_safe(unsigned msr,
+                                       unsigned low, unsigned high)
 {
-     return PVOP_CALL3(int, pv_cpu_ops.write_msr, msr, low, high);
+     return PVOP_CALL3(int, pv_cpu_ops.write_msr_safe, msr, low, high);
 }
 
 /* These should all do BUG_ON(_err), but our headers are too tangled. */
 #define rdmsr(msr, val1, val2)                       \
 do {                                         \
      int _err;                               \
-     u64 _l = paravirt_read_msr(msr, &_err); \
+     u64 _l = paravirt_read_msr_safe(msr, &_err);    \
      val1 = (u32)_l;                         \
      val2 = _l >> 32;                        \
 } while (0)
 
 #define wrmsr(msr, val1, val2)                       \
 do {                                         \
-     paravirt_write_msr(msr, val1, val2);    \
+     paravirt_write_msr_safe(msr, val1, val2);       \
 } while (0)
 
 #define rdmsrl(msr, val)                     \
 do {                                         \
      int _err;                               \
-     val = paravirt_read_msr(msr, &_err);    \
+     val = paravirt_read_msr_safe(msr, &_err);       \
 } while (0)
 
 static inline void wrmsrl(unsigned msr, u64 val)

@@ -164,23 +165,23 @@ static inline void wrmsrl(unsigned msr, u64 val)
      wrmsr(msr, (u32)val, (u32)(val>>32));
 }
 
-#define wrmsr_safe(msr, a, b)        paravirt_write_msr(msr, a, b)
+#define wrmsr_safe(msr, a, b)        paravirt_write_msr_safe(msr, a, b)
 
 /* rdmsr with exception handling */
-#define rdmsr_safe(msr, a, b)                        \
-({                                           \
-     int _err;                               \
-     u64 _l = paravirt_read_msr(msr, &_err); \
-     (*a) = (u32)_l;                         \
-     (*b) = _l >> 32;                        \
-     _err;                                   \
+#define rdmsr_safe(msr, a, b)                                \
+({                                                   \
+     int _err;                                       \
+     u64 _l = paravirt_read_msr_safe(msr, &_err);    \
+     (*a) = (u32)_l;                                 \
+     (*b) = _l >> 32;                                \
+     _err;                                           \
 })
 
 static inline int rdmsrl_safe(unsigned msr, unsigned long long *p)
 {
      int err;
 
-     *p = paravirt_read_msr(msr, &err);
+     *p = paravirt_read_msr_safe(msr, &err);
      return err;
 }
 

arch/x86/include/asm/paravirt_types.h +0 -0

index 77db5616a473..5a06cccd36f0 100644

arch/x86/include/asm/paravirt_types.h struct pv_cpu_ops { · line 159

Quoted 9 lines

arch/x86/include/asm/paravirt_types.h +2 -4

@@ -155,10 +155,10 @@ struct pv_cpu_ops {
      void (*cpuid)(unsigned int *eax, unsigned int *ebx,
                    unsigned int *ecx, unsigned int *edx);
 
-     /* MSR, PMC and TSR operations.
-        err = 0/-EFAULT.  wrmsr returns 0/-EFAULT. */
-     u64 (*read_msr)(unsigned int msr, int *err);
-     int (*write_msr)(unsigned int msr, unsigned low, unsigned high);
+     /* MSR operations.
+        err = 0/-EIO.  wrmsr returns 0/-EIO. */

Please fix the comment format while you're touching this :

/*
* A sentence.
* Another sentence.
*/

Other than that:

Acked-by Borislav Petkov <[email protected]>

This demotes an OOPS and likely panic due to a failed non-"safe" MSR access to a WARN_ONCE and, for RDMSR, a return value of zero. If panic_on_oops is set, then failed unsafe MSR accesses will still oops and panic.

To be clear, this type of failure should *not* happen. This patch exists to minimize the chance of nasty undebuggable failures due on systems that used to work due to a now-fixed CONFIG_PARAVIRT=y bug.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/msr.h | 10 ++++++++--
 arch/x86/mm/extable.c      | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

arch/x86/include/asm/msr.h +4 -1

index 93fb7c1cffda..1487054a1a70 100644
@@ -92,7 +92,10 @@ static inline unsigned long long native_read_msr(unsigned int msr)
 {
 	DECLARE_ARGS(val, low, high);
 
-	asm volatile("rdmsr" : EAX_EDX_RET(val, low, high) : "c" (msr));
+	asm volatile("1: rdmsr\n"
+		     "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);

This demotes an OOPS and likely panic due to a failed non-"safe" MSR access to a WARN_ONCE and, for RDMSR, a return value of zero. If panic_on_oops is set, then failed unsafe MSR accesses will still oops and panic.

To be clear, this type of failure should *not* happen. This patch exists to minimize the chance of nasty undebuggable failures due on systems that used to work due to a now-fixed CONFIG_PARAVIRT=y bug.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/msr.h | 10 ++++++++--
 arch/x86/mm/extable.c      | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

arch/x86/include/asm/msr.h +4 -1

index 93fb7c1cffda..1487054a1a70 100644
@@ -92,7 +92,10 @@ static inline unsigned long long native_read_msr(unsigned int msr)
 {
 	DECLARE_ARGS(val, low, high);
 
-	asm volatile("rdmsr" : EAX_EDX_RET(val, low, high) : "c" (msr));
+	asm volatile("1: rdmsr\n"
+		     "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);

Quoted 32 lines · Andy Lutomirski

This demotes an OOPS and likely panic due to a failed non-"safe" MSR access to a WARN_ONCE and, for RDMSR, a return value of zero. If panic_on_oops is set, then failed unsafe MSR accesses will still oops and panic.

To be clear, this type of failure should *not* happen. This patch exists to minimize the chance of nasty undebuggable failures due on systems that used to work due to a now-fixed CONFIG_PARAVIRT=y bug.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/msr.h | 10 ++++++++--
 arch/x86/mm/extable.c      | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

arch/x86/include/asm/msr.h +4 -1

index 93fb7c1cffda..1487054a1a70 100644
@@ -92,7 +92,10 @@ static inline unsigned long long native_read_msr(unsigned int msr)
 {
      DECLARE_ARGS(val, low, high);
 
-     asm volatile("rdmsr" : EAX_EDX_RET(val, low, high) : "c" (msr));
+     asm volatile("1: rdmsr\n"
+                  "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);

arch/x86/include/asm/msr.h static inline unsigned long long native_read_msr_safe(unsigned int msr, · line 127

Quoted 7 lines

arch/x86/include/asm/msr.h +3 -1

@@ -119,7 +122,10 @@ static inline unsigned long long native_read_msr_safe(unsigned int msr,
 static inline void native_write_msr(unsigned int msr,
                                  unsigned low, unsigned high)
 {
-     asm volatile("wrmsr" : : "c" (msr), "a"(low), "d" (high) : "memory");
+     asm volatile("1: wrmsr\n"
+                  "2:\n"
+                  _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe)

This might be a good idea:

[    0.220066] cpuidle: using governor menu
[    0.224000] ------------[ cut here ]------------
[    0.224000] WARNING: CPU: 0 PID: 1 at arch/x86/mm/extable.c:74 ex_handler_wrmsr_unsafe+0x73/0x80()
[    0.224000] unchecked MSR access error: WRMSR to 0xdeadbeef (tried to write 0x000000000000caca)
[    0.224000] Modules linked in:
[    0.224000] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.5.0-rc7+ #7
[    0.224000] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
[    0.224000]  0000000000000000 ffff88007c0d7c08 ffffffff812f13a3 ffff88007c0d7c50
[    0.224000]  ffffffff81a40ffe ffff88007c0d7c40 ffffffff8105c3b1 ffffffff81717710
[    0.224000]  ffff88007c0d7d18 0000000000000000 ffffffff816207d0 0000000000000000
[    0.224000] Call Trace:
[    0.224000]  [<ffffffff812f13a3>] dump_stack+0x67/0x94
[    0.224000]  [<ffffffff8105c3b1>] warn_slowpath_common+0x91/0xd0
[    0.224000]  [<ffffffff816207d0>] ? amd_cpu_notify+0x40/0x40
[    0.224000]  [<ffffffff8105c43c>] warn_slowpath_fmt+0x4c/0x50
[    0.224000]  [<ffffffff816207d0>] ? amd_cpu_notify+0x40/0x40
[    0.224000]  [<ffffffff8131de53>] ? __this_cpu_preempt_check+0x13/0x20
[    0.224000]  [<ffffffff8104efe3>] ex_handler_wrmsr_unsafe+0x73/0x80

and it looks helpful and all but when you do it pretty early - for example I added a

         wrmsrl(0xdeadbeef, 0xcafe);

at the end of pat_bsp_init() and the machine explodes with an early panic. I'm wondering what is better - early panic or an early #GP from a missing MSR.

And more specifically, can we do better to handle the early case gracefully too?

We could probably remove that check and let custom fixups run early. I don't see any compelling reason to keep them disabled. That should probably be a separate change, though.

Quoted 4 lines · Andy Lutomirski

We could probably remove that check and let custom fixups run early. I don't see any compelling reason to keep them disabled. That should probably be a separate change, though.

Or we could just use the existing wrmsr_safe() code and not add this new special code at all.

Look, why are you doing this? We should get rid of the difference between wrmsr and wrmsr_safe(), not make it bigger.

Just make everything safe. There has never in the history of anything been an advantage to making things oops and to making things more complicated.

Why is rd/wrmsr() so magically important?

So yes, let's please warn. I'm okay with removing the panic_on_oops thing though. (But if anyone suggests that we should stop OOPSing on bad kernel page faults, I *will* fight back.)

Quoted 4 lines · Andy Lutomirski

So yes, let's please warn. I'm okay with removing the panic_on_oops thing though. (But if anyone suggests that we should stop OOPSing on bad kernel page faults, I *will* fight back.)

Bad kernel page faults are something completely different. They would be actual bugs regardless.

The MSR thing has *often* been just silly "this CPU doesn't do that MSR". Generic bootup setup code etc that just didn't know or care about the particular badly documented rule for one particular random CPU version and stepping.

In fact, when I say "often", I suspect I should really just say "always". I don't think we've ever found a case where oopsing would have been the right thing. But it has definitely caused lots of problems, especially in the early paths where your code doesn't even work right now.

Now, when it comes to the warning, I guess I could live with it, but I think it's stupid to make this a low-level exception handler thing.

So what I think should be done:

- make sure that wr/rdmsr_safe() actually works during very early
init. At some point it didn't.

- get rid of the current wrmsr/rdmsr *entirely*. It's shit.

- Add this wrapper:

#define wrmsr(msr, low, high) \
WARN_ON_ONCE(wrmsr_safe(msr, low, high))

and be done with it. We could even decide to make that WARN_ON_ONCE() be something we could configure out, because it's really a debugging thing and isn't like it should be all that fatal.

None of this insane complicated crap that buys us exactly *nothing*, and depends on fancy new exception handling support etc etc.

So what's the downside to just doing this simple thing?

A couple of the wrmsr users actually care about performance. These are the ones involved in context switching and, to a lesser extent, in switching in and out of guest mode.

A couple of the wrmsr users actually care about performance. These are the ones involved in context switching and, to a lesser extent, in switching in and out of guest mode.

Quoted 4 lines · Andy Lutomirski

A couple of the wrmsr users actually care about performance. These are the ones involved in context switching and, to a lesser extent, in switching in and out of guest mode.

.. ok, see the crossed emails.

I'd *much* rather special case the special cases. Not make the generic case something complex.

And *particularly* not make the generic case be something where people think it's sane to oops and kill the machine. Christ.

The code in my queue is, literally:

bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup,
struct pt_regs *regs, int trapnr)

{
    WARN_ONCE(1, "unchecked MSR access error: RDMSR from 0x%x",
          (unsigned int)regs->cx);

    /* Pretend that the read succeeded and returned 0. */
    regs->ip = ex_fixup_addr(fixup);
    regs->ax = 0;
    regs->dx = 0;
    return true;
}

EXPORT_SYMBOL(ex_handler_rdmsr_unsafe);

The code in my queue is, literally:

bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup,
struct pt_regs *regs, int trapnr)

{
    WARN_ONCE(1, "unchecked MSR access error: RDMSR from 0x%x",
          (unsigned int)regs->cx);

    /* Pretend that the read succeeded and returned 0. */
    regs->ip = ex_fixup_addr(fixup);
    regs->ax = 0;
    regs->dx = 0;
    return true;
}

EXPORT_SYMBOL(ex_handler_rdmsr_unsafe);

Quoted 15 lines · Andy Lutomirski

The code in my queue is, literally:

bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup,
struct pt_regs *regs, int trapnr)

{
    WARN_ONCE(1, "unchecked MSR access error: RDMSR from 0x%x",
          (unsigned int)regs->cx);

    /* Pretend that the read succeeded and returned 0. */
    regs->ip = ex_fixup_addr(fixup);
    regs->ax = 0;
    regs->dx = 0;
    return true;
}

Quoted 1 line

EXPORT_SYMBOL(ex_handler_rdmsr_unsafe);

I guess I can live with this, as long as we also extend the early-fault handling to work with the special exception handlers.

Quoted 15 lines · Andy Lutomirski

The code in my queue is, literally:

bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup,
struct pt_regs *regs, int trapnr)

{
    WARN_ONCE(1, "unchecked MSR access error: RDMSR from 0x%x",
          (unsigned int)regs->cx);

    /* Pretend that the read succeeded and returned 0. */
    regs->ip = ex_fixup_addr(fixup);
    regs->ax = 0;
    regs->dx = 0;
    return true;
}

Quoted 1 line

EXPORT_SYMBOL(ex_handler_rdmsr_unsafe);

I guess I can live with this, as long as we also extend the early-fault handling to work with the special exception handlers.

Quoted 18 lines · Linus Torvalds

Quoted 16 lines · Andy Lutomirski

The code in my queue is, literally:

bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup,
struct pt_regs *regs, int trapnr)

{
    WARN_ONCE(1, "unchecked MSR access error: RDMSR from 0x%x",
          (unsigned int)regs->cx);

    /* Pretend that the read succeeded and returned 0. */
    regs->ip = ex_fixup_addr(fixup);
    regs->ax = 0;
    regs->dx = 0;
    return true;
}

EXPORT_SYMBOL(ex_handler_rdmsr_unsafe);

Quoted 2 lines

I guess I can live with this, as long as we also extend the early-fault handling to work with the special exception handlers.

OK, will do. I need to rewrork the early IDT code a bit so it generates a real pt_regs layout, but that's arguably a cleanup anyway.

Quoted 18 lines · Linus Torvalds

Quoted 16 lines · Andy Lutomirski

The code in my queue is, literally:

bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup,
struct pt_regs *regs, int trapnr)

{
    WARN_ONCE(1, "unchecked MSR access error: RDMSR from 0x%x",
          (unsigned int)regs->cx);

    /* Pretend that the read succeeded and returned 0. */
    regs->ip = ex_fixup_addr(fixup);
    regs->ax = 0;
    regs->dx = 0;
    return true;
}

EXPORT_SYMBOL(ex_handler_rdmsr_unsafe);

Quoted 2 lines

I guess I can live with this, as long as we also extend the early-fault handling to work with the special exception handlers.

OK, will do. I need to rewrork the early IDT code a bit so it generates a real pt_regs layout, but that's arguably a cleanup anyway.

Quoted 20 lines · Andy Lutomirski

Quoted 18 lines · Linus Torvalds

Quoted 16 lines · Andy Lutomirski

The code in my queue is, literally:

bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup,
struct pt_regs *regs, int trapnr)

{
    WARN_ONCE(1, "unchecked MSR access error: RDMSR from 0x%x",
          (unsigned int)regs->cx);

    /* Pretend that the read succeeded and returned 0. */
    regs->ip = ex_fixup_addr(fixup);
    regs->ax = 0;
    regs->dx = 0;
    return true;
}

EXPORT_SYMBOL(ex_handler_rdmsr_unsafe);

Quoted 5 lines

Quoted 2 lines

I guess I can live with this, as long as we also extend the early-fault handling to work with the special exception handlers.

OK, will do. I need to rewrork the early IDT code a bit so it generates a real pt_regs layout, but that's arguably a cleanup anyway.

Ok, with that's I'm pretty happy about it as well.

Quoted 20 lines · Andy Lutomirski

Quoted 18 lines · Linus Torvalds

Quoted 16 lines · Andy Lutomirski

The code in my queue is, literally:

bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup,
struct pt_regs *regs, int trapnr)

{
    WARN_ONCE(1, "unchecked MSR access error: RDMSR from 0x%x",
          (unsigned int)regs->cx);

    /* Pretend that the read succeeded and returned 0. */
    regs->ip = ex_fixup_addr(fixup);
    regs->ax = 0;
    regs->dx = 0;
    return true;
}

EXPORT_SYMBOL(ex_handler_rdmsr_unsafe);

Quoted 5 lines

Quoted 2 lines

I guess I can live with this, as long as we also extend the early-fault handling to work with the special exception handlers.

OK, will do. I need to rewrork the early IDT code a bit so it generates a real pt_regs layout, but that's arguably a cleanup anyway.

Ok, with that's I'm pretty happy about it as well.

Quoted 4 lines · Andy Lutomirski

A couple of the wrmsr users actually care about performance. These are the ones involved in context switching and, to a lesser extent, in switching in and out of guest mode.

.. ok, see the crossed emails.

I'd *much* rather special case the special cases. Not make the generic case something complex.

And *particularly* not make the generic case be something where people think it's sane to oops and kill the machine. Christ.

A couple of the wrmsr users actually care about performance. These are the ones involved in context switching and, to a lesser extent, in switching in and out of guest mode.

A couple of the wrmsr users actually care about performance. These are the ones involved in context switching and, to a lesser extent, in switching in and out of guest mode.

Quoted 4 lines · Andy Lutomirski

So yes, let's please warn. I'm okay with removing the panic_on_oops thing though. (But if anyone suggests that we should stop OOPSing on bad kernel page faults, I *will* fight back.)

Bad kernel page faults are something completely different. They would be actual bugs regardless.

The MSR thing has *often* been just silly "this CPU doesn't do that MSR". Generic bootup setup code etc that just didn't know or care about the particular badly documented rule for one particular random CPU version and stepping.

In fact, when I say "often", I suspect I should really just say "always". I don't think we've ever found a case where oopsing would have been the right thing. But it has definitely caused lots of problems, especially in the early paths where your code doesn't even work right now.

Now, when it comes to the warning, I guess I could live with it, but I think it's stupid to make this a low-level exception handler thing.

So what I think should be done:

- make sure that wr/rdmsr_safe() actually works during very early
init. At some point it didn't.

- get rid of the current wrmsr/rdmsr *entirely*. It's shit.

- Add this wrapper:

#define wrmsr(msr, low, high) \
WARN_ON_ONCE(wrmsr_safe(msr, low, high))

and be done with it. We could even decide to make that WARN_ON_ONCE() be something we could configure out, because it's really a debugging thing and isn't like it should be all that fatal.

None of this insane complicated crap that buys us exactly *nothing*, and depends on fancy new exception handling support etc etc.

So what's the downside to just doing this simple thing?

None of this insane complicated crap that buys us exactly *nothing*, and depends on fancy new exception handling support etc etc.

None of this insane complicated crap that buys us exactly *nothing*, and depends on fancy new exception handling support etc etc.

Quoted 4 lines · Andy Lutomirski

So yes, let's please warn. I'm okay with removing the panic_on_oops thing though. (But if anyone suggests that we should stop OOPSing on bad kernel page faults, I *will* fight back.)

Bad kernel page faults are something completely different. They would be actual bugs regardless.

The MSR thing has *often* been just silly "this CPU doesn't do that MSR". Generic bootup setup code etc that just didn't know or care about the particular badly documented rule for one particular random CPU version and stepping.

In fact, when I say "often", I suspect I should really just say "always". I don't think we've ever found a case where oopsing would have been the right thing. But it has definitely caused lots of problems, especially in the early paths where your code doesn't even work right now.

Now, when it comes to the warning, I guess I could live with it, but I think it's stupid to make this a low-level exception handler thing.

So what I think should be done:

- make sure that wr/rdmsr_safe() actually works during very early
init. At some point it didn't.

- get rid of the current wrmsr/rdmsr *entirely*. It's shit.

- Add this wrapper:

#define wrmsr(msr, low, high) \
WARN_ON_ONCE(wrmsr_safe(msr, low, high))

and be done with it. We could even decide to make that WARN_ON_ONCE() be something we could configure out, because it's really a debugging thing and isn't like it should be all that fatal.

None of this insane complicated crap that buys us exactly *nothing*, and depends on fancy new exception handling support etc etc.

So what's the downside to just doing this simple thing?

Quoted 4 lines · Andy Lutomirski

So yes, let's please warn. I'm okay with removing the panic_on_oops thing though. (But if anyone suggests that we should stop OOPSing on bad kernel page faults, I *will* fight back.)

Bad kernel page faults are something completely different. They would be actual bugs regardless.

The MSR thing has *often* been just silly "this CPU doesn't do that MSR". Generic bootup setup code etc that just didn't know or care about the particular badly documented rule for one particular random CPU version and stepping.

In fact, when I say "often", I suspect I should really just say "always". I don't think we've ever found a case where oopsing would have been the right thing. But it has definitely caused lots of problems, especially in the early paths where your code doesn't even work right now.

Now, when it comes to the warning, I guess I could live with it, but I think it's stupid to make this a low-level exception handler thing.

So what I think should be done:

- make sure that wr/rdmsr_safe() actually works during very early
init. At some point it didn't.

- get rid of the current wrmsr/rdmsr *entirely*. It's shit.

- Add this wrapper:

#define wrmsr(msr, low, high) \
WARN_ON_ONCE(wrmsr_safe(msr, low, high))

and be done with it. We could even decide to make that WARN_ON_ONCE() be something we could configure out, because it's really a debugging thing and isn't like it should be all that fatal.

None of this insane complicated crap that buys us exactly *nothing*, and depends on fancy new exception handling support etc etc.

So what's the downside to just doing this simple thing?

So yes, let's please warn. I'm okay with removing the panic_on_oops thing though. (But if anyone suggests that we should stop OOPSing on bad kernel page faults, I *will* fight back.)

Quoted 4 lines · Andy Lutomirski

We could probably remove that check and let custom fixups run early. I don't see any compelling reason to keep them disabled. That should probably be a separate change, though.

Or we could just use the existing wrmsr_safe() code and not add this new special code at all.

Look, why are you doing this? We should get rid of the difference between wrmsr and wrmsr_safe(), not make it bigger.

Just make everything safe. There has never in the history of anything been an advantage to making things oops and to making things more complicated.

Why is rd/wrmsr() so magically important?

Quoted 32 lines · Andy Lutomirski

This demotes an OOPS and likely panic due to a failed non-"safe" MSR access to a WARN_ONCE and, for RDMSR, a return value of zero. If panic_on_oops is set, then failed unsafe MSR accesses will still oops and panic.

To be clear, this type of failure should *not* happen. This patch exists to minimize the chance of nasty undebuggable failures due on systems that used to work due to a now-fixed CONFIG_PARAVIRT=y bug.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/msr.h | 10 ++++++++--
 arch/x86/mm/extable.c      | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

arch/x86/include/asm/msr.h +4 -1

index 93fb7c1cffda..1487054a1a70 100644
@@ -92,7 +92,10 @@ static inline unsigned long long native_read_msr(unsigned int msr)
 {
      DECLARE_ARGS(val, low, high);
 
-     asm volatile("rdmsr" : EAX_EDX_RET(val, low, high) : "c" (msr));
+     asm volatile("1: rdmsr\n"
+                  "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);

arch/x86/include/asm/msr.h static inline unsigned long long native_read_msr_safe(unsigned int msr, · line 127

Quoted 7 lines

arch/x86/include/asm/msr.h +3 -1

@@ -119,7 +122,10 @@ static inline unsigned long long native_read_msr_safe(unsigned int msr,
 static inline void native_write_msr(unsigned int msr,
                                  unsigned low, unsigned high)
 {
-     asm volatile("wrmsr" : : "c" (msr), "a"(low), "d" (high) : "memory");
+     asm volatile("1: wrmsr\n"
+                  "2:\n"
+                  _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe)

This might be a good idea:

[    0.220066] cpuidle: using governor menu
[    0.224000] ------------[ cut here ]------------
[    0.224000] WARNING: CPU: 0 PID: 1 at arch/x86/mm/extable.c:74 ex_handler_wrmsr_unsafe+0x73/0x80()
[    0.224000] unchecked MSR access error: WRMSR to 0xdeadbeef (tried to write 0x000000000000caca)
[    0.224000] Modules linked in:
[    0.224000] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.5.0-rc7+ #7
[    0.224000] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
[    0.224000]  0000000000000000 ffff88007c0d7c08 ffffffff812f13a3 ffff88007c0d7c50
[    0.224000]  ffffffff81a40ffe ffff88007c0d7c40 ffffffff8105c3b1 ffffffff81717710
[    0.224000]  ffff88007c0d7d18 0000000000000000 ffffffff816207d0 0000000000000000
[    0.224000] Call Trace:
[    0.224000]  [<ffffffff812f13a3>] dump_stack+0x67/0x94
[    0.224000]  [<ffffffff8105c3b1>] warn_slowpath_common+0x91/0xd0
[    0.224000]  [<ffffffff816207d0>] ? amd_cpu_notify+0x40/0x40
[    0.224000]  [<ffffffff8105c43c>] warn_slowpath_fmt+0x4c/0x50
[    0.224000]  [<ffffffff816207d0>] ? amd_cpu_notify+0x40/0x40
[    0.224000]  [<ffffffff8131de53>] ? __this_cpu_preempt_check+0x13/0x20
[    0.224000]  [<ffffffff8104efe3>] ex_handler_wrmsr_unsafe+0x73/0x80

and it looks helpful and all but when you do it pretty early - for example I added a

         wrmsrl(0xdeadbeef, 0xcafe);

at the end of pat_bsp_init() and the machine explodes with an early panic. I'm wondering what is better - early panic or an early #GP from a missing MSR.

And more specifically, can we do better to handle the early case gracefully too?

Setting CONFIG_PARAVIRT=y has an unintended side effect: it silently turns all rdmsr and wrmsr operations into the safe variants without any checks that the operations actually succeed.

With CONFIG_PARAVIRT=n, unchecked MSR failures OOPS and probably cause boot to fail if they happen before init starts.

Neither behavior is very good, and it's particularly unfortunate that the behavior changes depending on CONFIG_PARAVIRT.

In particular, KVM guests might be unwittingly depending on the PARAVIRT=y behavior because CONFIG_KVM_GUEST currently depends on CONFIG_PARAVIRT, and, because accesses in that case are completely unchecked, we wouldn't even see a warning.

This series changes the native behavior, regardless of CONFIG_PARAVIRT. A non-"safe" MSR failure will give an informative warning once and will be fixed up -- native_read_msr will return zero, and both reads and writes will continue where they left off.

If panic_on_oops is set, they will still OOPS and panic.

By using the shiny new custom exception handler infrastructure, there should be no overhead on the success paths.

I didn't change the behavior on Xen, but, with this series applied, it would be straightforward for the Xen maintainers to make the corresponding change -- knowledge of whether the access is "safe" is now propagated into the pvops.

Doing this is probably a prerequisite to sanely decoupling CONFIG_KVM_GUEST and CONFIG_PARAVIRT, which would probably make Arjan and the rest of the Clear Containers people happy :)

There's also room to reduce the code size of the "safe" variants using custom exception handlers in the future.

Changes from v3:
- WARN_ONCE instead of WARN (Ingo)
- In the warning text, s/unsafe/unchecked/ (Ingo, sort of)

Changes from earlier versions: lots of changes!

Andy Lutomirski (5):
x86/paravirt: Add _safe to the read_msr and write_msr PV hooks x86/msr: Carry on after a non-"safe" MSR access fails without
!panic_on_oops
x86/paravirt: Add paravirt_{read,write}_msr x86/paravirt: Make "unsafe" MSR accesses unsafe even if PARAVIRT=y x86/msr: Set the return value to zero when native_rdmsr_safe fails

  arch/x86/include/asm/msr.h            | 20 ++++++++++++----
  arch/x86/include/asm/paravirt.h       | 45 +++++++++++++++++++++--------------
  arch/x86/include/asm/paravirt_types.h | 14 +++++++----
  arch/x86/kernel/paravirt.c            |  6 +++--
  arch/x86/mm/extable.c                 | 33 +++++++++++++++++++++++++
  arch/x86/xen/enlighten.c              | 27 +++++++++++++++++++--
  6 files changed, 114 insertions(+), 31 deletions(-)

Setting CONFIG_PARAVIRT=y has an unintended side effect: it silently turns all rdmsr and wrmsr operations into the safe variants without any checks that the operations actually succeed.

With CONFIG_PARAVIRT=n, unchecked MSR failures OOPS and probably cause boot to fail if they happen before init starts.

Neither behavior is very good, and it's particularly unfortunate that the behavior changes depending on CONFIG_PARAVIRT.

In particular, KVM guests might be unwittingly depending on the PARAVIRT=y behavior because CONFIG_KVM_GUEST currently depends on CONFIG_PARAVIRT, and, because accesses in that case are completely unchecked, we wouldn't even see a warning.

This series changes the native behavior, regardless of CONFIG_PARAVIRT. A non-"safe" MSR failure will give an informative warning once and will be fixed up -- native_read_msr will return zero, and both reads and writes will continue where they left off.

If panic_on_oops is set, they will still OOPS and panic.

By using the shiny new custom exception handler infrastructure, there should be no overhead on the success paths.

I didn't change the behavior on Xen, but, with this series applied, it would be straightforward for the Xen maintainers to make the corresponding change -- knowledge of whether the access is "safe" is now propagated into the pvops.

Doing this is probably a prerequisite to sanely decoupling CONFIG_KVM_GUEST and CONFIG_PARAVIRT, which would probably make Arjan and the rest of the Clear Containers people happy :)

There's also room to reduce the code size of the "safe" variants using custom exception handlers in the future.

Changes from v3:
- WARN_ONCE instead of WARN (Ingo)
- In the warning text, s/unsafe/unchecked/ (Ingo, sort of)

Changes from earlier versions: lots of changes!

Andy Lutomirski (5):
x86/paravirt: Add _safe to the read_msr and write_msr PV hooks x86/msr: Carry on after a non-"safe" MSR access fails without
!panic_on_oops
x86/paravirt: Add paravirt_{read,write}_msr x86/paravirt: Make "unsafe" MSR accesses unsafe even if PARAVIRT=y x86/msr: Set the return value to zero when native_rdmsr_safe fails

  arch/x86/include/asm/msr.h            | 20 ++++++++++++----
  arch/x86/include/asm/paravirt.h       | 45 +++++++++++++++++++++--------------
  arch/x86/include/asm/paravirt_types.h | 14 +++++++----
  arch/x86/kernel/paravirt.c            |  6 +++--
  arch/x86/mm/extable.c                 | 33 +++++++++++++++++++++++++
  arch/x86/xen/enlighten.c              | 27 +++++++++++++++++++--
  6 files changed, 114 insertions(+), 31 deletions(-)