[U-Boot] [PATCH V2 1/2] ARM: tegra: reserve unmapped RAM so EFI doesn't use it

[Alexander Graf]

On 30.08.18 17:45, Stephen Warren wrote:
> On 08/30/2018 12:43 AM, Alexander Graf wrote:
>>
>>
>> On 29.08.18 23:52, Heinrich Schuchardt wrote:
>>> On 08/29/2018 11:34 PM, Stephen Warren wrote:
>>>> From: Stephen Warren <swarren at nvidia.com>
>>>>
>>>> Tegra U-Boot ensures that board_get_usable_ram_top() never returns a
>>>> value
>>>> over 4GB, since some peripherals can't access such addresses.
>>>> However, on
>>>> systems with more than 2GB of RAM, RAM bank 1 does describe this extra
>>>> RAM, so that Linux (or whatever OS) can use it, subject to DMA
>>>> limitations. Since board_get_usable_ram_top() points at the top of RAM
>>>> bank 0, the memory locations describes by RAM bank 1 are not mapped by
>>>> U-Boot's MMU configuration, and so cannot be used for anything.
>>>>
>>>> For some completely inexplicable reason, U-Boot's EFI support
>>>> ignores the
>>>> value returned by board_get_usable_ram_top(), and EFI memory allocation
>>>> routines will return values above U-Boot's RAM top. This causes
>>>> U-Boot to
>>>> crash when it accesses that RAM, since it isn't mapped by the MMU. One
>>>> use-case where this happens is TFTP download of a file on Jetson TX1
>>>> (p2371-2180).
>>>>
>>>> This change explicitly tells the EFI code that this extra RAM should
>>>> not
>>>> be used, thus avoiding the crash.
>>>>
>>>> A previous attempt to make EFI honor board_get_usable_ram_top() was
>>>> rejected. So, this patch will need to be replicated for any board that
>>>> implements board_get_usable_ram_top().
>>>>
>>>> Fixes: aa909462d018 ("efi_loader: efi_allocate_pages is too
>>>> restrictive")
>>>> Signed-off-by: Stephen Warren <swarren at nvidia.com>
>>>> ---
>>>> v2:
>>>> - Don't hard-code EFI page size.
>>>> - Register RAM as a boot services data rather than reserved.
>>>> ---
>>>>   arch/arm/mach-tegra/board2.c | 14 ++++++++++++++
>>>>   1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/arch/arm/mach-tegra/board2.c
>>>> b/arch/arm/mach-tegra/board2.c
>>>> index 421a71b3014d..f893966140a1 100644
>>>> --- a/arch/arm/mach-tegra/board2.c
>>>> +++ b/arch/arm/mach-tegra/board2.c
>>>> @@ -6,6 +6,7 @@
>>>>     #include <common.h>
>>>>   #include <dm.h>
>>>> +#include <efi_loader.h>
>>>>   #include <errno.h>
>>>>   #include <ns16550.h>
>>>>   #include <usb.h>
>>>> @@ -210,6 +211,19 @@ int board_early_init_f(void)
>>>>     int board_late_init(void)
>>>>   {
>>>> +#ifdef CONFIG_EFI_LOADER
>>>> +    if (gd->bd->bi_dram[1].start) {
>>>> +        /*
>>>> +         * Only bank 0 is below board_get_usable_ram_top(), so all of
>>>> +         * bank 1 is not mapped by the U-Boot MMU configuration,
>>>> and so
>>>> +         * we must prevent EFI from using it.
>>>> +         */
>>>> +        efi_add_memory_map(gd->bd->bi_dram[1].start,
>>>> +                   gd->bd->bi_dram[1].size / EFI_PAGE_SIZE,
>>>
>>> Are you sure all boards do the division without library function?
>>> This is why I prefer >> EFI_PAGE_SHIFT.
>>>
>>> Compiling SPL fails for harmony_defconfig.
>>>
>>>    LD      spl/common/spl/built-in.o
>>> arch/arm/mach-tegra/board2.c: In function ‘board_late_init’:
>>> arch/arm/mach-tegra/board2.c:221:3: warning: implicit declaration of
>>> function ‘efi_add_memory_map’; did you mean ‘fdt_add_mem_rsv’?
>>> [-Wimplicit-function-declaration]
>>>     efi_add_memory_map(gd->bd->bi_dram[1].start,
>>>     ^~~~~~~~~~~~~~~~~~
>>>     fdt_add_mem_rsv
> 
> Oh, I figured that warning were errors and only checked that Jenkins
> passed, not for warnings in the logs.
> 
>> I think that means you want #if CONFIG_IS_ENABLED(EFI_LOADER) because
>> the file is also compiled for SPL.
> 
> That does work, but include/efi_loader.h uses the following, so I think
> it makes sense to use the exact same ifdef in the source code:
> 
> #if defined(CONFIG_EFI_LOADER) && !defined(CONFIG_SPL_BUILD)
> 
> Or, should I update the header to use CONFIG_IS_ENABLED too (and hope
> that won't break any other boards).

Yes, please. I just didn't know about CONFIG_IS_ENABLED() back when I
wrote those lines.


Alex