[PATCH] efi_loader: Clean up efi_dp_append and efi_dp_concat
Heinrich Schuchardt
xypron.glpk at gmx.de
Mon Nov 13 15:37:38 CET 2023
On 11/7/23 18:36, Ilias Apalodimas wrote:
> Looking back at the initrd storing functionality, we introduced three
> functions, efi_dp_append_or_concatenate(), efi_dp_append/concat(). In
> hindsight we could have simplified that by a lot. First of all none of
> the functions append anything. They all allocate a new device path and
> concatenate the contents of two device paths in one. A boolean parameter
> controls the final device path -- if that's true an end node is injected
> between the two device paths.
>
> So let's rewrite this and make it a bit easier to read. Get rid of
> efi_dp_append(), efi_dp_concat() and rename
> efi_dp_append_or_concatenate() to efi_dp_concat(). This is far more
> intuitive and the only adjustment that is needed is an extra boolean
> argument on all callsites.
After this patch we still have efi_dp_append_instance(). The only
difference to efi_dp_concat(,,true) seems only to be the type of end
node used as separator.
Hence the last argument of efi_dp_contat() should be be either of:
* 0
* DEVICE_PATH_SUB_TYPE_INSTANCE_END
* DEVICE_PATH_SUB_TYPE_END
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> ---
> Kojima-san, I think this might affect your EFI HTTP boot series.
> I don't mind waiting for this and merging it after your series goes in
> (and adjust it). The changes should be trivial anyway
>
> cmd/bootefi.c | 4 +-
> cmd/eficonfig.c | 7 ++-
> cmd/efidebug.c | 6 +-
> include/efi_loader.h | 5 +-
> lib/efi_loader/efi_bootmgr.c | 2 +-
> lib/efi_loader/efi_boottime.c | 2 +-
> lib/efi_loader/efi_device_path.c | 66 ++++++----------------
> lib/efi_loader/efi_device_path_utilities.c | 2 +-
> 8 files changed, 32 insertions(+), 62 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 20e5c94a33a4..eb839136bf7e 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -513,8 +513,8 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
> goto out;
> msg_path = file_path;
> } else {
> - file_path = efi_dp_append(bootefi_device_path,
> - bootefi_image_path);
> + file_path = efi_dp_concat(bootefi_device_path,
> + bootefi_image_path, false);
> msg_path = bootefi_image_path;
> log_debug("Loaded from disk\n");
> }
> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> index e6e8a0a488e7..470adb7eddbf 100644
> --- a/cmd/eficonfig.c
> +++ b/cmd/eficonfig.c
> @@ -528,7 +528,7 @@ struct efi_device_path *eficonfig_create_device_path(struct efi_device_path *dp_
> p += fp_size;
> *((struct efi_device_path *)p) = END;
>
> - dp = efi_dp_append(dp_volume, (struct efi_device_path *)buf);
> + dp = efi_dp_concat(dp_volume, (struct efi_device_path *)buf, false);
> free(buf);
>
> return dp;
> @@ -1481,7 +1481,8 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
> ret = EFI_OUT_OF_RESOURCES;
> goto out;
> }
> - initrd_dp = efi_dp_append((const struct efi_device_path *)&id_dp, dp);
> + initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_dp,
> + dp, false);
> efi_free_pool(dp);
> }
>
> @@ -1492,7 +1493,7 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
> }
> final_dp_size = efi_dp_size(dp) + sizeof(END);
> if (initrd_dp) {
> - final_dp = efi_dp_concat(dp, initrd_dp);
> + final_dp = efi_dp_concat(dp, initrd_dp, true);
> final_dp_size += efi_dp_size(initrd_dp) + sizeof(END);
> } else {
> final_dp = efi_dp_dup(dp);
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index 201531ac19fc..a62298bf5987 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -698,8 +698,8 @@ struct efi_device_path *create_initrd_dp(const char *dev, const char *part,
> if (!short_fp)
> short_fp = tmp_fp;
>
> - initrd_dp = efi_dp_append((const struct efi_device_path *)&id_dp,
> - short_fp);
> + initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_dp,
> + short_fp, false);
>
> out:
> efi_free_pool(tmp_dp);
> @@ -841,7 +841,7 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
> goto out;
> }
>
> - final_fp = efi_dp_concat(file_path, initrd_dp);
> + final_fp = efi_dp_concat(file_path, initrd_dp, true);
> if (!final_fp) {
> printf("Cannot create final device path\n");
> r = CMD_RET_FAILURE;
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index e24410505f40..398cd20c7ae6 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -808,8 +808,6 @@ efi_uintn_t efi_dp_instance_size(const struct efi_device_path *dp);
> /* size of multi-instance device path excluding end node */
> efi_uintn_t efi_dp_size(const struct efi_device_path *dp);
> struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp);
> -struct efi_device_path *efi_dp_append(const struct efi_device_path *dp1,
> - const struct efi_device_path *dp2);
> struct efi_device_path *efi_dp_append_node(const struct efi_device_path *dp,
> const struct efi_device_path *node);
> /* Create a device path node of given type, sub-type, length */
> @@ -928,7 +926,8 @@ struct efi_load_option {
> struct efi_device_path *efi_dp_from_lo(struct efi_load_option *lo,
> const efi_guid_t *guid);
> struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1,
> - const struct efi_device_path *dp2);
> + const struct efi_device_path *dp2,
> + bool split_end_node);
> struct efi_device_path *search_gpt_dp_node(struct efi_device_path *device_path);
> efi_status_t efi_deserialize_load_option(struct efi_load_option *lo, u8 *data,
> efi_uintn_t *size);
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index a40762c74c83..646c7c7faaad 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -110,7 +110,7 @@ static efi_status_t try_load_from_file_path(efi_handle_t *fs_handles,
> if (!dp)
> continue;
>
> - dp = efi_dp_append(dp, fp);
> + dp = efi_dp_concat(dp, fp, false);
> if (!dp)
> continue;
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 0b7579cb5af1..709ea07655c2 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1821,7 +1821,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
> if (device_path) {
> info->device_handle = efi_dp_find_obj(device_path, NULL, NULL);
>
> - dp = efi_dp_append(device_path, file_path);
> + dp = efi_dp_concat(device_path, file_path, false);
> if (!dp) {
> ret = EFI_OUT_OF_RESOURCES;
> goto failure;
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index ed7214f3a347..ad79b65843e9 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -272,30 +272,27 @@ struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp)
> }
>
> /**
> - * efi_dp_append_or_concatenate() - Append or concatenate two device paths.
> - * Concatenated device path will be separated
> - * by a sub-type 0xff end node
> + * efi_dp_concat() - Concatenate two device paths and add and terminate them
> + * with an end node.
> *
> - * @dp1: First device path
> - * @dp2: Second device path
> - * @concat: If true the two device paths will be concatenated and separated
> - * by an end of entrire device path sub-type 0xff end node.
> - * If true the second device path will be appended to the first and
> - * terminated by an end node
> + * @dp1: First device path
> + * @dp2: Second device path
> + * @split_end_node: If true the two device paths will be concatenated and
> + * separated by an end node.
This description is unclear. Do you mean DEVICE_PATH_SUB_TYPE_END or
DEVICE_PATH_SUB_TYPE_INSTANCE_END?
We would better make the parameter a three-state value:
* 0
* DEVICE_PATH_SUB_TYPE_INSTANCE_END
* DEVICE_PATH_SUB_TYPE_END
> + * If false the second device path will be concatenated to the
> + * first one as-is.
> *
> * Return:
> * concatenated device path or NULL. Caller must free the returned value
> */
> -static struct
> -efi_device_path *efi_dp_append_or_concatenate(const struct efi_device_path *dp1,
> - const struct efi_device_path *dp2,
> - bool concat)
> +struct
> +efi_device_path *efi_dp_concat(const struct efi_device_path *dp1,
> + const struct efi_device_path *dp2,
> + bool split_end_node)
> {
> struct efi_device_path *ret;
> size_t end_size = sizeof(END);
This assignment is not needed if (!dp1 || !dp2).
>
> - if (concat)
> - end_size = 2 * sizeof(END);
> if (!dp1 && !dp2) {
> /* return an end node */
> ret = efi_dp_dup(&END);
> @@ -307,14 +304,18 @@ efi_device_path *efi_dp_append_or_concatenate(const struct efi_device_path *dp1,
> /* both dp1 and dp2 are non-null */
> unsigned sz1 = efi_dp_size(dp1);
> unsigned sz2 = efi_dp_size(dp2);
> - void *p = efi_alloc(sz1 + sz2 + end_size);
> + void *p;
> +
> + if (split_end_node)
> + end_size = 2 * sizeof(END);
We assign end_size twice in this case. We should instead move the first
assignment to the if:
if (!split_end_node)
end_size = sizeof(END);
else
end_size = 2 * sizeof(END);
Best regards
Heinrich
> + p = efi_alloc(sz1 + sz2 + end_size);
> if (!p)
> return NULL;
> ret = p;
> memcpy(p, dp1, sz1);
> p += sz1;
>
> - if (concat) {
> + if (split_end_node) {
> memcpy(p, &END, sizeof(END));
> p += sizeof(END);
> }
> @@ -328,37 +329,6 @@ efi_device_path *efi_dp_append_or_concatenate(const struct efi_device_path *dp1,
> return ret;
> }
>
> -/**
> - * efi_dp_append() - Append a device to an existing device path.
> - *
> - * @dp1: First device path
> - * @dp2: Second device path
> - *
> - * Return:
> - * concatenated device path or NULL. Caller must free the returned value
> - */
> -struct efi_device_path *efi_dp_append(const struct efi_device_path *dp1,
> - const struct efi_device_path *dp2)
> -{
> - return efi_dp_append_or_concatenate(dp1, dp2, false);
> -}
> -
> -/**
> - * efi_dp_concat() - Concatenate 2 device paths. The final device path will
> - * contain two device paths separated by and end node (0xff).
> - *
> - * @dp1: First device path
> - * @dp2: Second device path
> - *
> - * Return:
> - * concatenated device path or NULL. Caller must free the returned value
> - */
> -struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1,
> - const struct efi_device_path *dp2)
> -{
> - return efi_dp_append_or_concatenate(dp1, dp2, true);
> -}
> -
> struct efi_device_path *efi_dp_append_node(const struct efi_device_path *dp,
> const struct efi_device_path *node)
> {
> diff --git a/lib/efi_loader/efi_device_path_utilities.c b/lib/efi_loader/efi_device_path_utilities.c
> index a07d9bab3a3c..fa211aee495e 100644
> --- a/lib/efi_loader/efi_device_path_utilities.c
> +++ b/lib/efi_loader/efi_device_path_utilities.c
> @@ -77,7 +77,7 @@ static struct efi_device_path * EFIAPI append_device_path(
> const struct efi_device_path *src2)
> {
> EFI_ENTRY("%pD, %pD", src1, src2);
> - return EFI_EXIT(efi_dp_append(src1, src2));
> + return EFI_EXIT(efi_dp_concat(src1, src2, false));
> }
>
> /*
> --
> 2.40.1
>
More information about the U-Boot
mailing list