Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/18526 )
Change subject: binaryPI: Drop CAR teardown without POSTCAR_STAGE ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/18526/8/src/vendorcode/amd/pi/00630... File src/vendorcode/amd/pi/00630F01/binaryPI/gcccar.inc:
https://review.coreboot.org/c/coreboot/+/18526/8/src/vendorcode/amd/pi/00630... PS8, Line 920: wbinvd
Was this used as a very hacky way to make sure cbmem hits ram during romstage? That is definitely nasty and makes me wonder if S3 resume is even possible doing this.
I kind of doubt it was for cbmem, although its addition predates me. There was definitely a "known" low memory corruption problem for S3 when I started working on it, so that was defeatured for all binaryPI IIRC. In those days, cbmem on AMD was initialized "late". I speculate the wbinvd was a way of preserving CAR data and stack info. I seem to recall Kyösti had solved the non-binaryPI AGESA stack problem by ensuring it was 100% popped when it got reassigned. Also a bit of trivia: a challenge to moving Stoney's cbmem_init to early was that detecting whether migration had occurred was unreliable because the CAR region was over top of DRAM. Intel implementations placed it in MMIO, ensuring FFs were read once CAR was disabled. Aaron pointed me at postcar to get around that.
Kyösti, BTW if I'm able to license the binaryPI glue, or whatever that looks like, we need to make sure that the APs aren't also using a wbinvd! Unsure what implementation made that change. We fixed it for ST, then ultimately changed binaryPI to call out to coreboot for tearing down APs' CAR.
https://review.coreboot.org/c/coreboot/+/18526/8/src/vendorcode/amd/pi/00630... PS8, Line 1550: * AMD_DISABLE_STACK: Destroy the stack inside the cache. This routine : * should only be executed on the BSP
Is there some documentation of what is run on AP's by AGESA and in what state those are?
I sort of implied it in my other comment, however the binary implementations I've looked at use duplicated CAR teardown routines, one still inside AGESA image for the APs, and this one run by the BSP.
When the BSP hits the AmdInitEarly() entry point, it releases the APs which start running from the reset vector (vs. an INIT SIPI). The BSP waits for the APs to catch up inside AmdInitEarly() before parking them and continuing. (Sorry but...) without taking the time to go back and look at it, IIRC the BSP's teardown process triggers the APs to run the internal teardown routine.
-
Marshall Dawson (Code Review)