[PATCH v4 06/16] efi_loader: define UpdateCapsule api

[AKASHI Takahiro]

Heinrich,

On Thu, Jul 23, 2020 at 05:54:27PM +0200, Heinrich Schuchardt wrote:
> On 22.07.20 08:05, AKASHI Takahiro wrote:
> > In this commit, skeleton functions for capsule-related API's are
> > added under CONFIG_EFI_UPDATE_CAPSULE configuration.
> > Detailed implementation for a specific capsule type will be added
> > in the succeeding patches.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > ---
> >  include/efi_api.h            |  12 +++
> >  include/efi_loader.h         |  13 +++
> >  lib/efi_loader/Kconfig       |  11 +++
> >  lib/efi_loader/Makefile      |   1 +
> >  lib/efi_loader/efi_capsule.c | 168 +++++++++++++++++++++++++++++++++++
> >  lib/efi_loader/efi_runtime.c | 104 +++++++++++++---------
> >  lib/efi_loader/efi_setup.c   |  33 +++++--
> >  7 files changed, 290 insertions(+), 52 deletions(-)
> >  create mode 100644 lib/efi_loader/efi_capsule.c
> >
> > diff --git a/include/efi_api.h b/include/efi_api.h
> > index 5744f6aed86d..c128a0a66ce8 100644
> > --- a/include/efi_api.h
> > +++ b/include/efi_api.h
> > @@ -217,6 +217,10 @@ enum efi_reset_type {
> >  #define CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE	0x00020000
> >  #define CAPSULE_FLAGS_INITIATE_RESET		0x00040000
> >
> > +#define EFI_CAPSULE_REPORT_GUID \
> > +	EFI_GUID(0x39b68c46, 0xf7fb, 0x441b, 0xb6, 0xec, \
> > +		 0x16, 0xb0, 0xf6, 0x98, 0x21, 0xf3)
> > +
> >  struct efi_capsule_header {
> >  	efi_guid_t capsule_guid;
> >  	u32 header_size;
> > @@ -224,6 +228,14 @@ struct efi_capsule_header {
> >  	u32 capsule_image_size;
> >  } __packed;
> >
> > +struct efi_capsule_result_variable_header {
> > +	u32 variable_total_size;
> > +	u32 reserved;
> > +	efi_guid_t capsule_guid;
> > +	struct efi_time capsule_processed;
> > +	efi_status_t capsule_status;
> > +} __packed;
> > +
> >  #define EFI_RT_SUPPORTED_GET_TIME			0x0001
> >  #define EFI_RT_SUPPORTED_SET_TIME			0x0002
> >  #define EFI_RT_SUPPORTED_GET_WAKEUP_TIME		0x0004
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index df8dc377257c..a754fb0ed460 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -208,6 +208,8 @@ extern const efi_guid_t efi_guid_cert_type_pkcs7;
> >
> >  /* GUID of RNG protocol */
> >  extern const efi_guid_t efi_guid_rng_protocol;
> > +/* GUID of capsule update result */
> > +extern const efi_guid_t efi_guid_capsule_report;
> >
> >  extern unsigned int __efi_runtime_start, __efi_runtime_stop;
> >  extern unsigned int __efi_runtime_rel_start, __efi_runtime_rel_stop;
> > @@ -795,6 +797,17 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
> >  /* runtime implementation of memcpy() */
> >  void efi_memcpy_runtime(void *dest, const void *src, size_t n);
> >
> > +/* Capsule update */
> > +efi_status_t EFIAPI efi_update_capsule(
> > +		struct efi_capsule_header **capsule_header_array,
> > +		efi_uintn_t capsule_count,
> > +		u64 scatter_gather_list);
> > +efi_status_t EFIAPI efi_query_capsule_caps(
> > +		struct efi_capsule_header **capsule_header_array,
> > +		efi_uintn_t capsule_count,
> > +		u64 *maximum_capsule_size,
> > +		u32 *reset_type);
> > +
> >  #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
> >
> >  /* Without CONFIG_EFI_LOADER we don't have a runtime section, stub it out */
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index cbd8fe8c0ad2..ee9ebe348ad9 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -93,6 +93,17 @@ config EFI_SET_TIME
> >  	  Provide the SetTime() runtime service at boottime. This service
> >  	  can be used by an EFI application to adjust the real time clock.
> >
> > +config EFI_HAVE_CAPSULE_SUPPORT
> > +	bool
> > +
> 
> This symbol is not needed. You already have EFI_RUNTIME_UPDATE_CAPSULE.

If you carefully read my code, you will notice that EFI_CAPSULE_ON_DISK
can be enabled without EFI_RUNTIME_UDPATE_CAPSULE which allows for
exporting UpdateCapsule API.

Due to the nature of this API, I'm reluctant to enable this interface,
which currently has a limited functionality under my implementation,
unconditionally as runtime API.
That is why I introduced a separate EFI_HAVE_CAPSULE_SUPPORT.

> > +config EFI_RUNTIME_UPDATE_CAPSULE
> > +	bool "UpdateCapsule() runtime service"
> > +	default n
> > +	select EFI_HAVE_CAPSULE_SUPPORT
> > +	help
> > +	  Select this option if you want to use UpdateCapsule and
> > +	  QueryCapsuleCapabilities API's.
> > +
> >  config EFI_DEVICE_PATH_TO_TEXT
> >  	bool "Device path to text protocol"
> >  	default y
> > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > index 441ac9432e99..54de0fe51b94 100644
> > --- a/lib/efi_loader/Makefile
> > +++ b/lib/efi_loader/Makefile
> > @@ -23,6 +23,7 @@ endif
> >  obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
> >  obj-y += efi_bootmgr.o
> >  obj-y += efi_boottime.o
> > +obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o
> >  obj-y += efi_console.o
> >  obj-y += efi_device_path.o
> >  obj-$(CONFIG_EFI_DEVICE_PATH_TO_TEXT) += efi_device_path_to_text.o
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > new file mode 100644
> > index 000000000000..cfe422bee924
> > --- /dev/null
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -0,0 +1,168 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + *  EFI Capsule
> > + *
> > + *  Copyright (c) 2018 Linaro Limited
> > + *			Author: AKASHI Takahiro
> > + */
> > +
> > +#include <common.h>
> > +#include <efi_loader.h>
> > +#include <fs.h>
> > +#include <malloc.h>
> > +#include <sort.h>
> > +
> > +const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID;
> > +
> > +/**
> > + * get_last_capsule - get the last capsule number
> > + *
> > + * Retrieve the number of capsule invoked last time from "CapsuleLast"
> 
> Retrieve the index of the last capsule from "CapsuleLast"

Okay. 'Index' would be less confusing.

> > + * variable.
> > + *
> > + * Return:
> > + * * > 0	- the last capsule number invoked
> 
> The UEFI spec says: "starting at Capsule0000".

I don't get your point.

The index used to store a capsule update result will be
incremented at every time an update is invoked either
by API or a capsule-on-disk capsule file.

> > + * * 0xffff	- on error, or no capsule invoked yet

I admit that the description can be misleading, but

> CapsuleFFFF may exist and is not an error.
> 
> If there is no capsule, return -1. So the next number will be 0000.

The result is the same. The new index starts at '0000'.
We don't have to distinguish the two cases.

> > + */
> > +static __maybe_unused int get_last_capsule(void)
> > +{
> 
> This function is called by efi_launch_capsules(). Why should it be
> __maybe_unused?
> 
> Just move the function into the same patch as the function consuming it.
> 
> > +	u16 value16[11]; /* "CapsuleXXXX": non-null-terminated */
> > +	char value[11], *p;
> > +	efi_uintn_t size;
> > +	unsigned long num = 0xffff;
> 
> int num = -1;

See above.

> > +	efi_status_t ret;
> > +
> > +	size = sizeof(value16);
> > +	ret = EFI_CALL(efi_get_variable(L"CapsuleLast",
> 
> Please, avoid EFI_CALL(). Use efi_get_variable_int().

Okay.

> > +					&efi_guid_capsule_report,
> > +					NULL, &size, value16));
> > +	if (ret != EFI_SUCCESS || u16_strncmp(value16, L"Capsule", 7))
> > +		goto err;
> > +
> > +	p = value;
> > +	utf16_utf8_strcpy(&p, value16);
> > +	strict_strtoul(&value[7], 16, &num);
> > +err:
> > +	return (int)num;
> 
> Please, avoid superflous conversions.

Okay if it doesn't generate a compiler warning.

> > +}
> > +
> > +/**
> > + * set_capsule_result - set a result variable
> > + * @capsule:		Capsule
> > + * @return_status:	Return status
> > + *
> > + * Create and set a result variable, "CapsuleXXXX", for the capsule,
> > + * @capsule.
> > + */
> > +static __maybe_unused
> > +void set_capsule_result(int num, struct efi_capsule_header *capsule,
> > +			efi_status_t return_status)
> > +{
> > +	char variable_name[12];
> > +	u16 variable_name16[12], *p;
> > +	struct efi_capsule_result_variable_header result;
> > +	struct efi_time time;
> > +	efi_status_t ret;
> > +
> > +	sprintf(variable_name, "Capsule%04X", num);
> > +	p = variable_name16;
> > +	utf8_utf16_strncpy(&p, variable_name, 11);
> > +	result.variable_total_size = sizeof(result);
> > +	result.capsule_guid = capsule->capsule_guid;
> > +	ret = EFI_CALL((*efi_runtime_services.get_time)(&time, NULL));
> > +	if (ret == EFI_SUCCESS)
> > +		memcpy(&result.capsule_processed, &time, sizeof(time));
> > +	else
> > +		memset(&result.capsule_processed, 0, sizeof(time));
> > +	result.capsule_status = return_status;
> > +	ret = EFI_CALL(efi_set_variable(variable_name16,
> 
> Please, avoid EFI_CALL().
> 
> > +					&efi_guid_capsule_report,
> > +					EFI_VARIABLE_NON_VOLATILE |
> > +					EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +					EFI_VARIABLE_RUNTIME_ACCESS,
> > +					sizeof(result), &result));
> > +	if (ret)
> > +		printf("EFI: creating %s failed\n", variable_name);
> 
> Please, use log_err().

I think that I have made the same comment several times before.
printf() is used widely even in efi_loader.

> > +}
> > +
> > +/**
> > + * efi_update_capsule() - process information from operating system
> > + * @capsule_header_array:	Array of virtual address pointers
> > + * @capsule_count:		Number of pointers in capsule_header_array
> > + * @scatter_gather_list:	Array of physical address pointers
> > + *
> > + * This function implements the UpdateCapsule() runtime service.
> > + *
> > + * See the Unified Extensible Firmware Interface (UEFI) specification for
> > + * details.
> > + *
> > + * Return:			status code
> > + */
> > +efi_status_t EFIAPI efi_update_capsule(
> > +		struct efi_capsule_header **capsule_header_array,
> > +		efi_uintn_t capsule_count,
> > +		u64 scatter_gather_list)
> 
> All parameters - especially efi_capsule_header - should be marked as const.

No. This is the API defined in UEFI specification.

> > +{
> > +	struct efi_capsule_header *capsule;
> > +	unsigned int i;
> > +	efi_status_t ret;
> > +
> > +	EFI_ENTRY("%p, %lu, %llu\n", capsule_header_array, capsule_count,
> > +		  scatter_gather_list);
> > +
> > +	if (!capsule_count) {
> > +		ret = EFI_INVALID_PARAMETER;
> > +		goto out;
> > +	}
> > +
> > +	ret = EFI_SUCCESS;
> 
> In the current state (after merging patches 1-6) the function does not
> update. So the return value must be EFI_UNSUPPORTED.

Okay.

> Can't you move the contents of this function to patch 7/16? Then there
> is one patch with the whole code of the function to review.

This patch is a remnant when I had another commit against
supporting variable updates via capsule files.

Patch#6 provides the whole framework for capsules while patch#7
(and later) supports a specific capsule type.
So I think that we should have separate patches.

> > +	for (i = 0, capsule = *capsule_header_array; i < capsule_count;
> > +	     i++, capsule = *(++capsule_header_array)) {
> > +	}
> > +out:
> > +	return EFI_EXIT(ret);
> > +}
> > +
> > +/**
> > + * efi_query_capsule_caps() - check if capsule is supported
> > + * @capsule_header_array:	Array of virtual pointers
> > + * @capsule_count:		Number of pointers in capsule_header_array
> > + * @maximum_capsule_size:	Maximum capsule size
> > + * @reset_type:			Type of reset needed for capsule update
> > + *
> > + * This function implements the QueryCapsuleCapabilities() runtime service.
> > + *
> > + * See the Unified Extensible Firmware Interface (UEFI) specification for
> > + * details.
> > + *
> > + * Return:			status code
> > + */
> > +efi_status_t EFIAPI efi_query_capsule_caps(
> > +		struct efi_capsule_header **capsule_header_array,
> > +		efi_uintn_t capsule_count,
> > +		u64 *maximum_capsule_size,
> > +		u32 *reset_type)
> > +{
> > +	struct efi_capsule_header *capsule __attribute__((unused));
> > +	unsigned int i;
> > +	efi_status_t ret;
> > +
> > +	EFI_ENTRY("%p, %lu, %p, %p\n", capsule_header_array, capsule_count,
> > +		  maximum_capsule_size, reset_type);
> > +
> > +	if (!maximum_capsule_size) {
> > +		ret = EFI_INVALID_PARAMETER;
> > +		goto out;
> > +	}
> > +
> > +	*maximum_capsule_size = U64_MAX;
> > +	*reset_type = EFI_RESET_COLD;
> > +
> > +	ret = EFI_SUCCESS;
> 
> EFI_UNSUPPORTED!

No. At least, two parameters are actually returned.

-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> > +	for (i = 0, capsule = *capsule_header_array; i < capsule_count;
> > +	     i++, capsule = *(++capsule_header_array)) {
> > +		/* TODO */
> > +	}
> > +out:
> > +	return EFI_EXIT(ret);
> > +}
> > diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> > index 91a45514488e..6227bda3a268 100644
> > --- a/lib/efi_loader/efi_runtime.c
> > +++ b/lib/efi_loader/efi_runtime.c
> > @@ -133,6 +133,10 @@ efi_status_t efi_init_runtime_supported(void)
> >  #ifdef CONFIG_EFI_HAVE_RUNTIME_RESET
> >  	rt_table->runtime_services_supported |= EFI_RT_SUPPORTED_RESET_SYSTEM;
> >  #endif
> > +	if (IS_ENABLED(CONFIG_EFI_RUNTIME_UPDATE_CAPSULE))
> > +		rt_table->runtime_services_supported |=
> > +			(EFI_RT_SUPPORTED_UPDATE_CAPSULE |
> > +			 EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES);
> >
> >  	ret = efi_install_configuration_table(&efi_rt_properties_table_guid,
> >  					      rt_table);
> > @@ -432,6 +436,50 @@ efi_status_t __weak __efi_runtime EFIAPI efi_set_time(struct efi_time *time)
> >  	return EFI_UNSUPPORTED;
> >  }
> >
> > +/**
> > + * efi_update_capsule_unsupported() - process information from operating system
> > + *
> > + * This function implements the UpdateCapsule() runtime service.
> > + *
> > + * See the Unified Extensible Firmware Interface (UEFI) specification for
> > + * details.
> > + *
> > + * @capsule_header_array:	pointer to array of virtual pointers
> > + * @capsule_count:		number of pointers in capsule_header_array
> > + * @scatter_gather_list:	pointer to array of physical pointers
> > + * Returns:			status code
> > + */
> > +efi_status_t __efi_runtime EFIAPI efi_update_capsule_unsupported(
> > +			struct efi_capsule_header **capsule_header_array,
> > +			efi_uintn_t capsule_count,
> > +			u64 scatter_gather_list)
> > +{
> > +	return EFI_UNSUPPORTED;
> > +}
> > +
> > +/**
> > + * efi_query_capsule_caps_unsupported() - check if capsule is supported
> > + *
> > + * This function implements the QueryCapsuleCapabilities() runtime service.
> > + *
> > + * See the Unified Extensible Firmware Interface (UEFI) specification for
> > + * details.
> > + *
> > + * @capsule_header_array:	pointer to array of virtual pointers
> > + * @capsule_count:		number of pointers in capsule_header_array
> > + * @maximum_capsule_size:	maximum capsule size
> > + * @reset_type:			type of reset needed for capsule update
> > + * Returns:			status code
> > + */
> > +efi_status_t __efi_runtime EFIAPI efi_query_capsule_caps_unsupported(
> > +			struct efi_capsule_header **capsule_header_array,
> > +			efi_uintn_t capsule_count,
> > +			u64 *maximum_capsule_size,
> > +			u32 *reset_type)
> > +{
> > +	return EFI_UNSUPPORTED;
> > +}
> > +
> >  /**
> >   * efi_is_runtime_service_pointer() - check if pointer points to runtime table
> >   *
> > @@ -455,6 +503,13 @@ void efi_runtime_detach(void)
> >  	efi_runtime_services.reset_system = efi_reset_system;
> >  	efi_runtime_services.get_time = efi_get_time;
> >  	efi_runtime_services.set_time = efi_set_time;
> > +	if (IS_ENABLED(CONFIG_EFI_RUNTIME_UPDATE_CAPSULE)) {
> > +		/* won't support at runtime */
> > +		efi_runtime_services.update_capsule =
> > +				efi_update_capsule_unsupported;
> > +		efi_runtime_services.query_capsule_caps =
> > +				efi_query_capsule_caps_unsupported;
> > +	}
> >
> >  	/* Update CRC32 */
> >  	efi_update_table_header_crc32(&efi_runtime_services.hdr);
> > @@ -863,50 +918,6 @@ static efi_status_t __efi_runtime EFIAPI efi_unimplemented(void)
> >  	return EFI_UNSUPPORTED;
> >  }
> >
> > -/**
> > - * efi_update_capsule() - process information from operating system
> > - *
> > - * This function implements the UpdateCapsule() runtime service.
> > - *
> > - * See the Unified Extensible Firmware Interface (UEFI) specification for
> > - * details.
> > - *
> > - * @capsule_header_array:	pointer to array of virtual pointers
> > - * @capsule_count:		number of pointers in capsule_header_array
> > - * @scatter_gather_list:	pointer to arry of physical pointers
> > - * Returns:			status code
> > - */
> > -efi_status_t __efi_runtime EFIAPI efi_update_capsule(
> > -			struct efi_capsule_header **capsule_header_array,
> > -			efi_uintn_t capsule_count,
> > -			u64 scatter_gather_list)
> > -{
> > -	return EFI_UNSUPPORTED;
> > -}
> > -
> > -/**
> > - * efi_query_capsule_caps() - check if capsule is supported
> > - *
> > - * This function implements the QueryCapsuleCapabilities() runtime service.
> > - *
> > - * See the Unified Extensible Firmware Interface (UEFI) specification for
> > - * details.
> > - *
> > - * @capsule_header_array:	pointer to array of virtual pointers
> > - * @capsule_count:		number of pointers in capsule_header_array
> > - * @maximum_capsule_size:	maximum capsule size
> > - * @reset_type:			type of reset needed for capsule update
> > - * Returns:			status code
> > - */
> > -efi_status_t __efi_runtime EFIAPI efi_query_capsule_caps(
> > -			struct efi_capsule_header **capsule_header_array,
> > -			efi_uintn_t capsule_count,
> > -			u64 *maximum_capsule_size,
> > -			u32 *reset_type)
> > -{
> > -	return EFI_UNSUPPORTED;
> > -}
> > -
> >  struct efi_runtime_services __efi_runtime_data efi_runtime_services = {
> >  	.hdr = {
> >  		.signature = EFI_RUNTIME_SERVICES_SIGNATURE,
> > @@ -924,7 +935,12 @@ struct efi_runtime_services __efi_runtime_data efi_runtime_services = {
> >  	.set_variable = efi_set_variable,
> >  	.get_next_high_mono_count = (void *)&efi_unimplemented,
> >  	.reset_system = &efi_reset_system_boottime,
> > +#ifdef CONFIG_EFI_RUNTIME_UPDATE_CAPSULE
> >  	.update_capsule = efi_update_capsule,
> >  	.query_capsule_caps = efi_query_capsule_caps,
> > +#else
> > +	.update_capsule = efi_update_capsule_unsupported,
> > +	.query_capsule_caps = efi_query_capsule_caps_unsupported,
> > +#endif
> >  	.query_variable_info = efi_query_variable_info,
> >  };
> > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > index 6196c0a06cd7..2fc0c5d091b8 100644
> > --- a/lib/efi_loader/efi_setup.c
> > +++ b/lib/efi_loader/efi_setup.c
> > @@ -117,6 +117,30 @@ static efi_status_t efi_init_secure_boot(void)
> >  }
> >  #endif /* CONFIG_EFI_SECURE_BOOT */
> >
> > +/**
> > + * efi_init_os_indications() - indicate supported features for OS requests
> > + *
> > + * Set the OsIndicationsSupported variable.
> > + *
> > + * Return:	status code
> > + */
> > +static efi_status_t efi_init_os_indications(void)
> > +{
> > +	u64 os_indications_supported = 0;
> > +
> > +	if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
> > +		os_indications_supported |=
> > +			EFI_OS_INDICATIONS_CAPSULE_RESULT_VAR_SUPPORTED;
> > +
> > +	return efi_set_variable_int(L"OsIndicationsSupported",
> > +				    &efi_global_variable_guid,
> > +				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +				    EFI_VARIABLE_RUNTIME_ACCESS |
> > +				    EFI_VARIABLE_READ_ONLY,
> > +				    sizeof(os_indications_supported),
> > +				    &os_indications_supported, false);
> > +}
> > +
> >  /**
> >   * efi_init_obj_list() - Initialize and populate EFI object list
> >   *
> > @@ -124,7 +148,6 @@ static efi_status_t efi_init_secure_boot(void)
> >   */
> >  efi_status_t efi_init_obj_list(void)
> >  {
> > -	u64 os_indications_supported = 0; /* None */
> >  	efi_status_t ret = EFI_SUCCESS;
> >
> >  	/* Initialize once only */
> > @@ -162,13 +185,7 @@ efi_status_t efi_init_obj_list(void)
> >  		goto out;
> >
> >  	/* Indicate supported features */
> > -	ret = efi_set_variable_int(L"OsIndicationsSupported",
> > -				   &efi_global_variable_guid,
> > -				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > -				   EFI_VARIABLE_RUNTIME_ACCESS |
> > -				   EFI_VARIABLE_READ_ONLY,
> > -				   sizeof(os_indications_supported),
> > -				   &os_indications_supported, false);
> > +	ret = efi_init_os_indications();
> >  	if (ret != EFI_SUCCESS)
> >  		goto out;
> >
> >
>