Attention is currently required from: Hung-Te Lin, Rex-BC Chen, Jianjun Wang. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63251 )
Change subject: coreboot tables: Add PCIe info to coreboot table ......................................................................
Patch Set 8:
(3 comments)
File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/c/coreboot/+/63251/comment/efbeb354_6caf97e7 PS8, Line 91: struct cb_pcie pcie_info; Copying whole coreboot table entries in here isn't great (I guess we were lazy for the framebuffer because it's so big), it wastes space for `tag` and `size` entries that are no longer relevant here. You should define individual fields instead.
File src/commonlib/include/commonlib/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/63251/comment/4a153ce7_a0422a86 PS8, Line 176: uint64_t atu_base; /* Base address of Address translation unit */ You should really use struct lb_uint64 for all of these to avoid unintentional padding differences between architectures. Some of the existing records already do that wrong (and should probably be fixed) but they happen to get lucky with the alignment. You don't.
(The whole struct lb_uint64 mechanism is really ugly to work with, of course. I think the problem could be solved in a much cleaner way with something like `typedef __aligned(4) uint64_t lb_uint64_t;`, if someone was willing to spend the time to convert it all.)
File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/63251/comment/6123ea77_fb0612f7 PS8, Line 136: lb_pcie->config_size = pcie->config_size; Sorry, but something looks off here. You're just shuffling everything from the `pcie` argument into the coreboot table here, but in CB:63252 `pcie` is a stack allocation with only one initialized field. All those other fields would be leaking random stack garbage into the coreboot table. If you don't need those fields on that platform, they should be deterministically zeroed.
The whole call order here seems a bit weird, I'd try to follow the lb_gpios() model instead. So write_coreboot_table() would call
void lb_pcie(struct lb_header *header) { struct lb_pcie pcie = { .tag = LB_TAG_PCIE, .size = sizeof(pcie) };
if (fill_lb_pcie(&pcie) != CB_SUCCESS) return; memcpy(lb_new_record(header), &pcie, sizeof(pcie)); }
and then the platform can choose to implement fill_lb_pcie() to fill out the structure, or otherwise a default weak implementation would return CB_ERR.