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 6:
(11 comments)
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
XIP from boot media.
Changed my approach.
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
I know this is for stages post bootblock where the platform uses C environment for bootblock. […]
Correct. BTW, even though the change to this file goes away in the next PS, I still need to select C_ENVIRONMENT_BOOTBLOCK for the system to run properly. This was one of the little gotchas that result from using romstage source.
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.
Comments in this file need to be updated in general to account for the changes being made.
Will be OBE in the next PS.
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
Right.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 36: _romstage_in_ram_continue:
Ah. I don't see why you need to define _start in CB:33759 at reset vector location. […]
I could never link with two _start symbols, and I forget precisely the reason I needed it in the other file. It seems like when it was named differently, even if .globl, the linker wouldn't find it, or wouldn't position it properly -- it was something like that. So I went with _start = first instruction.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 43: call gdt_init
I see some GDT init in followup reset_vector.S already.
Right. Ideally I would've enjoyed starting inside this file at _start but (1) I already initialized the GDT, (2) I'm putting BIST and an initial timestamp on the stack. Line 46 would reset the pointer and wipe out the info.
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?
Right, it's only PROGBITS: https://review.coreboot.org/c/coreboot/+/33401.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 55: rep stosl
Isn't this now redundant with wiping _car_region_start .. _end in reset_vector. […]
Agreed, it's executing the stosl 14 times. Wasn't ideal but I was trying to minimize changes to this file. Goes away in next PS.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/assembly_entry... PS6, Line 72: call car_stage_entry
At the end of the day, does this entire file always reduce to the last call?
Yes, well starting at line 50. My thinking was to preserve the ability to enable the options above, as well not make too many assumptions for how a competing similar product may behave.
BTW, I'd already tried adding the lines above to picasso/reset_vector.S before noticing your comments in the later patch. The next PS will use that instead of this file.
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)
Why isn't this line up near line 37?
Sounds good and I can remove "SECTIONS" from my soc/.ld file.
https://review.coreboot.org/c/coreboot/+/32414/6/src/arch/x86/memlayout.ld@6... PS6, Line 61: #include <soc/romstage.ld>
To include car.ld for this concept seems invalid to me. […]
That's one of the examples of things you'd expect to work that don't. I want to get the platform up first, then make things outside of soc/amd/picasso more flexible where they make sense.