diff mbox

[U-Boot,v2,16/21] hash: Add a flag to support saving hashes in the environment

Message ID 1361379515-23802-17-git-send-email-sjg@chromium.org
State Superseded, archived
Headers show

Commit Message

Simon Glass Feb. 20, 2013, 4:58 p.m. UTC
Some hashing commands permit saving the hash in an environment variable,
and verifying a hash from there. But the crc32 command does not support
this. In order to permit crc32 to use the generic hashing infrastructure,
add a flag to select which behaviour to use.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Add new patch to control saving hashes in the environment

 common/cmd_hash.c    |  8 +++---
 common/cmd_sha1sum.c |  6 ++---
 common/hash.c        | 69 +++++++++++++++++++++++++++++++++++++++-------------
 include/hash.h       |  9 +++++--
 4 files changed, 66 insertions(+), 26 deletions(-)

Comments

Wolfgang Denk Feb. 21, 2013, 12:04 p.m. UTC | #1
Dear Simon Glass,

In message <1361379515-23802-17-git-send-email-sjg@chromium.org> you wrote:
> Some hashing commands permit saving the hash in an environment variable,
> and verifying a hash from there. But the crc32 command does not support
> this. In order to permit crc32 to use the generic hashing infrastructure,
> add a flag to select which behaviour to use.

<nitpick>

...
> +	/*
> +	 * If environment variables are allowed, then we assume that 'dest'
> +	 * is an environment variable, unless it starts with *, in which
> +	 * case we assume it is an address. If not allowed, it ia always an

s/it ia/it is/

</nitpick>

What's the impact of these changes on the memory footprint?

Best regards,

Wolfgang Denk
Simon Glass Feb. 25, 2013, 6:21 a.m. UTC | #2
Hi Wolfgang,

On Thu, Feb 21, 2013 at 4:04 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1361379515-23802-17-git-send-email-sjg@chromium.org> you wrote:
>> Some hashing commands permit saving the hash in an environment variable,
>> and verifying a hash from there. But the crc32 command does not support
>> this. In order to permit crc32 to use the generic hashing infrastructure,
>> add a flag to select which behaviour to use.
>
> <nitpick>
>
> ...
>> +     /*
>> +      * If environment variables are allowed, then we assume that 'dest'
>> +      * is an environment variable, unless it starts with *, in which
>> +      * case we assume it is an address. If not allowed, it ia always an
>
> s/it ia/it is/
>
> </nitpick>
>
> What's the impact of these changes on the memory footprint?

Overall it is pretty neutral, although there are some variations. For
powerpc the overall affect is to save an average of 13.9 bytes across
the whole image. arm does a bit better:

24: hash: Use lower case for hash algorithm names
   sandbox:    sandbox
       x86: (for 1/1 boards)  all -74.0  data +16.0  rodata +10.0  text -100.0
      m68k: (for 41/50 boards)  all -44.7  bss +0.5  data +0.8  rodata
-1.0  text -44.9
   powerpc: (for 633/635 boards)  all -13.9  bss +14.4  data -3.6
rodata -19.6  text -5.1
   sandbox: (for 1/1 boards)  all +24336.0  bss +48.0  data +168.0
rodata +432.0  text +23688.0
        sh: (for 16/21 boards)  all +18.8  rodata -1.2  text +20.0
     nios2: (for 3/3 boards)  all +277.3  text +277.3
microblaze: (for 1/1 boards)  all -832.0  bss +8.0  data -16.0  rodata
-120.0  text -704.0
       arm: (for 292/292 boards)  all -277.8  bss +0.4  data +0.3
rodata -3.8  spl/u-boot-spl:all -0.2  spl/u-boot-spl:rodata -0.1
spl/u-boot-spl:text -0.1  text -274.7
     nds32: (for 3/3 boards)  all -116.0  data +4.0  rodata -16.0  text -104.0

Bad boards define CONFIG_CMD_CRC32 along with CONFIG_CRC32_VERIFY,
thus bringing in the hash code, but they don't enable the hash command
or have sha1, so this is the worst case. Good boards benefit from the
getenv/setenv helpers and use the small version of the hash code
(since they only define CONFIG_CMD_CRC32). The vast majority of boards
show a slight improvement overall.

This is the worst powerpc board:

   powerpc: (for 2/2 boards)  all +284.5  bss +12.0  rodata -13.5  text +286.0
            stxxtc         :  all +941  bss +24  data +16  rodata +89  text +812
               u-boot: add: 8/0, grow: 3/-10 bytes: 1254/-432 (822)
                 function                                   old     new   delta
                 hash_command                                 -     740    +740
                 show_hash                                    -     108    +108
                 simple_itoa                                  -     104    +104
                 crc32_wd_buf                                 -      76     +76
                 setenv_hex                                   -      64     +64
                 setenv_ulong                                 -      52     +52
                 do_mem_mtest                               468     512     +44
                 static.local                                 -      22     +22
                 do_mem_loop                                268     288     +20
                 hash_algo                                    -      16     +16
                 do_mem_cmp                                 356     364      +8
                 do_mem_mw                                  224     220      -4
                 set_working_fdt_addr                        68      52     -16
                 load_serial_ymodem                         288     272     -16
                 load_serial                                504     488     -16
                 do_load_serial_bin                        1812    1796     -16
                 do_imgextract                              656     620     -36
                 NetLoop                                    784     748     -36
                 do_bootm                                  1216    1164     -52
                 do_mem_cp                                  344     288     -56
                 do_mem_crc                                 308     124    -184

This is the best powerpc board:

            P2041RDB_SRIO_PCIE_BOOT:  all -372  data -16  rodata -116  text -240
               u-boot: add: 5/-1, grow: 2/-15 bytes: 446/-640 (-194)
                 function                                   old     new   delta
                 hash_command                                 -     176    +176
                 simple_itoa                                  -     104    +104
                 setenv_hex                                   -      64     +64
                 setenv_ulong                                 -      52     +52
                 static.local                                 -      22     +22
                 do_mem_loop                                268     288     +20
                 do_mem_cmp                                 324     332      +8
                 do_mem_mw                                  224     220      -4
                 set_working_fdt_addr                        68      52     -16
                 load_serial_ymodem                         228     212     -16
                 load_serial                                460     444     -16
                 hash_algo                                   16       -     -16
                 fm_init_common                            1336    1320     -16
                 do_load_serial_bin                        1812    1796     -16
                 do_load                                    460     444     -16
                 do_imgextract                              864     828     -36
                 NetLoop                                    780     744     -36
                 nand_print_and_set_info                    224     172     -52
                 do_bootm                                  1236    1184     -52
                 do_setexpr                                 436     376     -60
                 do_mem_cp                                  244     156     -88
                 do_mem_crc                                 184      88     -96
                 do_mem_mtest                              1400    1296    -104

This is tricky because I am trying to bring in a generic
infrastructure for hashing and make use of it everywhere, but
unfortunately those boards that don't really make much use of it still
have to pay a price. That said, only 15 powerpc boards are in the
'bad' camp and none is as bad as stxxtc.

There is also some saving to be had by changing simple_itoa() to use
the existing put_dec() if we want to. Overall I'm hoping that the
crc32 patch is worth the trouble.

Regards,
Simon

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> Let us not look back in anger or  forward  in  fear,  but  around  in
> awareness.                                            - James Thurber
diff mbox

Patch

diff --git a/common/cmd_hash.c b/common/cmd_hash.c
index eb6a338..8c03b5c 100644
--- a/common/cmd_hash.c
+++ b/common/cmd_hash.c
@@ -30,22 +30,22 @@ 
 static int do_hash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 #ifdef CONFIG_HASH_VERIFY
-	int verify = 0;
+	int flags = HASH_FLAG_ENV;
 
 	if (argc < 4)
 		return CMD_RET_USAGE;
 	if (!strcmp(argv[1], "-v")) {
-		verify = 1;
+		flags |= HASH_FLAG_VERIFY;
 		argc--;
 		argv++;
 	}
 #else
-	const int verify = 0;
+	const int flags = HASH_FLAG_ENV;
 #endif
 	/* Move forward to 'algorithm' parameter */
 	argc--;
 	argv++;
-	return hash_command(*argv, verify, cmdtp, flag, argc - 1, argv + 1);
+	return hash_command(*argv, flags, cmdtp, flag, argc - 1, argv + 1);
 }
 
 #ifdef CONFIG_HASH_VERIFY
diff --git a/common/cmd_sha1sum.c b/common/cmd_sha1sum.c
index fe927ab..9f08629 100644
--- a/common/cmd_sha1sum.c
+++ b/common/cmd_sha1sum.c
@@ -31,7 +31,7 @@ 
 
 int do_sha1sum(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
-	int verify = 0;
+	int flags = HASH_FLAG_ENV;
 	int ac;
 	char * const *av;
 
@@ -42,13 +42,13 @@  int do_sha1sum(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	ac = argc - 1;
 #ifdef CONFIG_SHA1SUM_VERIFY
 	if (strcmp(*av, "-v") == 0) {
-		verify = 1;
+		flags |= HASH_FLAG_VERIFY;
 		av++;
 		ac--;
 	}
 #endif
 
-	return hash_command("sha1", verify, cmdtp, flag, ac, av);
+	return hash_command("sha1", flags, cmdtp, flag, ac, av);
 }
 
 #ifdef CONFIG_SHA1SUM_VERIFY
diff --git a/common/hash.c b/common/hash.c
index e3a6e43..021c4d9 100644
--- a/common/hash.c
+++ b/common/hash.c
@@ -58,19 +58,31 @@  static struct hash_algo hash_algo[] = {
  * @algo:		Hash algorithm being used
  * @sum:		Hash digest (algo->digest_size bytes)
  * @dest:		Destination, interpreted as a hex address if it starts
- *			with * or otherwise as an environment variable.
+ *			with * (or allow_env_vars is 0) or otherwise as an
+ *			environment variable.
+ * @allow_env_vars:	non-zero to permit storing the result to an
+ *			variable environment
  */
 static void store_result(struct hash_algo *algo, const u8 *sum,
-			 const char *dest)
+			 const char *dest, int allow_env_vars)
 {
 	unsigned int i;
+	int env_var = 0;
 
-	if (*dest == '*') {
-		u8 *ptr;
+	/*
+	 * If environment variables are allowed, then we assume that 'dest'
+	 * is an environment variable, unless it starts with *, in which
+	 * case we assume it is an address. If not allowed, it ia always an
+	 * address. This is to support the crc32 command.
+	 */
+	if (allow_env_vars) {
+		if (*dest == '*')
+			dest++;
+		else
+			env_var = 1;
+	}
 
-		ptr = (u8 *)simple_strtoul(dest + 1, NULL, 16);
-		memcpy(ptr, sum, algo->digest_size);
-	} else {
+	if (env_var) {
 		char str_output[HASH_MAX_DIGEST_SIZE * 2 + 1];
 		char *str_ptr = str_output;
 
@@ -80,6 +92,11 @@  static void store_result(struct hash_algo *algo, const u8 *sum,
 		}
 		str_ptr = '\0';
 		setenv(dest, str_output);
+	} else {
+		u8 *ptr;
+
+		ptr = (u8 *)simple_strtoul(dest, NULL, 16);
+		memcpy(ptr, sum, algo->digest_size);
 	}
 }
 
@@ -94,14 +111,28 @@  static void store_result(struct hash_algo *algo, const u8 *sum,
  *			Otherwise we assume it is an environment variable, and
  *			look up its value (it must contain a hex digest).
  * @vsum:		Returns binary digest value (algo->digest_size bytes)
+ * @allow_env_vars:	non-zero to permit storing the result to an environment
+ *			variable. If 0 then verify_str is assumed to be an
+ *			address, and the * prefix is not expected.
  * @return 0 if ok, non-zero on error
  */
-static int parse_verify_sum(struct hash_algo *algo, char *verify_str, u8 *vsum)
+static int parse_verify_sum(struct hash_algo *algo, char *verify_str, u8 *vsum,
+			    int allow_env_vars)
 {
-	if (*verify_str == '*') {
+	int env_var = 0;
+
+	/* See comment above in store_result() */
+	if (allow_env_vars) {
+		if (*verify_str == '*')
+			verify_str++;
+		else
+			env_var = 1;
+	}
+
+	if (env_var) {
 		u8 *ptr;
 
-		ptr = (u8 *)simple_strtoul(verify_str + 1, NULL, 16);
+		ptr = (u8 *)simple_strtoul(verify_str, NULL, 16);
 		memcpy(vsum, ptr, algo->digest_size);
 	} else {
 		unsigned int i;
@@ -158,7 +189,7 @@  static void show_hash(struct hash_algo *algo, ulong addr, ulong len,
 		printf("%02x", output[i]);
 }
 
-int hash_command(const char *algo_name, int verify, cmd_tbl_t *cmdtp, int flag,
+int hash_command(const char *algo_name, int flags, cmd_tbl_t *cmdtp, int flag,
 		 int argc, char * const argv[])
 {
 	struct hash_algo *algo;
@@ -169,13 +200,14 @@  int hash_command(const char *algo_name, int verify, cmd_tbl_t *cmdtp, int flag,
 	if (argc < 2)
 		return CMD_RET_USAGE;
 
+	addr = simple_strtoul(*argv++, NULL, 16);
+	len = simple_strtoul(*argv++, NULL, 16);
+
 	algo = find_hash_algo(algo_name);
 	if (!algo) {
 		printf("Unknown hash algorithm '%s'\n", algo_name);
 		return CMD_RET_USAGE;
 	}
-	addr = simple_strtoul(*argv++, NULL, 16);
-	len = simple_strtoul(*argv++, NULL, 16);
 	argc -= 2;
 
 	if (algo->digest_size > HASH_MAX_DIGEST_SIZE) {
@@ -188,13 +220,14 @@  int hash_command(const char *algo_name, int verify, cmd_tbl_t *cmdtp, int flag,
 
 	/* Try to avoid code bloat when verify is not needed */
 #ifdef CONFIG_HASH_VERIFY
-	if (verify) {
+	if (flags & HASH_FLAG_VERIFY) {
 #else
 	if (0) {
 #endif
 		if (!argc)
 			return CMD_RET_USAGE;
-		if (parse_verify_sum(algo, *argv, vsum)) {
+		if (parse_verify_sum(algo, *argv, vsum,
+					flags & HASH_FLAG_ENV)) {
 			printf("ERROR: %s does not contain a valid %s sum\n",
 				*argv, algo->name);
 			return 1;
@@ -213,8 +246,10 @@  int hash_command(const char *algo_name, int verify, cmd_tbl_t *cmdtp, int flag,
 		show_hash(algo, addr, len, output);
 		printf("\n");
 
-		if (argc)
-			store_result(algo, output, *argv);
+		if (argc) {
+			store_result(algo, output, *argv,
+				flags & HASH_FLAG_ENV);
+		}
 	}
 
 	return 0;
diff --git a/include/hash.h b/include/hash.h
index 34ba558..88fa2b5 100644
--- a/include/hash.h
+++ b/include/hash.h
@@ -51,19 +51,24 @@  struct hash_algo {
  */
 #define HASH_MAX_DIGEST_SIZE	32
 
+enum {
+	HASH_FLAG_VERIFY	= 1 << 0,	/* Enable verify mode */
+	HASH_FLAG_ENV		= 1 << 1,	/* Allow env vars */
+};
+
 /**
  * hash_command: Process a hash command for a particular algorithm
  *
  * This common function is used to implement specific hash commands.
  *
  * @algo_name:		Hash algorithm being used
- * @verify:		Non-zero to enable verify mode
+ * @flags:		Flags value (HASH_FLAG_...)
  * @cmdtp:		Pointer to command table entry
  * @flag:		Some flags normally 0 (see CMD_FLAG_.. above)
  * @argc:		Number of arguments (arg 0 must be the command text)
  * @argv:		Arguments
  */
-int hash_command(const char *algo_name, int verify, cmd_tbl_t *cmdtp, int flag,
+int hash_command(const char *algo_name, int flags, cmd_tbl_t *cmdtp, int flag,
 		 int argc, char * const argv[]);
 
 #endif