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

[Kalra, Ashish]

On 11/2/2026 1:31 PM, 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();
> }
> 
> 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.
> 

Yes, that is a good point, but do note that the cpuid-dep only fires when the SEV_SNP CPUID bit is cleared though,
and the IOMMU "can't enable SNP" path clears only CC_ATTR_HOST_SEV_SNP, not the CPUID bit.

> 2. snp_enable_rmpopt() needs to check SNP first, ofc, and then RMPOPT.

Yes.

> 
> 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();
>  }
> 
> ---

Looks good, the simplification is nice. Two things worth noting:

1). With the leaner snp_probe_rmptable_info(), a probe_segmented_rmptable_info() failure no longer clears RMPOPT inline — it now relies
on the new {RMPOPT, SEV_SNP} dep: the false return (from probe_segmented_rmptable_info()) makes bsp_determine_snp() clear SEV_SNP, 
which clears RMPOPT. 

2). As i mentioned above, the cpuid-dep only fires when the SEV_SNP CPUID bit is cleared though, and the IOMMU "can't enable SNP" path
clears only CC_ATTR_HOST_SEV_SNP, not the CPUID bit. So we still need the rmpopt_init() (in patch 2 of this series) move into 
snp_rmptable_init() to avoid announcing RMPOPT / allocating the workqueue on a host where the IOMMU turned SNP off — 
this dep doesn't cover that case.

Rest looks right.

Thanks,
Ashish

> 
> Thx.
>