lkml.org 
[lkml]   [2023]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH v2 17/20] x86: efistub: Check SEV/SNP support while running in the firmware
From
On 5/8/23 02:03, Ard Biesheuvel wrote:
> The decompressor executes in an environment with little or no access to
> a console, and without any ability to return an error back to the caller
> (the bootloader). So the only recourse we have when the SEV/SNP context
> is not quite what the kernel expects is to terminate the guest entirely.
>
> This is a bit harsh, and also unnecessary when booting via the EFI stub,
> given that it provides all the support that SEV guests need to probe the
> underlying platform.
>
> So let's do the SEV initialization and SNP feature check before calling
> ExitBootServices(), and simply return with an error if the SNP feature
> mask is not as expected.

My SEV-ES / SEV-SNP guests started crashing when I applied this patch.
Turns out that sev_es_negotiate_protocol() used to be called when no #VC
exceptions were being generated before a valid GHCB was setup. Because
of that the current GHCB MSR value was not saved/restored. But now,
sev_es_negotiate_protocol() is called earlier in the boot process and
there are still messages being issued by UEFI, e.g.:

SetUefiImageMemoryAttributes - 0x000000007F6D7000 - 0x0000000000006000 (0x0000000000000008)

Similarly, get_hv_features() didn't worry about saving the GHCB MSR value
and an SNP guest crashed after fixing sev_es_negotiate_protocol().

The following changes got me past everything:

diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index 3a5b0c9c4fcc..23450628d41c 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -106,15 +106,19 @@ static void __noreturn sev_es_terminate(unsigned int set, unsigned int reason)
*/
static u64 get_hv_features(void)
{
- u64 val;
+ u64 val, save;

if (ghcb_version < 2)
return 0;

+ save = sev_es_rd_ghcb_msr();
+
sev_es_wr_ghcb_msr(GHCB_MSR_HV_FT_REQ);
VMGEXIT();
-
val = sev_es_rd_ghcb_msr();
+
+ sev_es_wr_ghcb_msr(save);
+
if (GHCB_RESP_CODE(val) != GHCB_MSR_HV_FT_RESP)
return 0;

@@ -139,13 +143,17 @@ static void snp_register_ghcb_early(unsigned long paddr)

static bool sev_es_negotiate_protocol(void)
{
- u64 val;
+ u64 val, save;
+
+ save = sev_es_rd_ghcb_msr();

/* Do the GHCB protocol version negotiation */
sev_es_wr_ghcb_msr(GHCB_MSR_SEV_INFO_REQ);
VMGEXIT();
val = sev_es_rd_ghcb_msr();

+ sev_es_wr_ghcb_msr(save);
+
if (GHCB_MSR_INFO(val) != GHCB_MSR_SEV_INFO_RESP)
return false;

Thanks,
Tom

>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> arch/x86/boot/compressed/sev.c | 12 ++++++++----
> arch/x86/include/asm/sev.h | 4 ++++
> drivers/firmware/efi/libstub/x86-stub.c | 17 +++++++++++++++++
> 3 files changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index 014b89c890887b9a..19c40873fdd209b5 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -315,20 +315,24 @@ static void enforce_vmpl0(void)
> */
> #define SNP_FEATURES_PRESENT (0)
>
> +u64 snp_get_unsupported_features(void)
> +{
> + if (!(sev_status & MSR_AMD64_SEV_SNP_ENABLED))
> + return 0;
> + return sev_status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT;
> +}
> +
> void snp_check_features(void)
> {
> u64 unsupported;
>
> - if (!(sev_status & MSR_AMD64_SEV_SNP_ENABLED))
> - return;
> -
> /*
> * Terminate the boot if hypervisor has enabled any feature lacking
> * guest side implementation. Pass on the unsupported features mask through
> * EXIT_INFO_2 of the GHCB protocol so that those features can be reported
> * as part of the guest boot failure.
> */
> - unsupported = sev_status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT;
> + unsupported = snp_get_unsupported_features();
> if (unsupported) {
> if (ghcb_version < 2 || (!boot_ghcb && !early_setup_ghcb()))
> sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 13dc2a9d23c1eb25..bf27b91644d0da5a 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -157,6 +157,7 @@ static __always_inline void sev_es_nmi_complete(void)
> __sev_es_nmi_complete();
> }
> extern int __init sev_es_efi_map_ghcbs(pgd_t *pgd);
> +extern void sev_enable(struct boot_params *bp);
>
> static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs)
> {
> @@ -202,12 +203,14 @@ void snp_set_wakeup_secondary_cpu(void);
> bool snp_init(struct boot_params *bp);
> void __init __noreturn snp_abort(void);
> int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct snp_guest_request_ioctl *rio);
> +u64 snp_get_unsupported_features(void);
> #else
> static inline void sev_es_ist_enter(struct pt_regs *regs) { }
> static inline void sev_es_ist_exit(void) { }
> static inline int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) { return 0; }
> static inline void sev_es_nmi_complete(void) { }
> static inline int sev_es_efi_map_ghcbs(pgd_t *pgd) { return 0; }
> +static inline void sev_enable(struct boot_params *bp) { }
> static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate) { return 0; }
> static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs) { return 0; }
> static inline void setup_ghcb(void) { }
> @@ -225,6 +228,7 @@ static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *in
> {
> return -ENOTTY;
> }
> +static inline u64 snp_get_unsupported_features(void) { return 0; }
> #endif
>
> #endif
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index ce8434fce0c37982..33d11ba78f1d8c4f 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -15,6 +15,7 @@
> #include <asm/setup.h>
> #include <asm/desc.h>
> #include <asm/boot.h>
> +#include <asm/sev.h>
>
> #include "efistub.h"
>
> @@ -714,6 +715,22 @@ static efi_status_t exit_boot_func(struct efi_boot_memmap *map,
> &p->efi->efi_memmap, &p->efi->efi_memmap_hi);
> p->efi->efi_memmap_size = map->map_size;
>
> + /*
> + * Call the SEV init code while still running with the firmware's
> + * GDT/IDT, so #VC exceptions will be handled by EFI.
> + */
> + if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)) {
> + u64 unsupported;
> +
> + sev_enable(p->boot_params);
> + unsupported = snp_get_unsupported_features();
> + if (unsupported) {
> + efi_err("Unsupported SEV-SNP features detected: 0x%llx\n",
> + unsupported);
> + return EFI_UNSUPPORTED;
> + }
> + }
> +
> return EFI_SUCCESS;
> }
>

\
 
 \ /
  Last update: 2023-05-18 22:17    [W:0.175 / U:21.972 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site