Hello Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/46834
to review the following change.
Change subject: x86: Put bootblock startup code into .text._start section ......................................................................
x86: Put bootblock startup code into .text._start section
The initial bootblock assembly code on x86 is just put into the .text section, which just happens to come before all the individual .text.* function sections in the program.ld script. So it tends to be at the start of the image, bit if you inserted another linker script section with contents before .text, it would cause a problem. (I'm not sure if it's an architectural requirement for _start16bit to come at the start of the image, but at least its 4K alignment requirement would waste a lot of space if it didn't.)
This patch moves the section to .text._start which is the name other architectures use for the code they want in the very front of the image and which is listed first in program.ld.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Ia84e6e33ec29584d356e226e8fdcb8c9334d49af --- M src/arch/x86/bootblock_crt0.S 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/46834/1
diff --git a/src/arch/x86/bootblock_crt0.S b/src/arch/x86/bootblock_crt0.S index 3f41464..82ae97f 100644 --- a/src/arch/x86/bootblock_crt0.S +++ b/src/arch/x86/bootblock_crt0.S @@ -10,7 +10,7 @@
#include <cpu/x86/cr.h>
-.section .text +.section .text._start
/* * Include the old code for reset vector and protected mode entry. That code has
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46834 )
Change subject: x86: Put bootblock startup code into .text._start section ......................................................................
Patch Set 1: Code-Review+2
(3 comments)
I don't think anything is exceeding CONFIG_C_ENV_BOOTBLOCK_SIZE of 64KiB:
$ git grep -A 2 C_ENV_BOOTBLOCK_SIZE | grep default | sed -e 's/if .*//' | awk '{ print $NF}' | sort | uniq -c 2 0x10000 7 0x4000 7 0x8000 10 0xC000
So I think we should be good.
https://review.coreboot.org/c/coreboot/+/46834/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46834/1//COMMIT_MSG@12 PS1, Line 12: bit but
https://review.coreboot.org/c/coreboot/+/46834/1//COMMIT_MSG@14 PS1, Line 14: _start16bit It needs to be within 64KiB of reset vector which is the end of the bootblock program for dealing with a 16-bit jump instructions.
https://review.coreboot.org/c/coreboot/+/46834/1//COMMIT_MSG@16 PS1, Line 16: lot of space if it didn't.) The 4KiB alignment requirement is only for certain platforms where the APs need to be SIPI'd towards the same flow as BSP. It doesn't need to be there all the time. You can see a symbol being made, ap_sipi_vector_in_rom, in src/cpu/x86/16bit/entry16.ld. From the looks of it, only src/cpu/intel/car/p4-netburst/cache_as_ram.S has a reference to that symbol.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46834 )
Change subject: x86: Put bootblock startup code into .text._start section ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/46834/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46834/3//COMMIT_MSG@12 PS3, Line 12: bit but
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46834 )
Change subject: x86: Put bootblock startup code into .text._start section ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46834/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46834/1//COMMIT_MSG@14 PS1, Line 14: _start16bit
It needs to be within 64KiB of reset vector which is the end of the bootblock program for dealing wi […]
CB:47599 enforces this too.
https://review.coreboot.org/c/coreboot/+/46834/1//COMMIT_MSG@16 PS1, Line 16: lot of space if it didn't.)
The 4KiB alignment requirement is only for certain platforms where the APs need to be SIPI'd towards […]
CB:47599 drops static C_ENV_BOOTBLOCK_SIZE. Due the different arrangement of sections, the problematic bootblock intel/d945gclf actually increases to 19KiB, but log2 size requirement was already dropped with preceding works on topic:x86-bootblock.
I have certainty with mPGA604 and LGA755 sockets, but I am not sure if SIPI_VECTOR_IN_ROM is a requirement for any CPU model intel/d945gclf is compatible with. We go back to P4 NetBurst microarchitecture and cirka year 2009 silicons.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46834 )
Change subject: x86: Put bootblock startup code into .text._start section ......................................................................
Patch Set 4: Code-Review+1
Patrick Georgi has uploaded a new patch set (#5) to the change originally created by Julius Werner. ( https://review.coreboot.org/c/coreboot/+/46834 )
Change subject: x86: Put bootblock startup code into .text._start section ......................................................................
x86: Put bootblock startup code into .text._start section
The initial bootblock assembly code on x86 is just put into the .text section, which just happens to come before all the individual .text.* function sections in the program.ld script. So it tends to be at the start of the image, but if you inserted another linker script section with contents before .text, it would cause a problem. (I'm not sure if it's an architectural requirement for _start16bit to come at the start of the image, but at least its 4K alignment requirement would waste a lot of space if it didn't.)
This patch moves the section to .text._start which is the name other architectures use for the code they want in the very front of the image and which is listed first in program.ld.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Ia84e6e33ec29584d356e226e8fdcb8c9334d49af --- M src/arch/x86/bootblock_crt0.S 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/46834/5
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46834 )
Change subject: x86: Put bootblock startup code into .text._start section ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46834/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46834/1//COMMIT_MSG@12 PS1, Line 12: bit
but
Ack
https://review.coreboot.org/c/coreboot/+/46834/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46834/3//COMMIT_MSG@12 PS3, Line 12: bit
but
Ack
Patrick Georgi has uploaded a new patch set (#6) to the change originally created by Julius Werner. ( https://review.coreboot.org/c/coreboot/+/46834 )
Change subject: x86: Put bootblock startup code into .text._start section ......................................................................
x86: Put bootblock startup code into .text._start section
The initial bootblock assembly code on x86 is just put into the .text section, which just happens to come before all the individual .text.* function sections in the program.ld script. So it tends to be at the start of the image, bit if you inserted another linker script section with contents before .text, it would cause a problem. (I'm not sure if it's an architectural requirement for _start16bit to come at the start of the image, but at least its 4K alignment requirement would waste a lot of space if it didn't.)
This patch moves the section to .text._start which is the name other architectures use for the code they want in the very front of the image and which is listed first in program.ld.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Ia84e6e33ec29584d356e226e8fdcb8c9334d49af --- M src/arch/x86/bootblock_crt0.S 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/46834/6
Patrick Georgi has uploaded a new patch set (#7) to the change originally created by Julius Werner. ( https://review.coreboot.org/c/coreboot/+/46834 )
Change subject: x86: Put bootblock startup code into .text._start section ......................................................................
x86: Put bootblock startup code into .text._start section
The initial bootblock assembly code on x86 is just put into the .text section, which just happens to come before all the individual .text.* function sections in the program.ld script. So it tends to be at the start of the image, but if you inserted another linker script section with contents before .text, it would cause a problem. (I'm not sure if it's an architectural requirement for _start16bit to come at the start of the image, but at least its 4K alignment requirement would waste a lot of space if it didn't.)
This patch moves the section to .text._start which is the name other architectures use for the code they want in the very front of the image and which is listed first in program.ld.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Ia84e6e33ec29584d356e226e8fdcb8c9334d49af --- M src/arch/x86/bootblock_crt0.S 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/46834/7
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46834 )
Change subject: x86: Put bootblock startup code into .text._start section ......................................................................
Patch Set 7: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46834 )
Change subject: x86: Put bootblock startup code into .text._start section ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46834/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46834/1//COMMIT_MSG@16 PS1, Line 16: lot of space if it didn't.)
CB:47599 drops static C_ENV_BOOTBLOCK_SIZE. […]
I hope it's okay that I merge this for now so I can proceed here. Feel free to undo it again or rewrite it in a better way with CB:47599.
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46834 )
Change subject: x86: Put bootblock startup code into .text._start section ......................................................................
x86: Put bootblock startup code into .text._start section
The initial bootblock assembly code on x86 is just put into the .text section, which just happens to come before all the individual .text.* function sections in the program.ld script. So it tends to be at the start of the image, but if you inserted another linker script section with contents before .text, it would cause a problem. (I'm not sure if it's an architectural requirement for _start16bit to come at the start of the image, but at least its 4K alignment requirement would waste a lot of space if it didn't.)
This patch moves the section to .text._start which is the name other architectures use for the code they want in the very front of the image and which is listed first in program.ld.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Ia84e6e33ec29584d356e226e8fdcb8c9334d49af Reviewed-on: https://review.coreboot.org/c/coreboot/+/46834 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/arch/x86/bootblock_crt0.S 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/arch/x86/bootblock_crt0.S b/src/arch/x86/bootblock_crt0.S index 3f41464..82ae97f 100644 --- a/src/arch/x86/bootblock_crt0.S +++ b/src/arch/x86/bootblock_crt0.S @@ -10,7 +10,7 @@
#include <cpu/x86/cr.h>
-.section .text +.section .text._start
/* * Include the old code for reset vector and protected mode entry. That code has
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Julius Werner (Code Review)
-
Kyösti Mälkki (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)