Re: [PATCH RFC] EFI: allow retry of ExitBootServices() call

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Nov 14, 2014 at 12:37:30PM +0000, Jan Beulich wrote:
> The specification is kind of vague under what conditions
> ExitBootServices() may legitimately fail, requiring the OS loader to
> retry:
> 
> "If MapKey value is incorrect, ExitBootServices() returns
>  EFI_INVALID_PARAMETER and GetMemoryMap() with ExitBootServices() must
>  be called again. Firmware implementation may choose to do a partial
>  shutdown of the boot services during the first call to
>  ExitBootServices(). EFI OS loader should not make calls to any boot
>  service function other then GetMemoryMap() after the first call to
>  ExitBootServices()."
> 
> While our code guarantees the map key to be valid, there are systems
> where a firmware internal notification sent while processing
> ExitBootServices() reportedly results in changes to the memory map.

s/reportedly/in fact/
> In that case, make a best effort second try: Avoid any boot service
> calls other than the two named above, with the possible exception of
> error paths. Those aren't a problem, since if we end up needing to
> retry, we're hosed when something goes wrong as much as if we didn't
> make the retry attempt.
> 
> For x86, a minimal adjustment to efi_arch_process_memory_map() is
> needed for it to cope with potentially being called a second time.

Wow. Talk about timing. We saw this and were going to see
doing something similar.


> 
> For arm64, while efi_process_memory_map_bootinfo() is easy to verify
> that it can safely be called more than once without violating spec
> constraints, it's not so obvious for fdt_add_uefi_nodes(), hence a
> step by step approach:
> - deletion of memory nodes and memory reserve map entries: the 2nd pass
>   shouldn't find any as the 1st one deleted them all,
> - a "chosen" node should be found as it got added in the 1st pass,
> - the various "linux,uefi-*" nodes all got added during the 1st pass
>   and hence only their contents may get updated.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -140,7 +140,7 @@ static void __init efi_arch_process_memo
>  
>      /* Populate E820 table and check trampoline area availability. */
>      e = e820map - 1;
> -    for ( i = 0; i < map_size; i += desc_size )
> +    for ( e820nr = i = 0; i < map_size; i += desc_size )
>      {
>          EFI_MEMORY_DESCRIPTOR *desc = map + i;
>          u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -703,7 +703,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
>      EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
>      EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info;
>      union string section = { NULL }, name;
> -    bool_t base_video = 0;
> +    bool_t base_video = 0, retry;
>      char *option_str;
>      bool_t use_cfg_file;
>  
> @@ -1051,17 +1051,23 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
>      if ( !efi_memmap )
>          blexit(L"Unable to allocate memory for EFI memory map");
>  
> -    status = efi_bs->GetMemoryMap(&efi_memmap_size, efi_memmap, &map_key,
> -                                  &efi_mdesc_size, &mdesc_ver);
> -    if ( EFI_ERROR(status) )
> -        PrintErrMesg(L"Cannot obtain memory map", status);
> +    for ( retry = 0; ; retry = 1 )
> +    {
> +        status = efi_bs->GetMemoryMap(&efi_memmap_size, efi_memmap, &map_key,
> +                                      &efi_mdesc_size, &mdesc_ver);
> +        if ( EFI_ERROR(status) )
> +            PrintErrMesg(L"Cannot obtain memory map", status);
>  
> -    efi_arch_process_memory_map(SystemTable, efi_memmap, efi_memmap_size,
> -                                efi_mdesc_size, mdesc_ver);
> +        efi_arch_process_memory_map(SystemTable, efi_memmap, efi_memmap_size,
> +                                    efi_mdesc_size, mdesc_ver);
>  
> -    efi_arch_pre_exit_boot();
> +        efi_arch_pre_exit_boot();
> +
> +        status = efi_bs->ExitBootServices(ImageHandle, map_key);
> +        if ( status != EFI_INVALID_PARAMETER || retry )
> +            break;
> +    }

Any reason for just doing the loop at max twice? Could we iterate
more than those (say forever?) with an printk at suitable intervals
to notify the user?

>  
> -    status = efi_bs->ExitBootServices(ImageHandle, map_key);
>      if ( EFI_ERROR(status) )
>          PrintErrMesg(L"Cannot exit boot services", status);
>  
> 
>