[U-Boot] [PATCH 06/11] efi_loader: implement SetWatchdogTimer

Alexander Graf agraf at suse.de
Mon Oct 9 07:06:21 UTC 2017



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.glpk at 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


More information about the U-Boot mailing list