Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36143 )
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
arch/x86/boot.c: Pass arguments when running programs
Payloads can use coreboot tables passed on the stack. Stages could make use of an argument passed on the stack.
The assumption is made that stages and payloads start in assembly where stack alignment does not matter.
Change-Id: Ie0f44e9e1992221e02c49d0492cdd2a3d9013560 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/boot.c 1 file changed, 3 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/36143/1
diff --git a/src/arch/x86/boot.c b/src/arch/x86/boot.c index 5f60f13..4cdced3 100644 --- a/src/arch/x86/boot.c +++ b/src/arch/x86/boot.c @@ -32,11 +32,13 @@ { __asm__ volatile ( #ifdef __x86_64__ + "push %%rax\n" "jmp *%%rdi\n" #else + "push %%eax\n" "jmp *%%edi\n" #endif
- :: "D"(prog_entry(prog)) + :: "a" (prog_entry_arg(prog)), "D"(prog_entry(prog)) ); }
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36143
to look at the new patch set (#2).
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
arch/x86/boot.c: Pass arguments when running programs
Payloads can use coreboot tables passed on the stack. Stages could make use of an argument passed on the stack.
The assumption is made that stages and payloads start in assembly where stack alignment does not matter.
Change-Id: Ie0f44e9e1992221e02c49d0492cdd2a3d9013560 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/boot.c 1 file changed, 3 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/36143/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36143 )
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36143/2/src/arch/x86/boot.c File src/arch/x86/boot.c:
https://review.coreboot.org/c/coreboot/+/36143/2/src/arch/x86/boot.c@33 PS2, Line 33: void(* doit)(void *); missing space after return type
https://review.coreboot.org/c/coreboot/+/36143/2/src/arch/x86/boot.c@33 PS2, Line 33: void(* doit)(void *); Unnecessary space before function pointer name
https://review.coreboot.org/c/coreboot/+/36143/2/src/arch/x86/boot.c@33 PS2, Line 33: void(* doit)(void *); space prohibited after that '*' (ctx:BxW)
https://review.coreboot.org/c/coreboot/+/36143/2/src/arch/x86/boot.c@33 PS2, Line 33: void(* doit)(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/+/36143 )
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36143/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36143/2//COMMIT_MSG@9 PS2, Line 9: Payloads can use coreboot tables passed on the stack. only on x86_32 arguments are pushed onto the stack
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36143 )
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36143/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36143/2//COMMIT_MSG@9 PS2, Line 9: Payloads can use coreboot tables passed on the stack.
only on x86_32 arguments are pushed onto the stack
Correct. Calling through a function pointer is subject to the ABI the compiler is targeting.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36143 )
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36143/2/src/arch/x86/boot.c File src/arch/x86/boot.c:
https://review.coreboot.org/c/coreboot/+/36143/2/src/arch/x86/boot.c@33 PS2, Line 33: doit You should decorate this w/ asmlinkage to ensure the i386 ABI is used in case compiler flags are flipped.
Hello Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36143
to look at the new patch set (#3).
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
arch/x86/boot.c: Pass arguments when running programs
Payloads can use coreboot tables passed on the stack. Stages could make use of an argument passed on the stack.
The assumption is made that stages and payloads start in assembly where stack alignment does not matter.
Change-Id: Ie0f44e9e1992221e02c49d0492cdd2a3d9013560 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/boot.c 1 file changed, 4 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/36143/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36143 )
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36143/3/src/arch/x86/boot.c File src/arch/x86/boot.c:
https://review.coreboot.org/c/coreboot/+/36143/3/src/arch/x86/boot.c@34 PS3, Line 34: asmlinkage void(* doit)(void *); missing space after return type
https://review.coreboot.org/c/coreboot/+/36143/3/src/arch/x86/boot.c@34 PS3, Line 34: asmlinkage void(* doit)(void *); Unnecessary space before function pointer name
https://review.coreboot.org/c/coreboot/+/36143/3/src/arch/x86/boot.c@34 PS3, Line 34: asmlinkage void(* doit)(void *); space prohibited after that '*' (ctx:BxW)
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36143 )
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
Patch Set 3: Code-Review+1
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36143 )
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36143/3/src/arch/x86/boot.c File src/arch/x86/boot.c:
https://review.coreboot.org/c/coreboot/+/36143/3/src/arch/x86/boot.c@34 PS3, Line 34: asmlinkage I requested this thinking about x86-32. However, I'm not sure what this decoration does when compiling for x86-64. Arthur, what's the disassembly look like with that change?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36143 )
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36143/3/src/arch/x86/boot.c File src/arch/x86/boot.c:
https://review.coreboot.org/c/coreboot/+/36143/3/src/arch/x86/boot.c@34 PS3, Line 34: asmlinkage
I requested this thinking about x86-32. However, I'm not sure what this decoration does when compiling for x86-64. Arthur, what's the disassembly look like with that change?
On x86-32:
2000229: 8b 44 24 04 mov 0x4(%esp),%eax 200022d: 8b 50 20 mov 0x20(%eax),%edx 2000230: 89 54 24 04 mov %edx,0x4(%esp) 2000234: ff 60 1c jmp *0x1c(%eax)
The asmlinkage flag does not change this.
Hello Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36143
to look at the new patch set (#5).
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
arch/x86/boot.c: Pass arguments when running programs
Payloads can use coreboot tables passed on the stack. Stages could make use of an argument passed on the stack.
The assumption is made that stages and payloads start in assembly where stack alignment does not matter.
Change-Id: Ie0f44e9e1992221e02c49d0492cdd2a3d9013560 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/boot.c 1 file changed, 6 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/36143/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36143 )
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36143/5/src/arch/x86/boot.c File src/arch/x86/boot.c:
https://review.coreboot.org/c/coreboot/+/36143/5/src/arch/x86/boot.c@34 PS5, Line 34: void(*doit)(void *); missing space after return type
https://review.coreboot.org/c/coreboot/+/36143/5/src/arch/x86/boot.c@34 PS5, Line 34: void(*doit)(void *); function definition argument 'void *' should also have an identifier name
https://review.coreboot.org/c/coreboot/+/36143/5/src/arch/x86/boot.c@37 PS5, Line 37: asmlinkage void(*doit)(void *); missing space after return type
Hello Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36143
to look at the new patch set (#6).
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
arch/x86/boot.c: Pass arguments when running programs
Payloads can use coreboot tables passed on the stack. Stages could make use of an argument passed on the stack.
The assumption is made that stages and payloads start in assembly where stack alignment does not matter.
Change-Id: Ie0f44e9e1992221e02c49d0492cdd2a3d9013560 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/boot.c 1 file changed, 6 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/36143/6
Hello Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36143
to look at the new patch set (#7).
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
arch/x86/boot.c: Pass arguments when running programs
Payloads can use coreboot tables passed on the stack. Stages could make use of an argument passed on the stack.
The assumption is made that stages and payloads start in assembly where stack alignment does not matter.
Change-Id: Ie0f44e9e1992221e02c49d0492cdd2a3d9013560 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/boot.c 1 file changed, 5 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/36143/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36143 )
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36143/7/src/arch/x86/boot.c File src/arch/x86/boot.c:
https://review.coreboot.org/c/coreboot/+/36143/7/src/arch/x86/boot.c@34 PS7, Line 34: void(*doit)(void *arg); missing space after return type
https://review.coreboot.org/c/coreboot/+/36143/7/src/arch/x86/boot.c@37 PS7, Line 37: asmlinkage void(*doit)(void *arg); missing space after return type
Hello Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36143
to look at the new patch set (#8).
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
arch/x86/boot.c: Pass arguments when running programs
Payloads can use coreboot tables passed on the stack. Stages could make use of an argument passed on the stack.
The assumption is made that stages and payloads start in assembly where stack alignment does not matter.
Change-Id: Ie0f44e9e1992221e02c49d0492cdd2a3d9013560 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/boot.c 1 file changed, 5 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/36143/8
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36143 )
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36143/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36143/8//COMMIT_MSG@12 PS8, Line 12: The assumption is made that stages and payloads start in assembly : where stack alignment does not matter. Well actually this assumption does not have to be made I think.
Hello Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36143
to look at the new patch set (#9).
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
arch/x86/boot.c: Pass arguments when running programs
Payloads can use coreboot tables passed on the stack. Stages could make use of an argument passed on the stack.
Change-Id: Ie0f44e9e1992221e02c49d0492cdd2a3d9013560 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/boot.c 1 file changed, 5 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/36143/9
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36143 )
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
Patch Set 9: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/36143/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36143/9//COMMIT_MSG@10 PS9, Line 10: Stages could make use of an argument passed on the stack. fwiw, x86-64 abi is not using the stack. It's using the sys-v calling convention of passing things through registers.
Hello Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36143
to look at the new patch set (#10).
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
arch/x86/boot.c: Pass arguments when running programs
Payloads can use coreboot tables passed on via arguments instead of via a pointer in lower memory.
Stages can make use of the argument to pass on information.
Change-Id: Ie0f44e9e1992221e02c49d0492cdd2a3d9013560 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/boot.c 1 file changed, 5 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/36143/10
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36143 )
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36143/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36143/9//COMMIT_MSG@10 PS9, Line 10: Stages could make use of an argument passed on the stack.
fwiw, x86-64 abi is not using the stack. […]
Done
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36143 )
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
Patch Set 10:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36143/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36143/2//COMMIT_MSG@9 PS2, Line 9: Payloads can use coreboot tables passed on the stack.
Correct. Calling through a function pointer is subject to the ABI the compiler is targeting.
Done
https://review.coreboot.org/c/coreboot/+/36143/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36143/8//COMMIT_MSG@12 PS8, Line 12: The assumption is made that stages and payloads start in assembly : where stack alignment does not matter.
Well actually this assumption does not have to be made I think.
Done
https://review.coreboot.org/c/coreboot/+/36143/2/src/arch/x86/boot.c File src/arch/x86/boot.c:
https://review.coreboot.org/c/coreboot/+/36143/2/src/arch/x86/boot.c@33 PS2, Line 33: doit
You should decorate this w/ asmlinkage to ensure the i386 ABI is used in case compiler flags are fli […]
Done
https://review.coreboot.org/c/coreboot/+/36143/3/src/arch/x86/boot.c File src/arch/x86/boot.c:
https://review.coreboot.org/c/coreboot/+/36143/3/src/arch/x86/boot.c@34 PS3, Line 34: asmlinkage
I requested this thinking about x86-32. […]
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36143 )
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
Patch Set 10: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36143 )
Change subject: arch/x86/boot.c: Pass arguments when running programs ......................................................................
arch/x86/boot.c: Pass arguments when running programs
Payloads can use coreboot tables passed on via arguments instead of via a pointer in lower memory.
Stages can make use of the argument to pass on information.
Change-Id: Ie0f44e9e1992221e02c49d0492cdd2a3d9013560 Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/36143 Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/arch/x86/boot.c 1 file changed, 5 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Aaron Durbin: Looks good to me, approved
diff --git a/src/arch/x86/boot.c b/src/arch/x86/boot.c index 5f60f13..ada49d0 100644 --- a/src/arch/x86/boot.c +++ b/src/arch/x86/boot.c @@ -30,13 +30,12 @@
void arch_prog_run(struct prog *prog) { - __asm__ volatile ( #ifdef __x86_64__ - "jmp *%%rdi\n" + void (*doit)(void *arg); #else - "jmp *%%edi\n" + /* Ensure the argument is pushed on the stack. */ + asmlinkage void (*doit)(void *arg); #endif - - :: "D"(prog_entry(prog)) - ); + doit = prog_entry(prog); + doit(prog_entry_arg(prog)); }
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)