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

On 7/27/26 14:04, Ashish Kalra wrote:
> From: Ashish Kalra <[email protected]>
> 
> While SNP is active, every memory write is checked against the RMP to
> protect SEV-SNP guest memory.  A core performs these RMP checks only once
> SNP has been initialized via SNP_INIT and the SNP-enable bit in SYSCFG is
> set on that core; the firmware requires the SNP-enable bit to be set on
> every present CPU before SNP initialization.
> 
> A core that is not SNP-enabled and not SNP-initialized performs no RMP
> checks at all, so there is no valid configuration with SNP active and any
> CPU exempt from RMP checks.
> 
> The firmware determines which CPUs are present from the processor and the
> BIOS/UEFI configuration (e.g. SMT disabled in the BIOS) and enumerates
> them at SNP init; it is not aware of the OS bringing CPUs online or
> offline afterwards.
> 
> SNP_INIT fails unless SnpEn is set on all CPUs, so a CPU that is offline
> when SNP_INIT is issued, does not have SnpEn set, SNP_INIT fails, and
> there can be no SNP guest memory.  OS CPU hotplug can thus diverge from
> the firmware's expectations and break SNP.
> 
> Tie CPU hotplug to the SNP-enable bit: disable it in snp_prepare() before
> SNP is enabled, and re-enable it in snp_shutdown() once the firmware has
> disabled SNP.
> 
> If snp_prepare() fails before enabling SNP it re-enables hotplug itself;
> once SNP is enabled hotplug stays disabled, including across a failed
> SNP_INIT and across the legacy SNP_SHUTDOWN_EX path, both of which leave
> SNP enabled.
> 
> A kexec target that boots with SNP already enabled, disables hotplug once
> in snp_rmptable_init(), since snp_prepare() bails when SNP is already
> enabled.
> 
> 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.
> 
> 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();
>  
>  	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();
> +
>  	clear_rmp();
>  	on_each_cpu(mfd_reconfigure, NULL, 1);
>  }
> @@ -577,6 +585,8 @@ EXPORT_SYMBOL_FOR_MODULES(snp_shutdown, "ccp");
>   */
>  int __init snp_rmptable_init(void)
>  {
> +	u64 val;
> +
>  	if (WARN_ON_ONCE(!cc_platform_has(CC_ATTR_HOST_SEV_SNP)))
>  		return -ENOSYS;
>  
> @@ -586,6 +596,15 @@ int __init snp_rmptable_init(void)
>  	if (!setup_rmptable())
>  		return -ENOSYS;
>  
> +	/*
> +	 * On a kexec boot SNP may already be enabled (legacy firmware leaves
> +	 * SnpEn set across shutdown), in which case snp_prepare() bails without
> +	 * disabling CPU hotplug, so disable it here.
> +	 */
> +	rdmsrq(MSR_AMD64_SYSCFG, val);
> +	if (val & MSR_AMD64_SYSCFG_SNP_EN)
> +		cpu_hotplug_disable();
> +

Why not just put the cpu_hotplug_disable() at the start of snp_prepare()
then? Wouldn't that take care of both situations and only end up with a
single disable point?

Thanks,
Tom

>  	/*
>  	 * Setting crash_kexec_post_notifiers to 'true' to ensure that SNP panic
>  	 * notifier is invoked to do SNP IOMMU shutdown before kdump.