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.mari...@arm.com>
> CC: Heinrich Schuchardt <xypron.g...@gmx.de>
> CC: Sughosh Ganu <sughosh.g...@linaro.org>
> CC: AKASHI Takahiro <takahiro.aka...@linaro.org>
> CC: Andre Przywara <andre.przyw...@arm.com>
> CC: Alexander Graf <ag...@csgraf.de>
> CC: n...@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)
>
