[RFC PATCH 0/4] EFI boot stub memory map fix

[RFC PATCH 0/4] EFI boot stub memory map fix

[Linn Crosetto]

I realize the EFI stub for ARM patches are in flight, 

https://lkml.org/lkml/2013/8/9/554

and overlap with some of the files but I wanted to send these out for comment.

This series fixes a problem with EFI memory maps larger than 128 entries when
booting using the EFI boot stub, which results in overflowing the e820_map in
boot_params and an eventual halt when checking the map size in
sanitize_e820_map().

The fix implemented is to add the EFI memory map from setup_arch() via a
memory_setup hook.

Two options were considered:

 1. Use the SETUP_E820_EXT setup_data type to add the extra entries.

 2. Create a memory_setup function to be enabled when the EFI memory map is
    needed.

Option 2 appeared to be the cleaner solution, reducing duplication with
existing code, given a reasonable mechanism for determining when to
replace the default memory_setup function.


Linn Crosetto (4):
  efi: Decouple efi_memmap_init() and do_add_efi_memmap()
  efi: Add memory_setup function efi_memory_setup()
  efi: Add efi_memmap_needed()
  x86: Fix EFI boot stub for large memory maps

 arch/x86/boot/compressed/eboot.c | 64 ++--------------------------------------
 arch/x86/kernel/setup.c          |  3 ++
 arch/x86/platform/efi/efi.c      | 51 +++++++++++++++++++++++---------
 include/linux/efi.h              |  2 ++
 4 files changed, 45 insertions(+), 75 deletions(-)

-- 
1.7.11.3

[RFC PATCH 1/4] efi: Decouple efi_memmap_init() and do_add_efi_memmap()

[Linn Crosetto]

In order to enable the EFI memory map to be initialized outside of
efi_init(), decouple the two functions efi_memmap_init() and
do_add_efi_memmap() and set the x86_facility EFI_MEMMAP in
efi_memmap_init(). Additionally, check EFI_MEMMAP to determine whether
efi_memmap_init() has been previously called to avoid doing the
initialization more than once.

Signed-off-by: Linn Crosetto <linn-VXdhtT5mjnY@public.gmane.org>
---
 arch/x86/platform/efi/efi.c | 44 ++++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 90f6ed1..32da922 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -326,6 +326,24 @@ void efi_get_time(struct timespec *now)
 	now->tv_nsec = 0;
 }
 
+static int __init efi_memmap_init(void)
+{
+	if (efi_enabled(EFI_MEMMAP))
+		return 0; /* already initialized */
+
+	/* Map the EFI memory map */
+	memmap.map = early_ioremap((unsigned long)memmap.phys_map,
+				   memmap.nr_map * memmap.desc_size);
+	if (memmap.map == NULL) {
+		pr_err("Could not map the memory map!\n");
+		return -ENOMEM;
+	}
+	memmap.map_end = memmap.map + (memmap.nr_map * memmap.desc_size);
+
+	set_bit(EFI_MEMMAP, &x86_efi_facility);
+	return 0;
+}
+
 /*
  * Tell the kernel about the EFI memory map.  This might include
  * more than the max 128 entries that can fit in the e820 legacy
@@ -336,6 +354,9 @@ static void __init do_add_efi_memmap(void)
 {
 	void *p;
 
+	if (!efi_enabled(EFI_MEMMAP) && efi_memmap_init())
+		return;
+
 	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
 		efi_memory_desc_t *md = p;
 		unsigned long long start = md->phys_addr;
@@ -409,6 +430,9 @@ static void __init print_efi_memmap(void)
 	void *p;
 	int i;
 
+	if (!efi_enabled(EFI_MEMMAP))
+		return;
+
 	for (p = memmap.map, i = 0;
 	     p < memmap.map_end;
 	     p += memmap.desc_size, i++) {
@@ -687,23 +711,6 @@ static int __init efi_runtime_init(void)
 	return 0;
 }
 
-static int __init efi_memmap_init(void)
-{
-	/* Map the EFI memory map */
-	memmap.map = early_ioremap((unsigned long)memmap.phys_map,
-				   memmap.nr_map * memmap.desc_size);
-	if (memmap.map == NULL) {
-		pr_err("Could not map the memory map!\n");
-		return -ENOMEM;
-	}
-	memmap.map_end = memmap.map + (memmap.nr_map * memmap.desc_size);
-
-	if (add_efi_memmap)
-		do_add_efi_memmap();
-
-	return 0;
-}
-
 void __init efi_init(void)
 {
 	efi_char16_t *c16;
@@ -766,7 +773,8 @@ void __init efi_init(void)
 	if (efi_memmap_init())
 		return;
 
-	set_bit(EFI_MEMMAP, &x86_efi_facility);
+	if (add_efi_memmap)
+		do_add_efi_memmap();
 
 #ifdef CONFIG_X86_32
 	if (efi_is_native()) {
-- 
1.7.11.3

Re: [RFC PATCH 0/4] EFI boot stub memory map fix

[H. Peter Anvin]

I would strongly disagree that option 2 is the cleaner solution.

Linn Crosetto <linn-VXdhtT5mjnY@public.gmane.org> wrote:
>I realize the EFI stub for ARM patches are in flight, 
>
>https://lkml.org/lkml/2013/8/9/554
>
>and overlap with some of the files but I wanted to send these out for
>comment.
>
>This series fixes a problem with EFI memory maps larger than 128
>entries when
>booting using the EFI boot stub, which results in overflowing the
>e820_map in
>boot_params and an eventual halt when checking the map size in
>sanitize_e820_map().
>
>The fix implemented is to add the EFI memory map from setup_arch() via
>a
>memory_setup hook.
>
>Two options were considered:
>
> 1. Use the SETUP_E820_EXT setup_data type to add the extra entries.
>
>2. Create a memory_setup function to be enabled when the EFI memory map
>is
>    needed.
>
>Option 2 appeared to be the cleaner solution, reducing duplication with
>existing code, given a reasonable mechanism for determining when to
>replace the default memory_setup function.
>
>
>Linn Crosetto (4):
>  efi: Decouple efi_memmap_init() and do_add_efi_memmap()
>  efi: Add memory_setup function efi_memory_setup()
>  efi: Add efi_memmap_needed()
>  x86: Fix EFI boot stub for large memory maps
>
>arch/x86/boot/compressed/eboot.c | 64
>++--------------------------------------
> arch/x86/kernel/setup.c          |  3 ++
> arch/x86/platform/efi/efi.c      | 51 +++++++++++++++++++++++---------
> include/linux/efi.h              |  2 ++
> 4 files changed, 45 insertions(+), 75 deletions(-)

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

Re: [RFC PATCH 0/4] EFI boot stub memory map fix

[Yinghai Lu]

On Mon, Aug 19, 2013 at 1:06 PM, H. Peter Anvin <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org> wrote:
> I would strongly disagree that option 2 is the cleaner solution.

Agreed.

>
> Linn Crosetto <linn-VXdhtT5mjnY@public.gmane.org> wrote:
>>I realize the EFI stub for ARM patches are in flight,
>>
>>https://lkml.org/lkml/2013/8/9/554
>>
>>and overlap with some of the files but I wanted to send these out for
>>comment.
>>
>>This series fixes a problem with EFI memory maps larger than 128
>>entries when
>>booting using the EFI boot stub, which results in overflowing the
>>e820_map in
>>boot_params and an eventual halt when checking the map size in
>>sanitize_e820_map().
>>
>>The fix implemented is to add the EFI memory map from setup_arch() via
>>a
>>memory_setup hook.
>>
>>Two options were considered:
>>
>> 1. Use the SETUP_E820_EXT setup_data type to add the extra entries.
>>
>>2. Create a memory_setup function to be enabled when the EFI memory map
>>is
>>    needed.
>>
>>Option 2 appeared to be the cleaner solution, reducing duplication with
>>existing code, given a reasonable mechanism for determining when to
>>replace the default memory_setup function.

If boot_loader could create setup_data with SETUP_E820_EXT,
efi_stub should go that path too.
We should not add another path.

Thanks

Yinghai

Re: [RFC PATCH 0/4] EFI boot stub memory map fix

[Linn Crosetto]

On Mon, Aug 19, 2013 at 01:47:41PM -0700, Yinghai Lu wrote:
> On Mon, Aug 19, 2013 at 1:06 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> > I would strongly disagree that option 2 is the cleaner solution.
> 
> Agreed.
> 
> >
> > Linn Crosetto <linn@hp.com> wrote:
> >>I realize the EFI stub for ARM patches are in flight,
> >>
> >>https://lkml.org/lkml/2013/8/9/554
> >>
> >>and overlap with some of the files but I wanted to send these out for
> >>comment.
> >>
> >>This series fixes a problem with EFI memory maps larger than 128
> >>entries when
> >>booting using the EFI boot stub, which results in overflowing the
> >>e820_map in
> >>boot_params and an eventual halt when checking the map size in
> >>sanitize_e820_map().
> >>
> >>The fix implemented is to add the EFI memory map from setup_arch() via
> >>a
> >>memory_setup hook.
> >>
> >>Two options were considered:
> >>
> >> 1. Use the SETUP_E820_EXT setup_data type to add the extra entries.
> >>
> >>2. Create a memory_setup function to be enabled when the EFI memory map
> >>is
> >>    needed.
> >>
> >>Option 2 appeared to be the cleaner solution, reducing duplication with
> >>existing code, given a reasonable mechanism for determining when to
> >>replace the default memory_setup function.
> 
> If boot_loader could create setup_data with SETUP_E820_EXT,
> efi_stub should go that path too.
> We should not add another path.

My consideration was in leveraging do_add_efi_memmap(), which duplicates what is
done with the map in exit_boot(), and can already be called via the
"add_efi_memmap" parameter. One complication with SETUP_E820_EXT is in
determining the size needed, since a call to allocate_pool will change the
memory map. 

I will send another version which uses SETUP_E820_EXT.

Thanks,
Linn

Re: [RFC PATCH 0/4] EFI boot stub memory map fix

[Yinghai Lu]

On Mon, Aug 19, 2013 at 2:09 PM, Linn Crosetto <linn-VXdhtT5mjnY@public.gmane.org> wrote:

> One complication with SETUP_E820_EXT is in
> determining the size needed, since a call to allocate_pool will change the
> memory map.

second try or while loop could handle that.

Yinghai

[RFC PATCH 2/4] efi: Add memory_setup function efi_memory_setup()

[Linn Crosetto]

Adding efi_memory_setup() to be used to override the default
memory_setup function when the EFI memory map should be used instead of
a BIOS provided e820 map.

Signed-off-by: Linn Crosetto <linn@hp.com>
---
 arch/x86/platform/efi/efi.c | 8 ++++++++
 include/linux/efi.h         | 1 +
 2 files changed, 9 insertions(+)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 32da922..c2a660c 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -711,6 +711,14 @@ static int __init efi_runtime_init(void)
 	return 0;
 }
 
+char * __init efi_memory_setup(void)
+{
+	char *who = "EFI";
+	efi_memmap_init();
+	do_add_efi_memmap();
+	return who;
+}
+
 void __init efi_init(void)
 {
 	efi_char16_t *c16;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 5f8f176..977bbc7 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -567,6 +567,7 @@ efi_guid_unparse(efi_guid_t *guid, char *out)
         return out;
 }
 
+extern char *efi_memory_setup(void);
 extern void efi_init (void);
 extern void *efi_get_pal_addr (void);
 extern void efi_map_pal_code (void);
-- 
1.7.11.3

[RFC PATCH 3/4] efi: Add efi_memmap_needed()

[Linn Crosetto]

The function efi_memmap_needed() is used to determine when to use the
EFI memory map during boot instead of a BIOS provided e820.

The intent is to determine whether we are booted on an EFI system from
the EFI boot stub (came in through efi_main(), implemented by checking
efi_is_native() and the boot loader identifier), and that there is no
e820 map already provided.

Signed-off-by: Linn Crosetto <linn@hp.com>
---
 arch/x86/platform/efi/efi.c | 7 +++++++
 include/linux/efi.h         | 1 +
 2 files changed, 8 insertions(+)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index c2a660c..af32a21 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -719,6 +719,13 @@ char * __init efi_memory_setup(void)
 	return who;
 }
 
+int __init efi_memmap_needed(void)
+{
+	return efi_is_native() &&
+	       boot_params.e820_entries == 0 &&
+	       boot_params.hdr.type_of_loader >> 4 == 2; /* bootsect-loader */
+}
+
 void __init efi_init(void)
 {
 	efi_char16_t *c16;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 977bbc7..95b2251 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -567,6 +567,7 @@ efi_guid_unparse(efi_guid_t *guid, char *out)
         return out;
 }
 
+extern int efi_memmap_needed(void);
 extern char *efi_memory_setup(void);
 extern void efi_init (void);
 extern void *efi_get_pal_addr (void);
-- 
1.7.11.3

[RFC PATCH 4/4] x86: Fix EFI boot stub for large memory maps

[Linn Crosetto]

This patch adds support for EFI memory maps larger than 128 entries when
booted using the EFI boot stub.

The e820 map in struct boot_params can only hold 128 entries, but the
memory map provided by EFI may be larger. If the map contained more than
128 entries, exit_boot() would write beyond the e820_map array in
boot_params and the system would eventually halt in the BUG_ON check in
sanitize_e820_map().

In the case we come in through efi_main(), the EFI memory map is used.
Instead of populating e820_map in boot_params, create the e820 from the
EFI memory map in setup_arch() via a memory_setup hook.

The EFI memory map may also be added in efi_init() if the command line
parameter "add_efi_memmap" is specified. This is left intact to allow
the command line parameter to remain effective.

Signed-off-by: Linn Crosetto <linn@hp.com>
---
 arch/x86/boot/compressed/eboot.c | 64 ++--------------------------------------
 arch/x86/kernel/setup.c          |  3 ++
 2 files changed, 6 insertions(+), 61 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index b7388a4..66b8d3d 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -973,6 +973,9 @@ struct boot_params *make_boot_params(void *handle, efi_system_table_t *_table)
 	if (status != EFI_SUCCESS)
 		goto fail2;
 
+	/* Use EFI memory map */
+	boot_params->e820_entries = 0;
+
 	return boot_params;
 fail2:
 	if (options_size)
@@ -986,8 +989,6 @@ static efi_status_t exit_boot(struct boot_params *boot_params,
 			      void *handle)
 {
 	struct efi_info *efi = &boot_params->efi_info;
-	struct e820entry *e820_map = &boot_params->e820_map[0];
-	struct e820entry *prev = NULL;
 	unsigned long size, key, desc_size, _size;
 	efi_memory_desc_t *mem_map;
 	efi_status_t status;
@@ -1049,65 +1050,6 @@ get_map:
 	/* Historic? */
 	boot_params->alt_mem_k = 32 * 1024;
 
-	/*
-	 * Convert the EFI memory map to E820.
-	 */
-	nr_entries = 0;
-	for (i = 0; i < size / desc_size; i++) {
-		efi_memory_desc_t *d;
-		unsigned int e820_type = 0;
-		unsigned long m = (unsigned long)mem_map;
-
-		d = (efi_memory_desc_t *)(m + (i * desc_size));
-		switch (d->type) {
-		case EFI_RESERVED_TYPE:
-		case EFI_RUNTIME_SERVICES_CODE:
-		case EFI_RUNTIME_SERVICES_DATA:
-		case EFI_MEMORY_MAPPED_IO:
-		case EFI_MEMORY_MAPPED_IO_PORT_SPACE:
-		case EFI_PAL_CODE:
-			e820_type = E820_RESERVED;
-			break;
-
-		case EFI_UNUSABLE_MEMORY:
-			e820_type = E820_UNUSABLE;
-			break;
-
-		case EFI_ACPI_RECLAIM_MEMORY:
-			e820_type = E820_ACPI;
-			break;
-
-		case EFI_LOADER_CODE:
-		case EFI_LOADER_DATA:
-		case EFI_BOOT_SERVICES_CODE:
-		case EFI_BOOT_SERVICES_DATA:
-		case EFI_CONVENTIONAL_MEMORY:
-			e820_type = E820_RAM;
-			break;
-
-		case EFI_ACPI_MEMORY_NVS:
-			e820_type = E820_NVS;
-			break;
-
-		default:
-			continue;
-		}
-
-		/* Merge adjacent mappings */
-		if (prev && prev->type == e820_type &&
-		    (prev->addr + prev->size) == d->phys_addr)
-			prev->size += d->num_pages << 12;
-		else {
-			e820_map->addr = d->phys_addr;
-			e820_map->size = d->num_pages << 12;
-			e820_map->type = e820_type;
-			prev = e820_map++;
-			nr_entries++;
-		}
-	}
-
-	boot_params->e820_entries = nr_entries;
-
 	return EFI_SUCCESS;
 
 free_mem_map:
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index f8ec578..9682c8c 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -920,6 +920,9 @@ void __init setup_arch(char **cmdline_p)
 
 	if (efi_enabled(EFI_BOOT))
 		efi_memblock_x86_reserve_range();
+
+	if (efi_memmap_needed())
+		x86_init.resources.memory_setup = efi_memory_setup;
 #endif
 
 	x86_init.oem.arch_setup();
-- 
1.7.11.3