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

[Tom Rini]

On Fri, Jun 12, 2020 at 12:02:24AM +0300, Vladimir Oltean wrote:
> On Thu, 11 Jun 2020 at 23:30, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Thu, Jun 11, 2020 at 10:31:32PM +0300, Vladimir Oltean wrote:
> > > Hi Tom,
> > >
> > > On Wed, 10 Jun 2020 at 23:17, Tom Rini <trini at 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 at denx.de>
> > > > Cc: Fabio Estevam <festevam at gmail.com>
> > > > Cc: NXP i.MX U-Boot Team <uboot-imx at nxp.com>
> > > > Cc: Eddy Petrișor <eddy.petrisor at gmail.com>
> > > > Cc: Shawn Guo <shawnguo at kernel.org>
> > > > Cc: Vladimir Oltean <olteanv at gmail.com>
> > > > Cc: Priyanka Jain <priyanka.jain at nxp.com>
> > > > Fixes: d714a75fd4dc ("imx: replace CONFIG_SECURE_BOOT with CONFIG_IMX_HAB")
> > > > Signed-off-by: Tom Rini <trini at 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.
> >
> > Note that today CONFIG_SECURE_BOOT is not defined anywhere and the
> > commit you mention next replaced the only places that set
> > CONFIG_SECURE_BOOT with CONFIG_IMX_HAB.
> >
> 
> Actually looks like the ls1021atsn_sdcard_SECURE_BOOT_defconfig that
> defined it (via CONFIG_SYS_EXTRA_OPTIONS) hasn't made it upstream yet,
> so it's a bit unfair to say it, but things under CONFIG_SECURE_BOOT
> are not dead code as you want to imply.

Ah, sorry, I was trying to fix inadvertently broken code.

> > > 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 at 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 at denx.de>
> >
> > The problem is that SECURE_BOOT is very generic.  We have quite a few
> > different "secure boot" implementations in the tree and another pointed
> > out what a bad name this one is.  And just to be clear, I'm the only one
> > (intentionally) touching non-i.MX spots here.
> >
> 
> Ok, agree that it's a bad name for something that lives under board/freescale/.
> 
> > > 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.
> >
> > OK.  I (and others on the thread at the time) were asking for someone to
> > group things right and provide a new symbol.  What's in there is what we
> > got, but more details are always better as there were a few cases that
> > didn't get updated.
> >
> > > 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).
> >
> > So for this patch here it's a few instances of CONFIG_CSF_SIZE on i.MX
> > files, a change to S32V that looks quite a lot like i.MX (the file notes
> > as much) and a layerscape change to CONFIG_U_BOOT_HDR_SIZE.  I'm quite
> > happy to spin v2 dropping the layerscape part out and waiting to see
> > what Eddy says for S32V.  We have a CONFIG_NXP_ESBC symbol today, would
> > that make sense to use in the check on include/configs/ls1021atsn.h and
> > top-level Makefile for not making u-boot.pbl sometimes?  Thanks again!
> >
> 
> Yes, yes, for ls1021atsn.h and for the top-level Makefile,
> CONFIG_NXP_ESBC is exactly what is needed! For the top-level Makefile
> in particular, I believe it was missed during this conversion:
> 
> commit 5536c3c9d0d10c1a4e440e71eac389df3a3dbfa7
> Author: Udit Agarwal <udit.agarwal at nxp.com>
> Date:   Thu Nov 7 16:11:32 2019 +0000
> 
>     freescale/layerscape: Rename the config CONFIG_SECURE_BOOT name
> 
>     Rename CONFIG_SECURE_BOOT to CONFIG_NXP_ESBC to avoid conflict
>     with UEFI secure boot.
> 
>     Signed-off-by: Udit Agarwal <udit.agarwal at nxp.com>
>     Signed-off-by: Priyanka Jain <priyanka.jain at nxp.com>
> 
> I'm not sure how I missed it during my previous reply.

Ah-ha!  I'll catch this in v2, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200611/9ef85ed2/attachment.sig>