[U-Boot] [PATCH 1/1] bootefi: allow return without EFI_BOOT_SERVICES.Exit

[Alexander Graf]

On 03.07.17 18:36, Heinrich Schuchardt wrote:
> On 07/03/2017 02:01 PM, Alexander Graf wrote:
>> On 06/24/2017 05:39 PM, Heinrich Schuchardt wrote:
>>> The Unified Extensible Firmware Interface Specification, version 2.7,
>>> defines in chapter 2.1.2 - UEFI Application that an EFI application may
>>> either directly return or call EFI_BOOT_SERVICES.Exit().
>>>
>>> Unfortunately U-Boot makes the incorrect assumption that
>>> EFI_BOOT_SERVICES.Exit() is always called.
>>>
>>> So the following application leads to a memory exception on the aarch64
>>> architecture when returning:
>>>
>>> EFI_STATUS efi_main(
>>>     EFI_HANDLE handle,
>>>     EFI_SYSTEM_TABlE systable) {
>>>      return EFI_SUCCESS;
>>> }
>>>
>>> With this patch the entry point is stored in the image handle.
>>>
>>> The new wrapper function do_enter is used to call the EFI entry point.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>> ---
>>>    cmd/bootefi.c     | 18 ++++++++++++++++--
>>>    include/efi_api.h |  4 ++++
>>>    2 files changed, 20 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index a0a5434967..fcb223e999 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -9,6 +9,7 @@
>>>    #include <common.h>
>>>    #include <command.h>
>>>    #include <dm.h>
>>> +#include <efi_api.h>
>>>    #include <efi_loader.h>
>>>    #include <errno.h>
>>>    #include <libfdt.h>
>>> @@ -141,6 +142,18 @@ static void *copy_fdt(void *fdt)
>>>        return new_fdt;
>>>    }
>>>    +static asmlinkage ulong do_enter(void *image_handle,
>>
>> Please call it efi_do_enter instead :). Otherwise we might get into
>> naming conflicts sooner or later.
> 
> Sure.
> 
>>
>>> +                 struct efi_system_table *st)
>>> +{
>>> +    struct efi_loaded_image *handle = image_handle;
>>> +    efi_status_t ret = EFI_LOAD_ERROR;
>>> +
>>> +    if (handle->entry)
>>> +        ret = handle->entry(image_handle, st);
>>> +    st->boottime->exit(image_handle, ret, 0 , NULL);
>>> +    return ret;
>>> +}
>>> +
>>>    #ifdef CONFIG_ARM64
>>>    static unsigned long efi_run_in_el2(ulong (*entry)(void *image_handle,
>>>            struct efi_system_table *st), void *image_handle,
>>> @@ -149,7 +162,7 @@ static unsigned long efi_run_in_el2(ulong
>>> (*entry)(void *image_handle,
>>>        /* Enable caches again */
>>>        dcache_enable();
>>>    -    return entry(image_handle, st);
>>> +    return do_enter(image_handle, st);
>>>    }
>>>    #endif
>>>    @@ -237,6 +250,7 @@ static unsigned long do_bootefi_exec(void *efi,
>>> void *fdt)
>>>            efi_status_t status = loaded_image_info.exit_status;
>>>            return status == EFI_SUCCESS ? 0 : -EINVAL;
>>>        }
>>> +    loaded_image_info.entry = entry;
>>>      #ifdef CONFIG_ARM64
>>>        /* On AArch64 we need to make sure we call our payload in < EL3 */
>>> @@ -254,7 +268,7 @@ static unsigned long do_bootefi_exec(void *efi,
>>> void *fdt)
>>>        }
>>>    #endif
>>>    -    return entry(&loaded_image_info, &systab);
>>> +    return do_enter(&loaded_image_info, &systab);
>>>    }
>>>      diff --git a/include/efi_api.h b/include/efi_api.h
>>> index 5c3836a51b..611c35c422 100644
>>> --- a/include/efi_api.h
>>> +++ b/include/efi_api.h
>>> @@ -253,6 +253,10 @@ struct efi_loaded_image {
>>>        efi_status_t exit_status;
>>>        struct jmp_buf_data exit_jmp;
>>>    #endif
>>> +#ifdef CONFIG_CMD_BOOTEFI
>>> +    ulong (*entry)(void *image_handle, struct efi_system_table *st)
>>> +        asmlinkage;
>>
>> I don't understand why we need another field in the loaded image struct
>> here. Can't we just pass entry as a parameter to do_enter()?
>>
> 
> I could not see how to pass 'entry' to do_enter() as parameter without
> changing assembly code routine armv8_switch_to_el2.

armv8_switch_to_el2 simply takes x0-x3 and passes it into the payload 
function (efi_run_in_el2 in our case). So we would have 4 parameters to 
pass.

However, efi_run_in_el2() already gets the entry pointer today:

static unsigned long efi_run_in_el2(ulong (*entry)(void *image_handle,
                 struct efi_system_table *st), void *image_handle,
                 struct efi_system_table *st)

So instead of calling

         return entry(image_handle, st);

you would just change to that

         return efi_do_enter(image_handle, st, entry);

and have the entry function pointer available in your function.


Alex