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

[Heinrich Schuchardt]

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.

I would prefer not to touch that assembly code.

Furthermore I have no board which will call armv8_switch_to_el2. So I
would not be able to test the any assembly code changes.

Using a separate static variable would not provide any advantage.

Best regards

Heinrich