[PATCH 0/5] x86/sev: Fix SNP host late disable

[PATCH 0/5] x86/sev: Fix SNP host late disable

[Borislav Petkov]

From: "Borislav Petkov (AMD)" <bp@alien8.de>

Hi,

the intention to track SNP host status with the CPU feature bit
X86_FEATURE_SEV_SNP was all fine and dandy but that can't work if stuff
needs to be disabled late, after alternatives patching - see patch 5.

Therefore, convert the SNP status tracking to a cc_platform*() bit.

The first two are long overdue cleanups.

If no objections, 3-5 should go in now so that 6.9 releases fixed.

Thx.

Borislav Petkov (AMD) (5):
  x86/alternatives: Remove a superfluous newline in _static_cpu_has()
  x86/alternatives: Catch late X86_FEATURE modifiers
  x86/kvm/Kconfig: Have KVM_AMD_SEV select ARCH_HAS_CC_PLATFORM
  x86/cc: Add cc_platform_set/_clear() helpers
  x86/CPU/AMD: Track SNP host status with cc_platform_*()

 arch/x86/coco/core.c               | 52 ++++++++++++++++++++++++++++++
 arch/x86/include/asm/cpufeature.h  | 11 ++++---
 arch/x86/include/asm/sev.h         |  4 +--
 arch/x86/kernel/cpu/amd.c          | 38 +++++++++++++---------
 arch/x86/kernel/cpu/cpuid-deps.c   |  3 ++
 arch/x86/kernel/cpu/mtrr/generic.c |  2 +-
 arch/x86/kernel/sev.c              | 10 ------
 arch/x86/kvm/Kconfig               |  1 +
 arch/x86/kvm/svm/sev.c             |  2 +-
 arch/x86/virt/svm/sev.c            | 26 ++++++++++-----
 drivers/crypto/ccp/sev-dev.c       |  2 +-
 drivers/iommu/amd/init.c           |  4 ++-
 include/linux/cc_platform.h        | 12 +++++++
 13 files changed, 124 insertions(+), 43 deletions(-)

-- 
2.43.0

[PATCH 1/5] x86/alternatives: Remove a superfluous newline in _static_cpu_has()

[Borislav Petkov]

From: "Borislav Petkov (AMD)" <bp@alien8.de>

No functional changes.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
---
 arch/x86/include/asm/cpufeature.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index a1273698fc43..1ef620d508f4 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -168,8 +168,7 @@ extern void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int bit);
  */
 static __always_inline bool _static_cpu_has(u16 bit)
 {
-	asm goto(
-		ALTERNATIVE_TERNARY("jmp 6f", %P[feature], "", "jmp %l[t_no]")
+	asm goto(ALTERNATIVE_TERNARY("jmp 6f", %P[feature], "", "jmp %l[t_no]")
 		".pushsection .altinstr_aux,\"ax\"\n"
 		"6:\n"
 		" testb %[bitnum]," _ASM_RIP(%P[cap_byte]) "\n"
-- 
2.43.0

[PATCH 2/5] x86/alternatives: Catch late X86_FEATURE modifiers

[Borislav Petkov]

From: "Borislav Petkov (AMD)" <bp@alien8.de>

After alternatives have been patched, changes to the X86_FEATURE flags
won't take effect and could potentially even be wrong.

Warn about it.

This is something which has been long overdue.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
---
 arch/x86/include/asm/cpufeature.h | 8 ++++++--
 arch/x86/kernel/cpu/cpuid-deps.c  | 3 +++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 1ef620d508f4..d0b9c411144b 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -146,8 +146,12 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
 extern void setup_clear_cpu_cap(unsigned int bit);
 extern void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int bit);
 
-#define setup_force_cpu_cap(bit) do { \
-	set_cpu_cap(&boot_cpu_data, bit);	\
+#define setup_force_cpu_cap(bit) do {			\
+							\
+	if (!boot_cpu_has(bit))				\
+		WARN_ON(alternatives_patched);		\
+							\
+	set_cpu_cap(&boot_cpu_data, bit);		\
 	set_bit(bit, (unsigned long *)cpu_caps_set);	\
 } while (0)
 
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index b7174209d855..5dd427c6feb2 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -114,6 +114,9 @@ static void do_clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature)
 	if (WARN_ON(feature >= MAX_FEATURE_BITS))
 		return;
 
+	if (boot_cpu_has(feature))
+		WARN_ON(alternatives_patched);
+
 	clear_feature(c, feature);
 
 	/* Collect all features to disable, handling dependencies */
-- 
2.43.0

Re: [PATCH 2/5] x86/alternatives: Catch late X86_FEATURE modifiers

[Nikolay Borisov]

On 27.03.24 г. 17:43 ч., Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> 
> After alternatives have been patched, changes to the X86_FEATURE flags
> won't take effect and could potentially even be wrong.
> 
> Warn about it.
> 
> This is something which has been long overdue.
> 
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>

nit: While cleaning this bit mind if you also switch 
alternatives_patched to a bool?

Re: [PATCH 2/5] x86/alternatives: Catch late X86_FEATURE modifiers

[Borislav Petkov]

On Wed, Mar 27, 2024 at 05:57:01PM +0200, Nikolay Borisov wrote:
> nit: While cleaning this bit mind if you also switch alternatives_patched to
> a bool?

Busy as hell right now. But I take patches ontop. :-)

-- 
Regards/Gruss,
    Boris.

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

[PATCH 3/5] x86/kvm/Kconfig: Have KVM_AMD_SEV select ARCH_HAS_CC_PLATFORM

[Borislav Petkov]

From: "Borislav Petkov (AMD)" <bp@alien8.de>

The functionality to load SEV-SNP guests by the host will soon rely on
cc_platform* helpers because the cpu_feature* API with the early
patching is insufficient when SNP support needs to be disabled late.

Therefore, pull that functionality in.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
---
 arch/x86/kvm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index 3aaf7e86a859..0ebdd088f28b 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig
@@ -122,6 +122,7 @@ config KVM_AMD_SEV
 	default y
 	depends on KVM_AMD && X86_64
 	depends on CRYPTO_DEV_SP_PSP && !(KVM_AMD=y && CRYPTO_DEV_CCP_DD=m)
+	select ARCH_HAS_CC_PLATFORM
 	help
 	  Provides support for launching Encrypted VMs (SEV) and Encrypted VMs
 	  with Encrypted State (SEV-ES) on AMD processors.
-- 
2.43.0

Re: [PATCH 3/5] x86/kvm/Kconfig: Have KVM_AMD_SEV select ARCH_HAS_CC_PLATFORM

[Tom Lendacky]

On 3/27/24 10:43, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> 
> The functionality to load SEV-SNP guests by the host will soon rely on
> cc_platform* helpers because the cpu_feature* API with the early
> patching is insufficient when SNP support needs to be disabled late.
> 
> Therefore, pull that functionality in.
> 
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>   arch/x86/kvm/Kconfig | 1 +
>   1 file changed, 1 insertion(+)
> 

[PATCH 4/5] x86/cc: Add cc_platform_set/_clear() helpers

[Borislav Petkov]

From: "Borislav Petkov (AMD)" <bp@alien8.de>

Add functionality to set and/or clear different attributes of the
machine as a confidential computing platform. Add the first one too:
whether the machine is running as a host for SEV-SNP guests.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
---
 arch/x86/coco/core.c        | 52 +++++++++++++++++++++++++++++++++++++
 include/linux/cc_platform.h | 12 +++++++++
 2 files changed, 64 insertions(+)

diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
index d07be9d05cd0..8c3fae23d3c6 100644
--- a/arch/x86/coco/core.c
+++ b/arch/x86/coco/core.c
@@ -16,6 +16,11 @@
 enum cc_vendor cc_vendor __ro_after_init = CC_VENDOR_NONE;
 u64 cc_mask __ro_after_init;
 
+static struct cc_attr_flags {
+	__u64 host_sev_snp	: 1,
+	      __resv		: 63;
+} cc_flags;
+
 static bool noinstr intel_cc_platform_has(enum cc_attr attr)
 {
 	switch (attr) {
@@ -89,6 +94,9 @@ static bool noinstr amd_cc_platform_has(enum cc_attr attr)
 	case CC_ATTR_GUEST_SEV_SNP:
 		return sev_status & MSR_AMD64_SEV_SNP_ENABLED;
 
+	case CC_ATTR_HOST_SEV_SNP:
+		return cc_flags.host_sev_snp;
+
 	default:
 		return false;
 	}
@@ -148,3 +156,47 @@ u64 cc_mkdec(u64 val)
 	}
 }
 EXPORT_SYMBOL_GPL(cc_mkdec);
+
+static void amd_cc_platform_clear(enum cc_attr attr)
+{
+	switch (attr) {
+	case CC_ATTR_HOST_SEV_SNP:
+		cc_flags.host_sev_snp = 0;
+		break;
+	default:
+		break;
+	}
+}
+
+void cc_platform_clear(enum cc_attr attr)
+{
+	switch (cc_vendor) {
+	case CC_VENDOR_AMD:
+		amd_cc_platform_clear(attr);
+		break;
+	default:
+		break;
+	}
+}
+
+static void amd_cc_platform_set(enum cc_attr attr)
+{
+	switch (attr) {
+	case CC_ATTR_HOST_SEV_SNP:
+		cc_flags.host_sev_snp = 1;
+		break;
+	default:
+		break;
+	}
+}
+
+void cc_platform_set(enum cc_attr attr)
+{
+	switch (cc_vendor) {
+	case CC_VENDOR_AMD:
+		amd_cc_platform_set(attr);
+		break;
+	default:
+		break;
+	}
+}
diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h
index cb0d6cd1c12f..60693a145894 100644
--- a/include/linux/cc_platform.h
+++ b/include/linux/cc_platform.h
@@ -90,6 +90,14 @@ enum cc_attr {
 	 * Examples include TDX Guest.
 	 */
 	CC_ATTR_HOTPLUG_DISABLED,
+
+	/**
+	 * @CC_ATTR_HOST_SEV_SNP: AMD SNP enabled on the host.
+	 *
+	 * The host kernel is running with the necessary features
+	 * enabled to run SEV-SNP guests.
+	 */
+	CC_ATTR_HOST_SEV_SNP,
 };
 
 #ifdef CONFIG_ARCH_HAS_CC_PLATFORM
@@ -107,10 +115,14 @@ enum cc_attr {
  * * FALSE - Specified Confidential Computing attribute is not active
  */
 bool cc_platform_has(enum cc_attr attr);
+void cc_platform_set(enum cc_attr attr);
+void cc_platform_clear(enum cc_attr attr);
 
 #else	/* !CONFIG_ARCH_HAS_CC_PLATFORM */
 
 static inline bool cc_platform_has(enum cc_attr attr) { return false; }
+static inline void cc_platform_set(enum cc_attr attr) { }
+static inline void cc_platform_clear(enum cc_attr attr) { }
 
 #endif	/* CONFIG_ARCH_HAS_CC_PLATFORM */
 
-- 
2.43.0

Re: [PATCH 4/5] x86/cc: Add cc_platform_set/_clear() helpers

[Tom Lendacky]

On 3/27/24 10:43, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> 
> Add functionality to set and/or clear different attributes of the
> machine as a confidential computing platform. Add the first one too:
> whether the machine is running as a host for SEV-SNP guests.
> 
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>   arch/x86/coco/core.c        | 52 +++++++++++++++++++++++++++++++++++++
>   include/linux/cc_platform.h | 12 +++++++++
>   2 files changed, 64 insertions(+)
> 

[PATCH 5/5] x86/CPU/AMD: Track SNP host status with cc_platform_*()

[Borislav Petkov]

From: "Borislav Petkov (AMD)" <bp@alien8.de>

The host SNP worthiness can determined later, after alternatives have
been patched, in snp_rmptable_init() depending on cmdline options like
iommu=pt which is incompatible with SNP, for example.

Which means that one cannot use X86_FEATURE_SEV_SNP and will need to
have a special flag for that control.

Use that newly added CC_ATTR_HOST_SEV_SNP in the appropriate places.

Move kdump_sev_callback() to its rightfull place, while at it.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
---
 arch/x86/include/asm/sev.h         |  4 ++--
 arch/x86/kernel/cpu/amd.c          | 38 ++++++++++++++++++------------
 arch/x86/kernel/cpu/mtrr/generic.c |  2 +-
 arch/x86/kernel/sev.c              | 10 --------
 arch/x86/kvm/svm/sev.c             |  2 +-
 arch/x86/virt/svm/sev.c            | 26 +++++++++++++-------
 drivers/crypto/ccp/sev-dev.c       |  2 +-
 drivers/iommu/amd/init.c           |  4 +++-
 8 files changed, 49 insertions(+), 39 deletions(-)

diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 9477b4053bce..780182cda3ab 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -228,7 +228,6 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct sn
 void snp_accept_memory(phys_addr_t start, phys_addr_t end);
 u64 snp_get_unsupported_features(u64 status);
 u64 sev_get_status(void);
-void kdump_sev_callback(void);
 void sev_show_status(void);
 #else
 static inline void sev_es_ist_enter(struct pt_regs *regs) { }
@@ -258,7 +257,6 @@ static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *in
 static inline void snp_accept_memory(phys_addr_t start, phys_addr_t end) { }
 static inline u64 snp_get_unsupported_features(u64 status) { return 0; }
 static inline u64 sev_get_status(void) { return 0; }
-static inline void kdump_sev_callback(void) { }
 static inline void sev_show_status(void) { }
 #endif
 
@@ -270,6 +268,7 @@ int psmash(u64 pfn);
 int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, u32 asid, bool immutable);
 int rmp_make_shared(u64 pfn, enum pg_level level);
 void snp_leak_pages(u64 pfn, unsigned int npages);
+void kdump_sev_callback(void);
 #else
 static inline bool snp_probe_rmptable_info(void) { return false; }
 static inline int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) { return -ENODEV; }
@@ -282,6 +281,7 @@ static inline int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, u32 as
 }
 static inline int rmp_make_shared(u64 pfn, enum pg_level level) { return -ENODEV; }
 static inline void snp_leak_pages(u64 pfn, unsigned int npages) {}
+static inline void kdump_sev_callback(void) { }
 #endif
 
 #endif
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 6d8677e80ddb..9bf17c9c29da 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -345,6 +345,28 @@ static void srat_detect_node(struct cpuinfo_x86 *c)
 #endif
 }
 
+static void bsp_determine_snp(struct cpuinfo_x86 *c)
+{
+#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
+	cc_vendor = CC_VENDOR_AMD;
+
+	if (cpu_has(c, X86_FEATURE_SEV_SNP)) {
+		/*
+		 * RMP table entry format is not architectural and is defined by the
+		 * per-processor PPR. Restrict SNP support on the known CPU models
+		 * for which the RMP table entry format is currently defined for.
+		 */
+		if (!cpu_has(c, X86_FEATURE_HYPERVISOR) &&
+		    c->x86 >= 0x19 && snp_probe_rmptable_info()) {
+			cc_platform_set(CC_ATTR_HOST_SEV_SNP);
+		} else {
+			setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
+			cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
+		}
+	}
+#endif
+}
+
 static void bsp_init_amd(struct cpuinfo_x86 *c)
 {
 	if (cpu_has(c, X86_FEATURE_CONSTANT_TSC)) {
@@ -452,21 +474,7 @@ static void bsp_init_amd(struct cpuinfo_x86 *c)
 		break;
 	}
 
-	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 (!boot_cpu_has(X86_FEATURE_ZEN3) &&
-		    !boot_cpu_has(X86_FEATURE_ZEN4) &&
-		    !boot_cpu_has(X86_FEATURE_ZEN5))
-			setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
-		else if (!snp_probe_rmptable_info())
-			setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
-	}
-
+	bsp_determine_snp(c);
 	return;
 
 warn:
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 422a4ddc2ab7..7b29ebda024f 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -108,7 +108,7 @@ static inline void k8_check_syscfg_dram_mod_en(void)
 	      (boot_cpu_data.x86 >= 0x0f)))
 		return;
 
-	if (cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP))
 		return;
 
 	rdmsr(MSR_AMD64_SYSCFG, lo, hi);
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index b59b09c2f284..1e1a3c3bd1e8 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -2287,16 +2287,6 @@ static int __init snp_init_platform_device(void)
 }
 device_initcall(snp_init_platform_device);
 
-void kdump_sev_callback(void)
-{
-	/*
-	 * Do wbinvd() on remote CPUs when SNP is enabled in order to
-	 * safely do SNP_SHUTDOWN on the local CPU.
-	 */
-	if (cpu_feature_enabled(X86_FEATURE_SEV_SNP))
-		wbinvd();
-}
-
 void sev_show_status(void)
 {
 	int i;
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index ae0ac12382b9..3d310b473e05 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3174,7 +3174,7 @@ struct page *snp_safe_alloc_page(struct kvm_vcpu *vcpu)
 	unsigned long pfn;
 	struct page *p;
 
-	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
 		return alloc_page(GFP_KERNEL_ACCOUNT | __GFP_ZERO);
 
 	/*
diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index cffe1157a90a..ab0e8448bb6e 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -77,7 +77,7 @@ static int __mfd_enable(unsigned int cpu)
 {
 	u64 val;
 
-	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
 		return 0;
 
 	rdmsrl(MSR_AMD64_SYSCFG, val);
@@ -98,7 +98,7 @@ static int __snp_enable(unsigned int cpu)
 {
 	u64 val;
 
-	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
 		return 0;
 
 	rdmsrl(MSR_AMD64_SYSCFG, val);
@@ -174,11 +174,11 @@ static int __init snp_rmptable_init(void)
 	u64 rmptable_size;
 	u64 val;
 
-	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
 		return 0;
 
 	if (!amd_iommu_snp_en)
-		return 0;
+		goto nosnp;
 
 	if (!probed_rmp_size)
 		goto nosnp;
@@ -225,7 +225,7 @@ static int __init snp_rmptable_init(void)
 	return 0;
 
 nosnp:
-	setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
+	cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
 	return -ENOSYS;
 }
 
@@ -246,7 +246,7 @@ static struct rmpentry *__snp_lookup_rmpentry(u64 pfn, int *level)
 {
 	struct rmpentry *large_entry, *entry;
 
-	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
 		return ERR_PTR(-ENODEV);
 
 	entry = get_rmpentry(pfn);
@@ -363,7 +363,7 @@ int psmash(u64 pfn)
 	unsigned long paddr = pfn << PAGE_SHIFT;
 	int ret;
 
-	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
 		return -ENODEV;
 
 	if (!pfn_valid(pfn))
@@ -472,7 +472,7 @@ static int rmpupdate(u64 pfn, struct rmp_state *state)
 	unsigned long paddr = pfn << PAGE_SHIFT;
 	int ret, level;
 
-	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
 		return -ENODEV;
 
 	level = RMP_TO_PG_LEVEL(state->pagesize);
@@ -558,3 +558,13 @@ void snp_leak_pages(u64 pfn, unsigned int npages)
 	spin_unlock(&snp_leaked_pages_list_lock);
 }
 EXPORT_SYMBOL_GPL(snp_leak_pages);
+
+void kdump_sev_callback(void)
+{
+	/*
+	 * Do wbinvd() on remote CPUs when SNP is enabled in order to
+	 * safely do SNP_SHUTDOWN on the local CPU.
+	 */
+	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP))
+		wbinvd();
+}
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index f44efbb89c34..2102377f727b 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1090,7 +1090,7 @@ static int __sev_snp_init_locked(int *error)
 	void *arg = &data;
 	int cmd, rc = 0;
 
-	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
 		return -ENODEV;
 
 	sev = psp->sev_data;
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index e7a44929f0da..33228c1c8980 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -3228,7 +3228,7 @@ static bool __init detect_ivrs(void)
 static void iommu_snp_enable(void)
 {
 #ifdef CONFIG_KVM_AMD_SEV
-	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
 		return;
 	/*
 	 * The SNP support requires that IOMMU must be enabled, and is
@@ -3236,12 +3236,14 @@ static void iommu_snp_enable(void)
 	 */
 	if (no_iommu || iommu_default_passthrough()) {
 		pr_err("SNP: IOMMU disabled or configured in passthrough mode, SNP cannot be supported.\n");
+		cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
 		return;
 	}
 
 	amd_iommu_snp_en = check_feature(FEATURE_SNP);
 	if (!amd_iommu_snp_en) {
 		pr_err("SNP: IOMMU SNP feature not enabled, SNP cannot be supported.\n");
+		cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
 		return;
 	}
 
-- 
2.43.0

Re: [PATCH 5/5] x86/CPU/AMD: Track SNP host status with cc_platform_*()

[Jeremi Piotrowski]

On 27/03/2024 16:43, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> 
> The host SNP worthiness can determined later, after alternatives have
> been patched, in snp_rmptable_init() depending on cmdline options like
> iommu=pt which is incompatible with SNP, for example.
> 
> Which means that one cannot use X86_FEATURE_SEV_SNP and will need to
> have a special flag for that control.
> 
> Use that newly added CC_ATTR_HOST_SEV_SNP in the appropriate places.
> 
> Move kdump_sev_callback() to its rightfull place, while at it.
> 
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> ---
>  arch/x86/include/asm/sev.h         |  4 ++--
>  arch/x86/kernel/cpu/amd.c          | 38 ++++++++++++++++++------------
>  arch/x86/kernel/cpu/mtrr/generic.c |  2 +-
>  arch/x86/kernel/sev.c              | 10 --------
>  arch/x86/kvm/svm/sev.c             |  2 +-
>  arch/x86/virt/svm/sev.c            | 26 +++++++++++++-------
>  drivers/crypto/ccp/sev-dev.c       |  2 +-
>  drivers/iommu/amd/init.c           |  4 +++-
>  8 files changed, 49 insertions(+), 39 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 9477b4053bce..780182cda3ab 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -228,7 +228,6 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct sn
>  void snp_accept_memory(phys_addr_t start, phys_addr_t end);
>  u64 snp_get_unsupported_features(u64 status);
>  u64 sev_get_status(void);
> -void kdump_sev_callback(void);
>  void sev_show_status(void);
>  #else
>  static inline void sev_es_ist_enter(struct pt_regs *regs) { }
> @@ -258,7 +257,6 @@ static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *in
>  static inline void snp_accept_memory(phys_addr_t start, phys_addr_t end) { }
>  static inline u64 snp_get_unsupported_features(u64 status) { return 0; }
>  static inline u64 sev_get_status(void) { return 0; }
> -static inline void kdump_sev_callback(void) { }
>  static inline void sev_show_status(void) { }
>  #endif
>  
> @@ -270,6 +268,7 @@ int psmash(u64 pfn);
>  int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, u32 asid, bool immutable);
>  int rmp_make_shared(u64 pfn, enum pg_level level);
>  void snp_leak_pages(u64 pfn, unsigned int npages);
> +void kdump_sev_callback(void);
>  #else
>  static inline bool snp_probe_rmptable_info(void) { return false; }
>  static inline int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) { return -ENODEV; }
> @@ -282,6 +281,7 @@ static inline int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, u32 as
>  }
>  static inline int rmp_make_shared(u64 pfn, enum pg_level level) { return -ENODEV; }
>  static inline void snp_leak_pages(u64 pfn, unsigned int npages) {}
> +static inline void kdump_sev_callback(void) { }
>  #endif
>  
>  #endif
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 6d8677e80ddb..9bf17c9c29da 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -345,6 +345,28 @@ static void srat_detect_node(struct cpuinfo_x86 *c)
>  #endif
>  }
>  
> +static void bsp_determine_snp(struct cpuinfo_x86 *c)
> +{
> +#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
> +	cc_vendor = CC_VENDOR_AMD;

Shouldn't this line be inside the cpu_has(c, X86_FEATURE_SEV_SNP) check?

> +
> +	if (cpu_has(c, X86_FEATURE_SEV_SNP)) {
> +		/*
> +		 * RMP table entry format is not architectural and is defined by the
> +		 * per-processor PPR. Restrict SNP support on the known CPU models
> +		 * for which the RMP table entry format is currently defined for.
> +		 */> +		if (!cpu_has(c, X86_FEATURE_HYPERVISOR) &&

How about turning this into a more specific check:

  if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP) &&

Thanks,
Jeremi

> +		    c->x86 >= 0x19 && snp_probe_rmptable_info()) {
> +			cc_platform_set(CC_ATTR_HOST_SEV_SNP);
> +		} else {
> +			setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
> +			cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
> +		}
> +	}
> +#endif
> +}
> +
>  static void bsp_init_amd(struct cpuinfo_x86 *c)
>  {
>  	if (cpu_has(c, X86_FEATURE_CONSTANT_TSC)) {
> @@ -452,21 +474,7 @@ static void bsp_init_amd(struct cpuinfo_x86 *c)
>  		break;
>  	}
>  
> -	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 (!boot_cpu_has(X86_FEATURE_ZEN3) &&
> -		    !boot_cpu_has(X86_FEATURE_ZEN4) &&
> -		    !boot_cpu_has(X86_FEATURE_ZEN5))
> -			setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
> -		else if (!snp_probe_rmptable_info())
> -			setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
> -	}
> -
> +	bsp_determine_snp(c);
>  	return;
>  
>  warn:

Re: [PATCH 5/5] x86/CPU/AMD: Track SNP host status with cc_platform_*()

[Borislav Petkov]

On Thu, Mar 28, 2024 at 12:51:17PM +0100, Jeremi Piotrowski wrote:
> Shouldn't this line be inside the cpu_has(c, X86_FEATURE_SEV_SNP) check?

The cc_vendor is not dependent on X86_FEATURE_SEV_SNP.

> How about turning this into a more specific check:
> 
>   if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP) &&

Why?

The check is "am I running as a hypervisor on baremetal".

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 5/5] x86/CPU/AMD: Track SNP host status with cc_platform_*()

[Jeremi Piotrowski]

On 28/03/2024 14:41, Borislav Petkov wrote:
> On Thu, Mar 28, 2024 at 12:51:17PM +0100, Jeremi Piotrowski wrote:
>> Shouldn't this line be inside the cpu_has(c, X86_FEATURE_SEV_SNP) check?
> 
> The cc_vendor is not dependent on X86_FEATURE_SEV_SNP.
>

It's not but if you set it before the check it will be set for all AMD systems,
even if they are neither CC hosts nor CC guests.

cc_vendor being unset is handled correctly in cc_platform_has() checks.

>> How about turning this into a more specific check:
>>
>>   if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP) &&
> 
> Why?
> 

To leave open the possibility of an SNP hypervisor running nested.

> The check is "am I running as a hypervisor on baremetal".
> 

I thought you wanted to filter out SEV-SNP guests, which also have X86_FEATURE_SEV_SNP
CPUID bit set.

My understanding is that these are the cases:

CPUID(SEV_SNP) | MSR(SEV_SNP)     | what am I
---------------------------------------------
set            | set              | SNP-guest
set            | unset            | SNP-host
unset          | ??               | not SNP

Re: [PATCH 5/5] x86/CPU/AMD: Track SNP host status with cc_platform_*()

[Borislav Petkov]

On Thu, Mar 28, 2024 at 03:24:29PM +0100, Jeremi Piotrowski wrote:
> It's not but if you set it before the check it will be set for all AMD
> systems, even if they are neither CC hosts nor CC guests.

That a problem?

It is under a CONFIG_ARCH_HAS_CC_PLATFORM...

> To leave open the possibility of an SNP hypervisor running nested.

But !CC_ATTR_GUEST_SEV_SNP doesn't mean that. It means it is not
a SEV-SNP guest.

> I thought you wanted to filter out SEV-SNP guests, which also have
> X86_FEATURE_SEV_SNP CPUID bit set.

I want to run snp_probe_rmptable_info() only on baremetal where it makes
sense.

> My understanding is that these are the cases:
> 
> CPUID(SEV_SNP) | MSR(SEV_SNP)     | what am I
> ---------------------------------------------
> set            | set              | SNP-guest
> set            | unset            | SNP-host
> unset          | ??               | not SNP

So as you can see, we can't use X86_FEATURE_SEV_SNP for anything due to
the late disable need. So we should be moving away from it.

So we need a test for "am I a nested SNP hypervisor?"

So, can your thing clear X86_FEATURE_HYPERVISOR and thus "emulate"
baremetal?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 5/5] x86/CPU/AMD: Track SNP host status with cc_platform_*()

[Jeremi Piotrowski]

On 28/03/2024 16:39, Borislav Petkov wrote:
> On Thu, Mar 28, 2024 at 03:24:29PM +0100, Jeremi Piotrowski wrote:
>> It's not but if you set it before the check it will be set for all AMD
>> systems, even if they are neither CC hosts nor CC guests.
> 
> That a problem?
> 

No problem now but I did find it odd that cc_vendor will now always be set for AMD but
not for Intel. For Intel the various checks would automatically return true. Something
to look out for in the future when adding CC_ATTR's - no one can assume that the checks
will only run when actively dealing with confidential computing.

> It is under a CONFIG_ARCH_HAS_CC_PLATFORM...
>>> To leave open the possibility of an SNP hypervisor running nested.
> 
> But !CC_ATTR_GUEST_SEV_SNP doesn't mean that. It means it is not
> a SEV-SNP guest.
> 
>> I thought you wanted to filter out SEV-SNP guests, which also have
>> X86_FEATURE_SEV_SNP CPUID bit set.
> 
> I want to run snp_probe_rmptable_info() only on baremetal where it makes
> sense.
>>> My understanding is that these are the cases:
>>
>> CPUID(SEV_SNP) | MSR(SEV_SNP)     | what am I
>> ---------------------------------------------
>> set            | set              | SNP-guest
>> set            | unset            | SNP-host
>> unset          | ??               | not SNP
> 
> So as you can see, we can't use X86_FEATURE_SEV_SNP for anything due to
> the late disable need. So we should be moving away from it.
> 

I see your point about the disable needing to happen late - but then how about we remove
the setup_clear_cpu_cap(X86_FEATURE_SEV_SNP) too? No code depends on it any more and it would
help my cause as well.

> So we need a test for "am I a nested SNP hypervisor?"
> 
> So, can your thing clear X86_FEATURE_HYPERVISOR and thus "emulate"
> baremetal?
> 

Can't do that... it is a VM and hypervisor detection and various paravirt interfaces depend on
X86_FEATURE_HYPERVISOR.

Re: [PATCH 5/5] x86/CPU/AMD: Track SNP host status with cc_platform_*()

[Borislav Petkov]

On Thu, Apr 04, 2024 at 07:07:26PM +0200, Jeremi Piotrowski wrote:
> On 28/03/2024 16:39, Borislav Petkov wrote:
> > On Thu, Mar 28, 2024 at 03:24:29PM +0100, Jeremi Piotrowski wrote:
> >> It's not but if you set it before the check it will be set for all AMD
> >> systems, even if they are neither CC hosts nor CC guests.
> > 
> > That a problem?
> > 
> 
> No problem now but I did find it odd that cc_vendor will now always be set for AMD but
> not for Intel. For Intel the various checks would automatically return true. Something
> to look out for in the future when adding CC_ATTR's - no one can assume that the checks
> will only run when actively dealing with confidential computing.

Right, I haven't made up my mind fully here yet... setting cc_vendor
*only* when running as some sort of a confidential computing guest kinda
makes sense.

And if it is not set, then that can be used to catch cases where the
cc_* helpers are used outside of confidential computing cases...

Do we want those assertions? I don't know...

> I see your point about the disable needing to happen late - but then how about we remove
> the setup_clear_cpu_cap(X86_FEATURE_SEV_SNP) too? No code depends on it any more and it would
> help my cause as well.
> 
> > So we need a test for "am I a nested SNP hypervisor?"
> > 
> > So, can your thing clear X86_FEATURE_HYPERVISOR and thus "emulate"
> > baremetal?
> > 
> 
> Can't do that... it is a VM and hypervisor detection and various paravirt interfaces depend on
> X86_FEATURE_HYPERVISOR.

Right, but "your cause" as you call it above looks like a constant
whack'a'mole game everytime we change something in the kernel when
enabling those things and that breaks your cause.

Do you really want that?

Or would you prefer to define your nested solution properly and then
have upstream code support it like the next well-defined coco platform
instead?

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 5/5] x86/CPU/AMD: Track SNP host status with cc_platform_*()

[Tom Lendacky]

On 3/27/24 10:43, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> 
> The host SNP worthiness can determined later, after alternatives have
> been patched, in snp_rmptable_init() depending on cmdline options like
> iommu=pt which is incompatible with SNP, for example.
> 
> Which means that one cannot use X86_FEATURE_SEV_SNP and will need to
> have a special flag for that control.
> 
> Use that newly added CC_ATTR_HOST_SEV_SNP in the appropriate places.
> 
> Move kdump_sev_callback() to its rightfull place, while at it.
> 
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>

If late disabling of CPU feature flags is ever supported in the future, we 
should come back and possibly remove this. But until then...

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>   arch/x86/include/asm/sev.h         |  4 ++--
>   arch/x86/kernel/cpu/amd.c          | 38 ++++++++++++++++++------------
>   arch/x86/kernel/cpu/mtrr/generic.c |  2 +-
>   arch/x86/kernel/sev.c              | 10 --------
>   arch/x86/kvm/svm/sev.c             |  2 +-
>   arch/x86/virt/svm/sev.c            | 26 +++++++++++++-------
>   drivers/crypto/ccp/sev-dev.c       |  2 +-
>   drivers/iommu/amd/init.c           |  4 +++-
>   8 files changed, 49 insertions(+), 39 deletions(-)
> 

Re: [PATCH 0/5] x86/sev: Fix SNP host late disable

[Aithal, Srikanth]

On 3/27/2024 9:13 PM, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> 
> Hi,
> 
> the intention to track SNP host status with the CPU feature bit
> X86_FEATURE_SEV_SNP was all fine and dandy but that can't work if stuff
> needs to be disabled late, after alternatives patching - see patch 5.
> 
> Therefore, convert the SNP status tracking to a cc_platform*() bit.
> 
> The first two are long overdue cleanups.
> 
> If no objections, 3-5 should go in now so that 6.9 releases fixed.
> 
> Thx.
> 
> Borislav Petkov (AMD) (5):
>    x86/alternatives: Remove a superfluous newline in _static_cpu_has()
>    x86/alternatives: Catch late X86_FEATURE modifiers
>    x86/kvm/Kconfig: Have KVM_AMD_SEV select ARCH_HAS_CC_PLATFORM
>    x86/cc: Add cc_platform_set/_clear() helpers
>    x86/CPU/AMD: Track SNP host status with cc_platform_*()
> 
>   arch/x86/coco/core.c               | 52 ++++++++++++++++++++++++++++++
>   arch/x86/include/asm/cpufeature.h  | 11 ++++---
>   arch/x86/include/asm/sev.h         |  4 +--
>   arch/x86/kernel/cpu/amd.c          | 38 +++++++++++++---------
>   arch/x86/kernel/cpu/cpuid-deps.c   |  3 ++
>   arch/x86/kernel/cpu/mtrr/generic.c |  2 +-
>   arch/x86/kernel/sev.c              | 10 ------
>   arch/x86/kvm/Kconfig               |  1 +
>   arch/x86/kvm/svm/sev.c             |  2 +-
>   arch/x86/virt/svm/sev.c            | 26 ++++++++++-----
>   drivers/crypto/ccp/sev-dev.c       |  2 +-
>   drivers/iommu/amd/init.c           |  4 ++-
>   include/linux/cc_platform.h        | 12 +++++++
>   13 files changed, 124 insertions(+), 43 deletions(-)
> 
Tested this patch. I could boot with snp enabled and iommu=pt mode,kexec 
as well works fine. Thank you.

Tested-by: Srikanth Aithal <sraithal@amd.com>