arch: x86: platform: efi: Disabling interrupt around kmalloc

PATCH New Tapasweni Pathak 3 participants 5 messages Mar 3, 2015 – Mar 10, 2015

Subsystems (2)

x86 EFI

lore

Share & tools

lore

Ingo Molnar

2015-03-03 06:34 UTC #1 lore raw

PATCH 1/1 efi: Disable interrupts around EFI calls, not in the epilog/prolog calls

Quoted 17 lines · Tapasweni Pathak

Disabling interrupts around kmalloc() is less than ideal. Move it after the kmalloc().
Found using Coccinelle.
Signed-off-by Tapasweni Pathak <[email protected]>
Suggested-by Matt Fleming <[email protected]>
---
 arch/x86/platform/efi/efi_64.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/platform/efi/efi_64.c +0 -0
index 17e80d8..5d6af59 100644
@@ -88,10 +88,10 @@ void __init efi_call_phys_prolog(void)
 		return;

arch/x86/platform/efi/efi_64.c void __init efi_call_phys_prolog(void) · line 97

Quoted 10 lines

arch/x86/platform/efi/efi_64.c +1 -1

 
 	early_code_mapping_set_exec(1);
-	local_irq_save(efi_flags);
 
 	n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
 	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_KERNEL);
+	local_irq_save(efi_flags);
 
 	for (pgd = 0; pgd < n_pgds; pgd++) {
 		save_pgd[pgd] = *pgd_offset_k(pgd * PGDIR_SIZE);

So why are interrupts disabled around page table operations to begin with? It's not like any of this can execute on two CPUs at once, nor can this be executed from interrupt context.

So, shouldn't we only protect the EFI calls themselves? If we did that we could save the efi_flags global variable ugliness as well.

I.e. something like the attached patch. (not tested and all that)

Thanks,

Ingo

===================>

Quoted 1 line

From 160eef628f9e803ba62707ec7f610ea9f0f13984 Mon Sep 17 00:00:00 2001

From: Ingo Molnar <[email protected]>
Date: Tue, 3 Mar 2015 07:31:56 +0100
Subject: [PATCH] efi: Disable interrupts around EFI calls, not in the epilog/prolog calls

Tapasweni Pathak reported that we do a kmalloc() in efi_call_phys_prolog() on x86-64 while having interrupts disabled, which is a big no-no, as kmalloc() can sleep.

Solve this by removing the irq disabling from the prolog/epilog calls around EFI calls: it's unnecessary, as in this stage we are single threaded in the boot thread, and we don't ever execute this from interrupt contexts.

Reported-by Tapasweni Pathak <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/platform/efi/efi.c    |  7 +++++++
 arch/x86/platform/efi/efi_32.c | 11 +++--------
 arch/x86/platform/efi/efi_64.c |  3 ---
 3 files changed, 10 insertions(+), 11 deletions(-)

arch/x86/platform/efi/efi.c +7 -0

index dbc8627a5cdf..2490a15d18f1 100644
@@ -85,12 +85,19 @@ static efi_status_t __init phys_efi_set_virtual_address_map(
 	efi_memory_desc_t *virtual_map)
 {
 	efi_status_t status;
+	unsigned long flags;
 
 	efi_call_phys_prolog();
+
+	/* Disable interrupts around EFI calls: */
+	local_irq_save(flags);
 	status = efi_call_phys(efi_phys.set_virtual_address_map,
 			       memory_map_size, descriptor_size,
 			       descriptor_version, virtual_map);
+	local_irq_restore(flags);
+
 	efi_call_phys_epilog();
+
 	return status;
 }

arch/x86/platform/efi/efi_32.c +3 -8

index 40e7cda52936..abecc6e1dc90 100644
@@ -33,11 +33,10 @@
 
 /*
  * To make EFI call EFI runtime service in physical addressing mode we need
- * prolog/epilog before/after the invocation to disable interrupt, to
- * claim EFI runtime service handler exclusively and to duplicate a memory in
- * low memory space say 0 - 3G.
+ * prolog/epilog before/after the invocation to claim the EFI runtime service
+ * handler exclusively and to duplicate a memory mapping in low memory space,
+ * say 0 - 3G.
  */
-static unsigned long efi_rt_eflags;
 
 void efi_sync_low_kernel_mappings(void) {}
 void __init efi_dump_pagetable(void) {}

@@ -61,8 +60,6 @@ void __init efi_call_phys_prolog(void)
 {
 	struct desc_ptr gdt_descr;
 
-	local_irq_save(efi_rt_eflags);
-
 	load_cr3(initial_page_table);
 	__flush_tlb_all();

@@ -81,8 +78,6 @@ void __init efi_call_phys_epilog(void)
 
 	load_cr3(swapper_pg_dir);
 	__flush_tlb_all();
-
-	local_irq_restore(efi_rt_eflags);
 }
 
 void __init efi_runtime_mkexec(void)

arch/x86/platform/efi/efi_64.c +0 -3

index 17e80d829df0..427eb3540e5f 100644
@@ -42,7 +42,6 @@
 #include <asm/time.h>
 
 static pgd_t *save_pgd __initdata;
-static unsigned long efi_flags __initdata;
 
 /*
  * We allocate runtime services regions bottom-up, starting from -4G, i.e.

@@ -88,7 +87,6 @@ void __init efi_call_phys_prolog(void)
 		return;
 
 	early_code_mapping_set_exec(1);
-	local_irq_save(efi_flags);
 
 	n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
 	save_pgd = kmalloc(n_pgds * sizeof(pgd_t), GFP_KERNEL);

@@ -116,7 +114,6 @@ void __init efi_call_phys_epilog(void)
 		set_pgd(pgd_offset_k(pgd * PGDIR_SIZE), save_pgd[pgd]);
 	kfree(save_pgd);
 	__flush_tlb_all();
-	local_irq_restore(efi_flags);
 	early_code_mapping_set_exec(0);
 }

Ingo Molnar

2015-03-03 06:48 UTC #2 lore raw

PATCH 1/1 efi: Clean up the efi_call_phys_[prolog|epilog]() save/restore interaction

Also clean up the save_pgd global variable while at it.

untested as well.

Thanks,

Ingo

==============>

Quoted 1 line

From 166625ceaef68fcbeee63adc63c02d75abcaf0db Mon Sep 17 00:00:00 2001

From: Ingo Molnar <[email protected]>
Date: Tue, 3 Mar 2015 07:42:48 +0100
Subject: [PATCH] efi: Clean up the efi_call_phys_[prolog|epilog]() save/restore interaction

Currently x86-64 efi_call_phys_prolog() saves into a global variable (save_pgd), and efi_call_phys_epilog() restores the kernel pagetables from that global variable.

Change this to a cleaner save/restore pattern where the saving function returns the saved object and the restore function restores that.

Apply the same concept to the 32-bit code as well.

Plus this approach, as an added bonus, allows us to express the !efi_enabled(EFI_OLD_MEMMAP) situation in a clean fashion as well, via a 'NULL' return value.

Cc Tapasweni Pathak <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/include/asm/efi.h     |  6 ++++--
 arch/x86/platform/efi/efi.c    |  5 +++--
 arch/x86/platform/efi/efi_32.c | 11 ++++++++---
 arch/x86/platform/efi/efi_64.c | 26 ++++++++++++++++----------
 4 files changed, 31 insertions(+), 17 deletions(-)

arch/x86/include/asm/efi.h +4 -2

index 25bce45c6fc4..3738b138b843 100644
@@ -2,6 +2,8 @@
 #define _ASM_X86_EFI_H
 
 #include <asm/i387.h>
+#include <asm/pgtable.h>
+
 /*
  * We map the EFI regions needed for runtime services non-contiguously,
  * with preserved alignment on virtual addresses starting from -4G down

@@ -89,8 +91,8 @@ extern void __iomem *__init efi_ioremap(unsigned long addr, unsigned long size,
 extern struct efi_scratch efi_scratch;
 extern void __init efi_set_executable(efi_memory_desc_t *md, bool executable);
 extern int __init efi_memblock_x86_reserve_range(void);
-extern void __init efi_call_phys_prolog(void);
-extern void __init efi_call_phys_epilog(void);
+extern pgd_t * __init efi_call_phys_prolog(void);
+extern void __init efi_call_phys_epilog(pgd_t *save_pgd);
 extern void __init efi_unmap_memmap(void);
 extern void __init efi_memory_uc(u64 addr, unsigned long size);
 extern void __init efi_map_region(efi_memory_desc_t *md);

arch/x86/platform/efi/efi.c +3 -2

index 2490a15d18f1..aa5bd2b986c2 100644
@@ -86,8 +86,9 @@ static efi_status_t __init phys_efi_set_virtual_address_map(
 {
 	efi_status_t status;
 	unsigned long flags;
+	pgd_t *save_pgd;
 
-	efi_call_phys_prolog();
+	save_pgd = efi_call_phys_prolog();
 
 	/* Disable interrupts around EFI calls: */
 	local_irq_save(flags);

@@ -96,7 +97,7 @@ static efi_status_t __init phys_efi_set_virtual_address_map(
 			       descriptor_version, virtual_map);
 	local_irq_restore(flags);
 
-	efi_call_phys_epilog();
+	efi_call_phys_epilog(save_pgd);
 
 	return status;
 }

arch/x86/platform/efi/efi_32.c +8 -3

index abecc6e1dc90..ed5b67338294 100644
@@ -56,19 +56,24 @@ void __init efi_map_region(efi_memory_desc_t *md)
 void __init efi_map_region_fixed(efi_memory_desc_t *md) {}
 void __init parse_efi_setup(u64 phys_addr, u32 data_len) {}
 
-void __init efi_call_phys_prolog(void)
+pgd_t * __init efi_call_phys_prolog(void)
 {
 	struct desc_ptr gdt_descr;
+	pgd_t *save_pgd;
 
+	/* Current pgd is swapper_pg_dir, we'll restore it later: */
+	save_pgd = swapper_pg_dir;
 	load_cr3(initial_page_table);
 	__flush_tlb_all();
 
 	gdt_descr.address = __pa(get_cpu_gdt_table(0));
 	gdt_descr.size = GDT_SIZE - 1;
 	load_gdt(&gdt_descr);
+
+	return save_pgd;
 }
 
-void __init efi_call_phys_epilog(void)
+void __init efi_call_phys_epilog(pgd_t *save_pgd)
 {
 	struct desc_ptr gdt_descr;

@@ -76,7 +81,7 @@ void __init efi_call_phys_epilog(void)
 	gdt_descr.size = GDT_SIZE - 1;
 	load_gdt(&gdt_descr);
 
-	load_cr3(swapper_pg_dir);
+	load_cr3(save_pgd);
 	__flush_tlb_all();
 }

arch/x86/platform/efi/efi_64.c +16 ~~-10~~

index 427eb3540e5f..a0ac0f9c307f 100644
@@ -41,8 +41,6 @@
 #include <asm/realmode.h>
 #include <asm/time.h>
 
-static pgd_t *save_pgd __initdata;
-
 /*
  * We allocate runtime services regions bottom-up, starting from -4G, i.e.
  * 0xffff_ffff_0000_0000 and limit EFI VA mapping space to 64G.

@@ -77,14 +75,16 @@ static void __init early_code_mapping_set_exec(int executable)
 	}
 }
 
-void __init efi_call_phys_prolog(void)
+pgd_t * __init efi_call_phys_prolog(void)
 {
 	unsigned long vaddress;
+	pgd_t *save_pgd;
+
 	int pgd;
 	int n_pgds;
 
 	if (!efi_enabled(EFI_OLD_MEMMAP))
-		return;
+		return NULL;
 
 	early_code_mapping_set_exec(1);

@@ -97,22 +97,28 @@ void __init efi_call_phys_prolog(void)
 		set_pgd(pgd_offset_k(pgd * PGDIR_SIZE), *pgd_offset_k(vaddress));
 	}
 	__flush_tlb_all();
+
+	return save_pgd;
 }
 
-void __init efi_call_phys_epilog(void)
+void __init efi_call_phys_epilog(pgd_t *save_pgd)
 {
 	/*
 	 * After the lock is released, the original page table is restored.
 	 */
-	int pgd;
-	int n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT) , PGDIR_SIZE);
+	int pgd_idx;
+	int nr_pgds;
 
-	if (!efi_enabled(EFI_OLD_MEMMAP))
+	if (!save_pgd)
 		return;
 
-	for (pgd = 0; pgd < n_pgds; pgd++)
-		set_pgd(pgd_offset_k(pgd * PGDIR_SIZE), save_pgd[pgd]);
+	nr_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT) , PGDIR_SIZE);
+
+	for (pgd_idx = 0; pgd_idx < nr_pgds; pgd_idx++)
+		set_pgd(pgd_offset_k(pgd_idx * PGDIR_SIZE), save_pgd[pgd_idx]);
+
 	kfree(save_pgd);
+
 	__flush_tlb_all();
 	early_code_mapping_set_exec(0);
 }

Matt Fleming

Applied

2015-03-10 13:36 UTC #3 lore raw

Re: [PATCH] efi: Clean up the efi_call_phys_[prolog|epilog]() save/restore interaction

Quoted 28 lines · Ingo Molnar

Also clean up the save_pgd global variable while at it.
untested as well.
Thanks,
Ingo
==============>
From 166625ceaef68fcbeee63adc63c02d75abcaf0db Mon Sep 17 00:00:00 2001
From: Ingo Molnar <[email protected]>
Date: Tue, 3 Mar 2015 07:42:48 +0100
Subject: [PATCH] efi: Clean up the efi_call_phys_[prolog|epilog]() save/restore interaction
Currently x86-64 efi_call_phys_prolog() saves into a global variable (save_pgd), and efi_call_phys_epilog() restores the kernel pagetables from that global variable.
Change this to a cleaner save/restore pattern where the saving function returns the saved object and the restore function restores that.
Apply the same concept to the 32-bit code as well.
Plus this approach, as an added bonus, allows us to express the !efi_enabled(EFI_OLD_MEMMAP) situation in a clean fashion as well, via a 'NULL' return value.

Quoted 8 lines

Cc Tapasweni Pathak <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/include/asm/efi.h     |  6 ++++--
 arch/x86/platform/efi/efi.c    |  5 +++--
 arch/x86/platform/efi/efi_32.c | 11 ++++++++---
 arch/x86/platform/efi/efi_64.c | 26 ++++++++++++++++----------
 4 files changed, 31 insertions(+), 17 deletions(-)

Applied, thanks!

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Matt Fleming

2015-03-10 13:36 UTC #4 lore raw

Re: [PATCH] efi: Disable interrupts around EFI calls, not in the epilog/prolog calls

In reply to Ingo Molnar

Quoted 9 lines · Ingo Molnar

So why are interrupts disabled around page table operations to begin with? It's not like any of this can execute on two CPUs at once, nor can this be executed from interrupt context.
So, shouldn't we only protect the EFI calls themselves? If we did that we could save the efi_flags global variable ugliness as well.
I.e. something like the attached patch. (not tested and all that)

Looks good to me Ingo. I'll run it through the test farm. Thanks!

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center