| Message ID | 1397747435-24042-3-git-send-email-p.aubert@staubli.com |
|---|---|
| State | Superseded |
| Delegated to: | Pantelis Antoniou |
| Headers | show |
Dear Pierre Aubert, In message <1397747435-24042-3-git-send-email-p.aubert@staubli.com> you wrote: > This sub-command adds support for the RPMB partition of an eMMC: > * mmc rpmb key <address of the authentication key> > Programs the authentication key in the eMMC This key can not > be overwritten. > * mmc rpmb read <address> <block> <#count> [address of key] > Reads <#count> blocks of 256 bytes in the RPMB partition > beginning at block number <block>. If the optionnal > address of the authentication key is provided, the > Message Authentication Code (MAC) is verified on each > block. > * mmc rpmb write <address> <block> <#count> <address of key> > Writes <#count> blocks of 256 bytes in the RPMB partition > beginning at block number <block>. The datas are signed > with the key provided. > * mmc rpmb counter > Returns the 'Write counter' of the RPMB partition. > > The sub-command is conditional on compilation flag CONFIG_SUPPORT_EMMC_RPMB Such new options must be documented in the README. > Signed-off-by: Pierre Aubert <p.aubert@staubli.com> > CC: Pantelis Antoniou <panto@antoniou-consulting.com> > --- > common/cmd_mmc.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 files changed, 127 insertions(+), 1 deletions(-) > > diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c > index c1916c9..3cf11e7 100644 > --- a/common/cmd_mmc.c > +++ b/common/cmd_mmc.c > @@ -130,7 +130,123 @@ U_BOOT_CMD( > "display MMC info", > "- display info of the current MMC device" > ); > +#ifdef CONFIG_SUPPORT_EMMC_RPMB > +static int confirm_key_prog(void) > +{ > + puts("Warning: Programming authentication key can be done only once !\n" > + " Use this command only if you are sure of what you are doing,\n" > + "Really perform the key programming ? "); > + if (getc() == 'y') { Would it not makes sense to flush the input before reading the char, so that you don;t react on any type-ahead that might already be buffered? > + int c; > + > + putc('y'); > + c = getc(); > + putc('\n'); > + if (c == '\r') > + return 1; > + } Should we allow for 'Y"? And for "yes" / "Yes"? We have getenv_yesno() - maybe we should provide a similar function that can be used in other places where such interactive confirmation is needed? > + if (state != RPMB_INVALID) { Change this into if (state == RPMB_INVALID) return CMD_RET_USAGE; and avoid one level of indentation; this will make the code much easier to read. > + if (IS_SD(mmc)) { Is IS_SD() a reliable test for eMMC devics, or would that also return true in other cases? > + if (confirm_key_prog()) { > + if (mmc_rpmb_set_key(mmc, key_addr)) { > + printf("ERROR - Key already programmed ?\n"); > + ret = CMD_RET_FAILURE; > + } > + } else { > + ret = CMD_RET_FAILURE; > + } You should really avoid deep nesting and take early exits. You can write this as: if (!confirm_key_prog()) return CMD_RET_FAILURE; if (mmc_rpmb_set_key(mmc, key_addr)) { printf("ERROR - Key already programmed ?\n"); ret = CMD_RET_FAILURE; } Please fix globally. > + } else if (state == RPMB_COUNTER) { > + unsigned long counter; > + if (mmc_rpmb_get_counter(mmc, &counter)) Please insert a blank line between declarations and code. > + printf("%d RPMB blocks %s: %s\n", > + n, argv[2], (n == cnt) ? "OK" : "ERROR"); As the input is in hex, it is usually also a good idea to (also) print the count in hex. > #endif /* CONFIG_SUPPORT_EMMC_BOOT */ > +#ifdef CONFIG_SUPPORT_EMMC_RPMB > + } else if (strcmp(argv[1], "rpmb") == 0) { > + return do_mmcrpmb(argc, argv); > +#endif /* CONFIG_SUPPORT_EMMC_RPMB */ I think that now, with more subcommands being added, we should convert the mmc code to proper subcommand handling. [It might even make sense to do so for "mmc rpmb", too.] Best regards, Wolfgang Denk
Hello Wolfgang, Le 17/04/2014 21:56, Wolfgang Denk a écrit : > Dear Pierre Aubert, > > In message <1397747435-24042-3-git-send-email-p.aubert@staubli.com> you wrote: >> This sub-command adds support for the RPMB partition of an eMMC: >> * mmc rpmb key <address of the authentication key> >> Programs the authentication key in the eMMC This key can not >> be overwritten. >> * mmc rpmb read <address> <block> <#count> [address of key] >> Reads <#count> blocks of 256 bytes in the RPMB partition >> beginning at block number <block>. If the optionnal >> address of the authentication key is provided, the >> Message Authentication Code (MAC) is verified on each >> block. >> * mmc rpmb write <address> <block> <#count> <address of key> >> Writes <#count> blocks of 256 bytes in the RPMB partition >> beginning at block number <block>. The datas are signed >> with the key provided. >> * mmc rpmb counter >> Returns the 'Write counter' of the RPMB partition. >> >> The sub-command is conditional on compilation flag CONFIG_SUPPORT_EMMC_RPMB > Such new options must be documented in the README. I will add it in a V3 > >> Signed-off-by: Pierre Aubert <p.aubert@staubli.com> >> CC: Pantelis Antoniou <panto@antoniou-consulting.com> >> --- >> common/cmd_mmc.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 files changed, 127 insertions(+), 1 deletions(-) >> >> diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c >> index c1916c9..3cf11e7 100644 >> --- a/common/cmd_mmc.c >> +++ b/common/cmd_mmc.c >> @@ -130,7 +130,123 @@ U_BOOT_CMD( >> "display MMC info", >> "- display info of the current MMC device" >> ); >> +#ifdef CONFIG_SUPPORT_EMMC_RPMB >> +static int confirm_key_prog(void) >> +{ >> + puts("Warning: Programming authentication key can be done only once !\n" >> + " Use this command only if you are sure of what you are doing,\n" >> + "Really perform the key programming ? "); >> + if (getc() == 'y') { > Would it not makes sense to flush the input before reading the char, > so that you don;t react on any type-ahead that might already be > buffered? > >> + int c; >> + >> + putc('y'); >> + c = getc(); >> + putc('\n'); >> + if (c == '\r') >> + return 1; >> + } > Should we allow for 'Y"? And for "yes" / "Yes"? > > We have getenv_yesno() - maybe we should provide a similar function > that can be used in other places where such interactive confirmation > is needed? I have found such a confirmation in cmd_fuse, cmd_otp and cmd_mmc. It makes sense to provide a global function. I will try to submit a patch for that. >> + if (state != RPMB_INVALID) { > Change this into > > if (state == RPMB_INVALID) > return CMD_RET_USAGE; > > and avoid one level of indentation; this will make the code much > easier to read. > >> + if (IS_SD(mmc)) { > Is IS_SD() a reliable test for eMMC devics, or would that also return > true in other cases? You're right, the test must be more restrictive. The RPMB partition is available only since the release 4.41 of the Jedec standard. I will fix it in a V3. > >> + if (confirm_key_prog()) { >> + if (mmc_rpmb_set_key(mmc, key_addr)) { >> + printf("ERROR - Key already programmed ?\n"); >> + ret = CMD_RET_FAILURE; >> + } >> + } else { >> + ret = CMD_RET_FAILURE; >> + } > You should really avoid deep nesting and take early exits. > You can write this as: > > if (!confirm_key_prog()) > return CMD_RET_FAILURE; > > if (mmc_rpmb_set_key(mmc, key_addr)) { > printf("ERROR - Key already programmed ?\n"); > ret = CMD_RET_FAILURE; > } > > Please fix globally. No problem, it will be fixed globally in V3 > >> + } else if (state == RPMB_COUNTER) { >> + unsigned long counter; >> + if (mmc_rpmb_get_counter(mmc, &counter)) > Please insert a blank line between declarations and code. Ok > >> + printf("%d RPMB blocks %s: %s\n", >> + n, argv[2], (n == cnt) ? "OK" : "ERROR"); > As the input is in hex, it is usually also a good idea to (also) print > the count in hex. For coherency with the mmc read and mmc write, I kept the same output. But it can be changed, of course. > >> #endif /* CONFIG_SUPPORT_EMMC_BOOT */ >> +#ifdef CONFIG_SUPPORT_EMMC_RPMB >> + } else if (strcmp(argv[1], "rpmb") == 0) { >> + return do_mmcrpmb(argc, argv); >> +#endif /* CONFIG_SUPPORT_EMMC_RPMB */ > I think that now, with more subcommands being added, we should > convert the mmc code to proper subcommand handling. [It might even > make sense to do so for "mmc rpmb", too.] Do you think about the use of the macro U_BOOT_CMD_MKENT ? > > Best regards, > > Wolfgang Denk > Best regards Pierre Aubert
Dear Pierre, In message <5350C8BC.9000607@staubli.com> you wrote: > > > I think that now, with more subcommands being added, we should > > convert the mmc code to proper subcommand handling. [It might even > > make sense to do so for "mmc rpmb", too.] > Do you think about the use of the macro U_BOOT_CMD_MKENT ? Yes - see for example how the "env" sub-commands are implemented. Best regards, Wolfgang Denk
diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c index c1916c9..3cf11e7 100644 --- a/common/cmd_mmc.c +++ b/common/cmd_mmc.c @@ -130,7 +130,123 @@ U_BOOT_CMD( "display MMC info", "- display info of the current MMC device" ); +#ifdef CONFIG_SUPPORT_EMMC_RPMB +static int confirm_key_prog(void) +{ + puts("Warning: Programming authentication key can be done only once !\n" + " Use this command only if you are sure of what you are doing,\n" + "Really perform the key programming ? "); + if (getc() == 'y') { + int c; + + putc('y'); + c = getc(); + putc('\n'); + if (c == '\r') + return 1; + } + puts("Authentication key programming aborted\n"); + return 0; +} +static int do_mmcrpmb(int argc, char * const argv[]) +{ + enum rpmb_state { + RPMB_INVALID, + RPMB_READ, + RPMB_WRITE, + RPMB_KEY, + RPMB_COUNTER, + } state; + + state = RPMB_INVALID; + if (argc == 4 && strcmp(argv[2], "key") == 0) + state = RPMB_KEY; + if ((argc == 6 || argc == 7) && strcmp(argv[2], "read") == 0) + state = RPMB_READ; + else if (argc == 7 && strcmp(argv[2], "write") == 0) + state = RPMB_WRITE; + else if (argc == 3 && strcmp(argv[2], "counter") == 0) + state = RPMB_COUNTER; + + if (state != RPMB_INVALID) { + struct mmc *mmc = find_mmc_device(curr_device); + void *key_addr; + char original_part; + int ret; + + if (!mmc) { + printf("no mmc device at slot %x\n", curr_device); + return CMD_RET_FAILURE; + } + mmc_init(mmc); + if (IS_SD(mmc)) { + printf("It is not a EMMC device\n"); + return CMD_RET_FAILURE; + } + /* Switch to the RPMB partition */ + original_part = mmc->part_num; + if (mmc->part_num != MMC_PART_RPMB) { + if (mmc_switch_part(curr_device, MMC_PART_RPMB) != 0) + return CMD_RET_FAILURE; + mmc->part_num = MMC_PART_RPMB; + } + ret = CMD_RET_SUCCESS; + if (state == RPMB_KEY) { + key_addr = (void *)simple_strtoul(argv[3], NULL, 16); + if (confirm_key_prog()) { + if (mmc_rpmb_set_key(mmc, key_addr)) { + printf("ERROR - Key already programmed ?\n"); + ret = CMD_RET_FAILURE; + } + } else { + ret = CMD_RET_FAILURE; + } + } else if (state == RPMB_COUNTER) { + unsigned long counter; + if (mmc_rpmb_get_counter(mmc, &counter)) + ret = CMD_RET_FAILURE; + else + printf("Write counter= %lx\n", counter); + } else { + u16 blk, cnt; + void *addr; + int n; + + addr = (void *)simple_strtoul(argv[3], NULL, 16); + blk = simple_strtoul(argv[4], NULL, 16); + cnt = simple_strtoul(argv[5], NULL, 16); + + if (state == RPMB_READ) { + key_addr = (argc == 7) ? + (void *)simple_strtoul(argv[6], + NULL, 16) : + NULL; + n = mmc_rpmb_read(mmc, addr, blk, cnt, + key_addr); + } else { + key_addr = (void *)simple_strtoul(argv[6], + NULL, 16); + n = mmc_rpmb_write(mmc, addr, blk, cnt, + key_addr); + } + printf("%d RPMB blocks %s: %s\n", + n, argv[2], (n == cnt) ? "OK" : "ERROR"); + if (n != cnt) + ret = CMD_RET_FAILURE; + } + + /* Return to orginal partition */ + if (mmc->part_num != original_part) { + if (mmc_switch_part(curr_device, original_part) != 0) + return CMD_RET_FAILURE; + mmc->part_num = original_part; + } + return ret; + } else + return CMD_RET_USAGE; +} +#endif static int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { enum mmc_state state; @@ -365,6 +481,10 @@ static int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return mmc_set_rst_n_function(mmc, enable); #endif /* CONFIG_SUPPORT_EMMC_BOOT */ +#ifdef CONFIG_SUPPORT_EMMC_RPMB + } else if (strcmp(argv[1], "rpmb") == 0) { + return do_mmcrpmb(argc, argv); +#endif /* CONFIG_SUPPORT_EMMC_RPMB */ } else if (argc == 3 && strcmp(argv[1], "setdsr") == 0) { @@ -454,7 +574,7 @@ static int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) } U_BOOT_CMD( - mmc, 6, 1, do_mmcops, + mmc, 7, 1, do_mmcops, "MMC sub system", "read addr blk# cnt\n" "mmc write addr blk# cnt\n" @@ -474,6 +594,12 @@ U_BOOT_CMD( " - Change the RST_n_FUNCTION field of the specified device\n" " WARNING: This is a write-once field and 0 / 1 / 2 are the only valid values.\n" #endif +#ifdef CONFIG_SUPPORT_EMMC_RPMB + "mmc rpmb read addr blk# cnt [address of auth-key] - block size is 256 bytes\n" + "mmc rpmb write addr blk# cnt <address of auth-key> - block size is 256 bytes\n" + "mmc rpmb key <address of auth-key> - program the RPMB authentication key.\n" + "mmc rpmb counter - read the value of the write counter\n" +#endif "mmc setdsr - set DSR register value\n" ); #endif /* !CONFIG_GENERIC_MMC */
This sub-command adds support for the RPMB partition of an eMMC: * mmc rpmb key <address of the authentication key> Programs the authentication key in the eMMC This key can not be overwritten. * mmc rpmb read <address> <block> <#count> [address of key] Reads <#count> blocks of 256 bytes in the RPMB partition beginning at block number <block>. If the optionnal address of the authentication key is provided, the Message Authentication Code (MAC) is verified on each block. * mmc rpmb write <address> <block> <#count> <address of key> Writes <#count> blocks of 256 bytes in the RPMB partition beginning at block number <block>. The datas are signed with the key provided. * mmc rpmb counter Returns the 'Write counter' of the RPMB partition. The sub-command is conditional on compilation flag CONFIG_SUPPORT_EMMC_RPMB Signed-off-by: Pierre Aubert <p.aubert@staubli.com> CC: Pantelis Antoniou <panto@antoniou-consulting.com> --- common/cmd_mmc.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 127 insertions(+), 1 deletions(-)