Re: [PATCH v16 3/5] x86/sev: Initialize RMPOPT configuration MSRs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] x86/sev: missing MSR configuration on CPU hotplug cycle due to local check
--

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

x86/sev: Initialize RMPOPT configuration MSRs

The new RMPOPT instruction helps manage per-CPU RMP optimization
structures inside the CPU. Initialize the per-CPU RMPOPT table base to
the starting physical address to enable RMP optimization for up to 2 TB
of system RAM on all CPUs.

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

[ ... ]

> @@ -575,6 +577,42 @@ void snp_shutdown(void)
>  }
>  EXPORT_SYMBOL_FOR_MODULES(snp_shutdown, "ccp");
>  
> +static bool rmpopt_capable(void)
> +{
> +	return cpu_feature_enabled(X86_FEATURE_RMPOPT) &&
> +	       cc_platform_has(CC_ATTR_HOST_SEV_SNP);
> +}
> +
> +void snp_enable_rmpopt(void)
> +{
> +	u64 base;
> +	int cpu;
> +
> +	if (!rmpopt_capable())
> +		return;
> +
> +	rmpopt_pa_start = ALIGN_DOWN(PFN_PHYS(min_low_pfn), SZ_1G);
> +
> +	/*
> +	 * Per-CPU RMPOPT tables cover at most 2 TB.  Program each core's
> +	 * RMPOPT_BASE with the start of RAM to optimize up to 2 TB.  The MSR
> +	 * can only be written after SNP is enabled, so this runs from the ccp
> +	 * SNP init path (and again on guest teardown) rather than an initcall.
> +	 *
> +	 * The loop below programs RMPOPT_BASE on all primary threads.  RMPOPT_EN
> +	 * cannot be cleared while SNP is enabled, and CPU hotplug is disabled
> +	 * while SNP is active, so once programmed the MSRs stay set on all CPUs
> +	 * until SNP is disabled.  A set RMPOPT_EN on the local CPU therefore
> +	 * means the programming has already been done, so skip it.
> +	 */
> +	rdmsrq(MSR_AMD64_RMPOPT_BASE, base);
> +	if (!(base & MSR_AMD64_RMPOPT_ENABLE))
> +		for_each_cpu(cpu, cpu_primary_thread_mask)
> +			wrmsrq_on_cpu(cpu, MSR_AMD64_RMPOPT_BASE,
> +				      rmpopt_pa_start | MSR_AMD64_RMPOPT_ENABLE);

[Severity: High]
Could this local CPU check cause hotplugged CPUs to miss MSR configuration?

If SNP is shut down, CPU hotplug is re-enabled but the MSR remains intact on
existing CPUs. If a CPU is offlined and brought back online, its MSR is reset
to the default. 

Later, when SNP is re-enabled, the local CPU executing snp_enable_rmpopt()
might still have its own RMPOPT_ENABLE bit set. Will it erroneously assume
all CPUs are programmed and skip the wrmsrq_on_cpu() loop, leaving the
hotplugged CPU unconfigured?

> +}
> +EXPORT_SYMBOL_FOR_MODULES(snp_enable_rmpopt, "ccp");
> +

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