Patchwork [v2] efi/arm: enable CP15 DMB instructions before cleaning the cache

login
register
mail settings
Submitter Ard Biesheuvel
Date April 9, 2019, 4:42 p.m.
Message ID <20190409164220.5993-1-ard.biesheuvel@linaro.org>
Download mbox | patch
Permalink /patch/768813/
State New
Headers show

Comments

Ard Biesheuvel - April 9, 2019, 4:42 p.m.
The EFI stub is entered with the caches and MMU enabled by the
firmware, and once the stub is ready to hand over to the decompressor,
we clean and disable the caches.

The cache clean routines use CP15 barrier instructions, which can be
disabled via SCTLR. Normally, when using the provided cache handling
routines to enable the caches and MMU, this bit is enabled as well.
However, but since we entered the stub with the caches already enabled,
this routine is not executed before we call the cache clean routines,
resulting in undefined instruction exceptions if the firmware never
enabled this bit.

So set the bit explicitly in the EFI entry code, but do so in a way that
guarantees that the resulting code can still run on v6 cores as well
(which are guaranteed to have CP15 barriers enabled)

Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/boot/compressed/head.S | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
Ard Biesheuvel - April 12, 2019, 8:38 p.m.
On Tue, 9 Apr 2019 at 09:42, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> The EFI stub is entered with the caches and MMU enabled by the
> firmware, and once the stub is ready to hand over to the decompressor,
> we clean and disable the caches.
>
> The cache clean routines use CP15 barrier instructions, which can be
> disabled via SCTLR. Normally, when using the provided cache handling
> routines to enable the caches and MMU, this bit is enabled as well.
> However, but since we entered the stub with the caches already enabled,
> this routine is not executed before we call the cache clean routines,
> resulting in undefined instruction exceptions if the firmware never
> enabled this bit.
>
> So set the bit explicitly in the EFI entry code, but do so in a way that
> guarantees that the resulting code can still run on v6 cores as well
> (which are guaranteed to have CP15 barriers enabled)
>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm/boot/compressed/head.S | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
> index 6c7ccb428c07..c174098acdf1 100644
> --- a/arch/arm/boot/compressed/head.S
> +++ b/arch/arm/boot/compressed/head.S
> @@ -1438,7 +1438,21 @@ ENTRY(efi_stub_entry)
>
>                 @ Preserve return value of efi_entry() in r4
>                 mov     r4, r0
> -               bl      cache_clean_flush
> +
> +               @ our cache maintenance code relies on CP15 barrier instructions
> +               @ but since we arrived here with the MMU and caches configured
> +               @ by UEFI, we must check that the CP15BEN bit is set in SCTLR.
> +               @ Note that this bit is RAO/WI on v6 and earlier, so the ISB in
> +               @ the enable path will be executed on v7+ only.
> +               mrc     p15, 0, r1, c1, c0, 0   @ read SCTLR
> +               tst     r1, #(1 << 5)           @ CP15BEN bit set?
> +               bne     0f
> +               orr     r1, r1, #(1 << 5)       @ CP15 barrier instructions
> +               mcr     p15, 0, r1, c1, c0, 0   @ write SCTLR
> + ARM(          .inst   0xf57ff06f              @ isb           )
> + THUMB(                isb                                             )
> +
> +0:             bl      cache_clean_flush
>                 bl      cache_off
>
>                 @ Set parameters for booting zImage according to boot protocol
> --
> 2.17.1
>

Russell, do you mind if I take this through the EFI tree? I had
trouble logging into your patch system (it no longer recognizes my
email address but when I try to re-create the account, my name is
already taken)

Thanks,
Ard.
Russell King - ARM Linux - April 12, 2019, 8:58 p.m.
On Fri, Apr 12, 2019 at 01:38:12PM -0700, Ard Biesheuvel wrote:
> On Tue, 9 Apr 2019 at 09:42, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > The EFI stub is entered with the caches and MMU enabled by the
> > firmware, and once the stub is ready to hand over to the decompressor,
> > we clean and disable the caches.
> >
> > The cache clean routines use CP15 barrier instructions, which can be
> > disabled via SCTLR. Normally, when using the provided cache handling
> > routines to enable the caches and MMU, this bit is enabled as well.
> > However, but since we entered the stub with the caches already enabled,
> > this routine is not executed before we call the cache clean routines,
> > resulting in undefined instruction exceptions if the firmware never
> > enabled this bit.
> >
> > So set the bit explicitly in the EFI entry code, but do so in a way that
> > guarantees that the resulting code can still run on v6 cores as well
> > (which are guaranteed to have CP15 barriers enabled)
> >
> > Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  arch/arm/boot/compressed/head.S | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
> > index 6c7ccb428c07..c174098acdf1 100644
> > --- a/arch/arm/boot/compressed/head.S
> > +++ b/arch/arm/boot/compressed/head.S
> > @@ -1438,7 +1438,21 @@ ENTRY(efi_stub_entry)
> >
> >                 @ Preserve return value of efi_entry() in r4
> >                 mov     r4, r0
> > -               bl      cache_clean_flush
> > +
> > +               @ our cache maintenance code relies on CP15 barrier instructions
> > +               @ but since we arrived here with the MMU and caches configured
> > +               @ by UEFI, we must check that the CP15BEN bit is set in SCTLR.
> > +               @ Note that this bit is RAO/WI on v6 and earlier, so the ISB in
> > +               @ the enable path will be executed on v7+ only.
> > +               mrc     p15, 0, r1, c1, c0, 0   @ read SCTLR
> > +               tst     r1, #(1 << 5)           @ CP15BEN bit set?
> > +               bne     0f
> > +               orr     r1, r1, #(1 << 5)       @ CP15 barrier instructions
> > +               mcr     p15, 0, r1, c1, c0, 0   @ write SCTLR
> > + ARM(          .inst   0xf57ff06f              @ isb           )
> > + THUMB(                isb                                             )
> > +
> > +0:             bl      cache_clean_flush
> >                 bl      cache_off
> >
> >                 @ Set parameters for booting zImage according to boot protocol
> > --
> > 2.17.1
> >
> 
> Russell, do you mind if I take this through the EFI tree?

Yes I do mind.

> I had trouble logging into your patch system (it no longer recognizes my
> email address

I can't debug it based on that - what does it say?

> but when I try to re-create the account, my name is already taken)

Yes, duplicate accounts can't be created, but there is also a forgotten
password feature.

I've just checked and I can log in fine.
Ard Biesheuvel - April 12, 2019, 9 p.m.
On Fri, 12 Apr 2019 at 13:58, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Fri, Apr 12, 2019 at 01:38:12PM -0700, Ard Biesheuvel wrote:
> > On Tue, 9 Apr 2019 at 09:42, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > >
> > > The EFI stub is entered with the caches and MMU enabled by the
> > > firmware, and once the stub is ready to hand over to the decompressor,
> > > we clean and disable the caches.
> > >
> > > The cache clean routines use CP15 barrier instructions, which can be
> > > disabled via SCTLR. Normally, when using the provided cache handling
> > > routines to enable the caches and MMU, this bit is enabled as well.
> > > However, but since we entered the stub with the caches already enabled,
> > > this routine is not executed before we call the cache clean routines,
> > > resulting in undefined instruction exceptions if the firmware never
> > > enabled this bit.
> > >
> > > So set the bit explicitly in the EFI entry code, but do so in a way that
> > > guarantees that the resulting code can still run on v6 cores as well
> > > (which are guaranteed to have CP15 barriers enabled)
> > >
> > > Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > >  arch/arm/boot/compressed/head.S | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
> > > index 6c7ccb428c07..c174098acdf1 100644
> > > --- a/arch/arm/boot/compressed/head.S
> > > +++ b/arch/arm/boot/compressed/head.S
> > > @@ -1438,7 +1438,21 @@ ENTRY(efi_stub_entry)
> > >
> > >                 @ Preserve return value of efi_entry() in r4
> > >                 mov     r4, r0
> > > -               bl      cache_clean_flush
> > > +
> > > +               @ our cache maintenance code relies on CP15 barrier instructions
> > > +               @ but since we arrived here with the MMU and caches configured
> > > +               @ by UEFI, we must check that the CP15BEN bit is set in SCTLR.
> > > +               @ Note that this bit is RAO/WI on v6 and earlier, so the ISB in
> > > +               @ the enable path will be executed on v7+ only.
> > > +               mrc     p15, 0, r1, c1, c0, 0   @ read SCTLR
> > > +               tst     r1, #(1 << 5)           @ CP15BEN bit set?
> > > +               bne     0f
> > > +               orr     r1, r1, #(1 << 5)       @ CP15 barrier instructions
> > > +               mcr     p15, 0, r1, c1, c0, 0   @ write SCTLR
> > > + ARM(          .inst   0xf57ff06f              @ isb           )
> > > + THUMB(                isb                                             )
> > > +
> > > +0:             bl      cache_clean_flush
> > >                 bl      cache_off
> > >
> > >                 @ Set parameters for booting zImage according to boot protocol
> > > --
> > > 2.17.1
> > >
> >
> > Russell, do you mind if I take this through the EFI tree?
>
> Yes I do mind.
>

Fair enough

> > I had trouble logging into your patch system (it no longer recognizes my
> > email address
>
> I can't debug it based on that - what does it say?
>
> > but when I try to re-create the account, my name is already taken)
>
> Yes, duplicate accounts can't be created, but there is also a forgotten
> password feature.
>
> I've just checked and I can log in fine.
>


If I use the forgotten password feature, it tells me

"""
The email address you provided (ard.biesheuvel@linaro.org) is not
valid. Please try again.
"""

(and I am typing the numbers correctly, in case you were wondering)
Russell King - ARM Linux - April 12, 2019, 9:20 p.m.
On Fri, Apr 12, 2019 at 02:00:16PM -0700, Ard Biesheuvel wrote:
> If I use the forgotten password feature, it tells me
> 
> """
> The email address you provided (ard.biesheuvel@linaro.org) is not
> valid. Please try again.
> """

Fixed, consequence of the upgrades back in January, requiring a
change to the regular expression system, which caused my simple
fixups to then generate an invalid RE.  You should have received
the new password as a result of my testing.

Thanks for reporting the problem - that's the only way problems
get fixed!
Ard Biesheuvel - April 12, 2019, 9:21 p.m.
On Fri, 12 Apr 2019 at 14:21, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Fri, Apr 12, 2019 at 02:00:16PM -0700, Ard Biesheuvel wrote:
> > If I use the forgotten password feature, it tells me
> >
> > """
> > The email address you provided (ard.biesheuvel@linaro.org) is not
> > valid. Please try again.
> > """
>
> Fixed, consequence of the upgrades back in January, requiring a
> change to the regular expression system, which caused my simple
> fixups to then generate an invalid RE.  You should have received
> the new password as a result of my testing.
>
> Thanks for reporting the problem - that's the only way problems
> get fixed!
>

Indeed. Thanks for solving it so quickly.

Patch

diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
index 6c7ccb428c07..c174098acdf1 100644
--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -1438,7 +1438,21 @@  ENTRY(efi_stub_entry)
 
 		@ Preserve return value of efi_entry() in r4
 		mov	r4, r0
-		bl	cache_clean_flush
+
+		@ our cache maintenance code relies on CP15 barrier instructions
+		@ but since we arrived here with the MMU and caches configured
+		@ by UEFI, we must check that the CP15BEN bit is set in SCTLR.
+		@ Note that this bit is RAO/WI on v6 and earlier, so the ISB in
+		@ the enable path will be executed on v7+ only.
+		mrc	p15, 0, r1, c1, c0, 0	@ read SCTLR
+		tst	r1, #(1 << 5)		@ CP15BEN bit set?
+		bne	0f
+		orr	r1, r1, #(1 << 5)	@ CP15 barrier instructions
+		mcr	p15, 0, r1, c1, c0, 0	@ write SCTLR
+ ARM(		.inst	0xf57ff06f		@ isb		)
+ THUMB(		isb						)
+
+0:		bl	cache_clean_flush
 		bl	cache_off
 
 		@ Set parameters for booting zImage according to boot protocol