[PATCH v2 2/3] efi: add EFI_DEBUG_IMAGE_INFO_TABLE for debug

[Paul Liu]

On Thu, 24 Apr 2025 at 23:02, Heinrich Schuchardt <xypron.glpk at gmx.de>
wrote:

> On 24.04.25 22:19, Ying-Chun Liu (PaulLiu) wrote:
> > From: "Ying-Chun Liu (PaulLiu)" <paul.liu at linaro.org>
> >
> > EFI_DEBUG_IMAGE_INFO_TABLE is used to store EFI_LOADED_IMAGE for
> > debug purpose. This commit adds the table to the EFI_CONFIGURATION_TABLE.
> >
> > This feature is described in UEFI Spec version 2.10. Section 18.4.
> >
> > Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu at linaro.org>
> > Cc: Heinrich Schuchardt <xypron.glpk at gmx.de>
> > Cc: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > ---
> >   include/efi_api.h             | 24 ++++++++++++++++++++++++
> >   lib/efi_loader/efi_boottime.c | 16 ++++++++++++++++
> >   2 files changed, 40 insertions(+)
> >
> > diff --git a/include/efi_api.h b/include/efi_api.h
> > index 5017d9037cd..0f4245091f1 100644
> > --- a/include/efi_api.h
> > +++ b/include/efi_api.h
> > @@ -238,6 +238,10 @@ enum efi_reset_type {
> >       EFI_GUID(0xcce33c35, 0x74ac, 0x4087, 0xbc, 0xe7, \
> >                0x8b, 0x29, 0xb0, 0x2e, 0xeb, 0x27)
> >
> > +#define EFI_DEBUG_IMAGE_INFO_TABLE_GUID \
> > +     EFI_GUID(0x49152e77, 0x1ada, 0x4764, 0xb7, 0xa2, \
> > +              0x7a, 0xfe, 0xfe, 0xd9, 0x5e, 0x8b)
> > +
> >   struct efi_conformance_profiles_table {
> >       u16 version;
> >       u16 number_of_profiles;
> > @@ -564,6 +568,26 @@ struct efi_loaded_image {
> >       efi_status_t (EFIAPI *unload)(efi_handle_t image_handle);
> >   };
> >
> > +#define EFI_DEBUG_IMAGE_INFO_UPDATE_IN_PROGRESS 0x01
> > +#define EFI_DEBUG_IMAGE_INFO_TABLE_MODIFIED     0x02
> > +
>
> All structures and unions and their elements should be describe according
> to
>
> https://docs.kernel.org/doc-guide/kernel-doc.html#structure-union-and-enumeration-documentation
>
> > +struct efi_debug_image_info_normal {
> > +     u32 image_info_type;
> > +     struct efi_loaded_image *loaded_image_protocol_instance;
> > +     efi_handle_t image_handle;
> > +};
>
> What are the assumptions about alignment?
> Should this structure be marked as packed like most UEFI structures?
>
>
Not aligned. This structure in EDK2 is not aligned, not packed. And it is
really not packed because the size of this structure is larger. 4 bytes are
padded in the 64-bit environment I think.


> > +
> > +union efi_debug_image_info {
>
> Why do we need the union? The first field of struct
> efi_debug_image_info_normal is image_info_type.
>
>
I just copied it from the EDK2. But we can remove it. I think edk2 wants to
support not only normal images. However, currently there is only a normal
image.
So it looks redundant.
I will remove this union in the next version.


> > +     u32 *image_info_type;
> > +     struct efi_debug_image_info_normal *normal_image;
> > +};
> > +
> > +struct efi_debug_image_info_table_header {
> > +     volatile u32 update_status;
>
> Why is volatile needed here?
>
>
The volatile is needed because we like the status to be updated immediately
to the memory.
If we are using a hardware jtag. And it stops the code at random places at
the beginning. It might have paused just inside these logics.
So this update_status is to tell the gdb python extensions to not
referencing EFI tables because it is not ready yet.
Thus this value cannot be optimized for updating later. And thus the
volatile is needed.



> Best regards
>
> Heinrich
>
> > +     u32 table_size;
> > +     union efi_debug_image_info *efi_debug_image_info_table;
> > +};
> > +
> >   #define EFI_DEVICE_PATH_PROTOCOL_GUID \
> >       EFI_GUID(0x09576e91, 0x6d3f, 0x11d2, \
> >                0x8e, 0x39, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
> > diff --git a/lib/efi_loader/efi_boottime.c
> b/lib/efi_loader/efi_boottime.c
> > index 366bc0d2b1d..e2665459e44 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -4038,6 +4038,16 @@ efi_status_t efi_initialize_system_table(void)
> >       return ret;
> >   }
> >
>
>
>
> > +struct efi_debug_image_info_table_header __efi_runtime_data
>
>
>
> > +efi_m_debug_info_table_header = {
> > +     0,
> > +     0,
> > +     NULL
> > +};
> > +
> > +const efi_guid_t efi_debug_image_info_table_guid =
> > +     EFI_DEBUG_IMAGE_INFO_TABLE_GUID;
> > +
> >   /**
> >    * efi_initialize_system_table() - Initialize system table
> >    *
> > @@ -4053,6 +4063,8 @@ efi_status_t
> efi_initialize_system_table_pointer(void)
> >       u64 addr;
> >       u64 aligned_addr;
> >       u32 crc32_value;
> > +     efi_guid_t debug_image_info_table_guid =
> > +             efi_debug_image_info_table_guid;
> >
> >       /* Allocate configuration table array */
> >       ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
> > @@ -4077,5 +4089,9 @@ efi_status_t
> efi_initialize_system_table_pointer(void)
> >                           (const unsigned char *)systab_pointer,
> >                           sizeof(struct efi_system_table_pointer));
> >       systab_pointer->crc32 = crc32_value;
> > +
> > +     ret = efi_install_configuration_table(&debug_image_info_table_guid,
> > +
>  &efi_m_debug_info_table_header);
> > +
> >       return ret;
> >   }
>
>