Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46605 )
Change subject: lib/libpayload: Replace strapping_ids with new board configuration entry ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46605/9/payloads/libpayload/include... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/c/coreboot/+/46605/9/payloads/libpayload/include... PS9, Line 111: UNDEFINED_FW_CONFIG
Is the payload expected to test whether the fw_config is all 1s before treating it as a valid value?
Yep.
One special thing about fw_config compared to the other IDs is that it is a bit-field member and so the payload will have to be careful about treating all 1s as all 0s. Should we just set the UNDEFINED_FW_CONFIG as all 0s instead here?
We could, it means that you can't distinguish between a board that doesn't provide a source for FW_CONFIG (so this code will use UNDEFINED_) versus one that has it, but is defined as all 0s.
In our case, we'll have to check lib_sysinfo.fw_config one way or the other in the payload, to know which case is the "I'm empty, enable everything" default case.
https://review.coreboot.org/c/coreboot/+/46605/9/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/46605/9/src/lib/coreboot_table.c@a2... PS9, Line 222: if (bid == UNDEFINED_STRAPPING_ID)
One slight change in behavior with the new code is that all the IDs are now added to coreboot tables […]
I don't believe so; libpayload parses the entire table and all of the entries found into fields in the `lib_sysinfo` object. I don't think the payload is supposed to care whether an entry exists or not, only the values in lib_sysinfo.