Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support

From: Paolo Bonzini <[email protected]>

svm_recalc_instruction_intercepts() is always called at least once before the vCPU is started, so the setting or clearing of the RDTSCP intercept can be dropped from the TSC_AUX virtualization support.

Extracted from a patch by Tom Lendacky.

Cc [email protected]

Fixes 296d5a17e793 ("KVM: SEV-ES: Use V_TSC_AUX if available instead of RDTSC/MSR_TSC_AUX intercepts")

Signed-off-by Paolo Bonzini <[email protected]>

(cherry picked from commit e8d93d5d93f85949e7299be289c6e7e1154b2f78)

Signed-off-by Michael Roth <[email protected]>

---
 arch/x86/kvm/svm/sev.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Quoted 8 lines · Michael Roth

From: Paolo Bonzini <[email protected]>

svm_recalc_instruction_intercepts() is always called at least once before the vCPU is started, so the setting or clearing of the RDTSCP intercept can be dropped from the TSC_AUX virtualization support.

Extracted from a patch by Tom Lendacky.

Quoted 8 lines

Cc [email protected]

Fixes 296d5a17e793 ("KVM: SEV-ES: Use V_TSC_AUX if available instead of RDTSC/MSR_TSC_AUX intercepts")

Signed-off-by Paolo Bonzini <[email protected]>

(cherry picked from commit e8d93d5d93f85949e7299be289c6e7e1154b2f78)

Signed-off-by Michael Roth <[email protected]>

---
  arch/x86/kvm/svm/sev.c | 5 +----
  1 file changed, 1 insertion(+), 4 deletions(-)

What stable tree(s) are you wanting this applied to (same for the others in this series)? It's already in the 6.1.56 release, and the Fixes tag is for 5.19, so I don't see where it could be missing from?

Quoted 18 lines · Greg KH

Quoted 16 lines · Michael Roth

From: Paolo Bonzini <[email protected]>

svm_recalc_instruction_intercepts() is always called at least once before the vCPU is started, so the setting or clearing of the RDTSCP intercept can be dropped from the TSC_AUX virtualization support.

Extracted from a patch by Tom Lendacky.

Cc [email protected]

Fixes 296d5a17e793 ("KVM: SEV-ES: Use V_TSC_AUX if available instead of RDTSC/MSR_TSC_AUX intercepts")

Signed-off-by Paolo Bonzini <[email protected]>

(cherry picked from commit e8d93d5d93f85949e7299be289c6e7e1154b2f78)

Signed-off-by Michael Roth <[email protected]>

---
  arch/x86/kvm/svm/sev.c | 5 +----
  1 file changed, 1 insertion(+), 4 deletions(-)

Quoted 3 lines

What stable tree(s) are you wanting this applied to (same for the others in this series)? It's already in the 6.1.56 release, and the Fixes tag is for 5.19, so I don't see where it could be missing from?

I tink it's missing in the (destined for 6.7) tree that Michael is basing this series on, so he's cherry picking it from Linus's tree.

Paolo

Address this by disabling intercepts of MSR_IA32_XSS for SEV-ES guests if the host/guest configuration allows it. If the host/guest configuration doesn't allow for MSR_IA32_XSS, leave it intercepted so that it can be caught by the existing checks in kvm_{set,get}_msr_common() if the guest still attempts to access it.

Quoted 5 lines · Michael Roth

Address this by disabling intercepts of MSR_IA32_XSS for SEV-ES guests if the host/guest configuration allows it. If the host/guest configuration doesn't allow for MSR_IA32_XSS, leave it intercepted so that it can be caught by the existing checks in kvm_{set,get}_msr_common() if the guest still attempts to access it.

This is wrong, because it allows the guest to do untrapped writes to MSR_IA32_XSS and therefore (via XRSTORS) to MSRs that the host might not save or restore.

If the processor cannot let the host validate writes to MSR_IA32_XSS, KVM simply cannot expose XSAVES to SEV-ES (and SEV-SNP) guests.

Because SVM doesn't provide a way to disable just XSAVES in the guest, all that KVM can do is keep on trapping MSR_IA32_XSS (which the guest shouldn't read or write to). In other words the crash on accesses to MSR_IA32_XSS is not a bug but a feature (of the hypervisor, that wants/needs to protect itself just as much as the guest wants to).

The bug is that there is no API to tell userspace "do not enable this and that CPUID for SEV guests", there is only the extremely limited KVM_GET_SUPPORTED_CPUID system ioctl.

For now, all we can do is document our wishes, with which userspace had better comply. Please send a patch to QEMU that makes it obey.

Quoted 2 lines

For now, all we can do is document our wishes, with which userspace had better comply. Please send a patch to QEMU that makes it obey.

Discussed this early today with Paolo at PUCK and pointed out that (a) the CPU context switches the underlying state, (b) SVM doesn't allow intercepting*just* XSAVES, and (c) SNP's AP creation can bypass XSS interception.

So while we all (all == KVM folks) agree that this is rather terrifying, e.g. gives KVM zero option if there is a hardware issue, it's "fine" to let the guest use XSAVES/XSS.

From: Brijesh Singh <[email protected]>

Add CPU feature detection for Secure Encrypted Virtualization with Secure Nested Paging. This feature adds a strong memory integrity protection to help prevent malicious hypervisor-based attacks like data replay, memory re-mapping, and more.

Signed-off-by Brijesh Singh <[email protected]>

Signed-off-by Jarkko Sakkinen <[email protected]>

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

Signed-off-by Michael Roth <[email protected]>

---
  arch/x86/include/asm/cpufeatures.h       | 1 +
  arch/x86/kernel/cpu/amd.c                | 5 +++--
  tools/arch/x86/include/asm/cpufeatures.h | 1 +
  3 files changed, 5 insertions(+), 2 deletions(-)

arch/x86/include/asm/cpufeatures.h +1 -0

index 58cb9495e40f..1640cedd77f1 100644
@@ -437,6 +437,7 @@
  #define X86_FEATURE_SEV			(19*32+ 1) /* AMD Secure Encrypted Virtualization */
  #define X86_FEATURE_VM_PAGE_FLUSH	(19*32+ 2) /* "" VM Page Flush MSR is supported */
  #define X86_FEATURE_SEV_ES		(19*32+ 3) /* AMD Secure Encrypted Virtualization - Encrypted State */
+#define X86_FEATURE_SEV_SNP		(19*32+ 4) /* AMD Secure Encrypted Virtualization - Secure Nested Paging */
  #define X86_FEATURE_V_TSC_AUX		(19*32+ 9) /* "" Virtual TSC_AUX */
  #define X86_FEATURE_SME_COHERENT	(19*32+10) /* "" AMD hardware-enforced cache coherency */
  #define X86_FEATURE_DEBUG_SWAP		(19*32+14) /* AMD SEV-ES full debug state swap support */

arch/x86/kernel/cpu/amd.c +3 -2

index dd8379d84445..14ee7f750cc7 100644
@@ -630,8 +630,8 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
  	 *	      SME feature (set in scattered.c).
  	 *	      If the kernel has not enabled SME via any means then
  	 *	      don't advertise the SME feature.
-	 *   For SEV: If BIOS has not enabled SEV then don't advertise the
-	 *            SEV and SEV_ES feature (set in scattered.c).
+	 *   For SEV: If BIOS has not enabled SEV then don't advertise SEV and
+	 *	      any additional functionality based on it.
  	 *
  	 *   In all cases, since support for SME and SEV requires long mode,
  	 *   don't advertise the feature under CONFIG_X86_32.

@@ -666,6 +666,7 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
  clear_sev:
  		setup_clear_cpu_cap(X86_FEATURE_SEV);
  		setup_clear_cpu_cap(X86_FEATURE_SEV_ES);
+		setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
  	}
  }
 

tools/arch/x86/include/asm/cpufeatures.h +1 -0

index 798e60b5454b..669f45eefa0c 100644
@@ -432,6 +432,7 @@
  #define X86_FEATURE_SEV			(19*32+ 1) /* AMD Secure Encrypted Virtualization */
  #define X86_FEATURE_VM_PAGE_FLUSH	(19*32+ 2) /* "" VM Page Flush MSR is supported */
  #define X86_FEATURE_SEV_ES		(19*32+ 3) /* AMD Secure Encrypted Virtualization - Encrypted State */
+#define X86_FEATURE_SEV_SNP		(19*32+ 4) /* AMD Secure Encrypted Virtualization - Secure Nested Paging */
  #define X86_FEATURE_V_TSC_AUX		(19*32+ 9) /* "" Virtual TSC_AUX */
  #define X86_FEATURE_SME_COHERENT	(19*32+10) /* "" AMD hardware-enforced cache coherency */
  #define X86_FEATURE_DEBUG_SWAP		(19*32+14) /* AMD SEV-ES full debug state swap support */

Quoted 7 lines · Michael Roth

From: Brijesh Singh <[email protected]>

Add CPU feature detection for Secure Encrypted Virtualization with Secure Nested Paging. This feature adds a strong memory integrity protection to help prevent malicious hypervisor-based attacks like data replay, memory re-mapping, and more.

Quoted 4 lines

Signed-off-by Brijesh Singh <[email protected]>

Signed-off-by Jarkko Sakkinen <[email protected]>

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

Signed-off-by Michael Roth <[email protected]>

Queued, thanks.

Quoted 13 lines · Paolo Bonzini

Quoted 11 lines · Michael Roth

From: Brijesh Singh <[email protected]>

Add CPU feature detection for Secure Encrypted Virtualization with Secure Nested Paging. This feature adds a strong memory integrity protection to help prevent malicious hypervisor-based attacks like data replay, memory re-mapping, and more.

Signed-off-by Brijesh Singh <[email protected]>

Signed-off-by Jarkko Sakkinen <[email protected]>

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

Signed-off-by Michael Roth <[email protected]>

Quoted 1 line

Queued, thanks.

Paolo, please stop queueing x86 patches through your tree. I'll give you an immutable branch with the x86 bits when the stuff has been reviewed.

Sure, I only queued it because you gave Acked-by for 05/50 and this is an obvious dependency. I would like to get things in as they are ready (whenever it makes sense), so if you want to include those two in the x86 tree for 6.8, that would work for me.

Quoted 4 lines

Sure, I only queued it because you gave Acked-by for 05/50 and this is an obvious dependency. I would like to get things in as they are ready (whenever it makes sense), so if you want to include those two in the x86 tree for 6.8, that would work for me.

It doesn't make sense to include them into 6.8 because the two alone are simply dead code in 6.8.

The plan is to put the x86 patches first in the next submission, I'll pick them up for 6.9 and then give you an immutable branch to apply the KVM bits ontop. This way it all goes together.

Why are they dead code? X86_FEATURE_SEV_SNP is set automatically based on CPUID, therefore patch 5 is a performance improvement on all processors that support SEV-SNP. This is independent of whether KVM can create SEV-SNP guests or not.

Quoted 4 lines · Paolo Bonzini

Why are they dead code? X86_FEATURE_SEV_SNP is set automatically based on CPUID, therefore patch 5 is a performance improvement on all processors that support SEV-SNP. This is independent of whether KVM can create SEV-SNP guests or not.

No, it is not. This CPUID bit means:

"RMP table can be enabled to protect memory even from hypervisor."

Without the SNP host patches, it is dead code.

And regardless, arch/x86/kvm/ patches go through the kvm tree. The rest of arch/x86/ through the tip tree. We've been over this a bunch of times already.

If you don't agree with this split, let's discuss it offlist with all tip and kvm maintainers, reach an agreement who picks up what and to put an end to this nonsense.

Thx.

Surely we can agree that cpu_feature_enabled(X86_FEATURE_SEV_SNP) has nothing to do with SEV-SNP host patches being present?

And that therefore retpolines are preferred even without any SEV-SNP support in KVM?

And can we agree that "Acked-by" means "feel free and take it if you wish,

I'm asking because I'm not sure if we agree on these two things, but they really seem basic to me?

Quoted 2 lines · Paolo Bonzini

Surely we can agree that cpu_feature_enabled(X86_FEATURE_SEV_SNP) has nothing to do with SEV-SNP host patches being present?

It does - we're sanitizing the meaning of a CPUID flag present in /proc/cpuinfo, see here:

https://git.kernel.org/tip/79c603ee43b2674fba0257803bab265147821955

Quoted 2 lines

And that therefore retpolines are preferred even without any SEV-SNP support in KVM?

No, automatic IBRS should be disabled when SNP is enabled. Not CPUID present - enabled.

We clear that bit on a couple of occasions in the SNP
host patchset if we determine that SNP host support is not possible so 4/50 needs to go together with the rest to mean something.

1) patch 4 should have unconditionally cleared the feature (until the initialization code comes around in patch 6); and it should have mentioned in the commit message that we don't want X86_FEATURE_SEV_SNP to be set, unless SNP can be enabled via MSR_AMD64_SYSCFG.

2) possibly, the commit message of patch 5 could have said something like "at this point in the kernel SNP is never enabled".

3) Patch 23 should have been placed before the SNP initialization, because as things stand the patches (mildly) break bisectability.

Understood now. With the patch ordering and commit message edits I suggested above, indeed I would not have picked up patch 4.

But with your explanation, I would even say that "4/50 needs to go together with the rest" *for correctness*, not just to mean something.

From: Kim Phillips <[email protected]>

Without SEV-SNP, Automatic IBRS protects only the kernel. But when SEV-SNP is enabled, the Automatic IBRS protection umbrella widens to all host-side code, including userspace. This protection comes at a cost:
reduced userspace indirect branch performance.

To avoid this performance loss, don't use Automatic IBRS on SEV-SNP hosts. Fall back to retpolines instead.

Signed-off-by Kim Phillips <[email protected]>

[mdr: squash in changes from review discussion]

Signed-off-by Michael Roth <[email protected]>

---
 arch/x86/kernel/cpu/common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Without SEV-SNP, Automatic IBRS protects only the kernel. But when SEV-SNP is enabled, the Automatic IBRS protection umbrella widens to all host-side code, including userspace. This protection comes at a cost:
reduced userspace indirect branch performance.

To avoid this performance loss, don't use Automatic IBRS on SEV-SNP hosts. Fall back to retpolines instead.

From: Kim Phillips <[email protected]>

Without SEV-SNP, Automatic IBRS protects only the kernel. But when SEV-SNP is enabled, the Automatic IBRS protection umbrella widens to all host-side code, including userspace. This protection comes at a cost:
reduced userspace indirect branch performance.

To avoid this performance loss, don't use Automatic IBRS on SEV-SNP hosts. Fall back to retpolines instead.

Signed-off-by Kim Phillips <[email protected]>

[mdr: squash in changes from review discussion]

Signed-off-by Michael Roth <[email protected]>

---
  arch/x86/kernel/cpu/common.c | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

arch/x86/kernel/cpu/common.c +6 -1

index 382d4e6b848d..11fae89b799e 100644
@@ -1357,8 +1357,13 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
  	/*
  	 * AMD's AutoIBRS is equivalent to Intel's eIBRS - use the Intel feature
  	 * flag and protect from vendor-specific bugs via the whitelist.
+	 *
+	 * Don't use AutoIBRS when SNP is enabled because it degrades host
+	 * userspace indirect branch performance.
  	 */
-	if ((ia32_cap & ARCH_CAP_IBRS_ALL) || cpu_has(c, X86_FEATURE_AUTOIBRS)) {
+	if ((ia32_cap & ARCH_CAP_IBRS_ALL) ||
+	    (cpu_has(c, X86_FEATURE_AUTOIBRS) &&
+	     !cpu_feature_enabled(X86_FEATURE_SEV_SNP))) {
  		setup_force_cpu_cap(X86_FEATURE_IBRS_ENHANCED);
  		if (!cpu_matches(cpu_vuln_whitelist, NO_EIBRS_PBRSB) &&
  		    !(ia32_cap & ARCH_CAP_PBRSB_NO))

From: Brijesh Singh <[email protected]>

The memory integrity guarantees of SEV-SNP are enforced through a new structure called the Reverse Map Table (RMP). The RMP is a single data structure shared across the system that contains one entry for every 4K page of DRAM that may be used by SEV-SNP VMs. APM2 section 15.36 details a number of steps needed to detect/enable SEV-SNP and RMP table support on the host:

- Detect SEV-SNP support based on CPUID bit
- Initialize the RMP table memory reported by the RMP base/end MSR registers and configure IOMMU to be compatible with RMP access restrictions
- Set the MtrrFixDramModEn bit in SYSCFG MSR
- Set the SecureNestedPagingEn and VMPLEn bits in the SYSCFG MSR
- Configure IOMMU

RMP table entry format is non-architectural and it can vary by processor. It is defined by the PPR. Restrict SNP support to CPU models/families which are compatible with the current RMP table entry format to guard against any undefined behavior when running on other system types. Future models/support will handle this through an architectural mechanism to allow for broader compatibility.

SNP host code depends on CONFIG_KVM_AMD_SEV config flag, which may be enabled even when CONFIG_AMD_MEM_ENCRYPT isn't set, so update the SNP-specific IOMMU helpers used here to rely on CONFIG_KVM_AMD_SEV instead of CONFIG_AMD_MEM_ENCRYPT.

Co-developed-by Ashish Kalra <[email protected]>

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

Co-developed-by Tom Lendacky <[email protected]>

Signed-off-by Tom Lendacky <[email protected]>

Signed-off-by Brijesh Singh <[email protected]>

[mdr: rework commit message to be clearer about what patch does, squash
in early_rmptable_check() handling from Tom]

Signed-off-by Michael Roth <[email protected]>

---
  arch/x86/Kbuild                          |   2 +
  arch/x86/include/asm/disabled-features.h |   8 +-
  arch/x86/include/asm/msr-index.h         |  11 +-
  arch/x86/include/asm/sev.h               |   6 +
  arch/x86/kernel/cpu/amd.c                |  19 ++
  arch/x86/virt/svm/Makefile               |   3 +
  arch/x86/virt/svm/sev.c                  | 239 +++++++++++++++++++++++
  drivers/iommu/amd/init.c                 |   2 +-
  include/linux/amd-iommu.h                |   2 +-
  9 files changed, 288 insertions(+), 4 deletions(-)

create mode 100644 arch/x86/virt/svm/Makefile
create mode 100644 arch/x86/virt/svm/sev.c

arch/x86/Kbuild +2 -0

index 5a83da703e87..6a1f36df6a18 100644
@@ -28,5 +28,7 @@ obj-y += net/
 
  obj-$(CONFIG_KEXEC_FILE) += purgatory/
 
+obj-y += virt/svm/
+
  # for cleaning
  subdir- += boot tools

arch/x86/include/asm/disabled-features.h +7 -1

index 702d93fdd10e..83efd407033b 100644
@@ -117,6 +117,12 @@
  #define DISABLE_IBT	(1 << (X86_FEATURE_IBT & 31))
  #endif
 
+#ifdef CONFIG_KVM_AMD_SEV
+# define DISABLE_SEV_SNP	0
+#else
+# define DISABLE_SEV_SNP	(1 << (X86_FEATURE_SEV_SNP & 31))
+#endif
+
  /*
   * Make sure to add features to the correct mask
   */

@@ -141,7 +147,7 @@
  			 DISABLE_ENQCMD)
  #define DISABLED_MASK17	0
  #define DISABLED_MASK18	(DISABLE_IBT)
-#define DISABLED_MASK19	0
+#define DISABLED_MASK19	(DISABLE_SEV_SNP)
  #define DISABLED_MASK20	0
  #define DISABLED_MASK_CHECK BUILD_BUG_ON_ZERO(NCAPINTS != 21)
 

arch/x86/include/asm/msr-index.h +10 -1

index 1d111350197f..2be74afb4cbd 100644
@@ -589,6 +589,8 @@
  #define MSR_AMD64_SEV_ENABLED		BIT_ULL(MSR_AMD64_SEV_ENABLED_BIT)
  #define MSR_AMD64_SEV_ES_ENABLED	BIT_ULL(MSR_AMD64_SEV_ES_ENABLED_BIT)
  #define MSR_AMD64_SEV_SNP_ENABLED	BIT_ULL(MSR_AMD64_SEV_SNP_ENABLED_BIT)
+#define MSR_AMD64_RMP_BASE		0xc0010132
+#define MSR_AMD64_RMP_END		0xc0010133
 
  /* SNP feature bits enabled by the hypervisor */
  #define MSR_AMD64_SNP_VTOM			BIT_ULL(3)

@@ -690,7 +692,14 @@
  #define MSR_K8_TOP_MEM2			0xc001001d
  #define MSR_AMD64_SYSCFG		0xc0010010
  #define MSR_AMD64_SYSCFG_MEM_ENCRYPT_BIT	23
-#define MSR_AMD64_SYSCFG_MEM_ENCRYPT	BIT_ULL(MSR_AMD64_SYSCFG_MEM_ENCRYPT_BIT)
+#define MSR_AMD64_SYSCFG_MEM_ENCRYPT		BIT_ULL(MSR_AMD64_SYSCFG_MEM_ENCRYPT_BIT)
+#define MSR_AMD64_SYSCFG_SNP_EN_BIT		24
+#define MSR_AMD64_SYSCFG_SNP_EN		BIT_ULL(MSR_AMD64_SYSCFG_SNP_EN_BIT)
+#define MSR_AMD64_SYSCFG_SNP_VMPL_EN_BIT	25
+#define MSR_AMD64_SYSCFG_SNP_VMPL_EN		BIT_ULL(MSR_AMD64_SYSCFG_SNP_VMPL_EN_BIT)
+#define MSR_AMD64_SYSCFG_MFDM_BIT		19
+#define MSR_AMD64_SYSCFG_MFDM			BIT_ULL(MSR_AMD64_SYSCFG_MFDM_BIT)
+
  #define MSR_K8_INT_PENDING_MSG		0xc0010055
  /* C1E active bits in int pending message */
  #define K8_INTP_C1E_ACTIVE_MASK		0x18000000

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

index 5b4a1ce3d368..b05fcd0ab7e4 100644
@@ -243,4 +243,10 @@ static inline u64 snp_get_unsupported_features(u64 status) { return 0; }
  static inline u64 sev_get_status(void) { return 0; }
  #endif
 
+#ifdef CONFIG_KVM_AMD_SEV
+bool snp_get_rmptable_info(u64 *start, u64 *len);
+#else
+static inline bool snp_get_rmptable_info(u64 *start, u64 *len) { return false; }
+#endif
+
  #endif

arch/x86/kernel/cpu/amd.c +1 -0

index 14ee7f750cc7..6cc2074fcea3 100644
@@ -20,6 +20,7 @@
  #include <asm/delay.h>
  #include <asm/debugreg.h>
  #include <asm/resctrl.h>
+#include <asm/sev.h>
 
  #ifdef CONFIG_X86_64
  # include <asm/mmconfig.h>

@@ -618,6 +619,20 @@ static void bsp_init_amd(struct cpuinfo_x86 *c)
  	resctrl_cpu_detect(c);
  }
 

arch/x86/kernel/cpu/amd.c +8 -0

+
+	return snp_get_rmptable_info(&rmp_base, &rmp_size);
+}
+
  static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
  {
  	u64 msr;

@@ -659,6 +674,9 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
  		if (!(msr & MSR_K7_HWCR_SMMLOCK))
  			goto clear_sev;
 
+		if (cpu_has(c, X86_FEATURE_SEV_SNP) && !early_rmptable_check())
+			goto clear_snp;
+
  		return;
 
  clear_all:

@@ -666,6 +684,7 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
  clear_sev:
  		setup_clear_cpu_cap(X86_FEATURE_SEV);
  		setup_clear_cpu_cap(X86_FEATURE_SEV_ES);
+clear_snp:
  		setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
  	}
  }

arch/x86/virt/svm/Makefile new file +3 -0

new file mode 100644
index 000000000000..ef2a31bdcc70
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_KVM_AMD_SEV) += sev.o

arch/x86/virt/svm/sev.c new file +239 -0

new file mode 100644
index 000000000000..8b9ed72489e4
@@ -0,0 +1,239 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * AMD SVM-SEV Host Support.
+ *
+ * Copyright (C) 2023 Advanced Micro Devices, Inc.
+ *
+ * Author: Ashish Kalra <[email protected]>
+ *
+ */
+
+#include <linux/cc_platform.h>
+#include <linux/printk.h>
+#include <linux/mm_types.h>
+#include <linux/set_memory.h>
+#include <linux/memblock.h>
+#include <linux/kernel.h>
+#include <linux/mm.h>
+#include <linux/cpumask.h>
+#include <linux/iommu.h>
+#include <linux/amd-iommu.h>
+
+#include <asm/sev.h>
+#include <asm/processor.h>
+#include <asm/setup.h>
+#include <asm/svm.h>
+#include <asm/smp.h>
+#include <asm/cpu.h>
+#include <asm/apic.h>
+#include <asm/cpuid.h>
+#include <asm/cmdline.h>
+#include <asm/iommu.h>
+
+/*
+ * The RMP entry format is not architectural. The format is defined in PPR
+ * Family 19h Model 01h, Rev B1 processor.
+ */
+struct rmpentry {
+	u64	assigned	: 1,
+		pagesize	: 1,
+		immutable	: 1,
+		rsvd1		: 9,
+		gpa		: 39,
+		asid		: 10,
+		vmsa		: 1,
+		validated	: 1,
+		rsvd2		: 1;
+	u64 rsvd3;
+} __packed;
+
+/*
+ * The first 16KB from the RMP_BASE is used by the processor for the
+ * bookkeeping, the range needs to be added during the RMP entry lookup.
+ */
+#define RMPTABLE_CPU_BOOKKEEPING_SZ	0x4000
+
+static struct rmpentry *rmptable_start __ro_after_init;
+static u64 rmptable_max_pfn __ro_after_init;
+
+#undef pr_fmt
+#define pr_fmt(fmt)	"SEV-SNP: " fmt
+
+static int __mfd_enable(unsigned int cpu)
+{
+	u64 val;
+
+	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+		return 0;
+
+	rdmsrl(MSR_AMD64_SYSCFG, val);
+
+	val |= MSR_AMD64_SYSCFG_MFDM;
+
+	wrmsrl(MSR_AMD64_SYSCFG, val);
+
+	return 0;
+}
+
+static __init void mfd_enable(void *arg)
+{
+	__mfd_enable(smp_processor_id());
+}
+
+static int __snp_enable(unsigned int cpu)
+{
+	u64 val;
+
+	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+		return 0;
+
+	rdmsrl(MSR_AMD64_SYSCFG, val);
+
+	val |= MSR_AMD64_SYSCFG_SNP_EN;
+	val |= MSR_AMD64_SYSCFG_SNP_VMPL_EN;
+
+	wrmsrl(MSR_AMD64_SYSCFG, val);
+
+	return 0;
+}
+
+static __init void snp_enable(void *arg)
+{
+	__snp_enable(smp_processor_id());
+}
+
+#define RMP_ADDR_MASK GENMASK_ULL(51, 13)
+
+bool snp_get_rmptable_info(u64 *start, u64 *len)
+{
+	u64 max_rmp_pfn, calc_rmp_sz, rmp_sz, rmp_base, rmp_end;
+
+	rdmsrl(MSR_AMD64_RMP_BASE, rmp_base);
+	rdmsrl(MSR_AMD64_RMP_END, rmp_end);
+
+	if (!(rmp_base & RMP_ADDR_MASK) || !(rmp_end & RMP_ADDR_MASK)) {
+		pr_err("Memory for the RMP table has not been reserved by BIOS\n");
+		return false;
+	}
+
+	if (rmp_base > rmp_end) {
+		pr_err("RMP configuration not valid: base=%#llx, end=%#llx\n", rmp_base, rmp_end);
+		return false;
+	}
+
+	rmp_sz = rmp_end - rmp_base + 1;
+
+	/*
+	 * Calculate the amount the memory that must be reserved by the BIOS to
+	 * address the whole RAM, including the bookkeeping area. The RMP itself
+	 * must also be covered.
+	 */
+	max_rmp_pfn = max_pfn;
+	if (PHYS_PFN(rmp_end) > max_pfn)
+		max_rmp_pfn = PHYS_PFN(rmp_end);
+
+	calc_rmp_sz = (max_rmp_pfn << 4) + RMPTABLE_CPU_BOOKKEEPING_SZ;
+
+	if (calc_rmp_sz > rmp_sz) {
+		pr_err("Memory reserved for the RMP table does not cover full system RAM (expected 0x%llx got 0x%llx)\n",
+		       calc_rmp_sz, rmp_sz);
+		return false;
+	}
+
+	*start = rmp_base;
+	*len = rmp_sz;
+
+	return true;
+}
+
+static __init int __snp_rmptable_init(void)
+{
+	u64 rmp_base, rmp_size;
+	void *rmp_start;
+	u64 val;
+
+	if (!snp_get_rmptable_info(&rmp_base, &rmp_size))
+		return 1;
+
+	pr_info("RMP table physical address [0x%016llx - 0x%016llx]\n",
+		rmp_base, rmp_base + rmp_size - 1);
+
+	rmp_start = memremap(rmp_base, rmp_size, MEMREMAP_WB);
+	if (!rmp_start) {
+		pr_err("Failed to map RMP table addr 0x%llx size 0x%llx\n", rmp_base, rmp_size);
+		return 1;
+	}
+
+	/*
+	 * Check if SEV-SNP is already enabled, this can happen in case of
+	 * kexec boot.
+	 */
+	rdmsrl(MSR_AMD64_SYSCFG, val);
+	if (val & MSR_AMD64_SYSCFG_SNP_EN)
+		goto skip_enable;
+
+	/* Initialize the RMP table to zero */
+	memset(rmp_start, 0, rmp_size);
+
+	/* Flush the caches to ensure that data is written before SNP is enabled. */
+	wbinvd_on_all_cpus();
+
+	/* MFDM must be enabled on all the CPUs prior to enabling SNP. */
+	on_each_cpu(mfd_enable, NULL, 1);
+
+	/* Enable SNP on all CPUs. */
+	on_each_cpu(snp_enable, NULL, 1);
+
+skip_enable:
+	rmp_start += RMPTABLE_CPU_BOOKKEEPING_SZ;
+	rmp_size -= RMPTABLE_CPU_BOOKKEEPING_SZ;
+
+	rmptable_start = (struct rmpentry *)rmp_start;
+	rmptable_max_pfn = rmp_size / sizeof(struct rmpentry) - 1;
+
+	return 0;
+}
+
+static int __init snp_rmptable_init(void)
+{
+	int family, model;
+
+	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+		return 0;
+
+	family = boot_cpu_data.x86;
+	model  = boot_cpu_data.x86_model;
+
+	/*
+	 * RMP table entry format is not architectural and it can vary by processor and
+	 * is defined by the per-processor PPR. Restrict SNP support on the known CPU
+	 * model and family for which the RMP table entry format is currently defined for.
+	 */
+	if (family != 0x19 || model > 0xaf)
+		goto nosnp;
+
+	if (amd_iommu_snp_enable())
+		goto nosnp;
+
+	if (__snp_rmptable_init())
+		goto nosnp;
+
+	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "x86/rmptable_init:online", __snp_enable, NULL);
+
+	return 0;
+
+nosnp:
+	setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
+	return -ENOSYS;
+}
+
+/*
+ * This must be called after the PCI subsystem. This is because amd_iommu_snp_enable()
+ * is called to ensure the IOMMU supports the SEV-SNP feature, which can only be
+ * called after subsys_initcall().
+ *
+ * NOTE: IOMMU is enforced by SNP to ensure that hypervisor cannot program DMA
+ * directly into guest private memory. In case of SNP, the IOMMU ensures that
+ * the page(s) used for DMA are hypervisor owned.
+ */
+fs_initcall(snp_rmptable_init);

drivers/iommu/amd/init.c +1 -1

index 45efb7e5d725..1c9924de607a 100644
@@ -3802,7 +3802,7 @@ int amd_iommu_pc_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr, u8 fxn, u64
  	return iommu_pc_get_set_reg(iommu, bank, cntr, fxn, value, true);
  }
 
-#ifdef CONFIG_AMD_MEM_ENCRYPT
+#ifdef CONFIG_KVM_AMD_SEV
  int amd_iommu_snp_enable(void)
  {
  	/*

include/linux/amd-iommu.h +1 -1

index 99a5201d9e62..55fc03cb3968 100644
@@ -205,7 +205,7 @@ int amd_iommu_pc_get_reg(struct amd_iommu *iommu, u8 bank, u8 cntr, u8 fxn,
  		u64 *value);
  struct amd_iommu *get_amd_iommu(unsigned int idx);
 
-#ifdef CONFIG_AMD_MEM_ENCRYPT
+#ifdef CONFIG_KVM_AMD_SEV
  int amd_iommu_snp_enable(void);
  #endif
 

arch/x86/kernel/cpu/amd.c +8 -0

+
+	return snp_get_rmptable_info(&rmp_base, &rmp_size);
+}
+
 static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
 {
 	u64 msr;

@@ -659,6 +674,9 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
 		if (!(msr & MSR_K7_HWCR_SMMLOCK))
 			goto clear_sev;
 
+		if (cpu_has(c, X86_FEATURE_SEV_SNP) && !early_rmptable_check())
+			goto clear_snp;
+
 		return;
 
 clear_all:

@@ -666,6 +684,7 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
 clear_sev:
 		setup_clear_cpu_cap(X86_FEATURE_SEV);
 		setup_clear_cpu_cap(X86_FEATURE_SEV_ES);
+clear_snp:
 		setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
 	}
 }

arch/x86/virt/svm/sev.c new file +29 -0

+		pr_err("RMP configuration not valid: base=%#llx, end=%#llx\n", rmp_base, rmp_end);
+		return false;
+	}
+
+	rmp_sz = rmp_end - rmp_base + 1;
+
+	/*
+	 * Calculate the amount the memory that must be reserved by the BIOS to
+	 * address the whole RAM, including the bookkeeping area. The RMP itself
+	 * must also be covered.
+	 */
+	max_rmp_pfn = max_pfn;
+	if (PHYS_PFN(rmp_end) > max_pfn)
+		max_rmp_pfn = PHYS_PFN(rmp_end);
+
+	calc_rmp_sz = (max_rmp_pfn << 4) + RMPTABLE_CPU_BOOKKEEPING_SZ;
+
+	if (calc_rmp_sz > rmp_sz) {
+		pr_err("Memory reserved for the RMP table does not cover full system RAM (expected 0x%llx got 0x%llx)\n",
+		       calc_rmp_sz, rmp_sz);
+		return false;
+	}
+
+	*start = rmp_base;
+	*len = rmp_sz;
+
+	return true;
+}
+

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

+	on_each_cpu(snp_enable, NULL, 1);
+
+skip_enable:
+	rmp_start += RMPTABLE_CPU_BOOKKEEPING_SZ;
+	rmp_size -= RMPTABLE_CPU_BOOKKEEPING_SZ;
+
+	rmptable_start = (struct rmpentry *)rmp_start;
+	rmptable_max_pfn = rmp_size / sizeof(struct rmpentry) - 1;
+
+	return 0;
+}

arch/x86/virt/svm/sev.c new file +23 -0

+	/*
+	 * RMP table entry format is not architectural and it can vary by processor and
+	 * is defined by the per-processor PPR. Restrict SNP support on the known CPU
+	 * model and family for which the RMP table entry format is currently defined for.
+	 */
+	if (family != 0x19 || model > 0xaf)
+		goto nosnp;
+
+	if (amd_iommu_snp_enable())
+		goto nosnp;
+
+	if (__snp_rmptable_init())
+		goto nosnp;
+
+	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "x86/rmptable_init:online", __snp_enable, NULL);
+
+	return 0;
+
+nosnp:
+	setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
+	return -ENOSYS;
+}
+

arch/x86/kernel/cpu/amd.c static void bsp_init_amd(struct cpuinfo_x86 *c) · line 631

Quoted 10 lines · Michael Roth

arch/x86/kernel/cpu/amd.c +10 -0

+static bool early_rmptable_check(void)
+{
+	u64 rmp_base, rmp_size;
+
+	/*
+	 * For early BSP initialization, max_pfn won't be set up yet, wait until
+	 * it is set before performing the RMP table calculations.
+	 */
+	if (!max_pfn)
+		return true;

This already says that this is called at the wrong point during init.

Right now we have

early_identify_cpu -> early_init_amd -> early_detect_mem_encrypt

which runs only on the BSP but then early_init_amd() is called in init_amd() too so that it takes care of the APs too.

Which ends up doing a lot of unnecessary work on each AP in early_detect_mem_encrypt() like calculating the RMP size on each AP unnecessarily where this needs to happen exactly once.

Is there any reason why this function cannot be moved to init_amd() where it'll do the normal, per-AP init?

And the stuff that needs to happen once, needs to be called once too.

Quoted 25 lines

arch/x86/kernel/cpu/amd.c +8 -0

+
+	return snp_get_rmptable_info(&rmp_base, &rmp_size);
+}
+
  static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
  {
  	u64 msr;

@@ -659,6 +674,9 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
  		if (!(msr & MSR_K7_HWCR_SMMLOCK))
  			goto clear_sev;
 
+		if (cpu_has(c, X86_FEATURE_SEV_SNP) && !early_rmptable_check())
+			goto clear_snp;
+
  		return;
 
  clear_all:

@@ -666,6 +684,7 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
  clear_sev:
  		setup_clear_cpu_cap(X86_FEATURE_SEV);
  		setup_clear_cpu_cap(X86_FEATURE_SEV_ES);
+clear_snp:
  		setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
  	}
  }

...

arch/x86/virt/svm/sev.c line 117

Quoted 11 lines

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

+bool snp_get_rmptable_info(u64 *start, u64 *len)
+{
+	u64 max_rmp_pfn, calc_rmp_sz, rmp_sz, rmp_base, rmp_end;
+
+	rdmsrl(MSR_AMD64_RMP_BASE, rmp_base);
+	rdmsrl(MSR_AMD64_RMP_END, rmp_end);
+
+	if (!(rmp_base & RMP_ADDR_MASK) || !(rmp_end & RMP_ADDR_MASK)) {
+		pr_err("Memory for the RMP table has not been reserved by BIOS\n");
+		return false;
+	}

If you're masking off bits 0-12 above...

Review

Quoted 3 lines

(quoted diff) +2 -0

+
+	if (rmp_base > rmp_end) {

... why aren't you using the masked out vars further on?

I know, the hw will say, yeah, those bits are 0 but still. IOW, do:

	rmp_base &= RMP_ADDR_MASK;
	rmp_end  &= RMP_ADDR_MASK;

after reading them.

It needs to be called early enough to allow for AutoIBRS to not be disabled just because SNP is supported. By calling it where it is currently called, the SNP feature can be cleared if, even though supported, SNP can't be used, allowing AutoIBRS to be used as a more performant Spectre mitigation.

Quoted 29 lines · Borislav Petkov

And the stuff that needs to happen once, needs to be called once too.

Quoted 23 lines

arch/x86/kernel/cpu/amd.c +4 -0

+
+    return snp_get_rmptable_info(&rmp_base, &rmp_size);
+}
+

  static void early_detect_mem_encrypt(struct cpuinfo_x86 *c) {
      u64 msr;

arch/x86/kernel/cpu/amd.c +2 -0

@@ -659,6 +674,9 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
           if (!(msr & MSR_K7_HWCR_SMMLOCK))
               goto clear_sev;
   +        if (cpu_has(c, X86_FEATURE_SEV_SNP) && !early_rmptable_check())
+            goto clear_snp;
+
           return;

    clear_all:

(quoted diff) +0 -0

@@ -666,6 +684,7 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)

  clear_sev:
          setup_clear_cpu_cap(X86_FEATURE_SEV); setup_clear_cpu_cap(X86_FEATURE_SEV_ES); +clear_snp:
          setup_clear_cpu_cap(X86_FEATURE_SEV_SNP); }
  }

...

Quoted 13 lines

arch/x86/virt/svm/sev.c line 117

Quoted 11 lines

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

+bool snp_get_rmptable_info(u64 *start, u64 *len)
+{
+    u64 max_rmp_pfn, calc_rmp_sz, rmp_sz, rmp_base, rmp_end;
+
+    rdmsrl(MSR_AMD64_RMP_BASE, rmp_base);
+    rdmsrl(MSR_AMD64_RMP_END, rmp_end);
+
+    if (!(rmp_base & RMP_ADDR_MASK) || !(rmp_end & RMP_ADDR_MASK)) {
+        pr_err("Memory for the RMP table has not been reserved by BIOS\n");
+        return false;
+    }

If you're masking off bits 0-12 above...

Because the RMP_END MSR, most specifically, has a default value of 0x1fff, where bits [12:0] are reserved. So to specifically check if the MSR has been set to a non-zero end value, the bit are masked off. However, ...

Quoted 44 lines · Tom Lendacky

Quoted 42 lines · Borislav Petkov

And the stuff that needs to happen once, needs to be called once too.

Quoted 23 lines

arch/x86/kernel/cpu/amd.c +7 -0

+
+    return snp_get_rmptable_info(&rmp_base, &rmp_size);
+}
+
   static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
   {
       u64 msr;

@@ -659,6 +674,9 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
           if (!(msr & MSR_K7_HWCR_SMMLOCK))
               goto clear_sev;
   +        if (cpu_has(c, X86_FEATURE_SEV_SNP) && !early_rmptable_check())
+            goto clear_snp;
+
           return;
     clear_all:

@@ -666,6 +684,7 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
   clear_sev:
           setup_clear_cpu_cap(X86_FEATURE_SEV);
           setup_clear_cpu_cap(X86_FEATURE_SEV_ES);
+clear_snp:
           setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
       }
   }

...

arch/x86/virt/svm/sev.c line 117

Quoted 11 lines

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

+bool snp_get_rmptable_info(u64 *start, u64 *len)
+{
+    u64 max_rmp_pfn, calc_rmp_sz, rmp_sz, rmp_base, rmp_end;
+
+    rdmsrl(MSR_AMD64_RMP_BASE, rmp_base);
+    rdmsrl(MSR_AMD64_RMP_END, rmp_end);
+
+    if (!(rmp_base & RMP_ADDR_MASK) || !(rmp_end & RMP_ADDR_MASK)) {
+        pr_err("Memory for the RMP table has not been reserved by BIOS\n");
+        return false;
+    }

If you're masking off bits 0-12 above...

Quoted 2 lines

Because the RMP_END MSR, most specifically, has a default value of 0x1fff, where bits [12:0] are reserved. So to specifically check if the MSR has been set to a non-zero end value, the bit are masked off. However, ...

Do you have a source for this? Because the APM vol. 2, table A.7 says the reset value of RMP_END is all zeros.

Thanks,
Jeremi

arch/x86/kernel/cpu/amd.c static void bsp_init_amd(struct cpuinfo_x86 *c) · line 631

Quoted 10 lines · Michael Roth

arch/x86/kernel/cpu/amd.c +10 -0

+static bool early_rmptable_check(void)
+{
+	u64 rmp_base, rmp_size;
+
+	/*
+	 * For early BSP initialization, max_pfn won't be set up yet, wait until
+	 * it is set before performing the RMP table calculations.
+	 */
+	if (!max_pfn)
+		return true;

This already says that this is called at the wrong point during init.

Right now we have

early_identify_cpu -> early_init_amd -> early_detect_mem_encrypt

which runs only on the BSP but then early_init_amd() is called in init_amd() too so that it takes care of the APs too.

Which ends up doing a lot of unnecessary work on each AP in early_detect_mem_encrypt() like calculating the RMP size on each AP unnecessarily where this needs to happen exactly once.

Is there any reason why this function cannot be moved to init_amd() where it'll do the normal, per-AP init?

And the stuff that needs to happen once, needs to be called once too.

Quoted 25 lines

arch/x86/kernel/cpu/amd.c +8 -0

+
+	return snp_get_rmptable_info(&rmp_base, &rmp_size);
+}
+
  static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
  {
  	u64 msr;

@@ -659,6 +674,9 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
  		if (!(msr & MSR_K7_HWCR_SMMLOCK))
  			goto clear_sev;
 
+		if (cpu_has(c, X86_FEATURE_SEV_SNP) && !early_rmptable_check())
+			goto clear_snp;
+
  		return;
 
  clear_all:

@@ -666,6 +684,7 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
  clear_sev:
  		setup_clear_cpu_cap(X86_FEATURE_SEV);
  		setup_clear_cpu_cap(X86_FEATURE_SEV_ES);
+clear_snp:
  		setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
  	}
  }

...

arch/x86/virt/svm/sev.c line 117

Quoted 11 lines

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

+bool snp_get_rmptable_info(u64 *start, u64 *len)
+{
+	u64 max_rmp_pfn, calc_rmp_sz, rmp_sz, rmp_base, rmp_end;
+
+	rdmsrl(MSR_AMD64_RMP_BASE, rmp_base);
+	rdmsrl(MSR_AMD64_RMP_END, rmp_end);
+
+	if (!(rmp_base & RMP_ADDR_MASK) || !(rmp_end & RMP_ADDR_MASK)) {
+		pr_err("Memory for the RMP table has not been reserved by BIOS\n");
+		return false;
+	}

If you're masking off bits 0-12 above...

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	if (rmp_base > rmp_end) {

... why aren't you using the masked out vars further on?

I know, the hw will say, yeah, those bits are 0 but still. IOW, do:

	rmp_base &= RMP_ADDR_MASK;
	rmp_end  &= RMP_ADDR_MASK;

after reading them.

Quoted 29 lines

arch/x86/virt/svm/sev.c new file +29 -0

+		pr_err("RMP configuration not valid: base=%#llx, end=%#llx\n", rmp_base, rmp_end);
+		return false;
+	}
+
+	rmp_sz = rmp_end - rmp_base + 1;
+
+	/*
+	 * Calculate the amount the memory that must be reserved by the BIOS to
+	 * address the whole RAM, including the bookkeeping area. The RMP itself
+	 * must also be covered.
+	 */
+	max_rmp_pfn = max_pfn;
+	if (PHYS_PFN(rmp_end) > max_pfn)
+		max_rmp_pfn = PHYS_PFN(rmp_end);
+
+	calc_rmp_sz = (max_rmp_pfn << 4) + RMPTABLE_CPU_BOOKKEEPING_SZ;
+
+	if (calc_rmp_sz > rmp_sz) {
+		pr_err("Memory reserved for the RMP table does not cover full system RAM (expected 0x%llx got 0x%llx)\n",
+		       calc_rmp_sz, rmp_sz);
+		return false;
+	}
+
+	*start = rmp_base;
+	*len = rmp_sz;
+
+	return true;
+}
+

arch/x86/virt/svm/sev.c line 158

Quoted 10 lines

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

+static __init int __snp_rmptable_init(void)
+{
+	u64 rmp_base, rmp_size;
+	void *rmp_start;
+	u64 val;
+
+	if (!snp_get_rmptable_info(&rmp_base, &rmp_size))
+		return 1;
+
+	pr_info("RMP table physical address [0x%016llx - 0x%016llx]\n",

That's "RMP table physical range"

arch/x86/virt/svm/sev.c line 163

Quoted 5 lines

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

+		rmp_base, rmp_base + rmp_size - 1);
+
+	rmp_start = memremap(rmp_base, rmp_size, MEMREMAP_WB);
+	if (!rmp_start) {
+		pr_err("Failed to map RMP table addr 0x%llx size 0x%llx\n", rmp_base, rmp_size);

No need to dump rmp_base and rmp_size again here - you're dumping them above.

arch/x86/virt/svm/sev.c line 175

Quoted 12 lines

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

+		return 1;
+	}
+
+	/*
+	 * Check if SEV-SNP is already enabled, this can happen in case of
+	 * kexec boot.
+	 */
+	rdmsrl(MSR_AMD64_SYSCFG, val);
+	if (val & MSR_AMD64_SYSCFG_SNP_EN)
+		goto skip_enable;
+
+	/* Initialize the RMP table to zero */

Again: useless comment.

arch/x86/virt/svm/sev.c line 181

Quoted 6 lines

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

+	memset(rmp_start, 0, rmp_size);
+
+	/* Flush the caches to ensure that data is written before SNP is enabled. */
+	wbinvd_on_all_cpus();
+
+	/* MFDM must be enabled on all the CPUs prior to enabling SNP. */

First of all, use the APM bit name here pls: MtrrFixDramModEn.

And then, for the life of me, I can't find any mention in the APM why this bit is needed. Neither in "15.36.2 Enabling SEV-SNP" nor in "15.34.3 Enabling SEV".

Looking at the bit defintions of WrMem an RdMem - read and write requests get directed to system memory instead of MMIO so I guess you don't want to be able to write MMIO for certain physical ranges when SNP is enabled but it'll be good to have this properly explained instead of a "this must happen" information-less sentence.

arch/x86/virt/svm/sev.c line 184

Quoted 4 lines

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

+	on_each_cpu(mfd_enable, NULL, 1);
+
+	/* Enable SNP on all CPUs. */

Useless comment.

Quoted 11 lines

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

+	on_each_cpu(snp_enable, NULL, 1);
+
+skip_enable:
+	rmp_start += RMPTABLE_CPU_BOOKKEEPING_SZ;
+	rmp_size -= RMPTABLE_CPU_BOOKKEEPING_SZ;
+
+	rmptable_start = (struct rmpentry *)rmp_start;
+	rmptable_max_pfn = rmp_size / sizeof(struct rmpentry) - 1;
+
+	return 0;
+}

arch/x86/virt/svm/sev.c line 205

Quoted 10 lines

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

+
+static int __init snp_rmptable_init(void)
+{
+	int family, model;
+
+	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+		return 0;
+
+	family = boot_cpu_data.x86;
+	model  = boot_cpu_data.x86_model;

Looks useless - just use boot_cpu_data directly below.

As mentioned here already https://lore.kernel.org/all/[email protected]/

And I already mentioned that for v9:

https://lore.kernel.org/r/20230621094236.GZZJLGDAicp1guNPvD@fat_crate.local

Next time I'm NAKing this patch until you incorporate all review comments or you give a technical reason why you disagree with them.

Quoted 23 lines

arch/x86/virt/svm/sev.c new file +23 -0

+	/*
+	 * RMP table entry format is not architectural and it can vary by processor and
+	 * is defined by the per-processor PPR. Restrict SNP support on the known CPU
+	 * model and family for which the RMP table entry format is currently defined for.
+	 */
+	if (family != 0x19 || model > 0xaf)
+		goto nosnp;
+
+	if (amd_iommu_snp_enable())
+		goto nosnp;
+
+	if (__snp_rmptable_init())
+		goto nosnp;
+
+	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "x86/rmptable_init:online", __snp_enable, NULL);
+
+	return 0;
+
+nosnp:
+	setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
+	return -ENOSYS;
+}
+

arch/x86/virt/svm/sev.c line 239

Quoted 10 lines

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

+/*
+ * This must be called after the PCI subsystem. This is because amd_iommu_snp_enable()
+ * is called to ensure the IOMMU supports the SEV-SNP feature, which can only be
+ * called after subsys_initcall().
+ *
+ * NOTE: IOMMU is enforced by SNP to ensure that hypervisor cannot program DMA
+ * directly into guest private memory. In case of SNP, the IOMMU ensures that
+ * the page(s) used for DMA are hypervisor owned.
+ */
+fs_initcall(snp_rmptable_init);

This looks backwards. AFAICT, the IOMMU code should call arch code to enable SNP at the right time, not the other way around - arch code calling driver code.

Especially if the SNP table enablement depends on some exact IOMMU init_state:

         if (init_state > IOMMU_ENABLED) {
		pr_err("SNP: Too late to enable SNP for IOMMU.\n");

Quoted 6 lines

First of all, use the APM bit name here pls: MtrrFixDramModEn.

And then, for the life of me, I can't find any mention in the APM why this bit is needed. Neither in "15.36.2 Enabling SEV-SNP" nor in "15.34.3 Enabling SEV".

Quoted 5 lines

Looking at the bit defintions of WrMem an RdMem - read and write requests get directed to system memory instead of MMIO so I guess you don't want to be able to write MMIO for certain physical ranges when SNP is enabled but it'll be good to have this properly explained instead of a "this must happen" information-less sentence.

This is a per-requisite for SNP_INIT as per the SNP Firmware ABI specifications, section 8.8.2:

Quoted 11 lines

This looks backwards. AFAICT, the IOMMU code should call arch code to enable SNP at the right time, not the other way around - arch code calling driver code.

Especially if the SNP table enablement depends on some exact IOMMU init_state:

         if (init_state > IOMMU_ENABLED) {
		pr_err("SNP: Too late to enable SNP for IOMMU.\n");

This is again as per SNP_INIT requirements, that SNP support is enabled in the IOMMU before SNP_INIT is done. The above function snp_rmptable_init() only calls the IOMMU driver to enable SNP support when it has detected and enabled platform support for SNP.
v
It is not that IOMMU driver has to call the arch code to enable SNP at the right time but it is the other way around that once platform support for SNP is enabled then the IOMMU driver has to be called to enable the same for the IOMMU and this needs to be done before the CCP driver is loaded and does SNP_INIT.

Arch code does not call drivers - arch code sets up the arch and provides facilities which the drivers use.

Quoted 2 lines · Borislav Petkov

Arch code does not call drivers - arch code sets up the arch and provides facilities which the drivers use.

IOW (just an example diff):

drivers/iommu/amd/init.c +1 -10

index 1c9924de607a..00cdbc844961 100644
@@ -3290,6 +3290,7 @@ static int __init state_next(void)
  		break;
  	case IOMMU_ENABLED:
  		register_syscore_ops(&amd_iommu_syscore_ops);
+		amd_iommu_snp_enable();
  		ret = amd_iommu_init_pci();
  		init_state = ret ? IOMMU_INIT_ERROR : IOMMU_PCI_INIT;
  		break;

@@ -3814,16 +3815,6 @@ int amd_iommu_snp_enable(void)
  		return -EINVAL;
  	}
 
-	/*
-	 * Prevent enabling SNP after IOMMU_ENABLED state because this process
-	 * affect how IOMMU driver sets up data structures and configures
-	 * IOMMU hardware.
-	 */
-	if (init_state > IOMMU_ENABLED) {
-		pr_err("SNP: Too late to enable SNP for IOMMU.\n");
-		return -EINVAL;
-	}
-
  	amd_iommu_snp_en = check_feature_on_all_iommus(FEATURE_SNP);
  	if (!amd_iommu_snp_en)
  		return -EINVAL;

and now you only need to line up snp_rmptable_init() after IOMMU init instead of having it be a fs_initcall which happens right after pci_subsys_init() so that PCI is there but at the right time when iommu init state is at IOMMU_ENABLED but no later because then it is too late.

And there you need to test amd_iommu_snp_en which is already exported anyway.

Ok?

No, this is not correct as this will always enable SNP support on IOMMU even when SNP support is not supported and enabled on the platform,

And isn't IOMMU driver always going to be built-in and isn't it part of the platform support (not arch code, but surely platform specific code)? (IOMMU enablement is requirement for SNP).

We will still need some method to tell the IOMMU driver if SNP support/feature is disabled by this function, for example, when CPU family and model is not supported by SNP and we jump to no_snp label.

The reliable way for this to work is to ensure snp_rmptable_init() is called before IOMMU initialization and then IOMMU initialization depends on SNP feature flag setup by snp_rmptable_init() to enable SNP support on IOMMU or not.

Quoted 3 lines · Ashish

We will still need some method to tell the IOMMU driver if SNP support/feature is disabled by this function, for example, when CPU family and model is not supported by SNP and we jump to no_snp label.

See below.

Quoted 4 lines

The reliable way for this to work is to ensure snp_rmptable_init() is called before IOMMU initialization and then IOMMU initialization depends on SNP feature flag setup by snp_rmptable_init() to enable SNP support on IOMMU or not.

Yes, this whole SNP initialization needs to be reworked and split this way:

- early detection work which needs to be done once goes to bsp_init_amd(): that's basically your early_detect_mem_encrypt() stuff which needs to happen exactly only once and early.

- Any work like:

	 c->x86_phys_bits -= (cpuid_ebx(0x8000001f) >> 6) & 0x3f;

and the like which needs to happen on each AP, gets put in a function which gets called by init_amd().

By the time IOMMU gets to init, you already know whether it should enable SNP and check X86_FEATURE_SEV_SNP.

Finally, you call __snp_rmptable_init() which does the *per-CPU* init work which is still pending.

Ok?

Ontop of the previous ontop patch:

---

arch/x86/kernel/cpu/amd.c +13 -2

index 6cc2074fcea3..a9c95e5d6b06 100644
@@ -674,8 +674,19 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
 		if (!(msr & MSR_K7_HWCR_SMMLOCK))
 			goto clear_sev;
 
-		if (cpu_has(c, X86_FEATURE_SEV_SNP) && !early_rmptable_check())
-			goto clear_snp;
+		if (cpu_has(c, X86_FEATURE_SEV_SNP)) {
+			/*
+			 * RMP table entry format is not architectural and it can vary by processor
+			 * and is defined by the per-processor PPR. Restrict SNP support on the known
+			 * CPU model and family for which the RMP table entry format is currently
+			 * defined for.
+			 */
+			if (c->x86 != 0x19 || c->x86_model > 0xaf)
+				goto clear_snp;
+
+			if (!early_rmptable_check())
+				goto clear_snp;
+		}
 
 		return;
 

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

index 9237c327ad6d..5a71df9ae4cb 100644
@@ -199,14 +199,6 @@ static int __init snp_rmptable_init(void)
 	if (!amd_iommu_snp_en)
 		return 0;
 
-	/*
-	 * RMP table entry format is not architectural and it can vary by processor and
-	 * is defined by the per-processor PPR. Restrict SNP support on the known CPU
-	 * model and family for which the RMP table entry format is currently defined for.
-	 */
-	if (boot_cpu_data.x86 != 0x19 || boot_cpu_data.x86_model > 0xaf)
-		goto nosnp;
-
 	if (__snp_rmptable_init())
 		goto nosnp;
 

Quoted 3 lines · Ashish

We will still need some method to tell the IOMMU driver if SNP support/feature is disabled by this function, for example, when CPU family and model is not supported by SNP and we jump to no_snp label.

See below.

Quoted 4 lines

The reliable way for this to work is to ensure snp_rmptable_init() is called before IOMMU initialization and then IOMMU initialization depends on SNP feature flag setup by snp_rmptable_init() to enable SNP support on IOMMU or not.

Yes, this whole SNP initialization needs to be reworked and split this way:

- early detection work which needs to be done once goes to bsp_init_amd(): that's basically your early_detect_mem_encrypt() stuff which needs to happen exactly only once and early.

- Any work like:

	 c->x86_phys_bits -= (cpuid_ebx(0x8000001f) >> 6) & 0x3f;

and the like which needs to happen on each AP, gets put in a function which gets called by init_amd().

By the time IOMMU gets to init, you already know whether it should enable SNP and check X86_FEATURE_SEV_SNP.

Finally, you call __snp_rmptable_init() which does the *per-CPU* init work which is still pending. >
Ok?

Hello Boris,

Addressing of some of the remaining comments:

Quoted 161 lines · Borislav Petkov

arch/x86/kernel/cpu/amd.c static void bsp_init_amd(struct cpuinfo_x86 *c) · line 631

Quoted 10 lines · Michael Roth

arch/x86/kernel/cpu/amd.c +10 -0

+static bool early_rmptable_check(void)
+{
+    u64 rmp_base, rmp_size;
+
+    /*
+     * For early BSP initialization, max_pfn won't be set up yet, wait until
+     * it is set before performing the RMP table calculations.
+     */
+    if (!max_pfn)
+        return true;

This already says that this is called at the wrong point during init.

Right now we have

early_identify_cpu -> early_init_amd -> early_detect_mem_encrypt

which runs only on the BSP but then early_init_amd() is called in init_amd() too so that it takes care of the APs too.

Which ends up doing a lot of unnecessary work on each AP in early_detect_mem_encrypt() like calculating the RMP size on each AP unnecessarily where this needs to happen exactly once.

Is there any reason why this function cannot be moved to init_amd() where it'll do the normal, per-AP init?

And the stuff that needs to happen once, needs to be called once too.

Quoted 23 lines

arch/x86/kernel/cpu/amd.c +4 -0

+
+    return snp_get_rmptable_info(&rmp_base, &rmp_size);
+}
+

  static void early_detect_mem_encrypt(struct cpuinfo_x86 *c) {
      u64 msr;

arch/x86/kernel/cpu/amd.c +2 -0

@@ -659,6 +674,9 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
           if (!(msr & MSR_K7_HWCR_SMMLOCK))
               goto clear_sev;
   +        if (cpu_has(c, X86_FEATURE_SEV_SNP) && !early_rmptable_check())
+            goto clear_snp;
+
           return;

    clear_all:

(quoted diff) +0 -0

@@ -666,6 +684,7 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)

  clear_sev:
          setup_clear_cpu_cap(X86_FEATURE_SEV); setup_clear_cpu_cap(X86_FEATURE_SEV_ES); +clear_snp:
          setup_clear_cpu_cap(X86_FEATURE_SEV_SNP); }
  }

...

arch/x86/virt/svm/sev.c line 117

Quoted 11 lines

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

+bool snp_get_rmptable_info(u64 *start, u64 *len)
+{
+    u64 max_rmp_pfn, calc_rmp_sz, rmp_sz, rmp_base, rmp_end;
+
+    rdmsrl(MSR_AMD64_RMP_BASE, rmp_base);
+    rdmsrl(MSR_AMD64_RMP_END, rmp_end);
+
+    if (!(rmp_base & RMP_ADDR_MASK) || !(rmp_end & RMP_ADDR_MASK)) {
+        pr_err("Memory for the RMP table has not been reserved by BIOS\n");
+        return false;
+    }

If you're masking off bits 0-12 above...

Quoted 2 lines

+
+    if (rmp_base > rmp_end) {

... why aren't you using the masked out vars further on?

I know, the hw will say, yeah, those bits are 0 but still. IOW, do:

    rmp_base &= RMP_ADDR_MASK;
    rmp_end  &= RMP_ADDR_MASK;

after reading them.

Quoted 29 lines

+        pr_err("RMP configuration not valid: base=%#llx, end=%#llx\n", rmp_base, rmp_end); +        return false;
+    }

arch/x86/virt/svm/sev.c new file +26 -0

+
+    rmp_sz = rmp_end - rmp_base + 1;
+
+    /*
+     * Calculate the amount the memory that must be reserved by the BIOS to
+     * address the whole RAM, including the bookkeeping area. The RMP itself
+     * must also be covered.
+     */
+    max_rmp_pfn = max_pfn;
+    if (PHYS_PFN(rmp_end) > max_pfn)
+        max_rmp_pfn = PHYS_PFN(rmp_end);
+
+    calc_rmp_sz = (max_rmp_pfn << 4) + RMPTABLE_CPU_BOOKKEEPING_SZ;
+
+    if (calc_rmp_sz > rmp_sz) {
+        pr_err("Memory reserved for the RMP table does not cover full system RAM (expected 0x%llx got 0x%llx)\n",
+               calc_rmp_sz, rmp_sz);
+        return false;
+    }
+
+    *start = rmp_base;
+    *len = rmp_sz;
+
+    return true;
+}
+

arch/x86/virt/svm/sev.c line 158

Quoted 10 lines

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

+static __init int __snp_rmptable_init(void)
+{
+    u64 rmp_base, rmp_size;
+    void *rmp_start;
+    u64 val;
+
+    if (!snp_get_rmptable_info(&rmp_base, &rmp_size))
+        return 1;
+
+    pr_info("RMP table physical address [0x%016llx - 0x%016llx]\n",

That's "RMP table physical range"

Quoted 5 lines

+        rmp_base, rmp_base + rmp_size - 1);
+
+    rmp_start = memremap(rmp_base, rmp_size, MEMREMAP_WB);
+    if (!rmp_start) {
+        pr_err("Failed to map RMP table addr 0x%llx size 0x%llx\n", rmp_base, rmp_size);

No need to dump rmp_base and rmp_size again here - you're dumping them above.

arch/x86/virt/svm/sev.c line 175

Quoted 12 lines

+        return 1;
+    }

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

+
+    /*
+     * Check if SEV-SNP is already enabled, this can happen in case of
+     * kexec boot.
+     */
+    rdmsrl(MSR_AMD64_SYSCFG, val);
+    if (val & MSR_AMD64_SYSCFG_SNP_EN)
+        goto skip_enable;
+
+    /* Initialize the RMP table to zero */

Again: useless comment.

arch/x86/virt/svm/sev.c line 181

Quoted 6 lines

+    memset(rmp_start, 0, rmp_size);

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

+
+    /* Flush the caches to ensure that data is written before SNP is enabled. */
+    wbinvd_on_all_cpus();
+
+    /* MFDM must be enabled on all the CPUs prior to enabling SNP. */

First of all, use the APM bit name here pls: MtrrFixDramModEn.

And then, for the life of me, I can't find any mention in the APM why this bit is needed. Neither in "15.36.2 Enabling SEV-SNP" nor in "15.34.3 Enabling SEV".

Quoted 5 lines

Looking at the bit defintions of WrMem an RdMem - read and write requests get directed to system memory instead of MMIO so I guess you don't want to be able to write MMIO for certain physical ranges when SNP is enabled but it'll be good to have this properly explained instead of a "this must happen" information-less sentence.

This is a per-requisite for SNP_INIT as per the SNP Firmware ABI specifications, section 8.8.2:

From the SNP FW ABI specs:

If INIT_RMP is 1, then the firmware ensures the following system requirements are met:
• SYSCFG[MemoryEncryptionModEn] must be set to 1 across all cores. (SEV must be enabled.)> • SYSCFG[SecureNestedPagingEn] must be set to 1 across all cores.
• SYSCFG[VMPLEn] must be set to 1 across all cores.
• SYSCFG[MFDM] must be set to 1 across all cores.

• VM_HSAVE_PA (MSR C001_0117) must be set to 0h across all cores.
• HWCR[SmmLock] (MSR C001_0015) must be set to 1 across all cores.

So, this platform enabling code for SNP needs to ensure that these conditions are met before SNP_INIT is called.

Hi Ashish,

I just noticed that the kernel shouts at me about this bit when I offline->online a CPU in an SNP host:

[2692586.589194] smpboot: CPU 63 is now offline
[2692589.366822] [Firmware Warn]: MTRR: CPU 0: SYSCFG[MtrrFixDramModEn] not cleared by BIOS, clearing this bit
[2692589.376582] smpboot: Booting Node 0 Processor 63 APIC 0x3f
[2692589.378070] [Firmware Warn]: MTRR: CPU 63: SYSCFG[MtrrFixDramModEn] not cleared by BIOS, clearing this bit
[2692589.388845] microcode: CPU63: new patch_level=0x0a0011d1

Now I understand if you say "CPU offlining is not supported" but there's nothing currently blocking it.

arch/x86/kernel/cpu/amd.c static void bsp_init_amd(struct cpuinfo_x86 *c) · line 631

Quoted 10 lines · Michael Roth

arch/x86/kernel/cpu/amd.c +10 -0

+static bool early_rmptable_check(void)
+{
+	u64 rmp_base, rmp_size;
+
+	/*
+	 * For early BSP initialization, max_pfn won't be set up yet, wait until
+	 * it is set before performing the RMP table calculations.
+	 */
+	if (!max_pfn)
+		return true;

This already says that this is called at the wrong point during init.

Right now we have

early_identify_cpu -> early_init_amd -> early_detect_mem_encrypt

which runs only on the BSP but then early_init_amd() is called in init_amd() too so that it takes care of the APs too.

Which ends up doing a lot of unnecessary work on each AP in early_detect_mem_encrypt() like calculating the RMP size on each AP unnecessarily where this needs to happen exactly once.

Is there any reason why this function cannot be moved to init_amd() where it'll do the normal, per-AP init?

And the stuff that needs to happen once, needs to be called once too.

From: Brijesh Singh <[email protected]>

The snp_lookup_page_in_rmptable() can be used by the host to read the RMP

entry for a given page. The RMP entry format is documented in AMD PPR, see
https://bugzilla.kernel.org/attachment.cgi?id=296015.

Co-developed-by Ashish Kalra <[email protected]>

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

Signed-off-by Brijesh Singh <[email protected]>

[mdr: separate 'assigned' indicator from return code]

Signed-off-by Michael Roth <[email protected]>

---
 arch/x86/include/asm/sev-common.h |  4 +++
 arch/x86/include/asm/sev-host.h   | 22 +++++++++++++
 arch/x86/virt/svm/sev.c           | 53 +++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+)
 create mode 100644 arch/x86/include/asm/sev-host.h

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

index b463fcbd4b90..1e6fb93d8ab0 100644

new file mode 100644
index 000000000000..4c487ce8457f

arch/x86/include/asm/sev-host.h new file +0 -0

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

+
+	ret = rmptable_entry(pfn, entry);
+	if (ret)
+		return ret;
+
+	/*
+	 * Find the authoritative RMP entry for a PFN. This can be either a 4K
+	 * RMP entry or a special large RMP entry that is authoritative for a
+	 * whole 2M area.
+	 */
+	ret = rmptable_entry(pfn & PFN_PMD_MASK, &large_entry);
+	if (ret)
+		return ret;
+
+	*level = RMP_TO_X86_PG_LEVEL(large_entry.pagesize);
+
+	return 0;
+}
+
+int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level)
+{
+	struct rmpentry e;

Quoted 3 lines · Michael Roth

From: Brijesh Singh <[email protected]>

The snp_lookup_page_in_rmptable() can be used by the host to read the RMP

$ git grep snp_lookup_page_in_rmptable
$

Stale commit message. And not very telling. Please rewrite.

Quoted 2 lines

entry for a given page. The RMP entry format is documented in AMD PPR, see
https://bugzilla.kernel.org/attachment.cgi?id=296015.

<--- Brijesh's SOB comes first here if he's the primary author.

Quoted 20 lines

Co-developed-by Ashish Kalra <[email protected]>

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

Signed-off-by Brijesh Singh <[email protected]>

[mdr: separate 'assigned' indicator from return code]

Signed-off-by Michael Roth <[email protected]>

---
 arch/x86/include/asm/sev-common.h |  4 +++
 arch/x86/include/asm/sev-host.h   | 22 +++++++++++++
 arch/x86/virt/svm/sev.c           | 53 +++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+)
 create mode 100644 arch/x86/include/asm/sev-host.h

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

index b463fcbd4b90..1e6fb93d8ab0 100644
@@ -173,4 +173,8 @@ struct snp_psc_desc {
 #define GHCB_ERR_INVALID_INPUT		5
 #define GHCB_ERR_INVALID_EVENT		6
 

arch/x86/include/asm/sev-common.h struct snp_psc_desc { · line 177

Quoted 2 lines

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

+/* RMP page size */
+#define RMP_PG_SIZE_4K			0

RMP_PG_LEVEL_4K just like the generic ones.

arch/x86/include/asm/sev-common.h struct snp_psc_desc { · line 178

Quoted 2 lines

arch/x86/include/asm/sev-common.h +1 -0

+#define RMP_TO_X86_PG_LEVEL(level)	(((level) == RMP_PG_SIZE_4K) ? PG_LEVEL_4K : PG_LEVEL_2M)

What else is there besides X86 PG level?

IOW, RMP_TO_PG_LEVEL simply.

arch/x86/include/asm/sev-host.h

Quoted 4 lines

arch/x86/include/asm/sev-common.h +1 -0

+
 #endif

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

Nah, we don't need a third sev header:

arch/x86/include/asm/sev-common.h
arch/x86/include/asm/sev.h
arch/x86/include/asm/sev-host.h

Put it in sev.h pls.

sev-common.h should be merged into sev.h too unless there's a compelling reason not to which I don't see atm.

Quoted 1 line

Thx.

Signature · 4 lines

Regards/Gruss,
    Boris.

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

I've moved this to sev.h, but it RMP_PG_SIZE_4K is already defined there and used by a bunch of guest code so it's a bit out-of-place to update those as part of this patchset. I can send a follow-up series to clean up some of the naming and get rid of sev-common.h

Doesn't seem like it would be an issue, maybe some fallout from any files that previously only included sev-common.h and now need to pull in guest struct definitions as well, but those definitions don't have a lot of external dependencies so don't anticipate any header include hellishness. I'll send that as a separate follow-up, along with some of the renames you suggested above since they'll touch guest code and create unecessary churn for SNP host support.

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

+	struct rmpentry e;
+	u64 pfn, pfn_end;
+	int level, ret;
+	u64 *e_data;
+
+	ret = __snp_lookup_rmpentry(dumped_pfn, &e, &level);
+	if (ret) {
+		pr_info("Failed to read RMP entry for PFN 0x%llx, error %d\n",
+			dumped_pfn, ret);
+		return;
+	}
+
+	e_data = (u64 *)&e;
+	if (e.assigned) {
+		pr_info("RMP entry for PFN 0x%llx: [high=0x%016llx low=0x%016llx]\n",
+			dumped_pfn, e_data[1], e_data[0]);
+		return;
+	}
+
+	/*
+	 * If the RMP entry for a particular PFN is not in an assigned state,
+	 * then it is sometimes useful to get an idea of whether or not any RMP
+	 * entries for other PFNs within the same 2MB region are assigned, since
+	 * those too can affect the ability to access a particular PFN in

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

+			pfn++;
+			continue;
+		}
+

arch/x86/virt/svm/sev.c int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) · line 300

Quoted 7 lines · Michael Roth

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

+/*
+ * Dump the raw RMP entry for a particular PFN. These bits are documented in the
+ * PPR for a particular CPU model and provide useful information about how a
+ * particular PFN is being utilized by the kernel/firmware at the time certain
+ * unexpected events occur, such as RMP faults.
+ */
+static void sev_dump_rmpentry(u64 dumped_pfn)

Just "dump_rmentry"

s/dumped_pfn/pfn/g

Quoted 28 lines

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

+	struct rmpentry e;
+	u64 pfn, pfn_end;
+	int level, ret;
+	u64 *e_data;
+
+	ret = __snp_lookup_rmpentry(dumped_pfn, &e, &level);
+	if (ret) {
+		pr_info("Failed to read RMP entry for PFN 0x%llx, error %d\n",
+			dumped_pfn, ret);
+		return;
+	}
+
+	e_data = (u64 *)&e;
+	if (e.assigned) {
+		pr_info("RMP entry for PFN 0x%llx: [high=0x%016llx low=0x%016llx]\n",
+			dumped_pfn, e_data[1], e_data[0]);
+		return;
+	}
+
+	/*
+	 * If the RMP entry for a particular PFN is not in an assigned state,
+	 * then it is sometimes useful to get an idea of whether or not any RMP
+	 * entries for other PFNs within the same 2MB region are assigned, since
+	 * those too can affect the ability to access a particular PFN in
+	 * certain situations, such as when the PFN is being accessed via a 2MB
+	 * mapping in the host page table.
+	 */
+	pfn = ALIGN(dumped_pfn, PTRS_PER_PMD);

arch/x86/virt/svm/sev.c int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) · line 335

Quoted 6 lines

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

+	pfn_end = pfn + PTRS_PER_PMD;
+
+	while (pfn < pfn_end) {
+		ret = __snp_lookup_rmpentry(pfn, &e, &level);
+		if (ret) {
+			pr_info_ratelimited("Failed to read RMP entry for PFN 0x%llx\n", pfn);

Why ratelmited?

No need to print anything if you fail to read it - simply dump the range [pfn, pfn_end], _data[0], e_data[1] exactly *once* before the loop and inside the loop dump only the ones you can lookup...

Quoted 5 lines

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

+			pfn++;
+			continue;
+		}
+

arch/x86/virt/svm/sev.c int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) · line 349

Quoted 10 lines

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

+		if (e_data[0] || e_data[1]) {
+			pr_info("No assigned RMP entry for PFN 0x%llx, but the 2MB region contains populated RMP entries, e.g.: PFN 0x%llx: [high=0x%016llx low=0x%016llx]\n",
+				dumped_pfn, pfn, e_data[1], e_data[0]);
+			return;
+		}
+		pfn++;
+	}
+
+	pr_info("No populated RMP entries in the 2MB region containing PFN 0x%llx\n",
+		dumped_pfn);

... and then you don't need this one either.

arch/x86/virt/svm/sev.c int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) · line 360

Quoted 11 lines

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

+}
+
+void sev_dump_hva_rmpentry(unsigned long hva)
+{
+	unsigned int level;
+	pgd_t *pgd;
+	pte_t *pte;
+
+	pgd = __va(read_cr3_pa());
+	pgd += pgd_index(hva);
+	pte = lookup_address_in_pgd(pgd, hva, &level);

If this is using the current CR3, why aren't you simply using lookup_address() here without the need to read pgd?