Skip to contentSkip to conversation

Apply RMP table fixups for kexec.

PATCH Superseded Ashish Kalra 3 participants 13 messages Apr 15, 2024 – Nov 16, 2024

Subsystems (2)
x86 x86 MM

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Ashish Kalra

author
#0 lore raw

v2 cover Apply RMP table fixups for kexec.

From: Ashish Kalra <[email protected]>

Handle cases where the RMP table placement in the BIOS is not 2M aligned and then the kexec kernel could try to allocate from within that chunk and that causes a fatal RMP fault. Check if RMP table start & end physical range in e820_table is not aligned to 2MB and in that case use e820__range_update() to map this range to reserved.

The callback to apply these RMP table fixups needs to be called after the e820 tables are setup/populated and before the e820 map has been converted to the standard Linux memory resources and e820 map is no longer used and modifying it has no effect.

v2:
- Remove overriding e820__memory_setup_default() to invoke snp_rmptable_e820_fixup() to apply the RMP table fixups.
- This callback snp_rmptable_e820_fixup() is now invoked after e820__memory_setup() and before e820__reserve_resources().
- Expose e820 API interfaces to update e820_table_kexec and e820_table_firmware externally.
- snp_rmptable_e820_fixup() now calls these new external API interfaces to update e820_table_kexec and e820_table_firmware.

Ashish Kalra (2):
x86/e820: Expose API to update e820 kexec and firmware tables
externally.
x86/sev: Add callback to apply RMP table fixups for kexec.

 arch/x86/include/asm/e820/api.h |  3 +++
 arch/x86/include/asm/sev.h      |  2 ++
 arch/x86/kernel/e820.c          |  7 +++++-
 arch/x86/mm/mem_encrypt.c       |  3 +++
 arch/x86/virt/svm/sev.c         | 44 +++++++++++++++++++++++++++++++++
 5 files changed, 58 insertions(+), 1 deletion(-)
Signature · 1 line
2.34.1
author
#1 lore raw

PATCH v2 1/2 x86/e820: Expose API to update e820 kexec and firmware tables externally.

From: Ashish Kalra <[email protected]>

Export the API interfaces to update the e820_table_kexec and e820_table_firmware.

Signed-off-by Ashish Kalra <[email protected]>
---
 arch/x86/include/asm/e820/api.h | 3 +++
 arch/x86/kernel/e820.c          | 7 ++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)
arch/x86/include/asm/e820/api.h +3 -0
index e8f58ddd06d9..09fe1ddcdf2b 100644
@@ -18,6 +18,9 @@ extern void e820__range_add   (u64 start, u64 size, enum e820_type type);
 extern u64  e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
 extern u64  e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type);
 
+extern u64 e820__range_update_kexec(u64 start, u64 size, enum e820_type old_type, enum e820_type  new_type);
+extern u64 e820__range_update_firmware(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
+
 extern void e820__print_table(char *who);
 extern int  e820__update_table(struct e820_table *table);
 extern void e820__update_table_print(void);
arch/x86/kernel/e820.c +6 -1
index 6f1b379e3b38..c59d1bbbe3f2 100644
@@ -532,7 +532,12 @@ u64 __init e820__range_update(u64 start, u64 size, enum e820_type old_type, enum
 	return __e820__range_update(e820_table, start, size, old_type, new_type);
 }
 
-static u64 __init e820__range_update_kexec(u64 start, u64 size, enum e820_type old_type, enum e820_type  new_type)
+u64 __init e820__range_update_firmware(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type)
+{
+	return __e820__range_update(e820_table_firmware, start, size, old_type, new_type);
+}
+
+u64 __init e820__range_update_kexec(u64 start, u64 size, enum e820_type old_type, enum e820_type  new_type)
 {
 	return __e820__range_update(e820_table_kexec, start, size, old_type, new_type);
 }
Signature · 1 line
2.34.1
maintainer
#2 lore raw

Re: [PATCH v2 1/2] x86/e820: Expose API to update e820 kexec and firmware tables externally.

arch/x86/kernel/e820.c u64 __init e820__range_update(u64 start, u64 size, enum e820_type old_type, enum · line 540

Quoted 7 lines · Ashish Kalra
arch/x86/kernel/e820.c +6 -1
-static u64 __init e820__range_update_kexec(u64 start, u64 size, enum e820_type old_type, enum e820_type  new_type)
+u64 __init e820__range_update_firmware(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type)
+{
+	return __e820__range_update(e820_table_firmware, start, size, old_type, new_type);
+}
+
+u64 __init e820__range_update_kexec(u64 start, u64 size, enum e820_type old_type, enum e820_type  new_type)

Yah, no point in defining silly wrappers _kexec() and _firmware() if the actual e820 tables are already exported in asm/e820/api.h

You need a single

e820__range_update_table(struct e820_table *t, ..)

helper and move all current and future users to it while leaving e820__range_update() alone which works on the e820_table.

As a future cleanup, e820__range_update() should be changed to use the new e820__range_update_table() helper and then perhaps all code should be converted back to a new

e820__range_update()

which takes a table as a first argument.

But the cleanup can go in later, after the current issue has been resolved.

Thx.

Signature · 4 lines
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
author
#3 lore raw

PATCH v2 2/2 x86/sev: Add callback to apply RMP table fixups for kexec.

In reply to Ashish Kalra

From: Ashish Kalra <[email protected]>

Handle cases where the RMP table placement in the BIOS is not 2M aligned and then the kexec kernel could try to allocate from within that chunk and that causes a fatal RMP fault. Check if RMP table start & end physical range in e820_table is not aligned to 2MB and in that case use e820__range_update() to map this range to reserved.

The callback to apply these RMP table fixups needs to be called after the e820 tables are setup/populated and before the e820 map has been converted to the standard Linux memory resources and e820 map is no longer used and modifying it has no effect.

Fixes c3b86e61b756 ("x86/cpufeatures: Enable/unmask SEV-SNP CPU feature")
Signed-off-by Ashish Kalra <[email protected]>
---
 arch/x86/include/asm/sev.h |  2 ++
 arch/x86/mm/mem_encrypt.c  |  3 +++
 arch/x86/virt/svm/sev.c    | 44 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+)
arch/x86/include/asm/sev.h +2 -0
index 7f57382afee4..6600ac467cf9 100644
@@ -269,6 +269,7 @@ int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, u32 asid, bool immut
 int rmp_make_shared(u64 pfn, enum pg_level level);
 void snp_leak_pages(u64 pfn, unsigned int npages);
 void kdump_sev_callback(void);
+void snp_rmptable_e820_fixup(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 +283,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) { }
+static inline void snp_rmptable_e820_fixup(void) {}
 #endif
 
 #endif
arch/x86/mm/mem_encrypt.c +3 -0
index 6f3b3e028718..765ce94e4b89 100644
@@ -102,6 +102,9 @@ void __init mem_encrypt_setup_arch(void)
 	phys_addr_t total_mem = memblock_phys_mem_size();
 	unsigned long size;
 
+	if (cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+		snp_rmptable_e820_fixup();
+
 	if (!cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
 		return;
arch/x86/virt/svm/sev.c +44 -0

50 lines not on this page · raw message

Signature · 1 line
2.34.1
maintainer
#4 lore raw

Re: [PATCH v2 2/2] x86/sev: Add callback to apply RMP table fixups for kexec.

Quoted 1 line · Ashish Kalra

Subject: Re: [PATCH v2 2/2] x86/sev: Add callback to apply RMP table fixups for kexec.

										       ^

patch subject doesn't need a fullstop: -------|

Quoted 9 lines

From: Ashish Kalra <[email protected]>

Handle cases where the RMP table placement in the BIOS is not 2M aligned and then the kexec kernel could try to allocate from within that chunk and that causes a fatal RMP fault.

Check if RMP table start & end physical range in e820_table is not aligned to 2MB and in that case use e820__range_update() to map this range to reserved.

Why do you keep explaining in your commit messages what a patch does?

Do not talk about *what* the patch is doing in the commit message - that should be obvious from the diff itself. Rather, concentrate on the *why* it needs to be done.

Imagine one fine day you're doing git archeology, you find the place in the code about which you want to find out why it was changed the way it is now.

You do git annotate <filename> ... find the line, see the commit id and you do:

git show <commit id>

You read the commit message and there's just gibberish and nothing's explaining *why* that change was done. And you start scratching your head, trying to figure out why. Because the damn commit message is worth sh*t.

Quoted 4 lines

The callback to apply these RMP table fixups needs to be called after the e820 tables are setup/populated and before the e820 map has been converted to the standard Linux memory resources and e820 map is no longer used and modifying it has no effect.

This commit message is not even trying to summarize what was figured out in previous review in the thread here:

https://lore.kernel.org/r/[email protected]

Please restructure this commit message using all the info from that thread and use this structure, for example:

1. Prepare the context for the explanation briefly.

2. Explain the problem at hand.

3. "It happens because of <...>"

4. "Fix it by doing X"

5. "(Potentially do Y)."

And some of those above are optional depending on the issue being explained.

For more detailed info, see
Documentation/process/submitting-patches.rst,
Section "2) Describe your changes".

Also, to the tone, from Documentation/process/submitting-patches.rst:

"Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour."

Quoted 33 lines
Fixes c3b86e61b756 ("x86/cpufeatures: Enable/unmask SEV-SNP CPU feature")
Signed-off-by Ashish Kalra <[email protected]>
---
 arch/x86/include/asm/sev.h |  2 ++
 arch/x86/mm/mem_encrypt.c  |  3 +++
 arch/x86/virt/svm/sev.c    | 44 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+)
arch/x86/include/asm/sev.h +2 -0
index 7f57382afee4..6600ac467cf9 100644
@@ -269,6 +269,7 @@ int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, u32 asid, bool immut
 int rmp_make_shared(u64 pfn, enum pg_level level);
 void snp_leak_pages(u64 pfn, unsigned int npages);
 void kdump_sev_callback(void);
+void snp_rmptable_e820_fixup(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 +283,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) { }
+static inline void snp_rmptable_e820_fixup(void) {}
 #endif
 
 #endif
arch/x86/mm/mem_encrypt.c +0 -0
index 6f3b3e028718..765ce94e4b89 100644

arch/x86/mm/mem_encrypt.c void __init mem_encrypt_setup_arch(void) · line 105

Quoted 4 lines
arch/x86/mm/mem_encrypt.c +1 -0
@@ -102,6 +102,9 @@ void __init mem_encrypt_setup_arch(void)
 	phys_addr_t total_mem = memblock_phys_mem_size();
 	unsigned long size;
 
+	if (cpu_feature_enabled(X86_FEATURE_SEV_SNP))

We use CC_ATTR_HOST_SEV_SNP for host SNP support checks, including RMP table viability.

Also, why isn't this called in snp_init()?

If there's a reason why (I think there is) put that reason as a comment above it why this thing needs to be called here exactly.

arch/x86/mm/mem_encrypt.c void __init mem_encrypt_setup_arch(void) · line 106

Quoted 1 line
arch/x86/mm/mem_encrypt.c +1 -0
+		snp_rmptable_e820_fixup();

IOW, point to the comment above that function's definition.

Quoted 18 lines
arch/x86/mm/mem_encrypt.c +1 -0
+
 	if (!cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
 		return;
arch/x86/virt/svm/sev.c +6 -0
index ab0e8448bb6e..d999ff7f1671 100644
@@ -163,6 +163,50 @@ bool snp_probe_rmptable_info(void)
 	return true;
 }
 
+/*
+ * Callback to do any RMP table fixups, needs to be called
+ * after e820__memory_setup(), after the e820 tables are
+ * setup/populated and before e820__reserve_resources(), before
+ * the e820 map has been converted to the standard Linux memory
+ * resources and e820 map is no longer used and modifying it

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

Quoted 10 lines
arch/x86/virt/svm/sev.c +10 -0
+ * has no effect.
+ */
+void __init snp_rmptable_e820_fixup(void)
+{
+	u64 pa;
+
+	/*
+	 * Handle cases where the RMP table placement in the BIOS is not 2M aligned
+	 * and then the kexec kernel could try to allocate from within that chunk
+	 * and that causes a fatal RMP fault.

Merge this comment with the one above the function and put it all there.

arch/x86/virt/svm/sev.c

Quoted 6 lines

Check if RMP table start & end

arch/x86/virt/svm/sev.c +5 -0
+	 * physical range in e820_table is not aligned to 2MB and in that case use
+	 * e820__range_update() to map this range to reserved, e820__range_update()
+	 * nicely handles partial range update and also merges any consecutive
+	 * ranges of the same type.
+	 */

This comment talks about what this does and is kinda obvious but then talks about e820__range_update() and not the other ones. Just put the gist of what this is supposed to do and do not explain the code step by step.

What is really missing here and what is not really trivial is why all three e820 tables need updating.

Quoted 12 lines
arch/x86/virt/svm/sev.c +12 -0
+	pa = probed_rmp_base;
+	if (!IS_ALIGNED(pa, PMD_SIZE)) {
+		pa = ALIGN_DOWN(pa, PMD_SIZE);
+		if (e820__mapped_any(pa, pa + PMD_SIZE, E820_TYPE_RAM)) {
+			pr_info("Reserving start of RMP table on a 2MB boundary [0x%016llx]\n", pa);
+			e820__range_update(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
+			e820__range_update_kexec(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
+			e820__range_update_firmware(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
+		}
+	}
+
+	pa = probed_rmp_base + probed_rmp_size;

arch/x86/virt/svm/sev.c

Quoted 10 lines
arch/x86/virt/svm/sev.c +10 -0
+	if (!IS_ALIGNED(pa, PMD_SIZE)) {
+		pa = ALIGN_DOWN(pa, PMD_SIZE);
+		if (e820__mapped_any(pa, pa + PMD_SIZE, E820_TYPE_RAM)) {
+			pr_info("Reserving end of RMP table on a 2MB boundary [0x%016llx]\n", pa);
+			e820__range_update(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
+			e820__range_update_kexec(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
+			e820__range_update_firmware(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
+		}
+	}
+}

Ontop for less duplication:

arch/x86/virt/svm/sev.c +17 -23
index be17661fee9b..118dfe61f80e 100644
@@ -163,6 +163,21 @@ bool snp_probe_rmptable_info(void)
 	return true;
 }
 
+static void __init __snp_e820_tables_fixup(u64 pa)
+{
+	if (IS_ALIGNED(pa, PMD_SIZE))
+		return;
+
+	pa = ALIGN_DOWN(pa, PMD_SIZE);
+	if (!e820__mapped_any(pa, pa + PMD_SIZE, E820_TYPE_RAM))
+		return;
+
+	pr_info("Reserving chunk of RMP table on a 2MB boundary [0x%016llx]\n", pa);
+	e820__range_update(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
+	e820__range_update_kexec(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
+	e820__range_update_firmware(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
+}
+
 /*
  * Callback to do any RMP table fixups, needs to be called
  * after e820__memory_setup(), after the e820 tables are

@@ -173,8 +188,6 @@ bool snp_probe_rmptable_info(void)
  */
 void __init snp_rmptable_e820_fixup(void)
 {
-	u64 pa;
-
 	/*
 	 * Handle cases where the RMP table placement in the BIOS is not 2M aligned
 	 * and then the kexec kernel could try to allocate from within that chunk

@@ -184,27 +197,8 @@ void __init snp_rmptable_e820_fixup(void)
 	 * nicely handles partial range update and also merges any consecutive
 	 * ranges of the same type.
 	 */
-	pa = probed_rmp_base;
-	if (!IS_ALIGNED(pa, PMD_SIZE)) {
-		pa = ALIGN_DOWN(pa, PMD_SIZE);
-		if (e820__mapped_any(pa, pa + PMD_SIZE, E820_TYPE_RAM)) {
-			pr_info("Reserving start of RMP table on a 2MB boundary [0x%016llx]\n", pa);
-			e820__range_update(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
-			e820__range_update_kexec(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
-			e820__range_update_firmware(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
-		}
-	}
-
-	pa = probed_rmp_base + probed_rmp_size;
-	if (!IS_ALIGNED(pa, PMD_SIZE)) {
-		pa = ALIGN_DOWN(pa, PMD_SIZE);
-		if (e820__mapped_any(pa, pa + PMD_SIZE, E820_TYPE_RAM)) {
-			pr_info("Reserving end of RMP table on a 2MB boundary [0x%016llx]\n", pa);
-			e820__range_update(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
-			e820__range_update_kexec(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
-			e820__range_update_firmware(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
-		}
-	}
+	__snp_e820_tables_fixup(probed_rmp_base);
+	__snp_e820_tables_fixup(probed_rmp_base + probed_rmp_size);
 }
 
 /*
Signature · 4 lines
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
author
#5 lore raw

Re: [PATCH v2 2/2] x86/sev: Add callback to apply RMP table fixups for kexec.

Hello Boris,

Quoted 3 lines

Subject: Re: [PATCH v2 2/2] x86/sev: Add callback to apply RMP table fixups for kexec.

From: Ashish Kalra<[email protected]>

<snip>

Quoted 1 line

Why do you keep explaining in your commit messages what a patch does?

I will fix the commit message.

Quoted 36 lines
Quoted 36 lines
Fixes c3b86e61b756 ("x86/cpufeatures: Enable/unmask SEV-SNP CPU feature")
Signed-off-by Ashish Kalra <[email protected]>
---
  arch/x86/include/asm/sev.h |  2 ++
  arch/x86/mm/mem_encrypt.c  |  3 +++
  arch/x86/virt/svm/sev.c    | 44 ++++++++++++++++++++++++++++++++++++++
  3 files changed, 49 insertions(+)
arch/x86/include/asm/sev.h +2 -0
index 7f57382afee4..6600ac467cf9 100644
@@ -269,6 +269,7 @@ int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, u32 asid, bool immut
  int rmp_make_shared(u64 pfn, enum pg_level level);
  void snp_leak_pages(u64 pfn, unsigned int npages);
  void kdump_sev_callback(void);
+void snp_rmptable_e820_fixup(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 +283,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) { }
+static inline void snp_rmptable_e820_fixup(void) {}
  #endif
 
  #endif
arch/x86/mm/mem_encrypt.c +0 -0
index 6f3b3e028718..765ce94e4b89 100644
@@ -102,6 +102,9 @@ void __init mem_encrypt_setup_arch(void)
  	phys_addr_t total_mem = memblock_phys_mem_size();
  	unsigned long size;
Quoted 3 lines

arch/x86/mm/mem_encrypt.c void __init mem_encrypt_setup_arch(void) · line 105

Quoted 1 line
arch/x86/mm/mem_encrypt.c +1 -0
+	if (cpu_feature_enabled(X86_FEATURE_SEV_SNP))

We use CC_ATTR_HOST_SEV_SNP for host SNP support checks, including RMP table viability.

Ok.

Quoted 5 lines

Also, why isn't this called in snp_init()?

If there's a reason why (I think there is) put that reason as a comment above it why this thing needs to be called here exactly.

This callback needs to be invoked as part of setup_arch() as it needs e820 table to be setup in e820__memory_setup() before the callback is invoked and snp_init() is called from sme_enable() in kernel/head_64.S (startup_64), which is much before start_kernel() -> setup_arch() is invoked.

I will add the comment here.

Quoted 24 lines

arch/x86/mm/mem_encrypt.c void __init mem_encrypt_setup_arch(void) · line 106

Quoted 1 line
arch/x86/mm/mem_encrypt.c +1 -0
+		snp_rmptable_e820_fixup();

IOW, point to the comment above that function's definition.

Quoted 21 lines
arch/x86/mm/mem_encrypt.c +1 -0
+
  	if (!cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
  		return;
arch/x86/virt/svm/sev.c +9 -0
index ab0e8448bb6e..d999ff7f1671 100644
@@ -163,6 +163,50 @@ bool snp_probe_rmptable_info(void)
  	return true;
  }
 
+/*
+ * Callback to do any RMP table fixups, needs to be called
+ * after e820__memory_setup(), after the e820 tables are
+ * setup/populated and before e820__reserve_resources(), before
+ * the e820 map has been converted to the standard Linux memory
+ * resources and e820 map is no longer used and modifying it
+ * has no effect.
+ */
+void __init snp_rmptable_e820_fixup(void)
Quoted 8 lines

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

Quoted 7 lines
arch/x86/virt/svm/sev.c +7 -0
+{
+	u64 pa;
+
+	/*
+	 * Handle cases where the RMP table placement in the BIOS is not 2M aligned
+	 * and then the kexec kernel could try to allocate from within that chunk
+	 * and that causes a fatal RMP fault.

Merge this comment with the one above the function and put it all there.

Ok.

Quoted 11 lines

arch/x86/virt/svm/sev.c

Quoted 6 lines

Check if RMP table start & end

arch/x86/virt/svm/sev.c +5 -0
+	 * physical range in e820_table is not aligned to 2MB and in that case use
+	 * e820__range_update() to map this range to reserved, e820__range_update()
+	 * nicely handles partial range update and also merges any consecutive
+	 * ranges of the same type.
+	 */

This comment talks about what this does and is kinda obvious but then talks about e820__range_update() and not the other ones. Just put the gist of what this is supposed to do and do not explain the code step by step.

Quoted 2 lines

What is really missing here and what is not really trivial is why all three e820 tables need updating.

I will add all these details.

Quoted 79 lines
Quoted 13 lines
arch/x86/virt/svm/sev.c +12 -0
+	pa = probed_rmp_base;
+	if (!IS_ALIGNED(pa, PMD_SIZE)) {
+		pa = ALIGN_DOWN(pa, PMD_SIZE);
+		if (e820__mapped_any(pa, pa + PMD_SIZE, E820_TYPE_RAM)) {
+			pr_info("Reserving start of RMP table on a 2MB boundary [0x%016llx]\n", pa);
+			e820__range_update(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
+			e820__range_update_kexec(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
+			e820__range_update_firmware(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
+		}
+	}
+
+	pa = probed_rmp_base + probed_rmp_size;

arch/x86/virt/svm/sev.c

Quoted 10 lines
arch/x86/virt/svm/sev.c +10 -0
+	if (!IS_ALIGNED(pa, PMD_SIZE)) {
+		pa = ALIGN_DOWN(pa, PMD_SIZE);
+		if (e820__mapped_any(pa, pa + PMD_SIZE, E820_TYPE_RAM)) {
+			pr_info("Reserving end of RMP table on a 2MB boundary [0x%016llx]\n", pa);
+			e820__range_update(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
+			e820__range_update_kexec(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
+			e820__range_update_firmware(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
+		}
+	}
+}

Ontop for less duplication:

arch/x86/virt/svm/sev.c +15 -17
index be17661fee9b..118dfe61f80e 100644
@@ -163,6 +163,21 @@ bool snp_probe_rmptable_info(void)
  	return true;
  }
 
+static void __init __snp_e820_tables_fixup(u64 pa)
+{
+	if (IS_ALIGNED(pa, PMD_SIZE))
+		return;
+
+	pa = ALIGN_DOWN(pa, PMD_SIZE);
+	if (!e820__mapped_any(pa, pa + PMD_SIZE, E820_TYPE_RAM))
+		return;
+
+	pr_info("Reserving chunk of RMP table on a 2MB boundary [0x%016llx]\n", pa);
+	e820__range_update(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
+	e820__range_update_kexec(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
+	e820__range_update_firmware(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
+}
+
  /*
   * Callback to do any RMP table fixups, needs to be called
   * after e820__memory_setup(), after the e820 tables are

@@ -173,8 +188,6 @@ bool snp_probe_rmptable_info(void)
   */
  void __init snp_rmptable_e820_fixup(void)
  {
-	u64 pa;
-
  	/*
  	 * Handle cases where the RMP table placement in the BIOS is not 2M aligned
  	 * and then the kexec kernel could try to allocate from within that chunk

@@ -184,27 +197,8 @@ void __init snp_rmptable_e820_fixup(void)
  	 * nicely handles partial range update and also merges any consecutive
  	 * ranges of the same type.
  	 */
-	pa = probed_rmp_base;
-	if (!IS_ALIGNED(pa, PMD_SIZE)) {
-		pa = ALIGN_DOWN(pa, PMD_SIZE);
-		if (e820__mapped_any(pa, pa + PMD_SIZE, E820_TYPE_RAM)) {
-			pr_info("Reserving start of RMP table on a 2MB boundary [0x%016llx]\n", pa);
-			e820__range_update(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
-			e820__range_update_kexec(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
-			e820__range_update_firmware(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
-		}
-	}
-
-	pa = probed_rmp_base + probed_rmp_size;
-	if (!IS_ALIGNED(pa, PMD_SIZE)) {
-		pa = ALIGN_DOWN(pa, PMD_SIZE);
-		if (e820__mapped_any(pa, pa + PMD_SIZE, E820_TYPE_RAM)) {

arch/x86/virt/svm/sev.c void __init snp_rmptable_e820_fixup(void) · line 203

Quoted 10 lines
arch/x86/virt/svm/sev.c +2 -6
-			pr_info("Reserving end of RMP table on a 2MB boundary [0x%016llx]\n", pa);
-			e820__range_update(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
-			e820__range_update_kexec(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
-			e820__range_update_firmware(pa, PMD_SIZE, E820_TYPE_RAM, E820_TYPE_RESERVED);
-		}
-	}
+	__snp_e820_tables_fixup(probed_rmp_base);
+	__snp_e820_tables_fixup(probed_rmp_base + probed_rmp_size);
  }

Thanks, Ashish

maintainer
#6 lore raw

Re: [PATCH v2 2/2] x86/sev: Add callback to apply RMP table fixups for kexec.

Quoted 4 lines · Ashish

This callback needs to be invoked as part of setup_arch() as it needs e820 table to be setup in e820__memory_setup() before the callback is invoked and snp_init() is called from sme_enable() in kernel/head_64.S (startup_64), which is much before start_kernel() -> setup_arch() is invoked.

So?

snp_init() still runs before e820__memory_setup(). So what's stopping you?

Signature · 4 lines
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
author
#7 lore raw

Re: [PATCH v2 2/2] x86/sev: Add callback to apply RMP table fixups for kexec.

Quoted 9 lines · Borislav Petkov
Quoted 4 lines · Ashish

This callback needs to be invoked as part of setup_arch() as it needs e820 table to be setup in e820__memory_setup() before the callback is invoked and snp_init() is called from sme_enable() in kernel/head_64.S (startup_64), which is much before start_kernel() -> setup_arch() is invoked.

So?

snp_init() still runs before e820__memory_setup(). So what's stopping you?

As i have already explained above, snp_init() runs before e820__memory_setup() so we can't invoke this callback in snp_init() as e820 tables have still not been setup. Again to summarize, the e820 tables are setup in e820__memory_setup() which runs after snp_init().

Additionally, RMP table also get probed after snp_init(). So this callback cannot be invoked in snp_init().

I have added this comment to v3 of this patch series which i posted yesterday, so please look at those.

Thanks, Ashish

#8 lore raw

Re: [PATCH v2 2/2] x86/sev: Add callback to apply RMP table fixups for kexec.

Hi,

(sorry for jumping really late, I've just found this accidentally)

Quoted 9 lines · Ashish
Quoted 7 lines · Borislav Petkov
Quoted 4 lines · Ashish

This callback needs to be invoked as part of setup_arch() as it needs e820 table to be setup in e820__memory_setup() before the callback is invoked and snp_init() is called from sme_enable() in kernel/head_64.S (startup_64), which is much before start_kernel() -> setup_arch() is invoked.

So?

Quoted 7 lines
Quoted 2 lines

snp_init() still runs before e820__memory_setup(). So what's stopping you?

As i have already explained above, snp_init() runs before e820__memory_setup() so we can't invoke this callback in snp_init() as e820 tables have still not been setup. Again to summarize, the e820 tables are setup in e820__memory_setup() which runs after snp_init().

I agree that snp_fixup_e820_tables() should run after e820__memory_setup(), but I wonder if it's ok for it to run *after* e820__memblock_setup().

At the very least making changes to e820 table after they were translated to memblock creates inconsistency between the e820 and core mm views of the memory.

So I wonder what would happen if the memory from the problematic chunk was allocated by kernel rather than by kexec? Couldn't it cause RMP fault?

-
Sincerely yours,
Mike.

author
#9 lore raw

Re: [PATCH v2 2/2] x86/sev: Add callback to apply RMP table fixups for kexec.

Quoted 25 lines · Mike Rapoport

Hi,

(sorry for jumping really late, I've just found this accidentally)

Quoted 9 lines · Ashish
Quoted 7 lines · Borislav Petkov
Quoted 4 lines · Ashish

This callback needs to be invoked as part of setup_arch() as it needs e820 table to be setup in e820__memory_setup() before the callback is invoked and snp_init() is called from sme_enable() in kernel/head_64.S (startup_64), which is much before start_kernel() -> setup_arch() is invoked.

So?

Quoted 7 lines
Quoted 2 lines

snp_init() still runs before e820__memory_setup(). So what's stopping you?

As i have already explained above, snp_init() runs before e820__memory_setup() so we can't invoke this callback in snp_init() as e820 tables have still not been setup. Again to summarize, the e820 tables are setup in e820__memory_setup() which runs after snp_init().

I agree that snp_fixup_e820_tables() should run after e820__memory_setup(), but I wonder if it's ok for it to run *after* e820__memblock_setup().

Quoted 7 lines

At the very least making changes to e820 table after they were translated to memblock creates inconsistency between the e820 and core mm views of the memory.

So I wonder what would happen if the memory from the problematic chunk was allocated by kernel rather than by kexec? Couldn't it cause RMP fault?

Yes, we discovered an issue with SNP guests when trying to use one of these pages and there was a fix pushed and merged to ensure that RMP table fixups are (also) reserved for memblock.

Here is a detailed description from the patch:

The RMP table fixups are done after the e820 ranges have been added to memblock, allowing the fixup ranges to still be allocated and used by the system. The problem is that this memory range is now marked reserved in the e820 tables and during SNP initialization these reserved ranges are made HV-Fixed. This means that the pages cannot be used by an SNP guest, only by the hypervisor. However, the memory management subsystem does not make this distinction and can allocate one of those pages to an SNP guest. This will ultimately result in RMPUPDATE failures associated with the guest, causing it to fail to start or terminate when accessing the HV-Fixed page.

Link to the patch:
https://lore.kernel.org/lkml/[email protected]/

Thanks,
Ashish

Quoted 3 lines

-
Sincerely yours,
Mike.

maintainer
#10 lore raw
In reply to Ashish Kalra

Drop [email protected] from Cc.

Quoted 5 lines · Ashish Kalra

From: Ashish Kalra <[email protected]>

Handle cases where the RMP table placement in the BIOS is not 2M aligned and then the kexec kernel could try to allocate from within that chunk and that causes a fatal RMP fault.

Is this something that happens on existing systems currently so that it needs to be fixed upstream?

Signature · 4 lines
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
author
#11 lore raw

Hello Boris,

Quoted 10 lines · Borislav Petkov

Drop [email protected] from Cc.

Quoted 5 lines · Ashish Kalra

From: Ashish Kalra <[email protected]>

Handle cases where the RMP table placement in the BIOS is not 2M aligned and then the kexec kernel could try to allocate from within that chunk and that causes a fatal RMP fault.

Is this something that happens on existing systems currently so that it needs to be fixed upstream?

Patch 2/2 - includes the following Fixes tag, do i need to include the Fixes tag in the cover letter too ?

Fixes c3b86e61b756 ("x86/cpufeatures: Enable/unmask SEV-SNP CPU feature")

Thanks,
Ashish

maintainer
#12 lore raw
Quoted 4 lines · Ashish

Patch 2/2 - includes the following Fixes tag, do i need to include the Fixes tag in the cover letter too ?

Fixes c3b86e61b756 ("x86/cpufeatures: Enable/unmask SEV-SNP CPU feature")

People slap Fixes tags on pretty much every patch nowadays. I'd prefer it if you spell it out in the future what the problem is you're encountering so that I can make a decision where to route the patches through.

Thx.

Signature · 4 lines
Regards/Gruss,
    Boris.

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