[AMD Official Use Only - AMD Internal Distribution Only] Hi Tomas,
Thank you for the review. > Add command for checking if boot was authenticated. > > Signed-off-by: Igor Opaniuk <[email protected]> > Signed-off-by: Neal Frager <[email protected]> > --- > V1->V2: > - extended zynqmp command with verify_auth sub-command > - changed return value, so it can be used with scripts > --- > arch/arm/mach-zynqmp/zynqmp.c | 44 ++++++++++++++++++++++++++++++++++- > 1 file changed, 43 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-zynqmp/zynqmp.c b/arch/arm/mach-zynqmp/zynqmp.c > index c0398a466ff..b15c31919c6 100644 > --- a/arch/arm/mach-zynqmp/zynqmp.c > +++ b/arch/arm/mach-zynqmp/zynqmp.c > @@ -362,6 +362,45 @@ static int do_zynqmp_reboot(struct cmd_tbl *cmdtp, int > flag, > return CMD_RET_SUCCESS; > } > > +static bool zynqmp_verify_auth(void) > +{ > + u32 status = 0; > + int ret; > + > + ret = zynqmp_mmio_read((ulong)&csu_base->status, &status); > + if (ret) { > + printf("Can't obtain boot auth state"); > + return false; > Using boolean as return value here is not practical as this condition > should rather signal error than return false. Good point. I will change it to int. > + } > + > + return (status & BIT(0)); > It would be better to have a named bit here instead. I will fix this. > +} > + > +static int do_zynqmp_verify_auth(struct cmd_tbl *cmdtp, int flag, > + int argc, char * const argv[]) > +{ > + u32 status = 0; > No need to init status value here. Ok > + int ret; > + > + status = zynqmp_verify_auth(); > + if (status) { > + printf("Boot is authenticated\n"); > + > + ret = env_set("zynqmp_auth", "1"); > + if (ret) > + return CMD_RET_FAILURE; > + } else { > Here it might be that reading reg actually failed, which should return > CMD_RET_FAILURE instead. I will check for this in V3. > + printf("Boot is not authenticated\n"); > + > + ret = env_set("zynqmp_auth", "0"); > Could use a more descriptive name, like fsbl_auth? It applies to either fsbl_auth or spl_auth as the boot.bin has been authenticated. Technically, this means the pmufw too in both cases. Perhaps "boot_auth" would be a good name? > + if (ret) > + return CMD_RET_FAILURE; > + > + } > + > + return status; > Should it be CMD_RET_SUCCESS? I was also hoping to return a 0 or 1 value for using the command with a u-boot script. I guess the u-boot script could just use the env variable? > Thanks, > Tomas Thanks for your thorough review. > +} > + > static struct cmd_tbl cmd_zynqmp_sub[] = { > U_BOOT_CMD_MKENT(secure, 5, 0, do_zynqmp_verify_secure, "", ""), > U_BOOT_CMD_MKENT(pmufw, 4, 0, do_zynqmp_pmufw, "", ""), > @@ -371,6 +410,7 @@ static struct cmd_tbl cmd_zynqmp_sub[] = { > U_BOOT_CMD_MKENT(rsa, 7, 0, do_zynqmp_rsa, "", ""), > U_BOOT_CMD_MKENT(sha3, 5, 0, do_zynqmp_sha3, "", ""), > U_BOOT_CMD_MKENT(reboot, 3, 0, do_zynqmp_reboot, "", ""), > + U_BOOT_CMD_MKENT(verify_auth, 2, 0, do_zynqmp_verify_auth, "", ""), > #ifdef CONFIG_DEFINE_TCM_OCM_MMAP > U_BOOT_CMD_MKENT(tcminit, 3, 0, do_zynqmp_tcm_init, "", ""), > #endif > @@ -446,10 +486,12 @@ U_BOOT_LONGHELP(zynqmp, > " 48 bytes hash value into srcaddr\n" > " Optional key_addr can be specified for saving sha3 hash value\n" > " Note: srcaddr/srclen should not be 0\n" > + "zynqmp verify_auth - verifies if boot.bin was authenticated\n" > + " Returns: 0 not authenticated, 1 authenticated\n" > ); > > U_BOOT_CMD( > - zynqmp, 9, 1, do_zynqmp, > + zynqmp, 10, 1, do_zynqmp, > "ZynqMP sub-system", > zynqmp_help_text > ); Best regards, Neal Frager AMD
