Re: [PATCH v6 2/6] 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] Late clearing of X86_FEATURE_RMPOPT via setup_clear_cpu_cap() fails to un-patch CPU alternatives, causing snp_setup_rmpopt() to dangerously enable RMPOPT on unsupported Contiguous RMP configurations.
--

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

x86/sev: Initialize RMPOPT configuration MSRs

This commit introduces RMPOPT instruction support to manage per-CPU RMP
optimization structures. It sets up the RMPOPT table base to enable
optimizations for up to 2TB of system RAM once SNP is enabled. It also
attempts to clear the RMPOPT CPU capability if Segmented RMP is disabled.

> diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
> index 989ca9f72ba30..7fdef00ca8f22 100644
> --- a/arch/x86/coco/core.c
> +++ b/arch/x86/coco/core.c
> @@ -172,6 +172,7 @@ static void amd_cc_platform_clear(enum cc_attr attr)
>  	switch (attr) {
>  	case CC_ATTR_HOST_SEV_SNP:
>  		cc_flags.host_sev_snp = 0;
> +		setup_clear_cpu_cap(X86_FEATURE_RMPOPT);
>  		break;
>  	default:
>  		break;

[ ... ]

> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index 8bcdce98f6dce..089c9a14edc74 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c

[ ... ]

> @@ -488,9 +491,13 @@ static bool __init setup_segmented_rmptable(void)
>  static bool __init setup_rmptable(void)
>  {
>  	if (rmp_cfg & MSR_AMD64_SEG_RMP_ENABLED) {
> -		if (!setup_segmented_rmptable())
> +		if (!setup_segmented_rmptable()) {
> +			setup_clear_cpu_cap(X86_FEATURE_RMPOPT);
>  			return false;
> +		}
>  	} else {
> +		/* Note that Segmented RMP must be enabled to enable RMPOPT. */
> +		setup_clear_cpu_cap(X86_FEATURE_RMPOPT);

[Severity: High]
Will calling setup_clear_cpu_cap() this late in the boot sequence effectively
disable the feature?

Since setup_rmptable() and amd_cc_platform_clear() are invoked during late
IOMMU initialization, this executes after check_bugs() has already permanently
patched the alternative instructions.

While do_clear_cpu_cap() will trigger a kernel WARNING and update the
in-memory boot_cpu_data array, it cannot undo the static code patching of
static_cpu_has().

>  		if (!setup_contiguous_rmptable())
>  			return false;
>  	}

[ ... ]

> +void snp_setup_rmpopt(void)
> +{
> +	u64 rmpopt_base;
> +	int cpu;
> +
> +	if (!cpu_feature_enabled(X86_FEATURE_RMPOPT))
> +		return;

[Severity: High]
If setup_clear_cpu_cap() failed to un-patch the alternatives earlier, will
cpu_feature_enabled() incorrectly return true here even when falling back to
Contiguous RMP?

Because cpu_feature_enabled() relies on static_cpu_has(), it seems it will
execute the previously patched instruction and erroneously bypass this check.

Would this result in snp_setup_rmpopt() wrongly configuring
MSR_AMD64_RMPOPT_BASE and dangerously enabling RMPOPT on unsupported
configurations, risking CPU exceptions or memory corruption?

Could a global boolean flag be used to track the RMPOPT state instead of
dynamically mutating a CPU feature after alternatives are patched?

> +
> +	cpus_read_lock();

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