Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35349 )
Change subject: drivers/intel/fsp2_0: Allocate cfg_region_size for UPD ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35349/2/src/drivers/intel/fsp2_0/si... File src/drivers/intel/fsp2_0/silicon_init.c:
https://review.coreboot.org/c/coreboot/+/35349/2/src/drivers/intel/fsp2_0/si... PS2, Line 43: (hdr->cfg_region_size)
I could go either way. […]
Passing NULL is legal, but the cfg when it is NULL should be coming from this region specifically. If it is 0 in size I'd say that's a bigger problem with expectations that we'd need to sort out in the future. Alternatively, if sizeof(FSPS_UPD) == 0 then cfg_region_size should also be 0.
https://review.coreboot.org/c/coreboot/+/35349/2/src/drivers/intel/fsp2_0/si... PS2, Line 48: printk(BIOS_ERR, "FSP error: more UPD specified than allowed by header cfg_region_size\n");
Shouldn't this just die() here?
die() is fine w/ me. Everything's undefined at this point since we don't have matching object sizes to start from.
I think we can explicitly comment that we aren't supporting running with whatever defaults were baked into the fsp blob. In practice, those defaults are typically wrong for the specific platform. We can certainly add an option in the future if that use-case is picked up (by people using bct tool to modify the blob).