[U-Boot] arm: U-Boot API - clang support broke ABI
Hi,
Commit fe1378a - "ARM: use r9 for gd" - broke the ABI for users of the U-Boot API on ARM. Users I am aware of are GRUB and the FreeBSD loader. Since I only spotted this on Saturday, this code is well and truly in the wild, so any users of the API will need to preserve both r8 and r9 on syscalls for the foreseeable future (which is not the end of the world).
Sometime last year I proposed a change to make the API syscall entry and exit points responsible for maintaining full procedure-call standard compliance, although the patch I provided was on the hackier side of good (and probably wrong).
I would now like to ask in a more open-ended way - is there any chance that we could solve this in a generic way, such that consumers of the API need not be aware of U-Boot internal ABI changes?
If the assumtions that the global data pointer is: a) a pointer b) writable are not both safe, then this could be achieved by arch-specific asm entry/exit points.
/ Leif
Hi Albert,
I have just bisect why on the latest kernel hello world example doesn't work and I have end up in this patch too. After reverting hello world example is working.
Also I would like to know if we have still bootm support for standlone http://www.denx.de/wiki/view/DULG/UBootStandalone#Section_5.12.3. Because it is not working for me with error "ERROR: booting os 'U-Boot' (17) is not supported" and I can't see IH_OS_U_BOOT in boot_os table in cmd_bootm.c
I have created image like this. ./tools/mkimage -n "hello" -A arm -O u-boot -T standalone -C none -a c100000 -d examples/standalone/hello_world.bin -v /tftpboot/hello.ub
Thanks, Michal
On 11/18/2013 12:26 PM, Leif Lindholm wrote:
Hi,
Commit fe1378a - "ARM: use r9 for gd" - broke the ABI for users of the U-Boot API on ARM. Users I am aware of are GRUB and the FreeBSD loader. Since I only spotted this on Saturday, this code is well and truly in the wild, so any users of the API will need to preserve both r8 and r9 on syscalls for the foreseeable future (which is not the end of the world).
Sometime last year I proposed a change to make the API syscall entry and exit points responsible for maintaining full procedure-call standard compliance, although the patch I provided was on the hackier side of good (and probably wrong).
I would now like to ask in a more open-ended way - is there any chance that we could solve this in a generic way, such that consumers of the API need not be aware of U-Boot internal ABI changes?
If the assumtions that the global data pointer is: a) a pointer b) writable are not both safe, then this could be achieved by arch-specific asm entry/exit points.
/ Leif _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
On Tue, Nov 19, 2013 at 10:09:44AM +0100, Michal Simek wrote:
Hi Albert,
I have just bisect why on the latest kernel hello world example doesn't work and I have end up in this patch too. After reverting hello world example is working.
Also I would like to know if we have still bootm support for standlone http://www.denx.de/wiki/view/DULG/UBootStandalone#Section_5.12.3. Because it is not working for me with error "ERROR: booting os 'U-Boot' (17) is not supported" and I can't see IH_OS_U_BOOT in boot_os table in cmd_bootm.c
I have created image like this. ./tools/mkimage -n "hello" -A arm -O u-boot -T standalone -C none -a c100000 -d examples/standalone/hello_world.bin -v /tftpboot/hello.ub
Let's get Albert on CC here, since, yeah, this is a problem...
On 11/18/2013 12:26 PM, Leif Lindholm wrote:
Hi,
Commit fe1378a - "ARM: use r9 for gd" - broke the ABI for users of the U-Boot API on ARM. Users I am aware of are GRUB and the FreeBSD loader. Since I only spotted this on Saturday, this code is well and truly in the wild, so any users of the API will need to preserve both r8 and r9 on syscalls for the foreseeable future (which is not the end of the world).
Sometime last year I proposed a change to make the API syscall entry and exit points responsible for maintaining full procedure-call standard compliance, although the patch I provided was on the hackier side of good (and probably wrong).
I would now like to ask in a more open-ended way - is there any chance that we could solve this in a generic way, such that consumers of the API need not be aware of U-Boot internal ABI changes?
If the assumtions that the global data pointer is: a) a pointer b) writable are not both safe, then this could be achieved by arch-specific asm entry/exit points.
/ Leif _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
-- Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91 w: www.monstr.eu p: +42-0-721842854 Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/ Maintainer of Linux kernel - Xilinx Zynq ARM architecture Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
On 11/19/2013 09:07 PM, Tom Rini wrote:
On Tue, Nov 19, 2013 at 10:09:44AM +0100, Michal Simek wrote:
Hi Albert,
I have just bisect why on the latest kernel hello world example doesn't work and I have end up in this patch too. After reverting hello world example is working.
Also I would like to know if we have still bootm support for standlone http://www.denx.de/wiki/view/DULG/UBootStandalone#Section_5.12.3. Because it is not working for me with error "ERROR: booting os 'U-Boot' (17) is not supported" and I can't see IH_OS_U_BOOT in boot_os table in cmd_bootm.c
I have created image like this. ./tools/mkimage -n "hello" -A arm -O u-boot -T standalone -C none -a c100000 -d examples/standalone/hello_world.bin -v /tftpboot/hello.ub
Let's get Albert on CC here, since, yeah, this is a problem...
This is not just arm specific problem. I see the same problem on Microblaze too. Was the support removed or just not tested and something broke it?
Thanks, Michal
On Wed, Nov 20, 2013 at 08:59:43AM +0100, Michal Simek wrote:
On 11/19/2013 09:07 PM, Tom Rini wrote:
On Tue, Nov 19, 2013 at 10:09:44AM +0100, Michal Simek wrote:
Hi Albert,
I have just bisect why on the latest kernel hello world example doesn't work and I have end up in this patch too. After reverting hello world example is working.
Also I would like to know if we have still bootm support for standlone http://www.denx.de/wiki/view/DULG/UBootStandalone#Section_5.12.3. Because it is not working for me with error "ERROR: booting os 'U-Boot' (17) is not supported" and I can't see IH_OS_U_BOOT in boot_os table in cmd_bootm.c
I have created image like this. ./tools/mkimage -n "hello" -A arm -O u-boot -T standalone -C none -a c100000 -d examples/standalone/hello_world.bin -v /tftpboot/hello.ub
Let's get Albert on CC here, since, yeah, this is a problem...
This is not just arm specific problem. I see the same problem on Microblaze too. Was the support removed or just not tested and something broke it?
Can you bisect down to where this breaks on microblaze?
On 11/20/2013 02:49 PM, Tom Rini wrote:
On Wed, Nov 20, 2013 at 08:59:43AM +0100, Michal Simek wrote:
On 11/19/2013 09:07 PM, Tom Rini wrote:
On Tue, Nov 19, 2013 at 10:09:44AM +0100, Michal Simek wrote:
Hi Albert,
I have just bisect why on the latest kernel hello world example doesn't work and I have end up in this patch too. After reverting hello world example is working.
Also I would like to know if we have still bootm support for standlone http://www.denx.de/wiki/view/DULG/UBootStandalone#Section_5.12.3. Because it is not working for me with error "ERROR: booting os 'U-Boot' (17) is not supported" and I can't see IH_OS_U_BOOT in boot_os table in cmd_bootm.c
I have created image like this. ./tools/mkimage -n "hello" -A arm -O u-boot -T standalone -C none -a c100000 -d examples/standalone/hello_world.bin -v /tftpboot/hello.ub
Let's get Albert on CC here, since, yeah, this is a problem...
This is not just arm specific problem. I see the same problem on Microblaze too. Was the support removed or just not tested and something broke it?
Can you bisect down to where this breaks on microblaze?
I even don't know if this has ever worked on Microblaze. The best will be if we can get any input from Denx before someone dive to it. They should be aware when this was working and on which arch/board.
Thanks, Michal
On Wed, Nov 20, 2013 at 02:53:31PM +0100, Michal Simek wrote:
On 11/20/2013 02:49 PM, Tom Rini wrote:
On Wed, Nov 20, 2013 at 08:59:43AM +0100, Michal Simek wrote:
On 11/19/2013 09:07 PM, Tom Rini wrote:
On Tue, Nov 19, 2013 at 10:09:44AM +0100, Michal Simek wrote:
Hi Albert,
I have just bisect why on the latest kernel hello world example doesn't work and I have end up in this patch too. After reverting hello world example is working.
Also I would like to know if we have still bootm support for standlone http://www.denx.de/wiki/view/DULG/UBootStandalone#Section_5.12.3. Because it is not working for me with error "ERROR: booting os 'U-Boot' (17) is not supported" and I can't see IH_OS_U_BOOT in boot_os table in cmd_bootm.c
I have created image like this. ./tools/mkimage -n "hello" -A arm -O u-boot -T standalone -C none -a c100000 -d examples/standalone/hello_world.bin -v /tftpboot/hello.ub
Let's get Albert on CC here, since, yeah, this is a problem...
This is not just arm specific problem. I see the same problem on Microblaze too. Was the support removed or just not tested and something broke it?
Can you bisect down to where this breaks on microblaze?
I even don't know if this has ever worked on Microblaze. The best will be if we can get any input from Denx before someone dive to it. They should be aware when this was working and on which arch/board.
Well, if you jump back to v2013.01 you should be well before any of this happened. If it works there, try releases up to the last to find the bisect range. If 2013.01 fails then, OK, different problem.
On 11/20/2013 03:01 PM, Tom Rini wrote:
On Wed, Nov 20, 2013 at 02:53:31PM +0100, Michal Simek wrote:
On 11/20/2013 02:49 PM, Tom Rini wrote:
On Wed, Nov 20, 2013 at 08:59:43AM +0100, Michal Simek wrote:
On 11/19/2013 09:07 PM, Tom Rini wrote:
On Tue, Nov 19, 2013 at 10:09:44AM +0100, Michal Simek wrote:
Hi Albert,
I have just bisect why on the latest kernel hello world example doesn't work and I have end up in this patch too. After reverting hello world example is working.
Also I would like to know if we have still bootm support for standlone http://www.denx.de/wiki/view/DULG/UBootStandalone#Section_5.12.3. Because it is not working for me with error "ERROR: booting os 'U-Boot' (17) is not supported" and I can't see IH_OS_U_BOOT in boot_os table in cmd_bootm.c
I have created image like this. ./tools/mkimage -n "hello" -A arm -O u-boot -T standalone -C none -a c100000 -d examples/standalone/hello_world.bin -v /tftpboot/hello.ub
Let's get Albert on CC here, since, yeah, this is a problem...
This is not just arm specific problem. I see the same problem on Microblaze too. Was the support removed or just not tested and something broke it?
Can you bisect down to where this breaks on microblaze?
I even don't know if this has ever worked on Microblaze. The best will be if we can get any input from Denx before someone dive to it. They should be aware when this was working and on which arch/board.
Well, if you jump back to v2013.01 you should be well before any of this happened. If it works there, try releases up to the last to find the bisect range. If 2013.01 fails then, OK, different problem.
ok. It happened between 2013.04 and 2013.07 by the patch from Simon below. I have to run now that's why I can't look what exactly it is wrong.
Thanks, Michal
35fc84fa1ff51e15ecd3e464dac87eb105ffed30 is the first bad commit commit 35fc84fa1ff51e15ecd3e464dac87eb105ffed30 Author: Simon Glass sjg@chromium.org Date: Tue Jun 11 11:14:47 2013 -0700
Refactor the bootm command to reduce code duplication
At present the bootm code is mostly duplicated for the plain 'bootm' command and its sub-command variant. This makes the code harder to maintain and means that changes must be made to several places.
Introduce do_bootm_states() which performs selected portions of the bootm work, so that both plain 'bootm' and 'bootm <sub_command>' can use the same code.
Additional duplication exists in bootz, so tidy that up as well. This is not intended to change behaviour, apart from minor fixes where the previously-duplicated code missed some chunks of code.
Signed-off-by: Simon Glass sjg@chromium.org
:040000 040000 d474a190a80c65a99484351c6021ba4f25904025 27382e1ba0944a264044075e01fe99d83852c56e M common :040000 040000 48e1b21b4ad811a0aa718d52253fb264f31b9ffb 82c78cb1928c5288cbbc651fa01b9588b3f26ac7 M include
Hi Leif,
On 11/18/2013 12:26 PM, Leif Lindholm wrote:
Commit fe1378a - "ARM: use r9 for gd" - broke the ABI for users of the U-Boot API on ARM. Users I am aware of are GRUB and the FreeBSD loader. Since I only spotted this on Saturday, this code is well and truly in the wild, so any users of the API will need to preserve both r8 and r9 on syscalls for the foreseeable future (which is not the end of the world).
I actually broke it a bit more than that, since the stubs appear to use the gd register directly (where I wasn't aware of and still surprises me a bit, I will have a better look later why this is). The patch below should help for the hello_world example on master. (as in both u-boot and the application use r9 for gd)
I am aware this doesn't answer your question and I will come back on that if I have a bit more time to look into it (Saterday I guess).
Regards, Jeroen
diff --git a/examples/standalone/stubs.c b/examples/standalone/stubs.c index 8fb1765..5d2ab56 100644 --- a/examples/standalone/stubs.c +++ b/examples/standalone/stubs.c @@ -40,14 +40,14 @@ gd_t *global_data; : : "i"(offsetof(gd_t, jt)), "i"(XF_ ## x * sizeof(void *)) : "r11"); #elif defined(CONFIG_ARM) /* - * r8 holds the pointer to the global_data, ip is a call-clobbered + * r9 holds the pointer to the global_data, ip is a call-clobbered * register */ #define EXPORT_FUNC(x) \ asm volatile ( \ " .globl " #x "\n" \ #x ":\n" \ -" ldr ip, [r8, %0]\n" \ +" ldr ip, [r9, %0]\n" \ " ldr pc, [ip, %1]\n" \ : : "i"(offsetof(gd_t, jt)), "i"(XF_ ## x * sizeof(void *)) : "ip"); #elif defined(CONFIG_MIPS)
Hello Leif,
On 11/18/2013 12:26 PM, Leif Lindholm wrote:
Commit fe1378a - "ARM: use r9 for gd" - broke the ABI for users of the U-Boot API on ARM. Users I am aware of are GRUB and the FreeBSD loader. Since I only spotted this on Saturday, this code is well and truly in the wild, so any users of the API will need to preserve both r8 and r9 on syscalls for the foreseeable future (which is not the end of the world).
After looking into it a bit better it seems the standalone applications don't use the api, but poke into u-boot directly to look for a separate jump-table in gd. I am still a bit surprised it works like that, but at least the api doesn't. (I sent a patch to fix them)
The api issue is therefore only related to requirement that r9 needs to be preserved. The README states since the beginning of the git repro (2002-11-03) [1] that r8 _and_ r9 have special meaning in U-boot. Albert removed the need for the GOT pointer (r9) over time by limiting the supported relocation types, so the move from r8 to r9 was not expected to cause an ABI problem, since both were special register already. Anyway, I will have a look if it did break the ubldr.
Sometime last year I proposed a change to make the API syscall entry and exit points responsible for maintaining full procedure-call standard compliance, although the patch I provided was on the hackier side of good (and probably wrong).
After the switch to r9 the call on ARM is eabi compliant, with r9 being a platform register as allowed by the specs. Whether u-boot or the payload is responsible for preserving r9 / in general should preserve and restore its special registers in API calls is better answered by the maintainers.
Regards, Jeroen
Hi Jeroen,
On Sat, Nov 23, 2013 at 12:20:54PM +0100, Jeroen Hofstee wrote:
Commit fe1378a - "ARM: use r9 for gd" - broke the ABI for users of the U-Boot API on ARM. Users I am aware of are GRUB and the FreeBSD loader. Since I only spotted this on Saturday, this code is well and truly in the wild, so any users of the API will need to preserve both r8 and r9 on syscalls for the foreseeable future (which is not the end of the world).
After looking into it a bit better it seems the standalone applications don't use the api, but poke into u-boot directly to look for a separate jump-table in gd. I am still a bit surprised it works like that, but at least the api doesn't. (I sent a patch to fix them)
The api issue is therefore only related to requirement that r9 needs to be preserved. The README states since the beginning of the git repro (2002-11-03) [1] that r8 _and_ r9 have special meaning in U-boot.
[1] missing.
Upon inspection: the top-level README also states that R10 can be used as the stack limit, which is not descibed by the AAPCS. Nor is the use of R11 as a frame pointer.
Albert removed the need for the GOT pointer (r9) over time by limiting the supported relocation types, so the move from r8 to r9 was not expected to cause an ABI problem, since both were special register already. Anyway, I will have a look if it did break the ubldr.
http://svnweb.freebsd.org/base?view=revision&revision=258527
Sometime last year I proposed a change to make the API syscall entry and exit points responsible for maintaining full procedure-call standard compliance, although the patch I provided was on the hackier side of good (and probably wrong).
After the switch to r9 the call on ARM is eabi compliant, with r9 being a platform register as allowed by the specs.
EABI compliant for a specific platform that defines its use as such, yes. It's not a free-for-all: unless explicitly defined by the platform, r9 is 'variable register 6', a callee-saved register. (And here is where it all gets messy because U-Boot calls into the application, and then further down that call stack the application calls into U-Boot.)
Whether u-boot or the payload is responsible for preserving r9 / in general should preserve and restore its special registers in API calls is better answered by the maintainers.
There is only one agent involved in this equation which can always know how U-Boot deviates from the base AAPCS, and that is U-Boot. The most helpful thing it can do is to hide these deviations, which may deviate in different ways from that of the default behaviour of the toolchain used to build it based on which operating system that is running under.
/ Leif
participants (4)
-
Jeroen Hofstee -
Leif Lindholm -
Michal Simek -
Tom Rini