Change in coreboot[master]: soc/intel/common: Add support for CSE IOM/NPHY sub-parition update

[Julius Werner (Code Review)]

Attention is currently required from: Tim Wawrzynczak, Sridhar Siricilla, Krishna P Bhat D.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59685 )

Change subject: soc/intel/common: Add support for CSE IOM/NPHY sub-parition update
......................................................................

Patch Set 13:

(9 comments)

Patchset:

PS11:
> Do to urgency of this feature, we would like to update this in a subsequent patch, as it would requi […]

Well, what would you prefer: that you make this change now and test it on your hardware, or that I make this change in your code later blindly because I have no hardware to test it on? Besides, "we need this urgently" should never be a valid reason to reject review feedback on coreboot ToT. If individual companies have schedules to keep, they're free to build their temporary code in local branches while the upstream discussion is still ongoing. There should never be a need to rush unfinished code into the master branch.

Honestly, looking closer at what you're doing here, cbfs_locate_file_in_region() is absolutely not the right approach for that to begin with anyway. You're not trying to access a custom CBFS (like the other case that's looking into the actual CSE FMAP section), you're just reading the default boot CBFS here. You're always supposed to use the main cbfs_load()/cbfs_map() APIs for that, not manually dig through vboot internals and hardcode the matching FMAP sections yourself. Here, I'll point out in other comments how this code should be changed...

File src/soc/intel/common/block/cse/cse_lite.c:

https://review.coreboot.org/c/coreboot/+/59685/comment/4dd7a9fd_86fa5752 
PS13, Line 406: __weak bool is_cse_sub_part_update_req(void)
This doesn't seem like a good use case for weak functions (see CB:58934 for some ongoing discussion about where weak functions are appropriate and where they aren't). You already have a Kconfig for this feature, so it doesn't make sense to default to not using it on boards that have that Kconfig enabled unless they also implement some extra function. I would either flip this to default to doing the update (i.e. name it `bool skip_cse_sub_part_update()` and the update always happens on platforms with this Kconfig unless they explicitly implement this function and return true from it), or make the function non-weak so that every platform with the Kconfig is forced to implement it.

https://review.coreboot.org/c/coreboot/+/59685/comment/b40264dc_14b8c1d4 
PS13, Line 755: static ssize_t rdev_readat_offset(const struct region_device *rd, void *b, size_t offset,
Why are you redefining a well-known existing function with a different name, anyway? All this achieves is confuse people who are used to the existing name. Just use rdev_readat() directly.

https://review.coreboot.org/c/coreboot/+/59685/comment/2a2478c8_89993bfa 
PS13, Line 833: static const char *get_source_rdev_fmap_name(void)
Get rid of this function. Don't reimplement what vboot and the CBFS core are already doing for you.

https://review.coreboot.org/c/coreboot/+/59685/comment/1647e9cd_48e08e05 
PS13, Line 854: static bool cse_sub_part_get_source_rdev(struct region_device *rdev, const char *name)
Get rid of this function. The new CBFS API intentionally abstracts away from raw rdevs. Just map the whole file and use a pointer to that.

https://review.coreboot.org/c/coreboot/+/59685/comment/3b25b3ec_db33acb9 
PS13, Line 932: 	if (!cse_sub_part_get_source_rdev(&source_rdev, name))
If you want to get the `name` CBFS file from the currently active boot CBFS, just use

size_t size;
 void *subpart_cbfs_rw = cbfs_map(name, &size);
 if (!subpart_cbfs_rw)
   return ...;

https://review.coreboot.org/c/coreboot/+/59685/comment/f7de642b_08a71bcb 
PS13, Line 936: 	if (!cse_get_sub_part_fw_version(type, &source_rdev, &source_fw_ver))
Just pass the `subpart_cbfs_rw` pointer returned from the above in here, and add the appropriate offsets onto it when you dereference.

https://review.coreboot.org/c/coreboot/+/59685/comment/b05f97af_e66f8c8b 
PS13, Line 983: region_device_sz(&source_rdev)
Here you would just use the `size` variable that got filled out above by cbfs_map().

https://review.coreboot.org/c/coreboot/+/59685/comment/bdc3a107_563405a4 
PS13, Line 989: 	rdev_munmap(&source_rdev, subpart_cbfs_rw);
...and here you would use

cbfs_unmap(subpart_cbfs_rw);

instead.