Re: [PATCH] x86, efi: retry ExitBootServices() on failure

[joeyli]

Hi Zach, 

æ äï2013-06-11 æ 07:52 +0100ïMatt Fleming æåï
> From: Zach Bobroff <zacharyb@xxxxxxx>
> 
> ExitBootServices is absolutely supposed to return a failure if any
> ExitBootServices event handler changes the memory map.  Basically the
> get_map loop should run again if ExitBootServices returns an error the
> first time.  I would say it would be fair that if ExitBootServices gives
> an error the second time then Linux would be fine in returning control
> back to BIOS.
> 
> The second change is the following line:
> 
> again:
>         size += sizeof(*mem_map) * 2;
> 
> Originally you were incrementing it by the size of one memory map entry.
> The issue here is all related to the low_alloc routine you are using.
> In this routine you are making allocations to get the memory map itself.
> Doing this allocation or allocations can affect the memory map by more
> than one record.
> 
> [ mfleming - changelog, code style ]
> Signed-off-by: Zach Bobroff <zacharyb@xxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Matt Fleming <matt.fleming@xxxxxxxxx>
> ---
>  arch/x86/boot/compressed/eboot.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index 35ee62f..7c6e5d9 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -1037,18 +1037,20 @@ static efi_status_t exit_boot(struct boot_params *boot_params,
>  	efi_memory_desc_t *mem_map;
>  	efi_status_t status;
>  	__u32 desc_version;
> +	bool called_exit = false;
>  	u8 nr_entries;
>  	int i;
>  
>  	size = sizeof(*mem_map) * 32;
>  
>  again:
> -	size += sizeof(*mem_map);
> +	size += sizeof(*mem_map) * 2;
>  	_size = size;
>  	status = low_alloc(size, 1, (unsigned long *)&mem_map);

Can we know why increased the size of double *mem_map is big enough?
Does there have any real guarantee to be sufficient?

And, why would doubling the increment be necessary here, but not in
__get_map()?

>  	if (status != EFI_SUCCESS)
>  		return status;
>  
> +get_map:

The get_map label is being placed such that the size doesn't
get adjusted anymore, yet it is supposed to deal with an allocation
having happened during ExitBootServices(), which could have
grown the map.

Why doesn't direct goto 'again' label?

>  	status = efi_call_phys5(sys_table->boottime->get_memory_map, &size,
>  				mem_map, &key, &desc_size, &desc_version);
>  	if (status == EFI_BUFFER_TOO_SMALL) {
> @@ -1074,8 +1076,20 @@ again:
>  	/* Might as well exit boot services now */
>  	status = efi_call_phys2(sys_table->boottime->exit_boot_services,
>  				handle, key);
> -	if (status != EFI_SUCCESS)
> -		goto free_mem_map;
> +	if (status != EFI_SUCCESS) {
> +		/*
> +		 * ExitBootServices() will fail if any of the event
> +		 * handlers change the memory map. In which case, we
> +		 * must be prepared to retry, but only once so that
> +		 * we're guaranteed to exit on repeated failures instead
> +		 * of spinning forever.
> +		 */
> +		if (called_exit)
> +			goto free_mem_map;
> +
> +		called_exit = true;

Why a single retry is having reasonable guarantees to work when the
original one failed (nothing prevents an event handler to do another
allocation the next time through).

Why not retry 3, 4, 5 or even unlimited times?

> +		goto get_map;
> +	}
>  
>  	/* Historic? */
>  	boot_params->alt_mem_k = 32 * 1024;


Thanks a lot!
Joey Lee

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/