[coreboot-gerrit] Change in coreboot[master]: amd/pi: Add verstage support

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/18439 )

Change subject: amd/pi: Add verstage support
......................................................................


Patch Set 7:

(3 comments)

https://review.coreboot.org/#/c/18439/7/src/cpu/amd/pi/romstage_after_verstage.S
File src/cpu/amd/pi/romstage_after_verstage.S:

Line 21: 	call	romstage_after_verstage
> So why put it below this sequence? It shouldn't matter where it's included.
I was trying to match in 909c512c drivers/intel/fsp1_1/romstage_after_verstage.S as closely as possible.  I'm seeing the difference in flow now.


Line 23: 	#include "after_raminit.S"
> Right. I was a little confused, but why both have romstage_after_verstage()
> There already is src/arch/x86/verstage.c

That's only built when CONFIG_C_ENVIRONMENT_BOOTBLOCK=y but AMD doesn't implement any bootblock_pre_c_entry stuff like the newer Intel devices.


https://review.coreboot.org/#/c/18439/7/src/cpu/amd/pi/verstage.c
File src/cpu/amd/pi/verstage.c:

Line 19: void cache_as_ram_stage_main(void)
> Why isn't src/arch/x86/verstage.c just used since it provides car_stage_ent
Same lack of CONFIG_C_ENVIRONMENT_BOOTBLOCK=y response as before.

> Are you using lib/bootblock.c for bootblock?

No, we're currently defaulting to BOOTBLOCK_SIMPLE and not the lib version.  Tell me if I'm reading it right...  That looks like we're probably missing gathering timestamps.  The lib/bootblock.c flavor only calls run_romstage() but verification still occurs because of the vboot_locator in cbfs.c, right?

> Or are you running each node bsp through the verstage?

I wasn't intentional about that, but yes the APs are executing the vboot code as they're brought up.  Does Google have a security recommendation regarding all cores running verstage_main()?  Or could this risk confusing the NV in CMOS?


-- 
To view, visit https://review.coreboot.org/18439
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie43d87908c2d83b42b95b306419156e85993f7bd
Gerrit-PatchSet: 7
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Marc Jones <marc at marcjonesconsulting.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki at gmail.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd at gmail.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes