ARM: aarch64: lowlevel: potential bug in arm_cpu_lowlevel_init

ARM: aarch64: lowlevel: potential bug in arm_cpu_lowlevel_init

[Lior Weintraub]

Hi Andrew,

I am asking about a patch you've introduced about 4 years ago:
Commit: cd6e1857a6a824d562bd27379d191602c074f6b7

ENTRY(arm_cpu_lowlevel_init)
	switch_el x1, 3f, 2f, 1f

3:
	mov	x0, #1			/* Non-Secure EL0/1 */
	orr	x0, x0, #(1 << 10)	/* 64-bit EL2 */
	msr	scr_el3, x0
	msr	cptr_el3, xzr

	mrs	x0, sctlr_el3
	ldr	x1, =SCTLR_ELx_FLAGS
	bic	x0, x0, x1
	msr	sctlr_el3, x0
	isb

	b	done

This code has introduced a bug in our barebox porting.
It could be our mistake but then again we couldn't find any prerequisites conditions that bootloaders need to meet before passing control to barebox pbl.
There are 2 bugs that can happen here:
1. The bootloader enabled MMU and set the SRAM (given to barebox) as non-secure – This issue can be resolved with adding "dsb sy" command before the "isb"
2. The bootloader enabled MMU and dcache on SRAM (given to barebox) as non-secure – This is a bit harder to solve because it needs to call cache invalidate on the stack.

The bootrom can run in one of the following modes to avoid the bug:
1. Run with MMU disabled. In this case the SRAM is treated as a device and in secure mode.
2. Run with MMU enabled but set the SRAM to secure mode.

Explanation of the first potential bug:
Before the barebox calls arm_cpu_lowlevel_init (from our spider_lowlevel_init function) it pushes to the stack the return address.
At this point the stack is located on SRAM in non-secure mode.
When the function arm_cpu_lowlevel_init disables the MMU, the SRAM area becomes secure and as a result, reading from the stack (by "ret") is done before the write (few cycles before) was done.
The ARM core can do that because it assumes those are not the same locations (at least I think this explains the issue because this is what we saw in RTL verification environment).
Placing a "dsb sy" before the "isb" command solved the issue.

Explanation of the second potential bug:
Same as before but now it is not enough to call "dsb sy" because the data is fetched from a different cache line (there are different cache lines for secure and non-secure modes).
So to fix this issue, the function must call data cache invalidate on the area where the current stack pointer is located for N cache lines (I assume 1 cache line would be enough but I did not tested it).

Few questions:
1. Do you agree with the above analysis?
2. If so should I prepare a patch for it?
3. Should the patch also include a fix to the data cache issue?
4. If you think the code doesn't need to change, do you agree that Barebox documentation needs to include some guidance to bootloaders?
IMHO, the barebox should behave correctly no matter the initial conditions of the bootloader.
As you wrote in your commit:

"ARM: aarch64: lowlevel: Reset SCTLR_EL3 in arm_cpu_lowlevel_init()

There's no guarantee that when arm_cpu_lowlevel_init() runs at EL3,
SCTLR will be in a state we expect it to be. Add code to reset it to a
known state, so we'd always start form clean slate. This is also
matches what we've been doing non 64-bit ARMs.

Real word motivation for this patch is i.MX8MQ whose rev 2.1 silicon
appear to have different mask ROM behaviour where it now leaves MMU
enabled if no valid boot source is found. Page table it sets up
doesn't include DDR range, so trying to bootstrap the device via
JTAG/OpenOCD results in an abort.

The value for SCTLR_ELx_FLAGS was taken from Linux kernel."

Cheers,
Lior.

Re: ARM: aarch64: lowlevel: potential bug in arm_cpu_lowlevel_init

[Sascha Hauer]

Hi Lior,

On Mon, Aug 14, 2023 at 11:35:05AM +0000, Lior Weintraub wrote:
>    Link: [1]File-List
>    Hi Andrew,
>     
>    I am asking about a patch you've introduced about 4 years ago:
>    Commit: cd6e1857a6a824d562bd27379d191602c074f6b7
>     
>    ENTRY(arm_cpu_lowlevel_init)
>                    switch_el x1, 3f, 2f, 1f
>     
>    3:
>                    mov       x0, #1                                    /*
>    Non-Secure EL0/1 */
>                    orr          x0, x0, #(1 << 10)              /* 64-bit EL2
>    */
>                    msr        scr_el3, x0
>                    msr        cptr_el3, xzr
>     
>                    mrs        x0, sctlr_el3
>                    ldr           x1, =SCTLR_ELx_FLAGS
>                    bic          x0, x0, x1
>                    msr        sctlr_el3, x0
>                    isb
>     
>                    b             done
>     
>    This code has introduced a bug in our barebox porting.
>    It could be our mistake but then again we couldn't find any prerequisites
>    conditions that bootloaders need to meet before passing control to barebox
>    pbl.
>    There are 2 bugs that can happen here:
>     1. The bootloader enabled MMU and set the SRAM (given to barebox) as
>        non-secure – This issue can be resolved with adding "dsb sy" command
>        before the "isb"
>     2. The bootloader enabled MMU and dcache on SRAM (given to barebox) as
>        non-secure – This is a bit harder to solve because it needs to call
>        cache invalidate on the stack.

Eventhough it might not be explicitly documented, barebox normally
expects to be called with MMU disabled.

When the MMU is enabled it could have virtually any mapping and there's
no sane way to disable the MMU then.

If your bootloader enables the MMU and there is no way to change that,
then it's up to your board entry code to disable the MMU before calling into
barebox. That said, when there's something can change in arm_cpu_lowlevel_init()
to help you with your case, then we can do that, but I wouldn't consider
this a bug in barebox.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: ARM: aarch64: lowlevel: potential bug in arm_cpu_lowlevel_init

[Lior Weintraub]

Thanks Sascha!

Actually the bootloader is still under development so there is no problem to change and work without MMU.
(or at lease disable it before jumping to Barebox).
I totally agree that if virtual mapping was used it would be impossible to disable the MMU.
Just thought that it is totally harmless to add `dsb sy`.

Cheers,
Lior.

> -----Original Message-----
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Sent: Wednesday, August 16, 2023 1:02 PM
> To: Lior Weintraub <liorw@pliops.com>
> Cc: andrew.smirnov@gmail.com; barebox@lists.infradead.org; Ahmad
> Fatoum <a.fatoum@pengutronix.de>
> Subject: Re: ARM: aarch64: lowlevel: potential bug in arm_cpu_lowlevel_init
> 
> CAUTION: External Sender
> 
> Hi Lior,
> 
> On Mon, Aug 14, 2023 at 11:35:05AM +0000, Lior Weintraub wrote:
> >    Link: [1]File-List
> >    Hi Andrew,
> >
> >    I am asking about a patch you've introduced about 4 years ago:
> >    Commit: cd6e1857a6a824d562bd27379d191602c074f6b7
> >
> >    ENTRY(arm_cpu_lowlevel_init)
> >                    switch_el x1, 3f, 2f, 1f
> >
> >    3:
> >                    mov       x0, #1                                    /*
> >    Non-Secure EL0/1 */
> >                    orr          x0, x0, #(1 << 10)              /* 64-bit EL2
> >    */
> >                    msr        scr_el3, x0
> >                    msr        cptr_el3, xzr
> >
> >                    mrs        x0, sctlr_el3
> >                    ldr           x1, =SCTLR_ELx_FLAGS
> >                    bic          x0, x0, x1
> >                    msr        sctlr_el3, x0
> >                    isb
> >
> >                    b             done
> >
> >    This code has introduced a bug in our barebox porting.
> >    It could be our mistake but then again we couldn't find any prerequisites
> >    conditions that bootloaders need to meet before passing control to
> barebox
> >    pbl.
> >    There are 2 bugs that can happen here:
> >     1. The bootloader enabled MMU and set the SRAM (given to barebox) as
> >        non-secure – This issue can be resolved with adding "dsb sy" command
> >        before the "isb"
> >     2. The bootloader enabled MMU and dcache on SRAM (given to barebox)
> as
> >        non-secure – This is a bit harder to solve because it needs to call
> >        cache invalidate on the stack.
> 
> Eventhough it might not be explicitly documented, barebox normally
> expects to be called with MMU disabled.
> 
> When the MMU is enabled it could have virtually any mapping and there's
> no sane way to disable the MMU then.
> 
> If your bootloader enables the MMU and there is no way to change that,
> then it's up to your board entry code to disable the MMU before calling into
> barebox. That said, when there's something can change in
> arm_cpu_lowlevel_init()
> to help you with your case, then we can do that, but I wouldn't consider
> this a bug in barebox.
> 
> Sascha
> 
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://secure-
> web.cisco.com/17RdScP_TBoXqc1IerwhZX8kAHhqutjAnByPiUM5lNZJyN6OD
> jkdTnyYCzrqpRZ9mdLg4wp-dZiElAak3iS7lf-YQwNFSattw-4zxIt6ro_qHh-
> 8ovK33TBdMnr5qL5qPKFUriYYQ8bLMAnMHa4bKtTIfhZI2eSXUr6hqjDcnoKZ
> GKUO2dbBDXceX3NT-ql9GBlwdH-mmkhd9fgYV2r0UtR4ot5d58hg45JTJ-
> LsuvNVMsKrFZd1Iu3AiMSjmi4E5BDc6KpQx_MgAD410FQWC0HpDavhSc1DC
> BgdMO2RlGrZk0L-
> St3n_yH_QZvH4UfjY/http%3A%2F%2Fwww.pengutronix.de%2F  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: ARM: aarch64: lowlevel: potential bug in arm_cpu_lowlevel_init

[Ahmad Fatoum]

Hello Lior,

On 16.08.23 12:14, Lior Weintraub wrote:
> Thanks Sascha!
> 
> Actually the bootloader is still under development so there is no problem to change and work without MMU.
> (or at lease disable it before jumping to Barebox).
> I totally agree that if virtual mapping was used it would be impossible to disable the MMU.
> Just thought that it is totally harmless to add `dsb sy`.

as Sascha mentioned, we expect MMU to be off, but some BootROMs fail to
do that in some error cases. I thus think if a barrier would save us
in such a case, we should have it in arm_cpu_lowlevel_init().

Cheers,
Ahmad

> 
> Cheers,
> Lior.
> 
>> -----Original Message-----
>> From: Sascha Hauer <s.hauer@pengutronix.de>
>> Sent: Wednesday, August 16, 2023 1:02 PM
>> To: Lior Weintraub <liorw@pliops.com>
>> Cc: andrew.smirnov@gmail.com; barebox@lists.infradead.org; Ahmad
>> Fatoum <a.fatoum@pengutronix.de>
>> Subject: Re: ARM: aarch64: lowlevel: potential bug in arm_cpu_lowlevel_init
>>
>> CAUTION: External Sender
>>
>> Hi Lior,
>>
>> On Mon, Aug 14, 2023 at 11:35:05AM +0000, Lior Weintraub wrote:
>>>    Link: [1]File-List
>>>    Hi Andrew,
>>>
>>>    I am asking about a patch you've introduced about 4 years ago:
>>>    Commit: cd6e1857a6a824d562bd27379d191602c074f6b7
>>>
>>>    ENTRY(arm_cpu_lowlevel_init)
>>>                    switch_el x1, 3f, 2f, 1f
>>>
>>>    3:
>>>                    mov       x0, #1                                    /*
>>>    Non-Secure EL0/1 */
>>>                    orr          x0, x0, #(1 << 10)              /* 64-bit EL2
>>>    */
>>>                    msr        scr_el3, x0
>>>                    msr        cptr_el3, xzr
>>>
>>>                    mrs        x0, sctlr_el3
>>>                    ldr           x1, =SCTLR_ELx_FLAGS
>>>                    bic          x0, x0, x1
>>>                    msr        sctlr_el3, x0
>>>                    isb
>>>
>>>                    b             done
>>>
>>>    This code has introduced a bug in our barebox porting.
>>>    It could be our mistake but then again we couldn't find any prerequisites
>>>    conditions that bootloaders need to meet before passing control to
>> barebox
>>>    pbl.
>>>    There are 2 bugs that can happen here:
>>>     1. The bootloader enabled MMU and set the SRAM (given to barebox) as
>>>        non-secure – This issue can be resolved with adding "dsb sy" command
>>>        before the "isb"
>>>     2. The bootloader enabled MMU and dcache on SRAM (given to barebox)
>> as
>>>        non-secure – This is a bit harder to solve because it needs to call
>>>        cache invalidate on the stack.
>>
>> Eventhough it might not be explicitly documented, barebox normally
>> expects to be called with MMU disabled.
>>
>> When the MMU is enabled it could have virtually any mapping and there's
>> no sane way to disable the MMU then.
>>
>> If your bootloader enables the MMU and there is no way to change that,
>> then it's up to your board entry code to disable the MMU before calling into
>> barebox. That said, when there's something can change in
>> arm_cpu_lowlevel_init()
>> to help you with your case, then we can do that, but I wouldn't consider
>> this a bug in barebox.
>>
>> Sascha
>>
>> --
>> Pengutronix e.K.                           |                             |
>> Steuerwalder Str. 21                       | http://secure-
>> web.cisco.com/17RdScP_TBoXqc1IerwhZX8kAHhqutjAnByPiUM5lNZJyN6OD
>> jkdTnyYCzrqpRZ9mdLg4wp-dZiElAak3iS7lf-YQwNFSattw-4zxIt6ro_qHh-
>> 8ovK33TBdMnr5qL5qPKFUriYYQ8bLMAnMHa4bKtTIfhZI2eSXUr6hqjDcnoKZ
>> GKUO2dbBDXceX3NT-ql9GBlwdH-mmkhd9fgYV2r0UtR4ot5d58hg45JTJ-
>> LsuvNVMsKrFZd1Iu3AiMSjmi4E5BDc6KpQx_MgAD410FQWC0HpDavhSc1DC
>> BgdMO2RlGrZk0L-
>> St3n_yH_QZvH4UfjY/http%3A%2F%2Fwww.pengutronix.de%2F  |
>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: ARM: aarch64: lowlevel: potential bug in arm_cpu_lowlevel_init

[Lior Weintraub]

Thanks Ahmad,
Cheers,
Lior.

> -----Original Message-----
> From: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Sent: Thursday, September 7, 2023 11:27 AM
> To: Lior Weintraub <liorw@pliops.com>; Sascha Hauer
> <s.hauer@pengutronix.de>
> Cc: andrew.smirnov@gmail.com; barebox@lists.infradead.org
> Subject: Re: ARM: aarch64: lowlevel: potential bug in arm_cpu_lowlevel_init
> 
> CAUTION: External Sender
> 
> Hello Lior,
> 
> On 16.08.23 12:14, Lior Weintraub wrote:
> > Thanks Sascha!
> >
> > Actually the bootloader is still under development so there is no problem to
> change and work without MMU.
> > (or at lease disable it before jumping to Barebox).
> > I totally agree that if virtual mapping was used it would be impossible to
> disable the MMU.
> > Just thought that it is totally harmless to add `dsb sy`.
> 
> as Sascha mentioned, we expect MMU to be off, but some BootROMs fail to
> do that in some error cases. I thus think if a barrier would save us
> in such a case, we should have it in arm_cpu_lowlevel_init().
> 
> Cheers,
> Ahmad
> 
> >
> > Cheers,
> > Lior.
> >
> >> -----Original Message-----
> >> From: Sascha Hauer <s.hauer@pengutronix.de>
> >> Sent: Wednesday, August 16, 2023 1:02 PM
> >> To: Lior Weintraub <liorw@pliops.com>
> >> Cc: andrew.smirnov@gmail.com; barebox@lists.infradead.org; Ahmad
> >> Fatoum <a.fatoum@pengutronix.de>
> >> Subject: Re: ARM: aarch64: lowlevel: potential bug in
> arm_cpu_lowlevel_init
> >>
> >> CAUTION: External Sender
> >>
> >> Hi Lior,
> >>
> >> On Mon, Aug 14, 2023 at 11:35:05AM +0000, Lior Weintraub wrote:
> >>>    Link: [1]File-List
> >>>    Hi Andrew,
> >>>
> >>>    I am asking about a patch you've introduced about 4 years ago:
> >>>    Commit: cd6e1857a6a824d562bd27379d191602c074f6b7
> >>>
> >>>    ENTRY(arm_cpu_lowlevel_init)
> >>>                    switch_el x1, 3f, 2f, 1f
> >>>
> >>>    3:
> >>>                    mov       x0, #1                                    /*
> >>>    Non-Secure EL0/1 */
> >>>                    orr          x0, x0, #(1 << 10)              /* 64-bit EL2
> >>>    */
> >>>                    msr        scr_el3, x0
> >>>                    msr        cptr_el3, xzr
> >>>
> >>>                    mrs        x0, sctlr_el3
> >>>                    ldr           x1, =SCTLR_ELx_FLAGS
> >>>                    bic          x0, x0, x1
> >>>                    msr        sctlr_el3, x0
> >>>                    isb
> >>>
> >>>                    b             done
> >>>
> >>>    This code has introduced a bug in our barebox porting.
> >>>    It could be our mistake but then again we couldn't find any prerequisites
> >>>    conditions that bootloaders need to meet before passing control to
> >> barebox
> >>>    pbl.
> >>>    There are 2 bugs that can happen here:
> >>>     1. The bootloader enabled MMU and set the SRAM (given to barebox)
> as
> >>>        non-secure – This issue can be resolved with adding "dsb sy"
> command
> >>>        before the "isb"
> >>>     2. The bootloader enabled MMU and dcache on SRAM (given to
> barebox)
> >> as
> >>>        non-secure – This is a bit harder to solve because it needs to call
> >>>        cache invalidate on the stack.
> >>
> >> Eventhough it might not be explicitly documented, barebox normally
> >> expects to be called with MMU disabled.
> >>
> >> When the MMU is enabled it could have virtually any mapping and there's
> >> no sane way to disable the MMU then.
> >>
> >> If your bootloader enables the MMU and there is no way to change that,
> >> then it's up to your board entry code to disable the MMU before calling into
> >> barebox. That said, when there's something can change in
> >> arm_cpu_lowlevel_init()
> >> to help you with your case, then we can do that, but I wouldn't consider
> >> this a bug in barebox.
> >>
> >> Sascha
> >>
> >> --
> >> Pengutronix e.K.                           |                             |
> >> Steuerwalder Str. 21                       | http://secure-
> >>
> web.cisco.com/17RdScP_TBoXqc1IerwhZX8kAHhqutjAnByPiUM5lNZJyN6OD
> >> jkdTnyYCzrqpRZ9mdLg4wp-dZiElAak3iS7lf-YQwNFSattw-4zxIt6ro_qHh-
> >>
> 8ovK33TBdMnr5qL5qPKFUriYYQ8bLMAnMHa4bKtTIfhZI2eSXUr6hqjDcnoKZ
> >> GKUO2dbBDXceX3NT-ql9GBlwdH-mmkhd9fgYV2r0UtR4ot5d58hg45JTJ-
> >>
> LsuvNVMsKrFZd1Iu3AiMSjmi4E5BDc6KpQx_MgAD410FQWC0HpDavhSc1DC
> >> BgdMO2RlGrZk0L-
> >> St3n_yH_QZvH4UfjY/http%3A%2F%2Fwww.pengutronix.de%2F  |
> >> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> >> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> >
> 
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://secure-
> web.cisco.com/1SHUS_p9EYTjEgUvZXKrq9NvnxBMmGacitjdSEASh4bUAik3yk
> i5yL5y5nluvQb7DkrLuqG_IPCzZqH8TSaWdOvJIyrVbvcGJ4pOtQ0NV4imoJZUX
> r37E4-w5SymjRxps76WVat_7azm-
> Ozm6ceDhQblgMTonfl7Vvi8ZBw8FQsuRKCkGr8t5RreU9oexo-
> qsvzXPANGzFzvv9Il3mmmRE3R8M0p_QxJV8xvS4AkYz7IR64Xv7Olh6z5I8Vgy
> QBxVyB3dEXXmhpY6Uoje_ksuei3vMtNQ0aPgYssmwVWolyIh1NKPorpFykcw
> VhjL9z1k/http%3A%2F%2Fwww.pengutronix.de%2F  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>