Re: [PATCH v11 4/6] x86/sev: Add support to perform RMP optimizations asynchronously

On 7/27/2026 2:22 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Inline assembly for __rmpopt fails to specify EAX/RAX as a clobbered or output register, risking an infinite loop.
> - [High] Missing CPU yield in long-running RMP optimization loops leads to soft lockups on non-preemptible kernels.
> --
> 
> --- Patch [4]: [PATCH v11 4/6] x86/sev: Add support to perform RMP optimizations asynchronously ---
> commit 9b1e0ac7d870b3ba02472132010185afc997e02d
> Author: Ashish Kalra <[email protected]>
> 
> x86/sev: Add support to perform RMP optimizations asynchronously
> 
> This commit adds support for performing RMP optimizations asynchronously using
> a dedicated workqueue. It introduces a new RMPOPT instruction to skip RMP checks
> for 1GB regions not containing SEV-SNP guest memory. Optimizations are enabled
> for up to 2TB of system RAM starting at RMP initialization time.
> 
>> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
>> index 8bfd802848361..04b19e64f8329 100644
>> --- a/arch/x86/virt/svm/sev.c
>> +++ b/arch/x86/virt/svm/sev.c
> 
> [ ... ]
> 
>> +static inline bool __rmpopt(u64 pa_start, u64 op_type)
>> +{
>> +	bool optimized;
>> +
>> +	asm volatile(".byte 0xf2, 0x0f, 0x01, 0xfc"
>> +		     : "=@ccc" (optimized)
>> +		     : "a" (pa_start), "c" (op_type)
>> +		     : "memory", "cc");
> 
> [Severity: High]
> Does this inline assembly need to specify EAX/RAX as a clobbered or output
> register?
> 
> If the RMPOPT instruction modifies RAX (as AMD SEV-SNP instructions like
> RMPUPDATE typically return an execution status code in EAX/RAX), the current
> specification of RAX as input-only ("a" (pa_start)) could allow the compiler
> to optimize the loop counter into RAX and silently overwrite it.
> 
> This could lead to an infinite loop during rmpopt_work_handler() execution.
> 

RMPOPT's only output is RFLAGS.CF; it does not modify RAX. RAX is not modified and cannot be clobbered and
so the loop counter can't be corrupted. (RMPUPDATE differs: it returns a status in RAX, hence "=a".)

>> +
>> +	return optimized;
>> +}
> 
> [ ... ]
> 
>> +static void rmpopt_work_handler(struct work_struct *work)
>> +{
> 
> [ ... ]
> 
>> +	migrate_disable();
>> +	this_cpu = smp_processor_id();
>> +
>> +	cpumask_andnot(follower_mask, rmpopt_cpumask,
>> +		       topology_sibling_cpumask(this_cpu));
>> +
>> +	for (pa = rmpopt_pa_start; pa < rmpopt_pa_end; pa += SZ_1G)
>> +		rmpopt(pa);
> 
> [Severity: High]
> Does this loop need a cond_resched() to yield the CPU?
> 
> This loops up to 2048 times doing an expensive hardware scan inside a
> migrate_disable() block. On kernels with CONFIG_PREEMPT_NONE or
> PREEMPT_VOLUNTARY, preemption is not implicit.
> 
> Without explicitly yielding the CPU, this could cause severe scheduling latency
> spikes or trigger a soft lockup watchdog panic.
> 

Dropped intentionally: since commit 7dadeaa6e851 x86 has no non-preemptible model (PREEMPT_VOLUNTARY/PREEMPT_NONE aren't selectable), 
so cond_resched() is a no-op and the loops are already preemptible — migrate_disable() pins the CPU but doesn't disable preemption.
No soft-lockup risk.

>> +
>> +	migrate_enable();
>> +
>> +	/*
>> +	 * Followers: run RMPOPT on the remaining cores.  cpus_read_lock() is
>> +	 * intentionally not held here: CPU hotplug is disabled for the entire
>> +	 * time SNP is active (see snp_prepare()), and this work only runs while
>> +	 * SNP is active, so the follower set stays valid across the whole scan.
>> +	 */
>> +	for (pa = rmpopt_pa_start; pa < rmpopt_pa_end; pa += SZ_1G)
>> +		on_each_cpu_mask(follower_mask, rmpopt_smp, (void *)pa, true);
> 
> [Severity: High]
> Could this secondary follower loop also need a cond_resched() to prevent
> CPU monopolization?
> 
> This loop iterates up to 2048 times broadcasting IPIs via on_each_cpu_mask().
> Similar to the first loop, without a yield, this could lead to soft lockups on
> non-preemptible kernels.
> 

Dropped intentionally: since commit 7dadeaa6e851 x86 has no non-preemptible model (PREEMPT_VOLUNTARY/PREEMPT_NONE aren't selectable), 
so cond_resched() is a no-op and the loops are already preemptible — No soft-lockup risk.

Thanks,
Ashish

>> +
>> +	free_cpumask_var(follower_mask);
>> +}
>