[PATCH] x86/efi: delay efi_esrt_init to efi_late_init

[PATCH] x86/efi: delay efi_esrt_init to efi_late_init

[Ricardo Neri]

Commit 7b02d53e7852 introduced a new efi_mem_reserve to reserve the boot
services memory regions forever. This reservation involves allocating a new
EFI memory range descriptor. However, allocation can only succeed if there
is memory available for the allocation. Otherwise, error such as the
following may occur:

esrt: Reserving ESRT space from 0x000000003dd6a000 to 0x000000003dd6a010.
Kernel panic - not syncing: ERROR: Failed to allocate 0x9f0 bytes below 0x0.
CPU: 0 PID: 0 Comm: swapper Not tainted 4.7.0-rc5+ #503
 0000000000000000 ffffffff81e03ce0 ffffffff8131dae8 ffffffff81bb6c50
 ffffffff81e03d70 ffffffff81e03d60 ffffffff8111f4df 0000000000000018
 ffffffff81e03d70 ffffffff81e03d08 00000000000009f0 00000000000009f0
Call Trace:
 [<ffffffff8131dae8>] dump_stack+0x4d/0x65
 [<ffffffff8111f4df>] panic+0xc5/0x206
 [<ffffffff81f7c6d3>] memblock_alloc_base+0x29/0x2e
 [<ffffffff81f7c6e3>] memblock_alloc+0xb/0xd
 [<ffffffff81f6c86d>] efi_arch_mem_reserve+0xbc/0x134
 [<ffffffff81fa3280>] efi_mem_reserve+0x2c/0x31
 [<ffffffff81fa3280>] ? efi_mem_reserve+0x2c/0x31
 [<ffffffff81fa40d3>] efi_esrt_init+0x19e/0x1b4
 [<ffffffff81f6d2dd>] efi_init+0x398/0x44a
 [<ffffffff81f5c782>] setup_arch+0x415/0xc30
 [<ffffffff81f55af1>] start_kernel+0x5b/0x3ef
 [<ffffffff81f55434>] x86_64_start_reservations+0x2f/0x31
 [<ffffffff81f55520>] x86_64_start_kernel+0xea/0xed
---[ end Kernel panic - not syncing: ERROR: Failed to allocate 0x9f0
     bytes below 0x0.

An inspection of the memblock configuration reveals that there is no memory
available for the allocation:

MEMBLOCK configuration:
 memory size = 0x0 reserved size = 0x4f339c0
 memory.cnt  = 0x1
 memory[0x0]	[0x00000000000000-0xffffffffffffffff], 0x0 bytes on node 0 flags: 0x0
 reserved.cnt  = 0x4
 reserved[0x0]	[0x0000000008c000-0x0000000008c9bf], 0x9c0 bytes flags: 0x0
 reserved[0x1]	[0x0000000009f000-0x000000000fffff], 0x61000 bytes flags: 0x0
 reserved[0x2]	[0x00000002800000-0x0000000394bfff], 0x114c000 bytes flags: 0x0
 reserved[0x3]	[0x000000304e4000-0x00000034269fff], 0x3d86000 bytes flags: 0x0

efi_esrt_init is called from efi_init, which in x86 happens before
memblock_x86_fill is called. Since ESRT is not critical to boot but to
runtime, its initialization can be safely delayed to efi_late_init.

Signed-off-by: Ricardo Neri <ricardo.neri-calderon-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 arch/x86/platform/efi/efi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index ef9b6ab..d92c174 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -475,12 +475,11 @@ void __init efi_init(void)
 
 	if (efi_enabled(EFI_DBG))
 		efi_print_memmap();
-
-	efi_esrt_init();
 }
 
 void __init efi_late_init(void)
 {
+	efi_esrt_init();
 	efi_bgrt_init();
 }
 
-- 
2.9.2

Re: [PATCH] x86/efi: delay efi_esrt_init to efi_late_init

[Matt Fleming]

On Mon, 08 Aug, at 03:08:24PM, Ricardo Neri wrote:
> Commit 7b02d53e7852 introduced a new efi_mem_reserve to reserve the boot
> services memory regions forever. This reservation involves allocating a new
> EFI memory range descriptor. However, allocation can only succeed if there
> is memory available for the allocation. Otherwise, error such as the
> following may occur:
> 
> esrt: Reserving ESRT space from 0x000000003dd6a000 to 0x000000003dd6a010.
> Kernel panic - not syncing: ERROR: Failed to allocate 0x9f0 bytes below 0x0.
> CPU: 0 PID: 0 Comm: swapper Not tainted 4.7.0-rc5+ #503
>  0000000000000000 ffffffff81e03ce0 ffffffff8131dae8 ffffffff81bb6c50
>  ffffffff81e03d70 ffffffff81e03d60 ffffffff8111f4df 0000000000000018
>  ffffffff81e03d70 ffffffff81e03d08 00000000000009f0 00000000000009f0
> Call Trace:
>  [<ffffffff8131dae8>] dump_stack+0x4d/0x65
>  [<ffffffff8111f4df>] panic+0xc5/0x206
>  [<ffffffff81f7c6d3>] memblock_alloc_base+0x29/0x2e
>  [<ffffffff81f7c6e3>] memblock_alloc+0xb/0xd
>  [<ffffffff81f6c86d>] efi_arch_mem_reserve+0xbc/0x134
>  [<ffffffff81fa3280>] efi_mem_reserve+0x2c/0x31
>  [<ffffffff81fa3280>] ? efi_mem_reserve+0x2c/0x31
>  [<ffffffff81fa40d3>] efi_esrt_init+0x19e/0x1b4
>  [<ffffffff81f6d2dd>] efi_init+0x398/0x44a
>  [<ffffffff81f5c782>] setup_arch+0x415/0xc30
>  [<ffffffff81f55af1>] start_kernel+0x5b/0x3ef
>  [<ffffffff81f55434>] x86_64_start_reservations+0x2f/0x31
>  [<ffffffff81f55520>] x86_64_start_kernel+0xea/0xed
> ---[ end Kernel panic - not syncing: ERROR: Failed to allocate 0x9f0
>      bytes below 0x0.
> 
> An inspection of the memblock configuration reveals that there is no memory
> available for the allocation:
> 
> MEMBLOCK configuration:
>  memory size = 0x0 reserved size = 0x4f339c0
>  memory.cnt  = 0x1
>  memory[0x0]	[0x00000000000000-0xffffffffffffffff], 0x0 bytes on node 0 flags: 0x0
>  reserved.cnt  = 0x4
>  reserved[0x0]	[0x0000000008c000-0x0000000008c9bf], 0x9c0 bytes flags: 0x0
>  reserved[0x1]	[0x0000000009f000-0x000000000fffff], 0x61000 bytes flags: 0x0
>  reserved[0x2]	[0x00000002800000-0x0000000394bfff], 0x114c000 bytes flags: 0x0
>  reserved[0x3]	[0x000000304e4000-0x00000034269fff], 0x3d86000 bytes flags: 0x0
> 
> efi_esrt_init is called from efi_init, which in x86 happens before
> memblock_x86_fill is called. Since ESRT is not critical to boot but to
> runtime, its initialization can be safely delayed to efi_late_init.

Ouch. This looks like the right thing to do now that we no longer need
to reserve the ESRT data before efi_reserve_boot_services() runs.

Ard, could you confirm that this isn't does not exist for ARM/arm64?
It looks like reserve_regions() takes care of adding all RAM from the
EFI memmap, so efi_esrt_init() is free to make allocations.

Actually Ricardo, there's a slight hiccup with this patch in that
efi_esrt_init() is now being called at very different points during
boot.

ARM/arm64 call it at efi_init() time and the early_memremap()
calls in the ESRT driver make sense, but those calls don't make sense
when invoked from efi_late_init() on x86. Look how the ACPI BGRT
driver makes use of the standard memremap() API.

Instead, could you fold this call into the same location as
efi_find_mirror() and co. in x86's setup_arch()?

Re: [PATCH] x86/efi: delay efi_esrt_init to efi_late_init

[Ricardo Neri]

On Thu, 2016-08-11 at 11:29 +0100, Matt Fleming wrote:
> Actually Ricardo, there's a slight hiccup with this patch in that
> efi_esrt_init() is now being called at very different points during
> boot.
> 
> ARM/arm64 call it at efi_init() time and the early_memremap()
> calls in the ESRT driver make sense, but those calls don't make sense
> when invoked from efi_late_init() on x86. Look how the ACPI BGRT
> driver makes use of the standard memremap() API.

Ah, I just checked. Perhaps efi_late_init is too late. Also, one
question for my knowledge, why the ESRT driver uses eary_memremap()?
Can't it uses the regular memremap as the EFI BGRT driver? As far as I
can see, it only exposes ESRT objects in sysfs.
> 
> Instead, could you fold this call into the same location as
> efi_find_mirror() and co. in x86's setup_arch()?

Sure. I just sent a v2 with your suggestion.

Thanks and BR,
Ricardo

Re: [PATCH] x86/efi: delay efi_esrt_init to efi_late_init

[Ard Biesheuvel]

On 11 August 2016 at 12:29, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Mon, 08 Aug, at 03:08:24PM, Ricardo Neri wrote:
>> Commit 7b02d53e7852 introduced a new efi_mem_reserve to reserve the boot
>> services memory regions forever. This reservation involves allocating a new
>> EFI memory range descriptor. However, allocation can only succeed if there
>> is memory available for the allocation. Otherwise, error such as the
>> following may occur:
>>
>> esrt: Reserving ESRT space from 0x000000003dd6a000 to 0x000000003dd6a010.
>> Kernel panic - not syncing: ERROR: Failed to allocate 0x9f0 bytes below 0x0.
>> CPU: 0 PID: 0 Comm: swapper Not tainted 4.7.0-rc5+ #503
>>  0000000000000000 ffffffff81e03ce0 ffffffff8131dae8 ffffffff81bb6c50
>>  ffffffff81e03d70 ffffffff81e03d60 ffffffff8111f4df 0000000000000018
>>  ffffffff81e03d70 ffffffff81e03d08 00000000000009f0 00000000000009f0
>> Call Trace:
>>  [<ffffffff8131dae8>] dump_stack+0x4d/0x65
>>  [<ffffffff8111f4df>] panic+0xc5/0x206
>>  [<ffffffff81f7c6d3>] memblock_alloc_base+0x29/0x2e
>>  [<ffffffff81f7c6e3>] memblock_alloc+0xb/0xd
>>  [<ffffffff81f6c86d>] efi_arch_mem_reserve+0xbc/0x134
>>  [<ffffffff81fa3280>] efi_mem_reserve+0x2c/0x31
>>  [<ffffffff81fa3280>] ? efi_mem_reserve+0x2c/0x31
>>  [<ffffffff81fa40d3>] efi_esrt_init+0x19e/0x1b4
>>  [<ffffffff81f6d2dd>] efi_init+0x398/0x44a
>>  [<ffffffff81f5c782>] setup_arch+0x415/0xc30
>>  [<ffffffff81f55af1>] start_kernel+0x5b/0x3ef
>>  [<ffffffff81f55434>] x86_64_start_reservations+0x2f/0x31
>>  [<ffffffff81f55520>] x86_64_start_kernel+0xea/0xed
>> ---[ end Kernel panic - not syncing: ERROR: Failed to allocate 0x9f0
>>      bytes below 0x0.
>>
>> An inspection of the memblock configuration reveals that there is no memory
>> available for the allocation:
>>
>> MEMBLOCK configuration:
>>  memory size = 0x0 reserved size = 0x4f339c0
>>  memory.cnt  = 0x1
>>  memory[0x0]  [0x00000000000000-0xffffffffffffffff], 0x0 bytes on node 0 flags: 0x0
>>  reserved.cnt  = 0x4
>>  reserved[0x0]        [0x0000000008c000-0x0000000008c9bf], 0x9c0 bytes flags: 0x0
>>  reserved[0x1]        [0x0000000009f000-0x000000000fffff], 0x61000 bytes flags: 0x0
>>  reserved[0x2]        [0x00000002800000-0x0000000394bfff], 0x114c000 bytes flags: 0x0
>>  reserved[0x3]        [0x000000304e4000-0x00000034269fff], 0x3d86000 bytes flags: 0x0
>>
>> efi_esrt_init is called from efi_init, which in x86 happens before
>> memblock_x86_fill is called. Since ESRT is not critical to boot but to
>> runtime, its initialization can be safely delayed to efi_late_init.
>
> Ouch. This looks like the right thing to do now that we no longer need
> to reserve the ESRT data before efi_reserve_boot_services() runs.
>
> Ard, could you confirm that this isn't does not exist for ARM/arm64?
> It looks like reserve_regions() takes care of adding all RAM from the
> EFI memmap, so efi_esrt_init() is free to make allocations.
>

Apologies for the tardiness. But indeed. efi_esrt_init() is called
right after reserve_regions(), and so memblock_alloc() should be able
to succeed at this time, if it weren't for the case that we don't
allocate new EFI memmap entries on ARM to begin with (and the
memblock_reserve() in efi_mem_reserve() is perfectly safe even before
reserve_regions() has executed)

-- 
Ard.