arm64 EFI patches for 3.19

After the EFI stub has done its business, it jumps into the kernel by branching to offset #0 of the loaded Image, which is where it expects to find the header containing a 'branch to stext' instruction.

However, the UEFI spec 2.1.1 states the following regarding PE/COFF image loading:
"A UEFI image is loaded into memory through the LoadImage() Boot Service. This service loads an image with a PE32+ format into memory. This PE32+ loader is required to load all sections of the PE32+ image into memory."

In other words, it is /not/ required to load parts of the image that are not covered by a PE/COFF section, so it may not have loaded the header at the expected offset, as it is not covered by any PE/COFF section.

So instead, jump to 'stext' directly, which is at the base of the PE/COFF .text section, by supplying a symbol 'stext_offset' to efi-entry.o which contains the relative offset of stext into the Image. Also replace other open coded calculations of the same value with a reference to 'stext_offset'

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

---
v3:
- rebased onto 3.17+
- added spec reference to commit message

v2:
- drop :lo12: relocation against stext_offset in favor of using a literal '=stext_offset' which is safer

---
 arch/arm64/kernel/efi-entry.S |  3 ++-
 arch/arm64/kernel/head.S      | 10 ++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

arch/arm64/kernel/efi-entry.S +2 -1

index 619b1dd7bcde..a0016d3a17da 100644
@@ -61,7 +61,8 @@ ENTRY(efi_stub_entry)
 	 */
 	mov	x20, x0		// DTB address
 	ldr	x0, [sp, #16]	// relocated _text address
-	mov	x21, x0
+	ldr	x21, =stext_offset
+	add	x21, x0, x21
 
 	/*
 	 * Flush dcache covering current runtime addresses

arch/arm64/kernel/head.S +6 -4

index 0a6e4f924df8..8c06c9d269d2 100644
@@ -132,6 +132,8 @@ efi_head:
 #endif
 
 #ifdef CONFIG_EFI
+	.globl	stext_offset
+	.set	stext_offset, stext - efi_head
 	.align 3
 pe_header:
 	.ascii	"PE"

@@ -155,7 +157,7 @@ optional_header:
 	.long	0				// SizeOfInitializedData
 	.long	0				// SizeOfUninitializedData
 	.long	efi_stub_entry - efi_head	// AddressOfEntryPoint
-	.long	stext - efi_head		// BaseOfCode
+	.long	stext_offset			// BaseOfCode
 
 extra_header_fields:
 	.quad	0				// ImageBase

@@ -172,7 +174,7 @@ extra_header_fields:
 	.long	_end - efi_head			// SizeOfImage
 
 	// Everything before the kernel image is considered part of the header
-	.long	stext - efi_head		// SizeOfHeaders
+	.long	stext_offset			// SizeOfHeaders
 	.long	0				// CheckSum
 	.short	0xa				// Subsystem (EFI application)
 	.short	0				// DllCharacteristics

@@ -217,9 +219,9 @@ section_table:
 	.byte	0
 	.byte	0        		// end of 0 padding of section name
 	.long	_end - stext		// VirtualSize
-	.long	stext - efi_head	// VirtualAddress
+	.long	stext_offset		// VirtualAddress
 	.long	_edata - stext		// SizeOfRawData
-	.long	stext - efi_head	// PointerToRawData
+	.long	stext_offset		// PointerToRawData
 
 	.long	0		// PointerToRelocations (0 for executables)
 	.long	0		// PointerToLineNumbers (0 for executables)

Signature · 1 line

1.8.3.2

Position independent AArch64 code needs to be linked and loaded at the same relative offset from a 4 KB boundary, or adrp/add and adrp/ldr pairs will not work correctly. (This is how PC relative symbol references with a 4 GB reach are emitted)

We need to declare this in the PE/COFF header, otherwise the PE/COFF loader may load the Image and invoke the stub at an offset which violates this rule.

Reviewed-by Roy Franz <[email protected]>

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

---
v2: added comment explaining '.align 12' in head.S

---
 arch/arm64/kernel/head.S | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

arch/arm64/kernel/head.S +10 -2

index 8c06c9d269d2..8ae84d8c2a8c 100644
@@ -161,7 +161,7 @@ optional_header:
 
 extra_header_fields:
 	.quad	0				// ImageBase
-	.long	0x20				// SectionAlignment
+	.long	0x1000				// SectionAlignment
 	.long	0x8				// FileAlignment
 	.short	0				// MajorOperatingSystemVersion
 	.short	0				// MinorOperatingSystemVersion

@@ -228,7 +228,15 @@ section_table:
 	.short	0		// NumberOfRelocations  (0 for executables)
 	.short	0		// NumberOfLineNumbers  (0 for executables)
 	.long	0xe0500020	// Characteristics (section flags)
-	.align 5
+
+	/*
+	 * EFI will load stext onwards at the 4k section alignment
+	 * described in the PE/COFF header. To ensure that instruction
+	 * sequences using an adrp and a :lo12: immediate will function
+	 * correctly at this alignment, we must ensure that stext is
+	 * placed at a 4k boundary in the Image to begin with.
+	 */
+	.align 12
 #endif
 
 ENTRY(stext)

Signature · 1 line

1.8.3.2

Memory regions of type ACPI_MEMORY_NVS should be preserved by the OS, so make sure we reserve them at boot.

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

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

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

index 95c49ebc660d..71ea4fc0aa8a 100644

Quoted 15 lines · Ard Biesheuvel

Memory regions of type ACPI_MEMORY_NVS should be preserved by the OS, so make sure we reserve them at boot.

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

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

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

index 95c49ebc660d..71ea4fc0aa8a 100644
@@ -132,6 +132,7 @@ static __init int is_reserve_region(efi_memory_desc_t *md)
              return 1;
 

arch/arm64/kernel/efi.c static __init int is_reserve_region(efi_memory_desc_t *md) · line 137

Quoted 4 lines

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

      if (md->type == EFI_ACPI_RECLAIM_MEMORY ||
+         md->type == EFI_ACPI_MEMORY_NVS ||
          md->type == EFI_RESERVED_TYPE)
              return 1;

Shouldn't we also filter out EFI_UNUSABLE_MEMORY? Or does that happen elsewhere?

Perhaps instead we should invert this logic and assume memory should be reserved if not EfiLoaderCode, EfiLoaderData, EfiBootServicesCode, EfiBootServicesData, or EfiConventionalMemory. That looks to be what x86 does.

Quoted 16 lines · Mark Rutland

Quoted 15 lines · Ard Biesheuvel

Memory regions of type ACPI_MEMORY_NVS should be preserved by the OS, so make sure we reserve them at boot.

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

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

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

index 95c49ebc660d..71ea4fc0aa8a 100644
@@ -132,6 +132,7 @@ static __init int is_reserve_region(efi_memory_desc_t *md)
              return 1;
 

Quoted 8 lines

arch/arm64/kernel/efi.c static __init int is_reserve_region(efi_memory_desc_t *md) · line 137

Quoted 4 lines

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

      if (md->type == EFI_ACPI_RECLAIM_MEMORY ||
+         md->type == EFI_ACPI_MEMORY_NVS ||
          md->type == EFI_RESERVED_TYPE)
              return 1;

Shouldn't we also filter out EFI_UNUSABLE_MEMORY? Or does that happen elsewhere?

Yes, good point.

Quoted 5 lines

Perhaps instead we should invert this logic and assume memory should be reserved if not EfiLoaderCode, EfiLoaderData, EfiBootServicesCode, EfiBootServicesData, or EfiConventionalMemory. That looks to be what x86 does.

That would make it more robust against new types in future spec changes, I suppose, although that would seem unlikely.

I am happy to change the patch to take that approach instead, if others agree that it is preferable?

The EFI_CONFIG_TABLES bit already gets set by efi_config_init(), so there is no reason to set it again after this function returns successfully.

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

---
 arch/arm64/kernel/efi.c | 2 --
 1 file changed, 2 deletions(-)

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

index 71ea4fc0aa8a..51522ab0c6da 100644
@@ -112,8 +112,6 @@ static int __init uefi_init(void)
 		efi.systab->hdr.revision & 0xffff, vendor);
 
 	retval = efi_config_init(NULL);
-	if (retval == 0)
-		set_bit(EFI_CONFIG_TABLES, &efi.flags);
 
 out:
 	early_memunmap(efi.systab,  sizeof(efi_system_table_t));

Signature · 1 line

1.8.3.2

On systems that boot via UEFI, all memory nodes are deleted from the device tree, and instead, the size and location of system RAM is derived from the UEFI memory map. This is handled by reserve_regions, which not only reserves parts of memory that UEFI declares as reserved, but also installs the memblocks that cover the remaining usable memory.

Currently, reserve_regions() is only called if uefi_init() succeeds. However, it does not actually depend on anything that uefi_init() does, and not calling reserve_regions() results in a broken boot, so it is better to just call it unconditionally.

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

---
 arch/arm64/kernel/efi.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

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

index 51522ab0c6da..4cec21b1ecdd 100644

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

@@ -374,15 +373,13 @@ static int __init arm64_enter_virtual_mode(void)
 	int count = 0;
 	unsigned long flags;
 
-	if (!efi_enabled(EFI_BOOT)) {
-		pr_info("EFI services will not be available.\n");
-		return -1;
-	}
+	if (!efi_enabled(EFI_MEMMAP))
+		return 0;
 
 	mapsize = memmap.map_end - memmap.map;
 	early_memunmap(memmap.map, mapsize);
 
-	if (efi_runtime_disabled()) {
+	if (!efi_enabled(EFI_BOOT) || efi_runtime_disabled()) {
 		pr_info("EFI runtime services will be disabled.\n");
 		return -1;
 	}

Quoted 28 lines · Ard Biesheuvel

On systems that boot via UEFI, all memory nodes are deleted from the device tree, and instead, the size and location of system RAM is derived from the UEFI memory map. This is handled by reserve_regions, which not only reserves parts of memory that UEFI declares as reserved, but also installs the memblocks that cover the remaining usable memory.

Currently, reserve_regions() is only called if uefi_init() succeeds. However, it does not actually depend on anything that uefi_init() does, and not calling reserve_regions() results in a broken boot, so it is better to just call it unconditionally.

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

---
 arch/arm64/kernel/efi.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

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

index 51522ab0c6da..4cec21b1ecdd 100644
@@ -313,8 +313,7 @@ void __init efi_init(void)
      memmap.desc_size = params.desc_size;
      memmap.desc_version = params.desc_ver;
 
-     if (uefi_init() < 0)
-             return;
+     WARN_ON(uefi_init() < 0);
 

arch/arm64/kernel/efi.c void __init efi_init(void) · line 319

Quoted 2 lines

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

      reserve_regions();
 }

It also looks like EFI_BOOT flag will be set even if uefi_init fails. If uefi_init fails, we only need reserve_regions() for the purpose of adding memblocks. Otherwise, we end up wasting a lot of memory.

So, something like this would also be needed:

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

index 03aaa99..5a6e189 100644
@@ -81,9 +81,6 @@ static int __init uefi_init(void)
                return -ENOMEM;
        }
 
-       set_bit(EFI_BOOT, &efi.flags);
-       set_bit(EFI_64BIT, &efi.flags);
-
        /*
         * Verify the EFI Table
         */

@@ -109,6 +106,9 @@ static int __init uefi_init(void)
                efi.systab->hdr.revision >> 16,
                efi.systab->hdr.revision & 0xffff, vendor);
 
+       set_bit(EFI_BOOT, &efi.flags);
+       set_bit(EFI_64BIT, &efi.flags);
+
        retval = efi_config_init(NULL);
        if (retval == 0)
                set_bit(EFI_CONFIG_TABLES, &efi.flags);

@@ -177,9 +177,10 @@ 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 (efi_enabled(EFI_BOOT) &&
+                   (is_reserve_region(md) ||
+                    md->type == EFI_BOOT_SERVICES_CODE ||
+                    md->type == EFI_BOOT_SERVICES_DATA)) {
                        memblock_reserve(paddr, size);
                        if (uefi_debug)
                                pr_cont("*");

Quoted 20 lines

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

@@ -374,15 +373,13 @@ static int __init arm64_enter_virtual_mode(void)
      int count = 0;
      unsigned long flags;
 
-     if (!efi_enabled(EFI_BOOT)) {
-             pr_info("EFI services will not be available.\n");
-             return -1;
-     }
+     if (!efi_enabled(EFI_MEMMAP))
+             return 0;
 
      mapsize = memmap.map_end - memmap.map;
      early_memunmap(memmap.map, mapsize);
 
-     if (efi_runtime_disabled()) {
+     if (!efi_enabled(EFI_BOOT) || efi_runtime_disabled()) {
              pr_info("EFI runtime services will be disabled.\n");
              return -1;
      }

Quoted 29 lines · Mark Salter

Quoted 28 lines · Ard Biesheuvel

On systems that boot via UEFI, all memory nodes are deleted from the device tree, and instead, the size and location of system RAM is derived from the UEFI memory map. This is handled by reserve_regions, which not only reserves parts of memory that UEFI declares as reserved, but also installs the memblocks that cover the remaining usable memory.

Currently, reserve_regions() is only called if uefi_init() succeeds. However, it does not actually depend on anything that uefi_init() does, and not calling reserve_regions() results in a broken boot, so it is better to just call it unconditionally.

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

---
 arch/arm64/kernel/efi.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

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

index 51522ab0c6da..4cec21b1ecdd 100644
@@ -313,8 +313,7 @@ void __init efi_init(void)
      memmap.desc_size = params.desc_size;
      memmap.desc_version = params.desc_ver;
 
-     if (uefi_init() < 0)
-             return;
+     WARN_ON(uefi_init() < 0);
 

Quoted 6 lines

arch/arm64/kernel/efi.c void __init efi_init(void) · line 319

Quoted 2 lines

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

      reserve_regions();
 }

It also looks like EFI_BOOT flag will be set even if uefi_init fails. If uefi_init fails, we only need reserve_regions() for the purpose of adding memblocks. Otherwise, we end up wasting a lot of memory.

Indeed. But perhaps it would be cleaner to add the memblocks in a separate function that gets called before uefi_init(), let reserve_regions() do just what it name implies, and retain the above hunk so that the reserve_regions() call is only performed if UEFI initialized correctly.

Quoted 28 lines · Ard Biesheuvel

On systems that boot via UEFI, all memory nodes are deleted from the device tree, and instead, the size and location of system RAM is derived from the UEFI memory map. This is handled by reserve_regions, which not only reserves parts of memory that UEFI declares as reserved, but also installs the memblocks that cover the remaining usable memory.

Currently, reserve_regions() is only called if uefi_init() succeeds. However, it does not actually depend on anything that uefi_init() does, and not calling reserve_regions() results in a broken boot, so it is better to just call it unconditionally.

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

---
 arch/arm64/kernel/efi.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

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

index 51522ab0c6da..4cec21b1ecdd 100644
@@ -313,8 +313,7 @@ void __init efi_init(void)
 	memmap.desc_size = params.desc_size;
 	memmap.desc_version = params.desc_ver;
 
-	if (uefi_init() < 0)
-		return;
+	WARN_ON(uefi_init() < 0);
 

arch/arm64/kernel/efi.c void __init efi_init(void) · line 319

Quoted 2 lines

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

 	reserve_regions();
 }

It also looks like EFI_BOOT flag will be set even if uefi_init fails. If uefi_init fails, we only need reserve_regions() for the purpose of adding memblocks. Otherwise, we end up wasting a lot of memory.

Quoted 29 lines · Mark Salter

Quoted 28 lines · Ard Biesheuvel

On systems that boot via UEFI, all memory nodes are deleted from the device tree, and instead, the size and location of system RAM is derived from the UEFI memory map. This is handled by reserve_regions, which not only reserves parts of memory that UEFI declares as reserved, but also installs the memblocks that cover the remaining usable memory.

Currently, reserve_regions() is only called if uefi_init() succeeds. However, it does not actually depend on anything that uefi_init() does, and not calling reserve_regions() results in a broken boot, so it is better to just call it unconditionally.

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

---
 arch/arm64/kernel/efi.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

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

index 51522ab0c6da..4cec21b1ecdd 100644
@@ -313,8 +313,7 @@ void __init efi_init(void)
 	memmap.desc_size = params.desc_size;
 	memmap.desc_version = params.desc_ver;
 
-	if (uefi_init() < 0)
-		return;
+	WARN_ON(uefi_init() < 0);
 

Quoted 6 lines

arch/arm64/kernel/efi.c void __init efi_init(void) · line 319

Quoted 2 lines

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

 	reserve_regions();
 }

It also looks like EFI_BOOT flag will be set even if uefi_init fails. If uefi_init fails, we only need reserve_regions() for the purpose of adding memblocks. Otherwise, we end up wasting a lot of memory.

What memory are we wasting in that case? Surely the only items that we could choose to throw away if we failed uefi_init are EFI_ACPI_RECLAIM_MEMORY and EFI_MEMORY_RUNTIME?

We might want to keep those around so we can kexec into a kernel where we can make use of them. Surely they shouldn't take up a significant proportion of the available memory?

Quoted 37 lines · Mark Rutland

Quoted 29 lines · Mark Salter

Quoted 28 lines · Ard Biesheuvel

On systems that boot via UEFI, all memory nodes are deleted from the device tree, and instead, the size and location of system RAM is derived from the UEFI memory map. This is handled by reserve_regions, which not only reserves parts of memory that UEFI declares as reserved, but also installs the memblocks that cover the remaining usable memory.

Currently, reserve_regions() is only called if uefi_init() succeeds. However, it does not actually depend on anything that uefi_init() does, and not calling reserve_regions() results in a broken boot, so it is better to just call it unconditionally.

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

---
 arch/arm64/kernel/efi.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

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

index 51522ab0c6da..4cec21b1ecdd 100644
@@ -313,8 +313,7 @@ void __init efi_init(void)
  memmap.desc_size = params.desc_size;
  memmap.desc_version = params.desc_ver;
 
- if (uefi_init() < 0)
-         return;
+ WARN_ON(uefi_init() < 0);
 

Quoted 6 lines

arch/arm64/kernel/efi.c void __init efi_init(void) · line 319

Quoted 2 lines

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

  reserve_regions();
 }

It also looks like EFI_BOOT flag will be set even if uefi_init fails. If uefi_init fails, we only need reserve_regions() for the purpose of adding memblocks. Otherwise, we end up wasting a lot of memory.

Quoted 7 lines

What memory are we wasting in that case? Surely the only items that we could choose to throw away if we failed uefi_init are EFI_ACPI_RECLAIM_MEMORY and EFI_MEMORY_RUNTIME?

We might want to keep those around so we can kexec into a kernel where we can make use of them. Surely they shouldn't take up a significant proportion of the available memory?

In addition, reserve_regions() also reserves BOOT_SERVICES (which gets freed up later after set_virtual_address_map(). That won't happen if uefi_init() fails. In any case, if uefi_init() fails, we won't be able to find the ACPI tables kexec kernel won't be able to either. The BOOT_SERVICES stuff is the big chunk. There was some discussion of not reserving that but no one has gotten around to writing the patch to clean out the code that does it.

Quoted 8 lines

Quoted 3 lines

It also looks like EFI_BOOT flag will be set even if uefi_init fails. If uefi_init fails, we only need reserve_regions() for the purpose of adding memblocks. Otherwise, we end up wasting a lot of memory.

What memory are we wasting in that case? Surely the only items that we could choose to throw away if we failed uefi_init are EFI_ACPI_RECLAIM_MEMORY and EFI_MEMORY_RUNTIME?

Quoted 3 lines

We might want to keep those around so we can kexec into a kernel where we can make use of them. Surely they shouldn't take up a significant proportion of the available memory?

In addition, reserve_regions() also reserves BOOT_SERVICES (which gets freed up later after set_virtual_address_map(). That won't happen if uefi_init() fails. In any case, if uefi_init() fails, we won't be able to find the ACPI tables kexec kernel won't be able to either.

The BOOT_SERVICES stuff is the big chunk. There was some discussion of not reserving that but no one has gotten around to writing the patch to clean out the code that does it.

From 581d5bac5b4a6f93e23737012b71c58d809af6bb Mon Sep 17 00:00:00 2001

[...]

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

It also looks like EFI_BOOT flag will be set even if uefi_init fails. If uefi_init fails, we only need reserve_regions() for the purpose of adding memblocks. Otherwise, we end up wasting a lot of memory.

What memory are we wasting in that case? Surely the only items that we could choose to throw away if we failed uefi_init are EFI_ACPI_RECLAIM_MEMORY and EFI_MEMORY_RUNTIME?

Quoted 8 lines

Quoted 3 lines

We might want to keep those around so we can kexec into a kernel where we can make use of them. Surely they shouldn't take up a significant proportion of the available memory?

In addition, reserve_regions() also reserves BOOT_SERVICES (which gets freed up later after set_virtual_address_map(). That won't happen if uefi_init() fails. In any case, if uefi_init() fails, we won't be able to find the ACPI tables kexec kernel won't be able to either.

If you need the ACPI tables, the first kernel won't boot. However, if we fail to map the runtime services data and code, that doesn't mean the next kernel can't. We might have failed to map the runtime services due to an early_ioremap bug or similar, and that could be fixed in the kernel we're about to kexec to.

If we're going to nuke runtime regions, we should nuke them in the memory map descriptors as a courtesy to the next kernel. Otherwise we could be more courteous and simply leave them be (which is my preference).

Quoted 3 lines

The BOOT_SERVICES stuff is the big chunk. There was some discussion of not reserving that but no one has gotten around to writing the patch to clean out the code that does it.

I've just spent some time doing so; patch below. It boots happily on my Juno (with 4KiB pages at least), but I haven't given it much of a stress test.

I wasn't sure if unusable memory could show up with EFI_MEMORY_WB attributes. If so, this patch still won't prevent that from being mapped as cacheable, but that should be easy to fix.

Thanks,
Mark.

---->8----
From 581d5bac5b4a6f93e23737012b71c58d809af6bb Mon Sep 17 00:00:00 2001
From: Mark Rutland <[email protected]>
Date: Thu, 23 Oct 2014 19:41:55 +0100
Subject: [PATCH] arm64: efi: Simplify memory descriptor handling

Currently discovering the memory map from UEFI is a multi-stage affair, where the boot services code and data are kept around for much longer than necessary. Additionally, potential overlap between memory and IO mappings at kernel page granularity is not handled.

This patch attempts to solve both of these issues, with the addition and filtering of memory regions being handled in a single function.

First, all normal memory (i.e. anything which can be mapped writeback cacheable) is added to memblock. Second, IO and reserved regions are filtered out of memblock at kernel page granularity, to prevent potential conflicting mappings.

As some reserved regions must be present in the idmap these are reserved. Everything else that's not normal memory is removed, preventing the possibility of a cacheable mapping covering something it shouldn't.

Signed-off-by Mark Rutland <[email protected]>

Cc Ard Biesheuvel <[email protected]

Cc Catalin Marinas <[email protected]>

Cc Leif Lindholm <[email protected]

Cc Mark Salter <[email protected]>

---
 arch/arm64/kernel/efi.c | 181 +++++++++++-------------------------------------
 1 file changed, 42 insertions(+), 139 deletions(-)

arch/arm64/kernel/efi.c +42 -140

index 03aaa99..8873c28 100644
@@ -154,161 +154,66 @@ static __init int is_reserve_region(efi_memory_desc_t *md)
        return 0;
 }
 
-static __init void reserve_regions(void)
+static __init void efi_add_memory(void)
 {
        efi_memory_desc_t *md;
-       u64 paddr, npages, size;
+       u64 paddr, size;
 
        if (uefi_debug)
-               pr_info("Processing EFI memory map:\n");
+               pr_info("EFI: adding normal memory:\n");
 
        for_each_efi_memory_desc(&memmap, md) {
+               if (!is_normal_ram(md))
+                       continue;
+
                paddr = md->phys_addr;
-               npages = md->num_pages;
+               size = md->num_pages << EFI_PAGE_SHIFT;
 
                if (uefi_debug)
-                       pr_info("  0x%012llx-0x%012llx [%s]",
-                               paddr, paddr + (npages << EFI_PAGE_SHIFT) - 1,
+                       pr_info("  0x%012llx-0x%012llx [%s]\n",
+                               paddr, size - 1,
                                memory_type_name[md->type]);
 
-               memrange_efi_to_native(&paddr, &npages);
-               size = npages << PAGE_SHIFT;
+               early_init_dt_add_memory_arch(paddr, size);
+       }
+
+       /*
+        * Regions describe in an EFI_MEMORY_DESCRIPTOR are only guaranteed to
+        * be 4KiB aligned but the kernel page size could be larger, and
+        * regions with different attributes could fall into the same kernel
+        * page. Thus we must remove any memory in the same kernel page as an
+        * IO region.
+        *
+        * Reserved regions of memory need to be in the idmap, so just reserve
+        * them.
+        */
+       if (uefi_debug)
+               pr_info("EFI: correcting for overlapping regions:\n");
+
+       for_each_efi_memory_desc(&memmap, md) {
+               if (!is_reserve_region(md) && is_normal_ram(md))
+                       continue;
+
+               paddr = md->phys_addr;
+               size = md->num_pages << EFI_PAGE_SHIFT;
+
+               size = PAGE_ALIGN(size + (paddr & ~PAGE_MASK));
+               paddr &= PAGE_MASK;
 
-               if (is_normal_ram(md))
-                       early_init_dt_add_memory_arch(paddr, size);
+               if (uefi_debug)
+                       pr_info("  0x%012llx-0x%012llx [%s]\n",
+                               paddr, size - 1,
+                               memory_type_name[md->type]);
 
-               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("*");
-               }
-
-               if (uefi_debug)
-                       pr_cont("\n");
+               else
+                       memblock_remove(paddr, size);
        }
 
        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;

@@ -330,7 +235,7 @@ void __init efi_init(void)
        if (uefi_init() < 0)
                return;
 
-       reserve_regions();
+       efi_add_memory();
 }
 
 void __init efi_idmap_init(void)

@@ -452,8 +357,6 @@ static int __init arm64_enter_virtual_mode(void)
 
        kfree(virtmap);
 
-       free_boot_services();
-
        if (status != EFI_SUCCESS) {
                pr_err("Failed to set EFI virtual address map! [%lx]\n",
                        status);
--

1.9.1

This adds support to the UEFI side for detecting the presence of a SMBIOS 3.0 64-bit entry point. This allows the actual SMBIOS structure table to reside at a physical offset over 4 GB, which cannot be supported by the legacy SMBIOS 32-bit entry point.

Since the firmware can legally provide both entry points, store the SMBIOS 3.0 entry point in a separate variable, and let the DMI decoding layer decide which one will be used.

Tested-by Suravee Suthikulpanit <[email protected]>

Acked-by Leif Lindholm <[email protected]>

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

---
 drivers/firmware/efi/efi.c | 4 ++++
 include/linux/efi.h        | 6 +++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

Quoted 9 lines · Ard Biesheuvel

This adds support to the UEFI side for detecting the presence of a SMBIOS 3.0 64-bit entry point. This allows the actual SMBIOS structure table to reside at a physical offset over 4 GB, which cannot be supported by the legacy SMBIOS 32-bit entry point.

Since the firmware can legally provide both entry points, store the SMBIOS 3.0 entry point in a separate variable, and let the DMI decoding layer decide which one will be used.

Quoted 7 lines

Tested-by Suravee Suthikulpanit <[email protected]>

Acked-by Leif Lindholm <[email protected]>

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

---
 drivers/firmware/efi/efi.c | 4 ++++
 include/linux/efi.h        | 6 +++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

Looks like you might also need to change driver/xen/efi.c to set EFI_INVALID_TABLE_ADDR for the new table address?

This sets the DMI string, containing system type, serial number, firmware version etc. as dump stack arch description, so that oopses and other kernel stack dumps automatically have this information included, if available.

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

---
 arch/arm64/kernel/efi.c | 2 ++
 1 file changed, 2 insertions(+)

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

index 0e9da0067ef2..baab9344a32b 100644

Quoted 10 lines · Ard Biesheuvel

This sets the DMI string, containing system type, serial number, firmware version etc. as dump stack arch description, so that oopses and other kernel stack dumps automatically have this information included, if available.

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

---
 arch/arm64/kernel/efi.c | 2 ++
 1 file changed, 2 insertions(+)

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

Quoted 10 lines

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

index 0e9da0067ef2..baab9344a32b 100644
@@ -477,6 +477,8 @@ static int __init arm64_dmi_init(void)
       * itself, depends on dmi_scan_machine() having been called already.
       */
      dmi_scan_machine();
+     if (dmi_available)
+             dmi_set_dump_stack_arch_desc();

This looks fine, but I don't understand why you would ever *not* want to call this when DMI is available. In other words, why can't this be part of dmi_scan_machine?

This is a bit of a mixed bag of patches that we would like to see merged for 3.19. Most of them have been posted and discussed on linux-efi and/or LAKML before, and one of them has even been merged and reverted twice.

Patches #1 - #4 are fixes for compliance with the UEFI and PE/COFF specs. No issues are known that require these patches, so there is no reason to pull them into a stable release.

Patches #5 and #6 address minor issues in the arm64 EFI init code.

Patches #7 - #10 implement DMI/SMBIOS for arm64, both the existing 32-bit version and the upcoming 3.0 version that allows the SMBIOS structure table to reside at a physical offset that cannot be encoded in 32-bits. It also install a 'Hardware: xxx' string that is printed along with oopses and kernel call stack dumps on systems that implement DMI/SMBIOS.

Please refer to the patches themselves for version history. Acks and/or comments appreciated.

Hi Ard,

Quoted 16 lines · Ard Biesheuvel

This is a bit of a mixed bag of patches that we would like to see merged for 3.19. Most of them have been posted and discussed on linux-efi and/or LAKML before, and one of them has even been merged and reverted twice.

Patches #1 - #4 are fixes for compliance with the UEFI and PE/COFF specs. No issues are known that require these patches, so there is no reason to pull them into a stable release.

Patches #5 and #6 address minor issues in the arm64 EFI init code.

Patches #7 - #10 implement DMI/SMBIOS for arm64, both the existing 32-bit version and the upcoming 3.0 version that allows the SMBIOS structure table to reside at a physical offset that cannot be encoded in 32-bits. It also install a 'Hardware: xxx' string that is printed along with oopses and kernel call stack dumps on systems that implement DMI/SMBIOS.

Quoted 2 lines

Please refer to the patches themselves for version history. Acks and/or comments appreciated.

It would be helpful for me if you could split this into two series -- one for the arm64 tree and the other for the efi tree. That would also make the dependencies clearer, so I know whether any of the arch bits need to go via Matt.

From this series, only patches #7 and #8 need to go through Matt's

Yeah, I was struggling a bit with that. I think the agreement was that everything EFI related goes through Matt's tree, but I don't think that necessarily makes sense for patches that only touch arch/arm64, unless there are interdependencies with the generic code.

From this series, only patches #7 and #8 need to go through Matt's tree, and even if #9 and #10 are also related to SMBIOS, they are in fact orthogonal to #7 and #8, so those can still go through the arm64 tree without any merge order issues later on.

@Matt: any thoughts?

Quoted 11 lines · Ard Biesheuvel

Yeah, I was struggling a bit with that. I think the agreement was that everything EFI related goes through Matt's tree, but I don't think that necessarily makes sense for patches that only touch arch/arm64, unless there are interdependencies with the generic code.

From this series, only patches #7 and #8 need to go through Matt's tree, and even if #9 and #10 are also related to SMBIOS, they are in fact orthogonal to #7 and #8, so those can still go through the arm64 tree without any merge order issues later on.

Quoted 1 line

@Matt: any thoughts?

In this case, if it makes things easier for you folks in terms of dependencies to take this *all* through the arm64 tree, that seems sensible to me.