Re: [PATCH v6 3/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] Missing `cond_resched()` in the leader RMPOPT loops inside `rmpopt_work_handler` can trigger soft lockups.
- [High] The inline assembly for `__rmpopt()` fails to mark the RAX register as clobbered or output, which can corrupt the caller's loop variable.
--

commit dea16dc76b50a8acabc16915c0d451449f0d1849
Author: Ashish Kalra <ashish.kalra@amd.com>

x86/sev: Add support to perform RMP optimizations asynchronously

This patch introduces support for the RMPOPT instruction to minimize RMP check
overhead by skipping 1GB regions not containing SEV-SNP guest memory. It adds
a dedicated workqueue to process up to 2TB of RAM asynchronously.

> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index 089c9a14edc74..d7e40a5fe5ca6 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 mark the RAX register as an output
or clobber?

Like other SEV-SNP instructions like PVALIDATE or RMPUPDATE, RMPOPT
returns a status code in RAX. Since the inline assembly block does not
mark RAX as modified by using "+a" (pa_start), the compiler is unaware
that the register's value has changed.

When rmpopt is inlined into the scanning loop, the compiler might
map the loop variable pa to RAX. If the hardware overwrites RAX with
a status code, could this corrupt the memory scan logic by resetting
the physical address being scanned?

> +
> +	return optimized;
> +}

[ ... ]

> +	if (cpumask_test_cpu(this_cpu, follower_mask)) {
> +		/*
> +		 * Current CPU is a primary thread in rmpopt_cpumask.
> +		 * Run leader locally and remove from follower mask.
> +		 */
> +		cpumask_clear_cpu(this_cpu, follower_mask);
> +
> +		for (pa = rmpopt_pa_start; pa < rmpopt_pa_end; pa += SZ_1G)
> +			rmpopt(pa);

[Severity: High]
Could this loop run for a long time without yielding and trigger soft lockups?

Since rmpopt_pa_end can be up to 2TB, this loop might execute up to 2048
times. The follower loop further down includes a cond_resched(), but this
leader loop and the sibling thread leader loop below omit it entirely.

> +	} else if (cpumask_intersects(topology_sibling_cpumask(this_cpu),
> +				      follower_mask)) {
> +		/*
> +		 * Current CPU is a sibling thread whose primary is in
> +		 * rmpopt_cpumask.  RMPOPT_BASE MSR is per-core, so it
> +		 * is safe to run the leader locally.  Remove the sibling's
> +		 * primary from the follower mask as this core is already
> +		 * covered by the leader.
> +		 */
> +		cpumask_andnot(follower_mask, follower_mask,
> +			       topology_sibling_cpumask(this_cpu));
> +
> +		for (pa = rmpopt_pa_start; pa < rmpopt_pa_end; pa += SZ_1G)
> +			rmpopt(pa);
> +	} else {
> +		/*
> +		 * Current CPU does not have RMPOPT_BASE MSR programmed.
> +		 * Pick an explicit leader from the cpumask to avoid #UD.
> +		 */
> +		int leader_cpu = cpumask_first(follower_mask);
> +
> +		if (WARN_ON_ONCE(leader_cpu >= nr_cpu_ids)) {
> +			migrate_enable();
> +			goto out;
> +		}
> +
> +		cpumask_clear_cpu(leader_cpu, follower_mask);
> +
> +		cpus_read_lock();
> +		for (pa = rmpopt_pa_start; pa < rmpopt_pa_end; pa += SZ_1G)
> +			smp_call_function_single(leader_cpu, rmpopt_smp,
> +						 (void *)pa, true);

[Severity: High]
Does this loop also need to yield?

It sends an IPI and busy-waits with wait=true for the expensive scan up to
2048 times without yielding.

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