[PATCH] efi_loader: replace board_get_usable_ram_top by gd->ram_top
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.
Signed-off-by: Patrick Delaunay patrick.delaunay@foss.st.com ---
https://source.denx.de/u-boot/custodians/u-boot-stm/-/pipelines/7399
lib/efi_loader/efi_memory.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index be2f655dff..f5bc37a20a 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -7,7 +7,6 @@
#include <common.h> #include <efi_loader.h> -#include <init.h> #include <malloc.h> #include <mapmem.h> #include <watchdog.h> @@ -731,7 +730,7 @@ efi_status_t efi_add_conventional_memory_map(u64 ram_start, u64 ram_end,
__weak void efi_add_known_memory(void) { - u64 ram_top = board_get_usable_ram_top(0) & ~EFI_PAGE_MASK; + u64 ram_top = gd->ram_top & ~EFI_PAGE_MASK; int i;
/*
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.
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); arch/arm/cpu/armv8/fsl-layerscape/spl.c:114: gd->ram_top = gd->bd->bi_dram[0].start + gd->bd->bi_dram[0].size;
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);
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.
Best regards
Heinrich
Signed-off-by: Patrick Delaunay patrick.delaunay@foss.st.com
https://source.denx.de/u-boot/custodians/u-boot-stm/-/pipelines/7399
lib/efi_loader/efi_memory.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index be2f655dff..f5bc37a20a 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -7,7 +7,6 @@
#include <common.h> #include <efi_loader.h> -#include <init.h> #include <malloc.h> #include <mapmem.h> #include <watchdog.h> @@ -731,7 +730,7 @@ efi_status_t efi_add_conventional_memory_map(u64 ram_start, u64 ram_end,
__weak void efi_add_known_memory(void) {
- u64 ram_top = board_get_usable_ram_top(0) & ~EFI_PAGE_MASK;
u64 ram_top = gd->ram_top & ~EFI_PAGE_MASK; int i;
/*
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
participants (3)
-
Heinrich Schuchardt -
Patrick DELAUNAY -
Patrick Delaunay