Re: [PATCH v2 1/2] efi/arm: decompressor: deal with HYP mode boot gracefully

[Chen-Yu Tsai <wens@csie.org>]

On Mon, Jun 8, 2020 at 7:09 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Sun, 7 Jun 2020 at 19:24, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 6/7/20 3:58 PM, Ard Biesheuvel wrote:
> > > EFI on ARM only supports short descriptors, and given that it mandates
> > > that the MMU and caches are on, it is implied that booting in HYP mode
> > > is not supported.
> > >
> > > However, implementations of EFI exist (i.e., U-Boot) that ignore this
> > > requirement, which is not entirely unreasonable, given that it makes
> > > HYP mode inaccessible to the operating system.
> > >
> > > So let's make sure that we can deal with this condition gracefully.
> > > We already tolerate booting the EFI stub with the caches off (even
> > > though this violates the EFI spec as well), and so we should deal
> > > with HYP mode boot with MMU and caches either on or off.
> > >
> > > - When the MMU and caches are on, we can ignore the HYP stub altogether,
> > >   since we can carry on executing at HYP. We do need to ensure that we
> > >   disable the MMU at HYP before entering the kernel proper.
> > >
> > > - When the MMU and caches are off, we have to drop to SVC mode so that
> > >   we can set up the page tables using short descriptors. In this case,
> > >   we need to install the HYP stub as usual, so that we can return to HYP
> > >   mode before handing over to the kernel proper.
> >
> > To me it is still unclear why you need this patch. Please, describe the
> > problem this patch fixes.
> >
> > Is there any device that you cannot boot without the patch?
> >
>
> The code as it is today is broken, and if it works, it only does so by accident.
>
> (There were some recent changes but the old code is broken in a similar way)
>
> When we enter via the stub, we used to call cache_off() to disable the
> caches before calling the decompressor entry point. However, that
> disables the SVC mode caches, not the HYP mode caches, and so if we
> enter via EFI at HYP, we will call __hyp_stub_install() with the HYP
> mod MMU and caches enabled, which is explicitly forbidden (see
> hyp-stub.S)
>
> With the recent change, the EFI entry code doesn't call cache_off()
> anymore, but that does not remove the problem, it just moves it to the
> point where we hand over to the kernel proper.
>
> The problem is really on the u-boot side, and so we either have to
> follow the letter of the EFI spec and ban the practice of booting in
> HYP mode or with the caches off, or we work around this like I do in
> this patch. Doing nothing is not really an option.
>
> If we want EBBR and U-boot as EFI firmware to succeed, we should
> really fix issues such as these, and not pretend everything is fine
> when we know it is broken but happens to work on the few boards that
> we test. This is the reason we have architecture and firmware specs in
> the first place, and it is really quite unfortunate that we did not
> catch these u-boot issues before.
>
> As I said, I think booting at HYP can be tolerated, since the OS loses
> access to it otherwise (and maybe we should even update the EFI spec
> to allow this). But fiddling with the caches like we do should really
> be avoided (and the GRUB hack really needs to go as well)
>
>
>
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > >  arch/arm/boot/compressed/head.S | 61 ++++++++++++++++++++
> > >  1 file changed, 61 insertions(+)
> > >
> > > diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
> > > index c79db44ba128..3476e85c31e7 100644
> > > --- a/arch/arm/boot/compressed/head.S
> > > +++ b/arch/arm/boot/compressed/head.S
> > > @@ -1410,7 +1410,11 @@ memdump:       mov     r12, r0
> > >  __hyp_reentry_vectors:
> > >               W(b)    .                       @ reset
> > >               W(b)    .                       @ undef
> > > +#ifdef CONFIG_EFI_STUB
> > > +             W(b)    __enter_kernel_from_hyp @ hvc from HYP
> > > +#else
> > >               W(b)    .                       @ svc
> > > +#endif
> > >               W(b)    .                       @ pabort
> > >               W(b)    .                       @ dabort
> > >               W(b)    __enter_kernel          @ hyp
> > > @@ -1429,14 +1433,71 @@ __enter_kernel:
> > >  reloc_code_end:
> > >
> > >  #ifdef CONFIG_EFI_STUB
> > > +__enter_kernel_from_hyp:
> > > +             mrc     p15, 4, r0, c1, c0, 0   @ read HSCTLR
> > > +             bic     r0, r0, #0x5            @ disable MMU and caches
> > > +             mcr     p15, 4, r0, c1, c0, 0   @ write HSCTLR
> > > +             isb
> > > +             b       __enter_kernel
> > > +
> > >  ENTRY(efi_enter_kernel)
> > >               mov     r4, r0                  @ preserve image base
> > >               mov     r8, r1                  @ preserve DT pointer
> > >
> > > + ARM(                adrl    r0, call_cache_fn       )
> > > + THUMB(              adr     r0, call_cache_fn       )
> > > +             adr     r1, 0f                  @ clean the region of code we
> > > +             bl      cache_clean_flush       @ may run with the MMU off
> > > +
> > > +#ifdef CONFIG_ARM_VIRT_EXT
> > > +             @
> > > +             @ The EFI spec does not support booting on ARM in HYP mode,
> > > +             @ since it mandates that the MMU and caches are on, with all
> > > +             @ 32-bit addressable DRAM mapped 1:1 using short descriptors.
> > > +             @
> > > +             @ While the EDK2 reference implementation adheres to this,
> > > +             @ U-Boot might decide to enter the EFI stub in HYP mode
> > > +             @ anyway, with the MMU and caches either on or off.
> > > +             @
> > > +             mrs     r0, cpsr                @ get the current mode
> > > +             msr     spsr_cxsf, r0           @ record boot mode
> > > +             and     r0, r0, #MODE_MASK      @ are we running in HYP mode?
> > > +             cmp     r0, #HYP_MODE
> > > +             bne     .Lefi_svc
> > > +
> > > +             mrc     p15, 4, r1, c1, c0, 0   @ read HSCTLR
> > > +             tst     r1, #0x1                @ MMU enabled at HYP?
> > > +             beq     1f
> > > +
> > > +             @
> > > +             @ When running in HYP mode with the caches on, we're better
> > > +             @ off just carrying on using the cached 1:1 mapping that the
> > > +             @ firmware provided. Set up the HYP vectors so HVC instructions
> > > +             @ issued from HYP mode take us to the correct handler code. We
> > > +             @ will disable the MMU before jumping to the kernel proper.
> > > +             @
> > > +             adr     r0, __hyp_reentry_vectors
> > > +             mcr     p15, 4, r0, c12, c0, 0  @ set HYP vector base (HVBAR)
> > > +             isb
> > > +             b       .Lefi_hyp
> > > +
> > > +             @
> > > +             @ When running in HYP mode with the caches off, we need to drop
> > > +             @ into SVC mode now, and let the decompressor set up its cached
> > > +             @ 1:1 mapping as usual.
> > > +             @
> > > +1:           mov     r9, r4                  @ preserve image base
> > > +             bl      __hyp_stub_install      @ install HYP stub vectors
> > > +             safe_svcmode_maskall    r1      @ drop to SVC mode
> >
> > Are you returning to HYP mode somewhere?
> >
>
> Yes.
>
> > What is the effect on PSCI?
> >
>
> If you boot Linux in HYP then you cannot have PSCI in HYP as well.
> Linux will take ownership of HYP mode, and remove whatever was there.
> If you want to run PSCI at HYP, then the OS needs to boot in SVC mode.
>
> > The Allwinner/Sunxi boards must be booted in HYP mode to have PSCI
> > according to https://linux-sunxi.org/PSCI
> >
>
> See above. If PSCI runs in HYP, the OS needs to run at SVC

Heinrich probably misunderstood this. That page is saying that for PSCI
to be available to Linux, U-boot must be booting Linux in non-secure mode.
That is because the PSCI implementation runs in secure monitor mode under
Linux. Linux can boot in HYP or SVC as it so chooses.

> > Did you test that you still can reboot those boards?
> >
>
> No, I don't have such a board.

I'm not sure what reboot has to do with this, since the U-boot PSCI
implementation for sunxi does not support reboot or power-off. It only
supports CPU power on and off. The whole reason for this is to support
SMP in a way that HYP mode can be used.

Hope this clears things up. I'm not familiar with the specifics of
EFI nor do I use it in my setup.


Regards
ChenYu

> > Cc: Chen-Yu Tsai <wens@csie.org>
> >     (maintainer ARM/Allwinner sunXi SoC support)
> >
> > Best regards
> >
> > Heinrich
> >
> > > +             orr     r4, r9, #1              @ restore image base and set LSB
> > > +             b       .Lefi_hyp
> > > +.Lefi_svc:
> > > +#endif
> > >               mrc     p15, 0, r0, c1, c0, 0   @ read SCTLR
> > >               tst     r0, #0x1                @ MMU enabled?
> > >               orreq   r4, r4, #1              @ set LSB if not
> > >
> > > +.Lefi_hyp:
> > >               mov     r0, r8                  @ DT start
> > >               add     r1, r8, r2              @ DT end
> > >               bl      cache_clean_flush
> > >
> >