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

[Ilias Apalodimas]

Hi Jose, 

On Fri, Feb 19, 2021 at 06:04:20PM +0000, Jose Marinho wrote:
> The ESRT is initialised during efi_init_objlist after
> efi_initialize_system_table().
> 
> The ESRT is initially created with size for 50 FW image entries.
> The ESRT is resized when it runs out of space. Every resize adds 50
> additional entries.
> The ESRT is populated from information provided by FMP instances only.
> 
> 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: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> CC: Andre Przywara <andre.przywara at arm.com>
> CC: Alexander Graf <agraf at csgraf.de>
> CC: nd at arm.com
> 
> ---
>  cmd/efidebug.c               |   4 +
>  include/efi_api.h            |  21 ++
>  include/efi_loader.h         |  20 ++
>  lib/efi_loader/Kconfig       |   7 +
>  lib/efi_loader/Makefile      |   1 +
>  lib/efi_loader/efi_capsule.c |   8 +
>  lib/efi_loader/efi_esrt.c    | 522 +++++++++++++++++++++++++++++++++++
>  lib/efi_loader/efi_setup.c   |   6 +
>  8 files changed, 589 insertions(+)
>  create mode 100644 lib/efi_loader/efi_esrt.c
> 
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index bbbcb0a546..a7dace2f80 100644
> --- a/cmd/efidebug.c

[...]

> +static efi_status_t
> +efi_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)
> +{
> +	if (guidcmp(&entry->fw_class, &img_info->image_type_id)) {
> +		EFI_PRINT("ESRT entry %pUL mismatches img_type_id %pUL\n",
> +			  &entry->fw_class, &img_info->image_type_id);
> +		return EFI_INVALID_PARAMETER;
> +	}
> +
> +	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;
> +	}

You can ditch the {} here

> +
> +	/*
> +	 * 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.

[...]

> +static
> +efi_status_t efi_esrt_allocate_install(struct efi_boot_services *bt,
> +				       u32 num_entries)
> +{
> +	efi_status_t ret;
> +	struct efi_system_resource_table *new_esrt;
> +	u32 size = efi_esrt_entries_to_size(num_entries);
> +	efi_guid_t esrt_guid = efi_esrt_guid;
> +
> +	/* Allocated pages must be on the lower 32bit address space. */
> +	new_esrt = (struct efi_system_resource_table *)(uintptr_t)U32_MAX;

I don't think this is needed

> +
> +	/* Reserve num_pages for ESRT */
> +	ret = bt->allocate_pool(EFI_BOOT_SERVICES_DATA, size,
> +				(void **)&new_esrt);

allocate_pool will call allocate_pages with EFI_ALLOCATE_ANY_PAGES not 
EFI_ALLOCATE_ADDRESS (which I assume it was your intention here).
So you either need to check the address after the allocation or just don't
require it to be in the lower 32bit space.

> +
> +	if (ret != EFI_SUCCESS) {
> +		EFI_PRINT("ESRT cannot allocate memory for %d entries (%d bytes)\n",
> +			  num_entries, efi_esrt_entries_to_size(num_entries));
> +
> +		return ret;
> +	}
> +
> +	new_esrt->fw_resource_count_max = num_entries;
> +	new_esrt->fw_resource_count = 0;
> +	new_esrt->fw_resource_version = EFI_ESRT_VERSION;
> +
> +	/* Install the ESRT in the system configuration table. */
> +	ret = bt->install_configuration_table(&esrt_guid, (void *)new_esrt);
> +	if (ret != EFI_SUCCESS) {
> +		EFI_PRINT("ESRT failed to install the ESRT in the system table\n");
> +		return ret;
> +	}
> +
> +	/* If there was a previous ESRT, deallocate its memory now. */
> +	if (esrt)
> +		ret = bt->free_pool(esrt);
> +
> +	esrt = new_esrt;
> +
> +	return EFI_SUCCESS;
> +}

[...]

Regards
/Ilias