[PATCH 25/30] arm: imx: Finish migration from CONFIG_SECURE_BOOT to CONFIG_IMX_HAB

Vladimir Oltean
2020-06-11T19:31:32Z

Hi Tom,

On Wed, 10 Jun 2020 at 23:17, Tom Rini <trini@konsulko.com> wrote:
>
> There are a few remaining places where we say CONFIG_SECURE_BOOT rather
> than CONFIG_IMX HAB. Update these instances.
>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: NXP i.MX U-Boot Team <uboot-imx@nxp.com>
> Cc: Eddy Petri?or <eddy.petrisor@gmail.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Vladimir Oltean <olteanv@gmail.com>
> Cc: Priyanka Jain <priyanka.jain@nxp.com>
> Fixes: d714a75fd4dc ("imx: replace CONFIG_SECURE_BOOT with CONFIG_IMX_HAB")
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
> Note that we have one place left for CONFIG_SECURE_BOOT being in use but
> I think that is shared with PowerPC so I don't think IMX_HAB is the
> right name. But perhaps I'm wrong about it being used for PowerPC?

NACK on this patch.
I'm not actually sure what were the cross-architecture problems with
the CONFIG_SECURE_BOOT name that mandated Stefano to write this patch:

commit d714a75fd4dcfb0eb8b7e1dd29f43e07113cec0b
Author: Stefano Babic <sbabic@denx.de>
Date: Fri Sep 20 08:47:53 2019 +0200

imx: replace CONFIG_SECURE_BOOT with CONFIG_IMX_HAB

CONFIG_SECURE_BOOT is too generic and forbids to use it for cross
architecture purposes. If Secure Boot is required for imx, this means to
enable and use the HAB processor in the soc.

Signed-off-by: Stefano Babic <sbabic@denx.de>

but going the full way and grouping Layerscape, QorIQ and S32V secure
boot implementations together with a boot ROM feature available only
on i.MX 50, 53, 6, 7, 8M and 8MM is demonstrably incorrect.

I think the correct solution (beside leaving the CONFIG_SECURE_BOOT
name alone) would be to merge it, for the Layerscape (ls*) and PowerPC
instances, with CONFIG_CHAIN_OF_TRUST (defined under
board/freescale/common/Kconfig). But you or Stefano might argue that
CHAIN_OF_TRUST is still too generic for a name, and in that case,
maybe the whole thing can be renamed to CONFIG_FSL_ESBC (ESBC ==
"External Secure Boot Code", aka image validation code executed by the
bootloader as opposed to the [internal] boot ROM).

By the way, I have no idea what is the correct solution for S32V.

> ---
> arch/arm/mach-imx/spl_qspi.cfg | 2 +-
> board/ea/mx7ulp_com/imximage.cfg | 2 +-
> board/freescale/s32v234evb/s32v234evb.cfg | 2 +-
> board/novtech/meerkat96/imximage.cfg | 2 +-
> include/configs/ls1021atsn.h | 4 ++--
> 5 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/mach-imx/spl_qspi.cfg b/arch/arm/mach-imx/spl_qspi.cfg
> index 88956e626ffd..1e39ae2f01f7 100644
> --- a/arch/arm/mach-imx/spl_qspi.cfg
> +++ b/arch/arm/mach-imx/spl_qspi.cfg
> @@ -12,6 +12,6 @@ BOOT_FROM qspi
> /*
> * Secure boot support
> */
> -#ifdef CONFIG_SECURE_BOOT
> +#ifdef CONFIG_IMX_HAB
> CSF CONFIG_CSF_SIZE
> #endif
> diff --git a/board/ea/mx7ulp_com/imximage.cfg b/board/ea/mx7ulp_com/imximage.cfg
> index d298d17c1e92..1b218996aea9 100644
> --- a/board/ea/mx7ulp_com/imximage.cfg
> +++ b/board/ea/mx7ulp_com/imximage.cfg
> @@ -28,7 +28,7 @@ BOOT_FROM sd
> PLUGIN board/freescale/mx7ulp_evk/plugin.bin 0x2F020000
> #else
>
> -#ifdef CONFIG_SECURE_BOOT
> +#ifdef CONFIG_IMX_HAB
> CSF CONFIG_CSF_SIZE
> #endif
> /*
> diff --git a/board/freescale/s32v234evb/s32v234evb.cfg b/board/freescale/s32v234evb/s32v234evb.cfg
> index 7881512139d0..d7f722006312 100644
> --- a/board/freescale/s32v234evb/s32v234evb.cfg
> +++ b/board/freescale/s32v234evb/s32v234evb.cfg
> @@ -23,6 +23,6 @@ BOOT_FROM sd
> */
>
>
> -#ifdef CONFIG_SECURE_BOOT
> +#ifdef CONFIG_IMX_HAB
> SECURE_BOOT
> #endif
> diff --git a/board/novtech/meerkat96/imximage.cfg b/board/novtech/meerkat96/imximage.cfg
> index 3bd8cc55e53c..86275b84d9c8 100644
> --- a/board/novtech/meerkat96/imximage.cfg
> +++ b/board/novtech/meerkat96/imximage.cfg
> @@ -25,7 +25,7 @@ BOOT_FROM sd
> /*
> * Secure boot support
> */
> -#ifdef CONFIG_SECURE_BOOT
> +#ifdef CONFIG_IMX_HAB
> CSF CONFIG_CSF_SIZE
> #endif
>
> diff --git a/include/configs/ls1021atsn.h b/include/configs/ls1021atsn.h
> index e76e54e97fc9..efa708a239ea 100644
> --- a/include/configs/ls1021atsn.h
> +++ b/include/configs/ls1021atsn.h
> @@ -60,9 +60,9 @@
> #define CONFIG_SYS_FSL_PBL_RCW \
> "board/freescale/ls1021atsn/ls102xa_rcw_sd.cfg"
>
> -#ifdef CONFIG_SECURE_BOOT
> +#ifdef CONFIG_IMX_HAB
> #define CONFIG_U_BOOT_HDR_SIZE (16 << 10)
> -#endif /* ifdef CONFIG_SECURE_BOOT */
> +#endif /* ifdef CONFIG_IMX_HAB */
>
> #define CONFIG_SPL_MAX_SIZE 0x1a000
> #define CONFIG_SPL_STACK 0x1001d000
> --
> 2.17.1
>

Thanks,
-Vladimir