[L] Change in coreboot[main]: acpi: Add PPTT support

[Nico Huber (Code Review)]

Attention is currently required from: Arthur Heymans, Cliff Huang, David Milosevic, Lance Zhao, 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 8:

(7 comments)

Patchset:

PS8: 
This is nice code overall. Please don't mind the long comments,
these ACPI things are sometimes hard to get right from the start.

File src/acpi/acpi_pptt.c:

https://review.coreboot.org/c/coreboot/+/78071/comment/892443d4_4e280bd2 :
PS6, Line 66:
> Is that really necessary? I mean, Jenkins does not complain and it really is only a matter of prefer […]

I agree with Arthur here.

While the style guide is not explicit about this, it does argue against
unnecessary empty lines. It's also uncommon enough to make readers, who
are used to the kernel (and coreboot) style, stumble (for maybe a second,
I was wondering why Gerrit shows the code like this).

File src/acpi/acpi_pptt.c:

https://review.coreboot.org/c/coreboot/+/78071/comment/7835e789_1f4fdeee :
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
that were not quoted, hence provide no value without reading the actual spec.
My first impression was that this is about listing, for instance, shared caches
more than once. But it is not. What the spec describes is the possibility to
compact individual resources that have the exact same characteristics.

There is a related quirk that is, IMO, much more worth to mention: The current
structures don't allow to converge lists (cf. Fig. 5.15: Cache Type Structure -
Type 1 Example; here, L1D and L1I should point to the same L2 object as next
level). I'm not sure if that is something for a comment or something to fix.

https://review.coreboot.org/c/coreboot/+/78071/comment/67a4c1f0_0449eecf :
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
more than that "References are the offset from the start of the PPTT table.".
Drawing the picture here might make things more confusing, IMO, but I don't
mind.

https://review.coreboot.org/c/coreboot/+/78071/comment/660304db_ae4122d3 :
PS8, Line 56: u32
It's uncommon to use a fixed-size integer unless you need one (e.g. for
an external interface). Simply `unsigned int` would be preferred.

https://review.coreboot.org/c/coreboot/+/78071/comment/f45fc417_e93d239b :
PS8, Line 127: 	setup_topology(node->sibling, parent_ref, current);
This is just walking the list without any reference to the current
setup_topology()'s context, right? Recursion is generally something
that should only be used if necessary. And if we can avoid it here,
we should.

You can probably see clearly what is going on here, I assume Arthur
and Patrick, too. But the next person who gets to edit this code in
a year or so might not. And that's when bugs sneak in when the code
isn't written as simple as possible.

I hope I'm not wrong with this. Could this be written as follows,
with only one recursion where necessary?
```
static void setup_topology(const struct pptt_topology *node, const u32 parent_ref, unsigned long *current)
{
        for (; node; node = node->sibling) {
                const u32 cpu_ref = new_pptt_cpu(current, node, parent_ref);
                setup_topology(node->child, cpu_ref, current);
        }
}
```

File src/include/acpi/acpi.h:

https://review.coreboot.org/c/coreboot/+/78071/comment/369a4823_562fd0df :
PS3, Line 1442: structures
> > It looks like other platforms do not have custom data structures and instead call a function to le […]

I first leant to agree with Arthur, but it seems more delicate than one might
think. What we would want here is to design structures that are nicer to handle
in C but can still express the same as the flat ACPI ones, right?

The current structures and code, however, don't express everything ACPI can do
(cf. comment on the "Disclaimer" comment in the source). So it seems possible,
that if we don't get it right from the start, the next developer adding a
different CPU with different topology might have to overhaul the whole code
again...

Also, reusing the flags values creates redundancies. For instance, `is_leaf`
is something that is also expressed by a NULL-pointer in the structures.

To wrap this up, I don't mind merging it in this state. Just want to make it
obvious, that it won't be correct from the start unless we invest a lot more,
also to avoid to tailor this to a specific CPU. It's usually hard to get without
multiple, different, real-world examples (and right now we don't even have one
under review, AFAICS). If it's worth to even try, IDK.