Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30855
Change subject: arch/x86: Align _start16bit with C_ENVIRONMENT_BOOBLOCK ......................................................................
arch/x86: Align _start16bit with C_ENVIRONMENT_BOOBLOCK
Followup removes SIPI_VECTOR_IN_ROM and it seems reasonable enough to force the alignment unconditionally to page size.
Reason for the conditionals is the alignment is not possible with romcc bootblocks having total size less than 4 kiB.
Change-Id: I0ff2786f80a319ebb3215d4fd696cda3e15c3012 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/cpu/x86/16bit/entry16.inc 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/30855/1
diff --git a/src/cpu/x86/16bit/entry16.inc b/src/cpu/x86/16bit/entry16.inc index 55d9bd9..2a9f8c5 100644 --- a/src/cpu/x86/16bit/entry16.inc +++ b/src/cpu/x86/16bit/entry16.inc @@ -29,7 +29,8 @@
#include <arch/rom_segs.h>
-#if IS_ENABLED(CONFIG_SIPI_VECTOR_IN_ROM) +#if IS_ENABLED(CONFIG_C_ENVIRONMENT_BOOTBLOCK) || \ + IS_ENABLED(CONFIG_SIPI_VECTOR_IN_ROM) /* Symbol _start16bit must be aligned to 4kB to start AP CPUs with * Startup IPI message without RAM. */
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30855 )
Change subject: arch/x86: Align _start16bit with C_ENVIRONMENT_BOOBLOCK ......................................................................
Patch Set 1: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30855 )
Change subject: arch/x86: Align _start16bit with C_ENVIRONMENT_BOOBLOCK ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/30855/2/src/cpu/x86/16bit/entry16.inc File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/#/c/30855/2/src/cpu/x86/16bit/entry16.inc@32 PS2, Line 32: #if IS_ENABLED(CONFIG_C_ENVIRONMENT_BOOTBLOCK) || \ Presumably this works because _start16bit is < 2^11 away from reset vector? This symbol needs to be tightly coupled with reset16.inc. I'm guessing we just get lucky in our #includes that everything gets thrown in the same section in bootblock_crt0.S. We probably should be more explicit in what sections everything lives in, but I'd also like to see symbol dump with this CL to compare if we're wasting space between here and reset vector.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30855 )
Change subject: arch/x86: Align _start16bit with C_ENVIRONMENT_BOOBLOCK ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/30855/2/src/cpu/x86/16bit/entry16.inc File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/#/c/30855/2/src/cpu/x86/16bit/entry16.inc@32 PS2, Line 32: #if IS_ENABLED(CONFIG_C_ENVIRONMENT_BOOTBLOCK) || \
Presumably this works because _start16bit is < 2^11 away from reset vector? This symbol needs to be […]
Where does this 2^11 come from? 2^16 ? cpu/x86/16bit/reset.ld claims 4 GiB - 64KiB is acceptable, with an assert. I agree, increasing C_ENV_BOOTBLOCK_SIZE beyond 64 KiB would make this fail.
At the moment _start16bit is at the very beginning of bootblock, probably due the include order, at 4 GiB - C_ENV_BOOTBLOCK_SIZE.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30855 )
Change subject: arch/x86: Align _start16bit with C_ENVIRONMENT_BOOBLOCK ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/30855/2/src/cpu/x86/16bit/entry16.inc File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/#/c/30855/2/src/cpu/x86/16bit/entry16.inc@32 PS2, Line 32: #if IS_ENABLED(CONFIG_C_ENVIRONMENT_BOOTBLOCK) || \
Where does this 2^11 come from? 2^16 ? cpu/x86/16bit/reset. […]
Yes, sorry. I got confused with the alignment vs jmp instruction offset. 2^15 is more correct as the jump is relative signed offset. Anyway, we could improve the location by putting things into sections. However, I'm still not clear on the desire for the 4KiB alignment.
Even with 64KiB C_ENV_BOOTBLOCK_SIZE the reset vector won't work if _start16bit is more than 32KiB away because of the signedness of the jmp instruction. And since the reset vector is at the end of any C_ENV_BOOTBLOCK_SIZE we are limited on the placement of _start16bit symbol being 4GiB - 32KiB away.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30855 )
Change subject: arch/x86: Align _start16bit with C_ENVIRONMENT_BOOBLOCK ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/30855/2/src/cpu/x86/16bit/entry16.inc File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/#/c/30855/2/src/cpu/x86/16bit/entry16.inc@32 PS2, Line 32: #if IS_ENABLED(CONFIG_C_ENVIRONMENT_BOOTBLOCK) || \
Yes, sorry. I got confused with the alignment vs jmp instruction offset. […]
objdump -M i8086 -d build/cbfs/fallback/bootblock.elf
build/cbfs/fallback/bootblock.elf: file format elf32-i386
Disassembly of section .text:
ffff0000 <_start16bit>: ffff0000: fa cli
....
Disassembly of section .reset:
fffffff0 <_start>: fffffff0: e9 0d 00 jmp ffff0000 <_start16bit> fffffff3: ff (bad) fffffff4: ff 66 90 jmp *-0x70(%bp) fffffff7: 66 90 xchg %eax,%eax fffffff9: 66 90 xchg %eax,%eax fffffffb: 66 90 xchg %eax,%eax fffffffd: 66 90 xchg %eax,%eax ...
Well.. the above still does boot for me. It's real mode, 16bit IP overflows when given a positive rel16 argument with, yet that intrasegment near jump stays within the same CS segment?
Not really the scope of this patch, to address _start16bit<->reset distance?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30855 )
Change subject: arch/x86: Align _start16bit with C_ENVIRONMENT_BOOBLOCK ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/30855/2/src/cpu/x86/16bit/entry16.inc File src/cpu/x86/16bit/entry16.inc:
https://review.coreboot.org/#/c/30855/2/src/cpu/x86/16bit/entry16.inc@32 PS2, Line 32: #if IS_ENABLED(CONFIG_C_ENVIRONMENT_BOOTBLOCK) || \
objdump -M i8086 -d build/cbfs/fallback/bootblock.elf […]
Ya, it's just reliant on the wrap it seems with starting at the beginning of the segment.
(gdb) p/x 0xfffffff3 + 0x000d $1 = 0x0 gdb) p/x 0xfffffff3 + 0xffff000d $2 = 0xffff0000
So yes, you are right in that we're explicitly taking advantage of that:
-------.byte 0xe9 -------.int _start16bit - ( . + 2 )
We aren't relying on the linker picking the correct relocation offset. We're just forcing the math to rollover.
I don't think it hurts to add a sanity check w.r.t. enforcing our assumptions. In the current form, that's C_ENV_BOOTBLOCK_SIZE <= 64KiB. But, yes, you are correct that it's not strictly tied to this CL. However, having that check in place can fix any potential future breakage. I was hoping it'd be small enough that you could put it in here or another patch.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30855 )
Change subject: arch/x86: Align _start16bit with C_ENVIRONMENT_BOOBLOCK ......................................................................
Patch Set 2:
So we could isolate top 4 KiB? Use it for entry16, entry32, walkcbfs, update_microcode?
I'll try to learn how to do that, there's no rush submitting this one.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30855 )
Change subject: arch/x86: Align _start16bit with C_ENVIRONMENT_BOOBLOCK ......................................................................
Patch Set 2:
Patch Set 2:
So we could isolate top 4 KiB? Use it for entry16, entry32, walkcbfs, update_microcode?
I'll try to learn how to do that, there's no rush submitting this one.
Yes, we could do that. There's some gotchas there in how we add FIT entries and bizarre platform specifics such as apollolake. That said, I believe putting in a static assert for our current implementation of C_ENV_BOOTBLOCK_SIZE <= 64KiB would be sufficient. If we need to make it larger then we could look at fixing the location of those pieces of code. After working through the math, current implementation, and adding a static assert inherently documents and protects the current implementation. I'm fine with submitting this and following up.
This could be put in a better place, but we just need something to fail during compilation:
diff --git a/src/arch/x86/boot.c b/src/arch/x86/boot.c index 7819c0f2db..2b4a1c3c55 100644 --- a/src/arch/x86/boot.c +++ b/src/arch/x86/boot.c @@ -41,3 +41,11 @@ void arch_prog_run(struct prog *prog) :: "D"(prog_entry(prog)) ); } + +#if IS_ENABLED(CONFIG_C_ENV_BOOTBLOCK) +/* Please see src/cpu/x86/16bit/reset16.inc and the jump to _start16bit from the reset vector. + * The current implementation requires a max jump (currently includes math wrap around) size + * of 64 KiB. This check assumes the reset vector is at the end of bootblock and _start16bit + * symbol being farthest away at the beginning of bootblock. */ +_Static_assert(CONFIG_C_ENV_BOOTBLOCK_SIZE <= 64*KiB); +#endif
Kyösti Mälkki has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/30855 )
Change subject: arch/x86: Align _start16bit with C_ENVIRONMENT_BOOBLOCK ......................................................................
arch/x86: Align _start16bit with C_ENVIRONMENT_BOOBLOCK
Followup removes SIPI_VECTOR_IN_ROM and it seems reasonable enough to force the alignment unconditionally to page size.
Reason for the conditionals is the alignment is not possible with romcc bootblocks having total size less than 4 kiB.
Change-Id: I0ff2786f80a319ebb3215d4fd696cda3e15c3012 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/30855 Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/cpu/x86/16bit/entry16.inc 1 file changed, 2 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Arthur Heymans: Looks good to me, approved
diff --git a/src/cpu/x86/16bit/entry16.inc b/src/cpu/x86/16bit/entry16.inc index 55d9bd9..2a9f8c5 100644 --- a/src/cpu/x86/16bit/entry16.inc +++ b/src/cpu/x86/16bit/entry16.inc @@ -29,7 +29,8 @@
#include <arch/rom_segs.h>
-#if IS_ENABLED(CONFIG_SIPI_VECTOR_IN_ROM) +#if IS_ENABLED(CONFIG_C_ENVIRONMENT_BOOTBLOCK) || \ + IS_ENABLED(CONFIG_SIPI_VECTOR_IN_ROM) /* Symbol _start16bit must be aligned to 4kB to start AP CPUs with * Startup IPI message without RAM. */
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30855 )
Change subject: arch/x86: Align _start16bit with C_ENVIRONMENT_BOOBLOCK ......................................................................
Patch Set 3:
Patch Set 2:
Patch Set 2:
So we could isolate top 4 KiB? Use it for entry16, entry32, walkcbfs, update_microcode?
I'll try to learn how to do that, there's no rush submitting this one.
Yes, we could do that. There's some gotchas there in how we add FIT entries and bizarre platform specifics such as apollolake. That said, I believe putting in a static assert for our current implementation of C_ENV_BOOTBLOCK_SIZE <= 64KiB would be sufficient. If we need to make it larger then we could look at fixing the location of those pieces of code. After working through the math, current implementation, and adding a static assert inherently documents and protects the current implementation. I'm fine with submitting this and following up.
This could be put in a better place, but we just need something to fail during compilation:
LINK cbfs/fallback/bootblock.debug ... xgcc/bin/i386-elf-ld.bfd: _start16bit too low. Please report. src/arch/x86/Makefile.inc:120: recipe for target 'build/cbfs/fallback/bootblock.debug' failed make: *** [build/cbfs/fallback/bootblock.debug] Error 1
$ git grep "_start16bit too low" src/cpu/x86/16bit/reset16.ld: _bogus = ASSERT(_start16bit >= 0xffff0000, "_start16bit too low. Please report.");
IMHO it already fails compile-time with a reasonable error message.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30855 )
Change subject: arch/x86: Align _start16bit with C_ENVIRONMENT_BOOBLOCK ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 2:
Patch Set 2:
So we could isolate top 4 KiB? Use it for entry16, entry32, walkcbfs, update_microcode?
I'll try to learn how to do that, there's no rush submitting this one.
Yes, we could do that. There's some gotchas there in how we add FIT entries and bizarre platform specifics such as apollolake. That said, I believe putting in a static assert for our current implementation of C_ENV_BOOTBLOCK_SIZE <= 64KiB would be sufficient. If we need to make it larger then we could look at fixing the location of those pieces of code. After working through the math, current implementation, and adding a static assert inherently documents and protects the current implementation. I'm fine with submitting this and following up.
This could be put in a better place, but we just need something to fail during compilation:
LINK cbfs/fallback/bootblock.debug ... xgcc/bin/i386-elf-ld.bfd: _start16bit too low. Please report. src/arch/x86/Makefile.inc:120: recipe for target 'build/cbfs/fallback/bootblock.debug' failed make: *** [build/cbfs/fallback/bootblock.debug] Error 1
$ git grep "_start16bit too low" src/cpu/x86/16bit/reset16.ld: _bogus = ASSERT(_start16bit >= 0xffff0000, "_start16bit too low. Please report.");
IMHO it already fails compile-time with a reasonable error message.
Perfect. I completely forgot about reset16.ld.
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Kyösti Mälkki (Code Review)