Hi Heinrich,
Thanks for the review.
All of the issues in this commit will be fixed in the next version.
Thanks a lot.
Yours,
Paul
On Thu, 24 Apr 2025 at 23:34, Heinrich Schuchardt <xypron.g...@gmx.de>
wrote:
> 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;
> > +}
>
>
