quoted
+#include "../../entry/calling.h"
Rather than do the low level PUSH_REGS and POP_REGS, I vote to have core code
expose a FRED-specific wrapper for invoking external_interrupt(). More below.
Nice idea!
quoted
+ /*
+ * A FRED stack frame has extra 16 bytes of information pushed at the
+ * regular stack top compared to an IDT stack frame.
There is pretty much no chance that anyone remembers the layout of an IDT stack
frame off the top of their head. I.e. saying "FRED has 16 bytes more" isn't all
that useful. It also fails to capture the fact that FRED stuff a hell of a lot
more information in those "common" 48 bytes.
It'll be hard/impossible to capture all of the overload info in a comment, but
showing the actual layout of the frame would be super helpful, e.g. something
like
this
/*
* FRED stack frames are always 64 bytes:
*
* ------------------------------
* | Bytes | Usage |
* -----------------------------|
* | 63:56 | Reserved |
* | 55:48 | Event Data |
* | 47:40 | SS + Event Info |
* | 39:32 | RSP |
* | 31:24 | RFLAGS |
* | 23:16 | CS + Aux Info |
* | 15:8 | RIP |
* | 7:0 | Error Code |
* ------------------------------
*/
*
* Use LEA to get the return RIP and push it, then push an error code.
* Note, only NMI handling does an ERETS to the target! IRQ handling
* doesn't need to unmask NMIs and so simply uses CALL+RET, i.e. the
* RIP pushed here is only truly consumed for NMIs!
I take these as ASM code does need more love, i.e., nice comments :/
Otherwise only more confusion.
Jumping way back to providing a wrapper for FRED, if we do that, then there's no
need to include calling.h, and the weird wrinkle about the ERET target kinda goes
away too. E.g. provide this in arch/x86/entry/entry_64_fred.S
.section .text, "ax"
/* Note, this is instrumentation safe, and returns via RET, not ERETS! */
#if IS_ENABLED(CONFIG_KVM_INTEL)
SYM_CODE_START(fred_irq_entry_from_kvm)
FRED_ENTER
call external_interrupt
FRED_EXIT
RET
SYM_CODE_END(fred_irq_entry_from_kvm)
EXPORT_SYMBOL_GPL(fred_irq_entry_from_kvm);
#endif
and then the KVM side for this particular chunk is more simply:
lea 1f(%rip), %rax
push %rax
push $0 /* FRED error code, 0 for NMI and external
interrupts */
\branch_insn \branch_target
1:
VMX_DO_EVENT_FRAME_END
RET
Alternatively, the whole thing could be shoved into
arch/x86/entry/entry_64_fred.S,
but at a glance I don't think that would be a net positive due to the need to
handle
IRQs vs. NMIs.
quoted
+ \branch_insn \branch_target
+
+ .if \nmi == 0
+ POP_REGS
+ .endif
+
+1:
+ /*
+ * "Restore" RSP from RBP, even though IRET has already unwound RSP to
As mentioned above, this is incorrect on two fronts.
quoted
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 0ecf4be2c6af..4e90c69a92bf 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6890,6 +6890,14 @@ static void vmx_apicv_post_state_restore(struct
kvm_vcpu *vcpu)
quoted
memset(vmx->pi_desc.pir, 0, sizeof(vmx->pi_desc.pir));
}
+#ifdef CONFIG_X86_FRED
+void vmx_do_fred_interrupt_irqoff(unsigned int vector);
+void vmx_do_fred_nmi_irqoff(unsigned int vector);
+#else
+#define vmx_do_fred_interrupt_irqoff(x) BUG()
+#define vmx_do_fred_nmi_irqoff(x) BUG()
+#endif
My slight preference is to open code the BUG() as a ud2 in assembly, purely to
avoid more #ifdefs.
quoted
+
void vmx_do_interrupt_irqoff(unsigned long entry);
void vmx_do_nmi_irqoff(void);
@@ -6932,14 +6940,16 @@ static void handle_external_interrupt_irqoff(struct
kvm_vcpu *vcpu)
quoted
{
u32 intr_info = vmx_get_intr_info(vcpu);
unsigned int vector = intr_info & INTR_INFO_VECTOR_MASK;
- gate_desc *desc = (gate_desc *)host_idt_base + vector;
if (KVM_BUG(!is_external_intr(intr_info), vcpu->kvm,
"unexpected VM-Exit interrupt info: 0x%x", intr_info))
return;
kvm_before_interrupt(vcpu, KVM_HANDLING_IRQ);
- vmx_do_interrupt_irqoff(gate_offset(desc));
+ if (cpu_feature_enabled(X86_FEATURE_FRED))
+ vmx_do_fred_interrupt_irqoff(vector); /* Event type is 0 */
I strongly prefer to use code to document what's going on. E.g. the tail comment
just left me wondering, what event type is 0? Whereas this makes it quite clear
that KVM is signaling a hardware interrupt. The fact that it's a nop as far as
code generation goes is irrelevant.
vmx_do_fred_interrupt_irqoff((EVENT_TYPE_HWINT << 16) | vector);
Better readability.
quoted
+ else
+ vmx_do_interrupt_irqoff(gate_offset((gate_desc *)host_idt_base
+ vector));
quoted
kvm_after_interrupt(vcpu);
vcpu->arch.at_instruction_boundary = true;
Here's a diff for (hopefully) everything I've suggested above.
Thanks a lot! I will test it and update the patch in this mail thread.
quoted hunk ↗ jump to hunk
---
arch/x86/entry/entry_64_fred.S | 17 ++++++-
arch/x86/kernel/traps.c | 5 --
arch/x86/kvm/vmx/vmenter.S | 84 +++++++++++++++-------------------
arch/x86/kvm/vmx/vmx.c | 7 +--
4 files changed, 55 insertions(+), 58 deletions(-)
diff --git a/arch/x86/entry/entry_64_fred.S b/arch/x86/entry/entry_64_fred.S
index 12063267d2ac..a973c0bd29f6 100644
--- a/arch/x86/entry/entry_64_fred.S
+++ b/arch/x86/entry/entry_64_fred.S
@@ -10,7 +10,6 @@
#include "calling.h"
.code64
- .section ".noinstr.text", "ax"
.macro FRED_ENTER
UNWIND_HINT_END_OF_STACK
@@ -24,6 +23,22 @@
POP_REGS
.endm
+ .section .text, "ax"
+
+/* Note, this is instrumentation safe, and returns via RET, not ERETS! */
+#if IS_ENABLED(CONFIG_KVM_INTEL)
+SYM_CODE_START(fred_irq_entry_from_kvm)
+ FRED_ENTER
+ call external_interrupt
+ FRED_EXIT
+ RET
+SYM_CODE_END(fred_irq_entry_from_kvm)
+EXPORT_SYMBOL_GPL(fred_irq_entry_from_kvm);
+#endif
+
+ .section ".noinstr.text", "ax"
+
+
/*
* The new RIP value that FRED event delivery establishes is
* IA32_FRED_CONFIG & ~FFFH for events that occur in ring 3.
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 21eeba7b188f..cbcb83c71dab 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -1566,11 +1566,6 @@ int external_interrupt(struct pt_regs *regs)
return 0;
}
-#if IS_ENABLED(CONFIG_KVM_INTEL)
-/* For KVM VMX to handle IRQs in IRQ induced VM exits. */
-EXPORT_SYMBOL_GPL(external_interrupt);
-#endif
-
#endif /* CONFIG_X86_64 */
void __init trap_init(void)
diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
index 79a4c91d9434..e25df565c3f8 100644
--- a/arch/x86/kvm/vmx/vmenter.S
+++ b/arch/x86/kvm/vmx/vmenter.S
@@ -9,7 +9,6 @@
#include <asm/segment.h>
#include "kvm-asm-offsets.h"
#include "run_flags.h"
-#include "../../entry/calling.h"
#define WORD_SIZE (BITS_PER_LONG / 8)
@@ -33,15 +32,31 @@
#define VCPU_R15 __VCPU_REGS_R15 * WORD_SIZE
#endif
+.macro VMX_DO_EVENT_FRAME_BEGIN
+ /*
+ * Unconditionally create a stack frame, getting the correct RSP on the
+ * stack (for x86-64) would take two instructions anyways, and RBP can
+ * be used to restore RSP to make objtool happy (see below).
+ */
+ push %_ASM_BP
+ mov %_ASM_SP, %_ASM_BP
+.endm
+
+.macro VMX_DO_EVENT_FRAME_END
+ /*
+ * "Restore" RSP from RBP, even though {E,I}RET has already unwound RSP
+ * to the correct value *in most cases*. KVM's IRQ handling with FRED
+ * doesn't do ERETS, and objtool doesn't know the callee will IRET/ERET
+ * and, without the explicit restore, thinks the stack is getting walloped.
+ * Using an unwind hint is problematic due to x86-64's dynamic alignment.
+ */
+ mov %_ASM_BP, %_ASM_SP
+ pop %_ASM_BP
+.endm
+
#ifdef CONFIG_X86_FRED
.macro VMX_DO_FRED_EVENT_IRQOFF branch_insn branch_target nmi=0
- /*
- * Unconditionally create a stack frame, getting the correct RSP on the
- * stack (for x86-64) would take two instructions anyways, and RBP can
- * be used to restore RSP to make objtool happy (see below).
- */
- push %_ASM_BP
- mov %_ASM_SP, %_ASM_BP
+ VMX_DO_EVENT_FRAME_BEGIN
/*
* Don't check the FRED stack level, the call stack leading to this@@ -78,43 +93,23 @@
* push an error code before invoking the IRQ/NMI handler.
*
* Use LEA to get the return RIP and push it, then push an error code.
+ * Note, only NMI handling does an ERETS to the target! IRQ handling
+ * doesn't need to unmask NMIs and so simply uses CALL+RET, i.e. the
+ * RIP pushed here is only truly consumed for NMIs!
*/
lea 1f(%rip), %rax
push %rax
push $0 /* FRED error code, 0 for NMI and external
interrupts */
- .if \nmi == 0
- PUSH_REGS
- mov %rsp, %rdi
- .endif
-
\branch_insn \branch_target
-
- .if \nmi == 0
- POP_REGS
- .endif
-
1:
- /*
- * "Restore" RSP from RBP, even though IRET has already unwound RSP to
- * the correct value. objtool doesn't know the callee will IRET and,
- * without the explicit restore, thinks the stack is getting walloped.
- * Using an unwind hint is problematic due to x86-64's dynamic alignment.
- */
- mov %_ASM_BP, %_ASM_SP
- pop %_ASM_BP
+ VMX_DO_EVENT_FRAME_END
RET
.endm
#endif
.macro VMX_DO_EVENT_IRQOFF call_insn call_target
- /*
- * Unconditionally create a stack frame, getting the correct RSP on the
- * stack (for x86-64) would take two instructions anyways, and RBP can
- * be used to restore RSP to make objtool happy (see below).
- */
- push %_ASM_BP
- mov %_ASM_SP, %_ASM_BP
+ VMX_DO_EVENT_FRAME_BEGIN
#ifdef CONFIG_X86_64
/*
@@ -129,14 +124,7 @@
push $__KERNEL_CS
\call_insn \call_target
- /*
- * "Restore" RSP from RBP, even though IRET has already unwound RSP to
- * the correct value. objtool doesn't know the callee will IRET and,
- * without the explicit restore, thinks the stack is getting walloped.
- * Using an unwind hint is problematic due to x86-64's dynamic alignment.
- */
- mov %_ASM_BP, %_ASM_SP
- pop %_ASM_BP
+ VMX_DO_EVENT_FRAME_END
RET
.endm
@@ -375,11 +363,13 @@ SYM_INNER_LABEL_ALIGN(vmx_vmexit,
SYM_L_GLOBAL)
SYM_FUNC_END(__vmx_vcpu_run)
-#ifdef CONFIG_X86_FRED
SYM_FUNC_START(vmx_do_fred_nmi_irqoff)
+#ifdef CONFIG_X86_FRED
VMX_DO_FRED_EVENT_IRQOFF jmp fred_entrypoint_kernel nmi=1
+#else
+ ud2
+#endif
SYM_FUNC_END(vmx_do_fred_nmi_irqoff)
-#endif
SYM_FUNC_START(vmx_do_nmi_irqoff)
VMX_DO_EVENT_IRQOFF call asm_exc_nmi_kvm_vmx
@@ -438,11 +428,13 @@ SYM_FUNC_END(vmread_error_trampoline)
#endif
.section .text, "ax"
-#ifdef CONFIG_X86_FRED
SYM_FUNC_START(vmx_do_fred_interrupt_irqoff)
- VMX_DO_FRED_EVENT_IRQOFF call external_interrupt
+#ifdef CONFIG_X86_FRED
+ VMX_DO_FRED_EVENT_IRQOFF call fred_irq_entry_from_kvm
+#else
+ ud2
+#endif
SYM_FUNC_END(vmx_do_fred_interrupt_irqoff)
-#endif
SYM_FUNC_START(vmx_do_interrupt_irqoff)
VMX_DO_EVENT_IRQOFF CALL_NOSPEC _ASM_ARG1
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index bf757f5071e4..cb4675dd87df 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6919,13 +6919,8 @@ static void vmx_apicv_post_state_restore(struct
kvm_vcpu *vcpu)
memset(vmx->pi_desc.pir, 0, sizeof(vmx->pi_desc.pir));
}
-#ifdef CONFIG_X86_FRED
void vmx_do_fred_interrupt_irqoff(unsigned int vector);
void vmx_do_fred_nmi_irqoff(unsigned int vector);
-#else
-#define vmx_do_fred_interrupt_irqoff(x) BUG()
-#define vmx_do_fred_nmi_irqoff(x) BUG()
-#endif
void vmx_do_interrupt_irqoff(unsigned long entry);
void vmx_do_nmi_irqoff(void);
@@ -6976,7 +6971,7 @@ static void handle_external_interrupt_irqoff(struct
kvm_vcpu *vcpu)
kvm_before_interrupt(vcpu, KVM_HANDLING_IRQ);
if (cpu_feature_enabled(X86_FEATURE_FRED))
- vmx_do_fred_interrupt_irqoff(vector); /* Event type is 0 */
+ vmx_do_fred_interrupt_irqoff((EVENT_TYPE_HWINT << 16) |
vector);
else
vmx_do_interrupt_irqoff(gate_offset((gate_desc *)host_idt_base
+ vector));
kvm_after_interrupt(vcpu);
base-commit: 8961078ffe509a97ec7803b17912e57c47b93fa2
--