[PATCH v2 0/8] Various minor fixes

[PATCH v2 0/8] Various minor fixes

[Simon Glass]

This series collects a few minor fixes and improvements that have been
hanging around in a branch for a while.

Changes in v2:
- Drop the file instead of fixing it up
- Add new patch to add os_realloc()
- Drop unnecessary check before calling os_free()
- Update the comment for os_free()
- Also free the RAM buffer
- Convert the rest of the allocations in os.c, etc.

Simon Glass (8):
  spl: Drop duplicate 'Jumping to U-Boot' message
  binman: Indicate how to make binman verbose
  doc: Add a note about how to produce 'md' output using hexdump
  s5p4418_nanopi2: Drop dead code
  sandbox: Add os_realloc()
  sandbox: Avoid using malloc() for system state
  sandbox: Write out bloblist when existing
  bootm: Fix duplicate debugging in bootm_process_cmdline()

 Makefile                              |   1 +
 README                                |   4 +
 arch/arm/mach-nexell/Makefile         |   1 -
 arch/arm/mach-nexell/cmd_boot_linux.c | 144 --------------------------
 arch/sandbox/cpu/os.c                 |  62 ++++++++---
 arch/sandbox/cpu/spl.c                |  10 +-
 arch/sandbox/cpu/start.c              |  12 ++-
 arch/sandbox/cpu/state.c              |  23 ++--
 common/bootm.c                        |   2 +-
 common/spl/spl.c                      |   1 -
 include/os.h                          |  15 ++-
 tools/binman/README                   |   4 +-
 12 files changed, 100 insertions(+), 179 deletions(-)
 delete mode 100644 arch/arm/mach-nexell/cmd_boot_linux.c

-- 
2.30.0.280.ga3ce27912f-goog

[PATCH v2 1/8] spl: Drop duplicate 'Jumping to U-Boot' message

[Simon Glass]

This is printed twice but we only need one message, since there is very
little processing in between them. Drop the first one.

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

(no changes since v1)

 common/spl/spl.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/common/spl/spl.c b/common/spl/spl.c
index 12b00e2a407..7fe0812799f 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -693,7 +693,6 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
 #endif
 	switch (spl_image.os) {
 	case IH_OS_U_BOOT:
-		debug("Jumping to U-Boot\n");
 		break;
 #if CONFIG_IS_ENABLED(ATF)
 	case IH_OS_ARM_TRUSTED_FIRMWARE:
-- 
2.30.0.280.ga3ce27912f-goog

[PATCH v2 2/8] binman: Indicate how to make binman verbose

[Simon Glass]

Add notes about how to make binman produce verbose logging when building.

Add a comment on how to do this.

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

(no changes since v1)

 Makefile            | 1 +
 tools/binman/README | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index c2b7046ce3b..66f441f23aa 100644
--- a/Makefile
+++ b/Makefile
@@ -1326,6 +1326,7 @@ u-boot.ldr:	u-boot
 # binman
 # ---------------------------------------------------------------------------
 # Use 'make BINMAN_DEBUG=1' to enable debugging
+# Use 'make BINMAN_VERBOSE=3' to set vebosity level
 default_dt := $(if $(DEVICE_TREE),$(DEVICE_TREE),$(CONFIG_DEFAULT_DEVICE_TREE))
 quiet_cmd_binman = BINMAN  $@
 cmd_binman = $(srctree)/tools/binman/binman $(if $(BINMAN_DEBUG),-D) \
diff --git a/tools/binman/README b/tools/binman/README
index a00c9026163..45f0a0c2cd3 100644
--- a/tools/binman/README
+++ b/tools/binman/README
@@ -637,7 +637,8 @@ Logging
 
 Binman normally operates silently unless there is an error, in which case it
 just displays the error. The -D/--debug option can be used to create a full
-backtrace when errors occur.
+backtrace when errors occur. You can use BINMAN_DEBUG=1 when building to select
+this.
 
 Internally binman logs some output while it is running. This can be displayed
 by increasing the -v/--verbosity from the default of 1:
@@ -649,6 +650,7 @@ by increasing the -v/--verbosity from the default of 1:
    4: detailed information about each operation
    5: debug (all output)
 
+You can use BINMAN_VERBOSE=5 (for example) when building to select this.
 
 Hashing Entries
 ---------------
-- 
2.30.0.280.ga3ce27912f-goog

[PATCH v2 3/8] doc: Add a note about how to produce 'md' output using hexdump

[Simon Glass]

Comparing a hex dump on the U-Boot command line with the contents of a
file on the host system is fairly easy and convenient to do manually if
it is small. But the format used hexdump by default differs from that
shown by U-Boot. Add a note about how to make them the same.

(For large dumps, writing the data to the network with tftpput, or to a
USB stick with ext4save is easiest.)

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

(no changes since v1)

 README | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/README b/README
index afa33dc7f30..582bfb00348 100644
--- a/README
+++ b/README
@@ -3274,6 +3274,10 @@ TODO.
 
 For now: just type "help <command>".
 
+Note that the format of 'md' can be emulated from linux with:
+
+   hexdump -v -e '"%08.8_ax: " 16/1 "%02x " "   "' -e '16/1 "%_p" "\n" ' fname
+
 
 Environment Variables:
 ======================
-- 
2.30.0.280.ga3ce27912f-goog

[PATCH v2 4/8] s5p4418_nanopi2: Drop dead code

[Simon Glass]

This code is still using the old command typedef. It was not noticed since
this file is not currently built. It is using a non-existent option in the
Makefile.

Drop this file since it is not needed for correct operation.

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

Changes in v2:
- Drop the file instead of fixing it up

 arch/arm/mach-nexell/Makefile         |   1 -
 arch/arm/mach-nexell/cmd_boot_linux.c | 144 --------------------------
 2 files changed, 145 deletions(-)
 delete mode 100644 arch/arm/mach-nexell/cmd_boot_linux.c

diff --git a/arch/arm/mach-nexell/Makefile b/arch/arm/mach-nexell/Makefile
index 10b3963ed10..dda16dbb8e6 100644
--- a/arch/arm/mach-nexell/Makefile
+++ b/arch/arm/mach-nexell/Makefile
@@ -10,4 +10,3 @@ obj-y				+= nx_gpio.o
 obj-y				+= tieoff.o
 obj-$(CONFIG_ARCH_S5P4418)	+= reg-call.o
 obj-$(CONFIG_ARCH_S5P4418)	+= nx_sec_reg.o
-obj-$(CONFIG_CMD_BOOTL)		+= cmd_boot_linux.o
diff --git a/arch/arm/mach-nexell/cmd_boot_linux.c b/arch/arm/mach-nexell/cmd_boot_linux.c
deleted file mode 100644
index f2dedfe1625..00000000000
--- a/arch/arm/mach-nexell/cmd_boot_linux.c
+++ /dev/null
@@ -1,144 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * (C) Copyright 2016 nexell
- * jhkim <jhkim@nexell.co.kr>
- */
-
-#include <common.h>
-#include <bootm.h>
-#include <command.h>
-#include <environment.h>
-#include <errno.h>
-#include <image.h>
-#include <fdt_support.h>
-
-#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_CLI_FRAMEWORK)
-
-DECLARE_GLOBAL_DATA_PTR;
-
-static bootm_headers_t linux_images;
-
-static void boot_go_set_os(cmd_tbl_t *cmdtp, int flag, int argc,
-			   char * const argv[],
-			   bootm_headers_t *images)
-{
-	char * const img_addr = argv[0];
-
-	images->os.type = IH_TYPE_KERNEL;
-	images->os.comp = IH_COMP_NONE;
-	images->os.os = IH_OS_LINUX;
-	images->os.load = simple_strtoul(img_addr, NULL, 16);
-	images->ep = images->os.load;
-#if defined(CONFIG_ARM)
-	images->os.arch = IH_ARCH_ARM;
-#elif defined(CONFIG_ARM64)
-	images->os.arch = IH_ARCH_ARM64;
-#else
-	#error "Not support architecture ..."
-#endif
-	if (!IS_ENABLED(CONFIG_OF_LIBFDT) && !IS_ENABLED(CONFIG_SPL_BUILD)) {
-		/* set DTB address for linux kernel */
-		if (argc > 2) {
-			unsigned long ft_addr;
-
-			ft_addr = simple_strtol(argv[2], NULL, 16);
-			images->ft_addr = (char *)ft_addr;
-
-			/*
-			 * if not defined IMAGE_ENABLE_OF_LIBFDT,
-			 * must be set to fdt address
-			 */
-			if (!IMAGE_ENABLE_OF_LIBFDT)
-				gd->bd->bi_boot_params = ft_addr;
-
-			debug("## set ft:%08lx and boot params:%08lx [control of:%s]"
-			      "...\n", ft_addr, gd->bd->bi_boot_params,
-			      IMAGE_ENABLE_OF_LIBFDT ? "on" : "off");
-		}
-	}
-}
-
-#if defined(CONFIG_OF_LIBFDT) && defined(CONFIG_LMB)
-static void boot_start_lmb(bootm_headers_t *images)
-{
-	ulong		mem_start;
-	phys_size_t	mem_size;
-
-	lmb_init(&images->lmb);
-
-	mem_start = getenv_bootm_low();
-	mem_size = getenv_bootm_size();
-
-	lmb_add(&images->lmb, (phys_addr_t)mem_start, mem_size);
-
-	arch_lmb_reserve(&images->lmb);
-	board_lmb_reserve(&images->lmb);
-}
-#else
-#define lmb_reserve(lmb, base, size)
-static inline void boot_start_lmb(bootm_headers_t *images) { }
-#endif
-
-int do_boot_linux(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
-{
-	boot_os_fn *boot_fn;
-	bootm_headers_t *images = &linux_images;
-	int flags;
-	int ret;
-
-	boot_start_lmb(images);
-
-	flags  = BOOTM_STATE_START;
-
-	argc--; argv++;
-	boot_go_set_os(cmdtp, flag, argc, argv, images);
-
-	if (IS_ENABLED(CONFIG_OF_LIBFDT)) {
-		/* find flattened device tree */
-		ret = boot_get_fdt(flag, argc, argv, IH_ARCH_DEFAULT, images,
-				   &images->ft_addr, &images->ft_len);
-		if (ret) {
-			puts("Could not find a valid device tree\n");
-			return 1;
-		}
-		set_working_fdt_addr((ulong)images->ft_addr);
-	}
-
-	if (!IS_ENABLED(CONFIG_OF_LIBFDT))
-		flags |= BOOTM_STATE_OS_GO;
-
-	boot_fn = do_bootm_linux;
-	ret = boot_fn(flags, argc, argv, images);
-
-	if (ret == BOOTM_ERR_UNIMPLEMENTED)
-		show_boot_progress(BOOTSTAGE_ID_DECOMP_UNIMPL);
-	else if (ret == BOOTM_ERR_RESET)
-		do_reset(cmdtp, flag, argc, argv);
-
-	return ret;
-}
-
-U_BOOT_CMD(bootl, CONFIG_SYS_MAXARGS, 1, do_boot_linux,
-	   "boot linux image from memory",
-	   "[addr [arg ...]]\n    - boot linux image stored in memory\n"
-	   "\tuse a '-' for the DTB address\n"
-);
-#endif
-
-#if defined(CONFIG_CMD_BOOTD) && !defined(CONFIG_CMD_BOOTM)
-int do_bootd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
-{
-	return run_command(env_get("bootcmd"), flag);
-}
-
-U_BOOT_CMD(boot, 1, 1, do_bootd,
-	   "boot default, i.e., run 'bootcmd'",
-	   ""
-);
-
-/* keep old command name "bootd" for backward compatibility */
-U_BOOT_CMD(bootd, 1,	1,	do_bootd,
-	   "boot default, i.e., run 'bootcmd'",
-	   ""
-);
-#endif
-- 
2.30.0.280.ga3ce27912f-goog

[PATCH v2 5/8] sandbox: Add os_realloc()

[Simon Glass]

We provide os_malloc() and os_free() but not os_realloc(). Add this,
following the usual semantics. Also update os_malloc() to behave correctly
when passed a zero size.

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

Changes in v2:
- Add new patch to add os_realloc()

 arch/sandbox/cpu/os.c | 38 ++++++++++++++++++++++++++++++++++++++
 include/os.h          | 12 +++++++++++-
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index 80996a91ce7..69ce6afdfac 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -269,6 +269,8 @@ void *os_malloc(size_t length)
 	int page_size = getpagesize();
 	struct os_mem_hdr *hdr;
 
+	if (!length)
+		return NULL;
 	/*
 	 * Use an address that is hopefully available to us so that pointers
 	 * to this memory are fairly obvious. If we end up with a different
@@ -295,6 +297,42 @@ void os_free(void *ptr)
 	}
 }
 
+/* These macros are from kernel.h but not accessible in this file */
+#define ALIGN(x, a)		__ALIGN_MASK((x), (typeof(x))(a) - 1)
+#define __ALIGN_MASK(x, mask)	(((x) + (mask)) & ~(mask))
+
+void *os_realloc(void *ptr, size_t length)
+{
+	int page_size = getpagesize();
+	struct os_mem_hdr *hdr;
+	void *new_ptr;
+
+	/* Reallocating a NULL pointer is just an alloc */
+	if (!ptr)
+		return os_malloc(length);
+
+	/* Changing a length to 0 is just a free */
+	if (length) {
+		os_free(ptr);
+		return NULL;
+	}
+
+	/*
+	 * If the new size is the same number of pages as the old, nothing to
+	 * do. There isn't much point in shrinking things
+	 */
+	hdr = ptr - page_size;
+	if (ALIGN(length, page_size) <= ALIGN(hdr->length, page_size))
+		return ptr;
+
+	/* We have to grow it, so allocate something new */
+	new_ptr = os_malloc(length);
+	memcpy(new_ptr, ptr, hdr->length);
+	os_free(ptr);
+
+	return new_ptr;
+}
+
 void os_usleep(unsigned long usec)
 {
 	usleep(usec);
diff --git a/include/os.h b/include/os.h
index 0913b47b3a8..b4c5dd727c4 100644
--- a/include/os.h
+++ b/include/os.h
@@ -114,7 +114,7 @@ void os_fd_restore(void);
  * os_malloc() - aquires some memory from the underlying os.
  *
  * @length:	Number of bytes to be allocated
- * Return:	Pointer to length bytes or NULL on error
+ * Return:	Pointer to length bytes or NULL if @length is 0 or on error
  */
 void *os_malloc(size_t length);
 
@@ -127,6 +127,16 @@ void *os_malloc(size_t length);
  */
 void os_free(void *ptr);
 
+/**
+ * os_realloc() - reallocate memory
+ *
+ * This follows the semantics of realloc(), so can perform an os_malloc() or
+ * os_free() depending on @ptr and @length.
+ *
+ * Return:	Pointer to reallocated memory or NULL if @length is 0
+ */
+void *os_realloc(void *ptr, size_t length);
+
 /**
  * os_usleep() - access to the usleep function of the os
  *
-- 
2.30.0.280.ga3ce27912f-goog

[PATCH v2 6/8] sandbox: Avoid using malloc() for system state

[Simon Glass]

This state is not accessible to the running U-Boot but at present it is
allocated in the emulated SDRAM. This doesn't seem very useful. Adjust
it to allocate from the OS instead.

The RAM buffer is currently not free, but should be, so add that into
state_uninit(). Update the comment for os_free() to indicate that NULL is
a valid parameter value.

Note that the strdup() in spl_board_load_image() is changed as well, since
strdup() allocates memory in the RAM buffer.

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

Changes in v2:
- Drop unnecessary check before calling os_free()
- Update the comment for os_free()
- Also free the RAM buffer
- Convert the rest of the allocations in os.c, etc.

 arch/sandbox/cpu/os.c    | 24 ++++++++++++------------
 arch/sandbox/cpu/spl.c   | 10 +++++++---
 arch/sandbox/cpu/start.c | 12 +++++++-----
 arch/sandbox/cpu/state.c | 18 +++++++++---------
 include/os.h             |  3 ++-
 5 files changed, 37 insertions(+), 30 deletions(-)

diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index 69ce6afdfac..5df11007c9d 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -150,7 +150,7 @@ int os_read_file(const char *fname, void **bufp, int *sizep)
 		printf("Cannot seek to start of file '%s'\n", fname);
 		goto err;
 	}
-	*bufp = malloc(size);
+	*bufp = os_malloc(size);
 	if (!*bufp) {
 		printf("Not enough memory to read file '%s'\n", fname);
 		ret = -ENOMEM;
@@ -378,8 +378,8 @@ int os_parse_args(struct sandbox_state *state, int argc, char *argv[])
 	state->argv = argv;
 
 	/* dynamically construct the arguments to the system getopt_long */
-	short_opts = malloc(sizeof(*short_opts) * num_options * 2 + 1);
-	long_opts = malloc(sizeof(*long_opts) * (num_options + 1));
+	short_opts = os_malloc(sizeof(*short_opts) * num_options * 2 + 1);
+	long_opts = os_malloc(sizeof(*long_opts) * (num_options + 1));
 	if (!short_opts || !long_opts)
 		return 1;
 
@@ -458,7 +458,7 @@ void os_dirent_free(struct os_dirent_node *node)
 
 	while (node) {
 		next = node->next;
-		free(node);
+		os_free(node);
 		node = next;
 	}
 }
@@ -483,7 +483,7 @@ int os_dirent_ls(const char *dirname, struct os_dirent_node **headp)
 	/* Create a buffer upfront, with typically sufficient size */
 	dirlen = strlen(dirname) + 2;
 	len = dirlen + 256;
-	fname = malloc(len);
+	fname = os_malloc(len);
 	if (!fname) {
 		ret = -ENOMEM;
 		goto done;
@@ -496,7 +496,7 @@ int os_dirent_ls(const char *dirname, struct os_dirent_node **headp)
 			ret = errno;
 			break;
 		}
-		next = malloc(sizeof(*node) + strlen(entry->d_name) + 1);
+		next = os_malloc(sizeof(*node) + strlen(entry->d_name) + 1);
 		if (!next) {
 			os_dirent_free(head);
 			ret = -ENOMEM;
@@ -505,10 +505,10 @@ int os_dirent_ls(const char *dirname, struct os_dirent_node **headp)
 		if (dirlen + strlen(entry->d_name) > len) {
 			len = dirlen + strlen(entry->d_name);
 			old_fname = fname;
-			fname = realloc(fname, len);
+			fname = os_realloc(fname, len);
 			if (!fname) {
-				free(old_fname);
-				free(next);
+				os_free(old_fname);
+				os_free(next);
 				os_dirent_free(head);
 				ret = -ENOMEM;
 				goto done;
@@ -542,7 +542,7 @@ int os_dirent_ls(const char *dirname, struct os_dirent_node **headp)
 
 done:
 	closedir(dir);
-	free(fname);
+	os_free(fname);
 	return ret;
 }
 
@@ -659,7 +659,7 @@ static int add_args(char ***argvp, char *add_args[], int count)
 	for (argc = 0; (*argvp)[argc]; argc++)
 		;
 
-	argv = malloc((argc + count + 1) * sizeof(char *));
+	argv = os_malloc((argc + count + 1) * sizeof(char *));
 	if (!argv) {
 		printf("Out of memory for %d argv\n", count);
 		return -ENOMEM;
@@ -742,7 +742,7 @@ static int os_jump_to_file(const char *fname)
 		os_exit(2);
 
 	err = execv(fname, argv);
-	free(argv);
+	os_free(argv);
 	if (err) {
 		perror("Unable to run image");
 		printf("Image filename '%s'\n", fname);
diff --git a/arch/sandbox/cpu/spl.c b/arch/sandbox/cpu/spl.c
index 9a77da15619..197b98c9828 100644
--- a/arch/sandbox/cpu/spl.c
+++ b/arch/sandbox/cpu/spl.c
@@ -42,10 +42,14 @@ static int spl_board_load_image(struct spl_image_info *spl_image,
 		return ret;
 	}
 
-	/* Set up spl_image to boot from jump_to_image_no_args() */
-	spl_image->arg = strdup(fname);
+	/*
+	 * Set up spl_image to boot from jump_to_image_no_args(). Allocate this
+	 * outsdide the RAM buffer (i.e. don't use strdup()).
+	 */
+	spl_image->arg = os_malloc(strlen(fname) + 1);
 	if (!spl_image->arg)
-		return log_msg_ret("Setup exec filename", -ENOMEM);
+		return log_msg_ret("exec", -ENOMEM);
+	strcpy(spl_image->arg, fname);
 
 	return 0;
 }
diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c
index 8322ed7a1fe..180ecc770d4 100644
--- a/arch/sandbox/cpu/start.c
+++ b/arch/sandbox/cpu/start.c
@@ -86,7 +86,7 @@ int sandbox_early_getopt_check(void)
 
 	/* Sort the options */
 	size = sizeof(*sorted_opt) * num_options;
-	sorted_opt = malloc(size);
+	sorted_opt = os_malloc(size);
 	if (!sorted_opt) {
 		printf("No memory to sort options\n");
 		os_exit(1);
@@ -186,7 +186,7 @@ static int sandbox_cmdline_cb_default_fdt(struct sandbox_state *state,
 	int len;
 
 	len = strlen(state->argv[0]) + strlen(fmt) + 1;
-	fname = malloc(len);
+	fname = os_malloc(len);
 	if (!fname)
 		return -ENOMEM;
 	snprintf(fname, len, fmt, state->argv[0]);
@@ -206,7 +206,7 @@ static int sandbox_cmdline_cb_test_fdt(struct sandbox_state *state,
 	int len;
 
 	len = strlen(state->argv[0]) + strlen(fmt) + 1;
-	fname = malloc(len);
+	fname = os_malloc(len);
 	if (!fname)
 		return -ENOMEM;
 	strcpy(fname, state->argv[0]);
@@ -434,16 +434,18 @@ int main(int argc, char *argv[])
 {
 	struct sandbox_state *state;
 	gd_t data;
+	int size;
 	int ret;
 
 	/*
 	 * Copy argv[] so that we can pass the arguments in the original
 	 * sequence when resetting the sandbox.
 	 */
-	os_argv = calloc(argc + 1, sizeof(char *));
+	size = sizeof(char *) * (argc + 1);
+	os_argv = os_malloc(size);
 	if (!os_argv)
 		os_exit(1);
-	memcpy(os_argv, argv, sizeof(char *) * (argc + 1));
+	memcpy(os_argv, argv, size);
 
 	memset(&data, '\0', sizeof(data));
 	gd = &data;
diff --git a/arch/sandbox/cpu/state.c b/arch/sandbox/cpu/state.c
index b2901b7a8ca..4ffaf163789 100644
--- a/arch/sandbox/cpu/state.c
+++ b/arch/sandbox/cpu/state.c
@@ -29,17 +29,17 @@ static int state_ensure_space(int extra_size)
 		return 0;
 
 	size = used + extra_size;
-	buf = malloc(size);
+	buf = os_malloc(size);
 	if (!buf)
 		return -ENOMEM;
 
 	ret = fdt_open_into(blob, buf, size);
 	if (ret) {
-		free(buf);
+		os_free(buf);
 		return -EIO;
 	}
 
-	free(blob);
+	os_free(blob);
 	state->state_fdt = buf;
 	return 0;
 }
@@ -55,7 +55,7 @@ static int state_read_file(struct sandbox_state *state, const char *fname)
 		printf("Cannot find sandbox state file '%s'\n", fname);
 		return -ENOENT;
 	}
-	state->state_fdt = malloc(size);
+	state->state_fdt = os_malloc(size);
 	if (!state->state_fdt) {
 		puts("No memory to read sandbox state\n");
 		return -ENOMEM;
@@ -77,7 +77,7 @@ static int state_read_file(struct sandbox_state *state, const char *fname)
 err_read:
 	os_close(fd);
 err_open:
-	free(state->state_fdt);
+	os_free(state->state_fdt);
 	state->state_fdt = NULL;
 
 	return ret;
@@ -244,7 +244,7 @@ int sandbox_write_state(struct sandbox_state *state, const char *fname)
 	/* Create a state FDT if we don't have one */
 	if (!state->state_fdt) {
 		size = 0x4000;
-		state->state_fdt = malloc(size);
+		state->state_fdt = os_malloc(size);
 		if (!state->state_fdt) {
 			puts("No memory to create FDT\n");
 			return -ENOMEM;
@@ -302,7 +302,7 @@ int sandbox_write_state(struct sandbox_state *state, const char *fname)
 err_write:
 	os_close(fd);
 err_create:
-	free(state->state_fdt);
+	os_free(state->state_fdt);
 
 	return ret;
 }
@@ -419,8 +419,8 @@ int state_uninit(void)
 	if (state->jumped_fname)
 		os_unlink(state->jumped_fname);
 
-	if (state->state_fdt)
-		free(state->state_fdt);
+	os_free(state->state_fdt);
+	os_free(state->ram_buf);
 	memset(state, '\0', sizeof(*state));
 
 	return 0;
diff --git a/include/os.h b/include/os.h
index b4c5dd727c4..57d5ee78932 100644
--- a/include/os.h
+++ b/include/os.h
@@ -123,7 +123,8 @@ void *os_malloc(size_t length);
  *
  * This returns the memory to the OS.
  *
- * @ptr:	Pointer to memory block to free
+ * @ptr:	Pointer to memory block to free. If this is NULL then this
+ *		function does nothing
  */
 void os_free(void *ptr);
 
-- 
2.30.0.280.ga3ce27912f-goog

[PATCH v2 7/8] sandbox: Write out bloblist when existing

[Simon Glass]

Sandbox provides a way to write out its emulated memory on exit. This
makes it possible to pass a bloblist from one phase (e.g. SPL) to the
next.

However the bloblist is not closed off, so the checksum is generally
invalid. Fix this by finishing up the bloblist before writing the memory
file.

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

(no changes since v1)

 arch/sandbox/cpu/state.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/sandbox/cpu/state.c b/arch/sandbox/cpu/state.c
index 4ffaf163789..f63cfd38ee4 100644
--- a/arch/sandbox/cpu/state.c
+++ b/arch/sandbox/cpu/state.c
@@ -4,6 +4,7 @@
  */
 
 #include <common.h>
+#include <bloblist.h>
 #include <errno.h>
 #include <fdtdec.h>
 #include <log.h>
@@ -398,8 +399,12 @@ int state_uninit(void)
 {
 	int err;
 
+	log_info("Writing sandbox state\n");
 	state = &main_state;
 
+	/* Finish the bloblist, so that it is correct before writing memory */
+	bloblist_finish();
+
 	if (state->write_ram_buf) {
 		err = os_write_ram_buf(state->ram_buf_fname);
 		if (err) {
-- 
2.30.0.280.ga3ce27912f-goog

[PATCH v2 8/8] bootm: Fix duplicate debugging in bootm_process_cmdline()

[Simon Glass]

These two returns use the same string so are not distinguishable with
LOG_ERROR_RETURN. Fix it.

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

(no changes since v1)

 common/bootm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/bootm.c b/common/bootm.c
index 8298693900d..48a5b04cd7a 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -585,7 +585,7 @@ int bootm_process_cmdline(char *buf, int maxlen, int flags)
 	if (IS_ENABLED(CONFIG_BOOTARGS_SUBST) && (flags & BOOTM_CL_SUBST)) {
 		ret = process_subst(buf, maxlen);
 		if (ret)
-			return log_msg_ret("silent", ret);
+			return log_msg_ret("subst", ret);
 	}
 
 	return 0;
-- 
2.30.0.280.ga3ce27912f-goog

[PATCH v2 5/8] sandbox: Add os_realloc()

[Heinrich Schuchardt]

On 1/23/21 6:36 PM, Simon Glass wrote:
> We provide os_malloc() and os_free() but not os_realloc(). Add this,
> following the usual semantics. Also update os_malloc() to behave correctly
> when passed a zero size.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

In the code I am missing a comment why we are not simply using malloc(),
realloc(), free() instead of mmap(), munmap().

mmap() creates page size aligned addresses while mALLOc() in non-sandbox
U-Boot will creates MALLOC_ALIGNMENT aligned addresses. This may result
in errors due to misalignment not being caught on the sandbox.

Hence I would prefer if os_malloc() would add a prime number times
MALLOC_ALIGNMENT to the address returned by mmap() instead of page_size
to match the alignment guarantee of U-Boot. E.g.

     #define MALLOC_ALIGNMENT (2 * sizeof(size_t))

     void *os_malloc(size_t length)
     {
         ...
         return (void *)hdr + 19 * MALLOC_ALIGNMENT;
     }

We should put a magic string into the currently unused first bytes of
the memory provided by mmap() and check if the string is present in
os_free(). Crash the system if the magic is not found like U-Boot's
malloc() does.

POSIX requires that mmap() zeros any memory that it allocates. The
memory returned by mmap() should be filled with random bytes to catch
more errors.

What are your thought on these topics?

For the current patch:

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>
> Changes in v2:
> - Add new patch to add os_realloc()
>
>   arch/sandbox/cpu/os.c | 38 ++++++++++++++++++++++++++++++++++++++
>   include/os.h          | 12 +++++++++++-
>   2 files changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
> index 80996a91ce7..69ce6afdfac 100644
> --- a/arch/sandbox/cpu/os.c
> +++ b/arch/sandbox/cpu/os.c
> @@ -269,6 +269,8 @@ void *os_malloc(size_t length)
>   	int page_size = getpagesize();
>   	struct os_mem_hdr *hdr;
>
> +	if (!length)
> +		return NULL;
>   	/*
>   	 * Use an address that is hopefully available to us so that pointers
>   	 * to this memory are fairly obvious. If we end up with a different
> @@ -295,6 +297,42 @@ void os_free(void *ptr)
>   	}
>   }
>
> +/* These macros are from kernel.h but not accessible in this file */
> +#define ALIGN(x, a)		__ALIGN_MASK((x), (typeof(x))(a) - 1)
> +#define __ALIGN_MASK(x, mask)	(((x) + (mask)) & ~(mask))
> +
> +void *os_realloc(void *ptr, size_t length)
> +{
> +	int page_size = getpagesize();
> +	struct os_mem_hdr *hdr;
> +	void *new_ptr;
> +
> +	/* Reallocating a NULL pointer is just an alloc */
> +	if (!ptr)
> +		return os_malloc(length);
> +
> +	/* Changing a length to 0 is just a free */
> +	if (length) {
> +		os_free(ptr);
> +		return NULL;
> +	}
> +
> +	/*
> +	 * If the new size is the same number of pages as the old, nothing to
> +	 * do. There isn't much point in shrinking things
> +	 */
> +	hdr = ptr - page_size;
> +	if (ALIGN(length, page_size) <= ALIGN(hdr->length, page_size))
> +		return ptr;
> +
> +	/* We have to grow it, so allocate something new */
> +	new_ptr = os_malloc(length);
> +	memcpy(new_ptr, ptr, hdr->length);
> +	os_free(ptr);
> +
> +	return new_ptr;
> +}
> +
>   void os_usleep(unsigned long usec)
>   {
>   	usleep(usec);
> diff --git a/include/os.h b/include/os.h
> index 0913b47b3a8..b4c5dd727c4 100644
> --- a/include/os.h
> +++ b/include/os.h
> @@ -114,7 +114,7 @@ void os_fd_restore(void);
>    * os_malloc() - aquires some memory from the underlying os.
>    *
>    * @length:	Number of bytes to be allocated
> - * Return:	Pointer to length bytes or NULL on error
> + * Return:	Pointer to length bytes or NULL if @length is 0 or on error
>    */
>   void *os_malloc(size_t length);
>
> @@ -127,6 +127,16 @@ void *os_malloc(size_t length);
>    */
>   void os_free(void *ptr);
>
> +/**
> + * os_realloc() - reallocate memory
> + *
> + * This follows the semantics of realloc(), so can perform an os_malloc() or
> + * os_free() depending on @ptr and @length.
> + *
> + * Return:	Pointer to reallocated memory or NULL if @length is 0
> + */
> +void *os_realloc(void *ptr, size_t length);
> +
>   /**
>    * os_usleep() - access to the usleep function of the os
>    *
>

[PATCH v2 3/8] doc: Add a note about how to produce 'md' output using hexdump

[Heinrich Schuchardt]

On 1/23/21 6:36 PM, Simon Glass wrote:
> Comparing a hex dump on the U-Boot command line with the contents of a
> file on the host system is fairly easy and convenient to do manually if
> it is small. But the format used hexdump by default differs from that
> shown by U-Boot. Add a note about how to make them the same.
>
> (For large dumps, writing the data to the network with tftpput, or to a
> USB stick with ext4save is easiest.)
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v1)
>
>   README | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/README b/README
> index afa33dc7f30..582bfb00348 100644
> --- a/README
> +++ b/README
> @@ -3274,6 +3274,10 @@ TODO.
>
>   For now: just type "help <command>".
>
> +Note that the format of 'md' can be emulated from linux with:
> +
> +   hexdump -v -e '"%08.8_ax: " 16/1 "%02x " "   "' -e '16/1 "%_p" "\n" ' fname
> +

This is the wrong place to describe the md command.

Please, add a man-page to doc/usage/

Best regards

Heinrich

>
>   Environment Variables:
>   ======================
>

[PATCH v2 1/8] spl: Drop duplicate 'Jumping to U-Boot' message

[Pratyush Yadav]

On 23/01/21 10:36AM, Simon Glass wrote:
> This is printed twice but we only need one message, since there is very
> little processing in between them. Drop the first one.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> (no changes since v1)
> 
>  common/spl/spl.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index 12b00e2a407..7fe0812799f 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -693,7 +693,6 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
>  #endif
>  	switch (spl_image.os) {
>  	case IH_OS_U_BOOT:
> -		debug("Jumping to U-Boot\n");

IMO it would make more sense to drop the "jumping to u-boot" part from 
the "loaded - jumping to u-boot" message below. This way you avoid 
duplication in case of ATF, OPTEE, OpenSBI, etc. This would also avoid 
saying "jumping to u-boot" right after saying "jumping to linux".

>  		break;
>  #if CONFIG_IS_ENABLED(ATF)
>  	case IH_OS_ARM_TRUSTED_FIRMWARE:
> -- 
> 2.30.0.280.ga3ce27912f-goog
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v2 5/8] sandbox: Add os_realloc()

[Simon Glass]

Hi Heinrich,

On Sun, 24 Jan 2021 at 15:15, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 1/23/21 6:36 PM, Simon Glass wrote:
> > We provide os_malloc() and os_free() but not os_realloc(). Add this,
> > following the usual semantics. Also update os_malloc() to behave correctly
> > when passed a zero size.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
>
> In the code I am missing a comment why we are not simply using malloc(),
> realloc(), free() instead of mmap(), munmap().

OK I will add one.

>
> mmap() creates page size aligned addresses while mALLOc() in non-sandbox
> U-Boot will creates MALLOC_ALIGNMENT aligned addresses. This may result
> in errors due to misalignment not being caught on the sandbox.
>
> Hence I would prefer if os_malloc() would add a prime number times
> MALLOC_ALIGNMENT to the address returned by mmap() instead of page_size
> to match the alignment guarantee of U-Boot. E.g.
>
>      #define MALLOC_ALIGNMENT (2 * sizeof(size_t))
>
>      void *os_malloc(size_t length)
>      {
>          ...
>          return (void *)hdr + 19 * MALLOC_ALIGNMENT;
>      }

But these allocations are for internal sandbox use, so are never used
outside that code. For what you suggest I think the ram_buf could
perhaps be set up unaligned, but of course the U-Boot code uses
addresses so would be oblivious to the actual alignment and unable to
use memalign(), etc. to achieve any alignment at the hardware level.

>
> We should put a magic string into the currently unused first bytes of
> the memory provided by mmap() and check if the string is present in
> os_free(). Crash the system if the magic is not found like U-Boot's
> malloc() does.

Well you mean glibc malloc(), but the U-Boot malloc() doesn't seem to
do that. Again, this is all internal to sandbox.

>
> POSIX requires that mmap() zeros any memory that it allocates. The
> memory returned by mmap() should be filled with random bytes to catch
> more errors.
>
> What are your thought on these topics?

See above.

As an aside I would like to have a new malloc() function, something like:

malloc_tagged(size, const char *tag)

which stores the tag string with the allocation, so that it is
possible to figure out who allocated it. Good for memory leaks, double
frees, etc. Then the malloc space could be meaningfully dumped. It
would also be nice to have a way to store the function name and line
that did the malloc(), so perhaps malloc_logged()..

>
> For the current patch:
>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>

Regards,
Simon