Am 4. Juli 2025 13:59:53 MESZ schrieb Paul Liu <paul....@linaro.org>: >Hi Heinrich, Ilias, > >Sorry for the late reply. I'm a bit worried about this change. This >structure (efi_system_table_pointer), is not packed. >So its actual size is a bit larger. The crc32 field is not at the end of >the structure. Actually there are 4 bytes padding after it. But the CRC32 >function is calculating the crc sum on the whole structure. So it might >produce random value if we don't zero those padding bytes.
Thanks for highlighting this. Structures in UEFI are typically packed. Should padding bytes really be included in the crc32? We should add a test of the crc32 to https://github.com/trini/u-boot/blob/master/lib/efi_loader/dbginfodump.c so that we can compare U-Boot to EDK2. Best regards Heinrich > >Yours, >Paul > > >On Thu, 3 Jul 2025 at 08:25, Heinrich Schuchardt <xypron.g...@gmx.de> wrote: > >> On 03.07.25 08:28, Ying-Chun Liu (PaulLiu) wrote: >> > From: "Ying-Chun Liu (PaulLiu)" <paul....@linaro.org> >> > >> > Add EFI_SYSTEM_TABLE_POINTER structure for remote debugger to locate >> > the address of EFI_SYSTEM_TABLE. >> > >> > This feature is described in UEFI SPEC version 2.10. Section 18.4.2. >> > The implementation ensures support for hardware-assisted debugging and >> > provides a standardized mechanism for debuggers to discover the EFI >> > system table. >> > >> > Signed-off-by: Ying-Chun Liu (PaulLiu) <paul....@linaro.org> >> > Reviewed-by: Ilias Apalodimas <ilias.apalodi...@linaro.org> >> > Tested-by: Heinrich Schuchardt <xypron.g...@gmx.de> >> > Cc: Peter Robinson <pbrobin...@gmail.com> >> > Cc: Simon Glass <s...@chromium.org> >> > --- >> > V2: add Kconfig options to turn on/off this feature. >> > V3: make efi_initialize_system_table_pointer() do the job as described. >> > V4: use efi_alloc_aligned_pages() for system_table_pointer. >> > V5: move the code into a separate module. >> > V6: Use macros to replace size_4MB >> > V7: Fix the code based on the review. Update license field. >> > V8: Fix pointer casting that causes qemu_arm_defconfig failed. Modify >> KConfig >> > so that more defconfigs enabled by default. >> > --- >> > include/efi_api.h | 16 ++++++++++++ >> > include/efi_loader.h | 2 ++ >> > lib/efi_loader/Kconfig | 9 +++++++ >> > lib/efi_loader/Makefile | 1 + >> > lib/efi_loader/efi_debug_support.c | 40 ++++++++++++++++++++++++++++++ >> > lib/efi_loader/efi_setup.c | 7 ++++++ >> > 6 files changed, 75 insertions(+) >> > create mode 100644 lib/efi_loader/efi_debug_support.c >> > >> > diff --git a/include/efi_api.h b/include/efi_api.h >> > index eb61eafa028..6c4c1a0cc7b 100644 >> > --- a/include/efi_api.h >> > +++ b/include/efi_api.h >> > @@ -259,6 +259,22 @@ struct efi_capsule_result_variable_header { >> > efi_status_t capsule_status; >> > } __packed; >> > >> > +/** >> > + * struct efi_system_table_pointer - struct to store the pointer of >> system >> > + * table. >> > + * @signature: The signature of this struct. >> > + * @efi_system_table_base: The physical address of System Table. >> > + * @crc32: CRC32 checksum >> > + * >> > + * This struct is design for hardware debugger to search through memory >> to >> > + * get the address of EFI System Table. >> > + */ >> > +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 8f9f2bcf1cb..8f065244d8e 100644 >> > --- a/include/efi_loader.h >> > +++ b/include/efi_loader.h >> > @@ -646,6 +646,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 7f02a83e2a2..621db8b5c79 100644 >> > --- a/lib/efi_loader/Kconfig >> > +++ b/lib/efi_loader/Kconfig >> > @@ -71,6 +71,15 @@ config EFI_SECURE_BOOT >> > config EFI_SIGNATURE_SUPPORT >> > bool >> > >> > +config EFI_DEBUG_SUPPORT >> > + bool "EFI Debug Support" >> > + default y if !HAS_BOARD_SIZE_LIMIT >> > + help >> > + Select this option if you want to setup the EFI Debug Support >> > + Table and the EFI_SYSTEM_TABLE_POINTER 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/Makefile b/lib/efi_loader/Makefile >> > index cf050e5385d..51ccf1cd87e 100644 >> > --- a/lib/efi_loader/Makefile >> > +++ b/lib/efi_loader/Makefile >> > @@ -70,6 +70,7 @@ obj-$(CONFIG_EFI_RISCV_BOOT_PROTOCOL) += efi_riscv.o >> > obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_load_initrd.o >> > obj-$(CONFIG_EFI_SIGNATURE_SUPPORT) += efi_signature.o >> > obj-$(CONFIG_EFI_ECPT) += efi_conformance.o >> > +obj-$(CONFIG_EFI_DEBUG_SUPPORT) += efi_debug_support.o >> > >> > EFI_VAR_SEED_FILE := $(subst $\",,$(CONFIG_EFI_VAR_SEED_FILE)) >> > $(obj)/efi_var_seed.o: $(srctree)/$(EFI_VAR_SEED_FILE) >> > diff --git a/lib/efi_loader/efi_debug_support.c >> b/lib/efi_loader/efi_debug_support.c >> > new file mode 100644 >> > index 00000000000..7dc5fa93af9 >> > --- /dev/null >> > +++ b/lib/efi_loader/efi_debug_support.c >> > @@ -0,0 +1,40 @@ >> > +// SPDX-License-Identifier: GPL-2.0-or-later >> > +/* >> > + * EFI debug support >> > + * >> > + * Copyright (c) 2025 Ying-Chun Liu, Linaro Ltd. <paul....@linaro.org> >> > + */ >> > + >> > +#include <efi_loader.h> >> > +#include <linux/sizes.h> >> > +#include <u-boot/crc.h> >> > + >> > +struct efi_system_table_pointer __efi_runtime_data * systab_pointer = >> NULL; >> > + >> > +/** >> > + * efi_initialize_system_table_pointer() - Initialize system table >> pointer >> > + * >> > + * Return: status code >> > + */ >> > +efi_status_t efi_initialize_system_table_pointer(void) >> > +{ >> > + /* Allocate efi_system_table_pointer structure with 4MB alignment. >> */ >> > + systab_pointer = efi_alloc_aligned_pages(sizeof(struct >> efi_system_table_pointer), >> > + EFI_RUNTIME_SERVICES_DATA, >> > + SZ_4M); >> > + >> > + if (!systab_pointer) { >> > + log_err("Installing EFI system table pointer failed\n"); >> > + return EFI_OUT_OF_RESOURCES; >> > + } >> > + >> > + memset(systab_pointer, 0, sizeof(struct efi_system_table_pointer)); >> >> systab_pointer->crc32 = 0; >> >> would have been an alternative. Maybe Ilias wants to change that when >> merging. >> >> Reviewed-by: Heinrich Schuchardt <xypron.g...@gmx.de> >> >> > + >> > + systab_pointer->signature = EFI_SYSTEM_TABLE_SIGNATURE; >> > + systab_pointer->efi_system_table_base = (uintptr_t)&systab; >> > + systab_pointer->crc32 = crc32(0, >> > + (const unsigned char >> *)systab_pointer, >> > + sizeof(struct >> efi_system_table_pointer)); >> > + >> > + return EFI_SUCCESS; >> > +} >> > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c >> > index 48f91da5df7..78c5059256a 100644 >> > --- a/lib/efi_loader/efi_setup.c >> > +++ b/lib/efi_loader/efi_setup.c >> > @@ -278,6 +278,13 @@ efi_status_t efi_init_obj_list(void) >> > if (ret != EFI_SUCCESS) >> > goto out; >> > >> > + /* Initialize system table pointer */ >> > + if (IS_ENABLED(CONFIG_EFI_DEBUG_SUPPORT)) { >> > + ret = efi_initialize_system_table_pointer(); >> > + if (ret != EFI_SUCCESS) >> > + goto out; >> > + } >> > + >> > if (IS_ENABLED(CONFIG_EFI_ECPT)) { >> > ret = efi_ecpt_register(); >> > if (ret != EFI_SUCCESS) >> >>
