On 11/23/24 20:55, Matthew Garrett wrote:

From: Matthew Garrett mgarrett@aurora.tech

We want to jump into the EFI stub in the kernel so it can perform appropriate init and call ExitBootServices. Add support for doing that, including ensuring that we copy the kernel to somewhere that's not currently being used by the firmware.

Signed-off-by: Matthew Garrett mgarrett@aurora.tech

---

arch/x86/lib/bootm.c | 49 ++++++++++++++++++++++++++++++-------------- boot/bootm.c | 5 +++++ include/bootm.h | 6 ++++++ include/efi.h | 1 + lib/efi/efi.c | 1 + lib/efi/efi_app.c | 36 ++++++++++++++++++++++++++++++++ 6 files changed, 83 insertions(+), 15 deletions(-)

diff --git a/arch/x86/lib/bootm.c b/arch/x86/lib/bootm.c index 55f581836df..c4446b1f9c6 100644 --- a/arch/x86/lib/bootm.c +++ b/arch/x86/lib/bootm.c @@ -150,6 +150,38 @@ error: return 1; }

+typedef void(*handover_func)(void *, struct efi_system_table *sys_table, struct +boot_params *params);

+int efi_boot(ulong setup_base, ulong entry, bool image_64bit) +{

• struct boot_params *params = (struct boot_params *)setup_base;
• struct setup_header *hdr = &params->hdr;
• struct efi_priv *priv = efi_get_priv();
• handover_func hf;
• int offset = 0;
• if (IS_ENABLED(CONFIG_EFI_APP_64BIT)) {

• if (!image_64bit) {
  

• 	printf("## Can only boot 64-bit kernels\n");
  

• 	return 1;
  

• }
  

• offset = 512;
  

• } else if (image_64bit) {

• printf("# Can only boot 32-bit kernels\n");
  

• return 1;
  

• }
• hdr->code32_start = (int)entry;
• hdr->type_of_loader = 0x80; /* U-Boot, from Linux Documentation/x86/boot.rst */
• hf = (handover_func)(entry + hdr->handover_offset + offset);
• asm volatile ("cli");
• priv->loaded_image->image_base = (char *)entry;
• hf(priv->parent_image, priv->sys_table, params);
• return -EFAULT;

+}

• int boot_linux_kernel(ulong setup_base, ulong entry, bool image_64bit) { bootm_announce_and_cleanup();

@@ -158,21 +190,8 @@ int boot_linux_kernel(ulong setup_base, ulong entry, bool image_64bit) timestamp_add_now(TS_U_BOOT_START_KERNEL); #endif

• /*

• * Exit EFI boot services just before jumping, after all console
  

• * output, since the console won't be available afterwards.
  

• */
  

• if (IS_ENABLED(CONFIG_EFI_APP)) {

• int ret;
  

• ret = efi_store_memory_map(efi_get_priv());
  

• if (ret)
  

• 	return ret;
  

• printf("Exiting EFI boot services\n");
  

• ret = efi_call_exit_boot_services();
  

• if (ret)
  

• 	return ret;
  

• }

• if (IS_ENABLED(CONFIG_EFI_APP))

• return efi_boot(setup_base, entry, image_64bit)
  
  

  if (image_64bit) { if (!cpu_has_64bit()) {

diff --git a/boot/bootm.c b/boot/bootm.c index 16a43d519a8..d5caa4cdb31 100644 --- a/boot/bootm.c +++ b/boot/bootm.c @@ -49,6 +49,10 @@ __weak void board_quiesce_devices(void) { }

+__weak void board_fixup_os(image_info_t *os) +{ +}

• #if CONFIG_IS_ENABLED(LEGACY_IMAGE_FORMAT) /**
  • image_get_kernel - verify legacy format kernel image

@@ -999,6 +1003,7 @@ int bootm_run_states(struct bootm_info *bmi, int states) /* Load the OS */ if (!ret && (states & BOOTM_STATE_LOADOS)) { iflag = bootm_disable_interrupts();

• board_fixup_os(&images->os);
  

  ret = bootm_load_os(images, 0); if (ret && ret != BOOTM_ERR_OVERLAP) goto err;

diff --git a/include/bootm.h b/include/bootm.h index 61160705215..b0d10123b53 100644 --- a/include/bootm.h +++ b/include/bootm.h @@ -217,6 +217,12 @@ void arch_preboot_os(void); */ void board_quiesce_devices(void);

+/*

• • boards should define this if they need to fix up the kernel before boot
• • (eg, by modifying the desired load address).
• */

+void board_fixup_os(image_info_t *os);

• /**
  • switch_to_non_secure_mode() - switch to non-secure mode
  */

diff --git a/include/efi.h b/include/efi.h index c559fda3004..1d06230439f 100644 --- a/include/efi.h +++ b/include/efi.h @@ -459,6 +459,7 @@ static inline struct efi_mem_desc *efi_get_next_mem_desc( */ struct efi_priv { efi_handle_t parent_image;

• struct efi_loaded_image *loaded_image; struct efi_system_table *sys_table; struct efi_boot_services *boot; struct efi_runtime_services *run;

diff --git a/lib/efi/efi.c b/lib/efi/efi.c index bcb34d67465..bb1d9e24f84 100644 --- a/lib/efi/efi.c +++ b/lib/efi/efi.c @@ -113,6 +113,7 @@ int efi_init(struct efi_priv *priv, const char *banner, efi_handle_t image, efi_puts(priv, "Failed to get loaded image protocol\n"); return ret; }

• priv->loaded_image = loaded_image; priv->image_data_type = loaded_image->image_data_type;

  return 0;

diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c index 9b94a93ee4f..7c3ef9a7926 100644 --- a/lib/efi/efi_app.c +++ b/lib/efi/efi_app.c @@ -14,6 +14,7 @@ #include <efi.h> #include <efi_api.h> #include <errno.h> +#include <image.h> #include <init.h> #include <malloc.h> #include <sysreset.h> @@ -218,6 +219,41 @@ static int efi_sysreset_request(struct udevice *dev, enum sysreset_t type) return -EINPROGRESS; }

+/*

• • Attempt to relocate the kernel to somewhere the firmware isn't using
• */

+#define PAGE_SIZE_BITS 12 +void board_fixup_os(image_info_t *os) +{

• int pages;
• ulong load_addr;
• u64 addr;
• efi_status_t status;
• struct efi_priv *priv = efi_get_priv();
• struct efi_boot_services *boot = priv->boot;
• pages = (os->image_len + ((1 << PAGE_SIZE_BITS) - 1)) >> PAGE_SIZE_BITS;
• addr = os->load;
• /* Try to allocate at the preferred address */
• status = boot->allocate_pages(EFI_ALLOCATE_ADDRESS, EFI_LOADER_DATA,

• 		      pages, &addr);
  

• if (status == EFI_SUCCESS)

• return;
  

• /* That failed, so try allocating anywhere there's enough room */
• status = boot->allocate_pages(EFI_ALLOCATE_ANY_PAGES, EFI_LOADER_DATA, pages, &addr);
• if (status == EFI_SUCCESS) {

• /* Make sure bootm knows where we loaded the image */
  

• os->load = addr;
  

• return;
  

• }

Why don't you simply call LoadImage()?

Best regards

Heinrich

• printf("Failed to alloc %lx bytes at %lx: %lx\n", os->image_len, load_addr,

•       status);
  

+}

• static const struct udevice_id efi_sysreset_ids[] = { { .compatible = "efi,reset" }, { }

Hi Matthew,

On Sat, 23 Nov 2024 at 12:56, Matthew Garrett mjg59@srcf.ucam.org wrote:

From: Matthew Garrett mgarrett@aurora.tech

part_find takes a GPT GUID and searches for a partition that matches that. It then sets the target_part environment variable to the media type, device number and partition number that matched, allowing $target_part to be passed directly to bootm and similar commands.

Signed-off-by: Matthew Garrett mgarrett@aurora.tech

cmd/Kconfig | 10 ++++ cmd/Makefile | 1 + cmd/part_find.c | 156 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+) create mode 100644 cmd/part_find.c

Reviewed-by: Simon Glass sjg@chromium.org

Nits below. This needs an update to doc/usage/cmd/part.rst and a test. There is a ChromeOS bootimage used in the bootflow tests which might be helpful here, as it uses GPT.

diff --git a/cmd/Kconfig b/cmd/Kconfig index 1d7ddb4ed36..ee85928ca21 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1529,6 +1529,16 @@ config CMD_PART Read and display information about the partition table on various media.

+config CMD_PART_FIND

•   bool "part_find"
  

•   depends on PARTITIONS
  

•   select HAVE_BLOCK_DEVICE
  

•   select PARTITION_UUIDS
  

•   select PARTITION_TYPE_GUID
  

•   help
  

•     Find a partition with a given type GUID and set the target_part
  

•     environment variable if located.
  

config CMD_PCI bool "pci - Access PCI devices" help diff --git a/cmd/Makefile b/cmd/Makefile index d1f369deec0..16fd8dcd723 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -137,6 +137,7 @@ obj-$(CONFIG_CMD_NVEDIT_EFI) += nvedit_efi.o obj-$(CONFIG_CMD_ONENAND) += onenand.o obj-$(CONFIG_CMD_OSD) += osd.o obj-$(CONFIG_CMD_PART) += part.o +obj-$(CONFIG_CMD_PART_FIND) += part_find.o obj-$(CONFIG_CMD_PCAP) += pcap.o ifdef CONFIG_PCI obj-$(CONFIG_CMD_PCI) += pci.o diff --git a/cmd/part_find.c b/cmd/part_find.c new file mode 100644 index 00000000000..8c071d3c374 --- /dev/null +++ b/cmd/part_find.c @@ -0,0 +1,156 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Aurora Innovation, Inc. Copyright 2022.
• */

+#include <blk.h> +#include <config.h> +#include <command.h> +#include <dm.h> +#include <env.h> +#include <part.h> +#if defined(CONFIG_EFI) || defined(CONFIG_EFI_APP)

Header files should not be behind #ifdef

+#include <efi.h> +#include <efi_api.h>

+static bool partition_is_on_device(const struct efi_device_path *device,

•                              const struct efi_device_path *part,
  

•                              __u32 *part_no)
  

u32

+{

•   size_t d_len, p_len;
  

•   const struct efi_device_path *p, *d;
  

•   for (d = device; d->type != DEVICE_PATH_TYPE_END; d = (void *)d + d->length) {
  

•   }
  

•   d_len = (void *)d - (void *)device;
  

•   for (p = part; p->type != DEVICE_PATH_TYPE_END &&
  

•                !(p->type == DEVICE_PATH_TYPE_MEDIA_DEVICE &&
  

•                  p->sub_type == DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH);
  

•        p = (void *)p + p->length) {
  

•   }
  

•   if (p->type != DEVICE_PATH_TYPE_MEDIA_DEVICE ||
  

•       p->sub_type != DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH) {
  

•           // Not a partition.
  

•           return false;
  

•   }
  

•   p_len = (void *)p - (void *)part;
  

•   if (p_len == d_len && !memcmp(device, part, p_len)) {
  

•           if (part_no)
  

•                   *part_no = ((__u32 *)p)[1];
  

•           return true;
  

•   }
  

•   return false;
  

+} +#endif

+static int part_find(int argc, char *const argv[]) +{ +#if defined(CONFIG_EFI) || defined(CONFIG_EFI_APP)

•   efi_guid_t efi_devpath_guid = EFI_DEVICE_PATH_PROTOCOL_GUID;
  

•   struct efi_device_path *loaded_image_path = NULL;
  

•   struct efi_boot_services *boot = efi_get_boot();
  

•   struct efi_priv *priv = efi_get_priv();
  

•   bool part_self = false;
  

+#endif

•   struct driver *d = ll_entry_start(struct driver, driver);
  

•   const int n_ents = ll_entry_count(struct driver, driver);
  

•   struct disk_partition info;
  

•   struct blk_desc *desc;
  

•   struct driver *entry;
  

•   struct udevice *udev;
  

•   struct uclass *uc;
  

•   int ret;
  

•   if (argc != 2)
  

•           return CMD_RET_USAGE;
  

+#if defined(CONFIG_EFI) || defined (CONFIG_EFI_APP)

Please use if (IS_ENABLED(CONFIG_EFI) || ...

We try to avoid #ifdef in C code

•   part_self = !strncmp(argv[1], "self", 6);
  

•   if (part_self) {
  

•           ret = boot->handle_protocol(priv->loaded_image->device_handle,
  

•                                   &efi_devpath_guid,
  

•                                   (void **)&loaded_image_path);
  

•           if (ret)
  

•                   log_warning("failed to get device path for loaded image (ret=%d)", ret);
  

•   }
  

+#endif

•   ret = uclass_get(UCLASS_BLK, &uc);
  

•   if (ret) {
  

•           puts("Could not get BLK uclass.\n");
  

•           return CMD_RET_FAILURE;
  

•   }
  

•   for (entry = d; entry < d + n_ents; entry++) {
  

•           if (entry->id != UCLASS_BLK)
  

•                   continue;
  

•           uclass_foreach_dev(udev, uc) {
  

•                   int i;
  

•                   if (udev->driver != entry)
  

•                           continue;
  

You should be able to do:

struct udevice *dev;

blk_foreach(BLKF_BOTH, dev) {

•                   desc = dev_get_uclass_plat(udev);
  

+#if defined(CONFIG_EFI) || defined(CONFIG_EFI_APP)

if() again

•                   if (part_self) {
  

•                           if (desc->if_type == IF_TYPE_EFI_MEDIA) {
  

•                                   struct efi_media_plat *plat =
  

•                                           dev_get_plat(udev->parent);
  

•                                   __u32 loader_part_no;
  

•                                   if (partition_is_on_device(plat->device_path,
  

•                                                              loaded_image_path,
  

•                                                              &loader_part_no)) {
  

•                                           char env[256];
  

•                                           ret = snprintf(env, sizeof(env), "%s %d:%d", blk_get_if_type_name(desc->if_type), desc->devnum, loader_part_no);
  

line too long

Perhaps put this whole block in a function?

•                                           if (ret < 0 || ret == sizeof(env))
  

•                                                   return CMD_RET_FAILURE;
  

•                                           if (env_set("target_part", env))
  

•                                                   return CMD_RET_FAILURE;
  

•                                           return CMD_RET_SUCCESS;
  

We normally write this as 'return 0'

•                                   }
  

•                           }
  

•                   } else {
  

+#endif

•                           for (i = 1; i <= MAX_SEARCH_PARTITIONS; i++) {
  

•                                   ret = part_get_info(desc, i, &info);
  

•                                   if (ret)
  

•                                           break;
  

•                                   if (strcasecmp(argv[1], info.type_guid) == 0) {
  

•                                           char env[256];
  

•                                           ret = snprintf(env, sizeof(env), "%s %d:%d", blk_get_if_type_name(desc->if_type), desc->devnum, i);
  

•                                           if (ret < 0 || ret == sizeof(env))
  

•                                                   return CMD_RET_FAILURE;
  

•                                           env_set("target_part", env);
  

•                                           debug("Setting target_part to %s\n", env);
  

•                                           return CMD_RET_SUCCESS;
  

•                                   }
  

•                           }
  

+#if defined(CONFIG_EFI) || defined(CONFIG_EFI_APP)

•                   }
  

+#endif

•           }
  

•   }
  

•   return CMD_RET_FAILURE;
  

+}

+static int do_part_find(struct cmd_tbl *cmdtp, int flag, int argc,

•                   char *const argv[])
  

+{

•   return part_find(argc, argv);
  

+}

+U_BOOT_CMD(

•   part_find, 2, 0, do_part_find, "Find a partition",
  

•   "<guid>\n"
  

•   "- Examine the list of known partitions for one that has a type\n"
  

•   "  GUID that matches 'guid', expressed in the standard text format.\n"
  

•   "  If successful, the target_part environment variable will be set\n"
  

•   "  to the corresponding 'interface dev:part'.\n"
  

+);

2.47.0

Regards, Simon

On 23.11.24 20:55, Matthew Garrett wrote:

From: Matthew Garrett mgarrett@aurora.tech

part_find takes a GPT GUID and searches for a partition that matches that. It then sets the target_part environment variable to the media type, device number and partition number that matched, allowing $target_part to be passed directly to bootm and similar commands.

Signed-off-by: Matthew Garrett mgarrett@aurora.tech

For user to memorize the command it would be preferable to add the functionality as sub-command to the existing part command.

Best regards

Heinrich

---

cmd/Kconfig | 10 ++++ cmd/Makefile | 1 + cmd/part_find.c | 156 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+) create mode 100644 cmd/part_find.c

diff --git a/cmd/Kconfig b/cmd/Kconfig index 1d7ddb4ed36..ee85928ca21 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1529,6 +1529,16 @@ config CMD_PART Read and display information about the partition table on various media.

+config CMD_PART_FIND

• bool "part_find"
• depends on PARTITIONS
• select HAVE_BLOCK_DEVICE
• select PARTITION_UUIDS
• select PARTITION_TYPE_GUID
• help

•  Find a partition with a given type GUID and set the target_part
  

•  environment variable if located.
  

• config CMD_PCI bool "pci - Access PCI devices" help

diff --git a/cmd/Makefile b/cmd/Makefile index d1f369deec0..16fd8dcd723 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -137,6 +137,7 @@ obj-$(CONFIG_CMD_NVEDIT_EFI) += nvedit_efi.o obj-$(CONFIG_CMD_ONENAND) += onenand.o obj-$(CONFIG_CMD_OSD) += osd.o obj-$(CONFIG_CMD_PART) += part.o +obj-$(CONFIG_CMD_PART_FIND) += part_find.o obj-$(CONFIG_CMD_PCAP) += pcap.o ifdef CONFIG_PCI obj-$(CONFIG_CMD_PCI) += pci.o diff --git a/cmd/part_find.c b/cmd/part_find.c new file mode 100644 index 00000000000..8c071d3c374 --- /dev/null +++ b/cmd/part_find.c @@ -0,0 +1,156 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Aurora Innovation, Inc. Copyright 2022.
• */

+#include <blk.h> +#include <config.h> +#include <command.h> +#include <dm.h> +#include <env.h> +#include <part.h> +#if defined(CONFIG_EFI) || defined(CONFIG_EFI_APP) +#include <efi.h> +#include <efi_api.h>

+static bool partition_is_on_device(const struct efi_device_path *device,

• 		   const struct efi_device_path *part,
  

• 		   __u32 *part_no)
  

+{

• size_t d_len, p_len;
• const struct efi_device_path *p, *d;
• for (d = device; d->type != DEVICE_PATH_TYPE_END; d = (void *)d + d->length) {
• }
• d_len = (void *)d - (void *)device;
• for (p = part; p->type != DEVICE_PATH_TYPE_END &&

•      !(p->type == DEVICE_PATH_TYPE_MEDIA_DEVICE &&
  

•        p->sub_type == DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH);
  

•     p = (void *)p + p->length) {
  

• }
• if (p->type != DEVICE_PATH_TYPE_MEDIA_DEVICE ||

•    p->sub_type != DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH) {
  

• // Not a partition.
  

• return false;
  

• }
• p_len = (void *)p - (void *)part;
• if (p_len == d_len && !memcmp(device, part, p_len)) {

• if (part_no)
  

• 	*part_no = ((__u32 *)p)[1];
  

• return true;
  

• }
• return false;

+} +#endif

+static int part_find(int argc, char *const argv[]) +{ +#if defined(CONFIG_EFI) || defined(CONFIG_EFI_APP)

• efi_guid_t efi_devpath_guid = EFI_DEVICE_PATH_PROTOCOL_GUID;
• struct efi_device_path *loaded_image_path = NULL;
• struct efi_boot_services *boot = efi_get_boot();
• struct efi_priv *priv = efi_get_priv();
• bool part_self = false;

+#endif

• struct driver *d = ll_entry_start(struct driver, driver);
• const int n_ents = ll_entry_count(struct driver, driver);
• struct disk_partition info;
• struct blk_desc *desc;
• struct driver *entry;
• struct udevice *udev;
• struct uclass *uc;
• int ret;
• if (argc != 2)

• return CMD_RET_USAGE;
  

+#if defined(CONFIG_EFI) || defined (CONFIG_EFI_APP)

• part_self = !strncmp(argv[1], "self", 6);
• if (part_self) {

• ret = boot->handle_protocol(priv->loaded_image->device_handle,
  

• 			&efi_devpath_guid,
  

• 			(void **)&loaded_image_path);
  

• if (ret)
  

• 	log_warning("failed to get device path for loaded image (ret=%d)", ret);
  

• }

+#endif

• ret = uclass_get(UCLASS_BLK, &uc);
• if (ret) {

• puts("Could not get BLK uclass.\n");
  

• return CMD_RET_FAILURE;
  

• }
• for (entry = d; entry < d + n_ents; entry++) {

• if (entry->id != UCLASS_BLK)
  

• 	continue;
  

• uclass_foreach_dev(udev, uc) {
  

• 	int i;
  

• 	if (udev->driver != entry)
  

• 		continue;
  

• 	desc = dev_get_uclass_plat(udev);
  

+#if defined(CONFIG_EFI) || defined(CONFIG_EFI_APP)

• 	if (part_self) {
  

• 		if (desc->if_type == IF_TYPE_EFI_MEDIA) {
  

• 			struct efi_media_plat *plat =
  

• 				dev_get_plat(udev->parent);
  

• 			__u32 loader_part_no;
  

• 			if (partition_is_on_device(plat->device_path,
  

• 						   loaded_image_path,
  

• 						   &loader_part_no)) {
  

• 				char env[256];
  

• 				ret = snprintf(env, sizeof(env), "%s %d:%d", blk_get_if_type_name(desc->if_type), desc->devnum, loader_part_no);
  

• 				if (ret < 0 || ret == sizeof(env))
  

• 					return CMD_RET_FAILURE;
  

• 				if (env_set("target_part", env))
  

• 					return CMD_RET_FAILURE;
  

• 				return CMD_RET_SUCCESS;
  

• 			}
  

• 		}
  

• 	} else {
  

+#endif

• 		for (i = 1; i <= MAX_SEARCH_PARTITIONS; i++) {
  

• 			ret = part_get_info(desc, i, &info);
  

• 			if (ret)
  

• 				break;
  

• 			if (strcasecmp(argv[1], info.type_guid) == 0) {
  

• 				char env[256];
  

• 				ret = snprintf(env, sizeof(env), "%s %d:%d", blk_get_if_type_name(desc->if_type), desc->devnum, i);
  

• 				if (ret < 0 || ret == sizeof(env))
  

• 					return CMD_RET_FAILURE;
  

• 				env_set("target_part", env);
  

• 				debug("Setting target_part to %s\n", env);
  

• 				return CMD_RET_SUCCESS;
  

• 			}
  

• 		}
  

+#if defined(CONFIG_EFI) || defined(CONFIG_EFI_APP)

• 	}
  

+#endif

• }
  

• }
• return CMD_RET_FAILURE;

+}

+static int do_part_find(struct cmd_tbl *cmdtp, int flag, int argc,

• 	char *const argv[])
  

+{

• return part_find(argc, argv);

+}

+U_BOOT_CMD(

• part_find, 2, 0, do_part_find, "Find a partition",
• "<guid>\n"
• "- Examine the list of known partitions for one that has a type\n"
• " GUID that matches 'guid', expressed in the standard text format.\n"
• " If successful, the target_part environment variable will be set\n"
• " to the corresponding 'interface dev:part'.\n"

+);

On Sat, 23 Nov 2024 at 12:57, Matthew Garrett mjg59@srcf.ucam.org wrote:

From: Matthew Garrett mgarrett@aurora.tech

For systems with more complicated firmware, the firmware memory map may vary significantly based on a number of factors. This makes it difficult to pick a hardcoded load address. Add a command for finding an available address with sufficient room to load the provided path.

Signed-off-by: Matthew Garrett mgarrett@aurora.tech

cmd/Kconfig | 7 ++++ cmd/Makefile | 1 + cmd/addr_find.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+) create mode 100644 cmd/addr_find.c

Reviewed-by: Simon Glass sjg@chromium.org

(again, this needs doc/ and test/cmd/...)

diff --git a/cmd/Kconfig b/cmd/Kconfig index ee85928ca21..5ed6a50121c 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -128,6 +128,13 @@ config CMD_ACPI between the firmware and OS, and is particularly useful when you want to make hardware changes without the OS needing to be adjusted.

+config CMD_ADDR_FIND

•    bool "addr_find"
  

•   help
  

•     This command searches for an unused region of address space
  

•     sufficiently large to hold a file. If successful, it sets the
  

•     loadaddr variable to this address.
  

config CMD_ADDRMAP bool "addrmap" depends on ADDR_MAP diff --git a/cmd/Makefile b/cmd/Makefile index 16fd8dcd723..38cb7a4aea5 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -15,6 +15,7 @@ obj-y += version.o obj-$(CONFIG_CMD_ARMFFA) += armffa.o obj-$(CONFIG_CMD_2048) += 2048.o obj-$(CONFIG_CMD_ACPI) += acpi.o +obj-$(CONFIG_CMD_ADDR_FIND) += addr_find.o obj-$(CONFIG_CMD_ADDRMAP) += addrmap.o obj-$(CONFIG_CMD_AES) += aes.o obj-$(CONFIG_CMD_ADC) += adc.o diff --git a/cmd/addr_find.c b/cmd/addr_find.c new file mode 100644 index 00000000000..b187337d885 --- /dev/null +++ b/cmd/addr_find.c @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Aurora Innovation, Inc. Copyright 2022.
• */

+#include <blk.h> +#include <config.h> +#include <command.h> +#include <env.h> +#include <fs.h> +#include <lmb.h> +#include <asm/global_data.h>

+DECLARE_GLOBAL_DATA_PTR;

+int do_addr_find(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) +{

•   struct lmb_region *mem, *reserved;
  

•   const char *filename;
  

•   struct lmb lmb;
  

•   loff_t size;
  

•   int ret;
  

•   int i, j;
  

•   if (!gd->fdt_blob) {
  

•           log_err("No FDT setup\n");
  

•           return CMD_RET_FAILURE;
  

•   }
  

•   if (fs_set_blk_dev(argv[1], argc >= 3 ? argv[2] : NULL, FS_TYPE_FAT)) {
  

•           log_err("Can't set block device\n");
  

•           return CMD_RET_FAILURE;
  

•   }
  

•   if (argc >= 4) {
  

•           filename = argv[3];
  

•   } else {
  

•           filename = env_get("bootfile");
  

•           if (!filename) {
  

•                   log_err("No boot file defined\n");
  

•                   return CMD_RET_FAILURE;
  

•           }
  

•   }
  

•   ret = fs_size(filename, &size);
  

•   if (ret != 0) {
  

•           log_err("Failed to get file size\n");
  

•           return CMD_RET_FAILURE;
  

•   }
  

•   lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob);
  

As Tom says, lmb has changed to be global for now. However, since we are not using EFI_LOADER's memory allocation here, it seems fine to me.

•   mem = &lmb.memory;
  

•   reserved = &lmb.reserved;
  

•   for (i = 0; i < mem->cnt; i++) {
  

•           unsigned long long start, end;
  

•           start = mem->region[i].base;
  

•           end = mem->region[i].base + mem->region[i].size - 1;
  

•           if ((start + size) > end)
  

•                   continue;
  

•           for (j = 0; j < reserved->cnt; j++) {
  

•                   if ((reserved->region[j].base + reserved->region[j].size) < start)
  

•                           continue;
  

•                   if ((start + size) > reserved->region[j].base)
  

•                           start = reserved->region[j].base + reserved->region[j].size;
  

•           }
  

•           if ((start + size) <= end) {
  

•                   env_set_hex("loadaddr", start);
  

•                   debug("Set loadaddr to 0x%llx\n", start);
  

•                   return CMD_RET_SUCCESS;
  

•           }
  

•   }
  

•   log_err("Failed to find enough RAM for 0x%llx bytes\n", size);
  

•   return CMD_RET_FAILURE;
  

+}

+U_BOOT_CMD(

•   addr_find, 7, 1, do_addr_find,
  

•   "find a load address suitable for a file",
  

•   "<interface> [<dev[:part]>] <filename>\n"
  

•   "- find a consecutive region of memory sufficiently large to hold\n"
  

•   "  the file called 'filename' from 'dev' on 'interface'. If\n"
  

•   "  successful, 'loadaddr' will be set to the located address."
  

+);

2.47.0

Regards, Simon

Hi Matthew

On Sat, 23 Nov 2024 at 21:57, Matthew Garrett mjg59@srcf.ucam.org wrote:

From: Matthew Garrett mgarrett@aurora.tech

The UEFI app is an actual executable with things like section headers, so just gluing the DTB onto the end of it won't work. Add an additional section to contain this and allocate some space, and then during build copy the DTB into that section.

Signed-off-by: Matthew Garrett mgarrett@aurora.tech

Makefile | 7 ++++++- arch/x86/config.mk | 2 +- arch/x86/lib/elf_x86_64_efi.lds | 4 ++++ include/asm-generic/sections.h | 1 + lib/efi/Makefile | 2 +- lib/efi/efi_dtb.S | 6 ++++++ lib/fdtdec.c | 3 +++ 7 files changed, 22 insertions(+), 3 deletions(-) create mode 100644 lib/efi/efi_dtb.S

diff --git a/Makefile b/Makefile index 2eaae427961..18abaa1ac52 100644 --- a/Makefile +++ b/Makefile @@ -1067,6 +1067,10 @@ quiet_cmd_objcopy = OBJCOPY $@ cmd_objcopy = $(OBJCOPY) --gap-fill=0xff $(OBJCOPYFLAGS) \ $(OBJCOPYFLAGS_$(@F)) $< $@

+# Inject the DTB into u-boot +quiet_cmd_embeddtb = OBJCOPY $@ +cmd_embeddtb = $(OBJCOPY) --update-section .embedded_dtb=dts/dt.dtb --set-section-flags .embedded_dtb=contents,alloc,load,data $<

# Provide a version which does not do this, for use by EFI quiet_cmd_zobjcopy = OBJCOPY $@ cmd_zobjcopy = $(OBJCOPY) $(OBJCOPYFLAGS) $(OBJCOPYFLAGS_$(@F)) $< $@ @@ -1673,7 +1677,8 @@ u-boot-x86-reset16.bin: u-boot FORCE endif # CONFIG_X86

OBJCOPYFLAGS_u-boot-app.efi := $(OBJCOPYFLAGS_EFI) -u-boot-app.efi: u-boot FORCE +u-boot-app.efi: u-boot dts/dt.dtb FORCE

•   $(call if_changed,embeddtb)
    $(call if_changed,zobjcopy)
  
  

u-boot.bin.o: u-boot.bin FORCE diff --git a/arch/x86/config.mk b/arch/x86/config.mk index 6d4839dfb38..ac1f1922b12 100644 --- a/arch/x86/config.mk +++ b/arch/x86/config.mk @@ -45,7 +45,7 @@ LDFLAGS_EFI_PAYLOAD := -Bsymbolic -Bsymbolic-functions -shared --no-undefined \ -s -zexecstack

OBJCOPYFLAGS_EFI := -j .text -j .sdata -j .data -j .dynamic -j .dynsym \

•   -j .rel -j .rela -j .reloc --strip-all
  

•   -j .rel -j .rela -j .reloc -j .embedded_dtb --strip-all
  
  

# Compiler flags to be added when building UEFI applications CFLAGS_EFI := -fpic -fshort-wchar diff --git a/arch/x86/lib/elf_x86_64_efi.lds b/arch/x86/lib/elf_x86_64_efi.lds index ada024c05c3..cb656ac46ea 100644 --- a/arch/x86/lib/elf_x86_64_efi.lds +++ b/arch/x86/lib/elf_x86_64_efi.lds @@ -79,5 +79,9 @@ SECTIONS *(.note.GNU-stack) }

•   .embedded_dtb : {
  

•           *(.embedded_dtb)
  

•   }
  

•   .comment 0 : { *(.comment) }
  

} diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h index b6bca53db10..4113ea2a866 100644 --- a/include/asm-generic/sections.h +++ b/include/asm-generic/sections.h @@ -70,6 +70,7 @@ extern char __image_copy_start[], __image_copy_end[]; extern char __bss_end[]; extern char __rel_dyn_start[], __rel_dyn_end[]; extern char _image_binary_end[]; +extern char _dtb[];

/*

• This is the U-Boot entry point - prior to relocation it should be same

diff --git a/lib/efi/Makefile b/lib/efi/Makefile index 63845287336..9f51671c65d 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 efi_app_init.o efi_vars.o +obj-$(CONFIG_EFI_APP) += efi_app.o efi.o efi_app_init.o efi_vars.o efi_dtb.o obj-$(CONFIG_EFI_STUB) += efi_info.o

CFLAGS_REMOVE_efi_stub.o := -mregparm=3 \ diff --git a/lib/efi/efi_dtb.S b/lib/efi/efi_dtb.S new file mode 100644 index 00000000000..75e0c4a5765 --- /dev/null +++ b/lib/efi/efi_dtb.S @@ -0,0 +1,6 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +#ifdef CONFIG_OF_SEPARATE +.section .embedded_dtb, "a" +.globl __dtb +__dtb: .fill 1024*1024 +#endif diff --git a/lib/fdtdec.c b/lib/fdtdec.c index b0655988029..63853f816f4 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1236,6 +1236,9 @@ static void *fdt_find_separate(void) fdt_blob = (ulong *)_image_binary_end; else fdt_blob = (ulong *)__bss_end; +#elif defined CONFIG_EFI_APP

Don't you need CONFIG_OF_SEPARATE as well here? I haven't checked the EFI app, is this the only available option?

•   /* FDT is in a separate section */
  

•   fdt_blob = (ulong *)__dtb;
  

#else /* FDT is at end of image */ fdt_blob = (ulong *)_end; -- 2.47.0

Other than that this looks reasonable to me.

Thanks /Ilias

On Sat, 23 Nov 2024 at 12:57, Matthew Garrett mjg59@srcf.ucam.org wrote:

From: Matthew Garrett mgarrett@aurora.tech

CONFIG_SYS_BOOT_RAMDISK_HIGH copies the initrd out of the FIT and into correctly aligned RAM, but the addresses used for this are then discarded by the x86 bootm code. Fix that.

Signed-off-by: Matthew Garrett mgarrett@aurora.tech

arch/x86/lib/bootm.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

diff --git a/arch/x86/lib/bootm.c b/arch/x86/lib/bootm.c index c4446b1f9c6..cabf18fccb7 100644 --- a/arch/x86/lib/bootm.c +++ b/arch/x86/lib/bootm.c @@ -72,6 +72,7 @@ int arch_fixup_memory_node(void *blob) /* Subcommand: PREP */ static int boot_prep_linux(struct bootm_headers *images) {

•   ulong initrd_start, initrd_size;
    char *cmd_line_dest = NULL;
    struct legacy_img_hdr *hdr;
    int is_zimage = 0;
  

@@ -134,10 +135,16 @@ static int boot_prep_linux(struct bootm_headers *images) goto error; }

•   if (IS_ENABLED(CONFIG_SYS_BOOT_RAMDISK_HIGH)) {
  

•           initrd_start = images->initrd_start;
  

•           initrd_size = images->initrd_end - images->initrd_start;
  

•   } else {
  

•           initrd_start = images->rd_start;
  

•           initrd_size = images->rd_end - images->rd_start;
  

•   }
    printf("Setup at %#08lx\n", images->ep);
    ret = setup_zimage((void *)images->ep, cmd_line_dest,
  

•                   0, images->rd_start,
  

•                   images->rd_end - images->rd_start, 0);
  

•                      0, initrd_start, initrd_size, 0);
  
    if (ret) {
            printf("## Setting up boot parameters failed ...\n");
  

-- 2.47.0

Hi Ilias,

On Mon, 25 Nov 2024 at 06:41, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

Hi Matthew, Janis,

On Sat, 23 Nov 2024 at 21:57, Matthew Garrett mjg59@srcf.ucam.org wrote:

...

From: Janis Danisevskis jdanisevskis@aurora.tech

efi_bind_block had two issues.

1. A pointer to a the stack was inserted as plat structure and thus used

beyond its life time.

Yep and I wonder how the device_unbind() code managed to survice since it free that stack allocated memory... Probably never called.

:-)

...

2. Only the first segment of the device path was copied into the

platfom data structure resulting in an unterminated device path.

Signed-off-by: Janis Danisevskis jdanisevskis@aurora.tech Signed-off-by: Matthew Garrett mgarrett@aurora.tech

---

lib/efi/efi_app_init.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/lib/efi/efi_app_init.c b/lib/efi/efi_app_init.c index 9704020b749..cc91e1d74b8 100644 --- a/lib/efi/efi_app_init.c +++ b/lib/efi/efi_app_init.c @@ -19,6 +19,15 @@

DECLARE_GLOBAL_DATA_PTR;

+static size_t device_path_length(const struct efi_device_path *device_path) +{

•   const struct efi_device_path *d;
  

•   for (d = device_path; d->type != DEVICE_PATH_TYPE_END; d = (void *)d + d->length) {
  

•   }
  

•   return (void *)d - (void *)device_path + d->length;
  

+}

We already define efi_dp_size(), you can use that here.

Unfortunately that is only available in EFI_LOADER. Should we try to split some of the code out to lib/efi ?

...

/**

• efi_bind_block() - bind a new block device to an EFI device

@@ -39,19 +48,23 @@ 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 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)
  

•   size_t device_path_len = device_path_length(device_path);
  

•   plat = malloc(sizeof(struct efi_media_plat));
  

•   if (!plat)
  

•           return log_msg_ret("plat", -ENOMEM);
  

•   plat->handle = handle;
  

•   plat->blkio = blkio;
  

•   plat->device_path = malloc(device_path_len);
  

•   if (!plat->device_path)
  

you need to free plat here now

I will send a patch.

...

            return log_msg_ret("path", -ENOMEM);

•   memcpy(plat.device_path, device_path, device_path->length);
  

•   memcpy(plat->device_path, device_path, device_path_len);
    ret = device_bind(dm_root(), DM_DRIVER_GET(efi_media), "efi_media",
  

•                     &plat, ofnode_null(), &dev);
  

•                     plat, ofnode_null(), &dev);
    if (ret)
            return log_msg_ret("bind", ret);
  
  

-- 2.47.0

Regards, Simon

Hi Heinrich,

On Tue, 10 Dec 2024 at 01:50, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 23.11.24 20:55, Matthew Garrett wrote:

...

From: Janis Danisevskis jdanisevskis@aurora.tech

efi_bind_block had two issues.

1. A pointer to a the stack was inserted as plat structure and thus used

beyond its life time. 2. Only the first segment of the device path was copied into the platfom data structure resulting in an unterminated device path.

Signed-off-by: Janis Danisevskis jdanisevskis@aurora.tech Signed-off-by: Matthew Garrett mgarrett@aurora.tech

---

lib/efi/efi_app_init.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/lib/efi/efi_app_init.c b/lib/efi/efi_app_init.c index 9704020b749..cc91e1d74b8 100644 --- a/lib/efi/efi_app_init.c +++ b/lib/efi/efi_app_init.c @@ -19,6 +19,15 @@

DECLARE_GLOBAL_DATA_PTR;

+static size_t device_path_length(const struct efi_device_path *device_path) +{

• const struct efi_device_path *d;
  

• for (d = device_path; d->type != DEVICE_PATH_TYPE_END; d = (void *)d + d->length) {
  

• }
  

• return (void *)d - (void *)device_path + d->length;
  

+}

• /**
  • efi_bind_block() - bind a new block device to an EFI device

@@ -39,19 +48,23 @@ 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 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)
  

• size_t device_path_len = device_path_length(device_path);
  

• plat = malloc(sizeof(struct efi_media_plat));
  

• if (!plat)
  

•         return log_msg_ret("plat", -ENOMEM);
  

• plat->handle = handle;
  

• plat->blkio = blkio;
  

• plat->device_path = malloc(device_path_len);
  

• if (!plat->device_path)
          return log_msg_ret("path", -ENOMEM);
  

• memcpy(plat.device_path, device_path, device_path->length);
  

• memcpy(plat->device_path, device_path, device_path_len);
  ret = device_bind(dm_root(), DM_DRIVER_GET(efi_media), "efi_media",
  

•                   &plat, ofnode_null(), &dev);
  

•                   plat, ofnode_null(), &dev);
  if (ret)
          return log_msg_ret("bind", ret);
  

Here you have a memory leak for pointer plat if ret != 0.

Simon suggested a follow up patch. Instead we should fix it in this patch.

@Simon The logic in the caller setup_block() looks wrong.

LocateHandle() does not ensure that when you try to read from the block device the same still does exist. E.g. if a USB device is removed the block IO protocol could be uninstalled and the handle deleted and the EFI app may crash. Please, call OpenProtocol() with attribute BY_DRIVER.

HandleProtocol() is considered deprecated. Please, use OpenProtocol() instead.

Can you please send a follow-up with your ideas on this?

Regards, SImon

Hi everyone,

Good catch finding this leak. Would you like me to provide an updated patch? I am good either way. Let Simon take credit for the fix on the git history. I can live with shame :-).

Janis

On Tue, Dec 10, 2024 at 9:11 AM Tom Rini trini@konsulko.com wrote:

On Tue, Dec 10, 2024 at 09:17:26AM -0700, Simon Glass wrote:

...

Hi Heinrich,

On Tue, 10 Dec 2024 at 01:50, Heinrich Schuchardt xypron.glpk@gmx.de

wrote:

...

...

On 23.11.24 20:55, Matthew Garrett wrote:

...

From: Janis Danisevskis jdanisevskis@aurora.tech

efi_bind_block had two issues.

1. A pointer to a the stack was inserted as plat structure and thus

used

...

...

...

beyond its life time. 2. Only the first segment of the device path was copied into the platfom data structure resulting in an unterminated device path.

Signed-off-by: Janis Danisevskis jdanisevskis@aurora.tech Signed-off-by: Matthew Garrett mgarrett@aurora.tech

---

lib/efi/efi_app_init.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/lib/efi/efi_app_init.c b/lib/efi/efi_app_init.c index 9704020b749..cc91e1d74b8 100644 --- a/lib/efi/efi_app_init.c +++ b/lib/efi/efi_app_init.c @@ -19,6 +19,15 @@

DECLARE_GLOBAL_DATA_PTR;

+static size_t device_path_length(const struct efi_device_path

*device_path)

...

...

...

+{

• const struct efi_device_path *d;
  

• for (d = device_path; d->type != DEVICE_PATH_TYPE_END; d =
  

(void *)d + d->length) {

...

...

...

• }
  

• return (void *)d - (void *)device_path + d->length;
  

+}

• /**
  • efi_bind_block() - bind a new block device to an EFI device

@@ -39,19 +48,23 @@ 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 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)
  

• size_t device_path_len = device_path_length(device_path);
  

• plat = malloc(sizeof(struct efi_media_plat));
  

• if (!plat)
  

•         return log_msg_ret("plat", -ENOMEM);
  

• plat->handle = handle;
  

• plat->blkio = blkio;
  

• plat->device_path = malloc(device_path_len);
  

• if (!plat->device_path)
          return log_msg_ret("path", -ENOMEM);
  

• memcpy(plat.device_path, device_path, device_path->length);
  

• memcpy(plat->device_path, device_path, device_path_len);
  ret = device_bind(dm_root(), DM_DRIVER_GET(efi_media),
  

"efi_media",

...

...

...

•                   &plat, ofnode_null(), &dev);
  

•                   plat, ofnode_null(), &dev);
  if (ret)
          return log_msg_ret("bind", ret);
  

Here you have a memory leak for pointer plat if ret != 0.

Simon suggested a follow up patch. Instead we should fix it in this

patch.

...

...

@Simon The logic in the caller setup_block() looks wrong.

LocateHandle() does not ensure that when you try to read from the block device the same still does exist. E.g. if a USB device is removed the block IO protocol could be uninstalled and the handle deleted and the EFI app may crash. Please, call OpenProtocol() with attribute

BY_DRIVER.

...

...

HandleProtocol() is considered deprecated. Please, use OpenProtocol() instead.

Can you please send a follow-up with your ideas on this?

Simon, it's in your tree. The normal path here would be that the submitter would correct this in v2. But you're trying to short-circuit that path for your own reasons. No one should be making work on top of your tree except for you.

-- Tom

Hi Janis,

On Wed, 11 Dec 2024 at 10:50, Janis Danisevskis jdanisevskis@aurora.tech wrote:

Hi everyone,

Good catch finding this leak. Would you like me to provide an updated patch? I am good either way. Let Simon take credit for the fix on the git history. I can live with shame :-).

Thanks for the email. If you're able to send a v2, take a look at this series which has a few other things:

https://patchwork.ozlabs.org/project/uboot/list/?series=436150

You can incorporate any or all of those and there's no need to keep me as the author of anything in particular.

Regards, Simon

Janis

On Tue, Dec 10, 2024 at 9:11 AM Tom Rini trini@konsulko.com wrote:

...

On Tue, Dec 10, 2024 at 09:17:26AM -0700, Simon Glass wrote:

...

Hi Heinrich,

On Tue, 10 Dec 2024 at 01:50, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 23.11.24 20:55, Matthew Garrett wrote:

...

From: Janis Danisevskis jdanisevskis@aurora.tech

efi_bind_block had two issues.

1. A pointer to a the stack was inserted as plat structure and thus used

beyond its life time. 2. Only the first segment of the device path was copied into the platfom data structure resulting in an unterminated device path.

Signed-off-by: Janis Danisevskis jdanisevskis@aurora.tech Signed-off-by: Matthew Garrett mgarrett@aurora.tech

---

lib/efi/efi_app_init.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/lib/efi/efi_app_init.c b/lib/efi/efi_app_init.c index 9704020b749..cc91e1d74b8 100644 --- a/lib/efi/efi_app_init.c +++ b/lib/efi/efi_app_init.c @@ -19,6 +19,15 @@

DECLARE_GLOBAL_DATA_PTR;

+static size_t device_path_length(const struct efi_device_path *device_path) +{

• const struct efi_device_path *d;
  

• for (d = device_path; d->type != DEVICE_PATH_TYPE_END; d = (void *)d + d->length) {
  

• }
  

• return (void *)d - (void *)device_path + d->length;
  

+}

• /**
  • efi_bind_block() - bind a new block device to an EFI device

@@ -39,19 +48,23 @@ 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 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)
  

• size_t device_path_len = device_path_length(device_path);
  

• plat = malloc(sizeof(struct efi_media_plat));
  

• if (!plat)
  

•         return log_msg_ret("plat", -ENOMEM);
  

• plat->handle = handle;
  

• plat->blkio = blkio;
  

• plat->device_path = malloc(device_path_len);
  

• if (!plat->device_path)
          return log_msg_ret("path", -ENOMEM);
  

• memcpy(plat.device_path, device_path, device_path->length);
  

• memcpy(plat->device_path, device_path, device_path_len);
  ret = device_bind(dm_root(), DM_DRIVER_GET(efi_media), "efi_media",
  

•                   &plat, ofnode_null(), &dev);
  

•                   plat, ofnode_null(), &dev);
  if (ret)
          return log_msg_ret("bind", ret);
  

Here you have a memory leak for pointer plat if ret != 0.

Simon suggested a follow up patch. Instead we should fix it in this patch.

@Simon The logic in the caller setup_block() looks wrong.

LocateHandle() does not ensure that when you try to read from the block device the same still does exist. E.g. if a USB device is removed the block IO protocol could be uninstalled and the handle deleted and the EFI app may crash. Please, call OpenProtocol() with attribute BY_DRIVER.

HandleProtocol() is considered deprecated. Please, use OpenProtocol() instead.

Can you please send a follow-up with your ideas on this?

Simon, it's in your tree. The normal path here would be that the submitter would correct this in v2. But you're trying to short-circuit that path for your own reasons. No one should be making work on top of your tree except for you.

-- Tom

Hi Heinrich,

On Sun, 24 Nov 2024 at 08:11, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 11/23/24 20:55, Matthew Garrett wrote:

...

From: Matthew Garrett mgarrett@aurora.tech

We may want to make things conditional on EFI variable state

Signed-off-by: Matthew Garrett mgarrett@aurora.tech

cmd/Kconfig | 4 ++ cmd/Makefile | 1 + cmd/efigetenv.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 138 insertions(+) create mode 100644 cmd/efigetenv.c

diff --git a/cmd/Kconfig b/cmd/Kconfig index 33bf3d1ad39..118fb721081 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -752,6 +752,10 @@ config CMD_NVEDIT_SELECT help Select the compiled-in persistent storage of environment variables.

+config CMD_EFI_GET_ENV

• bool "efi get env"
  

• help
  

•   Set an environment variable to the contents of an EFI variable
  

  endmenu

  menu "Memory commands"

diff --git a/cmd/Makefile b/cmd/Makefile index 38cb7a4aea5..0507c204c0e 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -69,6 +69,7 @@ obj-$(CONFIG_CMD_EEPROM) += eeprom.o obj-$(CONFIG_EFI) += efi.o efi_common.o obj-$(CONFIG_CMD_EFIDEBUG) += efidebug.o efi_common.o obj-$(CONFIG_CMD_EFICONFIG) += eficonfig.o +obj-$(CONFIG_CMD_EFI_GET_ENV) += efigetenv.o ifdef CONFIG_CMD_EFICONFIG ifdef CONFIG_EFI_MM_COMM_TEE obj-$(CONFIG_EFI_SECURE_BOOT) += eficonfig_sbkey.o diff --git a/cmd/efigetenv.c b/cmd/efigetenv.c new file mode 100644 index 00000000000..5284ee92d6c --- /dev/null +++ b/cmd/efigetenv.c @@ -0,0 +1,133 @@ +// SPDX-License-Identifier: GPL-2.0+ +#include <charset.h> +#include <command.h> +#include <efi_loader.h> +#include <efi_variable.h> +#include <env.h> +#include <hexdump.h> +#include <malloc.h> +#include <uuid.h>

+/* Set a U-Boot environment variable to the contents of a UEFI variable */ +int do_efi_get_env(struct cmd_tbl *cmdtb, int flat, int argc, char *const argv[]) +{

• u16 *var_name = NULL;
  

• char *strdata = NULL;
  

• efi_uintn_t size = 0;
  

• bool var_content_is_utf16_string = false;
  

• efi_status_t ret;
  

• efi_guid_t guid;
  

• u8 *data = NULL;
  

• u32 attributes;
  

• size_t len;
  

• u64 time;
  

• u16 *p;
  

• ret = efi_init_obj_list();
  

• if (ret != EFI_SUCCESS) {
  

•         printf("Error: Cannot initialize UEFI sub-system, r = %lu\n",
  

•                ret & ~EFI_ERROR_MASK);
  

•         return CMD_RET_FAILURE;
  

• }
  

• argv++;
  

• argc--;
  

• if (argc != 3 && argc != 4)
  

•         return CMD_RET_USAGE;
  

• if (argc == 4) {
  

•         if (strcmp(argv[0], "-s"))
  

•                 return CMD_RET_USAGE;
  

•         var_content_is_utf16_string = true;
  

•         argv++;
  

•         argc--;
  

• }
  

• len = utf8_utf16_strnlen(argv[0], strlen(argv[0]));
  

• var_name = malloc((len + 1) * 2);
  

• if (!var_name) {
  

•         printf("## Out of memory\n");
  

•         return CMD_RET_FAILURE;
  

• }
  

• p = var_name;
  

• utf8_utf16_strncpy(&p, argv[0], len + 1);
  

• if (uuid_str_to_bin(argv[1], guid.b, UUID_STR_FORMAT_GUID)) {
  

•         ret = CMD_RET_USAGE;
  

•         goto out;
  

• }
  

• ret = efi_get_variable_int(var_name, &guid, &attributes, &size, data,
  

•                            &time);
  

• if (ret == EFI_BUFFER_TOO_SMALL) {
  

•         data = malloc(size);
  

•         if (!data) {
  

•                 printf("## Out of memory\n");
  

•                 ret = CMD_RET_FAILURE;
  

•                 goto out;
  

•         }
  

•         ret = efi_get_variable_int(var_name, &guid, &attributes,
  

•                                    &size, data, &time);
  

This duplicates code in efi_dump_single_var(), do_efi_capsule_res(), get_dp_device() and others.

We should carve out a function.

That can come later. In general there are EFI_LOADER functions which could be useful in lib/efi/

Reviewed-by: Simon Glass sjg@chromium.org

Regards, Simon

Hi Heinrich,

On Sun, 24 Nov 2024 at 08:11, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 11/23/24 20:55, Matthew Garrett wrote:

...

From: Matthew Garrett mgarrett@aurora.tech

We may want to make things conditional on EFI variable state

Signed-off-by: Matthew Garrett mgarrett@aurora.tech

cmd/Kconfig | 4 ++ cmd/Makefile | 1 + cmd/efigetenv.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 138 insertions(+) create mode 100644 cmd/efigetenv.c

diff --git a/cmd/Kconfig b/cmd/Kconfig index 33bf3d1ad39..118fb721081 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -752,6 +752,10 @@ config CMD_NVEDIT_SELECT help Select the compiled-in persistent storage of environment variables.

+config CMD_EFI_GET_ENV

• bool "efi get env"
  

• help
  

•   Set an environment variable to the contents of an EFI variable
  

  endmenu

  menu "Memory commands"

diff --git a/cmd/Makefile b/cmd/Makefile index 38cb7a4aea5..0507c204c0e 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -69,6 +69,7 @@ obj-$(CONFIG_CMD_EEPROM) += eeprom.o obj-$(CONFIG_EFI) += efi.o efi_common.o obj-$(CONFIG_CMD_EFIDEBUG) += efidebug.o efi_common.o obj-$(CONFIG_CMD_EFICONFIG) += eficonfig.o +obj-$(CONFIG_CMD_EFI_GET_ENV) += efigetenv.o ifdef CONFIG_CMD_EFICONFIG ifdef CONFIG_EFI_MM_COMM_TEE obj-$(CONFIG_EFI_SECURE_BOOT) += eficonfig_sbkey.o diff --git a/cmd/efigetenv.c b/cmd/efigetenv.c new file mode 100644 index 00000000000..5284ee92d6c --- /dev/null +++ b/cmd/efigetenv.c @@ -0,0 +1,133 @@ +// SPDX-License-Identifier: GPL-2.0+ +#include <charset.h> +#include <command.h> +#include <efi_loader.h> +#include <efi_variable.h> +#include <env.h> +#include <hexdump.h> +#include <malloc.h> +#include <uuid.h>

+/* Set a U-Boot environment variable to the contents of a UEFI variable */ +int do_efi_get_env(struct cmd_tbl *cmdtb, int flat, int argc, char *const argv[]) +{

• u16 *var_name = NULL;
  

• char *strdata = NULL;
  

• efi_uintn_t size = 0;
  

• bool var_content_is_utf16_string = false;
  

• efi_status_t ret;
  

• efi_guid_t guid;
  

• u8 *data = NULL;
  

• u32 attributes;
  

• size_t len;
  

• u64 time;
  

• u16 *p;
  

• ret = efi_init_obj_list();
  

• if (ret != EFI_SUCCESS) {
  

•         printf("Error: Cannot initialize UEFI sub-system, r = %lu\n",
  

•                ret & ~EFI_ERROR_MASK);
  

•         return CMD_RET_FAILURE;
  

• }
  

• argv++;
  

• argc--;
  

• if (argc != 3 && argc != 4)
  

•         return CMD_RET_USAGE;
  

• if (argc == 4) {
  

•         if (strcmp(argv[0], "-s"))
  

•                 return CMD_RET_USAGE;
  

•         var_content_is_utf16_string = true;
  

•         argv++;
  

•         argc--;
  

• }
  

• len = utf8_utf16_strnlen(argv[0], strlen(argv[0]));
  

• var_name = malloc((len + 1) * 2);
  

• if (!var_name) {
  

•         printf("## Out of memory\n");
  

•         return CMD_RET_FAILURE;
  

• }
  

• p = var_name;
  

• utf8_utf16_strncpy(&p, argv[0], len + 1);
  

• if (uuid_str_to_bin(argv[1], guid.b, UUID_STR_FORMAT_GUID)) {
  

•         ret = CMD_RET_USAGE;
  

•         goto out;
  

• }
  

• ret = efi_get_variable_int(var_name, &guid, &attributes, &size, data,
  

•                            &time);
  

• if (ret == EFI_BUFFER_TOO_SMALL) {
  

•         data = malloc(size);
  

•         if (!data) {
  

•                 printf("## Out of memory\n");
  

•                 ret = CMD_RET_FAILURE;
  

•                 goto out;
  

•         }
  

•         ret = efi_get_variable_int(var_name, &guid, &attributes,
  

•                                    &size, data, &time);
  

This duplicates code in efi_dump_single_var(), do_efi_capsule_res(), get_dp_device() and others.

We should carve out a function.

Can you have a look at what to do here and send a patch? As in the comment Ilias made, it seems like we may need some of the EFI-loader code to move to lib/efi

...

• }
  

• if (ret == EFI_NOT_FOUND) {
  

•         printf("Error: \"%ls\" not defined\n", var_name);
  

•         ret = CMD_RET_FAILURE;
  

•         goto out;
  

• }
  

• if (ret != EFI_SUCCESS) {
  

•         printf("Error: Cannot read variable, r = %lu\n",
  

•                ret & ~EFI_ERROR_MASK);
  

•         ret = CMD_RET_FAILURE;
  

•         goto out;
  

• }
  

• if (var_content_is_utf16_string) {
  

•         char *p;
  

•         len = utf16_utf8_strnlen((u16 *)data, size / 2);
  

•         strdata = malloc(len + 1);
  

•         if (!strdata) {
  

•                 printf("## Out of memory\n");
  

•                 ret = CMD_RET_FAILURE;
  

•                 goto out;
  

•         }
  

•         p = strdata;
  

•         utf16_utf8_strncpy(&p, (u16 *)data, size / 2);
  

• } else {
  

•         len = size * 2;
  

•         strdata = malloc(len + 1);
  

•         if (!strdata) {
  

•                 printf("## Out of memory\n");
  

•                 ret = CMD_RET_FAILURE;
  

•                 goto out;
  

•         }
  

•         bin2hex(strdata, data, size);
  

• }
  

• strdata[len] = '\0';
  

• ret = env_set(argv[2], strdata);
  

• if (ret) {
  

•         ret = CMD_RET_FAILURE;
  

•         goto out;
  

• }
  

• ret = CMD_RET_SUCCESS;
  

+out:

• free(strdata);
  

• free(data);
  

• free(var_name);
  

• return ret;
  

+}

+U_BOOT_CMD(

• efigetenv, 5, 4, do_efi_get_env,
  

• "set environment variable to content of EFI variable",
  

• "[-s] name guid envvar\n"
  

• "    - set environment variable 'envvar' to the EFI variable 'name'-'guid'\n"
  

• "      \"-s\": Interpret the EFI variable value as a UTF-16 string\n"
  

+);

Regards, SImon