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

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

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

On 24.03.2013 18:01, Leif Lindholm wrote:

> 
> 0004-arm-uboot-support.patch
> 
> 
> === modified file 'Makefile.util.def'
> --- Makefile.util.def	2013-03-24 13:03:12 +0000
> +++ Makefile.util.def	2013-03-24 13:03:31 +0000
> @@ -467,6 +467,7 @@
>    enable = mips_loongson;
>    enable = ia64_efi;
>    enable = powerpc_ieee1275;
> +  enable = arm_uboot;
>  };
>  
>  script = {
> 
> === modified file 'grub-core/Makefile.am'
> --- grub-core/Makefile.am	2013-03-20 16:13:31 +0000
> +++ grub-core/Makefile.am	2013-03-24 13:03:31 +0000
> @@ -211,6 +211,13 @@
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/lib/arg.h
>  endif
>  
> +if COND_arm_uboot
> +KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/uboot/uboot.h
> +KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/uboot/disk.h
> +KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/extcmd.h
> +KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/lib/arg.h
> +endif
> +
>  if COND_emu
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/datetime.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/emu/misc.h
> 
> === modified file 'grub-core/Makefile.core.def'
> --- grub-core/Makefile.core.def	2013-03-24 13:03:12 +0000
> +++ grub-core/Makefile.core.def	2013-03-24 13:03:31 +0000
> @@ -79,6 +79,7 @@
>    mips_startup = kern/mips/startup.S;
>    sparc64_ieee1275_startup = kern/sparc64/ieee1275/crt0.S;
>    powerpc_ieee1275_startup = kern/powerpc/ieee1275/startup.S;
> +  arm_uboot_startup = kern/arm/uboot/startup.S;
>  
>    common = kern/command.c;
>    common = kern/corecmd.c;
> 
> === 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
> @@ -0,0 +1,176 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2013  Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/offsets.h>
> +#include <grub/symbol.h>
> +#include <grub/machine/kernel.h>
> +
> +/*
> + * GRUB is called from U-Boot as a Linux Kernel type image, which
> + * means among other things that it always enters in ARM state.
> + *
> + *
> + * Overview of GRUB image layout:
> + *
> + * _start:
> + *              Entry point (1 ARM branch instruction, to "codestart")
> + * grub_total_module_size:
> + *              Data field: Size of included module blob
> + *              (when generated by grub-mkimage)
> + * codestart:
> + *              Remainder of statically-linked executable code and data.
> + * __bss_start:
> + *              Start of included module blob.
> + *              Also where global/static variables are located.
> + * _end:
> + *              End of bss region (but not necessarily module blob).
> + * <overflow>:
> + *              Any part of the module blob that extends beyond _end.
> + * <modules>:
> + *              Loadable modules, post relocation.
> + * <stack>:     
> + * <heap>:
> + */
> +	
> +	.text
> +	.arm
> +FUNCTION(_start)
> +	b	codestart
> +	
> +	@ Size of final image integrated module blob - set by grub-mkimage
> +	. = _start + GRUB_KERNEL_MACHINE_TOTAL_MODULE_SIZE
> +VARIABLE(grub_total_module_size)
> +	.long 	0
> +
> +FUNCTION(codestart)
> +	@ Store context: Machine ID, atags/dtb, ...
> +	@ U-Boot API signature is stored on the U-Boot heap
> +	@ Stack pointer used as start address for signature probing
> +	mov	r12, sp
> +	ldr	sp, =entry_state
> +	push	{r4-r12,lr}	@ store U-Boot context (sp in r12)
> +
> +	@ Put kernel parameters aside until we can store them (further down)
> +	mov	r4, r1		@ machine type
> +	mov	r5, r2		@ boot data
> +
> +	@ Modules have been stored as a blob in BSS,
> +	@ they need to be manually relocated to _end or
> +	@ (__bss_start + grub_total_module_size), whichever greater.
> +	bl	uboot_get_real_bss_start	@ r0 = src
> +	ldr	r1, =EXT_C(_end)		@ dst = End of BSS
> +	ldr	r2, grub_total_module_size	@ blob size
> +	add	r3, r0, r2			@ blob end
> +	cmp	r1, r3				@ _end < blob end?
> +	movlt	r1, r3				@ dst = blob end + blob size
> +	
> +1:	ldr	r3, [r0], #4 			@ r3 = *src++ 
> +	str	r3, [r1], #4			@ *dst++ = r3 
> +	subs	r2, #4				@ remaining -= 4
> +	bne	1b				@ while remaining != 0
> +	
> +	@ 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)

> +	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.

> +	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.

> +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
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)




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