[PATCH v2 1/1] board: zynqmp: add cmd for getting boot auth state

[Frager, Neal]

[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 <igor.opaniuk at foundries.io>
> Signed-off-by: Neal Frager <neal.frager at amd.com>
> ---
> 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