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

["Kalra, Ashish" <ashish.kalra@amd.com>]

On 9/2/2026 4:57 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Critical] Declare %rax as clobbered or output in rmpopt() to prevent infinite loops.
> - [High] Schedule work in process context; executing on_each_cpu_mask triggers hard lockups.
> - [High] Use ALIGN_DOWN for rmpopt_pa_end to prevent out-of-bounds RMP table reads.
> --
> 
> commit 0121e9d5bd127536db9548e1a9f202c9576ef5ff
> Author: Ashish Kalra <ashish.kalra@amd.com>
> 
> x86/sev: Add support to perform RMP optimizations asynchronously
> 
> This commit introduces support for asynchronous RMP checks on 1GB physical
> memory regions to optimize performance on hardware supporting RMPOPT. A
> dedicated workqueue triggers the optimization process during SNP
> initialization and after legacy shutdowns.
> 
>> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
>> index 07f178eb76c7e..a4c0fe49b9ec7 100644
>> --- a/arch/x86/virt/svm/sev.c
>> +++ b/arch/x86/virt/svm/sev.c
> 
> [ ... ]
> 
>> @@ -595,6 +621,44 @@ static bool rmpopt_capable(void)
>>  	       cc_platform_has(CC_ATTR_HOST_SEV_SNP);
>>  }
>>  
>> +/*
>> + * RMPOPT optimizations skip RMP checks at 1GB granularity if this range of
>> + * memory does not contain any SNP guest memory.
>> + *
>> + * @pa is a system physical address; RMPOPT operates on the containing 1GB.
>> + */
>> +static void rmpopt(u64 pa)
>> +{
>> +	enum rmpopt_op_type op = RMPOPT_OP_VERIFY_AND_REPORT_STATUS;
>> +	u64 pa_start = ALIGN_DOWN(pa, SZ_1G);
>> +
>> +	asm volatile(".byte 0xf2, 0x0f, 0x01, 0xfc"
>> +		     :: "a" (pa_start), "c" (op)
>> +		     : "memory", "cc");
> 
> [Severity: Critical]
> Does this inline assembly need to declare %rax as an output or clobber?
> 
> Since the RMPOPT hardware instruction returns a status code in %rax, could
> GCC assume %rax is unmodified because it is only provided as an input? If
> rmpopt() is inlined, GCC might allocate %rax for the induction variable pa
> in the calling loop. This would cause the loop step to compute incorrectly,
> potentially resulting in an infinite loop during boot.
> 

RMPOPT does not modify RAX — its only output is the carry flag (RFLAGS.CF),
which the asm already covers with the "cc" clobber. RAX (the 1GB-aligned
SPA) and RCX (the op type) are inputs only and are left unchanged, unlike
RMPREAD/PSMASH/RMPUPDATE which do return a status in RAX. So the input-only
"a"(pa_start) constraint describes RMPOPT correctly; GCC may keep the loop's
pa in RAX and the loop step is correct. (rmpopt() intentionally discards CF
on this path; only the debug/report path reads it, via a separate asm with a
"=@ccc" output.)

>> +}
>> +
>> +/* on_each_cpu() callback: optimize the whole RMPOPT range on this CPU. */
>> +static void rmpopt_scan_range(void *arg)
>> +{
>> +	u64 pa;
>> +
>> +	for (pa = rmpopt_pa_start; pa < rmpopt_pa_end; pa += SZ_1G)
>> +		rmpopt(pa);
>> +}
>> +
>> +static void do_rmpopt_work(struct work_struct *work)
>> +{
>> +	/*
>> +	 * RMPOPT caches the results of a RMP table scan in reserved processor
>> +	 * memory, allowing future invocations to skip such costly operations.
>> +	 */
>> +	migrate_disable();
>> +	rmpopt_scan_range(NULL);
>> +	migrate_enable();
>> +
>> +	on_each_cpu_mask(cpu_primary_thread_mask, rmpopt_scan_range, NULL, true);
> 
> [Severity: High]
> Could executing on_each_cpu_mask() here with wait=true trigger hard lockups?
> 
> This call runs the rmpopt_scan_range() loop synchronously on all primary
> threads in an IPI context with hardirqs disabled. Since the loop can
> iterate up to 2048 times invoking the slow RMPOPT hardware instruction,
> is it possible for remote CPUs with cold caches (like on multi-socket
> systems) to stall for seconds inside the IPI handler and trigger an NMI
> watchdog panic?
> 

The fan-out is reached only after the single warm-up scan that precedes it:
do_rmpopt_work() runs rmpopt_scan_range() once on the current CPU under
migrate_disable() (interrupts on, preemptible), which does the expensive RMP
scan and stores the result in the RMPOPT table. All primary threads are
programmed with the same RMPOPT_BASE, so that table is shared; the subsequent
on_each_cpu_mask() therefore issues only cache-hit RMPOPTs — reads of the
already-computed result, including on remote sockets — not re-scans.

Measured over a 2 TB window: the cache-hit fan-out is ~0.32 s per core — 
~30x under the 10 s NMI watchdog even with ~2048 iterations and cold data caches. 
The IRQ-disabled window per follower is bounded by the cache-hit cost, not the
scan cost.

>> +}
> 
> [ ... ]
> 
>> @@ -612,6 +704,23 @@ void snp_setup_rmpopt(void)
>>  	 */
>>  	for_each_cpu(cpu, cpu_primary_thread_mask)
>>  		wrmsrq_on_cpu(cpu, MSR_AMD64_RMPOPT_BASE, rmpopt_base);
>> +
>> +	rmpopt_pa_end = ALIGN(PFN_PHYS(max_pfn), SZ_1G);
> 
> [Severity: High]
> Should rmpopt_pa_end be calculated using ALIGN_DOWN() instead of ALIGN()?
> 
> If max_pfn is not 1GB aligned, aligning upwards creates a hardware scanning
> range that extends into unbacked physical memory. When rmpopt_scan_range()
> processes this final 1GB block, could the hardware instruction attempt to
> read RMP entries beyond the allocated RMP table and cause a Machine Check
> Exception or memory corruption?

RMPOPT does not read outside the RMP table in a way that can fault. It only
reads RMP table entries to determine whether the 1 GB region is entirely HV-owned;
for any address in the region that lies outside the RMP table's range, the
instruction simply treats that address as HV-owned. It does not raise a #MC
and does not perform an out-of-bounds access.
  
Rounding rmpopt_pa_end up with ALIGN() is therefore intentional and safe: it
lets the final, partially-populated 1 GB of RAM be optimized — the backed
part is checked normally and any tail beyond the RMP table is treated as
HV-owned — with no risk of reading past the table. Using ALIGN_DOWN() would
instead leave that last partial-1 GB region un-optimized for no safety
benefit, so ALIGN() is deliberate here.

Thanks,
Ashish
> 
>> +
>> +	/* Limit memory scanning to 2TB of RAM */
>> +	if ((rmpopt_pa_end - rmpopt_pa_start) > SZ_2T) {
>> +		pr_info("RMPOPT coverage limited to 2TB; memory above 0x%llx not optimized\n",
>> +			rmpopt_pa_start + SZ_2T);
>> +		rmpopt_pa_end = rmpopt_pa_start + SZ_2T;
>> +	}
>