[PATCH] [ARM] Enable boot module for arm

[PATCH] [ARM] Enable boot module for arm

[Francesco Lavra]

Since the last merge of mainline into the arm branch, building for arm
fails because commands/boot.c is not compiled anymore. The boot module
should be enabled for arm too.

2013-07-21  Francesco Lavra  <francescolavra.fl@gmail.com>

	* grub-core/Makefile.core.def (boot): Enable on ARM.

=== modified file 'grub-core/Makefile.core.def'
--- grub-core/Makefile.core.def	2013-07-16 16:43:43 +0000
+++ grub-core/Makefile.core.def	2013-07-21 10:16:34 +0000
@@ -691,6 +691,7 @@
   common = commands/boot.c;
   i386_pc = lib/i386/pc/biosnum.c;
   enable = x86;
+  enable = arm;
   enable = emu;
   enable = sparc64_ieee1275;
   enable = powerpc_ieee1275;

Re: [PATCH] [ARM] Enable boot module for arm

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 21.07.2013 15:20, Francesco Lavra wrote:
> Since the last merge of mainline into the arm branch, building for arm
> fails because commands/boot.c is not compiled anymore. The boot module
> should be enabled for arm too.
>
Few lines down you have:
   enable = arm_efi;
   enable = arm_uboot;

Didn't it work for you? Did you redo autogen after checkout in the first 
place?
> 2013-07-21  Francesco Lavra  <francescolavra.fl@gmail.com>
>
> 	* grub-core/Makefile.core.def (boot): Enable on ARM.
>
> === modified file 'grub-core/Makefile.core.def'
> --- grub-core/Makefile.core.def	2013-07-16 16:43:43 +0000
> +++ grub-core/Makefile.core.def	2013-07-21 10:16:34 +0000
> @@ -691,6 +691,7 @@
>     common = commands/boot.c;
>     i386_pc = lib/i386/pc/biosnum.c;
>     enable = x86;
> +  enable = arm;
>     enable = emu;
>     enable = sparc64_ieee1275;
>     enable = powerpc_ieee1275;
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

Re: [PATCH] [ARM] Enable boot module for arm

[Francesco Lavra]

On 07/25/2013 04:36 PM, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> On 21.07.2013 15:20, Francesco Lavra wrote:
>> Since the last merge of mainline into the arm branch, building for arm
>> fails because commands/boot.c is not compiled anymore. The boot module
>> should be enabled for arm too.
>>
> Few lines down you have:
>   enable = arm_efi;
>   enable = arm_uboot;
> 
> Didn't it work for you? Did you redo autogen after checkout in the first
> place?

Actually this patch was against the arm branch, not trunk. Only later I
noticed that the arm branch had been merged back into trunk, so maybe
the arm branch is not going to be maintained anymore... in this case,
sorry for the noise.

Re: [PATCH] [ARM] Enable boot module for arm

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 25.07.2013 16:45, Francesco Lavra wrote:
> On 07/25/2013 04:36 PM, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>> On 21.07.2013 15:20, Francesco Lavra wrote:
>>> Since the last merge of mainline into the arm branch, building for arm
>>> fails because commands/boot.c is not compiled anymore. The boot module
>>> should be enabled for arm too.
>>>
>> Few lines down you have:
>>    enable = arm_efi;
>>    enable = arm_uboot;
>>
>> Didn't it work for you? Did you redo autogen after checkout in the first
>> place?
>
> Actually this patch was against the arm branch, not trunk. Only later I
> noticed that the arm branch had been merged back into trunk, so maybe
> the arm branch is not going to be maintained anymore... in this case,
> sorry for the noise.
>
ARM branch won't be maintained anymore. My main problem with arm 
currently is that testsuite works neither for arm-efi nor for arm-uboot 
and I didn't find a way to make qemu load file from iso from either 
u-boot or efi.

Re: [PATCH] [ARM] Enable boot module for arm

[Andrey Borzenkov]

В Thu, 25 Jul 2013 17:04:34 +0200
Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> пишет:

> >
> ARM branch won't be maintained anymore. My main problem with arm 
> currently is that testsuite works neither for arm-efi nor for arm-uboot 
> and I didn't find a way to make qemu load file from iso from either 
> u-boot or efi.


Could you show command line used to run grub under qemu? So far I was
unsuccessful to find working combination. Do you need special u-boot
build?

Re: [PATCH] [ARM] Enable boot module for arm

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 27.07.2013 05:46, Andrey Borzenkov wrote:
> В Thu, 25 Jul 2013 17:04:34 +0200
> Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> пишет:
>
>>>
>> ARM branch won't be maintained anymore. My main problem with arm
>> currently is that testsuite works neither for arm-efi nor for arm-uboot
>> and I didn't find a way to make qemu load file from iso from either
>> u-boot or efi.
>
>
> Could you show command line used to run grub under qemu? So far I was
> unsuccessful to find working combination. Do you need special u-boot
> build?
I have exactly the same problem. It's possible to support verstile/pb 
with armv5 but available u-boot tutorials for qemu ask you to 
concatenate payload with u-boot itself which is annoying to handle.
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

Re: [PATCH] [ARM] Enable boot module for arm

[Andrey Borzenkov]

В Sat, 27 Jul 2013 06:21:56 +0200
Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> пишет:

> On 27.07.2013 05:46, Andrey Borzenkov wrote:
> > В Thu, 25 Jul 2013 17:04:34 +0200
> > Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> пишет:
> >
> >>>
> >> ARM branch won't be maintained anymore. My main problem with arm
> >> currently is that testsuite works neither for arm-efi nor for arm-uboot
> >> and I didn't find a way to make qemu load file from iso from either
> >> u-boot or efi.
> >
> >
> > Could you show command line used to run grub under qemu? So far I was
> > unsuccessful to find working combination. Do you need special u-boot
> > build?
> I have exactly the same problem. It's possible to support verstile/pb 
> with armv5 but available u-boot tutorials for qemu ask you to 
> concatenate payload with u-boot itself which is annoying to handle.


It would be the first step, but my problem is, I cannot make it work
even using concatenation. u-boot does show me correct image info for
embedded grub, but I cannot launch it. Using default load address
0x08000000 qemu errors out because address is not in RAM (and it will
not let me change amount of RAM for versatilepb); I tried to change
load address to something else, like 0x04000000 and it just immediately
resets in this case after "bootm 0x04000000".

Re: [PATCH] [ARM] Enable boot module for arm

[Vladimir 'φ-coder/phcoder' Serbinenko]

> It would be the first step, but my problem is, I cannot make it work
> even using concatenation. u-boot does show me correct image info for
> embedded grub, but I cannot launch it. Using default load address
> 0x08000000 qemu errors out because address is not in RAM (and it will
> not let me change amount of RAM for versatilepb); I tried to change
> load address to something else, like 0x04000000 and it just immediately
> resets in this case after "bootm 0x04000000".
>
You probably have to change GRUB link address. And not that the address 
is where to find the source image not where to load to.

Re: [PATCH] [ARM] Enable boot module for arm

[Andrey Borzenkov]

В Sat, 27 Jul 2013 16:44:39 +0200
Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> пишет:

> > It would be the first step, but my problem is, I cannot make it work
> > even using concatenation. u-boot does show me correct image info for
> > embedded grub, but I cannot launch it. Using default load address
> > 0x08000000 qemu errors out because address is not in RAM (and it will
> > not let me change amount of RAM for versatilepb); I tried to change
> > load address to something else, like 0x04000000 and it just immediately
> > resets in this case after "bootm 0x04000000".
> >
> You probably have to change GRUB link address. And not that the address 
> is where to find the source image not where to load to.
> 

I did try to change link address (arm_uboot_ldflags and
GRUB_KERNEL_ARM_UBOOT_LINK_ADDR). As I understand, bootm is expected to
relocate image to load address and jump to start address.

What I have now is

1. U-Boot is configured for versatileqemu. It works in the sense I can
start example standalone hello_world using bootm.

2. Grub us build for arm-uboot

bor@opensuse:~/build/grub> ./configure --with-platform=uboot TARGET_CC=arm-linux-gnueabi-gcc --target=arm TARGET_LD=arm-linux-gnueabi-ld 

(I also tried with TARGET_CFLAGS="-marm -mno-thumb-interwork
-mabi=aapcs-linux -march=armv5te" to match U-Boot flags)

3. core.img is generated using 

bor@opensuse:~/build/grub> ./grub-mkimage --verbose -d grub-core -O arm-uboot -o core.img
./grub-mkimage: info: the total module size is 0x20.
./grub-mkimage: info: getting the size of grub-core/kernel.img.
./grub-mkimage: info: reading grub-core/kernel.img.
./grub-mkimage: info: getting the size of grub-core/kernel.img.
./grub-mkimage: info: locating the section .text at 0x0.
./grub-mkimage: info: locating the section .rodata at 0x14750.
./grub-mkimage: info: locating the section .data at 0x165a0.
./grub-mkimage: info: locating the section .module_license at 0x16fa8.
./grub-mkimage: info: locating the section .bss at 0x16fc8.
./grub-mkimage: info: kernel_img=0x144dda0, kernel_size=0x16fc8.
./grub-mkimage: info: the core size is 0x16fe8.
./grub-mkimage: info: writing 0x17028 bytes.
bor@opensuse:~/build/grub> file core.img
core.img: u-boot legacy uImage, , Linux/ARM, OS Kernel Image (Not compressed), 94184 bytes, Sun Jul 28 20:46:54 2013, Load Address: 0x08000000, Entry Point: 0x08000000, Header CRC: 0x5F6F0B3E, Data CRC: 0x474F38F8

4. "flash" is built using

bor@opensuse:~/src/u-boot-2013.04> cat u-boot.bin ~/build/grub/core.img > test.bin

Offset of core.img is

bor@opensuse:~/src/u-boot-2013.04> printf "0x%X\n" $(expr $(stat -c%s u-boot.bin) + 65536)
0x2B558

5. I now can start it

bor@opensuse:~/src/u-boot-2013.04> qemu-system-arm -M versatilepb -m 256M -serial stdio -kernel test.bin
Warning: default mac address being used, creating potential for address conflict
audio: Unknown audio driver `pa'
audio: Run with -audio-help to list available drivers


U-Boot 2013.04 (Jul 28 2013 - 20:16:17)

DRAM:  128 MiB
WARNING: Caches not enabled
Using default environment

In:    serial
Out:   serial
Err:   serial
Net:   SMC91111-0
Warning: SMC91111-0 using MAC address from net device

VersatilePB # 

And it shows correct image info

VersatilePB # iminfo 0x2B558

## Checking Image at 0002b558 ...
   Legacy image found
   Image Name:   
   Image Type:   ARM Linux Kernel Image (uncompressed)
   Data Size:    94184 Bytes = 92 KiB
   Load Address: 08000000
   Entry Point:  08000000
   Verifying Checksum ... OK
VersatilePB # 

But it simply resets at the attempt to launch it

VersatilePB # bootm 0x2B558
## Booting kernel from Legacy Image at 0002b558 ...
   Image Name:   
   Image Type:   ARM Linux Kernel Image (uncompressed)
   Data Size:    94184 Bytes = 92 KiB
   Load Address: 08000000
   Entry Point:  08000000
   Loading Kernel Image ... OK
OK

Starting kernel ...

resetting ...

Re: [PATCH] [ARM] Enable boot module for arm

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 28.07.2013 18:51, Andrey Borzenkov wrote:
>     Load Address: 08000000
>     Entry Point:  08000000
This obviously can't work if this address is unavailable. You need to 
change linking address.

Re: [PATCH] [ARM] Enable boot module for arm

[Andrey Borzenkov]

В Sun, 28 Jul 2013 20:01:29 +0200
Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> пишет:

> On 28.07.2013 18:51, Andrey Borzenkov wrote:
> >     Load Address: 08000000
> >     Entry Point:  08000000
> This obviously can't work if this address is unavailable. You need to 
> change linking address.
> 

If you know which value it needs to have, you could just tell ...

Anyway the problem was that GRUB uboot code depends on U-Boot providing
API, and as it turned out it is optional (and not default). After
rebuilding U-Boot with CONFIG_API I now get

VersatilePB # bootm 0x2F7CC
## Current stack ends at 0x07facae8 *  kernel: cmdline image address = 0x0002f7cc
## Booting kernel from Legacy Image at 0002f7cc ...
   Image Name:   
   Image Type:   ARM Linux Kernel Image (uncompressed)
   Data Size:    94184 Bytes = 92 KiB
   Load Address: 08000000
   Entry Point:  08000000
   kernel data at 0x0002f80c, len = 0x00016fe8 (94184)
## No init Ramdisk
   ramdisk start = 0x00000000, ramdisk end = 0x00000000
   Loading Kernel Image ... OK
OK
   kernel loaded at 0x08000000, end = 0x08016fe8
using: ATAGS
## Transferring control to Linux (at address 08000000)...

Starting kernel ...

Welcome to GRUB!

error: variable `root' isn't set.
Entering rescue mode...
grub rescue> 

Re: [PATCH] [ARM] Enable boot module for arm

[Andrey Borzenkov]

В Sun, 28 Jul 2013 22:18:41 +0400
Andrey Borzenkov <arvidjaar@gmail.com> пишет:

> В Sun, 28 Jul 2013 20:01:29 +0200
> Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> пишет:
> 
> > On 28.07.2013 18:51, Andrey Borzenkov wrote:
> > >     Load Address: 08000000
> > >     Entry Point:  08000000
> > This obviously can't work if this address is unavailable. You need to 
> > change linking address.
> > 
> 
> If you know which value it needs to have, you could just tell ...
> 
> Anyway the problem was that GRUB uboot code depends on U-Boot providing
> API, and as it turned out it is optional (and not default). After
> rebuilding U-Boot with CONFIG_API I now get
> 
> VersatilePB # bootm 0x2F7CC
> ## Current stack ends at 0x07facae8 *  kernel: cmdline image address = 0x0002f7cc
> ## Booting kernel from Legacy Image at 0002f7cc ...
>    Image Name:   
>    Image Type:   ARM Linux Kernel Image (uncompressed)
>    Data Size:    94184 Bytes = 92 KiB
>    Load Address: 08000000
>    Entry Point:  08000000
>    kernel data at 0x0002f80c, len = 0x00016fe8 (94184)
> ## No init Ramdisk
>    ramdisk start = 0x00000000, ramdisk end = 0x00000000
>    Loading Kernel Image ... OK
> OK
>    kernel loaded at 0x08000000, end = 0x08016fe8
> using: ATAGS
> ## Transferring control to Linux (at address 08000000)...
> 
> Starting kernel ...
> 
> Welcome to GRUB!
> 
> error: variable `root' isn't set.
> Entering rescue mode...
> grub rescue> 

Unfortunately attempt to do anything more useful ended up in

Starting kernel ...

Welcome to GRUB!

Unsupported ARM ID 0x41069265
Aborted. Press any key to exit.resetting ...

Which makes sense as we do not have any cache implementation for
ARMv5 ... sigh, all other platforms listed by QEMU I tried would not
even run U-Boot (at least, would not produce any output on serial
console).

Re: [PATCH] [ARM] Enable boot module for arm

[Andrey Borzenkov]

В Sat, 27 Jul 2013 06:21:56 +0200
Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> пишет:

> On 27.07.2013 05:46, Andrey Borzenkov wrote:
> > В Thu, 25 Jul 2013 17:04:34 +0200
> > Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> пишет:
> >
> >>>
> >> ARM branch won't be maintained anymore. My main problem with arm
> >> currently is that testsuite works neither for arm-efi nor for arm-uboot
> >> and I didn't find a way to make qemu load file from iso from either
> >> u-boot or efi.
> >
> >
> > Could you show command line used to run grub under qemu? So far I was
> > unsuccessful to find working combination. Do you need special u-boot
> > build?
> I have exactly the same problem. It's possible to support verstile/pb 
> with armv5 but available u-boot tutorials for qemu ask you to 
> concatenate payload with u-boot itself which is annoying to handle.

With current grub (that added support for transparent caches on armv5)
I can finally boot grub over PXE (core.img or rescue image; need to add
netboot support) using qemu versatilepb.

Now the problem is to do anything useful hard disk is needed and
default u-boot for versatileqemu does not seem to offer any support (or
may be it is qemu -M versatilepb). Were you successful in running arm
port under qemu?

Re: [PATCH] [ARM] Enable boot module for arm

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 15.11.2013 20:13, Andrey Borzenkov wrote:
> В Sat, 27 Jul 2013 06:21:56 +0200
> Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> пишет:
> 
>> On 27.07.2013 05:46, Andrey Borzenkov wrote:
>>> В Thu, 25 Jul 2013 17:04:34 +0200
>>> Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> пишет:
>>>
>>>>>
>>>> ARM branch won't be maintained anymore. My main problem with arm
>>>> currently is that testsuite works neither for arm-efi nor for arm-uboot
>>>> and I didn't find a way to make qemu load file from iso from either
>>>> u-boot or efi.
>>>
>>>
>>> Could you show command line used to run grub under qemu? So far I was
>>> unsuccessful to find working combination. Do you need special u-boot
>>> build?
>> I have exactly the same problem. It's possible to support verstile/pb 
>> with armv5 but available u-boot tutorials for qemu ask you to 
>> concatenate payload with u-boot itself which is annoying to handle.
> 
> With current grub (that added support for transparent caches on armv5)
> I can finally boot grub over PXE (core.img or rescue image; need to add
> netboot support) using qemu versatilepb.
> 
> Now the problem is to do anything useful hard disk is needed and
> default u-boot for versatileqemu does not seem to offer any support (or
> may be it is qemu -M versatilepb). Were you successful in running arm
> port under qemu?
> 
I didn't do netboot but concatenated with u-boot. It worked to similar
extent as what you describe. Another promising board is vexpress but I
had problems with MMC support in u-boot don't remember what it was exactly.
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



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

Re: [PATCH] [ARM] Enable boot module for arm

[Leif Lindholm]

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

On Sat, Nov 16, 2013 at 01:45:31AM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> > Now the problem is to do anything useful hard disk is needed and
> > default u-boot for versatileqemu does not seem to offer any support (or
> > may be it is qemu -M versatilepb). Were you successful in running arm
> > port under qemu?
> > 
> I didn't do netboot but concatenated with u-boot. It worked to similar
> extent as what you describe. Another promising board is vexpress but I
> had problems with MMC support in u-boot don't remember what it was exactly.

I have been looking at this today together with Ian Campbell at the
mini Debconf-UK. Commit c9cd02c cleans up the uboot syscall interface,
but unfortunately it also corrupts the stack for va_arg passing.
It is only evident when passing a sufficient number of arguments, which
I think is only triggered by the disk accesses.

Patch attached, verified on Ian's qemu vexpress, which he may be posting
some updates on later.

/
    Leif

[-- Attachment #2: 0001-arm-fix-u-boot-port-syscall-interface-va_arg-handlin.patch --]
[-- Type: text/x-diff, Size: 1931 bytes --]

From 7d317fc56bb7c5e54b85caa9f749075f4e5bfaf2 Mon Sep 17 00:00:00 2001
From: Leif Lindholm <leif.lindholm@linaro.org>
Date: Sat, 16 Nov 2013 12:15:53 +0000
Subject: [PATCH] arm: fix u-boot port syscall interface va_arg handling

Commit c9cd02c broke the u-boot syscall API for va_args that spill over
to the stack, causing the disk support to stop working. This patch
resolves the problem, while keeping the new, cleaner transition_space
handling.
---
 ChangeLog                          |  5 +++++
 grub-core/kern/arm/uboot/startup.S | 11 -----------
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 6d502f8..91c0534 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2013-11-16  Leif Lindholm <leif.lindholm@linaro.org>
+
+	* grub-core/kern/arm/uboot/startup.S: fix grub_uboot_syscall va_arg
+	handling
+
 2013-11-15  Vladimir Serbinenko  <phcoder@gmail.com>
 
 	Replace libgcc version of ctz with our own.
diff --git a/grub-core/kern/arm/uboot/startup.S b/grub-core/kern/arm/uboot/startup.S
index f54b14b..ce3415c 100644
--- a/grub-core/kern/arm/uboot/startup.S
+++ b/grub-core/kern/arm/uboot/startup.S
@@ -130,15 +130,8 @@ FUNCTION(codestart)
 FUNCTION(grub_uboot_syscall)
 	str     r8, transition_space
 	str     lr, transition_space + 4
-	str     r9, transition_space + 8
-	str     sp, transition_space + 12
-
-	sub     sp, sp, #0x20
-	lsr     sp, sp, #3
-	lsl     sp, sp, #3
 
 	ldr	r8, gd_backup
-	ldr	r9, gd_backup + 4
 
 	mov	lr, pc
 	ldr	pc, grub_uboot_syscall_ptr
@@ -146,8 +139,6 @@ FUNCTION(grub_uboot_syscall)
 
 	ldr     r8, transition_space
 	ldr     lr, transition_space + 4
-	ldr     r9, transition_space + 8
-	ldr     sp, transition_space + 12
 
 	bx	lr
 	
@@ -179,8 +170,6 @@ entry_state:		@ backup for U-Boot context
 transition_space:	
 	.long	0	@ r8
 	.long	0	@ lr
-	.long	0	@ r9
-	.long	0	@ sp
 
 VARIABLE(grub_uboot_syscall_ptr)
 	.long	0	@
-- 
1.8.4.rc3

[PATCH] arm: fix u-boot port syscall interface va_arg handling (was Re: [PATCH] [ARM] Enable boot module for arm)

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 16.11.2013 13:39, Leif Lindholm wrote:
> -	str     r9, transition_space + 8
You need to save r9. Otherwise GRUB will crash if compiled with clang.


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

Re: [PATCH] arm: fix u-boot port syscall interface va_arg handling (was Re: [PATCH] [ARM] Enable boot module for arm)

[Leif Lindholm]

On Sat, Nov 16, 2013 at 01:49:51PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> On 16.11.2013 13:39, Leif Lindholm wrote:
> > -	str     r9, transition_space + 8
> You need to save r9. Otherwise GRUB will crash if compiled with clang.

Hmm, but this is a bug in clang in that case.

From an ABI perspective, grub_uboot_syscall is a veneer:
the only visible side effect it is permitted to have is to corrupt r12.

We need to additionally switch r8 between grub and u-boot copy because
u-boot uses it for its global data pointer, and lr because we need to
corrupt it to make u-boot return to this veneer to switch the r8 back
again.

This veneer should not touch anything else - so if we need to
temporarily work around a toolchain ABI bug, can we do this via an
ifdef and a config option?

/
    Leif

Re: [PATCH] arm: fix u-boot port syscall interface va_arg handling (was Re: [PATCH] [ARM] Enable boot module for arm)

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 16.11.2013 14:00, Leif Lindholm wrote:
> On Sat, Nov 16, 2013 at 01:49:51PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>> On 16.11.2013 13:39, Leif Lindholm wrote:
>>> -	str     r9, transition_space + 8
>> You need to save r9. Otherwise GRUB will crash if compiled with clang.
> 
> Hmm, but this is a bug in clang in that case.
> 
> From an ABI perspective, grub_uboot_syscall is a veneer:
> the only visible side effect it is permitted to have is to corrupt r12.
> 
According to wikipedia:
"Subroutines must preserve the contents of r4 to r11 and the stack pointer."
So changing r9 sounds to me like this is actually U-Boot bug and
preserving it sounds like right way to handle it.
What's the harm in preserving r9? Is it used for something in calling?
> We need to additionally switch r8 between grub and u-boot copy because
> u-boot uses it for its global data pointer, and lr because we need to
> corrupt it to make u-boot return to this veneer to switch the r8 back
> again.
> 
> This veneer should not touch anything else - so if we need to
> temporarily work around a toolchain ABI bug, can we do this via an
> ifdef and a config option?
> 
If the bug is on clang side (I doubt it right now), there is no way to
handle it without side effects and we can invent a test for it, we can
reject buggy compilers (clang is secondary compiler)
> /
>     Leif
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



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

Re: [PATCH] arm: fix u-boot port syscall interface va_arg handling (was Re: [PATCH] [ARM] Enable boot module for arm)

[Andrey Borzenkov]

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

В Sat, 16 Nov 2013 14:17:51 +0100
Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> пишет:

> On 16.11.2013 14:00, Leif Lindholm wrote:
> > On Sat, Nov 16, 2013 at 01:49:51PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> >> On 16.11.2013 13:39, Leif Lindholm wrote:
> >>> -	str     r9, transition_space + 8
> >> You need to save r9. Otherwise GRUB will crash if compiled with clang.
> > 
> > Hmm, but this is a bug in clang in that case.
> > 
> > From an ABI perspective, grub_uboot_syscall is a veneer:
> > the only visible side effect it is permitted to have is to corrupt r12.
> > 
> According to wikipedia:
> "Subroutines must preserve the contents of r4 to r11 and the stack pointer."

According to ARM ABI documentation

"A subroutine must preserve the contents of the registers r4 - r8, r10,
r11 and SP (and r9 in PCS variants that designate r9 as v6)."

So it sounds like whether r9 is preserved depends on platform.

> So changing r9 sounds to me like this is actually U-Boot bug and
> preserving it sounds like right way to handle it.
> What's the harm in preserving r9? Is it used for something in calling?
> > We need to additionally switch r8 between grub and u-boot copy because
> > u-boot uses it for its global data pointer, and lr because we need to
> > corrupt it to make u-boot return to this veneer to switch the r8 back
> > again.
> > 
> > This veneer should not touch anything else - so if we need to
> > temporarily work around a toolchain ABI bug, can we do this via an
> > ifdef and a config option?
> > 
> If the bug is on clang side (I doubt it right now), there is no way to
> handle it without side effects and we can invent a test for it, we can
> reject buggy compilers (clang is secondary compiler)
> > /
> >     Leif
> > 
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> > 
> 
> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] arm: fix u-boot port syscall interface va_arg handling (was Re: [PATCH] [ARM] Enable boot module for arm)

[Leif Lindholm]

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

On Sat, Nov 16, 2013 at 02:17:51PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> > From an ABI perspective, grub_uboot_syscall is a veneer:
> > the only visible side effect it is permitted to have is to corrupt r12.
> > 
> According to wikipedia:
> "Subroutines must preserve the contents of r4 to r11 and the stack pointer."
> So changing r9 sounds to me like this is actually U-Boot bug and
> preserving it sounds like right way to handle it.

No. grub_uboot_syscall is not a subroutine - it is a veneer.
We have a specific reason to need to preserve r8, over and above what
the ABI says, because u-boot has hijacked it.

But now I went to look at the u-boot code, and suddenly I want to cry.
Commit fe1378a961e508b31b1f29a2bb08ba1dac063155 changes the register
reserved for global data from r8 to r9. Which means we need to preserve
both since they didn't step the API version number.

*sigh*

Updated patch attached.

/
    Leif

[-- Attachment #2: 0001-arm-fix-u-boot-port-syscall-interface-va_arg-handlin.patch --]
[-- Type: text/x-diff, Size: 2169 bytes --]

From d1926ea10b47442b4302e0d474cf18dbe65167de Mon Sep 17 00:00:00 2001
From: Leif Lindholm <leif.lindholm@linaro.org>
Date: Sat, 16 Nov 2013 12:15:53 +0000
Subject: [PATCH] arm: fix u-boot port syscall interface va_arg handling

Commit c9cd02c broke the u-boot syscall API for va_args that spill over
to the stack, causing the disk support to stop working. This patch
resolves the problem, while keeping the new, cleaner transition_space
handling.
---
 ChangeLog                          |  5 +++++
 grub-core/kern/arm/uboot/startup.S | 11 ++---------
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 6d502f8..91c0534 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2013-11-16  Leif Lindholm <leif.lindholm@linaro.org>
+
+	* grub-core/kern/arm/uboot/startup.S: fix grub_uboot_syscall va_arg
+	handling
+
 2013-11-15  Vladimir Serbinenko  <phcoder@gmail.com>
 
 	Replace libgcc version of ctz with our own.
diff --git a/grub-core/kern/arm/uboot/startup.S b/grub-core/kern/arm/uboot/startup.S
index f54b14b..df1e329 100644
--- a/grub-core/kern/arm/uboot/startup.S
+++ b/grub-core/kern/arm/uboot/startup.S
@@ -131,11 +131,6 @@ FUNCTION(grub_uboot_syscall)
 	str     r8, transition_space
 	str     lr, transition_space + 4
 	str     r9, transition_space + 8
-	str     sp, transition_space + 12
-
-	sub     sp, sp, #0x20
-	lsr     sp, sp, #3
-	lsl     sp, sp, #3
 
 	ldr	r8, gd_backup
 	ldr	r9, gd_backup + 4
@@ -147,7 +142,6 @@ FUNCTION(grub_uboot_syscall)
 	ldr     r8, transition_space
 	ldr     lr, transition_space + 4
 	ldr     r9, transition_space + 8
-	ldr     sp, transition_space + 12
 
 	bx	lr
 	
@@ -166,8 +160,8 @@ entry_state_end:
 	.long	0	@ r6
 	.long	0	@ r7
 gd_backup:	
-	.long	0	@ r8 - U-Boot global data pointer
-	.long	0	@ r9
+	.long	0	@ r8 - U-Boot global data pointer up to 2013-09-21
+	.long	0	@ r9 - U-Boot global data pointer 2013-09-21 onwards
 	.long	0	@ r10
 	.long	0	@ r11
 VARIABLE(grub_uboot_search_hint)@ U-Boot stack pointer - 
@@ -180,7 +174,6 @@ transition_space:
 	.long	0	@ r8
 	.long	0	@ lr
 	.long	0	@ r9
-	.long	0	@ sp
 
 VARIABLE(grub_uboot_syscall_ptr)
 	.long	0	@
-- 
1.8.4.rc3

Re: [PATCH] arm: fix u-boot port syscall interface va_arg handling (was Re: [PATCH] [ARM] Enable boot module for arm)

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 16.11.2013 14:53, Leif Lindholm wrote:
> On Sat, Nov 16, 2013 at 02:17:51PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>>> From an ABI perspective, grub_uboot_syscall is a veneer:
>>> the only visible side effect it is permitted to have is to corrupt r12.
>>>
>> According to wikipedia:
>> "Subroutines must preserve the contents of r4 to r11 and the stack pointer."
>> So changing r9 sounds to me like this is actually U-Boot bug and
>> preserving it sounds like right way to handle it.
> 
> No. grub_uboot_syscall is not a subroutine - it is a veneer.
> We have a specific reason to need to preserve r8, over and above what
> the ABI says, because u-boot has hijacked it.
> 
> But now I went to look at the u-boot code, and suddenly I want to cry.
> Commit fe1378a961e508b31b1f29a2bb08ba1dac063155 changes the register
> reserved for global data from r8 to r9. Which means we need to preserve
> both since they didn't step the API version number.
> 
> *sigh*
> 
> Updated patch attached.

Go ahead.
> 
> /
>     Leif
> 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



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

Re: [PATCH] arm: fix u-boot port syscall interface va_arg handling (was Re: [PATCH] [ARM] Enable boot module for arm)

[Leif Lindholm]

On Sat, Nov 16, 2013 at 03:03:16PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> > Updated patch attached.
> 
> Go ahead.

Argh...

I pushed the above, and almost immediately I realised that in this
situation we also need to store u-boot's version of r9 on return:

diff --git a/grub-core/kern/arm/uboot/startup.S b/grub-core/kern/arm/uboot/start
index df1e329..26313ba 100644
--- a/grub-core/kern/arm/uboot/startup.S
+++ b/grub-core/kern/arm/uboot/startup.S
@@ -138,6 +138,7 @@ FUNCTION(grub_uboot_syscall)
        mov     lr, pc
        ldr     pc, grub_uboot_syscall_ptr
        str     r8, gd_backup
+       str     r9, gd_backup + 4
 
        ldr     r8, transition_space
        ldr     lr, transition_space + 4

Can I push this fix too?

/
    Leif

Re: [PATCH] arm: fix u-boot port syscall interface va_arg handling (was Re: [PATCH] [ARM] Enable boot module for arm)

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 16.11.2013 15:28, Leif Lindholm wrote:
> Argh...
> 
> I pushed the above, and almost immediately I realised that in this
> situation we also need to store u-boot's version of r9 on return:
> 
> diff --git a/grub-core/kern/arm/uboot/startup.S b/grub-core/kern/arm/uboot/start
> index df1e329..26313ba 100644
> --- a/grub-core/kern/arm/uboot/startup.S
> +++ b/grub-core/kern/arm/uboot/startup.S
> @@ -138,6 +138,7 @@ FUNCTION(grub_uboot_syscall)
>         mov     lr, pc
>         ldr     pc, grub_uboot_syscall_ptr
>         str     r8, gd_backup
> +       str     r9, gd_backup + 4
>  
>         ldr     r8, transition_space
>         ldr     lr, transition_space + 4
> 
> Can I push this fix too?
> 
But why does global pointer change at all?
What about exit function?
> /
>     Leif



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

Re: [PATCH] arm: fix u-boot port syscall interface va_arg handling (was Re: [PATCH] [ARM] Enable boot module for arm)

[Leif Lindholm]

On Sat, Nov 16, 2013 at 03:28:35PM +0100, Leif Lindholm wrote:
> I pushed the above, and almost immediately I realised that in this
> situation we also need to store u-boot's version of r9 on return:
> 
> diff --git a/grub-core/kern/arm/uboot/startup.S b/grub-core/kern/arm/uboot/start
> index df1e329..26313ba 100644
> --- a/grub-core/kern/arm/uboot/startup.S
> +++ b/grub-core/kern/arm/uboot/startup.S
> @@ -138,6 +138,7 @@ FUNCTION(grub_uboot_syscall)
>         mov     lr, pc
>         ldr     pc, grub_uboot_syscall_ptr
>         str     r8, gd_backup
> +       str     r9, gd_backup + 4
>  
>         ldr     r8, transition_space
>         ldr     lr, transition_space + 4
> 
> Can I push this fix too?

Umm, at second thought - the gd pointer(s) wouldn't be changing.
So the correct fix would rather be to 
-         str     r8, gd_backup
?

/
    Leif

Re: [PATCH] arm: fix u-boot port syscall interface va_arg handling (was Re: [PATCH] [ARM] Enable boot module for arm)

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 16.11.2013 15:45, Leif Lindholm wrote:
> On Sat, Nov 16, 2013 at 03:28:35PM +0100, Leif Lindholm wrote:
>> I pushed the above, and almost immediately I realised that in this
>> situation we also need to store u-boot's version of r9 on return:
>>
>> diff --git a/grub-core/kern/arm/uboot/startup.S b/grub-core/kern/arm/uboot/start
>> index df1e329..26313ba 100644
>> --- a/grub-core/kern/arm/uboot/startup.S
>> +++ b/grub-core/kern/arm/uboot/startup.S
>> @@ -138,6 +138,7 @@ FUNCTION(grub_uboot_syscall)
>>         mov     lr, pc
>>         ldr     pc, grub_uboot_syscall_ptr
>>         str     r8, gd_backup
>> +       str     r9, gd_backup + 4
>>  
>>         ldr     r8, transition_space
>>         ldr     lr, transition_space + 4
>>
>> Can I push this fix too?
> 
> Umm, at second thought - the gd pointer(s) wouldn't be changing.
> So the correct fix would rather be to 
> -         str     r8, gd_backup
> ?
> 
Was this tested?
> /
>     Leif
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



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

Re: [PATCH] arm: fix u-boot port syscall interface va_arg handling (was Re: [PATCH] [ARM] Enable boot module for arm)

[Leif Lindholm]

On Sat, Nov 16, 2013 at 04:19:48PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> >> @@ -138,6 +138,7 @@ FUNCTION(grub_uboot_syscall)
> >>         mov     lr, pc
> >>         ldr     pc, grub_uboot_syscall_ptr
> >>         str     r8, gd_backup
> >> +       str     r9, gd_backup + 4
> >>  
> >>         ldr     r8, transition_space
> >>         ldr     lr, transition_space + 4
> >>
> >> Can I push this fix too?
> > 
> > Umm, at second thought - the gd pointer(s) wouldn't be changing.
> > So the correct fix would rather be to 
> > -         str     r8, gd_backup
> > ?
> > 
> Was this tested?

Tested on Ian's qemu, with an "mmc" block device..

/
    Leif

Re: [PATCH] arm: fix u-boot port syscall interface va_arg handling (was Re: [PATCH] [ARM] Enable boot module for arm)

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 16.11.2013 16:23, Leif Lindholm wrote:
> On Sat, Nov 16, 2013 at 04:19:48PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>>>> @@ -138,6 +138,7 @@ FUNCTION(grub_uboot_syscall)
>>>>         mov     lr, pc
>>>>         ldr     pc, grub_uboot_syscall_ptr
>>>>         str     r8, gd_backup
>>>> +       str     r9, gd_backup + 4
>>>>  
>>>>         ldr     r8, transition_space
>>>>         ldr     lr, transition_space + 4
>>>>
>>>> Can I push this fix too?
>>>
>>> Umm, at second thought - the gd pointer(s) wouldn't be changing.
>>> So the correct fix would rather be to 
>>> -         str     r8, gd_backup
>>> ?
>>>
>> Was this tested?
> 
> Tested on Ian's qemu, with an "mmc" block device..
> 
Go ahead.
> /
>     Leif
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 



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

Re: [PATCH] [ARM] Enable boot module for arm

[Andrey Borzenkov]

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

В Sat, 16 Nov 2013 01:45:31 +0100
Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> пишет:

> On 15.11.2013 20:13, Andrey Borzenkov wrote:
> > В Sat, 27 Jul 2013 06:21:56 +0200
> > Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> пишет:
> > 
> >> On 27.07.2013 05:46, Andrey Borzenkov wrote:
> >>> В Thu, 25 Jul 2013 17:04:34 +0200
> >>> Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> пишет:
> >>>
> >>>>>
> >>>> ARM branch won't be maintained anymore. My main problem with arm
> >>>> currently is that testsuite works neither for arm-efi nor for arm-uboot
> >>>> and I didn't find a way to make qemu load file from iso from either
> >>>> u-boot or efi.
> >>>
> >>>
> >>> Could you show command line used to run grub under qemu? So far I was
> >>> unsuccessful to find working combination. Do you need special u-boot
> >>> build?
> >> I have exactly the same problem. It's possible to support verstile/pb 
> >> with armv5 but available u-boot tutorials for qemu ask you to 
> >> concatenate payload with u-boot itself which is annoying to handle.
> > 
> > With current grub (that added support for transparent caches on armv5)
> > I can finally boot grub over PXE (core.img or rescue image; need to add
> > netboot support) using qemu versatilepb.
> > 
> > Now the problem is to do anything useful hard disk is needed and
> > default u-boot for versatileqemu does not seem to offer any support (or
> > may be it is qemu -M versatilepb). Were you successful in running arm
> > port under qemu?
> > 
> I didn't do netboot but concatenated with u-boot. It worked to similar
> extent as what you describe. Another promising board is vexpress but I
> had problems with MMC support in u-boot don't remember what it was exactly.


Do you mean physical board? The only qemu machine for which I was able
to build working u-boot was versatilepb.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]