Hi Albert,
On 17 November 2015 at 05:59, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Hello Simon,
On Mon, 16 Nov 2015 21:11:38 -0700, Simon Glass sjg@chromium.org wrote:
+++ b/arch/arm/lib/crt0.S @@ -82,9 +82,11 @@ ENTRY(_main) #else bic sp, sp, #7 /* 8-byte alignment for ABI compliance */ #endif
bl board_init_f_get_reserve_sizesub sp, sp, r0mov r9, spWhy do you need that?
To set gd in ARM architecture, as arch_setup_gd() may not work for ARM, see my 'Note about the ARM case' in my answer dated Sun, 15 Nov 2015 14:51:04 +0100 for details:
https://www.mail-archive.com/u-boot@lists.denx.de/msg192494.html
OK I understand that now.
I shall post a 2-patch v6 with this fix standalone as 1/2 and the rest of the changes as 2/2.
+++ b/common/init/board_init.c @@ -21,39 +21,121 @@ DECLARE_GLOBAL_DATA_PTR; #define _USE_MEMCPY #endif
-/* Unfortunately x86 can't compile this code as gd cannot be assigned */ -#ifndef CONFIG_X86 +/* Unfortunately x86 or ARM can't compile this code as gd cannot be assigned */ +#if !defined(CONFIG_X86) && !defined(CONFIG_ARM) __weak void arch_setup_gd(struct global_data *gd_ptr) { gd = gd_ptr; } -#endif /* !CONFIG_X86 */ +#endif /* !CONFIG_X86 && !CONFIG_SYS_THUMB_BUILD */
-ulong board_init_f_mem(ulong top) +/*
- Compute size of space to reserve on stack for use as 'globals'
- and return size request for reserved area.
- Notes:
- Actual reservation cannot be done from within this function as
- it requires altering the C stack pointer, so this will be done by
- the caller upon return from this function.
- IMPORTANT:
- The return value of this function has two uses:
- it determines the amount of stack reserved for global data and
- the malloc arena;
early malloc() area
"Arena", and "malloc arena", are established designations for the memory space used by malloc implementations; and I prefer to use this more specific term, as people may use it as a search keyword when looking for malloc related stuff.
Arena is OK, but can you please mention 'early' each time? It's confusing otherwise. I think we should have a clear distinction between the early malloc() and full malloc().
Another option would be to pass the address and have this function return the new address. That would simplify the assembly code slightly for all archs I think. It would also allow you to align the address for sure, whereas at present it only works if the original address was aligned, and CONFIG_SYS_MALLOC_F_LEN is aligned.
Good point, with a few caveats though.
Regarding alignment of CONFIG_SYS_MALLOC_F_LEN:
Indeed, no attempt is made to check that it is aligned (and no attempt is made in the original code either) -- all the more since there is no strict definition of what it should be aligned to. There is, actually, no indication that it should be aligned, although it will probably only be used up until the last full 4-byte-word (or 8-byte word for 64-bit architectures). In fact, the alignment of CONFIG_SYS_MALLOC_F_LEN does not matter much, see (*) below.
Regarding alignment of the original/'top' address:
This top address is the SP for all architectures, and at least for ARM, it is aligned to an 8-byte boundary, as this is an ARM architecture EABI requirement. For other architectures, I cannot claim it is, but I suspect all initial values of SP, which generally are CONFIG_SPL_STACK or CONFIG_SYS_INIT_SP_ADDR, are (sufficiently) aligned.
And if CONFIG_SPL_STACK and CONFIG_SYS_INIT_SP_ADDR are not (sufficiently) aligned in their definition, then either we can fix these definitions (and that of CONFIG_SYS_MALLOC_F_LEN too, while we are at it), or, if we can only fix this at run time, then this should be done where the stack pointer is altered, in the crt0.S file or whatever its equivalent is for any given architecture, outside the C environment.
But that will have to go in another patch dealing with alignment.
If you can have it so that the stack top equals the global_data bottom, then we should be OK.
(*)
Indeed board_init_f_get_reserve_size may have to respect some location or size alignment for each of the chunks it reserves. Right now, for instance, GD is aligned to a 16-byte boundary, and the malloc arena is aligned by virtue of the rounded value of CONFIG_SYS_MALLOC_F_LEN.
And yes, should some new reservation be made beside GD and the malloc arena, it might require some specific alignment not dealt with so far; for instance, we may need to reserve some 4KB-aligned chunk for memory management purposes, or whatever, and there is no guarantee that the original stack is 4KB-aligned.
In that light, ulong board_init_f_get_reserve_size(void) does not permit controlled alignment, whereas ulong board_init_f_reserve(ulong top) does, since we can round 'top' down to any value we like.
Note, however, that it will not simplify assembly code: it will turn a subtraction from sp into an assignment to sp, which is not simpler, and it will add an assignment to whatever register represents the first argument, since we'll turn a (void) function into a (ulong top) function, so all in all, it will add one assembly instruction with respect to the 'ulong board_init_f_get_reserve_size(void)' approach.
True, but now we are just passing values around rather than doing arithmetic in assembler.
/* set GD unless architecture did it already */+#if !defined(CONFIG_X86) && !defined(CONFIG_ARM) arch_setup_gd(gd_ptr);
Why does this not work for ARM?
See above, but note that whatever the architecture, gd will be useable after this call, so :
+#endif
/* next alloc will be higher by one GD plus 16-byte alignment */base += roundup(sizeof(struct global_data), 16);/** record malloc arena start*/#if defined(CONFIG_SYS_MALLOC_F)
top -= CONFIG_SYS_MALLOC_F_LEN;gd->malloc_base = top;
/* go down one malloc arena */gd->malloc_base = base;gd_ptr?
gd works at this point, and I want to avoid any aliasing issue.
I don't really understand that, but if you want to use gd I think it would be worth a one-line comment.
Regards, Simon
Thanks for your comments! I'll turn
ulong board_init_f_get_reserve_size(void)into
ulong board_init_f_get_reserve(ulong top)So that arbitrary alignments will be possible, and I will move the r9 fix into its own patch.
OK sounds good.
Amicalement,
Albert.