| Message ID | 20231121113557.800353-7-sjg@chromium.org |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | efi: Partial attempt at a test for EFI bootmeth | expand |
On 11/21/23 12:35, Simon Glass wrote: > This allows testing of the exit_boot_services call, providing more > coverage of the EFI bootmeth. > > Signed-off-by: Simon Glass <sjg@chromium.org> I would prefer to keep helloworld.efi benign. Please, create a new EFI binary for testing ExitBootServices(). > --- > > lib/efi_loader/helloworld.c | 36 +++++++++++++++++++++++++++++++++++- > 1 file changed, 35 insertions(+), 1 deletion(-) > > diff --git a/lib/efi_loader/helloworld.c b/lib/efi_loader/helloworld.c > index 1135d3a3c37e..1fb5fb5a62f2 100644 > --- a/lib/efi_loader/helloworld.c > +++ b/lib/efi_loader/helloworld.c > @@ -206,6 +206,26 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle, > (con_out, u"Cannot open loaded image protocol\r\n"); > goto out; > } > + > + { > + ulong ptr = (ulong)loaded_image; > + u16 str[80]; > + int i; > + > + for (i = 0; i < 8; i++) { > + uint digit = (ptr >> ((7 - i) * 4)) & 0xf; > + > + if (digit > 9) > + digit = 'a' + digit - 10; > + else > + digit += '0'; > + str[i] = digit; > + } > + str[i] = 0; > + con_out->output_string(con_out, str); > + con_out->output_string(con_out, u"\n"); > + } > + > print_load_options(loaded_image); > > /* Get the device path to text protocol */ > @@ -243,7 +263,21 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle, > goto out; > > out: > - boottime->exit(handle, ret, 0, NULL); > + /* > + * TODO: Use vendor string to decide whether to call exit-boot-services > + */ > + efi_uintn_t map_size = 0; > + efi_uintn_t map_key; > + efi_uintn_t desc_size; > + u32 desc_version; > + > + ret = boottime->get_memory_map(&map_size, NULL, &map_key, &desc_size, > + &desc_version); > + con_out->output_string(con_out, u"Exiting boot sevices\n"); > + > + boottime->exit_boot_services(handle, map_key); > + > + ret = boottime->exit(handle, ret, 0, NULL); After ExitBootServices() you must not call boot services. Call runtime->reset_system(EFI_RESET_COLD, EFI_SUCCESS, 0, NULL) instead. Best regards Heinrich > > /* We should never arrive here */ > return ret;
Hi Heinrich, On Tue, 21 Nov 2023 at 10:17, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 11/21/23 12:35, Simon Glass wrote: > > This allows testing of the exit_boot_services call, providing more > > coverage of the EFI bootmeth. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > I would prefer to keep helloworld.efi benign. > > Please, create a new EFI binary for testing ExitBootServices(). Eek...isn't helloworld already used in tets? > > > --- > > > > lib/efi_loader/helloworld.c | 36 +++++++++++++++++++++++++++++++++++- > > 1 file changed, 35 insertions(+), 1 deletion(-) > > > > diff --git a/lib/efi_loader/helloworld.c b/lib/efi_loader/helloworld.c > > index 1135d3a3c37e..1fb5fb5a62f2 100644 > > --- a/lib/efi_loader/helloworld.c > > +++ b/lib/efi_loader/helloworld.c > > @@ -206,6 +206,26 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle, > > (con_out, u"Cannot open loaded image protocol\r\n"); > > goto out; > > } > > + > > + { > > + ulong ptr = (ulong)loaded_image; > > + u16 str[80]; > > + int i; > > + > > + for (i = 0; i < 8; i++) { > > + uint digit = (ptr >> ((7 - i) * 4)) & 0xf; > > + > > + if (digit > 9) > > + digit = 'a' + digit - 10; > > + else > > + digit += '0'; > > + str[i] = digit; > > + } > > + str[i] = 0; > > + con_out->output_string(con_out, str); > > + con_out->output_string(con_out, u"\n"); > > + } > > + > > print_load_options(loaded_image); > > > > /* Get the device path to text protocol */ > > @@ -243,7 +263,21 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle, > > goto out; > > > > out: > > - boottime->exit(handle, ret, 0, NULL); > > + /* > > + * TODO: Use vendor string to decide whether to call exit-boot-services > > + */ > > + efi_uintn_t map_size = 0; > > + efi_uintn_t map_key; > > + efi_uintn_t desc_size; > > + u32 desc_version; > > + > > + ret = boottime->get_memory_map(&map_size, NULL, &map_key, &desc_size, > > + &desc_version); > > + con_out->output_string(con_out, u"Exiting boot sevices\n"); > > + > > + boottime->exit_boot_services(handle, map_key); > > + > > + ret = boottime->exit(handle, ret, 0, NULL); > > After ExitBootServices() you must not call boot services. > > Call runtime->reset_system(EFI_RESET_COLD, EFI_SUCCESS, 0, NULL) instead. OK, will do. That will be ignored by sandbox, right? For now this is just a hack to try to resolve the problem mentioned. Regards, Simon
Hi Simon, On Wed, 22 Nov 2023 at 00:10, Simon Glass <sjg@chromium.org> wrote: > > Hi Heinrich, > > On Tue, 21 Nov 2023 at 10:17, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > On 11/21/23 12:35, Simon Glass wrote: > > > This allows testing of the exit_boot_services call, providing more > > > coverage of the EFI bootmeth. > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > I would prefer to keep helloworld.efi benign. > > > > Please, create a new EFI binary for testing ExitBootServices(). > > Eek...isn't helloworld already used in tets?\ Yes, but calling exitboot services from it means we need to be really careful when/how we call it, since once that app runs the boottime services along with any attached disks will disappear. Instead having a single EFI app that calls that and then tests for any runtime services is more scalable Thanks /Ilias > > > > > > --- > > > > > > lib/efi_loader/helloworld.c | 36 +++++++++++++++++++++++++++++++++++- > > > 1 file changed, 35 insertions(+), 1 deletion(-) > > > > > > diff --git a/lib/efi_loader/helloworld.c b/lib/efi_loader/helloworld.c > > > index 1135d3a3c37e..1fb5fb5a62f2 100644 > > > --- a/lib/efi_loader/helloworld.c > > > +++ b/lib/efi_loader/helloworld.c > > > @@ -206,6 +206,26 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle, > > > (con_out, u"Cannot open loaded image protocol\r\n"); > > > goto out; > > > } > > > + > > > + { > > > + ulong ptr = (ulong)loaded_image; > > > + u16 str[80]; > > > + int i; > > > + > > > + for (i = 0; i < 8; i++) { > > > + uint digit = (ptr >> ((7 - i) * 4)) & 0xf; > > > + > > > + if (digit > 9) > > > + digit = 'a' + digit - 10; > > > + else > > > + digit += '0'; > > > + str[i] = digit; > > > + } > > > + str[i] = 0; > > > + con_out->output_string(con_out, str); > > > + con_out->output_string(con_out, u"\n"); > > > + } > > > + > > > print_load_options(loaded_image); > > > > > > /* Get the device path to text protocol */ > > > @@ -243,7 +263,21 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle, > > > goto out; > > > > > > out: > > > - boottime->exit(handle, ret, 0, NULL); > > > + /* > > > + * TODO: Use vendor string to decide whether to call exit-boot-services > > > + */ > > > + efi_uintn_t map_size = 0; > > > + efi_uintn_t map_key; > > > + efi_uintn_t desc_size; > > > + u32 desc_version; > > > + > > > + ret = boottime->get_memory_map(&map_size, NULL, &map_key, &desc_size, > > > + &desc_version); > > > + con_out->output_string(con_out, u"Exiting boot sevices\n"); > > > + > > > + boottime->exit_boot_services(handle, map_key); > > > + > > > + ret = boottime->exit(handle, ret, 0, NULL); > > > > After ExitBootServices() you must not call boot services. > > > > Call runtime->reset_system(EFI_RESET_COLD, EFI_SUCCESS, 0, NULL) instead. > > OK, will do. That will be ignored by sandbox, right? > > For now this is just a hack to try to resolve the problem mentioned. > > Regards, > Simon
Hi Ilias, On Wed, 22 Nov 2023 at 00:39, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > On Wed, 22 Nov 2023 at 00:10, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Heinrich, > > > > On Tue, 21 Nov 2023 at 10:17, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > On 11/21/23 12:35, Simon Glass wrote: > > > > This allows testing of the exit_boot_services call, providing more > > > > coverage of the EFI bootmeth. > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > > > > I would prefer to keep helloworld.efi benign. > > > > > > Please, create a new EFI binary for testing ExitBootServices(). > > > > Eek...isn't helloworld already used in tets?\ > > Yes, but calling exitboot services from it means we need to be really > careful when/how we call it, since once that app runs the boottime > services along with any attached disks will disappear. > Instead having a single EFI app that calls that and then tests for any > runtime services is more scalable OK I can add a new app. I am still stuck on the actual bug mentioned in the cover letter, though. Regards, Simon
diff --git a/lib/efi_loader/helloworld.c b/lib/efi_loader/helloworld.c index 1135d3a3c37e..1fb5fb5a62f2 100644 --- a/lib/efi_loader/helloworld.c +++ b/lib/efi_loader/helloworld.c @@ -206,6 +206,26 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle, (con_out, u"Cannot open loaded image protocol\r\n"); goto out; } + + { + ulong ptr = (ulong)loaded_image; + u16 str[80]; + int i; + + for (i = 0; i < 8; i++) { + uint digit = (ptr >> ((7 - i) * 4)) & 0xf; + + if (digit > 9) + digit = 'a' + digit - 10; + else + digit += '0'; + str[i] = digit; + } + str[i] = 0; + con_out->output_string(con_out, str); + con_out->output_string(con_out, u"\n"); + } + print_load_options(loaded_image); /* Get the device path to text protocol */ @@ -243,7 +263,21 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle, goto out; out: - boottime->exit(handle, ret, 0, NULL); + /* + * TODO: Use vendor string to decide whether to call exit-boot-services + */ + efi_uintn_t map_size = 0; + efi_uintn_t map_key; + efi_uintn_t desc_size; + u32 desc_version; + + ret = boottime->get_memory_map(&map_size, NULL, &map_key, &desc_size, + &desc_version); + con_out->output_string(con_out, u"Exiting boot sevices\n"); + + boottime->exit_boot_services(handle, map_key); + + ret = boottime->exit(handle, ret, 0, NULL); /* We should never arrive here */ return ret;
This allows testing of the exit_boot_services call, providing more coverage of the EFI bootmeth. Signed-off-by: Simon Glass <sjg@chromium.org> --- lib/efi_loader/helloworld.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)