Hi Heinrich,
Yes. I just copied the code from EDK2. Seems this is incorrect. I'll try to
ask some help from EDK2 people.
Actually I'm working on Android (AOSP)'s GBL project. It is an EFI
application, used as a bootloader for loading and booting Android images.
So some people are asking how to debug that EFI application on real
hardwares. And since GBL runs on both U-boot and EDK2, the debug method
should
follow the standard so that the downstream people can have a unified way to
debug the application.
I'll ask EDK2 people for help and then continue working on this patch.
Thanks a lot,
Paul
On Mon, 28 Apr 2025 at 11:40, Heinrich Schuchardt <xypron.g...@gmx.de>
wrote:
> On 4/23/25 23:31, 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>
> > ---
> > include/efi_api.h | 2 +
> > include/efi_loader.h | 5 ++
> > lib/efi_loader/efi_boottime.c | 136 ++++++++++++++++++++++++++++++++++
> > 3 files changed, 143 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..bf37580c06b 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -2130,6 +2130,12 @@ efi_status_t EFIAPI efi_load_image(bool
> boot_policy,
> > *image_handle = NULL;
> > free(info);
> > }
> > +
> > + 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 +3365,7 @@ efi_status_t EFIAPI efi_unload_image(efi_handle_t
> image_handle)
> > ret = EFI_INVALID_PARAMETER;
> > goto out;
> > }
> > + efi_core_remove_debug_image_info_entry(image_handle);
> > switch (efiobj->type) {
> > case EFI_OBJECT_TYPE_STARTED_IMAGE:
> > /* Call the unload function */
> > @@ -4045,6 +4052,10 @@ efi_m_debug_info_table_header = {
> > NULL
> > };
> >
> > +static u32 efi_m_max_table_entries;
> > +
> > +#define EFI_DEBUG_TABLE_ENTRY_SIZE (sizeof(void *))
> > +
> > const efi_guid_t efi_debug_image_info_table_guid =
> > EFI_DEBUG_IMAGE_INFO_TABLE_GUID;
> >
> > @@ -4095,3 +4106,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)
> > +{
> > + 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
> > + * 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);
> > +
> > + /* 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));
> > + 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;
> > + 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;
>
> It seems that you just copied the logic from EDK II instead of
> programming according to the UEFI specification.
>
> I cannot see that the UEFI specification allows NULL pointers inside the
> array.
>
> > +
> > + /* 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--;
>
> According to the UEFI specification the table_size must match the array
> size and not the number of non-NULL elements. If there is a NULL pointer
> in the middle of the table and you decrease the table_size value, the
> last non-NULL element is not accessible anymore.
>
> A correct implementation would move all elements after the deleted one
> one slot to the front.
>
> Best regards
>
> Heinrich
>
> > + 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;
> > +}
>
>
