Attention is currently required from: Arthur Heymans, Cliff Huang, Lance Zhao, Maximilian Brune, Nico Huber, Patrick Rudolph, Tim Wawrzynczak.
David Milosevic has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78071?usp=email )
Change subject: acpi: Add PPTT support ......................................................................
Patch Set 9:
(7 comments)
File src/acpi/acpi_pptt.c:
https://review.coreboot.org/c/coreboot/+/78071/comment/166f1ef1_775ae89f : PS6, Line 66:
I agree with Arthur here. […]
Guess I am outnumbered here. I will drop the newline.
File src/acpi/acpi_pptt.c:
https://review.coreboot.org/c/coreboot/+/78071/comment/fdde42ee_86291740 : 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." : *
I found this to be more confusing than helpful. The quotes reference examples […]
With this comment I wanted to explain, that there is still room for optimizations regarding the table size, since the current approach creates unique structures instead of compacting them, in order to reduce code complexity.
The quotes simply serve as proof, that this is, nevertheless, still valid ACPI.
Quote 1 allows us to declare unique resources instead of compacting them into a single resource.
Quote 2 states, that there might be cases, where we really need unique resources (in order to have unique references), and therefore, have to avoid compaction.
By leveraging Quote 1, we avoid dealing with potential edge cases (see Quote 2), as we completely avoid resource compaction.
Regarding converging cache linked lists: I will take a look. Its probably something we would have to implement in order to be spec compliant.
https://review.coreboot.org/c/coreboot/+/78071/comment/987bd2f0_b144b690 : PS8, Line 40: * the difference between the start of the PPTT table
Nit, I would call it `offset`. And TBH, I don't think we need to say […]
I agree, the diagram is not really needed for such a simple concept, but was requested by Arthur, so I think we should keep it. I will adjust the description though.
https://review.coreboot.org/c/coreboot/+/78071/comment/e224b057_1595d51d : PS8, Line 56: u32
It's uncommon to use a fixed-size integer unless you need one (e.g. for […]
Although we don't populate the PPTT table directly with the return value of "count_resources", it is still based on the "Number of private resources" field (acpi_pptt_cpu_node_t.n_resources) which is exactly 4 bytes wide. That's why I chose "u32".
https://review.coreboot.org/c/coreboot/+/78071/comment/257e460d_9f3b2303 : PS8, Line 127: setup_topology(node->sibling, parent_ref, current);
This is just walking the list without any reference to the current […]
To be honest, I find it harder to understand/read when we mix recursion with loops. Not sure, if this is an improvement. Maybe a third opinion would be nice?
File src/include/acpi/acpi.h:
https://review.coreboot.org/c/coreboot/+/78071/comment/7598f8e4_e077332a : PS4, Line 1412: acpi_pptt_cpu_node
If you don't mind, I would mark this comment as done by end of tomorrow.
Done
https://review.coreboot.org/c/coreboot/+/78071/comment/f3fc7af2_db22f6b5 : PS4, Line 1416: u
If you don't mind, I would mark this comment as done by end of tomorrow.
Done
-
David Milosevic (Code Review)