[linux-sunxi] Re: [RFC PATCH] arm: EFI: Disallow EFI bootmgr when providing PSCI

[Andre Przywara]

On Sun, 24 Jan 2021 11:44:35 +0100
Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:

Hi Heinrich,

many thanks for digging into this!

> On 1/24/21 9:47 AM, Jernej Škrabec wrote:
> > Dne nedelja, 24. januar 2021 ob 09:33:15 CET je Jernej Škrabec napisal(a):  
> >> Hi!
> >>
> >> Dne nedelja, 24. januar 2021 ob 09:27:02 CET je Heinrich Schuchardt
> >>
> >> napisal(a):  
> >>> On 1/24/21 3:03 AM, Simon Glass wrote:  
> >>>> On Fri, 22 Jan 2021 at 05:05, Andre Przywara <andre.przywara at arm.com>  
> >>
> >> wrote:  
> >>>>> When "bootefi bootmgr" is run, it switches the CPU into non-secure
> >>>>> state. This breaks platforms like 32-bit Allwinner boards that rely on
> >>>>> running in secure state until late in the process, when they install
> >>>>> the PSCI handler in secure memory and drop into non-secure state.
> >>>>> They hang just before entering the kernel, after the "Starting the
> >>>>> kernel" message.  
> >>>
> >>> Dear Andre,
> >>>
> >>> thank you for reporting the issue.
> >>>
> >>> I have an Orange Pi PC with a 32 bit Allwinner CPU.
> >>> orangepi_pc_defconfig has CONFIG_ARMV7_PSCI=y.
> >>>
> >>> I use origin/master (e716c9022970dac9b) and the Orange PI boots
> >>> successfully using GRUB EFI into Linux 5.9.  
> >
> > Just one clarification - issue here is that "bootefi bootmgr" command
> > when unsuccessful breaks booting with bootm command.  
> 
> If I press the enter key to get into the console circumventing
> distro-boot, booting via bootz works. If I wait until distro_boot is
> finished, booting via bootz fails. Both with Linux 5.7.17.

Yes, that was exactly Jernej's and my observation.

> This confirms your finding that there is a problem with the
> initialization of the UEFI sub-system.
> 
> lib/efi_loader/efi_setup.c:192 is the only place where we call
> switch_to_non_secure_mode().
> 
> With the line removed:
> 
> * Booting via bootz is successful.
> * The EFI stub shows: Entering in SVC mode with MMU enabled
> * Booting via bootefi fails

Ah, thanks for giving this a try. I know next to nothing about U-Boot's
UEFI internals, so didn't dare to touch this code.

> switch_to_non_secure_mode() is safe to be called repeatedly. So we could
> move the switch_to_non_secure_mode() call to do_bootefi_exec(). This is
> after the boot manager has searched for a bootable image.
> 
> With the change (see diff below):
> 
> * Booting via bootz is successful.
> * EFI stub shows: Entering in HYP mode with MMU enabled
> * Booting via bootefi is successful.

Oh nice! I wasn't sure how much the UEFI initialisation actually relies
on non-secure mode, and if switching to non-secure *after* the
initialisation would change things.

> The downside of the change is that executing bootz will still fail after
> a UEFI binary returns to U-Boot.

OK, that is not nice, but not really a big issue. At least it's a lot
better than disabling bootmgr altogether or no bootz after a bootmgr
call.
If you deem this change being not very intrusive, I would very much
prefer this over my patch here.
 
> Running a shell in secure mode seems unwise. So we should dig a bit deeper:
> 
> Where in the code is the PSCI handler installed and where occurs the
> switch to non-secure state when booting via bootz/bootm? Can we move
> this to before distro-boot?

Well, yes, I think installing the PSCI handlers (and doing
everything that requires secure state) early is the best way forward,
but this is quite some change, and I would rather plug this problem now.

At the moment all of the v7 PSCI code is run as late as possible, so
U-Boot can run in secure state. I agree this *sounds* scary, but
running in secure is actually quite common for many ARM32 machines (even
Linux sometimes runs with the NS bit cleared).

For v7 Allwinner SoCs specifically we need access to the secure-only
SID registers for the MAC address generation, also secure SRAM becomes
inaccessible in non-secure world (as expected, but in contrast to the
ARMv8 chips).

So I think eventually we will need to bite the bullet and teach U-Boot
to cope with non-secure in sunxi-v7, but this needs some time and
requires possibly intrusive changes.

Cheers,
Andre

> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index c8eb5c32b0..81dd8e0284 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -8,6 +8,7 @@
>   #define LOG_CATEGORY LOGC_EFI
> 
>   #include <common.h>
> +#include <bootm.h>
>   #include <charset.h>
>   #include <command.h>
>   #include <dm.h>
> @@ -338,6 +339,9 @@ static efi_status_t do_bootefi_exec(efi_handle_t
> handle, void *load_options)
>          efi_uintn_t exit_data_size = 0;
>          u16 *exit_data = NULL;
> 
> +       /* On ARM switch from EL3 or secure mode to EL2 or non-secure
> mode */
> +       switch_to_non_secure_mode();
> +
>          /* Call our payload! */
>          ret = EFI_CALL(efi_start_image(handle, &exit_data_size,
> &exit_data));
>          if (ret != EFI_SUCCESS) {
> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index 5800cbf6d4..588fbda736 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -188,9 +188,6 @@ efi_status_t efi_init_obj_list(void)
>          /* Allow unaligned memory access */
>          allow_unaligned();
> 
> -       /* On ARM switch from EL3 or secure mode to EL2 or non-secure
> mode */
> -       switch_to_non_secure_mode();
> -
>          /* Initialize root node */
>          ret = efi_root_node_register();
>          if (ret != EFI_SUCCESS)