Re: [U-Boot] [RFC] sunxi: Store the device tree name in the SPL header

Am 02.06.2016 um 12:32 schrieb Siarhei Siamashka:

This patch updates the mksunxiboot tool to optionally add the default device tree name string to the SPL header. This information can be used by the firmware upgrade tools to protect users from harming themselves by trying to upgrade to an incompatible bootloader.

The primary use case here is a non-removable bootable media (such as NAND, eMMC or SPI flash), which already may have a properly working, but a little bit outdated bootloader installed. For example, the user may download or build a new U-Boot image for "Cubieboard", and then attemept to install it on a "Cubieboard2" hardware by mistake as a replacement for the already existing bootloader. If this happens, the flash programming tool can identify this problem and warn the user.

CONFIG_OF_LIST may provide more than one compatible device tree name when supporting multiple boards by a single defconfig. In such cases, the right device tree name is getting identified at runtime from the list of multiple options. The patch calculates a 32-bit hash value (crc32) of this list and stores it to the SPL header too, because this is better than nothing and still can be used to warn about potentially incompatible upgrades.

The size of the SPL header is also increased from 64 bytes to 96 bytes to provide enough space for the device tree name string.

Signed-off-by: Siarhei Siamashka siarhei.siamashka@gmail.com

arch/arm/include/asm/arch-sunxi/spl.h | 26 +++++++++++-- include/configs/sunxi-common.h | 12 +++--- scripts/Makefile.spl | 3 +- tools/Makefile | 1 + tools/mksunxiboot.c | 71 +++++++++++++++++++++++++++++++++-- 5 files changed, 98 insertions(+), 15 deletions(-)

Even with this, I guess there are still (slim) chances that users get themselves into trouble - e.g. by having 'bricked' their devices after they flashed an incompatible SPL/U-Boot, and then trying to repair the damage via "FEL recovery". (The existing SPL would carry the wrong info.)

But it seems a useful approach, as long as the flasher only warns and allows to continue - possibly requiring confirmation (enter "yes" to proceed) or the use of some "--force" option. In general, I like the idea.

diff --git a/arch/arm/include/asm/arch-sunxi/spl.h b/arch/arm/include/asm/arch-sunxi/spl.h index a0f33b0..f370222 100644 --- a/arch/arm/include/asm/arch-sunxi/spl.h +++ b/arch/arm/include/asm/arch-sunxi/spl.h @@ -10,7 +10,7 @@

#define BOOT0_MAGIC "eGON.BT0" #define SPL_SIGNATURE "SPL" /* marks "sunxi" SPL header */ -#define SPL_HEADER_VERSION 1 +#define SPL_HEADER_VERSION 2

#if defined(CONFIG_MACH_SUN9I) || defined(CONFIG_MACH_SUN50I) #define SPL_ADDR 0x10000 @@ -49,9 +49,27 @@ struct boot_file_head { uint8_t spl_signature[4]; }; uint32_t fel_script_address;

• uint32_t reserved1[3];
• uint32_t boot_media; /* written here by the boot ROM */
• uint32_t reserved2[5]; /* padding, align to 64 bytes */

• uint32_t reserved1[1];

There's no "reserved2" any more, and no need to keep that an array, so just "uint32_t reserved;"?

• /*

• * Offset of an ASCIIZ string (relative to the SPL header), which
  

• * contains the default device tree name (CONFIG_DEFAULT_DEVICE_TREE).
  

• * This is optional and may be set to NULL. Is intended to be used
  

• * by flash programming tools for providing nice informative messages
  

• * to the users.
  

• */
  

• uint32_t offset_of_the_default_device_tree_name;
• /*

• * A 32-bit hash value, calculated from the list of compatible device
  

• * tree names (CONFIG_OF_LIST or CONFIG_DEFAULT_DEVICE_TREE). Is
  

• * intended to be used by flash programming tools for safeguearding
  

• * against upgrading to an incompatible firmware.
  

• */
  

• uint32_t crc32_of_the_compatible_device_tree_list;
• /* The 'boot_media' code is written here by the boot ROM */
• uint32_t boot_media;
• /* A padding area (may be used for storing text strings) */
• uint32_t string_pool[13];
• /* The header must be a multiple of 32 bytes (for VBAR alignment) */

I see no particular reason why this should be uint32_t[13] instead of char[52], with a suitable (multiple of 4) array size to ensure proper header alignment. You're introducing a check / safeguard for the header size anyway, and char[] saves us from having to do a typecast later on.

};

#define is_boot0_magic(addr) (memcmp((void *)addr, BOOT0_MAGIC, 8) == 0) diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h index b33cfb8..e184d0c 100644 --- a/include/configs/sunxi-common.h +++ b/include/configs/sunxi-common.h @@ -189,14 +189,14 @@ #define CONFIG_SPL_BOARD_LOAD_IMAGE

#if defined(CONFIG_MACH_SUN9I) -#define CONFIG_SPL_TEXT_BASE 0x10040 /* sram start+header */ -#define CONFIG_SPL_MAX_SIZE 0x5fc0 /* ? KiB on sun9i */ +#define CONFIG_SPL_TEXT_BASE 0x10060 /* sram start+header */ +#define CONFIG_SPL_MAX_SIZE 0x5fa0 /* ? KiB on sun9i */ #elif defined(CONFIG_MACH_SUN50I) -#define CONFIG_SPL_TEXT_BASE 0x10040 /* sram start+header */ -#define CONFIG_SPL_MAX_SIZE 0x7fc0 /* 32 KiB on sun50i */ +#define CONFIG_SPL_TEXT_BASE 0x10060 /* sram start+header */ +#define CONFIG_SPL_MAX_SIZE 0x7fa0 /* 32 KiB on sun50i */ #else -#define CONFIG_SPL_TEXT_BASE 0x40 /* sram start+header */ -#define CONFIG_SPL_MAX_SIZE 0x5fc0 /* 24KB on sun4i/sun7i */ +#define CONFIG_SPL_TEXT_BASE 0x60 /* sram start+header */ +#define CONFIG_SPL_MAX_SIZE 0x5fa0 /* 24KB on sun4i/sun7i */ #endif

#define CONFIG_SPL_LIBDISK_SUPPORT diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index 6d2017d..54d8a2e 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -240,7 +240,8 @@ $(obj)/$(SPL_BIN).sfp: $(obj)/$(SPL_BIN).bin FORCE $(call if_changed,mkimage)

quiet_cmd_mksunxiboot = MKSUNXI $@ -cmd_mksunxiboot = $(objtree)/tools/mksunxiboot $< $@ +cmd_mksunxiboot = $(objtree)/tools/mksunxiboot --default-dt "$(CONFIG_DEFAULT_DEVICE_TREE)" \

• 			       --compatible-dt-list "$(CONFIG_OF_LIST)" $< $@
  

  $(obj)/sunxi-spl.bin: $(obj)/$(SPL_BIN).bin FORCE $(call if_changed,mksunxiboot)

diff --git a/tools/Makefile b/tools/Makefile index 63355aa..e6c0481 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -155,6 +155,7 @@ hostprogs-$(CONFIG_MX28) += mxsboot HOSTCFLAGS_mxsboot.o := -pedantic

hostprogs-$(CONFIG_SUNXI) += mksunxiboot +mksunxiboot-objs := mksunxiboot.o lib/crc32.o

hostprogs-$(CONFIG_NETCONSOLE) += ncb hostprogs-$(CONFIG_SHA1_CHECK_UB_IMG) += ubsha1 diff --git a/tools/mksunxiboot.c b/tools/mksunxiboot.c index 9c1c5b7..b45acec 100644 --- a/tools/mksunxiboot.c +++ b/tools/mksunxiboot.c @@ -17,6 +17,8 @@ #include <sys/stat.h> #include "asm/arch/spl.h"

+#include <u-boot/crc.h>

• #define STAMP_VALUE 0x5F0A6C39

  /* check sum functon from sun4i boot code */

@@ -70,11 +72,55 @@ int main(int argc, char *argv[]) struct boot_img img; unsigned file_size; int count;

• char *tool_name = argv[0];
• char *default_dt = NULL;
• char *compatible_dt_list = NULL;
• /* a sanity check */
• if ((sizeof(img.header) % 32) != 0) {

• fprintf(stderr, "ERROR: the SPL header must be a multiple ");
  

• fprintf(stderr, "of 32 bytes.\n");
  

Nitpick / coding style: You're using multiple fprintf(stderr, ...) where a simple string concatenation would do. Same applies for some printf() further down. Not a real concern though.

• return EXIT_FAILURE;
  

• }
• /* process optional command line switches */
• while (argc >= 2 && argv[1][0] == '-') {

• if (strcmp(argv[1], "--default-dt") == 0) {
  

• 	if (argc >= 3) {
  

• 		default_dt = argv[2];
  

• 		argv += 2;
  

• 		argc -= 2;
  

• 		continue;
  

• 	}
  

• 	fprintf(stderr, "ERROR: no --default-dt arg\n");
  

• 	return EXIT_FAILURE;
  

• } else if (strcmp(argv[1], "--compatible-dt-list") == 0) {
  

• 	if (argc >= 3) {
  

• 		compatible_dt_list = argv[2];
  

• 		argv += 2;
  

• 		argc -= 2;
  

• 		continue;
  

• 	}
  

• 	fprintf(stderr, "ERROR: no --compatible-dt-list arg\n");
  

• 	return EXIT_FAILURE;
  

• } else {
  

• 	fprintf(stderr, "ERROR: bad option '%s'\n", argv[1]);
  

• 	return EXIT_FAILURE;
  

• }
  

• }

• if (argc < 2) {

• printf("\tThis program makes an input bin file to sun4i " \
  

•        "bootable image.\n" \
  

•        "\tUsage: %s input_file out_putfile\n", argv[0]);
  

• if (!compatible_dt_list || strlen(compatible_dt_list) == 0)

• compatible_dt_list = default_dt;
  

• if (argc < 3) {

• printf("This program converts an input binary file to a sunxi bootable image.\n");
  

• printf("\nUsage: %s [options] input_file output_file\n",
  

•        tool_name);
  

• printf("Where [options] may be:\n");
  

• printf("  --default-dt arg         - 'arg' is the default device tree name\n");
  

• printf("                             (CONFIG_DEFAULT_DEVICE_TREE).\n");
  

• printf("  --compatible-dt-list arg - 'arg' is the space separated list of the\n");
  

• printf("                             compatible device tree names (CONFIG_OF_LIST).\n");
  

  return EXIT_FAILURE; }

@@ -122,6 +168,23 @@ int main(int argc, char *argv[]) memcpy(img.header.spl_signature, SPL_SIGNATURE, 3); /* "sunxi" marker */ img.header.spl_signature[3] = SPL_HEADER_VERSION;

• if (default_dt) {

• if (strlen(default_dt) + 1 <= sizeof(img.header.string_pool)) {
  

• 	strcpy((char *)img.header.string_pool, default_dt);
  

(The cast would be obsolete with a "char stringpool[];" type.)

• 	img.header.offset_of_the_default_device_tree_name =
  

• 		cpu_to_le32(offsetof(struct boot_file_head,
  

• 				     string_pool));
  

• } else {
  

• 	printf("WARNING: The SPL header is too small\n");
  

• 	printf("         and has no space to store the dt name.\n");
  

• }
  

• }

• if (compatible_dt_list) {

• img.header.crc32_of_the_compatible_device_tree_list =
  

• 	cpu_to_le32(crc32(0, (uint8_t *)compatible_dt_list,
  

• 			  strlen(compatible_dt_list)));
  

• }

• gen_check_sum(&img.header);

  count = write(fd_out, &img, le32_to_cpu(img.header.length));

Regards, B. Nortmann