[U-Boot] [PATCH v2 18/18] efi_loader: fix ExitBootServices

[Alexander Graf]

On 17.01.18 20:16, Heinrich Schuchardt wrote:
> This patch lets the implementation of ExitBootServices conform to
> the UEFI standard.
> 
> The timer events must be disabled before calling the notification
> functions of the exit boot services events.
> 
> The boot services must be disabled in the system table.
> 
> The handles in the system table should be defined as efi_handle_t.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> ---
> v2
> 	new patch
> ---
>  include/efi_api.h             |  6 +++---
>  lib/efi_loader/efi_boottime.c | 36 +++++++++++++++++++++++++++++++-----
>  2 files changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/include/efi_api.h b/include/efi_api.h
> index 0bc244444d..4252d11398 100644
> --- a/include/efi_api.h
> +++ b/include/efi_api.h
> @@ -247,11 +247,11 @@ struct efi_system_table {
>  	struct efi_table_hdr hdr;
>  	unsigned long fw_vendor;   /* physical addr of wchar_t vendor string */
>  	u32 fw_revision;
> -	unsigned long con_in_handle;
> +	efi_handle_t con_in_handle;
>  	struct efi_simple_input_interface *con_in;
> -	unsigned long con_out_handle;
> +	efi_handle_t con_out_handle;
>  	struct efi_simple_text_output_protocol *con_out;
> -	unsigned long stderr_handle;
> +	efi_handle_t stderr_handle;
>  	struct efi_simple_text_output_protocol *std_err;
>  	struct efi_runtime_services *runtime;
>  	struct efi_boot_services *boottime;
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 4b3b63e39a..2c5499e0c8 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1645,12 +1645,16 @@ static void efi_exit_caches(void)
>  }
>  
>  /*
> - * Stop boot services.
> + * Stop all boot services.
>   *
>   * This function implements the ExitBootServices service.
>   * See the Unified Extensible Firmware Interface (UEFI) specification
>   * for details.
>   *
> + * All timer events are disabled.
> + * For exit boot services events the notification function is called.
> + * The boot services are disabled in the system table.
> + *
>   * @image_handle	handle of the loaded image
>   * @map_key		key of the memory map
>   * @return		status code
> @@ -1662,16 +1666,24 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
>  
>  	EFI_ENTRY("%p, %ld", image_handle, map_key);
>  
> +	/* Make sure that notification functions are not called anymore */
> +	efi_tpl = TPL_HIGH_LEVEL;
> +
> +	/* Check if ExitBootServices has already been called */
> +	if (!systab.boottime)
> +		return EFI_EXIT(EFI_SUCCESS);
> +
>  	/* Notify that ExitBootServices is invoked. */
>  	for (i = 0; i < ARRAY_SIZE(efi_events); ++i) {
>  		if (efi_events[i].type != EVT_SIGNAL_EXIT_BOOT_SERVICES)
>  			continue;
> -		efi_signal_event(&efi_events[i]);
> +		if (!efi_events[i].notify_function)
> +			continue;
> +		EFI_CALL_VOID(efi_events[i].notify_function(
> +				&efi_events[i], efi_events[i].notify_context));

This basically just bypasses the event->notify_tpl check. Can't you add
an enum parameter to efi_signal_event to indicate EFI_SIGNAL_CHECK_TPL
vs EFI_SIGNAL_NOCHECK and only do the TPL check in there if
EFI_SIGNAL_CHECK_TPL is set?


Alex

>  	}
> -	/* Make sure that notification functions are not called anymore */
> -	efi_tpl = TPL_HIGH_LEVEL;
>  
> -	/* XXX Should persist EFI variables here */
> +	/* TODO Should persist EFI variables here */
>  
>  	board_quiesce_devices();
>  
> @@ -1681,6 +1693,20 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
>  	/* This stops all lingering devices */
>  	bootm_disable_interrupts();
>  
> +	/* Disable boottime services */
> +	systab.con_in_handle = NULL;
> +	systab.con_in = NULL;
> +	systab.con_out_handle = NULL;
> +	systab.con_out = NULL;
> +	systab.stderr_handle = NULL;
> +	systab.std_err = NULL;
> +	systab.boottime = NULL;
> +
> +	/* Recalculate CRC32 */
> +	systab.hdr.crc32 = 0;
> +	systab.hdr.crc32 = crc32(0, (const unsigned char *)&systab,
> +				 sizeof(struct efi_system_table));
> +
>  	/* Give the payload some time to boot */
>  	efi_set_watchdog(0);
>  	WATCHDOG_RESET();
>