Re: [PATCH v2 1/2] x86/sev: Do not initialize SNP if missing CPUs

[Tom Lendacky <thomas.lendacky@amd.com>]

On 4/7/26 12:47, Tycho Andersen wrote:
> From: "Tycho Andersen (AMD)" <tycho@kernel.org>
> 
> The SEV firmware checks that the SNP enable bit is set on each CPU during
> SNP initialization, and will fail if it is not. If there are some CPUs
> offline, they will not run the setup functions, so SNP initialization will
> always fail.
> 
> Skip the IPIs in this case and return an error so that the CCP driver can
> skip the SNP_INIT that will fail. Also print the CPU masks as a breadcrumb
> so people can figure out what happened.
> 
> Suggested-by: Borislav Petkov (AMD) <bp@alien8.de>
> Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
> ---
>  arch/x86/include/asm/sev.h |  4 ++--
>  arch/x86/virt/svm/sev.c    | 15 +++++++++++++--
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 09e605c85de4..594cfa19cbd4 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -661,7 +661,7 @@ static inline void snp_leak_pages(u64 pfn, unsigned int pages)
>  {
>  	__snp_leak_pages(pfn, pages, true);
>  }
> -void snp_prepare(void);
> +int snp_prepare(void);
>  void snp_shutdown(void);
>  #else
>  static inline bool snp_probe_rmptable_info(void) { return false; }
> @@ -679,7 +679,7 @@ static inline void __snp_leak_pages(u64 pfn, unsigned int npages, bool dump_rmp)
>  static inline void snp_leak_pages(u64 pfn, unsigned int npages) {}
>  static inline void kdump_sev_callback(void) { }
>  static inline void snp_fixup_e820_tables(void) {}
> -static inline void snp_prepare(void) {}
> +static inline int snp_prepare(void) { return -ENODEV; }
>  static inline void snp_shutdown(void) {}
>  #endif
>  
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index 41f76f15caa1..160e60f5f3fb 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
> @@ -511,8 +511,9 @@ static void clear_hsave_pa(void *arg)
>  	wrmsrq(MSR_VM_HSAVE_PA, 0);
>  }
>  
> -void snp_prepare(void)
> +int snp_prepare(void)
>  {
> +	int ret = -EOPNOTSUPP;

I prefer seeing the return value set in the error condition, but that's
just my preference.

>  	u64 val;
>  
>  	/*
> @@ -521,12 +522,19 @@ void snp_prepare(void)
>  	 */
>  	rdmsrq(MSR_AMD64_SYSCFG, val);
>  	if (val & MSR_AMD64_SYSCFG_SNP_EN)
> -		return;
> +		return 0;
>  
>  	clear_rmp();
>  
>  	cpus_read_lock();
>  
> +	if (!cpumask_equal(cpu_online_mask, cpu_present_mask)) {
> +		pr_warn("Skipping SNP initialization. CPUs online %*pbl, present %*pbl\n",
> +			cpumask_pr_args(cpu_online_mask),
> +			cpumask_pr_args(cpu_present_mask));
> +		goto unlock;
> +	}
> +
>  	/*
>  	 * MtrrFixDramModEn is not shared between threads on a core,
>  	 * therefore it must be set on all CPUs prior to enabling SNP.
> @@ -537,7 +545,10 @@ void snp_prepare(void)
>  	/* SNP_INIT requires MSR_VM_HSAVE_PA to be cleared on all CPUs. */
>  	on_each_cpu(clear_hsave_pa, NULL, 1);
>  
> +	ret = 0;

I would also prefer a blank line here...

> +unlock:
>  	cpus_read_unlock();

and here. But, again, that's just me.

If you don't need another version, that's not enough to redo it.

Reviewed-by: Tom Lendacky <thomas.lendacky@gmail.com>

> +	return ret;
>  }
>  EXPORT_SYMBOL_FOR_MODULES(snp_prepare, "ccp");
>