Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37487 )
Change subject: program.ld: Qualify .bss linking with ENV_EARLY_RAM ......................................................................
program.ld: Qualify .bss linking with ENV_EARLY_RAM
Systems with ENV_EARLY_RAM need to position .bss in a consistent location across early stages, and not necessarily within the program region. Skip the default .bss section for these designs. A subsequent patch will add the early linker script.
Change-Id: Icde1cfd02611b8cce634aa76f5c192d5b637cf44 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/lib/program.ld 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/37487/1
diff --git a/src/lib/program.ld b/src/lib/program.ld index a9d4e48..1c8f99e 100644 --- a/src/lib/program.ld +++ b/src/lib/program.ld @@ -125,7 +125,7 @@ } #endif
-#if ENV_STAGE_HAS_BSS_SECTION && !ENV_CACHE_AS_RAM +#if (ENV_STAGE_HAS_BSS_SECTION && !ENV_CACHE_AS_RAM && !ENV_EARLY_RAM) .bss . : { . = ALIGN(ARCH_POINTER_ALIGN_SIZE); _bss = .;
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37487 )
Change subject: program.ld: Qualify .bss linking with ENV_EARLY_RAM ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37487/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37487/1//COMMIT_MSG@10 PS1, Line 10: location across early stages, and not necessarily within the The semantics of .bss is that it is not shared between stages and therefore its location has not been fixed. Why do you need to change this?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37487 )
Change subject: program.ld: Qualify .bss linking with ENV_EARLY_RAM ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37487/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37487/1//COMMIT_MSG@10 PS1, Line 10: location across early stages, and not necessarily within the
The semantics of . […]
This came about from adding a bootblock/romstage implementation, which I gathered you wanted (but I'm somewhat confident others will too).
If the hybrid-romstage implementation was the only one available, this could be simplified. However this solves a big problem for the bootblock/romstage scenario. In a CAR-based bootblock/romstage system with postcar, the bss region vanishes once CAR is torn down. In PCO's case, using bss means I need to burn DRAM, then reserve it so the OS doesn't use it.
So this purpose here isn't to share bss between stages. It's only to reuse the same DRAM in romstage as was used in bootblock (and bss gets re-zeroed). So all this change does is it allows me to position bss manually using early_ram.ld (CB:35035), the same way car.ld does for ENV_ROMSTAGE_OR_BEFORE.
Now, FWIW, this implementation (i.e. the stack ending with CB:37486 PS21) has romstage still xip when using a bootblock/romstage. I noticed that I'm able to run successfully using NO_XIP_EARLY_STAGES, with romstage running at the default 02000000. However that DRAM isn't getting reserved. So I guess NO_XIP_EARLY_STAGES probably assumes early_stage programs and their data also just vanish once CAR is torn down.
Would you prefer the way I have it, or making NO_XIP_EARLY_STAGES compatible with Picasso?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37487 )
Change subject: program.ld: Qualify .bss linking with ENV_EARLY_RAM ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37487/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37487/1//COMMIT_MSG@10 PS1, Line 10: location across early stages, and not necessarily within the
This came about from adding a bootblock/romstage implementation, which I gathered you wanted (but I' […]
I am not the one giving -2/+2 on this question, but I'll give my opinions anyway.
a) I believe x86 CAR approach uses shared .bss for ENV_ROMSTATE_OR_BEFORE because it has to, not because we want it to. CAR is a restricted resource and the setup is volatile which has to be done in bootblock already. It's not really possible for romstage to extend the .bss region that was setup in cache-as-ram.S.
b) IMHO .bss within program(.ld) is the clean approach, and with DRAM available the complications of CAR do not apply. This would be the same approach ARCH_ARM uses already? I would choose wasting some tens of KiB of DRAM (from OS) over toolchain complications.
c) As far as wasted DRAM is concerned, I don't know why you would want to keep the DRAM bootblock/verstage/romstage used reserved from OS, one should be allowed to discard those early in ramstage. I have not tried to understand or visualise your plans for S3 resume path, nor do I have access to required documentation. Solution where S3 resume path only uses memory in CBMEM or TSEG would be preferred to avoid any low-memory reservations or backups.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37487 )
Change subject: program.ld: Qualify .bss linking with ENV_EARLY_RAM ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37487/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37487/1//COMMIT_MSG@10 PS1, Line 10: location across early stages, and not necessarily within the
I am not the one giving -2/+2 on this question, but I'll give my opinions anyway. […]
a) Agree. b) I think you're advocating for cleaner elfs/stages, and using NO_XIP_EARLY_STAGES work. (I'm not up on ARCH_ARM.) c) I'm late on starting S3 work, however I'm presuming the PSP still overwrites my initial stage on each cycle and that must be reserved to avoid corrupting OS memory. After the 1st stage, i.e. in a bootblock->romstage scenario, I don't get cbmem until romstage. But if bootblock loads a non-xip romstage into DRAM, that clobbers OS memory, it seems to me. That's why I inferred that NO_XIP_EARLY_STAGES must be using all CAR and no DRAM. It would explain why it's not currently reserved, i.e. it would simply disappear at teardown.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37487 )
Change subject: program.ld: Qualify .bss linking with ENV_EARLY_RAM ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37487/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37487/1//COMMIT_MSG@10 PS1, Line 10: location across early stages, and not necessarily within the
a) Agree. […]
First, I guess there is some chance of current work getting approved, while it is not optimal it is not that intrusive or bad either.
I have not yet abandoned the idea of storing romstage as an rmodule and have it relocated somewhere high in between CBMEM and TSEG.
Discuss your options about S3 resume path early with ChromeOS security team. If Secure Process does not load bootblock and romstage from SPI and also uses a separate resume vector (I remember something like that from MullinsPI and StoneyPI), that resume vector possibly has to come from (""secure"") stage cache inside TSEG.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37487 )
Change subject: program.ld: Qualify .bss linking with ENV_EARLY_RAM ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37487/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37487/1//COMMIT_MSG@10 PS1, Line 10: location across early stages, and not necessarily within the
First, I guess there is some chance of current work getting approved, while it is not optimal it is […]
Ack. Over the holiday break, we reconsidered what vboot will look like. Our new direction should perform better than originally planned, and will dictate/allow that we do a more traditional bootblock/verstage/romstage/ramstage progression. The process will begin in the PSP, but finish on the x86. Will put more details in another gerrit entry. But hopefully I can make this one go away.
Re. the S3 resume vector, I've been trying to make time to implement S3 on Picasso. But AFAIK the vector inside TSEG is only when the PSP's more traditional Secure Boot is enabled. For Stoney Ridge in coreboot, of course we didn't use Secure Boot, but we resume back through the bootblock/romstage path. I anticipate that being the same with Picasso.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37487 )
Change subject: program.ld: Qualify .bss linking with ENV_EARLY_RAM ......................................................................
Patch Set 4:
obsolete approach?
Marshall Dawson has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37487 )
Change subject: program.ld: Qualify .bss linking with ENV_EARLY_RAM ......................................................................
Abandoned
Yes, this is an old approach. I missed abandoning it before.
-
Kyösti Mälkki (Code Review) -
Marshall Dawson (Code Review)