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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] x86/sev: missing output constraint in RMPOPT asm causes infinite loop
- [High] ccp: race condition in SEV-SNP shutdown causes RMPOPT #UD panic
- [High] x86/sev: per-CPU RMPOPT cache warmup logic causes massive NMI lockups
--

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

    x86/sev: Perform RMP optimizations asynchronously
    
    Add support for performing RMP optimizations asynchronously using a
    dedicated per-CPU workqueue. The workqueue is allocated from an initcall,
    and snp_enable_rmpopt() queues the optimization pass.

> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index f458128b9864f..54520d4cf2803 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c

[ ... ]

> @@ -565,6 +590,8 @@ void snp_shutdown(void)
>  	if (syscfg & MSR_AMD64_SYSCFG_SNP_EN)
>  		return;
>  
> +	rmpopt_disable();

[Severity: High]
Does canceling the delayed work here leave a race window that could cause an
invalid opcode panic?

The SEV-SNP shutdown sequence sends the SEV_CMD_SNP_SHUTDOWN_EX command to the
firmware (e.g. from __sev_snp_shutdown_locked() in
drivers/crypto/ccp/sev-dev.c), which clears the SYSCFG.SNPEn MSR and disables
SNP enforcement in hardware.

By the time snp_shutdown() is called here and confirms the MSR is cleared, the
hardware feature is already disabled. If the delayed workqueue is executing
concurrently on another CPU during this window, could it attempt to execute the
RMPOPT instruction while SNP is disabled, triggering an Invalid Opcode (#UD)?

[ ... ]

> @@ -580,8 +607,69 @@ EXPORT_SYMBOL_FOR_MODULES(snp_shutdown, "ccp");
>  static bool rmpopt_capable(void)
>  {
>  	return cpu_feature_enabled(X86_FEATURE_RMPOPT) &&
> -	       cc_platform_has(CC_ATTR_HOST_SEV_SNP);
> +	       cc_platform_has(CC_ATTR_HOST_SEV_SNP) && rmpopt_enabled;
>  }

[Severity: High]
Does checking static boot attributes rather than dynamic state here allow
subsequent caller sequences to bypass the capability check after a shutdown?

Because cc_platform_has(CC_ATTR_HOST_SEV_SNP) reflects boot state, if SNP is
dynamically disabled during a shutdown or kexec, rmpopt_capable() could still
return true, allowing the workqueue to be improperly re-armed or the
MSR_AMD64_RMPOPT_BASE to be rewritten after a shutdown.

> +/*
> + * 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);
> +
> +	/* Supported by binutils 2.48+ */
> +	asm volatile(".byte 0xf2, 0x0f, 0x01, 0xfc"
> +		     :: "a" (pa_start), "c" (op)
> +		     : "memory", "cc");

[Severity: Critical]
Is an output constraint missing for %rax here?

The RMPOPT instruction evaluates a physical memory region and returns a status
code in %rax. The inline assembly passes pa_start via the "a" constraint
(placing it in %rax) but doesn't declare %rax as an output or clobber.

When rmpopt() is inlined into rmpopt_scan_range(), the compiler might place the
loop variable pa in %rax, assuming it is preserved. After the assembly executes,
the hardware overwrites %rax with the status code (e.g., 0 for success).

Could this cause the loop to increment the status code by SZ_1G instead of the
original physical address, resulting in an infinite loop?

> +}
> +
> +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
> +	 * enabled, 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]
Is this per-CPU warmup logic sufficient to prevent cache-miss latencies on all
other CPUs?

Because RMPOPT tables and their caches are strictly per-CPU (as configured via
MSR_AMD64_RMPOPT_BASE), warming up the cache on a single worker CPU does not
populate the caches on the other CPUs.

When on_each_cpu_mask() broadcasts the scan with interrupts disabled (true),
won't every other primary thread execute up to 2048 cold-cache RMPOPT
instructions in hardirq context, potentially triggering NMI watchdog lockups?

> +}

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