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

On 7/28/2026 9:15 PM, Borislav Petkov wrote:
> 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"

Will drop the future patch reference from the commit log.

> 
>> Suggested-by: Thomas Lendacky <[email protected]>
>> Suggested-by: Borislav Petkov (AMD) <[email protected]>
>> Signed-off-by: Ashish Kalra <[email protected]>
>> ---
>>  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:
> 

Will trim the comment.

> 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()?
> 
>>  }
> 

Yes, re-enable should happens last in snp_shutdown() (after clear_rmp()/mfd_reconfigure()).

Thanks,
Ashish