Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30686 )
Change subject: soc/intel/fsp1.1: Implement postcar stage ......................................................................
Patch Set 28:
(5 comments)
https://review.coreboot.org/#/c/30686/28/src/drivers/intel/fsp1_1/exit_car.S File src/drivers/intel/fsp1_1/exit_car.S:
https://review.coreboot.org/#/c/30686/28/src/drivers/intel/fsp1_1/exit_car.S... PS28, Line 23: andl $0xfffffff0, %esp The stack needs to be 16 byte aligned upon entry into C. There need to be 3 pushes prior to push %ebx. And similar on the pop for stack balance.
https://review.coreboot.org/#/c/30686/28/src/drivers/intel/fsp1_1/exit_car.S... PS28, Line 28: pop %ebx What is the purpose of ebx saving? Isn't the stack contents migrated? If not, who is managing the value?
https://review.coreboot.org/#/c/30686/28/src/drivers/intel/fsp1_1/romstage.c File src/drivers/intel/fsp1_1/romstage.c:
https://review.coreboot.org/#/c/30686/28/src/drivers/intel/fsp1_1/romstage.c... PS28, Line 95: #if IS_ENABLED(CONFIG_HAVE_SMI_HANDLER) Can't we make this a normal if() ?
https://review.coreboot.org/#/c/30686/28/src/drivers/intel/fsp1_1/temp_ram_e... File src/drivers/intel/fsp1_1/temp_ram_exit.c:
https://review.coreboot.org/#/c/30686/28/src/drivers/intel/fsp1_1/temp_ram_e... PS28, Line 23: fih = NULL; if we're dying why set fih to NULL?
https://review.coreboot.org/#/c/30686/28/src/drivers/intel/fsp1_1/temp_ram_e... PS28, Line 28: fih = find_fsp((uintptr_t)rdev_mmap_full(prog_rdev(&fsp))); Should we sanity check fih is not bad?