Re: [PATCH 4/7] Support for ARM/U-Boot platforms

[Leif Lindholm]

On Tue, Apr 09, 2013 at 02:15:20AM +0200, Vladimir '??-coder/phcoder' 
Serbinenko wrote:
> > === added directory 'grub-core/kern/arm/uboot'
> > === added file 'grub-core/kern/arm/uboot/startup.S'
> > --- grub-core/kern/arm/uboot/startup.S      1970-01-01 00:00:00 +0000
> > +++ grub-core/kern/arm/uboot/startup.S      2013-03-24 13:03:31 +0000
[...]
> > +   @ Set up a new stack, beyond the end of copied modules.
> > +   ldr     r3, =GRUB_KERNEL_MACHINE_STACK_SIZE
> > +   add     r3, r1, r3      @ Place stack beyond end of modules
> > +   and     sp, r3, #~0x7   @ Ensure 8-byte alignment
> > +
> > +   @ Since we _are_ the C run-time, we need to manually zero the BSS
> > +   @ region before continuing
> > +   bl      uboot_get_real_bss_start        @ zero from here
> 
> This start is wrong. Even if modules start later due to alignment, BSS
> starts at __bss_start. Additional problem is that both __bss_start and
> _end may be unaligned unless special care is taken (like putting a dummy
> uint32_t at the beginning of BSS by putting it at the end of startup.S
> or using ld options)
 
My main concern here is getting the module start address right.
But see below.

> > +   ldr     r1, =EXT_C(_end)                @ to here
> > +   mov     r2, #0
> > +1: str     r2, [r0], #4
> > +   cmp     r0, r1
> > +   bne     1b
> > +
> > +   @ Global variables now accessible - store kernel parameters in memory
> > +   ldr     r12, =EXT_C(uboot_machine_type)
> > +   str     r4, [r12]
> > +   ldr     r12, =EXT_C(uboot_boot_data)
> > +   str     r5, [r12]
> > +   
> 
> Instead of temporary stashing the values to registers you can just init
> those values in C code to e.g. 0x55aa55aa so they'll be in .data and so
> accessible from the very beginning.

Neat :)
I'll do that.
 
> > +   b       EXT_C(grub_main)
> > +
> > +   /*
> > +    * __bss_start does not actually point to the start of the runtime
> > +    * BSS, but rather to the next byte following the preceding data.
> > +    */
> 
> Only modules are aligned. BSS itself is still at _bss.
 
My issue with this statement is that this definition of BSS can
include padding before the first symbol inside the BSS.
I accept that it can also contain less-aligned symbols, which is a
problem that I need to handle in the code above.

> > +FUNCTION (uboot_get_real_bss_start)
> > +   ldr     r0, =EXT_C(__bss_start)         @ src
> > +   tst     r0, #(GRUB_KERNEL_MACHINE_MOD_ALIGN - 1)
> > +   beq     1f
> > +   mvn     r1, #(GRUB_KERNEL_MACHINE_MOD_ALIGN - 1)
> > +   and     r0, r0, r1
> > +   add     r0, r0, #(GRUB_KERNEL_MACHINE_MOD_ALIGN)
> 
> Can be trivially simplified to:
> mvn   r1, #(GRUB_KERNEL_MACHINE_MOD_ALIGN - 1)
> add   r0, r0, r1
> and   r0, r0, r1

Yes, I may have been a bit silly when writing the above (and now), but
I don't follow (0xfffffff8 + __bss_start) & 0xfffffff8 ?
Do you mean:
        mvn   r1, #(GRUB_KERNEL_MACHINE_MOD_ALIGN)
        add   r0, r0, #(GRUB_KERNEL_MACHINE_MOD_ALIGN - 1)
        and   r0, r0, r1

> which can be then inlined. Also it's more reliable to save grub_modbase
> as soon as we computed it in asm part (and use 0x55aa55aa trick to make
> it accessible early)
 
OK, that makes sense. And having done that, it can be inlined, since I no
longer need it from within uboot/init.c.

/
    Leif