Attention is currently required from: Arthur Heymans, Benjamin Doron, David Milosevic, Patrick Rudolph.
Angel Pons has posted comments on this change by David Milosevic. ( https://review.coreboot.org/c/coreboot/+/79108?usp=email )
Change subject: mb/emulation/qemu-sbsa: Generate PPTT ACPI table ......................................................................
Patch Set 7:
(4 comments)
File src/mainboard/emulation/qemu-sbsa/pptt.c:
https://review.coreboot.org/c/coreboot/+/79108/comment/d1e28884_f1e8f2b0?usp... : PS7, Line 9: #define CACHE_NODE_FLAGS 0xd7 // everything valid except, write-policy and allocation type : #define CLUSTER_FLAGS 0x11 // physical package, ID invalid, no thread, no leaf, identical impl. : #define CORE_FLAGS 0x1a // no physical package, ID valid, no thread, leaf, identical impl. : : #define CACHE_ATTR_TYPE_DATA (0) : #define CACHE_ATTR_TYPE_INSTRUCTION (0x1 << 2) : #define CACHE_ATTR_TYPE_UNIFIED (0x1 << 3)
Done […]
+1, please use the structs/bitfields if possible. If bitfields cannot possibly work properly because of implementation defined behaviour, then the bitfields are misleading and should not exist (or be fixed somehow, if possible)
https://review.coreboot.org/c/coreboot/+/79108/comment/3aaf8402_c4ab68fb?usp... : PS7, Line 19: static struct pptt_cache l2 = { Why is this static? For lifetime reasons, I imagine?
If so: I'm not too fond of this. I'm not sure if there are any drawbacks but I'd consider having these as static globals outside the function instead.
https://review.coreboot.org/c/coreboot/+/79108/comment/2e690bb3_0952e04b?usp... : PS7, Line 47: static struct pptt_topology root_topology = { : .flags.raw = CLUSTER_FLAGS, : .resources = NULL, : .sibling = NULL, : .child = NULL // updated in runtime : }; You can move this below `static struct pptt_topology entries[CONFIG_MAX_CPUS] = {};` and directly set `.child = entries,` in the struct initialiser.
https://review.coreboot.org/c/coreboot/+/79108/comment/182af4bf_33f93b8d?usp... : PS7, Line 54: struct cache_info info; : : /* update cache information (L1I) */ : : cpu_get_cache_info(CACHE_L1, CACHE_INSTRUCTION, &info); : : l1i.size = info.size; : l1i.associativity = info.associativity; : l1i.numsets = info.numsets; : l1i.line_size = info.line_bytes; Would it make sense to encapsulate this in a function?
``` static void fill_pptt_cache_info(const enum cache_level level, const enum cache_type type, struct pptt_cache *cache) { struct cache_info info; cpu_get_cache_info(level, type, &info); cache->size = info.size; cache->associativity = info.associativity; cache->numsets = info.numsets; cache->line_size = info.line_bytes; } ```