[PATCH v4 0/7] Move SNP initialization to the CCP driver

[PATCH v4 0/7] Move SNP initialization to the CCP driver

[Tycho Andersen]

From: "Tycho Andersen (AMD)" <tycho@kernel.org>

Changes are:
* commit message fixes: snp -> sev, add arch/x86/ to path, capitalize things
* rename snp_set_hsave_pa() -> clear_hsave_pa(), since it's clearing the register
* snp_prepare_for_snp_init() -> snp_prepare()
* snp_x86_shutdown() -> snp_shutdown()
* 0 -> NULL, drop a newline in snp_shutdown()
* carry Herbert's acks as appropriate

v3 is here: https://lore.kernel.org/all/20260317162157.150842-1-tycho@kernel.org/

Tom Lendacky (2):
  x86/sev: Create a function to clear/zero the RMP
  crypto/ccp: Update HV_FIXED page states to allow freeing of memory

Tycho Andersen (AMD) (5):
  x86/sev: Create snp_prepare()
  x86/sev: Create snp_shutdown()
  x86/sev, crypto/ccp: Move SNP init to ccp driver
  x86/sev, crypto/ccp: Move HSAVE_PA setup to arch/x86/
  crypto/ccp: Implement SNP x86 shutdown

 arch/x86/include/asm/sev.h   |   4 ++
 arch/x86/virt/svm/sev.c      | 111 ++++++++++++++++++++++++-----------
 drivers/crypto/ccp/sev-dev.c |  62 ++++++++++---------
 include/linux/psp-sev.h      |   5 +-
 4 files changed, 120 insertions(+), 62 deletions(-)


base-commit: 2ca26dad836fb4cd18694ef85af7a71d2878b239
-- 
2.53.0

[PATCH v4 1/7] x86/sev: Create a function to clear/zero the RMP

[Tycho Andersen]

From: Tom Lendacky <thomas.lendacky@amd.com>

In preparation for delayed SNP initialization and disablement on shutdown,
create a function, clear_rmp(), that clears the RMP bookkeeping area
and the RMP entries.

Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
---
 arch/x86/virt/svm/sev.c | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index e35fac0a8a3d..025606969823 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -242,6 +242,32 @@ void __init snp_fixup_e820_tables(void)
 	}
 }
 
+static void clear_rmp(void)
+{
+	unsigned int i;
+	u64 val;
+
+	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
+		return;
+
+	/* Clearing the RMP while SNP is enabled will cause an exception */
+	rdmsrq(MSR_AMD64_SYSCFG, val);
+	if (WARN_ON_ONCE(val & MSR_AMD64_SYSCFG_SNP_EN))
+		return;
+
+	memset(rmp_bookkeeping, 0, RMPTABLE_CPU_BOOKKEEPING_SZ);
+
+	for (i = 0; i < rst_max_index; i++) {
+		struct rmp_segment_desc *desc;
+
+		desc = rmp_segment_table[i];
+		if (!desc)
+			continue;
+
+		memset(desc->rmp_entry, 0, desc->size);
+	}
+}
+
 static bool __init alloc_rmp_segment_desc(u64 segment_pa, u64 segment_size, u64 pa)
 {
 	u64 rst_index, rmp_segment_size_max;
@@ -484,7 +510,6 @@ static bool __init setup_rmptable(void)
  */
 int __init snp_rmptable_init(void)
 {
-	unsigned int i;
 	u64 val;
 
 	if (WARN_ON_ONCE(!cc_platform_has(CC_ATTR_HOST_SEV_SNP)))
@@ -504,19 +529,7 @@ int __init snp_rmptable_init(void)
 	if (val & MSR_AMD64_SYSCFG_SNP_EN)
 		goto skip_enable;
 
-	/* Zero out the RMP bookkeeping area */
-	memset(rmp_bookkeeping, 0, RMPTABLE_CPU_BOOKKEEPING_SZ);
-
-	/* Zero out the RMP entries */
-	for (i = 0; i < rst_max_index; i++) {
-		struct rmp_segment_desc *desc;
-
-		desc = rmp_segment_table[i];
-		if (!desc)
-			continue;
-
-		memset(desc->rmp_entry, 0, desc->size);
-	}
+	clear_rmp();
 
 	/* MtrrFixDramModEn must be enabled on all the CPUs prior to enabling SNP. */
 	on_each_cpu(mfd_enable, NULL, 1);
-- 
2.53.0

[PATCH v4 2/7] x86/sev: Create snp_prepare()

[Tycho Andersen]

From: "Tycho Andersen (AMD)" <tycho@kernel.org>

In preparation for delayed SNP initialization, create a function
snp_prepare() that does the necessary architecture setup.
Export this function for the ccp module to allow it to do the setup as
necessary.

Also move {mfd,snp}_enable out of the __init section, since these will be
called later.

Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 arch/x86/include/asm/sev.h |  2 ++
 arch/x86/virt/svm/sev.c    | 46 ++++++++++++++++++++++----------------
 2 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 0e6c0940100f..2140e26dec6c 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -661,6 +661,7 @@ static inline void snp_leak_pages(u64 pfn, unsigned int pages)
 {
 	__snp_leak_pages(pfn, pages, true);
 }
+void snp_prepare(void);
 #else
 static inline bool snp_probe_rmptable_info(void) { return false; }
 static inline int snp_rmptable_init(void) { return -ENOSYS; }
@@ -677,6 +678,7 @@ static inline void __snp_leak_pages(u64 pfn, unsigned int npages, bool dump_rmp)
 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 void snp_prepare(void) {}
 #endif
 
 #endif
diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index 025606969823..6f4c3f6e2082 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -132,7 +132,7 @@ static unsigned long snp_nr_leaked_pages;
 #undef pr_fmt
 #define pr_fmt(fmt)	"SEV-SNP: " fmt
 
-static __init void mfd_enable(void *arg)
+static void mfd_enable(void *arg)
 {
 	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
 		return;
@@ -140,7 +140,7 @@ static __init void mfd_enable(void *arg)
 	msr_set_bit(MSR_AMD64_SYSCFG, MSR_AMD64_SYSCFG_MFDM_BIT);
 }
 
-static __init void snp_enable(void *arg)
+static void snp_enable(void *arg)
 {
 	u64 val;
 
@@ -503,6 +503,30 @@ static bool __init setup_rmptable(void)
 	return true;
 }
 
+void snp_prepare(void)
+{
+	u64 val;
+
+	/*
+	 * Check if SEV-SNP is already enabled, this can happen in case of
+	 * kexec boot.
+	 */
+	rdmsrq(MSR_AMD64_SYSCFG, val);
+	if (val & MSR_AMD64_SYSCFG_SNP_EN)
+		return;
+
+	clear_rmp();
+
+	/*
+	 * MtrrFixDramModEn is not shared between threads on a core,
+	 * therefore it must be set on all CPUs prior to enabling SNP.
+	 */
+	on_each_cpu(mfd_enable, NULL, 1);
+
+	on_each_cpu(snp_enable, NULL, 1);
+}
+EXPORT_SYMBOL_FOR_MODULES(snp_prepare, "ccp");
+
 /*
  * Do the necessary preparations which are verified by the firmware as
  * described in the SNP_INIT_EX firmware command description in the SNP
@@ -510,8 +534,6 @@ static bool __init setup_rmptable(void)
  */
 int __init snp_rmptable_init(void)
 {
-	u64 val;
-
 	if (WARN_ON_ONCE(!cc_platform_has(CC_ATTR_HOST_SEV_SNP)))
 		return -ENOSYS;
 
@@ -521,22 +543,8 @@ int __init snp_rmptable_init(void)
 	if (!setup_rmptable())
 		return -ENOSYS;
 
-	/*
-	 * Check if SEV-SNP is already enabled, this can happen in case of
-	 * kexec boot.
-	 */
-	rdmsrq(MSR_AMD64_SYSCFG, val);
-	if (val & MSR_AMD64_SYSCFG_SNP_EN)
-		goto skip_enable;
-
-	clear_rmp();
-
-	/* MtrrFixDramModEn must be enabled on all the CPUs prior to enabling SNP. */
-	on_each_cpu(mfd_enable, NULL, 1);
-
-	on_each_cpu(snp_enable, NULL, 1);
+	snp_prepare();
 
-skip_enable:
 	/*
 	 * Setting crash_kexec_post_notifiers to 'true' to ensure that SNP panic
 	 * notifier is invoked to do SNP IOMMU shutdown before kdump.
-- 
2.53.0

[PATCH v4 3/7] x86/sev: Create snp_shutdown()

[Tycho Andersen]

From: "Tycho Andersen (AMD)" <tycho@kernel.org>

After SNP_SHUTDOWN, two things should be done:

1. clear the RMP table
2. disable MFDM to prevent the FW_WARN in k8_check_syscfg_dram_mod_en() in
   the event of a kexec

Create and export to the CCP driver a function that does them.

Also change the MFDM helper to allow for disabling the bit, since the SNP
x86 shutdown path needs to disable MFDM. The comment for
k8_check_syscfg_dram_mod_en() notes, the "BIOS" is supposed clear it, or
the kernel in the case of module unload and shutdown followed by kexec.

Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 arch/x86/include/asm/sev.h |  2 ++
 arch/x86/virt/svm/sev.c    | 22 +++++++++++++++++++---
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 2140e26dec6c..09e605c85de4 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -662,6 +662,7 @@ static inline void snp_leak_pages(u64 pfn, unsigned int pages)
 	__snp_leak_pages(pfn, pages, true);
 }
 void snp_prepare(void);
+void snp_shutdown(void);
 #else
 static inline bool snp_probe_rmptable_info(void) { return false; }
 static inline int snp_rmptable_init(void) { return -ENOSYS; }
@@ -679,6 +680,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 void snp_prepare(void) {}
+static inline void snp_shutdown(void) {}
 #endif
 
 #endif
diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index 6f4c3f6e2082..bcb791a56053 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -132,12 +132,15 @@ static unsigned long snp_nr_leaked_pages;
 #undef pr_fmt
 #define pr_fmt(fmt)	"SEV-SNP: " fmt
 
-static void mfd_enable(void *arg)
+static void mfd_reconfigure(void *arg)
 {
 	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
 		return;
 
-	msr_set_bit(MSR_AMD64_SYSCFG, MSR_AMD64_SYSCFG_MFDM_BIT);
+	if (arg)
+		msr_set_bit(MSR_AMD64_SYSCFG, MSR_AMD64_SYSCFG_MFDM_BIT);
+	else
+		msr_clear_bit(MSR_AMD64_SYSCFG, MSR_AMD64_SYSCFG_MFDM_BIT);
 }
 
 static void snp_enable(void *arg)
@@ -521,12 +524,25 @@ void snp_prepare(void)
 	 * MtrrFixDramModEn is not shared between threads on a core,
 	 * therefore it must be set on all CPUs prior to enabling SNP.
 	 */
-	on_each_cpu(mfd_enable, NULL, 1);
+	on_each_cpu(mfd_reconfigure, (void *)1, 1);
 
 	on_each_cpu(snp_enable, NULL, 1);
 }
 EXPORT_SYMBOL_FOR_MODULES(snp_prepare, "ccp");
 
+void snp_shutdown(void)
+{
+	u64 syscfg;
+
+	rdmsrq(MSR_AMD64_SYSCFG, syscfg);
+	if (syscfg & MSR_AMD64_SYSCFG_SNP_EN)
+		return;
+
+	clear_rmp();
+	on_each_cpu(mfd_reconfigure, NULL, 1);
+}
+EXPORT_SYMBOL_FOR_MODULES(snp_shutdown, "ccp");
+
 /*
  * Do the necessary preparations which are verified by the firmware as
  * described in the SNP_INIT_EX firmware command description in the SNP
-- 
2.53.0

[PATCH v4 4/7] x86/sev, crypto/ccp: Move SNP init to ccp driver

[Tycho Andersen]

From: "Tycho Andersen (AMD)" <tycho@kernel.org>

Use the new snp_prepare() to initialize SNP from the ccp driver instead of
at boot time. This means that SNP is not enabled unless it is really going
to be used (i.e. kvm_amd loads the ccp driver automatically).

Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 arch/x86/virt/svm/sev.c      | 2 --
 drivers/crypto/ccp/sev-dev.c | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index bcb791a56053..bf0572c0c16e 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -559,8 +559,6 @@ int __init snp_rmptable_init(void)
 	if (!setup_rmptable())
 		return -ENOSYS;
 
-	snp_prepare();
-
 	/*
 	 * Setting crash_kexec_post_notifiers to 'true' to ensure that SNP panic
 	 * notifier is invoked to do SNP IOMMU shutdown before kdump.
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index aebf4dad545e..4915b0125e8d 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1373,6 +1373,8 @@ static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
 		return -EOPNOTSUPP;
 	}
 
+	snp_prepare();
+
 	/* SNP_INIT requires MSR_VM_HSAVE_PA to be cleared on all CPUs. */
 	on_each_cpu(snp_set_hsave_pa, NULL, 1);
 
-- 
2.53.0

[PATCH v4 5/7] x86/sev, crypto/ccp: Move HSAVE_PA setup to arch/x86/

[Tycho Andersen]

From: "Tycho Andersen (AMD)" <tycho@kernel.org>

Now that there is snp_prepare() that indicates when the CCP driver wants to
prepare the architecture for SNP_INIT(_EX), move this architecture-specific
bit of code to a more sensible place.

Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 arch/x86/virt/svm/sev.c      | 8 ++++++++
 drivers/crypto/ccp/sev-dev.c | 8 --------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index bf0572c0c16e..d9e0eda7993f 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -506,6 +506,11 @@ static bool __init setup_rmptable(void)
 	return true;
 }
 
+static void clear_hsave_pa(void *arg)
+{
+	wrmsrq(MSR_VM_HSAVE_PA, 0);
+}
+
 void snp_prepare(void)
 {
 	u64 val;
@@ -527,6 +532,9 @@ void snp_prepare(void)
 	on_each_cpu(mfd_reconfigure, (void *)1, 1);
 
 	on_each_cpu(snp_enable, NULL, 1);
+
+	/* SNP_INIT requires MSR_VM_HSAVE_PA to be cleared on all CPUs. */
+	on_each_cpu(clear_hsave_pa, NULL, 1);
 }
 EXPORT_SYMBOL_FOR_MODULES(snp_prepare, "ccp");
 
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 4915b0125e8d..47cb8fca4e6c 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1076,11 +1076,6 @@ static inline int __sev_do_init_locked(int *psp_ret)
 		return __sev_init_locked(psp_ret);
 }
 
-static void snp_set_hsave_pa(void *arg)
-{
-	wrmsrq(MSR_VM_HSAVE_PA, 0);
-}
-
 /* Hypervisor Fixed pages API interface */
 static void snp_hv_fixed_pages_state_update(struct sev_device *sev,
 					    enum snp_hv_fixed_pages_state page_state)
@@ -1375,9 +1370,6 @@ static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
 
 	snp_prepare();
 
-	/* SNP_INIT requires MSR_VM_HSAVE_PA to be cleared on all CPUs. */
-	on_each_cpu(snp_set_hsave_pa, NULL, 1);
-
 	/*
 	 * Starting in SNP firmware v1.52, the SNP_INIT_EX command takes a list
 	 * of system physical address ranges to convert into HV-fixed page
-- 
2.53.0

[PATCH v4 6/7] crypto/ccp: Implement SNP x86 shutdown

[Tycho Andersen]

From: "Tycho Andersen (AMD)" <tycho@kernel.org>

The SEV firmware has support to disable SNP during an SNP_SHUTDOWN_EX
command. Verify that this support is available and set the flag so that SNP
is disabled when it is not being used. In cases where SNP is disabled, skip
the call to amd_iommu_snp_disable(), as all of the IOMMU pages have already
been made shared. Also skip the panic case, since snp_shutdown() does
IPIs.

Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 drivers/crypto/ccp/sev-dev.c | 41 +++++++++++++++++++++---------------
 include/linux/psp-sev.h      |  5 ++++-
 2 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 47cb8fca4e6c..366303ff6466 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -2039,6 +2039,8 @@ static int __sev_snp_shutdown_locked(int *error, bool panic)
 	memset(&data, 0, sizeof(data));
 	data.len = sizeof(data);
 	data.iommu_snp_shutdown = 1;
+	if (sev->snp_feat_info_0.ecx & SNP_X86_SHUTDOWN_SUPPORTED)
+		data.x86_snp_shutdown = 1;
 
 	/*
 	 * If invoked during panic handling, local interrupts are disabled
@@ -2072,23 +2074,28 @@ static int __sev_snp_shutdown_locked(int *error, bool panic)
 		return ret;
 	}
 
-	/*
-	 * SNP_SHUTDOWN_EX with IOMMU_SNP_SHUTDOWN set to 1 disables SNP
-	 * enforcement by the IOMMU and also transitions all pages
-	 * associated with the IOMMU to the Reclaim state.
-	 * Firmware was transitioning the IOMMU pages to Hypervisor state
-	 * before version 1.53. But, accounting for the number of assigned
-	 * 4kB pages in a 2M page was done incorrectly by not transitioning
-	 * to the Reclaim state. This resulted in RMP #PF when later accessing
-	 * the 2M page containing those pages during kexec boot. Hence, the
-	 * firmware now transitions these pages to Reclaim state and hypervisor
-	 * needs to transition these pages to shared state. SNP Firmware
-	 * version 1.53 and above are needed for kexec boot.
-	 */
-	ret = amd_iommu_snp_disable();
-	if (ret) {
-		dev_err(sev->dev, "SNP IOMMU shutdown failed\n");
-		return ret;
+	if (data.x86_snp_shutdown) {
+		if (!panic)
+			snp_shutdown();
+	} else {
+		/*
+		 * SNP_SHUTDOWN_EX with IOMMU_SNP_SHUTDOWN set to 1 disables SNP
+		 * enforcement by the IOMMU and also transitions all pages
+		 * associated with the IOMMU to the Reclaim state.
+		 * Firmware was transitioning the IOMMU pages to Hypervisor state
+		 * before version 1.53. But, accounting for the number of assigned
+		 * 4kB pages in a 2M page was done incorrectly by not transitioning
+		 * to the Reclaim state. This resulted in RMP #PF when later accessing
+		 * the 2M page containing those pages during kexec boot. Hence, the
+		 * firmware now transitions these pages to Reclaim state and hypervisor
+		 * needs to transition these pages to shared state. SNP Firmware
+		 * version 1.53 and above are needed for kexec boot.
+		 */
+		ret = amd_iommu_snp_disable();
+		if (ret) {
+			dev_err(sev->dev, "SNP IOMMU shutdown failed\n");
+			return ret;
+		}
 	}
 
 	snp_leak_hv_fixed_pages();
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 69ffa4b4d1fa..d5099a2baca5 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -829,12 +829,14 @@ struct sev_data_range_list {
  *
  * @len: length of the command buffer read by the PSP
  * @iommu_snp_shutdown: Disable enforcement of SNP in the IOMMU
+ * @x86_snp_shutdown: Disable SNP on all cores
  * @rsvd1: reserved
  */
 struct sev_data_snp_shutdown_ex {
 	u32 len;
 	u32 iommu_snp_shutdown:1;
-	u32 rsvd1:31;
+	u32 x86_snp_shutdown:1;
+	u32 rsvd1:30;
 } __packed;
 
 /**
@@ -891,6 +893,7 @@ struct snp_feature_info {
 } __packed;
 
 /* Feature bits in ECX */
+#define SNP_X86_SHUTDOWN_SUPPORTED		BIT(1)
 #define SNP_RAPL_DISABLE_SUPPORTED		BIT(2)
 #define SNP_CIPHER_TEXT_HIDING_SUPPORTED	BIT(3)
 #define SNP_AES_256_XTS_POLICY_SUPPORTED	BIT(4)
-- 
2.53.0

[PATCH v4 7/7] crypto/ccp: Update HV_FIXED page states to allow freeing of memory

[Tycho Andersen]

From: Tom Lendacky <thomas.lendacky@amd.com>

After SNP is disabled, any pages allocated as HV_FIXED can now be freed.
Update the page state of these pages and the snp_leak_hv_fixed_pages()
function to free pages on SNP_SHUTDOWN.

Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 drivers/crypto/ccp/sev-dev.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 366303ff6466..939fa8aa155c 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1219,7 +1219,7 @@ static void snp_add_hv_fixed_pages(struct sev_device *sev, struct sev_data_range
 
 static void snp_leak_hv_fixed_pages(void)
 {
-	struct snp_hv_fixed_pages_entry *entry;
+	struct snp_hv_fixed_pages_entry *entry, *nentry;
 
 	/* List is protected by sev_cmd_mutex */
 	lockdep_assert_held(&sev_cmd_mutex);
@@ -1227,10 +1227,16 @@ static void snp_leak_hv_fixed_pages(void)
 	if (list_empty(&snp_hv_fixed_pages))
 		return;
 
-	list_for_each_entry(entry, &snp_hv_fixed_pages, list)
-		if (entry->page_state == HV_FIXED)
+	list_for_each_entry_safe(entry, nentry, &snp_hv_fixed_pages, list) {
+		if (entry->free && entry->page_state != HV_FIXED)
+			__free_pages(entry->page, entry->order);
+		else
 			__snp_leak_pages(page_to_pfn(entry->page),
 					 1 << entry->order, false);
+
+		list_del(&entry->list);
+		kfree(entry);
+	}
 }
 
 bool sev_is_snp_ciphertext_hiding_supported(void)
@@ -2077,6 +2083,7 @@ static int __sev_snp_shutdown_locked(int *error, bool panic)
 	if (data.x86_snp_shutdown) {
 		if (!panic)
 			snp_shutdown();
+		snp_hv_fixed_pages_state_update(sev, ALLOCATED);
 	} else {
 		/*
 		 * SNP_SHUTDOWN_EX with IOMMU_SNP_SHUTDOWN set to 1 disables SNP
-- 
2.53.0

Re: [PATCH v4 0/7] Move SNP initialization to the CCP driver

[Borislav Petkov]

On March 24, 2026 4:12:54 PM UTC, Tycho Andersen <tycho@kernel.org> wrote:
>From: "Tycho Andersen (AMD)" <tycho@kernel.org>
>
>Changes are:
>* commit message fixes: snp -> sev, add arch/x86/ to path, capitalize things
>* rename snp_set_hsave_pa() -> clear_hsave_pa(), since it's clearing the register
>* snp_prepare_for_snp_init() -> snp_prepare()
>* snp_x86_shutdown() -> snp_shutdown()
>* 0 -> NULL, drop a newline in snp_shutdown()
>* carry Herbert's acks as appropriate
>
>v3 is here: https://lore.kernel.org/all/20260317162157.150842-1-tycho@kernel.org/
>
>Tom Lendacky (2):
>  x86/sev: Create a function to clear/zero the RMP
>  crypto/ccp: Update HV_FIXED page states to allow freeing of memory
>
>Tycho Andersen (AMD) (5):
>  x86/sev: Create snp_prepare()
>  x86/sev: Create snp_shutdown()
>  x86/sev, crypto/ccp: Move SNP init to ccp driver
>  x86/sev, crypto/ccp: Move HSAVE_PA setup to arch/x86/
>  crypto/ccp: Implement SNP x86 shutdown
>
> arch/x86/include/asm/sev.h   |   4 ++
> arch/x86/virt/svm/sev.c      | 111 ++++++++++++++++++++++++-----------
> drivers/crypto/ccp/sev-dev.c |  62 ++++++++++---------
> include/linux/psp-sev.h      |   5 +-
> 4 files changed, 120 insertions(+), 62 deletions(-)
>
>
>base-commit: 2ca26dad836fb4cd18694ef85af7a71d2878b239

Sachiko has some questions:

https://sashiko.dev/#/patchset/20260324161301.1353976-1-tycho%40kernel.org
-- 
Small device. Typos and formatting crap

Re: [PATCH v4 0/7] Move SNP initialization to the CCP driver

[Tycho Andersen]

On Wed, Mar 25, 2026 at 09:07:42AM +0000, Borislav Petkov wrote:
> Sachiko has some questions:
> 
> https://sashiko.dev/#/patchset/20260324161301.1353976-1-tycho%40kernel.org

Interesting, review-prompts directly didn't complain about these,

> Could this lead to a NULL pointer dereference in clear_rmp() if
> setup_rmptable() fails during early boot?
>
> If setup_rmptable() returns false (for example, if the BIOS did not
> reserve memory for the RMP table), it exits without allocating
> rmp_bookkeeping or rmp_segment_table, but leaves the CC_ATTR_HOST_SEV_SNP
> platform attribute active.

If setup_rmptable() fails, snp_rmptable_init() returns -ENOSYS and
iommu_snp_enable() clears CC_ATTR_HOST_SEV_SNP.
__sev_snp_init_locked() checks CC_ATTR_HOST_SEV_SNP before doing
anything else, so I think this is not possible.

You did complain about this call chain before off-list though, maybe
we should clear CC_ATTR_HOST_SEV_SNP in more places directly vs.
returning an errno to make it more obvious?

> Is there a race condition with CPU hotplug here?
>
> Since snp_prepare() lacks cpus_read_lock() protection, a CPU could
> come online exactly between the two passes, missing the mfd_enable step
> but receiving snp_enable.

I think it makes sense to do the operations on the same set of CPUs
even if we don't support hotplug. I will resend with
cpus_read_lock().

> Additionally, without a CPU hotplug state callback (such as
> cpuhp_setup_state()), any CPUs brought online after snp_prepare()
> completes will miss these MSR configurations completely. If an SNP
> guest is later scheduled on one of these uninitialized CPUs, will it cause
> hardware exceptions?

Yes, WONTFIX.

> Can deferring this call cause a NULL pointer dereference? Previously,
> snp_prepare() was called only after setup_rmptable() succeeded. By
> moving it to the CCP driver, it may be called unconditionally as long
> as CC_ATTR_HOST_SEV_SNP is true, even if setup_rmptable() was skipped
> or failed. Does clear_rmp() inside snp_prepare() safely handle the
> uninitialized rmp_bookkeeping pointer?

Same as above, CC_ATTR_HOST_SEV_SNP should perhaps be cleared in more
places.

> Also, moving snp_prepare() out of early boot might expand the window for
> CPU hotplug events. Does this create an asymmetry where newly onlined
> CPUs miss the MSR_AMD64_SYSCFG_SNP_EN configuration applied by
> on_each_cpu(snp_enable, NULL, 1) because the SEV subsystem lacks a CPU
> hotplug callback? Could this cause host crashes when KVM schedules VMs
> on these uninitialized CPUs?

Same as above, WONTFIX.

> Additionally, does deferring SNP enablement in the hardware coordinate
> with the IOMMU feature degradation? The AMD IOMMU driver evaluates
> amd_iommu_snp_en at early boot and permanently disables identity mapping
> domains and IOMMUv2. Will the system suffer this permanent penalty even
> if the CCP driver is never loaded?

This is about the bios setting:

        amd_iommu_snp_en = check_feature(FEATURE_SNP);
        if (!amd_iommu_snp_en) {
                pr_warn("SNP: IOMMU SNP feature not enabled, SNP cannot be supported.\n");
                goto disable_snp;
        }

so I don't think the question really makes sense.

> Could placing snp_prepare() here cause prolonged blocking of all SEV
> operations? Since __sev_snp_init_locked() holds sev_cmd_mutex, the long
> execution time of clear_rmp() (which zeroes the entire RMP table and scales
> with system RAM) might block SEV firmware ioctls and VM lifecycle operations
> globally for several seconds.

Yes, it does :). Actually the firmware call is the expensive part, not
the rmp zeroing, but it definitely blocks for a while.

> This isn't a bug introduced by this commit, but if SEV initialization
> fails and KVM is actively running normal VMs, could a userspace process
> trigger this code path via /dev/sev ioctls (e.g., SEV_PDH_GEN) and zero out
> MSR_VM_HSAVE_PA globally? Would the next VMRUN execution for an active VM
> trigger a general protection fault and crash the host?

Oof, yes. I wonder if we shouldn't set psp_dead = true if
sev_platform_init() sees an error. After this series, if
the error is e.g. init_ex_path failure, you can unload, fix the
failure, and try again.

>  	if (sev_version_greater_or_equal(SNP_MIN_API_MAJOR, 52)) {
[ ... ]
>  		memset(&data, 0, sizeof(data));
[ ... ]
>  		data.tio_en = tio_supp && sev_tio_enabled && amd_iommu_sev_tio_supported();
[ ... ]
>  	} else {
>  		cmd = SEV_CMD_SNP_INIT;
>  		arg = NULL;
>  	}
> This isn't a bug introduced by this commit, but is the stack variable
> data left uninitialized when taking the else branch? Since data.tio_en is
> later evaluated unconditionally, could stack garbage cause it to evaluate
> to true, leading to erroneous attempts to allocate pages and initialize
> SEV-TIO on unsupported hardware?

No, arg is the actual pointer passed, and it is set to NULL. non-EX
init doesn't support TIO anyway...

> Also, regarding the bounds check in snp_filter_reserved_mem_regions()
> called via walk_iomem_res_desc(): does the check
> if ((range_list->num_elements * 16 + 8) > PAGE_SIZE)
> allow an off-by-one heap buffer overflow?
>
> If range_list->num_elements is 255, 255 * 16 + 8 = 4088, which is <= 4096.
> Writing range->base (8 bytes) fills 4088-4095, but writing range->page_count
> (4 bytes) would write to 4096-4099, overflowing the kzalloc-allocated
> PAGE_SIZE buffer.

Yes, this also looks real, and needs:

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 939fa8aa155c..3642226c0fc0 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1328,10 +1328,11 @@ static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
 	size_t size;
 
 	/*
-	 * Ensure the list of HV_FIXED pages that will be passed to firmware
-	 * do not exceed the page-sized argument buffer.
+	 * Ensure the list of HV_FIXED pages including the one we are about to
+	 * use that will be passed to firmware do not exceed the page-sized
+	 * argument buffer.
 	 */
-	if ((range_list->num_elements * sizeof(struct sev_data_range) +
+	if (((range_list->num_elements + 1) * sizeof(struct sev_data_range) +
 	     sizeof(struct sev_data_range_list)) > PAGE_SIZE)
 		return -E2BIG;

I have another bug that review-prompts found unrelated to this series.
I can put the two fixes above with that or include them here, let me
know what you prefer. Either way I'll resend one more with
cpus_read_lock().

Thanks,

Tycho

Re: [PATCH v4 0/7] Move SNP initialization to the CCP driver

[Borislav Petkov]

On Wed, Mar 25, 2026 at 09:25:05AM -0600, Tycho Andersen wrote:
> You did complain about this call chain before off-list though, maybe
> we should clear CC_ATTR_HOST_SEV_SNP in more places directly vs.
> returning an errno to make it more obvious?

iommu_snp_enable() already has the disable_snp label where it does that. If
you wanna clear the flag in snp_rmptable_init() you'll simply have to sprinkle
flag clearing across more places. I dunno if that is really better frankly...

> > Is there a race condition with CPU hotplug here?
> >
> > Since snp_prepare() lacks cpus_read_lock() protection, a CPU could
> > come online exactly between the two passes, missing the mfd_enable step
> > but receiving snp_enable.
> 
> I think it makes sense to do the operations on the same set of CPUs
> even if we don't support hotplug. I will resend with
> cpus_read_lock().

Right, especially if that function would run now at arbitrary points in time
- as this is the main reason we're doing this whole dance.

BUT!

If you grab the hotplug lock and you realize that you don't have all CPUs
online and since we zapped the hotplug notifier and since SNP enable would
fail anyway, we should simply check if all CPUs are online and return error
if not instead of running the IPIs.

> > Could placing snp_prepare() here cause prolonged blocking of all SEV
> > operations? Since __sev_snp_init_locked() holds sev_cmd_mutex, the long
> > execution time of clear_rmp() (which zeroes the entire RMP table and scales
> > with system RAM) might block SEV firmware ioctls and VM lifecycle operations
> > globally for several seconds.
> 
> Yes, it does :). Actually the firmware call is the expensive part, not
> the rmp zeroing, but it definitely blocks for a while.

But we've delayed this init to the latest possible moment.

So much so that when this "prolonged blocking" happens, that is very much
absolutely indispensable.

And besides, we're not really running SNP guests here so to me that feedback
doesn't really make much sense...

> > This isn't a bug introduced by this commit, but if SEV initialization
> > fails and KVM is actively running normal VMs, could a userspace process
> > trigger this code path via /dev/sev ioctls (e.g., SEV_PDH_GEN) and zero out
> > MSR_VM_HSAVE_PA globally? Would the next VMRUN execution for an active VM
> > trigger a general protection fault and crash the host?
> 
> Oof, yes. I wonder if we shouldn't set psp_dead = true if
> sev_platform_init() sees an error. After this series, if
> the error is e.g. init_ex_path failure, you can unload, fix the
> failure, and try again.

Let's slow down here.

So the LLM is talking about a use case where you have unencrypted VMs running
and then userspace can go and poke /dev/sev, zero out that MSR_VM_HSAVE_PA in
the process but that's the MSR which contains the physical address where host
state is saved on a VMRUN and if that MSR is cleared because SNP init needs
it, the machine would explode.

Ok, so far so good, I don't see anything wrong with the use case - nothing's
stopping the admin from modprobing ccp and then launching SNP guests.

Now, you're talking about some psp_dead - yet another silly boolean folks love
to introduce in the SEV code - and then something about that init_ex_path
hack. I don't know what that means, frankly.

What my simple intuition says is, *if* snp_prepare() runs,  then no guests
should do VMRUN anymore until they're ready to do that again.

Which begs the question: if snp_prepare() clears MSR_VM_HSAVE_PA, how can you
even run normal VMs after that?

Hmmm.

> >  	if (sev_version_greater_or_equal(SNP_MIN_API_MAJOR, 52)) {
> [ ... ]
> >  		memset(&data, 0, sizeof(data));
> [ ... ]
> >  		data.tio_en = tio_supp && sev_tio_enabled && amd_iommu_sev_tio_supported();
> [ ... ]
> >  	} else {
> >  		cmd = SEV_CMD_SNP_INIT;
> >  		arg = NULL;
> >  	}
> > This isn't a bug introduced by this commit, but is the stack variable
> > data left uninitialized when taking the else branch? Since data.tio_en is
> > later evaluated unconditionally, could stack garbage cause it to evaluate
> > to true, leading to erroneous attempts to allocate pages and initialize
> > SEV-TIO on unsupported hardware?
> 
> No, arg is the actual pointer passed, and it is set to NULL. non-EX
> init doesn't support TIO anyway...

This code is a total mess.

struct sev_data_snp_init_ex data;
...

... the else branch executes so you do

	arg = NULL;

...

and now *after* it, you're testing data:

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

Which *is* uninitialized stack data.

So the AI is right AFAICT.

If I were the AI, I'd say, what a total mess this code is. This
__sev_snp_init_locked() thing needs serious cleanup because it is too
convoluted to exist. And silly bugs like that creep in, as a result.

If I were maintaining this, I'd enforce a mandatory driver cleanup before any
new features come in. For example, __sev_snp_init_locked() needs proper
splitting and streamlining instead of doing gazillion things and with
a conditional in it which has consequences about the code after it. :-\

> > Also, regarding the bounds check in snp_filter_reserved_mem_regions()
> > called via walk_iomem_res_desc(): does the check
> > if ((range_list->num_elements * 16 + 8) > PAGE_SIZE)
> > allow an off-by-one heap buffer overflow?
> >
> > If range_list->num_elements is 255, 255 * 16 + 8 = 4088, which is <= 4096.
> > Writing range->base (8 bytes) fills 4088-4095, but writing range->page_count
> > (4 bytes) would write to 4096-4099, overflowing the kzalloc-allocated
> > PAGE_SIZE buffer.

That's a good catch.

> Yes, this also looks real, and needs:
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 939fa8aa155c..3642226c0fc0 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1328,10 +1328,11 @@ static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
>  	size_t size;
>  
>  	/*
> -	 * Ensure the list of HV_FIXED pages that will be passed to firmware
> -	 * do not exceed the page-sized argument buffer.
> +	 * Ensure the list of HV_FIXED pages including the one we are about to

No "we" - use passive voice pls.

> +	 * use that will be passed to firmware do not exceed the page-sized
> +	 * argument buffer.
>  	 */
> -	if ((range_list->num_elements * sizeof(struct sev_data_range) +
> +	if (((range_list->num_elements + 1) * sizeof(struct sev_data_range) +
>  	     sizeof(struct sev_data_range_list)) > PAGE_SIZE)
>  		return -E2BIG;

Yes, this is a short-term fix for stable.

But that "handling" in there is just nuts. You have this:

	snp_range_list = kzalloc(PAGE_SIZE, GFP_KERNEL);

	...

        rc = walk_iomem_res_desc(IORES_DESC_NONE, IORESOURCE_MEM, 0, ~0,
				  snp_range_list, snp_filter_reserved_mem_regions);

That function calls

		snp_filter_reserved_mem_regions(resource *, snp_range_list);

and that resource walking BIOS-like yuck code is iterating over the resources
and calling ->func each time.

So we pass in a *page* but then that range list *array* we turn it into, is
not a multiple of the element size of 24 AFAICT.

So that last element can trail and overflow heap. Lovely.

So this thing needs complete change: *actually* pass in an array instead of
a page so that you're not trailing, check the current element index against
the array size instead of doing obscure struct size calculations which are
visible only to very motivated reviewers like an AI and then just get rid of
the overflow possibility in the first place.

> I have another bug that review-prompts found unrelated to this series.
> I can put the two fixes above with that or include them here, let me
> know what you prefer. Either way I'll resend one more with
> cpus_read_lock().

So, your set is kinda ready to go and I'll take it but if I were you, right
after this, I'll sit down and fix all that crap in sev-dev.c. Do a nice
patchset, simple backportable fixes first and proper refactoring and cleanup
ontop.

Just piling up more stuff ontop is not maintanable in the long run.

But hey, I'm not maintaining this thing so I'm off the chain here for
a change!

:-P :-P

-- 
Regards/Gruss,
    Boris.

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