On Wed, 18 Jun 2025 at 07:39, Heinrich Schuchardt <[email protected]> wrote:
> On 6/17/25 14:08, Ying-Chun Liu (PaulLiu) wrote: > > From: "Ying-Chun Liu (PaulLiu)" <[email protected]> > > > > 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. > > The implementation ensures support for hardware-assisted debugging and > > provides a standardized mechanism for debuggers to discover and interact > > with system-level debug resources. > > > > Signed-off-by: Ying-Chun Liu (PaulLiu) <[email protected]> > > Reviewed-by: Ilias Apalodimas <[email protected]> > > Cc: Heinrich Schuchardt <[email protected]> > > Cc: Peter Robinson <[email protected]> > > Cc: Simon Glass <[email protected]> > > --- > > V3: Fix the way of installation of configuration table. > > V5: move the code into a separate module. > > --- > > include/efi_api.h | 53 ++++++++++++++++++++++++++++++ > > include/efi_loader.h | 2 ++ > > lib/efi_loader/efi_debug_support.c | 6 ++++ > > lib/efi_loader/efi_setup.c | 12 +++++++ > > 4 files changed, 73 insertions(+) > > > > diff --git a/include/efi_api.h b/include/efi_api.h > > index 6c4c1a0cc7b..8da0a350ce3 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; > > @@ -574,6 +578,55 @@ 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 - Store Debug Information for > normal > > + * image. > > + * @image_info_type: the type of image info. > > + * @loaded_image_protocol_instance: the pointer to struct > efi_loaded_image. > > + * @image_handle: the EFI handle of the image. > > + * > > + * This struct is created by efi_load_image() and store the information > > + * for debugging an normal image. > > + */ > > +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 - The union to store a pointer for EFI > > + * DEBUG IMAGE INFO. > > + * @image_info_type: the type of the image_info if it is not a normal > image. > > + * @normal_image: The pointer to a normal image. > > + * > > + * This union is for a pointer that can point to the struct of > normal_image. > > + * Or it points to an image_info_type. > > + */ > > +union efi_debug_image_info { > > + u32 *image_info_type; > > + struct efi_debug_image_info_normal *normal_image; > > +}; > > + > > +/** > > + * struct efi_debug_image_info_table_header - store the array of > > + * struct efi_debug_image_info. > > + * @update_status: Status to notify this struct is ready to use or not. > > + * @table_size: The number of elements of efi_debug_image_info_table. > > + * @efi_debug_image_info_table: The array of efi_debug_image_info. > > + * > > + * This struct stores the array of efi_debug_image_info. The > > + * number of elements is table_size. > > + */ > > +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/include/efi_loader.h b/include/efi_loader.h > > index 8f065244d8e..d0c72d0bc58 100644 > > --- a/include/efi_loader.h > > +++ b/include/efi_loader.h > > @@ -315,6 +315,8 @@ extern const struct efi_hii_config_routing_protocol > efi_hii_config_routing; > > extern const struct efi_hii_config_access_protocol > efi_hii_config_access; > > extern const struct efi_hii_database_protocol efi_hii_database; > > extern const struct efi_hii_string_protocol efi_hii_string; > > +/* structure for EFI_DEBUG_SUPPORT_PROTOCOL */ > > +extern struct efi_debug_image_info_table_header > efi_m_debug_info_table_header; > > > > uint16_t *efi_dp_str(struct efi_device_path *dp); > > > > diff --git a/lib/efi_loader/efi_debug_support.c > b/lib/efi_loader/efi_debug_support.c > > index 3794b31c4cd..f2dfcc5e6b2 100644 > > --- a/lib/efi_loader/efi_debug_support.c > > +++ b/lib/efi_loader/efi_debug_support.c > > @@ -11,6 +11,12 @@ > > > > struct efi_system_table_pointer __efi_runtime_data * systab_pointer = > NULL; > > > > +struct efi_debug_image_info_table_header efi_m_debug_info_table_header > = { > > + 0, > > + 0, > > + NULL > > +}; > > + > > /** > > * efi_initialize_system_table_pointer() - Initialize system table > pointer > > * > > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c > > index 6193b9f6d0c..60f3a7e5fbf 100644 > > --- a/lib/efi_loader/efi_setup.c > > +++ b/lib/efi_loader/efi_setup.c > > @@ -18,6 +18,9 @@ > > > > efi_status_t efi_obj_list_initialized = OBJ_LIST_NOT_INITIALIZED; > > > > +const efi_guid_t efi_debug_image_info_table_guid = > > + EFI_DEBUG_IMAGE_INFO_TABLE_GUID; > > + > > /* > > * Allow unaligned memory access. > > * > > @@ -280,10 +283,19 @@ efi_status_t efi_init_obj_list(void) > > > > /* Initialize system table pointer */ > > if (IS_ENABLED(CONFIG_EFI_DEBUG_SUPPORT)) { > > + efi_guid_t debug_image_info_table_guid = > > This variable shadows the variable defined above. > > > + efi_debug_image_info_table_guid; > > This self assignment provides a random value from the stack and not the > expected GUID. > > Please, provide a test in lib/efi_selftest/ verifying that the table is > installed with the correct GUID. > > Hi Heinrich, This is not a self assignment. The new variable is called "debug_image_info_table_guid". The global one is called "efi_debug_image_info_table_guid". I'll implement a selftest anyway to make sure that the GUID is correct. > > + > > ret = efi_initialize_system_table_pointer(); > > if (ret != EFI_SUCCESS) { > > goto out; > > } > > + > > + ret = > efi_install_configuration_table(&debug_image_info_table_guid, > > + > &efi_m_debug_info_table_header); > > + if (ret != EFI_SUCCESS) { > > + goto out; > > + } > > No braces here. Please, use scripts/checkpatch.pl before resubmitting. > > Do you set some parameters of the scripts/checkpatch.pl ? I'm using ./scripts/checkpatch.pl -g <COMMIT_ID> --u-boot. But it only gives me a warning about using the volatile. And for patch 1/4 it only gives me a warning about adding new file and the MAINTAINER stuff. This is weird. I didn't get the warning about the braces at all. Yours, Paul
