[PATCH v3 1/2] efi: Fix allocation problems if ExitBootServices() fails

[PATCH v3 1/2] efi: Fix allocation problems if ExitBootServices() fails

[Ross Lagerwall]

If calling ExitBootServices() fails, the required memory map size may
have increased. When initially allocating the memory map, allocate a
slightly larger buffer (by an arbitrary 8 entries) to fix this.

The ARM code path was already allocating a larger buffer than required,
so this moves the code to be common for all architectures.

This was seen on the following machine when using the iscsidxe UEFI
driver. The machine would consistently fail the first call to
ExitBootServices().
System Information
        Manufacturer: Supermicro
        Product Name: X10SLE-F/HF
BIOS Information
        Vendor: American Megatrends Inc.
        Version: 2.00
        Release Date: 04/24/2014

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/arm/efi/efi-boot.h | 6 ++----
 xen/arch/x86/efi/efi-boot.h | 4 ++--
 xen/common/efi/boot.c       | 8 +++++---
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index b02cc02..3297f27 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -370,16 +370,14 @@ static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *sect
 {
 }
 
-static void *__init efi_arch_allocate_mmap_buffer(UINTN *map_size)
+static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size)
 {
     void *ptr;
     EFI_STATUS status;
-    UINTN map_size_alloc = *map_size + EFI_PAGE_SIZE;
 
-    status = efi_bs->AllocatePool(EfiLoaderData, map_size_alloc, &ptr);
+    status = efi_bs->AllocatePool(EfiLoaderData, map_size, &ptr);
     if ( status != EFI_SUCCESS )
         return NULL;
-    *map_size = map_size_alloc;
     return ptr;
 }
 
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 3a3b4fe..cd14c19 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -190,10 +190,10 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
 
 }
 
-static void *__init efi_arch_allocate_mmap_buffer(UINTN *map_size)
+static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size)
 {
     place_string(&mbi.mem_upper, NULL);
-    mbi.mem_upper -= *map_size;
+    mbi.mem_upper -= map_size;
     mbi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR);
     if ( mbi.mem_upper < xen_phys_start )
         return NULL;
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index ef8476c..60c1b8d 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -700,7 +700,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     EFI_STATUS status;
     unsigned int i, argc;
     CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL;
-    UINTN map_key, info_size, gop_mode = ~0;
+    UINTN map_alloc_size, map_key, info_size, gop_mode = ~0;
     EFI_HANDLE *handles = NULL;
     EFI_SHIM_LOCK_PROTOCOL *shim_lock;
     EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
@@ -1053,14 +1053,16 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
             efi_arch_video_init(gop, info_size, mode_info);
     }
 
-    efi_bs->GetMemoryMap(&efi_memmap_size, NULL, &map_key,
+    efi_bs->GetMemoryMap(&map_alloc_size, NULL, &map_key,
                          &efi_mdesc_size, &mdesc_ver);
-    efi_memmap = efi_arch_allocate_mmap_buffer(&efi_memmap_size);
+    map_alloc_size += 8 * efi_mdesc_size;
+    efi_memmap = efi_arch_allocate_mmap_buffer(map_alloc_size);
     if ( !efi_memmap )
         blexit(L"Unable to allocate memory for EFI memory map");
 
     for ( retry = 0; ; retry = 1 )
     {
+        efi_memmap_size = map_alloc_size;
         status = efi_bs->GetMemoryMap(&efi_memmap_size, efi_memmap, &map_key,
                                       &efi_mdesc_size, &mdesc_ver);
         if ( EFI_ERROR(status) )
-- 
2.1.0

[PATCH v3 2/2] efi: Avoid calling boot services after ExitBootServices()

[Ross Lagerwall]

After the first call to ExitBootServices(), avoid calling any boot
services by setting setting efi_bs to NULL and entering an infinite loop
in blexit().

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/common/efi/boot.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 60c1b8d..5c5c158 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -216,6 +216,12 @@ static void __init noreturn blexit(const CHAR16 *str)
         PrintStr((CHAR16 *)str);
     PrintStr(newline);
 
+    if ( !efi_bs )
+    {
+        for ( ; ; )
+            ;
+    }
+
     if ( cfg.addr )
         efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
     if ( kernel.addr )
@@ -1063,8 +1069,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     for ( retry = 0; ; retry = 1 )
     {
         efi_memmap_size = map_alloc_size;
-        status = efi_bs->GetMemoryMap(&efi_memmap_size, efi_memmap, &map_key,
-                                      &efi_mdesc_size, &mdesc_ver);
+        status = SystemTable->BootServices->GetMemoryMap(&efi_memmap_size,
+                                                         efi_memmap, &map_key,
+                                                         &efi_mdesc_size,
+                                                         &mdesc_ver);
         if ( EFI_ERROR(status) )
             PrintErrMesg(L"Cannot obtain memory map", status);
 
@@ -1073,7 +1081,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
 
         efi_arch_pre_exit_boot();
 
-        status = efi_bs->ExitBootServices(ImageHandle, map_key);
+        status = SystemTable->BootServices->ExitBootServices(ImageHandle,
+                                                             map_key);
+        efi_bs = NULL;
         if ( status != EFI_INVALID_PARAMETER || retry )
             break;
     }
-- 
2.1.0

Re: [PATCH v3 2/2] efi: Avoid calling boot services after ExitBootServices()

[Andrew Cooper]

On 01/06/15 11:17, Ross Lagerwall wrote:
> After the first call to ExitBootServices(), avoid calling any boot
> services by setting setting efi_bs to NULL and entering an infinite loop
> in blexit().
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  xen/common/efi/boot.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 60c1b8d..5c5c158 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -216,6 +216,12 @@ static void __init noreturn blexit(const CHAR16 *str)
>          PrintStr((CHAR16 *)str);
>      PrintStr(newline);
>  
> +    if ( !efi_bs )
> +    {
> +        for ( ; ; )
> +            ;

At the very least this should be halt() to avoid spinning in a busy
loop, and probably with a local_irq_disable() ahead of the for.

~Andrew

> +    }
> +
>      if ( cfg.addr )
>          efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
>      if ( kernel.addr )
> @@ -1063,8 +1069,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>      for ( retry = 0; ; retry = 1 )
>      {
>          efi_memmap_size = map_alloc_size;
> -        status = efi_bs->GetMemoryMap(&efi_memmap_size, efi_memmap, &map_key,
> -                                      &efi_mdesc_size, &mdesc_ver);
> +        status = SystemTable->BootServices->GetMemoryMap(&efi_memmap_size,
> +                                                         efi_memmap, &map_key,
> +                                                         &efi_mdesc_size,
> +                                                         &mdesc_ver);
>          if ( EFI_ERROR(status) )
>              PrintErrMesg(L"Cannot obtain memory map", status);
>  
> @@ -1073,7 +1081,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>  
>          efi_arch_pre_exit_boot();
>  
> -        status = efi_bs->ExitBootServices(ImageHandle, map_key);
> +        status = SystemTable->BootServices->ExitBootServices(ImageHandle,
> +                                                             map_key);
> +        efi_bs = NULL;
>          if ( status != EFI_INVALID_PARAMETER || retry )
>              break;
>      }

Re: [PATCH v3 2/2] efi: Avoid calling boot services after ExitBootServices()

[Jan Beulich]

>>> On 01.06.15 at 12:26, <andrew.cooper3@citrix.com> wrote:
> On 01/06/15 11:17, Ross Lagerwall wrote:
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -216,6 +216,12 @@ static void __init noreturn blexit(const CHAR16 *str)
>>          PrintStr((CHAR16 *)str);
>>      PrintStr(newline);
>>  
>> +    if ( !efi_bs )
>> +    {
>> +        for ( ; ; )
>> +            ;
> 
> At the very least this should be halt() to avoid spinning in a busy
> loop, and probably with a local_irq_disable() ahead of the for.

Suitably abstracted, yes: ARM has no halt(), and I don't think we
should assume local_irq_disable() can be used here in a completely
arch-independent fashion. I.e. perhaps the whole body of the if()
should become a new arch hook.

Jan

Re: [PATCH v3 1/2] efi: Fix allocation problems if ExitBootServices() fails

[Jan Beulich]

>>> On 01.06.15 at 12:17, <ross.lagerwall@citrix.com> wrote:
> If calling ExitBootServices() fails, the required memory map size may
> have increased. When initially allocating the memory map, allocate a
> slightly larger buffer (by an arbitrary 8 entries) to fix this.
> 
> The ARM code path was already allocating a larger buffer than required,
> so this moves the code to be common for all architectures.
> 
> This was seen on the following machine when using the iscsidxe UEFI
> driver. The machine would consistently fail the first call to
> ExitBootServices().
> System Information
>         Manufacturer: Supermicro
>         Product Name: X10SLE-F/HF
> BIOS Information
>         Vendor: American Megatrends Inc.
>         Version: 2.00
>         Release Date: 04/24/2014
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Provided ARM folks are happy with the reduced increase,
Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v3 1/2] efi: Fix allocation problems if ExitBootServices() fails

[Ian Campbell]

On Mon, 2015-06-01 at 12:10 +0100, Jan Beulich wrote:
> >>> On 01.06.15 at 12:17, <ross.lagerwall@citrix.com> wrote:
> > If calling ExitBootServices() fails, the required memory map size may
> > have increased. When initially allocating the memory map, allocate a
> > slightly larger buffer (by an arbitrary 8 entries) to fix this.
> > 
> > The ARM code path was already allocating a larger buffer than required,
> > so this moves the code to be common for all architectures.
> > 
> > This was seen on the following machine when using the iscsidxe UEFI
> > driver. The machine would consistently fail the first call to
> > ExitBootServices().
> > System Information
> >         Manufacturer: Supermicro
> >         Product Name: X10SLE-F/HF
> > BIOS Information
> >         Vendor: American Megatrends Inc.
> >         Version: 2.00
> >         Release Date: 04/24/2014
> > 
> > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> Provided ARM folks are happy with the reduced increase,

Hi Roy,

This patch[0] turns a +PAGE_SIZE in efi_arch_allocate_mmap_buffer into a
"8 * efi_mdesc_size" in the common code.

The +PAGE_SIZE came from [1] so I think it is as arbitrary as the
+8*sizeof here.

IOW this change looks ok to me, what do you think?

Thanks,
Ian.

[0] http://lists.xen.org/archives/html/xen-devel/2015-06/msg00067.html
[1] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=932881d953476444ed934c94dbc098c0fefb4d77

> Acked-by: Jan Beulich <jbeulich@suse.com>
> 

Re: [PATCH v3 1/2] efi: Fix allocation problems if ExitBootServices() fails

[Roy Franz]

On Mon, Jun 1, 2015 at 4:24 AM, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Mon, 2015-06-01 at 12:10 +0100, Jan Beulich wrote:
>> >>> On 01.06.15 at 12:17, <ross.lagerwall@citrix.com> wrote:
>> > If calling ExitBootServices() fails, the required memory map size may
>> > have increased. When initially allocating the memory map, allocate a
>> > slightly larger buffer (by an arbitrary 8 entries) to fix this.
>> >
>> > The ARM code path was already allocating a larger buffer than required,
>> > so this moves the code to be common for all architectures.
>> >
>> > This was seen on the following machine when using the iscsidxe UEFI
>> > driver. The machine would consistently fail the first call to
>> > ExitBootServices().
>> > System Information
>> >         Manufacturer: Supermicro
>> >         Product Name: X10SLE-F/HF
>> > BIOS Information
>> >         Vendor: American Megatrends Inc.
>> >         Version: 2.00
>> >         Release Date: 04/24/2014
>> >
>> > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>>
>> Provided ARM folks are happy with the reduced increase,
>
> Hi Roy,
>
> This patch[0] turns a +PAGE_SIZE in efi_arch_allocate_mmap_buffer into a
> "8 * efi_mdesc_size" in the common code.
>
> The +PAGE_SIZE came from [1] so I think it is as arbitrary as the
> +8*sizeof here.
>
> IOW this change looks ok to me, what do you think?

Yeah, this should be fine.  Most EFI allocations have page-size
granularity within the firmware,
so there wasn't much point doing something smaller.  I haven't
actually used firmware that
changed the memmap size on ExitBootServices, so that size was not
based on any actual
firmware's behavior.  The x86 allocations are done differently and are
more size constrained,
so a smaller value should be fine for common code.

Roy

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



>
> Thanks,
> Ian.
>
> [0] http://lists.xen.org/archives/html/xen-devel/2015-06/msg00067.html
> [1] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=932881d953476444ed934c94dbc098c0fefb4d77
>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>
>
>

Re: [PATCH v3 1/2] efi: Fix allocation problems if ExitBootServices() fails

[Ian Campbell]

On Mon, 2015-06-01 at 14:20 -0700, Roy Franz wrote:
> On Mon, Jun 1, 2015 at 4:24 AM, Ian Campbell <ian.campbell@citrix.com> wrote:
> > On Mon, 2015-06-01 at 12:10 +0100, Jan Beulich wrote:
> >> >>> On 01.06.15 at 12:17, <ross.lagerwall@citrix.com> wrote:
> >> > If calling ExitBootServices() fails, the required memory map size may
> >> > have increased. When initially allocating the memory map, allocate a
> >> > slightly larger buffer (by an arbitrary 8 entries) to fix this.
> >> >
> >> > The ARM code path was already allocating a larger buffer than required,
> >> > so this moves the code to be common for all architectures.
> >> >
> >> > This was seen on the following machine when using the iscsidxe UEFI
> >> > driver. The machine would consistently fail the first call to
> >> > ExitBootServices().
> >> > System Information
> >> >         Manufacturer: Supermicro
> >> >         Product Name: X10SLE-F/HF
> >> > BIOS Information
> >> >         Vendor: American Megatrends Inc.
> >> >         Version: 2.00
> >> >         Release Date: 04/24/2014
> >> >
> >> > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> >>
> >> Provided ARM folks are happy with the reduced increase,
> >
> > Hi Roy,
> >
> > This patch[0] turns a +PAGE_SIZE in efi_arch_allocate_mmap_buffer into a
> > "8 * efi_mdesc_size" in the common code.
> >
> > The +PAGE_SIZE came from [1] so I think it is as arbitrary as the
> > +8*sizeof here.
> >
> > IOW this change looks ok to me, what do you think?
> 
> Yeah, this should be fine.  Most EFI allocations have page-size
> granularity within the firmware,
> so there wasn't much point doing something smaller.  I haven't
> actually used firmware that
> changed the memmap size on ExitBootServices, so that size was not
> based on any actual
> firmware's behavior.  The x86 allocations are done differently and are
> more size constrained,
> so a smaller value should be fine for common code.
> 
> Roy
> 
> Reviewed-by: Roy Franz <roy.franz@linaro.org>

Thanks. On that basis:

Acked-by: Ian Campbell <ian.campbell@citrix.com>