[efibootguard][PATCH 1/2] Enable boot without watchdog

[dmi...@toubelis.org]

From: Dmitri Toubelis <dmitri....@alkeron.com>

This patch allows booting an image without arming watchdog timer
whwn "watchdog=0" parameter is passed to the to the module.

This covers the use case when availability of a hardware driver is
unknown ahead of time and can only be established upon first boot.
The code that runs on the first boot can then identify if supported
hardware watchdog is available and the required driver is present
in the kernel and either update boot configuration re-arming the
timer or issuing a warning unsupported hardware. This also allows
for booting images in virtual environments.

Signed-off-by: Dmitri Toubelis <dmitri....@litmusautomation.com>
---
main.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/main.c b/main.c
index b71f811..a527e84 100644
--- a/main.c
+++ b/main.c
@@ -50,6 +50,11 @@ static EFI_STATUS scan_devices(EFI_LOADED_IMAGE *loaded_image, UINTN timeout)
EFI_STATUS status;
UINT32 value;

+ if (timeout == 0) {
+ Print(L"Watchdog is disabled.\n");
+ return EFI_SUCCESS;
+ }
+
status = uefi_call_wrapper(BS->LocateHandle, 5, ByProtocol,
&PciIoProtocol, NULL, &size, devices);
if (EFI_ERROR(status)) {
--
2.7.4


From 9f355887dd370b3682f41bc8474955823f09e38b Mon Sep 17 00:00:00 2001
From: Dmitri Toubelis <dmitri....@litmusautomation.com>
Date: Thu, 1 Feb 2018 00:40:56 -0500
Subject: [efibootguard][PATCH 2/2] Allow "watchdog" be equal "0"

Signed-off-by: Dmitri Toubelis <dmitri....@litmusautomation.com>
---
tools/bg_setenv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index b397aca..37f50ce 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -338,7 +338,7 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
break;
case 'w':
i = parse_int(arg);
- if (errno || i == 0) {
+ if (errno || i < 0) {
fprintf(stderr,
"Invalid watchdog timeout specified.\n");
return 1;
--
2.7.4

[Jan Kiszka]

On 2018-02-01 07:15, dmi...@toubelis.org wrote:
> From: Dmitri Toubelis <dmitri....@alkeron.com>
>
> This patch allows booting an image without arming watchdog timer
> whwn "watchdog=0" parameter is passed to the to the module.

when

>
> This covers the use case when availability of a hardware driver is
> unknown ahead of time and can only be established upon first boot.
> The code that runs on the first boot can then identify if supported
> hardware watchdog is available and the required driver is present
> in the kernel and either update boot configuration re-arming the
> timer or issuing a warning unsupported hardware. This also allows
> for booting images in virtual environments.
>
> Signed-off-by: Dmitri Toubelis <dmitri....@litmusautomation.com>
> ---
> main.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/main.c b/main.c
> index b71f811..a527e84 100644
> --- a/main.c
> +++ b/main.c
> @@ -50,6 +50,11 @@ static EFI_STATUS scan_devices(EFI_LOADED_IMAGE *loaded_image, UINTN timeout)
> EFI_STATUS status;
> UINT32 value;
>
> + if (timeout == 0) {
> + Print(L"Watchdog is disabled.\n");
> + return EFI_SUCCESS;
> + }
> +

Indention should be done via tabs.

But let's rather move this code to efi_main.

> status = uefi_call_wrapper(BS->LocateHandle, 5, ByProtocol,
> &PciIoProtocol, NULL, &size, devices);
> if (EFI_ERROR(status)) {
>

Subject suggests that there is a patch 2/2, but I'm not seeing anything
on the list.

Thanks,
Jan

--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

[dmi...@toubelis.org]

From: Dmitri Toubelis <dmitri....@alkeron.com>

This patch allows booting an image without arming watchdog timer
whwn "watchdog=0" parameter is passed to the to the module.

This covers the use case when availability of a hardware driver is
unknown ahead of time and can only be established upon first boot.
The code that runs on the first boot can then identify if supported
hardware watchdog is available and the required driver is present
in the kernel and either update boot configuration re-arming the
timer or issuing a warning unsupported hardware. This also allows
for booting images in virtual environments.

Allow "watchdog" be equal "0"

Move bypass switch to efi_main

Signed-off-by: Dmitri Toubelis <dmitri....@litmusautomation.com>
---

main.c | 11 ++++++++---
tools/bg_setenv.c | 2 +-
2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/main.c b/main.c
index b71f811..64d93b8 100644
--- a/main.c
+++ b/main.c
@@ -157,9 +157,14 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
EFI_OUT_OF_RESOURCES);
}

- status = scan_devices(loaded_image, bg_loader_params.timeout);
- if (EFI_ERROR(status)) {
- error_exit(L"Could not probe watchdog.", status);
+ if (bg_loader_params.timeout == 0) {

+ Print(L"Watchdog is disabled.\n");
+ return EFI_SUCCESS;

+ } else {
+ status = scan_devices(loaded_image, bg_loader_params.timeout);
+ if (EFI_ERROR(status)) {
+ error_exit(L"Could not probe watchdog.", status);
+ }
}

/* Load and start image */

[dmitri....@litmusautomation.com]

Sorry, I messed up and combined two patches in one and forgot to edit the subject.

Nevertheless, I moved the code into `efi_main`, replaced spaces with tabs and sent a new one.

Regards,

-Dmitri

[Jan Kiszka]

On 2018-02-01 14:24, dmi...@toubelis.org wrote:
> From: Dmitri Toubelis <dmitri....@alkeron.com>
>
> This patch allows booting an image without arming watchdog timer
> whwn "watchdog=0" parameter is passed to the to the module.
>
> This covers the use case when availability of a hardware driver is
> unknown ahead of time and can only be established upon first boot.
> The code that runs on the first boot can then identify if supported
> hardware watchdog is available and the required driver is present
> in the kernel and either update boot configuration re-arming the
> timer or issuing a warning unsupported hardware. This also allows
> for booting images in virtual environments.
>
> Allow "watchdog" be equal "0"
>
> Move bypass switch to efi_main

Removing the last two lines (patch history usually goes after "---"), I
would take this into next now - if you could clarify your preferred
email domain (@alkeron or @litmusautomation). Just to be consistent
between authorship and signed off.

Thanks,
Jan

[Claudius Heine]

Hi Dimitri,

please version your patches.

On 02/01/2018 02:24 PM, dmi...@toubelis.org wrote:
> From: Dmitri Toubelis <dmitri....@alkeron.com>
>
> This patch allows booting an image without arming watchdog timer
> whwn "watchdog=0" parameter is passed to the to the module.

Typo, as Jan already pointed out in previous patch.

Otherwise looks got from my POV.

Thanks,
Claudius

DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: c...@denx.de

[Dmitri Toubelis]

@litmusautomation is the correct domain for this submission.

Thanks
-Dmitri

[dmitri....@litmusautomation.com]

Hi Claudius,

I'm still learning about submitting patches via email, so please bear with me :-) Could you please explain what do you mean by "versioning patches"? I did rebase to squash history before sending the last patch, is it not how you do it? Would you like me to re-send the patch with typo corrected?

-Dmitri

[Jan Kiszka]

On 2018-02-01 14:24, dmi...@toubelis.org wrote:

WAIT! This code wasn't tested... ;)

Jan

> + } else {
> + status = scan_devices(loaded_image, bg_loader_params.timeout);
> + if (EFI_ERROR(status)) {
> + error_exit(L"Could not probe watchdog.", status);
> + }
> }
>
> /* Load and start image */
> diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
> index b397aca..37f50ce 100644
> --- a/tools/bg_setenv.c
> +++ b/tools/bg_setenv.c
> @@ -338,7 +338,7 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
> break;
> case 'w':
> i = parse_int(arg);
> - if (errno || i == 0) {
> + if (errno || i < 0) {
> fprintf(stderr,
> "Invalid watchdog timeout specified.\n");
> return 1;
>

--

[Claudius Heine]

Hi Dimitri,

On 02/01/2018 03:00 PM, dmitri....@litmusautomation.com wrote:
> Hi Claudius,
>
> I'm still learning about submitting patches via email, so please bear with
> me :-) Could you please explain what do you mean by "versioning patches"?

Don't worry, we all were there once :)

I do like this:

git format-patch --subject-prefix='PATCH v2' ...

Then the subject of all patches changes from '[PATCH]' to '[PATCH v2]'.
This helps reviewers to find the latest version of a patch in a busy inbox.

> I did rebase to squash history before sending the last patch, is it not how
> you do it? Would you like me to re-send the patch with typo corrected?

If no other reviewer finds an issue, then this could be done while
merging. In your place I would wait for now.

Cheers,
Claudius

>
> -Dmitri
>
> On Thursday, February 1, 2018 at 8:41:41 AM UTC-5, Claudius Heine wrote:
>>
>> Hi Dimitri,
>>
>> please version your patches.
>>

>> On 02/01/2018 02:24 PM, dmi...@toubelis.org <javascript:> wrote:
>>> From: Dmitri Toubelis <dmitri....@alkeron.com <javascript:>>

>>>
>>> This patch allows booting an image without arming watchdog timer
>>> whwn "watchdog=0" parameter is passed to the to the module.
>>
>> Typo, as Jan already pointed out in previous patch.
>>
>> Otherwise looks got from my POV.
>>
>> Thanks,
>> Claudius
>>
>>>
>>> This covers the use case when availability of a hardware driver is
>>> unknown ahead of time and can only be established upon first boot.
>>> The code that runs on the first boot can then identify if supported
>>> hardware watchdog is available and the required driver is present
>>> in the kernel and either update boot configuration re-arming the
>>> timer or issuing a warning unsupported hardware. This also allows
>>> for booting images in virtual environments.
>>>
>>> Allow "watchdog" be equal "0"
>>>
>>> Move bypass switch to efi_main
>>>
>>> Signed-off-by: Dmitri Toubelis <dmitri....@litmusautomation.com

>> <javascript:>>

>> <javascript:>

[dmitri....@litmusautomation.com]

Good catch. It has a bug and it doesn't work with watchdog=0 (there shouldn't be return statement in the if clause). Testing it now. Will send update shortly.

[dmi...@toubelis.org]

From: Dmitri Toubelis <dmi...@toubelis.org>

This patch allows booting an image without arming watchdog timer

when "watchdog=0" parameter is passed to the to the module.

This covers the use case when availability of a hardware driver is
unknown ahead of time and can only be established upon first boot.
The code that runs on the first boot can then identify if supported

hardware watchdog is available, the required driver is present in

the kernel and either update boot configuration re-arming the timer

or issuing a warning about unsupported hardware. This would also
allow for booting images in virtual environments.

Signed-off-by: Dmitri Toubelis <dmitri....@litmusautomation.com>
---

main.c | 10 +++++++---
tools/bg_setenv.c | 2 +-
2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/main.c b/main.c
index b71f811..bd8a668 100644
--- a/main.c
+++ b/main.c
@@ -157,9 +157,13 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)

EFI_OUT_OF_RESOURCES);
}

- status = scan_devices(loaded_image, bg_loader_params.timeout);
- if (EFI_ERROR(status)) {
- error_exit(L"Could not probe watchdog.", status);
+ if (bg_loader_params.timeout == 0) {
+ Print(L"Watchdog is disabled.\n");

+ } else {

+ status = scan_devices(loaded_image, bg_loader_params.timeout);
+ if (EFI_ERROR(status)) {
+ error_exit(L"Could not probe watchdog.", status);
+ }
}

/* Load and start image */
diff --git a/tools/bg_setenv.c b/tools/bg_setenv.c
index b397aca..37f50ce 100644
--- a/tools/bg_setenv.c
+++ b/tools/bg_setenv.c
@@ -338,7 +338,7 @@ static error_t parse_opt(int key, char *arg, struct argp_state *state)
break;
case 'w':
i = parse_int(arg);
- if (errno || i == 0) {
+ if (errno || i < 0) {
fprintf(stderr,
"Invalid watchdog timeout specified.\n");
return 1;
--

2.7.4

[dmitri....@litmusautomation.com]

Tested and boots as expected. However, I was not able to run `make check` on the project. It was complaining about missing `ttt.h` header and I'm not that familiar with `ttt` project to fix it in reasonable time, so I omitted this step.

Regards,

-Dmitri

[Jan Kiszka]

Thanks, applied to next.

Jan

[Jan Kiszka]

...while fixing your From: once again. Please fix for future patches.