Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41388 )
Change subject: acpi: add a function to read a table and verify it ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41388/2/src/acpi/acpi.c File src/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/41388/2/src/acpi/acpi.c@268 PS2, Line 268: file = acpi_read_table(CONFIG_CBFS_PREFIX "/madt.aml", "APIC");
so, for now, I'd rather not take on fixing all those problems, but if I take it as an action to fix them *along with this one* can we proceed?
Which problems?
What's the recommended function to use here, btw? I'm happy to make that fix everywhere it's used in this file.
You should do a rdev_mmap() -> rdev_read() -> rdev_unmap() in the acpi_read_table() function. That function should contain the memmove() itself instead of separating the functionality. The semantics that you want are "try and fill in this table from cbfs file". And this function should be used for all the tables accessing cbfs.
What's your favorite new config variable name? I'm not a huge fan of continuing to add more config variables but if it's seen to be needed I guess we can do it.
It helps cull the bloat and surface are. Not having Kconfigs definitely allows ease of use by just adding files to cbfs, but it does bring that surface area. This is my opinion, of course.