Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46312 )
Change subject: soc/intel/common: Move CSE RW into new FMAP region to optimize boot time ......................................................................
Patch Set 11:
(33 comments)
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... File src/soc/intel/common/block/cse/Kconfig:
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 33: FW_MAIN_A
CSE_RW_A
ME_RW_A (inline with SI_ME) will be used for consistency
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 40: FW_MAIN_B
CSE_RW_B
ME_RW_B will be used for consistency
https://review.coreboot.org/c/coreboot/+/46312/9/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/46312/9/src/soc/intel/common/block/... PS9, Line 159: struct cse_cbfs_rw_info {
metadata structure is already defined in https://review.coreboot. […]
Ack
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 47: CSE_LITE_SKU_NO_ERROR = 0,
nit: blank line after this to match the other entries in the enum.
Ack
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 49: = 1,
General comment: Why do we need to manually assign numbers to each of the enums? It should be fine t […]
True, I assigned numbers manually so as to quickly identify value to enum definition.
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 73: 9
nit: `9,`
Ack
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 159: struct cse_cbfs_rw_info {
This seems a duplicate definition of struct cse_rw_metadata. […]
Ack
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 159: struct cse_cbfs_rw_info {
This structure definition is not required. This is exactly the same as `struct cse_rw_metadata`. […]
Ack
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 166: authenticness
authenticity
structure is removed.
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 167: containing precalculated
containing a precalculated
structure is removed.
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 167: of CSE
of the CSE
structure is removed.
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 169: Fimrware
firmware
structure is removed.
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 436: cse_get_cbfs_rdev
This function can now be completely dropped. See my comments below.
Ack
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 523: if the CSE CBFS RW placed outside : * of FW_MAIN_A and FW_MAIN_B.
This is currently always true. I don't think this comment is required.
Ack
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 535: return NULL;
deadcode.
Ack
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 542: static
Why static?
Static is not required. Thanks
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 562: (void *)
unnecessary cast, any pointer type can automatically cast to `void *`
The function is removed.
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 568: printk(BIOS_SPEW, "cse_lite: CSE CBFS RW version : %d.%d.%d.%d\n", : source_info->fw_ver.major, : source_info->fw_ver.minor, : source_info->fw_ver.hotfix, : source_info->fw_ver.build);
Logged again in line 594. So not required.
The function is removed
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 576: for (uint8_t i = 0; i < VB2_SHA256_DIGEST_SIZE; i++) : printk(BIOS_SPEW, "%x", source_info->hash[i]); : : printk(BIOS_SPEW, "\n");
Do we need to log SHA Digest? Not sure how it helps with debugging.
The function is removed
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 590: source_info
cse_rw_metadata
source_info or source_metadata?
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 601: : if (source_info->fw_ver.major != cse_rw_ver->major) : return source_info->fw_ver.major - cse_rw_ver->major; : else if (source_info->fw_ver.minor != cse_rw_ver->minor) : return source_info->fw_ver.minor - cse_rw_ver->minor; : else if (source_info->fw_ver.hotfix != cse_rw_ver->hotfix) : return source_info->fw_ver.hotfix - cse_rw_ver->hotfix; : else : return source_info->fw_ver.build - cse_rw_ver->build;
This should be changed to: […]
memcmp() does a byte wise comparison.So, it doesn't work here. For example, when comparing two CSE Kit versions (having build numbers CBFS RW blob# 954, RW partition# 1234,), memcmp() compares first byte in the build numbers and returns 20. So, we can't use memcmp here.
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 626: is not matching with
"does not match"
Ack
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 630: with
remove this word
Ack
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 725: exit_rw_update:
this label is now obsolete; the above gotos can just be changed to `return false`
Ack
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 764: *source_info
cse_rw_metadata
source_info or source_metadata?
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 768: cse_get_cbfs_rw
Probably name cse_get_source_rdev()?
Ack
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 771: enum csme_failure_reason rv;
Place all the declarations at the start of the function.
Ack
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 772: rdev_mmap(&source_rdev, 0, region_device_sz(&source_rdev));
rdev_mmap_full(&source_rdev);
Ack
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 773:
Ack
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 794: const struct region_device *source_info_rdev
This parameter is not required at all with the Kconfig check in caller.
Ack
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 803: struct cse_cbfs_rw_info source_info; : : /* Read CSE CBFS RW metadata */ : if (!cse_get_cbfs_rw_metadata(source_info_rdev, &source_info)) { : printk(BIOS_ERR, "cse_lite: Failed to get CSE RW blob metadata\n"); : return CSE_LITE_SKU_UNSPECIFIED; : }
This can be simplified as mentioned below: […]
Ack
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 811: &source_info
This will then change to `&cse_rw_metadata`.
Ack
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 851: struct region_device source_info_rdev; : if (cse_get_cbfs_rdev(&source_info_rdev))
Things actually become much simpler now that the CBFS file is added as a struct. […]
Ack