[PATCH 00/10] Improve UEFI app support

[PATCH 00/10] Improve UEFI app support

[Matthew Garrett]

We run a mixed environment including traditional embedded Linux and 
UEFI. For consistency it's valuable to use U-Boot in both cases, but on 
UEFI systems we want to run the full Linux kernel UEFI boot stub. This 
patchset adds support for that, along with various other quality-of-life 
improvements such as automatically finding partitions based on their 
GUID, automatic identification of load addresses where the firmware has 
given us constraints, support for EFI variable access in the EFI app, 
drivers for EFI network adapters and TPMs, support for embedding DTBs in 
the EFI app, and a couple of bug fixes.


Janis Danisevskis (1):
  Fix efi_bind_block.

Matthew Garrett (9):
  Add EFI handover support to bootm
  Add part_find command
  Add a command to find a load address
  Hook up EFI env variable support in the EFI app
  Add EFI network driver
  Add UEFI TPM2 driver
  Support separate DTB files with the UEFI app
  Use the correct ramdisk address
  Add command to set an environment variable to an EFI variable

 Makefile                        |   7 +-
 arch/x86/config.mk              |   2 +-
 arch/x86/lib/bootm.c            |  60 ++++++++----
 arch/x86/lib/elf_x86_64_efi.lds |   4 +
 boot/bootm.c                    |   5 +
 cmd/Kconfig                     |  23 ++++-
 cmd/Makefile                    |   3 +
 cmd/addr_find.c                 |  87 +++++++++++++++++
 cmd/efigetenv.c                 | 133 ++++++++++++++++++++++++++
 cmd/part_find.c                 | 156 ++++++++++++++++++++++++++++++
 drivers/net/Kconfig             |   7 ++
 drivers/net/Makefile            |   1 +
 drivers/net/efi_net.c           | 110 +++++++++++++++++++++
 drivers/tpm/Kconfig             |   7 ++
 drivers/tpm/Makefile            |   1 +
 drivers/tpm/tpm2_efi.c          |  97 +++++++++++++++++++
 include/asm-generic/sections.h  |   1 +
 include/bootm.h                 |   6 ++
 include/efi.h                   |  24 +++++
 include/efi_tcg2.h              |   1 +
 lib/efi/Makefile                |   2 +-
 lib/efi/efi.c                   |   1 +
 lib/efi/efi_app.c               |  41 ++++++++
 lib/efi/efi_app_init.c          | 163 ++++++++++++++++++++++++++++++--
 lib/efi/efi_dtb.S               |   6 ++
 lib/efi/efi_vars.c              |  44 +++++++++
 lib/fdtdec.c                    |   3 +
 27 files changed, 967 insertions(+), 28 deletions(-)
 create mode 100644 cmd/addr_find.c
 create mode 100644 cmd/efigetenv.c
 create mode 100644 cmd/part_find.c
 create mode 100644 drivers/net/efi_net.c
 create mode 100644 drivers/tpm/tpm2_efi.c
 create mode 100644 lib/efi/efi_dtb.S
 create mode 100644 lib/efi/efi_vars.c

-- 
2.47.0

[PATCH 01/10] Add EFI handover support to bootm

[Matthew Garrett]

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;
+	}
+
+	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" },
 	{ }
-- 
2.47.0

Re: [PATCH 01/10] Add EFI handover support to bootm

[Heinrich Schuchardt]

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" },
>   	{ }

Re: [PATCH 01/10] Add EFI handover support to bootm

[Matthew Garrett]

On Sun, Nov 24, 2024 at 03:43:12PM +0100, Heinrich Schuchardt wrote:
> > +	/* 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()?

With secure boot that requires that the kernel image have a trusted 
signature, whereas we're relying on a signed FIT.

Re: [PATCH 01/10] Add EFI handover support to bootm

[Heinrich Schuchardt]

Am 24. November 2024 20:29:22 MEZ schrieb Matthew Garrett <mjg59@srcf.ucam.org>:
>On Sun, Nov 24, 2024 at 03:43:12PM +0100, Heinrich Schuchardt wrote:
>> > +	/* 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()?
>
>With secure boot that requires that the kernel image have a trusted 
>signature, whereas we're relying on a signed FIT.


Please, provide an answer describing your problem and why LoadImage() implemented in U-Boot's efi_load_image() won't work.

Best regards

Heinrich

Re: [PATCH 01/10] Add EFI handover support to bootm

[Simon Glass]

Hi Heinrich,

On Sun, 24 Nov 2024 at 13:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Am 24. November 2024 20:29:22 MEZ schrieb Matthew Garrett <mjg59@srcf.ucam.org>:
> >On Sun, Nov 24, 2024 at 03:43:12PM +0100, Heinrich Schuchardt wrote:
> >> > +  /* 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()?
> >
> >With secure boot that requires that the kernel image have a trusted
> >signature, whereas we're relying on a signed FIT.
>
>
> Please, provide an answer describing your problem and why LoadImage() implemented in U-Boot's efi_load_image() won't work.

efi_load_image() is in the EFI_LOADER. This is the EFI app.

Regards,
Simon

Re: [PATCH 01/10] Add EFI handover support to bootm

[Ilias Apalodimas]

Hi Matthew,

On Sun, 24 Nov 2024 at 21:29, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>
> On Sun, Nov 24, 2024 at 03:43:12PM +0100, Heinrich Schuchardt wrote:
> > > +   /* That failed, so try allocating anywhere there's enough room */
> > > +   status = boot->allocate_pages(EFI_ALLOCATE_ANY_PAGES, EFI_LOADER_DATA, pages, &addr);

I don't think you can use this as is. IIRC the PE/COFF header defines
the alignment of the loaded image that's why we have
efi_alloc_aligned_pages()

> > > +   if (status == EFI_SUCCESS) {
> > > +           /* Make sure bootm knows where we loaded the image */
> > > +           os->load = addr;
> > > +           return;
> > > +   }
> >
> > Why don't you simply call LoadImage()?
>
> With secure boot that requires that the kernel image have a trusted
> signature, whereas we're relying on a signed FIT.

That signed FIT, contains a kernel compiled as a PE/COFF and you
*want* to jump the the efi stub right? If that's the case and we trust
FIT, why don't we just ignore the crypto checks on LoadImage?

Thanks
/Ilias

Re: [PATCH 01/10] Add EFI handover support to bootm

[Simon Glass]

On Sat, 23 Nov 2024 at 12:56, Matthew Garrett <mjg59@srcf.ucam.org> 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(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 01/10] Add EFI handover support to bootm

[Simon Glass]

On Sat, 23 Nov 2024 at 12:56, Matthew Garrett <mjg59@srcf.ucam.org> 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(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to ci/master, thanks!

[PATCH 02/10] Add part_find command

[Matthew Garrett]

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

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"
+);
-- 
2.47.0

Re: [PATCH 02/10] Add part_find command

[Simon Glass]

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

Re: [PATCH 02/10] Add part_find command

[Simon Glass]

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.

>
Applied to ci/master, thanks!

Re: [PATCH 02/10] Add part_find command

[Heinrich Schuchardt]

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"
> +);

Re: [PATCH 02/10] Add part_find command

[Simon Glass]

Hi Heinrich,

On Tue, 10 Dec 2024 at 01:21, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> 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.

I looked at that when sending this series. My only concern is that it
is wildly different from the existing 'part' command, in that it does
special EFI things.

But I don't have any strong opinions so am happy to incorporate it,
with its own Kconfig option, of course.

Regards,
Simon

[PATCH 03/10] Add a command to find a load address

[Matthew Garrett]

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

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);
+	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

Re: [PATCH 03/10] Add a command to find a load address

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1275 bytes --]

On Sat, Nov 23, 2024 at 11:55:02AM -0800, Matthew Garrett 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

This needs to be reworked on top of how lmb works today (which did
change post v2024.10) and looks a lot like what
arch/arm/mach-apple/board.c::board_late_init() was doing, even back in
2022. Do you end up using this functionality on more than one board /
family of boards? The easy path is likely to just follow what mach-apple
does, but the more broad path would be adding an option to configure the
key environment variables that are used as buffers and documented in
doc/develop/bootstd/overview.rst / doc/develop/distro.rst dynamically
rather than statically.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 03/10] Add a command to find a load address

[Simon Glass]

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

Re: [PATCH 03/10] Add a command to find a load address

[Simon Glass]

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/...)

>
Applied to ci/master, thanks!

[PATCH 04/10] Hook up EFI env variable support in the EFI app

[Matthew Garrett]

From: Matthew Garrett <mgarrett@aurora.tech>

Add simple support for accessing EFI variables when in EFI app mode

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

 cmd/Kconfig        |  2 +-
 lib/efi/Makefile   |  2 +-
 lib/efi/efi_app.c  |  5 +++++
 lib/efi/efi_vars.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 51 insertions(+), 2 deletions(-)
 create mode 100644 lib/efi/efi_vars.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 5ed6a50121c..33bf3d1ad39 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -717,7 +717,7 @@ config CMD_ENV_FLAGS
 
 config CMD_NVEDIT_EFI
 	bool "env [set|print] -e - set/print UEFI variables"
-	depends on EFI_LOADER
+	depends on EFI_LOADER || EFI_APP
 	imply HEXDUMP
 	help
 	  UEFI variables are encoded as some form of U-Boot variables.
diff --git a/lib/efi/Makefile b/lib/efi/Makefile
index 232fa684360..63845287336 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
+obj-$(CONFIG_EFI_APP) += efi_app.o efi.o efi_app_init.o efi_vars.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 7c3ef9a7926..77d5da40a5d 100644
--- a/lib/efi/efi_app.c
+++ b/lib/efi/efi_app.c
@@ -30,6 +30,11 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
+int efi_init_obj_list(void)
+{
+	return EFI_SUCCESS;
+}
+
 int efi_info_get(enum efi_entry_t type, void **datap, int *sizep)
 {
 	return -ENOSYS;
diff --git a/lib/efi/efi_vars.c b/lib/efi/efi_vars.c
new file mode 100644
index 00000000000..099a59b2a1e
--- /dev/null
+++ b/lib/efi/efi_vars.c
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Aurora Innovation, Inc. Copyright 2022.
+ *
+ */
+
+#define __efi_runtime
+
+#include <errno.h>
+#include <asm/global_data.h>
+#include <efi.h>
+#include <efi_api.h>
+#include <efi_variable.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+efi_status_t efi_get_variable_int(const u16 *variable_name, const efi_guid_t *vendor,
+				  u32 *attributes, efi_uintn_t *data_size,
+				  void *data, u64 *timep)
+{
+	struct efi_priv *priv = efi_get_priv();
+	struct efi_runtime_services *run = priv->run;
+
+	return run->get_variable((u16 *)variable_name, vendor, attributes, data_size, data);
+}
+
+efi_status_t efi_set_variable_int(const u16 *variable_name, const efi_guid_t *vendor,
+				  u32 attributes, efi_uintn_t data_size, const void *data,
+				  bool ro_check)
+{
+	struct efi_priv *priv = efi_get_priv();
+	struct efi_runtime_services *run = priv->run;
+
+	return run->set_variable((u16 *)variable_name, vendor, attributes, data_size, data);
+}
+
+efi_status_t efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
+					    u16 *variable_name, efi_guid_t *vendor)
+{
+	struct efi_priv *priv = efi_get_priv();
+	struct efi_runtime_services *run = priv->run;
+
+	return run->get_next_variable_name(variable_name_size, variable_name, vendor);
+}
-- 
2.47.0

Re: [PATCH 04/10] Hook up EFI env variable support in the EFI app

[Simon Glass]

Hi Matthew,

On Sat, 23 Nov 2024 at 12:57, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>
> From: Matthew Garrett <mgarrett@aurora.tech>
>
> Add simple support for accessing EFI variables when in EFI app mode
>
> Signed-off-by: Matthew Garrett <mgarrett@aurora.tech>
> ---
>
>  cmd/Kconfig        |  2 +-
>  lib/efi/Makefile   |  2 +-
>  lib/efi/efi_app.c  |  5 +++++
>  lib/efi/efi_vars.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 51 insertions(+), 2 deletions(-)
>  create mode 100644 lib/efi/efi_vars.c
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Again this needs doc/ and test/env

Regards,
Simon

Re: [PATCH 04/10] Hook up EFI env variable support in the EFI app

[Simon Glass]

Hi Matthew,

On Sat, 23 Nov 2024 at 12:57, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>
> From: Matthew Garrett <mgarrett@aurora.tech>
>
> Add simple support for accessing EFI variables when in EFI app mode
>
> Signed-off-by: Matthew Garrett <mgarrett@aurora.tech>
> ---
>
>  cmd/Kconfig        |  2 +-
>  lib/efi/Makefile   |  2 +-
>  lib/efi/efi_app.c  |  5 +++++
>  lib/efi/efi_vars.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 51 insertions(+), 2 deletions(-)
>  create mode 100644 lib/efi/efi_vars.c
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Again this needs doc/ and test/env

Regards,
Simon

Applied to ci/master, thanks!

[PATCH 05/10] Add EFI network driver

[Matthew Garrett]

From: Matthew Garrett <mgarrett@aurora.tech>

Add a driver that makes use of the UEFI Simple Network Protocol to
support network access when using the UEFI app implementation, and hook
up the app code to instantiate it for probed devices.

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

 drivers/net/Kconfig    |   7 +++
 drivers/net/Makefile   |   1 +
 drivers/net/efi_net.c  | 110 +++++++++++++++++++++++++++++++++++++++++
 include/efi.h          |  12 +++++
 lib/efi/efi_app_init.c |  67 +++++++++++++++++++++++++
 5 files changed, 197 insertions(+)
 create mode 100644 drivers/net/efi_net.c

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 576cd2d50ad..693d00fa2e3 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -333,6 +333,13 @@ config EEPRO100
 	  This driver supports Intel(R) PRO/100 82557/82559/82559ER fast
 	  ethernet family of adapters.
 
+config EFI_NET
+        bool "UEFI firmware-based networking support"
+	depends on EFI_APP
+	help
+	  This driver allows the use of UEFI-provided network interfaces
+	  as network devices within U-Boot.
+
 config ESSEDMA
 	bool "Qualcomm ESS Edma support"
 	depends on DM_ETH && ARCH_IPQ40XX
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index f5ab1f5dedf..f58c62cdb40 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_DWC_ETH_QOS_STM32) += dwc_eth_qos_stm32.o
 obj-$(CONFIG_E1000) += e1000.o
 obj-$(CONFIG_E1000_SPI) += e1000_spi.o
 obj-$(CONFIG_EEPRO100) += eepro100.o
+obj-$(CONFIG_EFI_NET) += efi_net.o
 obj-$(CONFIG_ESSEDMA) += essedma.o
 obj-$(CONFIG_ETHOC) += ethoc.o
 obj-$(CONFIG_ETH_DESIGNWARE) += designware.o
diff --git a/drivers/net/efi_net.c b/drivers/net/efi_net.c
new file mode 100644
index 00000000000..370056040f3
--- /dev/null
+++ b/drivers/net/efi_net.c
@@ -0,0 +1,110 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Aurora Innovation, Inc. Copyright 2022.
+ *
+ */
+
+#include <config.h>
+#include <dm.h>
+#include <efi.h>
+#include <efi_api.h>
+#include <malloc.h>
+#include <net.h>
+
+struct efi_net_priv {
+	void *buffer;
+};
+
+static int efi_net_bind(struct udevice *dev)
+{
+	struct efi_net_plat *plat = dev_get_plat(dev);
+	struct efi_net_priv *priv = dev_get_priv(dev);
+	efi_status_t status;
+
+	priv->buffer = malloc(PKTSIZE);
+
+	status = plat->snp->start(plat->snp);
+	if (status != EFI_SUCCESS)
+		return -ENOENT;
+
+	return 0;
+}
+
+static int efi_net_init(struct udevice *dev)
+{
+	struct efi_net_plat *plat = dev_get_plat(dev);
+	efi_status_t status;
+
+	status = plat->snp->initialize(plat->snp, 0, 0);
+	if (status != EFI_SUCCESS)
+		return -ENOENT;
+
+	return 0;
+}
+
+static void efi_net_halt(struct udevice *dev)
+{
+	struct efi_net_plat *plat = dev_get_plat(dev);
+
+	plat->snp->shutdown(plat->snp);
+	plat->snp->stop(plat->snp);
+}
+
+static int efi_net_send(struct udevice *dev, void *packet, int length)
+{
+	struct efi_net_plat *plat = dev_get_plat(dev);
+	efi_status_t status;
+
+	status = plat->snp->transmit(plat->snp, 0, length, packet, NULL, NULL,
+				     NULL);
+	if (status != EFI_SUCCESS)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int efi_net_recv(struct udevice *dev, int flags, uchar **packetp)
+{
+	struct efi_net_plat *plat = dev_get_plat(dev);
+	struct efi_net_priv *priv = dev_get_priv(dev);
+	efi_status_t status;
+	size_t size = PKTSIZE;
+
+	status = plat->snp->receive(plat->snp, 0, &size, priv->buffer, NULL,
+				    NULL, NULL);
+	if (status == EFI_NOT_READY)
+		return -EAGAIN;
+	else if (status != EFI_SUCCESS)
+		return -EINVAL;
+
+	*packetp = priv->buffer;
+
+	return 0;
+}
+
+static int efi_net_read_hwaddr(struct udevice *dev)
+{
+	struct efi_net_plat *plat = dev_get_plat(dev);
+
+	memcpy(plat->eth_pdata.enetaddr,
+	       (void *)&plat->snp->mode->permanent_address, ARP_HLEN);
+
+	return 0;
+}
+
+static const struct eth_ops efi_net_ops = {
+	.start	= efi_net_init,
+	.stop	= efi_net_halt,
+	.send	= efi_net_send,
+	.recv	= efi_net_recv,
+	.read_rom_hwaddr = efi_net_read_hwaddr,
+};
+
+U_BOOT_DRIVER(efi_net) = {
+	.name	= "efi_net",
+	.id	= UCLASS_ETH,
+	.bind	= efi_net_bind,
+	.ops	= &efi_net_ops,
+	.plat_auto = sizeof(struct efi_net_plat),
+	.priv_auto = sizeof(struct efi_net_priv),
+};
diff --git a/include/efi.h b/include/efi.h
index 1d06230439f..3c4c321362f 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -16,6 +16,7 @@
 #ifndef _EFI_H
 #define _EFI_H
 
+#include <net.h>
 #include <linux/linkage.h>
 #include <linux/string.h>
 #include <linux/types.h>
@@ -494,6 +495,17 @@ struct efi_media_plat {
 	struct efi_device_path *device_path;
 };
 
+/*
+ * EFI attributes of the udevice handled by efi_net driver
+ *
+ * @handle: handle of the controller on which this driver is installed
+ */
+struct efi_net_plat {
+	struct eth_pdata eth_pdata;
+	efi_handle_t handle;
+	struct efi_simple_network *snp;
+};
+
 /* Base address of the EFI image */
 extern char image_base[];
 
diff --git a/lib/efi/efi_app_init.c b/lib/efi/efi_app_init.c
index c5e4192fe06..bc09eb063a1 100644
--- a/lib/efi/efi_app_init.c
+++ b/lib/efi/efi_app_init.c
@@ -183,6 +183,70 @@ static int setup_block(void)
 	return 0;
 }
 
+/**
+ * setup_net() - Find all network devices and setup EFI devices for them
+ *
+ * Return: 0 if found, -ENOSYS if there is no boot-services table, -ENOTSUPP
+ *	if a required protocol is not supported
+ */
+static int setup_net(void)
+{
+	efi_guid_t efi_snp_guid = EFI_SIMPLE_NETWORK_PROTOCOL_GUID;
+	struct efi_boot_services *boot = efi_get_boot();
+	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 simple network protocol */
+	ret = boot->locate_handle_buffer(BY_PROTOCOL, &efi_snp_guid, NULL,
+				  &num_handles, &handle);
+
+	if (ret)
+		return 0;
+	log_debug("Found %d net handles:\n", (int)num_handles);
+
+	for (i = 0; i < num_handles; i++) {
+		struct efi_simple_network *snp;
+		struct efi_net_plat *plat;
+		struct udevice *dev;
+		char name[18];
+
+		ret = boot->handle_protocol(handle[i], &efi_snp_guid,
+					    (void **)&snp);
+
+		if (ret != EFI_SUCCESS) {
+			log_warning("- snp %d failed (ret=0x%x\n", i, ret);
+			continue;
+		}
+
+		plat = malloc(sizeof(*plat));
+		if (!plat) {
+			log_warning("- snp %d failed to alloc platform data", i);
+			continue;
+		}
+		plat->handle = handle[i];
+		plat->snp = snp;
+		ret = device_bind(dm_root(), DM_DRIVER_GET(efi_net), "efi_net",
+				  plat, ofnode_null(), &dev);
+		if (ret) {
+			log_warning("- bind snp %d failed (ret=0x%x)\n", i,
+				    ret);
+			continue;
+		}
+
+		snprintf(name, sizeof(name), "efi_net_%x", dev_seq(dev));
+		device_set_name(dev, name);
+
+		printf("%2d: %-12s\n", i, dev->name);
+	}
+	boot->free_pool(handle);
+
+	return 0;
+}
+
 /**
  * board_early_init_r() - Scan for UEFI devices that should be available
  *
@@ -199,6 +263,9 @@ int board_early_init_r(void)
 		ret = setup_block();
 		if (ret)
 			return ret;
+		ret = setup_net();
+		if (ret)
+			return ret;
 	}
 
 	return 0;
-- 
2.47.0

Re: [PATCH 05/10] Add EFI network driver

[Simon Glass]

On Sat, 23 Nov 2024 at 12:57, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>
> From: Matthew Garrett <mgarrett@aurora.tech>
>
> Add a driver that makes use of the UEFI Simple Network Protocol to
> support network access when using the UEFI app implementation, and hook
> up the app code to instantiate it for probed devices.
>
> Signed-off-by: Matthew Garrett <mgarrett@aurora.tech>
> ---
>
>  drivers/net/Kconfig    |   7 +++
>  drivers/net/Makefile   |   1 +
>  drivers/net/efi_net.c  | 110 +++++++++++++++++++++++++++++++++++++++++
>  include/efi.h          |  12 +++++
>  lib/efi/efi_app_init.c |  67 +++++++++++++++++++++++++
>  5 files changed, 197 insertions(+)
>  create mode 100644 drivers/net/efi_net.c

Reviewed-by: Simon Glass <sjg@chromium.org>

You don't need to include config.h anymore

Re: [PATCH 05/10] Add EFI network driver

[Simon Glass]

On Sat, 23 Nov 2024 at 12:57, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>
> From: Matthew Garrett <mgarrett@aurora.tech>
>
> Add a driver that makes use of the UEFI Simple Network Protocol to
> support network access when using the UEFI app implementation, and hook
> up the app code to instantiate it for probed devices.
>
> Signed-off-by: Matthew Garrett <mgarrett@aurora.tech>
> ---
>
>  drivers/net/Kconfig    |   7 +++
>  drivers/net/Makefile   |   1 +
>  drivers/net/efi_net.c  | 110 +++++++++++++++++++++++++++++++++++++++++
>  include/efi.h          |  12 +++++
>  lib/efi/efi_app_init.c |  67 +++++++++++++++++++++++++
>  5 files changed, 197 insertions(+)
>  create mode 100644 drivers/net/efi_net.c

Reviewed-by: Simon Glass <sjg@chromium.org>

You don't need to include config.h anymore

Applied to ci/master, thanks!

[PATCH 06/10] Add UEFI TPM2 driver

[Matthew Garrett]

From: Matthew Garrett <mgarrett@aurora.tech>

Add support for driving a TPM via UEFI firmware provided drivers, and
bind those devices from the UEFI app.

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

 drivers/tpm/Kconfig    |  7 +++
 drivers/tpm/Makefile   |  1 +
 drivers/tpm/tpm2_efi.c | 97 ++++++++++++++++++++++++++++++++++++++++++
 include/efi.h          | 11 +++++
 include/efi_tcg2.h     |  1 +
 lib/efi/efi_app_init.c | 69 ++++++++++++++++++++++++++++++
 6 files changed, 186 insertions(+)
 create mode 100644 drivers/tpm/tpm2_efi.c

diff --git a/drivers/tpm/Kconfig b/drivers/tpm/Kconfig
index d59102d9a6b..36546c2c00e 100644
--- a/drivers/tpm/Kconfig
+++ b/drivers/tpm/Kconfig
@@ -209,6 +209,13 @@ config TPM2_MMIO
 	  to the device using the standard TPM Interface Specification (TIS)
 	  protocol.
 
+config TPM2_EFI
+        bool "UEFI firmware based TPM2 Interface"
+	depends on TPM_V2 && EFI_APP
+	help
+	  This driver supports the use of UEFI firmware-provided drivers for
+	  interfacing with a TPM 2.
+
 endif # TPM_V2
 
 endmenu
diff --git a/drivers/tpm/Makefile b/drivers/tpm/Makefile
index 76e516dbbaf..4b7da33e660 100644
--- a/drivers/tpm/Makefile
+++ b/drivers/tpm/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_TPM2_TIS_SPI) += tpm2_tis_core.o tpm2_tis_spi.o
 obj-$(CONFIG_TPM2_TIS_I2C) += tpm2_tis_core.o tpm2_tis_i2c.o
 obj-$(CONFIG_TPM2_FTPM_TEE) += tpm2_ftpm_tee.o
 obj-$(CONFIG_TPM2_MMIO) += tpm2_tis_core.o tpm2_tis_mmio.o
+obj-$(CONFIG_TPM2_EFI) += tpm2_efi.o
diff --git a/drivers/tpm/tpm2_efi.c b/drivers/tpm/tpm2_efi.c
new file mode 100644
index 00000000000..2eb144891d8
--- /dev/null
+++ b/drivers/tpm/tpm2_efi.c
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Aurora Innovation, Inc. Copyright 2022.
+ *
+ */
+
+#include <config.h>
+#include <dm.h>
+#include <efi.h>
+#include <efi_api.h>
+#include <efi_tcg2.h>
+#include <malloc.h>
+#include <tpm-v2.h>
+
+static int efi_tpm_bind(struct udevice *dev)
+{
+	struct tpm_chip_priv *priv = dev_get_uclass_priv(dev);
+	struct efi_tpm_plat *plat = dev_get_plat(dev);
+	struct efi_tcg2_boot_service_capability caps;
+	efi_status_t status;
+
+	caps.size = sizeof(caps);
+	status = plat->proto->get_capability(plat->proto, &caps);
+	if (status != EFI_SUCCESS)
+		return -EINVAL;
+
+	if (!caps.tpm_present_flag)
+		return -ENODEV;
+
+	priv->pcr_count = 24;
+	priv->pcr_select_min = 3;
+	priv->version = TPM_V2;
+
+	return 0;
+}
+
+static int efi_tpm_open(struct udevice *dev)
+{
+	return 0;
+}
+
+static int efi_tpm_close(struct udevice *dev)
+{
+	return 0;
+}
+
+static int efi_tpm_xfer(struct udevice *dev, const u8 *sendbuf,
+			size_t send_size, u8 *recvbuf,
+			size_t *recv_len)
+{
+	struct efi_tpm_plat *plat = dev_get_plat(dev);
+	efi_status_t status;
+
+	status = plat->proto->submit_command(plat->proto, send_size,
+					     (u8 *)sendbuf, *recv_len,
+					     recvbuf);
+	switch (status) {
+	case EFI_BUFFER_TOO_SMALL:
+		debug("%s:response length is bigger than receive buffer\n",
+		      __func__);
+		return -EIO;
+	case EFI_DEVICE_ERROR:
+		debug("%s:received error from device on write\n", __func__);
+		return -EIO;
+	case EFI_INVALID_PARAMETER:
+		debug("%s:invalid parameter\n", __func__);
+		return -EINVAL;
+	case EFI_SUCCESS:
+		return 0;
+	default:
+		debug("%s:received unknown error 0x%lx\n", __func__, status);
+		return -EIO;
+	}
+}
+
+static int efi_tpm_desc(struct udevice *dev, char *buf, int size)
+{
+	if (size < 9)
+		return -ENOSPC;
+
+	return snprintf(buf, size, "UEFI TPM");
+}
+
+static const struct tpm_ops efi_tpm_ops = {
+	.open		= efi_tpm_open,
+	.close		= efi_tpm_close,
+	.get_desc	= efi_tpm_desc,
+	.xfer		= efi_tpm_xfer,
+};
+
+U_BOOT_DRIVER(efi_tpm) = {
+	.name	= "efi_tpm",
+	.id	= UCLASS_TPM,
+	.bind	= efi_tpm_bind,
+	.ops	= &efi_tpm_ops,
+	.plat_auto = sizeof(struct efi_tpm_plat),
+};
diff --git a/include/efi.h b/include/efi.h
index 3c4c321362f..2eb9556770d 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -506,6 +506,17 @@ struct efi_net_plat {
 	struct efi_simple_network *snp;
 };
 
+/*
+ * EFI attributes of the udevice handled by efi_tpm driver
+ *
+ * @handle: handle of the controller on which this driver is installed
+ * @proto: pointer to the TCG2 EFI protocol
+ */
+struct efi_tpm_plat {
+	efi_handle_t handle;
+	struct efi_tcg2_protocol *proto;
+};
+
 /* Base address of the EFI image */
 extern char image_base[];
 
diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
index 8dfb1bc9527..7dff89722bc 100644
--- a/include/efi_tcg2.h
+++ b/include/efi_tcg2.h
@@ -17,6 +17,7 @@
 #define _EFI_TCG2_PROTOCOL_H_
 
 #include <efi_api.h>
+#include <part_efi.h>
 #include <tpm-v2.h>
 #include <tpm_tcg2.h>
 
diff --git a/lib/efi/efi_app_init.c b/lib/efi/efi_app_init.c
index bc09eb063a1..9704020b749 100644
--- a/lib/efi/efi_app_init.c
+++ b/lib/efi/efi_app_init.c
@@ -9,6 +9,7 @@
 #include <dm.h>
 #include <efi.h>
 #include <efi_api.h>
+#include <efi_tcg2.h>
 #include <errno.h>
 #include <malloc.h>
 #include <asm/global_data.h>
@@ -247,6 +248,71 @@ static int setup_net(void)
 	return 0;
 }
 
+/**
+ * setup_tpm() - Find all TPMs and setup EFI devices for them
+ *
+ * Return: 0 if found, -ENOSYS if there is no boot-services table, -ENOTSUPP
+ *	if a required protocol is not supported
+ */
+static int setup_tpm(void)
+{
+	efi_guid_t efi_tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
+	struct efi_boot_services *boot = efi_get_boot();
+	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 TCG2 protocol */
+	ret = boot->locate_handle_buffer(BY_PROTOCOL, &efi_tcg2_guid, NULL,
+				  &num_handles, &handle);
+
+	if (ret)
+		return 0;
+	log_debug("Found %d TPM handles:\n", (int)num_handles);
+
+	for (i = 0; i < num_handles; i++) {
+		struct efi_tcg2_protocol *proto;
+		struct efi_tpm_plat *plat;
+		struct udevice *dev;
+		char name[18];
+
+		ret = boot->handle_protocol(handle[i], &efi_tcg2_guid,
+					    (void **)&proto);
+
+		if (ret != EFI_SUCCESS) {
+			log_warning("- TPM %d failed (ret=0x%x)\n", i, ret);
+			continue;
+		}
+
+		plat = malloc(sizeof(*plat));
+		if (!plat) {
+			log_warning("- TPM %d failed to alloc platform data", i);
+			continue;
+		}
+
+		plat->handle = handle[i];
+		plat->proto = proto;
+		ret = device_bind(dm_root(), DM_DRIVER_GET(efi_net), "efi_tpm",
+				  plat, ofnode_null(), &dev);
+		if (ret) {
+			log_warning("- bind TPM %d failed (ret=0x%x)\n", i,
+				    ret);
+			continue;
+		}
+
+		snprintf(name, sizeof(name), "efi_tpm_%x", dev_seq(dev));
+		device_set_name(dev, name);
+
+		printf("%2d: %-12s\n", i, dev->name);
+	}
+	boot->free_pool(handle);
+
+	return 0;
+}
+
 /**
  * board_early_init_r() - Scan for UEFI devices that should be available
  *
@@ -266,6 +332,9 @@ int board_early_init_r(void)
 		ret = setup_net();
 		if (ret)
 			return ret;
+		ret = setup_tpm();
+		if (ret)
+			return ret;
 	}
 
 	return 0;
-- 
2.47.0

Re: [PATCH 06/10] Add UEFI TPM2 driver

[Simon Glass]

On Sat, 23 Nov 2024 at 12:57, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>
> From: Matthew Garrett <mgarrett@aurora.tech>
>
> Add support for driving a TPM via UEFI firmware provided drivers, and
> bind those devices from the UEFI app.
>
> Signed-off-by: Matthew Garrett <mgarrett@aurora.tech>
> ---
>
>  drivers/tpm/Kconfig    |  7 +++
>  drivers/tpm/Makefile   |  1 +
>  drivers/tpm/tpm2_efi.c | 97 ++++++++++++++++++++++++++++++++++++++++++
>  include/efi.h          | 11 +++++
>  include/efi_tcg2.h     |  1 +
>  lib/efi/efi_app_init.c | 69 ++++++++++++++++++++++++++++++
>  6 files changed, 186 insertions(+)
>  create mode 100644 drivers/tpm/tpm2_efi.c
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 06/10] Add UEFI TPM2 driver

[Simon Glass]

On Sat, 23 Nov 2024 at 12:57, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>
> From: Matthew Garrett <mgarrett@aurora.tech>
>
> Add support for driving a TPM via UEFI firmware provided drivers, and
> bind those devices from the UEFI app.
>
> Signed-off-by: Matthew Garrett <mgarrett@aurora.tech>
> ---
>
>  drivers/tpm/Kconfig    |  7 +++
>  drivers/tpm/Makefile   |  1 +
>  drivers/tpm/tpm2_efi.c | 97 ++++++++++++++++++++++++++++++++++++++++++
>  include/efi.h          | 11 +++++
>  include/efi_tcg2.h     |  1 +
>  lib/efi/efi_app_init.c | 69 ++++++++++++++++++++++++++++++
>  6 files changed, 186 insertions(+)
>  create mode 100644 drivers/tpm/tpm2_efi.c
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to ci/master, thanks!

[PATCH 07/10] Support separate DTB files with the UEFI app

[Matthew Garrett]

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
+	/* FDT is in a separate section */
+	fdt_blob = (ulong *)__dtb;
 #else
 	/* FDT is at end of image */
 	fdt_blob = (ulong *)_end;
-- 
2.47.0

Re: [PATCH 07/10] Support separate DTB files with the UEFI app

[Ilias Apalodimas]

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

Re: [PATCH 07/10] Support separate DTB files with the UEFI app

[Simon Glass]

On Sat, 23 Nov 2024 at 12: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

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 07/10] Support separate DTB files with the UEFI app

[Simon Glass]

On Sat, 23 Nov 2024 at 12: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

Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to ci/master, thanks!

[PATCH 08/10] Use the correct ramdisk address

[Matthew Garrett]

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(-)

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

Re: [PATCH 08/10] Use the correct ramdisk address

[Simon Glass]

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
>

Re: [PATCH 08/10] Use the correct ramdisk address

[Simon Glass]

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>


>
Applied to ci/master, thanks!

[PATCH 09/10] Fix efi_bind_block.

[Matthew Garrett]

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);
 
-- 
2.47.0

Re: [PATCH 09/10] Fix efi_bind_block.

[Ilias Apalodimas]

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.

> +
>  /**
>   * 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

>                 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
>

Thanks
/Ilias

Re: [PATCH 09/10] Fix efi_bind_block.

[Simon Glass]

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

Re: [PATCH 09/10] Fix efi_bind_block.

[Simon Glass]

On Sat, 23 Nov 2024 at 12: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.
> 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(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 09/10] Fix efi_bind_block.

[Simon Glass]

On Sat, 23 Nov 2024 at 12: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.
> 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(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to ci/master, thanks!

Re: [PATCH 09/10] Fix efi_bind_block.

[Heinrich Schuchardt]

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.

Best regards

Heinrich

Re: [PATCH 09/10] Fix efi_bind_block.

[Simon Glass]

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

Re: [PATCH 09/10] Fix efi_bind_block.

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3929 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 09/10] Fix efi_bind_block.

[Janis Danisevskis]

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
>

Re: [PATCH 09/10] Fix efi_bind_block.

[Ilias Apalodimas]

Hi Janis

On Wed, 11 Dec 2024 at 19:50, Janis Danisevskis
<jdanisevskis@aurora.tech> wrote:
>
> Hi everyone,
>
> Good catch finding this leak. Would you like me to provide an updated patch?

There's a confusion here let me try to explain. I generally think the
patchset is useful and I'd like us to pull it. But we had some review
comments, Simon pulled the patch in his own private tree but it's not
in U-Boot -master or -next -- apologies for any confusion.

If you can go through the comments and let us know, I can work with
you to fix the remaining issues.

Thanks!
/Ilias
> I am good either way. Let Simon take credit for the fix on the git history. I can live with
> shame :-).

We all have our special moments of shame, it's a public ML :)


>
> 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

Re: [PATCH 09/10] Fix efi_bind_block.

[Simon Glass]

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

Re: [PATCH 09/10] Fix efi_bind_block.

[Janis Danisevskis]

[-- Attachment #1: Type: text/plain, Size: 5924 bytes --]

Here is an updated version of the patch.

Please bear with me while I learn this email based code review process.
I am trying to find resources on this, but there are a lot and I don't know
what applies here.

Anyway, I believe there was another leak. If the call to device_bind fails
both plat and plat->device_path need to be freed.
I think I captured this in the new version.

The other open issue was the duplication of efi_dp_size code. But if I
understand correctly
this was not available in the configuration that we are using. Not sure
what the path forward should be here.

With kind regards,
Janis

On Wed, Dec 11, 2024 at 12:23 PM Simon Glass <sjg@chromium.org> wrote:

> 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
>

[-- Attachment #2: 0001-Fix-efi_bind_block.patch --]
[-- Type: application/x-patch, Size: 2829 bytes --]

[PATCH 10/10] Add command to set an environment variable to an EFI variable

[Matthew Garrett]

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);
+	}
+
+	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"
+);
-- 
2.47.0

Re: [PATCH 10/10] Add command to set an environment variable to an EFI variable

[Heinrich Schuchardt]

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.

Best regards

Heinrich

> +	}
> +
> +	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"
> +);

Re: [PATCH 10/10] Add command to set an environment variable to an EFI variable

[Simon Glass]

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

Re: [PATCH 10/10] Add command to set an environment variable to an EFI variable

[Simon Glass]

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
> >
Applied to ci/master, thanks!

Re: [PATCH 10/10] Add command to set an environment variable to an EFI variable

[Simon Glass]

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

Re: [PATCH 00/10] Improve UEFI app support

[Heinrich Schuchardt]

On 11/23/24 20:54, Matthew Garrett wrote:
> We run a mixed environment including traditional embedded Linux and
> UEFI. For consistency it's valuable to use U-Boot in both cases, but on
> UEFI systems we want to run the full Linux kernel UEFI boot stub. This
> patchset adds support for that, along with various other quality-of-life
> improvements such as automatically finding partitions based on their
> GUID, automatic identification of load addresses where the firmware has
> given us constraints, support for EFI variable access in the EFI app,
> drivers for EFI network adapters and TPMs, support for embedding DTBs in
> the EFI app, and a couple of bug fixes.

Hello Matthew,

Unfortunately from what you write I do not understand what you are missing.

We already have the bootefi command to execute EFI applications
including the EFI stub.

We have support for FIT images including an EFI kernel and a
device-tree. Initrd support could be added here.

Embedding DTBs into the EFI app is something you should implement in the
EFI stub. The EFI stub should invoke the EFI_DT_FIXUP_PROTOCOL after
choosing the device-tree.

Could you, please, elaborate a bit more on you target.

>
>
> Janis Danisevskis (1):
>    Fix efi_bind_block.
>
> Matthew Garrett (9):
>    Add EFI handover support to bootm

This already exists if you enable CONFIG_BOOTM_EFI.

Best regards

Heinrich

>    Add part_find command
>    Add a command to find a load address
>    Hook up EFI env variable support in the EFI app
>    Add EFI network driver
>    Add UEFI TPM2 driver
>    Support separate DTB files with the UEFI app
>    Use the correct ramdisk address
>    Add command to set an environment variable to an EFI variable
>
>   Makefile                        |   7 +-
>   arch/x86/config.mk              |   2 +-
>   arch/x86/lib/bootm.c            |  60 ++++++++----
>   arch/x86/lib/elf_x86_64_efi.lds |   4 +
>   boot/bootm.c                    |   5 +
>   cmd/Kconfig                     |  23 ++++-
>   cmd/Makefile                    |   3 +
>   cmd/addr_find.c                 |  87 +++++++++++++++++
>   cmd/efigetenv.c                 | 133 ++++++++++++++++++++++++++
>   cmd/part_find.c                 | 156 ++++++++++++++++++++++++++++++
>   drivers/net/Kconfig             |   7 ++
>   drivers/net/Makefile            |   1 +
>   drivers/net/efi_net.c           | 110 +++++++++++++++++++++
>   drivers/tpm/Kconfig             |   7 ++
>   drivers/tpm/Makefile            |   1 +
>   drivers/tpm/tpm2_efi.c          |  97 +++++++++++++++++++
>   include/asm-generic/sections.h  |   1 +
>   include/bootm.h                 |   6 ++
>   include/efi.h                   |  24 +++++
>   include/efi_tcg2.h              |   1 +
>   lib/efi/Makefile                |   2 +-
>   lib/efi/efi.c                   |   1 +
>   lib/efi/efi_app.c               |  41 ++++++++
>   lib/efi/efi_app_init.c          | 163 ++++++++++++++++++++++++++++++--
>   lib/efi/efi_dtb.S               |   6 ++
>   lib/efi/efi_vars.c              |  44 +++++++++
>   lib/fdtdec.c                    |   3 +
>   27 files changed, 967 insertions(+), 28 deletions(-)
>   create mode 100644 cmd/addr_find.c
>   create mode 100644 cmd/efigetenv.c
>   create mode 100644 cmd/part_find.c
>   create mode 100644 drivers/net/efi_net.c
>   create mode 100644 drivers/tpm/tpm2_efi.c
>   create mode 100644 lib/efi/efi_dtb.S
>   create mode 100644 lib/efi/efi_vars.c
>

Re: [PATCH 00/10] Improve UEFI app support

[Simon Glass]

Hi Matthew,

On Sat, 23 Nov 2024 at 12:56, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>
> We run a mixed environment including traditional embedded Linux and
> UEFI. For consistency it's valuable to use U-Boot in both cases, but on
> UEFI systems we want to run the full Linux kernel UEFI boot stub. This
> patchset adds support for that, along with various other quality-of-life
> improvements such as automatically finding partitions based on their
> GUID, automatic identification of load addresses where the firmware has
> given us constraints, support for EFI variable access in the EFI app,
> drivers for EFI network adapters and TPMs, support for embedding DTBs in
> the EFI app, and a couple of bug fixes.
>
>
> Janis Danisevskis (1):
>   Fix efi_bind_block.
>
> Matthew Garrett (9):
>   Add EFI handover support to bootm
>   Add part_find command
>   Add a command to find a load address
>   Hook up EFI env variable support in the EFI app
>   Add EFI network driver
>   Add UEFI TPM2 driver
>   Support separate DTB files with the UEFI app
>   Use the correct ramdisk address
>   Add command to set an environment variable to an EFI variable
>
>  Makefile                        |   7 +-
>  arch/x86/config.mk              |   2 +-
>  arch/x86/lib/bootm.c            |  60 ++++++++----
>  arch/x86/lib/elf_x86_64_efi.lds |   4 +
>  boot/bootm.c                    |   5 +
>  cmd/Kconfig                     |  23 ++++-
>  cmd/Makefile                    |   3 +
>  cmd/addr_find.c                 |  87 +++++++++++++++++
>  cmd/efigetenv.c                 | 133 ++++++++++++++++++++++++++
>  cmd/part_find.c                 | 156 ++++++++++++++++++++++++++++++
>  drivers/net/Kconfig             |   7 ++
>  drivers/net/Makefile            |   1 +
>  drivers/net/efi_net.c           | 110 +++++++++++++++++++++
>  drivers/tpm/Kconfig             |   7 ++
>  drivers/tpm/Makefile            |   1 +
>  drivers/tpm/tpm2_efi.c          |  97 +++++++++++++++++++
>  include/asm-generic/sections.h  |   1 +
>  include/bootm.h                 |   6 ++
>  include/efi.h                   |  24 +++++
>  include/efi_tcg2.h              |   1 +
>  lib/efi/Makefile                |   2 +-
>  lib/efi/efi.c                   |   1 +
>  lib/efi/efi_app.c               |  41 ++++++++
>  lib/efi/efi_app_init.c          | 163 ++++++++++++++++++++++++++++++--
>  lib/efi/efi_dtb.S               |   6 ++
>  lib/efi/efi_vars.c              |  44 +++++++++
>  lib/fdtdec.c                    |   3 +
>  27 files changed, 967 insertions(+), 28 deletions(-)
>  create mode 100644 cmd/addr_find.c
>  create mode 100644 cmd/efigetenv.c
>  create mode 100644 cmd/part_find.c
>  create mode 100644 drivers/net/efi_net.c
>  create mode 100644 drivers/tpm/tpm2_efi.c
>  create mode 100644 lib/efi/efi_dtb.S
>  create mode 100644 lib/efi/efi_vars.c
>
> --
> 2.47.0
>

Thank you for sending this and for your interesting talk at plumbers.
It is a novel approach and potentially provides an alternative to grub
and EFI_LOADER, etc.

I'd like to try getting this running in CI as we don't have any such
tests for the app or payload at present.

Regards,
Simon

Re: [PATCH 00/10] Improve UEFI app support

[Simon Glass]

Hi,

On Sun, 1 Dec 2024 at 09:15, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Matthew,
>
> On Sat, 23 Nov 2024 at 12:56, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> >
> > We run a mixed environment including traditional embedded Linux and
> > UEFI. For consistency it's valuable to use U-Boot in both cases, but on
> > UEFI systems we want to run the full Linux kernel UEFI boot stub. This
> > patchset adds support for that, along with various other quality-of-life
> > improvements such as automatically finding partitions based on their
> > GUID, automatic identification of load addresses where the firmware has
> > given us constraints, support for EFI variable access in the EFI app,
> > drivers for EFI network adapters and TPMs, support for embedding DTBs in
> > the EFI app, and a couple of bug fixes.
> >
> >
> > Janis Danisevskis (1):
> >   Fix efi_bind_block.
> >
> > Matthew Garrett (9):
> >   Add EFI handover support to bootm
> >   Add part_find command
> >   Add a command to find a load address
> >   Hook up EFI env variable support in the EFI app
> >   Add EFI network driver
> >   Add UEFI TPM2 driver
> >   Support separate DTB files with the UEFI app
> >   Use the correct ramdisk address
> >   Add command to set an environment variable to an EFI variable
> >
> >  Makefile                        |   7 +-
> >  arch/x86/config.mk              |   2 +-
> >  arch/x86/lib/bootm.c            |  60 ++++++++----
> >  arch/x86/lib/elf_x86_64_efi.lds |   4 +
> >  boot/bootm.c                    |   5 +
> >  cmd/Kconfig                     |  23 ++++-
> >  cmd/Makefile                    |   3 +
> >  cmd/addr_find.c                 |  87 +++++++++++++++++
> >  cmd/efigetenv.c                 | 133 ++++++++++++++++++++++++++
> >  cmd/part_find.c                 | 156 ++++++++++++++++++++++++++++++
> >  drivers/net/Kconfig             |   7 ++
> >  drivers/net/Makefile            |   1 +
> >  drivers/net/efi_net.c           | 110 +++++++++++++++++++++
> >  drivers/tpm/Kconfig             |   7 ++
> >  drivers/tpm/Makefile            |   1 +
> >  drivers/tpm/tpm2_efi.c          |  97 +++++++++++++++++++
> >  include/asm-generic/sections.h  |   1 +
> >  include/bootm.h                 |   6 ++
> >  include/efi.h                   |  24 +++++
> >  include/efi_tcg2.h              |   1 +
> >  lib/efi/Makefile                |   2 +-
> >  lib/efi/efi.c                   |   1 +
> >  lib/efi/efi_app.c               |  41 ++++++++
> >  lib/efi/efi_app_init.c          | 163 ++++++++++++++++++++++++++++++--
> >  lib/efi/efi_dtb.S               |   6 ++
> >  lib/efi/efi_vars.c              |  44 +++++++++
> >  lib/fdtdec.c                    |   3 +
> >  27 files changed, 967 insertions(+), 28 deletions(-)
> >  create mode 100644 cmd/addr_find.c
> >  create mode 100644 cmd/efigetenv.c
> >  create mode 100644 cmd/part_find.c
> >  create mode 100644 drivers/net/efi_net.c
> >  create mode 100644 drivers/tpm/tpm2_efi.c
> >  create mode 100644 lib/efi/efi_dtb.S
> >  create mode 100644 lib/efi/efi_vars.c
> >
> > --
> > 2.47.0
> >
>
> Thank you for sending this and for your interesting talk at plumbers.
> It is a novel approach and potentially provides an alternative to grub
> and EFI_LOADER, etc.
>
> I'd like to try getting this running in CI as we don't have any such
> tests for the app or payload at present.

I'm planning to pull this in as this seems like an important use case.
I'm not sure how much time Matthew will have for this, and I want to
avoid this series going stale.

The comments which are not 'why do you want to do this?' are fairly
minor, so I'll send a follow-up to resolve them next week. We should
also think about what code we could move from lib/efi_loader to
lib/efi so we can share it with the app.

Matthew, thanks again for sending this. if you are planning to do more
here in the short term, please let me know.

Regards,
Simon

Re: [PATCH 00/10] Improve UEFI app support

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4539 bytes --]

On Sun, Dec 08, 2024 at 08:28:42AM -0700, Simon Glass wrote:
> Hi,
> 
> On Sun, 1 Dec 2024 at 09:15, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Matthew,
> >
> > On Sat, 23 Nov 2024 at 12:56, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > >
> > > We run a mixed environment including traditional embedded Linux and
> > > UEFI. For consistency it's valuable to use U-Boot in both cases, but on
> > > UEFI systems we want to run the full Linux kernel UEFI boot stub. This
> > > patchset adds support for that, along with various other quality-of-life
> > > improvements such as automatically finding partitions based on their
> > > GUID, automatic identification of load addresses where the firmware has
> > > given us constraints, support for EFI variable access in the EFI app,
> > > drivers for EFI network adapters and TPMs, support for embedding DTBs in
> > > the EFI app, and a couple of bug fixes.
> > >
> > >
> > > Janis Danisevskis (1):
> > >   Fix efi_bind_block.
> > >
> > > Matthew Garrett (9):
> > >   Add EFI handover support to bootm
> > >   Add part_find command
> > >   Add a command to find a load address
> > >   Hook up EFI env variable support in the EFI app
> > >   Add EFI network driver
> > >   Add UEFI TPM2 driver
> > >   Support separate DTB files with the UEFI app
> > >   Use the correct ramdisk address
> > >   Add command to set an environment variable to an EFI variable
> > >
> > >  Makefile                        |   7 +-
> > >  arch/x86/config.mk              |   2 +-
> > >  arch/x86/lib/bootm.c            |  60 ++++++++----
> > >  arch/x86/lib/elf_x86_64_efi.lds |   4 +
> > >  boot/bootm.c                    |   5 +
> > >  cmd/Kconfig                     |  23 ++++-
> > >  cmd/Makefile                    |   3 +
> > >  cmd/addr_find.c                 |  87 +++++++++++++++++
> > >  cmd/efigetenv.c                 | 133 ++++++++++++++++++++++++++
> > >  cmd/part_find.c                 | 156 ++++++++++++++++++++++++++++++
> > >  drivers/net/Kconfig             |   7 ++
> > >  drivers/net/Makefile            |   1 +
> > >  drivers/net/efi_net.c           | 110 +++++++++++++++++++++
> > >  drivers/tpm/Kconfig             |   7 ++
> > >  drivers/tpm/Makefile            |   1 +
> > >  drivers/tpm/tpm2_efi.c          |  97 +++++++++++++++++++
> > >  include/asm-generic/sections.h  |   1 +
> > >  include/bootm.h                 |   6 ++
> > >  include/efi.h                   |  24 +++++
> > >  include/efi_tcg2.h              |   1 +
> > >  lib/efi/Makefile                |   2 +-
> > >  lib/efi/efi.c                   |   1 +
> > >  lib/efi/efi_app.c               |  41 ++++++++
> > >  lib/efi/efi_app_init.c          | 163 ++++++++++++++++++++++++++++++--
> > >  lib/efi/efi_dtb.S               |   6 ++
> > >  lib/efi/efi_vars.c              |  44 +++++++++
> > >  lib/fdtdec.c                    |   3 +
> > >  27 files changed, 967 insertions(+), 28 deletions(-)
> > >  create mode 100644 cmd/addr_find.c
> > >  create mode 100644 cmd/efigetenv.c
> > >  create mode 100644 cmd/part_find.c
> > >  create mode 100644 drivers/net/efi_net.c
> > >  create mode 100644 drivers/tpm/tpm2_efi.c
> > >  create mode 100644 lib/efi/efi_dtb.S
> > >  create mode 100644 lib/efi/efi_vars.c
> > >
> > > --
> > > 2.47.0
> > >
> >
> > Thank you for sending this and for your interesting talk at plumbers.
> > It is a novel approach and potentially provides an alternative to grub
> > and EFI_LOADER, etc.
> >
> > I'd like to try getting this running in CI as we don't have any such
> > tests for the app or payload at present.
> 
> I'm planning to pull this in as this seems like an important use case.
> I'm not sure how much time Matthew will have for this, and I want to
> avoid this series going stale.
> 
> The comments which are not 'why do you want to do this?' are fairly
> minor, so I'll send a follow-up to resolve them next week. We should
> also think about what code we could move from lib/efi_loader to
> lib/efi so we can share it with the app.
> 
> Matthew, thanks again for sending this. if you are planning to do more
> here in the short term, please let me know.

Please don't grab other peoples patch series and pick them up for your,
well, whatever you want to call "ci/master". You can do what you like
with patches you wrote but taking submissions from others is just going
to confuse the situation as to if you are, or are not, forking the
project at this point.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 00/10] Improve UEFI app support

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4875 bytes --]

On Sun, Dec 08, 2024 at 09:51:23AM -0600, Tom Rini wrote:
> On Sun, Dec 08, 2024 at 08:28:42AM -0700, Simon Glass wrote:
> > Hi,
> > 
> > On Sun, 1 Dec 2024 at 09:15, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Matthew,
> > >
> > > On Sat, 23 Nov 2024 at 12:56, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > > >
> > > > We run a mixed environment including traditional embedded Linux and
> > > > UEFI. For consistency it's valuable to use U-Boot in both cases, but on
> > > > UEFI systems we want to run the full Linux kernel UEFI boot stub. This
> > > > patchset adds support for that, along with various other quality-of-life
> > > > improvements such as automatically finding partitions based on their
> > > > GUID, automatic identification of load addresses where the firmware has
> > > > given us constraints, support for EFI variable access in the EFI app,
> > > > drivers for EFI network adapters and TPMs, support for embedding DTBs in
> > > > the EFI app, and a couple of bug fixes.
> > > >
> > > >
> > > > Janis Danisevskis (1):
> > > >   Fix efi_bind_block.
> > > >
> > > > Matthew Garrett (9):
> > > >   Add EFI handover support to bootm
> > > >   Add part_find command
> > > >   Add a command to find a load address
> > > >   Hook up EFI env variable support in the EFI app
> > > >   Add EFI network driver
> > > >   Add UEFI TPM2 driver
> > > >   Support separate DTB files with the UEFI app
> > > >   Use the correct ramdisk address
> > > >   Add command to set an environment variable to an EFI variable
> > > >
> > > >  Makefile                        |   7 +-
> > > >  arch/x86/config.mk              |   2 +-
> > > >  arch/x86/lib/bootm.c            |  60 ++++++++----
> > > >  arch/x86/lib/elf_x86_64_efi.lds |   4 +
> > > >  boot/bootm.c                    |   5 +
> > > >  cmd/Kconfig                     |  23 ++++-
> > > >  cmd/Makefile                    |   3 +
> > > >  cmd/addr_find.c                 |  87 +++++++++++++++++
> > > >  cmd/efigetenv.c                 | 133 ++++++++++++++++++++++++++
> > > >  cmd/part_find.c                 | 156 ++++++++++++++++++++++++++++++
> > > >  drivers/net/Kconfig             |   7 ++
> > > >  drivers/net/Makefile            |   1 +
> > > >  drivers/net/efi_net.c           | 110 +++++++++++++++++++++
> > > >  drivers/tpm/Kconfig             |   7 ++
> > > >  drivers/tpm/Makefile            |   1 +
> > > >  drivers/tpm/tpm2_efi.c          |  97 +++++++++++++++++++
> > > >  include/asm-generic/sections.h  |   1 +
> > > >  include/bootm.h                 |   6 ++
> > > >  include/efi.h                   |  24 +++++
> > > >  include/efi_tcg2.h              |   1 +
> > > >  lib/efi/Makefile                |   2 +-
> > > >  lib/efi/efi.c                   |   1 +
> > > >  lib/efi/efi_app.c               |  41 ++++++++
> > > >  lib/efi/efi_app_init.c          | 163 ++++++++++++++++++++++++++++++--
> > > >  lib/efi/efi_dtb.S               |   6 ++
> > > >  lib/efi/efi_vars.c              |  44 +++++++++
> > > >  lib/fdtdec.c                    |   3 +
> > > >  27 files changed, 967 insertions(+), 28 deletions(-)
> > > >  create mode 100644 cmd/addr_find.c
> > > >  create mode 100644 cmd/efigetenv.c
> > > >  create mode 100644 cmd/part_find.c
> > > >  create mode 100644 drivers/net/efi_net.c
> > > >  create mode 100644 drivers/tpm/tpm2_efi.c
> > > >  create mode 100644 lib/efi/efi_dtb.S
> > > >  create mode 100644 lib/efi/efi_vars.c
> > > >
> > > > --
> > > > 2.47.0
> > > >
> > >
> > > Thank you for sending this and for your interesting talk at plumbers.
> > > It is a novel approach and potentially provides an alternative to grub
> > > and EFI_LOADER, etc.
> > >
> > > I'd like to try getting this running in CI as we don't have any such
> > > tests for the app or payload at present.
> > 
> > I'm planning to pull this in as this seems like an important use case.
> > I'm not sure how much time Matthew will have for this, and I want to
> > avoid this series going stale.
> > 
> > The comments which are not 'why do you want to do this?' are fairly
> > minor, so I'll send a follow-up to resolve them next week. We should
> > also think about what code we could move from lib/efi_loader to
> > lib/efi so we can share it with the app.
> > 
> > Matthew, thanks again for sending this. if you are planning to do more
> > here in the short term, please let me know.
> 
> Please don't grab other peoples patch series and pick them up for your,
> well, whatever you want to call "ci/master". You can do what you like
> with patches you wrote but taking submissions from others is just going
> to confuse the situation as to if you are, or are not, forking the
> project at this point.

And really, do not change patchwork state. That reflects the project
itself.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 00/10] Improve UEFI app support

[Simon Glass]

Hi Tom,

On Sun, Dec 8, 2024, 08:51 Tom Rini <trini@konsulko.com> wrote:
>
> On Sun, Dec 08, 2024 at 08:28:42AM -0700, Simon Glass wrote:
> > Hi,
> >
> > On Sun, 1 Dec 2024 at 09:15, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Matthew,
> > >
> > > On Sat, 23 Nov 2024 at 12:56, Matthew Garrett <mjg59@srcf.ucam.org>
wrote:
> > > >
> > > > We run a mixed environment including traditional embedded Linux and
> > > > UEFI. For consistency it's valuable to use U-Boot in both cases,
but on
> > > > UEFI systems we want to run the full Linux kernel UEFI boot stub.
This
> > > > patchset adds support for that, along with various other
quality-of-life
> > > > improvements such as automatically finding partitions based on their
> > > > GUID, automatic identification of load addresses where the firmware
has
> > > > given us constraints, support for EFI variable access in the EFI
app,
> > > > drivers for EFI network adapters and TPMs, support for embedding
DTBs in
> > > > the EFI app, and a couple of bug fixes.
> > > >
> > > >
> > > > Janis Danisevskis (1):
> > > >   Fix efi_bind_block.
> > > >
> > > > Matthew Garrett (9):
> > > >   Add EFI handover support to bootm
> > > >   Add part_find command
> > > >   Add a command to find a load address
> > > >   Hook up EFI env variable support in the EFI app
> > > >   Add EFI network driver
> > > >   Add UEFI TPM2 driver
> > > >   Support separate DTB files with the UEFI app
> > > >   Use the correct ramdisk address
> > > >   Add command to set an environment variable to an EFI variable
> > > >
> > > >  Makefile                        |   7 +-
> > > >  arch/x86/config.mk              |   2 +-
> > > >  arch/x86/lib/bootm.c            |  60 ++++++++----
> > > >  arch/x86/lib/elf_x86_64_efi.lds |   4 +
> > > >  boot/bootm.c                    |   5 +
> > > >  cmd/Kconfig                     |  23 ++++-
> > > >  cmd/Makefile                    |   3 +
> > > >  cmd/addr_find.c                 |  87 +++++++++++++++++
> > > >  cmd/efigetenv.c                 | 133 ++++++++++++++++++++++++++
> > > >  cmd/part_find.c                 | 156
++++++++++++++++++++++++++++++
> > > >  drivers/net/Kconfig             |   7 ++
> > > >  drivers/net/Makefile            |   1 +
> > > >  drivers/net/efi_net.c           | 110 +++++++++++++++++++++
> > > >  drivers/tpm/Kconfig             |   7 ++
> > > >  drivers/tpm/Makefile            |   1 +
> > > >  drivers/tpm/tpm2_efi.c          |  97 +++++++++++++++++++
> > > >  include/asm-generic/sections.h  |   1 +
> > > >  include/bootm.h                 |   6 ++
> > > >  include/efi.h                   |  24 +++++
> > > >  include/efi_tcg2.h              |   1 +
> > > >  lib/efi/Makefile                |   2 +-
> > > >  lib/efi/efi.c                   |   1 +
> > > >  lib/efi/efi_app.c               |  41 ++++++++
> > > >  lib/efi/efi_app_init.c          | 163
++++++++++++++++++++++++++++++--
> > > >  lib/efi/efi_dtb.S               |   6 ++
> > > >  lib/efi/efi_vars.c              |  44 +++++++++
> > > >  lib/fdtdec.c                    |   3 +
> > > >  27 files changed, 967 insertions(+), 28 deletions(-)
> > > >  create mode 100644 cmd/addr_find.c
> > > >  create mode 100644 cmd/efigetenv.c
> > > >  create mode 100644 cmd/part_find.c
> > > >  create mode 100644 drivers/net/efi_net.c
> > > >  create mode 100644 drivers/tpm/tpm2_efi.c
> > > >  create mode 100644 lib/efi/efi_dtb.S
> > > >  create mode 100644 lib/efi/efi_vars.c
> > > >
> > > > --
> > > > 2.47.0
> > > >
> > >
> > > Thank you for sending this and for your interesting talk at plumbers.
> > > It is a novel approach and potentially provides an alternative to grub
> > > and EFI_LOADER, etc.
> > >
> > > I'd like to try getting this running in CI as we don't have any such
> > > tests for the app or payload at present.
> >
> > I'm planning to pull this in as this seems like an important use case.
> > I'm not sure how much time Matthew will have for this, and I want to
> > avoid this series going stale.
> >
> > The comments which are not 'why do you want to do this?' are fairly
> > minor, so I'll send a follow-up to resolve them next week. We should
> > also think about what code we could move from lib/efi_loader to
> > lib/efi so we can share it with the app.
> >
> > Matthew, thanks again for sending this. if you are planning to do more
> > here in the short term, please let me know.
>
> Please don't grab other peoples patch series and pick them up for your,
> well, whatever you want to call "ci/master". You can do what you like
> with patches you wrote but taking submissions from others is just going
> to confuse the situation as to if you are, or are not, forking the
> project at this point.

This is a special situation as I had requested the series based on a talk
that I saw. I am interested in building on this work and obviously don't
want to do it on an out-of-tree basis.

I have to say I felt quite uncomfortable at the response we gave this new
contributor. If I had received such a response on my first series to U-Boot
I would never have come back.

I intend to keep the openness to new ideas in the future.

Re the patchwork thing, it was temporary while I was bringing in the
series. It should be fixed now.

Regards,
Simon