| Message ID | 20250424201927.2027257-2-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> > > Add EFI_SYSTEM_TABLE_POINTER structure for debugger to locate > the address of EFI_SYSTEM_TABLE. > > This feature is described in UEFI SPEC version 2.10. Section 18.4.2. Hello Ying-Chun, Thank you for your contribution. Unfortunately it is not clear what your use case is. The reason for implementing a feature should be mentioned in the commit message. Cover-letters for a patch series are the right place to describe overarching topics. The relevant information needed to debug U-Boot is the u-boot.map file and the relocation address. On arm64 the relocation address can be found via the global data structure pointed to by register x18. Once you have the relocation offset, you can determine the system table address via the u-boot.map file. If you need information about loaded EFI images, you could enable debug messages using U-Boot's log command. > > 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> > --- > V2: add Kconfig options to turn on/off this feature. > --- > include/efi_api.h | 6 +++++ > include/efi_loader.h | 2 ++ > lib/efi_loader/Kconfig | 7 ++++++ > lib/efi_loader/efi_boottime.c | 44 +++++++++++++++++++++++++++++++++++ > lib/efi_loader/efi_setup.c | 9 +++++++ > 5 files changed, 68 insertions(+) > > diff --git a/include/efi_api.h b/include/efi_api.h > index eb61eafa028..5017d9037cd 100644 > --- a/include/efi_api.h > +++ b/include/efi_api.h > @@ -259,6 +259,12 @@ struct efi_capsule_result_variable_header { > efi_status_t capsule_status; > } __packed; > > +struct efi_system_table_pointer { > + u64 signature; > + efi_physical_addr_t efi_system_table_base; > + u32 crc32; > +}; > + > struct efi_memory_range { > efi_physical_addr_t address; > u64 length; > diff --git a/include/efi_loader.h b/include/efi_loader.h > index 1d75d97ebbc..370bc042f70 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -615,6 +615,8 @@ efi_status_t efi_tcg2_measure_dtb(void *dtb); > efi_status_t efi_root_node_register(void); > /* Called by bootefi to initialize runtime */ > efi_status_t efi_initialize_system_table(void); > +/* Called by bootefi to initialize debug */ > +efi_status_t efi_initialize_system_table_pointer(void); > /* 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/Kconfig b/lib/efi_loader/Kconfig > index d4f6b56afaa..94a75981389 100644 > --- a/lib/efi_loader/Kconfig > +++ b/lib/efi_loader/Kconfig > @@ -70,6 +70,13 @@ config EFI_SECURE_BOOT > config EFI_SIGNATURE_SUPPORT > bool > > +config EFI_DEBUG_SUPPORT_TABLE > + bool "EFI Debug Support Table" > + help > + Select this option if you want to setup the EFI Debug Support > + Table which is used by the debug agent or an external debugger to > + determine loaded image information in a quiescent manner. > + > menu "UEFI services" > > config EFI_GET_TIME > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c > index 5164cb15986..366bc0d2b1d 100644 > --- a/lib/efi_loader/efi_boottime.c > +++ b/lib/efi_loader/efi_boottime.c > @@ -4001,6 +4001,8 @@ struct efi_system_table __efi_runtime_data systab = { > .tables = NULL, > }; > > +struct efi_system_table_pointer __efi_runtime_data * systab_pointer = NULL; > + > /** > * efi_initialize_system_table() - Initialize system table > * > @@ -4035,3 +4037,45 @@ efi_status_t efi_initialize_system_table(void) > > return ret; > } > + > +/** > + * efi_initialize_system_table() - Initialize system table > + * > + * Return: status code > + */ > +efi_status_t efi_initialize_system_table_pointer(void) This function name seems to be a misnomer as you are setting up much more with the next patches. > +{ > + efi_status_t ret; > + const int size_4MB = 0x00400000; > + int pages = efi_size_in_pages(sizeof(struct efi_system_table_pointer)); > + int alignment_mask = size_4MB - 1; > + int real_pages = pages + efi_size_in_pages(size_4MB); > + u64 addr; > + u64 aligned_addr; > + u32 crc32_value; > + > + /* Allocate configuration table array */ > + ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, > + EFI_RUNTIME_SERVICES_DATA, > + real_pages, > + &addr); > + > + /* alignment to 4MB boundary */ > + aligned_addr = (addr + alignment_mask) & ~alignment_mask; > + > + systab_pointer = (struct efi_system_table_pointer *)aligned_addr; > + memset(systab_pointer, 0, sizeof(struct efi_system_table_pointer)); > + > + /* > + * These entries will be set to NULL in ExitBootServices(). To avoid > + * relocation in SetVirtualAddressMap(), set them dynamically. > + */ This comment does not match the submitted code. > + systab_pointer->signature = EFI_SYSTEM_TABLE_SIGNATURE; > + systab_pointer->efi_system_table_base = (efi_physical_addr_t)&systab; > + systab_pointer->crc32 = 0; > + crc32_value = crc32(0, > + (const unsigned char *)systab_pointer, > + sizeof(struct efi_system_table_pointer)); > + systab_pointer->crc32 = crc32_value; > + return ret; > +} > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c > index aa59bc7779d..6be23ca43bf 100644 > --- a/lib/efi_loader/efi_setup.c > +++ b/lib/efi_loader/efi_setup.c > @@ -259,6 +259,15 @@ efi_status_t efi_init_obj_list(void) > if (ret != EFI_SUCCESS) > goto out; > > + /* Initialize system table */ > + if (IS_ENABLED(CONFIG_EFI_DEBUG_SUPPORT_TABLE)) { > + ret = efi_initialize_system_table_pointer(); > + if (ret != EFI_SUCCESS) { > + printf("EFI init system table pointer fail\n"); Maybe: "Installing EFI system table pointer failed\n". Please, use log_error() and move the output to the appropriate place in the efi_initialize_system_table_pointer() function. Best regards Heinrich > + goto out; > + } > + } > + > if (IS_ENABLED(CONFIG_EFI_ECPT)) { > ret = efi_ecpt_register(); > if (ret != EFI_SUCCESS)
Hi Heinrich, Thanks for the review. On Thu, 24 Apr 2025 at 22:46, 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> > > > > Add EFI_SYSTEM_TABLE_POINTER structure for debugger to locate > > the address of EFI_SYSTEM_TABLE. > > > > This feature is described in UEFI SPEC version 2.10. Section 18.4.2. > > Hello Ying-Chun, > > Thank you for your contribution. > > Unfortunately it is not clear what your use case is. The reason for > implementing a feature should be mentioned in the commit message. > Cover-letters for a patch series are the right place to describe > overarching topics. > I'll enhance the commit message in next version. > > The relevant information needed to debug U-Boot is the u-boot.map file > and the relocation address. On arm64 the relocation address can be found > via the global data structure pointed to by register x18. > > Yes. Previously we use p/x (*(struct global_data*)\$x18)->relocaddr add-symbol-file <Symbols.deug> $1 For loading the debug symbols for U-boot. > Once you have the relocation offset, you can determine the system table > address via the u-boot.map file. > > If you need information about loaded EFI images, you could enable debug > messages using U-Boot's log command. > > The idea here is to provide another way to get the information about the loaded EFI images. So that we can have an easier way to start debugging the EFI applications. I think for both EDK2 and U-boot, we can get the information through debug messages. EDK2 also outputs debug messages through IO port 0x402 on X86. So the developers can get information there. But let's assume that our environment is just an ARM board. With hardware JTAG connects to it. So through the JTAG, gdb can read the memory. And can stop program running at a place. And through this implementation, we can use some python extensions to directly get the information of the EFI applications for debug. First, the extension will look for this SystemTablePointer structure through the memory address. And then we can get information about the EFI applications. There is already a python gdb extension in EDK2. 1. https://github.com/tianocore/edk2/blob/master/BaseTools/Scripts/efi_debugging.py 2. https://github.com/tianocore/edk2/blob/master/BaseTools/Scripts/efi_gdb.py And we implemented the missing parts through this PR. https://github.com/tianocore/edk2/pull/10980 I've also implemented a gdb python extension by reusing the efi_debugging.py in EDK2. https://paste.debian.net/hidden/c3122c5b/ By this way it makes things a bit easier. So that we don't need to load U-boot debug symbols. Don't need debug logs. Just the EFI application and its debug symbols are needed. And then we can start debugging on the real hardware. > > > > 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> > > --- > > V2: add Kconfig options to turn on/off this feature. > > --- > > include/efi_api.h | 6 +++++ > > include/efi_loader.h | 2 ++ > > lib/efi_loader/Kconfig | 7 ++++++ > > lib/efi_loader/efi_boottime.c | 44 +++++++++++++++++++++++++++++++++++ > > lib/efi_loader/efi_setup.c | 9 +++++++ > > 5 files changed, 68 insertions(+) > > > > diff --git a/include/efi_api.h b/include/efi_api.h > > index eb61eafa028..5017d9037cd 100644 > > --- a/include/efi_api.h > > +++ b/include/efi_api.h > > @@ -259,6 +259,12 @@ struct efi_capsule_result_variable_header { > > efi_status_t capsule_status; > > } __packed; > > > > +struct efi_system_table_pointer { > > + u64 signature; > > + efi_physical_addr_t efi_system_table_base; > > + u32 crc32; > > +}; > > + > > struct efi_memory_range { > > efi_physical_addr_t address; > > u64 length; > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > index 1d75d97ebbc..370bc042f70 100644 > > --- a/include/efi_loader.h > > +++ b/include/efi_loader.h > > @@ -615,6 +615,8 @@ efi_status_t efi_tcg2_measure_dtb(void *dtb); > > efi_status_t efi_root_node_register(void); > > /* Called by bootefi to initialize runtime */ > > efi_status_t efi_initialize_system_table(void); > > +/* Called by bootefi to initialize debug */ > > +efi_status_t efi_initialize_system_table_pointer(void); > > /* 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/Kconfig b/lib/efi_loader/Kconfig > > index d4f6b56afaa..94a75981389 100644 > > --- a/lib/efi_loader/Kconfig > > +++ b/lib/efi_loader/Kconfig > > @@ -70,6 +70,13 @@ config EFI_SECURE_BOOT > > config EFI_SIGNATURE_SUPPORT > > bool > > > > +config EFI_DEBUG_SUPPORT_TABLE > > + bool "EFI Debug Support Table" > > + help > > + Select this option if you want to setup the EFI Debug Support > > + Table which is used by the debug agent or an external debugger to > > + determine loaded image information in a quiescent manner. > > + > > menu "UEFI services" > > > > config EFI_GET_TIME > > diff --git a/lib/efi_loader/efi_boottime.c > b/lib/efi_loader/efi_boottime.c > > index 5164cb15986..366bc0d2b1d 100644 > > --- a/lib/efi_loader/efi_boottime.c > > +++ b/lib/efi_loader/efi_boottime.c > > @@ -4001,6 +4001,8 @@ struct efi_system_table __efi_runtime_data systab > = { > > .tables = NULL, > > }; > > > > +struct efi_system_table_pointer __efi_runtime_data * systab_pointer = > NULL; > > + > > /** > > * efi_initialize_system_table() - Initialize system table > > * > > @@ -4035,3 +4037,45 @@ efi_status_t efi_initialize_system_table(void) > > > > return ret; > > } > > + > > +/** > > + * efi_initialize_system_table() - Initialize system table > > + * > > + * Return: status code > > + */ > > +efi_status_t efi_initialize_system_table_pointer(void) > > This function name seems to be a misnomer as you are setting up much > more with the next patches. > I will fix this in the next version. Plan to separate it into two functions. One for the system_table_pointer. Another for the configuration table. > > > +{ > > + efi_status_t ret; > > + const int size_4MB = 0x00400000; > > + int pages = efi_size_in_pages(sizeof(struct > efi_system_table_pointer)); > > + int alignment_mask = size_4MB - 1; > > + int real_pages = pages + efi_size_in_pages(size_4MB); > > + u64 addr; > > + u64 aligned_addr; > > + u32 crc32_value; > > + > > + /* Allocate configuration table array */ > > + ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, > > + EFI_RUNTIME_SERVICES_DATA, > > + real_pages, > > + &addr); > > + > > + /* alignment to 4MB boundary */ > > + aligned_addr = (addr + alignment_mask) & ~alignment_mask; > > + > > + systab_pointer = (struct efi_system_table_pointer *)aligned_addr; > > + memset(systab_pointer, 0, sizeof(struct efi_system_table_pointer)); > > + > > + /* > > + * These entries will be set to NULL in ExitBootServices(). To > avoid > > + * relocation in SetVirtualAddressMap(), set them dynamically. > > + */ > > This comment does not match the submitted code. > > Will fix this in the next version. > > + systab_pointer->signature = EFI_SYSTEM_TABLE_SIGNATURE; > > + systab_pointer->efi_system_table_base = > (efi_physical_addr_t)&systab; > > + systab_pointer->crc32 = 0; > > + crc32_value = crc32(0, > > + (const unsigned char *)systab_pointer, > > + sizeof(struct efi_system_table_pointer)); > > + systab_pointer->crc32 = crc32_value; > > + return ret; > > +} > > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c > > index aa59bc7779d..6be23ca43bf 100644 > > --- a/lib/efi_loader/efi_setup.c > > +++ b/lib/efi_loader/efi_setup.c > > @@ -259,6 +259,15 @@ efi_status_t efi_init_obj_list(void) > > if (ret != EFI_SUCCESS) > > goto out; > > > > + /* Initialize system table */ > > + if (IS_ENABLED(CONFIG_EFI_DEBUG_SUPPORT_TABLE)) { > > + ret = efi_initialize_system_table_pointer(); > > + if (ret != EFI_SUCCESS) { > > + printf("EFI init system table pointer fail\n"); > > Maybe: "Installing EFI system table pointer failed\n". > > Please, use log_error() and move the output to the appropriate place in > the efi_initialize_system_table_pointer() function. > > Will fix it in the next version. > Best regards > > Heinrich > > > + goto out; > > + } > > + } > > + > > if (IS_ENABLED(CONFIG_EFI_ECPT)) { > > ret = efi_ecpt_register(); > > if (ret != EFI_SUCCESS) > > Thanks a lot. Yours, Paul
diff --git a/include/efi_api.h b/include/efi_api.h index eb61eafa028..5017d9037cd 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -259,6 +259,12 @@ struct efi_capsule_result_variable_header { efi_status_t capsule_status; } __packed; +struct efi_system_table_pointer { + u64 signature; + efi_physical_addr_t efi_system_table_base; + u32 crc32; +}; + struct efi_memory_range { efi_physical_addr_t address; u64 length; diff --git a/include/efi_loader.h b/include/efi_loader.h index 1d75d97ebbc..370bc042f70 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -615,6 +615,8 @@ efi_status_t efi_tcg2_measure_dtb(void *dtb); efi_status_t efi_root_node_register(void); /* Called by bootefi to initialize runtime */ efi_status_t efi_initialize_system_table(void); +/* Called by bootefi to initialize debug */ +efi_status_t efi_initialize_system_table_pointer(void); /* 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/Kconfig b/lib/efi_loader/Kconfig index d4f6b56afaa..94a75981389 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -70,6 +70,13 @@ config EFI_SECURE_BOOT config EFI_SIGNATURE_SUPPORT bool +config EFI_DEBUG_SUPPORT_TABLE + bool "EFI Debug Support Table" + help + Select this option if you want to setup the EFI Debug Support + Table which is used by the debug agent or an external debugger to + determine loaded image information in a quiescent manner. + menu "UEFI services" config EFI_GET_TIME diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 5164cb15986..366bc0d2b1d 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -4001,6 +4001,8 @@ struct efi_system_table __efi_runtime_data systab = { .tables = NULL, }; +struct efi_system_table_pointer __efi_runtime_data * systab_pointer = NULL; + /** * efi_initialize_system_table() - Initialize system table * @@ -4035,3 +4037,45 @@ efi_status_t efi_initialize_system_table(void) return ret; } + +/** + * efi_initialize_system_table() - Initialize system table + * + * Return: status code + */ +efi_status_t efi_initialize_system_table_pointer(void) +{ + efi_status_t ret; + const int size_4MB = 0x00400000; + int pages = efi_size_in_pages(sizeof(struct efi_system_table_pointer)); + int alignment_mask = size_4MB - 1; + int real_pages = pages + efi_size_in_pages(size_4MB); + u64 addr; + u64 aligned_addr; + u32 crc32_value; + + /* Allocate configuration table array */ + ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, + EFI_RUNTIME_SERVICES_DATA, + real_pages, + &addr); + + /* alignment to 4MB boundary */ + aligned_addr = (addr + alignment_mask) & ~alignment_mask; + + systab_pointer = (struct efi_system_table_pointer *)aligned_addr; + memset(systab_pointer, 0, sizeof(struct efi_system_table_pointer)); + + /* + * These entries will be set to NULL in ExitBootServices(). To avoid + * relocation in SetVirtualAddressMap(), set them dynamically. + */ + systab_pointer->signature = EFI_SYSTEM_TABLE_SIGNATURE; + systab_pointer->efi_system_table_base = (efi_physical_addr_t)&systab; + systab_pointer->crc32 = 0; + crc32_value = crc32(0, + (const unsigned char *)systab_pointer, + sizeof(struct efi_system_table_pointer)); + systab_pointer->crc32 = crc32_value; + return ret; +} diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index aa59bc7779d..6be23ca43bf 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -259,6 +259,15 @@ efi_status_t efi_init_obj_list(void) if (ret != EFI_SUCCESS) goto out; + /* Initialize system table */ + if (IS_ENABLED(CONFIG_EFI_DEBUG_SUPPORT_TABLE)) { + ret = efi_initialize_system_table_pointer(); + if (ret != EFI_SUCCESS) { + printf("EFI init system table pointer fail\n"); + goto out; + } + } + if (IS_ENABLED(CONFIG_EFI_ECPT)) { ret = efi_ecpt_register(); if (ret != EFI_SUCCESS)