stable UEFI virtual mappings for kexec

Split of the remapping code from efi_config_init() so that the caller can perform its own remapping. This is necessary to correctly handle virtually remapped UEFI memory regions under kexec, as efi.systab will have been updated to a virtual address.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/firmware/efi/efi.c | 49 +++++++++++++++++++++++++++++-----------------
 include/linux/efi.h        |  2 ++
 2 files changed, 33 insertions(+), 18 deletions(-)

Split of the remapping code from efi_config_init() so that the caller can perform its own remapping. This is necessary to correctly handle virtually remapped UEFI memory regions under kexec, as efi.systab will have been updated to a virtual address.

Signed-off-by Ard Biesheuvel <[email protected]>

drivers/firmware/efi/efi.c +12 -3

@@ -344,13 +333,37 @@ int __init efi_config_init(efi_config_table_type_t *arch_tables)
 		tablep += sz;
 	}
 	pr_cont("\n");
-	early_memunmap(config_tables, efi.systab->nr_tables * sz);
-
 	set_bit(EFI_CONFIG_TABLES, &efi.flags);
-
 	return 0;
 }
 
+int __init efi_config_init(efi_config_table_type_t *arch_tables)
+{
+	void *config_tables;
+	int sz, ret;
+
+	if (efi_enabled(EFI_64BIT))
+		sz = sizeof(efi_config_table_64_t);
+	else
+		sz = sizeof(efi_config_table_32_t);
+
+	/*
+	 * Let's see what config tables the firmware passed to us.

On systems with 64 KB pages, it is preferable for UEFI memory map entries to be 64 KB aligned multiples of 64 KB, because it relieves us of having to deal with the residues.
So, if EFI_ALLOC_ALIGN is #define'd by the platform, use it to round up all memory allocations made.

Acked-by Matt Fleming <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

Quoted 6 lines · Ard Biesheuvel

On systems with 64 KB pages, it is preferable for UEFI memory map entries to be 64 KB aligned multiples of 64 KB, because it relieves us of having to deal with the residues.
So, if EFI_ALLOC_ALIGN is #define'd by the platform, use it to round up all memory allocations made.

Quoted 2 lines

Acked-by Matt Fleming <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

Acked-by Borislav Petkov <[email protected]>

Do you have any comments regarding patches #3 and #6 in this series?

Quoted 1 line · Ard Biesheuvel

Do you have any comments regarding patches #3 and #6 in this series?

Sorry, have had no time so far to take a look. Holidays and all... You're on the TODO list though :-)

Set EFI_ALLOC_ALIGN to 64 KB so that all allocations done by the stub are naturally compatible with a 64 KB granule kernel.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/efi.h | 2 ++
 1 file changed, 2 insertions(+)

arch/arm64/include/asm/efi.h +2 -0

index a34fd3b12e2b..71291253114f 100644
@@ -44,4 +44,6 @@ extern void efi_idmap_init(void);
 
 #define efi_call_early(f, ...) sys_table_arg->boottime->f(__VA_ARGS__)
 
+#define EFI_ALLOC_ALIGN		SZ_64K
+
 #endif /* _ASM_EFI_H */

Signature · 1 line

1.8.3.2

In order to support kexec, the kernel needs to be able to deal with the state of the UEFI firmware after SetVirtualAddressMap() has been called. To avoid having separate code paths for non-kexec and kexec, let's move the call to SetVirtualAddressMap() to the stub: this will guarantee us that it will only be called once (since the stub is not executed during kexec), and ensures that the UEFI state is identical between kexec and normal boot.

This implies that the layout of the virtual mapping needs to be created by the stub as well. All regions are rounded up to a naturally aligned multiple of 64 KB (for compatibility with 64k pages kernels) and recorded in the UEFI memory map. The kernel proper reads those values and installs the mappings in a dedicated set of page tables that are swapped in during UEFI Runtime Services calls.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/efi.h       |  20 +++-
 arch/arm64/kernel/efi.c            | 223 ++++++++++++++++++++-----------------
 arch/arm64/kernel/setup.c          |   1 +
 drivers/firmware/efi/libstub/fdt.c | 137 ++++++++++++++++++++++-
 4 files changed, 270 insertions(+), 111 deletions(-)

arch/arm64/kernel/efi.c +7 -0

+void __init efi_virtmap_init(void)
+{
+       efi_memory_desc_t *md;
+
+       if (!efi_enabled(EFI_BOOT))
+               return;
+

drivers/firmware/efi/libstub/fdt.c +32 -0

+
+static void update_memory_map(efi_memory_desc_t *memory_map,
+                             unsigned long map_size, unsigned long desc_size,
+                             int *count)
+{
+       u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
+       efi_memory_desc_t *out = (void *)memory_map + map_size;
+       int l;
+
+       for (l = 0; l < map_size; l += desc_size) {
+               efi_memory_desc_t *in = (void *)memory_map + l;
+               u64 paddr, size;
+
+               if (!(in->attribute & EFI_MEMORY_RUNTIME))
+                       continue;
+
+               /*
+                * Make the mapping compatible with 64k pages: this allows
+                * a 4k page size kernel to kexec a 64k page size kernel and
+                * vice versa.
+                */
+               paddr = round_down(in->phys_addr, SZ_64K);
+               size = round_up(in->num_pages * EFI_PAGE_SIZE +
+                               in->phys_addr - paddr, SZ_64K);
+
+               /*
+                * Avoid wasting memory on PTEs by choosing a virtual base that
+                * is compatible with section mappings if this region has the
+                * appropriate size and physical alignment. (Sections are 2 MB
+                * on 4k granule kernels)
+                */
+               if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)

drivers/firmware/efi/libstub/fdt.c +1 -0

@@ -248,12 +337,52 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
                }
        }
 
+       /*

Hi Ard,

I have a few (minor) comments below.

Quoted 22 lines · Ard Biesheuvel

In order to support kexec, the kernel needs to be able to deal with the state of the UEFI firmware after SetVirtualAddressMap() has been called. To avoid having separate code paths for non-kexec and kexec, let's move the call to SetVirtualAddressMap() to the stub: this will guarantee us that it will only be called once (since the stub is not executed during kexec), and ensures that the UEFI state is identical between kexec and normal boot.

This implies that the layout of the virtual mapping needs to be created by the stub as well. All regions are rounded up to a naturally aligned multiple of 64 KB (for compatibility with 64k pages kernels) and recorded in the UEFI memory map. The kernel proper reads those values and installs the mappings in a dedicated set of page tables that are swapped in during UEFI Runtime Services calls.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/efi.h       |  20 +++-
 arch/arm64/kernel/efi.c            | 223 ++++++++++++++++++++-----------------
 arch/arm64/kernel/setup.c          |   1 +
 drivers/firmware/efi/libstub/fdt.c | 137 ++++++++++++++++++++++-
 4 files changed, 270 insertions(+), 111 deletions(-)

[...]

arch/arm64/kernel/efi.c static int __init arm64_dmi_init(void) · line 449

Quoted 7 lines

arch/arm64/kernel/efi.c +7 -0

+static void efi_set_pgd(struct mm_struct *mm)
+{
+       cpu_switch_mm(mm->pgd, mm);
+       flush_tlb_all();
+       if (icache_is_aivivt())
+               __flush_icache_all();
+}

Do we have any idea how often we call runtime services?

I assume not all that often (read the RTC at boot, set/get variables).

If we're nuking the TLBs and I-cache a lot we'll probably need to reserve an asid for the EFI virtmap.

Quoted 4 lines

(quoted diff) +0 -0

@@ -248,12 +337,52 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
                }
        }
 

drivers/firmware/efi/libstub/fdt.c efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table, · line 350

Quoted 11 lines

drivers/firmware/efi/libstub/fdt.c +11 -0

+       /*
+        * Update the memory map with virtual addresses. The function will also
+        * populate the spare second half of the memory_map allocation with
+        * copies of just the EFI_MEMORY_RUNTIME entries so that we can pass it
+        * straight into SetVirtualAddressMap()
+        */
+       update_memory_map(memory_map, map_size, desc_size,
+                         &runtime_entry_count);
+
+       pr_efi(sys_table,
+              "Exiting boot services and installing virtual address map...\n");

I believe that the memory map is allowed to change as a result of this call, so I think this needs to be moved before update_memory_map.

Hi Ard,

I have a few (minor) comments below.

Quoted 22 lines · Ard Biesheuvel

In order to support kexec, the kernel needs to be able to deal with the state of the UEFI firmware after SetVirtualAddressMap() has been called. To avoid having separate code paths for non-kexec and kexec, let's move the call to SetVirtualAddressMap() to the stub: this will guarantee us that it will only be called once (since the stub is not executed during kexec), and ensures that the UEFI state is identical between kexec and normal boot.

This implies that the layout of the virtual mapping needs to be created by the stub as well. All regions are rounded up to a naturally aligned multiple of 64 KB (for compatibility with 64k pages kernels) and recorded in the UEFI memory map. The kernel proper reads those values and installs the mappings in a dedicated set of page tables that are swapped in during UEFI Runtime Services calls.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/efi.h       |  20 +++-
 arch/arm64/kernel/efi.c            | 223 ++++++++++++++++++++-----------------
 arch/arm64/kernel/setup.c          |   1 +
 drivers/firmware/efi/libstub/fdt.c | 137 ++++++++++++++++++++++-
 4 files changed, 270 insertions(+), 111 deletions(-)

[...]

arch/arm64/kernel/efi.c static int __init arm64_dmi_init(void) · line 449

Quoted 7 lines

arch/arm64/kernel/efi.c +7 -0

+static void efi_set_pgd(struct mm_struct *mm)
+{
+       cpu_switch_mm(mm->pgd, mm);
+       flush_tlb_all();
+       if (icache_is_aivivt())
+               __flush_icache_all();
+}

Do we have any idea how often we call runtime services?

I assume not all that often (read the RTC at boot, set/get variables).

If we're nuking the TLBs and I-cache a lot we'll probably need to reserve an asid for the EFI virtmap.

Quoted 14 lines

arch/arm64/kernel/efi.c +14 -0

+void __init efi_virtmap_init(void)
+{
+       efi_memory_desc_t *md;
+
+       if (!efi_enabled(EFI_BOOT))
+               return;
+
+       for_each_efi_memory_desc(&memmap, md) {
+               u64 paddr, npages, size;
+               pgprot_t prot;
+
+               if (!(md->attribute & EFI_MEMORY_RUNTIME))
+                       continue;
+               if (WARN(md->virt_addr == 0,

arch/arm64/kernel/efi.c static int __init arm64_dmi_init(void) · line 478

Quoted 3 lines

arch/arm64/kernel/efi.c +3 -0

+                        "UEFI virtual mapping incomplete or missing -- no entry found for 0x%llx\n",
+                        md->phys_addr))
+                       return;

I wonder if we might want to use a different address to signal a bad mapping (e.g. 0UL-1 as it's not even EFI_PAGE_SIZE aligned), just in case we have to handle a valid use of zero in future for some reason -- perhaps coming from another OS.

arch/arm64/kernel/efi.c static int __init arm64_dmi_init(void) · line 485

Quoted 7 lines

arch/arm64/kernel/efi.c +7 -0

+
+               paddr = md->phys_addr;
+               npages = md->num_pages;
+               memrange_efi_to_native(&paddr, &npages);
+               size = npages << PAGE_SHIFT;
+
+               pr_info("  EFI remap 0x%012llx => %p\n",

Why not use %016llx? We might only have 48-bit PAs currently, but we may as well keep the VA and PA equally long when printing out -- that'll also help to identify issues with garbage in the upper 16 bits of the PA field.

[...]

drivers/firmware/efi/libstub/fdt.c fdt_set_fail: · line 214

Quoted 6 lines

drivers/firmware/efi/libstub/fdt.c +6 -0

+/*
+ * This is the base address at which to start allocating virtual memory ranges
+ * for UEFI Runtime Services. This is a userland range so that we can use any
+ * allocation we choose, and eliminate the risk of a conflict after kexec.
+ */
+#define EFI_RT_VIRTUAL_BASE    0x40000000

Nit: I'm not keen on the term "userland" here. You can map stuff to EL0 in the high half of the address space if you wanted, so TTBR0/TTBR1 aren't architecturally user/kernel.

s/userland/low half/, perhaps?

It might also be worth pointing out that the arbitrary base address isn't zero so as to be less likely to be an idmap.

Quoted 32 lines

drivers/firmware/efi/libstub/fdt.c +32 -0

+
+static void update_memory_map(efi_memory_desc_t *memory_map,
+                             unsigned long map_size, unsigned long desc_size,
+                             int *count)
+{
+       u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
+       efi_memory_desc_t *out = (void *)memory_map + map_size;
+       int l;
+
+       for (l = 0; l < map_size; l += desc_size) {
+               efi_memory_desc_t *in = (void *)memory_map + l;
+               u64 paddr, size;
+
+               if (!(in->attribute & EFI_MEMORY_RUNTIME))
+                       continue;
+
+               /*
+                * Make the mapping compatible with 64k pages: this allows
+                * a 4k page size kernel to kexec a 64k page size kernel and
+                * vice versa.
+                */
+               paddr = round_down(in->phys_addr, SZ_64K);
+               size = round_up(in->num_pages * EFI_PAGE_SIZE +
+                               in->phys_addr - paddr, SZ_64K);
+
+               /*
+                * Avoid wasting memory on PTEs by choosing a virtual base that
+                * is compatible with section mappings if this region has the
+                * appropriate size and physical alignment. (Sections are 2 MB
+                * on 4k granule kernels)
+                */
+               if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)

drivers/firmware/efi/libstub/fdt.c fdt_set_fail: · line 256

Quoted 10 lines

drivers/firmware/efi/libstub/fdt.c +10 -0

+                       efi_virt_base = round_up(efi_virt_base, SZ_2M);
+
+               in->virt_addr = efi_virt_base + in->phys_addr - paddr;
+               efi_virt_base += size;
+
+               memcpy(out, in, desc_size);
+               out = (void *)out + desc_size;
+               ++*count;
+       }
+}

This feels like this should live in arch/arm64, or under some other directory specific to arm/arm64. That said, I guess the fdt stuff is currently arm-specific anyway, so perhaps this is fine.

[...]

Quoted 4 lines

(quoted diff) +0 -0

@@ -248,12 +337,52 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
                }
        }
 

drivers/firmware/efi/libstub/fdt.c efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table, · line 350

Quoted 11 lines

drivers/firmware/efi/libstub/fdt.c +11 -0

+       /*
+        * Update the memory map with virtual addresses. The function will also
+        * populate the spare second half of the memory_map allocation with
+        * copies of just the EFI_MEMORY_RUNTIME entries so that we can pass it
+        * straight into SetVirtualAddressMap()
+        */
+       update_memory_map(memory_map, map_size, desc_size,
+                         &runtime_entry_count);
+
+       pr_efi(sys_table,
+              "Exiting boot services and installing virtual address map...\n");

I believe that the memory map is allowed to change as a result of this call, so I think this needs to be moved before update_memory_map.

In order to support kexec, the kernel needs to be able to deal with the state of the UEFI firmware after SetVirtualAddressMap() has been called. To avoid having separate code paths for non-kexec and kexec, let's move the call to SetVirtualAddressMap() to the stub: this will guarantee us that it will only be called once (since the stub is not executed during kexec), and ensures that the UEFI state is identical between kexec and normal boot.

This implies that the layout of the virtual mapping needs to be created by the stub as well. All regions are rounded up to a naturally aligned multiple of 64 KB (for compatibility with 64k pages kernels) and recorded in the UEFI memory map. The kernel proper reads those values and installs the mappings in a dedicated set of page tables that are swapped in during UEFI Runtime Services calls.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/efi.h       |  20 +++-
 arch/arm64/kernel/efi.c            | 223 ++++++++++++++++++++-----------------
 arch/arm64/kernel/setup.c          |   1 +
 drivers/firmware/efi/libstub/fdt.c | 137 ++++++++++++++++++++++-
 4 files changed, 270 insertions(+), 111 deletions(-)

drivers/firmware/efi/libstub/fdt.c +28 -0

index c846a9608cbd..76bc8abf41d1 100644
@@ -167,6 +167,94 @@ fdt_set_fail:
 #define EFI_FDT_ALIGN EFI_PAGE_SIZE
 #endif
 
+static efi_status_t get_memory_map(efi_system_table_t *sys_table_arg,
+				   efi_memory_desc_t **map,
+				   unsigned long *map_size,
+				   unsigned long *desc_size,
+				   u32 *desc_ver, unsigned long *key_ptr)
+{
+	efi_status_t status;
+
+	/*
+	 * Call get_memory_map() with 0 size to retrieve the size of the
+	 * required allocation.
+	 */
+	*map_size = 0;
+	status = efi_call_early(get_memory_map, map_size, NULL,
+				key_ptr, desc_size, desc_ver);
+	if (status != EFI_BUFFER_TOO_SMALL)
+		return EFI_LOAD_ERROR;
+
+	/*
+	 * Add an additional efi_memory_desc_t to map_size because we're doing
+	 * an allocation which may be in a new descriptor region. Then double it
+	 * to give us some scratch space to prepare the input virtmap to give
+	 * to SetVirtualAddressMap(). Note that this is EFI_LOADER_DATA memory,
+	 * and the kernel memblock_reserve()'s only the size of the actual
+	 * memory map, so the scratch space is freed again automatically.
+	 */
+	*map_size += *desc_size;
+	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,

drivers/firmware/efi/libstub/fdt.c +39 -0

+/*
+ * This is the base address at which to start allocating virtual memory ranges
+ * for UEFI Runtime Services. This is a userland range so that we can use any
+ * allocation we choose, and eliminate the risk of a conflict after kexec.
+ */
+#define EFI_RT_VIRTUAL_BASE	0x40000000
+
+static void update_memory_map(efi_memory_desc_t *memory_map,
+			      unsigned long map_size, unsigned long desc_size,
+			      int *count)
+{
+	u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
+	efi_memory_desc_t *out = (void *)memory_map + map_size;
+	int l;
+
+	for (l = 0; l < map_size; l += desc_size) {
+		efi_memory_desc_t *in = (void *)memory_map + l;
+		u64 paddr, size;
+
+		if (!(in->attribute & EFI_MEMORY_RUNTIME))
+			continue;
+
+		/*
+		 * Make the mapping compatible with 64k pages: this allows
+		 * a 4k page size kernel to kexec a 64k page size kernel and
+		 * vice versa.
+		 */
+		paddr = round_down(in->phys_addr, SZ_64K);
+		size = round_up(in->num_pages * EFI_PAGE_SIZE +
+				in->phys_addr - paddr, SZ_64K);
+
+		/*
+		 * Avoid wasting memory on PTEs by choosing a virtual base that
+		 * is compatible with section mappings if this region has the
+		 * appropriate size and physical alignment. (Sections are 2 MB
+		 * on 4k granule kernels)
+		 */
+		if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
+			efi_virt_base = round_up(efi_virt_base, SZ_2M);

Quoted 33 lines

drivers/firmware/efi/libstub/fdt.c +33 -0

+static efi_status_t get_memory_map(efi_system_table_t *sys_table_arg,
+				   efi_memory_desc_t **map,
+				   unsigned long *map_size,
+				   unsigned long *desc_size,
+				   u32 *desc_ver, unsigned long *key_ptr)
+{
+	efi_status_t status;
+
+	/*
+	 * Call get_memory_map() with 0 size to retrieve the size of the
+	 * required allocation.
+	 */
+	*map_size = 0;
+	status = efi_call_early(get_memory_map, map_size, NULL,
+				key_ptr, desc_size, desc_ver);
+	if (status != EFI_BUFFER_TOO_SMALL)
+		return EFI_LOAD_ERROR;
+
+	/*
+	 * Add an additional efi_memory_desc_t to map_size because we're doing
+	 * an allocation which may be in a new descriptor region. Then double it
+	 * to give us some scratch space to prepare the input virtmap to give
+	 * to SetVirtualAddressMap(). Note that this is EFI_LOADER_DATA memory,
+	 * and the kernel memblock_reserve()'s only the size of the actual
+	 * memory map, so the scratch space is freed again automatically.
+	 */
+	*map_size += *desc_size;
+	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
+				*map_size * 2, (void **)map);
+	if (status != EFI_SUCCESS)
+		return status;
+
+	status = efi_call_early(get_memory_map, map_size, *map,

drivers/firmware/efi/libstub/fdt.c fdt_set_fail: · line 207

Quoted 5 lines

drivers/firmware/efi/libstub/fdt.c +5 -0

+				key_ptr, desc_size, desc_ver);
+	if (status != EFI_SUCCESS)
+		efi_call_early(free_pool, *map);
+	return status;
+}

We've now got two (slightly different) versions of this function. Is there any way we could make do with just one?

Quoted 43 lines

drivers/firmware/efi/libstub/fdt.c +43 -0

+/*
+ * This is the base address at which to start allocating virtual memory ranges
+ * for UEFI Runtime Services. This is a userland range so that we can use any
+ * allocation we choose, and eliminate the risk of a conflict after kexec.
+ */
+#define EFI_RT_VIRTUAL_BASE	0x40000000
+
+static void update_memory_map(efi_memory_desc_t *memory_map,
+			      unsigned long map_size, unsigned long desc_size,
+			      int *count)
+{
+	u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
+	efi_memory_desc_t *out = (void *)memory_map + map_size;
+	int l;
+
+	for (l = 0; l < map_size; l += desc_size) {
+		efi_memory_desc_t *in = (void *)memory_map + l;
+		u64 paddr, size;
+
+		if (!(in->attribute & EFI_MEMORY_RUNTIME))
+			continue;
+
+		/*
+		 * Make the mapping compatible with 64k pages: this allows
+		 * a 4k page size kernel to kexec a 64k page size kernel and
+		 * vice versa.
+		 */
+		paddr = round_down(in->phys_addr, SZ_64K);
+		size = round_up(in->num_pages * EFI_PAGE_SIZE +
+				in->phys_addr - paddr, SZ_64K);
+
+		/*
+		 * Avoid wasting memory on PTEs by choosing a virtual base that
+		 * is compatible with section mappings if this region has the
+		 * appropriate size and physical alignment. (Sections are 2 MB
+		 * on 4k granule kernels)
+		 */
+		if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
+			efi_virt_base = round_up(efi_virt_base, SZ_2M);
+
+		in->virt_addr = efi_virt_base + in->phys_addr - paddr;
+		efi_virt_base += size;
+

drivers/firmware/efi/libstub/fdt.c fdt_set_fail: · line 257

Quoted 6 lines

drivers/firmware/efi/libstub/fdt.c +6 -0

+		memcpy(out, in, desc_size);
+		out = (void *)out + desc_size;
+		++*count;
+	}
+}
+

fdt.c is starting to become a dumping ground for arm*-specific stuff :-(

Is there no general solution for sharing code between arm and aarch64 in the kernel so we don't have to stick things like this in drivers/firmware/efi/?

On Mon, Jan 05, 2015 at 04:54:02PM +0000, Matt Fleming wrote:

I think all we really need above what efi_get_memory_map() provides is the scratch space. Would we care about temporarily wasting a little bit of EFI_LOADER_DATA on all platforms, or could we just swap the function body in efi-stub-helper.c for Ard's version above?

(I would guess memory maps with <= 32 entries are uncommon anyway, so the existing version would already make the bootservice call twice.)

Mmm, not optimal.
That said, the only arm*-specific things about this particular function are the page sizes. Should this move to efi-stub-helper.c with EFI_RT_VIRTUAL_BASE moved to arch/<x>/include/asm/efi.h and joined by EFI_RT_VIRTUAL_BASE_ALIGN and EFI_RT_VIRTUAL_REGION_ALIGN?

Quoted 22 lines · Ard Biesheuvel

In order to support kexec, the kernel needs to be able to deal with the state of the UEFI firmware after SetVirtualAddressMap() has been called. To avoid having separate code paths for non-kexec and kexec, let's move the call to SetVirtualAddressMap() to the stub: this will guarantee us that it will only be called once (since the stub is not executed during kexec), and ensures that the UEFI state is identical between kexec and normal boot.

This implies that the layout of the virtual mapping needs to be created by the stub as well. All regions are rounded up to a naturally aligned multiple of 64 KB (for compatibility with 64k pages kernels) and recorded in the UEFI memory map. The kernel proper reads those values and installs the mappings in a dedicated set of page tables that are swapped in during UEFI Runtime Services calls.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/efi.h       |  20 +++-
 arch/arm64/kernel/efi.c            | 223 ++++++++++++++++++++-----------------
 arch/arm64/kernel/setup.c          |   1 +
 drivers/firmware/efi/libstub/fdt.c | 137 ++++++++++++++++++++++-
 4 files changed, 270 insertions(+), 111 deletions(-)

[...]

Quoted 3 lines

arch/arm64/include/asm/efi.h +0 -0

@@ -45,5 +53,9 @@ extern void efi_idmap_init(void);
 #define efi_call_early(f, ...) sys_table_arg->boottime->f(__VA_ARGS__)
 

arch/arm64/include/asm/efi.h extern void efi_idmap_init(void); · line 56

Quoted 2 lines

arch/arm64/include/asm/efi.h +1 -0

 #define EFI_ALLOC_ALIGN              SZ_64K
+#define EFI_VIRTMAP          EFI_ARCH_1

Any chance of documenting what EFI_VIRTMAP means for posterity and why you want to use one of the EFI arch config bits? How is this different from EFI_RUNTIME_SERVICES?

Take a look at EFI_OLD_MEMMAP in arch/x86/include/asm/efi.h for a crackingly well documented example from Borislav.

[...]

Quoted 41 lines

drivers/firmware/efi/libstub/fdt.c +33 -0

index c846a9608cbd..76bc8abf41d1 100644
@@ -167,6 +167,94 @@ fdt_set_fail:
 #define EFI_FDT_ALIGN EFI_PAGE_SIZE
 #endif
 
+static efi_status_t get_memory_map(efi_system_table_t *sys_table_arg,
+                                efi_memory_desc_t **map,
+                                unsigned long *map_size,
+                                unsigned long *desc_size,
+                                u32 *desc_ver, unsigned long *key_ptr)
+{
+     efi_status_t status;
+
+     /*
+      * Call get_memory_map() with 0 size to retrieve the size of the
+      * required allocation.
+      */
+     *map_size = 0;
+     status = efi_call_early(get_memory_map, map_size, NULL,
+                             key_ptr, desc_size, desc_ver);
+     if (status != EFI_BUFFER_TOO_SMALL)
+             return EFI_LOAD_ERROR;
+
+     /*
+      * Add an additional efi_memory_desc_t to map_size because we're doing
+      * an allocation which may be in a new descriptor region. Then double it
+      * to give us some scratch space to prepare the input virtmap to give
+      * to SetVirtualAddressMap(). Note that this is EFI_LOADER_DATA memory,
+      * and the kernel memblock_reserve()'s only the size of the actual
+      * memory map, so the scratch space is freed again automatically.
+      */
+     *map_size += *desc_size;
+     status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
+                             *map_size * 2, (void **)map);
+     if (status != EFI_SUCCESS)
+             return status;
+
+     status = efi_call_early(get_memory_map, map_size, *map,

drivers/firmware/efi/libstub/fdt.c fdt_set_fail: · line 207

Quoted 5 lines

drivers/firmware/efi/libstub/fdt.c +5 -0

+                             key_ptr, desc_size, desc_ver);
+     if (status != EFI_SUCCESS)
+             efi_call_early(free_pool, *map);
+     return status;
+}

We've now got two (slightly different) versions of this function. Is there any way we could make do with just one?

Quoted 39 lines

drivers/firmware/efi/libstub/fdt.c +39 -0

+/*
+ * This is the base address at which to start allocating virtual memory ranges
+ * for UEFI Runtime Services. This is a userland range so that we can use any
+ * allocation we choose, and eliminate the risk of a conflict after kexec.
+ */
+#define EFI_RT_VIRTUAL_BASE  0x40000000
+
+static void update_memory_map(efi_memory_desc_t *memory_map,
+                           unsigned long map_size, unsigned long desc_size,
+                           int *count)
+{
+     u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
+     efi_memory_desc_t *out = (void *)memory_map + map_size;
+     int l;
+
+     for (l = 0; l < map_size; l += desc_size) {
+             efi_memory_desc_t *in = (void *)memory_map + l;
+             u64 paddr, size;
+
+             if (!(in->attribute & EFI_MEMORY_RUNTIME))
+                     continue;
+
+             /*
+              * Make the mapping compatible with 64k pages: this allows
+              * a 4k page size kernel to kexec a 64k page size kernel and
+              * vice versa.
+              */
+             paddr = round_down(in->phys_addr, SZ_64K);
+             size = round_up(in->num_pages * EFI_PAGE_SIZE +
+                             in->phys_addr - paddr, SZ_64K);
+
+             /*
+              * Avoid wasting memory on PTEs by choosing a virtual base that
+              * is compatible with section mappings if this region has the
+              * appropriate size and physical alignment. (Sections are 2 MB
+              * on 4k granule kernels)
+              */
+             if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
+                     efi_virt_base = round_up(efi_virt_base, SZ_2M);

drivers/firmware/efi/libstub/fdt.c fdt_set_fail: · line 257

Quoted 10 lines

drivers/firmware/efi/libstub/fdt.c +10 -0

+
+             in->virt_addr = efi_virt_base + in->phys_addr - paddr;
+             efi_virt_base += size;
+
+             memcpy(out, in, desc_size);
+             out = (void *)out + desc_size;
+             ++*count;
+     }
+}
+

fdt.c is starting to become a dumping ground for arm*-specific stuff :-(

Is there no general solution for sharing code between arm and aarch64 in the kernel so we don't have to stick things like this in drivers/firmware/efi/?

In order to support kexec, the kernel needs to be able to deal with the state of the UEFI firmware after SetVirtualAddressMap() has been called. To avoid having separate code paths for non-kexec and kexec, let's move the call to SetVirtualAddressMap() to the stub: this will guarantee us that it will only be called once (since the stub is not executed during kexec), and ensures that the UEFI state is identical between kexec and normal boot.

This implies that the layout of the virtual mapping needs to be created by the stub as well. All regions are rounded up to a naturally aligned multiple of 64 KB (for compatibility with 64k pages kernels) and recorded in the UEFI memory map. The kernel proper reads those values and installs the mappings in a dedicated set of page tables that are swapped in during UEFI Runtime Services calls.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/efi.h       |  20 +++-
 arch/arm64/kernel/efi.c            | 223 ++++++++++++++++++++-----------------
 arch/arm64/kernel/setup.c          |   1 +
 drivers/firmware/efi/libstub/fdt.c | 137 ++++++++++++++++++++++-
 4 files changed, 270 insertions(+), 111 deletions(-)

arch/arm64/include/asm/efi.h +2 -0

index 71291253114f..6cc668a378c5 100644
@@ -7,28 +7,36 @@
 #ifdef CONFIG_EFI
 extern void efi_init(void);
 extern void efi_idmap_init(void);
+extern void efi_virtmap_init(void);
 #else
 #define efi_init()
 #define efi_idmap_init()
+#define efi_virtmap_init

arch/arm64/include/asm/efi.h +3 -0

+	efi_virtmap_load();						\
+	__f = efi.systab->runtime->f;					\
 	__s = __f(__VA_ARGS__);						\
+	efi_virtmap_unload();						\
 	kernel_neon_end();						\

arch/arm64/include/asm/efi.h +7 -0

+	efi_virtmap_load();						\
+	__f = efi.systab->runtime->f;					\
 	__f(__VA_ARGS__);						\
+	efi_virtmap_unload();						\
 	kernel_neon_end();						\
 })
 

@@ -45,5 +53,9 @@ extern void efi_idmap_init(void);
 #define efi_call_early(f, ...) sys_table_arg->boottime->f(__VA_ARGS__)
 
 #define EFI_ALLOC_ALIGN		SZ_64K
+#define EFI_VIRTMAP		EFI_ARCH_1
+
+void efi_virtmap_load(void);
+void efi_virtmap_unload(void);
 
 #endif /* _ASM_EFI_H */

arch/arm64/kernel/efi.c +39 -30

index 6fac253bc783..2ebe67ffb629 100644
@@ -11,25 +11,30 @@
  *
  */
 
+#include <linux/atomic.h>
 #include <linux/dmi.h>
 #include <linux/efi.h>
 #include <linux/export.h>
 #include <linux/memblock.h>
+#include <linux/mm_types.h>
 #include <linux/bootmem.h>
 #include <linux/of.h>
 #include <linux/of_fdt.h>
+#include <linux/rbtree.h>
+#include <linux/rwsem.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 
 #include <asm/cacheflush.h>
 #include <asm/efi.h>
 #include <asm/tlbflush.h>
 #include <asm/mmu_context.h>
+#include <asm/mmu.h>
+#include <asm/pgtable.h>
 
 struct efi_memory_map memmap;
 
-static efi_runtime_services_t *runtime;
-
 static u64 efi_system_table;
 
 static int uefi_debug __initdata;

@@ -69,9 +74,33 @@ static void __init efi_setup_idmap(void)
 	}
 }
 
+/*
+ * Translate a EFI virtual address into a physical address: this is necessary,
+ * as some data members of the EFI system table are virtually remapped after
+ * SetVirtualAddressMap() has been called.
+ */
+static phys_addr_t efi_to_phys(unsigned long addr)
+{
+	efi_memory_desc_t *md;
+
+	for_each_efi_memory_desc(&memmap, md) {
+		if (!(md->attribute & EFI_MEMORY_RUNTIME))
+			continue;
+		if (md->virt_addr == 0)
+			/* no virtual mapping has been installed by the stub */
+			break;
+		if (md->virt_addr <= addr &&
+		    (addr - md->virt_addr) < (md->num_pages << EFI_PAGE_SHIFT))
+			return md->phys_addr + addr - md->virt_addr;
+	}
+	return addr;
+}
+
 static int __init uefi_init(void)
 {
 	efi_char16_t *c16;
+	void *config_tables;
+	u64 table_size;
 	char vendor[100] = "unknown";
 	int i, retval;
 

@@ -99,7 +128,7 @@ static int __init uefi_init(void)
 			efi.systab->hdr.revision & 0xffff);
 
 	/* Show what we know for posterity */
-	c16 = early_memremap(efi.systab->fw_vendor,
+	c16 = early_memremap(efi_to_phys(efi.systab->fw_vendor),
 			     sizeof(vendor));
 	if (c16) {
 		for (i = 0; i < (int) sizeof(vendor) - 1 && *c16; ++i)

@@ -112,8 +141,14 @@ static int __init uefi_init(void)
 		efi.systab->hdr.revision >> 16,
 		efi.systab->hdr.revision & 0xffff, vendor);
 
-	retval = efi_config_init(NULL);
+	table_size = sizeof(efi_config_table_64_t) * efi.systab->nr_tables;
+	config_tables = early_memremap(efi_to_phys(efi.systab->tables),
+				       table_size);
+
+	retval = efi_config_parse_tables(config_tables,
+					 efi.systab->nr_tables, NULL);
 
+	early_memunmap(config_tables, table_size);
 out:
 	early_memunmap(efi.systab,  sizeof(efi_system_table_t));
 	return retval;

@@ -328,51 +363,9 @@ void __init efi_idmap_init(void)
 	efi_setup_idmap();
 }
 
-static int __init remap_region(efi_memory_desc_t *md, void **new)
-{
-	u64 paddr, vaddr, npages, size;
-
-	paddr = md->phys_addr;
-	npages = md->num_pages;
-	memrange_efi_to_native(&paddr, &npages);
-	size = npages << PAGE_SHIFT;
-
-	if (is_normal_ram(md))
-		vaddr = (__force u64)ioremap_cache(paddr, size);
-	else
-		vaddr = (__force u64)ioremap(paddr, size);
-
-	if (!vaddr) {
-		pr_err("Unable to remap 0x%llx pages @ %p\n",
-		       npages, (void *)paddr);
-		return 0;
-	}
-
-	/* adjust for any rounding when EFI and system pagesize differs */
-	md->virt_addr = vaddr + (md->phys_addr - paddr);
-
-	if (uefi_debug)
-		pr_info("  EFI remap 0x%012llx => %p\n",
-			md->phys_addr, (void *)md->virt_addr);

arch/arm64/kernel/efi.c +81 -63

 static int __init arm64_enter_virtual_mode(void)
 {
-	efi_memory_desc_t *md;
-	phys_addr_t virtmap_phys;
-	void *virtmap, *virt_md;
-	efi_status_t status;
 	u64 mapsize;
-	int count = 0;
-	unsigned long flags;
 
 	if (!efi_enabled(EFI_BOOT)) {
 		pr_info("EFI services will not be available.\n");

@@ -395,79 +388,28 @@ static int __init arm64_enter_virtual_mode(void)
 
 	efi.memmap = &memmap;
 
-	/* Map the runtime regions */
-	virtmap = kmalloc(mapsize, GFP_KERNEL);
-	if (!virtmap) {
-		pr_err("Failed to allocate EFI virtual memmap\n");
-		return -1;
-	}
-	virtmap_phys = virt_to_phys(virtmap);
-	virt_md = virtmap;
-
-	for_each_efi_memory_desc(&memmap, md) {
-		if (!(md->attribute & EFI_MEMORY_RUNTIME))
-			continue;
-		if (!remap_region(md, &virt_md))
-			goto err_unmap;
-		++count;
-	}
-
-	efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table);
+	efi.systab = (__force void *)ioremap_cache(efi_system_table,
+						   sizeof(efi_system_table_t));
 	if (!efi.systab) {
-		/*
-		 * If we have no virtual mapping for the System Table at this
-		 * point, the memory map doesn't cover the physical offset where
-		 * it resides. This means the System Table will be inaccessible
-		 * to Runtime Services themselves once the virtual mapping is
-		 * installed.
-		 */
-		pr_err("Failed to remap EFI System Table -- buggy firmware?\n");
-		goto err_unmap;
+		pr_err("Failed to remap EFI System Table\n");
+		return -1;
 	}
 	set_bit(EFI_SYSTEM_TABLES, &efi.flags);
 
-	local_irq_save(flags);
-	cpu_switch_mm(idmap_pg_dir, &init_mm);
-
-	/* Call SetVirtualAddressMap with the physical address of the map */
-	runtime = efi.systab->runtime;
-	efi.set_virtual_address_map = runtime->set_virtual_address_map;
-
-	status = efi.set_virtual_address_map(count * memmap.desc_size,
-					     memmap.desc_size,
-					     memmap.desc_version,
-					     (efi_memory_desc_t *)virtmap_phys);
-	cpu_set_reserved_ttbr0();
-	flush_tlb_all();
-	local_irq_restore(flags);
-
-	kfree(virtmap);
-
 	free_boot_services();
 
-	if (status != EFI_SUCCESS) {
-		pr_err("Failed to set EFI virtual address map! [%lx]\n",
-			status);
+	if (!efi_enabled(EFI_VIRTMAP)) {
+		pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
 		return -1;
 	}
 
 	/* Set up runtime services function pointers */
-	runtime = efi.systab->runtime;
 	efi_native_runtime_setup();
 	set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
 
 	efi.runtime_version = efi.systab->hdr.revision;
 
 	return 0;
-
-err_unmap:
-	/* unmap all mappings that succeeded: there are 'count' of those */
-	for (virt_md = virtmap; count--; virt_md += memmap.desc_size) {
-		md = virt_md;
-		iounmap((__force void __iomem *)md->virt_addr);
-	}
-	kfree(virtmap);
-	return -1;
 }
 early_initcall(arm64_enter_virtual_mode);
 

@@ -484,3 +426,78 @@ static int __init arm64_dmi_init(void)
 	return 0;
 }
 core_initcall(arm64_dmi_init);
+
+static pgd_t efi_pgd[PTRS_PER_PGD] __page_aligned_bss;
+
+static struct mm_struct efi_mm = {
+	.mm_rb			= RB_ROOT,
+	.pgd			= efi_pgd,
+	.mm_users		= ATOMIC_INIT(2),
+	.mm_count		= ATOMIC_INIT(1),
+	.mmap_sem		= __RWSEM_INITIALIZER(efi_mm.mmap_sem),
+	.page_table_lock	= __SPIN_LOCK_UNLOCKED(efi_mm.page_table_lock),
+	.mmlist			= LIST_HEAD_INIT(efi_mm.mmlist),
+	INIT_MM_CONTEXT(efi_mm)
+};
+
+static void efi_set_pgd(struct mm_struct *mm)
+{
+	cpu_switch_mm(mm->pgd, mm);
+	flush_tlb_all();
+	if (icache_is_aivivt())
+		__flush_icache_all();
+}
+
+void efi_virtmap_load(void)
+{
+	WARN_ON(preemptible());
+	efi_set_pgd(&efi_mm);
+}
+
+void efi_virtmap_unload(void)
+{
+	efi_set_pgd(current->active_mm);
+}
+
+void __init efi_virtmap_init(void)
+{
+	efi_memory_desc_t *md;
+
+	if (!efi_enabled(EFI_BOOT))
+		return;
+
+	for_each_efi_memory_desc(&memmap, md) {
+		u64 paddr, npages, size;
+		pgprot_t prot;
+
+		if (!(md->attribute & EFI_MEMORY_RUNTIME))
+			continue;
+		if (WARN(md->virt_addr == 0,
+			 "UEFI virtual mapping incomplete or missing -- no entry found for 0x%llx\n",
+			 md->phys_addr))
+			return;
+
+		paddr = md->phys_addr;
+		npages = md->num_pages;
+		memrange_efi_to_native(&paddr, &npages);
+		size = npages << PAGE_SHIFT;
+
+		pr_info("  EFI remap 0x%012llx => %p\n",
+			md->phys_addr, (void *)md->virt_addr);
+
+		/*
+		 * Only regions of type EFI_RUNTIME_SERVICES_CODE need to be
+		 * executable, everything else can be mapped with the XN bits
+		 * set.
+		 */
+		if (!is_normal_ram(md))
+			prot = __pgprot(PROT_DEVICE_nGnRE);
+		else if (md->type == EFI_RUNTIME_SERVICES_CODE)
+			prot = PAGE_KERNEL_EXEC;
+		else
+			prot = PAGE_KERNEL;
+
+		create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, prot);
+	}
+	set_bit(EFI_VIRTMAP, &efi.flags);
+}

arch/arm64/kernel/setup.c +0 -0

index b80991166754..d8390f507da0 100644

arch/arm64/kernel/setup.c +0 -0

 	unflatten_device_tree();
 

drivers/firmware/efi/libstub/fdt.c +133 -4

index c846a9608cbd..76bc8abf41d1 100644
@@ -167,6 +167,94 @@ fdt_set_fail:
 #define EFI_FDT_ALIGN EFI_PAGE_SIZE
 #endif
 
+static efi_status_t get_memory_map(efi_system_table_t *sys_table_arg,
+				   efi_memory_desc_t **map,
+				   unsigned long *map_size,
+				   unsigned long *desc_size,
+				   u32 *desc_ver, unsigned long *key_ptr)
+{
+	efi_status_t status;
+
+	/*
+	 * Call get_memory_map() with 0 size to retrieve the size of the
+	 * required allocation.
+	 */
+	*map_size = 0;
+	status = efi_call_early(get_memory_map, map_size, NULL,
+				key_ptr, desc_size, desc_ver);
+	if (status != EFI_BUFFER_TOO_SMALL)
+		return EFI_LOAD_ERROR;
+
+	/*
+	 * Add an additional efi_memory_desc_t to map_size because we're doing
+	 * an allocation which may be in a new descriptor region. Then double it
+	 * to give us some scratch space to prepare the input virtmap to give
+	 * to SetVirtualAddressMap(). Note that this is EFI_LOADER_DATA memory,
+	 * and the kernel memblock_reserve()'s only the size of the actual
+	 * memory map, so the scratch space is freed again automatically.
+	 */
+	*map_size += *desc_size;
+	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
+				*map_size * 2, (void **)map);
+	if (status != EFI_SUCCESS)
+		return status;
+
+	status = efi_call_early(get_memory_map, map_size, *map,
+				key_ptr, desc_size, desc_ver);
+	if (status != EFI_SUCCESS)
+		efi_call_early(free_pool, *map);
+	return status;
+}
+
+/*
+ * This is the base address at which to start allocating virtual memory ranges
+ * for UEFI Runtime Services. This is a userland range so that we can use any
+ * allocation we choose, and eliminate the risk of a conflict after kexec.
+ */
+#define EFI_RT_VIRTUAL_BASE	0x40000000
+
+static void update_memory_map(efi_memory_desc_t *memory_map,
+			      unsigned long map_size, unsigned long desc_size,
+			      int *count)
+{
+	u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
+	efi_memory_desc_t *out = (void *)memory_map + map_size;
+	int l;
+
+	for (l = 0; l < map_size; l += desc_size) {
+		efi_memory_desc_t *in = (void *)memory_map + l;
+		u64 paddr, size;
+
+		if (!(in->attribute & EFI_MEMORY_RUNTIME))
+			continue;
+
+		/*
+		 * Make the mapping compatible with 64k pages: this allows
+		 * a 4k page size kernel to kexec a 64k page size kernel and
+		 * vice versa.
+		 */
+		paddr = round_down(in->phys_addr, SZ_64K);
+		size = round_up(in->num_pages * EFI_PAGE_SIZE +
+				in->phys_addr - paddr, SZ_64K);
+
+		/*
+		 * Avoid wasting memory on PTEs by choosing a virtual base that
+		 * is compatible with section mappings if this region has the
+		 * appropriate size and physical alignment. (Sections are 2 MB
+		 * on 4k granule kernels)
+		 */
+		if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
+			efi_virt_base = round_up(efi_virt_base, SZ_2M);
+
+		in->virt_addr = efi_virt_base + in->phys_addr - paddr;
+		efi_virt_base += size;
+
+		memcpy(out, in, desc_size);
+		out = (void *)out + desc_size;
+		++*count;
+	}
+}
+
 /*
  * Allocate memory for a new FDT, then add EFI, commandline, and
  * initrd related fields to the FDT.  This routine increases the

@@ -196,6 +284,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 	efi_memory_desc_t *memory_map;
 	unsigned long new_fdt_size;
 	efi_status_t status;
+	int runtime_entry_count = 0;
 
 	/*
 	 * Estimate size of new FDT, and allocate memory for it. We

@@ -216,8 +305,8 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 		 * we can get the memory map key  needed for
 		 * exit_boot_services().
 		 */
-		status = efi_get_memory_map(sys_table, &memory_map, &map_size,
-					    &desc_size, &desc_ver, &mmap_key);
+		status = get_memory_map(sys_table, &memory_map, &map_size,
+					&desc_size, &desc_ver, &mmap_key);
 		if (status != EFI_SUCCESS)
 			goto fail_free_new_fdt;
 

@@ -248,12 +337,52 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 		}
 	}
 
+	/*
+	 * Update the memory map with virtual addresses. The function will also
+	 * populate the spare second half of the memory_map allocation with
+	 * copies of just the EFI_MEMORY_RUNTIME entries so that we can pass it
+	 * straight into SetVirtualAddressMap()
+	 */
+	update_memory_map(memory_map, map_size, desc_size,
+			  &runtime_entry_count);
+
+	pr_efi(sys_table,
+	       "Exiting boot services and installing virtual address map...\n");
+
 	/* Now we are ready to exit_boot_services.*/
 	status = sys_table->boottime->exit_boot_services(handle, mmap_key);
 
+	if (status == EFI_SUCCESS) {
+		efi_set_virtual_address_map_t *svam;
 
-	if (status == EFI_SUCCESS)
-		return status;
+		/* Install the new virtual address map */
+		svam = sys_table->runtime->set_virtual_address_map;
+		status = svam(runtime_entry_count * desc_size, desc_size,
+			      desc_ver, (void *)memory_map + map_size);
+
+		/*
+		 * We are beyond the point of no return here, so if the call to
+		 * SetVirtualAddressMap() failed, we need to signal that to the
+		 * incoming kernel but proceed normally otherwise.
+		 */
+		if (status != EFI_SUCCESS) {
+			int l;
+
+			/*
+			 * Set the virtual address field of all
+			 * EFI_MEMORY_RUNTIME entries to 0. This will signal
+			 * the incoming kernel that no virtual translation has
+			 * been installed.
+			 */
+			for (l = 0; l < map_size; l += desc_size) {
+				efi_memory_desc_t *p = (void *)memory_map + l;
+
+				if (p->attribute & EFI_MEMORY_RUNTIME)
+					p->virt_addr = 0;
+			}
+		}
+		return EFI_SUCCESS;
+	}
 
 	pr_efi_err(sys_table, "Exit boot services failed.\n");
 

Signature · 1 line

1.8.3.2

Quoted 36 lines · Ard Biesheuvel

In order to support kexec, the kernel needs to be able to deal with the state of the UEFI firmware after SetVirtualAddressMap() has been called. To avoid having separate code paths for non-kexec and kexec, let's move the call to SetVirtualAddressMap() to the stub: this will guarantee us that it will only be called once (since the stub is not executed during kexec), and ensures that the UEFI state is identical between kexec and normal boot.

This implies that the layout of the virtual mapping needs to be created by the stub as well. All regions are rounded up to a naturally aligned multiple of 64 KB (for compatibility with 64k pages kernels) and recorded in the UEFI memory map. The kernel proper reads those values and installs the mappings in a dedicated set of page tables that are swapped in during UEFI Runtime Services calls.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/efi.h       |  20 +++-
 arch/arm64/kernel/efi.c            | 223 ++++++++++++++++++++-----------------
 arch/arm64/kernel/setup.c          |   1 +
 drivers/firmware/efi/libstub/fdt.c | 137 ++++++++++++++++++++++-
 4 files changed, 270 insertions(+), 111 deletions(-)

arch/arm64/include/asm/efi.h +2 -0

index 71291253114f..6cc668a378c5 100644
@@ -7,28 +7,36 @@
 #ifdef CONFIG_EFI
 extern void efi_init(void);
 extern void efi_idmap_init(void);
+extern void efi_virtmap_init(void);
 #else
 #define efi_init()
 #define efi_idmap_init()
+#define efi_virtmap_init

arch/arm64/include/asm/efi.h line 22

Quoted 10 lines

arch/arm64/include/asm/efi.h +2 -2

 #endif
 
 #define efi_call_virt(f, ...)                                                \
 ({                                                                   \
-     efi_##f##_t *__f = efi.systab->runtime->f;                      \
+     efi_##f##_t *__f;                                               \
      efi_status_t __s;                                               \
                                                                      \
-     kernel_neon_begin();                                            \
+     kernel_neon_begin();    /* disables preemption */               \

Nitpick: adding comment to otherwise untouched source line.

Quoted 5 lines

arch/arm64/include/asm/efi.h +3 -0

+     efi_virtmap_load();                                             \
+     __f = efi.systab->runtime->f;                                   \
      __s = __f(__VA_ARGS__);                                         \
+     efi_virtmap_unload();                                           \
      kernel_neon_end();                                              \

arch/arm64/include/asm/efi.h line 35

Quoted 10 lines

arch/arm64/include/asm/efi.h +2 -2

      __s;                                                            \
 })
 
 #define __efi_call_virt(f, ...)                                              \
 ({                                                                   \
-     efi_##f##_t *__f = efi.systab->runtime->f;                      \
+     efi_##f##_t *__f;                                               \
                                                                      \
-     kernel_neon_begin();                                            \
+     kernel_neon_begin();    /* disables preemption */               \

Same nitpick.

Quoted 147 lines

arch/arm64/include/asm/efi.h +7 -0

+     efi_virtmap_load();                                             \
+     __f = efi.systab->runtime->f;                                   \
      __f(__VA_ARGS__);                                               \
+     efi_virtmap_unload();                                           \
      kernel_neon_end();                                              \
 })
 

@@ -45,5 +53,9 @@ extern void efi_idmap_init(void);
 #define efi_call_early(f, ...) sys_table_arg->boottime->f(__VA_ARGS__)
 
 #define EFI_ALLOC_ALIGN              SZ_64K
+#define EFI_VIRTMAP          EFI_ARCH_1
+
+void efi_virtmap_load(void);
+void efi_virtmap_unload(void);
 
 #endif /* _ASM_EFI_H */

arch/arm64/kernel/efi.c +39 -34

index 6fac253bc783..2ebe67ffb629 100644
@@ -11,25 +11,30 @@
  *
  */
 
+#include <linux/atomic.h>
 #include <linux/dmi.h>
 #include <linux/efi.h>
 #include <linux/export.h>
 #include <linux/memblock.h>
+#include <linux/mm_types.h>
 #include <linux/bootmem.h>
 #include <linux/of.h>
 #include <linux/of_fdt.h>
+#include <linux/rbtree.h>
+#include <linux/rwsem.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 
 #include <asm/cacheflush.h>
 #include <asm/efi.h>
 #include <asm/tlbflush.h>
 #include <asm/mmu_context.h>
+#include <asm/mmu.h>
+#include <asm/pgtable.h>
 
 struct efi_memory_map memmap;
 
-static efi_runtime_services_t *runtime;
-
 static u64 efi_system_table;
 
 static int uefi_debug __initdata;

@@ -69,9 +74,33 @@ static void __init efi_setup_idmap(void)
      }
 }
 
+/*
+ * Translate a EFI virtual address into a physical address: this is necessary,
+ * as some data members of the EFI system table are virtually remapped after
+ * SetVirtualAddressMap() has been called.
+ */
+static phys_addr_t efi_to_phys(unsigned long addr)
+{
+     efi_memory_desc_t *md;
+
+     for_each_efi_memory_desc(&memmap, md) {
+             if (!(md->attribute & EFI_MEMORY_RUNTIME))
+                     continue;
+             if (md->virt_addr == 0)
+                     /* no virtual mapping has been installed by the stub */
+                     break;
+             if (md->virt_addr <= addr &&
+                 (addr - md->virt_addr) < (md->num_pages << EFI_PAGE_SHIFT))
+                     return md->phys_addr + addr - md->virt_addr;
+     }
+     return addr;
+}
+
 static int __init uefi_init(void)
 {
      efi_char16_t *c16;
+     void *config_tables;
+     u64 table_size;
      char vendor[100] = "unknown";
      int i, retval;
 

@@ -99,7 +128,7 @@ static int __init uefi_init(void)
                      efi.systab->hdr.revision & 0xffff);
 
      /* Show what we know for posterity */
-     c16 = early_memremap(efi.systab->fw_vendor,
+     c16 = early_memremap(efi_to_phys(efi.systab->fw_vendor),
                           sizeof(vendor));
      if (c16) {
              for (i = 0; i < (int) sizeof(vendor) - 1 && *c16; ++i)

@@ -112,8 +141,14 @@ static int __init uefi_init(void)
              efi.systab->hdr.revision >> 16,
              efi.systab->hdr.revision & 0xffff, vendor);
 
-     retval = efi_config_init(NULL);
+     table_size = sizeof(efi_config_table_64_t) * efi.systab->nr_tables;
+     config_tables = early_memremap(efi_to_phys(efi.systab->tables),
+                                    table_size);
+
+     retval = efi_config_parse_tables(config_tables,
+                                      efi.systab->nr_tables, NULL);
 
+     early_memunmap(config_tables, table_size);
 out:
      early_memunmap(efi.systab,  sizeof(efi_system_table_t));
      return retval;

@@ -328,51 +363,9 @@ void __init efi_idmap_init(void)
      efi_setup_idmap();
 }
 
-static int __init remap_region(efi_memory_desc_t *md, void **new)
-{
-     u64 paddr, vaddr, npages, size;
-
-     paddr = md->phys_addr;
-     npages = md->num_pages;
-     memrange_efi_to_native(&paddr, &npages);
-     size = npages << PAGE_SHIFT;
-
-     if (is_normal_ram(md))
-             vaddr = (__force u64)ioremap_cache(paddr, size);
-     else
-             vaddr = (__force u64)ioremap(paddr, size);
-
-     if (!vaddr) {
-             pr_err("Unable to remap 0x%llx pages @ %p\n",
-                    npages, (void *)paddr);
-             return 0;
-     }
-
-     /* adjust for any rounding when EFI and system pagesize differs */
-     md->virt_addr = vaddr + (md->phys_addr - paddr);
-
-     if (uefi_debug)
-             pr_info("  EFI remap 0x%012llx => %p\n",
-                     md->phys_addr, (void *)md->virt_addr);
-
-     memcpy(*new, md, memmap.desc_size);
-     *new += memmap.desc_size;
-

arch/arm64/kernel/efi.c void __init efi_idmap_init(void) · line 366

Quoted 6 lines

arch/arm64/kernel/efi.c +0 -6

-     return 1;
-}
-
-/*
- * Switch UEFI from an identity map to a kernel virtual map
- */

No function description at all?

Arguably this function could change name now as well, since UEFI will already be in virtual mode. arm64_enable_runtime_map()?

Quoted 184 lines

arch/arm64/kernel/efi.c +81 -63

 static int __init arm64_enter_virtual_mode(void)
 {
-     efi_memory_desc_t *md;
-     phys_addr_t virtmap_phys;
-     void *virtmap, *virt_md;
-     efi_status_t status;
      u64 mapsize;
-     int count = 0;
-     unsigned long flags;
 
      if (!efi_enabled(EFI_BOOT)) {
              pr_info("EFI services will not be available.\n");

@@ -395,79 +388,28 @@ static int __init arm64_enter_virtual_mode(void)
 
      efi.memmap = &memmap;
 
-     /* Map the runtime regions */
-     virtmap = kmalloc(mapsize, GFP_KERNEL);
-     if (!virtmap) {
-             pr_err("Failed to allocate EFI virtual memmap\n");
-             return -1;
-     }
-     virtmap_phys = virt_to_phys(virtmap);
-     virt_md = virtmap;
-
-     for_each_efi_memory_desc(&memmap, md) {
-             if (!(md->attribute & EFI_MEMORY_RUNTIME))
-                     continue;
-             if (!remap_region(md, &virt_md))
-                     goto err_unmap;
-             ++count;
-     }
-
-     efi.systab = (__force void *)efi_lookup_mapped_addr(efi_system_table);
+     efi.systab = (__force void *)ioremap_cache(efi_system_table,
+                                                sizeof(efi_system_table_t));
      if (!efi.systab) {
-             /*
-              * If we have no virtual mapping for the System Table at this
-              * point, the memory map doesn't cover the physical offset where
-              * it resides. This means the System Table will be inaccessible
-              * to Runtime Services themselves once the virtual mapping is
-              * installed.
-              */
-             pr_err("Failed to remap EFI System Table -- buggy firmware?\n");
-             goto err_unmap;
+             pr_err("Failed to remap EFI System Table\n");
+             return -1;
      }
      set_bit(EFI_SYSTEM_TABLES, &efi.flags);
 
-     local_irq_save(flags);
-     cpu_switch_mm(idmap_pg_dir, &init_mm);
-
-     /* Call SetVirtualAddressMap with the physical address of the map */
-     runtime = efi.systab->runtime;
-     efi.set_virtual_address_map = runtime->set_virtual_address_map;
-
-     status = efi.set_virtual_address_map(count * memmap.desc_size,
-                                          memmap.desc_size,
-                                          memmap.desc_version,
-                                          (efi_memory_desc_t *)virtmap_phys);
-     cpu_set_reserved_ttbr0();
-     flush_tlb_all();
-     local_irq_restore(flags);
-
-     kfree(virtmap);
-
      free_boot_services();
 
-     if (status != EFI_SUCCESS) {
-             pr_err("Failed to set EFI virtual address map! [%lx]\n",
-                     status);
+     if (!efi_enabled(EFI_VIRTMAP)) {
+             pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
              return -1;
      }
 
      /* Set up runtime services function pointers */
-     runtime = efi.systab->runtime;
      efi_native_runtime_setup();
      set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
 
      efi.runtime_version = efi.systab->hdr.revision;
 
      return 0;
-
-err_unmap:
-     /* unmap all mappings that succeeded: there are 'count' of those */
-     for (virt_md = virtmap; count--; virt_md += memmap.desc_size) {
-             md = virt_md;
-             iounmap((__force void __iomem *)md->virt_addr);
-     }
-     kfree(virtmap);
-     return -1;
 }
 early_initcall(arm64_enter_virtual_mode);
 

@@ -484,3 +426,78 @@ static int __init arm64_dmi_init(void)
      return 0;
 }
 core_initcall(arm64_dmi_init);
+
+static pgd_t efi_pgd[PTRS_PER_PGD] __page_aligned_bss;
+
+static struct mm_struct efi_mm = {
+     .mm_rb                  = RB_ROOT,
+     .pgd                    = efi_pgd,
+     .mm_users               = ATOMIC_INIT(2),
+     .mm_count               = ATOMIC_INIT(1),
+     .mmap_sem               = __RWSEM_INITIALIZER(efi_mm.mmap_sem),
+     .page_table_lock        = __SPIN_LOCK_UNLOCKED(efi_mm.page_table_lock),
+     .mmlist                 = LIST_HEAD_INIT(efi_mm.mmlist),
+     INIT_MM_CONTEXT(efi_mm)
+};
+
+static void efi_set_pgd(struct mm_struct *mm)
+{
+     cpu_switch_mm(mm->pgd, mm);
+     flush_tlb_all();
+     if (icache_is_aivivt())
+             __flush_icache_all();
+}
+
+void efi_virtmap_load(void)
+{
+     WARN_ON(preemptible());
+     efi_set_pgd(&efi_mm);
+}
+
+void efi_virtmap_unload(void)
+{
+     efi_set_pgd(current->active_mm);
+}
+
+void __init efi_virtmap_init(void)
+{
+     efi_memory_desc_t *md;
+
+     if (!efi_enabled(EFI_BOOT))
+             return;
+
+     for_each_efi_memory_desc(&memmap, md) {
+             u64 paddr, npages, size;
+             pgprot_t prot;
+
+             if (!(md->attribute & EFI_MEMORY_RUNTIME))
+                     continue;
+             if (WARN(md->virt_addr == 0,
+                      "UEFI virtual mapping incomplete or missing -- no entry found for 0x%llx\n",
+                      md->phys_addr))
+                     return;
+
+             paddr = md->phys_addr;
+             npages = md->num_pages;
+             memrange_efi_to_native(&paddr, &npages);
+             size = npages << PAGE_SHIFT;
+
+             pr_info("  EFI remap 0x%012llx => %p\n",
+                     md->phys_addr, (void *)md->virt_addr);
+
+             /*
+              * Only regions of type EFI_RUNTIME_SERVICES_CODE need to be
+              * executable, everything else can be mapped with the XN bits
+              * set.
+              */
+             if (!is_normal_ram(md))
+                     prot = __pgprot(PROT_DEVICE_nGnRE);
+             else if (md->type == EFI_RUNTIME_SERVICES_CODE)
+                     prot = PAGE_KERNEL_EXEC;
+             else
+                     prot = PAGE_KERNEL;
+
+             create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, prot);
+     }
+     set_bit(EFI_VIRTMAP, &efi.flags);
+}

arch/arm64/kernel/setup.c +0 -0

index b80991166754..d8390f507da0 100644
@@ -402,6 +402,7 @@ void __init setup_arch(char **cmdline_p)
      request_standard_resources();
 

arch/arm64/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 405

Quoted 2 lines

arch/arm64/kernel/setup.c +1 -0

      efi_idmap_init();
+     efi_virtmap_init();

Could these two be merged together into one function? Say efi_memmap_init()?

Quoted 177 lines

arch/arm64/kernel/setup.c +0 -0

      unflatten_device_tree();
 

drivers/firmware/efi/libstub/fdt.c +133 -5

index c846a9608cbd..76bc8abf41d1 100644
@@ -167,6 +167,94 @@ fdt_set_fail:
 #define EFI_FDT_ALIGN EFI_PAGE_SIZE
 #endif
 
+static efi_status_t get_memory_map(efi_system_table_t *sys_table_arg,
+                                efi_memory_desc_t **map,
+                                unsigned long *map_size,
+                                unsigned long *desc_size,
+                                u32 *desc_ver, unsigned long *key_ptr)
+{
+     efi_status_t status;
+
+     /*
+      * Call get_memory_map() with 0 size to retrieve the size of the
+      * required allocation.
+      */
+     *map_size = 0;
+     status = efi_call_early(get_memory_map, map_size, NULL,
+                             key_ptr, desc_size, desc_ver);
+     if (status != EFI_BUFFER_TOO_SMALL)
+             return EFI_LOAD_ERROR;
+
+     /*
+      * Add an additional efi_memory_desc_t to map_size because we're doing
+      * an allocation which may be in a new descriptor region. Then double it
+      * to give us some scratch space to prepare the input virtmap to give
+      * to SetVirtualAddressMap(). Note that this is EFI_LOADER_DATA memory,
+      * and the kernel memblock_reserve()'s only the size of the actual
+      * memory map, so the scratch space is freed again automatically.
+      */
+     *map_size += *desc_size;
+     status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
+                             *map_size * 2, (void **)map);
+     if (status != EFI_SUCCESS)
+             return status;
+
+     status = efi_call_early(get_memory_map, map_size, *map,
+                             key_ptr, desc_size, desc_ver);
+     if (status != EFI_SUCCESS)
+             efi_call_early(free_pool, *map);
+     return status;
+}
+
+/*
+ * This is the base address at which to start allocating virtual memory ranges
+ * for UEFI Runtime Services. This is a userland range so that we can use any
+ * allocation we choose, and eliminate the risk of a conflict after kexec.
+ */
+#define EFI_RT_VIRTUAL_BASE  0x40000000
+
+static void update_memory_map(efi_memory_desc_t *memory_map,
+                           unsigned long map_size, unsigned long desc_size,
+                           int *count)
+{
+     u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
+     efi_memory_desc_t *out = (void *)memory_map + map_size;
+     int l;
+
+     for (l = 0; l < map_size; l += desc_size) {
+             efi_memory_desc_t *in = (void *)memory_map + l;
+             u64 paddr, size;
+
+             if (!(in->attribute & EFI_MEMORY_RUNTIME))
+                     continue;
+
+             /*
+              * Make the mapping compatible with 64k pages: this allows
+              * a 4k page size kernel to kexec a 64k page size kernel and
+              * vice versa.
+              */
+             paddr = round_down(in->phys_addr, SZ_64K);
+             size = round_up(in->num_pages * EFI_PAGE_SIZE +
+                             in->phys_addr - paddr, SZ_64K);
+
+             /*
+              * Avoid wasting memory on PTEs by choosing a virtual base that
+              * is compatible with section mappings if this region has the
+              * appropriate size and physical alignment. (Sections are 2 MB
+              * on 4k granule kernels)
+              */
+             if (IS_ALIGNED(in->phys_addr, SZ_2M) && size >= SZ_2M)
+                     efi_virt_base = round_up(efi_virt_base, SZ_2M);
+
+             in->virt_addr = efi_virt_base + in->phys_addr - paddr;
+             efi_virt_base += size;
+
+             memcpy(out, in, desc_size);
+             out = (void *)out + desc_size;
+             ++*count;
+     }
+}
+
 /*
  * Allocate memory for a new FDT, then add EFI, commandline, and
  * initrd related fields to the FDT.  This routine increases the

@@ -196,6 +284,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
      efi_memory_desc_t *memory_map;
      unsigned long new_fdt_size;
      efi_status_t status;
+     int runtime_entry_count = 0;
 
      /*
       * Estimate size of new FDT, and allocate memory for it. We

@@ -216,8 +305,8 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
               * we can get the memory map key  needed for
               * exit_boot_services().
               */
-             status = efi_get_memory_map(sys_table, &memory_map, &map_size,
-                                         &desc_size, &desc_ver, &mmap_key);
+             status = get_memory_map(sys_table, &memory_map, &map_size,
+                                     &desc_size, &desc_ver, &mmap_key);
              if (status != EFI_SUCCESS)
                      goto fail_free_new_fdt;
 

@@ -248,12 +337,52 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
              }
      }
 
+     /*
+      * Update the memory map with virtual addresses. The function will also
+      * populate the spare second half of the memory_map allocation with
+      * copies of just the EFI_MEMORY_RUNTIME entries so that we can pass it
+      * straight into SetVirtualAddressMap()
+      */
+     update_memory_map(memory_map, map_size, desc_size,
+                       &runtime_entry_count);
+
+     pr_efi(sys_table,
+            "Exiting boot services and installing virtual address map...\n");
+
      /* Now we are ready to exit_boot_services.*/
      status = sys_table->boottime->exit_boot_services(handle, mmap_key);
 
+     if (status == EFI_SUCCESS) {
+             efi_set_virtual_address_map_t *svam;
 
-     if (status == EFI_SUCCESS)
-             return status;
+             /* Install the new virtual address map */
+             svam = sys_table->runtime->set_virtual_address_map;
+             status = svam(runtime_entry_count * desc_size, desc_size,
+                           desc_ver, (void *)memory_map + map_size);
+
+             /*
+              * We are beyond the point of no return here, so if the call to
+              * SetVirtualAddressMap() failed, we need to signal that to the
+              * incoming kernel but proceed normally otherwise.
+              */
+             if (status != EFI_SUCCESS) {
+                     int l;
+
+                     /*
+                      * Set the virtual address field of all
+                      * EFI_MEMORY_RUNTIME entries to 0. This will signal
+                      * the incoming kernel that no virtual translation has
+                      * been installed.
+                      */
+                     for (l = 0; l < map_size; l += desc_size) {
+                             efi_memory_desc_t *p = (void *)memory_map + l;
+
+                             if (p->attribute & EFI_MEMORY_RUNTIME)
+                                     p->virt_addr = 0;
+                     }
+             }
+             return EFI_SUCCESS;
+     }
 
      pr_efi_err(sys_table, "Exit boot services failed.\n");
 
--

Quoted 1 line

1.8.3.2

Apart from this, and other comments in the thread, looks good.

/
Leif

Quoted 42 lines

Quoted 13 lines

arch/arm64/include/asm/efi.h +2 -0

index 71291253114f..6cc668a378c5 100644
@@ -7,28 +7,36 @@
 #ifdef CONFIG_EFI
 extern void efi_init(void);
 extern void efi_idmap_init(void);
+extern void efi_virtmap_init(void);
 #else
 #define efi_init()
 #define efi_idmap_init()
+#define efi_virtmap_init

arch/arm64/include/asm/efi.h line 22

Quoted 10 lines

arch/arm64/include/asm/efi.h +2 -2

 #endif
 
 #define efi_call_virt(f, ...)                                                \
 ({                                                                   \
-     efi_##f##_t *__f = efi.systab->runtime->f;                      \
+     efi_##f##_t *__f;                                               \
      efi_status_t __s;                                               \
                                                                      \
-     kernel_neon_begin();                                            \
+     kernel_neon_begin();    /* disables preemption */               \

Nitpick: adding comment to otherwise untouched source line.

Quoted 15 lines

arch/arm64/include/asm/efi.h +5 -2

+     efi_virtmap_load();                                             \
+     __f = efi.systab->runtime->f;                                   \
      __s = __f(__VA_ARGS__);                                         \
+     efi_virtmap_unload();                                           \
      kernel_neon_end();                                              \
      __s;                                                            \
 })
 
 #define __efi_call_virt(f, ...)                                              \
 ({                                                                   \
-     efi_##f##_t *__f = efi.systab->runtime->f;                      \
+     efi_##f##_t *__f;                                               \
                                                                      \
-     kernel_neon_begin();                                            \
+     kernel_neon_begin();    /* disables preemption */               \

Quoted 2 lines

Same nitpick.

Is there anything wrong with that?

Would you prefer the comment to be on a separate line?

Quoted 7 lines

Quoted 7 lines

arch/arm64/kernel/setup.c +0 -0

index b80991166754..d8390f507da0 100644
@@ -402,6 +402,7 @@ void __init setup_arch(char **cmdline_p)
      request_standard_resources();
 

Quoted 6 lines

arch/arm64/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 405

Quoted 2 lines

arch/arm64/kernel/setup.c +1 -0

      efi_idmap_init();
+     efi_virtmap_init();

Could these two be merged together into one function? Say efi_memmap_init()?

Well, I decided to do it like this because efi_idmap_init() gets removed in its entirety (including this invocation) in a subsequent patch.

Quoted 42 lines · Ard Biesheuvel

Quoted 42 lines

Quoted 13 lines

arch/arm64/include/asm/efi.h +2 -0

index 71291253114f..6cc668a378c5 100644
@@ -7,28 +7,36 @@
 #ifdef CONFIG_EFI
 extern void efi_init(void);
 extern void efi_idmap_init(void);
+extern void efi_virtmap_init(void);
 #else
 #define efi_init()
 #define efi_idmap_init()
+#define efi_virtmap_init

arch/arm64/include/asm/efi.h line 22

Quoted 10 lines

arch/arm64/include/asm/efi.h +2 -2

 #endif
 
 #define efi_call_virt(f, ...)                                                \
 ({                                                                   \
-     efi_##f##_t *__f = efi.systab->runtime->f;                      \
+     efi_##f##_t *__f;                                               \
      efi_status_t __s;                                               \
                                                                      \
-     kernel_neon_begin();                                            \
+     kernel_neon_begin();    /* disables preemption */               \

Nitpick: adding comment to otherwise untouched source line.

Quoted 15 lines

arch/arm64/include/asm/efi.h +5 -2

+     efi_virtmap_load();                                             \
+     __f = efi.systab->runtime->f;                                   \
      __s = __f(__VA_ARGS__);                                         \
+     efi_virtmap_unload();                                           \
      kernel_neon_end();                                              \
      __s;                                                            \
 })
 
 #define __efi_call_virt(f, ...)                                              \
 ({                                                                   \
-     efi_##f##_t *__f = efi.systab->runtime->f;                      \
+     efi_##f##_t *__f;                                               \
                                                                      \
-     kernel_neon_begin();                                            \
+     kernel_neon_begin();    /* disables preemption */               \

Quoted 4 lines

Quoted 2 lines

Same nitpick.

Is there anything wrong with that?

I said nitpick.

My (very minor) objection is that a (very reasonable) comment is added to existing functionality by a patch that adds new functionality. It makes the git blame/praise output less clear.

Quoted 1 line

Would you prefer the comment to be on a separate line?

I would _prefer_ the comments to be a separate patch. But again, a nitpick.

Quoted 14 lines

Quoted 7 lines

Quoted 7 lines

arch/arm64/kernel/setup.c +0 -0

index b80991166754..d8390f507da0 100644
@@ -402,6 +402,7 @@ void __init setup_arch(char **cmdline_p)
      request_standard_resources();
 

Quoted 6 lines

arch/arm64/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 405

Quoted 2 lines

arch/arm64/kernel/setup.c +1 -0

      efi_idmap_init();
+     efi_virtmap_init();

Could these two be merged together into one function? Say efi_memmap_init()?

Quoted 3 lines

Well, I decided to do it like this because efi_idmap_init() gets removed in its entirety (including this invocation) in a subsequent patch.

Ah yeah. I knew that yesterday. Ignore me. More coffee.

/
Leif

Quoted 15 lines

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

+ kernel_neon_begin(); /* disables preemption */ \

Same nitpick.

Is there anything wrong with that?

I said nitpick.

My (very minor) objection is that a (very reasonable) comment is added to existing functionality by a patch that adds new functionality. It makes the git blame/praise output less clear.

Quoted 1 line

Would you prefer the comment to be on a separate line?

Quoted 3 lines

I would _prefer_ the comments to be a separate patch. But again, a nitpick.

Well, adding the comment is relevant to this patch, as we need to disable preemption now before switching to the new address space. In fact, it might be better even to drop the comment, and add an explicit (if redundant) preempt_disable/preempt_enable pair.

Now that we are calling SetVirtualAddressMap() from the stub, there is no need to reserve boot-only memory regions, which implies that there is also no reason to free them again later.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/efi.c | 123 +-----------------------------------------------
 1 file changed, 1 insertion(+), 122 deletions(-)

arch/arm64/kernel/efi.c +1 -122

index 2ebe67ffb629..e969c5076713 100644
@@ -198,9 +198,7 @@ static __init void reserve_regions(void)
 		if (is_normal_ram(md))
 			early_init_dt_add_memory_arch(paddr, size);
 
-		if (is_reserve_region(md) ||
-		    md->type == EFI_BOOT_SERVICES_CODE ||
-		    md->type == EFI_BOOT_SERVICES_DATA) {
+		if (is_reserve_region(md)) {
 			memblock_reserve(paddr, size);
 			if (uefi_debug)
 				pr_cont("*");

@@ -213,123 +211,6 @@ static __init void reserve_regions(void)
 	set_bit(EFI_MEMMAP, &efi.flags);
 }
 
-
-static u64 __init free_one_region(u64 start, u64 end)
-{
-	u64 size = end - start;
-
-	if (uefi_debug)
-		pr_info("  EFI freeing: 0x%012llx-0x%012llx\n",	start, end - 1);
-
-	free_bootmem_late(start, size);
-	return size;
-}
-
-static u64 __init free_region(u64 start, u64 end)
-{
-	u64 map_start, map_end, total = 0;
-
-	if (end <= start)
-		return total;
-
-	map_start = (u64)memmap.phys_map;
-	map_end = PAGE_ALIGN(map_start + (memmap.map_end - memmap.map));
-	map_start &= PAGE_MASK;
-
-	if (start < map_end && end > map_start) {
-		/* region overlaps UEFI memmap */
-		if (start < map_start)
-			total += free_one_region(start, map_start);
-
-		if (map_end < end)
-			total += free_one_region(map_end, end);
-	} else
-		total += free_one_region(start, end);
-
-	return total;
-}
-
-static void __init free_boot_services(void)
-{
-	u64 total_freed = 0;
-	u64 keep_end, free_start, free_end;
-	efi_memory_desc_t *md;
-
-	/*
-	 * If kernel uses larger pages than UEFI, we have to be careful
-	 * not to inadvertantly free memory we want to keep if there is
-	 * overlap at the kernel page size alignment. We do not want to
-	 * free is_reserve_region() memory nor the UEFI memmap itself.
-	 *
-	 * The memory map is sorted, so we keep track of the end of
-	 * any previous region we want to keep, remember any region
-	 * we want to free and defer freeing it until we encounter
-	 * the next region we want to keep. This way, before freeing
-	 * it, we can clip it as needed to avoid freeing memory we
-	 * want to keep for UEFI.
-	 */
-
-	keep_end = 0;
-	free_start = 0;
-
-	for_each_efi_memory_desc(&memmap, md) {
-		u64 paddr, npages, size;
-
-		if (is_reserve_region(md)) {
-			/*
-			 * We don't want to free any memory from this region.
-			 */
-			if (free_start) {
-				/* adjust free_end then free region */
-				if (free_end > md->phys_addr)
-					free_end -= PAGE_SIZE;
-				total_freed += free_region(free_start, free_end);
-				free_start = 0;
-			}
-			keep_end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT);
-			continue;
-		}
-
-		if (md->type != EFI_BOOT_SERVICES_CODE &&
-		    md->type != EFI_BOOT_SERVICES_DATA) {
-			/* no need to free this region */
-			continue;
-		}
-
-		/*
-		 * We want to free memory from this region.
-		 */
-		paddr = md->phys_addr;
-		npages = md->num_pages;
-		memrange_efi_to_native(&paddr, &npages);
-		size = npages << PAGE_SHIFT;
-
-		if (free_start) {
-			if (paddr <= free_end)
-				free_end = paddr + size;
-			else {
-				total_freed += free_region(free_start, free_end);
-				free_start = paddr;
-				free_end = paddr + size;
-			}
-		} else {
-			free_start = paddr;
-			free_end = paddr + size;
-		}
-		if (free_start < keep_end) {
-			free_start += PAGE_SIZE;
-			if (free_start >= free_end)
-				free_start = 0;
-		}
-	}
-	if (free_start)
-		total_freed += free_region(free_start, free_end);
-
-	if (total_freed)
-		pr_info("Freed 0x%llx bytes of EFI boot services memory",
-			total_freed);
-}
-
 void __init efi_init(void)
 {
 	struct efi_fdt_params params;

@@ -396,8 +277,6 @@ static int __init arm64_enter_virtual_mode(void)
 	}
 	set_bit(EFI_SYSTEM_TABLES, &efi.flags);
 
-	free_boot_services();
-
 	if (!efi_enabled(EFI_VIRTMAP)) {
 		pr_err("No UEFI virtual mapping was installed -- runtime services will not be available\n");
 		return -1;

Signature · 1 line

1.8.3.2

Now that we have moved the call to SetVirtualAddressMap() to the stub, UEFI has no use for the ID map, so we can drop the code that installs ID mappings for UEFI memory regions.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/efi.h |  2 --
 arch/arm64/include/asm/mmu.h |  2 --
 arch/arm64/kernel/efi.c      | 30 ------------------------------
 arch/arm64/kernel/setup.c    |  1 -
 arch/arm64/mm/mmu.c          | 12 ------------
 5 files changed, 47 deletions(-)

arch/arm64/include/asm/efi.h +0 -2

index 6cc668a378c5..84c50853b5dc 100644
@@ -6,11 +6,9 @@
 
 #ifdef CONFIG_EFI
 extern void efi_init(void);
-extern void efi_idmap_init(void);
 extern void efi_virtmap_init(void);
 #else
 #define efi_init()
-#define efi_idmap_init()
 #define efi_virtmap_init
 #endif
 

arch/arm64/include/asm/mmu.h +0 -2

index 5fd40c43be80..3d311761e3c2 100644
@@ -31,8 +31,6 @@ extern void paging_init(void);
 extern void setup_mm_for_reboot(void);
 extern void __iomem *early_io_map(phys_addr_t phys, unsigned long virt);
 extern void init_mem_pgprot(void);
-/* create an identity mapping for memory (or io if map_io is true) */
-extern void create_id_mapping(phys_addr_t addr, phys_addr_t size, int map_io);
 extern void create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
 			       unsigned long virt, phys_addr_t size,
 			       pgprot_t prot);

arch/arm64/kernel/efi.c +0 -30

index e969c5076713..d7d2e818c856 100644
@@ -53,27 +53,6 @@ static int __init is_normal_ram(efi_memory_desc_t *md)
 	return 0;
 }
 
-static void __init efi_setup_idmap(void)
-{
-	struct memblock_region *r;
-	efi_memory_desc_t *md;
-	u64 paddr, npages, size;
-
-	for_each_memblock(memory, r)
-		create_id_mapping(r->base, r->size, 0);
-
-	/* map runtime io spaces */
-	for_each_efi_memory_desc(&memmap, md) {
-		if (!(md->attribute & EFI_MEMORY_RUNTIME) || is_normal_ram(md))
-			continue;
-		paddr = md->phys_addr;
-		npages = md->num_pages;
-		memrange_efi_to_native(&paddr, &npages);
-		size = npages << PAGE_SHIFT;
-		create_id_mapping(paddr, size, 1);
-	}
-}
-
 /*
  * Translate a EFI virtual address into a physical address: this is necessary,
  * as some data members of the EFI system table are virtually remapped after

@@ -235,15 +214,6 @@ void __init efi_init(void)
 	reserve_regions();
 }
 
-void __init efi_idmap_init(void)
-{
-	if (!efi_enabled(EFI_BOOT))
-		return;
-
-	/* boot time idmap_pg_dir is incomplete, so fill in missing parts */
-	efi_setup_idmap();
-}
-
 static int __init arm64_enter_virtual_mode(void)
 {
 	u64 mapsize;

arch/arm64/kernel/setup.c +0 -1

index d8390f507da0..0771de962f37 100644
@@ -401,7 +401,6 @@ void __init setup_arch(char **cmdline_p)
 	paging_init();
 	request_standard_resources();
 
-	efi_idmap_init();
 	efi_virtmap_init();
 
 	unflatten_device_tree();

arch/arm64/mm/mmu.c +0 -12

index 3f3d5aa4a8b1..328638548871 100644
@@ -271,18 +271,6 @@ static void __init create_mapping(phys_addr_t phys, unsigned long virt,
 			 size, PAGE_KERNEL_EXEC);
 }
 
-void __init create_id_mapping(phys_addr_t addr, phys_addr_t size, int map_io)
-{
-	if ((addr >> PGDIR_SHIFT) >= ARRAY_SIZE(idmap_pg_dir)) {
-		pr_warn("BUG: not creating id mapping for %pa\n", &addr);
-		return;
-	}
-	__create_mapping(&init_mm, &idmap_pg_dir[pgd_index(addr)],
-			 addr, addr, size,
-			 map_io ? __pgprot(PROT_DEVICE_nGnRE)
-				: PAGE_KERNEL_EXEC);
-}
-
 void __init create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
 			       unsigned long virt, phys_addr_t size,
 			       pgprot_t prot)

Signature · 1 line

1.8.3.2