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

[Vladimir 'φ-coder/phcoder' Serbinenko]

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)

signature.asc
Description: OpenPGP digital signature