Fix broken SNP support with KVM module built-in

From: Ashish Kalra <[email protected]>

This patch fixes issues with enabling SNP host support and effectively

---
 arch/x86/include/asm/sev.h |  2 ++
 arch/x86/virt/svm/sev.c    | 23 +++++++----------------
 2 files changed, 9 insertions(+), 16 deletions(-)

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

index 5d9685f92e5c..1581246491b5 100644
@@ -531,6 +531,7 @@ static inline void __init snp_secure_tsc_init(void) { }
 
 #ifdef CONFIG_KVM_AMD_SEV
 bool snp_probe_rmptable_info(void);
+int snp_rmptable_init(void);
 int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level);
 void snp_dump_hva_rmpentry(unsigned long address);
 int psmash(u64 pfn);

@@ -541,6 +542,7 @@ void kdump_sev_callback(void);
 void snp_fixup_e820_tables(void);
 #else
 static inline bool snp_probe_rmptable_info(void) { return false; }
+static inline int snp_rmptable_init(void) { return -ENOSYS; }
 static inline int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) { return -ENODEV; }
 static inline void snp_dump_hva_rmpentry(unsigned long address) {}
 static inline int psmash(u64 pfn) { return -ENODEV; }

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

index 1dcc027ec77e..42e74a5a7d78 100644
@@ -505,19 +505,19 @@ static bool __init setup_rmptable(void)
  * described in the SNP_INIT_EX firmware command description in the SNP
  * firmware ABI spec.
  */
-static int __init snp_rmptable_init(void)
+int __init snp_rmptable_init(void)
 {
 	unsigned int i;
 	u64 val;
 
-	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
-		return 0;
+	if (WARN_ON_ONCE(!cc_platform_has(CC_ATTR_HOST_SEV_SNP)))
+		return -ENOSYS;
 
-	if (!amd_iommu_snp_en)
-		goto nosnp;
+	if (WARN_ON_ONCE(!amd_iommu_snp_en))
+		return -ENOSYS;
 
 	if (!setup_rmptable())
-		goto nosnp;
+		return -ENOSYS;
 
 	/*
 	 * Check if SEV-SNP is already enabled, this can happen in case of

@@ -530,7 +530,7 @@ static int __init snp_rmptable_init(void)
 	/* Zero out the RMP bookkeeping area */
 	if (!clear_rmptable_bookkeeping()) {
 		free_rmp_segment_table();
-		goto nosnp;
+		return -ENOSYS;
 	}
 
 	/* Zero out the RMP entries */

@@ -562,17 +562,8 @@ static int __init snp_rmptable_init(void)
 	crash_kexec_post_notifiers = true;
 
 	return 0;

Quoted 3 lines · Ashish Kalra

From: Ashish Kalra <[email protected]>

This patch fixes issues with enabling SNP host support and effectively

^^^^^^^^^^

Quoted 74 lines

---
 arch/x86/include/asm/sev.h |  2 ++
 arch/x86/virt/svm/sev.c    | 23 +++++++----------------
 2 files changed, 9 insertions(+), 16 deletions(-)

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

index 5d9685f92e5c..1581246491b5 100644
@@ -531,6 +531,7 @@ static inline void __init snp_secure_tsc_init(void) { }
 
 #ifdef CONFIG_KVM_AMD_SEV
 bool snp_probe_rmptable_info(void);
+int snp_rmptable_init(void);
 int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level);
 void snp_dump_hva_rmpentry(unsigned long address);
 int psmash(u64 pfn);

@@ -541,6 +542,7 @@ void kdump_sev_callback(void);
 void snp_fixup_e820_tables(void);
 #else
 static inline bool snp_probe_rmptable_info(void) { return false; }
+static inline int snp_rmptable_init(void) { return -ENOSYS; }
 static inline int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) { return -ENODEV; }
 static inline void snp_dump_hva_rmpentry(unsigned long address) {}
 static inline int psmash(u64 pfn) { return -ENODEV; }

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

index 1dcc027ec77e..42e74a5a7d78 100644
@@ -505,19 +505,19 @@ static bool __init setup_rmptable(void)
  * described in the SNP_INIT_EX firmware command description in the SNP
  * firmware ABI spec.
  */
-static int __init snp_rmptable_init(void)
+int __init snp_rmptable_init(void)
 {
 	unsigned int i;
 	u64 val;
 
-	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
-		return 0;
+	if (WARN_ON_ONCE(!cc_platform_has(CC_ATTR_HOST_SEV_SNP)))
+		return -ENOSYS;
 
-	if (!amd_iommu_snp_en)
-		goto nosnp;
+	if (WARN_ON_ONCE(!amd_iommu_snp_en))
+		return -ENOSYS;
 
 	if (!setup_rmptable())
-		goto nosnp;
+		return -ENOSYS;
 
 	/*
 	 * Check if SEV-SNP is already enabled, this can happen in case of

@@ -530,7 +530,7 @@ static int __init snp_rmptable_init(void)
 	/* Zero out the RMP bookkeeping area */
 	if (!clear_rmptable_bookkeeping()) {
 		free_rmp_segment_table();
-		goto nosnp;
+		return -ENOSYS;
 	}
 
 	/* Zero out the RMP entries */

@@ -562,17 +562,8 @@ static int __init snp_rmptable_init(void)
 	crash_kexec_post_notifiers = true;
 
 	return 0;
-
-nosnp:
-	cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
-	return -ENOSYS;
 }
 

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

Quoted 4 lines

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

-/*
- * This must be called after the IOMMU has been initialized.
- */
-device_initcall(snp_rmptable_init);

There's the wee little problem that snp_rmptable_init() is never called as of this patch. Dropping the device_initcall() needs to happen in the same patch that wires up the IOMMU code to invoke snp_rmptable_init().

At a glance, I don't see anything in this patch that can reasonably go in before the IOMMU change.

Quoted 6 lines · Sean Christopherson

Quoted 5 lines · Ashish Kalra

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

-nosnp:
-	cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
-	return -ENOSYS;
 }

Quoted 8 lines

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

Quoted 4 lines

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

-/*
- * This must be called after the IOMMU has been initialized.
- */
-device_initcall(snp_rmptable_init);

There's the wee little problem that snp_rmptable_init() is never called as of this patch. Dropping the device_initcall() needs to happen in the same patch that wires up the IOMMU code to invoke snp_rmptable_init().

The issue with that is the IOMMU and x86 maintainers are different, so i believe that we will need to split the dropping of device_initcall() in platform code and the code to wire up the IOMMU driver to invoke snp_rmptable_init(), to get the patch merged in different trees ?

From: Sean Christopherson <[email protected]>

This patch fixes the current SNP host enabling code and effectively SNP

---
 drivers/iommu/amd/init.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

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

index c5cd92edada0..ee887aa4442f 100644

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

 {
 #ifdef CONFIG_KVM_AMD_SEV
 	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))

@@ -3219,6 +3219,11 @@ static void iommu_snp_enable(void)
 		goto disable_snp;
 	}
 
+	if (snp_rmptable_init()) {
+		pr_warn("SNP: RMP initialization failed, SNP cannot be supported.\n");
+		goto disable_snp;
+	}
+
 	pr_info("IOMMU SNP support enabled.\n");
 	return;
 

@@ -3426,18 +3431,23 @@ void __init amd_iommu_detect(void)
 	int ret;
 
 	if (no_iommu || (iommu_detected && !gart_iommu_aperture))
-		return;

drivers/iommu/amd/init.c +5 -0

 	amd_iommu_detected = true;
 	iommu_detected = 1;
 	x86_init.iommu.iommu_init = amd_iommu_init;
+	return;
+
+disable_snp:
+	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP))
+		cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
 }
 
 /****************************************************************************

Signature · 1 line

2.34.1

Quoted 3 lines · Ashish Kalra

From: Sean Christopherson <[email protected]>

This patch fixes the current SNP host enabling code and effectively SNP

^^^^^^^^^^

Quoted 9 lines

---
 drivers/iommu/amd/init.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

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

index c5cd92edada0..ee887aa4442f 100644

drivers/iommu/amd/init.c static bool __init detect_ivrs(void) · line 3197

Quoted 5 lines

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

@@ -3194,7 +3194,7 @@ static bool __init detect_ivrs(void)
 	return true;
 }
 
-static void iommu_snp_enable(void)
+static __init void iommu_snp_enable(void)

If you're feeling nitpicky, adding "__init" could be done in a separate patch.

Quoted 20 lines

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

 {
 #ifdef CONFIG_KVM_AMD_SEV
 	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))

@@ -3219,6 +3219,11 @@ static void iommu_snp_enable(void)
 		goto disable_snp;
 	}
 
+	if (snp_rmptable_init()) {
+		pr_warn("SNP: RMP initialization failed, SNP cannot be supported.\n");
+		goto disable_snp;
+	}
+
 	pr_info("IOMMU SNP support enabled.\n");
 	return;
 

@@ -3426,18 +3431,23 @@ void __init amd_iommu_detect(void)
 	int ret;
 
 	if (no_iommu || (iommu_detected && !gart_iommu_aperture))
-		return;

drivers/iommu/amd/init.c void __init amd_iommu_detect(void) · line 3441

Quoted 10 lines

drivers/iommu/amd/init.c +3 -2

+		goto disable_snp;
 
 	if (!amd_iommu_sme_check())
-		return;
+		goto disable_snp;
 
 	ret = iommu_go_to_state(IOMMU_IVRS_DETECTED);
 	if (ret)
-		return;
+		goto disable_snp;

This handles initial failure, but it won't handle the case where amd_iommu_prepare() fails, as the iommu_go_to_state() call from amd_iommu_enable() will get short-circuited. I don't see any pleasant options. Maybe this?

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

index c5cd92edada0..436e47f13f8f 100644

Somewhat of a side topic, what happens if the RMP is fully configured and _then_ IOMMU initialization fails? I.e. if amd_iommu_init_pci() or amd_iommu_enable_interrupts() fails? Is that even survivable?

Quoted 16 lines

drivers/iommu/amd/init.c +5 -0

 	amd_iommu_detected = true;
 	iommu_detected = 1;
 	x86_init.iommu.iommu_init = amd_iommu_init;
+	return;
+
+disable_snp:
+	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP))
+		cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
 }
 
 /****************************************************************************

Signature · 1 line

2.34.1

Quoted 3 lines · Ashish Kalra

From: Sean Christopherson <[email protected]>

This patch fixes the current SNP host enabling code and effectively SNP

^^^^^^^^^^

Quoted 9 lines

---
 drivers/iommu/amd/init.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

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

index c5cd92edada0..ee887aa4442f 100644

drivers/iommu/amd/init.c static bool __init detect_ivrs(void) · line 3197

Quoted 5 lines

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

@@ -3194,7 +3194,7 @@ static bool __init detect_ivrs(void)
 	return true;
 }
 
-static void iommu_snp_enable(void)
+static __init void iommu_snp_enable(void)

If you're feeling nitpicky, adding "__init" could be done in a separate patch.

Quoted 20 lines

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

 {
 #ifdef CONFIG_KVM_AMD_SEV
 	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))

@@ -3219,6 +3219,11 @@ static void iommu_snp_enable(void)
 		goto disable_snp;
 	}
 
+	if (snp_rmptable_init()) {
+		pr_warn("SNP: RMP initialization failed, SNP cannot be supported.\n");
+		goto disable_snp;
+	}
+
 	pr_info("IOMMU SNP support enabled.\n");
 	return;
 

@@ -3426,18 +3431,23 @@ void __init amd_iommu_detect(void)
 	int ret;
 
 	if (no_iommu || (iommu_detected && !gart_iommu_aperture))
-		return;

drivers/iommu/amd/init.c void __init amd_iommu_detect(void) · line 3441

Quoted 10 lines

drivers/iommu/amd/init.c +3 -2

+		goto disable_snp;
 
 	if (!amd_iommu_sme_check())
-		return;
+		goto disable_snp;
 
 	ret = iommu_go_to_state(IOMMU_IVRS_DETECTED);
 	if (ret)
-		return;
+		goto disable_snp;

This handles initial failure, but it won't handle the case where amd_iommu_prepare() fails, as the iommu_go_to_state() call from amd_iommu_enable() will get short-circuited. I don't see any pleasant options. Maybe this?

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

index c5cd92edada0..436e47f13f8f 100644

Quoted 29 lines · Sean Christopherson

Quoted 5 lines · Ashish Kalra

(quoted diff) +0 -1

@@ -3426,18 +3431,23 @@ void __init amd_iommu_detect(void)
 	int ret;
 
 	if (no_iommu || (iommu_detected && !gart_iommu_aperture))
-		return;

Review void __init amd_iommu_detect(void) · line 3441

Quoted 10 lines

(quoted diff) +3 -2

+		goto disable_snp;
 
 	if (!amd_iommu_sme_check())
-		return;
+		goto disable_snp;
 
 	ret = iommu_go_to_state(IOMMU_IVRS_DETECTED);
 	if (ret)
-		return;
+		goto disable_snp;

This handles initial failure, but it won't handle the case where amd_iommu_prepare() fails, as the iommu_go_to_state() call from amd_iommu_enable() will get short-circuited. I don't see any pleasant options. Maybe this?

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

index c5cd92edada0..436e47f13f8f 100644
@@ -3318,6 +3318,8 @@ static int __init iommu_go_to_state(enum iommu_init_state state)
                ret = state_next();
        }
 

drivers/iommu/amd/init.c static int __init iommu_go_to_state(enum iommu_init_state state) · line 3321

Quoted 1 line

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

+       if (ret && !amd_iommu_snp_en && cc_platform_has(CC_ATTR_HOST_SEV_SNP))

I think we should clear when amd_iommu_snp_en is true.

May be below check is enough?

	if (ret && amd_iommu_snp_en)