[U-Boot] [PATCH 1/1] efi_loader: efi_allocate_pages is too restrictive

[Heinrich Schuchardt]

On 03/12/2018 11:48 AM, Alexander Graf wrote:
> On 03/09/2018 05:35 PM, Heinrich Schuchardt wrote:
>> On 03/09/2018 05:19 PM, Alexander Graf wrote:
>>> On 03/09/2018 04:58 PM, Heinrich Schuchardt wrote:
>>>> On 03/09/2018 01:48 PM, Alexander Graf wrote:
>>>>> On 03/03/2018 03:48 PM, Heinrich Schuchardt wrote:
>>>>>> When running on the sandbox the stack is not necessarily at a higher
>>>>>> memory
>>>>>> address than the highest free memory.
>>>>>>
>>>>>> There is no reason why the checking of the highest memory address
>>>>>> should be
>>>>>> more restrictive for EFI_ALLOCATE_ANY_PAGES than for
>>>>>> EFI_ALLOCATE_MAX_ADDRESS.
>>>>>>
>>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>>>>> ---
>>>>>>     lib/efi_loader/efi_memory.c | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/lib/efi_loader/efi_memory.c
>>>>>> b/lib/efi_loader/efi_memory.c
>>>>>> index cc271e0709d..0efbb973231 100644
>>>>>> --- a/lib/efi_loader/efi_memory.c
>>>>>> +++ b/lib/efi_loader/efi_memory.c
>>>>>> @@ -294,7 +294,7 @@ efi_status_t efi_allocate_pages(int type, int
>>>>>> memory_type,
>>>>>>         switch (type) {
>>>>>>         case EFI_ALLOCATE_ANY_PAGES:
>>>>>>             /* Any page */
>>>>>> -        addr = efi_find_free_memory(len, gd->start_addr_sp);
>>>>>> +        addr = efi_find_free_memory(len, (uint64_t)-1);
>>>>> This will break on systems that do not map high address space into the
>>>>> linear map (IIRC nvidia systems had that issue).
>>>>>
>>>> The memory map is also passed on to the operating system when booting.
>>>> If a memory reservation is missing for any board it has to be fixed in
>>>> the board or driver files, cf.
>>>>
>>>> sunxi: video: mark framebuffer as EFI reserved memory
>>>> https://lists.denx.de/pipermail/u-boot/2018-March/321820.htm
>>>>
>>>> For type = EFI_ALLOCATE_MAX_ADDRESS we don't care about
>>>> gd->start_addr_sp. So if the memory map is incomplete the current code
>>>> may fail. Keeping things as they are is not a viable option.
>>>>
>>>> Could you, please, identify for which Nvidia system a problem was
>>>> reported? Then we can add a call to efi_add_memory_map() for the board.
>>> Git blame points to this commit. I guess -1 should do the same thing
>>> then, true.
>>>
>>> Andreas, would you see any reason -1 will not work?
>> Are we talking about this line:
>>
>> arch/arm/mach-tegra/board2.c:317:
>> gd->pci_ram_top = gd->bd->bi_dram[0].start + gd->bd->bi_dram[0].size;
> 
> pci_ram_top != ram_top, no? :)
> 
> I'd rather assume it's this one:
> 
> /*
>  * Most hardware on 64-bit Tegra is still restricted to DMA to the lower
>  * 32-bits of the physical address space. Cap the maximum usable RAM area
>  * at 4 GiB to avoid DMA buffers from being allocated beyond the 32-bit
>  * boundary that most devices can address. Also, don't let U-Boot use any
>  * carve-out, as mentioned above.
>  *
>  * This function is called before dram_init_banksize(), so we can't simply
>  * return gd->bd->bi_dram[1].start + gd->bd->bi_dram[1].size.
>  */
> ulong board_get_usable_ram_top(ulong total_size)
> {
>         return CONFIG_SYS_SDRAM_BASE + usable_ram_size_below_4g();
> }
> 
> But the real problem is that ram_top of 0 is perfectly valid for 32bit
> systems.

But where is the problem? If ram_top == 0 on a 32bit system
efi_memory_init() will create a memory map for the high memory reaching
from start_addr_sp - uboot_stack_size to 0xffffffff
marked as EFI_LOADER_DATA.

So efi_allocate_pages will never touch that memory even if we start our
search at 0xffffffffffffffff.

And on a 32 bit system we will not have any memory map beyond 0xffffffff.

So essentially in this case the patch does not lead to a different
outcome of the search for free pages.

Best regards

Heinrich

> 
> Also ram_top may be used by platforms (like tegra again) to ensure that
> we don't use addresses >32bit for DMA. I guess the real solution to that
> would be to enable bounce buffers for tegra though. But we should make
> sure we don't regress tegra support, so we need to enable bounce buffers
> first for tegra and then allow the allocation to walk the full address
> space.
> 
> 
> Alex
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot