[PATCH] Support additional boot delay before starting image

[Earl Chew]

Support a configuration option to insert a delay before
starting the image to give the operator a chance to capture
diagnostic messages.

Signed-off-by: Earl Chew <earl...@yahoo.com>
---
configure.ac | 15 +++++++++++++++
main.c | 3 +++
2 files changed, 18 insertions(+)

diff --git a/configure.ac b/configure.ac
index b000603..1738968 100644
--- a/configure.ac
+++ b/configure.ac
@@ -156,6 +156,20 @@ AC_ARG_WITH([env-backend],

AC_SUBST([env_api_file], [${ENV_API_FILE}])

+AC_ARG_WITH([boot-delay],
+ AS_HELP_STRING([--with-boot-delay=INT],
+ [specify the additional boot delay in seconds, defaults to 3]),
+ [
+ ENV_BOOT_DELAY=${withval:-3}
+ AS_IF([test "${ENV_BOOT_DELAY}" -lt "0"],
+ [
+ AC_MSG_ERROR([Invalid boot delay.])
+ ])
+ ],
+ [ ENV_BOOT_DELAY=0 ])
+
+AC_DEFINE_UNQUOTED([ENV_BOOT_DELAY], [${ENV_BOOT_DELAY}], [Additional boot delay])
+
AC_ARG_WITH([num-config-parts],
AS_HELP_STRING([--with-num-config-parts=INT],
[specify the number of used config partitions, defaults to 2]),
@@ -248,4 +262,5 @@ AC_MSG_RESULT([
environment backend: ${ENV_API_FILE}.c
number of config parts: ${ENV_NUM_CONFIG_PARTS}
reserved for uservars: ${ENV_MEM_USERVARS} bytes
+ boot delay: ${ENV_BOOT_DELAY} seconds
])
diff --git a/main.c b/main.c
index 0ff121a..52d65c0 100644
--- a/main.c
+++ b/main.c
@@ -214,5 +214,8 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
INFO(L"Starting %s with watchdog set to %d seconds ...\n",
bg_loader_params.payload_path, bg_loader_params.timeout);

+ for (UINTN delay = ENV_BOOT_DELAY; delay; --delay)
+ BS->Stall(1000 * 1000);
+
return BS->StartImage(payload_handle, NULL, NULL);
}
--
2.39.1

[Jan Kiszka]

Why this loop? Stall() should be usable with up to 2000 seconds.

Jan

> +
> return BS->StartImage(payload_handle, NULL, NULL);
> }

--

Siemens AG, Technology
Linux Expert Center

[Earl Chew]

Jan,

On 2024-02-12 00:25, Jan Kiszka wrote:
>> + for (UINTN delay = ENV_BOOT_DELAY; delay; --delay)
>> + BS->Stall(1000 * 1000);
>
> Why this loop? Stall() should be usable with up to 2000 seconds.

I think this route makes it straightforward to reason about correctness
for all allowable values, and I do not think reducing the number of
calls provides any useful gain.

As I understand it, Stall(UINTN) accepts either an unsigned 32 bit,
or 64 bit, integer depending on the host architecture.

Would you prefer a different approach?

Earl

[Jan Kiszka]

Oh, it's even unsigned - 2 times more seconds.

>
> Would you prefer a different approach?

Yeah, the simpler one. Neither 4000 nor 4000000000 seconds are
reasonable values here, so a simple Stall() suffices.

Jan

[Earl Chew]

Support a configuration option to insert a delay before
starting the image to give the operator a chance to capture
diagnostic messages.

Signed-off-by: Earl Chew <earl...@yahoo.com>
---

configure.ac | 17 +++++++++++++++++
main.c | 4 ++++
2 files changed, 21 insertions(+)

diff --git a/configure.ac b/configure.ac
index b000603..de7ef3e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -156,6 +156,22 @@ AC_ARG_WITH([env-backend],

AC_SUBST([env_api_file], [${ENV_API_FILE}])

+AC_ARG_WITH([boot-delay],
+ AS_HELP_STRING([--with-boot-delay=INT],
+ [specify the additional boot delay in seconds, defaults to 3]),
+ [

+ # Limit the boot delay to 1 hour to avoid overflowing
+ # BS->Stall() on 32 bit hosts.
+ ENV_BOOT_DELAY=${withval:-3}
+ AS_IF([test "${ENV_BOOT_DELAY}" -lt "0" -o "${ENV_BOOT_DELAY}" -gt "3600"],

+ [
+ AC_MSG_ERROR([Invalid boot delay.])
+ ])
+ ],
+ [ ENV_BOOT_DELAY=0 ])
+
+AC_DEFINE_UNQUOTED([ENV_BOOT_DELAY], [${ENV_BOOT_DELAY}], [Additional boot delay])
+
AC_ARG_WITH([num-config-parts],
AS_HELP_STRING([--with-num-config-parts=INT],
[specify the number of used config partitions, defaults to 2]),

@@ -248,4 +264,5 @@ AC_MSG_RESULT([

environment backend: ${ENV_API_FILE}.c
number of config parts: ${ENV_NUM_CONFIG_PARTS}
reserved for uservars: ${ENV_MEM_USERVARS} bytes
+ boot delay: ${ENV_BOOT_DELAY} seconds
])
diff --git a/main.c b/main.c

index 0ff121a..0da6a05 100644
--- a/main.c
+++ b/main.c
@@ -214,5 +214,9 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)

INFO(L"Starting %s with watchdog set to %d seconds ...\n",
bg_loader_params.payload_path, bg_loader_params.timeout);

+ const UINTN one_hour = 3600;
+ BS->Stall(1000 * 1000 *
+ (ENV_BOOT_DELAY < one_hour ? ENV_BOOT_DELAY : one_hour));

+
return BS->StartImage(payload_handle, NULL, NULL);
}
--

2.39.1

[Jan Kiszka]

Why do you do this twice? You already detect this case during configure
time.

Jan

> +
> return BS->StartImage(payload_handle, NULL, NULL);
> }

--

[Earl Chew]

Jan,

> Why do you do this twice? You already detect this case during configure
> time.

The runtime check ensures that the argument to BS->Stall() never overflows.
Having said that, perhaps this would be more fitting, and then forget about
the check at configure time:

+ const UINTN max_delay = ~ (UINTN) 0 / 1000 / 1000;

+ BS->Stall(1000 * 1000 *

+ (ENV_BOOT_DELAY < max_delay ? ENV_BOOT_DELAY : max_delay));

Earl

[Jan Kiszka]

On 19.02.24 05:10, 'Earl Chew' via EFI Boot Guard wrote:
> Jan,
>
>> Why do you do this twice? You already detect this case during configure
>> time.
>
> The runtime check ensures that the argument to BS->Stall() never overflows.
> Having said that, perhaps this would be more fitting, and then forget about
> the check at configure time:
>
> +    const UINTN max_delay = ~ (UINTN) 0 / 1000 / 1000;
> +    BS->Stall(1000 * 1000 *
> +        (ENV_BOOT_DELAY < max_delay ? ENV_BOOT_DELAY : max_delay));
>

This is a build-time configurable, so let's check at build-time.

Jan

[Earl Chew]

Support a configuration option to insert a delay before
starting the image to give the operator a chance to capture
diagnostic messages.

Signed-off-by: Earl Chew <earl...@yahoo.com>
---
configure.ac | 17 +++++++++++++++++

main.c | 2 ++
2 files changed, 19 insertions(+)

index 0ff121a..a0b1b60 100644
--- a/main.c
+++ b/main.c
@@ -214,5 +214,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)

INFO(L"Starting %s with watchdog set to %d seconds ...\n",
bg_loader_params.payload_path, bg_loader_params.timeout);

+ BS->Stall(1000 * 1000 * ENV_BOOT_DELAY);

+
return BS->StartImage(payload_handle, NULL, NULL);
}

--
2.39.1

[Jan Kiszka]

Thanks, merged. Please consider checking latest next in the future to
avoid conflicts (resolved this one myself).