On 09.04.2013 13:39, Leif Lindholm wrote: > 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. >
Actually since BSS surely contains at least one uint32_t its start and has to be aligned to 32-bit. So we can just align_up __bss_start to 4 and _end align_down to 4. This would work correctly enough even with the presence of the bug you rerported to GCC folks. >>> +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 > Yes, sorry, I proposed it without testing and I'm a beginner in ARM asm. >> 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 >
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
