[PATCH 0/5] bootstd: Fix some reported problems

[PATCH 0/5] bootstd: Fix some reported problems

[Simon Glass]

This little series fixes a few problems reported over the past few
months.

The main changes is to the EFI bootmeth, which now delays reading the .efi
file until it is actually needed.


Simon Glass (5):
  bootflow: Export setup_fs()
  bootstd: Use a function to detect network in EFI bootmeth
  bootstd: Avoid allocating memory for the EFI file
  bootstd: Init the size before reading the devicetree
  bootstd: Init the size before reading extlinux file

 boot/bootmeth-uclass.c | 23 ++++---------
 boot/bootmeth_efi.c    | 76 ++++++++++++++++++++++++++++++------------
 boot/bootmeth_pxe.c    |  3 ++
 include/bootmeth.h     | 13 ++++++++
 4 files changed, 76 insertions(+), 39 deletions(-)

-- 
2.41.0.487.g6d72f3e995-goog

[PATCH 1/5] bootflow: Export setup_fs()

[Simon Glass]

This function is used in some bootmeth implementations. Export it.

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

 boot/bootmeth-uclass.c | 23 ++++++-----------------
 include/bootmeth.h     | 13 +++++++++++++
 2 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/boot/bootmeth-uclass.c b/boot/bootmeth-uclass.c
index eeded08dd428..175eb1de5e1e 100644
--- a/boot/bootmeth-uclass.c
+++ b/boot/bootmeth-uclass.c
@@ -240,18 +240,7 @@ int bootmeth_set_order(const char *order_str)
 	return 0;
 }
 
-/**
- * setup_fs() - Set up read to read a file
- *
- * We must redo the setup before each filesystem operation. This function
- * handles that, including setting the filesystem type if a block device is not
- * being used
- *
- * @bflow: Information about file to try
- * @desc: Block descriptor to read from (NULL if not a block device)
- * Return: 0 if OK, -ve on error
- */
-static int setup_fs(struct bootflow *bflow, struct blk_desc *desc)
+int bootmeth_setup_fs(struct bootflow *bflow, struct blk_desc *desc)
 {
 	int ret;
 
@@ -288,7 +277,7 @@ int bootmeth_try_file(struct bootflow *bflow, struct blk_desc *desc,
 	log_debug("   %s - err=%d\n", path, ret);
 
 	/* Sadly FS closes the file after fs_size() so we must redo this */
-	ret2 = setup_fs(bflow, desc);
+	ret2 = bootmeth_setup_fs(bflow, desc);
 	if (ret2)
 		return log_msg_ret("fs", ret2);
 
@@ -337,14 +326,14 @@ int bootmeth_alloc_other(struct bootflow *bflow, const char *fname,
 	if (bflow->blk)
 		desc = dev_get_uclass_plat(bflow->blk);
 
-	ret = setup_fs(bflow, desc);
+	ret = bootmeth_setup_fs(bflow, desc);
 	if (ret)
 		return log_msg_ret("fs", ret);
 
 	ret = fs_size(path, &size);
 	log_debug("   %s - err=%d\n", path, ret);
 
-	ret = setup_fs(bflow, desc);
+	ret = bootmeth_setup_fs(bflow, desc);
 	if (ret)
 		return log_msg_ret("fs", ret);
 
@@ -369,7 +358,7 @@ int bootmeth_common_read_file(struct udevice *dev, struct bootflow *bflow,
 	if (bflow->blk)
 		desc = dev_get_uclass_plat(bflow->blk);
 
-	ret = setup_fs(bflow, desc);
+	ret = bootmeth_setup_fs(bflow, desc);
 	if (ret)
 		return log_msg_ret("fs", ret);
 
@@ -379,7 +368,7 @@ int bootmeth_common_read_file(struct udevice *dev, struct bootflow *bflow,
 	if (size > *sizep)
 		return log_msg_ret("spc", -ENOSPC);
 
-	ret = setup_fs(bflow, desc);
+	ret = bootmeth_setup_fs(bflow, desc);
 	if (ret)
 		return log_msg_ret("fs", ret);
 
diff --git a/include/bootmeth.h b/include/bootmeth.h
index c3df9702e871..7cb7da33deaa 100644
--- a/include/bootmeth.h
+++ b/include/bootmeth.h
@@ -262,6 +262,19 @@ int bootmeth_setup_iter_order(struct bootflow_iter *iter, bool include_global);
  */
 int bootmeth_set_order(const char *order_str);
 
+/**
+ * bootmeth_setup_fs() - Set up read to read a file
+ *
+ * We must redo the setup before each filesystem operation. This function
+ * handles that, including setting the filesystem type if a block device is not
+ * being used
+ *
+ * @bflow: Information about file to try
+ * @desc: Block descriptor to read from (NULL if not a block device)
+ * Return: 0 if OK, -ve on error
+ */
+int bootmeth_setup_fs(struct bootflow *bflow, struct blk_desc *desc);
+
 /**
  * bootmeth_try_file() - See we can access a given file
  *
-- 
2.41.0.487.g6d72f3e995-goog

Re: [PATCH 1/5] bootflow: Export setup_fs()

[Tom Rini]

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

On Wed, Jul 26, 2023 at 09:01:21PM -0600, Simon Glass wrote:

> This function is used in some bootmeth implementations. Export it.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom

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

[PATCH 2/5] bootstd: Use a function to detect network in EFI bootmeth

[Simon Glass]

This checks for a network-based bootflow in two places, one of which is
less than ideal. Move the correct test into a function and use it in both
places.

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

 boot/bootmeth_efi.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
index af31fbfc85db..e88e171ccc1b 100644
--- a/boot/bootmeth_efi.c
+++ b/boot/bootmeth_efi.c
@@ -94,6 +94,20 @@ static int get_efi_pxe_vci(char *str, int max_len)
 	return 0;
 }
 
+/**
+ * bootmeth_uses_network() - check if the media device is Ethernet
+ *
+ * @bflow: Bootflow to check
+ * Returns: true if the media device is Ethernet, else false
+ */
+static bool bootmeth_uses_network(struct bootflow *bflow)
+{
+	const struct udevice *media = dev_get_parent(bflow->dev);
+
+	return IS_ENABLED(CONFIG_CMD_DHCP) &&
+	    device_get_uclass_id(media) == UCLASS_ETH;
+}
+
 static void set_efi_bootdev(struct blk_desc *desc, struct bootflow *bflow)
 {
 	const struct udevice *media_dev;
@@ -354,11 +368,9 @@ static int distro_efi_read_bootflow_net(struct bootflow *bflow)
 
 static int distro_efi_read_bootflow(struct udevice *dev, struct bootflow *bflow)
 {
-	const struct udevice *media = dev_get_parent(bflow->dev);
 	int ret;
 
-	if (IS_ENABLED(CONFIG_CMD_DHCP) &&
-	    device_get_uclass_id(media) == UCLASS_ETH) {
+	if (bootmeth_uses_network(bflow)) {
 		/* we only support reading from one device, so ignore 'dev' */
 		ret = distro_efi_read_bootflow_net(bflow);
 		if (ret)
@@ -378,7 +390,7 @@ int distro_efi_boot(struct udevice *dev, struct bootflow *bflow)
 	char cmd[50];
 
 	/* A non-zero buffer indicates the kernel is there */
-	if (bflow->buf) {
+	if (!bootmeth_uses_network(bflow)) {
 		/* Set the EFI bootdev again, since reading an FDT loses it! */
 		if (bflow->blk) {
 			struct blk_desc *desc = dev_get_uclass_plat(bflow->blk);
-- 
2.41.0.487.g6d72f3e995-goog

Re: [PATCH 2/5] bootstd: Use a function to detect network in EFI bootmeth

[Tom Rini]

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

On Wed, Jul 26, 2023 at 09:01:22PM -0600, Simon Glass wrote:

> This checks for a network-based bootflow in two places, one of which is
> less than ideal. Move the correct test into a function and use it in both
> places.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom

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

[PATCH 3/5] bootstd: Avoid allocating memory for the EFI file

[Simon Glass]

The current bootflow-iteration algorithm reads the bootflow file into
an allocated memory buffer so it can be examined. This works well in
most cases.

However, while the common case is that the first bootflow is immediately
booted, it is also possible just to scan for available bootflows, perhaps
selecting one to boot later.

Even with the common case, EFI bootflows can be quite large. It doesn't
make sense to read it into an allocated buffer when we have kernel_addr_t
providing a suitable address for it. Even if we do have plenty of malloc()
space available, it is a violation of U-Boot's lazy-init principle to
read the bootflow before it is needed.

So overall it seems better to make a change.

Adjust the logic to read just the size of the EFI file at first. Later,
when the bootflow is booted, read the rest of the file into the designated
kernel buffer.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reported-by: Da Xue <da@libre.computer>
Reported-by: Vincent Stehlé <vincent.stehle@arm.com>
---

 boot/bootmeth_efi.c | 50 ++++++++++++++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
index e88e171ccc1b..bceec0d12ef3 100644
--- a/boot/bootmeth_efi.c
+++ b/boot/bootmeth_efi.c
@@ -143,13 +143,24 @@ static void set_efi_bootdev(struct blk_desc *desc, struct bootflow *bflow)
 	efi_set_bootdev(dev_name, devnum_str, bflow->fname, bflow->buf, size);
 }
 
-static int efiload_read_file(struct blk_desc *desc, struct bootflow *bflow)
+static int efiload_read_file(struct bootflow *bflow, ulong addr)
 {
+	struct blk_desc *desc = NULL;
+	loff_t bytes_read;
 	int ret;
 
-	ret = bootmeth_alloc_file(bflow, 0x2000000, 0x10000);
+	if (bflow->blk)
+		 desc = dev_get_uclass_plat(bflow->blk);
+	ret = bootmeth_setup_fs(bflow, desc);
+	if (ret)
+		return log_msg_ret("set", ret);
+
+	ret = fs_read(bflow->fname, addr, 0, bflow->size, &bytes_read);
 	if (ret)
 		return log_msg_ret("read", ret);
+	bflow->buf = map_sysmem(addr, bflow->size);
+
+	set_efi_bootdev(desc, bflow);
 
 	return 0;
 }
@@ -223,7 +234,18 @@ static int distro_efi_get_fdt_name(char *fname, int size, int seq)
 	return 0;
 }
 
-static int distro_efi_read_bootflow_file(struct udevice *dev,
+/*
+ * distro_efi_try_bootflow_files() - Check that files are present
+ *
+ * This reads any FDT file and checks whether the bootflow file is present, for
+ * later reading. We avoid reading the bootflow now, since it is likely large,
+ * it may take a long time and we want to avoid needing to allocate memory for
+ * it
+ *
+ * @dev: bootmeth device to use
+ * @bflow: bootflow to update
+ */
+static int distro_efi_try_bootflow_files(struct udevice *dev,
 					 struct bootflow *bflow)
 {
 	struct blk_desc *desc = NULL;
@@ -247,9 +269,8 @@ static int distro_efi_read_bootflow_file(struct udevice *dev,
 	if (ret)
 		return log_msg_ret("try", ret);
 
-	ret = efiload_read_file(desc, bflow);
-	if (ret)
-		return log_msg_ret("read", ret);
+	/* Since we can access the file, let's call it ready */
+	bflow->state = BOOTFLOWST_READY;
 
 	fdt_addr = env_get_hex("fdt_addr_r", 0);
 
@@ -376,7 +397,7 @@ static int distro_efi_read_bootflow(struct udevice *dev, struct bootflow *bflow)
 		if (ret)
 			return log_msg_ret("net", ret);
 	} else {
-		ret = distro_efi_read_bootflow_file(dev, bflow);
+		ret = distro_efi_try_bootflow_files(dev, bflow);
 		if (ret)
 			return log_msg_ret("blk", ret);
 	}
@@ -388,17 +409,13 @@ int distro_efi_boot(struct udevice *dev, struct bootflow *bflow)
 {
 	ulong kernel, fdt;
 	char cmd[50];
+	int ret;
 
-	/* A non-zero buffer indicates the kernel is there */
+	kernel = env_get_hex("kernel_addr_r", 0);
 	if (!bootmeth_uses_network(bflow)) {
-		/* Set the EFI bootdev again, since reading an FDT loses it! */
-		if (bflow->blk) {
-			struct blk_desc *desc = dev_get_uclass_plat(bflow->blk);
-
-			set_efi_bootdev(desc, bflow);
-		}
-
-		kernel = (ulong)map_to_sysmem(bflow->buf);
+		ret = efiload_read_file(bflow, kernel);
+		if (ret)
+			return log_msg_ret("read", ret);
 
 		/*
 		 * use the provided device tree if available, else fall back to
@@ -417,7 +434,6 @@ int distro_efi_boot(struct udevice *dev, struct bootflow *bflow)
 		 * But this is the same behaviour for distro boot, so it can be
 		 * fixed here.
 		 */
-		kernel = env_get_hex("kernel_addr_r", 0);
 		fdt = env_get_hex("fdt_addr_r", 0);
 	}
 
-- 
2.41.0.487.g6d72f3e995-goog

Re: [PATCH 3/5] bootstd: Avoid allocating memory for the EFI file

[Tom Rini]

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

On Wed, Jul 26, 2023 at 09:01:23PM -0600, Simon Glass wrote:

> The current bootflow-iteration algorithm reads the bootflow file into
> an allocated memory buffer so it can be examined. This works well in
> most cases.
> 
> However, while the common case is that the first bootflow is immediately
> booted, it is also possible just to scan for available bootflows, perhaps
> selecting one to boot later.
> 
> Even with the common case, EFI bootflows can be quite large. It doesn't
> make sense to read it into an allocated buffer when we have kernel_addr_t
> providing a suitable address for it. Even if we do have plenty of malloc()
> space available, it is a violation of U-Boot's lazy-init principle to
> read the bootflow before it is needed.
> 
> So overall it seems better to make a change.
> 
> Adjust the logic to read just the size of the EFI file at first. Later,
> when the bootflow is booted, read the rest of the file into the designated
> kernel buffer.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reported-by: Da Xue <da@libre.computer>
> Reported-by: Vincent Stehlé <vincent.stehle@arm.com>

Applied to u-boot/master, thanks!

-- 
Tom

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

[PATCH 4/5] bootstd: Init the size before reading the devicetree

[Simon Glass]

The implementation in distro_efi_try_bootflow_files() does not pass a
valid size to bootmeth_common_read_file(), so this can fail if the
uninted value happens to be too small.

Fix this.

This was reported by someone but I cannot now find the email.

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

 boot/bootmeth_efi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
index bceec0d12ef3..1c9f2b1e2fe4 100644
--- a/boot/bootmeth_efi.c
+++ b/boot/bootmeth_efi.c
@@ -21,6 +21,7 @@
 #include <mmc.h>
 #include <net.h>
 #include <pxe_utils.h>
+#include <linux/sizes.h>
 
 #define EFI_DIRNAME	"efi/boot/"
 
@@ -281,9 +282,12 @@ static int distro_efi_try_bootflow_files(struct udevice *dev,
 		ret = distro_efi_get_fdt_name(fname, sizeof(fname), seq);
 		if (ret == -EALREADY)
 			bflow->flags = BOOTFLOWF_USE_PRIOR_FDT;
-		if (!ret)
+		if (!ret) {
+			/* Limit FDT files to 4MB */
+			size = SZ_4M;
 			ret = bootmeth_common_read_file(dev, bflow, fname,
 							fdt_addr, &size);
+		}
 	}
 
 	if (*fname) {
-- 
2.41.0.487.g6d72f3e995-goog

Re: [PATCH 4/5] bootstd: Init the size before reading the devicetree

[Tom Rini]

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

On Wed, Jul 26, 2023 at 09:01:24PM -0600, Simon Glass wrote:

> The implementation in distro_efi_try_bootflow_files() does not pass a
> valid size to bootmeth_common_read_file(), so this can fail if the
> uninted value happens to be too small.
> 
> Fix this.
> 
> This was reported by someone but I cannot now find the email.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom

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

[PATCH 5/5] bootstd: Init the size before reading extlinux file

[Simon Glass]

The implementation in extlinux_pxe_getfile() does not pass a valid size
to bootmeth_read_file(), so this can fail if the uninited value happens to
be too small.

Fix this.

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

 boot/bootmeth_pxe.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/boot/bootmeth_pxe.c b/boot/bootmeth_pxe.c
index ce986bd260d1..8d489a11aa40 100644
--- a/boot/bootmeth_pxe.c
+++ b/boot/bootmeth_pxe.c
@@ -31,6 +31,9 @@ static int extlinux_pxe_getfile(struct pxe_context *ctx, const char *file_path,
 	int ret;
 
 	addr = simple_strtoul(file_addr, NULL, 16);
+
+	/* Allow up to 1GB */
+	*sizep = 1 << 30;
 	ret = bootmeth_read_file(info->dev, info->bflow, file_path, addr,
 				 sizep);
 	if (ret)
-- 
2.41.0.487.g6d72f3e995-goog

Re: [PATCH 5/5] bootstd: Init the size before reading extlinux file

[Tom Rini]

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

On Wed, Jul 26, 2023 at 09:01:25PM -0600, Simon Glass wrote:

> The implementation in extlinux_pxe_getfile() does not pass a valid size
> to bootmeth_read_file(), so this can fail if the uninited value happens to
> be too small.
> 
> Fix this.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom

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