[RFC RESEND 1/2] efi: Add ESRT to the EFI system table
AKASHI Takahiro
takahiro.akashi at linaro.org
Fri Jan 29 06:26:49 CET 2021
First of all, one comment:
It would be nice to have a list of what features are supported
and what are not in this patch series.
For example, with this patch, I suspect
- FwVersion in ESRT entry will never be updated at capsule update
- LastAttemptVersion/LastAttemptStatus will not be sustained across reboots.
So I'm not sure that the proposed implementation is useful
in a practical manner.
On Thu, Jan 28, 2021 at 04:29:57PM +0100, Heinrich Schuchardt wrote:
> 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.
* I don't think that it is a good idea to add a protocol-specific
handling in a generic function.
* efi_add_protocol() is not even a right place to call esrt_from_fmp()
because
- calling get_image_info() makes sense only for a specific instance
of firmware image.
- as I said above, FwVersion should be updated logically at every
time when capsule update takes place.
Meanwhile, the initial entry in ESRT should be separately created at booting
depending on what "firmware" are to be provided on a specific system.
-Takahiro Akashi
> > + }
> > +
> > 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)
> >
>
More information about the U-Boot
mailing list