| Message ID | 20170624153930.3546-1-xypron.glpk@gmx.de |
|---|---|
| State | Superseded |
| Delegated to: | Alexander Graf |
| Headers | show |
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@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. > + 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()? Alex > +#endif > }; > > #define DEVICE_PATH_GUID \
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@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
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@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
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, + 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; +#endif }; #define DEVICE_PATH_GUID \
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@gmx.de> --- cmd/bootefi.c | 18 ++++++++++++++++-- include/efi_api.h | 4 ++++ 2 files changed, 20 insertions(+), 2 deletions(-)