On 24.04.25 22:19, Ying-Chun Liu (PaulLiu) wrote:
From: "Ying-Chun Liu (PaulLiu)" <paul....@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....@linaro.org>
Cc: Heinrich Schuchardt <xypron.g...@gmx.de>
Cc: Ilias Apalodimas <ilias.apalodi...@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;
+}

Reply via email to