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

[Stephen Warren]

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).