[RFC v2 0/7] Introduce SMP Cache Topology

[RFC v2 0/7] Introduce SMP Cache Topology

[Zhao Liu]

Hi,

Now that the i386 cache model has been able to define the topology
clearly, it's time to move on to discussing/advancing this feature about
configuring the cache topology with -smp as the following example:

-smp 32,sockets=2,dies=2,modules=2,cores=2,threads=2,maxcpus=32,\
     l1d-cache=core,l1i-cache=core,l2-cache=core,l3-cache=die

With the new cache topology options ("l1d-cache", "l1i-cache",
"l2-cache" and "l3-cache"), we could adjust the cache topology via -smp.

But in a recent discussion with Daniel, I think there's currently
uncertainty as to whether newly added parameters should have default
parameter values added or should be omitted by default...therefore, I
keep the "RFC" tag for this series.

For my difficulties with this OPEN, see the first section below ("Open
about How to Handle the Default Options").


This patch set is based on a little cleanup:
https://lore.kernel.org/qemu-devel/20240527131837.2630961-1-zhao1.liu@intel.com/

And you can find the RFC v1 there:
https://lore.kernel.org/qemu-devel/20240220092504.726064-1-zhao1.liu@linux.intel.com/

Since the ARM [1] and RISC-V [2] folks have similar needs for the cache
topology, I also cc'd the ARM and RISC-V folks and lists.

I've gone to explain the current problem much in the first section
below, and I appreciate your time your patience. Welcome your feedback!


Open about How to Handle the Default Options
============================================

(For the detailed description of this series, pls skip this "long"
section and review the subsequent content.)


Background of OPEN
------------------

Daniel and I discussed initial thoughts on cache topology, and there was
an idea that the default *cache_topo is on the CORE level [3]:

> simply preferring "cores" for everything is a reasonable
> default long term plan for everything, unless the specific
> architecture target has no concept of "cores".

The original purpose of considering *_cache_topo=core was to achieve
similar behavior like "parameters=1" supported by other CPU topology
options in -smp.

This way, even if machine doesn't support configuring cache topology (by
l1_separated_cache_supported/[l2|l3]_unified_cache_supported = false),
such a *_cache_topo=core can be treated as a valid parameter, except
that it will be quietly ignored.

This has the advantage of facilitating upper-level libvirt support;
currently, there is no way for cache topology support information to be
exposed to libvirt, so libvirt doesn't know which machines to give
support to for these cache topology options.

However, I have a problem here.


Problem with this OPEN
----------------------

Some arches have their own arch-specific cache topology, such as l1 per
core/l2 per core/l3 per die for i386. And as Jeehang proposed for
RISC-V, the cache topologies are like: l1/l2 per core and l3 per
cluster. 

Taking L3 as an example, logically there is a difference between the two
starting points of user-specified core level and with the default core
level.

For example,

"(user-specified) l3-cache-topo=core" should override i386's default l3
per core, but i386's default l3 per core should also override
"(default) l3-cache-topo=core" because this default value is like a
placeholder that specifies nothing.

However, from a command line parsing perspective, it's impossible to
tell what the “l3-cache-topo=core” setting is for...


Options to solve OPEN
---------------------

So, I think we have the following options:


1. Can we avoid such default parameters?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This would reduce the pain in QEMU, but I'm not sure if it's possible to
make libvirt happy?

It is also possible to expose Cache topology information as the CPU
properties in “query-cpu-model-expansion type=full”, but that adds
arch-specific work.

If omitted, I think it's just like omitting “cores”/“sockets”,
leaving it up to the machine to decide based on the specific CPU model
(and now the cache topology is indeed determined by the CPU model as
well).


2. If default is required, can we use a specific abstract word?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

That is, is it possible to use a specific word like “auto”/“invalid”
/“default” and avoid a specific topology level?

Like setting “l3-cache-topo=invalid” (since I've only added the invalid
hierarchy so far ;-) ).

I found the cache topology of arches varies so much that I'm sorry to
say it's hard to have a uniform default cache topology.


I apologize for the very lengthy note and appreciate you reviewing it
here as well as your time!


Introduction
============

Background
----------

Intel client platforms (ADL/RPL/MTL) and E core server platforms (SRF)
share the L2 cache domain among multiple E cores (in the same module).

Thus we need a way to adjust the cache topology so that users could
create the cache topology for Guest that is nearly identical to Host.

This is necessary in cases where there are bound vCPUs, especially
considering that Guest scheduling often takes into account the cache
topology as well (e.g. Linux cluster aware scheduling, i.e. L2 cache
scheduling).

Previously, we introduced a x86 specific option to adjust the cache
topology:

-cpu x-l2-cache-topo=[core|module] [4]

However, considering the needs of other arches, we re-implemented the
generic cache topology (aslo in response to Michael's [5] and Daniel's
comment [6]) in this series.


Cache Topology Representation
-----------------------------

We consider to define the cache topology based on CPU topology level for
two reasons:

1. In practice, a cache will always be bound to the CPU container -
   "CPU container" indicates to a set of CPUs that refer to a certain
   level of CPU topology - where the cache is either private in that
   CPU container or shared among multiple containers.

2. The x86's cache-related CPUIDs encode cache topology based on APIC
   ID's CPU topology layout. And the ACPI PPTT table that ARM/RISCV
   relies on also requires CPU containers (CPU topology) to help
   indicate the private shared hierarchy of the cache.

Therefore, for SMP systems, it is natural to use the CPU topology
hierarchy directly in QEMU to define the cache topology.

And currently, separated L1 cache (L1 data cache and L1 instruction
cache) with unified higher-level caches (e.g., unified L2 and L3
caches), is the most common cache architectures.

Thus, we define the topology for L1 D-cache, L1 I-cache, L2 cache and L3
cache in MachineState as the basic cache topology support:

typedef struct CacheTopology {
    CPUTopoLevel l1d;
    CPUTopoLevel l1i;
    CPUTopoLevel l2;
    CPUTopoLevel l3;
} CacheTopology;

Machines may also only support a subset of the cache topology
to be configured in -smp by setting the SMP property of MachineClass:

typedef struct {
    ...
    bool l1_separated_cache_supported;
    bool l2_unified_cache_supported;
    bool l3_unified_cache_supported;
} SMPCompatProps;


Cache Topology Configuration in -smp
------------------------------------

Further, we add new parameters to -smp:
* l1d-cache=topo_level
* l1i-cache=topo_level
* l2-cache=topo_level
* l3-cache=topo_level

These cache topology parameters accept the strings of CPU topology
levels (such as "drawer", "book", "socket", "die", "cluster", "module",
"core" or "thread"). Exactly which topology level strings could be
accepted as the parameter depends on the machine's support for the
corresponding CPU topology level.

Unsupported cache topology parameters will cause error.

In this series, we add the cache topology support in -smp for x86 PC
machine.

The following example defines a 3-level cache topology hierarchy (L1
D-cache per core, L1 I-cache per core, L2 cache per core and L3 cache per
die) for PC machine.

-smp 32,sockets=2,dies=2,modules=2,cores=2,threads=2,maxcpus=32,\
     l1d-cache=core,l1i-cache=core,l2-cache=core,l3-cache=die


Reference
---------

[1]: [ARM] Jonathan's proposal to adjust cache topology:
     https://lore.kernel.org/qemu-devel/20230808115713.2613-2-Jonathan.Cameron@huawei.com/
[2]: [RISC-V] Discussion between JeeHeng and Jonathan about cache
     topology:
     https://lore.kernel.org/qemu-devel/20240131155336.000068d1@Huawei.com/
[3]: Discussion with Daniel about default cache topology:
     https://lore.kernel.org/qemu-devel/ZkTrsDdyGRFzVULG@redhat.com/
[4]: Previous x86 specific cache topology option:
     https://lore.kernel.org/qemu-devel/20230914072159.1177582-22-zhao1.liu@linux.intel.com/
[5]: Michael's comment about generic cache topology support:
     https://lore.kernel.org/qemu-devel/20231003085516-mutt-send-email-mst@kernel.org/
[6]: Daniel's question about how x86 support L2 cache domain (cluster)
     configuration:
     https://lore.kernel.org/qemu-devel/ZcUG0Uc8KylEQhUW@redhat.com/

Thanks and Best Regards,
Zhao
---
Changelog:

Main changes since RFC v1:
 * Split CpuTopology renaimg out of this RFC.
 * Use QAPI to enumerate CPU topology levels.
 * Drop string_to_cpu_topo() since QAPI will help to parse the topo
   levels.
 * Set has_*_cache field in machine_get_smp(). (JeeHeng)
 * Use "*_cache=topo_level" as -smp example as the original "level"
   term for a cache has a totally different meaning. (Jonathan)

---
Zhao Liu (7):
  hw/core: Make CPU topology enumeration arch-agnostic
  hw/core: Define cache topology for machine
  hw/core: Add cache topology options in -smp
  i386/cpu: Support thread and module level cache topology
  i386/cpu: Update cache topology with machine's configuration
  i386/pc: Support cache topology in -smp for PC machine
  qemu-options: Add the cache topology description of -smp

 MAINTAINERS                    |   2 +
 hw/core/cpu-topology.c         |  36 ++++++++
 hw/core/machine-smp.c          | 146 +++++++++++++++++++++++++++++++++
 hw/core/machine.c              |  25 ++++++
 hw/core/meson.build            |   1 +
 hw/i386/pc.c                   |   3 +
 include/hw/boards.h            |  25 ++++++
 include/hw/core/cpu-topology.h |  20 +++++
 include/hw/i386/topology.h     |  18 +---
 qapi/machine.json              |  63 +++++++++++++-
 qemu-options.hx                |  50 +++++++++--
 system/vl.c                    |  12 +++
 target/i386/cpu.c              |  59 +++++++++----
 target/i386/cpu.h              |   4 +-
 tests/unit/meson.build         |   3 +-
 15 files changed, 422 insertions(+), 45 deletions(-)
 create mode 100644 hw/core/cpu-topology.c
 create mode 100644 include/hw/core/cpu-topology.h

-- 
2.34.1

[RFC v2 1/7] hw/core: Make CPU topology enumeration arch-agnostic

[Zhao Liu]

Cache topology needs to be defined based on CPU topology levels. Thus,
define CPU topology enumeration in qapi/machine.json to make it generic
for all architectures.

To match the general topology naming style, rename CPU_TOPO_LEVEL_SMT
and CPU_TOPO_LEVEL_PACKAGE to CPU_TOPO_LEVEL_THREAD and
CPU_TOPO_LEVEL_SOCKET.

Also, enumerate additional topology levels for non-i386 arches, and add
helpers for topology enumeration and string conversion.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC v1:
 * Use QAPI to enumerate CPU topology levels.
 * Drop string_to_cpu_topo() since QAPI will help to parse the topo
   levels.
---
 MAINTAINERS                    |  2 ++
 hw/core/cpu-topology.c         | 36 ++++++++++++++++++++++++++++++
 hw/core/meson.build            |  1 +
 include/hw/core/cpu-topology.h | 20 +++++++++++++++++
 include/hw/i386/topology.h     | 18 +--------------
 qapi/machine.json              | 40 ++++++++++++++++++++++++++++++++++
 target/i386/cpu.c              | 30 ++++++++++++-------------
 target/i386/cpu.h              |  4 ++--
 8 files changed, 117 insertions(+), 34 deletions(-)
 create mode 100644 hw/core/cpu-topology.c
 create mode 100644 include/hw/core/cpu-topology.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 448dc951c509..09173e8c953d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1875,6 +1875,7 @@ R: Yanan Wang <wangyanan55@huawei.com>
 S: Supported
 F: hw/core/cpu-common.c
 F: hw/core/cpu-sysemu.c
+F: hw/core/cpu-topology.c
 F: hw/core/machine-qmp-cmds.c
 F: hw/core/machine.c
 F: hw/core/machine-smp.c
@@ -1886,6 +1887,7 @@ F: qapi/machine-common.json
 F: qapi/machine-target.json
 F: include/hw/boards.h
 F: include/hw/core/cpu.h
+F: include/hw/core/cpu-topology.h
 F: include/hw/cpu/cluster.h
 F: include/sysemu/numa.h
 F: tests/unit/test-smp-parse.c
diff --git a/hw/core/cpu-topology.c b/hw/core/cpu-topology.c
new file mode 100644
index 000000000000..20b5d708cb54
--- /dev/null
+++ b/hw/core/cpu-topology.c
@@ -0,0 +1,36 @@
+/*
+ * QEMU CPU Topology Representation
+ *
+ * Copyright (c) 2024 Intel Corporation
+ *
+ * Authors:
+ *  Zhao Liu <zhao1.liu@intel.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later.  See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/core/cpu-topology.h"
+
+typedef struct CPUTopoInfo {
+    const char *name;
+} CPUTopoInfo;
+
+CPUTopoInfo cpu_topo_descriptors[] = {
+    [CPU_TOPO_LEVEL_INVALID] = { .name = "invalid", },
+    [CPU_TOPO_LEVEL_THREAD]  = { .name = "thread",  },
+    [CPU_TOPO_LEVEL_CORE]    = { .name = "core",    },
+    [CPU_TOPO_LEVEL_MODULE]  = { .name = "module",  },
+    [CPU_TOPO_LEVEL_CLUSTER] = { .name = "cluster", },
+    [CPU_TOPO_LEVEL_DIE]     = { .name = "die",     },
+    [CPU_TOPO_LEVEL_SOCKET]  = { .name = "socket",  },
+    [CPU_TOPO_LEVEL_BOOK]    = { .name = "book",    },
+    [CPU_TOPO_LEVEL_DRAWER]  = { .name = "drawer",  },
+    [CPU_TOPO_LEVEL__MAX]    = { .name = NULL,      },
+};
+
+const char *cpu_topo_to_string(CPUTopoLevel topo)
+{
+    return cpu_topo_descriptors[topo].name;
+}
diff --git a/hw/core/meson.build b/hw/core/meson.build
index a3d9bab9f42a..71dc396e9bfc 100644
--- a/hw/core/meson.build
+++ b/hw/core/meson.build
@@ -13,6 +13,7 @@ hwcore_ss.add(files(
 ))
 
 common_ss.add(files('cpu-common.c'))
+common_ss.add(files('cpu-topology.c'))
 common_ss.add(files('machine-smp.c'))
 system_ss.add(when: 'CONFIG_FITLOADER', if_true: files('loader-fit.c'))
 system_ss.add(when: 'CONFIG_GENERIC_LOADER', if_true: files('generic-loader.c'))
diff --git a/include/hw/core/cpu-topology.h b/include/hw/core/cpu-topology.h
new file mode 100644
index 000000000000..0e21fe8a9bf8
--- /dev/null
+++ b/include/hw/core/cpu-topology.h
@@ -0,0 +1,20 @@
+/*
+ * QEMU CPU Topology Representation Header
+ *
+ * Copyright (c) 2024 Intel Corporation
+ *
+ * Authors:
+ *  Zhao Liu <zhao1.liu@intel.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later.  See the COPYING file in the top-level directory.
+ */
+
+#ifndef CPU_TOPOLOGY_H
+#define CPU_TOPOLOGY_H
+
+#include "qapi/qapi-types-machine.h"
+
+const char *cpu_topo_to_string(CPUTopoLevel topo);
+
+#endif /* CPU_TOPOLOGY_H */
diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
index dff49fce1154..c6ff75f23991 100644
--- a/include/hw/i386/topology.h
+++ b/include/hw/i386/topology.h
@@ -39,7 +39,7 @@
  *  CPUID Fn8000_0008_ECX[ApicIdCoreIdSize[3:0]] is set to apicid_core_width().
  */
 
-
+#include "hw/core/cpu-topology.h"
 #include "qemu/bitops.h"
 
 /*
@@ -62,22 +62,6 @@ typedef struct X86CPUTopoInfo {
     unsigned threads_per_core;
 } X86CPUTopoInfo;
 
-/*
- * CPUTopoLevel is the general i386 topology hierarchical representation,
- * ordered by increasing hierarchical relationship.
- * Its enumeration value is not bound to the type value of Intel (CPUID[0x1F])
- * or AMD (CPUID[0x80000026]).
- */
-enum CPUTopoLevel {
-    CPU_TOPO_LEVEL_INVALID,
-    CPU_TOPO_LEVEL_SMT,
-    CPU_TOPO_LEVEL_CORE,
-    CPU_TOPO_LEVEL_MODULE,
-    CPU_TOPO_LEVEL_DIE,
-    CPU_TOPO_LEVEL_PACKAGE,
-    CPU_TOPO_LEVEL_MAX,
-};
-
 /* Return the bit width needed for 'count' IDs */
 static unsigned apicid_bitwidth_for_count(unsigned count)
 {
diff --git a/qapi/machine.json b/qapi/machine.json
index bce6e1bbc412..7ac5a05bb9c9 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1667,6 +1667,46 @@
      '*reboot-timeout': 'int',
      '*strict': 'bool' } }
 
+##
+# @CPUTopoLevel:
+#
+# An enumeration of CPU topology levels.
+#
+# @invalid: Invalid topology level, used as a placeholder.
+#
+# @thread: thread level, which would also be called SMT level or logical
+#     processor level. The @threads option in -smp is used to configure
+#     the topology of this level.
+#
+# @core: core level. The @cores option in -smp is used to configure the
+#     topology of this level.
+#
+# @module: module level. The @modules option in -smp is used to
+#     configure the topology of this level.
+#
+# @cluster: cluster level. The @clusters option in -smp is used to
+#     configure the topology of this level.
+#
+# @die: die level. The @dies option in -smp is used to configure the
+#     topology of this level.
+#
+# @socket: socket level, which would also be called package level. The
+#     @sockets option in -smp is used to configure the topology of this
+#     level.
+#
+# @book: book level. The @books option in -smp is used to configure the
+#     topology of this level.
+#
+# @drawer: drawer level. The @drawers option in -smp is used to
+#     configure the topology of this level.
+#
+# Since: 9.1
+##
+{ 'enum': 'CPUTopoLevel',
+  'prefix': 'CPU_TOPO_LEVEL',
+  'data': [ 'invalid', 'thread', 'core', 'module', 'cluster',
+            'die', 'socket', 'book', 'drawer' ] }
+
 ##
 # @SMPConfiguration:
 #
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index bc2dceb647fa..b11097b5bafd 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -247,12 +247,12 @@ static uint32_t max_thread_ids_for_cache(X86CPUTopoInfo *topo_info,
     case CPU_TOPO_LEVEL_DIE:
         num_ids = 1 << apicid_die_offset(topo_info);
         break;
-    case CPU_TOPO_LEVEL_PACKAGE:
+    case CPU_TOPO_LEVEL_SOCKET:
         num_ids = 1 << apicid_pkg_offset(topo_info);
         break;
     default:
         /*
-         * Currently there is no use case for SMT and MODULE, so use
+         * Currently there is no use case for THREAD and MODULE, so use
          * assert directly to facilitate debugging.
          */
         g_assert_not_reached();
@@ -304,7 +304,7 @@ static uint32_t num_threads_by_topo_level(X86CPUTopoInfo *topo_info,
                                           enum CPUTopoLevel topo_level)
 {
     switch (topo_level) {
-    case CPU_TOPO_LEVEL_SMT:
+    case CPU_TOPO_LEVEL_THREAD:
         return 1;
     case CPU_TOPO_LEVEL_CORE:
         return topo_info->threads_per_core;
@@ -313,7 +313,7 @@ static uint32_t num_threads_by_topo_level(X86CPUTopoInfo *topo_info,
     case CPU_TOPO_LEVEL_DIE:
         return topo_info->threads_per_core * topo_info->cores_per_module *
                topo_info->modules_per_die;
-    case CPU_TOPO_LEVEL_PACKAGE:
+    case CPU_TOPO_LEVEL_SOCKET:
         return topo_info->threads_per_core * topo_info->cores_per_module *
                topo_info->modules_per_die * topo_info->dies_per_pkg;
     default:
@@ -326,7 +326,7 @@ static uint32_t apicid_offset_by_topo_level(X86CPUTopoInfo *topo_info,
                                             enum CPUTopoLevel topo_level)
 {
     switch (topo_level) {
-    case CPU_TOPO_LEVEL_SMT:
+    case CPU_TOPO_LEVEL_THREAD:
         return 0;
     case CPU_TOPO_LEVEL_CORE:
         return apicid_core_offset(topo_info);
@@ -334,7 +334,7 @@ static uint32_t apicid_offset_by_topo_level(X86CPUTopoInfo *topo_info,
         return apicid_module_offset(topo_info);
     case CPU_TOPO_LEVEL_DIE:
         return apicid_die_offset(topo_info);
-    case CPU_TOPO_LEVEL_PACKAGE:
+    case CPU_TOPO_LEVEL_SOCKET:
         return apicid_pkg_offset(topo_info);
     default:
         g_assert_not_reached();
@@ -347,7 +347,7 @@ static uint32_t cpuid1f_topo_type(enum CPUTopoLevel topo_level)
     switch (topo_level) {
     case CPU_TOPO_LEVEL_INVALID:
         return CPUID_1F_ECX_TOPO_LEVEL_INVALID;
-    case CPU_TOPO_LEVEL_SMT:
+    case CPU_TOPO_LEVEL_THREAD:
         return CPUID_1F_ECX_TOPO_LEVEL_SMT;
     case CPU_TOPO_LEVEL_CORE:
         return CPUID_1F_ECX_TOPO_LEVEL_CORE;
@@ -371,7 +371,7 @@ static void encode_topo_cpuid1f(CPUX86State *env, uint32_t count,
     unsigned long level, next_level;
     uint32_t num_threads_next_level, offset_next_level;
 
-    assert(count + 1 < CPU_TOPO_LEVEL_MAX);
+    assert(count + 1 < CPU_TOPO_LEVEL__MAX);
 
     /*
      * Find the No.(count + 1) topology level in avail_cpu_topo bitmap.
@@ -380,7 +380,7 @@ static void encode_topo_cpuid1f(CPUX86State *env, uint32_t count,
     level = CPU_TOPO_LEVEL_INVALID;
     for (int i = 0; i <= count; i++) {
         level = find_next_bit(env->avail_cpu_topo,
-                              CPU_TOPO_LEVEL_PACKAGE,
+                              CPU_TOPO_LEVEL_SOCKET,
                               level + 1);
 
         /*
@@ -388,7 +388,7 @@ static void encode_topo_cpuid1f(CPUX86State *env, uint32_t count,
          * and it just encodes the invalid level (all fields are 0)
          * into the last subleaf of 0x1f.
          */
-        if (level == CPU_TOPO_LEVEL_PACKAGE) {
+        if (level == CPU_TOPO_LEVEL_SOCKET) {
             level = CPU_TOPO_LEVEL_INVALID;
             break;
         }
@@ -399,7 +399,7 @@ static void encode_topo_cpuid1f(CPUX86State *env, uint32_t count,
         offset_next_level = 0;
     } else {
         next_level = find_next_bit(env->avail_cpu_topo,
-                                   CPU_TOPO_LEVEL_PACKAGE,
+                                   CPU_TOPO_LEVEL_SOCKET,
                                    level + 1);
         num_threads_next_level = num_threads_by_topo_level(topo_info,
                                                            next_level);
@@ -6435,7 +6435,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
 
                     /* Share the cache at package level. */
                     *eax |= max_thread_ids_for_cache(&topo_info,
-                                CPU_TOPO_LEVEL_PACKAGE) << 14;
+                                CPU_TOPO_LEVEL_SOCKET) << 14;
                 }
             }
         } else if (cpu->vendor_cpuid_only && IS_AMD_CPU(env)) {
@@ -7935,10 +7935,10 @@ static void x86_cpu_init_default_topo(X86CPU *cpu)
     env->nr_modules = 1;
     env->nr_dies = 1;
 
-    /* SMT, core and package levels are set by default. */
-    set_bit(CPU_TOPO_LEVEL_SMT, env->avail_cpu_topo);
+    /* thread, core and socket levels are set by default. */
+    set_bit(CPU_TOPO_LEVEL_THREAD, env->avail_cpu_topo);
     set_bit(CPU_TOPO_LEVEL_CORE, env->avail_cpu_topo);
-    set_bit(CPU_TOPO_LEVEL_PACKAGE, env->avail_cpu_topo);
+    set_bit(CPU_TOPO_LEVEL_SOCKET, env->avail_cpu_topo);
 }
 
 static void x86_cpu_initfn(Object *obj)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index c64ef0c1a287..c6d07f38a1e8 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1606,7 +1606,7 @@ typedef struct CPUCacheInfo {
      * Used to encode CPUID[4].EAX[bits 25:14] or
      * CPUID[0x8000001D].EAX[bits 25:14].
      */
-    enum CPUTopoLevel share_level;
+    CPUTopoLevel share_level;
 } CPUCacheInfo;
 
 
@@ -1921,7 +1921,7 @@ typedef struct CPUArchState {
     unsigned nr_modules;
 
     /* Bitmap of available CPU topology levels for this CPU. */
-    DECLARE_BITMAP(avail_cpu_topo, CPU_TOPO_LEVEL_MAX);
+    DECLARE_BITMAP(avail_cpu_topo, CPU_TOPO_LEVEL__MAX);
 } CPUX86State;
 
 struct kvm_msrs;
-- 
2.34.1

[RFC v2 2/7] hw/core: Define cache topology for machine

[Zhao Liu]

Define the cache topology based on CPU topology level for two reasons:

1. In practice, a cache will always be bound to the CPU container
   (either private in the CPU container or shared among multiple
   containers), and CPU container is often expressed in terms of CPU
   topology level.
2. The x86's cache-related CPUIDs encode cache topology based on APIC
   ID's CPU topology layout. And the ACPI PPTT table that ARM/RISCV
   relies on also requires CPU containers to help indicate the private
   shared hierarchy of the cache. Therefore, for SMP systems, it is
   natural to use the CPU topology hierarchy directly in QEMU to define
   the cache topology.

Currently, separated L1 cache (L1 data cache and L1 instruction cache)
with unified higher-level cache (e.g., unified L2 and L3 caches), is the
most common cache architectures.

Therefore, define the topology for L1 D-cache, L1 I-cache, L2 cache and
L3 cache in machine as the basic cache topology support.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/core/machine.c   |  5 +++++
 include/hw/boards.h | 25 +++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 8087026b45da..e31d0f3cb4b0 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1175,6 +1175,11 @@ static void machine_initfn(Object *obj)
     ms->smp.cores = 1;
     ms->smp.threads = 1;
 
+    ms->smp_cache.l1d = CPU_TOPO_LEVEL_INVALID;
+    ms->smp_cache.l1i = CPU_TOPO_LEVEL_INVALID;
+    ms->smp_cache.l2 = CPU_TOPO_LEVEL_INVALID;
+    ms->smp_cache.l3 = CPU_TOPO_LEVEL_INVALID;
+
     machine_copy_boot_config(ms, &(BootConfiguration){ 0 });
 }
 
diff --git a/include/hw/boards.h b/include/hw/boards.h
index c1737f2a5736..e70b2a1bdca2 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -10,6 +10,7 @@
 #include "qemu/module.h"
 #include "qom/object.h"
 #include "hw/core/cpu.h"
+#include "hw/core/cpu-topology.h"
 
 #define TYPE_MACHINE_SUFFIX "-machine"
 
@@ -145,6 +146,12 @@ typedef struct {
  * @books_supported - whether books are supported by the machine
  * @drawers_supported - whether drawers are supported by the machine
  * @modules_supported - whether modules are supported by the machine
+ * @l1_separated_cache_supported - whether l1 data and instruction cache
+ *                                 topology are supported by the machine
+ * @l2_unified_cache_supported - whether l2 unified cache topology are
+ *                               supported by the machine
+ * @l3_unified_cache_supported - whether l3 unified cache topology are
+ *                               supported by the machine
  */
 typedef struct {
     bool prefer_sockets;
@@ -154,6 +161,9 @@ typedef struct {
     bool books_supported;
     bool drawers_supported;
     bool modules_supported;
+    bool l1_separated_cache_supported;
+    bool l2_unified_cache_supported;
+    bool l3_unified_cache_supported;
 } SMPCompatProps;
 
 /**
@@ -359,6 +369,20 @@ typedef struct CPUTopology {
     unsigned int max_cpus;
 } CPUTopology;
 
+/**
+ * CPUTopology:
+ * @l1d: the CPU topology hierarchy the L1 data cache is shared at.
+ * @l1i: the CPU topology hierarchy the L1 instruction cache is shared at.
+ * @l2: the CPU topology hierarchy the L2 (unified) cache is shared at.
+ * @l3: the CPU topology hierarchy the L3 (unified) cache is shared at.
+ */
+typedef struct CacheTopology {
+    CPUTopoLevel l1d;
+    CPUTopoLevel l1i;
+    CPUTopoLevel l2;
+    CPUTopoLevel l3;
+} CacheTopology;
+
 /**
  * MachineState:
  */
@@ -410,6 +434,7 @@ struct MachineState {
     AccelState *accelerator;
     CPUArchIdList *possible_cpus;
     CPUTopology smp;
+    CacheTopology smp_cache;
     struct NVDIMMState *nvdimms_state;
     struct NumaState *numa_state;
 };
-- 
2.34.1

[RFC v2 3/7] hw/core: Add cache topology options in -smp

[Zhao Liu]

Add "l1d-cache", "l1i-cache". "l2-cache", and "l3-cache" options in
-smp to define the cache topology for SMP system.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC v1:
 * Set has_*_cache field in machine_get_smp(). (JeeHeng)
 * Adjust string breaking style in error_setg() for more semantic
   sentence breaking conventions. (Jonathan)
 * Add more description about cache options. (Markus)
 * Now in v2, config->*_cache field stores topology enumeration instead
   of string, no need to parse, so just make machine_check_cache_topo()
   return boolean.
---
 hw/core/machine-smp.c  | 146 +++++++++++++++++++++++++++++++++++++++++
 hw/core/machine.c      |  20 ++++++
 qapi/machine.json      |  23 ++++++-
 system/vl.c            |  12 ++++
 tests/unit/meson.build |   3 +-
 5 files changed, 202 insertions(+), 2 deletions(-)

diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 5d8d7edcbd3f..c79464cf3d2c 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -61,6 +61,150 @@ static char *cpu_hierarchy_to_string(MachineState *ms)
     return g_string_free(s, false);
 }
 
+static bool machine_check_topo_support(MachineState *ms,
+                                       CPUTopoLevel topo)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+
+    if (topo == CPU_TOPO_LEVEL_MODULE && !mc->smp_props.modules_supported) {
+        return false;
+    }
+
+    if (topo == CPU_TOPO_LEVEL_CLUSTER && !mc->smp_props.clusters_supported) {
+        return false;
+    }
+
+    if (topo == CPU_TOPO_LEVEL_DIE && !mc->smp_props.dies_supported) {
+        return false;
+    }
+
+    if (topo == CPU_TOPO_LEVEL_BOOK && !mc->smp_props.books_supported) {
+        return false;
+    }
+
+    if (topo == CPU_TOPO_LEVEL_DRAWER && !mc->smp_props.drawers_supported) {
+        return false;
+    }
+
+    return true;
+}
+
+static bool machine_check_cache_topo(MachineState *ms,
+                                     CPUTopoLevel topo,
+                                     Error **errp)
+{
+    if (topo == CPU_TOPO_LEVEL__MAX || topo == CPU_TOPO_LEVEL_INVALID) {
+        error_setg(errp,
+                   "Invalid cache topology level: %s. "
+                   "The cache topology should match the "
+                   "valid CPU topology level",
+                   cpu_topo_to_string(topo));
+        return false;
+    }
+
+    if (!machine_check_topo_support(ms, topo)) {
+        error_setg(errp,
+                   "Invalid cache topology level: %s. "
+                   "The topology level is not supported by this machine",
+                   cpu_topo_to_string(topo));
+        return false;
+    }
+
+    return true;
+}
+
+static void machine_parse_smp_cache_config(MachineState *ms,
+                                           const SMPConfiguration *config,
+                                           Error **errp)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+
+    /*
+     * The cache topology does not support a default entry similar to
+     * CPU topology with parameters=1. So when the machine explicitly
+     * does not support cache topology, return the error.
+     */
+    if (config->has_l1d_cache) {
+        if (!mc->smp_props.l1_separated_cache_supported) {
+            error_setg(errp,
+                       "L1 D-cache topology not supported by this machine");
+            return;
+        }
+
+        if (!machine_check_cache_topo(ms, config->l1d_cache, errp)) {
+            return;
+        }
+
+        ms->smp_cache.l1d = config->l1d_cache;
+    }
+
+    if (config->has_l1i_cache) {
+        if (!mc->smp_props.l1_separated_cache_supported) {
+            error_setg(errp,
+                       "L1 I-cache topology not supported by this machine");
+            return;
+        }
+
+        if (!machine_check_cache_topo(ms, config->l1i_cache, errp)) {
+            return;
+        }
+
+        ms->smp_cache.l1i = config->l1i_cache;
+    }
+
+    if (config->has_l2_cache) {
+        if (!mc->smp_props.l2_unified_cache_supported) {
+            error_setg(errp,
+                       "L2 cache topology not supported by this machine");
+            return;
+        }
+
+        if (!machine_check_cache_topo(ms, config->l2_cache, errp)) {
+            return;
+        }
+
+        ms->smp_cache.l2 = config->l2_cache;
+
+        /*
+         * Cache topology is initialized by default to CPU_TOPO_LEVEL_INVALID,
+         * which is the lowest level, so such a check is OK, even if the config
+         * doesn't override that field.
+         */
+        if (ms->smp_cache.l1d > ms->smp_cache.l2 ||
+            ms->smp_cache.l1i > ms->smp_cache.l2) {
+            error_setg(errp,
+                       "Invalid L2 cache topology. "
+                       "L2 cache topology level should not be lower than "
+                       "L1 D-cache/L1 I-cache");
+            return;
+        }
+    }
+
+    if (config->has_l3_cache) {
+        if (!mc->smp_props.l2_unified_cache_supported) {
+            error_setg(errp,
+                       "L3 cache topology not supported by this machine");
+            return;
+        }
+
+        if (!machine_check_cache_topo(ms, config->l3_cache, errp)) {
+            return;
+        }
+
+        ms->smp_cache.l3 = config->l3_cache;
+
+        if (ms->smp_cache.l1d > ms->smp_cache.l3 ||
+            ms->smp_cache.l1i > ms->smp_cache.l3 ||
+            ms->smp_cache.l2 > ms->smp_cache.l3) {
+            error_setg(errp,
+                       "Invalid L3 cache topology. "
+                       "L3 cache topology level should not be lower than "
+                       "L1 D-cache/L1 I-cache/L2 cache");
+            return;
+        }
+    }
+}
+
 /*
  * machine_parse_smp_config: Generic function used to parse the given
  *                           SMP configuration
@@ -259,6 +403,8 @@ void machine_parse_smp_config(MachineState *ms,
                    mc->name, mc->max_cpus);
         return;
     }
+
+    machine_parse_smp_cache_config(ms, config, errp);
 }
 
 unsigned int machine_topo_get_cores_per_socket(const MachineState *ms)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index e31d0f3cb4b0..f705485f83c0 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -900,6 +900,26 @@ static void machine_get_smp(Object *obj, Visitor *v, const char *name,
         .has_maxcpus = true, .maxcpus = ms->smp.max_cpus,
     };
 
+    if (ms->smp_cache.l1d != CPU_TOPO_LEVEL_INVALID) {
+        config->has_l1d_cache = true;
+        config->l1d_cache = ms->smp_cache.l1d;
+    }
+
+    if (ms->smp_cache.l1i != CPU_TOPO_LEVEL_INVALID) {
+        config->has_l1i_cache = true;
+        config->l1i_cache = ms->smp_cache.l1i;
+    }
+
+    if (ms->smp_cache.l2 != CPU_TOPO_LEVEL_INVALID) {
+        config->has_l2_cache = true;
+        config->l2_cache = ms->smp_cache.l2;
+    }
+
+    if (ms->smp_cache.l3 != CPU_TOPO_LEVEL_INVALID) {
+        config->has_l3_cache = true;
+        config->l3_cache = ms->smp_cache.l3;
+    }
+
     if (!visit_type_SMPConfiguration(v, name, &config, &error_abort)) {
         return;
     }
diff --git a/qapi/machine.json b/qapi/machine.json
index 7ac5a05bb9c9..8fa5af69b1bf 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1746,6 +1746,23 @@
 #
 # @threads: number of threads per core
 #
+# @l1d-cache: topology hierarchy of L1 data cache. It accepts the CPU
+#     topology enumeration as the parameter, i.e., CPUs in the same
+#     topology container share the same L1 data cache. (since 9.1)
+#
+# @l1i-cache: topology hierarchy of L1 instruction cache. It accepts
+#     the CPU topology enumeration as the parameter, i.e., CPUs in the
+#     same topology container share the same L1 instruction cache.
+#     (since 9.1)
+#
+# @l2-cache: topology hierarchy of L2 unified cache. It accepts the CPU
+#     topology enumeration as the parameter, i.e., CPUs in the same
+#     topology container share the same L2 unified cache. (since 9.1)
+#
+# @l3-cache: topology hierarchy of L3 unified cache. It accepts the CPU
+#     topology enumeration as the parameter, i.e., CPUs in the same
+#     topology container share the same L3 unified cache. (since 9.1)
+#
 # Since: 6.1
 ##
 { 'struct': 'SMPConfiguration', 'data': {
@@ -1758,7 +1775,11 @@
      '*modules': 'int',
      '*cores': 'int',
      '*threads': 'int',
-     '*maxcpus': 'int' } }
+     '*maxcpus': 'int',
+     '*l1d-cache': 'CPUTopoLevel',
+     '*l1i-cache': 'CPUTopoLevel',
+     '*l2-cache': 'CPUTopoLevel',
+     '*l3-cache': 'CPUTopoLevel' } }
 
 ##
 # @x-query-irq:
diff --git a/system/vl.c b/system/vl.c
index a3eede5fa5b8..c7c94d41bd01 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -753,6 +753,18 @@ static QemuOptsList qemu_smp_opts = {
         }, {
             .name = "maxcpus",
             .type = QEMU_OPT_NUMBER,
+        }, {
+            .name = "l1d-cache",
+            .type = QEMU_OPT_STRING,
+        }, {
+            .name = "l1i-cache",
+            .type = QEMU_OPT_STRING,
+        }, {
+            .name = "l2-cache",
+            .type = QEMU_OPT_STRING,
+        }, {
+            .name = "l3-cache",
+            .type = QEMU_OPT_STRING,
         },
         { /*End of list */ }
     },
diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 26c109c968ce..8877dbbc00c9 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -138,7 +138,8 @@ if have_system
     'test-util-sockets': ['socket-helpers.c'],
     'test-base64': [],
     'test-bufferiszero': [],
-    'test-smp-parse': [qom, meson.project_source_root() / 'hw/core/machine-smp.c'],
+    'test-smp-parse': [qom, meson.project_source_root() / 'hw/core/machine-smp.c',
+                       meson.project_source_root() / 'hw/core/cpu-topology.c'],
     'test-vmstate': [migration, io],
     'test-yank': ['socket-helpers.c', qom, io, chardev]
   }
-- 
2.34.1

[RFC v2 4/7] i386/cpu: Support thread and module level cache topology

[Zhao Liu]

Allows cache to be defined at the thread and module level. This
increases flexibility for x86 users to customize their cache topology.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 target/i386/cpu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b11097b5bafd..3a2dadb4bce0 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -241,9 +241,15 @@ static uint32_t max_thread_ids_for_cache(X86CPUTopoInfo *topo_info,
     uint32_t num_ids = 0;
 
     switch (share_level) {
+    case CPU_TOPO_LEVEL_THREAD:
+        num_ids = 1;
+        break;
     case CPU_TOPO_LEVEL_CORE:
         num_ids = 1 << apicid_core_offset(topo_info);
         break;
+    case CPU_TOPO_LEVEL_MODULE:
+        num_ids = 1 << apicid_module_offset(topo_info);
+        break;
     case CPU_TOPO_LEVEL_DIE:
         num_ids = 1 << apicid_die_offset(topo_info);
         break;
@@ -251,10 +257,6 @@ static uint32_t max_thread_ids_for_cache(X86CPUTopoInfo *topo_info,
         num_ids = 1 << apicid_pkg_offset(topo_info);
         break;
     default:
-        /*
-         * Currently there is no use case for THREAD and MODULE, so use
-         * assert directly to facilitate debugging.
-         */
         g_assert_not_reached();
     }
 
-- 
2.34.1

[RFC v2 5/7] i386/cpu: Update cache topology with machine's configuration

[Zhao Liu]

User will configure SMP cache topology via -smp.

For this case, update the x86 CPUs' cache topology with user's
configuration in MachineState.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 target/i386/cpu.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 3a2dadb4bce0..1bd1860ae625 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7764,6 +7764,27 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
 #ifndef CONFIG_USER_ONLY
     MachineState *ms = MACHINE(qdev_get_machine());
+
+    if (ms->smp_cache.l1d != CPU_TOPO_LEVEL_INVALID) {
+        env->cache_info_cpuid4.l1d_cache->share_level = ms->smp_cache.l1d;
+        env->cache_info_amd.l1d_cache->share_level = ms->smp_cache.l1d;
+    }
+
+    if (ms->smp_cache.l1i != CPU_TOPO_LEVEL_INVALID) {
+        env->cache_info_cpuid4.l1i_cache->share_level = ms->smp_cache.l1i;
+        env->cache_info_amd.l1i_cache->share_level = ms->smp_cache.l1i;
+    }
+
+    if (ms->smp_cache.l2 != CPU_TOPO_LEVEL_INVALID) {
+        env->cache_info_cpuid4.l2_cache->share_level = ms->smp_cache.l2;
+        env->cache_info_amd.l2_cache->share_level = ms->smp_cache.l2;
+    }
+
+    if (ms->smp_cache.l3 != CPU_TOPO_LEVEL_INVALID) {
+        env->cache_info_cpuid4.l3_cache->share_level = ms->smp_cache.l3;
+        env->cache_info_amd.l3_cache->share_level = ms->smp_cache.l3;
+    }
+
     qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
 
     if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || ms->smp.cpus > 1) {
-- 
2.34.1

[RFC v2 6/7] i386/pc: Support cache topology in -smp for PC machine

[Zhao Liu]

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/i386/pc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7b638da7aaa8..2e03b33a4116 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1844,6 +1844,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     mc->nvdimm_supported = true;
     mc->smp_props.dies_supported = true;
     mc->smp_props.modules_supported = true;
+    mc->smp_props.l1_separated_cache_supported = true;
+    mc->smp_props.l2_unified_cache_supported = true;
+    mc->smp_props.l3_unified_cache_supported = true;
     mc->default_ram_id = "pc.ram";
     pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_AUTO;
 
-- 
2.34.1

[RFC v2 7/7] qemu-options: Add the cache topology description of -smp

[Zhao Liu]

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC v1:
 * Use "*_cache=topo_level" as -smp example as the original "level"
   term for a cache has a totally different meaning. (Jonathan)
---
 qemu-options.hx | 50 +++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 8ca7f34ef0c8..29d8a4b9b68b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -282,7 +282,8 @@ ERST
 DEF("smp", HAS_ARG, QEMU_OPTION_smp,
     "-smp [[cpus=]n][,maxcpus=maxcpus][,drawers=drawers][,books=books][,sockets=sockets]\n"
     "               [,dies=dies][,clusters=clusters][,modules=modules][,cores=cores]\n"
-    "               [,threads=threads]\n"
+    "               [,threads=threads][,l1d-cache=topo_level][,l1i-cache=topo_level]\n"
+    "               [,l2-cache=topo_level][,l3-cache=topo_level]\n"
     "                set the number of initial CPUs to 'n' [default=1]\n"
     "                maxcpus= maximum number of total CPUs, including\n"
     "                offline CPUs for hotplug, etc\n"
@@ -294,7 +295,11 @@ DEF("smp", HAS_ARG, QEMU_OPTION_smp,
     "                modules= number of modules in one cluster\n"
     "                cores= number of cores in one module\n"
     "                threads= number of threads in one core\n"
-    "Note: Different machines may have different subsets of the CPU topology\n"
+    "                l1d-cache= topology level of L1 D-cache\n"
+    "                l1i-cache= topology level of L1 I-cache\n"
+    "                l2-cache= topology level of L2 cache\n"
+    "                l3-cache= topology level of L3 cache\n"
+    "Note: Different machines may have different subsets of the CPU and cache topology\n"
     "      parameters supported, so the actual meaning of the supported parameters\n"
     "      will vary accordingly. For example, for a machine type that supports a\n"
     "      three-level CPU hierarchy of sockets/cores/threads, the parameters will\n"
@@ -308,7 +313,7 @@ DEF("smp", HAS_ARG, QEMU_OPTION_smp,
     "      must be set as 1 in the purpose of correct parsing.\n",
     QEMU_ARCH_ALL)
 SRST
-``-smp [[cpus=]n][,maxcpus=maxcpus][,drawers=drawers][,books=books][,sockets=sockets][,dies=dies][,clusters=clusters][,modules=modules][,cores=cores][,threads=threads]``
+``-smp [[cpus=]n][,maxcpus=maxcpus][,drawers=drawers][,books=books][,sockets=sockets][,dies=dies][,clusters=clusters][,modules=modules][,cores=cores][,threads=threads][,l1d-cache=topo_level][,l1i-cache=topo_level][,l2-cache=topo_level][,l3-cache=topo_level]``
     Simulate a SMP system with '\ ``n``\ ' CPUs initially present on
     the machine type board. On boards supporting CPU hotplug, the optional
     '\ ``maxcpus``\ ' parameter can be set to enable further CPUs to be
@@ -322,15 +327,34 @@ SRST
     Both parameters are subject to an upper limit that is determined by
     the specific machine type chosen.
 
+    CPU topology parameters include '\ ``drawers``\ ', '\ ``books``\ ',
+    '\ ``sockets``\ ', '\ ``dies``\ ', '\ ``clusters``\ ', '\ ``modules``\ ',
+    '\ ``cores``\ ' and '\ ``threads``\ '. These CPU parameters accept only
+    integers and are used to specify the number of specific topology domains
+    under the corresponding topology level.
+
     To control reporting of CPU topology information, values of the topology
     parameters can be specified. Machines may only support a subset of the
-    parameters and different machines may have different subsets supported
-    which vary depending on capacity of the corresponding CPU targets. So
-    for a particular machine type board, an expected topology hierarchy can
+    CPU topology parameters and different machines may have different subsets
+    supported which vary depending on capacity of the corresponding CPU targets.
+    So for a particular machine type board, an expected topology hierarchy can
     be defined through the supported sub-option. Unsupported parameters can
     also be provided in addition to the sub-option, but their values must be
     set as 1 in the purpose of correct parsing.
 
+    Cache topology parameters include '\ ``l1d-cache``\ ', '\ ``l1i-cache``\ ',
+    '\ ``l2-cache``\ ' and '\ ``l3-cache``\ '. These cache topology parameters
+    accept the strings of CPU topology levels (such as '\ ``drawer``\ ', '\ ``book``\ ',
+    '\ ``socket``\ ', '\ ``die``\ ', '\ ``cluster``\ ', '\ ``module``\ ',
+    '\ ``core``\ ' or '\ ``thread``\ '). Exactly which topology level strings
+    could be accepted as the parameter depends on the machine's support for the
+    corresponding CPU topology level.
+
+    Machines may also only support a subset of the cache topology parameters.
+    Unsupported cache topology parameters will be omitted, and correspondingly,
+    the target CPU's cache topology will use the its default cache topology
+    setting.
+
     Either the initial CPU count, or at least one of the topology parameters
     must be specified. The specified parameters must be greater than zero,
     explicit configuration like "cpus=0" is not allowed. Values for any
@@ -356,6 +380,20 @@ SRST
 
         -smp 32,sockets=2,dies=2,modules=2,cores=2,threads=2,maxcpus=32
 
+    The following sub-option defines a CPU topology hierarchy (2 sockets
+    totally on the machine, 2 dies per socket, 2 modules per die, 2 cores per
+    module, 2 threads per core) with 3-level cache topology hierarchy (L1
+    D-cache per core, L1 I-cache per core, L2 cache per core and L3 cache per
+    die) for PC machines which support sockets/dies/modules/cores/threads.
+    Some members of the CPU topology option can be omitted but their values
+    will be automatically computed. Some members of the cache topology
+    option can also be omitted and target CPU will use the default topology.:
+
+    ::
+
+        -smp 32,sockets=2,dies=2,modules=2,cores=2,threads=2,maxcpus=32,\
+             l1d-cache=core,l1i-cache=core,l2-cache=core,l3-cache=die
+
     The following sub-option defines a CPU topology hierarchy (2 sockets
     totally on the machine, 2 clusters per socket, 2 cores per cluster,
     2 threads per core) for ARM virt machines which support sockets/clusters
-- 
2.34.1

Re: [RFC v2 1/7] hw/core: Make CPU topology enumeration arch-agnostic

[Markus Armbruster]

Zhao Liu <zhao1.liu@intel.com> writes:

> Cache topology needs to be defined based on CPU topology levels. Thus,
> define CPU topology enumeration in qapi/machine.json to make it generic
> for all architectures.
>
> To match the general topology naming style, rename CPU_TOPO_LEVEL_SMT
> and CPU_TOPO_LEVEL_PACKAGE to CPU_TOPO_LEVEL_THREAD and
> CPU_TOPO_LEVEL_SOCKET.
>
> Also, enumerate additional topology levels for non-i386 arches, and add
> helpers for topology enumeration and string conversion.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
> Changes since RFC v1:
>  * Use QAPI to enumerate CPU topology levels.
>  * Drop string_to_cpu_topo() since QAPI will help to parse the topo
>    levels.
> ---
>  MAINTAINERS                    |  2 ++
>  hw/core/cpu-topology.c         | 36 ++++++++++++++++++++++++++++++
>  hw/core/meson.build            |  1 +
>  include/hw/core/cpu-topology.h | 20 +++++++++++++++++
>  include/hw/i386/topology.h     | 18 +--------------
>  qapi/machine.json              | 40 ++++++++++++++++++++++++++++++++++
>  target/i386/cpu.c              | 30 ++++++++++++-------------
>  target/i386/cpu.h              |  4 ++--
>  8 files changed, 117 insertions(+), 34 deletions(-)
>  create mode 100644 hw/core/cpu-topology.c
>  create mode 100644 include/hw/core/cpu-topology.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 448dc951c509..09173e8c953d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1875,6 +1875,7 @@ R: Yanan Wang <wangyanan55@huawei.com>
>  S: Supported
>  F: hw/core/cpu-common.c
>  F: hw/core/cpu-sysemu.c
> +F: hw/core/cpu-topology.c
>  F: hw/core/machine-qmp-cmds.c
>  F: hw/core/machine.c
>  F: hw/core/machine-smp.c
> @@ -1886,6 +1887,7 @@ F: qapi/machine-common.json
>  F: qapi/machine-target.json
>  F: include/hw/boards.h
>  F: include/hw/core/cpu.h
> +F: include/hw/core/cpu-topology.h
>  F: include/hw/cpu/cluster.h
>  F: include/sysemu/numa.h
>  F: tests/unit/test-smp-parse.c
> diff --git a/hw/core/cpu-topology.c b/hw/core/cpu-topology.c
> new file mode 100644
> index 000000000000..20b5d708cb54
> --- /dev/null
> +++ b/hw/core/cpu-topology.c
> @@ -0,0 +1,36 @@
> +/*
> + * QEMU CPU Topology Representation
> + *
> + * Copyright (c) 2024 Intel Corporation
> + *
> + * Authors:
> + *  Zhao Liu <zhao1.liu@intel.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * later.  See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/core/cpu-topology.h"
> +
> +typedef struct CPUTopoInfo {
> +    const char *name;
> +} CPUTopoInfo;
> +
> +CPUTopoInfo cpu_topo_descriptors[] = {
> +    [CPU_TOPO_LEVEL_INVALID] = { .name = "invalid", },
> +    [CPU_TOPO_LEVEL_THREAD]  = { .name = "thread",  },
> +    [CPU_TOPO_LEVEL_CORE]    = { .name = "core",    },
> +    [CPU_TOPO_LEVEL_MODULE]  = { .name = "module",  },
> +    [CPU_TOPO_LEVEL_CLUSTER] = { .name = "cluster", },
> +    [CPU_TOPO_LEVEL_DIE]     = { .name = "die",     },
> +    [CPU_TOPO_LEVEL_SOCKET]  = { .name = "socket",  },
> +    [CPU_TOPO_LEVEL_BOOK]    = { .name = "book",    },
> +    [CPU_TOPO_LEVEL_DRAWER]  = { .name = "drawer",  },
> +    [CPU_TOPO_LEVEL__MAX]    = { .name = NULL,      },
> +};

This looks redundant with generated

    const QEnumLookup CPUTopoLevel_lookup = {
        .array = (const char *const[]) {
            [CPU_TOPO_LEVEL_INVALID] = "invalid",
            [CPU_TOPO_LEVEL_THREAD] = "thread",
            [CPU_TOPO_LEVEL_CORE] = "core",
            [CPU_TOPO_LEVEL_MODULE] = "module",
            [CPU_TOPO_LEVEL_CLUSTER] = "cluster",
            [CPU_TOPO_LEVEL_DIE] = "die",
            [CPU_TOPO_LEVEL_SOCKET] = "socket",
            [CPU_TOPO_LEVEL_BOOK] = "book",
            [CPU_TOPO_LEVEL_DRAWER] = "drawer",
        },
        .size = CPU_TOPO_LEVEL__MAX
    };

> +
> +const char *cpu_topo_to_string(CPUTopoLevel topo)
> +{
> +    return cpu_topo_descriptors[topo].name;
> +}

And this with generated CPUTopoLevel_str().

> diff --git a/hw/core/meson.build b/hw/core/meson.build
> index a3d9bab9f42a..71dc396e9bfc 100644
> --- a/hw/core/meson.build
> +++ b/hw/core/meson.build
> @@ -13,6 +13,7 @@ hwcore_ss.add(files(
>  ))
>  
>  common_ss.add(files('cpu-common.c'))
> +common_ss.add(files('cpu-topology.c'))
>  common_ss.add(files('machine-smp.c'))
>  system_ss.add(when: 'CONFIG_FITLOADER', if_true: files('loader-fit.c'))
>  system_ss.add(when: 'CONFIG_GENERIC_LOADER', if_true: files('generic-loader.c'))
> diff --git a/include/hw/core/cpu-topology.h b/include/hw/core/cpu-topology.h
> new file mode 100644
> index 000000000000..0e21fe8a9bf8
> --- /dev/null
> +++ b/include/hw/core/cpu-topology.h
> @@ -0,0 +1,20 @@
> +/*
> + * QEMU CPU Topology Representation Header
> + *
> + * Copyright (c) 2024 Intel Corporation
> + *
> + * Authors:
> + *  Zhao Liu <zhao1.liu@intel.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * later.  See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef CPU_TOPOLOGY_H
> +#define CPU_TOPOLOGY_H
> +
> +#include "qapi/qapi-types-machine.h"
> +
> +const char *cpu_topo_to_string(CPUTopoLevel topo);
> +
> +#endif /* CPU_TOPOLOGY_H */
> diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
> index dff49fce1154..c6ff75f23991 100644
> --- a/include/hw/i386/topology.h
> +++ b/include/hw/i386/topology.h
> @@ -39,7 +39,7 @@
>   *  CPUID Fn8000_0008_ECX[ApicIdCoreIdSize[3:0]] is set to apicid_core_width().
>   */
>  
> -
> +#include "hw/core/cpu-topology.h"
>  #include "qemu/bitops.h"
>  
>  /*
> @@ -62,22 +62,6 @@ typedef struct X86CPUTopoInfo {
>      unsigned threads_per_core;
>  } X86CPUTopoInfo;
>  
> -/*
> - * CPUTopoLevel is the general i386 topology hierarchical representation,
> - * ordered by increasing hierarchical relationship.
> - * Its enumeration value is not bound to the type value of Intel (CPUID[0x1F])
> - * or AMD (CPUID[0x80000026]).
> - */
> -enum CPUTopoLevel {
> -    CPU_TOPO_LEVEL_INVALID,
> -    CPU_TOPO_LEVEL_SMT,
> -    CPU_TOPO_LEVEL_CORE,
> -    CPU_TOPO_LEVEL_MODULE,
> -    CPU_TOPO_LEVEL_DIE,
> -    CPU_TOPO_LEVEL_PACKAGE,
> -    CPU_TOPO_LEVEL_MAX,
> -};
> -
>  /* Return the bit width needed for 'count' IDs */
>  static unsigned apicid_bitwidth_for_count(unsigned count)
>  {
> diff --git a/qapi/machine.json b/qapi/machine.json
> index bce6e1bbc412..7ac5a05bb9c9 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1667,6 +1667,46 @@
>       '*reboot-timeout': 'int',
>       '*strict': 'bool' } }
>  
> +##
> +# @CPUTopoLevel:
> +#
> +# An enumeration of CPU topology levels.
> +#
> +# @invalid: Invalid topology level, used as a placeholder.

Placeholder for what?

> +#
> +# @thread: thread level, which would also be called SMT level or logical
> +#     processor level. The @threads option in -smp is used to configure
> +#     the topology of this level.
> +#
> +# @core: core level. The @cores option in -smp is used to configure the
> +#     topology of this level.
> +#
> +# @module: module level. The @modules option in -smp is used to
> +#     configure the topology of this level.
> +#
> +# @cluster: cluster level. The @clusters option in -smp is used to
> +#     configure the topology of this level.
> +#
> +# @die: die level. The @dies option in -smp is used to configure the
> +#     topology of this level.
> +#
> +# @socket: socket level, which would also be called package level. The
> +#     @sockets option in -smp is used to configure the topology of this
> +#     level.
> +#
> +# @book: book level. The @books option in -smp is used to configure the
> +#     topology of this level.
> +#
> +# @drawer: drawer level. The @drawers option in -smp is used to
> +#     configure the topology of this level.

As far as I can tell, -smp is sugar for machine property "smp" of QAPI
type SMPConfiguration.  Should we refer to SMPConfiguration instead of
-smp?

> +#
> +# Since: 9.1
> +##
> +{ 'enum': 'CPUTopoLevel',
> +  'prefix': 'CPU_TOPO_LEVEL',
> +  'data': [ 'invalid', 'thread', 'core', 'module', 'cluster',
> +            'die', 'socket', 'book', 'drawer' ] }
> +
>  ##
>  # @SMPConfiguration:
>  #

[...]

Re: [RFC v2 1/7] hw/core: Make CPU topology enumeration arch-agnostic

[Markus Armbruster]

Zhao Liu <zhao1.liu@intel.com> writes:

> Cache topology needs to be defined based on CPU topology levels. Thus,
> define CPU topology enumeration in qapi/machine.json to make it generic
> for all architectures.
>
> To match the general topology naming style, rename CPU_TOPO_LEVEL_SMT
> and CPU_TOPO_LEVEL_PACKAGE to CPU_TOPO_LEVEL_THREAD and
> CPU_TOPO_LEVEL_SOCKET.
>
> Also, enumerate additional topology levels for non-i386 arches, and add
> helpers for topology enumeration and string conversion.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>

[...]

> diff --git a/qapi/machine.json b/qapi/machine.json
> index bce6e1bbc412..7ac5a05bb9c9 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1667,6 +1667,46 @@
>       '*reboot-timeout': 'int',
>       '*strict': 'bool' } }
>  
> +##
> +# @CPUTopoLevel:

I understand you're moving existing enum CPUTopoLevel into the QAPI
schema.  I think the idiomatic QAPI name would be CpuTopologyLevel.
Would you be willing to rename it, or would that be too much churn?

> +#
> +# An enumeration of CPU topology levels.
> +#
> +# @invalid: Invalid topology level, used as a placeholder.
> +#
> +# @thread: thread level, which would also be called SMT level or logical
> +#     processor level. The @threads option in -smp is used to configure
> +#     the topology of this level.
> +#
> +# @core: core level. The @cores option in -smp is used to configure the
> +#     topology of this level.
> +#
> +# @module: module level. The @modules option in -smp is used to
> +#     configure the topology of this level.
> +#
> +# @cluster: cluster level. The @clusters option in -smp is used to
> +#     configure the topology of this level.
> +#
> +# @die: die level. The @dies option in -smp is used to configure the
> +#     topology of this level.
> +#
> +# @socket: socket level, which would also be called package level. The
> +#     @sockets option in -smp is used to configure the topology of this
> +#     level.
> +#
> +# @book: book level. The @books option in -smp is used to configure the
> +#     topology of this level.
> +#
> +# @drawer: drawer level. The @drawers option in -smp is used to
> +#     configure the topology of this level.

docs/devel/qapi-code-gen.rst section Documentation markup:

    For legibility, wrap text paragraphs so every line is at most 70
    characters long.

    Separate sentences with two spaces.

> +#
> +# Since: 9.1
> +##
> +{ 'enum': 'CPUTopoLevel',
> +  'prefix': 'CPU_TOPO_LEVEL',
> +  'data': [ 'invalid', 'thread', 'core', 'module', 'cluster',
> +            'die', 'socket', 'book', 'drawer' ] }
> +
>  ##
>  # @SMPConfiguration:
>  #

[...]

Re: [RFC v2 1/7] hw/core: Make CPU topology enumeration arch-agnostic

[Zhao Liu]

Hi Markus,

On Mon, Jun 03, 2024 at 02:25:36PM +0200, Markus Armbruster wrote:
> Date: Mon, 03 Jun 2024 14:25:36 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [RFC v2 1/7] hw/core: Make CPU topology enumeration
>  arch-agnostic
> 
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > Cache topology needs to be defined based on CPU topology levels. Thus,
> > define CPU topology enumeration in qapi/machine.json to make it generic
> > for all architectures.
> >
> > To match the general topology naming style, rename CPU_TOPO_LEVEL_SMT
> > and CPU_TOPO_LEVEL_PACKAGE to CPU_TOPO_LEVEL_THREAD and
> > CPU_TOPO_LEVEL_SOCKET.
> >
> > Also, enumerate additional topology levels for non-i386 arches, and add
> > helpers for topology enumeration and string conversion.
> >
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> 
> [...]
> 
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index bce6e1bbc412..7ac5a05bb9c9 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -1667,6 +1667,46 @@
> >       '*reboot-timeout': 'int',
> >       '*strict': 'bool' } }
> >  
> > +##
> > +# @CPUTopoLevel:
> 
> I understand you're moving existing enum CPUTopoLevel into the QAPI
> schema.  I think the idiomatic QAPI name would be CpuTopologyLevel.
> Would you be willing to rename it, or would that be too much churn?

Sure, I'll rename it as you suggested.

> > +#
> > +# An enumeration of CPU topology levels.
> > +#
> > +# @invalid: Invalid topology level, used as a placeholder.
> > +#
> > +# @thread: thread level, which would also be called SMT level or logical
> > +#     processor level. The @threads option in -smp is used to configure
> > +#     the topology of this level.
> > +#
> > +# @core: core level. The @cores option in -smp is used to configure the
> > +#     topology of this level.
> > +#
> > +# @module: module level. The @modules option in -smp is used to
> > +#     configure the topology of this level.
> > +#
> > +# @cluster: cluster level. The @clusters option in -smp is used to
> > +#     configure the topology of this level.
> > +#
> > +# @die: die level. The @dies option in -smp is used to configure the
> > +#     topology of this level.
> > +#
> > +# @socket: socket level, which would also be called package level. The
> > +#     @sockets option in -smp is used to configure the topology of this
> > +#     level.
> > +#
> > +# @book: book level. The @books option in -smp is used to configure the
> > +#     topology of this level.
> > +#
> > +# @drawer: drawer level. The @drawers option in -smp is used to
> > +#     configure the topology of this level.
> 
> docs/devel/qapi-code-gen.rst section Documentation markup:
> 
>     For legibility, wrap text paragraphs so every line is at most 70
>     characters long.
> 
>     Separate sentences with two spaces.

Thank you for pointing this.

About separating sentences, is this what I should be doing?

# @drawer: drawer level. The @drawers option in -smp is used to
#  configure the topology of this level.


Thanks,
Zhao

Re: [RFC v2 1/7] hw/core: Make CPU topology enumeration arch-agnostic

[Zhao Liu]

[snip]

> > +CPUTopoInfo cpu_topo_descriptors[] = {
> > +    [CPU_TOPO_LEVEL_INVALID] = { .name = "invalid", },
> > +    [CPU_TOPO_LEVEL_THREAD]  = { .name = "thread",  },
> > +    [CPU_TOPO_LEVEL_CORE]    = { .name = "core",    },
> > +    [CPU_TOPO_LEVEL_MODULE]  = { .name = "module",  },
> > +    [CPU_TOPO_LEVEL_CLUSTER] = { .name = "cluster", },
> > +    [CPU_TOPO_LEVEL_DIE]     = { .name = "die",     },
> > +    [CPU_TOPO_LEVEL_SOCKET]  = { .name = "socket",  },
> > +    [CPU_TOPO_LEVEL_BOOK]    = { .name = "book",    },
> > +    [CPU_TOPO_LEVEL_DRAWER]  = { .name = "drawer",  },
> > +    [CPU_TOPO_LEVEL__MAX]    = { .name = NULL,      },
> > +};
> 
> This looks redundant with generated
> 
>     const QEnumLookup CPUTopoLevel_lookup = {
>         .array = (const char *const[]) {
>             [CPU_TOPO_LEVEL_INVALID] = "invalid",
>             [CPU_TOPO_LEVEL_THREAD] = "thread",
>             [CPU_TOPO_LEVEL_CORE] = "core",
>             [CPU_TOPO_LEVEL_MODULE] = "module",
>             [CPU_TOPO_LEVEL_CLUSTER] = "cluster",
>             [CPU_TOPO_LEVEL_DIE] = "die",
>             [CPU_TOPO_LEVEL_SOCKET] = "socket",
>             [CPU_TOPO_LEVEL_BOOK] = "book",
>             [CPU_TOPO_LEVEL_DRAWER] = "drawer",
>         },
>         .size = CPU_TOPO_LEVEL__MAX
>     };
> 
> > +
> > +const char *cpu_topo_to_string(CPUTopoLevel topo)
> > +{
> > +    return cpu_topo_descriptors[topo].name;
> > +}
> 
> And this with generated CPUTopoLevel_str().

Thanks! I missed these generated helpers.

[snip]

> > +##
> > +# @CPUTopoLevel:
> > +#
> > +# An enumeration of CPU topology levels.
> > +#
> > +# @invalid: Invalid topology level, used as a placeholder.
> 
> Placeholder for what?

I was trying to express that when no specific topology level is
specified, it will be initialized to this value by default.

Or what about just deleting this placeholder related words and just
saying it's "Invalid topology level"?

> > +#
> > +# @thread: thread level, which would also be called SMT level or logical
> > +#     processor level. The @threads option in -smp is used to configure
> > +#     the topology of this level.
> > +#
> > +# @core: core level. The @cores option in -smp is used to configure the
> > +#     topology of this level.
> > +#
> > +# @module: module level. The @modules option in -smp is used to
> > +#     configure the topology of this level.
> > +#
> > +# @cluster: cluster level. The @clusters option in -smp is used to
> > +#     configure the topology of this level.
> > +#
> > +# @die: die level. The @dies option in -smp is used to configure the
> > +#     topology of this level.
> > +#
> > +# @socket: socket level, which would also be called package level. The
> > +#     @sockets option in -smp is used to configure the topology of this
> > +#     level.
> > +#
> > +# @book: book level. The @books option in -smp is used to configure the
> > +#     topology of this level.
> > +#
> > +# @drawer: drawer level. The @drawers option in -smp is used to
> > +#     configure the topology of this level.
> 
> As far as I can tell, -smp is sugar for machine property "smp" of QAPI
> type SMPConfiguration.  Should we refer to SMPConfiguration instead of
> -smp?

Yes, SMPConfiguration is better.

Thanks,
Zhao

Re: [RFC v2 1/7] hw/core: Make CPU topology enumeration arch-agnostic

[Markus Armbruster]

Zhao Liu <zhao1.liu@intel.com> writes:

[...]

>> > +##
>> > +# @CPUTopoLevel:
>> > +#
>> > +# An enumeration of CPU topology levels.
>> > +#
>> > +# @invalid: Invalid topology level, used as a placeholder.
>> 
>> Placeholder for what?
>
> I was trying to express that when no specific topology level is
> specified, it will be initialized to this value by default.
>
> Or what about just deleting this placeholder related words and just
> saying it's "Invalid topology level"?

Works for me.

[...]

Re: [RFC v2 1/7] hw/core: Make CPU topology enumeration arch-agnostic

[Markus Armbruster]

Zhao Liu <zhao1.liu@intel.com> writes:

> Hi Markus,
>
> On Mon, Jun 03, 2024 at 02:25:36PM +0200, Markus Armbruster wrote:
>> Date: Mon, 03 Jun 2024 14:25:36 +0200
>> From: Markus Armbruster <armbru@redhat.com>
>> Subject: Re: [RFC v2 1/7] hw/core: Make CPU topology enumeration
>>  arch-agnostic
>> 
>> Zhao Liu <zhao1.liu@intel.com> writes:
>> 
>> > Cache topology needs to be defined based on CPU topology levels. Thus,
>> > define CPU topology enumeration in qapi/machine.json to make it generic
>> > for all architectures.
>> >
>> > To match the general topology naming style, rename CPU_TOPO_LEVEL_SMT
>> > and CPU_TOPO_LEVEL_PACKAGE to CPU_TOPO_LEVEL_THREAD and
>> > CPU_TOPO_LEVEL_SOCKET.
>> >
>> > Also, enumerate additional topology levels for non-i386 arches, and add
>> > helpers for topology enumeration and string conversion.
>> >
>> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
>> 
>> [...]
>> 
>> > diff --git a/qapi/machine.json b/qapi/machine.json
>> > index bce6e1bbc412..7ac5a05bb9c9 100644
>> > --- a/qapi/machine.json
>> > +++ b/qapi/machine.json
>> > @@ -1667,6 +1667,46 @@
>> >       '*reboot-timeout': 'int',
>> >       '*strict': 'bool' } }
>> >  
>> > +##
>> > +# @CPUTopoLevel:
>> 
>> I understand you're moving existing enum CPUTopoLevel into the QAPI
>> schema.  I think the idiomatic QAPI name would be CpuTopologyLevel.
>> Would you be willing to rename it, or would that be too much churn?
>
> Sure, I'll rename it as you suggested.
>
>> > +#
>> > +# An enumeration of CPU topology levels.
>> > +#
>> > +# @invalid: Invalid topology level, used as a placeholder.
>> > +#
>> > +# @thread: thread level, which would also be called SMT level or logical
>> > +#     processor level. The @threads option in -smp is used to configure
>> > +#     the topology of this level.
>> > +#
>> > +# @core: core level. The @cores option in -smp is used to configure the
>> > +#     topology of this level.
>> > +#
>> > +# @module: module level. The @modules option in -smp is used to
>> > +#     configure the topology of this level.
>> > +#
>> > +# @cluster: cluster level. The @clusters option in -smp is used to
>> > +#     configure the topology of this level.
>> > +#
>> > +# @die: die level. The @dies option in -smp is used to configure the
>> > +#     topology of this level.
>> > +#
>> > +# @socket: socket level, which would also be called package level. The
>> > +#     @sockets option in -smp is used to configure the topology of this
>> > +#     level.
>> > +#
>> > +# @book: book level. The @books option in -smp is used to configure the
>> > +#     topology of this level.
>> > +#
>> > +# @drawer: drawer level. The @drawers option in -smp is used to
>> > +#     configure the topology of this level.
>> 
>> docs/devel/qapi-code-gen.rst section Documentation markup:
>> 
>>     For legibility, wrap text paragraphs so every line is at most 70
>>     characters long.
>> 
>>     Separate sentences with two spaces.
>
> Thank you for pointing this.
>
> About separating sentences, is this what I should be doing?
>
> # @drawer: drawer level. The @drawers option in -smp is used to
> #  configure the topology of this level.

Like this:

##
# @CPUTopoLevel:
#
# An enumeration of CPU topology levels.
#
# @invalid: Invalid topology level.
#
# @thread: thread level, which would also be called SMT level or
#     logical processor level.  The @threads option in -smp is used to
#     configure the topology of this level.
#
# @core: core level.  The @cores option in -smp is used to configure
#     the topology of this level.
#
# @module: module level.  The @modules option in -smp is used to
#     configure the topology of this level.
#
# @cluster: cluster level.  The @clusters option in -smp is used to
#     configure the topology of this level.
#
# @die: die level.  The @dies option in -smp is used to configure the
#     topology of this level.
#
# @socket: socket level, which would also be called package level.
#     The @sockets option in -smp is used to configure the topology of
#     this level.
#
# @book: book level.  The @books option in -smp is used to configure
#     the topology of this level.
#
# @drawer: drawer level.  The @drawers option in -smp is used to
#     configure the topology of this level.
#
# Since: 9.1
##

Re: [RFC v2 3/7] hw/core: Add cache topology options in -smp

[Markus Armbruster]

Zhao Liu <zhao1.liu@intel.com> writes:

> Add "l1d-cache", "l1i-cache". "l2-cache", and "l3-cache" options in
> -smp to define the cache topology for SMP system.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>

[...]

> diff --git a/qapi/machine.json b/qapi/machine.json
> index 7ac5a05bb9c9..8fa5af69b1bf 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1746,6 +1746,23 @@
>  #
>  # @threads: number of threads per core
>  #
> +# @l1d-cache: topology hierarchy of L1 data cache. It accepts the CPU
> +#     topology enumeration as the parameter, i.e., CPUs in the same
> +#     topology container share the same L1 data cache. (since 9.1)
> +#
> +# @l1i-cache: topology hierarchy of L1 instruction cache. It accepts
> +#     the CPU topology enumeration as the parameter, i.e., CPUs in the
> +#     same topology container share the same L1 instruction cache.
> +#     (since 9.1)
> +#
> +# @l2-cache: topology hierarchy of L2 unified cache. It accepts the CPU
> +#     topology enumeration as the parameter, i.e., CPUs in the same
> +#     topology container share the same L2 unified cache. (since 9.1)
> +#
> +# @l3-cache: topology hierarchy of L3 unified cache. It accepts the CPU
> +#     topology enumeration as the parameter, i.e., CPUs in the same
> +#     topology container share the same L3 unified cache. (since 9.1)
> +#
>  # Since: 6.1
>  ##

The new members are all optional.  What does "absent" mean?  No such
cache?  Some default topology?

Is this sufficiently general?  Do all machines of interest have a split
level 1 cache, a level 2 cache, and a level 3 cache?

Is the CPU topology level the only cache property we'll want to
configure here?  If the answer isn't "yes", then we should perhaps wrap
it in an object, so we can easily add more members later.

Two spaces between sentences for consistency, please.

>  { 'struct': 'SMPConfiguration', 'data': {
> @@ -1758,7 +1775,11 @@
>       '*modules': 'int',
>       '*cores': 'int',
>       '*threads': 'int',
> -     '*maxcpus': 'int' } }
> +     '*maxcpus': 'int',
> +     '*l1d-cache': 'CPUTopoLevel',
> +     '*l1i-cache': 'CPUTopoLevel',
> +     '*l2-cache': 'CPUTopoLevel',
> +     '*l3-cache': 'CPUTopoLevel' } }
>  
>  ##
>  # @x-query-irq:
> diff --git a/system/vl.c b/system/vl.c

[...]

Re: [RFC v2 1/7] hw/core: Make CPU topology enumeration arch-agnostic

[Zhao Liu]

On Tue, Jun 04, 2024 at 10:47:44AM +0200, Markus Armbruster wrote:
> Date: Tue, 04 Jun 2024 10:47:44 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [RFC v2 1/7] hw/core: Make CPU topology enumeration
>  arch-agnostic
> 
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > Hi Markus,
> >
> > On Mon, Jun 03, 2024 at 02:25:36PM +0200, Markus Armbruster wrote:
> >> Date: Mon, 03 Jun 2024 14:25:36 +0200
> >> From: Markus Armbruster <armbru@redhat.com>
> >> Subject: Re: [RFC v2 1/7] hw/core: Make CPU topology enumeration
> >>  arch-agnostic
> >> 
> >> Zhao Liu <zhao1.liu@intel.com> writes:
> >> 
> >> > Cache topology needs to be defined based on CPU topology levels. Thus,
> >> > define CPU topology enumeration in qapi/machine.json to make it generic
> >> > for all architectures.
> >> >
> >> > To match the general topology naming style, rename CPU_TOPO_LEVEL_SMT
> >> > and CPU_TOPO_LEVEL_PACKAGE to CPU_TOPO_LEVEL_THREAD and
> >> > CPU_TOPO_LEVEL_SOCKET.
> >> >
> >> > Also, enumerate additional topology levels for non-i386 arches, and add
> >> > helpers for topology enumeration and string conversion.
> >> >
> >> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> >> 
> >> [...]
> >> 
> >> > diff --git a/qapi/machine.json b/qapi/machine.json
> >> > index bce6e1bbc412..7ac5a05bb9c9 100644
> >> > --- a/qapi/machine.json
> >> > +++ b/qapi/machine.json
> >> > @@ -1667,6 +1667,46 @@
> >> >       '*reboot-timeout': 'int',
> >> >       '*strict': 'bool' } }
> >> >  
> >> > +##
> >> > +# @CPUTopoLevel:
> >> 
> >> I understand you're moving existing enum CPUTopoLevel into the QAPI
> >> schema.  I think the idiomatic QAPI name would be CpuTopologyLevel.
> >> Would you be willing to rename it, or would that be too much churn?
> >
> > Sure, I'll rename it as you suggested.
> >
> >> > +#
> >> > +# An enumeration of CPU topology levels.
> >> > +#
> >> > +# @invalid: Invalid topology level, used as a placeholder.
> >> > +#
> >> > +# @thread: thread level, which would also be called SMT level or logical
> >> > +#     processor level. The @threads option in -smp is used to configure
> >> > +#     the topology of this level.
> >> > +#
> >> > +# @core: core level. The @cores option in -smp is used to configure the
> >> > +#     topology of this level.
> >> > +#
> >> > +# @module: module level. The @modules option in -smp is used to
> >> > +#     configure the topology of this level.
> >> > +#
> >> > +# @cluster: cluster level. The @clusters option in -smp is used to
> >> > +#     configure the topology of this level.
> >> > +#
> >> > +# @die: die level. The @dies option in -smp is used to configure the
> >> > +#     topology of this level.
> >> > +#
> >> > +# @socket: socket level, which would also be called package level. The
> >> > +#     @sockets option in -smp is used to configure the topology of this
> >> > +#     level.
> >> > +#
> >> > +# @book: book level. The @books option in -smp is used to configure the
> >> > +#     topology of this level.
> >> > +#
> >> > +# @drawer: drawer level. The @drawers option in -smp is used to
> >> > +#     configure the topology of this level.
> >> 
> >> docs/devel/qapi-code-gen.rst section Documentation markup:
> >> 
> >>     For legibility, wrap text paragraphs so every line is at most 70
> >>     characters long.
> >> 
> >>     Separate sentences with two spaces.
> >
> > Thank you for pointing this.
> >
> > About separating sentences, is this what I should be doing?
> >
> > # @drawer: drawer level. The @drawers option in -smp is used to
> > #  configure the topology of this level.
> 
> Like this:
> 
> ##
> # @CPUTopoLevel:
> #
> # An enumeration of CPU topology levels.
> #
> # @invalid: Invalid topology level.
> #
> # @thread: thread level, which would also be called SMT level or
> #     logical processor level.  The @threads option in -smp is used to
> #     configure the topology of this level.
> #
> # @core: core level.  The @cores option in -smp is used to configure
> #     the topology of this level.
> #
> # @module: module level.  The @modules option in -smp is used to
> #     configure the topology of this level.
> #
> # @cluster: cluster level.  The @clusters option in -smp is used to
> #     configure the topology of this level.
> #
> # @die: die level.  The @dies option in -smp is used to configure the
> #     topology of this level.
> #
> # @socket: socket level, which would also be called package level.
> #     The @sockets option in -smp is used to configure the topology of
> #     this level.
> #
> # @book: book level.  The @books option in -smp is used to configure
> #     the topology of this level.
> #
> # @drawer: drawer level.  The @drawers option in -smp is used to
> #     configure the topology of this level.
> #
> # Since: 9.1
> ##
>

I see, thanks very much for your patience.

Re: [RFC v2 0/7] Introduce SMP Cache Topology

[Daniel P. Berrangé]

On Thu, May 30, 2024 at 06:15:32PM +0800, Zhao Liu wrote:
> Hi,
> 
> Now that the i386 cache model has been able to define the topology
> clearly, it's time to move on to discussing/advancing this feature about
> configuring the cache topology with -smp as the following example:
> 
> -smp 32,sockets=2,dies=2,modules=2,cores=2,threads=2,maxcpus=32,\
>      l1d-cache=core,l1i-cache=core,l2-cache=core,l3-cache=die
> 
> With the new cache topology options ("l1d-cache", "l1i-cache",
> "l2-cache" and "l3-cache"), we could adjust the cache topology via -smp.

Switching to QAPI for a second, your proposal is effectively

    { 'enum': 'SMPCacheTopo',
      'data': [ 'default','socket','die','cluster','module','core','thread'] }

   { 'struct': 'SMPConfiguration',
     'data': {
       '*cpus': 'int',
       '*drawers': 'int',
       '*books': 'int',
       '*sockets': 'int',
       '*dies': 'int',
       '*clusters': 'int',
       '*modules': 'int',
       '*cores': 'int',
       '*threads': 'int',
       '*maxcpus': 'int',
       '*l1d-cache': 'SMPCacheTopo',
       '*l1i-cache': 'SMPCacheTopo',
       '*l2-cache': 'SMPCacheTopo',
       '*l3-cache': 'SMPCacheTopo',
     } }

I think that would be more natural to express as an array of structs
thus:

    { 'enum': 'SMPCacheTopo',
      'data': [ 'default','socket','die','cluster','module','core','thread'] }

    { 'enum': 'SMPCacheType',
      'data': [ 'l1d', 'l1i', 'l2', 'l3' ] }
     
    { 'struct': 'SMPCache',
      'data': {
        'type': 'SMPCacheType',
        'topo': 'SMPCacheTopo',
      } }

   { 'struct': 'SMPConfiguration',
     'data': {
       '*cpus': 'int',
       '*drawers': 'int',
       '*books': 'int',
       '*sockets': 'int',
       '*dies': 'int',
       '*clusters': 'int',
       '*modules': 'int',
       '*cores': 'int',
       '*threads': 'int',
       '*maxcpus': 'int',
       'caches': [ 'SMPCache' ]
     } }

Giving an example in (hypothetical) JSON cli syntax of:

  -smp  "{'cpus':32,'sockets':2,'dies':2,'modules':2,
          'cores':2,'threads':2,'maxcpus':32,'caches': [
	    {'type':'l1d','topo':'core' },
	    {'type':'l1i','topo':'core' },
	    {'type':'l2','topo':'core' },
	    {'type':'l3','topo':'die' },
	  ]}"


> Open about How to Handle the Default Options
> ============================================
> 
> (For the detailed description of this series, pls skip this "long"
> section and review the subsequent content.)
> 
> 
> Background of OPEN
> ------------------
> 
> Daniel and I discussed initial thoughts on cache topology, and there was
> an idea that the default *cache_topo is on the CORE level [3]:
> 
> > simply preferring "cores" for everything is a reasonable
> > default long term plan for everything, unless the specific
> > architecture target has no concept of "cores".

FYI, when I wrote that I wasn't specifically thinking about cache
mappings. I just meant that when exposing SMP topology to guests,
'cores' is a good default, compared to 'sockets', or 'threads',etc.

Defaults for cache <-> topology  mappings should be whatever makes
most sense to the architecture target/cpu.

> Problem with this OPEN
> ----------------------
> 
> Some arches have their own arch-specific cache topology, such as l1 per
> core/l2 per core/l3 per die for i386. And as Jeehang proposed for
> RISC-V, the cache topologies are like: l1/l2 per core and l3 per
> cluster. 
> 
> Taking L3 as an example, logically there is a difference between the two
> starting points of user-specified core level and with the default core
> level.
> 
> For example,
> 
> "(user-specified) l3-cache-topo=core" should override i386's default l3
> per core, but i386's default l3 per core should also override
> "(default) l3-cache-topo=core" because this default value is like a
> placeholder that specifies nothing.
> 
> However, from a command line parsing perspective, it's impossible to
> tell what the “l3-cache-topo=core” setting is for...

Yes, we need to explicitly distinguish built-in defaults from
user specified data, otherwise we risk too many mistakes.

> Options to solve OPEN
> ---------------------
> 
> So, I think we have the following options:
> 
> 
> 1. Can we avoid such default parameters?
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> This would reduce the pain in QEMU, but I'm not sure if it's possible to
> make libvirt happy?

I think having an explicit "defualt" value is inevitable, not simply
because of libvirt. Long experiance with QEMU shows that we need to
be able to reliably distinguish direct user input from  built-in
defaults in cases like this.

> 
> It is also possible to expose Cache topology information as the CPU
> properties in “query-cpu-model-expansion type=full”, but that adds
> arch-specific work.
> 
> If omitted, I think it's just like omitting “cores”/“sockets”,
> leaving it up to the machine to decide based on the specific CPU model
> (and now the cache topology is indeed determined by the CPU model as
> well).
> 
> 
> 2. If default is required, can we use a specific abstract word?
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> That is, is it possible to use a specific word like “auto”/“invalid”
> /“default” and avoid a specific topology level?

"invalid" feels a bit wierd, but 'auto' or 'default' are fine,
and possibly "unspecified"

> Like setting “l3-cache-topo=invalid” (since I've only added the invalid
> hierarchy so far ;-) ).
> 
> I found the cache topology of arches varies so much that I'm sorry to
> say it's hard to have a uniform default cache topology.
> 
> 
> I apologize for the very lengthy note and appreciate you reviewing it
> here as well as your time!

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC v2 3/7] hw/core: Add cache topology options in -smp

[Daniel P. Berrangé]

On Tue, Jun 04, 2024 at 10:54:51AM +0200, Markus Armbruster wrote:
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > Add "l1d-cache", "l1i-cache". "l2-cache", and "l3-cache" options in
> > -smp to define the cache topology for SMP system.
> >
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> 
> [...]
> 
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index 7ac5a05bb9c9..8fa5af69b1bf 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -1746,6 +1746,23 @@
> >  #
> >  # @threads: number of threads per core
> >  #
> > +# @l1d-cache: topology hierarchy of L1 data cache. It accepts the CPU
> > +#     topology enumeration as the parameter, i.e., CPUs in the same
> > +#     topology container share the same L1 data cache. (since 9.1)
> > +#
> > +# @l1i-cache: topology hierarchy of L1 instruction cache. It accepts
> > +#     the CPU topology enumeration as the parameter, i.e., CPUs in the
> > +#     same topology container share the same L1 instruction cache.
> > +#     (since 9.1)
> > +#
> > +# @l2-cache: topology hierarchy of L2 unified cache. It accepts the CPU
> > +#     topology enumeration as the parameter, i.e., CPUs in the same
> > +#     topology container share the same L2 unified cache. (since 9.1)
> > +#
> > +# @l3-cache: topology hierarchy of L3 unified cache. It accepts the CPU
> > +#     topology enumeration as the parameter, i.e., CPUs in the same
> > +#     topology container share the same L3 unified cache. (since 9.1)
> > +#
> >  # Since: 6.1
> >  ##
> 
> The new members are all optional.  What does "absent" mean?  No such
> cache?  Some default topology?
> 
> Is this sufficiently general?  Do all machines of interest have a split
> level 1 cache, a level 2 cache, and a level 3 cache?

Level 4 cache is apparently a thing

https://www.guru3d.com/story/intel-confirms-l4-cache-in-upcoming-meteor-lake-cpus/

but given that any new cache levels will require new code in QEMU to
wire up, its not a big deal to add new properties at the same time.

That said see my reply just now to the cover letter, where I suggest
we should have a "caches" property that takes an array of cache
info objects.

> 
> Is the CPU topology level the only cache property we'll want to
> configure here?  If the answer isn't "yes", then we should perhaps wrap
> it in an object, so we can easily add more members later.

Cache size is a piece of info I could see us wanting to express

> Two spaces between sentences for consistency, please.
> 
> >  { 'struct': 'SMPConfiguration', 'data': {
> > @@ -1758,7 +1775,11 @@
> >       '*modules': 'int',
> >       '*cores': 'int',
> >       '*threads': 'int',
> > -     '*maxcpus': 'int' } }
> > +     '*maxcpus': 'int',
> > +     '*l1d-cache': 'CPUTopoLevel',
> > +     '*l1i-cache': 'CPUTopoLevel',
> > +     '*l2-cache': 'CPUTopoLevel',
> > +     '*l3-cache': 'CPUTopoLevel' } }
> >  
> >  ##
> >  # @x-query-irq:
> > diff --git a/system/vl.c b/system/vl.c
> 
> [...]
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC v2 0/7] Introduce SMP Cache Topology

[Zhao Liu]

Hi Daniel,

On Tue, Jun 04, 2024 at 10:29:15AM +0100, Daniel P. Berrangé wrote:
> Date: Tue, 4 Jun 2024 10:29:15 +0100
> From: "Daniel P. Berrangé" <berrange@redhat.com>
> Subject: Re: [RFC v2 0/7] Introduce SMP Cache Topology
> 
> On Thu, May 30, 2024 at 06:15:32PM +0800, Zhao Liu wrote:
> > Hi,
> > 
> > Now that the i386 cache model has been able to define the topology
> > clearly, it's time to move on to discussing/advancing this feature about
> > configuring the cache topology with -smp as the following example:
> > 
> > -smp 32,sockets=2,dies=2,modules=2,cores=2,threads=2,maxcpus=32,\
> >      l1d-cache=core,l1i-cache=core,l2-cache=core,l3-cache=die
> > 
> > With the new cache topology options ("l1d-cache", "l1i-cache",
> > "l2-cache" and "l3-cache"), we could adjust the cache topology via -smp.
> 
> Switching to QAPI for a second, your proposal is effectively
> 
>     { 'enum': 'SMPCacheTopo',
>       'data': [ 'default','socket','die','cluster','module','core','thread'] }
> 
>    { 'struct': 'SMPConfiguration',
>      'data': {
>        '*cpus': 'int',
>        '*drawers': 'int',
>        '*books': 'int',
>        '*sockets': 'int',
>        '*dies': 'int',
>        '*clusters': 'int',
>        '*modules': 'int',
>        '*cores': 'int',
>        '*threads': 'int',
>        '*maxcpus': 'int',
>        '*l1d-cache': 'SMPCacheTopo',
>        '*l1i-cache': 'SMPCacheTopo',
>        '*l2-cache': 'SMPCacheTopo',
>        '*l3-cache': 'SMPCacheTopo',
>      } }
> 
> I think that would be more natural to express as an array of structs
> thus:
> 
>     { 'enum': 'SMPCacheTopo',
>       'data': [ 'default','socket','die','cluster','module','core','thread'] }
> 
>     { 'enum': 'SMPCacheType',
>       'data': [ 'l1d', 'l1i', 'l2', 'l3' ] }
>      
>     { 'struct': 'SMPCache',
>       'data': {
>         'type': 'SMPCacheType',
>         'topo': 'SMPCacheTopo',
>       } }
> 
>    { 'struct': 'SMPConfiguration',
>      'data': {
>        '*cpus': 'int',
>        '*drawers': 'int',
>        '*books': 'int',
>        '*sockets': 'int',
>        '*dies': 'int',
>        '*clusters': 'int',
>        '*modules': 'int',
>        '*cores': 'int',
>        '*threads': 'int',
>        '*maxcpus': 'int',
>        'caches': [ 'SMPCache' ]
>      } }
> 
> Giving an example in (hypothetical) JSON cli syntax of:
> 
>   -smp  "{'cpus':32,'sockets':2,'dies':2,'modules':2,
>           'cores':2,'threads':2,'maxcpus':32,'caches': [
> 	    {'type':'l1d','topo':'core' },
> 	    {'type':'l1i','topo':'core' },
> 	    {'type':'l2','topo':'core' },
> 	    {'type':'l3','topo':'die' },
> 	  ]}"
 
Thanks! Looks clean to me and I think it is ok.

Just one further question, for this case where it must be expressed in a
raw JSON string, is there any requirement in QEMU that a simple
command-line friendly variant must be provided that corresponds to it?

> > Open about How to Handle the Default Options
> > ============================================
> > 
> > (For the detailed description of this series, pls skip this "long"
> > section and review the subsequent content.)
> > 
> > 
> > Background of OPEN
> > ------------------
> > 
> > Daniel and I discussed initial thoughts on cache topology, and there was
> > an idea that the default *cache_topo is on the CORE level [3]:
> > 
> > > simply preferring "cores" for everything is a reasonable
> > > default long term plan for everything, unless the specific
> > > architecture target has no concept of "cores".
> 
> FYI, when I wrote that I wasn't specifically thinking about cache
> mappings. I just meant that when exposing SMP topology to guests,
> 'cores' is a good default, compared to 'sockets', or 'threads',etc.
> 
> Defaults for cache <-> topology  mappings should be whatever makes
> most sense to the architecture target/cpu.

Thank you for the additional clarification!

> > Problem with this OPEN
> > ----------------------
> > 
> > Some arches have their own arch-specific cache topology, such as l1 per
> > core/l2 per core/l3 per die for i386. And as Jeehang proposed for
> > RISC-V, the cache topologies are like: l1/l2 per core and l3 per
> > cluster. 
> > 
> > Taking L3 as an example, logically there is a difference between the two
> > starting points of user-specified core level and with the default core
> > level.
> > 
> > For example,
> > 
> > "(user-specified) l3-cache-topo=core" should override i386's default l3
> > per core, but i386's default l3 per core should also override
> > "(default) l3-cache-topo=core" because this default value is like a
> > placeholder that specifies nothing.
> > 
> > However, from a command line parsing perspective, it's impossible to
> > tell what the “l3-cache-topo=core” setting is for...
> 
> Yes, we need to explicitly distinguish built-in defaults from
> user specified data, otherwise we risk too many mistakes.
> 
> > Options to solve OPEN
> > ---------------------
> > 
> > So, I think we have the following options:
> > 
> > 
> > 1. Can we avoid such default parameters?
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > This would reduce the pain in QEMU, but I'm not sure if it's possible to
> > make libvirt happy?
> 
> I think having an explicit "defualt" value is inevitable, not simply
> because of libvirt. Long experiance with QEMU shows that we need to
> be able to reliably distinguish direct user input from  built-in
> defaults in cases like this.

Thanks, that gives me an answer to that question!

> > 
> > It is also possible to expose Cache topology information as the CPU
> > properties in “query-cpu-model-expansion type=full”, but that adds
> > arch-specific work.
> > 
> > If omitted, I think it's just like omitting “cores”/“sockets”,
> > leaving it up to the machine to decide based on the specific CPU model
> > (and now the cache topology is indeed determined by the CPU model as
> > well).
> > 
> > 
> > 2. If default is required, can we use a specific abstract word?
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > That is, is it possible to use a specific word like “auto”/“invalid”
> > /“default” and avoid a specific topology level?
> 
> "invalid" feels a bit wierd, but 'auto' or 'default' are fine,
> and possibly "unspecified"

I prefer the "default" like you listed in your QAPI example. :)

> > Like setting “l3-cache-topo=invalid” (since I've only added the invalid
> > hierarchy so far ;-) ).
> > 
> > I found the cache topology of arches varies so much that I'm sorry to
> > say it's hard to have a uniform default cache topology.
> > 
> > 
> > I apologize for the very lengthy note and appreciate you reviewing it
> > here as well as your time!

Thanks,
Zhao

Re: [RFC v2 3/7] hw/core: Add cache topology options in -smp

[Zhao Liu]

Hi Markus and Daniel,

(I replied to both of you because I discussed the idea of cache object
at the end)

On Tue, Jun 04, 2024 at 10:32:14AM +0100, Daniel P. Berrangé wrote:
> Date: Tue, 4 Jun 2024 10:32:14 +0100
> From: "Daniel P. Berrangé" <berrange@redhat.com>
> Subject: Re: [RFC v2 3/7] hw/core: Add cache topology options in -smp
> 
> On Tue, Jun 04, 2024 at 10:54:51AM +0200, Markus Armbruster wrote:
> > Zhao Liu <zhao1.liu@intel.com> writes:
> > 
> > > Add "l1d-cache", "l1i-cache". "l2-cache", and "l3-cache" options in
> > > -smp to define the cache topology for SMP system.
> > >
> > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > 
> > [...]
> > 
> > > diff --git a/qapi/machine.json b/qapi/machine.json
> > > index 7ac5a05bb9c9..8fa5af69b1bf 100644
> > > --- a/qapi/machine.json
> > > +++ b/qapi/machine.json
> > > @@ -1746,6 +1746,23 @@
> > >  #
> > >  # @threads: number of threads per core
> > >  #
> > > +# @l1d-cache: topology hierarchy of L1 data cache. It accepts the CPU
> > > +#     topology enumeration as the parameter, i.e., CPUs in the same
> > > +#     topology container share the same L1 data cache. (since 9.1)
> > > +#
> > > +# @l1i-cache: topology hierarchy of L1 instruction cache. It accepts
> > > +#     the CPU topology enumeration as the parameter, i.e., CPUs in the
> > > +#     same topology container share the same L1 instruction cache.
> > > +#     (since 9.1)
> > > +#
> > > +# @l2-cache: topology hierarchy of L2 unified cache. It accepts the CPU
> > > +#     topology enumeration as the parameter, i.e., CPUs in the same
> > > +#     topology container share the same L2 unified cache. (since 9.1)
> > > +#
> > > +# @l3-cache: topology hierarchy of L3 unified cache. It accepts the CPU
> > > +#     topology enumeration as the parameter, i.e., CPUs in the same
> > > +#     topology container share the same L3 unified cache. (since 9.1)
> > > +#
> > >  # Since: 6.1
> > >  ##
> > 
> > The new members are all optional.  What does "absent" mean?  No such
> > cache?  Some default topology?

I suppose "absent" means using the the default arch-specific cache topo.

For example, x86 has its own default cache topo. my purpose in providing
this interface is to allow users to override the default (arch-specific)
cache topology.

Daniel suggested in cover letter's reply that I could add the value
“default”, I think omitting it would have the same effect as setting it
to “default”, and omitting it shouldn't be regarded as disabling a
particular cache, since the interface is only about the topology!

> > Is this sufficiently general?  Do all machines of interest have a split
> > level 1 cache, a level 2 cache, and a level 3 cache?

The different cache support can be extended by such control fields in
MachineClass:

typedef struct {
    ...
    bool l1_separated_cache_supported;
    bool l2_unified_cache_supported;
    bool l3_unified_cache_supported;
} SMPCompatProps;

For example, if a machine with l1 unified cache appears, it can add the
"l1_unified_cache_supported", and add "l1" option in QAPI.

Then separate L1 cache has “l1i” and “l1d” options, and unified L1 cache
should have “l1” option.

> Level 4 cache is apparently a thing
> 
> https://www.guru3d.com/story/intel-confirms-l4-cache-in-upcoming-meteor-lake-cpus/
> 
> but given that any new cache levels will require new code in QEMU to
> wire up, its not a big deal to add new properties at the same time.
> 
> That said see my reply just now to the cover letter, where I suggest
> we should have a "caches" property that takes an array of cache
> info objects.

Yes, I agree.

> > 
> > Is the CPU topology level the only cache property we'll want to
> > configure here?  If the answer isn't "yes", then we should perhaps wrap
> > it in an object, so we can easily add more members later.
> 
> Cache size is a piece of info I could see us wanting to express

For the simplicity and clarity, I think it's better to only cover
topology in -smp, including the current CPU topology and the cache
topology I'm working on.

I've thought about the idea of converting the cache into the device,
which in turn could define more properties for the cache.

This one is mostly in connection with the previous qom-topo idea (aka
abstracting CPU topology device [1]):

    -device cpu-socket,id=sock0 \
    -device cpu-die,id=die0,parent=sock0 \
    -device cpu-core,id=core0,parent=die0,nr-threads=2 \
    -device cpu-cache,id=cache0,parent=core0,level=l1,type=inst \
    -device cpu-cache,id=cache1,parent=core0,level=l1,type=data \
    -device cpu-cache,id=cache2,parent=core0,level=l2,type=unif \
    -device cpu-cache,id=cache3,parent=die0,level=l3,type=unif

Cache device idea was mainly to support hybrid cache topology, because
Intel client hybrid architecture has hybrid cache topology (on MTL,
compute die has a L3 and SOC die has no L3).

Based on such a cache device, we can define more properties for the
cache and also meet flexible topology requirements at the same time.

This cache device I had planned to implement after the cpu topo device.
It's a long and hard way to go, I wonder if this future I'm portraying
will alleviate your concerns about more cache properties support. ;-)

Soon I'm also planning to refresh the qom-topo series again, reducing
hack changes, deleting dead codes, etc. Maybe it shouldn't be called
qom-topo, because I want to display topology tree in qom path by
qdev_set_id().

[1]: https://lore.kernel.org/qemu-devel/20231130144203.2307629-1-zhao1.liu@linux.intel.com/

> > Two spaces between sentences for consistency, please.
> >

Sure, thanks!

Regards,
Zhao