x86/sev: Fixes for SEV-ES Guest Support
Subsystems (4)
v7 cover x86/sev: Fixes for SEV-ES Guest Support
From: Joerg Roedel <[email protected]>
Hi,
please find here the next iteration of my pending fixes for SEV-ES guest support in Linux. This version addresses the comments I received from Peter on the previous version, in particular:
- Removed IRQ disable/enable calls in the ap-hlt loop code
- Made __sev_get/put_ghcb() noinstr instead of __always_inline and put instrumentation_begin()/end() calls around panic() to fix an objtool warning.
- Split up #DB handling in the #VC handler into a from-user and from-kernel part as well.
The patches are again tested with a kernel with various debugging options enabled, running as an SEV-ES guest.
The previous version of these patches can be found here:
https://lore.kernel.org/lkml/[email protected]/
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 | 182 ++++++++++++++++++-------------- 3 files changed, 113 insertions(+), 102 deletions(-)
base-commit: 07570cef5e5c3fcec40f82a9075abb4c1da63319
Signature · 1 line
2.31.1
PATCH v7 1/2 x86/sev: Make sure IRQs are disabled while GHCB is active
From: Joerg Roedel <[email protected]>
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().
--- arch/x86/kernel/sev.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-)
arch/x86/kernel/sev.c +22 -12
@@ -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 noinstr 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;
@@ -213,7 +220,9 @@ static __always_inline struct ghcb *sev_es_get_ghcb(struct ghcb_state *state) data->ghcb_active = false; data->backup_ghcb_active = false; +instrumentation_begin ();panic ("Unable to handle #VC exception! GHCB and Backup GHCB are already in use" ); +instrumentation_end (); }/* Mark backup_ghcb active before writing to it */
@@ -486,11 +495,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 noinstr 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 +525,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 +535,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 +547,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 +561,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);
@@ -675,7 +686,7 @@ static void sev_es_ap_hlt_loop(void)struct ghcb_state state;struct ghcb *ghcb; - ghcb =sev_es_get_ghcb (&state); + ghcb =__sev_get_ghcb (&state);while (true) {vc_ghcb_invalidate (ghcb);
@@ -692,7 +703,7 @@ static void sev_es_ap_hlt_loop(void)break ; } -sev_es_put_ghcb (&state); +__sev_put_ghcb (&state); }/*
@@ -1351,7 +1362,6 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) } irq_state =irqentry_nmi_enter (regs); -lockdep_assert_irqs_disabled ();instrumentation_begin ();/*
@@ -1360,7 +1370,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 +1378,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) {
Signature · 1 line
2.31.1
Borislav Petkov
maintainerRe: [PATCH v7 1/2] x86/sev: Make sure IRQs are disabled while GHCB is active
Quoted 10 lines · Joerg Roedel
From: Joerg Roedel <[email protected]>
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 insev_es_put_ghcb().
Note for committer: update those function names.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette[tip: x86/sev] x86/sev: Make sure IRQs are disabled while GHCB is active
In reply to Joerg RoedelThe following commit has been merged into the x86/sev branch of tip:
The following commit has been merged into the x86/sev branch of tip:
Commit-ID: d187f217335dba2b49fc9002aab2004e04acddee Gitweb: https://git.kernel.org/tip/d187f217335dba2b49fc9002aab2004e04acddee Author: Joerg Roedel <[email protected]> AuthorDate: Fri, 18 Jun 2021 13:54:08 +02:00 Committer: Borislav Petkov <[email protected]> CommitterDate: Mon, 21 Jun 2021 15:51:21 +02:00
x86/sev: Make sure IRQs are disabled while GHCB is active
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 making sure that callers of __sev_{get,put}_ghcb() have disabled interrupts upfront.
[ bp: Massage commit message. ]
--- arch/x86/kernel/sev.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-)
arch/x86/kernel/sev.c +22 -12
@@ -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 noinstr 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;
@@ -213,7 +220,9 @@ static __always_inline struct ghcb *sev_es_get_ghcb(struct ghcb_state *state) data->ghcb_active = false; data->backup_ghcb_active = false; +instrumentation_begin ();panic ("Unable to handle #VC exception! GHCB and Backup GHCB are already in use" ); +instrumentation_end (); }/* Mark backup_ghcb active before writing to it */
@@ -486,11 +495,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 noinstr 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 +525,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 +535,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 +547,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 +561,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);
@@ -675,7 +686,7 @@ static void sev_es_ap_hlt_loop(void)struct ghcb_state state;struct ghcb *ghcb; - ghcb =sev_es_get_ghcb (&state); + ghcb =__sev_get_ghcb (&state);while (true) {vc_ghcb_invalidate (ghcb);
@@ -692,7 +703,7 @@ static void sev_es_ap_hlt_loop(void)break ; } -sev_es_put_ghcb (&state); +__sev_put_ghcb (&state); }/*
@@ -1351,7 +1362,6 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) } irq_state =irqentry_nmi_enter (regs); -lockdep_assert_irqs_disabled ();instrumentation_begin ();/*
@@ -1360,7 +1370,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 +1378,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) {
PATCH v7 2/2 x86/sev: Split up runtime #VC handler for correct state tracking
In reply to Joerg RoedelFrom: Joerg Roedel <[email protected]>
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.
--- 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(-)
arch/x86/entry/entry_64.S +2 -2
@@ -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)
arch/x86/include/asm/idtentry.h +10 -19
@@ -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 */
arch/x86/kernel/sev.c +79 -69
203 lines not on this page · raw message
Signature · 1 line
2.31.1
[tip: x86/sev] x86/sev: Split up runtime #VC handler for correct state tracking
The following commit has been merged into the x86/sev branch of tip:
The following commit has been merged into the x86/sev branch of tip:
Commit-ID: be1a5408868af341f61f93c191b5e346ee88c82a Gitweb: https://git.kernel.org/tip/be1a5408868af341f61f93c191b5e346ee88c82a Author: Joerg Roedel <[email protected]> AuthorDate: Fri, 18 Jun 2021 13:54:09 +02:00 Committer: Borislav Petkov <[email protected]> CommitterDate: Mon, 21 Jun 2021 16:01:05 +02:00
x86/sev: Split up runtime #VC handler for correct state tracking
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.
--- 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(-)
arch/x86/entry/entry_64.S +2 -2
@@ -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)
arch/x86/include/asm/idtentry.h +10 -19
@@ -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 */
arch/x86/kernel/sev.c +79 -69
203 lines not on this page · raw message
Quoted 3 lines · Joerg Roedel
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
Quoted 6 lines · Peter Zijlstra
Quoted 3 lines · Joerg Roedel
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
Thanks Peter, also for your help with improving this code.