| Message ID | 20210113111149.64567-2-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Change logic of EFI LoadFile2 protocol for initrd loading | expand |
On 13.01.21 12:11, Ilias Apalodimas wrote: > A following patch introduces a different logic for loading initrd's > based on the EFI_LOAD_FILE2_PROTOCOL. > Since similar logic can be applied in the future for other system files > (i.e DTBs), let's add some helper functions which will retrieve and > parse file paths stored in EFI load options. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > include/efi_helper.h | 23 ++++++ > lib/efi_loader/efi_helper.c | 146 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 169 insertions(+) > create mode 100644 include/efi_helper.h > create mode 100644 lib/efi_loader/efi_helper.c > > diff --git a/include/efi_helper.h b/include/efi_helper.h > new file mode 100644 > index 000000000000..4e6bd2f036df > --- /dev/null > +++ b/include/efi_helper.h > @@ -0,0 +1,23 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > +/* > + * Copyright (c) 2020, Linaro Limited > + */ > + > +#if !defined _EFI_HELPER_H_ > +#define _EFI_HELPER_H > + > +#include <efi.h> > +#include <efi_api.h> > + > +enum load_option_dp_type { > + BOOT_IMAGE_DP, > + INITRD_DP, > + DTB_DP, > +}; > + > +void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size); > +struct efi_device_path *efi_get_fp_from_boot(int idx); > +struct efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp, > + efi_uintn_t *size, int idx); > + > +#endif > diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c > new file mode 100644 > index 000000000000..e2437a4f698b > --- /dev/null > +++ b/lib/efi_loader/efi_helper.c > @@ -0,0 +1,146 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (c) 2020, Linaro Limited > + */ > + > +#include <common.h> > +#include <env.h> > +#include <malloc.h> > +#include <dm.h> > +#include <fs.h> > +#include <efi_helper.h> > +#include <efi_loader.h> > +#include <efi_variable.h> > + > +/** > + * efi_get_var() - read value of an EFI variable > + * > + * @name: variable name > + * @start: vendor GUID > + * @size: size of allocated buffer > + * > + * Return: buffer with variable data or NULL > + */ > +void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size) > +{ > + efi_status_t ret; > + void *buf = NULL; > + > + *size = 0; > + ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL); > + if (ret == EFI_BUFFER_TOO_SMALL) { > + buf = malloc(*size); Please, always check the output of malloc(), e.g. if (!buf) ret = EFI_OUT_OF_RESOURCES; else > + ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL); > + } > + > + if (ret != EFI_SUCCESS) { > + free(buf); > + *size = 0; > + return NULL; > + } > + > + return buf; > +} > + > +/** > + * efi_dp_instance_by_idx() - Get a file path with a specific index > + * > + * @name: device path array > + * @size: size of the discovered device path > + * @idx: index of the device path > + * > + * Return: device path or NULL. Caller must free the returned value > + */ > + > +struct > +efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp, > + efi_uintn_t *size, int idx) > +{ idx should be of an unsigned type as we cannot have negative indices. > + struct efi_device_path *instance = NULL; > + efi_uintn_t instance_size = 0; > + > + if (!efi_dp_is_multi_instance(dp)) Given a device path with one instance, why don't you allow me to read index 0? I assume this check can be removed. > + return NULL; > + > + while (idx >= 0 && dp) { > + instance = efi_dp_get_next_instance(&dp, &instance_size); > + if (idx && instance) { > + efi_free_pool(instance); > + instance_size = 0; > + instance = NULL; > + } > + idx--; > + } > + *size = instance_size; > + > + return instance; This can be simplified with unsigned idx: for (; dp; --idx) { instance = efi_dp_get_next_instance(&dp, size); if (!idx) { return instance; efi_free_pool(instance); } return NULL; > +} > + > +/** > + * create_boot_var_indexed() - Return Boot#### name were #### is replaced by > + * the value of BootCurrent > + * > + * @var_name: variable name > + * @var_name_size: size of var_name > + * > + * Return: Status code > + */ > +static efi_status_t create_boot_var_indexed(u16 var_name[], size_t var_name_size) > +{ > + efi_uintn_t boot_order_size; You are reading BootCurrent, not BootOrder. > + efi_status_t ret; > + u16 boot_order; Same here. > + u16 *pos; > + > + boot_order_size = sizeof(boot_order); > + ret = efi_get_variable_int(L"BootCurrent", > + &efi_global_variable_guid, NULL, > + &boot_order_size, &boot_order, NULL); > + if (ret != EFI_SUCCESS) > + goto out; > + > + pos = efi_create_indexed_name(var_name, var_name_size, "Boot", > + boot_order); > + if (!pos) { > + ret = EFI_OUT_OF_RESOURCES; > + goto out; > + } > + > +out: > + return ret; > +} > + > +/** > + * efi_get_fp_from_var() - Retrieve and return a device path from an EFI > + * Boot### variable > + * > + * @idx: index of the instance to retrieve > + * > + * Return: device path of NULL. Caller must free the returned value %s/of/or/ > + */ > +struct efi_device_path *efi_get_fp_from_boot(int idx) > +{ > + struct efi_device_path *file_path; > + struct efi_load_option lo; > + void *var_value = NULL; > + efi_uintn_t size; > + efi_status_t ret; > + u16 var_name[16]; > + > + ret = create_boot_var_indexed(var_name, sizeof(var_name)); > + if (ret != EFI_SUCCESS) > + return NULL; > + > + var_value = efi_get_var(var_name, &efi_global_variable_guid, &size); > + if (!var_value) > + return NULL; > + > + efi_deserialize_load_option(&lo, var_value, &size); > + file_path = efi_dp_instance_by_idx(lo.file_path, &size, idx); > + if (!file_path) { > + printf("Instance not found\n"); This message is not enough for a user to take action. I suggest ("Missing file path with index %d in %ls", idx, var_name) We want to use logging. I assume this is not an error. Can we use log_debug() here? > + return NULL; > + } > + > + return file_path; > +} > Some other functions would fit into the same C module. Candidates are: * efi_create_indexed_name() * efi_deserialize_load_option() * efi_serialize_load_option() But that can be done in a separate patch. Best regards Heinrich
Hi Heinrich, > > + efi_status_t ret; > > + void *buf = NULL; > > + > > + *size = 0; > > + ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL); > > + if (ret == EFI_BUFFER_TOO_SMALL) { > > + buf = malloc(*size); > > Please, always check the output of malloc(), e.g. > > if (!buf) > ret = EFI_OUT_OF_RESOURCES; > else > I just moved the function from lib/efi_loader/efi_bootmgr.c (check for get_var()) and completely missed that. I'll fix it on the next revision. > > + ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL); > > + } > > + > > + if (ret != EFI_SUCCESS) { > > + free(buf); > > + *size = 0; > > + return NULL; > > + } > > + > > + return buf; > > +} > > + > > +/** > > + * efi_dp_instance_by_idx() - Get a file path with a specific index > > + * > > + * @name: device path array > > + * @size: size of the discovered device path > > + * @idx: index of the device path > > + * > > + * Return: device path or NULL. Caller must free the returned value > > + */ > > + > > +struct > > +efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp, > > + efi_uintn_t *size, int idx) > > +{ > > idx should be of an unsigned type as we cannot have negative indices. > > > + struct efi_device_path *instance = NULL; > > + efi_uintn_t instance_size = 0; > > + > > + if (!efi_dp_is_multi_instance(dp)) > > Given a device path with one instance, why don't you allow me to read > index 0? I assume this check can be removed. > Yea, but why would you ever use that? you can just retrieve the deserialized device path directly if there are no other instances. > > + return NULL; > > + > > + while (idx >= 0 && dp) { > > + instance = efi_dp_get_next_instance(&dp, &instance_size); > > + if (idx && instance) { > > + efi_free_pool(instance); > > + instance_size = 0; > > + instance = NULL; > > + } > > + idx--; > > + } > > + *size = instance_size; > > + > > + return instance; > > This can be simplified with unsigned idx: > > for (; dp; --idx) { > instance = efi_dp_get_next_instance(&dp, size); > if (!idx) { > return instance; > efi_free_pool(instance); > } > return NULL; Ok I'll have a look > > > +} > > + > > +/** > > + * create_boot_var_indexed() - Return Boot#### name were #### is replaced by > > + * the value of BootCurrent > > + * > > + * @var_name: variable name > > + * @var_name_size: size of var_name > > + * > > + * Return: Status code > > + */ > > +static efi_status_t create_boot_var_indexed(u16 var_name[], size_t var_name_size) > > +{ > > + efi_uintn_t boot_order_size; > > You are reading BootCurrent, not BootOrder. > > > + efi_status_t ret; > > + u16 boot_order; > > Same here. > Ok > > + if (!file_path) { > > + printf("Instance not found\n"); > > This message is not enough for a user to take action. I suggest > > ("Missing file path with index %d in %ls", idx, var_name) > > We want to use logging. I assume this is not an error. Can we use > log_debug() here? > That's a leftover from my opwn debugging that I neglected to remove prior to the RFC. I'll just add a log_debug() > > + return NULL; > > + } > > + > > + return file_path; > > +} > > > > Some other functions would fit into the same C module. Candidates are: > > * efi_create_indexed_name() > * efi_deserialize_load_option() > * efi_serialize_load_option() > > But that can be done in a separate patch. Yea, that's the idea, we can also use the efi_get_var() in multiple places, but I'll send different patches for that, once we merge this. I assume we agree on the architecture. If so I'll fix the selftests and post a proper patch Regards /Ilias > > Best regards > > Heinrich
On Wed, Jan 13, 2021 at 01:11:47PM +0200, Ilias Apalodimas wrote: > A following patch introduces a different logic for loading initrd's > based on the EFI_LOAD_FILE2_PROTOCOL. > Since similar logic can be applied in the future for other system files > (i.e DTBs), In this respect, > let's add some helper functions which will retrieve and > parse file paths stored in EFI load options. > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > include/efi_helper.h | 23 ++++++ > lib/efi_loader/efi_helper.c | 146 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 169 insertions(+) > create mode 100644 include/efi_helper.h > create mode 100644 lib/efi_loader/efi_helper.c > > diff --git a/include/efi_helper.h b/include/efi_helper.h > new file mode 100644 > index 000000000000..4e6bd2f036df > --- /dev/null > +++ b/include/efi_helper.h > @@ -0,0 +1,23 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > +/* > + * Copyright (c) 2020, Linaro Limited > + */ > + > +#if !defined _EFI_HELPER_H_ > +#define _EFI_HELPER_H > + > +#include <efi.h> > +#include <efi_api.h> > + > +enum load_option_dp_type { > + BOOT_IMAGE_DP, > + INITRD_DP, > + DTB_DP, > +}; > + > +void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size); > +struct efi_device_path *efi_get_fp_from_boot(int idx); > +struct efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp, > + efi_uintn_t *size, int idx); > + > +#endif > diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c > new file mode 100644 > index 000000000000..e2437a4f698b > --- /dev/null > +++ b/lib/efi_loader/efi_helper.c > @@ -0,0 +1,146 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (c) 2020, Linaro Limited > + */ > + > +#include <common.h> > +#include <env.h> > +#include <malloc.h> > +#include <dm.h> > +#include <fs.h> > +#include <efi_helper.h> > +#include <efi_loader.h> > +#include <efi_variable.h> > + > +/** > + * efi_get_var() - read value of an EFI variable > + * > + * @name: variable name > + * @start: vendor GUID > + * @size: size of allocated buffer > + * > + * Return: buffer with variable data or NULL > + */ > +void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size) > +{ > + efi_status_t ret; > + void *buf = NULL; > + > + *size = 0; > + ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL); > + if (ret == EFI_BUFFER_TOO_SMALL) { > + buf = malloc(*size); > + ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL); > + } > + > + if (ret != EFI_SUCCESS) { > + free(buf); > + *size = 0; > + return NULL; > + } > + > + return buf; > +} > + > +/** > + * efi_dp_instance_by_idx() - Get a file path with a specific index > + * > + * @name: device path array > + * @size: size of the discovered device path > + * @idx: index of the device path > + * > + * Return: device path or NULL. Caller must free the returned value > + */ > + > +struct > +efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp, > + efi_uintn_t *size, int idx) The type of "idx" should be 'enum load_option_dp_type'. Currently, "idx" is used as an index into the array of device paths, but given each device path is set to have its own guid, "idx" should be unlinked from the 'order' within the array. Even if you don't want so, this function should at least take care of some special cases like <execution image> + (no initrd) + <dtb> Alternatively, "END device path" can be put at the second place. I expect that handling those corner cases should be described explicitly. > +{ > + struct efi_device_path *instance = NULL; > + efi_uintn_t instance_size = 0; > + > + if (!efi_dp_is_multi_instance(dp)) > + return NULL; > + > + while (idx >= 0 && dp) { > + instance = efi_dp_get_next_instance(&dp, &instance_size); > + if (idx && instance) { > + efi_free_pool(instance); > + instance_size = 0; > + instance = NULL; > + } > + idx--; > + } > + *size = instance_size; > + > + return instance; > +} > + > +/** > + * create_boot_var_indexed() - Return Boot#### name were #### is replaced by > + * the value of BootCurrent > + * > + * @var_name: variable name > + * @var_name_size: size of var_name > + * > + * Return: Status code > + */ > +static efi_status_t create_boot_var_indexed(u16 var_name[], size_t var_name_size) > +{ > + efi_uintn_t boot_order_size; > + efi_status_t ret; > + u16 boot_order; > + u16 *pos; > + > + boot_order_size = sizeof(boot_order); > + ret = efi_get_variable_int(L"BootCurrent", > + &efi_global_variable_guid, NULL, > + &boot_order_size, &boot_order, NULL); > + if (ret != EFI_SUCCESS) > + goto out; > + > + pos = efi_create_indexed_name(var_name, var_name_size, "Boot", > + boot_order); > + if (!pos) { > + ret = EFI_OUT_OF_RESOURCES; > + goto out; > + } > + > +out: > + return ret; > +} > + > +/** > + * efi_get_fp_from_var() - Retrieve and return a device path from an EFI > + * Boot### variable > + * > + * @idx: index of the instance to retrieve > + * > + * Return: device path of NULL. Caller must free the returned value > + */ > +struct efi_device_path *efi_get_fp_from_boot(int idx) Are you sure that this function is called only when "BootCurrent" is appropriately set? > +{ > + struct efi_device_path *file_path; > + struct efi_load_option lo; > + void *var_value = NULL; > + efi_uintn_t size; > + efi_status_t ret; > + u16 var_name[16]; The size of var_name: 16 -> 8 -Takahiro Akashi > + ret = create_boot_var_indexed(var_name, sizeof(var_name)); > + if (ret != EFI_SUCCESS) > + return NULL; > + > + var_value = efi_get_var(var_name, &efi_global_variable_guid, &size); > + if (!var_value) > + return NULL; > + > + efi_deserialize_load_option(&lo, var_value, &size); > + file_path = efi_dp_instance_by_idx(lo.file_path, &size, idx); > + if (!file_path) { > + printf("Instance not found\n"); > + return NULL; > + } > + > + return file_path; > +} > -- > 2.30.0.rc2 >
Am 14. Januar 2021 05:48:28 MEZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>: >On Wed, Jan 13, 2021 at 01:11:47PM +0200, Ilias Apalodimas wrote: >> A following patch introduces a different logic for loading initrd's >> based on the EFI_LOAD_FILE2_PROTOCOL. >> Since similar logic can be applied in the future for other system >files >> (i.e DTBs), > >In this respect, > >> let's add some helper functions which will retrieve and >> parse file paths stored in EFI load options. >> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> >> --- >> include/efi_helper.h | 23 ++++++ >> lib/efi_loader/efi_helper.c | 146 >++++++++++++++++++++++++++++++++++++ >> 2 files changed, 169 insertions(+) >> create mode 100644 include/efi_helper.h >> create mode 100644 lib/efi_loader/efi_helper.c >> >> diff --git a/include/efi_helper.h b/include/efi_helper.h >> new file mode 100644 >> index 000000000000..4e6bd2f036df >> --- /dev/null >> +++ b/include/efi_helper.h >> @@ -0,0 +1,23 @@ >> +/* SPDX-License-Identifier: GPL-2.0+ */ >> +/* >> + * Copyright (c) 2020, Linaro Limited >> + */ >> + >> +#if !defined _EFI_HELPER_H_ >> +#define _EFI_HELPER_H >> + >> +#include <efi.h> >> +#include <efi_api.h> >> + >> +enum load_option_dp_type { >> + BOOT_IMAGE_DP, >> + INITRD_DP, >> + DTB_DP, >> +}; >> + >> +void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t >*size); >> +struct efi_device_path *efi_get_fp_from_boot(int idx); >> +struct efi_device_path *efi_dp_instance_by_idx(struct >efi_device_path *dp, >> + efi_uintn_t *size, int idx); >> + >> +#endif >> diff --git a/lib/efi_loader/efi_helper.c >b/lib/efi_loader/efi_helper.c >> new file mode 100644 >> index 000000000000..e2437a4f698b >> --- /dev/null >> +++ b/lib/efi_loader/efi_helper.c >> @@ -0,0 +1,146 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Copyright (c) 2020, Linaro Limited >> + */ >> + >> +#include <common.h> >> +#include <env.h> >> +#include <malloc.h> >> +#include <dm.h> >> +#include <fs.h> >> +#include <efi_helper.h> >> +#include <efi_loader.h> >> +#include <efi_variable.h> >> + >> +/** >> + * efi_get_var() - read value of an EFI variable >> + * >> + * @name: variable name >> + * @start: vendor GUID >> + * @size: size of allocated buffer >> + * >> + * Return: buffer with variable data or NULL >> + */ >> +void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t >*size) >> +{ >> + efi_status_t ret; >> + void *buf = NULL; >> + >> + *size = 0; >> + ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL); >> + if (ret == EFI_BUFFER_TOO_SMALL) { >> + buf = malloc(*size); >> + ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL); >> + } >> + >> + if (ret != EFI_SUCCESS) { >> + free(buf); >> + *size = 0; >> + return NULL; >> + } >> + >> + return buf; >> +} >> + >> +/** >> + * efi_dp_instance_by_idx() - Get a file path with a specific index >> + * >> + * @name: device path array >> + * @size: size of the discovered device path >> + * @idx: index of the device path >> + * >> + * Return: device path or NULL. Caller must free the returned value >> + */ >> + >> +struct >> +efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp, >> + efi_uintn_t *size, int idx) > >The type of "idx" should be 'enum load_option_dp_type'. There is nothing boot manager specific in this function. You can use it to access any multiple device path. An enum or #define can be used on the caller side. Should this function be moved to efi_devicepath.c? Best regards Heinrich > >Currently, "idx" is used as an index into the array of device paths, >but given each device path is set to have its own guid, "idx" should be >unlinked from the 'order' within the array. > >Even if you don't want so, this function should at least take care of >some special cases like ><execution image> + (no initrd) + <dtb> >Alternatively, "END device path" can be put at the second place. > >I expect that handling those corner cases should be described >explicitly. > >> +{ >> + struct efi_device_path *instance = NULL; >> + efi_uintn_t instance_size = 0; >> + >> + if (!efi_dp_is_multi_instance(dp)) >> + return NULL; >> + >> + while (idx >= 0 && dp) { >> + instance = efi_dp_get_next_instance(&dp, &instance_size); >> + if (idx && instance) { >> + efi_free_pool(instance); >> + instance_size = 0; >> + instance = NULL; >> + } >> + idx--; >> + } >> + *size = instance_size; >> + >> + return instance; >> +} >> + >> +/** >> + * create_boot_var_indexed() - Return Boot#### name were #### is >replaced by >> + * the value of BootCurrent >> + * >> + * @var_name: variable name >> + * @var_name_size: size of var_name >> + * >> + * Return: Status code >> + */ >> +static efi_status_t create_boot_var_indexed(u16 var_name[], size_t >var_name_size) >> +{ >> + efi_uintn_t boot_order_size; >> + efi_status_t ret; >> + u16 boot_order; >> + u16 *pos; >> + >> + boot_order_size = sizeof(boot_order); >> + ret = efi_get_variable_int(L"BootCurrent", >> + &efi_global_variable_guid, NULL, >> + &boot_order_size, &boot_order, NULL); >> + if (ret != EFI_SUCCESS) >> + goto out; >> + >> + pos = efi_create_indexed_name(var_name, var_name_size, "Boot", >> + boot_order); >> + if (!pos) { >> + ret = EFI_OUT_OF_RESOURCES; >> + goto out; >> + } >> + >> +out: >> + return ret; >> +} >> + >> +/** >> + * efi_get_fp_from_var() - Retrieve and return a device path from an >EFI >> + * Boot### variable >> + * >> + * @idx: index of the instance to retrieve >> + * >> + * Return: device path of NULL. Caller must free the returned value >> + */ >> +struct efi_device_path *efi_get_fp_from_boot(int idx) > >Are you sure that this function is called only when "BootCurrent" >is appropriately set? > >> +{ >> + struct efi_device_path *file_path; >> + struct efi_load_option lo; >> + void *var_value = NULL; >> + efi_uintn_t size; >> + efi_status_t ret; >> + u16 var_name[16]; > >The size of var_name: 16 -> 8 > >-Takahiro Akashi > >> + ret = create_boot_var_indexed(var_name, sizeof(var_name)); >> + if (ret != EFI_SUCCESS) >> + return NULL; >> + >> + var_value = efi_get_var(var_name, &efi_global_variable_guid, >&size); >> + if (!var_value) >> + return NULL; >> + >> + efi_deserialize_load_option(&lo, var_value, &size); >> + file_path = efi_dp_instance_by_idx(lo.file_path, &size, idx); >> + if (!file_path) { >> + printf("Instance not found\n"); >> + return NULL; >> + } >> + >> + return file_path; >> +} >> -- >> 2.30.0.rc2 >>
Akashi-san, > > + efi_uintn_t *size, int idx) [...] > > The type of "idx" should be 'enum load_option_dp_type'. > > Currently, "idx" is used as an index into the array of device paths, > but given each device path is set to have its own guid, "idx" should be > unlinked from the 'order' within the array. > > Even if you don't want so, this function should at least take care of > some special cases like > <execution image> + (no initrd) + <dtb> > Alternatively, "END device path" can be put at the second place. > > I expect that handling those corner cases should be described explicitly. > True, I'll refactor this a bit and allow us to extend it to load DTBs in the future. > > +{ > > + struct efi_device_path *instance = NULL; > > + [...] > > +/** > > + * efi_get_fp_from_var() - Retrieve and return a device path from an EFI > > + * Boot### variable > > + * > > + * @idx: index of the instance to retrieve > > + * > > + * Return: device path of NULL. Caller must free the returned value > > + */ > > +struct efi_device_path *efi_get_fp_from_boot(int idx) > > Are you sure that this function is called only when "BootCurrent" > is appropriately set? > Yea, this is either used on the function called by the EFI-stub or try_load_entry() where the BootCurrent value is set explicitly. Regards /Ilias
[...] > >> + * @size: size of the discovered device path > >> + * @idx: index of the device path > >> + * > >> + * Return: device path or NULL. Caller must free the returned value > >> + */ > >> + > >> +struct > >> +efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp, > >> + efi_uintn_t *size, int idx) > > > >The type of "idx" should be 'enum load_option_dp_type'. > > There is nothing boot manager specific in this function. You can use it to access any multiple device path. > > An enum or #define can be used on the caller side. > > Should this function be moved to efi_devicepath.c? Sure it's generic enough to be used for device paths with multiple instances. Regards /Ilias > Best regards > > Heinrich > [...]
diff --git a/include/efi_helper.h b/include/efi_helper.h new file mode 100644 index 000000000000..4e6bd2f036df --- /dev/null +++ b/include/efi_helper.h @@ -0,0 +1,23 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Copyright (c) 2020, Linaro Limited + */ + +#if !defined _EFI_HELPER_H_ +#define _EFI_HELPER_H + +#include <efi.h> +#include <efi_api.h> + +enum load_option_dp_type { + BOOT_IMAGE_DP, + INITRD_DP, + DTB_DP, +}; + +void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size); +struct efi_device_path *efi_get_fp_from_boot(int idx); +struct efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp, + efi_uintn_t *size, int idx); + +#endif diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c new file mode 100644 index 000000000000..e2437a4f698b --- /dev/null +++ b/lib/efi_loader/efi_helper.c @@ -0,0 +1,146 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) 2020, Linaro Limited + */ + +#include <common.h> +#include <env.h> +#include <malloc.h> +#include <dm.h> +#include <fs.h> +#include <efi_helper.h> +#include <efi_loader.h> +#include <efi_variable.h> + +/** + * efi_get_var() - read value of an EFI variable + * + * @name: variable name + * @start: vendor GUID + * @size: size of allocated buffer + * + * Return: buffer with variable data or NULL + */ +void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size) +{ + efi_status_t ret; + void *buf = NULL; + + *size = 0; + ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL); + if (ret == EFI_BUFFER_TOO_SMALL) { + buf = malloc(*size); + ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL); + } + + if (ret != EFI_SUCCESS) { + free(buf); + *size = 0; + return NULL; + } + + return buf; +} + +/** + * efi_dp_instance_by_idx() - Get a file path with a specific index + * + * @name: device path array + * @size: size of the discovered device path + * @idx: index of the device path + * + * Return: device path or NULL. Caller must free the returned value + */ + +struct +efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp, + efi_uintn_t *size, int idx) +{ + struct efi_device_path *instance = NULL; + efi_uintn_t instance_size = 0; + + if (!efi_dp_is_multi_instance(dp)) + return NULL; + + while (idx >= 0 && dp) { + instance = efi_dp_get_next_instance(&dp, &instance_size); + if (idx && instance) { + efi_free_pool(instance); + instance_size = 0; + instance = NULL; + } + idx--; + } + *size = instance_size; + + return instance; +} + +/** + * create_boot_var_indexed() - Return Boot#### name were #### is replaced by + * the value of BootCurrent + * + * @var_name: variable name + * @var_name_size: size of var_name + * + * Return: Status code + */ +static efi_status_t create_boot_var_indexed(u16 var_name[], size_t var_name_size) +{ + efi_uintn_t boot_order_size; + efi_status_t ret; + u16 boot_order; + u16 *pos; + + boot_order_size = sizeof(boot_order); + ret = efi_get_variable_int(L"BootCurrent", + &efi_global_variable_guid, NULL, + &boot_order_size, &boot_order, NULL); + if (ret != EFI_SUCCESS) + goto out; + + pos = efi_create_indexed_name(var_name, var_name_size, "Boot", + boot_order); + if (!pos) { + ret = EFI_OUT_OF_RESOURCES; + goto out; + } + +out: + return ret; +} + +/** + * efi_get_fp_from_var() - Retrieve and return a device path from an EFI + * Boot### variable + * + * @idx: index of the instance to retrieve + * + * Return: device path of NULL. Caller must free the returned value + */ +struct efi_device_path *efi_get_fp_from_boot(int idx) +{ + struct efi_device_path *file_path; + struct efi_load_option lo; + void *var_value = NULL; + efi_uintn_t size; + efi_status_t ret; + u16 var_name[16]; + + ret = create_boot_var_indexed(var_name, sizeof(var_name)); + if (ret != EFI_SUCCESS) + return NULL; + + var_value = efi_get_var(var_name, &efi_global_variable_guid, &size); + if (!var_value) + return NULL; + + efi_deserialize_load_option(&lo, var_value, &size); + file_path = efi_dp_instance_by_idx(lo.file_path, &size, idx); + if (!file_path) { + printf("Instance not found\n"); + return NULL; + } + + return file_path; +}
A following patch introduces a different logic for loading initrd's based on the EFI_LOAD_FILE2_PROTOCOL. Since similar logic can be applied in the future for other system files (i.e DTBs), let's add some helper functions which will retrieve and parse file paths stored in EFI load options. Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- include/efi_helper.h | 23 ++++++ lib/efi_loader/efi_helper.c | 146 ++++++++++++++++++++++++++++++++++++ 2 files changed, 169 insertions(+) create mode 100644 include/efi_helper.h create mode 100644 lib/efi_loader/efi_helper.c -- 2.30.0.rc2