Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32375
Change subject: postcar_loader: Use size_t ......................................................................
postcar_loader: Use size_t
To make it easier to pop arguments from stack use size_t instead of int. This is a noop on x86_32.
Change-Id: I226f79eaea425229eb2e3d9b9a312c9bf47d3b6a Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/arch/x86/include/arch/cpu.h M src/arch/x86/postcar_loader.c 2 files changed, 10 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/32375/1
diff --git a/src/arch/x86/include/arch/cpu.h b/src/arch/x86/include/arch/cpu.h index 61b17a6..1cdf2db 100644 --- a/src/arch/x86/include/arch/cpu.h +++ b/src/arch/x86/include/arch/cpu.h @@ -310,8 +310,8 @@ struct postcar_frame { uintptr_t stack; uint32_t upper_mask; - int max_var_mtrrs; - int num_var_mtrrs; + size_t max_var_mtrrs; + size_t num_var_mtrrs; };
/* diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index d62487e..a043f1b 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -25,13 +25,13 @@ #include <stage_cache.h> #include <timestamp.h>
-static inline void stack_push(struct postcar_frame *pcf, uint32_t val) -{ - uint32_t *ptr; - - pcf->stack -= sizeof(val); - ptr = (void *)pcf->stack; - *ptr = val; +#define stack_push(pcf, val) \ +{ \ + typeof(val) *ptr; \ + \ + pcf->stack -= sizeof(val); \ + ptr = (void *)pcf->stack; \ + *ptr = val; \ }
static void postcar_frame_prepare(struct postcar_frame *pcf) @@ -74,7 +74,7 @@ uint32_t mtrr_size;
if (pcf->num_var_mtrrs >= pcf->max_var_mtrrs) { - printk(BIOS_ERR, "No more variable MTRRs: %d\n", + printk(BIOS_ERR, "No more variable MTRRs: %zd\n", pcf->max_var_mtrrs); return; }
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32375 )
Change subject: postcar_loader: Use size_t ......................................................................
Patch Set 4:
(1 comment)
W
https://review.coreboot.org/#/c/32375/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32375/4//COMMIT_MSG@8 PS4, Line 8: : To make it easier to pop arguments from stack use size_t instead of int. : This is a noop on x86_32. I'm not very well aware of how amd64 works but why is using 32bit math/ stack pop not an option?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32375 )
Change subject: postcar_loader: Use size_t ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/#/c/32375/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32375/4//COMMIT_MSG@8 PS4, Line 8: : To make it easier to pop arguments from stack use size_t instead of int. : This is a noop on x86_32.
I'm not very well aware of how amd64 works but why is using 32bit math/ stack pop not an option?
POP r32 instruction is marked N.E. (not encodable) in 64-bit mode.
https://review.coreboot.org/#/c/32375/4/src/arch/x86/postcar_loader.c File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/#/c/32375/4/src/arch/x86/postcar_loader.c@32 PS4, Line 32: pcf->stack -= sizeof(val); \ It's confusing with x86_64 if you allow the stack pointer to break alignment to 8 bytes.
https://review.coreboot.org/#/c/32375/4/src/arch/x86/postcar_loader.c@99 PS4, Line 99: stack_push(pcf, addr | type); Proper aligment of values on the stack might be lost here with x86_64.
Patrick Rudolph has uploaded a new patch set (#9) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/32375 )
Change subject: postcar_loader: Use size_t ......................................................................
postcar_loader: Use size_t
To make it easier to pop arguments from stack use size_t instead of int. On AMD64 it's not possible to pop 32bit values from stack, thus use the native integer size to allow the use of a single instruction. This is a noop on x86_32.
Change-Id: I226f79eaea425229eb2e3d9b9a312c9bf47d3b6a Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/arch/x86/postcar_loader.c 1 file changed, 3 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/32375/9
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32375 )
Change subject: postcar_loader: Use size_t ......................................................................
Patch Set 9:
(3 comments)
https://review.coreboot.org/c/coreboot/+/32375/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32375/4//COMMIT_MSG@8 PS4, Line 8: : To make it easier to pop arguments from stack use size_t instead of int. : This is a noop on x86_32.
POP r32 instruction is marked N.E. (not encodable) in 64-bit mode.
added to commit message
https://review.coreboot.org/c/coreboot/+/32375/4/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/32375/4/src/arch/x86/postcar_loader... PS4, Line 32: pcf->stack -= sizeof(val); \
It's confusing with x86_64 if you allow the stack pointer to break alignment to 8 bytes.
Done
https://review.coreboot.org/c/coreboot/+/32375/4/src/arch/x86/postcar_loader... PS4, Line 99: stack_push(pcf, addr | type);
Proper aligment of values on the stack might be lost here with x86_64.
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32375 )
Change subject: postcar_loader: Use size_t ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32375/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32375/13//COMMIT_MSG@9 PS13, Line 9: To make it easier to pop arguments from stack use size_t instead of int. There's a lot of assumptions in this sentence and no comments in the code. I assume you are thinking postcar is being ran from x86-64 mode as well as the stage that is pushing values? Just changing things to size_t doesn't seem appropriate. I'd like to see an analysis of the source and destination ISA you and what you think the stack frame should look like. e.g. 64 bit values like msrs can pushed as one value. I don't think we should just be changing the type to size_t in the push helper. You might want to break things up that will push 64-bit values as well as size values you are wanting to target.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32375 )
Change subject: postcar_loader: Use size_t ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32375/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32375/13//COMMIT_MSG@9 PS13, Line 9: To make it easier to pop arguments from stack use size_t instead of int.
There's a lot of assumptions in this sentence and no comments in the code. […]
Yes. postcar is being ran from x86-64 mode as well as the stage that is pushing values. This is due to rmodules nature, which can be only loaded from a stage that has the same arch. If that's no longer true the postcar frame will be broken.
Right now all values are poped from stack, such that the consumer will always do "size_t" reads. We could pack msrs into one value, but I don't see the benefit.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32375 )
Change subject: postcar_loader: Use size_t ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32375/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32375/13//COMMIT_MSG@9 PS13, Line 9: To make it easier to pop arguments from stack use size_t instead of int.
Yes. postcar is being ran from x86-64 mode as well as the stage that is pushing values. This is due to rmodules nature, which can be only loaded from a stage that has the same arch. If that's no longer true the postcar frame will be broken.
This is an implicit assumption. And the MSRs just happen to work because msrs are read and written as edx:eax while being pushed as two 64-bit values. I do think it's helpful to comment the assumptions and explicitly handle the various types. That doesn't exist in this patch where the code was previously indicating type size (uint32_t) it's now not w/ no explanation.
Right now all values are poped from stack, such that the consumer will always do "size_t" reads. We could pack msrs into one value, but I don't see the benefit.
It would be explicit in the handling of different types with source loader and target stage ISA.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32375 )
Change subject: postcar_loader: Use size_t ......................................................................
Patch Set 13: Code-Review-1
Changed CB:30500 to use the existing postcar frame. This is now obsolete.
Patrick Rudolph has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/32375 )
Change subject: postcar_loader: Use size_t ......................................................................
Abandoned
Not needed any more
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Kyösti Mälkki (Code Review)
-
Patrick Rudolph (Code Review)