> On Oct 14, 2018, at 9:34 AM, Jeffrey Hugo <jhugo@xxxxxxxxxxxxxx> wrote:
>
> On 10/12/2018 3:46 PM, Ard Biesheuvel wrote:
>> On 9 August 2018 at 16:50, Eric Snowberg <eric.snowberg@xxxxxxxxxx> wrote:
>>> Commit d64934019f6c ("x86/efi: Use efi_exit_boot_services()")
>>> introduced a regression on systems with large memory maps
>>> causing them to hang on boot. The first "goto get_map" that was removed
>>> from exit_boot insured there was enough room for the memory map when
>>> efi_call_early(exit_boot_services) was called. This happens when
>>> (nr_desc > ARRAY_SIZE(params->e820_table).
>>>
>>> Chain of events:
>>> exit_boot()
>>> efi_exit_boot_services()
>>> efi_get_memory_map <- at this point the mm can't grow over 8 desc
>>> priv_func()
>>> exit_boot_func()
>>> allocate_e820ext() <- new mm grows over 8 desc from e820 alloc
>>> efi_call_early(exit_boot_services) <- mm key doesn't match so retry
>>> efi_call_early(get_memory_map) <- not enough room for new mm
>>> system hangs
>>>
>>> This patch allocates the e820 buffer before calling efi_exit_boot_services
>>> and fixes the regression.
>>>
>>> Signed-off-by: Eric Snowberg <eric.snowberg@xxxxxxxxxx>
>>> ---
>>> arch/x86/boot/compressed/eboot.c | 63 +++++++++++++++++++++++++---------------
>>> 1 file changed, 40 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
>>> index cbf4b87..91a650e 100644
>>> --- a/arch/x86/boot/compressed/eboot.c
>>> +++ b/arch/x86/boot/compressed/eboot.c
>>> @@ -866,11 +866,43 @@ static efi_status_t alloc_e820ext(u32 nr_desc, struct setup_data **e820ext,
>>> return status;
>>> }
>>>
>>> +static efi_status_t allocate_e820(struct boot_params *params,
>>> + struct setup_data **e820ext,
>>> + u32 *e820ext_size)
>>> +{
>>> + unsigned long map_size, desc_size, buff_size;
>>> + struct efi_boot_memmap boot_map;
>>> + efi_memory_desc_t *map;
>>> + efi_status_t status;
>>> + __u32 nr_desc;
>>> +
>>> + boot_map.map = ↦
>>> + boot_map.map_size = &map_size;
>>> + boot_map.desc_size = &desc_size;
>>> + boot_map.desc_ver = NULL;
>>> + boot_map.key_ptr = NULL;
>>> + boot_map.buff_size = &buff_size;
>>> +
>>> + status = efi_get_memory_map(sys_table, &boot_map);
>>> + if (status != EFI_SUCCESS)
>>> + return status;
>>> +
>>> + nr_desc = buff_size / desc_size;
>>> +
>> I think we should add some slack here. This function is now called
>> right before the first call to ExitBootServices(), which means events
>> may fire that allocate or free memory between this call to
>> efi_get_memory_map() and the final one which obtains the map key
>> (which is the whole reason for this complicated dance)
>
> Slack in addition to what efi_get_memory_map() provides?
Would additional slack be necessary here? This takes place before memory allocation is no longer possible. The efi_get_memory_map function will be called again afterwards insuring their is slack available for the memory map passed into EBS.
