[RFC 04/14] cmd: eficonfig: add support for setting fdt
E Shattow
lucent at gmail.com
Sun Apr 28 06:13:20 CEST 2024
On Sat, Apr 27, 2024 at 2:25 PM Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> 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?
If selections other than EFI System Partition may be valid for File,
Initrd File, and Fdt File here then it would be useful yes. As tested
there are no files or directories are displayed in eficonfig when
trying this Linux ext4 partition. Not any issue with this patch, then.
>
> >
> > 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.
Okay, outside of the scope of this patch then. Moving that to its own
discussion. Thanks.
>
> > 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.
>
Ack.
> 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