Message ID | 20211010214857.871670-1-marek.vasut@gmail.com |
---|---|
State | Changes Requested, archived |
Delegated to: | Heinrich Schuchardt |
Headers | show |
Series | efi_loader: Handle GD_FLG_SKIP_RELOC | expand |
On 10/10/21 23:48, marek.vasut@gmail.com wrote: > From: Marek Vasut <marek.vasut+renesas@gmail.com> > > In case U-Boot starts with GD_FLG_SKIP_RELOC, the efi loader > relocation code breaks down because it assumes gd->relocaddr > points to relocated U-Boot code, which is not the case. Add > special case for handling GD_FLG_SKIP_RELOC, which uses the > __image_copy_start instead of gd->relocaddr for efi loader > code relocation source address. GD_FLG_SKIP_RELOC is only to be used by the x86 EFI app which is incompatible with CONFIG_EFI_LOADER=y. lib/efi/efi_app.c:131: board_init_f(GD_FLG_SKIP_RELOC); Why do we need this patch? > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> > Cc: Simon Glass <sjg@chromium.org> > Cc: Tom Rini <trini@konsulko.com> > --- > common/board_r.c | 7 ++++++- > lib/efi_loader/efi_runtime.c | 19 ++++++++++++++++--- > 2 files changed, 22 insertions(+), 4 deletions(-) > > diff --git a/common/board_r.c b/common/board_r.c > index 31a59c585a..3e95123f95 100644 > --- a/common/board_r.c > +++ b/common/board_r.c > @@ -178,7 +178,12 @@ static int initr_reloc_global_data(void) > */ > efi_save_gd(); > > - efi_runtime_relocate(gd->relocaddr, NULL); > +#ifdef CONFIG_ARM Why should this be ARM specific? Best regards Heinrich > + if (gd->flags & GD_FLG_SKIP_RELOC) > + efi_runtime_relocate((ulong)__image_copy_start, NULL); > + else > +#endif > + efi_runtime_relocate(gd->relocaddr, NULL); > #endif > > return 0; > diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c > index 93a695fc27..876ca09106 100644 > --- a/lib/efi_loader/efi_runtime.c > +++ b/lib/efi_loader/efi_runtime.c > @@ -17,6 +17,8 @@ > #include <asm/global_data.h> > #include <u-boot/crc.h> > > +#include <asm/sections.h> > + > /* For manual relocation support */ > DECLARE_GLOBAL_DATA_PTR; > > @@ -629,13 +631,23 @@ out: > return ret; > } > > +static unsigned long efi_get_reloc_start(void) > +{ > +#ifdef CONFIG_ARM > + if (gd->flags & GD_FLG_SKIP_RELOC) > + return (unsigned long)__image_copy_start; > + else > +#endif > + return gd->relocaddr; > +} > + > static __efi_runtime void efi_relocate_runtime_table(ulong offset) > { > ulong patchoff; > void **pos; > > /* Relocate the runtime services pointers */ > - patchoff = offset - gd->relocaddr; > + patchoff = offset - efi_get_reloc_start(); > for (pos = (void **)&efi_runtime_services.get_time; > pos <= (void **)&efi_runtime_services.query_variable_info; ++pos) { > if (*pos) > @@ -681,7 +693,7 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map) > ulong *p; > ulong newaddr; > > - p = (void*)((ulong)rel->offset - base) + gd->relocaddr; > + p = (void*)((ulong)rel->offset - base) + efi_get_reloc_start(); > > /* > * The runtime services table is updated in > @@ -852,7 +864,8 @@ static efi_status_t EFIAPI efi_set_virtual_address_map( > map = (void*)virtmap + (descriptor_size * i); > if (map->type == EFI_RUNTIME_SERVICES_CODE) { > ulong new_offset = map->virtual_start - > - map->physical_start + gd->relocaddr; > + map->physical_start + > + efi_get_reloc_start(); > > efi_relocate_runtime_table(new_offset); > efi_runtime_relocate(new_offset, map); >
On 10/11/21 1:36 PM, Heinrich Schuchardt wrote: Hi, >> In case U-Boot starts with GD_FLG_SKIP_RELOC, the efi loader >> relocation code breaks down because it assumes gd->relocaddr >> points to relocated U-Boot code, which is not the case. Add >> special case for handling GD_FLG_SKIP_RELOC, which uses the >> __image_copy_start instead of gd->relocaddr for efi loader >> code relocation source address. > > GD_FLG_SKIP_RELOC is only to be used by the x86 EFI app which is > incompatible with CONFIG_EFI_LOADER=y. > lib/efi/efi_app.c:131: board_init_f(GD_FLG_SKIP_RELOC); > > Why do we need this patch? GD_FLG_SKIP_RELOC is set when U-Boot doesn't perform relocation, it has nothing to do with EFI. That case is currently broken, try setting GD_FLG_SKIP_RELOC early on in U-Boot (somewhere early, e.g. in mach_cpu_init()) and the system crashes on board_init_r when initializing EFI.
On 10/24/21 01:03, Marek Vasut wrote: > On 10/11/21 1:36 PM, Heinrich Schuchardt wrote: > Hi, > >>> In case U-Boot starts with GD_FLG_SKIP_RELOC, the efi loader >>> relocation code breaks down because it assumes gd->relocaddr >>> points to relocated U-Boot code, which is not the case. Add >>> special case for handling GD_FLG_SKIP_RELOC, which uses the >>> __image_copy_start instead of gd->relocaddr for efi loader >>> code relocation source address. >> >> GD_FLG_SKIP_RELOC is only to be used by the x86 EFI app which is >> incompatible with CONFIG_EFI_LOADER=y. >> lib/efi/efi_app.c:131: board_init_f(GD_FLG_SKIP_RELOC); >> >> Why do we need this patch? > > GD_FLG_SKIP_RELOC is set when U-Boot doesn't perform relocation, it has > nothing to do with EFI. That case is currently broken, try setting > GD_FLG_SKIP_RELOC early on in U-Boot (somewhere early, e.g. in > mach_cpu_init()) and the system crashes on board_init_r when > initializing EFI. You missed the second question: Why should the change be ARM specific? Best regards Heinrich
On 10/25/21 8:46 PM, Heinrich Schuchardt wrote: > > > On 10/24/21 01:03, Marek Vasut wrote: >> On 10/11/21 1:36 PM, Heinrich Schuchardt wrote: >> Hi, >> >>>> In case U-Boot starts with GD_FLG_SKIP_RELOC, the efi loader >>>> relocation code breaks down because it assumes gd->relocaddr >>>> points to relocated U-Boot code, which is not the case. Add >>>> special case for handling GD_FLG_SKIP_RELOC, which uses the >>>> __image_copy_start instead of gd->relocaddr for efi loader >>>> code relocation source address. >>> >>> GD_FLG_SKIP_RELOC is only to be used by the x86 EFI app which is >>> incompatible with CONFIG_EFI_LOADER=y. >>> lib/efi/efi_app.c:131: board_init_f(GD_FLG_SKIP_RELOC); >>> >>> Why do we need this patch? >> >> GD_FLG_SKIP_RELOC is set when U-Boot doesn't perform relocation, it has >> nothing to do with EFI. That case is currently broken, try setting >> GD_FLG_SKIP_RELOC early on in U-Boot (somewhere early, e.g. in >> mach_cpu_init()) and the system crashes on board_init_r when >> initializing EFI. > > You missed the second question: > > Why should the change be ARM specific? It shouldn't, do you have a more generic idea ?
diff --git a/common/board_r.c b/common/board_r.c index 31a59c585a..3e95123f95 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -178,7 +178,12 @@ static int initr_reloc_global_data(void) */ efi_save_gd(); - efi_runtime_relocate(gd->relocaddr, NULL); +#ifdef CONFIG_ARM + if (gd->flags & GD_FLG_SKIP_RELOC) + efi_runtime_relocate((ulong)__image_copy_start, NULL); + else +#endif + efi_runtime_relocate(gd->relocaddr, NULL); #endif return 0; diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index 93a695fc27..876ca09106 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -17,6 +17,8 @@ #include <asm/global_data.h> #include <u-boot/crc.h> +#include <asm/sections.h> + /* For manual relocation support */ DECLARE_GLOBAL_DATA_PTR; @@ -629,13 +631,23 @@ out: return ret; } +static unsigned long efi_get_reloc_start(void) +{ +#ifdef CONFIG_ARM + if (gd->flags & GD_FLG_SKIP_RELOC) + return (unsigned long)__image_copy_start; + else +#endif + return gd->relocaddr; +} + static __efi_runtime void efi_relocate_runtime_table(ulong offset) { ulong patchoff; void **pos; /* Relocate the runtime services pointers */ - patchoff = offset - gd->relocaddr; + patchoff = offset - efi_get_reloc_start(); for (pos = (void **)&efi_runtime_services.get_time; pos <= (void **)&efi_runtime_services.query_variable_info; ++pos) { if (*pos) @@ -681,7 +693,7 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map) ulong *p; ulong newaddr; - p = (void*)((ulong)rel->offset - base) + gd->relocaddr; + p = (void*)((ulong)rel->offset - base) + efi_get_reloc_start(); /* * The runtime services table is updated in @@ -852,7 +864,8 @@ static efi_status_t EFIAPI efi_set_virtual_address_map( map = (void*)virtmap + (descriptor_size * i); if (map->type == EFI_RUNTIME_SERVICES_CODE) { ulong new_offset = map->virtual_start - - map->physical_start + gd->relocaddr; + map->physical_start + + efi_get_reloc_start(); efi_relocate_runtime_table(new_offset); efi_runtime_relocate(new_offset, map);