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

[Angel Pons (Code Review)]

Attention is currently required from: Arthur Heymans, Cliff Huang, David Milosevic, Lance Zhao, Martin L Roth, Maximilian Brune, Nico Huber, Patrick Rudolph, Tim Wawrzynczak.

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78071?usp=email )

Change subject: acpi: Add PPTT support
......................................................................

Patch Set 21:

(6 comments)

File src/acpi/acpi_pptt.c:

https://review.coreboot.org/c/coreboot/+/78071/comment/66093488_05c550b0 :
PS17, Line 12: typedef
> IMO, it's highly subjective. Upstream, code is more often read than worked on. […]

I would prefer *not* to use typedefs for structs either, as they hide the fact that the underlying type is a struct.

Plus, if a function's signature is too long, you can spread the parameter list of a function over several lines. If you have too many parameters, consider passing a pointer to a struct instead.

https://review.coreboot.org/c/coreboot/+/78071/comment/35c482c2_cb4ee13d :
PS17, Line 131: cache_reference_t cache_refs[CONFIG_ACPI_PPTT_MAX_CACHES];
> As promised, here is the platform code https://review.coreboot. […]

Note: I may have mixed up "caches shared by several processors" and "several processors with the same kind of cache". The grouping approach is for shared caches only.

If I understand correctly, the problem here is that cache information can be the same across some but not all processors (e.g. big.LITTLE), and such redundancy should preferrably be avoided.

Looks like https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/05_ACPI_Software_Programming_M... says that the "processor hierarchy node structure (Type 0) [...] can be used to describe a single processor or a group". Looks like the code doesn't make use of this yet?

Processors with a shared cache must be grouped to accurately reflect reality. This grouping could already be done in `acpi_get_pptt_topology()`, i.e. the data passed to the PPTT serializer (this code).

There's a few things to keep in mind:

- The processor info needs to match that of MADT, not sure if MADT accounts for processor groups.
 - It is assumed that whoever defines `acpi_get_pptt_topology()` can figure out how to group the processors better than common code (e.g. soc/rockchip/rk3399 knows the chip has 2x Cortex-A72 and 4x Cortex-A53, common code doesn't).

[It was at this point that I realized I had mixed up both types of caches.]

As for private resources, they need to appear in every individual type 0 (processor) entry. Assuming SoC code is sane, the PPTT serializer could just compare pointers to check if they're the same structure. This works as long as the data structures returned by `acpi_get_pptt_topology()` are not modified at all during their serialization into ACPI PPTT.

If one processor has L1->L2->0 and another processor only has L2->0 (where 0 means NULL and L2 is the exact same data), one could try to reuse parts of the first processor's L1->L2-0 data for the second processor. This can also be done by comparing pointers and will work provided that `acpi_get_pptt_topology()` also reuses said pointers.

File src/acpi/acpi_pptt.c:

https://review.coreboot.org/c/coreboot/+/78071/comment/a5260444_a09284c4 :
PS21, Line 148: 		cpu_node->resources[cpu_node->n_resources++] = new_pptt_cache(current, it->cache, cache_list);
In https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/05_ACPI_Software_Programming_M... figure 5.15, the type 1 (cache) entries being placed above the type 0 (processor) entries.

Not sure if it matters, but writing down all the type 1 (cache) entries before the type 0 (processor) entries seems to make things like reusing entries easier to implement.

File src/include/acpi/acpi.h:

https://review.coreboot.org/c/coreboot/+/78071/comment/d7b800c7_2a1c40dc :
PS21, Line 1405: PPTT_PROCESSOR_FLAG_*
Macros do not exist

https://review.coreboot.org/c/coreboot/+/78071/comment/839ae3b0_bfa84f21 :
PS21, Line 1416: PPTT_CACHE_FLAG_*
Macros do not exist

https://review.coreboot.org/c/coreboot/+/78071/comment/369ac393_5c02b8f5 :
PS21, Line 1448: 	u32 size_valid          : 1;
               : 		u32 n_sets_valid        : 1;
               : 		u32 associativity_valid : 1;
               : 		u32 alloc_type_valid    : 1;
               : 		u32 cache_type_valid    : 1;
               : 		u32 write_policy_valid  : 1;
               : 		u32 line_size_valid     : 1;
               : 		u32 cache_id_valid      : 1;
               : 		u32 reserved            : 24;
Are bitfields portable? Or can they break depending on endianness?