Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32414 )
Change subject: arch/x86: Add option for using a custom assembly_entry ......................................................................
Patch Set 7:
(15 comments)
https://review.coreboot.org/c/coreboot/+/32414/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/32414/1/src/Kconfig@153 PS1, Line 153: x86
Fixed in the commit message.
Done
https://review.coreboot.org/c/coreboot/+/32414/6/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/32414/6/src/Kconfig@161 PS6, Line 161: XIP
Changed my approach.
Ack
https://review.coreboot.org/c/coreboot/+/32414/1/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/32414/1/src/arch/x86/assembly_entry... PS1, Line 29: IS_ENABLED
Done. Thanks.
Done
https://review.coreboot.org/c/coreboot/+/32414/2/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/32414/2/src/arch/x86/assembly_entry... PS2, Line 19: #if CONFIG(C_ENVIRONMENT_BOOTBLOCK)
So,these boards are supposed to select C_ENVIRONMENT_BOOTBLOCK even if they won't have a true bootbl […]
I still can't boot w/o it. That's one of the things I want to fix later, as it'll take some careful consideration of how to make the generic x86 code more flexible.
https://review.coreboot.org/c/coreboot/+/32414/2/src/arch/x86/assembly_entry... PS2, Line 49: CAR_GLOBAL
If the ROMSTAGE starts in RAM, why is CAR_GLOBAL area required?
I'm using the FSP 2.0 driver later in the patch stack that won't currently function properly unless all the assumptions are in place.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... File src/arch/x86/assembly_entry.S:
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 19: C_ENVIRONMENT_BOOTBLOCK
Correct. […]
Ack
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 23: The gdt is reloaded to accommodate : * platforms that are executing out of CAR.
Will be OBE in the next PS.
Ack
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 32: * executed code, are responsible for getting the processor into protected
reset_vector.S in followup […]
Ack
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 36: _romstage_in_ram_continue:
I could never link with two _start symbols, and I forget precisely the reason I needed it in the oth […]
Ack
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 43: call gdt_init
Right. […]
OBE in later PSs
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 49: /* clear CAR_GLOBAL area as it is not shared */
How is romstage being built and loaded? Is it only PROGBITS being loaded? […]
Ack
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 55: rep stosl
Agreed, it's executing the stosl 14 times. […]
OBE in later PSs
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 72: call car_stage_entry
Yes, well starting at line 50. […]
Ack
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/memlayout.ld@6... PS6, Line 60: #if ENV_ROMSTAGE && CONFIG(ROMSTAGE_IN_RAM)
Sounds good and I can remove "SECTIONS" from my soc/.ld file.
Done
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/memlayout.ld@6... PS6, Line 61: #include <soc/romstage.ld>
That's one of the examples of things you'd expect to work that don't. […]
Ack