[U-Boot] [PATCH 3/6] efi_loader: bootmgr: add load option helper functions

[AKASHI Takahiro]

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.akashi at 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.

> 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;
> >