[v2,3/3] efi: add EFI_DEBUG_IMAGE_INFO for debug

From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>

This commit adds the functionality of generate EFI_DEBUG_IMAGE_INFO
while loading the image.

This feature is described in UEFI Spec 2.10. Section 18.4.3.

Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
V2: use Kconfig options to turn on/off this feature.
---
 include/efi_api.h             |   2 +
 include/efi_loader.h          |   5 ++
 lib/efi_loader/efi_boottime.c | 140 ++++++++++++++++++++++++++++++++++
 3 files changed, 147 insertions(+)

On 24.04.25 22:19, Ying-Chun Liu (PaulLiu) wrote:
> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>
> 
> This commit adds the functionality of generate EFI_DEBUG_IMAGE_INFO
> while loading the image.
> 
> This feature is described in UEFI Spec 2.10. Section 18.4.3.
> 
> Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> V2: use Kconfig options to turn on/off this feature.
> ---
>   include/efi_api.h             |   2 +
>   include/efi_loader.h          |   5 ++
>   lib/efi_loader/efi_boottime.c | 140 ++++++++++++++++++++++++++++++++++
>   3 files changed, 147 insertions(+)
> 
> diff --git a/include/efi_api.h b/include/efi_api.h
> index 0f4245091f1..ba87af17c1f 100644
> --- a/include/efi_api.h
> +++ b/include/efi_api.h
> @@ -571,6 +571,8 @@ struct efi_loaded_image {
>   #define EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS 0x01
>   #define EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED     0x02
>   
> +#define EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL  0x01
> +
>   struct efi_debug_image_info_normal {
>   	u32 image_info_type;
>   	struct efi_loaded_image *loaded_image_protocol_instance;
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 370bc042f70..babe23a8a83 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -617,6 +617,11 @@ efi_status_t efi_root_node_register(void);
>   efi_status_t efi_initialize_system_table(void);
>   /* Called by bootefi to initialize debug */
>   efi_status_t efi_initialize_system_table_pointer(void);
> +/* Called by efi_load_image for register debug info */
> +void efi_core_new_debug_image_info_entry(u32 image_info_type,
> +					 struct efi_loaded_image *loaded_image,
> +					 efi_handle_t image_handle);
> +void efi_core_remove_debug_image_info_entry(efi_handle_t image_handle);
>   /* efi_runtime_detach() - detach unimplemented runtime functions */
>   void efi_runtime_detach(void);
>   /* efi_convert_pointer() - convert pointer to virtual address */
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index e2665459e44..0d4df5ee6ae 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -2130,6 +2130,14 @@ efi_status_t EFIAPI efi_load_image(bool boot_policy,
>   		*image_handle = NULL;
>   		free(info);
>   	}
> +
> +	if (IS_ENABLED(CONFIG_EFI_DEBUG_SUPPORT_TABLE)) {
> +		if (*image_handle) {
> +			efi_core_new_debug_image_info_entry(EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL,
> +							    info,
> +							    *image_handle);
> +		}
> +	}
>   error:
>   	return EFI_EXIT(ret);
>   }
> @@ -3359,6 +3367,9 @@ efi_status_t EFIAPI efi_unload_image(efi_handle_t image_handle)
>   		ret = EFI_INVALID_PARAMETER;
>   		goto out;
>   	}
> +	if (IS_ENABLED(CONFIG_EFI_DEBUG_SUPPORT_TABLE)) {
> +		efi_core_remove_debug_image_info_entry(image_handle);
> +	}
>   	switch (efiobj->type) {
>   	case EFI_OBJECT_TYPE_STARTED_IMAGE:
>   		/* Call the unload function */
> @@ -4045,6 +4056,10 @@ efi_m_debug_info_table_header = {
>   	NULL
>   };
>

Please, describe this field.

> +static u32 efi_m_max_table_entries;
> +

> +#define EFI_DEBUG_TABLE_ENTRY_SIZE  (sizeof(void *))

Wouldn't it be more readable to use sizeof(union efi_debug_image_info *) 
instead of this constant?

> +
>   const efi_guid_t efi_debug_image_info_table_guid =
>   	EFI_DEBUG_IMAGE_INFO_TABLE_GUID;
>   
> @@ -4095,3 +4110,128 @@ efi_status_t efi_initialize_system_table_pointer(void)
>   
>   	return ret;
>   }
> +
> +/**
> + * Add a new efi_loaded_image structure to the efi_debug_image_info Table.
> + * Re-Allocates the table if it's not large enough to accomidate another
> + * entry.
> + *
> + * @param image_info_type  type of debug image information
> + * @param loaded_image     pointer to the loaded image protocol for the image
> + *                         being loaded
> + * @param image_handle     image handle for the image being loaded
> + **/
> +void efi_core_new_debug_image_info_entry(u32 image_info_type,
> +					 struct efi_loaded_image *loaded_image,
> +					 efi_handle_t image_handle)

Reallocation can fail. The return value type void seems not to be adequate.

> +{
> +	union efi_debug_image_info *table;
> +	union efi_debug_image_info *new_table;
> +	u32 index;
> +	u32 table_size;
> +
> +	/* Set the flag indicating that we're in the process of updating
> +	 * the table.
> +	 */
> +	efi_m_debug_info_table_header.update_status |=
> +		EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS;
> +
> +	table = efi_m_debug_info_table_header.efi_debug_image_info_table;
> +
> +	if (efi_m_debug_info_table_header.table_size < efi_m_max_table_entries) {
> +		/* We still have empty entries in the Table, find the first
> +		 * empty entry.
> +		 */
> +		index = 0;
> +		while (table[index].normal_image)
> +			index++;
> +	} else {
> +		/* table is full, re-allocate another page for a larger

"Re-allocate the buffer increasing the size by 4 KiB" would be clearer.

> +		 * table.
> +		 */
> +		table_size = efi_m_max_table_entries * EFI_DEBUG_TABLE_ENTRY_SIZE;
> +		efi_allocate_pool(EFI_RUNTIME_SERVICES_DATA,
> +				  table_size + EFI_PAGE_SIZE,
> +				  (void **)&new_table);
> +
> +		if (!new_table) {
> +			efi_m_debug_info_table_header.update_status &=
> +				~EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS;
> +			return;
> +		}
> +
> +		memset(new_table, 0, table_size + EFI_PAGE_SIZE);

Please, carve out an efi_realloc() function and place it in 
lib/efi_loader/efi_memory.c beside efi_alloc().

> +
> +		/* Copy the old table into the new one. */
> +		memcpy(new_table, table, table_size);
> +		/* Free the old table. */
> +		efi_free_pool(table);
> +		/* Update the table header. */
> +		table = new_table;
> +		efi_m_debug_info_table_header.efi_debug_image_info_table =
> +			new_table;
> +
> +		/* Enlarge the max table entries and set the first empty
> +		 * entry index to be the original max table entries.
> +		 */
> +		index = efi_m_max_table_entries;
> +		efi_m_max_table_entries +=
> +			EFI_PAGE_SIZE / EFI_DEBUG_TABLE_ENTRY_SIZE;
> +	}
> +
> +	/* Allocate data for new entry. */
> +	efi_allocate_pool(EFI_RUNTIME_SERVICES_DATA,
> +			  sizeof(struct efi_debug_image_info_normal),
> +			  (void **)(&table[index].normal_image));

efi_allocate_pool() will consume a whole page. Shouldn't we have an 
array of consecutive structures and use realloc_pool() to enlarge it if 
we load too many images?

> +	if (table[index].normal_image) {
> +		/* Update the entry. */
> +		table[index].normal_image->image_info_type = image_info_type;
> +		table[index].normal_image->loaded_image_protocol_instance =
> +			loaded_image;

All protocols become invalid after ExitBootServices(). Why would you use 
EFI_RUNTIME_SERVICES_DATA to store references to something that does not 
exist at runtime?

EDK II uses AllocateZeroPool() which does not use EfiRuntimeServicesData.

Best regards

Heinrich

> +		table[index].normal_image->image_handle = image_handle;
> +
> +		/* Increase the number of EFI_DEBUG_IMAGE_INFO elements and
> +		 * set the efi_m_debug_info_table_header in modified status.
> +		 */
> +		efi_m_debug_info_table_header.table_size++;
> +		efi_m_debug_info_table_header.update_status |=
> +			EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;
> +	}
> +
> +	efi_m_debug_info_table_header.update_status &=
> +		~EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS;
> +}
> +
> +void efi_core_remove_debug_image_info_entry(efi_handle_t image_handle)
> +{
> +	union efi_debug_image_info *table;
> +	u32 index;
> +
> +	efi_m_debug_info_table_header.update_status |=
> +		EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS;
> +
> +	table = efi_m_debug_info_table_header.efi_debug_image_info_table;
> +
> +	for (index = 0; index < efi_m_max_table_entries; index++) {
> +		if (table[index].normal_image &&
> +		    table[index].normal_image->image_handle == image_handle) {
> +			/* Found a match. Free up the record, then NULL the
> +			 * pointer to indicate the slot is free.
> +			 */
> +			efi_free_pool(table[index].normal_image);
> +			table[index].normal_image = NULL;
> +
> +			/* Decrease the number of EFI_DEBUG_IMAGE_INFO
> +			 * elements and set the efi_m_debug_info_table_header
> +			 * in modified status.
> +			 */
> +			efi_m_debug_info_table_header.table_size--;
> +			efi_m_debug_info_table_header.update_status |=
> +				EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED;
> +			break;
> +		}
> +	}
> +
> +	efi_m_debug_info_table_header.update_status &=
> +		~EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS;
> +}