Re: [PATCH v11 2/6] x86/sev: Disable CPU hotplug while SNP is active

[Borislav Petkov]

On Mon, Jul 27, 2026 at 07:04:31PM +0000, Ashish Kalra wrote:
> With CPU hotplug now disabled while SNP is active, the online CPU mask is
> stable, so the cpus_read_lock() previously taken in snp_prepare() to
> iterate it is redundant.  Drop cpus_read_lock()/cpus_read_unlock() here.
> The RMPOPT setup and cleanup added later are introduced after this patch
> and never take the lock for the same reason.

Why do I even bother writing it?

"do ... not talk about future patches because git history is not always
linear"

> Suggested-by: Thomas Lendacky <thomas.lendacky@xxxxxxx>
> Suggested-by: Borislav Petkov (AMD) <bp@xxxxxxxxx>
> Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> ---
>  arch/x86/virt/svm/sev.c | 39 +++++++++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index cff285d8ad8e..e2f69fba0938 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
> @@ -513,7 +513,6 @@ static void clear_hsave_pa(void *arg)
>  
>  int snp_prepare(void)
>  {
> -	int ret;
>  	u64 val;
>  
>  	/*
> @@ -526,14 +525,21 @@ int snp_prepare(void)
>  
>  	clear_rmp();
>  
> -	cpus_read_lock();
> +	/*
> +	 * Disable CPU hotplug before enabling SNP: no CPU may come online
> +	 * without SnpEn while SNP is active, and none may go offline during
> +	 * enable.  This keeps cpu_online_mask stable for the check and the
> +	 * on_each_cpu() calls below, so cpus_read_lock() is not needed.  It is
> +	 * re-enabled in snp_shutdown() once the firmware disables SNP.
> +	 */
> +	cpu_hotplug_disable();

No need for too much splainin' and besides, that comment'll grow out-of-whack
sooner than you think:

diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index e2f69fba0938..731ea25fba37 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -528,9 +528,7 @@ int snp_prepare(void)
        /*
         * Disable CPU hotplug before enabling SNP: no CPU may come online
         * without SnpEn while SNP is active, and none may go offline during
-        * enable.  This keeps cpu_online_mask stable for the check and the
-        * on_each_cpu() calls below, so cpus_read_lock() is not needed.  It is
-        * re-enabled in snp_shutdown() once the firmware disables SNP.
+        * enable.
         */
        cpu_hotplug_disable();
 
>  	if (!cpumask_equal(cpu_online_mask, cpu_present_mask)) {
> -		ret = -EOPNOTSUPP;
> +		cpu_hotplug_enable();
>  		pr_warn("SNP init failed: not all CPUs online. (%*pbl online <-> %*pbl present masks).\n",
>  			cpumask_pr_args(cpu_online_mask),
>  			cpumask_pr_args(cpu_present_mask));
> -		goto unlock;
> +		return -EOPNOTSUPP;
>  	}
>  
>  	wbinvd_on_all_cpus();
> @@ -548,12 +554,7 @@ int 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;
> +	return 0;
>  }
>  EXPORT_SYMBOL_FOR_MODULES(snp_prepare, "ccp");
>  
> @@ -565,6 +566,13 @@ void snp_shutdown(void)
>  	if (syscfg & MSR_AMD64_SYSCFG_SNP_EN)
>  		return;
>  
> +	/*
> +	 * The firmware has disabled SNP (SnpEn is clear), so re-enable CPU
> +	 * hotplug.  A legacy SNP shutdown returns above with SnpEn still set and
> +	 * leaves hotplug disabled.
> +	 */
> +	cpu_hotplug_enable();

What happens if CPUs get offlined here after hotplug has been enabled and...

>  	clear_rmp();
>  	on_each_cpu(mfd_reconfigure, NULL, 1);

... they miss the mfd_reconfigure()?

>  }

-- 
Regards/Gruss,
    Boris.

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