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

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

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 4015 bytes --]

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.
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.

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;
+    }
 
-    status = efi_bs->ExitBootServices(ImageHandle, map_key);
     if ( EFI_ERROR(status) )
         PrintErrMesg(L"Cannot exit boot services", status);
 




[-- Attachment #2: EFI-exit-boot-services-retry.patch --]
[-- Type: text/plain, Size: 4056 bytes --]

EFI: allow retry of ExitBootServices() call

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.
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.

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;
+    }
 
-    status = efi_bs->ExitBootServices(ImageHandle, map_key);
     if ( EFI_ERROR(status) )
         PrintErrMesg(L"Cannot exit boot services", status);
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Konrad Rzeszutek Wilk]

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);
>  
> 
> 

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

[Jan Beulich]

>>> On 14.11.14 at 16:32, <konrad.wilk@oracle.com> wrote:
> On Fri, Nov 14, 2014 at 12:37:30PM +0000, Jan Beulich wrote:
>> @@ -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?

For one, there's no reason to do it more than twice. As said in the
patch description, even doing the first retry is already going beyond
what the current specification requires us to do. And just to make
this clear here too - when I first wrote this EFI boot code, I was
aware of the retry being done elsewhere, and I intentionally
decided against doing so as not being mandated by the spec. I'm
continuing to take the position that we shouldn't give firmware
writers more leeway than we absolutely have to, otherwise they
will never get their stuff conform to the spec.

And then, there's no printk() available at this point. And we mustn't
call boot services anymore either. So there's no way to get anything
out to the user.

Jan

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

[Roy Franz]

On Fri, Nov 14, 2014 at 7:37 AM, Jan Beulich <JBeulich@suse.com> 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.
> 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.
>
> 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>

Reviewed-by: Roy Franz <roy.franz@linaro.org>

I built and tested this, however since my firmware is not triggering the retry
the interesting case is not happening.  I agree with your analysis that
repeatedly calling  fdt_add_uefi_nodes() multiple times should be fine.

Roy

>
> --- 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;
> +    }
>
> -    status = efi_bs->ExitBootServices(ImageHandle, map_key);
>      if ( EFI_ERROR(status) )
>          PrintErrMesg(L"Cannot exit boot services", status);
>
>
>
>

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

[Jan Beulich]

>>> On 14.11.14 at 16:32, <konrad.wilk@oracle.com> wrote:
> 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.

So what are your thoughts then regarding this patch going into 4.5?
And for the LIST_POISON* override one?

Jan

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

[Konrad Rzeszutek Wilk]

On Mon, Nov 17, 2014 at 01:35:17PM +0000, Jan Beulich wrote:
> >>> On 14.11.14 at 16:32, <konrad.wilk@oracle.com> wrote:
> > 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.
> 
> So what are your thoughts then regarding this patch going into 4.5?

Definitly should go in - and I would even say backport to earlier
versions of Xen.

> And for the LIST_POISON* override one?

Yes as well please.
> 
> Jan
> 

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

[Jan Beulich]

>>> On 17.11.14 at 14:49, <konrad.wilk@oracle.com> wrote:
> On Mon, Nov 17, 2014 at 01:35:17PM +0000, Jan Beulich wrote:
>> >>> On 14.11.14 at 16:32, <konrad.wilk@oracle.com> wrote:
>> > 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.
>> 
>> So what are your thoughts then regarding this patch going into 4.5?
> 
> Definitly should go in - and I would even say backport to earlier
> versions of Xen.

Sure, backporting was planned anyway.

Jan