From 5a0970ee8cf4391835d21c59e44a65d54d42377c Mon Sep 17 00:00:00 2001 From: Masahisa Kojima Date: Mon, 18 Dec 2023 18:57:41 +0900 Subject: [PATCH 1/9] efi_loader: fix iteration of FMP protocols If one of the FMP protocols fails when calling GetImageInfo(), populating the ESRT ends up with failure and other FMP protocols are not added to the ESRT. We should still add all other FMP protocols to the ESRT. With this commit, iteration of all FMP protocols continues even though one of the FMP protocols fails. Signed-off-by: Masahisa Kojima Reviewed-by: Ilias Apalodimas Tested-by: Ilias Apalodimas --- lib/efi_loader/efi_esrt.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/lib/efi_loader/efi_esrt.c b/lib/efi_loader/efi_esrt.c index dafd447b6d7..443bd999ce1 100644 --- a/lib/efi_loader/efi_esrt.c +++ b/lib/efi_loader/efi_esrt.c @@ -364,7 +364,7 @@ efi_status_t efi_esrt_populate(void) if (ret != EFI_SUCCESS) { EFI_PRINT("ESRT Unable to find FMP handle (%u)\n", idx); - goto out; + continue; } fmp = handler->protocol_interface; @@ -379,15 +379,14 @@ efi_status_t efi_esrt_populate(void) * fmp->get_image_info to return BUFFER_TO_SMALL. */ EFI_PRINT("ESRT erroneous FMP implementation\n"); - ret = EFI_INVALID_PARAMETER; - goto out; + continue; } ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, info_size, (void **)&img_info); if (ret != EFI_SUCCESS) { EFI_PRINT("ESRT failed to allocate memory for image info\n"); - goto out; + continue; } /* @@ -405,7 +404,7 @@ efi_status_t efi_esrt_populate(void) if (ret != EFI_SUCCESS) { EFI_PRINT("ESRT failed to obtain image info from FMP\n"); efi_free_pool(img_info); - goto out; + continue; } num_entries += desc_count; @@ -413,6 +412,13 @@ efi_status_t efi_esrt_populate(void) efi_free_pool(img_info); } + /* error occurs in fmp->get_image_info() if num_entries is 0 here */ + if (!num_entries) { + EFI_PRINT("Error occurs, num_entries should not be 0\n"); + ret = EFI_INVALID_PARAMETER; + goto out; + } + EFI_PRINT("ESRT create table with %u entries\n", num_entries); /* * Allocate an ESRT with the sufficient number of entries to accommodate @@ -436,7 +442,7 @@ efi_status_t efi_esrt_populate(void) if (ret != EFI_SUCCESS) { EFI_PRINT("ESRT unable to find FMP handle (%u)\n", idx); - break; + continue; } fmp = handler->protocol_interface; From 50662260c41704d01a83f3d978a18d70eb799089 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Tue, 19 Dec 2023 07:27:19 -0700 Subject: [PATCH 2/9] efi: Avoid using dm_scan_other() This function is defined by bootstd so using it precludes using that feature. Use the board_early_init_r() feature instead. Nove the affected code into a new file, so that it is clear that this relates to board init. Signed-off-by: Simon Glass Reviewed-by: Heinrich Schuchardt --- configs/efi-x86_app64_defconfig | 1 + lib/efi/Makefile | 2 +- lib/efi/efi_app.c | 187 ----------------------------- lib/efi/efi_app_init.c | 205 ++++++++++++++++++++++++++++++++ 4 files changed, 207 insertions(+), 188 deletions(-) create mode 100644 lib/efi/efi_app_init.c diff --git a/configs/efi-x86_app64_defconfig b/configs/efi-x86_app64_defconfig index ceaebc7de44..3d021483dd0 100644 --- a/configs/efi-x86_app64_defconfig +++ b/configs/efi-x86_app64_defconfig @@ -18,6 +18,7 @@ CONFIG_BOOTCOMMAND="ext2load scsi 0:3 01000000 /boot/vmlinuz; zboot 01000000" CONFIG_SYS_PBSIZE=532 CONFIG_SYS_CONSOLE_INFO_QUIET=y CONFIG_DISPLAY_BOARDINFO_LATE=y +CONFIG_BOARD_EARLY_INIT_R=y CONFIG_HUSH_PARSER=y CONFIG_CMD_BOOTZ=y CONFIG_CMD_PART=y diff --git a/lib/efi/Makefile b/lib/efi/Makefile index a790d2d554c..232fa684360 100644 --- a/lib/efi/Makefile +++ b/lib/efi/Makefile @@ -2,7 +2,7 @@ # # (C) Copyright 2015 Google, Inc -obj-$(CONFIG_EFI_APP) += efi_app.o efi.o +obj-$(CONFIG_EFI_APP) += efi_app.o efi.o efi_app_init.o obj-$(CONFIG_EFI_STUB) += efi_info.o CFLAGS_REMOVE_efi_stub.o := -mregparm=3 \ diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c index 119db6602cf..88332c3c910 100644 --- a/lib/efi/efi_app.c +++ b/lib/efi/efi_app.c @@ -67,49 +67,6 @@ int efi_get_mmap(struct efi_mem_desc **descp, int *sizep, uint *keyp, return 0; } -/** - * efi_bind_block() - bind a new block device to an EFI device - * - * Binds a new top-level EFI_MEDIA device as well as a child block device so - * that the block device can be accessed in U-Boot. - * - * The device can then be accessed using 'part list efi 0', 'fat ls efi 0:1', - * for example, just like any other interface type. - * - * @handle: handle of the controller on which this driver is installed - * @blkio: block io protocol proxied by this driver - * @device_path: EFI device path structure for this - * @len: Length of @device_path in bytes - * @devp: Returns the bound device - * Return: 0 if OK, -ve on error - */ -int efi_bind_block(efi_handle_t handle, struct efi_block_io *blkio, - struct efi_device_path *device_path, int len, - struct udevice **devp) -{ - struct efi_media_plat plat; - struct udevice *dev; - char name[18]; - int ret; - - plat.handle = handle; - plat.blkio = blkio; - plat.device_path = malloc(device_path->length); - if (!plat.device_path) - return log_msg_ret("path", -ENOMEM); - memcpy(plat.device_path, device_path, device_path->length); - ret = device_bind(dm_root(), DM_DRIVER_GET(efi_media), "efi_media", - &plat, ofnode_null(), &dev); - if (ret) - return log_msg_ret("bind", ret); - - snprintf(name, sizeof(name), "efi_media_%x", dev_seq(dev)); - device_set_name(dev, name); - *devp = dev; - - return 0; -} - static efi_status_t setup_memory(struct efi_priv *priv) { struct efi_boot_services *boot = priv->boot; @@ -178,150 +135,6 @@ static void free_memory(struct efi_priv *priv) global_data_ptr = NULL; } -/** - * devpath_is_partition() - Figure out if a device path is a partition - * - * Checks if a device path refers to a partition on some media device. This - * works by checking for a valid partition number in a hard-driver media device - * as the final component of the device path. - * - * @path: device path - * Return: true if a partition, false if not - * (e.g. it might be media which contains partitions) - */ -static bool devpath_is_partition(const struct efi_device_path *path) -{ - const struct efi_device_path *p; - bool was_part = false; - - for (p = path; p->type != DEVICE_PATH_TYPE_END; - p = (void *)p + p->length) { - was_part = false; - if (p->type == DEVICE_PATH_TYPE_MEDIA_DEVICE && - p->sub_type == DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH) { - struct efi_device_path_hard_drive_path *hd = - (void *)path; - - if (hd->partition_number) - was_part = true; - } - } - - return was_part; -} - -/** - * setup_block() - Find all block devices and setup EFI devices for them - * - * Partitions are ignored, since U-Boot has partition handling. Errors with - * particular devices produce a warning but execution continues to try to - * find others. - * - * Return: 0 if found, -ENOSYS if there is no boot-services table, -ENOTSUPP - * if a required protocol is not supported - */ -static int setup_block(void) -{ - efi_guid_t efi_blkio_guid = EFI_BLOCK_IO_PROTOCOL_GUID; - efi_guid_t efi_devpath_guid = EFI_DEVICE_PATH_PROTOCOL_GUID; - efi_guid_t efi_pathutil_guid = EFI_DEVICE_PATH_UTILITIES_PROTOCOL_GUID; - efi_guid_t efi_pathtext_guid = EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID; - struct efi_boot_services *boot = efi_get_boot(); - struct efi_device_path_utilities_protocol *util; - struct efi_device_path_to_text_protocol *text; - struct efi_device_path *path; - struct efi_block_io *blkio; - efi_uintn_t num_handles; - efi_handle_t *handle; - int ret, i; - - if (!boot) - return log_msg_ret("sys", -ENOSYS); - - /* Find all devices which support the block I/O protocol */ - ret = boot->locate_handle_buffer(BY_PROTOCOL, &efi_blkio_guid, NULL, - &num_handles, &handle); - if (ret) - return log_msg_ret("loc", -ENOTSUPP); - log_debug("Found %d handles:\n", (int)num_handles); - - /* We need to look up the path size and convert it to text */ - ret = boot->locate_protocol(&efi_pathutil_guid, NULL, (void **)&util); - if (ret) - return log_msg_ret("util", -ENOTSUPP); - ret = boot->locate_protocol(&efi_pathtext_guid, NULL, (void **)&text); - if (ret) - return log_msg_ret("text", -ENOTSUPP); - - for (i = 0; i < num_handles; i++) { - struct udevice *dev; - const u16 *name; - bool is_part; - int len; - - ret = boot->handle_protocol(handle[i], &efi_devpath_guid, - (void **)&path); - if (ret) { - log_warning("- devpath %d failed (ret=%d)\n", i, ret); - continue; - } - - ret = boot->handle_protocol(handle[i], &efi_blkio_guid, - (void **)&blkio); - if (ret) { - log_warning("- blkio %d failed (ret=%d)\n", i, ret); - continue; - } - - name = text->convert_device_path_to_text(path, true, false); - is_part = devpath_is_partition(path); - - if (!is_part) { - len = util->get_device_path_size(path); - ret = efi_bind_block(handle[i], blkio, path, len, &dev); - if (ret) { - log_warning("- blkio bind %d failed (ret=%d)\n", - i, ret); - continue; - } - } else { - dev = NULL; - } - - /* - * Show the device name if we created one. Otherwise indicate - * that it is a partition. - */ - printf("%2d: %-12s %ls\n", i, dev ? dev->name : "", - name); - } - boot->free_pool(handle); - - return 0; -} - -/** - * dm_scan_other() - Scan for UEFI devices that should be available to U-Boot - * - * This sets up block devices within U-Boot for those found in UEFI. With this, - * U-Boot can access those devices - * - * @pre_reloc_only: true to only bind pre-relocation devices (ignored) - * Returns: 0 on success, -ve on error - */ -int dm_scan_other(bool pre_reloc_only) -{ - if (gd->flags & GD_FLG_RELOC) { - int ret; - - ret = setup_block(); - if (ret) - return ret; - } - - return 0; -} - static void scan_tables(struct efi_system_table *sys_table) { efi_guid_t acpi = EFI_ACPI_TABLE_GUID; diff --git a/lib/efi/efi_app_init.c b/lib/efi/efi_app_init.c new file mode 100644 index 00000000000..c5e4192fe06 --- /dev/null +++ b/lib/efi/efi_app_init.c @@ -0,0 +1,205 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * EFI-app board implementation + * + * Copyright 2023 Google LLC + * Written by Simon Glass + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +DECLARE_GLOBAL_DATA_PTR; + +/** + * efi_bind_block() - bind a new block device to an EFI device + * + * Binds a new top-level EFI_MEDIA device as well as a child block device so + * that the block device can be accessed in U-Boot. + * + * The device can then be accessed using 'part list efi 0', 'fat ls efi 0:1', + * for example, just like any other interface type. + * + * @handle: handle of the controller on which this driver is installed + * @blkio: block io protocol proxied by this driver + * @device_path: EFI device path structure for this + * @len: Length of @device_path in bytes + * @devp: Returns the bound device + * Return: 0 if OK, -ve on error + */ +int efi_bind_block(efi_handle_t handle, struct efi_block_io *blkio, + struct efi_device_path *device_path, int len, + struct udevice **devp) +{ + struct efi_media_plat plat; + struct udevice *dev; + char name[18]; + int ret; + + plat.handle = handle; + plat.blkio = blkio; + plat.device_path = malloc(device_path->length); + if (!plat.device_path) + return log_msg_ret("path", -ENOMEM); + memcpy(plat.device_path, device_path, device_path->length); + ret = device_bind(dm_root(), DM_DRIVER_GET(efi_media), "efi_media", + &plat, ofnode_null(), &dev); + if (ret) + return log_msg_ret("bind", ret); + + snprintf(name, sizeof(name), "efi_media_%x", dev_seq(dev)); + device_set_name(dev, name); + *devp = dev; + + return 0; +} + +/** + * devpath_is_partition() - Figure out if a device path is a partition + * + * Checks if a device path refers to a partition on some media device. This + * works by checking for a valid partition number in a hard-driver media device + * as the final component of the device path. + * + * @path: device path + * Return: true if a partition, false if not + * (e.g. it might be media which contains partitions) + */ +static bool devpath_is_partition(const struct efi_device_path *path) +{ + const struct efi_device_path *p; + bool was_part = false; + + for (p = path; p->type != DEVICE_PATH_TYPE_END; + p = (void *)p + p->length) { + was_part = false; + if (p->type == DEVICE_PATH_TYPE_MEDIA_DEVICE && + p->sub_type == DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH) { + struct efi_device_path_hard_drive_path *hd = + (void *)path; + + if (hd->partition_number) + was_part = true; + } + } + + return was_part; +} + +/** + * setup_block() - Find all block devices and setup EFI devices for them + * + * Partitions are ignored, since U-Boot has partition handling. Errors with + * particular devices produce a warning but execution continues to try to + * find others. + * + * Return: 0 if found, -ENOSYS if there is no boot-services table, -ENOTSUPP + * if a required protocol is not supported + */ +static int setup_block(void) +{ + efi_guid_t efi_blkio_guid = EFI_BLOCK_IO_PROTOCOL_GUID; + efi_guid_t efi_devpath_guid = EFI_DEVICE_PATH_PROTOCOL_GUID; + efi_guid_t efi_pathutil_guid = EFI_DEVICE_PATH_UTILITIES_PROTOCOL_GUID; + efi_guid_t efi_pathtext_guid = EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID; + struct efi_boot_services *boot = efi_get_boot(); + struct efi_device_path_utilities_protocol *util; + struct efi_device_path_to_text_protocol *text; + struct efi_device_path *path; + struct efi_block_io *blkio; + efi_uintn_t num_handles; + efi_handle_t *handle; + int ret, i; + + if (!boot) + return log_msg_ret("sys", -ENOSYS); + + /* Find all devices which support the block I/O protocol */ + ret = boot->locate_handle_buffer(BY_PROTOCOL, &efi_blkio_guid, NULL, + &num_handles, &handle); + if (ret) + return log_msg_ret("loc", -ENOTSUPP); + log_debug("Found %d handles:\n", (int)num_handles); + + /* We need to look up the path size and convert it to text */ + ret = boot->locate_protocol(&efi_pathutil_guid, NULL, (void **)&util); + if (ret) + return log_msg_ret("util", -ENOTSUPP); + ret = boot->locate_protocol(&efi_pathtext_guid, NULL, (void **)&text); + if (ret) + return log_msg_ret("text", -ENOTSUPP); + + for (i = 0; i < num_handles; i++) { + struct udevice *dev; + const u16 *name; + bool is_part; + int len; + + ret = boot->handle_protocol(handle[i], &efi_devpath_guid, + (void **)&path); + if (ret) { + log_warning("- devpath %d failed (ret=%d)\n", i, ret); + continue; + } + + ret = boot->handle_protocol(handle[i], &efi_blkio_guid, + (void **)&blkio); + if (ret) { + log_warning("- blkio %d failed (ret=%d)\n", i, ret); + continue; + } + + name = text->convert_device_path_to_text(path, true, false); + is_part = devpath_is_partition(path); + + if (!is_part) { + len = util->get_device_path_size(path); + ret = efi_bind_block(handle[i], blkio, path, len, &dev); + if (ret) { + log_warning("- blkio bind %d failed (ret=%d)\n", + i, ret); + continue; + } + } else { + dev = NULL; + } + + /* + * Show the device name if we created one. Otherwise indicate + * that it is a partition. + */ + printf("%2d: %-12s %ls\n", i, dev ? dev->name : "", + name); + } + boot->free_pool(handle); + + return 0; +} + +/** + * board_early_init_r() - Scan for UEFI devices that should be available + * + * This sets up block devices within U-Boot for those found in UEFI. With this, + * U-Boot can access those devices + * + * Returns: 0 on success, -ve on error + */ +int board_early_init_r(void) +{ + if (gd->flags & GD_FLG_RELOC) { + int ret; + + ret = setup_block(); + if (ret) + return ret; + } + + return 0; +} From 6caf3a3e89649052cab41bd1578452368a4a57a0 Mon Sep 17 00:00:00 2001 From: Masahisa Kojima Date: Mon, 25 Dec 2023 13:43:54 +0900 Subject: [PATCH 3/9] efi_loader: remove unused members from struct efi_disk_obj part and dev_index in struct efi_disk_obj are not used, let's remove it. This commit also removes the invalid structure comment for @dev, it does not exist. Signed-off-by: Masahisa Kojima Reviewed-by: Heinrich Schuchardt --- lib/efi_loader/efi_disk.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index ed997008c4e..013842f0776 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -31,20 +31,15 @@ const efi_guid_t efi_system_partition_guid = PARTITION_SYSTEM_GUID; * * @header: EFI object header * @ops: EFI disk I/O protocol interface - * @dev_index: device index of block device * @media: block I/O media information * @dp: device path to the block device - * @part: partition * @volume: simple file system protocol of the partition - * @dev: associated DM device */ struct efi_disk_obj { struct efi_object header; struct efi_block_io ops; - int dev_index; struct efi_block_io_media media; struct efi_device_path *dp; - unsigned int part; struct efi_simple_file_system_protocol *volume; }; @@ -382,7 +377,6 @@ static int efi_fs_exists(struct blk_desc *desc, int part) * @parent: parent handle * @dp_parent: parent device path * @desc: internal block device - * @dev_index: device index for block device * @part_info: partition info * @part: partition * @disk: pointer to receive the created handle @@ -393,7 +387,6 @@ static efi_status_t efi_disk_add_dev( efi_handle_t parent, struct efi_device_path *dp_parent, struct blk_desc *desc, - int dev_index, struct disk_partition *part_info, unsigned int part, struct efi_disk_obj **disk, @@ -455,7 +448,6 @@ static efi_status_t efi_disk_add_dev( diskobj->dp = efi_dp_from_part(desc, part); diskobj->media.last_block = desc->lba - 1; } - diskobj->part = part; /* * Install the device path and the block IO protocol. @@ -498,7 +490,6 @@ static efi_status_t efi_disk_add_dev( goto error; } diskobj->ops = block_io_disk_template; - diskobj->dev_index = dev_index; /* Fill in EFI IO Media info (for read/write callbacks) */ diskobj->media.removable_media = desc->removable; @@ -518,7 +509,7 @@ static efi_status_t efi_disk_add_dev( EFI_PRINT("BlockIO: part %u, present %d, logical %d, removable %d" ", last_block %llu\n", - diskobj->part, + part, diskobj->media.media_present, diskobj->media.logical_partition, diskobj->media.removable_media, @@ -565,7 +556,7 @@ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle) diskid = desc->devnum; ret = efi_disk_add_dev(NULL, NULL, desc, - diskid, NULL, 0, &disk, agent_handle); + NULL, 0, &disk, agent_handle); if (ret != EFI_SUCCESS) { if (ret == EFI_NOT_READY) { log_notice("Disk %s not ready\n", dev->name); @@ -626,7 +617,7 @@ static int efi_disk_create_part(struct udevice *dev, efi_handle_t agent_handle) return -1; dp_parent = (struct efi_device_path *)handler->protocol_interface; - ret = efi_disk_add_dev(parent, dp_parent, desc, diskid, + ret = efi_disk_add_dev(parent, dp_parent, desc, info, part, &disk, agent_handle); if (ret != EFI_SUCCESS) { log_err("Adding partition for %s failed\n", dev->name); From 86efd049344fe160185654548fa9ec1301d78758 Mon Sep 17 00:00:00 2001 From: Bin Meng Date: Fri, 5 Jan 2024 07:25:42 +0800 Subject: [PATCH 4/9] doc: coreboot: Update stale graphics enabling instructions With latest coreboot (e.g.: v4.22.01), the instructions to enable graphics support has changed. Refresh the doc. Signed-off-by: Bin Meng --- doc/board/coreboot/coreboot.rst | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/doc/board/coreboot/coreboot.rst b/doc/board/coreboot/coreboot.rst index 10a251c2b64..7154f59c374 100644 --- a/doc/board/coreboot/coreboot.rst +++ b/doc/board/coreboot/coreboot.rst @@ -32,10 +32,9 @@ of _x86boot_start (in arch/x86/cpu/start.S). If you want to use ELF as the coreboot payload, change U-Boot configuration to use CONFIG_OF_EMBED instead of CONFIG_OF_SEPARATE. -To enable video you must enable these options in coreboot: +To enable video you must enable CONFIG_GENERIC_LINEAR_FRAMEBUFFER in coreboot: - - Set framebuffer graphics resolution (1280x1024 32k-color (1:5:5)) - - Keep VESA framebuffer + - Devices->Display->Framebuffer mode->Linear "high resolution" framebuffer At present it seems that for Minnowboard Max, coreboot does not pass through the video information correctly (it always says the resolution is 0x0). This @@ -184,13 +183,13 @@ boot as a coreboot payload, based on a known-good build of coreboot. To update the `coreboot.rom` file which is used: -#. Build coreboot with `CONFIG_LINEAR_FRAMEBUFFER=y`. If using `make menuconfig` - this is under - `Devices ->Display->Framebuffer mode->Linear "high resolution" framebuffer`. +#. Build coreboot with `CONFIG_GENERIC_LINEAR_FRAMEBUFFER=y`. If using + `make menuconfig`, this is under + `Devices->Display->Framebuffer mode->Linear "high resolution" framebuffer`. #. Compress the resulting `coreboot.rom`:: - xz -c /path/to/coreboot/build/coreboot.rom >coreboot.rom.xz + xz -c /path/to/coreboot/build/coreboot.rom > coreboot.rom.xz #. Upload the file to Google drive From 753f76e417a91c19dd32571bf5398c5526fb21a5 Mon Sep 17 00:00:00 2001 From: Grzegorz Szymaszek Date: Sat, 6 Jan 2024 11:36:54 +0100 Subject: [PATCH 5/9] doc: fix three-dash references in reStructuredText The "Sending patches" and "Patman patch manager" documents refer to the three-dash, "---", that separates patch parts. The symbol is written in the documentation in the same form as it is in actual patches: as three U+002D HYPHEN-MINUS characters. When building the documentation, Sphinx converts this symbol to a U+2014 EM DASH, that is invalid in this case. Make the three-dash symbol a reStructuredText inline literal to prevent its conversion; that is, enclose it in a pair of double backquotes (`). Signed-off-by: Grzegorz Szymaszek Reviewed-by: Mattijs Korpershoek --- doc/develop/sending_patches.rst | 4 ++-- tools/patman/patman.rst | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/develop/sending_patches.rst b/doc/develop/sending_patches.rst index 5a6962f1021..3f25b1d0466 100644 --- a/doc/develop/sending_patches.rst +++ b/doc/develop/sending_patches.rst @@ -121,7 +121,7 @@ General Patch Submission Rules * For new features: a description of the feature and your implementation. * Additional comments which you don't want included in U-Boot's history can be - included below the first "---" in the message body. + included below the first "``---``" in the message body. * If your description gets too long, that's a strong indication that you should split up your patch. @@ -253,7 +253,7 @@ to observe the following rules. * Please make sure to keep a "change log", i.e. a description of what you have changed compared to previous versions of this patch. This change log should - be added below the "---" line in the patch, which starts the "comment + be added below the "``---``" line in the patch, which starts the "comment section", i.e. which contains text that does not get included into the actual commit message. Note: it is *not* sufficient to provide a change log in some cover letter diff --git a/tools/patman/patman.rst b/tools/patman/patman.rst index a8b317eed63..e01355824c5 100644 --- a/tools/patman/patman.rst +++ b/tools/patman/patman.rst @@ -280,7 +280,7 @@ Series-notes: Commit-notes: Similar, but for a single commit (patch). These notes will appear - immediately below the --- cut in the patch file:: + immediately below the ``---`` cut in the patch file:: Commit-notes: blah blah From f19171c919e03c2590dce2f5026de2bf43203f7c Mon Sep 17 00:00:00 2001 From: Ilias Apalodimas Date: Mon, 8 Jan 2024 10:55:33 +0200 Subject: [PATCH 6/9] efi_loader: Clean up efi_dp_append and efi_dp_concat 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. Signed-off-by: Ilias Apalodimas --- cmd/eficonfig.c | 7 ++- cmd/efidebug.c | 6 +- include/efi_loader.h | 5 +- lib/efi_loader/efi_bootmgr.c | 6 +- lib/efi_loader/efi_boottime.c | 2 +- lib/efi_loader/efi_device_path.c | 70 +++++++--------------- lib/efi_loader/efi_device_path_utilities.c | 2 +- 7 files changed, 35 insertions(+), 63 deletions(-) diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 34a59cb15d0..8234e602b8f 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -531,7 +531,7 @@ struct efi_device_path *eficonfig_create_device_path(struct efi_device_path *dp_ dp = efi_dp_shorten(dp_volume); if (!dp) dp = dp_volume; - dp = efi_dp_append(dp, &fp->dp); + dp = efi_dp_concat(dp, &fp->dp, false); free(buf); return dp; @@ -1484,7 +1484,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); } @@ -1495,7 +1496,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 e10fbf891a4..836014ab8ea 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -699,8 +699,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); @@ -916,7 +916,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 34e7fbbf184..d3725041b20 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -819,8 +819,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 */ @@ -937,7 +935,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 a032d3ae04e..dd2dd23c7b5 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -143,7 +143,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; @@ -1579,8 +1579,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/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index c579d89211b..1951291747c 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1816,7 +1816,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 8dbd8105ae2..500fa10d405 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -271,30 +271,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 (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); + size_t end_size; - if (concat) - end_size = 2 * sizeof(END); if (!dp1 && !dp2) { /* return an end node */ ret = efi_dp_dup(&END); @@ -306,14 +303,20 @@ 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); + else + end_size = sizeof(END); + 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); } @@ -327,37 +330,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 844d8acd67c..c95dbfa9b5f 100644 --- a/lib/efi_loader/efi_device_path_utilities.c +++ b/lib/efi_loader/efi_device_path_utilities.c @@ -76,7 +76,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)); } /* From af7a34acfd88815ead1882eb8b05ef088d7ca738 Mon Sep 17 00:00:00 2001 From: Masahisa Kojima Date: Thu, 11 Jan 2024 14:35:39 +0900 Subject: [PATCH 7/9] fwu: fix fwu_get_image_index interface The capsule update uses the DFU framework for updating storage. fwu_get_image_index() currently returns the image_index calculated by (dfu_alt_num + 1), but this is different from the image_index in UEFI terminology. Since capsule update implementation calls dfu_write_by_alt function, it is better that FWU returns the dfu_alt_num. Signed-off-by: Masahisa Kojima Reviewed-by: Ilias Apalodimas --- include/fwu.h | 13 +++++-------- lib/efi_loader/efi_firmware.c | 11 +++++++++-- lib/fwu_updates/fwu.c | 32 ++++++++++++-------------------- 3 files changed, 26 insertions(+), 30 deletions(-) diff --git a/include/fwu.h b/include/fwu.h index ac5c5de8706..eb5638f4f3a 100644 --- a/include/fwu.h +++ b/include/fwu.h @@ -122,21 +122,18 @@ int fwu_get_active_index(uint *active_idxp); int fwu_set_active_index(uint active_idx); /** - * fwu_get_image_index() - Get the Image Index to be used for capsule update - * @image_index: The Image Index for the image - * - * The FWU multi bank update feature computes the value of image_index at - * runtime, based on the bank to which the image needs to be written to. - * Derive the image_index value for the image. + * fwu_get_dfu_alt_num() - Get the dfu_alt_num to be used for capsule update + * @image_index: The Image Index for the image + * @alt_num: pointer to store dfu_alt_num * * Currently, the capsule update driver uses the DFU framework for * the updates. This function gets the DFU alt number which is to - * be used as the Image Index + * be used for capsule update. * * Return: 0 if OK, -ve on error * */ -int fwu_get_image_index(u8 *image_index); +int fwu_get_dfu_alt_num(u8 image_index, u8 *alt_num); /** * fwu_revert_boot_index() - Revert the active index in the FWU metadata diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index 1fde1885e3c..e558336bc10 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -610,6 +610,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image( u16 **abort_reason) { int ret; + u8 dfu_alt_num; efi_status_t status; struct fmp_state state = { 0 }; @@ -624,19 +625,25 @@ efi_status_t EFIAPI efi_firmware_raw_set_image( if (status != EFI_SUCCESS) return EFI_EXIT(status); + /* + * dfu_alt_num is assigned from 0 while image_index starts from 1. + * dfu_alt_num is calculated by (image_index - 1) when multi bank update + * is not used. + */ + dfu_alt_num = image_index - 1; if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { /* * Based on the value of update bank, derive the * image index value. */ - ret = fwu_get_image_index(&image_index); + ret = fwu_get_dfu_alt_num(image_index, &dfu_alt_num); if (ret) { log_debug("Unable to get FWU image_index\n"); return EFI_EXIT(EFI_DEVICE_ERROR); } } - if (dfu_write_by_alt(image_index - 1, (void *)image, image_size, + if (dfu_write_by_alt(dfu_alt_num, (void *)image, image_size, NULL, NULL)) return EFI_EXIT(EFI_DEVICE_ERROR); diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c index b5805740153..86518108c2d 100644 --- a/lib/fwu_updates/fwu.c +++ b/lib/fwu_updates/fwu.c @@ -125,16 +125,14 @@ static int in_trial_state(struct fwu_mdata *mdata) return 0; } -static int fwu_get_image_type_id(u8 *image_index, efi_guid_t *image_type_id) +static int fwu_get_image_type_id(u8 image_index, efi_guid_t *image_type_id) { - u8 index; int i; struct efi_fw_image *image; - index = *image_index; image = update_info.images; for (i = 0; i < update_info.num_images; i++) { - if (index == image[i].image_index) { + if (image_index == image[i].image_index) { guidcpy(image_type_id, &image[i].image_type_id); return 0; } @@ -332,24 +330,20 @@ int fwu_set_active_index(uint active_idx) } /** - * fwu_get_image_index() - Get the Image Index to be used for capsule update - * @image_index: The Image Index for the image - * - * The FWU multi bank update feature computes the value of image_index at - * runtime, based on the bank to which the image needs to be written to. - * Derive the image_index value for the image. + * fwu_get_dfu_alt_num() - Get the dfu_alt_num to be used for capsule update + * @image_index: The Image Index for the image + * @alt_num: pointer to store dfu_alt_num * * Currently, the capsule update driver uses the DFU framework for * the updates. This function gets the DFU alt number which is to - * be used as the Image Index + * be used for capsule update. * * Return: 0 if OK, -ve on error * */ -int fwu_get_image_index(u8 *image_index) +int fwu_get_dfu_alt_num(u8 image_index, u8 *alt_num) { int ret, i; - u8 alt_num; uint update_bank; efi_guid_t *image_guid, image_type_id; struct fwu_mdata *mdata = &g_mdata; @@ -365,7 +359,7 @@ int fwu_get_image_index(u8 *image_index) ret = fwu_get_image_type_id(image_index, &image_type_id); if (ret) { log_debug("Unable to get image_type_id for image_index %u\n", - *image_index); + image_index); goto out; } @@ -380,15 +374,13 @@ int fwu_get_image_index(u8 *image_index) img_entry = &mdata->img_entry[i]; img_bank_info = &img_entry->img_bank_info[update_bank]; image_guid = &img_bank_info->image_uuid; - ret = fwu_plat_get_alt_num(g_dev, image_guid, &alt_num); - if (ret) { + ret = fwu_plat_get_alt_num(g_dev, image_guid, alt_num); + if (ret) log_debug("alt_num not found for partition with GUID %pUs\n", image_guid); - } else { + else log_debug("alt_num %d for partition %pUs\n", - alt_num, image_guid); - *image_index = alt_num + 1; - } + *alt_num, image_guid); goto out; } From b20c9c3d8681547f7c839bb4f6fcd2f31e2b8585 Mon Sep 17 00:00:00 2001 From: Masahisa Kojima Date: Thu, 11 Jan 2024 14:35:40 +0900 Subject: [PATCH 8/9] efi_loader: support fmp versioning for multi bank update This commit stores the firmware version into the array of fmp_state structure to support the fmp versioning for multi bank update. The index of the array is identified by the bank index. This modification keeps the backward compatibility with the existing versioning feature. Signed-off-by: Masahisa Kojima Reviewed-by: Ilias Apalodimas --- lib/efi_loader/efi_firmware.c | 75 ++++++++++++++++++++++++++++------- 1 file changed, 60 insertions(+), 15 deletions(-) diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index e558336bc10..9fd13297a60 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -206,18 +206,10 @@ void efi_firmware_fill_version_info(struct efi_firmware_image_descriptor *image_ { u16 varname[13]; /* u"FmpStateXXXX" */ efi_status_t ret; - efi_uintn_t size; - struct fmp_state var_state = { 0 }; - - efi_create_indexed_name(varname, sizeof(varname), "FmpState", - fw_array->image_index); - size = sizeof(var_state); - ret = efi_get_variable_int(varname, &fw_array->image_type_id, - NULL, &size, &var_state, NULL); - if (ret == EFI_SUCCESS) - image_info->version = var_state.fw_version; - else - image_info->version = 0; + efi_uintn_t size, expected_size; + uint num_banks = 1; + uint active_index = 0; + struct fmp_state *var_state; efi_firmware_get_lsv_from_dtb(fw_array->image_index, &fw_array->image_type_id, @@ -226,6 +218,31 @@ void efi_firmware_fill_version_info(struct efi_firmware_image_descriptor *image_ image_info->version_name = NULL; /* not supported */ image_info->last_attempt_version = 0; image_info->last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; + image_info->version = 0; + + /* get the fw_version */ + efi_create_indexed_name(varname, sizeof(varname), "FmpState", + fw_array->image_index); + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { + ret = fwu_get_active_index(&active_index); + if (ret) + return; + + num_banks = CONFIG_FWU_NUM_BANKS; + } + + size = num_banks * sizeof(*var_state); + expected_size = size; + var_state = calloc(1, size); + if (!var_state) + return; + + ret = efi_get_variable_int(varname, &fw_array->image_type_id, + NULL, &size, var_state, NULL); + if (ret == EFI_SUCCESS && expected_size == size) + image_info->version = var_state[active_index].fw_version; + + free(var_state); } /** @@ -361,8 +378,11 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in { u16 varname[13]; /* u"FmpStateXXXX" */ efi_status_t ret; + uint num_banks = 1; + uint update_bank = 0; + efi_uintn_t size; efi_guid_t *image_type_id; - struct fmp_state var_state = { 0 }; + struct fmp_state *var_state; image_type_id = efi_firmware_get_image_type_id(image_index); if (!image_type_id) @@ -371,19 +391,44 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in efi_create_indexed_name(varname, sizeof(varname), "FmpState", image_index); + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { + ret = fwu_plat_get_update_index(&update_bank); + if (ret) + return EFI_INVALID_PARAMETER; + + num_banks = CONFIG_FWU_NUM_BANKS; + } + + size = num_banks * sizeof(*var_state); + var_state = calloc(1, size); + if (!var_state) + return EFI_OUT_OF_RESOURCES; + + /* + * GetVariable may fail, EFI_NOT_FOUND is returned if FmpState + * variable has not been set yet. + * Ignore the error here since the correct FmpState variable + * is set later. + */ + efi_get_variable_int(varname, image_type_id, NULL, &size, var_state, + NULL); + /* * Only the fw_version is set here. * lowest_supported_version in FmpState variable is ignored since * it can be tampered if the file based EFI variable storage is used. */ - var_state.fw_version = state->fw_version; + var_state[update_bank].fw_version = state->fw_version; + size = num_banks * sizeof(*var_state); ret = efi_set_variable_int(varname, image_type_id, EFI_VARIABLE_READ_ONLY | EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS, - sizeof(var_state), &var_state, false); + size, var_state, false); + + free(var_state); return ret; } From 55125bbfb48cb1330a692059bc972810f77b984a Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Thu, 11 Jan 2024 09:25:35 +0100 Subject: [PATCH 9/9] cmd: efidebug: check pointer after memory allocation After calling efi_alloc() we must check that the return value is not NULL. Fixes: f01c961ee34c ("cmd: efidebug: add uri device path") Addresses-Coverity-ID: 477185 Dereference null return value Signed-off-by: Heinrich Schuchardt Reviewed-by: Ilias Apalodimas --- cmd/efidebug.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cmd/efidebug.c b/cmd/efidebug.c index 836014ab8ea..aa8523797ef 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -754,6 +754,10 @@ static int efi_boot_add_uri(int argc, char *const argv[], u16 *var_name16, uridp_len = sizeof(struct efi_device_path) + strlen(argv[3]) + 1; uridp = efi_alloc(uridp_len + sizeof(END)); + if (!uridp) { + log_err("Out of memory\n"); + return CMD_RET_FAILURE; + } uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE; uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI; uridp->dp.length = uridp_len;