Re: [PATCH v4 15/15] x86/sev: Allow non-VMPL0 execution when an SVSM is present

[Borislav Petkov]

On Wed, Apr 24, 2024 at 10:58:11AM -0500, Tom Lendacky wrote:
> @@ -624,8 +626,12 @@ void sev_enable(struct boot_params *bp)
>  		 * modifies permission bits, it is still ok to do so currently because Linux
>  		 * SNP guests running at VMPL0 only run at VMPL0, so VMPL1 or higher
>  		 * permission mask changes are a don't-care.
> +		 *
> +		 * Running at VMPL0 is not required if an SVSM is present and the hypervisor
> +		 * supports the required SVSM GHCB events.
>  		 */
> -		if (rmpadjust((unsigned long)&boot_ghcb_page, RMP_PG_SIZE_4K, 1))
> +		if (rmpadjust((unsigned long)&boot_ghcb_page, RMP_PG_SIZE_4K, 1) &&
> +		    !(vmpl && (hv_features & GHCB_HV_FT_SNP_MULTI_VMPL)))
>  			sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
>  	}

Let's make that more readable:

diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index fb1e60165cd1..157f749faba0 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -610,8 +610,10 @@ void sev_enable(struct boot_params *bp)
 	 * features.
 	 */
 	if (sev_status & MSR_AMD64_SEV_SNP_ENABLED) {
-		u64 hv_features = get_hv_features();
+		u64 hv_features;
+		int rmpadj_ret;
 
+		hv_features = get_hv_features();
 		if (!(hv_features & GHCB_HV_FT_SNP))
 			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
 
@@ -626,11 +628,15 @@ void sev_enable(struct boot_params *bp)
 		 * modifies permission bits, it is still ok to do so currently because Linux
 		 * SNP guests running at VMPL0 only run at VMPL0, so VMPL1 or higher
 		 * permission mask changes are a don't-care.
-		 *
+		 */
+		rmpadj_ret = rmpadjust((unsigned long)&boot_ghcb_page, RMP_PG_SIZE_4K, 1);
+
+		/*
 		 * Running at VMPL0 is not required if an SVSM is present and the hypervisor
 		 * supports the required SVSM GHCB events.
 		 */
-		if (rmpadjust((unsigned long)&boot_ghcb_page, RMP_PG_SIZE_4K, 1) &&
+
+		if (rmpadj_ret &&
 		    !(vmpl && (hv_features & GHCB_HV_FT_SNP_MULTI_VMPL)))
 			sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_NOT_VMPL0);
 	}

> -static int __init report_cpuid_table(void)
> +static void __init report_cpuid_table(void)
>  {
>  	const struct snp_cpuid_table *cpuid_table = snp_cpuid_get_table();
>  
>  	if (!cpuid_table->count)
> -		return 0;
> +		return;
>  
>  	pr_info("Using SNP CPUID table, %d entries present.\n",
>  		cpuid_table->count);
>  
>  	if (sev_cfg.debug)
>  		dump_cpuid_table();
> +}
> +
> +static void __init report_vmpl_level(void)
> +{
> +	if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
> +		return;
> +
> +	pr_info("SNP running at VMPL%u.\n", vmpl);
> +}
> +
> +static int __init report_snp_info(void)
> +{
> +	report_vmpl_level();
> +	report_cpuid_table();
>  
>  	return 0;
>  }
> -arch_initcall(report_cpuid_table);
> +arch_initcall(report_snp_info);

Zap one more silly helper:

diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 7955c024d5d7..ff5a32b0b21c 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -2356,32 +2356,23 @@ static void dump_cpuid_table(void)
  * sort of indicator, and there's not really any other good place to do it,
  * so do it here.
  */
-static void __init report_cpuid_table(void)
+static int __init report_snp_info(void)
 {
 	const struct snp_cpuid_table *cpuid_table = snp_cpuid_get_table();
 
 	if (!cpuid_table->count)
-		return;
+		return 0;
 
 	pr_info("Using SNP CPUID table, %d entries present.\n",
 		cpuid_table->count);
 
 	if (sev_cfg.debug)
 		dump_cpuid_table();
-}
 
-static void __init report_vmpl_level(void)
-{
 	if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
-		return;
+		return 0;
 
 	pr_info("SNP running at VMPL%u.\n", vmpl);
-}
-
-static int __init report_snp_info(void)
-{
-	report_vmpl_level();
-	report_cpuid_table();
 
 	return 0;
 }

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette