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

[Tom Lendacky]

On 4/1/26 09:35, Tycho Andersen wrote:
> From: "Tycho Andersen (AMD)" <tycho@xxxxxxxxxx>
> 
> 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.
> 
> Suggested-by: Borislav Petkov (AMD) <bp@xxxxxxxxx>
> Signed-off-by: Tycho Andersen (AMD) <tycho@xxxxxxxxxx>
> ---
>  arch/x86/include/asm/sev.h |  4 ++--
>  arch/x86/virt/svm/sev.c    | 11 +++++++++--
>  2 files changed, 11 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..e9ded15dbe60 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;
>  	u64 val;
>  
>  	/*
> @@ -521,12 +522,15 @@ 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_possible_mask))

If CONFIG_INIT_ALL_POSSIBLE is set, won't that set cpu_possible_mask to
include all CPUs up to NR_CPUS? That would result in this always failing.

Not sure if this change is worth it.

Thanks,
Tom

> +		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 +541,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;
> +unlock:
>  	cpus_read_unlock();
> +	return ret;
>  }
>  EXPORT_SYMBOL_FOR_MODULES(snp_prepare, "ccp");
>  
> 
> base-commit: cf112712c193e837225d740ec3e139774f2496f2