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

[sashiko-bot@kernel.org]

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 <ashish.kalra@amd.com>

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.

> +
> +	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.

> +
> +	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.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784844080.git.ashish.kalra@amd.com?part=4