Attention is currently required from: Arthur Heymans, Cliff Huang, Lance Zhao, Martin L Roth, 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 18:
(6 comments)
File src/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/78071/comment/1f4be706_77c691dd : PS17, Line 1204: /* --- header generation --- */ :
Useless comments?
Done
File src/acpi/acpi_pptt.c:
https://review.coreboot.org/c/coreboot/+/78071/comment/95835cb5_6a7ed327 : PS17, Line 12: typedef
typedefs are against the style guide.
We are still within the ACPI subsystem, where typedefs are common. Would not it be inconsistent to drop them? We dropped the typedefs for the user structs (pptt_topology, ...), since they are used externally, but those typedefs are internal only.
https://review.coreboot.org/c/coreboot/+/78071/comment/256a9ab4_5ab6dbfc : PS17, Line 23: = 0x0
static variables are always initialised at 0.
Doesnt hurt to make it explicit, though?
https://review.coreboot.org/c/coreboot/+/78071/comment/13ddf98e_314d7524 : PS17, Line 74: u32 *n_caches
Do you need this? you memset cache_refs to 0. […]
Thought about this, but then we would have to add an additional element as terminator at the end. Otherwise we would have problems if we search a cache_ref buffer that is full (reading out of bounds). Technically, this is not a problem, but I would like to keep the array as small as possible, since we are in a recursion here.
On the other hand, it would be very nice to simplify the signature.
If you still think it would be better that way, I can change it :)
https://review.coreboot.org/c/coreboot/+/78071/comment/d612e606_6f89e52f : PS17, Line 131: cache_reference_t cache_refs[CONFIG_ACPI_PPTT_MAX_CACHES];
Is this not more appropriate to declare this in setup_topology so that all siblings can reuse it? No […]
I decided against it, since it is harder to restrict the array size then. We would have to limit the array size based on the number of CPUs times the number of caches per CPU. This would then eat the stack because of recursion. Basically, MAX_CPUS*MAX_CACHES_PER_CPU*(16 bytes for the struct)*MAX_DEPTH_OF_TREE.
https://review.coreboot.org/c/coreboot/+/78071/comment/5c865c25_7854e2de : PS17, Line 132: memset(cache_refs, 0x0, sizeof(cache_reference_t) * CONFIG_ACPI_PPTT_MAX_CACHES); :
The ideomatic way in C is "= {}" in the declaration of the variable.
Done