[PATCH 2/2] watchdog: add watchdog behavior configuration

[Michael Walle]

Hi,

Am 2020-09-23 20:51, schrieb Heinrich Schuchardt:
> On 9/23/20 7:53 PM, Mark Kettenis wrote:
>>> Date: Wed, 23 Sep 2020 13:14:09 -0400
>>> From: Tom Rini <trini at konsulko.com>
>>> 
>>> On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote:
>>>>> From: Michael Walle <michael at walle.cc>
>>>>> Date: Wed, 23 Sep 2020 18:45:27 +0200
>>>>> 
>>>>> Let the user choose between three different behaviours of the 
>>>>> watchdog:
>>>>>  (1) Keep the watchdog disabled
>>>>>  (2) Supervise u-boot
>>>>>  (3) Supervise u-boot and the operating systen (default)
>>>>> 
>>>>> Option (2) will disable the watchdog right before handing control 
>>>>> to the
>>>>> operating system. This is useful when the OS is not aware of the
>>>>> watchdog. Option (3) doesn't disable the watchdog and assumes the 
>>>>> OS
>>>>> will continue servicing.
>>>> 
>>>> (3) can't be the default, at least for EFI
>>>> 
>>>> The UEFI standard explicitly says that upon calling
>>>> ExitBootServices(), the watchdog timer is disabled.
>>>> 
> 
> So 3) must not be allowed for CONFIG_EFI_LOADER=y which we have enabled
> on most boards.

"most not be allowed" seems a bit strong to me. First it is the users
choice (well at least on compile-time) and there may still be OSes out
there which are booted by EFI but still support the watchdog. And I 
agree
with Tom, that it is bad to disable it just to re-enable it again.

That being said, I still think having the watchdog enabled by default
is the exceptional case and not the common one. I know U-Boot might
be highly targeted to an embedded use case, but with efi_loader now
available it is shifting to a more "generic" (in the lack of a
better word) bootloader.

The reason (3) is the default is that this was the current behaviour
and I didn't want to change that.

>>>> In general, you can't expect an OS to have support for a particular
>>>> watchdog timer.  So (3) only makes sense in cases where U-Boot is
>>>> bundled with an OS image.
>>> 
>>> We need to be careful here then.  The current and historical / 
>>> generally
>>> expected behavior is if we've enabled the watchdog we supervise it 
>>> and
>>> leave it enabled for the OS.  Given what UEFI requires I'd like to 
>>> see
>>> that case handled with a print about disabling the watchdog so it's 
>>> not
>>> a surprise to the user.  I say this because it's a surprise to me and 
>>> I
>>> guess answers the question of "how does x86 handle this?" I had the
>>> other day.
>> 
>> So the UEFI requirement actually is:
>> 
>> * Before starting an EFI payload a watchdog timer is started to reset
>>   the system after 5 minutes.
>> 
>> * This watchdog timer is cancelled as soon as the EFI payload calls
>>   ExitBootServices().
> 
> This requirement is currently fulfilled by a software watchdog in
> lib/efi_loader/efi_watchdog.c. We need an emulation because many boards
> don't offer a hardware watchdog in U-Boot.

If a board has a hardware watchdog then there will be two running,
correct? Could we detect (during runtime) if the hardware watchdog
is capable of doing the 5min timeout and then use that one instead
of a software timer?

Then we could have another behavior mode which can be EFI-compliant,
i.e. disable the watchdog before jumping to the OS when using bootm/go
but set it to 5mins and keep it enabled until ExitBootServices() is
called in the bootefi case.

>> The OpenBSD kernel in general does not supervise the watchdog unless
>> explicitly requested to do so by the user.  What may happen is that
>> the driver for the hardware stops the watchdog timer when it attaches,
>> but (a) that is just a side-effect and (b) watchdog timer support
>> isn't implemented for all supported SoCs.
>> 
>> The OpenBSD EFI bootloader does explicitly disable the watchdog timer
>> though by calling SetWatchdogTime() as soon as it starts.  This is to
>> prevent an automatic reboot if you leave it sitting at the boot prompt
>> for more than 5 minutes.  This is done across all our architectures
>> that support EFI, including amd64.  So maybe that hides any
>> non-conforming behaviour.
>> 
> 
> SetWatchdogTime() resets the software watchdog implemented in
> lib/efi_loader/efi_watchdog.c.
> 
> When an UEFI payload reads a key via the simple text input protocol or
> the extended simple text input protocol U-Boot calls efi_timer_check()
> which invokes WATCHDOG_RESET(). This is why hardware watchdogs don't
> kill an UEFI application waiting for keyboard input.

Ok I was already curious in which codepath the watchdog is kicked (its
also kicked in efi_check_event(), right?). But can we be sure that EFI
application will call that (periodically)?

-michael