From: Ard Biesheuvel <ardb@kernel.org>

The decompressor and the core kernel both check the hypervisor feature
mask exposed by the hypervisor, but test it in slightly different ways.

This disparity seems unintentional, and simply a result of the fact that
the decompressor and the core kernel evolve differently over time when
it comes to setting up the SEV-SNP execution context.

So move the HV feature check into a helper function and call that
instead. For the core kernel, move the check to an earlier boot stage,
right after the point where it is established that the guest is
executing in SEV-SNP mode.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/compressed/sev.c      | 19 +------------
 arch/x86/boot/startup/sev-shared.c  | 30 +++++++++++++++++---
 arch/x86/boot/startup/sme.c         |  2 ++
 arch/x86/coco/sev/core.c            | 11 -------
 arch/x86/include/asm/sev-internal.h |  2 +-
 arch/x86/include/asm/sev.h          |  2 ++
 6 files changed, 32 insertions(+), 34 deletions(-)

diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index 5fd51f51e55c..4bd7b45562ed 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -397,24 +397,7 @@ void sev_enable(struct boot_params *bp)
 			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SEV_ES_PROT_UNSUPPORTED);
 	}
 
-	/*
-	 * SNP is supported in v2 of the GHCB spec which mandates support for HV
-	 * features.
-	 */
-	if (sev_status & MSR_AMD64_SEV_SNP_ENABLED) {
-		u64 hv_features;
-
-		hv_features = get_hv_features();
-		if (!(hv_features & GHCB_HV_FT_SNP))
-			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
-
-		/*
-		 * Running at VMPL0 is required unless an SVSM is present and
-		 * the hypervisor supports the required SVSM GHCB events.
-		 */
-		if (snp_vmpl > 0 && !(hv_features & GHCB_HV_FT_SNP_MULTI_VMPL))
-			sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
-	}
+	snp_check_hv_features();
 
 	if (snp && !(sev_status & MSR_AMD64_SEV_SNP_ENABLED))
 		error("SEV-SNP supported indicated by CC blob, but not SEV status MSR.");
diff --git a/arch/x86/boot/startup/sev-shared.c b/arch/x86/boot/startup/sev-shared.c
index 1f2c4feeafce..a1d27a418421 100644
--- a/arch/x86/boot/startup/sev-shared.c
+++ b/arch/x86/boot/startup/sev-shared.c
@@ -66,10 +66,7 @@ sev_es_terminate(unsigned int set, unsigned int reason)
 		asm volatile("hlt\n" : : : "memory");
 }
 
-/*
- * The hypervisor features are available from GHCB version 2 onward.
- */
-u64 get_hv_features(void)
+static u64 __head get_hv_features(void)
 {
 	u64 val;
 
@@ -86,6 +83,31 @@ u64 get_hv_features(void)
 	return GHCB_MSR_HV_FT_RESP_VAL(val);
 }
 
+u64 __head snp_check_hv_features(void)
+{
+	/*
+	 * SNP is supported in v2 of the GHCB spec which mandates support for HV
+	 * features.
+	 */
+	if (sev_status & MSR_AMD64_SEV_SNP_ENABLED) {
+		u64 hv_features;
+
+		hv_features = get_hv_features();
+		if (!(hv_features & GHCB_HV_FT_SNP))
+			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
+
+		/*
+		 * Running at VMPL0 is required unless an SVSM is present and
+		 * the hypervisor supports the required SVSM GHCB events.
+		 */
+		if (snp_vmpl > 0 && !(hv_features & GHCB_HV_FT_SNP_MULTI_VMPL))
+			sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
+
+		return hv_features;
+	}
+	return 0;
+}
+
 int svsm_process_result_codes(struct svsm_call *call)
 {
 	switch (call->rax_out) {
diff --git a/arch/x86/boot/startup/sme.c b/arch/x86/boot/startup/sme.c
index 70ea1748c0a7..529090e20d2a 100644
--- a/arch/x86/boot/startup/sme.c
+++ b/arch/x86/boot/startup/sme.c
@@ -533,6 +533,8 @@ void __head sme_enable(struct boot_params *bp)
 	if (snp_en ^ !!(msr & MSR_AMD64_SEV_SNP_ENABLED))
 		snp_abort();
 
+	sev_hv_features = snp_check_hv_features();
+
 	/* Check if memory encryption is enabled */
 	if (feature_mask == AMD_SME_BIT) {
 		if (!(bp->hdr.xloadflags & XLF_MEM_ENCRYPTION))
diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 4fe0928bc0ad..f73dea313f55 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -1344,17 +1344,6 @@ void __init sev_es_init_vc_handling(void)
 	if (!sev_es_check_cpu_features())
 		panic("SEV-ES CPU Features missing");
 
-	/*
-	 * SNP is supported in v2 of the GHCB spec which mandates support for HV
-	 * features.
-	 */
-	if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP)) {
-		sev_hv_features = get_hv_features();
-
-		if (!(sev_hv_features & GHCB_HV_FT_SNP))
-			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
-	}
-
 	/* Initialize per-cpu GHCB pages */
 	for_each_possible_cpu(cpu) {
 		alloc_runtime_data(cpu);
diff --git a/arch/x86/include/asm/sev-internal.h b/arch/x86/include/asm/sev-internal.h
index bdfe008120f3..f0bfa46f3407 100644
--- a/arch/x86/include/asm/sev-internal.h
+++ b/arch/x86/include/asm/sev-internal.h
@@ -81,6 +81,6 @@ static __always_inline void sev_es_wr_ghcb_msr(u64 val)
 	native_wrmsr(MSR_AMD64_SEV_ES_GHCB, low, high);
 }
 
-u64 get_hv_features(void);
+void check_hv_features(void);
 
 const struct snp_cpuid_table *snp_cpuid_get_table(void);
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index f3acbfcdca9a..f20860187fe9 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -438,6 +438,7 @@ struct svsm_call {
 #ifdef CONFIG_AMD_MEM_ENCRYPT
 
 extern u8 snp_vmpl;
+extern u64 sev_hv_features;
 
 extern void __sev_es_ist_enter(struct pt_regs *regs);
 extern void __sev_es_ist_exit(void);
@@ -513,6 +514,7 @@ void snp_set_memory_private(unsigned long vaddr, unsigned long npages);
 void snp_set_wakeup_secondary_cpu(void);
 bool snp_init(struct boot_params *bp);
 void __noreturn snp_abort(void);
+u64 snp_check_hv_features(void);
 void snp_dmi_setup(void);
 int snp_issue_svsm_attest_req(u64 call_id, struct svsm_call *call, struct svsm_attest_call *input);
 void snp_accept_memory(phys_addr_t start, phys_addr_t end);
-- 
2.50.0.727.gbf7dc18ff4-goog

Ard Biesheuvel <ardb+git@google.com> writes:

> From: Ard Biesheuvel <ardb@kernel.org>

...

> So move the HV feature check into a helper function and call that
> instead. For the core kernel, move the check to an earlier boot stage,
> right after the point where it is established that the guest is
> executing in SEV-SNP mode.

This change is causing the SNP guest to fail ...

>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---

>  arch/x86/boot/startup/sme.c         |  2 ++
>  arch/x86/coco/sev/core.c            | 11 -------

> diff --git a/arch/x86/boot/startup/sme.c b/arch/x86/boot/startup/sme.c
> index 70ea1748c0a7..529090e20d2a 100644
> --- a/arch/x86/boot/startup/sme.c
> +++ b/arch/x86/boot/startup/sme.c
> @@ -533,6 +533,8 @@ void __head sme_enable(struct boot_params *bp)
>  	if (snp_en ^ !!(msr & MSR_AMD64_SEV_SNP_ENABLED))
>  		snp_abort();
>  
> +	sev_hv_features = snp_check _hv_features();
> +

...
snp_check_hv_features()
 -> get_hv_features() fails as ghcb_version is not yet initalized

ghcb_version is initialized as part of sev_es_negotiate_protocol(),
atleast this function needs to be called before get_hv_features() is
called.

>  	/* Check if memory encryption is enabled */
>  	if (feature_mask == AMD_SME_BIT) {
>  		if (!(bp->hdr.xloadflags & XLF_MEM_ENCRYPTION))
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index 4fe0928bc0ad..f73dea313f55 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -1344,17 +1344,6 @@ void __init sev_es_init_vc_handling(void)
>  	if (!sev_es_check_cpu_features())
>  		panic("SEV-ES CPU Features missing");
>  
> -	/*
> -	 * SNP is supported in v2 of the GHCB spec which mandates support for HV
> -	 * features.
> -	 */
> -	if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP)) {
> -		sev_hv_features = get_hv_features();
> -
> -		if (!(sev_hv_features & GHCB_HV_FT_SNP))
> -			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
> -	}
> -

With the below change, SNP-guest boots fine. I will wait for Tom's
confirmation if this is enough.

diff --git a/arch/x86/boot/startup/exports.h b/arch/x86/boot/startup/exports.h
index 01d2363dc445..dc7ca6f14cfe 100644
--- a/arch/x86/boot/startup/exports.h
+++ b/arch/x86/boot/startup/exports.h
@@ -12,3 +12,4 @@ PROVIDE(snp_cpuid			= __pi_snp_cpuid);
 PROVIDE(snp_cpuid_get_table		= __pi_snp_cpuid_get_table);
 PROVIDE(svsm_issue_call			= __pi_svsm_issue_call);
 PROVIDE(svsm_process_result_codes	= __pi_svsm_process_result_codes);
+PROVIDE(sev_es_negotiate_protocol      = __pi_sev_es_negotiate_protocol);
diff --git a/arch/x86/boot/startup/sev-shared.c b/arch/x86/boot/startup/sev-shared.c
index ce11bab57d4f..ff6f3372cbb4 100644
--- a/arch/x86/boot/startup/sev-shared.c
+++ b/arch/x86/boot/startup/sev-shared.c
@@ -740,3 +740,24 @@ static bool __init svsm_setup_ca(const struct cc_blob_sev_info *cc_info,
 
 	return true;
 }
+
+bool sev_es_negotiate_protocol(void)
+{
+        u64 val;
+
+        /* Do the GHCB protocol version negotiation */
+        sev_es_wr_ghcb_msr(GHCB_MSR_SEV_INFO_REQ);
+        VMGEXIT();
+        val = sev_es_rd_ghcb_msr();
+
+        if (GHCB_MSR_INFO(val) != GHCB_MSR_SEV_INFO_RESP)
+                return false;
+
+        if (GHCB_MSR_PROTO_MAX(val) < GHCB_PROTOCOL_MIN ||
+            GHCB_MSR_PROTO_MIN(val) > GHCB_PROTOCOL_MAX)
+                return false;
+
+        ghcb_version = min_t(size_t, GHCB_MSR_PROTO_MAX(val), GHCB_PROTOCOL_MAX);
+
+        return true;
+}
diff --git a/arch/x86/boot/startup/sme.c b/arch/x86/boot/startup/sme.c
index c3eff6d5102c..a7f8ee64e211 100644
--- a/arch/x86/boot/startup/sme.c
+++ b/arch/x86/boot/startup/sme.c
@@ -533,6 +533,9 @@ void __init sme_enable(struct boot_params *bp)
 	if (snp_en ^ !!(msr & MSR_AMD64_SEV_SNP_ENABLED))
 		snp_abort();
 
+	if (!sev_es_negotiate_protocol())
+		sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SEV_ES_GEN_REQ);
+
 	sev_hv_features = snp_check_hv_features();
 
 	/* Check if memory encryption is enabled */
diff --git a/arch/x86/coco/sev/vc-shared.c b/arch/x86/coco/sev/vc-shared.c
index 3d44474f46e7..af2d0fae2e18 100644
--- a/arch/x86/coco/sev/vc-shared.c
+++ b/arch/x86/coco/sev/vc-shared.c
@@ -622,24 +622,3 @@ bool __init sev_es_check_cpu_features(void)
 
 	return true;
 }
-
-bool sev_es_negotiate_protocol(void)
-{
-	u64 val;
-
-	/* Do the GHCB protocol version negotiation */
-	sev_es_wr_ghcb_msr(GHCB_MSR_SEV_INFO_REQ);
-	VMGEXIT();
-	val = sev_es_rd_ghcb_msr();
-
-	if (GHCB_MSR_INFO(val) != GHCB_MSR_SEV_INFO_RESP)
-		return false;
-
-	if (GHCB_MSR_PROTO_MAX(val) < GHCB_PROTOCOL_MIN ||
-	    GHCB_MSR_PROTO_MIN(val) > GHCB_PROTOCOL_MAX)
-		return false;
-
-	ghcb_version = min_t(size_t, GHCB_MSR_PROTO_MAX(val), GHCB_PROTOCOL_MAX);
-
-	return true;
-}

Regards
Nikunj

On Thu, 10 Jul 2025 at 14:21, Nikunj A Dadhania <nikunj@amd.com> wrote:
>
> Ard Biesheuvel <ardb+git@google.com> writes:
>
> > From: Ard Biesheuvel <ardb@kernel.org>
>
> ...
>
> > So move the HV feature check into a helper function and call that
> > instead. For the core kernel, move the check to an earlier boot stage,
> > right after the point where it is established that the guest is
> > executing in SEV-SNP mode.
>
> This change is causing the SNP guest to fail ...
>
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
>
> >  arch/x86/boot/startup/sme.c         |  2 ++
> >  arch/x86/coco/sev/core.c            | 11 -------
>
> > diff --git a/arch/x86/boot/startup/sme.c b/arch/x86/boot/startup/sme.c
> > index 70ea1748c0a7..529090e20d2a 100644
> > --- a/arch/x86/boot/startup/sme.c
> > +++ b/arch/x86/boot/startup/sme.c
> > @@ -533,6 +533,8 @@ void __head sme_enable(struct boot_params *bp)
> >       if (snp_en ^ !!(msr & MSR_AMD64_SEV_SNP_ENABLED))
> >               snp_abort();
> >
> > +     sev_hv_features = snp_check _hv_features();
> > +
>
> ...
> snp_check_hv_features()
>  -> get_hv_features() fails as ghcb_version is not yet initalized
>
> ghcb_version is initialized as part of sev_es_negotiate_protocol(),
> atleast this function needs to be called before get_hv_features() is
> called.
>

Thanks for the diagnosis.

I added back the ghcb_version check, even though it is redundant,
given that SNP support implies ghcb version >= 2

Would the below change be sufficient too?

--- a/arch/x86/boot/startup/sev-shared.c
+++ b/arch/x86/boot/startup/sev-shared.c
@@ -49,9 +49,6 @@ static u64 __init get_hv_features(void)
 {
        u64 val;

-       if (ghcb_version < 2)
-               return 0;
-
        sev_es_wr_ghcb_msr(GHCB_MSR_HV_FT_REQ);
        VMGEXIT();