[PATCH v2 1/2] cmd: eficonfig: add support for URI device path based boot options
Heinrich Schuchardt
xypron.glpk at gmx.de
Thu Jun 26 14:59:37 CEST 2025
On 26.06.25 11:28, Sughosh Ganu wrote:
> On Thu, 26 Jun 2025 at 13:52, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>
>> On 24.06.25 14:07, Sughosh Ganu wrote:
>>> The eficonfig command provides a menu based interface for maintenance
>>> of the EFI boot options. Add support for adding a URI based boot
>>> option. This boot option can then be used for HTTP boot.
>>>
>>> Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
>>> ---
>>> Changes since V1:
>>> * Move the option for entering the URI under the boot file.
>>> * All corresponding changes needed for the above change.
>>>
>>> cmd/eficonfig.c | 113 ++++++++++++++++++++++++++++++++++++++++---
>>> include/efi_config.h | 4 ++
>>> 2 files changed, 109 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
>>> index 6e14d34a6bd..917d79b8023 100644
>>> --- a/cmd/eficonfig.c
>>> +++ b/cmd/eficonfig.c
>>> @@ -35,6 +35,7 @@ static int avail_row;
>>>
>>> #define EFICONFIG_DESCRIPTION_MAX 32
>>> #define EFICONFIG_OPTIONAL_DATA_MAX 64
>>> +#define EFICONFIG_URI_MAX 512
>>
>> I am not aware of as standard setting a limit on URI sizes.
>>
>> https://www.rfc-editor.org/rfc/rfc9110#section-4.1-5 has this
>> recommendation:
>>
>> It is RECOMMENDED that all senders and recipients support, at a minimum,
>> URIs with lengths of 8000 octets in protocol elements.
>>
>> We should determine the length of the URI when the user makes his input.
>
> I had checked the corresponding RFC to check if there was a mention of
> size limit on the URI, and there is none. There is a mention about
> some applications using 2K as a limit for the URI length. But in
> U-Boot's context, I think that 512 bytes is a good enough limit, given
> that we are using the URI primarily for specifying distro installation
> image's path.
>
> Also, the handle_user_input() function in the eficonfig tool expects a
> size limit to be provided for all the user input, and I think this is
> a sane thing to do rather than allowing the user to input random
> strings. Do you see a realistic use-case where a URI of more than 512
> characters might be expected for a boot option?
I do not control how people are setting up their image servers. E.g. if
you create images on the fly and pass GET parameters URI's can become
long. That is why I refer to the RFC which indicates bounds. But I agree
that in most cases 512 will be enough.
You are using efi_console_get_u16_string() which is not usable to edit
strings that are longer than a screen line properly. If I enter 512
times the character "a" and the backspace 512 times, I get:
** Edit URI **
enter HTTP Boot URI:
a
a
a
a
I did not expect "Press ENTER to complete, ESC to quit" to be
overwritten and I did not expect characters at the start of the line not
being deleted when backspacing.
>
>>
>>> #define EFICONFIG_MENU_HEADER_ROW_NUM 3
>>> #define EFICONFIG_MENU_DESC_ROW_NUM 5
>>>
>>> @@ -538,6 +539,31 @@ struct efi_device_path *eficonfig_create_device_path(struct efi_device_path *dp_
>>> return dp;
>>> }
>>>
>>> +static struct efi_device_path *eficonfig_create_uri_device_path(u16 *uri_str)
>>> +{
>>> + char *pos, *p;
>>> + u32 len = 0;
>>> + efi_uintn_t uridp_len;
>>> + struct efi_device_path_uri *uridp;
>>> +
>>> + len = utf16_utf8_strlen(uri_str);
>>> +
>>> + uridp_len = sizeof(struct efi_device_path) + len + 1;
>>> + uridp = efi_alloc(uridp_len + sizeof(EFI_DP_END));
>>> + if (!uridp)
>>> + return NULL;
>>> +
>>> + uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
>>> + uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI;
>>> + uridp->dp.length = uridp_len;
>>> + p = (char *)&uridp->uri;
>>> + utf16_utf8_strcpy(&p, uri_str);
>>> + pos = (char *)uridp + uridp_len;
>>> + memcpy(pos, &EFI_DP_END, sizeof(EFI_DP_END));
>>> +
>>> + return &uridp->dp;
>>> +}
>>> +
>>> /**
>>> * eficonfig_file_selected() - handler of file selection
>>> *
>>> @@ -983,6 +1009,22 @@ static efi_status_t eficonfig_boot_add_optional_data(void *data)
>>> " enter optional data:");
>>> }
>>>
>>> +/**
>>> + * eficonfig_boot_add_uri() - handle user input for HTTP Boot URI
>>> + *
>>> + * @data: pointer to the internal boot option structure
>>> + * Return: status code
>>> + */
>>> +static efi_status_t eficonfig_boot_add_uri(void *data)
>>> +{
>>> + struct eficonfig_select_file_info *file_info = data;
>>> +
>>> + return handle_user_input(file_info->uri, EFICONFIG_URI_MAX, 24,
>>> + "\n ** Edit URI **\n"
>>> + "\n"
>>> + " enter HTTP Boot URI:");
>>> +}
>>> +
>>> /**
>>> * eficonfig_boot_edit_save() - handler to save the boot option
>>> *
>>> @@ -998,7 +1040,8 @@ static efi_status_t eficonfig_boot_edit_save(void *data)
>>> bo->edit_completed = false;
>>> return EFI_NOT_READY;
>>> }
>>> - if (u16_strlen(bo->file_info.current_path) == 0) {
>>> + if (u16_strlen(bo->file_info.current_path) == 0 &&
>>> + u16_strlen(bo->file_info.uri) == 0) {
>>> eficonfig_print_msg("File is not selected!");
>>> bo->edit_completed = false;
>>> return EFI_NOT_READY;
>>> @@ -1027,6 +1070,13 @@ efi_status_t eficonfig_process_clear_file_selection(void *data)
>>> return EFI_ABORTED;
>>> }
>>>
>>> +static struct eficonfig_item select_boot_file_menu_items[] = {
>>> + {"Select File", eficonfig_process_select_file},
>>> + {"Enter URI", eficonfig_boot_add_uri},
>>> + {"Clear", eficonfig_process_clear_file_selection},
>>> + {"Quit", eficonfig_process_quit},
>>> +};
>>> +
>>> static struct eficonfig_item select_file_menu_items[] = {
>>> {"Select File", eficonfig_process_select_file},
>>> {"Clear", eficonfig_process_clear_file_selection},
>>> @@ -1042,20 +1092,35 @@ static struct eficonfig_item select_file_menu_items[] = {
>>> efi_status_t eficonfig_process_show_file_option(void *data)
>>> {
>>> efi_status_t ret;
>>> + unsigned int menu_count;
>>> struct efimenu *efi_menu;
>>> + struct eficonfig_item *menu_items;
>>> + struct eficonfig_select_file_info *file_info = data;
>>> +
>>> + menu_items = file_info->boot_file_option ? select_boot_file_menu_items :
>>> + select_file_menu_items;
>>> +
>>> + menu_count = file_info->boot_file_option ?
>>> + ARRAY_SIZE(select_boot_file_menu_items) :
>>> + ARRAY_SIZE(select_file_menu_items);
>>>
>>> - select_file_menu_items[0].data = data;
>>> - select_file_menu_items[1].data = data;
>>> - efi_menu = eficonfig_create_fixed_menu(select_file_menu_items,
>>> - ARRAY_SIZE(select_file_menu_items));
>>> + menu_items[0].data = data;
>>> + menu_items[1].data = data;
>>> + menu_items[2].data = data;
>>> +
>>> + efi_menu = eficonfig_create_fixed_menu(menu_items, menu_count);
>>> if (!efi_menu)
>>> return EFI_OUT_OF_RESOURCES;
>>>
>>> - ret = eficonfig_process_common(efi_menu, " ** Update File **",
>>> + ret = eficonfig_process_common(efi_menu,
>>> + file_info->boot_file_option ?
>>> + " ** Update File/URI **" :
>>> + " ** Update File **",
>>> eficonfig_menu_desc,
>>> eficonfig_display_statusline,
>>> eficonfig_print_entry,
>>> eficonfig_choice_entry);
>>> +
>>> if (ret != EFI_SUCCESS) /* User selects "Clear" or "Quit" */
>>> ret = EFI_NOT_READY;
>>>
>>> @@ -1268,6 +1333,7 @@ static efi_status_t prepare_file_selection_entry(struct efimenu *efi_menu, char
>>> u16_strlcat(file_name, file_info->current_path, len);
>>> ret = create_boot_option_entry(efi_menu, title, file_name,
>>> eficonfig_process_show_file_option, file_info);
>>> +
>>> out:
>>> free(devname);
>>> free(file_name);
>>> @@ -1340,6 +1406,16 @@ out:
>>> return ret;
>>> }
>>>
>>
>> Please, document all functions according to
>> https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
>
> So I have had this comment from you on my other patches as well. I am
> not adding any API in my patch. What exactly is the rule for adding
> function documentation? I can understand if some static function
> happens to be doing a lot of work, and then it would be easier to
> understand its functionality through comments. But I don't think any
> function being added in this patch qualifies for such documentation
> being needed.
Developers succeeding you will want to know what each function is doing.
We should document non-API functions as well.
>
>>
>>> +static void fill_dp_uri(struct efi_device_path *dp, u16 **uri_str)
>>> +{
>>> + u16 *p = *uri_str;
>>> + struct efi_device_path_uri *uridp;
>>> +
>>> + uridp = (struct efi_device_path_uri *)dp;
>>> +
>>> + utf8_utf16_strcpy(&p, uridp->uri);
>>> +}
>>> +
>>> /**
>>> * fill_file_info() - fill the file info from efi_device_path structure
>>> *
>>> @@ -1392,10 +1468,13 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
>>> size_t len;
>>> efi_status_t ret;
>>> char *tmp = NULL, *p;
>>> + u16 *current_path = NULL;
>>> struct efi_load_option lo = {0};
>>> efi_uintn_t dp_size;
>>> struct efi_device_path *dp = NULL;
>>> efi_uintn_t size = load_option_size;
>>> + struct efi_device_path *dp_volume = NULL;
>>> + struct efi_device_path *uri_dp = NULL;
>>> struct efi_device_path *device_dp = NULL;
>>> struct efi_device_path *initrd_dp = NULL;
>>> struct efi_device_path *fdt_dp = NULL;
>>> @@ -1464,6 +1543,14 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
>>> goto out;
>>> }
>>>
>>> + bo->file_info.uri = calloc(1, EFICONFIG_URI_MAX * sizeof(u16));
>>
>> Where is the matching free()?
>
> There is one being added below.
>>
>>> + if (!bo->file_info.uri) {
>>> + ret = EFI_OUT_OF_RESOURCES;
>>> + goto out;
>>> + }
>>> +
>>> + bo->file_info.boot_file_option = true;
>>> +
>>> /* copy the preset value */
>>> if (load_option) {
>>> ret = efi_deserialize_load_option(&lo, load_option, &size);
>>> @@ -1481,7 +1568,10 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
>>> u16_strcpy(bo->description, lo.label);
>>>
>>> /* EFI image file path is a first instance */
>>> - if (lo.file_path)
>>> + if (lo.file_path && EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE,
>>> + MSG_URI))
>>> + fill_dp_uri(lo.file_path, &bo->file_info.uri);
>>> + else
>>> fill_file_info(lo.file_path, &bo->file_info, device_dp);
>>>
>>> /* Initrd file path (optional) is placed at second instance. */
>>> @@ -1512,6 +1602,9 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
>>> goto out;
>>> }
>>>
>>> + if (utf16_utf8_strlen(bo->file_info.uri))
>>> + uri_dp = eficonfig_create_uri_device_path(bo->file_info.uri);
>>> +
>>> if (bo->initrd_info.dp_volume) {
>>> dp = eficonfig_create_device_path(bo->initrd_info.dp_volume,
>>> bo->initrd_info.current_path);
>>> @@ -1536,7 +1629,10 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
>>> efi_free_pool(dp);
>>> }
>>>
>>> - dp = eficonfig_create_device_path(bo->file_info.dp_volume, bo->file_info.current_path);
>>> + dp_volume = bo->file_info.dp_volume;
>>> + current_path = bo->file_info.current_path;
>>> + dp = uri_dp ?
>>> + uri_dp : eficonfig_create_device_path(dp_volume, current_path);
>>> if (!dp) {
>>> ret = EFI_OUT_OF_RESOURCES;
>>> goto out;
>>> @@ -1560,6 +1656,7 @@ out:
>>> free(tmp);
>>> free(bo->optional_data);
>>> free(bo->description);
>>> + free(bo->file_info.uri);
>
> This is the matching free.
ok
Best regards
Heinrich
>
> -sughosh
>
>>> free(bo->file_info.current_path);
>>> free(bo->initrd_info.current_path);
>>> free(bo->fdt_info.current_path);
>>> diff --git a/include/efi_config.h b/include/efi_config.h
>>> index d7c1601137e..4438933db6a 100644
>>> --- a/include/efi_config.h
>>> +++ b/include/efi_config.h
>>> @@ -82,15 +82,19 @@ struct eficonfig_item {
>>> * @current_volume: pointer to the efi_simple_file_system_protocol
>>> * @dp_volume: pointer to device path of the selected device
>>> * @current_path: pointer to the selected file path string
>>> + * @uri: URI for HTTP Boot
>>> * @filepath_list: list_head structure for file path list
>>> * @file_selectred: flag indicates file selecting status
>>> + * @boot_file_option: flag indicating if option is for boot file
>>> */
>>> struct eficonfig_select_file_info {
>>> struct efi_simple_file_system_protocol *current_volume;
>>> struct efi_device_path *dp_volume;
>>> u16 *current_path;
>>> + u16 *uri;
>>> struct list_head filepath_list;
>>> bool file_selected;
>>> + bool boot_file_option;
>>> };
>>>
>>> void eficonfig_print_msg(char *msg);
>>
More information about the U-Boot
mailing list