Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33759 )
Change subject: soc/amd/picasso: Create a hybrid romstage to begin in DRAM ......................................................................
Patch Set 6:
(9 comments)
https://review.coreboot.org/c/coreboot/+/33759/5/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/33759/5/src/soc/amd/picasso/Kconfig... PS5, Line 99: and not usable by the OS.
Is romstage executed on S3 resume path? Comment of marking regions reserved also applies to where ro […]
Yes it is. I was going to put it in the source and not comment it above, however given that it wouldn't be obvious it makes sense.
https://review.coreboot.org/c/coreboot/+/33759/5/src/soc/amd/picasso/exit_ca... File src/soc/amd/picasso/exit_car.S:
https://review.coreboot.org/c/coreboot/+/33759/5/src/soc/amd/picasso/exit_ca... PS5, Line 23: * this file can be removed.
Little reason to drag along TODOs about postcar-stage. […]
Done
https://review.coreboot.org/c/coreboot/+/33759/4/src/soc/amd/picasso/reset_v... File src/soc/amd/picasso/reset_vector.S:
https://review.coreboot.org/c/coreboot/+/33759/4/src/soc/amd/picasso/reset_v... PS4, Line 135: */
Got out of sync when splitting. Will be N/A in a moment.
Done
https://review.coreboot.org/c/coreboot/+/33759/4/src/soc/amd/picasso/reset_v... PS4, Line 142: sub $8, %esp
Done
Done
https://review.coreboot.org/c/coreboot/+/33759/4/src/soc/amd/picasso/reset_v... PS4, Line 144: jmp _romstage_in_ram_continue
Ah, yeah. I realized that reading your comment in the x86 file about the code "reducing".
Ack
https://review.coreboot.org/c/coreboot/+/33759/4/src/soc/amd/picasso/reset_v... PS4, Line 149: _start:
In new PS it should be the only _start.
Done
https://review.coreboot.org/c/coreboot/+/33759/5/src/soc/amd/picasso/reset_v... File src/soc/amd/picasso/reset_vector.S:
https://review.coreboot.org/c/coreboot/+/33759/5/src/soc/amd/picasso/reset_v... PS5, Line 145: and $0xfffffff0, %esp
Need 'subl $4, %esp' here, as you push 12 bytes below.
Are you certain? I thought I recalled that functions should have their stack in alignment at the moment they're entered. So counting the return address with the call, that's 16. I looked to verify it's 16-byte aligned at that spot using this code. Maybe I remembered the requirement wrong.
https://review.coreboot.org/c/coreboot/+/33759/5/src/soc/amd/picasso/reset_v... PS5, Line 154: call hybrid_romstage_entry
Thanks, much cleaner this way.
Good suggestion
https://review.coreboot.org/c/coreboot/+/33759/5/src/soc/amd/picasso/reset_v... PS5, Line 165: _start:
IMHO _start should come after _start16bit in execution flow. This could be _reset. […]
I'm uncertain too. However in arch/x86/assembly_entry.S it looks right, as that's precisely the first instruction in the elf. It seems I can make an argument for leaving it here in this new file because it's at the first instruction I'm executing.
-
Marshall Dawson (Code Review)