| Message ID | 20200428151725.31091-43-joro@8bytes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: SEV-ES Guest Support | expand |
On Tue, Apr 28, 2020 at 05:16:52PM +0200, Joerg Roedel wrote: > diff --git a/arch/x86/include/asm/sev-es.h b/arch/x86/include/asm/sev-es.h > index b2cbcd40b52e..e1ed963a57ec 100644 > --- a/arch/x86/include/asm/sev-es.h > +++ b/arch/x86/include/asm/sev-es.h > @@ -74,5 +74,6 @@ static inline u64 lower_bits(u64 val, unsigned int bits) > } > > extern void vc_no_ghcb(void); > +extern bool vc_boot_ghcb(struct pt_regs *regs); Those function names need verbs: handle_vc_no_ghcb handle_vc_boot_ghcb > @@ -161,3 +176,104 @@ static enum es_result vc_read_mem(struct es_em_ctxt *ctxt, > > /* Include code shared with pre-decompression boot stage */ > #include "sev-es-shared.c" > + > +/* > + * This function runs on the first #VC exception after the kernel > + * switched to virtual addresses. > + */ > +static bool __init sev_es_setup_ghcb(void) There's already another sev_es_setup_ghcb() in compressed/. All those functions with the same name are just confusion waiting to happen. Let's prepend the ones in compressed/ with "early_" or so, so that their names are at least different even if they're in two different files with the same name. This way you know at least which function is used in which boot stages. > +{ > + /* First make sure the hypervisor talks a supported protocol. */ > + if (!sev_es_negotiate_protocol()) > + return false; <---- newline here. > + /* > + * Clear the boot_ghcb. The first exception comes in before the bss > + * section is cleared. > + */ > + memset(&boot_ghcb_page, 0, PAGE_SIZE); > + > + /* Alright - Make the boot-ghcb public */ > + boot_ghcb = &boot_ghcb_page; > + > + return true; > +} > + > +static void __init vc_early_vc_forward_exception(struct es_em_ctxt *ctxt) That second "vc" looks redundant. > +{ > + int trapnr = ctxt->fi.vector; > + > + if (trapnr == X86_TRAP_PF) > + native_write_cr2(ctxt->fi.cr2); > + > + ctxt->regs->orig_ax = ctxt->fi.error_code; > + do_early_exception(ctxt->regs, trapnr); > +} > + > +static enum es_result vc_handle_exitcode(struct es_em_ctxt *ctxt, > + struct ghcb *ghcb, > + unsigned long exit_code) > +{ > + enum es_result result; > + > + switch (exit_code) { > + default: > + /* > + * Unexpected #VC exception > + */ > + result = ES_UNSUPPORTED; > + } > + > + return result; > +} > + > +bool __init vc_boot_ghcb(struct pt_regs *regs) > +{ > + unsigned long exit_code = regs->orig_ax; > + struct es_em_ctxt ctxt; > + enum es_result result; > + > + /* Do initial setup or terminate the guest */ > + if (unlikely(boot_ghcb == NULL && !sev_es_setup_ghcb())) > + sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST); > + > + vc_ghcb_invalidate(boot_ghcb); Newline here... > + result = vc_init_em_ctxt(&ctxt, regs, exit_code); > + ... remove that one here. > + if (result == ES_OK) > + result = vc_handle_exitcode(&ctxt, boot_ghcb, exit_code); ...
On Wed, May 20, 2020 at 09:22:30PM +0200, Borislav Petkov wrote: > On Tue, Apr 28, 2020 at 05:16:52PM +0200, Joerg Roedel wrote: > > diff --git a/arch/x86/include/asm/sev-es.h b/arch/x86/include/asm/sev-es.h > > index b2cbcd40b52e..e1ed963a57ec 100644 > > --- a/arch/x86/include/asm/sev-es.h > > +++ b/arch/x86/include/asm/sev-es.h > > @@ -74,5 +74,6 @@ static inline u64 lower_bits(u64 val, unsigned int bits) > > } > > > > extern void vc_no_ghcb(void); > > +extern bool vc_boot_ghcb(struct pt_regs *regs); > > Those function names need verbs: > > handle_vc_no_ghcb > handle_vc_boot_ghcb This are IDT entry points and the names above follow the convention for them, like e.g. 'page_fault', 'nmi' or 'general_protection'. Should I still add the verbs or just add a comment explaining what those symbols are? > There's already another sev_es_setup_ghcb() in compressed/. All those > functions with the same name are just confusion waiting to happen. Let's > prepend the ones in compressed/ with "early_" or so, so that their names > are at least different even if they're in two different files with the > same name. > > This way you know at least which function is used in which boot stages. Okay, will see what can be changed. Some functions are part of the interface for sev-es-shared.c and need to have the same names, but sev_es_setup_ghcb() can be named differently. > > +static void __init vc_early_vc_forward_exception(struct es_em_ctxt *ctxt) > > That second "vc" looks redundant. Heh, search and replace artifact :) Fixed now. Joerg
On Thu, Jun 04, 2020 at 02:07:49PM +0200, Joerg Roedel wrote: > This are IDT entry points and the names above follow the convention for > them, like e.g. 'page_fault', 'nmi' or 'general_protection'. Should I > still add the verbs or just add a comment explaining what those symbols > are? Hmmkay, I see vc_no_ghcb doing call do_vc_no_ghcb and that's setup in early_idt_setup(). vc_boot_ghcb(), OTOH, is called by do_early_exception() only so that one could be called handle_vc_boot_ghcb(), no? I.e., I don't see it being an IDT entry point.
On Thu, Jun 04, 2020 at 05:30:27PM +0200, Borislav Petkov wrote: > Hmmkay, I see vc_no_ghcb doing > > call do_vc_no_ghcb > > and that's setup in early_idt_setup(). > > vc_boot_ghcb(), OTOH, is called by do_early_exception() only so that one > could be called handle_vc_boot_ghcb(), no? I.e., I don't see it being an > IDT entry point. Right, I renamed it to handle_vc_boot_ghcb(). Joerg
diff --git a/arch/x86/include/asm/segment.h b/arch/x86/include/asm/segment.h index 6669164abadc..5b648066504c 100644 --- a/arch/x86/include/asm/segment.h +++ b/arch/x86/include/asm/segment.h @@ -230,7 +230,7 @@ #define NUM_EXCEPTION_VECTORS 32 /* Bitmask of exception vectors which push an error code on the stack: */ -#define EXCEPTION_ERRCODE_MASK 0x00027d00 +#define EXCEPTION_ERRCODE_MASK 0x20027d00 #define GDT_SIZE (GDT_ENTRIES*8) #define GDT_ENTRY_TLS_ENTRIES 3 diff --git a/arch/x86/include/asm/sev-es.h b/arch/x86/include/asm/sev-es.h index b2cbcd40b52e..e1ed963a57ec 100644 --- a/arch/x86/include/asm/sev-es.h +++ b/arch/x86/include/asm/sev-es.h @@ -74,5 +74,6 @@ static inline u64 lower_bits(u64 val, unsigned int bits) } extern void vc_no_ghcb(void); +extern bool vc_boot_ghcb(struct pt_regs *regs); #endif diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c index 9586522bfcb3..d83d59c15548 100644 --- a/arch/x86/kernel/head64.c +++ b/arch/x86/kernel/head64.c @@ -386,6 +386,12 @@ void __init do_early_exception(struct pt_regs *regs, int trapnr) early_make_pgtable(native_read_cr2())) return; +#ifdef CONFIG_AMD_MEM_ENCRYPT + if (trapnr == X86_TRAP_VC && + vc_boot_ghcb(regs)) + return; +#endif + early_fixup_exception(regs, trapnr); } diff --git a/arch/x86/kernel/sev-es-shared.c b/arch/x86/kernel/sev-es-shared.c index 5703f9b17e70..3d2299fb5e3f 100644 --- a/arch/x86/kernel/sev-es-shared.c +++ b/arch/x86/kernel/sev-es-shared.c @@ -9,7 +9,7 @@ * and is included directly into both code-bases. */ -static void __maybe_unused sev_es_terminate(unsigned int reason) +static void sev_es_terminate(unsigned int reason) { u64 val = GHCB_SEV_TERMINATE; @@ -27,7 +27,7 @@ static void __maybe_unused sev_es_terminate(unsigned int reason) asm volatile("hlt\n" : : : "memory"); } -static bool __maybe_unused sev_es_negotiate_protocol(void) +static bool sev_es_negotiate_protocol(void) { u64 val; @@ -46,7 +46,7 @@ static bool __maybe_unused sev_es_negotiate_protocol(void) return true; } -static void __maybe_unused vc_ghcb_invalidate(struct ghcb *ghcb) +static void vc_ghcb_invalidate(struct ghcb *ghcb) { memset(ghcb->save.valid_bitmap, 0, sizeof(ghcb->save.valid_bitmap)); } @@ -58,9 +58,9 @@ static bool vc_decoding_needed(unsigned long exit_code) exit_code <= SVM_EXIT_LAST_EXCP); } -static enum es_result __maybe_unused vc_init_em_ctxt(struct es_em_ctxt *ctxt, - struct pt_regs *regs, - unsigned long exit_code) +static enum es_result vc_init_em_ctxt(struct es_em_ctxt *ctxt, + struct pt_regs *regs, + unsigned long exit_code) { enum es_result ret = ES_OK; @@ -73,7 +73,7 @@ static enum es_result __maybe_unused vc_init_em_ctxt(struct es_em_ctxt *ctxt, return ret; } -static void __maybe_unused vc_finish_insn(struct es_em_ctxt *ctxt) +static void vc_finish_insn(struct es_em_ctxt *ctxt) { ctxt->regs->ip += ctxt->insn.length; } diff --git a/arch/x86/kernel/sev-es.c b/arch/x86/kernel/sev-es.c index 0d20770decce..9de5bb23cb0a 100644 --- a/arch/x86/kernel/sev-es.c +++ b/arch/x86/kernel/sev-es.c @@ -7,7 +7,9 @@ * Author: Joerg Roedel <jroedel@suse.de> */ +#include <linux/sched/debug.h> /* For show_regs() */ #include <linux/kernel.h> +#include <linux/printk.h> #include <linux/mm.h> #include <asm/trap_defs.h> @@ -15,8 +17,21 @@ #include <asm/insn-eval.h> #include <asm/fpu/internal.h> #include <asm/processor.h> +#include <asm/trap_defs.h> #include <asm/svm.h> +/* For early boot hypervisor communication in SEV-ES enabled guests */ +static struct ghcb boot_ghcb_page __bss_decrypted __aligned(PAGE_SIZE); + +/* + * Needs to be in the .data section because we need it NULL before bss is + * cleared + */ +static struct ghcb __initdata *boot_ghcb; + +/* Needed in vc_early_vc_forward_exception */ +void do_early_exception(struct pt_regs *regs, int trapnr); + static inline u64 sev_es_rd_ghcb_msr(void) { return native_read_msr(MSR_AMD64_SEV_ES_GHCB); @@ -161,3 +176,104 @@ static enum es_result vc_read_mem(struct es_em_ctxt *ctxt, /* Include code shared with pre-decompression boot stage */ #include "sev-es-shared.c" + +/* + * This function runs on the first #VC exception after the kernel + * switched to virtual addresses. + */ +static bool __init sev_es_setup_ghcb(void) +{ + /* First make sure the hypervisor talks a supported protocol. */ + if (!sev_es_negotiate_protocol()) + return false; + /* + * Clear the boot_ghcb. The first exception comes in before the bss + * section is cleared. + */ + memset(&boot_ghcb_page, 0, PAGE_SIZE); + + /* Alright - Make the boot-ghcb public */ + boot_ghcb = &boot_ghcb_page; + + return true; +} + +static void __init vc_early_vc_forward_exception(struct es_em_ctxt *ctxt) +{ + int trapnr = ctxt->fi.vector; + + if (trapnr == X86_TRAP_PF) + native_write_cr2(ctxt->fi.cr2); + + ctxt->regs->orig_ax = ctxt->fi.error_code; + do_early_exception(ctxt->regs, trapnr); +} + +static enum es_result vc_handle_exitcode(struct es_em_ctxt *ctxt, + struct ghcb *ghcb, + unsigned long exit_code) +{ + enum es_result result; + + switch (exit_code) { + default: + /* + * Unexpected #VC exception + */ + result = ES_UNSUPPORTED; + } + + return result; +} + +bool __init vc_boot_ghcb(struct pt_regs *regs) +{ + unsigned long exit_code = regs->orig_ax; + struct es_em_ctxt ctxt; + enum es_result result; + + /* Do initial setup or terminate the guest */ + if (unlikely(boot_ghcb == NULL && !sev_es_setup_ghcb())) + sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST); + + vc_ghcb_invalidate(boot_ghcb); + result = vc_init_em_ctxt(&ctxt, regs, exit_code); + + if (result == ES_OK) + result = vc_handle_exitcode(&ctxt, boot_ghcb, exit_code); + + /* Done - now check the result */ + switch (result) { + case ES_OK: + vc_finish_insn(&ctxt); + break; + case ES_UNSUPPORTED: + early_printk("PANIC: Unsupported exit-code 0x%02lx in early #VC exception (IP: 0x%lx)\n", + exit_code, regs->ip); + goto fail; + case ES_VMM_ERROR: + early_printk("PANIC: Failure in communication with VMM (exit-code 0x%02lx IP: 0x%lx)\n", + exit_code, regs->ip); + goto fail; + case ES_DECODE_FAILED: + early_printk("PANIC: Failed to decode instruction (exit-code 0x%02lx IP: 0x%lx)\n", + exit_code, regs->ip); + goto fail; + case ES_EXCEPTION: + vc_early_vc_forward_exception(&ctxt); + break; + case ES_RETRY: + /* Nothing to do */ + break; + default: + BUG(); + } + + return true; + +fail: + show_regs(regs); + + while (true) + halt(); +} diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c index b991aa4bdfae..5bcbd413b409 100644 --- a/arch/x86/mm/extable.c +++ b/arch/x86/mm/extable.c @@ -5,6 +5,7 @@ #include <xen/xen.h> #include <asm/fpu/internal.h> +#include <asm/sev-es.h> #include <asm/traps.h> #include <asm/kdebug.h>