Hi Albert, On 17 November 2015 at 05:59, Albert ARIBAUD <albert.u.b...@aribaud.net> wrote: > Hello Simon, > > On Mon, 16 Nov 2015 21:11:38 -0700, Simon Glass <s...@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_size >> > + sub sp, sp, r0 >> > + mov r9, sp >> >> Why 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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot