[RFC RESEND 1/2] efi: Add ESRT to the EFI system table

[Heinrich Schuchardt]

On 28.01.21 14:29, Jose Marinho wrote:
> The ESRT is initialised during efi_init_objlist, after
> efi_initialize_system_table().
>
> In this commit, the ESRT occupies at most one page.
> During initialization, the list of all the already existing
> FMP instances is traversed in order to fill the corresponsing entries.
>
> Later, when any FMP is added, the corresponding ESRT entries are
> initialized as long as there is available entries in the table.
>
> Signed-off-by: Jose Marinho <jose.marinho at arm.com>
> CC: Heinrich Schuchardt	<xypron.glpk at gmx.de>
> CC: Sughosh Ganu <sughosh.ganu at linaro.org>
> CC: AKASHI Takahiro <takahiro.akashi at linaro.org>
> CC: Andre Przywara <andre.przywara at arm.com>
> CC: Alexander Graf <agraf at csgraf.de>
> CC: nd at arm.com
> ---
>  include/efi_api.h             |  21 +++
>  include/efi_loader.h          |  22 +++
>  lib/efi_loader/Makefile       |   1 +
>  lib/efi_loader/efi_boottime.c |  12 ++
>  lib/efi_loader/efi_esrt.c     | 294 ++++++++++++++++++++++++++++++++++
>  lib/efi_loader/efi_setup.c    |   4 +
>  6 files changed, 354 insertions(+)
>  create mode 100644 lib/efi_loader/efi_esrt.c
>
> diff --git a/include/efi_api.h b/include/efi_api.h
> index 48e48a6263..f4a88603e9 100644
> --- a/include/efi_api.h
> +++ b/include/efi_api.h
> @@ -1722,6 +1722,23 @@ struct efi_load_file_protocol {
>  					 void *buffer);
>  };
>
> +struct efi_system_resource_entry {
> +	efi_guid_t fw_class;
> +	uint32_t fw_type;
> +	uint32_t fw_version;
> +	uint32_t lowest_supported_fw_version;
> +	uint32_t capsule_flags;
> +	uint32_t last_attempt_version;
> +	uint32_t last_attempt_status;
> +} __attribute__((__packed__));
> +
> +struct efi_system_resource_table {
> +	uint32_t fw_resource_count;
> +	uint32_t fw_resource_count_max;
> +	uint64_t fw_resource_version;
> +	struct efi_system_resource_entry entries[];
> +} __attribute__((__packed__));
> +
>  /* Boot manager load options */
>  #define LOAD_OPTION_ACTIVE		0x00000001
>  #define LOAD_OPTION_FORCE_RECONNECT	0x00000002
> @@ -1740,6 +1757,10 @@ struct efi_load_file_protocol {
>  #define ESRT_FW_TYPE_DEVICEFIRMWARE	0x00000002
>  #define ESRT_FW_TYPE_UEFIDRIVER		0x00000003
>
> +#define EFI_SYSTEM_RESOURCE_TABLE_GUID\
> +	EFI_GUID(0xb122a263, 0x3661, 0x4f68,\
> +		0x99, 0x29, 0x78, 0xf8, 0xb0, 0xd6, 0x21, 0x80 )

Dear Joel,

thank you for addressing this missing piece in U-Boot.

Please, add the GUID to guid_list[] in cmd/efidebug.c.

> +
>  /* Last Attempt Status Values */
>  #define LAST_ATTEMPT_STATUS_SUCCESS			0x00000000
>  #define LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL		0x00000001
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index f470bbd636..7b20ec1ad2 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -884,4 +884,26 @@ static inline efi_status_t efi_launch_capsules(void)
>
>  #endif /* CONFIG_IS_ENABLED(EFI_LOADER) */
>
> +/*
> + * Install the ESRT system table.
> + *
> + * @return	status code
> + */
> +efi_status_t efi_esrt_register(void);
> +
> +/**
> + * esrt_add_from_fmp() - Populates a sequence of ESRT entries from the FW
> + * images in the FMP.
> + *
> + * @fmp:        the fmp from which fw images are added to the ESRT
> + * @image_type: the type of the FW image in the ESRT entry
> + * @flags:      the capsule flags to be set in the ESRT entry
> + *
> + * Return:
> + * - EFI_SUCCESS if all the FW images in the FMP are added to the ESRT
> + * - Error status otherwise
> + */
> +efi_status_t esrt_add_from_fmp(struct efi_firmware_management_protocol *fmp,
> +	u32 image_type, u32 flags);
> +
>  #endif /* _EFI_LOADER_H */
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index 10b42e8847..dec791b310 100644
> --- a/lib/efi_loader/Makefile
> +++ b/lib/efi_loader/Makefile
> @@ -52,6 +52,7 @@ obj-y += efi_variable.o
>  obj-$(CONFIG_EFI_VARIABLES_PRESEED) += efi_var_seed.o
>  endif
>  obj-y += efi_watchdog.o
> +obj-y += efi_esrt.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 ce658a8e73..8077b467c7 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1150,6 +1150,18 @@ efi_status_t efi_add_protocol(const efi_handle_t handle,
>
>  	if (!guidcmp(&efi_guid_device_path, protocol))
>  		EFI_PRINT("installed device path '%pD'\n", protocol_interface);
> +
> +	if (!guidcmp(&efi_guid_firmware_management_protocol, protocol)) {
> +		EFI_PRINT("installed FMP, updating ESRT entries\n");
> +
> +		ret = esrt_add_from_fmp((struct efi_firmware_management_protocol *)
> +			protocol_interface, ESRT_FW_TYPE_UNKNOWN, 0);
> +		if (ret != EFI_SUCCESS) {
> +			EFI_PRINT("Failed to update FMP ESRT entries\n");
> +			return ret;
> +		}

On some systems we are very tight on memory. So we should have a
configuration setting to disable the ESRT code.

Calling RegisterProtocolNotify() offers a mor UEFI style implementation
alternative but would need more code.

> +	}
> +
>  	return EFI_SUCCESS;
>  }
>
> diff --git a/lib/efi_loader/efi_esrt.c b/lib/efi_loader/efi_esrt.c
> new file mode 100644
> index 0000000000..c651ce6c73
> --- /dev/null
> +++ b/lib/efi_loader/efi_esrt.c
> @@ -0,0 +1,294 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + *  EFI application ESRT tables support
> + *
> + *  Copyright (C) 2021 Arm Ltd.
> + */
> +
> +#include <common.h>
> +#include <efi_loader.h>
> +#include <log.h>
> +#include <efi_api.h>
> +#include <malloc.h>
> +
> +static const efi_guid_t esrt_guid = EFI_SYSTEM_RESOURCE_TABLE_GUID;
> +static bool esrt_initialized;
> +static struct efi_system_resource_table *esrt;
> +
> +#define EFI_ESRT_NUM_PAGES 1
> +
> +#define EFI_ESRT_SIZE (EFI_ESRT_NUM_PAGES * EFI_PAGE_SIZE)
> +
> +#define EFI_ESRT_ENTRIES_MAX ((EFI_ESRT_SIZE -             \
> +	sizeof(struct efi_system_resource_table)) /    \
> +	sizeof(struct efi_system_resource_entry))
> +
> +#define EFI_ESRT_VERSION 1
> +
> +/**
> + * esrt_image_info_to_entry() - copy the information present in a fw image
> + * descriptor to a ESRT entry.
> + *
> + * @img_info:     the source image info descriptor
> + * @entry:        pointer to the ESRT entry to be filled
> + * @desc_version: the version of the elements in img_info
> + * @image_type:   the image type value to be set in the ESRT entry
> + * @flags:        the capsule flags value to be set in the ESRT entry
> + *
> + */
> +void
> +esrt_image_info_to_entry(struct efi_firmware_image_descriptor *img_info,
> +			 struct efi_system_resource_entry *entry,
> +			 u32 desc_version, u32 image_type, u32 flags)
> +{
> +	guidcpy(&entry->fw_class, &img_info->image_type_id);
> +	entry->fw_version = img_info->version;
> +
> +	entry->fw_type = image_type;
> +	entry->capsule_flags = flags;
> +
> +	/*
> +	 * The field lowest_supported_image_version is only present
> +	 * on image info structure of version 2 or greater.
> +	 * See the EFI_FIRMWARE_IMAGE_DESCRIPTOR definition in UEFI.
> +	 */
> +	if (desc_version >= 2) {
> +		entry->lowest_supported_fw_version =
> +			img_info->lowest_supported_image_version;
> +	} else {
> +		entry->lowest_supported_fw_version = 0;
> +	}
> +
> +	/*
> +	 * The fields last_attempt_version and last_attempt_status
> +	 * are only present on image info structure of version 3 or
> +	 * greater.
> +	 * See the EFI_FIRMWARE_IMAGE_DESCRIPTOR definition in UEFI.
> +	 */
> +	if (desc_version >= 3) {
> +		entry->last_attempt_version =
> +			img_info->last_attempt_version;
> +
> +		entry->last_attempt_status =
> +			img_info->last_attempt_status;
> +	} else {
> +		entry->last_attempt_version = 0;
> +		entry->last_attempt_status = EFI_SUCCESS;
> +	}
> +}
> +
> +/**
> + * esrt_find_entry() - Obtain the ESRT entry for the image with guid

%s/guid/GUID/

> + * img_fw_class.

%s/img_fw_class/@img_fw_class/

> + *
> + * If the img_fw_class is not yet present in the ESRT, this function
> + * reserves the tail element of the current ESRT as the entry for that fw_clas.
> + * The number of elements in the ESRT is updated in that case.
> + *
> + * @fw_img_class: the guid of the fw image which ESRT entry we want to obtain.
> + *
> + * Return:
> + *  - a pointer to the ESRT entry for the image with guid img_fw_class,
> + *  - NULL if there is no more space in the ESRT.
> + */
> +static
> +struct efi_system_resource_entry *esrt_find_entry(efi_guid_t *img_fw_class)
> +{
> +	// if this function is called before the esrt init then it is erroneous
> +	// behaviour.
> +	assert(esrt_initialized);

assert is a nop if _DEBUG = 0.
Why don't you return NULL in this case?

Can this really occur?

> +
> +	const u32 filled_entries = esrt->fw_resource_count;
> +	const u32 max_entries = esrt->fw_resource_count_max;
> +	struct efi_system_resource_entry *entry = esrt->entries;
> +
> +	for (u32 idx; idx < filled_entries; idx++) {
> +		if (!guidcmp(&entry[idx].fw_class, img_fw_class)) {
> +			log_debug("EFI ESRT: found entry for image %pUl\n",
> +				  img_fw_class);
> +			return &entry[idx];
> +		}
> +	}
> +
> +	if (filled_entries == max_entries) {
> +		log_warning("EFI ESRT: table is full\n");
> +		return NULL;
> +
> +	} else {
> +		/*
> +		 * This is a new entry for a fw image, increment the element
> +		 * number in the table and set the fw_class field.
> +		 */
> +		esrt->fw_resource_count++;
> +		entry[filled_entries].fw_class = *img_fw_class;
> +		return &entry[filled_entries];
> +	}
> +}
> +
> +/**
> + * esrt_add_from_fmp() - Populates a sequence of ESRT entries from the FW
> + * images in the FMP.
> + *
> + * @fmp:        the fmp from which fw images are added to the ESRT
> + * @image_type: the type of the FW image in the ESRT entry
> + * @flags:      the capsule flags to be set in the ESRT entry
> + *
> + * Return:
> + * - EFI_SUCCESS if all the FW images in the FMP are added to the ESRT
> + * - Error status otherwise
> + */
> +efi_status_t esrt_add_from_fmp(struct efi_firmware_management_protocol *fmp,
> +			       u32 image_type, u32 flags)
> +{
> +	struct efi_system_resource_entry *entry = NULL;
> +	size_t info_size = 0;
> +	struct efi_firmware_image_descriptor *img_info = NULL;
> +	u32 desc_version;
> +	u8 desc_count;
> +	size_t desc_size;
> +	efi_status_t ret = EFI_SUCCESS;
> +
> +	if (!esrt_initialized)
> +		return EFI_NOT_READY;

Can this really occur?

> +
> +	ret = fmp->get_image_info(fmp, &info_size, img_info,
> +			&desc_version, &desc_count,
> +			&desc_size, NULL, NULL);
> +
> +	/*
> +	 * An input of info_size=0 should always lead to BUFFER_TO_SMALL return
> +	 * if this is not the case then the get_image_info implementation is
> +	 * anoumalous, hence the assert.

%s/anoumalous/anomalous/

> +	 */
> +	assert_noisy(ret == EFI_BUFFER_TOO_SMALL);

What do you want the user to do when he sees this message? Why would you
continue.

I guess the right thing is to return an error EFI_INVALID_PARAMETER if
the fmp protocol is not compliant. You can use EFI_PRINT() to write a
debug message.


> +
> +	img_info = malloc(info_size);

Always check the result of malloc().

> +
> +	ret = fmp->get_image_info(fmp, &info_size, img_info,
> +			&desc_version, &desc_count,
> +			&desc_size, NULL, NULL);
> +	if (ret != EFI_SUCCESS) {
> +		log_err("EFI ESRT: failed to obtain the FMP image info\n");
> +		goto out;
> +	}
> +
> +	/*
> +	 * Iterate over all the FW images in the FMP.
> +	 */
> +	for (u32 desc_idx = 0; desc_idx < desc_count; desc_idx++) {
> +		struct efi_firmware_image_descriptor *cur_img_info =
> +			(struct efi_firmware_image_descriptor *)
> +			((uintptr_t)img_info) + desc_idx * desc_size;
> +
> +		/*
> +		 * Obtain the ESRT entry for the FW image with fw_class
> +		 * equal to cur_img_info->image_type_id.
> +		 */
> +		entry = esrt_find_entry(&cur_img_info->image_type_id);
> +
> +		if (entry) {
> +			esrt_image_info_to_entry(cur_img_info, entry,
> +						 desc_version, image_type,
> +						 flags);
> +		} else {
> +			log_err("EFI ESRT: failed to add esrt entry for %pUl\n",
> +				&cur_img_info->image_type_id);
> +			continue;
> +		}
> +	}
> +
> +out:
> +	free(img_info);
> +	return true;

EFI_SUCCESS

> +}
> +
> +/**
> + * esrt_populate() - Populates the ESRT entries from the FMP instances
> + * registered so far and performs basic initialization of the ESRT fields.
> + *
> + * Return:
> + * - true if ESRT initialized successfully.
> + * - false otherwise.
> + */
> +static bool esrt_populate(void)
> +{
> +	efi_handle_t *handle = NULL;
> +	size_t no_handles = 0;
> +	struct efi_firmware_management_protocol *fmp;
> +	efi_status_t ret;
> +
> +	esrt->fw_resource_count_max = EFI_ESRT_ENTRIES_MAX;
> +	esrt->fw_resource_version = EFI_ESRT_VERSION;
> +
> +	esrt_initialized = true;
> +
> +	/*
> +	 * Obtain the number of registered FMP handles.
> +	 */
> +	ret = EFI_CALL(efi_locate_handle_buffer(BY_PROTOCOL, &efi_guid_firmware_management_protocol,
> +						NULL, &no_handles,
> +						(efi_handle_t **)&handle));
> +
> +	if (ret != EFI_SUCCESS) {
> +		log_warning("EFI ESRT: failed to obtain FMP handles (%lx)\n",
> +			    ret);
> +		return esrt_initialized;
> +	}

Can't we initialize ESRT before any FMP is installed to get rid of this
function module?

> +
> +	log_debug("EFI ESRT: populate esrt from (%ld) avaialble FMP handles\n",
> +		  no_handles);
> +	/*
> +	 * Populate the ESRT entries with any already existing FMP.
> +	 */
> +	for (u32 idx = 0; idx < no_handles; idx++, handle++) {
> +		ret = EFI_CALL(efi_handle_protocol(*handle, &efi_guid_firmware_management_protocol,
> +						   (void **)&fmp));
> +		if (ret != EFI_SUCCESS) {
> +			log_err("EFI ESRT: Unable to find FMP handle (%d)\n",
> +				idx);
> +			continue;
> +		}
> +
> +		ret = esrt_add_from_fmp(fmp, ESRT_FW_TYPE_UNKNOWN, 0);
> +		if (ret != EFI_SUCCESS) {
> +			log_err("EFI ESRT: failed to add from FMP (%d)\n", idx);
> +			continue;
> +		}
> +	}
> +	return esrt_initialized;
> +}
> +
> +/**
> + * efi_esrt_register() - Install the ESRT system table.
> + *
> + * @return	status code
> + */
> +efi_status_t efi_esrt_register(void)
> +{
> +	esrt = (struct efi_system_resource_table *)(uintptr_t)U32_MAX;
> +	efi_status_t ret;
> +
> +	log_debug("EFI ESRT: ESRT creation start\n");
> +
> +	/* Reserve pages for ESRT */
> +	ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS,
> +				 EFI_RUNTIME_SERVICES_DATA, EFI_ESRT_NUM_PAGES,

Do we need an ESRT before the installation of the first FMP?

Why are you using a fixed number of pages? Doesn't the size depend on
the number of FMPs? Shouldn't the table be resized whenever a new
protocol is installed?

Unfortunately you never check if you overrun the allocated space. So I
just need an EFI driver that installs a high number of FMP and it will
crash U-Boot.

So I would drop this function and move (re-)allocating the ESRT to
esrt_add_from_fmp().

Best regards

Heinrich

> +		(uint64_t *)&esrt);
> +
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	if (!esrt_populate())
> +		log_warning("EFI ESRT: failed to initialize ESRT\n");
> +
> +	/* And expose them to our EFI payload */
> +	ret = efi_install_configuration_table(&esrt_guid, (void *)esrt);
> +	if (ret != EFI_SUCCESS) {
> +		log_err("EFI ESRT: failed to create ESRT\n");
> +		return ret;
> +	}
> +
> +	log_debug("EFI ESRT: table created\n");
> +
> +	return ret;
> +}
> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index 5800cbf6d4..7660a20822 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -231,6 +231,10 @@ efi_status_t efi_init_obj_list(void)
>  	if (ret != EFI_SUCCESS)
>  		goto out;
>
> +	ret = efi_esrt_register();
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
>  	if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
>  		ret = efi_tcg2_register();
>  		if (ret != EFI_SUCCESS)
>