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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] x86/sev: PM suspend bypasses cpu_hotplug_disable() leading to missing SnpEn
- [Medium] x86/sev: early return skips cpu_hotplug_disable() when SnpEn is already set
--

commit b1980431b3abd35a5fe5e83fed1107ef1f0ec745
Author: Ashish Kalra <ashish.kalra@amd.com>

x86/sev: Disable CPU hotplug while SNP is active

This patch ties CPU hotplug to the SNP-enable bit to prevent CPUs from
coming online without SNP enabled and initialized. It replaces the
cpus_read_lock() previously taken in snp_prepare() with
cpu_hotplug_disable() and reenables it in snp_shutdown(). Additionally,
it disables CPU hotplug in snp_rmptable_init() for kexec boots where
SNP is already active.

> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index cff285d8ad8e3..558f7924a3f85 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c

[ ... ]

> @@ -526,14 +525,18 @@ int snp_prepare(void)
>  
>  	clear_rmp();
>  
> -	cpus_read_lock();
> +	/*
> +	 * No CPU may come online without SnpEn while SNP is active; disable
> +	 * hotplug here and re-enable it in snp_shutdown().
> +	 */
> +	cpu_hotplug_disable();
>  
>  	if (!cpumask_equal(cpu_online_mask, cpu_present_mask)) {

[Severity: High]
Does this fully prevent secondary CPUs from coming online without SnpEn set
during power management suspend and resume?

Looking at the PM suspend path, freeze_secondary_cpus() calls _cpu_down(),
and thaw_secondary_cpus() calls _cpu_up().

Both of these internal functions bypass the cpu_hotplug_disabled counter
that is incremented by cpu_hotplug_disable().

Could a PM suspend/resume cycle successfully offline and online secondary
CPUs, causing them to resume without SnpEn set and bypassing hardware RMP
checks?

[ ... ]

> @@ -577,6 +582,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 +593,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();

[Severity: Medium]
Could this leave CPU hotplug enabled if an early initialization step fails
during a kexec boot where legacy firmware leaves SnpEn set?

If setup_rmptable() fails and returns false, the function returns -ENOSYS
early.

Because the check for MSR_AMD64_SYSCFG_SNP_EN and the call to
cpu_hotplug_disable() happen after these early returns, CPU hotplug would
remain enabled.

Newly onlined CPUs might lack SnpEn as a result, creating a mixed hardware
state. Should the hotplug disablement happen before these early returns?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789586444.git.ashish.kalra@amd.com?part=2