Hi Alireza,
On 8/27/25 11:21, Alireza Sanaee wrote:
Add two functions one of which finds the lowest level cache defined in
Maybe s/lowest level cache/lowest cache level/?
the cache description input, and the other checks if caches are defined
at a particular level.
Maybe improve the comment message with smtg like "if a given cache topology is
defined
at a particular cache level"? For reviewing this series I'm sticking with
the term "cache level" to mean the levels as in L1, L2, etc., and with the term
"topology level" to mean "thread", "core", "module", etc.
Signed-off-by: Alireza Sanaee <[email protected]>
---
hw/core/machine-smp.c | 56 +++++++++++++++++++++++++++++++++++++++++++
include/hw/boards.h | 5 ++++
2 files changed, 61 insertions(+)
diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 0be0ac044c..32f3e7d6c9 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -406,3 +406,59 @@ bool machine_check_smp_cache(const MachineState *ms, Error
**errp)
return true;
}
+
+/*
+ * This function assumes l3 and l2 have unified cache and l1 is split l1d and
+ * l1i.
+ */
Please, let's pick a form to write Ln cache and try to be consistent using it.
I prefer L1, L1d, etc. Here you're using l3, l2 form but in the comment on
machine_defines_cache_at_topo_level just below you use L2, L3. Also to be
observed
in the other patches in this series.
+bool machine_find_lowest_level_cache_at_topo_level(const MachineState *ms,
+ int *level_found,
+ CpuTopologyLevel topo_level)
"level_found" sounds a bit like a bool? how about "lowest_cache_level" instead?
+{
+
+ CpuTopologyLevel level;
+
+ level = machine_get_cache_topo_level(ms, CACHE_LEVEL_AND_TYPE_L1I);
+ if (level == topo_level) {
+ *level_found = 1;
+ return true;
+ }
+
+ level = machine_get_cache_topo_level(ms, CACHE_LEVEL_AND_TYPE_L1D);
+ if (level == topo_level) {
+ *level_found = 1;
+ return true;
+ }
+
+ level = machine_get_cache_topo_level(ms, CACHE_LEVEL_AND_TYPE_L2);
+ if (level == topo_level) {
+ *level_found = 2;
+ return true;
+ }
+
+ level = machine_get_cache_topo_level(ms, CACHE_LEVEL_AND_TYPE_L3);
+ if (level == topo_level) {
+ *level_found = 3;
+ return true;
+ }
+
+ return false;
+}
hmm it's a bit unfortunate that the cache level structure we use in QEMU
(actually it's a simple enum) doesn't allow us to obtain promptly the cache
level itself. Maybe, although just a slight enhancement, would be better to
iterate over the cache levels defined in QEMU and just set the cache level
for the separate cache levels, like:
enum CacheLevelAndType cache_level;
enum CpuTopologyLevel t;
for (cache_level = CACHE_LEVEL_AND_TYPE_L1D;
cache_level < CACHE_LEVEL_AND_TYPE__MAX;
cache_level++) {
t = machine_get_cache_topo_level(ms, cache_level);
if (t == topo_level) {
/* Assume L1 is split into L1d and L1i caches. */
if (cache_level == CACHE_LEVEL_AND_TYPE_L1D ||
cache_level == CACHE_LEVEL_AND_TYPE_L1I) {
*lowest_cache_level = 1; /* L1 */
} else {
/* Assume the other caches are unified. */
*lowest_cache_level = cache_level;
}
return true;
}
}
return false;
That won't avoid adding the separate caches in this function if new ones
are added to QEMU but will avoid adding the unified ones. Wdyt?
+/*
+ * Check if there are caches defined at a particular level. It supports only
+ * L1, L2 and L3 caches, but this can be extended to more levels as needed.
+ *
+ * Return True on success, False otherwise.
+ */
+bool machine_defines_cache_at_topo_level(const MachineState *ms,
+ CpuTopologyLevel level)
How about using "topology" for CpuTopologyLevel var. instead of "level". We use
level in "cache level" and in "topology level" currently, so I think it's better
being more specific here.
+{
+ if (machine_get_cache_topo_level(ms, CACHE_LEVEL_AND_TYPE_L3) == level ||
+ machine_get_cache_topo_level(ms, CACHE_LEVEL_AND_TYPE_L2) == level ||
+ machine_get_cache_topo_level(ms, CACHE_LEVEL_AND_TYPE_L1I) == level ||
+ machine_get_cache_topo_level(ms, CACHE_LEVEL_AND_TYPE_L1D) == level) {
+ return true;
+ }
+ return false;
+}
How about checking for all the levels defined in QEMU now and in future? That
looks
possible, like:
enum CacheLevelAndType cache_level;
for (cache_level = CACHE_LEVEL_AND_TYPE_L1D;
cache_level < CACHE_LEVEL_AND_TYPE__MAX;
cache_level++) {
if (machine_get_cache_topo_level(ms, cache_level) == topology) {
return true;
}
}
return false;
?
Cheers,
Gustavo
diff --git a/include/hw/boards.h b/include/hw/boards.h
index f94713e6e2..3c1a999791 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -55,6 +55,11 @@ void machine_set_cache_topo_level(MachineState *ms,
CacheLevelAndType cache,
CpuTopologyLevel level);
bool machine_check_smp_cache(const MachineState *ms, Error **errp);
void machine_memory_devices_init(MachineState *ms, hwaddr base, uint64_t
size);
+bool machine_defines_cache_at_topo_level(const MachineState *ms,
+ CpuTopologyLevel level);
+bool machine_find_lowest_level_cache_at_topo_level(const MachineState *ms,
+ int *level_found,
+ CpuTopologyLevel
topo_level);
/**
* machine_class_allow_dynamic_sysbus_dev: Add type to list of valid devices
