[PATCH] efi_loader: replace board_get_usable_ram_top by gd->ram_top

[Patrick DELAUNAY]

Hi Heinrich

On 7/18/21 9:29 AM, Heinrich Schuchardt wrote:
>
>
> On 7/9/21 12:46 PM, Patrick Delaunay wrote:
>> As gd->ram_top = board_get_usable_ram_top() in board_r
>> the EFI loader don't need to call this function again and after 
>> relocation.
>>
>> This patch avoid issue if board assumed that this function is called
>> only one time and before relocation.
>
> Hello Patrick,
>
> Which implementation of board_get_usable_ram_top() assumes that it is
> called only once before relocation? Please, mention this in the commit
> message.
>
I see the issue occur in stm32mp platform, in 
arch/arm/mach-stm32mp/dram_init.c

I made some treatment in this function = parsing of device tree to found the

reserved memory - reserved for OP-TEE - and cache attribute update.


Before the commit 7dc6068fc10d ("stm32mp: Increase the reserved memory in

board_get_usable_ram_top"), mmu_set_region_dcache_behaviour was called

unconditionally, and cause issue when the board_get_usable_ram_top()

was called in efi code (after relocation)


So I already correct this issue with the added test:

+    /* before relocation, mark the U-Boot memory as cacheable by default */
+    if (!(gd->flags & GD_FLG_RELOC))
+        mmu_set_region_dcache_behaviour(reg, size, DCACHE_DEFAULT_OPTION);


And I propose the patch to avoid issue on other platform...


But I don't correctly handle the case total_size = 0....


> gd->ram_top is set in multiple places:
>
> arch/arm/mach-rockchip/spl.c:149:       gd->ram_top = gd->ram_base +
> get_effective_memsize();
> arch/arm/mach-rockchip/spl.c:150: gd->ram_top =
> board_get_usable_ram_top(gd->ram_size);

yes

=> void board_init_f(ulong dummy)

> arch/arm/cpu/armv8/fsl-layerscape/spl.c:114: gd->ram_top =
> gd->bd->bi_dram[0].start + gd->bd->bi_dram[0].size;
>
yes

=>voidboard_init_f(ulong dummy)


The booth case are only for SPL not for U-Boot proper ...

where ram_top is only handle in common/board_f.c


> I guess you refer to:
>
> common/board_f.c:345:   gd->ram_top = gd->ram_base +
> get_effective_memsize();
> common/board_f.c:346:   gd->ram_top = 
> board_get_usable_ram_top(gd->mon_len);
>

Yes

in  first analysis, I assume that the computation is done one time

and we don't have reason to do it again.


> I would not expect board_get_usable_ram_top(gd->mon_len) and
> board_get_usable_ram_top(0) to be the same. So, please, describe in your
> patch why you assume that board_get_usable_ram_top(gd->mon_len) is the
> value we should use.


No, I don't assume it should be the same value....

and it shoul be the case for stm32mp arch.


When I propose the patch, I don't think that board_get_usable_ram_top should

use, except to select the U-Boot relocation address.


But after reflection and a other check I agree that for EFI point of 
view ram_top

should be respected (to present correct memory mapping to kernle)

and it is more a issue in stm32mp platform...


=> I need to support correctly board_get_usable_ram_top(0)



>
> Best regards
>
> Heinrich
>
>>

conclusion: I abandon this patch.


Patrick