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

[Alexander Graf]

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? It
probably also wants an efi_ prefix then :).

> +
> +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.

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.

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.

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 *).

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.


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