Re: [PATCH v4 12/24] x86/sev: Unify SEV-SNP hypervisor feature check

[Nikunj A Dadhania <nikunj@amd.com>]

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