Attention is currently required from: Arthur Heymans, Cliff Huang, David Milosevic, Lance Zhao, Martin L Roth, Maximilian Brune, Patrick Rudolph, Tim Wawrzynczak.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78071?usp=email )
Change subject: acpi: Add PPTT support ......................................................................
Patch Set 18:
(5 comments)
File src/acpi/acpi_pptt.c:
https://review.coreboot.org/c/coreboot/+/78071/comment/06eda3ee_47bcef0f : PS8, Line 13: * Disclaimer: : * : * The generated PPTT table might not be optimized for space, since this : * implementation creates unique resources (caches) for each CPU provided by : * the topology tree. Going by the ACPI 6.4 spec, this is fine. We do this to : * avoid further edge cases and keep the logic as simple as possible. : * : * "though less space efficient, it is also acceptable to declare a node : * for each instance of a resource. In the example above, it would be legal to : * declare an L1 for each processor." : * : * "Compaction of identical resources must be avoided if an implementation requires : * any resource instance to be referenced uniquely. For example, in the above example, : * the L1 resource of each processor must be declared using a dedicated structure to : * permit unique references to it." : *
Okay. […]
Marking this as resolved, further details can be discussed separately.
File src/acpi/acpi_pptt.c:
https://review.coreboot.org/c/coreboot/+/78071/comment/12b95de6_68c7f14c : PS17, Line 12: typedef
We are still within the ACPI subsystem, where typedefs are common. […]
I was also about to mention the pattern in acpigen code. However, typedefs there are usually for the structs that represent ACPI tables. So for this coreboot internal struct, I would also prefer *no* typedef. No strong feelings though.
https://review.coreboot.org/c/coreboot/+/78071/comment/0295564c_0f8d6902 : PS17, Line 74: u32 *n_caches
Forgot to mention, that we also need "n_caches" to index into "cache_refs". […]
I see no need for null-termination as reference_for_cache() already uses CONFIG_ACPI_PPTT_MAX_CACHES implicitly, by convention. So any loop over this array could use this globally known upper bound.
Adding a new entry to cache_refs[] would be less convenient, but still possible: search for the first `.cache != NULL`.
Alternatively, if we just want to simplify the signatures: Pack both into a single object ``` struct cache_list { cache_reference_t refs[CONFIG_ACPI_PPTT_MAX_CACHES]; unsigned int n_caches; }; ```
https://review.coreboot.org/c/coreboot/+/78071/comment/bcb483cc_db6e9134 : PS17, Line 131: cache_reference_t cache_refs[CONFIG_ACPI_PPTT_MAX_CACHES];
I decided against it, since it is harder to restrict the array size then. […]
They usually have the same kind of cache, but this is about individual instances of the caches, AIUI. Either the platform code would have to compact this already (re-use the same `struct pptt_cache` objects) or we would have to carefully deduplicate cache objects that have the exact same properties (recursively*), i.e. not compare pointers in reference_for_cache() but compare in depth (actually what we wanted to avoid in complexity, remember the disclaimer). To me, the current logic seems good enough.
*I wouldn't mind a full implementation, it looks like we have most of the complexity anyway. But we'd have to be careful. Beside comparing all the data in `struct pptt_cache`, we'd have to follow `.next_level` too, so something like this wouldn't get deduplicated by accident: ``` L1 (2KiB) L1 (2KiB) .next_level | .next_level | v v L2 (16KiB) L2 (18KiB) ``` (i.e. both L1 look the same, but point to different kinds of L2 caches)
File src/include/acpi/acpi.h:
https://review.coreboot.org/c/coreboot/+/78071/comment/f35ab5cc_45cd8b2e : PS3, Line 1442: structures
The current structures and code, however, don't express everything ACPI can do […]
Nothing on my end, but this is not my thread ;)
-
Nico Huber (Code Review)