[U-Boot] [PATCH 1/1] efi_loader: re-enable GRUB workaround on 32bit ARM

[Alexander Graf]

On 27.07.19 10:02, Heinrich Schuchardt wrote:
> GRUB on ARM 32bit prior to version 2.04 lacks proper handling of caches.
> In U-Boot v2019.04 a workaround for this was inadvertently removed.
>
> The workaround is currently also needed for booting on systems with caches
> that cannot be managed via CP15 (e.g. with an i.MX6 CPU).
>
> Re-enable the workaround and make it customizable.
>
> Fixes: f69d63fae281 ("efi_loader: use efi_start_image() for bootefi")
> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> ---
>   lib/efi_loader/Kconfig        |  8 ++++++++
>   lib/efi_loader/efi_boottime.c | 28 +++++++++++++---------------
>   2 files changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index a7f2c68fa9..c7027a9676 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -107,4 +107,12 @@ config EFI_HAVE_RUNTIME_RESET
>   	default y
>   	depends on ARCH_BCM283X || FSL_LAYERSCAPE || PSCI_RESET || SYSRESET_X86
>
> +config EFI_GRUB_ARM32_WORKAROUND
> +	bool "Workaround for GRUB on 32bit ARM"
> +	default y
> +	depends on ARM && !ARM64
> +	help
> +	  GRUB prior to version 2.04 requires U-Boot to disable caches. This
> +	  workaround currently is also needed on systems with caches that
> +	  cannot be managed via CP15.
>   endif
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 4f6e8d1679..f75ca1a67c 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -39,14 +39,6 @@ LIST_HEAD(efi_register_notify_events);
>   /* Handle of the currently executing image */
>   static efi_handle_t current_image;
>
> -/*
> - * If we're running on nasty systems (32bit ARM booting into non-EFI Linux)
> - * we need to do trickery with caches. Since we don't want to break the EFI
> - * aware boot path, only apply hacks when loading exiting directly (breaking
> - * direct Linux EFI booting along the way - oh well).
> - */
> -static bool efi_is_direct_boot = true;


By removing the toggling, you now couple a U-Boot configuration variant 
to a specific grub version. New grub versions will run Linux using the 
EFI stub. So enabling CONFIG_EFI_GRUB_ARM32_WORKAROUND means only old 
versions work, but new grub versions break.

We really should treat this like any other erratum and make it as 
runtime detected as possible, so that we don't block people from making 
forward progress towards a the "sane" boot path.

Btw, has there been any solution for the non-CP15 caches? They would 
still be broken with this applied, as payloads can rely on CP15 caches 
enabled (for unaligned accesses), but non-CP15 caches disabled, right?


Thanks,

Alex


> -
>   #ifdef CONFIG_ARM
>   /*
>    * The "gd" pointer lives in a register on ARM and AArch64 that we declare
> @@ -1911,13 +1903,21 @@ error:
>    */
>   static void efi_exit_caches(void)
>   {
> -#if defined(CONFIG_ARM) && !defined(CONFIG_ARM64)
> +#if defined(CONFIG_EFI_GRUB_ARM32_WORKAROUND)
>   	/*
> -	 * Grub on 32bit ARM needs to have caches disabled before jumping into
> -	 * a zImage, but does not know of all cache layers. Give it a hand.
> +	 * Boooting Linux via GRUB prior to version 2.04 fails on 32bit ARM if
> +	 * caches are enabled.
> +	 *
> +	 * TODO:
> +	 * According to the UEFI spec caches that can be managed via CP15
> +	 * operations should be enabled. Caches requiring platform information
> +	 * to manage should be disabled. This should not happen in
> +	 * ExitBootServices() but before invoking any UEFI binary is invoked.
> +	 *
> +	 * We want to keep the current workaround while GRUB prior to version
> +	 * 2.04 is still in use.
>   	 */
> -	if (efi_is_direct_boot)
> -		cleanup_before_linux();
> +	cleanup_before_linux();
>   #endif
>   }
>
> @@ -2893,8 +2893,6 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>   	if (ret != EFI_SUCCESS)
>   		return EFI_EXIT(EFI_INVALID_PARAMETER);
>
> -	efi_is_direct_boot = false;
> -
>   	image_obj->exit_data_size = exit_data_size;
>   	image_obj->exit_data = exit_data;
>
> --
> 2.20.1
>