* [PATCH] Fix undefined operation VMXOFF during reboot and crash
@ 2020-06-10 18:12 David P. Reed
2020-06-10 19:36 ` Randy Dunlap
` (3 more replies)
0 siblings, 4 replies; 20+ messages in thread
From: David P. Reed @ 2020-06-10 18:12 UTC (permalink / raw)
To: dpreed
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Allison Randal, Enrico Weigelt,
Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
Jann Horn, Dave Hansen, linux-kernel
If a panic/reboot occurs when CR4 has VMX enabled, a VMXOFF is
done on all CPUS, to allow the INIT IPI to function, since
INIT is suppressed when CPUs are in VMX root operation.
However, VMXOFF causes an undefined operation fault if the CPU is not
in VMX operation, that is, VMXON has not been executed, or VMXOFF
has been executed, but VMX is enabled. This fix makes the reboot
work more reliably by modifying the #UD handler to skip the VMXOFF
if VMX is enabled on the CPU and the VMXOFF is executed as part
of cpu_emergency_vmxoff().
The logic in reboot.c is also corrected, since the point of forcing
the processor out of VMX root operation is because when VMX root
operation is enabled, the processor INIT signal is always masked.
See Intel SDM section on differences between VMX Root operation and normal
operation. Thus every CPU must be forced out of VMX operation.
Since the CPU will hang rather than restart, a manual "reset" is the
only way out of this state (or if there is a BMC, it can issue a RESET
to the chip).
Signed-off-by: David P. Reed <dpreed@deepplum.com>
---
arch/x86/include/asm/virtext.h | 24 ++++++++++++----
arch/x86/kernel/reboot.c | 13 ++-------
arch/x86/kernel/traps.c | 52 ++++++++++++++++++++++++++++++++--
3 files changed, 71 insertions(+), 18 deletions(-)
diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
index 9aad0e0876fb..ea2d67191684 100644
--- a/arch/x86/include/asm/virtext.h
+++ b/arch/x86/include/asm/virtext.h
@@ -13,12 +13,16 @@
#ifndef _ASM_X86_VIRTEX_H
#define _ASM_X86_VIRTEX_H
+#include <linux/percpu.h>
+
#include <asm/processor.h>
#include <asm/vmx.h>
#include <asm/svm.h>
#include <asm/tlbflush.h>
+DECLARE_PER_CPU_READ_MOSTLY(int, doing_emergency_vmxoff);
+
/*
* VMX functions:
*/
@@ -33,8 +37,8 @@ static inline int cpu_has_vmx(void)
/** Disable VMX on the current CPU
*
* vmxoff causes a undefined-opcode exception if vmxon was not run
- * on the CPU previously. Only call this function if you know VMX
- * is enabled.
+ * on the CPU previously. Only call this function directly if you know VMX
+ * is enabled *and* CPU is in VMX root operation.
*/
static inline void cpu_vmxoff(void)
{
@@ -47,17 +51,25 @@ static inline int cpu_vmx_enabled(void)
return __read_cr4() & X86_CR4_VMXE;
}
-/** Disable VMX if it is enabled on the current CPU
+/** Force disable VMX if it is enabled on the current CPU.
+ * Note that if CPU is not in VMX root operation this
+ * VMXOFF will fault an undefined operation fault.
+ * So the 'doing_emergency_vmxoff' percpu flag is set,
+ * the trap handler for just restarts execution after
+ * the VMXOFF instruction.
*
- * You shouldn't call this if cpu_has_vmx() returns 0.
+ * You shouldn't call this directly if cpu_has_vmx() returns 0.
*/
static inline void __cpu_emergency_vmxoff(void)
{
- if (cpu_vmx_enabled())
+ if (cpu_vmx_enabled()) {
+ this_cpu_write(doing_emergency_vmxoff, 1);
cpu_vmxoff();
+ this_cpu_write(doing_emergency_vmxoff, 0);
+ }
}
-/** Disable VMX if it is supported and enabled on the current CPU
+/** Force disable VMX if it is supported and enabled on the current CPU
*/
static inline void cpu_emergency_vmxoff(void)
{
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 3ca43be4f9cf..abc8b51a57c7 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -540,21 +540,14 @@ static void emergency_vmx_disable_all(void)
*
* For safety, we will avoid running the nmi_shootdown_cpus()
* stuff unnecessarily, but we don't have a way to check
- * if other CPUs have VMX enabled. So we will call it only if the
- * CPU we are running on has VMX enabled.
- *
- * We will miss cases where VMX is not enabled on all CPUs. This
- * shouldn't do much harm because KVM always enable VMX on all
- * CPUs anyway. But we can miss it on the small window where KVM
- * is still enabling VMX.
+ * if other CPUs have VMX enabled.
*/
- if (cpu_has_vmx() && cpu_vmx_enabled()) {
+ if (cpu_has_vmx()) {
/* Disable VMX on this CPU. */
- cpu_vmxoff();
+ cpu_emergency_vmxoff();
/* Halt and disable VMX on the other CPUs */
nmi_shootdown_cpus(vmxoff_nmi);
-
}
}
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 4cc541051994..2dcf57ef467e 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -39,6 +39,7 @@
#include <linux/io.h>
#include <linux/hardirq.h>
#include <linux/atomic.h>
+#include <linux/percpu.h>
#include <asm/stacktrace.h>
#include <asm/processor.h>
@@ -59,6 +60,7 @@
#include <asm/umip.h>
#include <asm/insn.h>
#include <asm/insn-eval.h>
+#include <asm/virtext.h>
#ifdef CONFIG_X86_64
#include <asm/x86_init.h>
@@ -70,6 +72,8 @@
#include <asm/proto.h>
#endif
+DEFINE_PER_CPU_READ_MOSTLY(int, doing_emergency_vmxoff) = 0;
+
DECLARE_BITMAP(system_vectors, NR_VECTORS);
static inline void cond_local_irq_enable(struct pt_regs *regs)
@@ -115,6 +119,43 @@ int fixup_bug(struct pt_regs *regs, int trapnr)
return 0;
}
+/*
+ * Fix any unwanted undefined operation fault due to VMXOFF instruction that
+ * is needed to ensure that CPU is not in VMX root operation at time of
+ * a reboot/panic CPU reset. There is no safe and reliable way to know
+ * if a processor is in VMX root operation, other than to skip the
+ * VMXOFF. It is safe to just skip any VMXOFF that might generate this
+ * exception, when VMX operation is enabled in CR4. In the extremely
+ * rare case that a VMXOFF is erroneously executed while VMX is enabled,
+ * but VMXON has not been executed yet, the undefined opcode fault
+ * should not be missed by valid code, though it would be an error.
+ * To detect this, we could somehow restrict the instruction address
+ * to the specific use during reboot/panic.
+ */
+static int fixup_emergency_vmxoff(struct pt_regs *regs, int trapnr)
+{
+ const static u8 insn_vmxoff[3] = { 0x0f, 0x01, 0xc4 };
+ u8 ud[3];
+
+ if (trapnr != X86_TRAP_UD)
+ return 0;
+ if (!cpu_vmx_enabled())
+ return 0;
+ if (!this_cpu_read(doing_emergency_vmxoff))
+ return 0;
+
+ /* undefined instruction must be in kernel and be VMXOFF */
+ if (regs->ip < TASK_SIZE_MAX)
+ return 0;
+ if (probe_kernel_address((u8 *)regs->ip, ud))
+ return 0;
+ if (memcmp(ud, insn_vmxoff, sizeof(insn_vmxoff)))
+ return 0;
+
+ regs->ip += sizeof(insn_vmxoff);
+ return 1;
+}
+
static nokprobe_inline int
do_trap_no_signal(struct task_struct *tsk, int trapnr, const char *str,
struct pt_regs *regs, long error_code)
@@ -193,9 +234,16 @@ static void do_error_trap(struct pt_regs *regs, long error_code, char *str,
/*
* WARN*()s end up here; fix them up before we call the
* notifier chain.
+ * Also, VMXOFF causes unwanted fault during reboot
+ * if VMX is enabled, but not in VMX root operation. Fix
+ * before calling notifier chain.
*/
- if (!user_mode(regs) && fixup_bug(regs, trapnr))
- return;
+ if (!user_mode(regs)) {
+ if (fixup_bug(regs, trapnr))
+ return;
+ if (fixup_emergency_vmxoff(regs, trapnr))
+ return;
+ }
if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) !=
NOTIFY_STOP) {
--
2.26.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] Fix undefined operation VMXOFF during reboot and crash
2020-06-10 18:12 [PATCH] Fix undefined operation VMXOFF during reboot and crash David P. Reed
@ 2020-06-10 19:36 ` Randy Dunlap
2020-06-10 21:34 ` Thomas Gleixner
` (2 subsequent siblings)
3 siblings, 0 replies; 20+ messages in thread
From: Randy Dunlap @ 2020-06-10 19:36 UTC (permalink / raw)
To: David P. Reed
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Allison Randal, Enrico Weigelt,
Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
Martin Molnar, Andy Lutomirski, Alexandre Chartre, Jann Horn,
Dave Hansen, linux-kernel
Hi David,
If you happen to make a v2 of this patch, there are a few comments
that begin with "/**" but they are not kernel-doc comments, so they
should instead begin with just "/*". Please see below.
(and you did not introduce this comment style here.)
On 6/10/20 11:12 AM, David P. Reed wrote:
>
> Signed-off-by: David P. Reed <dpreed@deepplum.com>
> ---
> arch/x86/include/asm/virtext.h | 24 ++++++++++++----
> arch/x86/kernel/reboot.c | 13 ++-------
> arch/x86/kernel/traps.c | 52 ++++++++++++++++++++++++++++++++--
> 3 files changed, 71 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
> index 9aad0e0876fb..ea2d67191684 100644
> --- a/arch/x86/include/asm/virtext.h
> +++ b/arch/x86/include/asm/virtext.h
> @@ -13,12 +13,16 @@
> #ifndef _ASM_X86_VIRTEX_H
> #define _ASM_X86_VIRTEX_H
>
> +#include <linux/percpu.h>
> +
> #include <asm/processor.h>
>
> #include <asm/vmx.h>
> #include <asm/svm.h>
> #include <asm/tlbflush.h>
>
> +DECLARE_PER_CPU_READ_MOSTLY(int, doing_emergency_vmxoff);
> +
> /*
> * VMX functions:
> */
> @@ -33,8 +37,8 @@ static inline int cpu_has_vmx(void)
> /** Disable VMX on the current CPU
just
/* Disable VMX on the current CPU
> *
> * vmxoff causes a undefined-opcode exception if vmxon was not run
> - * on the CPU previously. Only call this function if you know VMX
> - * is enabled.
> + * on the CPU previously. Only call this function directly if you know VMX
> + * is enabled *and* CPU is in VMX root operation.
> */
> static inline void cpu_vmxoff(void)
> {
> @@ -47,17 +51,25 @@ static inline int cpu_vmx_enabled(void)
> return __read_cr4() & X86_CR4_VMXE;
> }
>
> -/** Disable VMX if it is enabled on the current CPU
> +/** Force disable VMX if it is enabled on the current CPU.
just
/* Force disable VMX if it is enabled on the current CPU.
> + * Note that if CPU is not in VMX root operation this
> + * VMXOFF will fault an undefined operation fault.
> + * So the 'doing_emergency_vmxoff' percpu flag is set,
> + * the trap handler for just restarts execution after
> + * the VMXOFF instruction.
> *
> - * You shouldn't call this if cpu_has_vmx() returns 0.
> + * You shouldn't call this directly if cpu_has_vmx() returns 0.
> */
> static inline void __cpu_emergency_vmxoff(void)
> {
> - if (cpu_vmx_enabled())
> + if (cpu_vmx_enabled()) {
> + this_cpu_write(doing_emergency_vmxoff, 1);
> cpu_vmxoff();
> + this_cpu_write(doing_emergency_vmxoff, 0);
> + }
> }
>
> -/** Disable VMX if it is supported and enabled on the current CPU
> +/** Force disable VMX if it is supported and enabled on the current CPU
ditto.
> */
> static inline void cpu_emergency_vmxoff(void)
> {
thanks.
--
~Randy
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] Fix undefined operation VMXOFF during reboot and crash
2020-06-10 18:12 [PATCH] Fix undefined operation VMXOFF during reboot and crash David P. Reed
2020-06-10 19:36 ` Randy Dunlap
@ 2020-06-10 21:34 ` Thomas Gleixner
2020-06-10 21:42 ` Sean Christopherson
2020-06-10 21:36 ` Sean Christopherson
2020-06-10 21:59 ` Andy Lutomirski
3 siblings, 1 reply; 20+ messages in thread
From: Thomas Gleixner @ 2020-06-10 21:34 UTC (permalink / raw)
To: David P. Reed, dpreed
Cc: Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin,
Allison Randal, Enrico Weigelt, Greg Kroah-Hartman, Kate Stewart,
Peter Zijlstra (Intel),
Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
Jann Horn, Dave Hansen, linux-kernel
"David P. Reed" <dpreed@deepplum.com> writes:
> +/*
> + * Fix any unwanted undefined operation fault due to VMXOFF instruction that
> + * is needed to ensure that CPU is not in VMX root operation at time of
> + * a reboot/panic CPU reset. There is no safe and reliable way to know
> + * if a processor is in VMX root operation, other than to skip the
> + * VMXOFF. It is safe to just skip any VMXOFF that might generate this
> + * exception, when VMX operation is enabled in CR4. In the extremely
> + * rare case that a VMXOFF is erroneously executed while VMX is enabled,
> + * but VMXON has not been executed yet, the undefined opcode fault
> + * should not be missed by valid code, though it would be an error.
> + * To detect this, we could somehow restrict the instruction address
> + * to the specific use during reboot/panic.
> + */
> +static int fixup_emergency_vmxoff(struct pt_regs *regs, int trapnr)
> +{
> + const static u8 insn_vmxoff[3] = { 0x0f, 0x01, 0xc4 };
> + u8 ud[3];
> +
> + if (trapnr != X86_TRAP_UD)
> + return 0;
> + if (!cpu_vmx_enabled())
> + return 0;
> + if (!this_cpu_read(doing_emergency_vmxoff))
> + return 0;
> +
> + /* undefined instruction must be in kernel and be VMXOFF */
> + if (regs->ip < TASK_SIZE_MAX)
> + return 0;
> + if (probe_kernel_address((u8 *)regs->ip, ud))
> + return 0;
> + if (memcmp(ud, insn_vmxoff, sizeof(insn_vmxoff)))
> + return 0;
> +
> + regs->ip += sizeof(insn_vmxoff);
> + return 1;
We have exception fixups to avoid exactly that kind of horrible
workarounds all over the place.
static inline int cpu_vmxoff_safe(void)
{
int err;
asm volatile("2: vmxoff; xor %[err],%[err]\n"
"1:\n\t"
".section .fixup,\"ax\"\n\t"
"3: mov %[fault],%[err] ; jmp 1b\n\t"
".previous\n\t"
_ASM_EXTABLE(2b, 3b)
: [err] "=a" (err)
: [fault] "i" (-EFAULT)
: "memory");
return err;
}
static inline void __cpu_emergency_vmxoff(void)
{
if (!cpu_vmx_enabled())
return;
if (!cpu_vmxoff_safe())
cr4_clear_bits(X86_CR4_VMXE);
}
Problem solved.
Thanks,
tglx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] Fix undefined operation VMXOFF during reboot and crash
2020-06-10 21:34 ` Thomas Gleixner
@ 2020-06-10 21:42 ` Sean Christopherson
2020-06-10 22:08 ` Thomas Gleixner
0 siblings, 1 reply; 20+ messages in thread
From: Sean Christopherson @ 2020-06-10 21:42 UTC (permalink / raw)
To: Thomas Gleixner
Cc: David P. Reed, Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin,
Allison Randal, Enrico Weigelt, Greg Kroah-Hartman, Kate Stewart,
Peter Zijlstra (Intel),
Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
Jann Horn, Dave Hansen, linux-kernel
Gah, I typed too slow :-)
On Wed, Jun 10, 2020 at 11:34:21PM +0200, Thomas Gleixner wrote:
> We have exception fixups to avoid exactly that kind of horrible
> workarounds all over the place.
>
> static inline int cpu_vmxoff_safe(void)
> {
> int err;
>
> asm volatile("2: vmxoff; xor %[err],%[err]\n"
> "1:\n\t"
> ".section .fixup,\"ax\"\n\t"
> "3: mov %[fault],%[err] ; jmp 1b\n\t"
> ".previous\n\t"
> _ASM_EXTABLE(2b, 3b)
> : [err] "=a" (err)
> : [fault] "i" (-EFAULT)
> : "memory");
> return err;
> }
>
> static inline void __cpu_emergency_vmxoff(void)
> {
> if (!cpu_vmx_enabled())
> return;
> if (!cpu_vmxoff_safe())
> cr4_clear_bits(X86_CR4_VMXE);
This bit is wrong, CR4.VMXE should be cleared even if VMXOFF faults, e.g.
if this is called in NMI context and the NMI arrived in KVM code between
VMXOFF and clearing CR4.VMXE.
All other VMXOFF faults are mode related, i.e. any fault is guaranteed to
be due to the !post-VMXON check unless we're magically in RM, VM86, compat
mode, or at CPL>0.
> }
>
> Problem solved.
>
> Thanks,
>
> tglx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] Fix undefined operation VMXOFF during reboot and crash
2020-06-10 21:42 ` Sean Christopherson
@ 2020-06-10 22:08 ` Thomas Gleixner
0 siblings, 0 replies; 20+ messages in thread
From: Thomas Gleixner @ 2020-06-10 22:08 UTC (permalink / raw)
To: Sean Christopherson
Cc: David P. Reed, Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin,
Allison Randal, Enrico Weigelt, Greg Kroah-Hartman, Kate Stewart,
Peter Zijlstra (Intel),
Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
Jann Horn, Dave Hansen, linux-kernel
Sean Christopherson <sean.j.christopherson@intel.com> writes:
> Gah, I typed too slow :-)
Haha. I had the same thought.
> On Wed, Jun 10, 2020 at 11:34:21PM +0200, Thomas Gleixner wrote:
>> We have exception fixups to avoid exactly that kind of horrible
>> workarounds all over the place.
>>
>> static inline int cpu_vmxoff_safe(void)
>> {
>> int err;
>>
>> asm volatile("2: vmxoff; xor %[err],%[err]\n"
>> "1:\n\t"
>> ".section .fixup,\"ax\"\n\t"
>> "3: mov %[fault],%[err] ; jmp 1b\n\t"
>> ".previous\n\t"
>> _ASM_EXTABLE(2b, 3b)
>> : [err] "=a" (err)
>> : [fault] "i" (-EFAULT)
>> : "memory");
>> return err;
>> }
>>
>> static inline void __cpu_emergency_vmxoff(void)
>> {
>> if (!cpu_vmx_enabled())
>> return;
>> if (!cpu_vmxoff_safe())
>> cr4_clear_bits(X86_CR4_VMXE);
>
> This bit is wrong, CR4.VMXE should be cleared even if VMXOFF faults, e.g.
> if this is called in NMI context and the NMI arrived in KVM code between
> VMXOFF and clearing CR4.VMXE.
Oh, right.
> All other VMXOFF faults are mode related, i.e. any fault is guaranteed to
> be due to the !post-VMXON check unless we're magically in RM, VM86, compat
> mode, or at CPL>0.
Your patch is simpler indeed.
Thanks,
tglx
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] Fix undefined operation VMXOFF during reboot and crash
2020-06-10 18:12 [PATCH] Fix undefined operation VMXOFF during reboot and crash David P. Reed
2020-06-10 19:36 ` Randy Dunlap
2020-06-10 21:34 ` Thomas Gleixner
@ 2020-06-10 21:36 ` Sean Christopherson
2020-06-10 21:59 ` Andy Lutomirski
3 siblings, 0 replies; 20+ messages in thread
From: Sean Christopherson @ 2020-06-10 21:36 UTC (permalink / raw)
To: David P. Reed
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Allison Randal, Enrico Weigelt,
Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
Jann Horn, Dave Hansen, linux-kernel
On Wed, Jun 10, 2020 at 02:12:50PM -0400, David P. Reed wrote:
> If a panic/reboot occurs when CR4 has VMX enabled, a VMXOFF is
> done on all CPUS, to allow the INIT IPI to function, since
> INIT is suppressed when CPUs are in VMX root operation.
> However, VMXOFF causes an undefined operation fault if the CPU is not
> in VMX operation, that is, VMXON has not been executed, or VMXOFF
> has been executed, but VMX is enabled. This fix makes the reboot
> work more reliably by modifying the #UD handler to skip the VMXOFF
> if VMX is enabled on the CPU and the VMXOFF is executed as part
> of cpu_emergency_vmxoff().
> The logic in reboot.c is also corrected, since the point of forcing
> the processor out of VMX root operation is because when VMX root
> operation is enabled, the processor INIT signal is always masked.
> See Intel SDM section on differences between VMX Root operation and normal
> operation. Thus every CPU must be forced out of VMX operation.
> Since the CPU will hang rather than restart, a manual "reset" is the
> only way out of this state (or if there is a BMC, it can issue a RESET
> to the chip).
>
> Signed-off-by: David P. Reed <dpreed@deepplum.com>
> ---
> @@ -47,17 +51,25 @@ static inline int cpu_vmx_enabled(void)
> return __read_cr4() & X86_CR4_VMXE;
> }
>
> -/** Disable VMX if it is enabled on the current CPU
> +/** Force disable VMX if it is enabled on the current CPU.
> + * Note that if CPU is not in VMX root operation this
> + * VMXOFF will fault an undefined operation fault.
> + * So the 'doing_emergency_vmxoff' percpu flag is set,
> + * the trap handler for just restarts execution after
> + * the VMXOFF instruction.
> *
> - * You shouldn't call this if cpu_has_vmx() returns 0.
> + * You shouldn't call this directly if cpu_has_vmx() returns 0.
> */
> static inline void __cpu_emergency_vmxoff(void)
> {
> - if (cpu_vmx_enabled())
> + if (cpu_vmx_enabled()) {
> + this_cpu_write(doing_emergency_vmxoff, 1);
> cpu_vmxoff();
> + this_cpu_write(doing_emergency_vmxoff, 0);
> + }
> }
...
> +/*
> + * Fix any unwanted undefined operation fault due to VMXOFF instruction that
> + * is needed to ensure that CPU is not in VMX root operation at time of
> + * a reboot/panic CPU reset. There is no safe and reliable way to know
> + * if a processor is in VMX root operation, other than to skip the
> + * VMXOFF. It is safe to just skip any VMXOFF that might generate this
> + * exception, when VMX operation is enabled in CR4. In the extremely
> + * rare case that a VMXOFF is erroneously executed while VMX is enabled,
> + * but VMXON has not been executed yet, the undefined opcode fault
> + * should not be missed by valid code, though it would be an error.
> + * To detect this, we could somehow restrict the instruction address
> + * to the specific use during reboot/panic.
> + */
> +static int fixup_emergency_vmxoff(struct pt_regs *regs, int trapnr)
> +{
> + const static u8 insn_vmxoff[3] = { 0x0f, 0x01, 0xc4 };
> + u8 ud[3];
> +
> + if (trapnr != X86_TRAP_UD)
> + return 0;
> + if (!cpu_vmx_enabled())
> + return 0;
> + if (!this_cpu_read(doing_emergency_vmxoff))
> + return 0;
> +
> + /* undefined instruction must be in kernel and be VMXOFF */
> + if (regs->ip < TASK_SIZE_MAX)
> + return 0;
> + if (probe_kernel_address((u8 *)regs->ip, ud))
> + return 0;
> + if (memcmp(ud, insn_vmxoff, sizeof(insn_vmxoff)))
> + return 0;
> +
> + regs->ip += sizeof(insn_vmxoff);
> + return 1;
> +}
> +
> static nokprobe_inline int
> do_trap_no_signal(struct task_struct *tsk, int trapnr, const char *str,
> struct pt_regs *regs, long error_code)
> @@ -193,9 +234,16 @@ static void do_error_trap(struct pt_regs *regs, long error_code, char *str,
> /*
> * WARN*()s end up here; fix them up before we call the
> * notifier chain.
> + * Also, VMXOFF causes unwanted fault during reboot
> + * if VMX is enabled, but not in VMX root operation. Fix
> + * before calling notifier chain.
> */
> - if (!user_mode(regs) && fixup_bug(regs, trapnr))
> - return;
> + if (!user_mode(regs)) {
> + if (fixup_bug(regs, trapnr))
> + return;
> + if (fixup_emergency_vmxoff(regs, trapnr))
> + return;
> + }
Isn't this just a really kludgy way of doing fixup on vmxoff? E.g. wouldn't
the below patch do the trick?
diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
index 9aad0e0876fb..54bc84d7028d 100644
--- a/arch/x86/include/asm/virtext.h
+++ b/arch/x86/include/asm/virtext.h
@@ -32,13 +32,15 @@ static inline int cpu_has_vmx(void)
/** Disable VMX on the current CPU
*
- * vmxoff causes a undefined-opcode exception if vmxon was not run
- * on the CPU previously. Only call this function if you know VMX
- * is enabled.
+ * VMXOFF causes a #UD if the CPU is not post-VMXON, eat any #UDs to handle
+ * races with a hypervisor doing VMXOFF, e.g. if an NMI arrived between VMXOFF
+ * and clearing CR4.VMXE.
*/
static inline void cpu_vmxoff(void)
{
- asm volatile ("vmxoff");
+ asm volatile("1: vmxoff\n\t"
+ "2:\n\t"
+ _ASM_EXTABLE(1b, 2b));
cr4_clear_bits(X86_CR4_VMXE);
}
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] Fix undefined operation VMXOFF during reboot and crash
2020-06-10 18:12 [PATCH] Fix undefined operation VMXOFF during reboot and crash David P. Reed
` (2 preceding siblings ...)
2020-06-10 21:36 ` Sean Christopherson
@ 2020-06-10 21:59 ` Andy Lutomirski
2020-06-11 0:00 ` Sean Christopherson
3 siblings, 1 reply; 20+ messages in thread
From: Andy Lutomirski @ 2020-06-10 21:59 UTC (permalink / raw)
To: David P. Reed
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Allison Randal, Enrico Weigelt,
Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
Jann Horn, Dave Hansen, linux-kernel
> On Jun 10, 2020, at 11:21 AM, David P. Reed <dpreed@deepplum.com> wrote:
>
> If a panic/reboot occurs when CR4 has VMX enabled, a VMXOFF is
> done on all CPUS, to allow the INIT IPI to function, since
> INIT is suppressed when CPUs are in VMX root operation.
> However, VMXOFF causes an undefined operation fault if the CPU is not
> in VMX operation, that is, VMXON has not been executed, or VMXOFF
> has been executed, but VMX is enabled.
I’m surprised. Wouldn’t this mean that emergency reboots always fail it a VM is running? I would think someone would have noticed before.
> This fix makes the reboot
> work more reliably by modifying the #UD handler to skip the VMXOFF
> if VMX is enabled on the CPU and the VMXOFF is executed as part
> of cpu_emergency_vmxoff().
NAK. See below.
> The logic in reboot.c is also corrected, since the point of forcing
> the processor out of VMX root operation is because when VMX root
> operation is enabled, the processor INIT signal is always masked.
> See Intel SDM section on differences between VMX Root operation and normal
> operation. Thus every CPU must be forced out of VMX operation.
> Since the CPU will hang rather than restart, a manual "reset" is the
> only way out of this state (or if there is a BMC, it can issue a RESET
> to the chip).
>
> Signed-off-by: David P. Reed <dpreed@deepplum.com>
> ---
> arch/x86/include/asm/virtext.h | 24 ++++++++++++----
> arch/x86/kernel/reboot.c | 13 ++-------
> arch/x86/kernel/traps.c | 52 ++++++++++++++++++++++++++++++++--
> 3 files changed, 71 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
> index 9aad0e0876fb..ea2d67191684 100644
> --- a/arch/x86/include/asm/virtext.h
> +++ b/arch/x86/include/asm/virtext.h
> @@ -13,12 +13,16 @@
> #ifndef _ASM_X86_VIRTEX_H
> #define _ASM_X86_VIRTEX_H
>
> +#include <linux/percpu.h>
> +
> #include <asm/processor.h>
>
> #include <asm/vmx.h>
> #include <asm/svm.h>
> #include <asm/tlbflush.h>
>
> +DECLARE_PER_CPU_READ_MOSTLY(int, doing_emergency_vmxoff);
> +
> /*
> * VMX functions:
> */
> @@ -33,8 +37,8 @@ static inline int cpu_has_vmx(void)
> /** Disable VMX on the current CPU
> *
> * vmxoff causes a undefined-opcode exception if vmxon was not run
> - * on the CPU previously. Only call this function if you know VMX
> - * is enabled.
> + * on the CPU previously. Only call this function directly if you know VMX
> + * is enabled *and* CPU is in VMX root operation.
> */
So presumably the bug is someone calling this inappropriatelet?
> static inline void cpu_vmxoff(void)
> {
> @@ -47,17 +51,25 @@ static inline int cpu_vmx_enabled(void)
> return __read_() & X86_CR4_VMXE;
> }
>
> -/** Disable VMX if it is enabled on the current CPU
> +/** Force disable VMX if it is enabled on the current CPU.
> + * Note that if CPU is not in VMX root operation this
> + * VMXOFF will fault an undefined operation fault.
> + * So the 'doing_emergency_vmxoff' percpu flag is set,
> + * the trap handler for just restarts execution after
> + * the VMXOFF instruction.
> *
> - * You shouldn't call this if cpu_has_vmx() returns 0.
> + * You shouldn't call this directly if cpu_has_vmx() returns 0.
> */
> static inline void __cpu_emergency_vmxoff(void)
> {
> - if (cpu_vmx_enabled())
> + if (cpu_vmx_enabled()) {
> + this_cpu_write(doing_emergency_vmxoff, 1);
> cpu_vmxoff();
> + this_cpu_write(doing_emergency_vmxoff, 0);
> + }
> }
NAK. Just write this in asm with an exception handler that does the right thing.
Please also try to identify the actual bug. Because I have a sneaking suspicion that you are running an out of tree module that has issues. If so, the patch should explain this.
>
> -/** Disable VMX if it is supported and enabled on the current CPU
> +/** Force disable VMX if it is supported and enabled on the current CPU
> */
> static inline void cpu_emergency_vmxoff(void)
> {
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index 3ca43be4f9cf..abc8b51a57c7 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -540,21 +540,14 @@ static void emergency_vmx_disable_all(void)
> *
> * For safety, we will avoid running the nmi_shootdown_cpus()
> * stuff unnecessarily, but we don't have a way to check
> - * if other CPUs have VMX enabled. So we will call it only if the
> - * CPU we are running on has VMX enabled.
> - *
> - * We will miss cases where VMX is not enabled on all CPUs. This
> - * shouldn't do much harm because KVM always enable VMX on all
> - * CPUs anyway. But we can miss it on the small window where KVM
> - * is still enabling VMX.
> + * if other CPUs have VMX enabled.
> */
> - if (cpu_has_vmx() && cpu_vmx_enabled()) {
> + if (cpu_has_vmx()) {
> /* Disable VMX on this CPU. */
> - cpu_vmxoff();
> + cpu_emergency_vmxoff();
>
> /* Halt and disable VMX on the other CPUs */
> nmi_shootdown_cpus(vmxoff_nmi);
> -
> }
> }
>
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 4cc541051994..2dcf57ef467e 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -39,6 +39,7 @@
> #include <linux/io.h>
> #include <linux/hardirq.h>
> #include <linux/atomic.h>
> +#include <linux/percpu.h>
>
> #include <asm/stacktrace.h>
> #include <asm/processor.h>
> @@ -59,6 +60,7 @@
> #include <asm/umip.h>
> #include <asm/insn.h>
> #include <asm/insn-eval.h>
> +#include <asm/virtext.h>
>
> #ifdef CONFIG_X86_64
> #include <asm/x86_init.h>
> @@ -70,6 +72,8 @@
> #include <asm/proto.h>
> #endif
>
> +DEFINE_PER_CPU_READ_MOSTLY(int, doing_emergency_vmxoff) = 0;
> +
> DECLARE_BITMAP(system_vectors, NR_VECTORS);
>
> static inline void cond_local_irq_enable(struct pt_regs *regs)
> @@ -115,6 +119,43 @@ int fixup_bug(struct pt_regs *regs, int trapnr)
> return 0;
> }
>
> +/*
> + * Fix any unwanted undefined operation fault due to VMXOFF instruction that
> + * is needed to ensure that CPU is not in VMX root operation at time of
> + * a reboot/panic CPU reset. There is no safe and reliable way to know
> + * if a processor is in VMX root operation, other than to skip the
> + * VMXOFF. It is safe to just skip any VMXOFF that might generate this
> + * exception, when VMX operation is enabled in CR4. In the extremely
> + * rare case that a VMXOFF is erroneously executed while VMX is enabled,
> + * but VMXON has not been executed yet, the undefined opcode fault
> + * should not be missed by valid code, though it would be an error.
> + * To detect this, we could somehow restrict the instruction address
> + * to the specific use during reboot/panic.
> + */
> +static int fixup_emergency_vmxoff(struct pt_regs *regs, int trapnr)
> +{
NAK.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] Fix undefined operation VMXOFF during reboot and crash
2020-06-10 21:59 ` Andy Lutomirski
@ 2020-06-11 0:00 ` Sean Christopherson
2020-06-11 0:15 ` Andy Lutomirski
0 siblings, 1 reply; 20+ messages in thread
From: Sean Christopherson @ 2020-06-11 0:00 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David P. Reed, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
x86, H. Peter Anvin, Allison Randal, Enrico Weigelt,
Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
Jann Horn, Dave Hansen, linux-kernel
On Wed, Jun 10, 2020 at 02:59:19PM -0700, Andy Lutomirski wrote:
>
>
> > On Jun 10, 2020, at 11:21 AM, David P. Reed <dpreed@deepplum.com> wrote:
> >
> > If a panic/reboot occurs when CR4 has VMX enabled, a VMXOFF is
> > done on all CPUS, to allow the INIT IPI to function, since
> > INIT is suppressed when CPUs are in VMX root operation.
> > However, VMXOFF causes an undefined operation fault if the CPU is not
> > in VMX operation, that is, VMXON has not been executed, or VMXOFF
> > has been executed, but VMX is enabled.
>
> I’m surprised. Wouldn’t this mean that emergency reboots always fail it a VM
> is running? I would think someone would have noticed before.
The call to cpu_vmxoff() is conditioned on CR4.VMXE==1, which KVM toggles in
tandem with VMXON and VMXOFF. Out of tree hypervisors presumably do the
same. That's obviously not atomic though, e.g. VMXOFF will #UD if the
vmxoff_nmi() NMI arrives between CR4.VMXE=1 and VMXON, or between VMXOFF
and CR4.VMXE=0.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] Fix undefined operation VMXOFF during reboot and crash
2020-06-11 0:00 ` Sean Christopherson
@ 2020-06-11 0:15 ` Andy Lutomirski
[not found] ` <1591893200.58634165@apps.rackspace.com>
0 siblings, 1 reply; 20+ messages in thread
From: Andy Lutomirski @ 2020-06-11 0:15 UTC (permalink / raw)
To: Sean Christopherson
Cc: David P. Reed, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
X86 ML, H. Peter Anvin, Allison Randal, Enrico Weigelt,
Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
Jann Horn, Dave Hansen, LKML
On Wed, Jun 10, 2020 at 5:00 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Wed, Jun 10, 2020 at 02:59:19PM -0700, Andy Lutomirski wrote:
> >
> >
> > > On Jun 10, 2020, at 11:21 AM, David P. Reed <dpreed@deepplum.com> wrote:
> > >
> > > If a panic/reboot occurs when CR4 has VMX enabled, a VMXOFF is
> > > done on all CPUS, to allow the INIT IPI to function, since
> > > INIT is suppressed when CPUs are in VMX root operation.
> > > However, VMXOFF causes an undefined operation fault if the CPU is not
> > > in VMX operation, that is, VMXON has not been executed, or VMXOFF
> > > has been executed, but VMX is enabled.
> >
> > I’m surprised. Wouldn’t this mean that emergency reboots always fail it a VM
> > is running? I would think someone would have noticed before.
>
> The call to cpu_vmxoff() is conditioned on CR4.VMXE==1, which KVM toggles in
> tandem with VMXON and VMXOFF. Out of tree hypervisors presumably do the
> same. That's obviously not atomic though, e.g. VMXOFF will #UD if the
> vmxoff_nmi() NMI arrives between CR4.VMXE=1 and VMXON, or between VMXOFF
> and CR4.VMXE=0.
It would be nice for the commit message to say "this happens when
nmxoff_nmi() races with KVM's VMXON/VMXOFF toggling". Or the commit
message should say something else if the bug happens for a different
reason.
The race with KVM should be quite unusual, since it involves rebooting
concurrently with loading or unloading KVM.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] Fix undefined operation VMXOFF during reboot and crash
@ 2020-06-25 14:45 David P. Reed
2020-06-25 14:59 ` David P. Reed
0 siblings, 1 reply; 20+ messages in thread
From: David P. Reed @ 2020-06-25 14:45 UTC (permalink / raw)
To: Sean Christopherson
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, X86 ML,
H. Peter Anvin, Allison Randal, Enrico Weigelt,
Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
Jann Horn, Dave Hansen, LKML
[Sorry: this is resent because my mailer included HTML, rejected by LKML]
Thanks for the response, Sean ... I had thought everyone was too busy to follow up from the first version.
I confess I'm not sure why this should be broken up into a patch series, given that it is so very small and is all aimed at the same category of bug.
And the "emergency" path pre-existed, I didn't want to propose removing it, since I assumed it was there for a reason. I didn't want to include my own judgement as to whether there should only be one path. (I'm pretty sure I didn't find a VMXOFF in KVM separately from the instance in this include file, but I will check).
A question: if I make it a series, I have to test each patch doesn't break something individually, in order to handle the case where one patch is accepted and the others are not. Do I need to test each individual patch thoroughly as an independent patch against all those cases?
I know the combination don't break anything and fixes the issues I've discovered by testing all combinations (and I've done some thorough testing of panics, oopses crashes, kexec, ... under all combinations of CR4.VMXE enablement and crash source to verify the fix fixes the problem's manifestations and to verify that it doesn't break any of the working paths.
That said, I'm willing to do a v3 "series" based on these suggestions if it will smooth its acceptance. If it's not going to get accepted after doing that, my motivation is flagging.
On Thursday, June 25, 2020 2:06am, "Sean Christopherson" <sean.j.christopherson@intel.com> said:
> On Thu, Jun 11, 2020 at 03:48:18PM -0400, David P. Reed wrote:
> > -/** Disable VMX on the current CPU
> > +/* Disable VMX on the current CPU
> > *
> > - * vmxoff causes a undefined-opcode exception if vmxon was not run
> > - * on the CPU previously. Only call this function if you know VMX
> > - * is enabled.
> > + * vmxoff causes an undefined-opcode exception if vmxon was not run
> > + * on the CPU previously. Only call this function directly if you know VMX
> > + * is enabled *and* CPU is in VMX root operation.
> > */
> > static inline void cpu_vmxoff(void)
> > {
> > - asm volatile ("vmxoff");
> > + asm volatile ("vmxoff" ::: "cc", "memory"); /* clears all flags on success
> */
> > cr4_clear_bits(X86_CR4_VMXE);
> > }
> >
> > @@ -47,17 +47,35 @@ static inline int cpu_vmx_enabled(void)
> > return __read_cr4() & X86_CR4_VMXE;
> > }
> >
> > -/** Disable VMX if it is enabled on the current CPU
> > - *
> > - * You shouldn't call this if cpu_has_vmx() returns 0.
> > +/*
> > + * Safely disable VMX root operation if active
> > + * Note that if CPU is not in VMX root operation this
> > + * VMXOFF will fault an undefined operation fault,
> > + * so use the exception masking facility to handle that RARE
> > + * case.
> > + * You shouldn't call this directly if cpu_has_vmx() returns 0
> > + */
> > +static inline void cpu_vmxoff_safe(void)
> > +{
> > + asm volatile("1:vmxoff\n\t" /* clears all flags on success */
>
> Eh, I wouldn't bother with the comment, there are a million other caveats
> with VMXOFF that are far more interesting.
>
> > + "2:\n\t"
> > + _ASM_EXTABLE(1b, 2b)
> > + ::: "cc", "memory");
>
> Adding the memory and flags clobber should be a separate patch.
>
> > + cr4_clear_bits(X86_CR4_VMXE);
> > +}
>
>
> I don't see any value in safe/unsafe variants. The only in-kernel user of
> VMXOFF outside of the emergency flows is KVM, which has its own VMXOFF
> helper, i.e. all users of cpu_vmxoff() want the "safe" variant. Just add
> the exception fixup to cpu_vmxoff() and call it good.
>
> > +
> > +/*
> > + * Force disable VMX if it is enabled on the current CPU,
> > + * when it is unknown whether CPU is in VMX operation.
> > */
> > static inline void __cpu_emergency_vmxoff(void)
> > {
> > - if (cpu_vmx_enabled())
> > - cpu_vmxoff();
> > + if (!cpu_vmx_enabled())
> > + return;
> > + cpu_vmxoff_safe();
>
> Unnecessary churn.
>
> > }
> >
> > -/** Disable VMX if it is supported and enabled on the current CPU
> > +/* Force disable VMX if it is supported on current CPU
> > */
> > static inline void cpu_emergency_vmxoff(void)
> > {
> > diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> > index e040ba6be27b..b0e6b106a67e 100644
> > --- a/arch/x86/kernel/reboot.c
> > +++ b/arch/x86/kernel/reboot.c
> > @@ -540,21 +540,14 @@ static void emergency_vmx_disable_all(void)
> > *
> > * For safety, we will avoid running the nmi_shootdown_cpus()
> > * stuff unnecessarily, but we don't have a way to check
> > - * if other CPUs have VMX enabled. So we will call it only if the
> > - * CPU we are running on has VMX enabled.
> > - *
> > - * We will miss cases where VMX is not enabled on all CPUs. This
> > - * shouldn't do much harm because KVM always enable VMX on all
> > - * CPUs anyway. But we can miss it on the small window where KVM
> > - * is still enabling VMX.
> > + * if other CPUs have VMX enabled.
> > */
> > - if (cpu_has_vmx() && cpu_vmx_enabled()) {
> > + if (cpu_has_vmx()) {
> > /* Disable VMX on this CPU. */
> > - cpu_vmxoff();
> > + cpu_emergency_vmxoff();
>
> This also needs to be in a separate patch. And it should use
> __cpu_emergency_vmxoff() instead of cpu_emergency_vmxoff().
>
> >
> > /* Halt and disable VMX on the other CPUs */
> > nmi_shootdown_cpus(vmxoff_nmi);
> > -
> > }
> > }
> >
> > --
> > 2.26.2
> >
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] Fix undefined operation VMXOFF during reboot and crash
2020-06-25 14:45 David P. Reed
@ 2020-06-25 14:59 ` David P. Reed
2020-06-29 20:54 ` David P. Reed
0 siblings, 1 reply; 20+ messages in thread
From: David P. Reed @ 2020-06-25 14:59 UTC (permalink / raw)
To: David P. Reed
Cc: Sean Christopherson, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, X86 ML, H. Peter Anvin, Allison Randal,
Enrico Weigelt, Greg Kroah-Hartman, Kate Stewart,
Peter Zijlstra (Intel),
Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
Jann Horn, Dave Hansen, LKML
Correction to my comment below.
On Thursday, June 25, 2020 10:45am, "David P. Reed" <dpreed@deepplum.com> said:
> [Sorry: this is resent because my mailer included HTML, rejected by LKML]
> Thanks for the response, Sean ... I had thought everyone was too busy to follow up
> from the first version.
>
> I confess I'm not sure why this should be broken up into a patch series, given
> that it is so very small and is all aimed at the same category of bug.
>
> And the "emergency" path pre-existed, I didn't want to propose removing it, since
> I assumed it was there for a reason. I didn't want to include my own judgement as
> to whether there should only be one path. (I'm pretty sure I didn't find a VMXOFF
> in KVM separately from the instance in this include file, but I will check).
Just checked. Yes, the kvm code's handling of VMXOFF is separate, and though it uses exception masking, seems to do other things, perhaps related to nested KVM, but I haven't studied the deep logic of KVM nesting.
>
> A question: if I make it a series, I have to test each patch doesn't break
> something individually, in order to handle the case where one patch is accepted
> and the others are not. Do I need to test each individual patch thoroughly as an
> independent patch against all those cases?
> I know the combination don't break anything and fixes the issues I've discovered
> by testing all combinations (and I've done some thorough testing of panics, oopses
> crashes, kexec, ... under all combinations of CR4.VMXE enablement and crash source
> to verify the fix fixes the problem's manifestations and to verify that it doesn't
> break any of the working paths.
>
> That said, I'm willing to do a v3 "series" based on these suggestions if it will
> smooth its acceptance. If it's not going to get accepted after doing that, my
> motivation is flagging.
> On Thursday, June 25, 2020 2:06am, "Sean Christopherson"
> <sean.j.christopherson@intel.com> said:
>
>
>
>> On Thu, Jun 11, 2020 at 03:48:18PM -0400, David P. Reed wrote:
>> > -/** Disable VMX on the current CPU
>> > +/* Disable VMX on the current CPU
>> > *
>> > - * vmxoff causes a undefined-opcode exception if vmxon was not run
>> > - * on the CPU previously. Only call this function if you know VMX
>> > - * is enabled.
>> > + * vmxoff causes an undefined-opcode exception if vmxon was not run
>> > + * on the CPU previously. Only call this function directly if you know VMX
>> > + * is enabled *and* CPU is in VMX root operation.
>> > */
>> > static inline void cpu_vmxoff(void)
>> > {
>> > - asm volatile ("vmxoff");
>> > + asm volatile ("vmxoff" ::: "cc", "memory"); /* clears all flags on success
>> */
>> > cr4_clear_bits(X86_CR4_VMXE);
>> > }
>> >
>> > @@ -47,17 +47,35 @@ static inline int cpu_vmx_enabled(void)
>> > return __read_cr4() & X86_CR4_VMXE;
>> > }
>> >
>> > -/** Disable VMX if it is enabled on the current CPU
>> > - *
>> > - * You shouldn't call this if cpu_has_vmx() returns 0.
>> > +/*
>> > + * Safely disable VMX root operation if active
>> > + * Note that if CPU is not in VMX root operation this
>> > + * VMXOFF will fault an undefined operation fault,
>> > + * so use the exception masking facility to handle that RARE
>> > + * case.
>> > + * You shouldn't call this directly if cpu_has_vmx() returns 0
>> > + */
>> > +static inline void cpu_vmxoff_safe(void)
>> > +{
>> > + asm volatile("1:vmxoff\n\t" /* clears all flags on success */
>>
>> Eh, I wouldn't bother with the comment, there are a million other caveats
>> with VMXOFF that are far more interesting.
>>
>> > + "2:\n\t"
>> > + _ASM_EXTABLE(1b, 2b)
>> > + ::: "cc", "memory");
>>
>> Adding the memory and flags clobber should be a separate patch.
>>
>> > + cr4_clear_bits(X86_CR4_VMXE);
>> > +}
>>
>>
>> I don't see any value in safe/unsafe variants. The only in-kernel user of
>> VMXOFF outside of the emergency flows is KVM, which has its own VMXOFF
>> helper, i.e. all users of cpu_vmxoff() want the "safe" variant. Just add
>> the exception fixup to cpu_vmxoff() and call it good.
>>
>> > +
>> > +/*
>> > + * Force disable VMX if it is enabled on the current CPU,
>> > + * when it is unknown whether CPU is in VMX operation.
>> > */
>> > static inline void __cpu_emergency_vmxoff(void)
>> > {
>> > - if (cpu_vmx_enabled())
>> > - cpu_vmxoff();
>> > + if (!cpu_vmx_enabled())
>> > + return;
>> > + cpu_vmxoff_safe();
>>
>> Unnecessary churn.
>>
>> > }
>> >
>> > -/** Disable VMX if it is supported and enabled on the current CPU
>> > +/* Force disable VMX if it is supported on current CPU
>> > */
>> > static inline void cpu_emergency_vmxoff(void)
>> > {
>> > diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
>> > index e040ba6be27b..b0e6b106a67e 100644
>> > --- a/arch/x86/kernel/reboot.c
>> > +++ b/arch/x86/kernel/reboot.c
>> > @@ -540,21 +540,14 @@ static void emergency_vmx_disable_all(void)
>> > *
>> > * For safety, we will avoid running the nmi_shootdown_cpus()
>> > * stuff unnecessarily, but we don't have a way to check
>> > - * if other CPUs have VMX enabled. So we will call it only if the
>> > - * CPU we are running on has VMX enabled.
>> > - *
>> > - * We will miss cases where VMX is not enabled on all CPUs. This
>> > - * shouldn't do much harm because KVM always enable VMX on all
>> > - * CPUs anyway. But we can miss it on the small window where KVM
>> > - * is still enabling VMX.
>> > + * if other CPUs have VMX enabled.
>> > */
>> > - if (cpu_has_vmx() && cpu_vmx_enabled()) {
>> > + if (cpu_has_vmx()) {
>> > /* Disable VMX on this CPU. */
>> > - cpu_vmxoff();
>> > + cpu_emergency_vmxoff();
>>
>> This also needs to be in a separate patch. And it should use
>> __cpu_emergency_vmxoff() instead of cpu_emergency_vmxoff().
>>
>> >
>> > /* Halt and disable VMX on the other CPUs */
>> > nmi_shootdown_cpus(vmxoff_nmi);
>> > -
>> > }
>> > }
>> >
>> > --
>> > 2.26.2
>> >
>>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] Fix undefined operation VMXOFF during reboot and crash
2020-06-25 14:59 ` David P. Reed
@ 2020-06-29 20:54 ` David P. Reed
2020-06-29 21:22 ` Andy Lutomirski
0 siblings, 1 reply; 20+ messages in thread
From: David P. Reed @ 2020-06-29 20:54 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Sean Christopherson, Ingo Molnar, Borislav Petkov, X86 ML,
H. Peter Anvin, Allison Randal, Enrico Weigelt,
Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
Jann Horn, Dave Hansen, LKML
Simple question for those on the To: and CC: list here. Should I abandon any hope of this patch being accepted? It's been a long time.
The non-response after I acknowledged that this was discovered by when working on a personal, non-commercial research project - which is "out-of-tree" (apparently dirty words on LKML) has me thinking my contribution is unwanted. That's fine, I suppose. I can maintain this patch out-of-tree as well.
I did incorporate all the helpful suggestions I received in this second patch, and given some encouragement, will happily submit a revised v3 if there is any likelihood of acceptance. I'm wary of doing more radical changes (like combining emergency and normal paths).
On Thursday, June 25, 2020 10:59am, "David P. Reed" <dpreed@deepplum.com> said:
> Correction to my comment below.
> On Thursday, June 25, 2020 10:45am, "David P. Reed" <dpreed@deepplum.com> said:
>
>> [Sorry: this is resent because my mailer included HTML, rejected by LKML]
>> Thanks for the response, Sean ... I had thought everyone was too busy to follow
>> up
>> from the first version.
>>
>> I confess I'm not sure why this should be broken up into a patch series, given
>> that it is so very small and is all aimed at the same category of bug.
>>
>> And the "emergency" path pre-existed, I didn't want to propose removing it, since
>> I assumed it was there for a reason. I didn't want to include my own judgement as
>> to whether there should only be one path. (I'm pretty sure I didn't find a VMXOFF
>> in KVM separately from the instance in this include file, but I will check).
> Just checked. Yes, the kvm code's handling of VMXOFF is separate, and though it
> uses exception masking, seems to do other things, perhaps related to nested KVM,
> but I haven't studied the deep logic of KVM nesting.
>
>>
>> A question: if I make it a series, I have to test each patch doesn't break
>> something individually, in order to handle the case where one patch is accepted
>> and the others are not. Do I need to test each individual patch thoroughly as an
>> independent patch against all those cases?
>> I know the combination don't break anything and fixes the issues I've discovered
>> by testing all combinations (and I've done some thorough testing of panics,
>> oopses
>> crashes, kexec, ... under all combinations of CR4.VMXE enablement and crash
>> source
>> to verify the fix fixes the problem's manifestations and to verify that it
>> doesn't
>> break any of the working paths.
>>
>> That said, I'm willing to do a v3 "series" based on these suggestions if it will
>> smooth its acceptance. If it's not going to get accepted after doing that, my
>> motivation is flagging.
>> On Thursday, June 25, 2020 2:06am, "Sean Christopherson"
>> <sean.j.christopherson@intel.com> said:
>>
>>
>>
>>> On Thu, Jun 11, 2020 at 03:48:18PM -0400, David P. Reed wrote:
>>> > -/** Disable VMX on the current CPU
>>> > +/* Disable VMX on the current CPU
>>> > *
>>> > - * vmxoff causes a undefined-opcode exception if vmxon was not run
>>> > - * on the CPU previously. Only call this function if you know VMX
>>> > - * is enabled.
>>> > + * vmxoff causes an undefined-opcode exception if vmxon was not run
>>> > + * on the CPU previously. Only call this function directly if you know VMX
>>> > + * is enabled *and* CPU is in VMX root operation.
>>> > */
>>> > static inline void cpu_vmxoff(void)
>>> > {
>>> > - asm volatile ("vmxoff");
>>> > + asm volatile ("vmxoff" ::: "cc", "memory"); /* clears all flags on success
>>> */
>>> > cr4_clear_bits(X86_CR4_VMXE);
>>> > }
>>> >
>>> > @@ -47,17 +47,35 @@ static inline int cpu_vmx_enabled(void)
>>> > return __read_cr4() & X86_CR4_VMXE;
>>> > }
>>> >
>>> > -/** Disable VMX if it is enabled on the current CPU
>>> > - *
>>> > - * You shouldn't call this if cpu_has_vmx() returns 0.
>>> > +/*
>>> > + * Safely disable VMX root operation if active
>>> > + * Note that if CPU is not in VMX root operation this
>>> > + * VMXOFF will fault an undefined operation fault,
>>> > + * so use the exception masking facility to handle that RARE
>>> > + * case.
>>> > + * You shouldn't call this directly if cpu_has_vmx() returns 0
>>> > + */
>>> > +static inline void cpu_vmxoff_safe(void)
>>> > +{
>>> > + asm volatile("1:vmxoff\n\t" /* clears all flags on success */
>>>
>>> Eh, I wouldn't bother with the comment, there are a million other caveats
>>> with VMXOFF that are far more interesting.
>>>
>>> > + "2:\n\t"
>>> > + _ASM_EXTABLE(1b, 2b)
>>> > + ::: "cc", "memory");
>>>
>>> Adding the memory and flags clobber should be a separate patch.
>>>
>>> > + cr4_clear_bits(X86_CR4_VMXE);
>>> > +}
>>>
>>>
>>> I don't see any value in safe/unsafe variants. The only in-kernel user of
>>> VMXOFF outside of the emergency flows is KVM, which has its own VMXOFF
>>> helper, i.e. all users of cpu_vmxoff() want the "safe" variant. Just add
>>> the exception fixup to cpu_vmxoff() and call it good.
>>>
>>> > +
>>> > +/*
>>> > + * Force disable VMX if it is enabled on the current CPU,
>>> > + * when it is unknown whether CPU is in VMX operation.
>>> > */
>>> > static inline void __cpu_emergency_vmxoff(void)
>>> > {
>>> > - if (cpu_vmx_enabled())
>>> > - cpu_vmxoff();
>>> > + if (!cpu_vmx_enabled())
>>> > + return;
>>> > + cpu_vmxoff_safe();
>>>
>>> Unnecessary churn.
>>>
>>> > }
>>> >
>>> > -/** Disable VMX if it is supported and enabled on the current CPU
>>> > +/* Force disable VMX if it is supported on current CPU
>>> > */
>>> > static inline void cpu_emergency_vmxoff(void)
>>> > {
>>> > diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
>>> > index e040ba6be27b..b0e6b106a67e 100644
>>> > --- a/arch/x86/kernel/reboot.c
>>> > +++ b/arch/x86/kernel/reboot.c
>>> > @@ -540,21 +540,14 @@ static void emergency_vmx_disable_all(void)
>>> > *
>>> > * For safety, we will avoid running the nmi_shootdown_cpus()
>>> > * stuff unnecessarily, but we don't have a way to check
>>> > - * if other CPUs have VMX enabled. So we will call it only if the
>>> > - * CPU we are running on has VMX enabled.
>>> > - *
>>> > - * We will miss cases where VMX is not enabled on all CPUs. This
>>> > - * shouldn't do much harm because KVM always enable VMX on all
>>> > - * CPUs anyway. But we can miss it on the small window where KVM
>>> > - * is still enabling VMX.
>>> > + * if other CPUs have VMX enabled.
>>> > */
>>> > - if (cpu_has_vmx() && cpu_vmx_enabled()) {
>>> > + if (cpu_has_vmx()) {
>>> > /* Disable VMX on this CPU. */
>>> > - cpu_vmxoff();
>>> > + cpu_emergency_vmxoff();
>>>
>>> This also needs to be in a separate patch. And it should use
>>> __cpu_emergency_vmxoff() instead of cpu_emergency_vmxoff().
>>>
>>> >
>>> > /* Halt and disable VMX on the other CPUs */
>>> > nmi_shootdown_cpus(vmxoff_nmi);
>>> > -
>>> > }
>>> > }
>>> >
>>> > --
>>> > 2.26.2
>>> >
>>>
>>
>>
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] Fix undefined operation VMXOFF during reboot and crash
2020-06-29 20:54 ` David P. Reed
@ 2020-06-29 21:22 ` Andy Lutomirski
2020-06-29 21:49 ` Sean Christopherson
0 siblings, 1 reply; 20+ messages in thread
From: Andy Lutomirski @ 2020-06-29 21:22 UTC (permalink / raw)
To: David P. Reed
Cc: Thomas Gleixner, Sean Christopherson, Ingo Molnar,
Borislav Petkov, X86 ML, H. Peter Anvin, Allison Randal,
Enrico Weigelt, Greg Kroah-Hartman, Kate Stewart,
Peter Zijlstra (Intel),
Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
Jann Horn, Dave Hansen, LKML
> On Jun 29, 2020, at 1:54 PM, David P. Reed <dpreed@deepplum.com> wrote:
>
> Simple question for those on the To: and CC: list here. Should I abandon any hope of this patch being accepted? It's been a long time.
>
> The non-response after I acknowledged that this was discovered by when working on a personal, non-commercial research project - which is "out-of-tree" (apparently dirty words on LKML) has me thinking my contribution is unwanted. That's fine, I suppose. I can maintain this patch out-of-tree as well.
> I did incorporate all the helpful suggestions I received in this second patch, and given some encouragement, will happily submit a revised v3 if there is any likelihood of acceptance. I'm wary of doing more radical changes (like combining emergency and normal paths).
>
Sorry about being slow and less actively encouraging than we should be. We absolutely welcome personal contributions. The actual problem is that everyone is worked and we’re all slow. Also, you may be hitting a corner case in the process: is this a KVM patch or an x86 patch?
>
> On Thursday, June 25, 2020 10:59am, "David P. Reed" <dpreed@deepplum.com> said:
>
>> Correction to my comment below.
>> On Thursday, June 25, 2020 10:45am, "David P. Reed" <dpreed@deepplum.com> said:
>>
>>> [Sorry: this is resent because my mailer included HTML, rejected by LKML]
>>> Thanks for the response, Sean ... I had thought everyone was too busy to follow
>>> up
>>> from the first version.
>>>
>>> I confess I'm not sure why this should be broken up into a patch series, given
>>> that it is so very small and is all aimed at the same category of bug.
>>>
>>> And the "emergency" path pre-existed, I didn't want to propose removing it, since
>>> I assumed it was there for a reason. I didn't want to include my own judgement as
>>> to whether there should only be one path. (I'm pretty sure I didn't find a VMXOFF
>>> in KVM separately from the instance in this include file, but I will check).
>> Just checked. Yes, the kvm code's handling of VMXOFF is separate, and though it
>> uses exception masking, seems to do other things, perhaps related to nested KVM,
>> but I haven't studied the deep logic of KVM nesting.
>>
>>>
>>> A question: if I make it a series, I have to test each patch doesn't break
>>> something individually, in order to handle the case where one patch is accepted
>>> and the others are not. Do I need to test each individual patch thoroughly as an
>>> independent patch against all those cases?
>>> I know the combination don't break anything and fixes the issues I've discovered
>>> by testing all combinations (and I've done some thorough testing of panics,
>>> oopses
>>> crashes, kexec, ... under all combinations of CR4.VMXE enablement and crash
>>> source
>>> to verify the fix fixes the problem's manifestations and to verify that it
>>> doesn't
>>> break any of the working paths.
>>>
>>> That said, I'm willing to do a v3 "series" based on these suggestions if it will
>>> smooth its acceptance. If it's not going to get accepted after doing that, my
>>> motivation is flagging.
>>> On Thursday, June 25, 2020 2:06am, "Sean Christopherson"
>>> <sean.j.christopherson@intel.com> said:
>>>
>>>
>>>
>>>>> On Thu, Jun 11, 2020 at 03:48:18PM -0400, David P. Reed wrote:
>>>>>> -/** Disable VMX on the current CPU
>>>>>> +/* Disable VMX on the current CPU
>>>>>> *
>>>>>> - * vmxoff causes a undefined-opcode exception if vmxon was not run
>>>>>> - * on the CPU previously. Only call this function if you know VMX
>>>>>> - * is enabled.
>>>>>> + * vmxoff causes an undefined-opcode exception if vmxon was not run
>>>>>> + * on the CPU previously. Only call this function directly if you know VMX
>>>>>> + * is enabled *and* CPU is in VMX root operation.
>>>>>> */
>>>>>> static inline void cpu_vmxoff(void)
>>>>>> {
>>>>>> - asm volatile ("vmxoff");
>>>>>> + asm volatile ("vmxoff" ::: "cc", "memory"); /* clears all flags on success
>>>>> */
>>>>>> cr4_clear_bits(X86_CR4_VMXE);
>>>>>> }
>>>>>>
>>>>>> @@ -47,17 +47,35 @@ static inline int cpu_vmx_enabled(void)
>>>>>> return __read_cr4() & X86_CR4_VMXE;
>>>>>> }
>>>>>>
>>>>>> -/** Disable VMX if it is enabled on the current CPU
>>>>>> - *
>>>>>> - * You shouldn't call this if cpu_has_vmx() returns 0.
>>>>>> +/*
>>>>>> + * Safely disable VMX root operation if active
>>>>>> + * Note that if CPU is not in VMX root operation this
>>>>>> + * VMXOFF will fault an undefined operation fault,
>>>>>> + * so use the exception masking facility to handle that RARE
>>>>>> + * case.
>>>>>> + * You shouldn't call this directly if cpu_has_vmx() returns 0
>>>>>> + */
>>>>>> +static inline void cpu_vmxoff_safe(void)
>>>>>> +{
>>>>>> + asm volatile("1:vmxoff\n\t" /* clears all flags on success */
>>>>>
>>>>> Eh, I wouldn't bother with the comment, there are a million other caveats
>>>>> with VMXOFF that are far more interesting.
>>>>>
>>>>>> + "2:\n\t"
>>>>>> + _ASM_EXTABLE(1b, 2b)
>>>>>> + ::: "cc", "memory");
>>>>>
>>>>> Adding the memory and flags clobber should be a separate patch.
>>>>>
>>>>>> + cr4_clear_bits(X86_CR4_VMXE);
>>>>>> +}
>>>>>
>>>>>
>>>>> I don't see any value in safe/unsafe variants. The only in-kernel user of
>>>>> VMXOFF outside of the emergency flows is KVM, which has its own VMXOFF
>>>>> helper, i.e. all users of cpu_vmxoff() want the "safe" variant. Just add
>>>>> the exception fixup to cpu_vmxoff() and call it good.
>>>>>
>>>>>> +
>>>>>> +/*
>>>>>> + * Force disable VMX if it is enabled on the current CPU,
>>>>>> + * when it is unknown whether CPU is in VMX operation.
>>>>>> */
>>>>>> static inline void __cpu_emergency_vmxoff(void)
>>>>>> {
>>>>>> - if (cpu_vmx_enabled())
>>>>>> - cpu_vmxoff();
>>>>>> + if (!cpu_vmx_enabled())
>>>>>> + return;
>>>>>> + cpu_vmxoff_safe();
>>>>>
>>>>> Unnecessary churn.
>>>>>
>>>>>> }
>>>>>>
>>>>>> -/** Disable VMX if it is supported and enabled on the current CPU
>>>>>> +/* Force disable VMX if it is supported on current CPU
>>>>>> */
>>>>>> static inline void cpu_emergency_vmxoff(void)
>>>>>> {
>>>>>> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
>>>>>> index e040ba6be27b..b0e6b106a67e 100644
>>>>>> --- a/arch/x86/kernel/reboot.c
>>>>>> +++ b/arch/x86/kernel/reboot.c
>>>>>> @@ -540,21 +540,14 @@ static void emergency_vmx_disable_all(void)
>>>>>> *
>>>>>> * For safety, we will avoid running the nmi_shootdown_cpus()
>>>>>> * stuff unnecessarily, but we don't have a way to check
>>>>>> - * if other CPUs have VMX enabled. So we will call it only if the
>>>>>> - * CPU we are running on has VMX enabled.
>>>>>> - *
>>>>>> - * We will miss cases where VMX is not enabled on all CPUs. This
>>>>>> - * shouldn't do much harm because KVM always enable VMX on all
>>>>>> - * CPUs anyway. But we can miss it on the small window where KVM
>>>>>> - * is still enabling VMX.
>>>>>> + * if other CPUs have VMX enabled.
>>>>>> */
>>>>>> - if (cpu_has_vmx() && cpu_vmx_enabled()) {
>>>>>> + if (cpu_has_vmx()) {
>>>>>> /* Disable VMX on this CPU. */
>>>>>> - cpu_vmxoff();
>>>>>> + cpu_emergency_vmxoff();
>>>>>
>>>>> This also needs to be in a separate patch. And it should use
>>>>> __cpu_emergency_vmxoff() instead of cpu_emergency_vmxoff().
>>>>>
>>>>>>
>>>>>> /* Halt and disable VMX on the other CPUs */
>>>>>> nmi_shootdown_cpus(vmxoff_nmi);
>>>>>> -
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> --
>>>>>> 2.26.2
>>>>>>
>>>>>
>>>
>>>
>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] Fix undefined operation VMXOFF during reboot and crash
2020-06-29 21:22 ` Andy Lutomirski
@ 2020-06-29 21:49 ` Sean Christopherson
2020-06-29 22:46 ` David P. Reed
0 siblings, 1 reply; 20+ messages in thread
From: Sean Christopherson @ 2020-06-29 21:49 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David P. Reed, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
X86 ML, H. Peter Anvin, Allison Randal, Enrico Weigelt,
Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
Jann Horn, Dave Hansen, LKML
On Mon, Jun 29, 2020 at 02:22:45PM -0700, Andy Lutomirski wrote:
>
>
> > On Jun 29, 2020, at 1:54 PM, David P. Reed <dpreed@deepplum.com> wrote:
> >
> > Simple question for those on the To: and CC: list here. Should I
> > abandon any hope of this patch being accepted? It's been a long time.
> >
> > The non-response after I acknowledged that this was discovered by when
> > working on a personal, non-commercial research project - which is
> > "out-of-tree" (apparently dirty words on LKML) has me thinking my
> > contribution is unwanted. That's fine, I suppose. I can maintain this patch
> > out-of-tree as well. I did incorporate all the helpful suggestions I
> > received in this second patch, and given some encouragement, will happily
> > submit a revised v3 if there is any likelihood of acceptance. I'm wary of
> > doing more radical changes (like combining emergency and normal paths).
> >
>
> Sorry about being slow and less actively encouraging than we should be. We
> absolutely welcome personal contributions. The actual problem is that
> everyone is worked and we’re all slow. Also, you may be hitting a corner case
> in the process: is this a KVM patch or an x86 patch?
It's an x86 patch as it's not KVM specific, e.g. this code also helps play
nice with out of tree hypervisors.
The code change is mostly good, but it needs to be split up as there are
three separate fixes:
1. Handle #UD on VMXON due to a race.
2. Mark memory and flags as clobbered by VMXON.
3. Change emergency_vmx_disable_all() to not manually check cpu_vmx_enabled().
Yes, the changes are tiny, but if for example #3 introduces a bug then we
don't have to revert #1 and #2. Or perhaps older kernels are only subject
to the #1 and #2 and thus dumping all three changes into a single patch makes
it all harder to backport. In other words, all the usual "one change per
patch" reasons.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] Fix undefined operation VMXOFF during reboot and crash
2020-06-29 21:49 ` Sean Christopherson
@ 2020-06-29 22:46 ` David P. Reed
0 siblings, 0 replies; 20+ messages in thread
From: David P. Reed @ 2020-06-29 22:46 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
X86 ML, H. Peter Anvin, Allison Randal, Enrico Weigelt,
Greg Kroah-Hartman, Kate Stewart, Peter Zijlstra (Intel),
Randy Dunlap, Martin Molnar, Andy Lutomirski, Alexandre Chartre,
Jann Horn, Dave Hansen, LKML
On Monday, June 29, 2020 5:49pm, "Sean Christopherson" <sean.j.christopherson@intel.com> said:
> On Mon, Jun 29, 2020 at 02:22:45PM -0700, Andy Lutomirski wrote:
>>
>>
>> > On Jun 29, 2020, at 1:54 PM, David P. Reed <dpreed@deepplum.com> wrote:
>> >
>> > Simple question for those on the To: and CC: list here. Should I
>> > abandon any hope of this patch being accepted? It's been a long time.
>> >
>> > The non-response after I acknowledged that this was discovered by when
>> > working on a personal, non-commercial research project - which is
>> > "out-of-tree" (apparently dirty words on LKML) has me thinking my
>> > contribution is unwanted. That's fine, I suppose. I can maintain this patch
>> > out-of-tree as well. I did incorporate all the helpful suggestions I
>> > received in this second patch, and given some encouragement, will happily
>> > submit a revised v3 if there is any likelihood of acceptance. I'm wary of
>> > doing more radical changes (like combining emergency and normal paths).
>> >
>>
>> Sorry about being slow and less actively encouraging than we should be. We
>> absolutely welcome personal contributions. The actual problem is that
>> everyone is worked and we’re all slow. Also, you may be hitting a corner
>> case
>> in the process: is this a KVM patch or an x86 patch?
>
> It's an x86 patch as it's not KVM specific, e.g. this code also helps play
> nice with out of tree hypervisors.
>
> The code change is mostly good, but it needs to be split up as there are
> three separate fixes:
>
> 1. Handle #UD on VMXON due to a race.
> 2. Mark memory and flags as clobbered by VMXON.
> 3. Change emergency_vmx_disable_all() to not manually check cpu_vmx_enabled().
>
> Yes, the changes are tiny, but if for example #3 introduces a bug then we
> don't have to revert #1 and #2. Or perhaps older kernels are only subject
> to the #1 and #2 and thus dumping all three changes into a single patch makes
> it all harder to backport. In other words, all the usual "one change per
> patch" reasons.
>
Thanks. If no one else responds with additional suggestions, I will make it into 3 patches.
I'm happy to learn the nuances of the kernel patch regimen.
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-06-29 22:46 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-10 18:12 [PATCH] Fix undefined operation VMXOFF during reboot and crash David P. Reed
2020-06-10 19:36 ` Randy Dunlap
2020-06-10 21:34 ` Thomas Gleixner
2020-06-10 21:42 ` Sean Christopherson
2020-06-10 22:08 ` Thomas Gleixner
2020-06-10 21:36 ` Sean Christopherson
2020-06-10 21:59 ` Andy Lutomirski
2020-06-11 0:00 ` Sean Christopherson
2020-06-11 0:15 ` Andy Lutomirski
[not found] ` <1591893200.58634165@apps.rackspace.com>
2020-06-11 17:00 ` Sean Christopherson
2020-06-11 17:02 ` Andy Lutomirski
2020-06-11 19:45 ` [PATCH v2] " David P. Reed
2020-06-11 19:48 ` David P. Reed
2020-06-25 6:06 ` Sean Christopherson
2020-06-25 14:45 David P. Reed
2020-06-25 14:59 ` David P. Reed
2020-06-29 20:54 ` David P. Reed
2020-06-29 21:22 ` Andy Lutomirski
2020-06-29 21:49 ` Sean Christopherson
2020-06-29 22:46 ` David P. Reed
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).