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

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3672 bytes --]

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
> 




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]