Re: [PATCH v8 2/7] x86/sev: Initialize RMPOPT configuration MSRs

[Kalra, Ashish]

Hello Prateek,

On 6/16/2026 1:03 AM, K Prateek Nayak wrote:
> Hello Ashish,
> 
> On 6/16/2026 1:18 AM, Ashish Kalra wrote:
>> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
>> index 8bcdce98f6dc..1b5c18408f0b 100644
>> --- a/arch/x86/virt/svm/sev.c
>> +++ b/arch/x86/virt/svm/sev.c
>> @@ -124,6 +124,10 @@ static void *rmp_bookkeeping __ro_after_init;
>>  
>>  static u64 probed_rmp_base, probed_rmp_size;
>>  
>> +static cpumask_t rmpopt_cpumask;
> 
> nit.
> 
> I believe you can use cpumask_var_t here and do a zalloc_cpumask_var()
> during snp_setup_rmpopt(). That way !X86_FEATURE_RMPOPT configs don't
> have to needlessly waste space to keep a redundant cpumask around.
> 
> Same comment for rmpopt_report_cpumask in Patch 7 which can be
> allocated dynamically during rmpopt_debugfs_setup().
> 

Yes.

>> +static phys_addr_t rmpopt_pa_start;
>> +static bool rmpopt_configured;
>> +
>>  static LIST_HEAD(snp_leaked_pages_list);
>>  static DEFINE_SPINLOCK(snp_leaked_pages_list_lock);
>>  
>> @@ -490,7 +494,12 @@ static bool __init setup_rmptable(void)
>>  	if (rmp_cfg & MSR_AMD64_SEG_RMP_ENABLED) {
>>  		if (!setup_segmented_rmptable())
>>  			return false;
>> +		rmpopt_configured = true;
>>  	} else {
>> +		/*
>> +		 * RMPOPT requires a segmented RMP table, so leave
>> +		 * rmpopt_configured clear on contiguous RMP systems.
>> +		 */
>>  		if (!setup_contiguous_rmptable())
>>  			return false;
>>  	}
>> @@ -555,6 +564,21 @@ int snp_prepare(void)
>>  }
>>  EXPORT_SYMBOL_FOR_MODULES(snp_prepare, "ccp");
>>  
>> +static void rmpopt_cleanup(void)
>> +{
>> +	int cpu;
>> +
>> +	cpus_read_lock();
> 
> nit.
> 
> You can use guard(cpus_read_lock)() unless there is a complicated
> locking pattern where you need to drop and re-acquire the read lock.

But if i use guard(cpus_read_lock)(), cpus_read_lock stays held across as it is
function-scope, so it will be still held for code following the wrmsrq_on_cpu(),
which is harmless but still changes code behavior.

Probably, the other option is to use scoped_guard form ? 

Thanks,
Ashish

> 
>> +
>> +	for_each_cpu(cpu, &rmpopt_cpumask)
>> +		WARN_ON_ONCE(wrmsrq_on_cpu(cpu, MSR_AMD64_RMPOPT_BASE, 0));
>> +
>> +	cpus_read_unlock();
>> +
>> +	cpumask_clear(&rmpopt_cpumask);
>> +	rmpopt_pa_start = 0;
>> +}
>> +
>>  void snp_shutdown(void)
>>  {
>>  	u64 syscfg;
>