On Tue, May 07, 2019 at 08:04:26AM +0200, Heinrich Schuchardt wrote: > On 5/7/19 7:16 AM, Heinrich Schuchardt wrote: > >On 5/7/19 3:53 AM, Takahiro Akashi wrote: > >>On Tue, Apr 30, 2019 at 08:11:15AM +0200, Heinrich Schuchardt wrote: > >>>The field boot OptionalData in structure _EFI_LOAD_OPTIONS is for binary > >>>data. > >>> > >>>When we use `efidebug boot add` we should convert the 5th argument from > >>>UTF-8 to UTF-16 before putting it into the BootXXXX variable. > >> > >>While optional_data holds u8 string in calling > >>efi_serialize_load_option(), > >>it holds u16 string in leaving from efi_deserialize_load_option(). > >>We should handle it in a consistent way if you want to keep optional_data > >>as "const u8." > > When communicating with Linux optional data may contain a u16 string. > But I cannot see were our coding is inconsistent.
I don't get your point. Do you want to allow 'u8 *' variable to hold u16 string? -Takahiro Akashi > Regards > > Heinrich > > > > >The patch is already merged so I will have to create a follow up patch. > > > >The UEFI spec has EFI_LOAD_OPTION.OptionalData as UINT8. So an odd > >number of bytes is a possibility. > > > >Best regards > > > >Heinrich > > > >> > >>Thanks, > >>-Takahiro Akashi > >> > >>>When printing boot variables with `efidebug boot dump` we should support > >>>the OptionalData being arbitrary binary data. So let's dump the data as > >>>hexadecimal values. > >>> > >>>Here is an example session protocol: > >>> > >>>=> efidebug boot add 00a1 label1 scsi 0:1 doit1 'my option' > >>>=> efidebug boot add 00a2 label2 scsi 0:1 doit2 > >>>=> efidebug boot dump > >>>Boot00A0: > >>> attributes: A-- (0x00000001) > >>> label: label1 > >>> file_path: .../HD(1,MBR,0xeac4e18b,0x800,0x3fffe)/doit1 > >>> data: > >>> 00000000: 6d 00 79 00 20 00 6f 00 70 00 74 00 69 00 6f 00 m.y. > >>>.o.p.t.i.o. > >>> 00000010: 6e 00 00 00 n... > >>>Boot00A1: > >>> attributes: A-- (0x00000001) > >>> label: label2 > >>> file_path: .../HD(1,MBR,0xeac4e18b,0x800,0x3fffe)/doit2 > >>> data: > >>> > >>>Signed-off-by: Heinrich Schuchardt <[email protected]> > >>>--- > >>>v2: > >>> remove statement without effect in efi_serialize_load_option() > >>>--- > >>> cmd/efidebug.c | 27 +++++++++++++++++---------- > >>> include/efi_loader.h | 2 +- > >>> lib/efi_loader/efi_bootmgr.c | 15 ++++++++------- > >>> 3 files changed, 26 insertions(+), 18 deletions(-) > >>> > >>>diff --git a/cmd/efidebug.c b/cmd/efidebug.c > >>>index efe4ea873e..c4ac9dd634 100644 > >>>--- a/cmd/efidebug.c > >>>+++ b/cmd/efidebug.c > >>>@@ -11,6 +11,7 @@ > >>> #include <efi_loader.h> > >>> #include <environment.h> > >>> #include <exports.h> > >>>+#include <hexdump.h> > >>> #include <malloc.h> > >>> #include <search.h> > >>> #include <linux/ctype.h> > >>>@@ -545,7 +546,10 @@ static int do_efi_boot_add(cmd_tbl_t *cmdtp, int > >>>flag, > >>> + sizeof(struct efi_device_path); /* for END */ > >>> > >>> /* optional data */ > >>>- lo.optional_data = (u8 *)(argc == 6 ? "" : argv[6]); > >>>+ if (argc < 6) > >>>+ lo.optional_data = NULL; > >>>+ else > >>>+ lo.optional_data = (const u8 *)argv[6]; > >>> > >>> size = efi_serialize_load_option(&lo, (u8 **)&data); > >>> if (!size) { > >>>@@ -615,12 +619,13 @@ static int do_efi_boot_rm(cmd_tbl_t *cmdtp, int > >>>flag, > >>> /** > >>> * show_efi_boot_opt_data() - dump UEFI load option > >>> * > >>>- * @id: Load option number > >>>- * @data: Value of UEFI load option variable > >>>+ * @id: load option number > >>>+ * @data: value of UEFI load option variable > >>>+ * @size: size of the boot option > >>> * > >>> * Decode the value of UEFI load option variable and print > >>>information. > >>> */ > >>>-static void show_efi_boot_opt_data(int id, void *data) > >>>+static void show_efi_boot_opt_data(int id, void *data, size_t size) > >>> { > >>> struct efi_load_option lo; > >>> char *label, *p; > >>>@@ -638,7 +643,7 @@ static void show_efi_boot_opt_data(int id, void > >>>*data) > >>> utf16_utf8_strncpy(&p, lo.label, label_len16); > >>> > >>> printf("Boot%04X:\n", id); > >>>- printf("\tattributes: %c%c%c (0x%08x)\n", > >>>+ printf(" attributes: %c%c%c (0x%08x)\n", > >>> /* ACTIVE */ > >>> lo.attributes & LOAD_OPTION_ACTIVE ? 'A' : '-', > >>> /* FORCE RECONNECT */ > >>>@@ -646,14 +651,16 @@ static void show_efi_boot_opt_data(int id, void > >>>*data) > >>> /* HIDDEN */ > >>> lo.attributes & LOAD_OPTION_HIDDEN ? 'H' : '-', > >>> lo.attributes); > >>>- printf("\tlabel: %s\n", label); > >>>+ printf(" label: %s\n", label); > >>> > >>> dp_str = efi_dp_str(lo.file_path); > >>>- printf("\tfile_path: %ls\n", dp_str); > >>>+ printf(" file_path: %ls\n", dp_str); > >>> efi_free_pool(dp_str); > >>> > >>>- printf("\tdata: %s\n", lo.optional_data); > >>>- > >>>+ printf(" data:\n"); > >>>+ print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, > >>>+ lo.optional_data, size + (u8 *)data - > >>>+ (u8 *)lo.optional_data, true); > >>> free(label); > >>> } > >>> > >>>@@ -686,7 +693,7 @@ static void show_efi_boot_opt(int id) > >>> data)); > >>> } > >>> if (ret == EFI_SUCCESS) > >>>- show_efi_boot_opt_data(id, data); > >>>+ show_efi_boot_opt_data(id, data, size); > >>> else if (ret == EFI_NOT_FOUND) > >>> printf("Boot%04X: not found\n", id); > >>> > >>>diff --git a/include/efi_loader.h b/include/efi_loader.h > >>>index 39ed8a6fa5..07b913d256 100644 > >>>--- a/include/efi_loader.h > >>>+++ b/include/efi_loader.h > >>>@@ -560,7 +560,7 @@ struct efi_load_option { > >>> u16 file_path_length; > >>> u16 *label; > >>> struct efi_device_path *file_path; > >>>- u8 *optional_data; > >>>+ const u8 *optional_data; > >>> }; > >>> > >>> void efi_deserialize_load_option(struct efi_load_option *lo, u8 > >>>*data); > >>>diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c > >>>index 4ccba22875..7bf51874c1 100644 > >>>--- a/lib/efi_loader/efi_bootmgr.c > >>>+++ b/lib/efi_loader/efi_bootmgr.c > >>>@@ -53,19 +53,20 @@ void efi_deserialize_load_option(struct > >>>efi_load_option *lo, u8 *data) > >>> */ > >>> unsigned long efi_serialize_load_option(struct efi_load_option *lo, > >>>u8 **data) > >>> { > >>>- unsigned long label_len, option_len; > >>>+ unsigned long label_len; > >>> unsigned long size; > >>> u8 *p; > >>> > >>> label_len = (u16_strlen(lo->label) + 1) * sizeof(u16); > >>>- option_len = strlen((char *)lo->optional_data); > >>> > >>> /* total size */ > >>> size = sizeof(lo->attributes); > >>> size += sizeof(lo->file_path_length); > >>> size += label_len; > >>> size += lo->file_path_length; > >>>- size += option_len + 1; > >>>+ if (lo->optional_data) > >>>+ size += (utf8_utf16_strlen((const char *)lo->optional_data) > >>>+ + 1) * sizeof(u16); > >>> p = malloc(size); > >>> if (!p) > >>> return 0; > >>>@@ -84,10 +85,10 @@ unsigned long efi_serialize_load_option(struct > >>>efi_load_option *lo, u8 **data) > >>> memcpy(p, lo->file_path, lo->file_path_length); > >>> p += lo->file_path_length; > >>> > >>>- memcpy(p, lo->optional_data, option_len); > >>>- p += option_len; > >>>- *(char *)p = '\0'; > >>>- > >>>+ if (lo->optional_data) { > >>>+ utf8_utf16_strcpy((u16 **)&p, (const char *)lo->optional_data); > >>>+ p += sizeof(u16); /* size of trailing \0 */ > >>>+ } > >>> return size; > >>> } > >>> > >>>-- > >>>2.20.1 > >>> > >> > > > > > _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
