Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37368 )
Change subject: arch/x86/car: Remove runtime stack alignment enforcing ......................................................................
arch/x86/car: Remove runtime stack alignment enforcing
This is now checked at buildtime.
Change-Id: Ice687b1a4de53de4799e90238c98cfef19a81136 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/non-evict/cache_as_ram.S M src/cpu/intel/car/p3/cache_as_ram.S M src/cpu/intel/car/p4-netburst/cache_as_ram.S M src/cpu/qemu-x86/cache_as_ram_bootblock.S M src/soc/intel/common/block/cpu/car/cache_as_ram.S 6 files changed, 0 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/37368/1
diff --git a/src/cpu/intel/car/core2/cache_as_ram.S b/src/cpu/intel/car/core2/cache_as_ram.S index 73618d9..be96633 100644 --- a/src/cpu/intel/car/core2/cache_as_ram.S +++ b/src/cpu/intel/car/core2/cache_as_ram.S @@ -173,7 +173,6 @@
/* Need to align stack to 16 bytes at call instruction. Account for the pushes below. */ - andl $0xfffffff0, %esp subl $4, %esp
/* push TSC and BIST to stack */ diff --git a/src/cpu/intel/car/non-evict/cache_as_ram.S b/src/cpu/intel/car/non-evict/cache_as_ram.S index 5a668c4..6c78ddb 100644 --- a/src/cpu/intel/car/non-evict/cache_as_ram.S +++ b/src/cpu/intel/car/non-evict/cache_as_ram.S @@ -219,7 +219,6 @@
/* Need to align stack to 16 bytes at call instruction. Account for the pushes below. */ - andl $0xfffffff0, %esp subl $4, %esp
/* push TSC and BIST to stack */ diff --git a/src/cpu/intel/car/p3/cache_as_ram.S b/src/cpu/intel/car/p3/cache_as_ram.S index 5262b18..8c3009a 100644 --- a/src/cpu/intel/car/p3/cache_as_ram.S +++ b/src/cpu/intel/car/p3/cache_as_ram.S @@ -161,7 +161,6 @@
/* Need to align stack to 16 bytes at call instruction. Account for the pushes below. */ - andl $0xfffffff0, %esp subl $4, %esp
/* push TSC and BIST to stack */ diff --git a/src/cpu/intel/car/p4-netburst/cache_as_ram.S b/src/cpu/intel/car/p4-netburst/cache_as_ram.S index fdeb0af..8fd240d 100644 --- a/src/cpu/intel/car/p4-netburst/cache_as_ram.S +++ b/src/cpu/intel/car/p4-netburst/cache_as_ram.S @@ -372,7 +372,6 @@
/* Need to align stack to 16 bytes at call instruction. Account for the pushes below. */ - andl $0xfffffff0, %esp subl $4, %esp
/* push TSC and BIST to stack */ diff --git a/src/cpu/qemu-x86/cache_as_ram_bootblock.S b/src/cpu/qemu-x86/cache_as_ram_bootblock.S index 1fa0018..c1fe52d 100644 --- a/src/cpu/qemu-x86/cache_as_ram_bootblock.S +++ b/src/cpu/qemu-x86/cache_as_ram_bootblock.S @@ -37,8 +37,6 @@ movl $_ecar_stack, %esp
/* Align the stack and keep aligned for call to bootblock_c_entry() */ - and $0xfffffff0, %esp - /* Restore the BIST result and timestamps. */ #if defined(__x86_64__) movd %mm1, %rdi diff --git a/src/soc/intel/common/block/cpu/car/cache_as_ram.S b/src/soc/intel/common/block/cpu/car/cache_as_ram.S index 0992d85..5d7eafa 100644 --- a/src/soc/intel/common/block/cpu/car/cache_as_ram.S +++ b/src/soc/intel/common/block/cpu/car/cache_as_ram.S @@ -174,7 +174,6 @@
/* Need to align stack to 16 bytes at call instruction. Account for the two pushes below. */ - andl $0xfffffff0, %esp sub $8, %esp
/* push TSC value to stack */
Hello Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37368
to look at the new patch set (#2).
Change subject: arch/x86/car: Remove runtime stack alignment enforcing ......................................................................
arch/x86/car: Remove runtime stack alignment enforcing
This is now checked at buildtime.
Change-Id: Ice687b1a4de53de4799e90238c98cfef19a81136 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/intel/car/core2/cache_as_ram.S M src/cpu/intel/car/non-evict/cache_as_ram.S M src/cpu/intel/car/p3/cache_as_ram.S M src/cpu/intel/car/p4-netburst/cache_as_ram.S M src/cpu/qemu-x86/cache_as_ram_bootblock.S M src/soc/intel/common/block/cpu/car/cache_as_ram.S 6 files changed, 0 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/37368/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37368 )
Change subject: arch/x86/car: Remove runtime stack alignment enforcing ......................................................................
Patch Set 2: Code-Review+1
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37368 )
Change subject: arch/x86/car: Remove runtime stack alignment enforcing ......................................................................
Patch Set 2: Code-Review-2
These sequences are for ensuring the compiler's ABI is met when getting into C code. The expectation is that the stack is aligned to 16-byte boundary at the time of the call.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37368 )
Change subject: arch/x86/car: Remove runtime stack alignment enforcing ......................................................................
Patch Set 2: Code-Review-1
Patch Set 2: Code-Review-2
These sequences are for ensuring the compiler's ABI is met when getting into C code. The expectation is that the stack is aligned to 16-byte boundary at the time of the call.
I see this alignment was directly after using _ecar_stack symbol and reliant upon the previous patch. I'm not sure we gain much but saving a few bytes of instruction with the linker assertion. To me it reads better keeping the code there. If you really want to keep save an instruction please annotate the usage of _ecar_stack with a comment indicating you are reliant upon that symbol being 16-byte aligned. As the patch currently stands we've lost all knowledge of the assumptions and intention.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37368 )
Change subject: arch/x86/car: Remove runtime stack alignment enforcing ......................................................................
Patch Set 2:
a
Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37368 )
Change subject: arch/x86/car: Remove runtime stack alignment enforcing ......................................................................
Abandoned
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Arthur Heymans (Code Review)
-
Paul Menzel (Code Review)