On 09.10.17 08:05, Heinrich Schuchardt wrote: > On 10/09/2017 06:42 AM, Alexander Graf wrote: >> >> >> On 08.10.17 06:57, Heinrich Schuchardt wrote: >>> The watchdog is initialized with a 5 minute timeout period. >>> It can be reset by SetWatchdogTimer. >>> It is stopped by ExitBoottimeServices. >>> >>> Signed-off-by: Heinrich Schuchardt <xypron.g...@gmx.de> >>> --- >>> cmd/bootefi.c | 1 + >>> include/efi_loader.h | 4 +++ >>> lib/efi_loader/Makefile | 2 +- >>> lib/efi_loader/efi_boottime.c | 15 ++--------- >>> lib/efi_loader/efi_watchdog.c | 59 >>> +++++++++++++++++++++++++++++++++++++++++++ >>> 5 files changed, 67 insertions(+), 14 deletions(-) >>> create mode 100644 lib/efi_loader/efi_watchdog.c >>> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >>> index b7087e3da8..24958ada46 100644 >>> --- a/cmd/bootefi.c >>> +++ b/cmd/bootefi.c >>> @@ -43,6 +43,7 @@ static void efi_init_obj_list(void) >>> #ifdef CONFIG_GENERATE_SMBIOS_TABLE >>> efi_smbios_register(); >>> #endif >>> + efi_watchdog_register(); >>> >>> /* Initialize EFI runtime services */ >>> efi_reset_system_init(); >>> diff --git a/include/efi_loader.h b/include/efi_loader.h >>> index e1179b7dcd..223d8d8222 100644 >>> --- a/include/efi_loader.h >>> +++ b/include/efi_loader.h >>> @@ -163,6 +163,8 @@ int efi_disk_register(void); >>> int efi_gop_register(void); >>> /* Called by bootefi to make the network interface available */ >>> int efi_net_register(void); >>> +/* Called by bootefi to make the watchdog available */ >>> +int efi_watchdog_register(void); >>> /* Called by bootefi to make SMBIOS tables available */ >>> void efi_smbios_register(void); >>> >>> @@ -171,6 +173,8 @@ efi_fs_from_path(struct efi_device_path *fp); >>> >>> /* Called by networking code to memorize the dhcp ack package */ >>> void efi_net_set_dhcp_ack(void *pkt, int len); >>> +/* Called by efi_set_watchdog_timer to reset the timer */ >>> +efi_status_t efi_set_watchdog(unsigned long timeout); >>> >>> /* Called from places to check whether a timer expired */ >>> void efi_timer_check(void); >>> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile >>> index ddb978f650..83d879b686 100644 >>> --- a/lib/efi_loader/Makefile >>> +++ b/lib/efi_loader/Makefile >>> @@ -17,7 +17,7 @@ endif >>> obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o >>> obj-y += efi_image_loader.o efi_boottime.o efi_runtime.o efi_console.o >>> obj-y += efi_memory.o efi_device_path_to_text.o efi_device_path.o >>> -obj-y += efi_file.o efi_variable.o efi_bootmgr.o >>> +obj-y += efi_file.o efi_variable.o efi_bootmgr.o efi_watchdog.o >>> obj-$(CONFIG_LCD) += efi_gop.o >>> obj-$(CONFIG_DM_VIDEO) += efi_gop.o >>> obj-$(CONFIG_PARTITIONS) += efi_disk.o >>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c >>> index 30577f717e..81e7d818fc 100644 >>> --- a/lib/efi_loader/efi_boottime.c >>> +++ b/lib/efi_loader/efi_boottime.c >>> @@ -155,18 +155,6 @@ void efi_signal_event(struct efi_event *event) >>> event->is_queued = false; >>> } >>> >>> -/* >>> - * Write a debug message for an EPI API service that is not implemented >>> yet. >>> - * >>> - * @funcname function that is not yet implemented >>> - * @return EFI_UNSUPPORTED >>> - */ >>> -static efi_status_t efi_unsupported(const char *funcname) >>> -{ >>> - debug("EFI: App called into unimplemented function %s\n", funcname); >>> - return EFI_EXIT(EFI_UNSUPPORTED); >>> -} >>> - >>> /* >>> * Raise the task priority level. >>> * >>> @@ -1454,6 +1442,7 @@ static efi_status_t EFIAPI >>> efi_exit_boot_services(void *image_handle, >>> bootm_disable_interrupts(); >>> >>> /* Give the payload some time to boot */ >>> + efi_set_watchdog(0); >>> WATCHDOG_RESET(); >>> >>> return EFI_EXIT(EFI_SUCCESS); >>> @@ -1514,7 +1503,7 @@ static efi_status_t EFIAPI >>> efi_set_watchdog_timer(unsigned long timeout, >>> { >>> EFI_ENTRY("%ld, 0x%"PRIx64", %ld, %p", timeout, watchdog_code, >>> data_size, watchdog_data); >>> - return efi_unsupported(__func__); >>> + return EFI_EXIT(efi_set_watchdog(timeout)); >>> } >>> >>> /* >>> diff --git a/lib/efi_loader/efi_watchdog.c b/lib/efi_loader/efi_watchdog.c >>> new file mode 100644 >>> index 0000000000..50e95290ea >>> --- /dev/null >>> +++ b/lib/efi_loader/efi_watchdog.c >>> @@ -0,0 +1,59 @@ >>> +/* >>> + * EFI device path interface >> >> Not quite I guess? >> >>> + * >>> + * Copyright (c) 2017 Heinrich Schuchardt >>> + * >>> + * SPDX-License-Identifier: GPL-2.0+ >>> + */ >>> + >>> +#include <common.h> >>> +#include <efi_loader.h> >>> + >>> +static struct efi_event *watchdog_timer_event; >>> + >>> +static void EFIAPI efi_watchdog_timer_notify(struct efi_event *event, >>> + void *context) >>> +{ >>> + EFI_ENTRY("%p, %p", event, context); >>> + >>> + printf("\nEFI: Watchdog timeout\n"); >>> + EFI_CALL_VOID(efi_runtime_services.reset_system(EFI_RESET_COLD, >>> + EFI_SUCCESS, 0, NULL)); >>> + >>> + EFI_EXIT(EFI_UNSUPPORTED); >>> +} >>> + >>> +efi_status_t efi_set_watchdog(unsigned long timeout) >>> +{ >>> + efi_status_t r; >>> + >>> + if (timeout) >>> + /* Reset watchdog */ >>> + r = efi_set_timer(watchdog_timer_event, EFI_TIMER_RELATIVE, >>> + 10000000 * timeout); >>> + else >>> + /* Deactivate watchdog */ >>> + r = efi_set_timer(watchdog_timer_event, EFI_TIMER_STOP, 0); >>> + return r; >>> +} >>> + >>> +/* This gets called from do_bootefi_exec(). */ >>> +int efi_watchdog_register(void) >>> +{ >>> + efi_status_t r; >>> + >>> + r = efi_create_event(EVT_TIMER | EVT_NOTIFY_SIGNAL, TPL_CALLBACK, >>> + efi_watchdog_timer_notify, NULL, >>> + &watchdog_timer_event); >> >> How useful is a timer based watchdog without proper timer implementation? >> > > Do we have boards where the timer does not work? > On these we would have problems with EFI applications.
Well, we don't actually do interrupts today. So while the timer in theory is "implemented", it's a polling timer. If the payload doesn't call something that polls it, we never trigger. The whole point of a watchdog is to trigger when something unexpected happens though ;). > >> Is there a specific case where you needed it working? > > Both EFI shell and iPXE reset the timer. Grub resets the watchdog timer too, but then never cares about it ever again. > >> >> Also, is there a good reason not to use the hardware watchdog when it's >> available? > How can I detect that the hardware watchdog is usable? From watchdog.h: #if defined(CONFIG_WATCHDOG) || defined(CONFIG_HW_WATCHDOG) #define INIT_FUNC_WATCHDOG_INIT init_func_watchdog_init, #define INIT_FUNC_WATCHDOG_RESET init_func_watchdog_reset, I guess the assumption is that if a watchdog config variable is set, it is usable ;). > Can I set hardware watchdogs to arbitrary timeouts? I don't know. In fact, I don't think the current watchdog infrastructure allows for any timeout setting. > We need the EFI timer to be working anyway for many applications. Fair enough - I guess we really do need to go the timer route... and probably implement real interrupts sooner or later. > >> >>> + if (r != EFI_SUCCESS) { >>> + printf("ERROR: Failed to register watchdog event\n"); >>> + return r; >>> + } >>> + /* Set watchdog to trigger after 5 minutes */ >>> + r = efi_set_watchdog(300); >> >> Is this expected somewhere? Does the spec mention default watchdog timeouts? > > UEFI Spec 2.7 Erratum A: > > <cite>The watchdog must be set to a period of 5 minutes. The EFI > Image may reset or disable the watchdog timer as needed. If control > is returned to the firmware's boot manager, the watchdog timer must > be disabled.</cite> Awesome, please note that reference in the comment :) Alex _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot