| Message ID | 20220116151441.219566-7-heinrich.schuchardt@canonical.com |
|---|---|
| State | Accepted, archived |
| Commit | 3adae64220be502cd6d522c96a3af7dd420a1a67 |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: simplify printing GUIDs | expand |
Hi Heinrich, On Sun, 16 Jan 2022 at 08:14, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > Use "%pS" to print text representations of GUIDs. > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > cmd/efidebug.c | 160 ++--------------------------------------- > include/efi_api.h | 8 +++ > include/efi_dt_fixup.h | 4 -- > include/efi_rng.h | 4 -- > lib/uuid.c | 116 ++++++++++++++++++++++++++++++ > 5 files changed, 128 insertions(+), 164 deletions(-) > Does this blow up the image size? These strings only in the debug side before. Having said that, I would much rather see a string than a guid, which I consider to be little more than an obfuscation. Regards, Simon
On 1/21/22 16:20, Simon Glass wrote: > Hi Heinrich, > > On Sun, 16 Jan 2022 at 08:14, Heinrich Schuchardt > <heinrich.schuchardt@canonical.com> wrote: >> >> Use "%pS" to print text representations of GUIDs. >> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >> --- >> cmd/efidebug.c | 160 ++--------------------------------------- >> include/efi_api.h | 8 +++ >> include/efi_dt_fixup.h | 4 -- >> include/efi_rng.h | 4 -- >> lib/uuid.c | 116 ++++++++++++++++++++++++++++++ >> 5 files changed, 128 insertions(+), 164 deletions(-) >> > > Does this blow up the image size? These strings only in the debug side before. It was to avoid image size increase that I added +#ifdef CONFIG_CMD_EFIDEBUG > > Having said that, I would much rather see a string than a guid, which > I consider to be little more than an obfuscation. That was my motivation. When debugging a boot failure I set DEBUG in lib/efi_loader/efi_boottime.c and reading GUIDs in the debug output was not helpful. Best regards Heinrich > > Regards, > Simon
Hi Heinrich, On Fri, 21 Jan 2022 at 09:03, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > On 1/21/22 16:20, Simon Glass wrote: > > Hi Heinrich, > > > > On Sun, 16 Jan 2022 at 08:14, Heinrich Schuchardt > > <heinrich.schuchardt@canonical.com> wrote: > >> > >> Use "%pS" to print text representations of GUIDs. > >> > >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > >> --- > >> cmd/efidebug.c | 160 ++--------------------------------------- > >> include/efi_api.h | 8 +++ > >> include/efi_dt_fixup.h | 4 -- > >> include/efi_rng.h | 4 -- > >> lib/uuid.c | 116 ++++++++++++++++++++++++++++++ > >> 5 files changed, 128 insertions(+), 164 deletions(-) > >> > > > > Does this blow up the image size? These strings only in the debug side before. > > It was to avoid image size increase that I added > +#ifdef CONFIG_CMD_EFIDEBUG Ah yes, thanks. Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > Having said that, I would much rather see a string than a guid, which > > I consider to be little more than an obfuscation. > > That was my motivation. When debugging a boot failure I set DEBUG in > lib/efi_loader/efi_boottime.c and reading GUIDs in the debug output was > not helpful. Indeed. Regards, SImon
On Fri, Jan 21, 2022 at 05:03:03PM +0100, Heinrich Schuchardt wrote: > On 1/21/22 16:20, Simon Glass wrote: > > Hi Heinrich, > > > > On Sun, 16 Jan 2022 at 08:14, Heinrich Schuchardt > > <heinrich.schuchardt@canonical.com> wrote: > > > > > > Use "%pS" to print text representations of GUIDs. > > > > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > > > --- > > > cmd/efidebug.c | 160 ++--------------------------------------- > > > include/efi_api.h | 8 +++ > > > include/efi_dt_fixup.h | 4 -- > > > include/efi_rng.h | 4 -- > > > lib/uuid.c | 116 ++++++++++++++++++++++++++++++ > > > 5 files changed, 128 insertions(+), 164 deletions(-) > > > > > > > Does this blow up the image size? These strings only in the debug side before. > > It was to avoid image size increase that I added > +#ifdef CONFIG_CMD_EFIDEBUG > > > > > Having said that, I would much rather see a string than a guid, which > > I consider to be little more than an obfuscation. > > That was my motivation. When debugging a boot failure I set DEBUG in > lib/efi_loader/efi_boottime.c and reading GUIDs in the debug output was not > helpful. But setting DEBUG in efi_boottime.c doesn't lead to CONFIG_CMD_EFIDEBUG being on in uuid.c. Do we want to have a more direct CONFIG? -Takahiro Akashi > Best regards > > Heinrich > > > > > Regards, > > Simon
On 1/24/22 08:23, AKASHI Takahiro wrote: > On Fri, Jan 21, 2022 at 05:03:03PM +0100, Heinrich Schuchardt wrote: >> On 1/21/22 16:20, Simon Glass wrote: >>> Hi Heinrich, >>> >>> On Sun, 16 Jan 2022 at 08:14, Heinrich Schuchardt >>> <heinrich.schuchardt@canonical.com> wrote: >>>> >>>> Use "%pS" to print text representations of GUIDs. >>>> >>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >>>> --- >>>> cmd/efidebug.c | 160 ++--------------------------------------- >>>> include/efi_api.h | 8 +++ >>>> include/efi_dt_fixup.h | 4 -- >>>> include/efi_rng.h | 4 -- >>>> lib/uuid.c | 116 ++++++++++++++++++++++++++++++ >>>> 5 files changed, 128 insertions(+), 164 deletions(-) >>>> >>> >>> Does this blow up the image size? These strings only in the debug side before. >> >> It was to avoid image size increase that I added >> +#ifdef CONFIG_CMD_EFIDEBUG >> >>> >>> Having said that, I would much rather see a string than a guid, which >>> I consider to be little more than an obfuscation. >> >> That was my motivation. When debugging a boot failure I set DEBUG in >> lib/efi_loader/efi_boottime.c and reading GUIDs in the debug output was not >> helpful. > > But setting DEBUG in efi_boottime.c doesn't lead to CONFIG_CMD_EFIDEBUG > being on in uuid.c. Do we want to have a more direct CONFIG? We could use CONFIG_LOGLEVEL >= LOGL_DEBUG because once that loglevel is set you can use the log command to log one of the EFI related functions. Overall the EFI debugging requires rethinking: If you simply add '#define DEFINE 1' to the top of lib/efi_loader/efi_boottime your screen will be flooded by function calls related to timer events. Probably those high frequency events should use LOGL_DEBUG_IO. Best regards Heinrich > > -Takahiro Akashi > >> Best regards >> >> Heinrich >> >>> >>> Regards, >>> Simon
On Mon, Jan 24, 2022 at 09:25:34AM +0100, Heinrich Schuchardt wrote: > On 1/24/22 08:23, AKASHI Takahiro wrote: > > On Fri, Jan 21, 2022 at 05:03:03PM +0100, Heinrich Schuchardt wrote: > > > On 1/21/22 16:20, Simon Glass wrote: > > > > Hi Heinrich, > > > > > > > > On Sun, 16 Jan 2022 at 08:14, Heinrich Schuchardt > > > > <heinrich.schuchardt@canonical.com> wrote: > > > > > > > > > > Use "%pS" to print text representations of GUIDs. > > > > > > > > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > > > > > --- > > > > > cmd/efidebug.c | 160 ++--------------------------------------- > > > > > include/efi_api.h | 8 +++ > > > > > include/efi_dt_fixup.h | 4 -- > > > > > include/efi_rng.h | 4 -- > > > > > lib/uuid.c | 116 ++++++++++++++++++++++++++++++ > > > > > 5 files changed, 128 insertions(+), 164 deletions(-) > > > > > > > > > > > > > Does this blow up the image size? These strings only in the debug side before. > > > > > > It was to avoid image size increase that I added > > > +#ifdef CONFIG_CMD_EFIDEBUG > > > > > > > > > > > Having said that, I would much rather see a string than a guid, which > > > > I consider to be little more than an obfuscation. > > > > > > That was my motivation. When debugging a boot failure I set DEBUG in > > > lib/efi_loader/efi_boottime.c and reading GUIDs in the debug output was not > > > helpful. > > > > But setting DEBUG in efi_boottime.c doesn't lead to CONFIG_CMD_EFIDEBUG > > being on in uuid.c. Do we want to have a more direct CONFIG? > > We could use CONFIG_LOGLEVEL >= LOGL_DEBUG because once that loglevel is > set you can use the log command to log one of the EFI related functions. I'm not sure the log level is the best choice for controlling messages. Showing a human-readable GUID would also be a help in case of errors. > Overall the EFI debugging requires rethinking: I definitely agree, no specific idea though. -Takahiro Akashi > If you simply add '#define DEFINE 1' to the top of > lib/efi_loader/efi_boottime your screen will be flooded by function > calls related to timer events. Probably those high frequency events > should use LOGL_DEBUG_IO. > > Best regards > > Heinrich > > > > > -Takahiro Akashi > > > > > Best regards > > > > > > Heinrich > > > > > > > > > > > Regards, > > > > Simon >
diff --git a/cmd/efidebug.c b/cmd/efidebug.c index a977ca9c72..66ce0fc305 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -502,149 +502,6 @@ static int do_efi_show_drivers(struct cmd_tbl *cmdtp, int flag, return CMD_RET_SUCCESS; } -static const struct { - const char *text; - const efi_guid_t guid; -} guid_list[] = { - { - "Device Path", - EFI_DEVICE_PATH_PROTOCOL_GUID, - }, - { - "Device Path To Text", - EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID, - }, - { - "Device Path Utilities", - EFI_DEVICE_PATH_UTILITIES_PROTOCOL_GUID, - }, - { - "Unicode Collation 2", - EFI_UNICODE_COLLATION_PROTOCOL2_GUID, - }, - { - "Driver Binding", - EFI_DRIVER_BINDING_PROTOCOL_GUID, - }, - { - "Simple Text Input", - EFI_SIMPLE_TEXT_INPUT_PROTOCOL_GUID, - }, - { - "Simple Text Input Ex", - EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL_GUID, - }, - { - "Simple Text Output", - EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL_GUID, - }, - { - "Block IO", - EFI_BLOCK_IO_PROTOCOL_GUID, - }, - { - "Simple File System", - EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID, - }, - { - "Loaded Image", - EFI_LOADED_IMAGE_PROTOCOL_GUID, - }, - { - "Graphics Output", - EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID, - }, - { - "HII String", - EFI_HII_STRING_PROTOCOL_GUID, - }, - { - "HII Database", - EFI_HII_DATABASE_PROTOCOL_GUID, - }, - { - "HII Config Routing", - EFI_HII_CONFIG_ROUTING_PROTOCOL_GUID, - }, - { - "Load File2", - EFI_LOAD_FILE2_PROTOCOL_GUID, - }, - { - "Random Number Generator", - EFI_RNG_PROTOCOL_GUID, - }, - { - "Simple Network", - EFI_SIMPLE_NETWORK_PROTOCOL_GUID, - }, - { - "PXE Base Code", - EFI_PXE_BASE_CODE_PROTOCOL_GUID, - }, - { - "Device-Tree Fixup", - EFI_DT_FIXUP_PROTOCOL_GUID, - }, - { - "System Partition", - PARTITION_SYSTEM_GUID - }, - { - "Firmware Management", - EFI_FIRMWARE_MANAGEMENT_PROTOCOL_GUID - }, - /* Configuration table GUIDs */ - { - "ACPI table", - EFI_ACPI_TABLE_GUID, - }, - { - "EFI System Resource Table", - EFI_SYSTEM_RESOURCE_TABLE_GUID, - }, - { - "device tree", - EFI_FDT_GUID, - }, - { - "SMBIOS table", - SMBIOS_TABLE_GUID, - }, - { - "Runtime properties", - EFI_RT_PROPERTIES_TABLE_GUID, - }, - { - "TCG2 Final Events Table", - EFI_TCG2_FINAL_EVENTS_TABLE_GUID, - }, -}; - -/** - * get_guid_text - get string of GUID - * - * Return description of GUID. - * - * @guid: GUID - * Return: description of GUID or NULL - */ -static const char *get_guid_text(const void *guid) -{ - int i; - - for (i = 0; i < ARRAY_SIZE(guid_list); i++) { - /* - * As guidcmp uses memcmp() we can safely accept unaligned - * GUIDs. - */ - if (!guidcmp(&guid_list[i].guid, guid)) - return guid_list[i].text; - } - - return NULL; -} - /** * do_efi_show_handles() - show UEFI handles * @@ -664,7 +521,6 @@ static int do_efi_show_handles(struct cmd_tbl *cmdtp, int flag, efi_handle_t *handles; efi_guid_t **guid; efi_uintn_t num, count, i, j; - const char *guid_text; efi_status_t ret; ret = EFI_CALL(efi_locate_handle_buffer(ALL_HANDLES, NULL, NULL, @@ -692,11 +548,7 @@ static int do_efi_show_handles(struct cmd_tbl *cmdtp, int flag, else putc(' '); - guid_text = get_guid_text(guid[j]); - if (guid_text) - puts(guid_text); - else - printf("%pUl", guid[j]); + printf("%pUs", guid[j]); } putc('\n'); } @@ -873,14 +725,10 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { efi_uintn_t i; - const char *guid_str; - for (i = 0; i < systab.nr_tables; ++i) { - guid_str = get_guid_text(&systab.tables[i].guid); - if (!guid_str) - guid_str = ""; - printf("%pUl %s\n", &systab.tables[i].guid, guid_str); - } + for (i = 0; i < systab.nr_tables; ++i) + printf("%pUl (%pUs)\n", + &systab.tables[i].guid, &systab.tables[i].guid); return CMD_RET_SUCCESS; } diff --git a/include/efi_api.h b/include/efi_api.h index ec9fa89a93..a60d1bc416 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -426,6 +426,14 @@ struct efi_runtime_services { EFI_GUID(0x1e2ed096, 0x30e2, 0x4254, 0xbd, \ 0x89, 0x86, 0x3b, 0xbe, 0xf8, 0x23, 0x25) +#define EFI_RNG_PROTOCOL_GUID \ + EFI_GUID(0x3152bca5, 0xeade, 0x433d, 0x86, 0x2e, \ + 0xc0, 0x1c, 0xdc, 0x29, 0x1f, 0x44) + +#define EFI_DT_FIXUP_PROTOCOL_GUID \ + EFI_GUID(0xe617d64c, 0xfe08, 0x46da, 0xf4, 0xdc, \ + 0xbb, 0xd5, 0x87, 0x0c, 0x73, 0x00) + /** * struct efi_configuration_table - EFI Configuration Table * diff --git a/include/efi_dt_fixup.h b/include/efi_dt_fixup.h index 9066e8dd8e..83382537d1 100644 --- a/include/efi_dt_fixup.h +++ b/include/efi_dt_fixup.h @@ -7,10 +7,6 @@ #include <efi_api.h> -#define EFI_DT_FIXUP_PROTOCOL_GUID \ - EFI_GUID(0xe617d64c, 0xfe08, 0x46da, 0xf4, 0xdc, \ - 0xbb, 0xd5, 0x87, 0x0c, 0x73, 0x00) - #define EFI_DT_FIXUP_PROTOCOL_REVISION 0x00010000 /* Add nodes and update properties */ diff --git a/include/efi_rng.h b/include/efi_rng.h index 35f59678c7..3c622381cb 100644 --- a/include/efi_rng.h +++ b/include/efi_rng.h @@ -10,10 +10,6 @@ #include <efi_api.h> /* EFI random number generation protocol related GUID definitions */ -#define EFI_RNG_PROTOCOL_GUID \ - EFI_GUID(0x3152bca5, 0xeade, 0x433d, 0x86, 0x2e, \ - 0xc0, 0x1c, 0xdc, 0x29, 0x1f, 0x44) - #define EFI_RNG_ALGORITHM_RAW \ EFI_GUID(0xe43176d7, 0xb6e8, 0x4827, 0xb7, 0x84, \ 0x7f, 0xfd, 0xc4, 0xb6, 0x85, 0x61) diff --git a/lib/uuid.c b/lib/uuid.c index 56c452ee77..60b7ca17f1 100644 --- a/lib/uuid.c +++ b/lib/uuid.c @@ -5,6 +5,7 @@ #include <common.h> #include <command.h> +#include <efi_api.h> #include <env.h> #include <rand.h> #include <time.h> @@ -101,6 +102,121 @@ static const struct { {"lvm", PARTITION_LINUX_LVM_GUID}, {"u-boot-env", PARTITION_U_BOOT_ENVIRONMENT}, #endif +#ifdef CONFIG_CMD_EFIDEBUG + { + "Device Path", + EFI_DEVICE_PATH_PROTOCOL_GUID, + }, + { + "Device Path To Text", + EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID, + }, + { + "Device Path Utilities", + EFI_DEVICE_PATH_UTILITIES_PROTOCOL_GUID, + }, + { + "Unicode Collation 2", + EFI_UNICODE_COLLATION_PROTOCOL2_GUID, + }, + { + "Driver Binding", + EFI_DRIVER_BINDING_PROTOCOL_GUID, + }, + { + "Simple Text Input", + EFI_SIMPLE_TEXT_INPUT_PROTOCOL_GUID, + }, + { + "Simple Text Input Ex", + EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL_GUID, + }, + { + "Simple Text Output", + EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL_GUID, + }, + { + "Block IO", + EFI_BLOCK_IO_PROTOCOL_GUID, + }, + { + "Simple File System", + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID, + }, + { + "Loaded Image", + EFI_LOADED_IMAGE_PROTOCOL_GUID, + }, + { + "Graphics Output", + EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID, + }, + { + "HII String", + EFI_HII_STRING_PROTOCOL_GUID, + }, + { + "HII Database", + EFI_HII_DATABASE_PROTOCOL_GUID, + }, + { + "HII Config Routing", + EFI_HII_CONFIG_ROUTING_PROTOCOL_GUID, + }, + { + "Load File2", + EFI_LOAD_FILE2_PROTOCOL_GUID, + }, + { + "Random Number Generator", + EFI_RNG_PROTOCOL_GUID, + }, + { + "Simple Network", + EFI_SIMPLE_NETWORK_PROTOCOL_GUID, + }, + { + "PXE Base Code", + EFI_PXE_BASE_CODE_PROTOCOL_GUID, + }, + { + "Device-Tree Fixup", + EFI_DT_FIXUP_PROTOCOL_GUID, + }, + { + "System Partition", + PARTITION_SYSTEM_GUID + }, + { + "Firmware Management", + EFI_FIRMWARE_MANAGEMENT_PROTOCOL_GUID + }, + /* Configuration table GUIDs */ + { + "ACPI table", + EFI_ACPI_TABLE_GUID, + }, + { + "EFI System Resource Table", + EFI_SYSTEM_RESOURCE_TABLE_GUID, + }, + { + "device tree", + EFI_FDT_GUID, + }, + { + "SMBIOS table", + SMBIOS_TABLE_GUID, + }, + { + "Runtime properties", + EFI_RT_PROPERTIES_TABLE_GUID, + }, + { + "TCG2 Final Events Table", + EFI_TCG2_FINAL_EVENTS_TABLE_GUID, + }, +#endif }; /*
Use "%pS" to print text representations of GUIDs. Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- cmd/efidebug.c | 160 ++--------------------------------------- include/efi_api.h | 8 +++ include/efi_dt_fixup.h | 4 -- include/efi_rng.h | 4 -- lib/uuid.c | 116 ++++++++++++++++++++++++++++++ 5 files changed, 128 insertions(+), 164 deletions(-)