On 04/18/2016 08:17 PM, Julius Werner wrote:
For comparison, the coreboot device tree interface on ARM (https://lkml.org/lkml/2014/6/16/622, unfortunately never picked up by the maintainers but still used on Chromebooks today) only exports start address and size of the coreboot table and the whole CBMEM area, so maybe ACPI should just match that. (I guess if CBMEM is referenced by the coreboot table like Aaron said, we could also drop that part.)
Did you get other feedback than the one mail by Rob Herring? We should keep pushing this.
No, I just never heard from them again (as I so often do trying to work with upstream Linux), got frustrated and gave up. Maybe we should try again... another two years of Chromebooks shipped with that interface might now convince them that there's no point bikeshedding the tiniest naming details anymore.
Here's the 1 million dollar question: Do we want to get rid of coreboot table and only have a coreboot> specific table?
I don't know enough about how ACPI tables look like to comment on the technical aspects for this, but please also keep the legacy support costs in mind when considering to change the whole format. There are millions of Chromebooks out there which still produce the old tables (they could be updated, but that would probably not be worth the effort), so the kernel driver and the cbmem utility would probably need to be able to detect and parse the old format anyway. At that point, haven't we just made it more complicated without really gaining anything?
This is somewhat orthogonal, but the coreboot tables are not really defined all that well. There's no specific endianness for each of the fields aside from whatever the endianness the CPUs was in during the time of the write. The structs used are also not marked as packed so there's not much of a guarantee on field alignment -- especially as they are allocated one after another within the table itself.
I think the alignment is actually standardized by C (both start and size get aligned to the largest-width data type in the struct). Since it looks like we always used explicit-width types and always wrote the structs in a way that avoids any need for padding, it should be pretty well-defined and match the reader's expectation. We just need to continue paying attention to that when we add entry types... marking the structs as packed shouldn't change anything but reduce performance. (There are a few hazards for unaligned uint64_t values, but we're already making sure our architectures can handle that for CBFS anyway.)
For the endianness issue, I would suggest to just declare that all the fields which aren't explicit right now should be little-endian (both in the coreboot tables and CBFS), and reflect that in the code the next time we try to add a big-endian architecture. It's unfortunate that some of the CBFS fields started out locked to the wrong endianness (and maybe we could even change that at some point), but little-endian rules the world these days and it's easiest to just use it throughout.
-
Julius Werner