| Message ID | 20210602140416.23573-7-brijesh.singh@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand |
On Wed, Jun 02, 2021 at 09:04:00AM -0500, Brijesh Singh wrote: > static bool early_setup_sev_es(void) This function is doing SNP init now too, so it should be called something generic like do_early_sev_setup() or so. > #define GHCB_SEV_ES_GEN_REQ 0 > #define GHCB_SEV_ES_PROT_UNSUPPORTED 1 > +#define GHCB_SEV_ES_SNP_UNSUPPORTED 2 GHCB_SNP_UNSUPPORTED > +static bool __init sev_snp_check_hypervisor_features(void) check_hv_features() is nice and short. > diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c > index 77a754365ba9..9b70b7332614 100644 > --- a/arch/x86/kernel/sev.c > +++ b/arch/x86/kernel/sev.c > @@ -609,6 +609,10 @@ static bool __init sev_es_setup_ghcb(void) Ditto for this one: setup_ghcb() Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
On 6/7/21 9:54 AM, Borislav Petkov wrote: > On Wed, Jun 02, 2021 at 09:04:00AM -0500, Brijesh Singh wrote: >> static bool early_setup_sev_es(void) > This function is doing SNP init now too, so it should be called > something generic like > > do_early_sev_setup() > > or so. Okay, noted. >> #define GHCB_SEV_ES_GEN_REQ 0 >> #define GHCB_SEV_ES_PROT_UNSUPPORTED 1 >> +#define GHCB_SEV_ES_SNP_UNSUPPORTED 2 > GHCB_SNP_UNSUPPORTED Noted. > >> +static bool __init sev_snp_check_hypervisor_features(void) > check_hv_features() > > is nice and short. Noted. >> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c >> index 77a754365ba9..9b70b7332614 100644 >> --- a/arch/x86/kernel/sev.c >> +++ b/arch/x86/kernel/sev.c >> @@ -609,6 +609,10 @@ static bool __init sev_es_setup_ghcb(void) > Ditto for this one: setup_ghcb() Noted. > > Thx. >
Hi Boris, On 6/7/2021 9:54 AM, Borislav Petkov wrote: > On Wed, Jun 02, 2021 at 09:04:00AM -0500, Brijesh Singh wrote: >> static bool early_setup_sev_es(void) > > This function is doing SNP init now too, so it should be called > something generic like > > do_early_sev_setup() > > or so. > >> #define GHCB_SEV_ES_GEN_REQ 0 >> #define GHCB_SEV_ES_PROT_UNSUPPORTED 1 >> +#define GHCB_SEV_ES_SNP_UNSUPPORTED 2 > > GHCB_SNP_UNSUPPORTED > >> +static bool __init sev_snp_check_hypervisor_features(void) > > check_hv_features() > Based on your feedback on AP creation patch to not use the accessors, I am inclined to remove this helper and have the caller directly check the feature bit, is that okay ? something like: if (sev_snp_enabled() && !(hv_features & GHCB_HV_FT_SNP)) sev_es_terminate(GHCB_SNP_UNSUPPORTED); Let me know if you think I should still keep the accessors. -Brijesh > is nice and short. > >> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c >> index 77a754365ba9..9b70b7332614 100644 >> --- a/arch/x86/kernel/sev.c >> +++ b/arch/x86/kernel/sev.c >> @@ -609,6 +609,10 @@ static bool __init sev_es_setup_ghcb(void) > > Ditto for this one: setup_ghcb() > > Thx. >
On Thu, Jun 17, 2021 at 01:46:08PM -0500, Brijesh Singh wrote: > Based on your feedback on AP creation patch to not use the accessors, I am inclined to > remove this helper and have the caller directly check the feature bit, is that okay ? > > something like: > > if (sev_snp_enabled() && !(hv_features & GHCB_HV_FT_SNP)) > sev_es_terminate(GHCB_SNP_UNSUPPORTED); > > Let me know if you think I should still keep the accessors. Yeah, looks about right. Let's keep hv_features in a sev-specific header so that there are no name clashes. Or maybe we should call it sev_hv_features since it is going to be read-only anyway. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c index 87621f4e4703..0745ea61d32e 100644 --- a/arch/x86/boot/compressed/sev.c +++ b/arch/x86/boot/compressed/sev.c @@ -25,6 +25,7 @@ struct ghcb boot_ghcb_page __aligned(PAGE_SIZE); struct ghcb *boot_ghcb; +static u64 msr_sev_status; /* * Copy a version of this function here - insn-eval.c can't be used in @@ -119,11 +120,32 @@ static enum es_result vc_read_mem(struct es_em_ctxt *ctxt, /* Include code for early handlers */ #include "../../kernel/sev-shared.c" +static inline bool sev_snp_enabled(void) +{ + unsigned long low, high; + + if (!msr_sev_status) { + asm volatile("rdmsr\n" + : "=a" (low), "=d" (high) + : "c" (MSR_AMD64_SEV)); + msr_sev_status = (high << 32) | low; + } + + return msr_sev_status & MSR_AMD64_SEV_SNP_ENABLED; +} + static bool early_setup_sev_es(void) { if (!sev_es_negotiate_protocol()) sev_es_terminate(0, GHCB_SEV_ES_PROT_UNSUPPORTED); + /* + * If SEV-SNP is enabled, then check if the hypervisor supports the SEV-SNP + * features. + */ + if (sev_snp_enabled() && !sev_snp_check_hypervisor_features()) + sev_es_terminate(0, GHCB_SEV_ES_SNP_UNSUPPORTED); + if (set_page_decrypted((unsigned long)&boot_ghcb_page)) return false; diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h index 981fff2257b9..3ebf00772f26 100644 --- a/arch/x86/include/asm/sev-common.h +++ b/arch/x86/include/asm/sev-common.h @@ -51,6 +51,8 @@ #define GHCB_MSR_HV_FT_POS 12 #define GHCB_MSR_HV_FT_MASK GENMASK_ULL(51, 0) +#define GHCB_HV_FT_SNP BIT_ULL(0) + #define GHCB_MSR_HV_FT_RESP_VAL(v) \ (((unsigned long)((v) & GHCB_MSR_HV_FT_MASK) >> GHCB_MSR_HV_FT_POS)) @@ -65,6 +67,7 @@ #define GHCB_SEV_ES_GEN_REQ 0 #define GHCB_SEV_ES_PROT_UNSUPPORTED 1 +#define GHCB_SEV_ES_SNP_UNSUPPORTED 2 #define GHCB_RESP_CODE(v) ((v) & GHCB_MSR_INFO_MASK) diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c index 94957c5bdb51..b8312ad66120 100644 --- a/arch/x86/kernel/sev-shared.c +++ b/arch/x86/kernel/sev-shared.c @@ -32,6 +32,17 @@ static bool __init sev_es_check_cpu_features(void) return true; } +static bool __init sev_snp_check_hypervisor_features(void) +{ + if (ghcb_version < 2) + return false; + + if (!(hv_features & GHCB_HV_FT_SNP)) + return false; + + return true; +} + static void __noreturn sev_es_terminate(unsigned int set, unsigned int reason) { u64 val = GHCB_MSR_TERM_REQ; diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c index 77a754365ba9..9b70b7332614 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -609,6 +609,10 @@ static bool __init sev_es_setup_ghcb(void) if (!sev_es_negotiate_protocol()) return false; + /* If SNP is active, make sure that hypervisor supports the feature. */ + if (sev_feature_enabled(SEV_SNP) && !sev_snp_check_hypervisor_features()) + sev_es_terminate(0, GHCB_SEV_ES_SNP_UNSUPPORTED); + /* * Clear the boot_ghcb. The first exception comes in before the bss * section is cleared.
Version 2 of the GHCB specification added the advertisement of features that are supported by the hypervisor. If hypervisor supports the SEV-SNP then it must set the SEV-SNP features bit to indicate that the base SEV-SNP is supported. Check the SEV-SNP feature while establishing the GHCB, if failed, terminate the guest. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- arch/x86/boot/compressed/sev.c | 22 ++++++++++++++++++++++ arch/x86/include/asm/sev-common.h | 3 +++ arch/x86/kernel/sev-shared.c | 11 +++++++++++ arch/x86/kernel/sev.c | 4 ++++ 4 files changed, 40 insertions(+)