Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: [WIP]arch/arm: Allow program loading of Linux kernels ......................................................................
[WIP]arch/arm: Allow program loading of Linux kernels
On ARM the linux kernel takes 3 arguments: r0 = 0 r1 = machine_type (0xffffffff if using FDT) r2 = &fdt
To allow this, a function with a different signature needs to be used when using a FIT payload.
Change-Id: Ie0dcc26d647941de71669345911ba288341b834b Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/arm/boot.c 1 file changed, 16 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/35409/1
diff --git a/src/arch/arm/boot.c b/src/arch/arm/boot.c index 9d1e4cd..fd7ba68 100644 --- a/src/arch/arm/boot.c +++ b/src/arch/arm/boot.c @@ -11,15 +11,29 @@ * GNU General Public License for more details. */
+#include <cbfs.h> #include <arch/cache.h> #include <program_loading.h>
void arch_prog_run(struct prog *prog) { void (*doit)(void *); + void (*doit_3)(void *, void *, void *); + char *program_arg = prog_entry_arg(prog);
cache_sync_instructions();
- doit = prog_entry(prog); - doit(prog_entry_arg(prog)); + /* The Linux kernel takes 3 dword's as argument */ + switch (prog_cbfs_type(prog)) { + case CBFS_TYPE_FIT: /* Flattened image tree */ + if (CONFIG(PAYLOAD_FIT_SUPPORT)) { + doit_3 = prog_entry(prog); + doit_3((void *)program_arg, (void *)(program_arg + sizeof(void *)), + (void *)(program_arg + 2 * sizeof(void *))); + break; + } /* else fall-through */ + default: + doit = prog_entry(prog); + doit((void *)program_arg); + } }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: [WIP]arch/arm: Allow program loading of Linux kernels ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35409/1/src/arch/arm/boot.c File src/arch/arm/boot.c:
https://review.coreboot.org/c/coreboot/+/35409/1/src/arch/arm/boot.c@21 PS1, Line 21: void (*doit_3)(void *, void *, void *); function definition argument 'void *' should also have an identifier name
https://review.coreboot.org/c/coreboot/+/35409/1/src/arch/arm/boot.c@21 PS1, Line 21: void (*doit_3)(void *, void *, void *); function definition argument 'void *' should also have an identifier name
https://review.coreboot.org/c/coreboot/+/35409/1/src/arch/arm/boot.c@21 PS1, Line 21: void (*doit_3)(void *, void *, void *); function definition argument 'void *' should also have an identifier name
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: [WIP]arch/arm: Allow program loading of Linux kernels ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35409/1/src/arch/arm/boot.c File src/arch/arm/boot.c:
https://review.coreboot.org/c/coreboot/+/35409/1/src/arch/arm/boot.c@31 PS1, Line 31: doit_3((void *)program_arg, (void *)(program_arg + sizeof(void *)), please use assembly code. as FIT is only used for LINUX kernel, the first two arguments should be hardcoded.
Hello Patrick Rudolph, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35409
to look at the new patch set (#2).
Change subject: [WIP]arch/arm: Allow program loading of Linux kernels ......................................................................
[WIP]arch/arm: Allow program loading of Linux kernels
On ARM the linux kernel takes 3 arguments: r0 = 0 r1 = machine_type (0xffffffff if using FDT) r2 = &fdt
To allow this, a function with a different signature needs to be used when using a FIT payload.
Change-Id: Ie0dcc26d647941de71669345911ba288341b834b Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/arm/boot.c 1 file changed, 16 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/35409/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: [WIP]arch/arm: Allow program loading of Linux kernels ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35409/2/src/arch/arm/boot.c File src/arch/arm/boot.c:
https://review.coreboot.org/c/coreboot/+/35409/2/src/arch/arm/boot.c@26 PS2, Line 26: case CBFS_TYPE_FIT: /* Flattened image tree */ You also need a dcache_mmu_disable() here, and an "msr cpsr_cxf, #0xd3" in assembly (to switch from System into SVC mode). (May also want to copy the warning about how this makes the stack pointer invalid from depthcharge: https://chromium.googlesource.com/chromiumos/platform/depthcharge/+/refs/hea... )
https://review.coreboot.org/c/coreboot/+/35409/2/src/arch/arm/boot.c@27 PS2, Line 27: if (CONFIG(PAYLOAD_FIT_SUPPORT)) { I'm not sure 3 instructions need to be compile-time guarded for space...
https://review.coreboot.org/c/coreboot/+/35409/2/src/arch/arm/boot.c@33 PS2, Line 33: : [fdt_ptr] "r" (prog_entry_arg(prog))); I think implementing this as an extra function in armv7/cpu.S would be cleaner than inline assembly.
Patrick Rudolph has uploaded a new patch set (#3) to the change originally created by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: arch/arm: Allow program loading of Linux kernels ......................................................................
arch/arm: Allow program loading of Linux kernels
On ARM the linux kernel takes 3 arguments: r0 = 0 r1 = machine_type (0xffffffff if using FDT) r2 = &fdt
To allow this, a function with a different signature needs to be used when using a FIT payload.
Tested on qemu-system-arm -m vepxress-a9 and uImage FIT: Boots to initramfs.
Change-Id: Ie0dcc26d647941de71669345911ba288341b834b Signed-off-by: Arthur Heymans arthur@aheymans.xyz Signed-off-by: Patrick Rudolph siro@das-labor.org --- M src/arch/arm/boot.c 1 file changed, 35 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/35409/3
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: arch/arm: Allow program loading of Linux kernels ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35409/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35409/3//COMMIT_MSG@18 PS3, Line 18: Boots to initramfs. What Linux version did you use?
https://review.coreboot.org/c/coreboot/+/35409/3/src/arch/arm/boot.c File src/arch/arm/boot.c:
https://review.coreboot.org/c/coreboot/+/35409/3/src/arch/arm/boot.c@41 PS3, Line 41: * here on! Fits in 96 characters?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: arch/arm: Allow program loading of Linux kernels ......................................................................
Patch Set 4: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/35409/4/src/arch/arm/boot.c File src/arch/arm/boot.c:
https://review.coreboot.org/c/coreboot/+/35409/4/src/arch/arm/boot.c@19 PS4, Line 19: "eor r0, r0\n\t" nit: This is an x86ism, I think mov r0, #0 is much more readable (and the same instruction length on Arm).
https://review.coreboot.org/c/coreboot/+/35409/4/src/arch/arm/boot.c@35 PS4, Line 35: * Switch to arm instruction set. We might be running in thumb2 mode. nit: technically could just do a blx r3 and that will automatically jump back to Arm mode (because bit 0 in r3 would be 0), but maybe being more explicit is good too.
Patrick Rudolph has uploaded a new patch set (#5) to the change originally created by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: arch/arm: Allow program loading of Linux kernels ......................................................................
arch/arm: Allow program loading of Linux kernels
On ARM the linux kernel takes 3 arguments: r0 = 0 r1 = machine_type (0xffffffff if using FDT) r2 = &fdt
To allow this, a function with a different signature needs to be used when using a FIT payload.
Tested on qemu-system-arm -m vepxress-a9 and uImage FIT: Boots to initramfs.
Change-Id: Ie0dcc26d647941de71669345911ba288341b834b Signed-off-by: Arthur Heymans arthur@aheymans.xyz Signed-off-by: Patrick Rudolph siro@das-labor.org --- M src/arch/arm/boot.c 1 file changed, 33 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/35409/5
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: arch/arm: Allow program loading of Linux kernels ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35409/2/src/arch/arm/boot.c File src/arch/arm/boot.c:
https://review.coreboot.org/c/coreboot/+/35409/2/src/arch/arm/boot.c@26 PS2, Line 26: case CBFS_TYPE_FIT: /* Flattened image tree */
You also need a dcache_mmu_disable() here, and an "msr cpsr_cxf, #0xd3" in assembly (to switch from […]
Done
https://review.coreboot.org/c/coreboot/+/35409/2/src/arch/arm/boot.c@27 PS2, Line 27: if (CONFIG(PAYLOAD_FIT_SUPPORT)) {
I'm not sure 3 instructions need to be compile-time guarded for space...
Done
https://review.coreboot.org/c/coreboot/+/35409/3/src/arch/arm/boot.c File src/arch/arm/boot.c:
https://review.coreboot.org/c/coreboot/+/35409/3/src/arch/arm/boot.c@41 PS3, Line 41: * here on!
Fits in 96 characters?
Done
https://review.coreboot.org/c/coreboot/+/35409/4/src/arch/arm/boot.c File src/arch/arm/boot.c:
https://review.coreboot.org/c/coreboot/+/35409/4/src/arch/arm/boot.c@19 PS4, Line 19: "eor r0, r0\n\t"
nit: This is an x86ism, I think mov r0, #0 is much more readable (and the same instruction length on […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: arch/arm: Allow program loading of Linux kernels ......................................................................
Patch Set 5: Code-Review+1
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: arch/arm: Allow program loading of Linux kernels ......................................................................
Patch Set 5: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: arch/arm: Allow program loading of Linux kernels ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
Some unresolved comments pending
https://review.coreboot.org/c/coreboot/+/35409/1/src/arch/arm/boot.c File src/arch/arm/boot.c:
https://review.coreboot.org/c/coreboot/+/35409/1/src/arch/arm/boot.c@31 PS1, Line 31: doit_3((void *)program_arg, (void *)(program_arg + sizeof(void *)),
please use assembly code. […]
Done
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: arch/arm: Allow program loading of Linux kernels ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35409/2/src/arch/arm/boot.c File src/arch/arm/boot.c:
https://review.coreboot.org/c/coreboot/+/35409/2/src/arch/arm/boot.c@33 PS2, Line 33: : [fdt_ptr] "r" (prog_entry_arg(prog)));
I think implementing this as an extra function in armv7/cpu.S would be cleaner than inline assembly.
is this still required? other arch use inline assembly as well.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: arch/arm: Allow program loading of Linux kernels ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35409/2/src/arch/arm/boot.c File src/arch/arm/boot.c:
https://review.coreboot.org/c/coreboot/+/35409/2/src/arch/arm/boot.c@33 PS2, Line 33: : [fdt_ptr] "r" (prog_entry_arg(prog)));
is this still required? other arch use inline assembly as well.
I'm okay with this patch as is.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: arch/arm: Allow program loading of Linux kernels ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35409/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35409/3//COMMIT_MSG@18 PS3, Line 18: Boots to initramfs.
What Linux version did you use?
Is anything needed here?
https://review.coreboot.org/c/coreboot/+/35409/4/src/arch/arm/boot.c File src/arch/arm/boot.c:
https://review.coreboot.org/c/coreboot/+/35409/4/src/arch/arm/boot.c@35 PS4, Line 35: * Switch to arm instruction set. We might be running in thumb2 mode.
nit: technically could just do a blx r3 and that will automatically jump back to Arm mode (because b […]
Is anything needed here?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: arch/arm: Allow program loading of Linux kernels ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35409/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35409/3//COMMIT_MSG@18 PS3, Line 18: Boots to initramfs.
Is anything needed here?
Linux 5.5.7
Sam Lewis has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: arch/arm: Allow program loading of Linux kernels ......................................................................
Patch Set 5:
Trying this on my Beaglebone Black port results in a _undefined_instruction interrupt when trying to jump to the loaded Linux kernel, even with the dcache_mmu_disable call added. This is with kernel version 5.4.
My (abandoned) version of this patch works OK - https://review.coreboot.org/c/coreboot/+/44376 .
I can try to debug further (and provide a bit more information) tomorrow, would be great to get this merged!
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: arch/arm: Allow program loading of Linux kernels ......................................................................
Patch Set 5:
Trying this on my Beaglebone Black port results in a _undefined_instruction interrupt when trying to jump to the loaded Linux kernel, even with the dcache_mmu_disable call added. This is with kernel version 5.4.
Well... can you provide a bit more information? Which instruction is it choking on? Is that still part of coreboot code or already of the kernel? Is the processor in Thumb or Arm mode when that happens? If you can provide the full exception dumb and disassembly of the relevant piece of code we might be able to help you better.
Another thing I noticed in our the depthcharge handoff code (which works) is that it does another tlb_invalidate_all() after disabling the MMU. Not really sure if that's needed, but at least it couldn't hurt...
I can try to debug further (and provide a bit more information) tomorrow, would be great to get this merged!
I mean... sounds like it's not working right now, so we should probably figure out your issues first before we merge it?
Sam Lewis has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: arch/arm: Allow program loading of Linux kernels ......................................................................
Patch Set 5:
So I finally had a bit of time to dig into why this didn't work for me - sorry, has been a bit hectic at my day job!
The inline assembly has a few issues, I think. I haven't done that much ARM inline assembly and have been learning as I'm going, so please correct anything I've misunderstood.
The first issue is that coreboot compiles ARMv7 code to thumb by default (-mthumb) so while the inline assembly here generates ARM instructions (with .arm), there's nothing to switch the processor to actually run in ARM mode. From what I understand of the ARM reference manual, I don't believe the `bl` branch will do it (is this the intention of that instruction?). I think it has to be a `blx` instead, otherwise the processor, while in thumb mode, will try to execute ARM encoded instructions and throw an undefined instruction exception.
The second issue is that GCC seems happy to choose registers for input operands that will clobber registers used in the inline assembly. Below is the output generated assembly for the arch_prog_run function. You can see that the fdt_ptr is loaded into r3 and the kernel_ptr is loaded into r2 at 0x80007bde but then because of the instruction at 80007bea, r2 gets overwritten with the fdt_ptr. So when the code later tries to jump to the kernel, it instead jumps to the fdt and then gets another undefined instruction exception.
void arch_prog_run(struct prog *prog) { 80007bcc: b510 push {r4, lr} 80007bce: 4604 mov r4, r0 void (*doit)(void *); cache_sync_instructions(); 80007bd0: f7ff fe00 bl 800077d4 <cache_sync_instructions> /* The Linux kernel takes 3 dword's as argument */ switch (prog_cbfs_type(prog)) { 80007bd4: 6863 ldr r3, [r4, #4] 80007bd6: 2b21 cmp r3, #33 ; 0x21 80007bd8: d113 bne.n 80007c02 <arch_prog_run+0x36> case CBFS_TYPE_FIT: /* Flattened image tree */ dcache_mmu_disable(); 80007bda: f7ff fdef bl 800077bc <dcache_mmu_disable> asm ( 80007bde: e9d4 2307 ldrd r2, r3, [r4, #28] 80007be2: f04f 0000 mov.w r0, #0 80007be6: f04f 31ff mov.w r1, #4294967295 ; 0xffffffff 80007bea: 461a mov r2, r3 80007bec: 4613 mov r3, r2 80007bee: f04f 04d3 mov.w r4, #211 ; 0xd3 80007bf2: f384 8b00 msr CPSR_fxc, r4 80007bf6: f000 e802 blx 80007bfc <arch_prog_run+0x30> 80007bfa: 0000 .short 0x0000 80007bfc: e1a0f003 mov pc, r3 break; default: doit = prog_entry(prog); doit(prog_entry_arg(prog)); } }
I'm guessing there's probably a way to tell GCC to use registers for input operands that won't interfere with registers used in the inline assembly, but perhaps this might be all more straight forward if we used non-inline assembly instead?
I saw that it has already been discussed a little, but what do people think about adapting this change to use an assembly function similar to how depthcharge does it? (https://chromium.googlesource.com/chromiumos/platform/depthcharge/+/refs/hea... and https://chromium.googlesource.com/chromiumos/platform/depthcharge/+/refs/hea...). I think (but correct me if I'm wrong) this should make the swap to ARM mode automatic (through the function call), and should make the input operand part a bit simpler (can use normal calling convention). I'm happy to give this a shot, but equally happy if Arthur or somebody else wants to do it.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: arch/arm: Allow program loading of Linux kernels ......................................................................
Patch Set 5: -Code-Review
The first issue is that coreboot compiles ARMv7 code to thumb by default (-mthumb) so while the inline assembly here generates ARM instructions (with .arm), there's nothing to switch the processor to actually run in ARM mode. From what I understand of the ARM reference manual, I don't believe the `bl` branch will do it (is this the intention of that instruction?). I think it has to be a `blx` instead, otherwise the processor, while in thumb mode, will try to execute ARM encoded instructions and throw an undefined instruction exception.
Right, that probably needs to be a BLX. I think there is sometimes some assembler magic where the assembler turns a BL into a BLX automatically depending on the mode of the current and the target function -- but that might rely on declaring the other code as a separate function to the assembler which this doesn't quite do. So maybe that's why it doesn't translate correctly? (Note that it actually says BLX in the disassembly you provided, is that from you fixing it manually?)
The second issue is that GCC seems happy to choose registers for input operands that will clobber registers used in the inline assembly. Below is the output generated assembly for the arch_prog_run function. You can see that the fdt_ptr is loaded into r3 and the kernel_ptr is loaded into r2 at 0x80007bde but then because of the instruction at 80007bea, r2 gets overwritten with the fdt_ptr. So when the code later tries to jump to the kernel, it instead jumps to the fdt and then gets another undefined instruction exception.
Yeah, probably need to add r0 through r4 to the clobber list there.
I'm guessing there's probably a way to tell GCC to use registers for input operands that won't interfere with registers used in the inline assembly, but perhaps this might be all more straight forward if we used non-inline assembly instead?
I agree, with all these intricacies, just making it a real assembly function in a separate file is probably better.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: arch/arm: Allow program loading of Linux kernels ......................................................................
Patch Set 5: Code-Review+1
Sam Lewis has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: arch/arm: Allow program loading of Linux kernels ......................................................................
Patch Set 5:
(Note that it actually says BLX in the disassembly you provided, is that from you fixing it manually?)
Correct, that was from me manually fixing it.
I agree, with all these intricacies, just making it a real assembly function in a separate file is probably better.
Have created a new patch that does this with an assembly function, submitted here: https://review.coreboot.org/c/coreboot/+/45335
Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35409 )
Change subject: arch/arm: Allow program loading of Linux kernels ......................................................................
Abandoned
Already in master in a different form
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Julius Werner (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Sam Lewis (Code Review)