diff mbox series

[U-Boot,v1] colibri_imx7: boot kernel in secure mode

Message ID 20190703135028.19271-1-igor.opaniuk@gmail.com
State Rejected
Delegated to: Stefano Babic
Headers show
Series [U-Boot,v1] colibri_imx7: boot kernel in secure mode | expand

Commit Message

Igor Opaniuk July 3, 2019, 1:50 p.m. UTC 
From: Igor Opaniuk <igor.opaniuk@toradex.com>

NXP downstream kernel uses legacy method to enable other cores,
which requires kernel to run in a security mode (althought upstream kernel
uses PSCI for this).

As we're using NXP kernel in our BSPs, lets enable this by default.

Signed-off-by: Igor Opaniuk <igor.opaniuk@toradex.com>
---

 include/configs/colibri_imx7.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Philippe Schenker July 10, 2019, 12:19 p.m. UTC | #1 
On Wed, 2019-07-03 at 16:50 +0300, Igor Opaniuk wrote:
> From: Igor Opaniuk <igor.opaniuk@toradex.com>
> 
> NXP downstream kernel uses legacy method to enable other cores,
> which requires kernel to run in a security mode (althought upstream kernel
> uses PSCI for this).
> 
> As we're using NXP kernel in our BSPs, lets enable this by default.
> 
> Signed-off-by: Igor Opaniuk <igor.opaniuk@toradex.com>

Reviewed-by: Philippe Schenker <philippe.schenker@toradex.com>
Tested-by: Philippe Schenker <philippe.schenker@toradex.com>

> ---
> 
>  include/configs/colibri_imx7.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/configs/colibri_imx7.h b/include/configs/colibri_imx7.h
> index 40173b18fa..5408577661 100644
> --- a/include/configs/colibri_imx7.h
> +++ b/include/configs/colibri_imx7.h
> @@ -164,7 +164,8 @@
>  		"${board}/flash_blk.img && source ${loadaddr}\0" \
>  	"setup=setenv setupargs " \
>  		"console=tty1 console=${console}" \
> -		",${baudrate}n8 ${memargs} consoleblank=0\0" \
> +		",${baudrate}n8 ${memargs} consoleblank=0; " \
> +		"setenv bootm_boot_mode sec\0" \
>  	"setupdate=run setsdupdate || run setusbupdate || run setethupdate\0" \
>  	"setusbupdate=usb start && setenv interface usb && " \
>  		"fatload ${interface} 0:1 ${loadaddr} " \
Tobias Junghans July 10, 2019, 12:35 p.m. UTC | #2 
> > NXP downstream kernel uses legacy method to enable other cores,
> > which requires kernel to run in a security mode (althought upstream
> > kernel
> > uses PSCI for this).
> > 
> > As we're using NXP kernel in our BSPs, lets enable this by default.
> > 
> > Signed-off-by: Igor Opaniuk <igor.opaniuk@toradex.com>

How does enabling both cores work in upstream kernel? It looks like PSCI does 
not work in secure mode using mainline 4.19 LTS kernel. Any hints? Until this 
is resolved booting in secure mode per default IMHO is not a good option.

Best regards

Tobias
Philippe Schenker July 10, 2019, 1:01 p.m. UTC | #3 
On Wed, 2019-07-10 at 14:35 +0200, Tobias Junghans wrote:
> > > NXP downstream kernel uses legacy method to enable other cores,
> > > which requires kernel to run in a security mode (althought upstream
> > > kernel
> > > uses PSCI for this).
> > > 
> > > As we're using NXP kernel in our BSPs, lets enable this by default.
> > > 
> > > Signed-off-by: Igor Opaniuk <igor.opaniuk@toradex.com>
> 
> How does enabling both cores work in upstream kernel? It looks like PSCI does 
> not work in secure mode using mainline 4.19 LTS kernel. Any hints? Until this 
> is resolved booting in secure mode per default IMHO is not a good option.
> 
> Best regards
> 
> Tobias

Hi Tobias,

I just tried it again and it works also with 4.19 LTS and 'bootm_boot_mode sec'
on my side.

> 
>
Igor Opaniuk July 11, 2019, 2:11 p.m. UTC | #4 
Hi Tobias,

On Wed, Jul 10, 2019 at 3:35 PM Tobias Junghans
<tobias.junghans@veyon.io> wrote:
>
> > > NXP downstream kernel uses legacy method to enable other cores,
> > > which requires kernel to run in a security mode (althought upstream
> > > kernel
> > > uses PSCI for this).
> > >
> > > As we're using NXP kernel in our BSPs, lets enable this by default.
> > >
> > > Signed-off-by: Igor Opaniuk <igor.opaniuk@toradex.com>
>
> How does enabling both cores work in upstream kernel? It looks like PSCI does
> not work in secure mode using mainline 4.19 LTS kernel. Any hints? Until this
> is resolved booting in secure mode per default IMHO is not a good option.
>
> Best regards
>
> Tobias
>
>

I'm afraid you're right.
Just after a bit of time researching and discussing with Stefan, seems
that we need to introduce two different wrappers for booting the
mainline kernel and downstream NXP kernel.

* NXP kernel has legacy code to enable all cores, which works only when
running in secure mode.
* Mainline kernel, as you said before, does use PSCI for this, which
is provided by U-boot (which adds proper psci nodes to the linux
dtb on-fly before transferring control to the linux kernel entry point). When we
try to load it in secure mode, it continues running on the same Secure PL1,
and communication using SMC calling convention doesn't make sense at this case.
Philippe Schenker July 11, 2019, 2:44 p.m. UTC | #5 
On Wed, 2019-07-10 at 12:19 +0000, Philippe Schenker wrote:
> On Wed, 2019-07-03 at 16:50 +0300, Igor Opaniuk wrote:
> > From: Igor Opaniuk <igor.opaniuk@toradex.com>
> > 
> > NXP downstream kernel uses legacy method to enable other cores,
> > which requires kernel to run in a security mode (althought upstream kernel
> > uses PSCI for this).
> > 
> > As we're using NXP kernel in our BSPs, lets enable this by default.
> > 
> > Signed-off-by: Igor Opaniuk <igor.opaniuk@toradex.com>
> 
> Reviewed-by: Philippe Schenker <philippe.schenker@toradex.com>
> Tested-by: Philippe Schenker <philippe.schenker@toradex.com>

Please ignore my Reviewed-by and Tested-by. I Tested it on downstream which
works fine but on mainline only one core comes up by design. I wasn't aware of
that.

Philippe

> 
> > ---
> > 
> >  include/configs/colibri_imx7.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/configs/colibri_imx7.h b/include/configs/colibri_imx7.h
> > index 40173b18fa..5408577661 100644
> > --- a/include/configs/colibri_imx7.h
> > +++ b/include/configs/colibri_imx7.h
> > @@ -164,7 +164,8 @@
> >  		"${board}/flash_blk.img && source ${loadaddr}\0" \
> >  	"setup=setenv setupargs " \
> >  		"console=tty1 console=${console}" \
> > -		",${baudrate}n8 ${memargs} consoleblank=0\0" \
> > +		",${baudrate}n8 ${memargs} consoleblank=0; " \
> > +		"setenv bootm_boot_mode sec\0" \
> >  	"setupdate=run setsdupdate || run setusbupdate || run setethupdate\0" \
> >  	"setusbupdate=usb start && setenv interface usb && " \
> >  		"fatload ${interface} 0:1 ${loadaddr} " \
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
Tobias Junghans July 23, 2019, 1:29 p.m. UTC | #6 
Hi Igor,

thanks for your comments! Is there any solution, patch or workaround I can try 
to power on the 2nd CPU core in secure mode with mainline kernel?

Thanks and best regards

Tobias

> I'm afraid you're right.
> Just after a bit of time researching and discussing with Stefan, seems
> that we need to introduce two different wrappers for booting the
> mainline kernel and downstream NXP kernel.
> 
> * NXP kernel has legacy code to enable all cores, which works only when
> running in secure mode.
> * Mainline kernel, as you said before, does use PSCI for this, which
> is provided by U-boot (which adds proper psci nodes to the linux
> dtb on-fly before transferring control to the linux kernel entry point).
> When we try to load it in secure mode, it continues running on the same
> Secure PL1, and communication using SMC calling convention doesn't make
> sense at this case.
Peng Fan July 24, 2019, 1:29 a.m. UTC | #7 
> Subject: Re: [U-Boot] [PATCH v1] colibri_imx7: boot kernel in secure mode
> 
> Hi Igor,
> 
> thanks for your comments! Is there any solution, patch or workaround I can
> try to power on the 2nd CPU core in secure mode with mainline kernel?

The upstream maintainer rejected the legacy method for i.MX7, so in upstream
psci was used, with psci, the kernel is booted in non-secure mode.

Regards,
Peng.

> 
> Thanks and best regards
> 
> Tobias
> 
> > I'm afraid you're right.
> > Just after a bit of time researching and discussing with Stefan, seems
> > that we need to introduce two different wrappers for booting the
> > mainline kernel and downstream NXP kernel.
> >
> > * NXP kernel has legacy code to enable all cores, which works only
> > when running in secure mode.
> > * Mainline kernel, as you said before, does use PSCI for this, which
> > is provided by U-boot (which adds proper psci nodes to the linux dtb
> > on-fly before transferring control to the linux kernel entry point).
> > When we try to load it in secure mode, it continues running on the
> > same Secure PL1, and communication using SMC calling convention
> > doesn't make sense at this case.
> 
>
Stefan Agner July 24, 2019, 11:29 a.m. UTC | #8 
On 2019-07-23 15:29, Tobias Junghans wrote:
> Hi Igor,
> 
> thanks for your comments! Is there any solution, patch or workaround I can try 
> to power on the 2nd CPU core in secure mode with mainline kernel?

I am afraid that is not possible since without PSCI mainline simply
lacks the code how to enable the secondary CPU. You cannot use PSCI and
stay in secure mode.

Anyway, why would you want to boot in secure mode anyway? With
non-secure mode your CPU has more features, e.g. you can actually use
virtualization feature (HYP). You do not have that in secure mode.

Booting in non-secure mode is the proper way(tm) to boot on this CPU.

That CAAM issue is unfortunate. But I think it is possible to enable
CAAM access in non-secure mode by making sure to write the proper
registers while being still in secure mode.

--
Stefan

> 
> Thanks and best regards
> 
> Tobias
> 
>> I'm afraid you're right.
>> Just after a bit of time researching and discussing with Stefan, seems
>> that we need to introduce two different wrappers for booting the
>> mainline kernel and downstream NXP kernel.
>>
>> * NXP kernel has legacy code to enable all cores, which works only when
>> running in secure mode.
>> * Mainline kernel, as you said before, does use PSCI for this, which
>> is provided by U-boot (which adds proper psci nodes to the linux
>> dtb on-fly before transferring control to the linux kernel entry point).
>> When we try to load it in secure mode, it continues running on the same
>> Secure PL1, and communication using SMC calling convention doesn't make
>> sense at this case.
> 
> 
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
Stefan Agner July 24, 2019, 11:32 a.m. UTC | #9 
On 2019-07-03 15:50, Igor Opaniuk wrote:
> From: Igor Opaniuk <igor.opaniuk@toradex.com>
> 
> NXP downstream kernel uses legacy method to enable other cores,
> which requires kernel to run in a security mode (althought upstream kernel
> uses PSCI for this).
> 
> As we're using NXP kernel in our BSPs, lets enable this by default.

I think I mentioned it internally, but just to be clear: I do not agree
with this patch.

Upstream U-Boot should be able to boot upstream Linux. I'd rather prefer
to carry that patch in our downstream fork (or set the bootm_boot_mode
environment in our boot script).

--
Stefan

> 
> Signed-off-by: Igor Opaniuk <igor.opaniuk@toradex.com>
> ---
> 
>  include/configs/colibri_imx7.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/configs/colibri_imx7.h b/include/configs/colibri_imx7.h
> index 40173b18fa..5408577661 100644
> --- a/include/configs/colibri_imx7.h
> +++ b/include/configs/colibri_imx7.h
> @@ -164,7 +164,8 @@
>  		"${board}/flash_blk.img && source ${loadaddr}\0" \
>  	"setup=setenv setupargs " \
>  		"console=tty1 console=${console}" \
> -		",${baudrate}n8 ${memargs} consoleblank=0\0" \
> +		",${baudrate}n8 ${memargs} consoleblank=0; " \
> +		"setenv bootm_boot_mode sec\0" \
>  	"setupdate=run setsdupdate || run setusbupdate || run setethupdate\0" \
>  	"setusbupdate=usb start && setenv interface usb && " \
>  		"fatload ${interface} 0:1 ${loadaddr} " \
Igor Opaniuk July 24, 2019, 12:19 p.m. UTC | #10 
Hi Stefan

On Wed, Jul 24, 2019 at 2:32 PM Stefan Agner <stefan@agner.ch> wrote:
>
> On 2019-07-03 15:50, Igor Opaniuk wrote:
> > From: Igor Opaniuk <igor.opaniuk@toradex.com>
> >
> > NXP downstream kernel uses legacy method to enable other cores,
> > which requires kernel to run in a security mode (althought upstream kernel
> > uses PSCI for this).
> >
> > As we're using NXP kernel in our BSPs, lets enable this by default.
>
> I think I mentioned it internally, but just to be clear: I do not agree
> with this patch.
>
> Upstream U-Boot should be able to boot upstream Linux. I'd rather prefer
> to carry that patch in our downstream fork (or set the bootm_boot_mode
> environment in our boot script).

Totally agree with you here,
BTW, seems that Stefano has applied by mistake to u-boot-imx
fcc79eee14("colibri_imx7_emmc: enable CONFIG_ARMV7_BOOT_SEC_DEFAULT"),
although I've asked to ignore it.

Probably will send another patch that reverts this.

>
> --
> Stefan
>
> >
> > Signed-off-by: Igor Opaniuk <igor.opaniuk@toradex.com>
> > ---
> >
> >  include/configs/colibri_imx7.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/configs/colibri_imx7.h b/include/configs/colibri_imx7.h
> > index 40173b18fa..5408577661 100644
> > --- a/include/configs/colibri_imx7.h
> > +++ b/include/configs/colibri_imx7.h
> > @@ -164,7 +164,8 @@
> >               "${board}/flash_blk.img && source ${loadaddr}\0" \
> >       "setup=setenv setupargs " \
> >               "console=tty1 console=${console}" \
> > -             ",${baudrate}n8 ${memargs} consoleblank=0\0" \
> > +             ",${baudrate}n8 ${memargs} consoleblank=0; " \
> > +             "setenv bootm_boot_mode sec\0" \
> >       "setupdate=run setsdupdate || run setusbupdate || run setethupdate\0" \
> >       "setusbupdate=usb start && setenv interface usb && " \
> >               "fatload ${interface} 0:1 ${loadaddr} " \
Stefano Babic Aug. 1, 2019, 1:19 p.m. UTC | #11 
On 24/07/19 14:19, Igor Opaniuk wrote:
> Hi Stefan
> 
> On Wed, Jul 24, 2019 at 2:32 PM Stefan Agner <stefan@agner.ch> wrote:
>>
>> On 2019-07-03 15:50, Igor Opaniuk wrote:
>>> From: Igor Opaniuk <igor.opaniuk@toradex.com>
>>>
>>> NXP downstream kernel uses legacy method to enable other cores,
>>> which requires kernel to run in a security mode (althought upstream kernel
>>> uses PSCI for this).
>>>
>>> As we're using NXP kernel in our BSPs, lets enable this by default.
>>
>> I think I mentioned it internally, but just to be clear: I do not agree
>> with this patch.
>>
>> Upstream U-Boot should be able to boot upstream Linux. I'd rather prefer
>> to carry that patch in our downstream fork (or set the bootm_boot_mode
>> environment in our boot script).
> 
> Totally agree with you here,
> BTW, seems that Stefano has applied by mistake to u-boot-imx
> fcc79eee14("colibri_imx7_emmc: enable CONFIG_ARMV7_BOOT_SEC_DEFAULT"),
> although I've asked to ignore it.

Sorry for this - I have taken a look and I drop this one but I took
fcc79eee14. Should I simply revert it on u-boot-imx  (you do not need to
send a patch, then)?

Regards,
Stefano

> 
> Probably will send another patch that reverts this.
> 
>>
>> --
>> Stefan
>>
>>>
>>> Signed-off-by: Igor Opaniuk <igor.opaniuk@toradex.com>
>>> ---
>>>
>>>  include/configs/colibri_imx7.h | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/configs/colibri_imx7.h b/include/configs/colibri_imx7.h
>>> index 40173b18fa..5408577661 100644
>>> --- a/include/configs/colibri_imx7.h
>>> +++ b/include/configs/colibri_imx7.h
>>> @@ -164,7 +164,8 @@
>>>               "${board}/flash_blk.img && source ${loadaddr}\0" \
>>>       "setup=setenv setupargs " \
>>>               "console=tty1 console=${console}" \
>>> -             ",${baudrate}n8 ${memargs} consoleblank=0\0" \
>>> +             ",${baudrate}n8 ${memargs} consoleblank=0; " \
>>> +             "setenv bootm_boot_mode sec\0" \
>>>       "setupdate=run setsdupdate || run setusbupdate || run setethupdate\0" \
>>>       "setusbupdate=usb start && setenv interface usb && " \
>>>               "fatload ${interface} 0:1 ${loadaddr} " \
> 
> 
>
Igor Opaniuk Aug. 1, 2019, 1:23 p.m. UTC | #12 
Hi Stefano,

On Thu, Aug 1, 2019, 16:19 Stefano Babic <sbabic@denx.de> wrote:

> On 24/07/19 14:19, Igor Opaniuk wrote:
> > Hi Stefan
> >
> > On Wed, Jul 24, 2019 at 2:32 PM Stefan Agner <stefan@agner.ch> wrote:
> >>
> >> On 2019-07-03 15:50, Igor Opaniuk wrote:
> >>> From: Igor Opaniuk <igor.opaniuk@toradex.com>
> >>>
> >>> NXP downstream kernel uses legacy method to enable other cores,
> >>> which requires kernel to run in a security mode (althought upstream
> kernel
> >>> uses PSCI for this).
> >>>
> >>> As we're using NXP kernel in our BSPs, lets enable this by default.
> >>
> >> I think I mentioned it internally, but just to be clear: I do not agree
> >> with this patch.
> >>
> >> Upstream U-Boot should be able to boot upstream Linux. I'd rather prefer
> >> to carry that patch in our downstream fork (or set the bootm_boot_mode
> >> environment in our boot script).
> >
> > Totally agree with you here,
> > BTW, seems that Stefano has applied by mistake to u-boot-imx
> > fcc79eee14("colibri_imx7_emmc: enable CONFIG_ARMV7_BOOT_SEC_DEFAULT"),
> > although I've asked to ignore it.
>
> Sorry for this - I have taken a look and I drop this one but I took
> fcc79eee14. Should I simply revert it on u-boot-imx  (you do not need to
> send a patch, then)?
>
Yes, please revert this patch, as it brakes boot of the mainline kernel.
Thanks!


> Regards,
> Stefano
>
> >
> > Probably will send another patch that reverts this.
> >
> >>
> >> --
> >> Stefan
> >>
> >>>
> >>> Signed-off-by: Igor Opaniuk <igor.opaniuk@toradex.com>
> >>> ---
> >>>
> >>>  include/configs/colibri_imx7.h | 3 ++-
> >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/configs/colibri_imx7.h
> b/include/configs/colibri_imx7.h
> >>> index 40173b18fa..5408577661 100644
> >>> --- a/include/configs/colibri_imx7.h
> >>> +++ b/include/configs/colibri_imx7.h
> >>> @@ -164,7 +164,8 @@
> >>>               "${board}/flash_blk.img && source ${loadaddr}\0" \
> >>>       "setup=setenv setupargs " \
> >>>               "console=tty1 console=${console}" \
> >>> -             ",${baudrate}n8 ${memargs} consoleblank=0\0" \
> >>> +             ",${baudrate}n8 ${memargs} consoleblank=0; " \
> >>> +             "setenv bootm_boot_mode sec\0" \
> >>>       "setupdate=run setsdupdate || run setusbupdate || run
> setethupdate\0" \
> >>>       "setusbupdate=usb start && setenv interface usb && " \
> >>>               "fatload ${interface} 0:1 ${loadaddr} " \
> >
> >
> >
>
>
> --
> =====================================================================
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
> =====================================================================
>
diff mbox series

Patch

diff --git a/include/configs/colibri_imx7.h b/include/configs/colibri_imx7.h
index 40173b18fa..5408577661 100644
--- a/include/configs/colibri_imx7.h
+++ b/include/configs/colibri_imx7.h
@@ -164,7 +164,8 @@ 
 		"${board}/flash_blk.img && source ${loadaddr}\0" \
 	"setup=setenv setupargs " \
 		"console=tty1 console=${console}" \
-		",${baudrate}n8 ${memargs} consoleblank=0\0" \
+		",${baudrate}n8 ${memargs} consoleblank=0; " \
+		"setenv bootm_boot_mode sec\0" \
 	"setupdate=run setsdupdate || run setusbupdate || run setethupdate\0" \
 	"setusbupdate=usb start && setenv interface usb && " \
 		"fatload ${interface} 0:1 ${loadaddr} " \