* [PATCH v6 0/2] x86/sev: Fixes for SEV-ES Guest Support @ 2021-06-16 18:49 Joerg Roedel 2021-06-16 18:49 ` [PATCH v6 1/2] x86/sev: Make sure IRQs are disabled while GHCB is active Joerg Roedel 2021-06-16 18:49 ` [PATCH v6 2/2] x86/sev: Split up runtime #VC handler for correct state tracking Joerg Roedel 0 siblings, 2 replies; 9+ messages in thread From: Joerg Roedel @ 2021-06-16 18:49 UTC (permalink / raw) To: x86 Cc: Joerg Roedel, Joerg Roedel, hpa, Andy Lutomirski, Dave Hansen, Peter Zijlstra, Jiri Slaby, Dan Williams, Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes, Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes, Sean Christopherson, Martin Radev, Arvind Sankar, linux-coco, linux-kernel, kvm, virtualization From: Joerg Roedel <jroedel@suse.de> Hi, here are the remainig patches in my queue for SEV-ES guest support. Changes to the previous version are: - Rebased to tip/x86/sev - Merged Boris' diff to patch 1 and fixed two minor issues in the result The patches are again tested with a kernel with various debuggin options enabled, running as an SEV-ES guest. The previous version of these patches can be found here: https://lore.kernel.org/lkml/20210614135327.9921-1-joro@8bytes.org/ Please review. Thanks, Joerg Joerg Roedel (2): x86/sev: Make sure IRQs are disabled while GHCB is active x86/sev: Split up runtime #VC handler for correct state tracking arch/x86/entry/entry_64.S | 4 +- arch/x86/include/asm/idtentry.h | 29 ++--- arch/x86/kernel/sev.c | 180 +++++++++++++++++++------------- 3 files changed, 119 insertions(+), 94 deletions(-) base-commit: 07570cef5e5c3fcec40f82a9075abb4c1da63319 -- 2.31.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v6 1/2] x86/sev: Make sure IRQs are disabled while GHCB is active 2021-06-16 18:49 [PATCH v6 0/2] x86/sev: Fixes for SEV-ES Guest Support Joerg Roedel @ 2021-06-16 18:49 ` Joerg Roedel 2021-06-17 15:00 ` Peter Zijlstra 2021-06-17 15:38 ` Peter Zijlstra 2021-06-16 18:49 ` [PATCH v6 2/2] x86/sev: Split up runtime #VC handler for correct state tracking Joerg Roedel 1 sibling, 2 replies; 9+ messages in thread From: Joerg Roedel @ 2021-06-16 18:49 UTC (permalink / raw) To: x86 Cc: Joerg Roedel, Joerg Roedel, hpa, Andy Lutomirski, Dave Hansen, Peter Zijlstra, Jiri Slaby, Dan Williams, Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes, Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes, Sean Christopherson, Martin Radev, Arvind Sankar, linux-coco, linux-kernel, kvm, virtualization From: Joerg Roedel <jroedel@suse.de> The #VC handler only cares about IRQs being disabled while the GHCB is active, as it must not be interrupted by something which could cause another #VC while it holds the GHCB (NMI is the exception for which the backup GHCB exits). Make sure nothing interrupts the code path while the GHCB is active by disabling IRQs in sev_es_get_ghcb() and restoring the previous irq state in sev_es_put_ghcb(). Signed-off-by: Joerg Roedel <jroedel@suse.de> --- arch/x86/kernel/sev.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c index 8178db07a06a..e0d12728bcb7 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -12,7 +12,6 @@ #include <linux/sched/debug.h> /* For show_regs() */ #include <linux/percpu-defs.h> #include <linux/mem_encrypt.h> -#include <linux/lockdep.h> #include <linux/printk.h> #include <linux/mm_types.h> #include <linux/set_memory.h> @@ -192,11 +191,19 @@ void noinstr __sev_es_ist_exit(void) this_cpu_write(cpu_tss_rw.x86_tss.ist[IST_INDEX_VC], *(unsigned long *)ist); } -static __always_inline struct ghcb *sev_es_get_ghcb(struct ghcb_state *state) +/* + * Nothing shall interrupt this code path while holding the per-CPU + * GHCB. The backup GHCB is only for NMIs interrupting this path. + * + * Callers must disable local interrupts around it. + */ +static __always_inline struct ghcb *__sev_get_ghcb(struct ghcb_state *state) { struct sev_es_runtime_data *data; struct ghcb *ghcb; + WARN_ON(!irqs_disabled()); + data = this_cpu_read(runtime_data); ghcb = &data->ghcb_page; @@ -486,11 +493,13 @@ static enum es_result vc_slow_virt_to_phys(struct ghcb *ghcb, struct es_em_ctxt /* Include code shared with pre-decompression boot stage */ #include "sev-shared.c" -static __always_inline void sev_es_put_ghcb(struct ghcb_state *state) +static __always_inline void __sev_put_ghcb(struct ghcb_state *state) { struct sev_es_runtime_data *data; struct ghcb *ghcb; + WARN_ON(!irqs_disabled()); + data = this_cpu_read(runtime_data); ghcb = &data->ghcb_page; @@ -514,7 +523,7 @@ void noinstr __sev_es_nmi_complete(void) struct ghcb_state state; struct ghcb *ghcb; - ghcb = sev_es_get_ghcb(&state); + ghcb = __sev_get_ghcb(&state); vc_ghcb_invalidate(ghcb); ghcb_set_sw_exit_code(ghcb, SVM_VMGEXIT_NMI_COMPLETE); @@ -524,7 +533,7 @@ void noinstr __sev_es_nmi_complete(void) sev_es_wr_ghcb_msr(__pa_nodebug(ghcb)); VMGEXIT(); - sev_es_put_ghcb(&state); + __sev_put_ghcb(&state); } static u64 get_jump_table_addr(void) @@ -536,7 +545,7 @@ static u64 get_jump_table_addr(void) local_irq_save(flags); - ghcb = sev_es_get_ghcb(&state); + ghcb = __sev_get_ghcb(&state); vc_ghcb_invalidate(ghcb); ghcb_set_sw_exit_code(ghcb, SVM_VMGEXIT_AP_JUMP_TABLE); @@ -550,7 +559,7 @@ static u64 get_jump_table_addr(void) ghcb_sw_exit_info_2_is_valid(ghcb)) ret = ghcb->save.sw_exit_info_2; - sev_es_put_ghcb(&state); + __sev_put_ghcb(&state); local_irq_restore(flags); @@ -673,9 +682,12 @@ static bool __init sev_es_setup_ghcb(void) static void sev_es_ap_hlt_loop(void) { struct ghcb_state state; + unsigned long flags; struct ghcb *ghcb; - ghcb = sev_es_get_ghcb(&state); + local_irq_save(flags); + + ghcb = __sev_get_ghcb(&state); while (true) { vc_ghcb_invalidate(ghcb); @@ -692,7 +704,9 @@ static void sev_es_ap_hlt_loop(void) break; } - sev_es_put_ghcb(&state); + __sev_put_ghcb(&state); + + local_irq_restore(flags); } /* @@ -1351,7 +1365,6 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) } irq_state = irqentry_nmi_enter(regs); - lockdep_assert_irqs_disabled(); instrumentation_begin(); /* @@ -1360,7 +1373,7 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) * keep the IRQs disabled to protect us against concurrent TLB flushes. */ - ghcb = sev_es_get_ghcb(&state); + ghcb = __sev_get_ghcb(&state); vc_ghcb_invalidate(ghcb); result = vc_init_em_ctxt(&ctxt, regs, error_code); @@ -1368,7 +1381,7 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) if (result == ES_OK) result = vc_handle_exitcode(&ctxt, ghcb, error_code); - sev_es_put_ghcb(&state); + __sev_put_ghcb(&state); /* Done - now check the result */ switch (result) { -- 2.31.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/2] x86/sev: Make sure IRQs are disabled while GHCB is active 2021-06-16 18:49 ` [PATCH v6 1/2] x86/sev: Make sure IRQs are disabled while GHCB is active Joerg Roedel @ 2021-06-17 15:00 ` Peter Zijlstra 2021-06-18 8:25 ` Joerg Roedel 2021-06-17 15:38 ` Peter Zijlstra 1 sibling, 1 reply; 9+ messages in thread From: Peter Zijlstra @ 2021-06-17 15:00 UTC (permalink / raw) To: Joerg Roedel Cc: x86, Joerg Roedel, hpa, Andy Lutomirski, Dave Hansen, Jiri Slaby, Dan Williams, Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes, Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes, Sean Christopherson, Martin Radev, Arvind Sankar, linux-coco, linux-kernel, kvm, virtualization On Wed, Jun 16, 2021 at 08:49:12PM +0200, Joerg Roedel wrote: > static void sev_es_ap_hlt_loop(void) > { > struct ghcb_state state; > + unsigned long flags; > struct ghcb *ghcb; > > - ghcb = sev_es_get_ghcb(&state); > + local_irq_save(flags); > + > + ghcb = __sev_get_ghcb(&state); > > while (true) { > vc_ghcb_invalidate(ghcb); > @@ -692,7 +704,9 @@ static void sev_es_ap_hlt_loop(void) > break; > } > > - sev_es_put_ghcb(&state); > + __sev_put_ghcb(&state); > + > + local_irq_restore(flags); > } I think this is broken, at this point RCU is quite dead on this CPU and local_irq_save/restore include tracing and all sorts. Also, shouldn't IRQs already be disabled by the time we get here? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/2] x86/sev: Make sure IRQs are disabled while GHCB is active 2021-06-17 15:00 ` Peter Zijlstra @ 2021-06-18 8:25 ` Joerg Roedel 0 siblings, 0 replies; 9+ messages in thread From: Joerg Roedel @ 2021-06-18 8:25 UTC (permalink / raw) To: Peter Zijlstra Cc: Joerg Roedel, x86, hpa, Andy Lutomirski, Dave Hansen, Jiri Slaby, Dan Williams, Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes, Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes, Sean Christopherson, Martin Radev, Arvind Sankar, linux-coco, linux-kernel, kvm, virtualization On Thu, Jun 17, 2021 at 05:00:48PM +0200, Peter Zijlstra wrote: > I think this is broken, at this point RCU is quite dead on this CPU and > local_irq_save/restore include tracing and all sorts. > > Also, shouldn't IRQs already be disabled by the time we get here? Yes it is, I removed these calls, made __sev_get/put_ghcb() noinstr instead of __always_inline and put instrumentation_begin()/end() around the panic() call in __sev_get_ghcb(). Regards, Joerg ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/2] x86/sev: Make sure IRQs are disabled while GHCB is active 2021-06-16 18:49 ` [PATCH v6 1/2] x86/sev: Make sure IRQs are disabled while GHCB is active Joerg Roedel 2021-06-17 15:00 ` Peter Zijlstra @ 2021-06-17 15:38 ` Peter Zijlstra 2021-06-18 8:17 ` Joerg Roedel 1 sibling, 1 reply; 9+ messages in thread From: Peter Zijlstra @ 2021-06-17 15:38 UTC (permalink / raw) To: Joerg Roedel Cc: x86, Joerg Roedel, hpa, Andy Lutomirski, Dave Hansen, Jiri Slaby, Dan Williams, Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes, Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes, Sean Christopherson, Martin Radev, Arvind Sankar, linux-coco, linux-kernel, kvm, virtualization On Wed, Jun 16, 2021 at 08:49:12PM +0200, Joerg Roedel wrote: > @@ -514,7 +523,7 @@ void noinstr __sev_es_nmi_complete(void) > struct ghcb_state state; > struct ghcb *ghcb; > > - ghcb = sev_es_get_ghcb(&state); > + ghcb = __sev_get_ghcb(&state); > > vc_ghcb_invalidate(ghcb); > ghcb_set_sw_exit_code(ghcb, SVM_VMGEXIT_NMI_COMPLETE); > @@ -524,7 +533,7 @@ void noinstr __sev_es_nmi_complete(void) > sev_es_wr_ghcb_msr(__pa_nodebug(ghcb)); > VMGEXIT(); > > - sev_es_put_ghcb(&state); > + __sev_put_ghcb(&state); > } I'm getting (with all of v6.1 applied): vmlinux.o: warning: objtool: __sev_es_nmi_complete()+0x1bf: call to panic() leaves .noinstr.text section $ ./scripts/faddr2line defconfig-build/vmlinux __sev_es_nmi_complete+0x1bf __sev_es_nmi_complete+0x1bf/0x1d0: __sev_get_ghcb at arch/x86/kernel/sev.c:223 (inlined by) __sev_es_nmi_complete at arch/x86/kernel/sev.c:519 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/2] x86/sev: Make sure IRQs are disabled while GHCB is active 2021-06-17 15:38 ` Peter Zijlstra @ 2021-06-18 8:17 ` Joerg Roedel 2021-06-18 10:53 ` Peter Zijlstra 0 siblings, 1 reply; 9+ messages in thread From: Joerg Roedel @ 2021-06-18 8:17 UTC (permalink / raw) To: Peter Zijlstra Cc: x86, Joerg Roedel, hpa, Andy Lutomirski, Dave Hansen, Jiri Slaby, Dan Williams, Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes, Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes, Sean Christopherson, Martin Radev, Arvind Sankar, linux-coco, linux-kernel, kvm, virtualization On Thu, Jun 17, 2021 at 05:38:46PM +0200, Peter Zijlstra wrote: > I'm getting (with all of v6.1 applied): > > vmlinux.o: warning: objtool: __sev_es_nmi_complete()+0x1bf: call to panic() leaves .noinstr.text section > > $ ./scripts/faddr2line defconfig-build/vmlinux __sev_es_nmi_complete+0x1bf > __sev_es_nmi_complete+0x1bf/0x1d0: > __sev_get_ghcb at arch/x86/kernel/sev.c:223 > (inlined by) __sev_es_nmi_complete at arch/x86/kernel/sev.c:519 I see where this is coming from, but can't create the same warning. I did run 'objtool check -n vmlinux'. Is there more to do to get the full check? Regards, Joerg ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/2] x86/sev: Make sure IRQs are disabled while GHCB is active 2021-06-18 8:17 ` Joerg Roedel @ 2021-06-18 10:53 ` Peter Zijlstra 0 siblings, 0 replies; 9+ messages in thread From: Peter Zijlstra @ 2021-06-18 10:53 UTC (permalink / raw) To: Joerg Roedel Cc: x86, Joerg Roedel, hpa, Andy Lutomirski, Dave Hansen, Jiri Slaby, Dan Williams, Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes, Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes, Sean Christopherson, Martin Radev, Arvind Sankar, linux-coco, linux-kernel, kvm, virtualization On Fri, Jun 18, 2021 at 10:17:54AM +0200, Joerg Roedel wrote: > On Thu, Jun 17, 2021 at 05:38:46PM +0200, Peter Zijlstra wrote: > > I'm getting (with all of v6.1 applied): > > > > vmlinux.o: warning: objtool: __sev_es_nmi_complete()+0x1bf: call to panic() leaves .noinstr.text section > > > > $ ./scripts/faddr2line defconfig-build/vmlinux __sev_es_nmi_complete+0x1bf > > __sev_es_nmi_complete+0x1bf/0x1d0: > > __sev_get_ghcb at arch/x86/kernel/sev.c:223 > > (inlined by) __sev_es_nmi_complete at arch/x86/kernel/sev.c:519 > > I see where this is coming from, but can't create the same warning. I > did run 'objtool check -n vmlinux'. Is there more to do to get the full > check? You get those when you enable CONFIG_DEBUG_ENTRY=y (make sure to have PARAVIRT=n, I so need to go fix that :/). ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v6 2/2] x86/sev: Split up runtime #VC handler for correct state tracking 2021-06-16 18:49 [PATCH v6 0/2] x86/sev: Fixes for SEV-ES Guest Support Joerg Roedel 2021-06-16 18:49 ` [PATCH v6 1/2] x86/sev: Make sure IRQs are disabled while GHCB is active Joerg Roedel @ 2021-06-16 18:49 ` Joerg Roedel 2021-06-17 13:55 ` [PATCH v6.1 " Joerg Roedel 1 sibling, 1 reply; 9+ messages in thread From: Joerg Roedel @ 2021-06-16 18:49 UTC (permalink / raw) To: x86 Cc: Joerg Roedel, Joerg Roedel, Peter Zijlstra, hpa, Andy Lutomirski, Dave Hansen, Jiri Slaby, Dan Williams, Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes, Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes, Sean Christopherson, Martin Radev, Arvind Sankar, linux-coco, linux-kernel, kvm, virtualization From: Joerg Roedel <jroedel@suse.de> Split up the #VC handler code into a from-user and a from-kernel part. This allows clean and correct state tracking, as the #VC handler needs to enter NMI-state when raised from kernel mode and plain IRQ state when raised from user-mode. Fixes: 62441a1fb532 ("x86/sev-es: Correctly track IRQ states in runtime #VC handler") Suggested-by: Peter Zijlstra <peterz@infradead.org> Signed-off-by: Joerg Roedel <jroedel@suse.de> --- arch/x86/entry/entry_64.S | 4 +- arch/x86/include/asm/idtentry.h | 29 +++---- arch/x86/kernel/sev.c | 143 ++++++++++++++++++-------------- 3 files changed, 94 insertions(+), 82 deletions(-) diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S index a16a5294d55f..1886aaf19914 100644 --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -506,7 +506,7 @@ SYM_CODE_START(\asmsym) movq %rsp, %rdi /* pt_regs pointer */ - call \cfunc + call kernel_\cfunc /* * No need to switch back to the IST stack. The current stack is either @@ -517,7 +517,7 @@ SYM_CODE_START(\asmsym) /* Switch to the regular task stack */ .Lfrom_usermode_switch_stack_\@: - idtentry_body safe_stack_\cfunc, has_error_code=1 + idtentry_body user_\cfunc, has_error_code=1 _ASM_NOKPROBE(\asmsym) SYM_CODE_END(\asmsym) diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h index 73d45b0dfff2..cd9f3e304944 100644 --- a/arch/x86/include/asm/idtentry.h +++ b/arch/x86/include/asm/idtentry.h @@ -312,8 +312,8 @@ static __always_inline void __##func(struct pt_regs *regs) */ #define DECLARE_IDTENTRY_VC(vector, func) \ DECLARE_IDTENTRY_RAW_ERRORCODE(vector, func); \ - __visible noinstr void ist_##func(struct pt_regs *regs, unsigned long error_code); \ - __visible noinstr void safe_stack_##func(struct pt_regs *regs, unsigned long error_code) + __visible noinstr void kernel_##func(struct pt_regs *regs, unsigned long error_code); \ + __visible noinstr void user_##func(struct pt_regs *regs, unsigned long error_code) /** * DEFINE_IDTENTRY_IST - Emit code for IST entry points @@ -355,33 +355,24 @@ static __always_inline void __##func(struct pt_regs *regs) DEFINE_IDTENTRY_RAW_ERRORCODE(func) /** - * DEFINE_IDTENTRY_VC_SAFE_STACK - Emit code for VMM communication handler - which runs on a safe stack. + * DEFINE_IDTENTRY_VC_KERNEL - Emit code for VMM communication handler + when raised from kernel mode * @func: Function name of the entry point * * Maps to DEFINE_IDTENTRY_RAW_ERRORCODE */ -#define DEFINE_IDTENTRY_VC_SAFE_STACK(func) \ - DEFINE_IDTENTRY_RAW_ERRORCODE(safe_stack_##func) +#define DEFINE_IDTENTRY_VC_KERNEL(func) \ + DEFINE_IDTENTRY_RAW_ERRORCODE(kernel_##func) /** - * DEFINE_IDTENTRY_VC_IST - Emit code for VMM communication handler - which runs on the VC fall-back stack + * DEFINE_IDTENTRY_VC_USER - Emit code for VMM communication handler + when raised from user mode * @func: Function name of the entry point * * Maps to DEFINE_IDTENTRY_RAW_ERRORCODE */ -#define DEFINE_IDTENTRY_VC_IST(func) \ - DEFINE_IDTENTRY_RAW_ERRORCODE(ist_##func) - -/** - * DEFINE_IDTENTRY_VC - Emit code for VMM communication handler - * @func: Function name of the entry point - * - * Maps to DEFINE_IDTENTRY_RAW_ERRORCODE - */ -#define DEFINE_IDTENTRY_VC(func) \ - DEFINE_IDTENTRY_RAW_ERRORCODE(func) +#define DEFINE_IDTENTRY_VC_USER(func) \ + DEFINE_IDTENTRY_RAW_ERRORCODE(user_##func) #else /* CONFIG_X86_64 */ diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c index e0d12728bcb7..696369a0f9bf 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -796,7 +796,7 @@ void __init sev_es_init_vc_handling(void) sev_es_setup_play_dead(); /* Secondary CPUs use the runtime #VC handler */ - initial_vc_handler = (unsigned long)safe_stack_exc_vmm_communication; + initial_vc_handler = (unsigned long)kernel_exc_vmm_communication; } static void __init vc_early_forward_exception(struct es_em_ctxt *ctxt) @@ -1337,41 +1337,13 @@ static __always_inline bool on_vc_fallback_stack(struct pt_regs *regs) return (sp >= __this_cpu_ist_bottom_va(VC2) && sp < __this_cpu_ist_top_va(VC2)); } -/* - * Main #VC exception handler. It is called when the entry code was able to - * switch off the IST to a safe kernel stack. - * - * With the current implementation it is always possible to switch to a safe - * stack because #VC exceptions only happen at known places, like intercepted - * instructions or accesses to MMIO areas/IO ports. They can also happen with - * code instrumentation when the hypervisor intercepts #DB, but the critical - * paths are forbidden to be instrumented, so #DB exceptions currently also - * only happen in safe places. - */ -DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) +static bool vc_raw_handle_exception(struct pt_regs *regs, unsigned long error_code) { - irqentry_state_t irq_state; struct ghcb_state state; struct es_em_ctxt ctxt; enum es_result result; struct ghcb *ghcb; - - /* - * Handle #DB before calling into !noinstr code to avoid recursive #DB. - */ - if (error_code == SVM_EXIT_EXCP_BASE + X86_TRAP_DB) { - vc_handle_trap_db(regs); - return; - } - - irq_state = irqentry_nmi_enter(regs); - instrumentation_begin(); - - /* - * This is invoked through an interrupt gate, so IRQs are disabled. The - * code below might walk page-tables for user or kernel addresses, so - * keep the IRQs disabled to protect us against concurrent TLB flushes. - */ + bool ret = true; ghcb = __sev_get_ghcb(&state); @@ -1391,15 +1363,18 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) case ES_UNSUPPORTED: pr_err_ratelimited("Unsupported exit-code 0x%02lx in #VC exception (IP: 0x%lx)\n", error_code, regs->ip); - goto fail; + ret = false; + break; case ES_VMM_ERROR: pr_err_ratelimited("Failure in communication with VMM (exit-code 0x%02lx IP: 0x%lx)\n", error_code, regs->ip); - goto fail; + ret = false; + break; case ES_DECODE_FAILED: pr_err_ratelimited("Failed to decode instruction (exit-code 0x%02lx IP: 0x%lx)\n", error_code, regs->ip); - goto fail; + ret = false; + break; case ES_EXCEPTION: vc_forward_exception(&ctxt); break; @@ -1415,24 +1390,55 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) BUG(); } -out: - instrumentation_end(); - irqentry_nmi_exit(regs, irq_state); + return ret; +} - return; +static bool noinstr vc_check_and_handle_db(struct pt_regs *regs, unsigned long error_code) +{ + if (likely(error_code != SVM_EXIT_EXCP_BASE + X86_TRAP_DB)) + return false; -fail: - if (user_mode(regs)) { - /* - * Do not kill the machine if user-space triggered the - * exception. Send SIGBUS instead and let user-space deal with - * it. - */ - force_sig_fault(SIGBUS, BUS_OBJERR, (void __user *)0); - } else { - pr_emerg("PANIC: Unhandled #VC exception in kernel space (result=%d)\n", - result); + vc_handle_trap_db(regs); + + return true; +} + +/* + * Runtime #VC exception handler when raised from kernel mode. Runs in NMI mode + * and will panic when an error happens. + */ +DEFINE_IDTENTRY_VC_KERNEL(exc_vmm_communication) +{ + irqentry_state_t irq_state; + /* + * With the current implementation it is always possible to switch to a + * safe stack because #VC exceptions only happen at known places, like + * intercepted instructions or accesses to MMIO areas/IO ports. They can + * also happen with code instrumentation when the hypervisor intercepts + * #DB, but the critical paths are forbidden to be instrumented, so #DB + * exceptions currently also only happen in safe places. + * + * But keep this here in case the noinstr annotations are violated due + * to bug elsewhere. + */ + if (unlikely(on_vc_fallback_stack(regs))) { + instrumentation_begin(); + panic("Can't handle #VC exception from unsupported context\n"); + instrumentation_end(); + } + + /* + * Handle #DB before calling into !noinstr code to avoid recursive #DB. + */ + if (vc_check_and_handle_db(regs, error_code)) + return; + + irq_state = irqentry_nmi_enter(regs); + + instrumentation_begin(); + + if (!vc_raw_handle_exception(regs, error_code)) { /* Show some debug info */ show_regs(regs); @@ -1443,23 +1449,38 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) panic("Returned from Terminate-Request to Hypervisor\n"); } - goto out; + instrumentation_end(); + irqentry_nmi_exit(regs, irq_state); } -/* This handler runs on the #VC fall-back stack. It can cause further #VC exceptions */ -DEFINE_IDTENTRY_VC_IST(exc_vmm_communication) +/* + * Runtime #VC exception handler when raised from user mode. Runs in IRQ mode + * and will kill the current task with SIGBUS when an error happens. + */ +DEFINE_IDTENTRY_VC_USER(exc_vmm_communication) { + irqentry_state_t irq_state; + + /* + * Handle #DB before calling into !noinstr code to avoid recursive #DB. + */ + if (vc_check_and_handle_db(regs, error_code)) + return; + + irq_state = irqentry_enter(regs); instrumentation_begin(); - panic("Can't handle #VC exception from unsupported context\n"); - instrumentation_end(); -} -DEFINE_IDTENTRY_VC(exc_vmm_communication) -{ - if (likely(!on_vc_fallback_stack(regs))) - safe_stack_exc_vmm_communication(regs, error_code); - else - ist_exc_vmm_communication(regs, error_code); + if (!vc_raw_handle_exception(regs, error_code)) { + /* + * Do not kill the machine if user-space triggered the + * exception. Send SIGBUS instead and let user-space deal with + * it. + */ + force_sig_fault(SIGBUS, BUS_OBJERR, (void __user *)0); + } + + instrumentation_end(); + irqentry_exit(regs, irq_state); } bool __init handle_vc_boot_ghcb(struct pt_regs *regs) -- 2.31.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v6.1 2/2] x86/sev: Split up runtime #VC handler for correct state tracking 2021-06-16 18:49 ` [PATCH v6 2/2] x86/sev: Split up runtime #VC handler for correct state tracking Joerg Roedel @ 2021-06-17 13:55 ` Joerg Roedel 0 siblings, 0 replies; 9+ messages in thread From: Joerg Roedel @ 2021-06-17 13:55 UTC (permalink / raw) To: x86 Cc: Joerg Roedel, Peter Zijlstra, hpa, Andy Lutomirski, Dave Hansen, Jiri Slaby, Dan Williams, Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes, Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes, Sean Christopherson, Martin Radev, Arvind Sankar, linux-coco, linux-kernel, kvm, virtualization From: Joerg Roedel <jroedel@suse.de> Split up the #VC handler code into a from-user and a from-kernel part. This allows clean and correct state tracking, as the #VC handler needs to enter NMI-state when raised from kernel mode and plain IRQ state when raised from user-mode. Fixes: 62441a1fb532 ("x86/sev-es: Correctly track IRQ states in runtime #VC handler") Suggested-by: Peter Zijlstra <peterz@infradead.org> Signed-off-by: Joerg Roedel <jroedel@suse.de> --- arch/x86/entry/entry_64.S | 4 +- arch/x86/include/asm/idtentry.h | 29 +++---- arch/x86/kernel/sev.c | 148 +++++++++++++++++--------------- 3 files changed, 91 insertions(+), 90 deletions(-) diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S index a16a5294d55f..1886aaf19914 100644 --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -506,7 +506,7 @@ SYM_CODE_START(\asmsym) movq %rsp, %rdi /* pt_regs pointer */ - call \cfunc + call kernel_\cfunc /* * No need to switch back to the IST stack. The current stack is either @@ -517,7 +517,7 @@ SYM_CODE_START(\asmsym) /* Switch to the regular task stack */ .Lfrom_usermode_switch_stack_\@: - idtentry_body safe_stack_\cfunc, has_error_code=1 + idtentry_body user_\cfunc, has_error_code=1 _ASM_NOKPROBE(\asmsym) SYM_CODE_END(\asmsym) diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h index 73d45b0dfff2..cd9f3e304944 100644 --- a/arch/x86/include/asm/idtentry.h +++ b/arch/x86/include/asm/idtentry.h @@ -312,8 +312,8 @@ static __always_inline void __##func(struct pt_regs *regs) */ #define DECLARE_IDTENTRY_VC(vector, func) \ DECLARE_IDTENTRY_RAW_ERRORCODE(vector, func); \ - __visible noinstr void ist_##func(struct pt_regs *regs, unsigned long error_code); \ - __visible noinstr void safe_stack_##func(struct pt_regs *regs, unsigned long error_code) + __visible noinstr void kernel_##func(struct pt_regs *regs, unsigned long error_code); \ + __visible noinstr void user_##func(struct pt_regs *regs, unsigned long error_code) /** * DEFINE_IDTENTRY_IST - Emit code for IST entry points @@ -355,33 +355,24 @@ static __always_inline void __##func(struct pt_regs *regs) DEFINE_IDTENTRY_RAW_ERRORCODE(func) /** - * DEFINE_IDTENTRY_VC_SAFE_STACK - Emit code for VMM communication handler - which runs on a safe stack. + * DEFINE_IDTENTRY_VC_KERNEL - Emit code for VMM communication handler + when raised from kernel mode * @func: Function name of the entry point * * Maps to DEFINE_IDTENTRY_RAW_ERRORCODE */ -#define DEFINE_IDTENTRY_VC_SAFE_STACK(func) \ - DEFINE_IDTENTRY_RAW_ERRORCODE(safe_stack_##func) +#define DEFINE_IDTENTRY_VC_KERNEL(func) \ + DEFINE_IDTENTRY_RAW_ERRORCODE(kernel_##func) /** - * DEFINE_IDTENTRY_VC_IST - Emit code for VMM communication handler - which runs on the VC fall-back stack + * DEFINE_IDTENTRY_VC_USER - Emit code for VMM communication handler + when raised from user mode * @func: Function name of the entry point * * Maps to DEFINE_IDTENTRY_RAW_ERRORCODE */ -#define DEFINE_IDTENTRY_VC_IST(func) \ - DEFINE_IDTENTRY_RAW_ERRORCODE(ist_##func) - -/** - * DEFINE_IDTENTRY_VC - Emit code for VMM communication handler - * @func: Function name of the entry point - * - * Maps to DEFINE_IDTENTRY_RAW_ERRORCODE - */ -#define DEFINE_IDTENTRY_VC(func) \ - DEFINE_IDTENTRY_RAW_ERRORCODE(func) +#define DEFINE_IDTENTRY_VC_USER(func) \ + DEFINE_IDTENTRY_RAW_ERRORCODE(user_##func) #else /* CONFIG_X86_64 */ diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c index e0d12728bcb7..fe98cec2973e 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -796,7 +796,7 @@ void __init sev_es_init_vc_handling(void) sev_es_setup_play_dead(); /* Secondary CPUs use the runtime #VC handler */ - initial_vc_handler = (unsigned long)safe_stack_exc_vmm_communication; + initial_vc_handler = (unsigned long)kernel_exc_vmm_communication; } static void __init vc_early_forward_exception(struct es_em_ctxt *ctxt) @@ -1234,14 +1234,6 @@ static enum es_result vc_handle_trap_ac(struct ghcb *ghcb, return ES_EXCEPTION; } -static __always_inline void vc_handle_trap_db(struct pt_regs *regs) -{ - if (user_mode(regs)) - noist_exc_debug(regs); - else - exc_debug(regs); -} - static enum es_result vc_handle_exitcode(struct es_em_ctxt *ctxt, struct ghcb *ghcb, unsigned long exit_code) @@ -1337,41 +1329,13 @@ static __always_inline bool on_vc_fallback_stack(struct pt_regs *regs) return (sp >= __this_cpu_ist_bottom_va(VC2) && sp < __this_cpu_ist_top_va(VC2)); } -/* - * Main #VC exception handler. It is called when the entry code was able to - * switch off the IST to a safe kernel stack. - * - * With the current implementation it is always possible to switch to a safe - * stack because #VC exceptions only happen at known places, like intercepted - * instructions or accesses to MMIO areas/IO ports. They can also happen with - * code instrumentation when the hypervisor intercepts #DB, but the critical - * paths are forbidden to be instrumented, so #DB exceptions currently also - * only happen in safe places. - */ -DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) +static bool vc_raw_handle_exception(struct pt_regs *regs, unsigned long error_code) { - irqentry_state_t irq_state; struct ghcb_state state; struct es_em_ctxt ctxt; enum es_result result; struct ghcb *ghcb; - - /* - * Handle #DB before calling into !noinstr code to avoid recursive #DB. - */ - if (error_code == SVM_EXIT_EXCP_BASE + X86_TRAP_DB) { - vc_handle_trap_db(regs); - return; - } - - irq_state = irqentry_nmi_enter(regs); - instrumentation_begin(); - - /* - * This is invoked through an interrupt gate, so IRQs are disabled. The - * code below might walk page-tables for user or kernel addresses, so - * keep the IRQs disabled to protect us against concurrent TLB flushes. - */ + bool ret = true; ghcb = __sev_get_ghcb(&state); @@ -1391,15 +1355,18 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) case ES_UNSUPPORTED: pr_err_ratelimited("Unsupported exit-code 0x%02lx in #VC exception (IP: 0x%lx)\n", error_code, regs->ip); - goto fail; + ret = false; + break; case ES_VMM_ERROR: pr_err_ratelimited("Failure in communication with VMM (exit-code 0x%02lx IP: 0x%lx)\n", error_code, regs->ip); - goto fail; + ret = false; + break; case ES_DECODE_FAILED: pr_err_ratelimited("Failed to decode instruction (exit-code 0x%02lx IP: 0x%lx)\n", error_code, regs->ip); - goto fail; + ret = false; + break; case ES_EXCEPTION: vc_forward_exception(&ctxt); break; @@ -1415,24 +1382,52 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) BUG(); } -out: - instrumentation_end(); - irqentry_nmi_exit(regs, irq_state); + return ret; +} - return; +static __always_inline bool vc_is_db(unsigned long error_code) +{ + return error_code == SVM_EXIT_EXCP_BASE + X86_TRAP_DB; +} -fail: - if (user_mode(regs)) { - /* - * Do not kill the machine if user-space triggered the - * exception. Send SIGBUS instead and let user-space deal with - * it. - */ - force_sig_fault(SIGBUS, BUS_OBJERR, (void __user *)0); - } else { - pr_emerg("PANIC: Unhandled #VC exception in kernel space (result=%d)\n", - result); +/* + * Runtime #VC exception handler when raised from kernel mode. Runs in NMI mode + * and will panic when an error happens. + */ +DEFINE_IDTENTRY_VC_KERNEL(exc_vmm_communication) +{ + irqentry_state_t irq_state; + + /* + * With the current implementation it is always possible to switch to a + * safe stack because #VC exceptions only happen at known places, like + * intercepted instructions or accesses to MMIO areas/IO ports. They can + * also happen with code instrumentation when the hypervisor intercepts + * #DB, but the critical paths are forbidden to be instrumented, so #DB + * exceptions currently also only happen in safe places. + * + * But keep this here in case the noinstr annotations are violated due + * to bug elsewhere. + */ + if (unlikely(on_vc_fallback_stack(regs))) { + instrumentation_begin(); + panic("Can't handle #VC exception from unsupported context\n"); + instrumentation_end(); + } + + /* + * Handle #DB before calling into !noinstr code to avoid recursive #DB. + */ + if (vc_is_db(error_code)) { + exc_debug(regs); + return; + } + + irq_state = irqentry_nmi_enter(regs); + instrumentation_begin(); + + if (!vc_raw_handle_exception(regs, error_code)) { /* Show some debug info */ show_regs(regs); @@ -1443,23 +1438,38 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) panic("Returned from Terminate-Request to Hypervisor\n"); } - goto out; + instrumentation_end(); + irqentry_nmi_exit(regs, irq_state); } -/* This handler runs on the #VC fall-back stack. It can cause further #VC exceptions */ -DEFINE_IDTENTRY_VC_IST(exc_vmm_communication) +/* + * Runtime #VC exception handler when raised from user mode. Runs in IRQ mode + * and will kill the current task with SIGBUS when an error happens. + */ +DEFINE_IDTENTRY_VC_USER(exc_vmm_communication) { + /* + * Handle #DB before calling into !noinstr code to avoid recursive #DB. + */ + if (vc_is_db(error_code)) { + noist_exc_debug(regs); + return; + } + + irqentry_enter_from_user_mode(regs); instrumentation_begin(); - panic("Can't handle #VC exception from unsupported context\n"); - instrumentation_end(); -} -DEFINE_IDTENTRY_VC(exc_vmm_communication) -{ - if (likely(!on_vc_fallback_stack(regs))) - safe_stack_exc_vmm_communication(regs, error_code); - else - ist_exc_vmm_communication(regs, error_code); + if (!vc_raw_handle_exception(regs, error_code)) { + /* + * Do not kill the machine if user-space triggered the + * exception. Send SIGBUS instead and let user-space deal with + * it. + */ + force_sig_fault(SIGBUS, BUS_OBJERR, (void __user *)0); + } + + instrumentation_end(); + irqentry_exit_to_user_mode(regs); } bool __init handle_vc_boot_ghcb(struct pt_regs *regs) -- 2.31.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-06-18 10:54 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-16 18:49 [PATCH v6 0/2] x86/sev: Fixes for SEV-ES Guest Support Joerg Roedel 2021-06-16 18:49 ` [PATCH v6 1/2] x86/sev: Make sure IRQs are disabled while GHCB is active Joerg Roedel 2021-06-17 15:00 ` Peter Zijlstra 2021-06-18 8:25 ` Joerg Roedel 2021-06-17 15:38 ` Peter Zijlstra 2021-06-18 8:17 ` Joerg Roedel 2021-06-18 10:53 ` Peter Zijlstra 2021-06-16 18:49 ` [PATCH v6 2/2] x86/sev: Split up runtime #VC handler for correct state tracking Joerg Roedel 2021-06-17 13:55 ` [PATCH v6.1 " Joerg Roedel
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).