Re: [PATCH v14 4/5] x86/sev: Perform RMP optimizations asynchronously

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

On 9/10/2026 5:11 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Re-initializing SNP after a CPU hotplug cycle skips programming the RMPOPT_BASE MSR on the newly onlined CPU, leading to an Invalid Opcode (#UD) exception when executing RMPOPT.
> - [High] The inline assembly for the RMPOPT instruction fails to declare `%rax` as an output constraint or clobber, leading to silent register corruption and unpredictable loop behavior.
> - [High] The RMPOPT 'warm-up' executes on a single CPU, causing the subsequent cross-CPU IPI to execute millions of slow RMPOPT instructions with IRQs disabled on all other CPUs with cold caches, triggering hard lockups.
> --
> 
> commit e2c3ac4ab3639e4c47a6c85a3acca0d88b6fb5a3
> Author: Ashish Kalra <ashish.kalra@amd.com>
> 
> x86/sev: Perform RMP optimizations asynchronously
> 
> This patch introduces asynchronous RMP optimizations by scanning and
> optimizing up to 2TB of system memory at 1GB boundaries using a per-CPU
> workqueue. When SNP guests are launched and regions are allocated, hardware
> clears the optimizations, so the workqueue asynchronously reapplies them.
> 
>> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
>> index a059327dc1071..35678b1f535d4 100644
>> --- a/arch/x86/virt/svm/sev.c
>> +++ b/arch/x86/virt/svm/sev.c
> [ ... ]
>> +static void rmpopt(u64 pa)
>> +{
>> +	enum rmpopt_op_type op = RMPOPT_OP_VERIFY_AND_REPORT_STATUS;
>> +	u64 pa_start = ALIGN_DOWN(pa, SZ_1G);
>> +
>> +	/* Supported by binutils 2.48+ */
>> +	asm volatile(".byte 0xf2, 0x0f, 0x01, 0xfc"
>> +		     :: "a" (pa_start), "c" (op)
>> +		     : "memory", "cc");
> 
> [Severity: High]
> Does this inline assembly need to declare %rax as an output or clobbered
> register?
> 
> Since RMPOPT_OP_VERIFY_AND_REPORT_STATUS instructs the hardware to return a
> status code in %rax, the compiler might allocate the loop counter pa to %rax
> in rmpopt_scan_range(). If hardware overwrites %rax, the loop counter would
> be reset, potentially causing an infinite loop.
> 

RMPOPT's only output is the carry flag (RFLAGS.CF); it does not modify %rax. The VERIFY_AND_REPORT_STATUS result is returned in CF, not %rax — unlike RMPREAD/PSMASH/RMPUPDATE, which return data in registers. Since RMPOPT leaves %rax unchanged, the input‑only "a" (pa_start) constraint is correct and no output/clobber for %rax is required; the loop counter cannot be corrupted.

>> +}
>> +
>> +/* 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)
>> +{
>> +	/*
>> +	 * Warm up the RMPOPT cache on this pinned per-CPU worker with interrupts
>> +	 * on, so the IRQ-disabled fan-out below only issues cache-hit RMPOPTs.
>> +	 */
>> +	rmpopt_scan_range(NULL);
>> +
>> +	on_each_cpu_mask(cpu_primary_thread_mask, rmpopt_scan_range, NULL, true);
> 
> [Severity: High]
> Could this fan-out cause significant stalls on other CPUs?
> 
> The RMPOPT cache is warmed up locally on the current CPU, but because the
> caches are strictly per-CPU, all other CPUs will still have a cold cache.
> When they receive the IPI, they will perform the full up to 2TB scan in
> hardirq context with a cold cache, which seems likely to trigger NMI
> watchdog timeouts across the system.
> 

The premise that the RMPOPT caches are strictly per‑CPU is not correct. The RMPOPT optimization results are held in shared memory, not in per‑CPU caches. Once the warm‑up pass on the local CPU — run with interrupts enabled and preemptible — has verified and recorded the 1GB regions, the follower CPUs read those already‑computed results from shared memory, so their RMPOPTs short‑circuit instead of re‑scanning the RMP. This is why the fan‑out is cheap: measured via the debug follower_timings node, the worst‑case follower IPI callback completes in ≈1.3 ms (1,341,505 ns) — roughly four orders of
magnitude below the soft‑lockup/NMI‑watchdog thresholds— confirming the followers do not each perform a cold up‑to‑2TB scan in hardirq.

>> +}
>> +
>> +void snp_setup_rmpopt(void)
>> +{
>> +	u64 rmpopt_base;
>> +	int cpu;
>> +
>> +	if (!rmpopt_capable())
>> +		return;
>> +
>> +	guard(mutex)(&rmpopt_wq_mutex);
>> +
>> +	/*
>> +	 * Set up once: the workqueue and RMPOPT_BASE MSRs are left in place on
>> +	 * shutdown, so a later re-initialization just re-queues the optimization
>> +	 * pass rather than redoing the setup.
>> +	 */
>> +	if (rmpopt_wq) {
>> +		queue_delayed_work(rmpopt_wq, &rmpopt_delayed_work, 0);
>> +		return;
> 
> [Severity: High]
> Is it safe to skip programming MSR_AMD64_RMPOPT_BASE on newly onlined CPUs?
> 
> If a CPU is hot-unplugged and hot-plugged back, its MSRs are reset to their
> defaults. If SNP is disabled and re-enabled during this cycle,
> snp_setup_rmpopt() will see that rmpopt_wq is already allocated and return
> early. This skips the MSR programming loop for the hotplugged CPU, which
> would lead to an Invalid Opcode (#UD) exception when executing RMPOPT on it.
> 

CPU hotplug is disabled the entire time SNP is active (patch 2/5), so a CPU's RMPOPT_BASE can only be reset by a hotplug cycle that occurs after a full SNP shutdown has cleared SnpEn and re‑enabled hotplug.
RMPOPT is set up once and left in place; re‑initialization simply re‑queues the optimization pass rather than reprogramming the MSRs. Guarding against a full‑shutdown + CPU‑hotplug + SNP‑re‑init sequence is outside the supported flow, so the setup is intentionally not re‑done on re‑initialization.

Thanks,
Ashish

>> +	}
>