From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Thu, 18 Oct 2018 16:57:14 +0900 Subject: [U-Boot] [PATCH 3/6] efi_loader: bootmgr: add load option helper functions In-Reply-To: <810867f4-7741-1a45-7580-d872b33d5865@suse.de> References: <20181017073207.13150-1-takahiro.akashi@linaro.org> <20181017073207.13150-4-takahiro.akashi@linaro.org> <810867f4-7741-1a45-7580-d872b33d5865@suse.de> Message-ID: <20181018075712.GJ32578@linaro.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > > --- > > 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. > 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'. > 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. > 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; > >