Add RMPOPT support.

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] System suspend/resume bypasses cpu_hotplug_disable(), causing secondary CPUs to come online without the SnpEn bit set. --

--- Patch [2]: [PATCH v11 2/6] x86/sev: Disable CPU hotplug while SNP is active --- commit 2ca360619d15fed203ba00f43d20453130503df3
Author: Ashish Kalra <[email protected]>

x86/sev: Disable CPU hotplug while SNP is active

This commit replaces cpus_read_lock() with cpu_hotplug_disable() in snp_prepare() to prevent CPU hotplug while SEV-SNP is being enabled. It additionally keeps hotplug disabled as long as SNP is active to ensure OS CPU hotplug does not diverge from firmware expectations.

Quoted 4 lines

arch/x86/virt/svm/sev.c +0 -0

index cff285d8ad8e3..e2f69fba09389 100644

[ ... ]

arch/x86/virt/svm/sev.c int snp_prepare(void) · line 535

Quoted 13 lines

arch/x86/virt/svm/sev.c +8 -1

@@ -526,14 +525,21 @@ int snp_prepare(void)
 
 	clear_rmp();
 
-	cpus_read_lock();
+	/*
+	 * Disable CPU hotplug before enabling SNP: no CPU may come online
+	 * without SnpEn while SNP is active, and none may go offline during
+	 * enable.  This keeps cpu_online_mask stable for the check and the
+	 * on_each_cpu() calls below, so cpus_read_lock() is not needed.  It is
+	 * re-enabled in snp_shutdown() once the firmware disables SNP.
+	 */
+	cpu_hotplug_disable();

[Severity: High]
This is a pre-existing issue, but does cpu_hotplug_disable() reliably prevent secondary CPUs from coming online without the SnpEn bit set?

System suspend and resume transitions bypass the sysfs hotplug locks. During a suspend and resume cycle, freeze_secondary_cpus() and thaw_secondary_cpus() directly invoke internal routines that offline and online CPUs, respectively.

Because snp_enable() is not registered in a CPU hotplug callback, if a secondary CPU is thawed, it appears it would wake up and come online without the MSR_AMD64_SYSCFG_SNP_EN bit set. Would this leave the system vulnerable by allowing arbitrary host access to SEV-SNP guest memory, since no RMP checks would be performed by those CPUs?

With CPU hotplug now disabled while SNP is active, the online CPU mask is stable, so the cpus_read_lock() previously taken in snp_prepare() to iterate it is redundant. Drop cpus_read_lock()/cpus_read_unlock() here. The RMPOPT setup and cleanup added later are introduced after this patch and never take the lock for the same reason.

Suggested-by Thomas Lendacky <[email protected]>

Suggested-by Borislav Petkov (AMD) <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
 arch/x86/virt/svm/sev.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

arch/x86/virt/svm/sev.c +0 -1

index cff285d8ad8e..e2f69fba0938 100644
@@ -513,7 +513,6 @@ static void clear_hsave_pa(void *arg)
 
 int snp_prepare(void)
 {
-	int ret;
 	u64 val;
 
 	/*

@@ -526,14 +525,21 @@ int snp_prepare(void)
 
 	clear_rmp();

arch/x86/virt/svm/sev.c +3 -8

 	if (!cpumask_equal(cpu_online_mask, cpu_present_mask)) {
-		ret = -EOPNOTSUPP;
+		cpu_hotplug_enable();
 		pr_warn("SNP init failed: not all CPUs online. (%*pbl online <-> %*pbl present masks).\n",
 			cpumask_pr_args(cpu_online_mask),
 			cpumask_pr_args(cpu_present_mask));
-		goto unlock;
+		return -EOPNOTSUPP;
 	}
 
 	wbinvd_on_all_cpus();

@@ -548,12 +554,7 @@ int snp_prepare(void)
 	/* SNP_INIT requires MSR_VM_HSAVE_PA to be cleared on all CPUs. */
 	on_each_cpu(clear_hsave_pa, NULL, 1);
 
-	ret = 0;
-
-unlock:
-	cpus_read_unlock();
-
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL_FOR_MODULES(snp_prepare, "ccp");
 

arch/x86/virt/svm/sev.c +0 -0

 }

Quoted 5 lines · Ashish Kalra

With CPU hotplug now disabled while SNP is active, the online CPU mask is stable, so the cpus_read_lock() previously taken in snp_prepare() to iterate it is redundant. Drop cpus_read_lock()/cpus_read_unlock() here. The RMPOPT setup and cleanup added later are introduced after this patch and never take the lock for the same reason.

Why do I even bother writing it?

"do ... not talk about future patches because git history is not always linear"

Quoted 24 lines

Suggested-by Thomas Lendacky <[email protected]>

Suggested-by Borislav Petkov (AMD) <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
 arch/x86/virt/svm/sev.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

arch/x86/virt/svm/sev.c +0 -1

index cff285d8ad8e..e2f69fba0938 100644
@@ -513,7 +513,6 @@ static void clear_hsave_pa(void *arg)
 
 int snp_prepare(void)
 {
-	int ret;
 	u64 val;
 
 	/*

@@ -526,14 +525,21 @@ int snp_prepare(void)
 
 	clear_rmp();
 

arch/x86/virt/svm/sev.c int snp_prepare(void) · line 535

Quoted 9 lines

arch/x86/virt/svm/sev.c +8 -1

-	cpus_read_lock();
+	/*
+	 * Disable CPU hotplug before enabling SNP: no CPU may come online
+	 * without SnpEn while SNP is active, and none may go offline during
+	 * enable.  This keeps cpu_online_mask stable for the check and the
+	 * on_each_cpu() calls below, so cpus_read_lock() is not needed.  It is
+	 * re-enabled in snp_shutdown() once the firmware disables SNP.
+	 */
+	cpu_hotplug_disable();

No need for too much splainin' and besides, that comment'll grow out-of-whack sooner than you think:

arch/x86/virt/svm/sev.c +1 -3

index e2f69fba0938..731ea25fba37 100644
@@ -528,9 +528,7 @@ int snp_prepare(void)
        /*
         * Disable CPU hotplug before enabling SNP: no CPU may come online
         * without SnpEn while SNP is active, and none may go offline during
-        * enable.  This keeps cpu_online_mask stable for the check and the
-        * on_each_cpu() calls below, so cpus_read_lock() is not needed.  It is
-        * re-enabled in snp_shutdown() once the firmware disables SNP.
+        * enable.
         */
        cpu_hotplug_disable();
 

Quoted 26 lines

arch/x86/virt/svm/sev.c +3 -8

 	if (!cpumask_equal(cpu_online_mask, cpu_present_mask)) {
-		ret = -EOPNOTSUPP;
+		cpu_hotplug_enable();
 		pr_warn("SNP init failed: not all CPUs online. (%*pbl online <-> %*pbl present masks).\n",
 			cpumask_pr_args(cpu_online_mask),
 			cpumask_pr_args(cpu_present_mask));
-		goto unlock;
+		return -EOPNOTSUPP;
 	}
 
 	wbinvd_on_all_cpus();

@@ -548,12 +554,7 @@ int snp_prepare(void)
 	/* SNP_INIT requires MSR_VM_HSAVE_PA to be cleared on all CPUs. */
 	on_each_cpu(clear_hsave_pa, NULL, 1);
 
-	ret = 0;
-
-unlock:
-	cpus_read_unlock();
-
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL_FOR_MODULES(snp_prepare, "ccp");
 

arch/x86/virt/svm/sev.c void snp_shutdown(void) · line 574

Quoted 9 lines

arch/x86/virt/svm/sev.c +6 -0

@@ -565,6 +566,13 @@ void snp_shutdown(void)
 	if (syscfg & MSR_AMD64_SYSCFG_SNP_EN)
 		return;
 
+	/*
+	 * The firmware has disabled SNP (SnpEn is clear), so re-enable CPU
+	 * hotplug.  A legacy SNP shutdown returns above with SnpEn still set and
+	 * leaves hotplug disabled.
+	 */
+	cpu_hotplug_enable();

What happens if CPUs get offlined here after hotplug has been enabled and...

arch/x86/virt/svm/sev.c void snp_shutdown(void) · line 576

Quoted 2 lines

arch/x86/virt/svm/sev.c +0 -0

 	clear_rmp();
 	on_each_cpu(mfd_reconfigure, NULL, 1);

... they miss the mfd_reconfigure()?

Quoted 1 line

arch/x86/virt/svm/sev.c +0 -0

 }

From: Ashish Kalra <[email protected]>

While SNP is active, every memory write is checked against the RMP to protect SEV-SNP guest memory. A core performs these RMP checks only once SNP has been initialized via SNP_INIT and the SNP-enable bit in SYSCFG is set on that core; the firmware requires the SNP-enable bit to be set on every present CPU before SNP initialization.

A core that is not SNP-enabled and not SNP-initialized performs no RMP checks at all, so there is no valid configuration with SNP active and any CPU exempt from RMP checks.

The firmware determines which CPUs are present from the processor and the BIOS/UEFI configuration (e.g. SMT disabled in the BIOS) and enumerates them at SNP init; it is not aware of the OS bringing CPUs online or offline afterwards.

SNP_INIT fails unless SnpEn is set on all CPUs, so a CPU that is offline when SNP_INIT is issued, does not have SnpEn set, SNP_INIT fails, and there can be no SNP guest memory. OS CPU hotplug can thus diverge from the firmware's expectations and break SNP.

Tie CPU hotplug to the SNP-enable bit: disable it in snp_prepare() before SNP is enabled, and re-enable it in snp_shutdown() once the firmware has disabled SNP.

If snp_prepare() fails before enabling SNP it re-enables hotplug itself; once SNP is enabled hotplug stays disabled, including across a failed SNP_INIT and across the legacy SNP_SHUTDOWN_EX path, both of which leave SNP enabled.

A kexec target that boots with SNP already enabled, disables hotplug once in snp_rmptable_init(), since snp_prepare() bails when SNP is already enabled.

With CPU hotplug now disabled while SNP is active, the online CPU mask is stable, so the cpus_read_lock() previously taken in snp_prepare() to iterate it is redundant. Drop cpus_read_lock()/cpus_read_unlock() here. The RMPOPT setup and cleanup added later are introduced after this patch and never take the lock for the same reason.

Suggested-by Thomas Lendacky <[email protected]>

Suggested-by Borislav Petkov (AMD) <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
 arch/x86/virt/svm/sev.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

arch/x86/virt/svm/sev.c +20 -10

index cff285d8ad8e..e2f69fba0938 100644
@@ -513,7 +513,6 @@ static void clear_hsave_pa(void *arg)
 
 int snp_prepare(void)
 {
-	int ret;
 	u64 val;
 
 	/*

@@ -526,14 +525,21 @@ int snp_prepare(void)
 
 	clear_rmp();
 
-	cpus_read_lock();
+	/*
+	 * Disable CPU hotplug before enabling SNP: no CPU may come online
+	 * without SnpEn while SNP is active, and none may go offline during
+	 * enable.  This keeps cpu_online_mask stable for the check and the
+	 * on_each_cpu() calls below, so cpus_read_lock() is not needed.  It is
+	 * re-enabled in snp_shutdown() once the firmware disables SNP.
+	 */
+	cpu_hotplug_disable();
 
 	if (!cpumask_equal(cpu_online_mask, cpu_present_mask)) {
-		ret = -EOPNOTSUPP;
+		cpu_hotplug_enable();
 		pr_warn("SNP init failed: not all CPUs online. (%*pbl online <-> %*pbl present masks).\n",
 			cpumask_pr_args(cpu_online_mask),
 			cpumask_pr_args(cpu_present_mask));
-		goto unlock;
+		return -EOPNOTSUPP;
 	}
 
 	wbinvd_on_all_cpus();

@@ -548,12 +554,7 @@ int snp_prepare(void)
 	/* SNP_INIT requires MSR_VM_HSAVE_PA to be cleared on all CPUs. */
 	on_each_cpu(clear_hsave_pa, NULL, 1);
 
-	ret = 0;
-
-unlock:
-	cpus_read_unlock();
-
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL_FOR_MODULES(snp_prepare, "ccp");
 

@@ -565,6 +566,13 @@ void snp_shutdown(void)
 	if (syscfg & MSR_AMD64_SYSCFG_SNP_EN)
 		return;
 
+	/*
+	 * The firmware has disabled SNP (SnpEn is clear), so re-enable CPU
+	 * hotplug.  A legacy SNP shutdown returns above with SnpEn still set and
+	 * leaves hotplug disabled.
+	 */
+	cpu_hotplug_enable();
+
 	clear_rmp();
 	on_each_cpu(mfd_reconfigure, NULL, 1);
 }

@@ -577,6 +585,8 @@ EXPORT_SYMBOL_FOR_MODULES(snp_shutdown, "ccp");
  */
 int __init snp_rmptable_init(void)
 {
+	u64 val;
+
 	if (WARN_ON_ONCE(!cc_platform_has(CC_ATTR_HOST_SEV_SNP)))
 		return -ENOSYS;
 

@@ -586,6 +596,15 @@ int __init snp_rmptable_init(void)
 	if (!setup_rmptable())
 		return -ENOSYS;

arch/x86/virt/svm/sev.c +0 -0

 	/*
 	 * Setting crash_kexec_post_notifiers to 'true' to ensure that SNP panic
 	 * notifier is invoked to do SNP IOMMU shutdown before kdump.

Quoted 11 lines

int __init snp_rmptable_init(void)
{

arch/x86/virt/svm/sev.c +2 -0

+	u64 val;
+
 	if (WARN_ON_ONCE(!cc_platform_has(CC_ATTR_HOST_SEV_SNP)))
 		return -ENOSYS;

@@ -586,6 +596,15 @@ int __init snp_rmptable_init(void)
 	if (!setup_rmptable())
 		return -ENOSYS;
 

arch/x86/virt/svm/sev.c int __init snp_rmptable_init(void) · line 607

Quoted 9 lines

arch/x86/virt/svm/sev.c +9 -0

+	/*
+	 * On a kexec boot SNP may already be enabled (legacy firmware leaves
+	 * SnpEn set across shutdown), in which case snp_prepare() bails without
+	 * disabling CPU hotplug, so disable it here.
+	 */
+	rdmsrq(MSR_AMD64_SYSCFG, val);
+	if (val & MSR_AMD64_SYSCFG_SNP_EN)
+		cpu_hotplug_disable();
+

Why not just put the cpu_hotplug_disable() at the start of snp_prepare() then? Wouldn't that take care of both situations and only end up with a single disable point?

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] snp_cleanup_rmpopt() unconditionally dereferences rmpopt_cpumask, causing a deterministic kernel panic on shutdown if the mask was never allocated (e.g., on CPUs without RMPOPT support or if allocation fails).
- [High] If SEV-SNP firmware initialization fails, the error path returns directly without cleaning up the state set by snp_prepare(), leaving CPU hotplug permanently disabled.
- [Medium] Re-initializing SEV-SNP after a legacy shutdown leaks the rmpopt_cpumask allocation because the old pointer is overwritten without being freed.
--

--- Patch [3]: [PATCH v11 3/6] x86/sev: Initialize RMPOPT configuration MSRs --- commit e4623d49ab6201adc5b609f5a6043ed6dfb7f55c
Author: Ashish Kalra <[email protected]>
Subject: x86/sev: Initialize RMPOPT configuration MSRs

The new RMPOPT instruction helps manage per-CPU RMP optimization structures inside the CPU. This commit initializes the per-CPU RMPOPT table base to enable RMP optimization for up to 2 TB of system RAM.

Quoted 4 lines

arch/x86/virt/svm/sev.c +0 -0

index e2f69fba09389..8bfd802848361 100644

[ ... ]

arch/x86/virt/svm/sev.c int snp_prepare(void) · line 568

Quoted 9 lines

arch/x86/virt/svm/sev.c +5 -0

@@ -558,6 +561,17 @@ int snp_prepare(void)
 }
 EXPORT_SYMBOL_FOR_MODULES(snp_prepare, "ccp");
 
+static void snp_cleanup_rmpopt(void)
+{
+	int cpu;
+
+	for_each_cpu(cpu, rmpopt_cpumask)

[Severity: High]
If rmpopt_cpumask was never allocated, does this macro dereference a NULL pointer?

In snp_setup_rmpopt(), if rmpopt_capable() is false, the mask remains unallocated. When snp_shutdown() subsequently calls snp_cleanup_rmpopt(), it iterates over the mask without checking if it is available.

Can this cause a kernel panic on system shutdown or module unload for systems without RMPOPT support?

Quoted 6 lines

arch/x86/virt/svm/sev.c +5 -0

+		wrmsrq_on_cpu(cpu, MSR_AMD64_RMPOPT_BASE, 0);
+
+	free_cpumask_var(rmpopt_cpumask);
+	rmpopt_pa_start = 0;
+}

[ ... ]

Quoted 9 lines

arch/x86/virt/svm/sev.c +5 -0

@@ -578,6 +593,46 @@ void snp_shutdown(void)
 }
 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);
+}

arch/x86/virt/svm/sev.c void snp_shutdown(void) · line 610

Quoted 10 lines

arch/x86/virt/svm/sev.c +10 -0

+
+void snp_setup_rmpopt(void)
+{
+	u64 rmpopt_base;
+	int cpu;
+
+	if (!rmpopt_capable())
+		return;
+
+	if (!zalloc_cpumask_var(&rmpopt_cpumask, GFP_KERNEL)) {

[Severity: Medium]
Does this unconditional allocation leak the previously allocated cpumask when SEV-SNP is re-initialized?

During a legacy shutdown, snp_shutdown() returns early when the SnpEn firmware bit is still set, skipping snp_cleanup_rmpopt():

arch/x86/virt/svm/sev.c:snp_shutdown() {
...

	rdmsrq(MSR_AMD64_SYSCFG, syscfg);
	if (syscfg & MSR_AMD64_SYSCFG_SNP_EN)
		return;

...
}

A subsequent call to __sev_snp_init_locked() will invoke snp_setup_rmpopt() again and overwrite the old pointer without freeing it.

Quoted 4 lines

arch/x86/virt/svm/sev.c +3 -0

+		pr_err("Failed to allocate RMPOPT cpumask\n");
+		return;
+	}

[ ... ]

drivers/crypto/ccp/sev-dev.c static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid) · line 1635

Quoted 11 lines

drivers/crypto/ccp/sev-dev.c +3 -0

index f833cb7e4da36..0b40d23a26b45 100644
@@ -1630,6 +1630,9 @@ static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
 	}
 
 	snp_hv_fixed_pages_state_update(sev, HV_FIXED);
+
+	snp_setup_rmpopt();
+

[Severity: High]
If the SEV-SNP firmware initialization fails, does this leave CPU hotplug permanently disabled?

A global cpu_hotplug_disable() call is made inside snp_prepare() right before we reach this code. If the subsequent firmware command fails in __sev_snp_init_locked(), it returns the error directly without cleaning up:

drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked() {
...

	rc = __sev_do_cmd_locked(cmd, arg, error);
	if (rc) {
		dev_err(sev->dev, "SEV-SNP: %s failed rc %d, error %#x\n",
			cmd == SEV_CMD_SNP_INIT_EX ? "SNP_INIT_EX" : "SNP_INIT",
			rc, *error);
		return rc;
	}

...
}

Since sev->snp_initialized remains false on this error path, snp_shutdown() is never executed on teardown, meaning cpu_hotplug_enable() is never invoked to release the lock.

Quoted 4 lines

drivers/crypto/ccp/sev-dev.c +0 -0

 	sev->snp_initialized = true;
 	dev_dbg(sev->dev, "SEV-SNP firmware initialized, SEV-TIO is %s\n",
 		data.tio_en ? "enabled" : "disabled");

arch/x86/virt/svm/sev.c bool snp_probe_rmptable_info(void) · line 766

Quoted 10 lines · Ashish Kalra

arch/x86/virt/svm/sev.c +3 -2

@@ -705,10 +760,12 @@ bool snp_probe_rmptable_info(void)
 	if (cpu_feature_enabled(X86_FEATURE_SEGMENTED_RMP))
 		rdmsrq(MSR_AMD64_RMP_CFG, rmp_cfg);
 
-	if (rmp_cfg & MSR_AMD64_SEG_RMP_ENABLED)
+	if (rmp_cfg & MSR_AMD64_SEG_RMP_ENABLED) {
 		return probe_segmented_rmptable_info();
-	else
+	} else {
+		setup_clear_cpu_cap(X86_FEATURE_RMPOPT);

That looks a bit squirreled away in a probing function. Let's make that a bit more obvious:

bool snp_probe_rmptable_info(void)

{
        if (cpu_feature_enabled(X86_FEATURE_SEGMENTED_RMP)) {
                rdmsrq(MSR_AMD64_RMP_CFG, rmp_cfg);

                if (rmp_cfg & MSR_AMD64_SEG_RMP_ENABLED)
                        return probe_segmented_rmptable_info();
        } else {

                /* RMPOPT is supported only with a segmented RMP table. */
                setup_clear_cpu_cap(X86_FEATURE_RMPOPT);

                return probe_contiguous_rmptable_info();
        }       
}

I believe there can be cases where CPUID advertises X86_FEATURE_SEGMENTED_RMP but rmf_cfg doesn't set MSR_AMD64_SEG_RMP_ENABLED because otherwise checking rmp_cfg in addition to the feature bit makes no sense.

In that case we should attempt a probe_contiguous_rmptable_info() even for X86_FEATURE_SEGMENTED_RMP.

Essentially drop that "else" and do something like:

bool snp_probe_rmptable_info(void)

{
	if (cpu_feature_enabled(X86_FEATURE_SEGMENTED_RMP)) {
		rdmsrq(MSR_AMD64_RMP_CFG, rmp_cfg);

		if (rmp_cfg & MSR_AMD64_SEG_RMP_ENABLED)
			return probe_segmented_rmptable_info();
	}

	/*

	 * Segmented RMP is either not supported on the platform or
	 * is disabled by the firmware. RMPOPT is not not supported
	 * without segmented RMP.
	 */
	setup_clear_cpu_cap(X86_FEATURE_RMPOPT);

	return probe_contiguous_rmptable_info();
}

Quoted 26 lines · K Prateek Nayak

I believe there can be cases where CPUID advertises X86_FEATURE_SEGMENTED_RMP but rmf_cfg doesn't set MSR_AMD64_SEG_RMP_ENABLED because otherwise checking rmp_cfg in addition to the feature bit makes no sense.

In that case we should attempt a probe_contiguous_rmptable_info() even for X86_FEATURE_SEGMENTED_RMP.

Essentially drop that "else" and do something like:

bool snp_probe_rmptable_info(void)

{
	if (cpu_feature_enabled(X86_FEATURE_SEGMENTED_RMP)) {
		rdmsrq(MSR_AMD64_RMP_CFG, rmp_cfg);

		if (rmp_cfg & MSR_AMD64_SEG_RMP_ENABLED)
			return probe_segmented_rmptable_info();
	}

	/*
	 * Segmented RMP is either not supported on the platform or
	 * is disabled by the firmware. RMPOPT is not not supported
	 * without segmented RMP.
	 */
	setup_clear_cpu_cap(X86_FEATURE_RMPOPT);

Quoted 2 lines

	return probe_contiguous_rmptable_info();
}

That makes sense.

Quoted 8 lines

drivers/crypto/ccp/sev-dev.c +0 -0

index ca473ca198b8..c002a7ca26a8 100644
@@ -1477,6 +1477,9 @@ static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
 	}
 

drivers/crypto/ccp/sev-dev.c static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid) · line 1481

Quoted 3 lines

drivers/crypto/ccp/sev-dev.c +2 -0

 	snp_hv_fixed_pages_state_update(sev, HV_FIXED);
+
+	snp_setup_rmpopt();

Why is this thing here and not at the end?

SEV-TIO is supposed to always run last or it doesn't matter?

No hard dependency — it only needs SNP initialized (post SNP_INIT_EX/DF_FLUSH), and the initial pass is async anyway, so it's independent of the SEV-TIO block and the panic notifier. So i can move it to the end (after the TIO block so it runs last without touching TIO ordering), but is there a specific reason you want me to move it to the end, as it can just stay here after SNP_INIT.

From: Ashish Kalra <[email protected]>

The new RMPOPT instruction helps manage per-CPU RMP optimization structures inside the CPU. It takes a 1GB-aligned physical address and either returns the status of the optimizations or tries to enable the optimizations.

Per-CPU RMPOPT tables support at most 2 TB of addressable memory for RMP optimizations.

Initialize the per-CPU RMPOPT table base to the starting physical address. This enables RMP optimization for up to 2 TB of system RAM on all CPUs.

Additionally, add support to setup and enable RMPOPT once SNP is enabled and initialized.

Suggested-by Thomas Lendacky <[email protected]>

Suggested-by Dave Hansen <[email protected]>

Suggested-by Borislav Petkov (AMD) <[email protected]>

Suggested-by K Prateek Nayak <[email protected]>

Reviewed-by Dave Hansen <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

---
 arch/x86/include/asm/msr-index.h |  3 ++
 arch/x86/include/asm/sev.h       |  2 +
 arch/x86/virt/svm/sev.c          | 67 +++++++++++++++++++++++++++++---
 drivers/crypto/ccp/sev-dev.c     |  3 ++
 4 files changed, 70 insertions(+), 5 deletions(-)

arch/x86/include/asm/msr-index.h +3 -0

index 18c4be75e927..d2cb0a7cd0a2 100644
@@ -761,6 +761,9 @@
 #define MSR_AMD64_SEG_RMP_ENABLED_BIT	0
 #define MSR_AMD64_SEG_RMP_ENABLED	BIT_ULL(MSR_AMD64_SEG_RMP_ENABLED_BIT)
 #define MSR_AMD64_RMP_SEGMENT_SHIFT(x)	(((x) & GENMASK_ULL(13, 8)) >> 8)
+#define MSR_AMD64_RMPOPT_BASE		0xc0010139
+#define MSR_AMD64_RMPOPT_ENABLE_BIT	0
+#define MSR_AMD64_RMPOPT_ENABLE		BIT_ULL(MSR_AMD64_RMPOPT_ENABLE_BIT)
 
 #define MSR_SVSM_CAA			0xc001f000
 

arch/x86/include/asm/sev.h +2 -0

index 594cfa19cbd4..6fd72a44a51e 100644
@@ -662,6 +662,7 @@ static inline void snp_leak_pages(u64 pfn, unsigned int pages)
 	__snp_leak_pages(pfn, pages, true);
 }
 int snp_prepare(void);
+void snp_setup_rmpopt(void);
 void snp_shutdown(void);
 #else
 static inline bool snp_probe_rmptable_info(void) { return false; }

@@ -680,6 +681,7 @@ static inline void snp_leak_pages(u64 pfn, unsigned int npages) {}
 static inline void kdump_sev_callback(void) { }
 static inline void snp_fixup_e820_tables(void) {}
 static inline int snp_prepare(void) { return -ENODEV; }
+static inline void snp_setup_rmpopt(void) {}
 static inline void snp_shutdown(void) {}
 #endif
 

arch/x86/virt/svm/sev.c +14 -0

index e2f69fba0938..8bfd80284836 100644
@@ -124,6 +124,9 @@ 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 LIST_HEAD(snp_leaked_pages_list);
 static DEFINE_SPINLOCK(snp_leaked_pages_list_lock);
 

@@ -558,6 +561,17 @@ int snp_prepare(void)
 }
 EXPORT_SYMBOL_FOR_MODULES(snp_prepare, "ccp");
 
+static void snp_cleanup_rmpopt(void)
+{
+	int cpu;
+
+	for_each_cpu(cpu, rmpopt_cpumask)
+		wrmsrq_on_cpu(cpu, MSR_AMD64_RMPOPT_BASE, 0);
+
+	free_cpumask_var(rmpopt_cpumask);
+	rmpopt_pa_start = 0;
+}
+
 void snp_shutdown(void)
 {
 	u64 syscfg;

@@ -567,10 +581,11 @@ void snp_shutdown(void)
 		return;

arch/x86/virt/svm/sev.c +44 -2

 	cpu_hotplug_enable();
 
 	clear_rmp();

@@ -578,6 +593,46 @@ void snp_shutdown(void)
 }
 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);
+}
+
+void snp_setup_rmpopt(void)
+{
+	u64 rmpopt_base;
+	int cpu;
+
+	if (!rmpopt_capable())
+		return;
+
+	if (!zalloc_cpumask_var(&rmpopt_cpumask, GFP_KERNEL)) {
+		pr_err("Failed to allocate RMPOPT cpumask\n");
+		return;
+	}
+
+	/*
+	 * The RMPOPT_BASE MSR is per-core, so only one thread per core needs
+	 * to set up the RMPOPT_BASE MSR.  All primary threads are online,
+	 * otherwise SNP would not have been enabled.
+	 */
+	for_each_online_cpu(cpu)
+		if (topology_is_primary_thread(cpu))
+			cpumask_set_cpu(cpu, rmpopt_cpumask);
+
+	rmpopt_pa_start = ALIGN_DOWN(PFN_PHYS(min_low_pfn), SZ_1G);
+	rmpopt_base = rmpopt_pa_start | MSR_AMD64_RMPOPT_ENABLE;
+
+	/*
+	 * Per-CPU RMPOPT tables cover at most 2 TB.  Program each core's
+	 * RMPOPT_BASE with the start of RAM to optimize up to 2 TB.
+	 */
+	for_each_cpu(cpu, rmpopt_cpumask)
+		wrmsrq_on_cpu(cpu, MSR_AMD64_RMPOPT_BASE, rmpopt_base);
+}
+EXPORT_SYMBOL_FOR_MODULES(snp_setup_rmpopt, "ccp");
+
 /*
  * Do the necessary preparations which are verified by the firmware as
  * described in the SNP_INIT_EX firmware command description in the SNP

@@ -705,10 +760,12 @@ bool snp_probe_rmptable_info(void)
 	if (cpu_feature_enabled(X86_FEATURE_SEGMENTED_RMP))
 		rdmsrq(MSR_AMD64_RMP_CFG, rmp_cfg);
 
-	if (rmp_cfg & MSR_AMD64_SEG_RMP_ENABLED)
+	if (rmp_cfg & MSR_AMD64_SEG_RMP_ENABLED) {
 		return probe_segmented_rmptable_info();
-	else
+	} else {
+		setup_clear_cpu_cap(X86_FEATURE_RMPOPT);
 		return probe_contiguous_rmptable_info();
+	}
 }
 
 /*

drivers/crypto/ccp/sev-dev.c +3 -0

index ca473ca198b8..c002a7ca26a8 100644
@@ -1477,6 +1477,9 @@ static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
 	}
 
 	snp_hv_fixed_pages_state_update(sev, HV_FIXED);
+
+	snp_setup_rmpopt();
+
 	sev->snp_initialized = true;
 	dev_dbg(sev->dev, "SEV-SNP firmware initialized, SEV-TIO is %s\n",
 		data.tio_en ? "enabled" : "disabled");

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 <[email protected]>

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.

Quoted 4 lines

arch/x86/virt/svm/sev.c +0 -0

index 8bfd802848361..04b19e64f8329 100644

[ ... ]

arch/x86/virt/svm/sev.c static bool rmpopt_capable(void) · line 636

Quoted 8 lines

arch/x86/virt/svm/sev.c +8 -0

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

Quoted 3 lines

arch/x86/virt/svm/sev.c +3 -0

+
+	return optimized;
+}

[ ... ]

Quoted 2 lines

arch/x86/virt/svm/sev.c +2 -0

+static void rmpopt_work_handler(struct work_struct *work)
+{

[ ... ]

arch/x86/virt/svm/sev.c static bool rmpopt_capable(void) · line 697

Quoted 8 lines

arch/x86/virt/svm/sev.c +8 -0

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

arch/x86/virt/svm/sev.c static bool rmpopt_capable(void) · line 708

Quoted 11 lines

arch/x86/virt/svm/sev.c +11 -0

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

Quoted 3 lines

arch/x86/virt/svm/sev.c +3 -0

+
+	free_cpumask_var(follower_mask);
+}

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

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.

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

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.

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(-)

arch/x86/virt/svm/sev.c +1 -0

index 8bfd80284836..04b19e64f832 100644
@@ -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>

arch/x86/virt/svm/sev.c +5 -0

+#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);

arch/x86/virt/svm/sev.c +6 -1

+
+	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);
 }
 

arch/x86/virt/svm/sev.c +10 -0

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

Quoted 4 lines · Ashish Kalra

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.

Quoted 7 lines

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.

Quoted 6 lines

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?

Quoted 5 lines

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.

Quoted 23 lines

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(-)

arch/x86/virt/svm/sev.c +1 -0

index 8bfd80284836..04b19e64f832 100644
@@ -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>

arch/x86/virt/svm/sev.c static void *rmp_bookkeeping __ro_after_init; · line 131

Quoted 7 lines

arch/x86/virt/svm/sev.c +3 -1

@@ -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

arch/x86/virt/svm/sev.c static void *rmp_bookkeeping __ro_after_init; · line 133

Quoted 2 lines

arch/x86/virt/svm/sev.c +2 -0

+	RMPOPT_FUNC_VERIFY_AND_REPORT_STATUS,
+	RMPOPT_FUNC_REPORT_STATUS

RMPOPT_OP_VERIFY...

arch/x86/virt/svm/sev.c static void *rmp_bookkeeping __ro_after_init; · line 135

Quoted 2 lines

arch/x86/virt/svm/sev.c +2 -0

+};
+

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

Quoted 9 lines

arch/x86/virt/svm/sev.c +5 -0

+#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);

arch/x86/virt/svm/sev.c static void snp_cleanup_rmpopt(void) · line 583

Quoted 7 lines

arch/x86/virt/svm/sev.c +4 -0

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

Quoted 19 lines

arch/x86/virt/svm/sev.c +6 -1

+
+	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);
 }
 

arch/x86/virt/svm/sev.c static bool rmpopt_capable(void) · line 632

Quoted 10 lines

arch/x86/virt/svm/sev.c +10 -0

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

arch/x86/virt/svm/sev.c static bool rmpopt_capable(void) · line 644

Quoted 12 lines

arch/x86/virt/svm/sev.c +12 -0

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

arch/x86/virt/svm/sev.c static bool rmpopt_capable(void) · line 646

Quoted 2 lines

arch/x86/virt/svm/sev.c +2 -0

+
+	__rmpopt(pa_start, op_type);

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

arch/x86/virt/svm/sev.c static bool rmpopt_capable(void) · line 652

Quoted 6 lines

arch/x86/virt/svm/sev.c +6 -0

+}
+
+/*
+ * '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...

arch/x86/virt/svm/sev.c static bool rmpopt_capable(void) · line 660

Quoted 8 lines

arch/x86/virt/svm/sev.c +8 -0

+{
+	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().

arch/x86/virt/svm/sev.c static bool rmpopt_capable(void) · line 664

Quoted 5 lines

arch/x86/virt/svm/sev.c +4 -0

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

arch/x86/virt/svm/sev.c static bool rmpopt_capable(void) · line 668

Quoted 5 lines

arch/x86/virt/svm/sev.c +4 -0

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

arch/x86/virt/svm/sev.c static bool rmpopt_capable(void) · line 673

Quoted 5 lines

arch/x86/virt/svm/sev.c +4 -0

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

Quoted 6 lines

arch/x86/virt/svm/sev.c +6 -0

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

arch/x86/virt/svm/sev.c static bool rmpopt_capable(void) · line 685

Quoted 6 lines

arch/x86/virt/svm/sev.c +6 -0

+	 *
+	 * 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?

arch/x86/virt/svm/sev.c static bool rmpopt_capable(void) · line 688

Quoted 5 lines

arch/x86/virt/svm/sev.c +2 -0

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

Use migrate_disable() rather than get_cpu() to

arch/x86/virt/svm/sev.c +1 -0

+	 * prevent migration while still allowing preemption.

No need to explain that.

Quoted 10 lines

arch/x86/virt/svm/sev.c +10 -0

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

arch/x86/virt/svm/sev.c static bool rmpopt_capable(void) · line 708

Quoted 10 lines

arch/x86/virt/svm/sev.c +10 -0

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

arch/x86/virt/svm/sev.c static bool rmpopt_capable(void) · line 711

Quoted 3 lines

arch/x86/virt/svm/sev.c +3 -0

+
+	free_cpumask_var(follower_mask);
+}

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

Thx.

Quoted 22 lines · Borislav Petkov

Quoted 9 lines

arch/x86/virt/svm/sev.c +9 -0

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

arch/x86/virt/svm/sev.c static bool rmpopt_capable(void) · line 708

Quoted 10 lines

arch/x86/virt/svm/sev.c +10 -0

+	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?!

Quoted 4 lines

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

Nope.

You're right — an IPI per 1 GB is far too many. Will restructure to a single IPI per follower core: a new on_each_cpu() callback can loop over the whole range on the CPU it runs on. The leader will call it directly (migrate-disabled) to populate the RMP scan cache, then one on_each_cpu_mask() will run it on the remaining cores.

This will also fold nicely with the earlier cleanup: __rmpopt() getting merged into rmpopt().

One important tradeoff to be aware of: each follower IPI handler will now run a 2048-iteration loop with IRQs disabled — but followers are RMP-scan cache hits (the leader populated the cache), so each rmpopt() there is cheap, and this only runs at setup and guest-teardown re-optimization time.

Followers are RMP-scan cache hits (cheap) and this only runs at setup/teardown

Quoted 1 line · Ashish

Followers are RMP-scan cache hits (cheap) and this only runs at setup/teardown

How?

I see the workqueue running periodically. And in talking about periodically, RMPOPT_WORK_TIMEOUT is not used anywhere.

What's up?

RMPOPT_WORK_TIMEOUT is used by snp_rmpopt_all_physmem() (mod_delayed_work()), which is added in the next patch — I'll move the define there so each patch is self-contained.

It isn't periodic either: the handler doesn't re-arm itself. The work runs at setup and re-init with queue_delayed_work(), and then per SNP-guest teardown via mod_delayed_work() (batched by the timeout) once the later patches wire that up.

Quoted 2 lines · Ashish

RMPOPT_WORK_TIMEOUT is used by snp_rmpopt_all_physmem() (mod_delayed_work()), which is added in the next patch — I'll move the define there so each patch is self-contained.

Yes, very much so.

Quoted 2 lines

It isn't periodic either: the handler doesn't re-arm itself. The work runs at setup and re-init with queue_delayed_work(), and then per SNP-guest teardown via mod_delayed_work() (batched by the timeout) once the later patches wire that up.

Then I am missing an important aspect here: RMPOPT optimizes RMP checks but then what's the point if you do this only on SNP guest teardown and I'm assuming at guest init too?

What happens to long-running guests which never get restarted?

This would lead to the system fragmenting ad absurdum and us never optimizing the RMP table practically.

But the guest could, in its lifetime, free up a contiguous 1G. When are we optimizing this one then?

Is that what the sentence about optimizing during 1GB page cleanup in your 0th message all about?

In any case, your commit message needs to talk about when those optimization runs are happening and why.

Thx.

yes, that's what the "optimizing during 1 GB page cleanup" sentence is about:

Quoted 1 line · Ashish

yes, that's what the "optimizing during 1 GB page cleanup" sentence is about:

What about doing the optimization runs periodically?

Periodic re-optimization was already considered in an earlier RMPOPT series

and dropped because of being too disruptive — a timer-based scan would run on all cores every time it fires, even when no memory has been freed.

Running it at guest teardown (batched by the timeout) is a better fit:
teardown is when large contiguous regions actually go back to the hypervisor, so the scan only runs when there's real work to do instead of on a fixed timer. The case of a guest freeing a 1 GB region mid-life is then handled by the 1 GB-hugetlb page-cleanup follow-on, not by a periodic scan.

I will also like to avoid looping on this — periodic vs teardown was already worked through in an earlier revision, and this is where we landed. Happy to revisit if there's a specific new concern, but otherwise I'd prefer to keep the current approach.

Quoted 1 line · Ashish

Periodic re-optimization was already considered in an earlier RMPOPT series

Link?

Quoted 3 lines

and dropped because of being too disruptive — a timer-based scan would run on all cores every time it fires, even when no memory has been freed.

What if it ran very seldomly, say once an hour?

Quoted 5 lines

Running it at guest teardown (batched by the timeout) is a better fit:
teardown is when large contiguous regions actually go back to the hypervisor, so the scan only runs when there's real work to do instead of on a fixed timer. The case of a guest freeing a 1 GB region mid-life is then handled by the 1 GB-hugetlb page-cleanup follow-on, not by a periodic scan.

Are you saying a periodic scan won't handle anything?

Because if it catches something, it will handle that something.

Quoted 5 lines

I will also like to avoid looping on this — periodic vs teardown was already worked through in an earlier revision, and this is where we landed. Happy to revisit if there's a specific new concern, but otherwise I'd prefer to keep the current approach.

Sorry, this is not how upstream review works.

- Under churn (many SNP guests terminating and respawning), an hourly scan is too coarse — freed regions would sit un-optimized for up to an hour. The teardown trigger re-optimizes right after each burst of terminations (batched by the timeout), so it's more responsive than a periodic scan exactly when there's the most optimization work to do.

- When no SNP guests run, a periodic scan achieves nothing: the initial pass already optimized all of memory and nothing has changed, so it just re-scans physmem for zero benefit.

So the event-driven approach self-scales with real work — it runs as often as memory is actually freed and not at all when the system is idle — whereas a fixed timer is both too slow under load and pure overhead when idle. The mid-life contiguous-1 GB case is then caught precisely by the 1 GB-hugetlb page-cleanup hook when that lands, rather than by a coarse sweep.

Quoted 5 lines · Ashish

- Under churn (many SNP guests terminating and respawning), an hourly scan is too coarse — freed regions would sit un-optimized for up to an hour. The teardown trigger re-optimizes right after each burst of terminations (batched by the timeout), so it's more responsive than a periodic scan exactly when there's the most optimization work to do.

Yes.

Quoted 3 lines

- When no SNP guests run, a periodic scan achieves nothing: the initial pass already optimized all of memory and nothing has changed, so it just re-scans physmem for zero benefit.

Well, no. You won't even run that when there are no SNP guests present.

Quoted 5 lines

So the event-driven approach self-scales with real work — it runs as often as memory is actually freed and not at all when the system is idle — whereas a fixed timer is both too slow under load and pure overhead when idle. The mid-life contiguous-1 GB case is then caught precisely by the 1 GB-hugetlb page-cleanup hook when that lands, rather than by a coarse sweep.

I wanted to have this in case pages do get freed up from time to time and you get some sort of a fragmentation. But I'm being told that we don't support balooning yet, neither memory hotplug and when the pages get freed, they remain guest-owned. So yes, the points in time where those page become HV-owned and potentially freed, then we should optimize.

Please hold down that in the commit message so that it is clear for future readers why we did it this way.

Quoted 2 lines

and dropped because of being too disruptive — a timer-based scan would run on all cores every time it fires, even when no memory has been freed.

What if it ran very seldomly, say once an hour?

Quoted 5 lines

Running it at guest teardown (batched by the timeout) is a better fit:
teardown is when large contiguous regions actually go back to the hypervisor, so the scan only runs when there's real work to do instead of on a fixed timer. The case of a guest freeing a 1 GB region mid-life is then handled by the 1 GB-hugetlb page-cleanup follow-on, not by a periodic scan.

Are you saying a periodic scan won't handle anything?

A periodic scan could be useful but only if it coincides with a VM teardown event, or other event that opens up new RMP optimizations. I _think_ we decided that with current upstream code, the only event that matters is VM teardown.

Yes, the checks are done on *all* memory but for ensuring the integrity of SNP guest memory, so that is what the above paragraph is mentioning.

The other way i can put it is: "RMP checks are skipped for 1-GB ranges that don't contain SNP guest memory and as SNP guests are launched, RMPUPDATE disables the corresponding optimizations".

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

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

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(-)

arch/x86/virt/svm/sev.c +1 -0

index 8bfd80284836..04b19e64f832 100644
@@ -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;
 

arch/x86/virt/svm/sev.c +149 -1

+
+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;
+
+	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;
+
+	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;
+
+	__rmpopt(pa_start, op_type);
+}
+
+/*
+ * 'val' is a system physical address.
+ */
+static void rmpopt_smp(void *val)
+{
+	rmpopt((u64)val);
+}
+
+/*
+ * RMPOPT optimizations skip RMP checks at 1GB granularity if this
+ * range of memory does not contain any SNP guest memory.
+ */
+static void rmpopt_work_handler(struct work_struct *work)
+{
+	cpumask_var_t follower_mask;
+	phys_addr_t 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);
+
+	if (!alloc_cpumask_var(&follower_mask, GFP_KERNEL)) {
+		pr_warn("RMP optimization pass skipped: cpumask allocation failed\n");
+		return;
+	}
+
+	/*
+	 * 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
+	 * 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.
+	 */
+	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);
+
+	free_cpumask_var(follower_mask);
+}
+
 void snp_setup_rmpopt(void)
 {
 	u64 rmpopt_base;

@@ -607,8 +718,38 @@ void snp_setup_rmpopt(void)
 	if (!rmpopt_capable())
 		return;
 
+	guard(mutex)(&rmpopt_wq_mutex);
+
+	/*
+	 * On re-initialization after a legacy SNP shutdown (SNP_SHUTDOWN_EX
+	 * with x86_snp_shutdown=0), snp_shutdown() and thus snp_cleanup_rmpopt()
+	 * are skipped, so the workqueue, delayed work, cpumask and per-CPU
+	 * RMPOPT_BASE MSRs are still set up and valid (SnpEn stayed set and
+	 * CPU hotplug stayed disabled).  Rather than re-doing the setup, which
+	 * would leak the existing state, just re-queue the optimization pass
+	 * to re-optimize any memory the previous SNP session de-optimized.
+	 */
+	if (rmpopt_wq) {
+		queue_delayed_work(rmpopt_wq, &rmpopt_delayed_work, 0);
+		return;
+	}
+
+	/*
+	 * Create an RMPOPT-specific workqueue to avoid scheduling
+	 * RMPOPT workitem on the global system workqueue.
+	 */
+	rmpopt_wq = alloc_workqueue("rmpopt_wq", WQ_UNBOUND, 1);
+	if (!rmpopt_wq) {
+		pr_err("Failed to allocate RMPOPT workqueue\n");
+		return;
+	}
+
+	INIT_DELAYED_WORK(&rmpopt_delayed_work, rmpopt_work_handler);
+
 	if (!zalloc_cpumask_var(&rmpopt_cpumask, GFP_KERNEL)) {
 		pr_err("Failed to allocate RMPOPT cpumask\n");
+		destroy_workqueue(rmpopt_wq);
+		rmpopt_wq = NULL;
 		return;
 	}
 

@@ -630,6 +771,21 @@ void snp_setup_rmpopt(void)
 	 */
 	for_each_cpu(cpu, rmpopt_cpumask)
 		wrmsrq_on_cpu(cpu, MSR_AMD64_RMPOPT_BASE, rmpopt_base);
+
+	rmpopt_pa_end = ALIGN(PFN_PHYS(max_pfn), SZ_1G);
+
+	/* Limit memory scanning to 2TB of RAM */
+	if ((rmpopt_pa_end - rmpopt_pa_start) > SZ_2T) {
+		pr_info("RMPOPT coverage limited to 2TB; memory above 0x%llx not optimized\n",
+			rmpopt_pa_start + SZ_2T);
+		rmpopt_pa_end = rmpopt_pa_start + SZ_2T;
+	}
+
+	/*
+	 * Once all per-CPU RMPOPT tables have been configured, enable RMPOPT
+	 * optimizations on all physical memory.
+	 */
+	queue_delayed_work(rmpopt_wq, &rmpopt_delayed_work, 0);
 }
 EXPORT_SYMBOL_FOR_MODULES(snp_setup_rmpopt, "ccp");
 

From: Ashish Kalra <[email protected]>

RMPOPT table is a per-CPU table which indicates if 1GB regions of physical memory are entirely hypervisor-owned or not.

When performing host memory accesses in hypervisor mode as well as non-SNP guest mode, the processor may consult the RMPOPT table to potentially skip an RMP access and improve performance.

Normal guest events clear RMP optimizations: pages are converted from

shared to private as SNP guests are launched, and large pages are split and collapsed during guest operation -- both clear the RMPOPT optimizations for the affected 1GB regions.

Conversely, guest pages are

converted back to shared during SNP guest termination, so those regions may become eligible for RMPOPT optimization again.

Without some intervention, all RMP optimizations would eventually be lost. Add an interface to re-optimize all of physical memory.

The interface uses mod_delayed_work() instead of queue_delayed_work() so that the delay timer is reset on each call. This provides proper batching semantics: re-optimization runs 10 seconds after the *last* VM termination rather than after the first. mod_delayed_work() also re-queues work that is already in-flight, so a re-scan request during an active scan is not silently dropped.