Re: [PATCH v4 1/2] x86/sev: Clear RMPOPT when segmented RMP is disabled by firmware

[Tom Lendacky]

On 11/2/26 13:31, Borislav Petkov wrote:
> On Mon, Nov 02, 2026 at 09:50:32AM -0600, Kalra, Ashish wrote:
>> The original patch returns false if segmented RMP is enabled but
>> probe_segmented_rmptable_info() fails, your version falls through to
>> probe_contiguous_rmptable_info() instead. 
>>
>> I believe it makes sense to keep the return false there: on a
>> segmented RMP enabled platform the RMP is segmented, not contiguous, so the
>> contiguous probe shouldn't run.
> 
> That simplifies the code even more:
> 
> bool snp_probe_rmptable_info(void)
> {
>         if (!cpu_feature_enabled(X86_FEATURE_SEGMENTED_RMP))
>                 goto out;
> 
>         rdmsrq(MSR_AMD64_RMP_CFG, rmp_cfg);
>         if (rmp_cfg & MSR_AMD64_SEG_RMP_ENABLED)
>                 return probe_segmented_rmptable_info();
> 
> out:
>         setup_clear_cpu_cap(X86_FEATURE_RMPOPT);
>         return probe_contiguous_rmptable_info();

The current code clears X86_FEATURE_RMPOPT when
probe_segmented_rmptable_info() fails. The above code doesn't. But since
SNP will be disabled on failure of snp_probe_rmptable_info(), I guess it
doesn't matter. In which case you can eliminate the goto by doing:

	if (cpu_feature_enabled(X86_FEATURE_SEGMENTED_RMP)) {
		rdmsrq(MSR_AMD64_RMP_CFG, rmp_cfg);
		if (rmp_cfg & MSR_AMD64_SEG_RMP_ENABLED)
			return probe_segmented_rmptable_info();
	}

	setup_clear_cpu_cap(X86_FEATURE_RMPOPT);
	return probe_contiguous_rmptable_info();

Which looks pretty clean.

Thanks,
Tom
		> }
> 
> Along with the below, which should have happened too:
> 
> 1. RMPOPT depends on SEV_SNP, ofc. This would make sure RMPOPT is cleared
>    everytime SNP is cleared, as it should be.
> 
> 2. snp_enable_rmpopt() needs to check SNP first, ofc, and then RMPOPT.
> 
> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
> index 99801e844b30..acbcec394ef1 100644
> --- a/arch/x86/kernel/cpu/cpuid-deps.c
> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> @@ -93,6 +93,7 @@ static const struct cpuid_dep cpuid_deps[] = {
>  	{ X86_FEATURE_SPEC_CTRL_SSBD,		X86_FEATURE_SPEC_CTRL },
>  	{ X86_FEATURE_LASS,			X86_FEATURE_SMAP      },
>  	{ X86_FEATURE_INVLPGB,			X86_FEATURE_PCID      },
> +	{ X86_FEATURE_RMPOPT,			X86_FEATURE_SEV_SNP   },
>  	{}
>  };
>  
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index ff6d17795054..a47b812b2ea3 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
> @@ -669,10 +669,10 @@ void snp_enable_rmpopt(void)
>  	u64 base;
>  	int cpu;
>  
> -	if (!cpu_feature_enabled(X86_FEATURE_RMPOPT))
> +	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
>  		return;
>  
> -	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
> +	if (!cpu_feature_enabled(X86_FEATURE_RMPOPT))
>  		return;
>  
>  	if (!rmpopt_soft_init)
> 
> ---
> 
> here's the full diff. Have a look pls and let me know what else I am missing:
> 
> ---
> 
> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
> index 99801e844b30..acbcec394ef1 100644
> --- a/arch/x86/kernel/cpu/cpuid-deps.c
> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> @@ -93,6 +93,7 @@ static const struct cpuid_dep cpuid_deps[] = {
>  	{ X86_FEATURE_SPEC_CTRL_SSBD,		X86_FEATURE_SPEC_CTRL },
>  	{ X86_FEATURE_LASS,			X86_FEATURE_SMAP      },
>  	{ X86_FEATURE_INVLPGB,			X86_FEATURE_PCID      },
> +	{ X86_FEATURE_RMPOPT,			X86_FEATURE_SEV_SNP   },
>  	{}
>  };
>  
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index bd70c4d0b774..a47b812b2ea3 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
> @@ -669,10 +669,10 @@ void snp_enable_rmpopt(void)
>  	u64 base;
>  	int cpu;
>  
> -	if (!cpu_feature_enabled(X86_FEATURE_RMPOPT))
> +	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
>  		return;
>  
> -	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
> +	if (!cpu_feature_enabled(X86_FEATURE_RMPOPT))
>  		return;
>  
>  	if (!rmpopt_soft_init)
> @@ -817,20 +817,15 @@ static bool probe_segmented_rmptable_info(void)
>  
>  bool snp_probe_rmptable_info(void)
>  {
> -	if (cpu_feature_enabled(X86_FEATURE_SEGMENTED_RMP)) {
> -		rdmsrq(MSR_AMD64_RMP_CFG, rmp_cfg);
> -
> -		if (rmp_cfg & MSR_AMD64_SEG_RMP_ENABLED) {
> -			if (probe_segmented_rmptable_info())
> -				return true;
> +	if (!cpu_feature_enabled(X86_FEATURE_SEGMENTED_RMP))
> +		goto out;
>  
> -			setup_clear_cpu_cap(X86_FEATURE_RMPOPT);
> -			return false;
> -		}
> -	} else {
> -		setup_clear_cpu_cap(X86_FEATURE_RMPOPT);
> -	}
> +	rdmsrq(MSR_AMD64_RMP_CFG, rmp_cfg);
> +	if (rmp_cfg & MSR_AMD64_SEG_RMP_ENABLED)
> +		return probe_segmented_rmptable_info();
>  
> +out:
> +	setup_clear_cpu_cap(X86_FEATURE_RMPOPT);
>  	return probe_contiguous_rmptable_info();
>  }
> 
> ---
> 
> Thx.
>