On Thu, Oct 18, 2018 at 10:39:30AM +0200, Alexander Graf wrote:
>
>
> On 18.10.18 09:57, AKASHI Takahiro wrote:
> > On Wed, Oct 17, 2018 at 10:40:26AM +0200, Alexander Graf wrote:
> >>
> >>
> >> On 17.10.18 09:32, AKASHI Takahiro wrote:
> >>> In this patch, helper functions for an load option variable (BootXXXX)
> >>> are added:
> >>> * efi_parse_load_option(): parse a string into load_option data
> >>> (renamed from parse_load_option and exported)
> >>> * efi_marshel_load_option(): convert load_option data into a string
> >>>
> >>> Those functions will be used to implement efishell command.
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org>
> >>> ---
> >>> include/efi_loader.h | 25 +++++++++++++
> >>> lib/efi_loader/efi_bootmgr.c | 68 ++++++++++++++++++++++++------------
> >>> 2 files changed, 70 insertions(+), 23 deletions(-)
> >>>
> >>> diff --git a/include/efi_loader.h b/include/efi_loader.h
> >>> index b73fbb6de23f..1cabb1680d20 100644
> >>> --- a/include/efi_loader.h
> >>> +++ b/include/efi_loader.h
> >>> @@ -502,6 +502,31 @@ efi_status_t EFIAPI efi_set_variable(u16
> >>> *variable_name, efi_guid_t *vendor,
> >>> u32 attributes, efi_uintn_t data_size,
> >>> void *data);
> >>>
> >>> +/*
> >>> + * See section 3.1.3 in the v2.7 UEFI spec for more details on
> >>> + * the layout of EFI_LOAD_OPTION. In short it is:
> >>> + *
> >>> + * typedef struct _EFI_LOAD_OPTION {
> >>> + * UINT32 Attributes;
> >>> + * UINT16 FilePathListLength;
> >>> + * // CHAR16 Description[]; <-- variable length, NULL terminated
> >>> + * // EFI_DEVICE_PATH_PROTOCOL FilePathList[];
> >>> + * <-- FilePathListLength
> >>> bytes
> >>> + * // UINT8 OptionalData[];
> >>> + * } EFI_LOAD_OPTION;
> >>> + */
> >>> +struct load_option {
> >>> + u32 attributes;
> >>> + u16 file_path_length;
> >>> + u16 *label;
> >>> + struct efi_device_path *file_path;
> >>> + u8 *optional_data;
> >>> +};
> >>
> >> If this is part of the spec, shouldn't it rather be in efi_api.h?
> >
> > While uefi spec mentions this structure, I don't have good confidence
> > that I can say that it is part of spec or API because there is no interface
> > (or function) that handles this structure.
>
> Good point, it's internal only. Then efi_loader.h is probably a good fit.
OK.
> >
> >> It
> >> probably also wants an efi_ prefix then :).
> >
> > OK.
> >
> >>> +
> >>> +void efi_parse_load_option(struct load_option *lo, void *ptr);
> >>> +unsigned long efi_marshal_load_option(u32 attr, u16 *label,
> >>> + struct efi_device_path *file_path,
> >>> + char *option, void **data);
> >>> void *efi_bootmgr_load(struct efi_device_path **device_path,
> >>> struct efi_device_path **file_path);
> >>>
> >>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> >>> index 0c5764db127b..c2d29f956065 100644
> >>> --- a/lib/efi_loader/efi_bootmgr.c
> >>> +++ b/lib/efi_loader/efi_bootmgr.c
> >>> @@ -30,28 +30,8 @@ static const struct efi_runtime_services *rs;
> >>> */
> >>>
> >>>
> >>> -/*
> >>> - * See section 3.1.3 in the v2.7 UEFI spec for more details on
> >>> - * the layout of EFI_LOAD_OPTION. In short it is:
> >>> - *
> >>> - * typedef struct _EFI_LOAD_OPTION {
> >>> - * UINT32 Attributes;
> >>> - * UINT16 FilePathListLength;
> >>> - * // CHAR16 Description[]; <-- variable length, NULL terminated
> >>> - * // EFI_DEVICE_PATH_PROTOCOL FilePathList[]; <--
> >>> FilePathListLength bytes
> >>> - * // UINT8 OptionalData[];
> >>> - * } EFI_LOAD_OPTION;
> >>> - */
> >>> -struct load_option {
> >>> - u32 attributes;
> >>> - u16 file_path_length;
> >>> - u16 *label;
> >>> - struct efi_device_path *file_path;
> >>> - u8 *optional_data;
> >>> -};
> >>> -
> >>> /* parse an EFI_LOAD_OPTION, as described above */
> >>> -static void parse_load_option(struct load_option *lo, void *ptr)
> >>> +void efi_parse_load_option(struct load_option *lo, void *ptr)
> >>
> >> I think you want to rename this to "deserialize" to better align with ...
> >>
> >>> {
> >>> lo->attributes = *(u32 *)ptr;
> >>> ptr += sizeof(u32);
> >>> @@ -60,7 +40,7 @@ static void parse_load_option(struct load_option *lo,
> >>> void *ptr)
> >>> ptr += sizeof(u16);
> >>>
> >>> lo->label = ptr;
> >>> - ptr += (u16_strlen(lo->label) + 1) * 2;
> >>> + ptr += (u16_strlen(lo->label) + 1) * sizeof(u16);
> >>>
> >>> lo->file_path = ptr;
> >>> ptr += lo->file_path_length;
> >>> @@ -68,6 +48,48 @@ static void parse_load_option(struct load_option *lo,
> >>> void *ptr)
> >>> lo->optional_data = ptr;
> >>> }
> >>>
> >>> +unsigned long efi_marshal_load_option(u32 attr, u16 *label,
> >>
> >> ... this function which really should be called "serialize" rather than
> >> "marshal". "Marshalling" is what you do to convert from one API to
> >> another. Here, we want to write an in-memory object to disk.
> >
> > Well, as you know, I borrow this word from RPC world.
> > While I believe that those two are interchangeable in most contexts,
> > I don't care either way if you prefer 'serialize'.
>
> Yeah, I read up on marshalling afterwards too because I got curious and
> I guess it really boils down to which world you come from. The Windows
> DOM world always calls everything marshalling, while the Java world
> calls it serialize usually.
>
> Either way, I personally find serialize the more obvious name :).
OK.
> >
> >> I also think the API would make more sense if you pass in a struct
> >> efi_load_option *. It's more symmetric to the deserialize one then.
> >
> > Absolutely agree.
> >
> >> You also want to add comments to the functions to say what they do and
> >> what their parameters are there for. That will make it easier for people
> >> to grasp on how to use this (now public) API.
> >
> > OK
> >
> >> And I really dislike void * for anything that isn't "opaque". In this
> >> function (and the one above) void * really gets used as byte pointer.
> >> Please mark it as such (u8 *).
> >
> > Right, but there are quite a few of places where "void *" is used
> > instead of "u8 *" for a string. For instance, get_var() in bootmgr.c
> > or more in other files.
> >
> > It's quite painful to change all the places for consistency.
>
> Yeah, let's focus on the serialize/deserialize functions for now and
> just pass u8* there ;). We can tackle the others when we get to them.
Sure.
-Takahiro Akashi
> Sorry to make you go through this - I should've caught that back when
> Rob submitted the patches.
>
>
> Alex
>
> >
> >> The next problem are the casts. If you cast from u8 * to u32 * and
> >> dereference it, the compiler does not know if that pointer is aligned or
> >> not. So on platforms that don't enable caches yet in the bootmgr path,
> >> we may get unaligned exceptions. So you want to use get_unaligned_le32()
> >> and friends or memcpy (as you did in your function) above.
> >
> > Good point. I will fix them.
> > (This reveals other bugs :)
> >
> > -Takahiro Akashi
> >
> >>
> >> Alex
> >>
> >>> + struct efi_device_path *file_path,
> >>> + char *option, void **data)
> >>> +{
> >>> + unsigned long size;
> >>> + unsigned long label_len, option_len;
> >>> + u16 file_path_len;
> >>> + void *p;
> >>> +
> >>> + label_len = (u16_strlen(label) + 1) * sizeof(u16);
> >>> + file_path_len = efi_dp_size(file_path)
> >>> + + sizeof(struct efi_device_path); /* for END */
> >>> + option_len = strlen(option);
> >>> +
> >>> + /* total size */
> >>> + size = sizeof(attr);
> >>> + size += file_path_len;
> >>> + size += label_len;
> >>> + size += option_len + 1;
> >>> + p = malloc(size);
> >>> + if (!p)
> >>> + return 0;
> >>> +
> >>> + /* copy data */
> >>> + *data = p;
> >>> + memcpy(p, &attr, sizeof(attr));
> >>> + p += sizeof(attr);
> >>> + memcpy(p, &file_path_len, sizeof(file_path_len));
> >>> + p += sizeof(file_path_len);
> >>> + memcpy(p, label, label_len);
> >>> + p += label_len;
> >>> +
> >>> + memcpy(p, file_path, file_path_len);
> >>> + p += file_path_len;
> >>> +
> >>> + memcpy(p, option, option_len);
> >>> + p += option_len;
> >>> + *(char *)p = '\0';
> >>> +
> >>> + return size;
> >>> +}
> >>> +
> >>> /* free() the result */
> >>> static void *get_var(u16 *name, const efi_guid_t *vendor,
> >>> efi_uintn_t *size)
> >>> @@ -115,7 +137,7 @@ static void *try_load_entry(uint16_t n, struct
> >>> efi_device_path **device_path,
> >>> if (!load_option)
> >>> return NULL;
> >>>
> >>> - parse_load_option(&lo, load_option);
> >>> + efi_parse_load_option(&lo, load_option);
> >>>
> >>> if (lo.attributes & LOAD_OPTION_ACTIVE) {
> >>> efi_status_t ret;
> >>>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
