[RFC 04/14] cmd: eficonfig: add support for setting fdt
Heinrich Schuchardt
heinrich.schuchardt at canonical.com
Sat Apr 27 23:25:06 CEST 2024
On 4/27/24 19:21, E Shattow wrote:
> On Fri, Apr 26, 2024 at 7:25 AM Heinrich Schuchardt
> <heinrich.schuchardt at canonical.com> wrote:
>>
>> We already support creating a load option where the device-path
>> field contains the concatenation of the binary device-path and
>> optionally the device path of the initrd which we expose via the
>> EFI_LOAD_FILE2_PROTOCOL.
>>
>> Allow to append another device-path pointing to the device-tree
>> identified by the device-tree GUID.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>> ---
>> cmd/eficonfig.c | 68 +++++++++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 63 insertions(+), 5 deletions(-)
>>
>> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
>> index 1c57e66040b..d314051ee58 100644
>> --- a/cmd/eficonfig.c
>> +++ b/cmd/eficonfig.c
>> @@ -62,6 +62,7 @@ struct eficonfig_filepath_info {
>> struct eficonfig_boot_option {
>> struct eficonfig_select_file_info file_info;
>> struct eficonfig_select_file_info initrd_info;
>> + struct eficonfig_select_file_info fdt_info;
>> unsigned int boot_index;
>> u16 *description;
>> u16 *optional_data;
>> @@ -1308,6 +1309,10 @@ static efi_status_t eficonfig_show_boot_option(struct eficonfig_boot_option *bo,
>> if (ret != EFI_SUCCESS)
>> goto out;
>>
>> + ret = prepare_file_selection_entry(efi_menu, "Fdt File: ", &bo->fdt_info);
>> + if (ret != EFI_SUCCESS)
>> + goto out;
>> +
>> ret = create_boot_option_entry(efi_menu, "Optional Data: ", bo->optional_data,
>> eficonfig_boot_add_optional_data, bo);
>> if (ret != EFI_SUCCESS)
>> @@ -1394,21 +1399,39 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
>> struct efi_device_path *final_dp = NULL;
>> struct efi_device_path *device_dp = NULL;
>> struct efi_device_path *initrd_dp = NULL;
>> + struct efi_device_path *fdt_dp = NULL;
>> struct efi_device_path *initrd_device_dp = NULL;
>> + struct efi_device_path *fdt_device_dp = NULL;
>>
>> - const struct efi_initrd_dp id_dp = {
>> + const struct efi_initrd_dp initrd_prefix = {
>> .vendor = {
>> {
>> DEVICE_PATH_TYPE_MEDIA_DEVICE,
>> DEVICE_PATH_SUB_TYPE_VENDOR_PATH,
>> - sizeof(id_dp.vendor),
>> + sizeof(initrd_prefix.vendor),
>> },
>> EFI_INITRD_MEDIA_GUID,
>> },
>> .end = {
>> DEVICE_PATH_TYPE_END,
>> DEVICE_PATH_SUB_TYPE_END,
>> - sizeof(id_dp.end),
>> + sizeof(initrd_prefix.end),
>> + }
>> + };
>> +
>> + const struct efi_initrd_dp fdt_prefix = {
>> + .vendor = {
>> + {
>> + DEVICE_PATH_TYPE_MEDIA_DEVICE,
>> + DEVICE_PATH_SUB_TYPE_VENDOR_PATH,
>> + sizeof(fdt_prefix.vendor),
>> + },
>> + EFI_FDT_GUID,
>> + },
>> + .end = {
>> + DEVICE_PATH_TYPE_END,
>> + DEVICE_PATH_SUB_TYPE_END,
>> + sizeof(initrd_prefix.end),
>> }
>> };
>>
>> @@ -1424,6 +1447,12 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
>> goto out;
>> }
>>
>> + bo->fdt_info.current_path = calloc(1, EFICONFIG_FILE_PATH_BUF_SIZE);
>> + if (!bo->fdt_info.current_path) {
>> + ret = EFI_OUT_OF_RESOURCES;
>> + goto out;
>> + }
>> +
>> bo->description = calloc(1, EFICONFIG_DESCRIPTION_MAX * sizeof(u16));
>> if (!bo->description) {
>> ret = EFI_OUT_OF_RESOURCES;
>> @@ -1456,13 +1485,20 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
>> if (lo.file_path)
>> fill_file_info(lo.file_path, &bo->file_info, device_dp);
>>
>> - /* Initrd file path(optional) is placed at second instance. */
>> + /* Initrd file path (optional) is placed at second instance. */
>> initrd_dp = efi_dp_from_lo(&lo, &efi_lf2_initrd_guid);
>> if (initrd_dp) {
>> fill_file_info(initrd_dp, &bo->initrd_info, initrd_device_dp);
>> efi_free_pool(initrd_dp);
>> }
>>
>> + /* Fdt file path (optional) is placed as third instance. */
>> + fdt_dp = efi_dp_from_lo(&lo, &efi_guid_fdt);
>> + if (fdt_dp) {
>> + fill_file_info(fdt_dp, &bo->fdt_info, fdt_device_dp);
>> + efi_free_pool(fdt_dp);
>> + }
>> +
>> if (size > 0)
>> memcpy(bo->optional_data, lo.optional_data, size);
>> }
>> @@ -1484,11 +1520,23 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
>> ret = EFI_OUT_OF_RESOURCES;
>> goto out;
>> }
>> - initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_dp,
>> + initrd_dp = efi_dp_concat((const struct efi_device_path *)&initrd_prefix,
>> dp);
>> efi_free_pool(dp);
>> }
>>
>> + if (bo->fdt_info.dp_volume) {
>> + dp = eficonfig_create_device_path(bo->fdt_info.dp_volume,
>> + bo->fdt_info.current_path);
>> + if (!dp) {
>> + ret = EFI_OUT_OF_RESOURCES;
>> + goto out;
>> + }
>> + fdt_dp = efi_dp_concat((const struct efi_device_path *)&fdt_prefix,
>> + dp);
>> + efi_free_pool(dp);
>> + }
>> +
>> dp = eficonfig_create_device_path(bo->file_info.dp_volume, bo->file_info.current_path);
>> if (!dp) {
>> ret = EFI_OUT_OF_RESOURCES;
>> @@ -1505,6 +1553,13 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
>> goto out;
>> }
>> }
>> + if (fdt_dp) {
>> + final_dp = efi_dp_merge(final_dp, &final_dp_size, fdt_dp);
>> + if (!final_dp) {
>> + ret = EFI_OUT_OF_RESOURCES;
>> + goto out;
>> + }
>> + }
>>
>> if (utf16_utf8_strlen(bo->optional_data)) {
>> len = utf16_utf8_strlen(bo->optional_data) + 1;
>> @@ -1522,9 +1577,12 @@ out:
>> free(bo->description);
>> free(bo->file_info.current_path);
>> free(bo->initrd_info.current_path);
>> + free(bo->fdt_info.current_path);
>> efi_free_pool(device_dp);
>> efi_free_pool(initrd_device_dp);
>> efi_free_pool(initrd_dp);
>> + efi_free_pool(fdt_device_dp);
>> + efi_free_pool(fdt_dp);
>> efi_free_pool(final_dp);
>>
>> return ret;
>> --
>> 2.43.0
>>
>
> Would it make sense to skip showing the user partitions that are not
> valid (or why does the Linux Swap partition not show here but the
> Linux partition with ext4 does? Neither is valid for selecting Fdt
> File ?) and/or extend eficonfig for user-entered data? For example if
> I was very sure I wanted U-Boot to search a location but I just didn't
> have the SD Card that configuration is meant for currently inserted, I
> must use efidebug to configure this?
The eficonfig command shows the partitions with a file-system that
U-Boot supports. What problems do you see in being able to specify that
a device-tree file shall be loaded from an ext4 file-system?
>
> It was always confusing how Edit action hides from view an automatic
> (global?) boot option i.e. 'mmc 0' that is configurable from Change
> Boot Order. I guess that there would have just been File
> EFI\BOOT\BOOTRISCV64.EFI to show and that is not interesting enough
> information for the added complexity of a save-disabled Edit entry.
> However now or in future there will be useful information to display
> so this should become viewable as a save-disabled entry from Edit
> (rename? View/Edit) action, where it is convenient to see File
> constant EFI\BOOT\BOOTRISCV64.EFI and also the detected values that
This file name is not stored in the Boot#### variable. It would be
confusing to show a file name here.
> will be used i.e. from parsing U-Boot variable $fdtfile. I do not mean
> that this should be a method for editing U-Boot environment variables,
> only that it would be useful to know for example when
> $fdtfile=vendor/boardname.dtb that the U-Boot EFI code heuristic has
> decided that this currently resolves to mmc
> 0:1/dtb\vendor\boardname.dtb value. If the automatic values are not
> what the user wants then they may create a new boot entry or leave the
> context of eficonfig to update something that affects the detection
> logic such as U-Boot env variables, and perhaps again enter eficonfig
> to confirm that the heuristic is now doing what they would like.
The eficonfig command is used to edit the EFI variables Boot#### and
BootOrder. This is the content shown by the command.
With patch 14 a logic is implemented to read a device-tree file supplied
on the boot partition.
Which file will be loaded is not known when the boot option is edited as
multiple directories are scanned and the value of $fdtfile may be
changed by the user before invoking the bootmgr. Scanning which such
file exists while running the editor would unnecessarily complicate the
code.
For sure the fall-back logic should be documented.
Best regards
Heinrich
> Without any visibility of this global boot option from the Edit
> action, it is not known to need to edit the boot order after adding a
> boot entry where before there was apparently none. Some of that could
> be addressed with documentation but the best result would be it being
> obvious through use. It's not really obvious now that something named
> 'mmc 0' only appearing in the Boot Order action of eficonfig what
> filename it requires or if it is going to use an Initrd and/or Fdt.
>
> Aside these more broad usability concerns there was success in adding
> a boot item with Fdt setting and changing the order that it be
> preferred entry.
>
> Tested-by: E Shattow <lucent at gmail.com>
More information about the U-Boot
mailing list