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

On Mon, Jul 27, 2026 at 07:05:29PM +0000, Ashish Kalra wrote:
> From: Ashish Kalra <[email protected]>
> 
> When SEV-SNP is enabled, all writes to memory are checked to ensure
> integrity of SNP guest memory. This imposes performance overhead on the

s/SNP//

The checks are done not only on SNP guest memory but on *all* memory, as your
next paragraph suggests.

> whole system.
> 
> RMPOPT is a new instruction that minimizes the performance overhead of
> RMP checks on the hypervisor and on non-SNP guests by allowing RMP
> checks to be skipped for 1GB regions of memory that are known not to
> contain any SEV-SNP guest memory.

Let's tone down the abbreviations. "SNP guest memory" is enough and let's
stick to that.

> Add support for performing RMP optimizations asynchronously using a
> dedicated workqueue.
> 
> Enable RMPOPT optimizations for up to 2TB of system RAM starting from
> the lowest physical memory address aligned down to a 1GB boundary at
> RMP initialization time. RMP checks can initially be skipped for 1GB

Why "initially"? What are you trying to say here?

> memory ranges that do not contain SEV-SNP guest memory (excluding
> preassigned pages such as the RMP table and firmware pages). As SNP
> guests are launched, RMPUPDATE will disable the corresponding RMPOPT
> optimizations.

Because it will add pages to the RMP table?

This paragraph needs clarification.

> Suggested-by: Thomas Lendacky <[email protected]>
> Suggested-by: Dave Hansen <[email protected]>
> Suggested-by: K Prateek Nayak <[email protected]>
> Reviewed-by: Ackerley Tng <[email protected]>
> Signed-off-by: Ashish Kalra <[email protected]>
> ---
>  arch/x86/virt/svm/sev.c | 160 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 158 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index 8bfd80284836..04b19e64f832 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
> @@ -19,6 +19,7 @@
>  #include <linux/iommu.h>
>  #include <linux/amd-iommu.h>
>  #include <linux/nospec.h>
> +#include <linux/workqueue.h>
>  
>  #include <asm/sev.h>
>  #include <asm/processor.h>
> @@ -125,7 +126,18 @@ static void *rmp_bookkeeping __ro_after_init;
>  static u64 probed_rmp_base, probed_rmp_size;
>  
>  static cpumask_var_t rmpopt_cpumask;
> -static phys_addr_t rmpopt_pa_start;
> +static phys_addr_t rmpopt_pa_start, rmpopt_pa_end;
> +
> +enum rmpopt_function {

	rmpopt_op_type

> +	RMPOPT_FUNC_VERIFY_AND_REPORT_STATUS,
> +	RMPOPT_FUNC_REPORT_STATUS

RMPOPT_OP_VERIFY...

> +};
> +

/*
 * This timeout was selected this way because...
 */

> +#define RMPOPT_WORK_TIMEOUT	10000
> +
> +static struct workqueue_struct *rmpopt_wq;
> +static struct delayed_work rmpopt_delayed_work;
> +static DEFINE_MUTEX(rmpopt_wq_mutex);
>  
>  static LIST_HEAD(snp_leaked_pages_list);
>  static DEFINE_SPINLOCK(snp_leaked_pages_list_lock);
> @@ -565,11 +577,20 @@ static void snp_cleanup_rmpopt(void)
>  {
>  	int cpu;
>  
> +	guard(mutex)(&rmpopt_wq_mutex);
> +
> +	if (!rmpopt_wq)
> +		return;

If there's no workqueue, you skip all the rest, including undoing things which
are not workqueue-related?

That workqueue pointer must be magical and special. Yet, I don't see anything
explaining that.

> +
> +	cancel_delayed_work_sync(&rmpopt_delayed_work);
> +	destroy_workqueue(rmpopt_wq);
> +
>  	for_each_cpu(cpu, rmpopt_cpumask)
>  		wrmsrq_on_cpu(cpu, MSR_AMD64_RMPOPT_BASE, 0);
>  
>  	free_cpumask_var(rmpopt_cpumask);
> -	rmpopt_pa_start = 0;
> +	rmpopt_pa_start = rmpopt_pa_end = 0;
> +	rmpopt_wq = NULL;
>  }
>  
>  void snp_shutdown(void)
> @@ -599,6 +620,96 @@ static bool rmpopt_capable(void)
>  	       cc_platform_has(CC_ATTR_HOST_SEV_SNP);
>  }
>  
> +/*
> + * RMPOPT: F2 0F 01 FC
> + *   Input:  RAX = system physical address (1GB aligned)
> + *           RCX = operation type
> + *   Output: CF set if the range was optimized
> + */
> +static inline bool __rmpopt(u64 pa_start, u64 op_type)
> +{
> +	bool optimized;
> +

	/*
	 * needs a comment here which says which binutils version
	 * supports the RMPOPT mnemonic.
	 */
> +	asm volatile(".byte 0xf2, 0x0f, 0x01, 0xfc"
> +		     : "=@ccc" (optimized)
> +		     : "a" (pa_start), "c" (op_type)
> +		     : "memory", "cc");
> +
> +	return optimized;
> +}
> +
> +static void rmpopt(u64 pa)
> +{
> +	u64 pa_start = ALIGN_DOWN(pa, SZ_1G);
> +	u64 op_type = RMPOPT_FUNC_VERIFY_AND_REPORT_STATUS;

	enum rmpopt_op_type op = ...

> +
> +	__rmpopt(pa_start, op_type);

Looks like the __rmpopt() carve out is not really necessary and you can merge
it back into rmpopt().

> +}
> +
> +/*
> + * 'val' is a system physical address.
> + */
> +static void rmpopt_smp(void *val)

You don't need that one - you can use rmpopt(). But keep on reading...

> +{
> +	rmpopt((u64)val);
> +}
> +
> +/*
> + * RMPOPT optimizations skip RMP checks at 1GB granularity if this
> + * range of memory does not contain any SNP guest memory.
> + */

Put that comment above rmpopt().

> +static void rmpopt_work_handler(struct work_struct *work)
> +{
> +	cpumask_var_t follower_mask;
> +	phys_addr_t pa;

So either phys_addr_t or u64 but not both for a pa.

> +	int this_cpu;
> +
> +	pr_info("Attempt RMP optimizations on physical address range @1GB alignment [0x%016llx - 0x%016llx]\n",
> +		rmpopt_pa_start, rmpopt_pa_end);

This is going to spam dmesg every time the workqueue runs?

Nope, zap it.

> +	if (!alloc_cpumask_var(&follower_mask, GFP_KERNEL)) {
> +		pr_warn("RMP optimization pass skipped: cpumask allocation failed\n");
> +		return;
> +	}

Why? Why isn't the follower mask allocated once at init time?

> +
> +	/*
> +	 * RMPOPT scans the RMP table, stores the result of the scan in the
> +	 * reserved processor memory. The RMP scan is the most expensive
> +	 * part. If a second RMPOPT occurs, it can skip the expensive scan
> +	 * if they can see a cached result in the reserved processor memory.
> +	 *
> +	 * Do RMPOPT on one CPU alone. Then, follow that up with RMPOPT
> +	 * on every other primary thread. Followers are "designed to"
> +	 * skip the scan if they see the "cached" scan results.
> +	 *
> +	 * Pin the worker to the current CPU for the leader loop so that

Isn't worker == leader here?

> +	 * this_cpu remains valid and the RMPOPT instruction executes on
> +	 * the correct CPU. 

> Use migrate_disable() rather than get_cpu() to
> +	 * prevent migration while still allowing preemption.

No need to explain that.

> +	 */
> +	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);
> +
> +	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);

An IPI per 1G pa?!?!? On each CPU?!

Instead of IPIing each CPU and inside the handler, doing the loop?

Nope.

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

Ok, enough for this part. Part II coming up later.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette