| Message ID | 20190311001643.29297-1-xypron.glpk@gmx.de |
|---|---|
| State | Superseded, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | [U-Boot,v4,1/1] efi_loader: Patch non-runtime code out at ExitBootServices already | expand |
On Mon, 11 Mar 2019 at 01:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > From: Alexander Graf <agraf@suse.de> > > While discussing something compeltely different, Ard pointed out > that it might be legal to omit calling SetVirtualAddressMap altogether. > > There is even a patch on the Linux Kernel Mailing List that implements > such behavior by now: > > https://patchwork.kernel.org/patch/10782393/ > > While that sounds great, we currently rely on the SetVirtualAddressMap > call to remove all references to code that would not work outside of > boot services. > > So let's patch out those bits already on the call to ExitBootServices, > so that we can successfully run even when an OS chooses to omit > any call to SetVirtualAddressMap. > > Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Signed-off-by: Alexander Graf <agraf@suse.de> > > OpenBSD is not calling SetVirtualAddressMap on ARM 32 bit. > I take it this means that OpenBSD does not support runtime services at all on 32-bit ARM? > Adjust selftest: expect 'U-Boot' instead of 'resetting'. > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > v4 > Adjust selftest: expect 'U-Boot' instead of 'resetting'. > (by fault sent as v2) > v3 > Add link to upstream Linux patch > v2 > Add missing icache invalidation > --- > include/efi_loader.h | 2 ++ > lib/efi_loader/efi_boottime.c | 1 + > lib/efi_loader/efi_runtime.c | 29 ++++++++++++++++++++--------- > test/py/tests/test_efi_selftest.py | 4 ++-- > 4 files changed, 25 insertions(+), 11 deletions(-) > > diff --git a/include/efi_loader.h b/include/efi_loader.h > index 512880ab8fb..82db7775c72 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -310,6 +310,8 @@ void efi_save_gd(void); > void efi_restore_gd(void); > /* Call this to relocate the runtime section to an address space */ > void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map); > +/* Call this when we start to live in a runtime only world */ > +void efi_runtime_detach(ulong offset); > /* Call this to set the current device name */ > void efi_set_bootdev(const char *dev, const char *devnr, const char *path); > /* Add a new object to the object list. */ > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c > index bd8b8a17ae7..233bca78e0a 100644 > --- a/lib/efi_loader/efi_boottime.c > +++ b/lib/efi_loader/efi_boottime.c > @@ -1967,6 +1967,7 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle, > bootm_disable_interrupts(); > > /* Disable boot time services */ > + efi_runtime_detach((ulong)gd->relocaddr); > systab.con_in_handle = NULL; > systab.con_in = NULL; > systab.con_out_handle = NULL; > diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c > index 636dfdab39d..17d22d429e2 100644 > --- a/lib/efi_loader/efi_runtime.c > +++ b/lib/efi_loader/efi_runtime.c > @@ -276,15 +276,11 @@ struct efi_runtime_detach_list_struct { > void *patchto; > }; > > -static const struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = { > +static struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = { > { > /* do_reset is gone */ > .ptr = &efi_runtime_services.reset_system, > .patchto = efi_reset_system, > - }, { > - /* invalidate_*cache_all are gone */ > - .ptr = &efi_runtime_services.set_virtual_address_map, > - .patchto = &efi_unimplemented, > }, { > /* RTC accessors are gone */ > .ptr = &efi_runtime_services.get_time, > @@ -328,7 +324,15 @@ static bool efi_runtime_tobedetached(void *p) > return false; > } > > -static void efi_runtime_detach(ulong offset) > +/** > + * efi_runtime_detach() - Remove any dependency on non-runtime sections > + * > + * This function patches all remaining code to be self-sufficient inside > + * runtime sections. Any calls to non-runtime will be removed after this. > + * > + * @offset: relocaddr for pre-set_v_a_space, offset to VA after > + */ > +__efi_runtime void efi_runtime_detach(ulong offset) > { > int i; > ulong patchoff = offset - (ulong)gd->relocaddr; > @@ -344,6 +348,8 @@ static void efi_runtime_detach(ulong offset) > > /* Update CRC32 */ > efi_update_table_header_crc32(&efi_runtime_services.hdr); > + > + invalidate_icache_all(); > } > > /* Relocate EFI runtime to uboot_reloc_base = offset */ > @@ -430,19 +436,25 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map) > * @virtmap: virtual address mapping information > * Return: status code > */ > -static efi_status_t EFIAPI efi_set_virtual_address_map( > +static __efi_runtime efi_status_t EFIAPI efi_set_virtual_address_map( > unsigned long memory_map_size, > unsigned long descriptor_size, > uint32_t descriptor_version, > struct efi_mem_desc *virtmap) > { > + static __efi_runtime_data bool is_patched; > int n = memory_map_size / descriptor_size; > int i; > int rt_code_sections = 0; > > + if (is_patched) > + return EFI_INVALID_PARAMETER; > + > EFI_ENTRY("%lx %lx %x %p", memory_map_size, descriptor_size, > descriptor_version, virtmap); > > + is_patched = true; > + > /* > * TODO: > * Further down we are cheating. While really we should implement > @@ -514,8 +526,7 @@ static efi_status_t EFIAPI efi_set_virtual_address_map( > map->physical_start + gd->relocaddr; > > efi_runtime_relocate(new_offset, map); > - /* Once we're virtual, we can no longer handle > - complex callbacks */ > + /* We need to repatch callbacks for their new VA */ > efi_runtime_detach(new_offset); > return EFI_EXIT(EFI_SUCCESS); > } > diff --git a/test/py/tests/test_efi_selftest.py b/test/py/tests/test_efi_selftest.py > index 36b35ee536b..6f5b2b8f406 100644 > --- a/test/py/tests/test_efi_selftest.py > +++ b/test/py/tests/test_efi_selftest.py > @@ -20,7 +20,7 @@ def test_efi_selftest(u_boot_console): > if m != 0: > raise Exception('Failures occurred during the EFI selftest') > u_boot_console.run_command(cmd='', wait_for_echo=False, wait_for_prompt=False); > - m = u_boot_console.p.expect(['resetting', 'U-Boot']) > + m = u_boot_console.p.expect(['U-Boot']) > if m != 0: > raise Exception('Reset failed during the EFI selftest') > u_boot_console.restart_uboot(); > @@ -46,7 +46,7 @@ def test_efi_selftest_watchdog_reboot(u_boot_console): > assert '\'watchdog reboot\'' in output > u_boot_console.run_command(cmd='setenv efi_selftest watchdog reboot') > u_boot_console.run_command(cmd='bootefi selftest', wait_for_prompt=False) > - m = u_boot_console.p.expect(['resetting', 'U-Boot']) > + m = u_boot_console.p.expect(['U-Boot']) > if m != 0: > raise Exception('Reset failed in \'watchdog reboot\' test') > u_boot_console.restart_uboot(); > -- > 2.20.1 >
On 3/11/19 9:03 AM, Ard Biesheuvel wrote: > On Mon, 11 Mar 2019 at 01:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> From: Alexander Graf <agraf@suse.de> >> >> While discussing something compeltely different, Ard pointed out >> that it might be legal to omit calling SetVirtualAddressMap altogether. >> >> There is even a patch on the Linux Kernel Mailing List that implements >> such behavior by now: >> >> https://patchwork.kernel.org/patch/10782393/ >> >> While that sounds great, we currently rely on the SetVirtualAddressMap >> call to remove all references to code that would not work outside of >> boot services. >> >> So let's patch out those bits already on the call to ExitBootServices, >> so that we can successfully run even when an OS chooses to omit >> any call to SetVirtualAddressMap. >> >> Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Signed-off-by: Alexander Graf <agraf@suse.de> >> >> OpenBSD is not calling SetVirtualAddressMap on ARM 32 bit. >> > > I take it this means that OpenBSD does not support runtime services at > all on 32-bit ARM? References to runtime services exist, e.g.: sys/arch/armv7/stand/efiboot/efiboot.c:666: EFI_CALL(RS->ResetSystem, EfiResetShutdown, EFI_SUCCESS, 0, NULL); But from what I see at first glance it seems to be for the interactive console before booting the kernel. Best regards Heinrich
On Mon, Mar 11, 2019 at 12:44:24PM +0100, Heinrich Schuchardt wrote: > On 3/11/19 9:03 AM, Ard Biesheuvel wrote: > > On Mon, 11 Mar 2019 at 01:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >> > >> From: Alexander Graf <agraf@suse.de> > >> > >> While discussing something compeltely different, Ard pointed out > >> that it might be legal to omit calling SetVirtualAddressMap altogether. > >> > >> There is even a patch on the Linux Kernel Mailing List that implements > >> such behavior by now: > >> > >> https://patchwork.kernel.org/patch/10782393/ > >> > >> While that sounds great, we currently rely on the SetVirtualAddressMap > >> call to remove all references to code that would not work outside of > >> boot services. > >> > >> So let's patch out those bits already on the call to ExitBootServices, > >> so that we can successfully run even when an OS chooses to omit > >> any call to SetVirtualAddressMap. > >> > >> Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> Signed-off-by: Alexander Graf <agraf@suse.de> > >> > >> OpenBSD is not calling SetVirtualAddressMap on ARM 32 bit. > >> > > > > I take it this means that OpenBSD does not support runtime services at > > all on 32-bit ARM? > > References to runtime services exist, e.g.: > > sys/arch/armv7/stand/efiboot/efiboot.c:666: > EFI_CALL(RS->ResetSystem, EfiResetShutdown, EFI_SUCCESS, 0, NULL); > > But from what I see at first glance it seems to be for the interactive > console before booting the kernel. Runtime services are used on OpenBSD/arm64 to be able to access the rtc on amd seattle machines. Not currently used by the kernel on any other platform (including amd64). > > Best regards > > Heinrich > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
diff --git a/include/efi_loader.h b/include/efi_loader.h index 512880ab8fb..82db7775c72 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -310,6 +310,8 @@ void efi_save_gd(void); void efi_restore_gd(void); /* Call this to relocate the runtime section to an address space */ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map); +/* Call this when we start to live in a runtime only world */ +void efi_runtime_detach(ulong offset); /* Call this to set the current device name */ void efi_set_bootdev(const char *dev, const char *devnr, const char *path); /* Add a new object to the object list. */ diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index bd8b8a17ae7..233bca78e0a 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1967,6 +1967,7 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle, bootm_disable_interrupts(); /* Disable boot time services */ + efi_runtime_detach((ulong)gd->relocaddr); systab.con_in_handle = NULL; systab.con_in = NULL; systab.con_out_handle = NULL; diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index 636dfdab39d..17d22d429e2 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -276,15 +276,11 @@ struct efi_runtime_detach_list_struct { void *patchto; }; -static const struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = { +static struct efi_runtime_detach_list_struct efi_runtime_detach_list[] = { { /* do_reset is gone */ .ptr = &efi_runtime_services.reset_system, .patchto = efi_reset_system, - }, { - /* invalidate_*cache_all are gone */ - .ptr = &efi_runtime_services.set_virtual_address_map, - .patchto = &efi_unimplemented, }, { /* RTC accessors are gone */ .ptr = &efi_runtime_services.get_time, @@ -328,7 +324,15 @@ static bool efi_runtime_tobedetached(void *p) return false; } -static void efi_runtime_detach(ulong offset) +/** + * efi_runtime_detach() - Remove any dependency on non-runtime sections + * + * This function patches all remaining code to be self-sufficient inside + * runtime sections. Any calls to non-runtime will be removed after this. + * + * @offset: relocaddr for pre-set_v_a_space, offset to VA after + */ +__efi_runtime void efi_runtime_detach(ulong offset) { int i; ulong patchoff = offset - (ulong)gd->relocaddr; @@ -344,6 +348,8 @@ static void efi_runtime_detach(ulong offset) /* Update CRC32 */ efi_update_table_header_crc32(&efi_runtime_services.hdr); + + invalidate_icache_all(); } /* Relocate EFI runtime to uboot_reloc_base = offset */ @@ -430,19 +436,25 @@ void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map) * @virtmap: virtual address mapping information * Return: status code */ -static efi_status_t EFIAPI efi_set_virtual_address_map( +static __efi_runtime efi_status_t EFIAPI efi_set_virtual_address_map( unsigned long memory_map_size, unsigned long descriptor_size, uint32_t descriptor_version, struct efi_mem_desc *virtmap) { + static __efi_runtime_data bool is_patched; int n = memory_map_size / descriptor_size; int i; int rt_code_sections = 0; + if (is_patched) + return EFI_INVALID_PARAMETER; + EFI_ENTRY("%lx %lx %x %p", memory_map_size, descriptor_size, descriptor_version, virtmap); + is_patched = true; + /* * TODO: * Further down we are cheating. While really we should implement @@ -514,8 +526,7 @@ static efi_status_t EFIAPI efi_set_virtual_address_map( map->physical_start + gd->relocaddr; efi_runtime_relocate(new_offset, map); - /* Once we're virtual, we can no longer handle - complex callbacks */ + /* We need to repatch callbacks for their new VA */ efi_runtime_detach(new_offset); return EFI_EXIT(EFI_SUCCESS); } diff --git a/test/py/tests/test_efi_selftest.py b/test/py/tests/test_efi_selftest.py index 36b35ee536b..6f5b2b8f406 100644 --- a/test/py/tests/test_efi_selftest.py +++ b/test/py/tests/test_efi_selftest.py @@ -20,7 +20,7 @@ def test_efi_selftest(u_boot_console): if m != 0: raise Exception('Failures occurred during the EFI selftest') u_boot_console.run_command(cmd='', wait_for_echo=False, wait_for_prompt=False); - m = u_boot_console.p.expect(['resetting', 'U-Boot']) + m = u_boot_console.p.expect(['U-Boot']) if m != 0: raise Exception('Reset failed during the EFI selftest') u_boot_console.restart_uboot(); @@ -46,7 +46,7 @@ def test_efi_selftest_watchdog_reboot(u_boot_console): assert '\'watchdog reboot\'' in output u_boot_console.run_command(cmd='setenv efi_selftest watchdog reboot') u_boot_console.run_command(cmd='bootefi selftest', wait_for_prompt=False) - m = u_boot_console.p.expect(['resetting', 'U-Boot']) + m = u_boot_console.p.expect(['U-Boot']) if m != 0: raise Exception('Reset failed in \'watchdog reboot\' test') u_boot_console.restart_uboot();