[PATCH 1/1] efi_loader: QEMU CONFIG_EFI_GRUB_ARM32_WORKAROUND=n

[PATCH 1/1] efi_loader: QEMU CONFIG_EFI_GRUB_ARM32_WORKAROUND=n

[Heinrich Schuchardt]

CONFIG_EFI_GRUB_ARM32 is only needed for architectures with caches that are
not managed via CP15 (or for some outdated buggy versions of GRUB). It
makes more sense to disable the setting per architecture than per defconfig.

Move QEMU's CONFIG_EFI_GRUB_ARM32_WORKAROUND=n from defconfig to Kconfig.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 configs/qemu_arm_defconfig | 1 -
 lib/efi_loader/Kconfig     | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/configs/qemu_arm_defconfig b/configs/qemu_arm_defconfig
index 1ffa727e63..278d8f41f4 100644
--- a/configs/qemu_arm_defconfig
+++ b/configs/qemu_arm_defconfig
@@ -56,4 +56,3 @@ CONFIG_USB=y
 CONFIG_DM_USB=y
 CONFIG_USB_EHCI_HCD=y
 CONFIG_USB_EHCI_PCI=y
-# CONFIG_EFI_GRUB_ARM32_WORKAROUND is not set
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index bad1a29ba8..ab42f3ba75 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -164,6 +164,7 @@ config EFI_HAVE_RUNTIME_RESET

 config EFI_GRUB_ARM32_WORKAROUND
 	bool "Workaround for GRUB on 32bit ARM"
+	default n if ARCH_QEMU
 	default y
 	depends on ARM && !ARM64
 	help
--
2.28.0

[PATCH 1/1] efi_loader: QEMU CONFIG_EFI_GRUB_ARM32_WORKAROUND=n

[Tom Rini]

On Thu, Sep 17, 2020 at 07:18:08PM +0200, Heinrich Schuchardt wrote:

> CONFIG_EFI_GRUB_ARM32 is only needed for architectures with caches that are
> not managed via CP15 (or for some outdated buggy versions of GRUB). It
> makes more sense to disable the setting per architecture than per defconfig.
> 
> Move QEMU's CONFIG_EFI_GRUB_ARM32_WORKAROUND=n from defconfig to Kconfig.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  configs/qemu_arm_defconfig | 1 -
>  lib/efi_loader/Kconfig     | 1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)

Why?  Are we about to move this to be a short list of "default y if ..."
instead?  Otherwise no, this is the level of detail that should be in a
defconfig I think and not Kconfig.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200917/4d2a2240/attachment.sig>

[PATCH 1/1] efi_loader: QEMU CONFIG_EFI_GRUB_ARM32_WORKAROUND=n

[Heinrich Schuchardt]

On 9/17/20 7:47 PM, Tom Rini wrote:
> On Thu, Sep 17, 2020 at 07:18:08PM +0200, Heinrich Schuchardt wrote:
>
>> CONFIG_EFI_GRUB_ARM32 is only needed for architectures with caches that are
>> not managed via CP15 (or for some outdated buggy versions of GRUB). It
>> makes more sense to disable the setting per architecture than per defconfig.
>>
>> Move QEMU's CONFIG_EFI_GRUB_ARM32_WORKAROUND=n from defconfig to Kconfig.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  configs/qemu_arm_defconfig | 1 -
>>  lib/efi_loader/Kconfig     | 1 +
>>  2 files changed, 1 insertion(+), 1 deletion(-)
>
> Why?  Are we about to move this to be a short list of "default y if ..."
> instead?  Otherwise no, this is the level of detail that should be in a
> defconfig I think and not Kconfig.
>

Hello Tom,

yes, in the long run I want to turn this into "default y if". But I
first need to a list of all ARCH_* which have a cache which is not
controlled via CP15.

Best regards

Heinrich

[PATCH 1/1] efi_loader: QEMU CONFIG_EFI_GRUB_ARM32_WORKAROUND=n

[Tom Rini]

On Thu, Sep 17, 2020 at 08:03:24PM +0200, Heinrich Schuchardt wrote:
> On 9/17/20 7:47 PM, Tom Rini wrote:
> > On Thu, Sep 17, 2020 at 07:18:08PM +0200, Heinrich Schuchardt wrote:
> >
> >> CONFIG_EFI_GRUB_ARM32 is only needed for architectures with caches that are
> >> not managed via CP15 (or for some outdated buggy versions of GRUB). It
> >> makes more sense to disable the setting per architecture than per defconfig.
> >>
> >> Move QEMU's CONFIG_EFI_GRUB_ARM32_WORKAROUND=n from defconfig to Kconfig.
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> ---
> >>  configs/qemu_arm_defconfig | 1 -
> >>  lib/efi_loader/Kconfig     | 1 +
> >>  2 files changed, 1 insertion(+), 1 deletion(-)
> >
> > Why?  Are we about to move this to be a short list of "default y if ..."
> > instead?  Otherwise no, this is the level of detail that should be in a
> > defconfig I think and not Kconfig.
> >
> 
> Hello Tom,
> 
> yes, in the long run I want to turn this into "default y if". But I
> first need to a list of all ARCH_* which have a cache which is not
> controlled via CP15.

Isn't the bigger problem / list "needs to support old / buggy GRUB" ?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200917/b5893f57/attachment.sig>

[PATCH 1/1] efi_loader: QEMU CONFIG_EFI_GRUB_ARM32_WORKAROUND=n

[Heinrich Schuchardt]

Am 17. September 2020 20:53:20 MESZ schrieb Tom Rini <trini@konsulko.com>:
>On Thu, Sep 17, 2020 at 08:03:24PM +0200, Heinrich Schuchardt wrote:
>> On 9/17/20 7:47 PM, Tom Rini wrote:
>> > On Thu, Sep 17, 2020 at 07:18:08PM +0200, Heinrich Schuchardt
>wrote:
>> >
>> >> CONFIG_EFI_GRUB_ARM32 is only needed for architectures with caches
>that are
>> >> not managed via CP15 (or for some outdated buggy versions of
>GRUB). It
>> >> makes more sense to disable the setting per architecture than per
>defconfig.
>> >>
>> >> Move QEMU's CONFIG_EFI_GRUB_ARM32_WORKAROUND=n from defconfig to
>Kconfig.
>> >>
>> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> >> ---
>> >>  configs/qemu_arm_defconfig | 1 -
>> >>  lib/efi_loader/Kconfig     | 1 +
>> >>  2 files changed, 1 insertion(+), 1 deletion(-)
>> >
>> > Why?  Are we about to move this to be a short list of "default y if
>..."
>> > instead?  Otherwise no, this is the level of detail that should be
>in a
>> > defconfig I think and not Kconfig.
>> >
>> 
>> Hello Tom,
>> 
>> yes, in the long run I want to turn this into "default y if". But I
>> first need to a list of all ARCH_* which have a cache which is not
>> controlled via CP15.
>
>Isn't the bigger problem / list "needs to support old / buggy GRUB" ?

There are no operating systems distributing 2021 U-Boot with 2018 GRUB.

Best regards

Heinrich

[PATCH 1/1] efi_loader: QEMU CONFIG_EFI_GRUB_ARM32_WORKAROUND=n

[Tom Rini]

On Fri, Sep 18, 2020 at 06:07:42AM +0200, Heinrich Schuchardt wrote:
> Am 17. September 2020 20:53:20 MESZ schrieb Tom Rini <trini@konsulko.com>:
> >On Thu, Sep 17, 2020 at 08:03:24PM +0200, Heinrich Schuchardt wrote:
> >> On 9/17/20 7:47 PM, Tom Rini wrote:
> >> > On Thu, Sep 17, 2020 at 07:18:08PM +0200, Heinrich Schuchardt
> >wrote:
> >> >
> >> >> CONFIG_EFI_GRUB_ARM32 is only needed for architectures with caches
> >that are
> >> >> not managed via CP15 (or for some outdated buggy versions of
> >GRUB). It
> >> >> makes more sense to disable the setting per architecture than per
> >defconfig.
> >> >>
> >> >> Move QEMU's CONFIG_EFI_GRUB_ARM32_WORKAROUND=n from defconfig to
> >Kconfig.
> >> >>
> >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> >> ---
> >> >>  configs/qemu_arm_defconfig | 1 -
> >> >>  lib/efi_loader/Kconfig     | 1 +
> >> >>  2 files changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > Why?  Are we about to move this to be a short list of "default y if
> >..."
> >> > instead?  Otherwise no, this is the level of detail that should be
> >in a
> >> > defconfig I think and not Kconfig.
> >> >
> >> 
> >> Hello Tom,
> >> 
> >> yes, in the long run I want to turn this into "default y if". But I
> >> first need to a list of all ARCH_* which have a cache which is not
> >> controlled via CP15.
> >
> >Isn't the bigger problem / list "needs to support old / buggy GRUB" ?
> 
> There are no operating systems distributing 2021 U-Boot with 2018 GRUB.

Then we should remove default y there after v2020.10 I think, and get
the distro folks to ack/review that.  I don't think there's any cases of
"SoC doesn't use CP15 for cache" and "SoC uses GRUB".

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200918/1a528e15/attachment.sig>

[PATCH 1/1] efi_loader: QEMU CONFIG_EFI_GRUB_ARM32_WORKAROUND=n

[Heinrich Schuchardt]

On 9/18/20 1:28 PM, Tom Rini wrote:
> On Fri, Sep 18, 2020 at 06:07:42AM +0200, Heinrich Schuchardt wrote:
>> Am 17. September 2020 20:53:20 MESZ schrieb Tom Rini <trini@konsulko.com>:
>>> On Thu, Sep 17, 2020 at 08:03:24PM +0200, Heinrich Schuchardt wrote:
>>>> On 9/17/20 7:47 PM, Tom Rini wrote:
>>>>> On Thu, Sep 17, 2020 at 07:18:08PM +0200, Heinrich Schuchardt
>>> wrote:
>>>>>
>>>>>> CONFIG_EFI_GRUB_ARM32 is only needed for architectures with caches
>>> that are
>>>>>> not managed via CP15 (or for some outdated buggy versions of
>>> GRUB). It
>>>>>> makes more sense to disable the setting per architecture than per
>>> defconfig.
>>>>>>
>>>>>> Move QEMU's CONFIG_EFI_GRUB_ARM32_WORKAROUND=n from defconfig to
>>> Kconfig.
>>>>>>
>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>>> ---
>>>>>>  configs/qemu_arm_defconfig | 1 -
>>>>>>  lib/efi_loader/Kconfig     | 1 +
>>>>>>  2 files changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> Why?  Are we about to move this to be a short list of "default y if
>>> ..."
>>>>> instead?  Otherwise no, this is the level of detail that should be
>>> in a
>>>>> defconfig I think and not Kconfig.
>>>>>
>>>>
>>>> Hello Tom,
>>>>
>>>> yes, in the long run I want to turn this into "default y if". But I
>>>> first need to a list of all ARCH_* which have a cache which is not
>>>> controlled via CP15.
>>>
>>> Isn't the bigger problem / list "needs to support old / buggy GRUB" ?
>>
>> There are no operating systems distributing 2021 U-Boot with 2018 GRUB.
>
> Then we should remove default y there after v2020.10 I think, and get
> the distro folks to ack/review that.  I don't think there's any cases of
> "SoC doesn't use CP15 for cache" and "SoC uses GRUB".
>

How about l2_cache_disable() in arch/arm/mach-kirkwood/cache.c?

Best regards

Heinrich

[PATCH 1/1] efi_loader: QEMU CONFIG_EFI_GRUB_ARM32_WORKAROUND=n

[Tom Rini]

On Fri, Sep 18, 2020 at 07:14:02PM +0200, Heinrich Schuchardt wrote:
> On 9/18/20 1:28 PM, Tom Rini wrote:
> > On Fri, Sep 18, 2020 at 06:07:42AM +0200, Heinrich Schuchardt wrote:
> >> Am 17. September 2020 20:53:20 MESZ schrieb Tom Rini <trini@konsulko.com>:
> >>> On Thu, Sep 17, 2020 at 08:03:24PM +0200, Heinrich Schuchardt wrote:
> >>>> On 9/17/20 7:47 PM, Tom Rini wrote:
> >>>>> On Thu, Sep 17, 2020 at 07:18:08PM +0200, Heinrich Schuchardt
> >>> wrote:
> >>>>>
> >>>>>> CONFIG_EFI_GRUB_ARM32 is only needed for architectures with caches
> >>> that are
> >>>>>> not managed via CP15 (or for some outdated buggy versions of
> >>> GRUB). It
> >>>>>> makes more sense to disable the setting per architecture than per
> >>> defconfig.
> >>>>>>
> >>>>>> Move QEMU's CONFIG_EFI_GRUB_ARM32_WORKAROUND=n from defconfig to
> >>> Kconfig.
> >>>>>>
> >>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>>>>> ---
> >>>>>>  configs/qemu_arm_defconfig | 1 -
> >>>>>>  lib/efi_loader/Kconfig     | 1 +
> >>>>>>  2 files changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> Why?  Are we about to move this to be a short list of "default y if
> >>> ..."
> >>>>> instead?  Otherwise no, this is the level of detail that should be
> >>> in a
> >>>>> defconfig I think and not Kconfig.
> >>>>>
> >>>>
> >>>> Hello Tom,
> >>>>
> >>>> yes, in the long run I want to turn this into "default y if". But I
> >>>> first need to a list of all ARCH_* which have a cache which is not
> >>>> controlled via CP15.
> >>>
> >>> Isn't the bigger problem / list "needs to support old / buggy GRUB" ?
> >>
> >> There are no operating systems distributing 2021 U-Boot with 2018 GRUB.
> >
> > Then we should remove default y there after v2020.10 I think, and get
> > the distro folks to ack/review that.  I don't think there's any cases of
> > "SoC doesn't use CP15 for cache" and "SoC uses GRUB".
> 
> How about l2_cache_disable() in arch/arm/mach-kirkwood/cache.c?

What's the board that uses that and where "SoC uses GRUB" comes in to
play.  What are the board(s) there we're talking about?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200918/04241995/attachment.sig>

[PATCH 1/1] efi_loader: QEMU CONFIG_EFI_GRUB_ARM32_WORKAROUND=n

[Mark Kettenis]

> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Date: Thu, 17 Sep 2020 20:03:24 +0200
> 
> On 9/17/20 7:47 PM, Tom Rini wrote:
> > On Thu, Sep 17, 2020 at 07:18:08PM +0200, Heinrich Schuchardt wrote:
> >
> >> CONFIG_EFI_GRUB_ARM32 is only needed for architectures with caches that are
> >> not managed via CP15 (or for some outdated buggy versions of GRUB). It
> >> makes more sense to disable the setting per architecture than per defconfig.
> >>
> >> Move QEMU's CONFIG_EFI_GRUB_ARM32_WORKAROUND=n from defconfig to Kconfig.
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> ---
> >>  configs/qemu_arm_defconfig | 1 -
> >>  lib/efi_loader/Kconfig     | 1 +
> >>  2 files changed, 1 insertion(+), 1 deletion(-)
> >
> > Why?  Are we about to move this to be a short list of "default y if ..."
> > instead?  Otherwise no, this is the level of detail that should be in a
> > defconfig I think and not Kconfig.
> >
> 
> Hello Tom,
> 
> yes, in the long run I want to turn this into "default y if". But I
> first need to a list of all ARCH_* which have a cache which is not
> controlled via CP15.

It is fairly standard for SoCs with Cortex-A9 cores.  Maybe some SoCs
with Cortex-A8 are affected as well.  Probably the list of affected
SoCs is those that implement v7_outer_cache_enable().

However, since other 32-bit ARM UEFI implementation are generally not
unavailable, I'm not sure disabling this workaround on other SoCs is
feasable since the code oath for booting OSes without the MMU enabled
may not be very well tested.

[PATCH 1/1] efi_loader: QEMU CONFIG_EFI_GRUB_ARM32_WORKAROUND=n

[Heinrich Schuchardt]

On 19.09.20 13:42, Mark Kettenis wrote:
>> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> Date: Thu, 17 Sep 2020 20:03:24 +0200
>>
>> On 9/17/20 7:47 PM, Tom Rini wrote:
>>> On Thu, Sep 17, 2020 at 07:18:08PM +0200, Heinrich Schuchardt wrote:
>>>
>>>> CONFIG_EFI_GRUB_ARM32 is only needed for architectures with caches that are
>>>> not managed via CP15 (or for some outdated buggy versions of GRUB). It
>>>> makes more sense to disable the setting per architecture than per defconfig.
>>>>
>>>> Move QEMU's CONFIG_EFI_GRUB_ARM32_WORKAROUND=n from defconfig to Kconfig.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> ---
>>>>  configs/qemu_arm_defconfig | 1 -
>>>>  lib/efi_loader/Kconfig     | 1 +
>>>>  2 files changed, 1 insertion(+), 1 deletion(-)
>>>
>>> Why?  Are we about to move this to be a short list of "default y if ..."
>>> instead?  Otherwise no, this is the level of detail that should be in a
>>> defconfig I think and not Kconfig.
>>>
>>
>> Hello Tom,
>>
>> yes, in the long run I want to turn this into "default y if". But I
>> first need to a list of all ARCH_* which have a cache which is not
>> controlled via CP15.
>
> It is fairly standard for SoCs with Cortex-A9 cores.  Maybe some SoCs
> with Cortex-A8 are affected as well.  Probably the list of affected
> SoCs is those that implement v7_outer_cache_enable().
>

git grep -n v7_outer_cache_enable

yields

arch/arm/cpu/armv7/cache_v7.c:210:__weak void v7_outer_cache_enable(void) {}
arch/arm/include/asm/armv7.h:128:void v7_outer_cache_enable(void);
arch/arm/mach-imx/cache.c:83:void v7_outer_cache_enable(void)
arch/arm/mach-mvebu/cpu.c:658:void v7_outer_cache_enable(void)
arch/arm/mach-omap2/omap3/board.c:433:void v7_outer_cache_enable(void)
arch/arm/mach-omap2/omap4/hwinit.c:166:void v7_outer_cache_enable(void)
arch/arm/mach-s5pc1xx/cache.c:27:void v7_outer_cache_enable(void)
arch/arm/mach-socfpga/misc.c:69:void v7_outer_cache_enable(void)
arch/arm/mach-uniphier/arm32/cache-uniphier.c:277:void
v7_outer_cache_enable(void)

arch/arm/mach-kirkwood/cache.c has an architecture specific
implementation of l2_cache_disable().

> However, since other 32-bit ARM UEFI implementation are generally not
> unavailable, I'm not sure disabling this workaround on other SoCs is
> feasable since the code oath for booting OSes without the MMU enabled
> may not be very well tested.
>

According to the UEFI spec we have to fulfill the following requirements
when handing over to the OS on ARM 32bit:

- MMU enabled
- Instruction and data caches enabled
- Access flag disabled
- Translation remap disabled
- Fast Context Switch Extension (FCSE) must be disabled
- caches requiring platform information to manage or invoke
  non-architectural cache/TLB lockdown mechanisms disabled

With the GRUB workaround enabled we call cleanup_before_linux().
According to README.arm_caches this shall disable the MMU.

Mark, I do not understand your concerns regarding a disabled MMU when
*not* calling cleanup_before_linux().

Best regards

Heinrich