[coreboot-gerrit] [M] Change in coreboot[main]: soc/intel/apollolake: Add support for IFWI Measured Boot

[Sean Rhodes (Code Review)]

Attention is currently required from: Andrey Petrov, Arthur Heymans, Werner Zeh.

Sean Rhodes has posted comments on this change by Sean Rhodes. ( https://review.coreboot.org/c/coreboot/+/65272?usp=email )

Change subject: soc/intel/apollolake: Add support for IFWI Measured Boot
......................................................................

Patch Set 51:

(13 comments)

File src/soc/intel/apollolake/Makefile.mk:

https://review.coreboot.org/c/coreboot/+/65272/comment/150f8517_9d0eee72?usp... :
PS50, Line 10: bootblock-y
> Maybe better ```bootblock-$(CONFIG_IFWI_MEASURED_BOOT) += measured_boot.c``` […]

Done

File src/soc/intel/apollolake/include/soc/fit.h:

https://review.coreboot.org/c/coreboot/+/65272/comment/83c292ad_e730148d?usp... :
PS50, Line 20: reserved;
> You compare this field in your code with 'sub_type'. […]

Done

File src/soc/intel/apollolake/include/soc/measured_boot.h:

https://review.coreboot.org/c/coreboot/+/65272/comment/959135a7_3bf51555?usp... :
PS50, Line 22:
> Why the TAB here and on the following lines?

Done

https://review.coreboot.org/c/coreboot/+/65272/comment/d923fbb6_e50390c5?usp... :
PS50, Line 23: 32
> Use the define ```SHA256_DIGEST_SIZE``` from tss_structures. […]

Done

File src/soc/intel/apollolake/measured_boot.c:

https://review.coreboot.org/c/coreboot/+/65272/comment/15635bec_c1b49101?usp... :
PS50, Line 15:
> The TABs are looking weird and there is no need to use them here at all. […]

Done

https://review.coreboot.org/c/coreboot/+/65272/comment/b889b97a_9e035628?usp... :
PS50, Line 16: uint64_t
> This should be uint32_t or? At least you are reading 32 bit into it and casting it to 32 bit later.

Done

https://review.coreboot.org/c/coreboot/+/65272/comment/6572872c_035379c1?usp... :
PS50, Line 19: read32((uint64_t *)(BASE_4GB - 0x40));
> Would it be sufficient to use 'fit_ptr' from the linker script (src/arch/x86/bootblock. […]

Not going to pretend I follow entirely, but seems to not break anything!

https://review.coreboot.org/c/coreboot/+/65272/comment/c5ef7fd8_3b2e1c72?usp... :
PS50, Line 38:
> Space instead of TAB.

Done

https://review.coreboot.org/c/coreboot/+/65272/comment/72e3ce7d_339b8561?usp... :
PS50, Line 64: 32
> Use the same define overall?

Done

https://review.coreboot.org/c/coreboot/+/65272/comment/c4a3dbb8_04469b8e?usp... :
PS50, Line 66: 0, TPM_ALG_SHA256
> So far VBOOT on APL uses PCR 0 with SHA1. […]

Adjusted preceding patch

https://review.coreboot.org/c/coreboot/+/65272/comment/988e68cd_18cfb400?usp... :
PS50, Line 67: sizeof(bpm_info->txe_hash) - 1)
> I am not following here. […]

Like that?

https://review.coreboot.org/c/coreboot/+/65272/comment/04180724_6f1acbb9?usp... :
PS50, Line 80: sizeof(bpm_info->ibbl_hash) - 1
> the above applies here as well.

Done

https://review.coreboot.org/c/coreboot/+/65272/comment/e4f66b14_3d6e760e?usp... :
PS50, Line 109: (sizeof(bpm_info->ibb_hash) - 1)
> And here.

Done