[PATCH v4] efi: Avoid calling boot services after ExitBootServices()

[PATCH v4] 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 halting in blexit().

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---

* Separated halt into an arch hook.
* Applies on top of the first patch from v3.
* Tested on x86, not sure if the ARM version is correct.

 xen/arch/arm/efi/efi-boot.h |  5 +++++
 xen/arch/x86/efi/efi-boot.h |  7 +++++++
 xen/common/efi/boot.c       | 13 ++++++++++---
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 3297f27..47efdfc 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -522,6 +522,11 @@ static void __init efi_arch_blexit(void)
         efi_bs->FreePool(memmap);
 }
 
+static void __init efi_arch_halt(void)
+{
+    stop_cpu();
+}
+
 static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image)
 {
     if ( (unsigned long)loaded_image->ImageBase & ((1 << 12) - 1) )
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index cd14c19..9f41793 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -614,6 +614,13 @@ static void __init efi_arch_blexit(void)
         efi_bs->FreePages(ucode.addr, PFN_UP(ucode.size));
 }
 
+static void __init efi_arch_halt(void)
+{
+    local_irq_disable();
+    for ( ; ; )
+        halt();
+}
+
 static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image)
 {
     xen_phys_start = (UINTN)loaded_image->ImageBase;
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 60c1b8d..4b816f2 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -216,6 +216,9 @@ static void __init noreturn blexit(const CHAR16 *str)
         PrintStr((CHAR16 *)str);
     PrintStr(newline);
 
+    if ( !efi_bs )
+        efi_arch_halt();
+
     if ( cfg.addr )
         efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
     if ( kernel.addr )
@@ -1063,8 +1066,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 +1078,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 v4] efi: Avoid calling boot services after ExitBootServices()

[Ross Lagerwall]

ping

On 06/02/2015 10:38 AM, Ross Lagerwall wrote:
> After the first call to ExitBootServices(), avoid calling any boot
> services by setting setting efi_bs to NULL and halting in blexit().
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>
> * Separated halt into an arch hook.
> * Applies on top of the first patch from v3.
> * Tested on x86, not sure if the ARM version is correct.
>
>   xen/arch/arm/efi/efi-boot.h |  5 +++++
>   xen/arch/x86/efi/efi-boot.h |  7 +++++++
>   xen/common/efi/boot.c       | 13 ++++++++++---
>   3 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index 3297f27..47efdfc 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -522,6 +522,11 @@ static void __init efi_arch_blexit(void)
>           efi_bs->FreePool(memmap);
>   }
>
> +static void __init efi_arch_halt(void)
> +{
> +    stop_cpu();
> +}
> +
>   static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image)
>   {
>       if ( (unsigned long)loaded_image->ImageBase & ((1 << 12) - 1) )
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index cd14c19..9f41793 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -614,6 +614,13 @@ static void __init efi_arch_blexit(void)
>           efi_bs->FreePages(ucode.addr, PFN_UP(ucode.size));
>   }
>
> +static void __init efi_arch_halt(void)
> +{
> +    local_irq_disable();
> +    for ( ; ; )
> +        halt();
> +}
> +
>   static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image)
>   {
>       xen_phys_start = (UINTN)loaded_image->ImageBase;
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 60c1b8d..4b816f2 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -216,6 +216,9 @@ static void __init noreturn blexit(const CHAR16 *str)
>           PrintStr((CHAR16 *)str);
>       PrintStr(newline);
>
> +    if ( !efi_bs )
> +        efi_arch_halt();
> +
>       if ( cfg.addr )
>           efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
>       if ( kernel.addr )
> @@ -1063,8 +1066,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 +1078,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 v4] efi: Avoid calling boot services after ExitBootServices()

[Jan Beulich]

>>> On 09.06.15 at 12:36, <ross.lagerwall@citrix.com> wrote:
> ping

I'm still waiting for the ARM maintainers to ack ...

>> --- a/xen/arch/arm/efi/efi-boot.h
>> +++ b/xen/arch/arm/efi/efi-boot.h
>> @@ -522,6 +522,11 @@ static void __init efi_arch_blexit(void)
>>           efi_bs->FreePool(memmap);
>>   }
>>
>> +static void __init efi_arch_halt(void)
>> +{
>> +    stop_cpu();
>> +}
>> +
>>   static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image)
>>   {
>>       if ( (unsigned long)loaded_image->ImageBase & ((1 << 12) - 1) )

... this.

Jan

Re: [PATCH v4] efi: Avoid calling boot services after ExitBootServices()

[Ian Campbell]

On Tue, 2015-06-09 at 12:51 +0100, Jan Beulich wrote:
> >>> On 09.06.15 at 12:36, <ross.lagerwall@citrix.com> wrote:
> > ping
> 
> I'm still waiting for the ARM maintainers to ack ...
> 
> >> --- a/xen/arch/arm/efi/efi-boot.h
> >> +++ b/xen/arch/arm/efi/efi-boot.h
> >> @@ -522,6 +522,11 @@ static void __init efi_arch_blexit(void)
> >>           efi_bs->FreePool(memmap);
> >>   }
> >>
> >> +static void __init efi_arch_halt(void)
> >> +{
> >> +    stop_cpu();
> >> +}
> >> +
> >>   static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image)
> >>   {
> >>       if ( (unsigned long)loaded_image->ImageBase & ((1 << 12) - 1) )
> 
> ... this.

I think it is ok, it does a bit more than the x86 variant, which is that
it would interlock correctly with __cpu_die, where the open coded on x86
wouldn't (assuming you have any interlock anyway.

In terms of the rest of the patch, are those two places changed to use
SystemTable->BootServices instead of efi_bs, if the point of the patch
is to not use BootServices after exit is called?



> 
> Jan
> 

Re: [PATCH v4] efi: Avoid calling boot services after ExitBootServices()

[Ross Lagerwall]

On 06/09/2015 01:53 PM, Ian Campbell wrote:
> On Tue, 2015-06-09 at 12:51 +0100, Jan Beulich wrote:
>>>>> On 09.06.15 at 12:36, <ross.lagerwall@citrix.com> wrote:
>>> ping
>>
>> I'm still waiting for the ARM maintainers to ack ...
>>
>>>> --- a/xen/arch/arm/efi/efi-boot.h
>>>> +++ b/xen/arch/arm/efi/efi-boot.h
>>>> @@ -522,6 +522,11 @@ static void __init efi_arch_blexit(void)
>>>>            efi_bs->FreePool(memmap);
>>>>    }
>>>>
>>>> +static void __init efi_arch_halt(void)
>>>> +{
>>>> +    stop_cpu();
>>>> +}
>>>> +
>>>>    static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image)
>>>>    {
>>>>        if ( (unsigned long)loaded_image->ImageBase & ((1 << 12) - 1) )
>>
>> ... this.
>
> I think it is ok, it does a bit more than the x86 variant, which is that
> it would interlock correctly with __cpu_die, where the open coded on x86
> wouldn't (assuming you have any interlock anyway.
>
> In terms of the rest of the patch, are those two places changed to use
> SystemTable->BootServices instead of efi_bs, if the point of the patch
> is to not use BootServices after exit is called?
>

GetMemoryMap() and ExitBootServices() are the only functions explicitly 
allowed after the first call to ExitBootServices() so they are called 
via SystemTable->BootServices. efi_bs is set to NULL to ensure that no 
other bootservices are called.

Cheers,
-- 
Ross Lagerwall

Re: [PATCH v4] efi: Avoid calling boot services after ExitBootServices()

[Ian Campbell]

On Tue, 2015-06-09 at 14:05 +0100, Ross Lagerwall wrote:
> On 06/09/2015 01:53 PM, Ian Campbell wrote:
> > On Tue, 2015-06-09 at 12:51 +0100, Jan Beulich wrote:
> >>>>> On 09.06.15 at 12:36, <ross.lagerwall@citrix.com> wrote:
> >>> ping
> >>
> >> I'm still waiting for the ARM maintainers to ack ...
> >>
> >>>> --- a/xen/arch/arm/efi/efi-boot.h
> >>>> +++ b/xen/arch/arm/efi/efi-boot.h
> >>>> @@ -522,6 +522,11 @@ static void __init efi_arch_blexit(void)
> >>>>            efi_bs->FreePool(memmap);
> >>>>    }
> >>>>
> >>>> +static void __init efi_arch_halt(void)
> >>>> +{
> >>>> +    stop_cpu();
> >>>> +}
> >>>> +
> >>>>    static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image)
> >>>>    {
> >>>>        if ( (unsigned long)loaded_image->ImageBase & ((1 << 12) - 1) )
> >>
> >> ... this.
> >
> > I think it is ok, it does a bit more than the x86 variant, which is that
> > it would interlock correctly with __cpu_die, where the open coded on x86
> > wouldn't (assuming you have any interlock anyway.
> >
> > In terms of the rest of the patch, are those two places changed to use
> > SystemTable->BootServices instead of efi_bs, if the point of the patch
> > is to not use BootServices after exit is called?
> >
> 
> GetMemoryMap() and ExitBootServices() are the only functions explicitly 
> allowed after the first call to ExitBootServices() so they are called 
> via SystemTable->BootServices. efi_bs is set to NULL to ensure that no 
> other bootservices are called.

I see. That might be worth a mention in the commit log?

Re: [PATCH v4] efi: Avoid calling boot services after ExitBootServices()

[Andrew Cooper]

On 09/06/15 14:24, Ian Campbell wrote:
> On Tue, 2015-06-09 at 14:05 +0100, Ross Lagerwall wrote:
>> On 06/09/2015 01:53 PM, Ian Campbell wrote:
>>> On Tue, 2015-06-09 at 12:51 +0100, Jan Beulich wrote:
>>>>>>> On 09.06.15 at 12:36, <ross.lagerwall@citrix.com> wrote:
>>>>> ping
>>>> I'm still waiting for the ARM maintainers to ack ...
>>>>
>>>>>> --- a/xen/arch/arm/efi/efi-boot.h
>>>>>> +++ b/xen/arch/arm/efi/efi-boot.h
>>>>>> @@ -522,6 +522,11 @@ static void __init efi_arch_blexit(void)
>>>>>>            efi_bs->FreePool(memmap);
>>>>>>    }
>>>>>>
>>>>>> +static void __init efi_arch_halt(void)
>>>>>> +{
>>>>>> +    stop_cpu();
>>>>>> +}
>>>>>> +
>>>>>>    static void __init efi_arch_load_addr_check(EFI_LOADED_IMAGE *loaded_image)
>>>>>>    {
>>>>>>        if ( (unsigned long)loaded_image->ImageBase & ((1 << 12) - 1) )
>>>> ... this.
>>> I think it is ok, it does a bit more than the x86 variant, which is that
>>> it would interlock correctly with __cpu_die, where the open coded on x86
>>> wouldn't (assuming you have any interlock anyway.
>>>
>>> In terms of the rest of the patch, are those two places changed to use
>>> SystemTable->BootServices instead of efi_bs, if the point of the patch
>>> is to not use BootServices after exit is called?
>>>
>> GetMemoryMap() and ExitBootServices() are the only functions explicitly 
>> allowed after the first call to ExitBootServices() so they are called 
>> via SystemTable->BootServices. efi_bs is set to NULL to ensure that no 
>> other bootservices are called.
> I see. That might be worth a mention in the commit log?

The commit message does say this.  Is there anything you think needs
further clarification?

~Andrew

Re: [PATCH v4] efi: Avoid calling boot services after ExitBootServices()

[Ian Campbell]

On Tue, 2015-06-09 at 14:26 +0100, Andrew Cooper wrote:
> On 09/06/15 14:24, Ian Campbell wrote:
> > On Tue, 2015-06-09 at 14:05 +0100, Ross Lagerwall wrote:
> >> On 06/09/2015 01:53 PM, Ian Campbell wrote:
> >>> In terms of the rest of the patch, are those two places changed to use
> >>> SystemTable->BootServices instead of efi_bs, if the point of the patch
> >>> is to not use BootServices after exit is called?
> >>>
> >> GetMemoryMap() and ExitBootServices() are the only functions explicitly 
> >> allowed after the first call to ExitBootServices() so they are called 
> >> via SystemTable->BootServices. efi_bs is set to NULL to ensure that no 
> >> other bootservices are called.
> > I see. That might be worth a mention in the commit log?
> 
> The commit message does say this.  Is there anything you think needs
> further clarification?

It says just:
        After the first call to ExitBootServices(), avoid calling any
        boot
        services by setting setting efi_bs to NULL and halting in
        blexit().

No mention of the fact that GetMemoryMap() and ExitBootServices() are
not covered by the blanket statement regarding boot services.

Ian.