On 22/09/2020 02:12, Samuel Holland wrote:
Hi,
On 9/21/20 7:41 PM, André Przywara wrote:
On 03/09/2020 06:07, Samuel Holland wrote:
This overwrites the name loaded from the SPL image. It will be different if there was previously no name provided, or if a more accurate name was determined by the board variant selection logic. This means that the DT> name in the SPL header now always matches the DT appended to U-Boot.
That's a nice way of preserving all this fancy DT selection choices for U-Boot proper!
Signed-off-by: Samuel Holland samuel@sholland.org
One hint below, but nevertheless:
Reviewed-by: Andre Przywara andre.przywara@arm.com
board/sunxi/board.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 3d64ed18664..eaa40a1ea96 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -331,6 +331,21 @@ static const char *get_spl_dt_name(void) return NULL; }
+static void set_spl_dt_name(const char *name) +{
- struct boot_file_head *spl = get_spl_header(SPL_ENV_HEADER_VERSION);
- if (spl == INVALID_SPL_HEADER)
return;- /* Promote the header version for U-Boot proper, if needed. */
- if (spl->spl_signature[3] < SPL_DT_HEADER_VERSION)
spl->spl_signature[3] = SPL_DT_HEADER_VERSION;- strcpy((char *)&spl->string_pool, name);
Let's hope nobody ever optimises the strcpy() routine, as this might break (when doing unaligned accesses) on device memory, as in this case.
Why would any part of SRAM A1 be mapped as device memory?
Because we do so in arch/arm/mach-sunxi/board.c: static struct mm_region sunxi_mem_map[] = { { /* SRAM, MMIO regions */ .virt = 0x0UL, .phys = 0x0UL, .size = 0x40000000UL, .attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) | PTE_BLOCK_NON_SHARE }, {
If U-Boot maps its own code as device memory,
DRAM is mapped as normal cacheable memory (the line after those above). So U-Boot's code and its own data are safe.
and then implements a strcpy that requires aligned multi-byte access (i.e. cannot handle arbitrary naturally-aligned char *), then this code here isn't the bug.
The problem is that letting the compiler generate code that does access non-normal memory is broken. Device memory needs to be accessed via MMIO accessors only, everything else is architecturally wrong. We are somewhat saved by the fact that SRAM, by its very nature, implements memory semantics, so it supports the GRE semantics that the compiler relies on. But unaligned accesses trap on device memory nevertheless. As I said, it probably works at the moment, under the current conditions, but is fragile.
Cheers, Andre
- spl->dt_name_offset = offsetof(struct boot_file_head, string_pool);
+}
int dram_init(void) { struct boot_file_head *spl = get_spl_header(SPL_DRAM_HEADER_VERSION); @@ -904,6 +919,7 @@ int ft_board_setup(void *blob, struct bd_info *bd) int board_fit_config_name_match(const char *name) { const char *best_dt_name = get_spl_dt_name();
- int ret;
#ifdef CONFIG_DEFAULT_DEVICE_TREE if (best_dt_name == NULL) @@ -941,6 +957,15 @@ int board_fit_config_name_match(const char *name) } #endif
- return strcmp(name, best_dt_name);
- ret = strcmp(name, best_dt_name);
- /*
* If one of the FIT configurations matches the most accurate DT name,* update the SPL header to provide that DT name to U-Boot proper.*/- if (ret == 0)
set_spl_dt_name(best_dt_name);- return ret;
} #endif