| Message ID | 20250424201927.2027257-3-paulliu@debian.org |
|---|---|
| State | New |
| Headers | show |
| Series | efi: Add EFI Debug Support Table feature | expand |
On 24.04.25 22:19, Ying-Chun Liu (PaulLiu) wrote: > From: "Ying-Chun Liu (PaulLiu)" <paul.liu@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@linaro.org> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> > Cc: Ilias Apalodimas <ilias.apalodimas@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? > + > +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. > + 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? 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; > }
On Thu, 24 Apr 2025 at 23:02, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 24.04.25 22:19, Ying-Chun Liu (PaulLiu) wrote: > > From: "Ying-Chun Liu (PaulLiu)" <paul.liu@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@linaro.org> > > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> > > Cc: Ilias Apalodimas <ilias.apalodimas@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; > > } > >
On 4/25/25 21:05, Paul Liu wrote: > > > On Thu, 24 Apr 2025 at 23:02, Heinrich Schuchardt <xypron.glpk@gmx.de > <mailto:xypron.glpk@gmx.de>> wrote: > > On 24.04.25 22:19, Ying-Chun Liu (PaulLiu) wrote: > > From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org > <mailto:paul.liu@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@linaro.org > <mailto:paul.liu@linaro.org>> > > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de > <mailto:xypron.glpk@gmx.de>> > > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org > <mailto:ilias.apalodimas@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 <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. All other structures in EDK II are packed. Did you look at the structure in a debugger? The compiler will assume that a pointer to "struct efi_debug_image_info_normal *" points to a sizeof(uintptr_t) aligned structure if no other indication is made. Best regards Heinrich > > > + > > +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; > > } >
On Fri, 25 Apr 2025 at 21:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 4/25/25 21:05, Paul Liu wrote: > > > > > > On Thu, 24 Apr 2025 at 23:02, Heinrich Schuchardt <xypron.glpk@gmx.de > > <mailto:xypron.glpk@gmx.de>> wrote: > > > > On 24.04.25 22:19, Ying-Chun Liu (PaulLiu) wrote: > > > From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org > > <mailto:paul.liu@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@linaro.org > > <mailto:paul.liu@linaro.org>> > > > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de > > <mailto:xypron.glpk@gmx.de>> > > > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org > > <mailto:ilias.apalodimas@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 <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. > > All other structures in EDK II are packed. Did you look at the structure > in a debugger? > > The compiler will assume that a pointer to "struct > efi_debug_image_info_normal *" points to a sizeof(uintptr_t) aligned > structure if no other indication is made. > > Best regards > > Heinrich > > > Hi Heinrich, Yes. All other structures in EDK II are packed. But this one is not packed. And yes, seems it is aligned to sizeof(uintptr_t) in edk2. How do we implement this if we want the alignment? I plan to just remove that union and use "struct efi_debug_image_info_normal" directly. But maybe the alignment is not important here? I'll do a bit of research on this. Because we are not searching for this structure in the debugger. We just read it from EFI_DEBUG_IMAGE_INFO_TABLE. Which will be in the Configuration Table with specific GUID. The only search in the debugger is for the SYSTEM_TABLE_POINTER structure. Yours, Paul
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 + +struct efi_debug_image_info_normal { + u32 image_info_type; + struct efi_loaded_image *loaded_image_protocol_instance; + efi_handle_t image_handle; +}; + +union efi_debug_image_info { + u32 *image_info_type; + struct efi_debug_image_info_normal *normal_image; +}; + +struct efi_debug_image_info_table_header { + volatile u32 update_status; + 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; }