On 9/23/20 10:23 PM, Tom Rini wrote: > On Wed, Sep 23, 2020 at 10:01:29PM +0200, Heinrich Schuchardt wrote: >> On 9/23/20 9:02 PM, Tom Rini wrote: >>> On Wed, Sep 23, 2020 at 08:51:53PM +0200, Heinrich Schuchardt wrote: >>>> On 9/23/20 7:53 PM, Mark Kettenis wrote: >>>>>> Date: Wed, 23 Sep 2020 13:14:09 -0400 >>>>>> From: Tom Rini <tr...@konsulko.com> >>>>>> >>>>>> On Wed, Sep 23, 2020 at 07:01:54PM +0200, Mark Kettenis wrote: >>>>>>>> From: Michael Walle <mich...@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. >>> >>> No, EFI is going to need to figure out how to deal with the watchdog. >>> See below... >> >> Any active watchdog after ExitBootServices() is forbidden by the UEFI spec. >> >> 3) does not describe disabling at ExitBootServices(). > > Well, that's why I go back and forth on having the EFI loader say it's > changing the current watchdog behavior. > > But just because EFI loader is enabled by default doesn't mean it gets > to set the overall default behavior for watchdogs. It's an option in a > lot of cases where it's not actually used and a traditional method is > instead used. > >>>>>>> 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. >>> >>> OK. >>> >>>>> 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. >>> >>> I think we need to make things such that if there's a HW watchdog, our >>> EFI loader/services make use of it. And maybe just the Kconfig help >>> text ends up spelling out what the UEFI spec says happens with >>> watchdogs, so it's documented and expected behavior. >> >> There is a symbol CONFIG_WATCHDOG where mentioning UEFI might make >> sense. But I could not find any software watchdog implementation >> matching this symbol. Does it exist at all? > > Do we have a pure software watchdog? I'm not 100% sure off-hand. > >>> My concern is that >>> it's not obvious to the average user/developer what the behavior is >>> going to be in this case and that they'll expect a watchdog to fire in a >>> case where we've turned it off, and that's an important detail that's >>> just within the spec. >>> >> >> git grep -n hw_watchdog_reset >> tells us that most boards don't have a hardware watchdog. > > That's the non-DM function. And looking at where we have drivers for > SoC watchdogs under drivers/watchdog/ I disagree with "most". We keep > adding more, even. > >> There is no maintainer for drivers/watchdog/wdt-uclass.c. > > Yes, just general "Is DM, check with Simon". > >> doc/README.watchdog provides no description on the usage. > > Given the amount of docs you've been writing of late (thanks!) yes, > neither of us are surprised docs aren't in sync with implementation > right now. > >> We do not even have a routine to set a time out period in struct wdt_ops. > > Changing the timeout at run time?
That is what EFI_BOOT_SERVICES.SetWatchdogTimer() is about. The UEFI application can set the watchdog timeout to a multiple of 1 second (without upper limit) or disable it completely. But this must not interfere with our serial drivers which are using hardware watchdogs. And now look at include/configs/M5373EVB.h:27: #define CONFIG_WATCHDOG_TIMEOUT 3360 /* timeout in ms, max is 3.36 sec */ The current use of hardware watchdog does not fit well to UEFI. A possible design would be a 100 Hz watchdog, to which U-Boot drivers can add listeners. Best regards Heinrich > >> Hardware watchdogs are already used by some serial drivers. So we cannot >> independently use them for UEFI. > > Yes, that's where we typically service the watchdog. > >> This does not look enticing. > > OK. So long as you don't break the non-EFI case when EFI is enabled, > whatever needs doing to meet the spec, until someone has time to poke > this area a bit harder still. >
