[PATCH 0/8] Introduce SMP Cache Topology

[PATCH 0/8] Introduce SMP Cache Topology

[Zhao Liu]

Hi all,

Since the previous RFC v2, I've reimplemented smp-cache object based on
Daniel's comment (thanks Daniel!), which is both flexible to support
current cache topology requirements and extensible.

So, I officially convert the RFC to PATCH.

Background on smp cache topology can be found in the previous RFC v2
cover letter:

https://lore.kernel.org/qemu-devel/20240530101539.768484-1-zhao1.liu@intel.com/

The following content focuses on this series implementation of the
smp-cache object.


About smp-cache
===============

In fact, the smp-cache object introduced in this series is a bit
different from Daniel's original suggestion. Instead of being integrated
into -smp, it is created explicitly via -object, and the smp-cache
property is added to -machine to link to this object.

An example is as follows:

-object '{"qom-type":"smp-cache","id":"cache0","caches":[{"name":"l1d","topo":"core"},{"name":"l1i","topo":"core"},{"name":"l2","topo":"module"},{"name":"l3","topo":"socket"}]}'
-machine smp-cache=cache0


Such the design change is based on the following 2 reasons:
 * Defining the cache with a JSON list is very extensible and can
   support more cache properties.

 * But -smp, as the sugar of machine property "smp", is based on keyval
   format, and doesn't support options with JSON format. Thus it's not
   possible to add a JSON format based option to -smp/-machine for now.

So, I decoupled the creation of the smp-cache object from the -machine
to make both -machine and -object happy!


Welcome your feedback!


Thanks and Best Regards,
Zhao
---
Changelog:

Main changes since RFC v2:
 * Dropped cpu-topology.h and cpu-topology.c since QAPI has the helper
   (CpuTopologyLevel_str) to convert enum to string. (Markus)
 * Fixed text format in machine.json (CpuTopologyLevel naming, 2 spaces
   between sentences). (Markus)
 * Added a new level "default" to de-compatibilize some arch-specific
   topo settings. (Daniel)
 * Moved CpuTopologyLevel to qapi/machine-common.json, at where the
   cache enumeration and smp-cache object would be added.
   - If smp-cache object is defined in qapi/machine.json, storage-daemon
     will complain about the qmp cmds in qapi/machine.json during
     compiling.
 * Referred to Daniel's suggestion to introduce cache JSON list, though
   as a standalone object since -smp/-machine can't support JSON.
 * Linked machine's smp_cache to smp-cache object instead of a builtin
   structure. This is to get around the fact that the keyval format of
   -machine can't support JSON.
 * Wrapped the cache topology level access into a helper.
 * Split as a separate commit to just include compatibility checking and
   topology checking.
 * Allow setting "default" topology level even though the cache
   isn't supported by machine. (Daniel)
 * Rewrote the document of smp-cache object.

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 (8):
  hw/core: Make CPU topology enumeration arch-agnostic
  qapi/qom: Introduce smp-cache object
  hw/core: Add smp cache topology for machine
  hw/core: Check smp cache topology support for machine
  i386/cpu: Support thread and module level cache topology
  i386/cpu: Update cache topology with machine's configuration
  i386/pc: Support cache topology in -machine for PC machine
  qemu-options: Add the description of smp-cache object

 MAINTAINERS                 |   2 +
 hw/core/machine-smp.c       |  86 ++++++++++++++++++++++++++++++
 hw/core/machine.c           |  22 ++++++++
 hw/core/meson.build         |   1 +
 hw/core/smp-cache.c         | 103 ++++++++++++++++++++++++++++++++++++
 hw/i386/pc.c                |   4 ++
 include/hw/boards.h         |  11 +++-
 include/hw/core/smp-cache.h |  27 ++++++++++
 include/hw/i386/topology.h  |  18 +------
 qapi/machine-common.json    |  97 ++++++++++++++++++++++++++++++++-
 qapi/qapi-schema.json       |   4 +-
 qapi/qom.json               |   3 ++
 qemu-options.hx             |  58 ++++++++++++++++++++
 target/i386/cpu.c           |  83 +++++++++++++++++++++--------
 target/i386/cpu.h           |   4 +-
 15 files changed, 478 insertions(+), 45 deletions(-)
 create mode 100644 hw/core/smp-cache.c
 create mode 100644 include/hw/core/smp-cache.h

-- 
2.34.1

[PATCH 1/8] 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
a CPU_TOPO_LEVEL_DEFAULT to help future smp-cache object de-compatibilize
arch-specific cache topology settings.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC v2:
 * Dropped cpu-topology.h and cpu-topology.c since QAPI has the helper
   (CpuTopologyLevel_str) to convert enum to string. (Markus)
 * Fixed text format in machine.json (CpuTopologyLevel naming, 2 spaces
   between sentences). (Markus)
 * Added a new level "default" to de-compatibilize some arch-specific
   topo settings. (Daniel)
 * Moved CpuTopologyLevel to qapi/machine-common.json, at where the
   cache enumeration and smp-cache object would be added.
   - If smp-cache object is defined in qapi/machine.json, storage-daemon
     will complain about the qmp cmds in qapi/machine.json during
     compiling.

Changes since RFC v1:
 * Used QAPI to enumerate CPU topology levels.
 * Dropped string_to_cpu_topo() since QAPI will help to parse the topo
   levels.
---
 include/hw/i386/topology.h | 18 +--------------
 qapi/machine-common.json   | 47 +++++++++++++++++++++++++++++++++++++-
 target/i386/cpu.c          | 38 +++++++++++++++---------------
 target/i386/cpu.h          |  4 ++--
 4 files changed, 68 insertions(+), 39 deletions(-)

diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
index dff49fce1154..10d05ff045c7 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 "qapi/qapi-types-machine-common.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-common.json b/qapi/machine-common.json
index fa6bd71d1280..82413c668bdb 100644
--- a/qapi/machine-common.json
+++ b/qapi/machine-common.json
@@ -5,7 +5,7 @@
 # See the COPYING file in the top-level directory.
 
 ##
-# = Machines S390 data types
+# = Common machine types
 ##
 
 ##
@@ -19,3 +19,48 @@
 { 'enum': 'CpuS390Entitlement',
   'prefix': 'S390_CPU_ENTITLEMENT',
   'data': [ 'auto', 'low', 'medium', 'high' ] }
+
+##
+# @CpuTopologyLevel:
+#
+# 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
+#     SMPConfiguration is used to configure the topology of this
+#     level.
+#
+# @core: core level.  The @cores option in SMPConfiguration is used
+#     to configure the topology of this level.
+#
+# @module: module level.  The @modules option in SMPConfiguration is
+#     used to configure the topology of this level.
+#
+# @cluster: cluster level.  The @clusters option in SMPConfiguration
+#     is used to configure the topology of this level.
+#
+# @die: die level.  The @dies option in SMPConfiguration is used to
+#     configure the topology of this level.
+#
+# @socket: socket level, which would also be called package level.
+#     The @sockets option in SMPConfiguration is used to configure
+#     the topology of this level.
+#
+# @book: book level.  The @books option in SMPConfiguration is used
+#     to configure the topology of this level.
+#
+# @drawer: drawer level.  The @drawers option in SMPConfiguration is
+#     used to configure the topology of this level.
+#
+# @default: default level.  Some architectures will have default
+#     topology settings (e.g., cache topology), and this special
+#     level means following the architecture-specific settings.
+#
+# Since: 9.1
+##
+{ 'enum': 'CpuTopologyLevel',
+  'prefix': 'CPU_TOPO_LEVEL',
+  'data': [ 'invalid', 'thread', 'core', 'module', 'cluster',
+            'die', 'socket', 'book', 'drawer', 'default' ] }
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 4c2e6f3a71e9..4ae3bbf30682 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -236,7 +236,7 @@ static uint8_t cpuid2_cache_descriptor(CPUCacheInfo *cache)
                        0 /* Invalid value */)
 
 static uint32_t max_thread_ids_for_cache(X86CPUTopoInfo *topo_info,
-                                         enum CPUTopoLevel share_level)
+                                         enum CpuTopologyLevel share_level)
 {
     uint32_t num_ids = 0;
 
@@ -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();
@@ -301,10 +301,10 @@ static void encode_cache_cpuid4(CPUCacheInfo *cache,
 }
 
 static uint32_t num_threads_by_topo_level(X86CPUTopoInfo *topo_info,
-                                          enum CPUTopoLevel topo_level)
+                                          enum CpuTopologyLevel 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:
@@ -323,10 +323,10 @@ static uint32_t num_threads_by_topo_level(X86CPUTopoInfo *topo_info,
 }
 
 static uint32_t apicid_offset_by_topo_level(X86CPUTopoInfo *topo_info,
-                                            enum CPUTopoLevel topo_level)
+                                            enum CpuTopologyLevel 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();
@@ -342,12 +342,12 @@ static uint32_t apicid_offset_by_topo_level(X86CPUTopoInfo *topo_info,
     return 0;
 }
 
-static uint32_t cpuid1f_topo_type(enum CPUTopoLevel topo_level)
+static uint32_t cpuid1f_topo_type(enum CpuTopologyLevel 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);
@@ -6462,7 +6462,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)) {
@@ -7963,10 +7963,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 29daf3704857..9ddba249aa9e 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1653,7 +1653,7 @@ typedef struct CPUCacheInfo {
      * Used to encode CPUID[4].EAX[bits 25:14] or
      * CPUID[0x8000001D].EAX[bits 25:14].
      */
-    enum CPUTopoLevel share_level;
+    CpuTopologyLevel share_level;
 } CPUCacheInfo;
 
 
@@ -1979,7 +1979,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

[PATCH 2/8] qapi/qom: Introduce smp-cache object

[Zhao Liu]

Introduce smp-cache object so that user could define cache properties.

In smp-cache object, define cache topology based on CPU topology level
with 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, enumerate the L1 D-cache, L1 I-cache, L2 cache and L3 cache
with smp-cache object to add the basic cache topology support.

Suggested-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Suggested by credit:
 * Referred to Daniel's suggestion to introduce cache JSON list, though
   as a standalone object since -smp/-machine can't support JSON.
---
Changes since RFC v2:
 * New commit to implement cache list with JSON format instead of
   multiple sub-options in -smp.
---
 MAINTAINERS                 |   2 +
 hw/core/meson.build         |   1 +
 hw/core/smp-cache.c         | 103 ++++++++++++++++++++++++++++++++++++
 include/hw/core/smp-cache.h |  27 ++++++++++
 qapi/machine-common.json    |  50 +++++++++++++++++
 qapi/qapi-schema.json       |   4 +-
 qapi/qom.json               |   3 ++
 7 files changed, 188 insertions(+), 2 deletions(-)
 create mode 100644 hw/core/smp-cache.c
 create mode 100644 include/hw/core/smp-cache.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 6725913c8b3a..b5391a7538de 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1885,12 +1885,14 @@ F: hw/core/machine.c
 F: hw/core/machine-smp.c
 F: hw/core/null-machine.c
 F: hw/core/numa.c
+F: hw/core/smp-cache.c
 F: hw/cpu/cluster.c
 F: qapi/machine.json
 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/smp-cache.h
 F: include/hw/cpu/cluster.h
 F: include/sysemu/numa.h
 F: tests/unit/test-smp-parse.c
diff --git a/hw/core/meson.build b/hw/core/meson.build
index a3d9bab9f42a..6d3dae3af62e 100644
--- a/hw/core/meson.build
+++ b/hw/core/meson.build
@@ -14,6 +14,7 @@ hwcore_ss.add(files(
 
 common_ss.add(files('cpu-common.c'))
 common_ss.add(files('machine-smp.c'))
+common_ss.add(files('smp-cache.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'))
 system_ss.add(when: 'CONFIG_GUEST_LOADER', if_true: files('guest-loader.c'))
diff --git a/hw/core/smp-cache.c b/hw/core/smp-cache.c
new file mode 100644
index 000000000000..c0157ce51c8f
--- /dev/null
+++ b/hw/core/smp-cache.c
@@ -0,0 +1,103 @@
+/*
+ * Cache Object for SMP machine
+ *
+ * Copyright (C) 2024 Intel Corporation.
+ *
+ * Author: 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/smp-cache.h"
+#include "qapi/error.h"
+#include "qapi/qapi-visit-machine-common.h"
+#include "qom/object_interfaces.h"
+
+static void
+smp_cache_get_cache_prop(Object *obj, Visitor *v, const char *name,
+                         void *opaque, Error **errp)
+{
+    SMPCache *cache = SMP_CACHE(obj);
+    SMPCachePropertyList *head = NULL;
+    SMPCachePropertyList **tail = &head;
+
+    for (int i = 0; i < SMP_CACHE__MAX; i++) {
+        SMPCacheProperty *node = g_new(SMPCacheProperty, 1);
+
+        node->name = cache->props[i].name;
+        node->topo = cache->props[i].topo;
+        QAPI_LIST_APPEND(tail, node);
+    }
+
+    if (!visit_type_SMPCachePropertyList(v, name, &head, errp)) {
+        return;
+    }
+    qapi_free_SMPCachePropertyList(head);
+}
+
+static void
+smp_cache_set_cache_prop(Object *obj, Visitor *v, const char *name,
+                         void *opaque, Error **errp)
+{
+    SMPCache *cache = SMP_CACHE(obj);
+    SMPCachePropertyList *list, *node;
+
+    if (!visit_type_SMPCachePropertyList(v, name, &list, errp)) {
+        return;
+    }
+
+    for (node = list; node; node = node->next) {
+        if (node->value->topo == CPU_TOPO_LEVEL_INVALID) {
+            error_setg(errp,
+                       "Invalid topology level: %s. "
+                       "The topology should match the valid CPU topology level",
+                       CpuTopologyLevel_str(node->value->topo));
+            goto out;
+        }
+        cache->props[node->value->name].topo = node->value->topo;
+    }
+
+out:
+    qapi_free_SMPCachePropertyList(list);
+}
+
+static void smp_cache_class_init(ObjectClass *oc, void *data)
+{
+    object_class_property_add(oc, "caches", "SMPCacheProperties",
+                              smp_cache_get_cache_prop,
+                              smp_cache_set_cache_prop,
+                              NULL, NULL);
+    object_class_property_set_description(oc, "caches",
+            "Cache property list for SMP machine");
+}
+
+static void smp_cache_instance_init(Object *obj)
+{
+    SMPCache *cache = SMP_CACHE(obj);
+    for (int i = 0; i < SMP_CACHE__MAX; i++) {
+        cache->props[i].name = (SMPCacheName)i;
+        cache->props[i].topo = CPU_TOPO_LEVEL_DEFAULT;
+    }
+}
+
+static const TypeInfo smp_cache_info = {
+    .parent = TYPE_OBJECT,
+    .name = TYPE_SMP_CACHE,
+    .class_init = smp_cache_class_init,
+    .instance_size = sizeof(SMPCache),
+    .instance_init = smp_cache_instance_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_USER_CREATABLE },
+        { }
+    }
+};
+
+static void smp_cache_register_type(void)
+{
+    type_register_static(&smp_cache_info);
+}
+
+type_init(smp_cache_register_type);
diff --git a/include/hw/core/smp-cache.h b/include/hw/core/smp-cache.h
new file mode 100644
index 000000000000..c6b4d9efc290
--- /dev/null
+++ b/include/hw/core/smp-cache.h
@@ -0,0 +1,27 @@
+/*
+ * Cache Object for SMP machine
+ *
+ * Copyright (C) 2024 Intel Corporation.
+ *
+ * Author: 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 SMP_CACHE_H
+#define SMP_CACHE_H
+
+#include "qapi/qapi-types-machine-common.h"
+#include "qom/object.h"
+
+#define TYPE_SMP_CACHE "smp-cache"
+OBJECT_DECLARE_SIMPLE_TYPE(SMPCache, SMP_CACHE)
+
+struct SMPCache {
+    Object parent_obj;
+
+    SMPCacheProperty props[SMP_CACHE__MAX];
+};
+
+#endif /* SMP_CACHE_H */
diff --git a/qapi/machine-common.json b/qapi/machine-common.json
index 82413c668bdb..8b8c0e9eeb86 100644
--- a/qapi/machine-common.json
+++ b/qapi/machine-common.json
@@ -64,3 +64,53 @@
   'prefix': 'CPU_TOPO_LEVEL',
   'data': [ 'invalid', 'thread', 'core', 'module', 'cluster',
             'die', 'socket', 'book', 'drawer', 'default' ] }
+
+##
+# @SMPCacheName:
+#
+# An enumeration of cache for SMP systems.  The cache name here is
+# a combination of cache level and cache type.
+#
+# @l1d: L1 data cache.
+#
+# @l1i: L1 instruction cache.
+#
+# @l2: L2 (unified) cache.
+#
+# @l3: L3 (unified) cache
+#
+# Since: 9.1
+##
+{ 'enum': 'SMPCacheName',
+  'prefix': 'SMP_CACHE',
+  'data': [ 'l1d', 'l1i', 'l2', 'l3' ] }
+
+##
+# @SMPCacheProperty:
+#
+# Cache information for SMP systems.
+#
+# @name: Cache name.
+#
+# @topo: Cache topology level.  It accepts the CPU topology
+#     enumeration as the parameter, i.e., CPUs in the same
+#     topology container share the same cache.
+#
+# Since: 9.1
+##
+{ 'struct': 'SMPCacheProperty',
+  'data': {
+  'name': 'SMPCacheName',
+  'topo': 'CpuTopologyLevel' } }
+
+##
+# @SMPCacheProperties:
+#
+# List wrapper of SMPCacheProperty.
+#
+# @caches: the SMPCacheProperty list.
+#
+# Since 9.1
+##
+{ 'struct': 'SMPCacheProperties',
+  'data': { 'caches': ['SMPCacheProperty'] } }
diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
index b1581988e4eb..25394f2cda50 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -64,11 +64,11 @@
 { 'include': 'compat.json' }
 { 'include': 'control.json' }
 { 'include': 'introspect.json' }
-{ 'include': 'qom.json' }
-{ 'include': 'qdev.json' }
 { 'include': 'machine-common.json' }
 { 'include': 'machine.json' }
 { 'include': 'machine-target.json' }
+{ 'include': 'qom.json' }
+{ 'include': 'qdev.json' }
 { 'include': 'replay.json' }
 { 'include': 'yank.json' }
 { 'include': 'misc.json' }
diff --git a/qapi/qom.json b/qapi/qom.json
index 8bd299265e39..797dd58a61f5 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -8,6 +8,7 @@
 { 'include': 'block-core.json' }
 { 'include': 'common.json' }
 { 'include': 'crypto.json' }
+{ 'include': 'machine-common.json' }
 
 ##
 # = QEMU Object Model (QOM)
@@ -1064,6 +1065,7 @@
       'if': 'CONFIG_SECRET_KEYRING' },
     'sev-guest',
     'sev-snp-guest',
+    'smp-cache',
     'thread-context',
     's390-pv-guest',
     'throttle-group',
@@ -1135,6 +1137,7 @@
                                       'if': 'CONFIG_SECRET_KEYRING' },
       'sev-guest':                  'SevGuestProperties',
       'sev-snp-guest':              'SevSnpGuestProperties',
+      'smp-cache':                  'SMPCacheProperties',
       'thread-context':             'ThreadContextProperties',
       'throttle-group':             'ThrottleGroupProperties',
       'tls-creds-anon':             'TlsCredsAnonProperties',
-- 
2.34.1

[PATCH 3/8] hw/core: Add smp cache topology for machine

[Zhao Liu]

With smp-cache object support, add smp cache topology for machine by
linking the smp-cache object.

Also add a helper to access cache topology level.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC v2:
 * Linked machine's smp_cache to smp-cache object instead of a builtin
   structure. This is to get around the fact that the keyval format of
   -machine can't support JSON.
 * Wrapped the cache topology level access into a helper.
---
 hw/core/machine-smp.c | 6 ++++++
 hw/core/machine.c     | 9 +++++++++
 include/hw/boards.h   | 5 ++++-
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 5d8d7edcbd3f..88a73743eb1c 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -270,3 +270,9 @@ unsigned int machine_topo_get_threads_per_socket(const MachineState *ms)
 {
     return ms->smp.threads * machine_topo_get_cores_per_socket(ms);
 }
+
+CpuTopologyLevel machine_get_cache_topo_level(const MachineState *ms,
+                                              SMPCacheName cache)
+{
+    return ms->smp_cache->props[cache].topo;
+}
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 655d75c21fc1..09ef9fcd4a0b 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1045,6 +1045,15 @@ static void machine_class_init(ObjectClass *oc, void *data)
     object_class_property_set_description(oc, "smp",
         "CPU topology");
 
+    /* TODO: Implement check() method based on machine support. */
+    object_class_property_add_link(oc, "smp-cache",
+                                   TYPE_SMP_CACHE,
+                                   offsetof(MachineState, smp_cache),
+                                   object_property_allow_set_link,
+                                   OBJ_PROP_LINK_STRONG);
+    object_class_property_set_description(oc, "smp-cache",
+        "SMP cache property");
+
     object_class_property_add(oc, "phandle-start", "int",
         machine_get_phandle_start, machine_set_phandle_start,
         NULL, NULL);
diff --git a/include/hw/boards.h b/include/hw/boards.h
index ef6f18f2c1a7..56fa252cfcd2 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -6,10 +6,10 @@
 #include "exec/memory.h"
 #include "sysemu/hostmem.h"
 #include "sysemu/blockdev.h"
-#include "qapi/qapi-types-machine.h"
 #include "qemu/module.h"
 #include "qom/object.h"
 #include "hw/core/cpu.h"
+#include "hw/core/smp-cache.h"
 
 #define TYPE_MACHINE_SUFFIX "-machine"
 
@@ -45,6 +45,8 @@ void machine_parse_smp_config(MachineState *ms,
                               const SMPConfiguration *config, Error **errp);
 unsigned int machine_topo_get_cores_per_socket(const MachineState *ms);
 unsigned int machine_topo_get_threads_per_socket(const MachineState *ms);
+CpuTopologyLevel machine_get_cache_topo_level(const MachineState *ms,
+                                              SMPCacheName cache);
 void machine_memory_devices_init(MachineState *ms, hwaddr base, uint64_t size);
 
 /**
@@ -409,6 +411,7 @@ struct MachineState {
     AccelState *accelerator;
     CPUArchIdList *possible_cpus;
     CpuTopology smp;
+    SMPCache *smp_cache;
     struct NVDIMMState *nvdimms_state;
     struct NumaState *numa_state;
 };
-- 
2.34.1

[PATCH 4/8] hw/core: Check smp cache topology support for machine

[Zhao Liu]

Add cache_supported flags in SMPCompatProps to allow machines to
configure various caches support.

And implement check() method for machine's "smp-cache" link property,
which will check the compatibility of the cache properties with the
machine support.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC v2:
 * Split as a separate commit to just include compatibility checking and
   topology checking.
 * Allow setting "default" topology level even though the cache
   isn't supported by machine. (Daniel)
---
 hw/core/machine-smp.c | 80 +++++++++++++++++++++++++++++++++++++++++++
 hw/core/machine.c     | 17 +++++++--
 include/hw/boards.h   |  6 ++++
 3 files changed, 101 insertions(+), 2 deletions(-)

diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 88a73743eb1c..bf6f2f91070d 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -276,3 +276,83 @@ CpuTopologyLevel machine_get_cache_topo_level(const MachineState *ms,
 {
     return ms->smp_cache->props[cache].topo;
 }
+
+static bool machine_check_topo_support(MachineState *ms,
+                                       CpuTopologyLevel topo,
+                                       Error **errp)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+
+    if ((topo == CPU_TOPO_LEVEL_MODULE && !mc->smp_props.modules_supported) ||
+        (topo == CPU_TOPO_LEVEL_CLUSTER && !mc->smp_props.clusters_supported) ||
+        (topo == CPU_TOPO_LEVEL_DIE && !mc->smp_props.dies_supported) ||
+        (topo == CPU_TOPO_LEVEL_BOOK && !mc->smp_props.books_supported) ||
+        (topo == CPU_TOPO_LEVEL_DRAWER && !mc->smp_props.drawers_supported)) {
+        error_setg(errp,
+                   "Invalid topology level: %s. "
+                   "The topology level is not supported by this machine",
+                   CpuTopologyLevel_str(topo));
+        return false;
+    }
+
+    return true;
+}
+
+/*
+ * When both cache1 and cache2 are configured with specific topology levels
+ * (not default level), is cache1's topology level higher than cache2?
+ */
+static bool smp_cache_topo_cmp(const SMPCache *smp_cache,
+                               SMPCacheName cache1,
+                               SMPCacheName cache2)
+{
+    if (smp_cache->props[cache1].topo != CPU_TOPO_LEVEL_DEFAULT &&
+        smp_cache->props[cache1].topo > smp_cache->props[cache2].topo) {
+        return true;
+    }
+    return false;
+}
+
+bool machine_check_smp_cache_support(MachineState *ms,
+                                     const SMPCache *smp_cache,
+                                     Error **errp)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+
+    for (int i = 0; i < SMP_CACHE__MAX; i++) {
+        const SMPCacheProperty *prop = &smp_cache->props[i];
+
+        /*
+         * Allow setting "default" topology level even though the cache
+         * isn't supported by machine.
+         */
+        if (prop->topo != CPU_TOPO_LEVEL_DEFAULT &&
+            !mc->smp_props.cache_supported[prop->name]) {
+            error_setg(errp,
+                       "%s cache topology not supported by this machine",
+                       SMPCacheName_str(prop->name));
+            return false;
+        }
+
+        if (!machine_check_topo_support(ms, prop->topo, errp)) {
+            return false;
+        }
+    }
+
+    if (smp_cache_topo_cmp(smp_cache, SMP_CACHE_L1D, SMP_CACHE_L2) ||
+        smp_cache_topo_cmp(smp_cache, SMP_CACHE_L1I, SMP_CACHE_L2)) {
+        error_setg(errp,
+                   "Invalid smp cache topology. "
+                   "L2 cache topology level shouldn't be lower than L1 cache");
+        return false;
+    }
+
+    if (smp_cache_topo_cmp(smp_cache, SMP_CACHE_L2, SMP_CACHE_L3)) {
+        error_setg(errp,
+                   "Invalid smp cache topology. "
+                   "L3 cache topology level shouldn't be lower than L2 cache");
+        return false;
+    }
+
+    return true;
+}
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 09ef9fcd4a0b..802dd56ba717 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -926,6 +926,20 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name,
     machine_parse_smp_config(ms, config, errp);
 }
 
+static void machine_check_smp_cache(const Object *obj, const char *name,
+                                    Object *child, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+    SMPCache *smp_cache = SMP_CACHE(child);
+
+    if (ms->smp_cache) {
+        error_setg(errp, "Invalid smp cache property. which has been set");
+        return;
+    }
+
+    machine_check_smp_cache_support(ms, smp_cache, errp);
+}
+
 static void machine_get_boot(Object *obj, Visitor *v, const char *name,
                             void *opaque, Error **errp)
 {
@@ -1045,11 +1059,10 @@ static void machine_class_init(ObjectClass *oc, void *data)
     object_class_property_set_description(oc, "smp",
         "CPU topology");
 
-    /* TODO: Implement check() method based on machine support. */
     object_class_property_add_link(oc, "smp-cache",
                                    TYPE_SMP_CACHE,
                                    offsetof(MachineState, smp_cache),
-                                   object_property_allow_set_link,
+                                   machine_check_smp_cache,
                                    OBJ_PROP_LINK_STRONG);
     object_class_property_set_description(oc, "smp-cache",
         "SMP cache property");
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 56fa252cfcd2..5455848c3e58 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -47,6 +47,9 @@ unsigned int machine_topo_get_cores_per_socket(const MachineState *ms);
 unsigned int machine_topo_get_threads_per_socket(const MachineState *ms);
 CpuTopologyLevel machine_get_cache_topo_level(const MachineState *ms,
                                               SMPCacheName cache);
+bool machine_check_smp_cache_support(MachineState *ms,
+                                     const SMPCache *smp_cache,
+                                     Error **errp);
 void machine_memory_devices_init(MachineState *ms, hwaddr base, uint64_t size);
 
 /**
@@ -147,6 +150,8 @@ 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
+ * @cache_supported - whether cache topologies (l1d, l1i, l2 and l3) are
+ *                    supported by the machine
  */
 typedef struct {
     bool prefer_sockets;
@@ -156,6 +161,7 @@ typedef struct {
     bool books_supported;
     bool drawers_supported;
     bool modules_supported;
+    bool cache_supported[SMP_CACHE__MAX];
 } SMPCompatProps;
 
 /**
-- 
2.34.1

[PATCH 5/8] 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 4ae3bbf30682..0ddbfa577caf 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

[PATCH 6/8] i386/cpu: Update cache topology with machine's configuration

[Zhao Liu]

User will configure smp cache topology via smp-cache object.

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

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC v2:
 * Used smp_cache array to override cache topology.
 * Wrapped the updating into a function.
---
 target/i386/cpu.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 0ddbfa577caf..403a089111ca 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7568,6 +7568,38 @@ static void x86_cpu_hyperv_realize(X86CPU *cpu)
     cpu->hyperv_limits[2] = 0;
 }
 
+#ifndef CONFIG_USER_ONLY
+static void x86_cpu_update_smp_cache_topo(MachineState *ms, X86CPU *cpu)
+{
+    CPUX86State *env = &cpu->env;
+    CpuTopologyLevel level;
+
+    level = machine_get_cache_topo_level(ms, SMP_CACHE_L1D);
+    if (level != CPU_TOPO_LEVEL_DEFAULT) {
+        env->cache_info_cpuid4.l1d_cache->share_level = level;
+        env->cache_info_amd.l1d_cache->share_level = level;
+    }
+
+    level = machine_get_cache_topo_level(ms, SMP_CACHE_L1I);
+    if (level != CPU_TOPO_LEVEL_DEFAULT) {
+        env->cache_info_cpuid4.l1i_cache->share_level = level;
+        env->cache_info_amd.l1i_cache->share_level = level;
+    }
+
+    level = machine_get_cache_topo_level(ms, SMP_CACHE_L2);
+    if (level != CPU_TOPO_LEVEL_DEFAULT) {
+        env->cache_info_cpuid4.l2_cache->share_level = level;
+        env->cache_info_amd.l2_cache->share_level = level;
+    }
+
+    level = machine_get_cache_topo_level(ms, SMP_CACHE_L3);
+    if (level != CPU_TOPO_LEVEL_DEFAULT) {
+        env->cache_info_cpuid4.l3_cache->share_level = level;
+        env->cache_info_amd.l3_cache->share_level = level;
+    }
+}
+#endif
+
 static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
@@ -7792,6 +7824,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
 #ifndef CONFIG_USER_ONLY
     MachineState *ms = MACHINE(qdev_get_machine());
+
+    if (ms->smp_cache) {
+        x86_cpu_update_smp_cache_topo(ms, cpu);
+    }
+
     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

[PATCH 7/8] i386/pc: Support cache topology in -machine for PC machine

[Zhao Liu]

Allow user to configure l1d, l1i, l2 and l3 cache topologies for PC
machine.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC v2:
 * Used cache_supported array.
---
 hw/i386/pc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 77415064c62e..1614a3b1bf19 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1773,6 +1773,10 @@ 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.cache_supported[SMP_CACHE_L1D] = true;
+    mc->smp_props.cache_supported[SMP_CACHE_L1I] = true;
+    mc->smp_props.cache_supported[SMP_CACHE_L2] = true;
+    mc->smp_props.cache_supported[SMP_CACHE_L3] = true;
     mc->default_ram_id = "pc.ram";
     pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_AUTO;
 
-- 
2.34.1

[PATCH 8/8] qemu-options: Add the description of smp-cache object

[Zhao Liu]

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes since RFC v2:
 * Rewrote the document of smp-cache object.

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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index 8ca7f34ef0c8..4b84f4508a6e 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -159,6 +159,15 @@ SRST
         ::
 
             -machine cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=128G,cxl-fmw.0.interleave-granularity=512
+
+    ``smp-cache='id'``
+        Allows to configure cache property (now only the cache topology level).
+
+        For example:
+        ::
+
+            -object '{"qom-type":"smp-cache","id":"cache","caches":[{"name":"l1d","topo":"core"},{"name":"l1i","topo":"core"},{"name":"l2","topo":"module"},{"name":"l3","topo":"die"}]}'
+            -machine smp-cache=cache
 ERST
 
 DEF("M", HAS_ARG, QEMU_OPTION_M,
@@ -5871,6 +5880,55 @@ SRST
         ::
 
             (qemu) qom-set /objects/iothread1 poll-max-ns 100000
+
+    ``-object '{"qom-type":"smp-cache","id":id,"caches":[{"name":cache_name,"topo":cache_topo}]}'``
+        Create an smp-cache object that configures machine's cache
+        property. Currently, cache property only include cache topology
+        level.
+
+        This option must be written in JSON format to support JSON list.
+
+        The ``caches`` parameter accepts a list of cache property in JSON
+        format.
+
+        A list element requires the cache name to be specified in the
+        ``name`` parameter (currently ``l1d``, ``l1i``, ``l2`` and ``l3``
+        are supported). ``topo`` parameter accepts CPU topology levels
+        including ``thread``, ``core``, ``module``, ``cluster``, ``die``,
+        ``socket``, ``book``, ``drawer`` and ``default``. The ``topo``
+        parameter indicates CPUs winthin the same CPU topology container
+        are sharing the same cache.
+
+        Some machines may have their own cache topology model, and this
+        object may override the machine-specific cache topology setting
+        by specifying smp-cache object in the -machine. When specifying
+        the cache topology level of ``default``, it will honor the default
+        machine-specific cache topology setting. For other topology levels,
+        they will override the default setting.
+
+        An example list of caches to configure the cache model (l1d cache
+        per core, l1i cache per core, l2 cache per module and l3 cache per
+        socket) supported by PC machine might look like:
+
+        ::
+
+              {
+                "caches": [
+                   { "name": "l1d", "topo": "core" },
+                   { "name": "l1i", "topo": "core" },
+                   { "name": "l2", "topo": "module" },
+                   { "name": "l3", "topo": "socket" },
+                ]
+              }
+
+        An example smp-cache object would look like:()
+
+        .. parsed-literal::
+
+             # |qemu_system| \\
+                 ... \\
+                 -object '{"qom-type":"smp-cache","id":id,"caches":[{"name":cache_name,"topo":cache_topo}]}' \\
+                 ...
 ERST
 
 
-- 
2.34.1

Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object

[Zhao Liu]

> diff --git a/hw/core/smp-cache.c b/hw/core/smp-cache.c
> new file mode 100644
> index 000000000000..c0157ce51c8f
> --- /dev/null
> +++ b/hw/core/smp-cache.c
> @@ -0,0 +1,103 @@
> +/*
> + * Cache Object for SMP machine
> + *
> + * Copyright (C) 2024 Intel Corporation.
> + *
> + * Author: 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/smp-cache.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-visit-machine-common.h"
> +#include "qom/object_interfaces.h"
> +
> +static void
> +smp_cache_get_cache_prop(Object *obj, Visitor *v, const char *name,
> +                         void *opaque, Error **errp)
> +{
> +    SMPCache *cache = SMP_CACHE(obj);
> +    SMPCachePropertyList *head = NULL;
> +    SMPCachePropertyList **tail = &head;
> +
> +    for (int i = 0; i < SMP_CACHE__MAX; i++) {
> +        SMPCacheProperty *node = g_new(SMPCacheProperty, 1);
> +
> +        node->name = cache->props[i].name;
> +        node->topo = cache->props[i].topo;
> +        QAPI_LIST_APPEND(tail, node);
> +    }
> +
> +    if (!visit_type_SMPCachePropertyList(v, name, &head, errp)) {
> +        return;

Oops, here I shouldn't return. Whether it succeeds or not, I should
continue with the following free().

> +    }
> +    qapi_free_SMPCachePropertyList(head);
> +}

Re: [PATCH 0/8] Introduce SMP Cache Topology

[Zhao Liu]

Hi Daniel and Markus,

A gentle ping. Would you kindly have a look at this version of the API
design? If it could meet your satisfaction, I’ll continue iterating.

Thanks,
Zhao

On Thu, Jul 04, 2024 at 11:15:55AM +0800, Zhao Liu wrote:
> Date: Thu, 4 Jul 2024 11:15:55 +0800
> From: Zhao Liu <zhao1.liu@intel.com>
> Subject: [PATCH 0/8] Introduce SMP Cache Topology
> X-Mailer: git-send-email 2.34.1
> 
> Hi all,
> 
> Since the previous RFC v2, I've reimplemented smp-cache object based on
> Daniel's comment (thanks Daniel!), which is both flexible to support
> current cache topology requirements and extensible.
> 
> So, I officially convert the RFC to PATCH.
> 
> Background on smp cache topology can be found in the previous RFC v2
> cover letter:
> 
> https://lore.kernel.org/qemu-devel/20240530101539.768484-1-zhao1.liu@intel.com/
> 
> The following content focuses on this series implementation of the
> smp-cache object.
> 
> 
> About smp-cache
> ===============
> 
> In fact, the smp-cache object introduced in this series is a bit
> different from Daniel's original suggestion. Instead of being integrated
> into -smp, it is created explicitly via -object, and the smp-cache
> property is added to -machine to link to this object.
> 
> An example is as follows:
> 
> -object '{"qom-type":"smp-cache","id":"cache0","caches":[{"name":"l1d","topo":"core"},{"name":"l1i","topo":"core"},{"name":"l2","topo":"module"},{"name":"l3","topo":"socket"}]}'
> -machine smp-cache=cache0
> 
> 
> Such the design change is based on the following 2 reasons:
>  * Defining the cache with a JSON list is very extensible and can
>    support more cache properties.
> 
>  * But -smp, as the sugar of machine property "smp", is based on keyval
>    format, and doesn't support options with JSON format. Thus it's not
>    possible to add a JSON format based option to -smp/-machine for now.
> 
> So, I decoupled the creation of the smp-cache object from the -machine
> to make both -machine and -object happy!
> 
> 
> Welcome your feedback!
> 
> 
> Thanks and Best Regards,
> Zhao
> ---
> Changelog:
> 
> Main changes since RFC v2:
>  * Dropped cpu-topology.h and cpu-topology.c since QAPI has the helper
>    (CpuTopologyLevel_str) to convert enum to string. (Markus)
>  * Fixed text format in machine.json (CpuTopologyLevel naming, 2 spaces
>    between sentences). (Markus)
>  * Added a new level "default" to de-compatibilize some arch-specific
>    topo settings. (Daniel)
>  * Moved CpuTopologyLevel to qapi/machine-common.json, at where the
>    cache enumeration and smp-cache object would be added.
>    - If smp-cache object is defined in qapi/machine.json, storage-daemon
>      will complain about the qmp cmds in qapi/machine.json during
>      compiling.
>  * Referred to Daniel's suggestion to introduce cache JSON list, though
>    as a standalone object since -smp/-machine can't support JSON.
>  * Linked machine's smp_cache to smp-cache object instead of a builtin
>    structure. This is to get around the fact that the keyval format of
>    -machine can't support JSON.
>  * Wrapped the cache topology level access into a helper.
>  * Split as a separate commit to just include compatibility checking and
>    topology checking.
>  * Allow setting "default" topology level even though the cache
>    isn't supported by machine. (Daniel)
>  * Rewrote the document of smp-cache object.
> 
> 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 (8):
>   hw/core: Make CPU topology enumeration arch-agnostic
>   qapi/qom: Introduce smp-cache object
>   hw/core: Add smp cache topology for machine
>   hw/core: Check smp cache topology support for machine
>   i386/cpu: Support thread and module level cache topology
>   i386/cpu: Update cache topology with machine's configuration
>   i386/pc: Support cache topology in -machine for PC machine
>   qemu-options: Add the description of smp-cache object
> 
>  MAINTAINERS                 |   2 +
>  hw/core/machine-smp.c       |  86 ++++++++++++++++++++++++++++++
>  hw/core/machine.c           |  22 ++++++++
>  hw/core/meson.build         |   1 +
>  hw/core/smp-cache.c         | 103 ++++++++++++++++++++++++++++++++++++
>  hw/i386/pc.c                |   4 ++
>  include/hw/boards.h         |  11 +++-
>  include/hw/core/smp-cache.h |  27 ++++++++++
>  include/hw/i386/topology.h  |  18 +------
>  qapi/machine-common.json    |  97 ++++++++++++++++++++++++++++++++-
>  qapi/qapi-schema.json       |   4 +-
>  qapi/qom.json               |   3 ++
>  qemu-options.hx             |  58 ++++++++++++++++++++
>  target/i386/cpu.c           |  83 +++++++++++++++++++++--------
>  target/i386/cpu.h           |   4 +-
>  15 files changed, 478 insertions(+), 45 deletions(-)
>  create mode 100644 hw/core/smp-cache.c
>  create mode 100644 include/hw/core/smp-cache.h
> 
> -- 
> 2.34.1
> 

Re: [PATCH 0/8] Introduce SMP Cache Topology

[Michael S. Tsirkin]

On Mon, Jul 22, 2024 at 03:33:37PM +0800, Zhao Liu wrote:
> Hi Daniel and Markus,
> 
> A gentle ping. Would you kindly have a look at this version of the API
> design? If it could meet your satisfaction, I’ll continue iterating.
> 
> Thanks,
> Zhao


I'm not really a QMP guy, you want Markus.

-- 
MST

Re: [PATCH 1/8] 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
> a CPU_TOPO_LEVEL_DEFAULT to help future smp-cache object de-compatibilize
> arch-specific cache topology settings.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>

QAPI schema
Acked-by: Markus Armbruster <armbru@redhat.com>

> ---
> Changes since RFC v2:
>  * Dropped cpu-topology.h and cpu-topology.c since QAPI has the helper
>    (CpuTopologyLevel_str) to convert enum to string. (Markus)
>  * Fixed text format in machine.json (CpuTopologyLevel naming, 2 spaces
>    between sentences). (Markus)
>  * Added a new level "default" to de-compatibilize some arch-specific
>    topo settings. (Daniel)
>  * Moved CpuTopologyLevel to qapi/machine-common.json, at where the
>    cache enumeration and smp-cache object would be added.
>    - If smp-cache object is defined in qapi/machine.json, storage-daemon
>      will complain about the qmp cmds in qapi/machine.json during
>      compiling.

At some point, we may have to rethink the split between machine.json,
machine-target.json, and machine-common.json.  Not this patch's problem.

Re: [PATCH 1/8] hw/core: Make CPU topology enumeration arch-agnostic

[Markus Armbruster]

One little thing...

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
> a CPU_TOPO_LEVEL_DEFAULT to help future smp-cache object de-compatibilize
> arch-specific cache topology settings.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>

[...]

> diff --git a/qapi/machine-common.json b/qapi/machine-common.json
> index fa6bd71d1280..82413c668bdb 100644
> --- a/qapi/machine-common.json
> +++ b/qapi/machine-common.json
> @@ -5,7 +5,7 @@
>  # See the COPYING file in the top-level directory.
>  
>  ##
> -# = Machines S390 data types
> +# = Common machine types
>  ##
>  
>  ##
> @@ -19,3 +19,48 @@
>  { 'enum': 'CpuS390Entitlement',
>    'prefix': 'S390_CPU_ENTITLEMENT',
>    'data': [ 'auto', 'low', 'medium', 'high' ] }
> +
> +##
> +# @CpuTopologyLevel:
> +#
> +# 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
> +#     SMPConfiguration is used to configure the topology of this
> +#     level.
> +#
> +# @core: core level.  The @cores option in SMPConfiguration is used
> +#     to configure the topology of this level.
> +#
> +# @module: module level.  The @modules option in SMPConfiguration is
> +#     used to configure the topology of this level.
> +#
> +# @cluster: cluster level.  The @clusters option in SMPConfiguration
> +#     is used to configure the topology of this level.
> +#
> +# @die: die level.  The @dies option in SMPConfiguration is used to
> +#     configure the topology of this level.
> +#
> +# @socket: socket level, which would also be called package level.
> +#     The @sockets option in SMPConfiguration is used to configure
> +#     the topology of this level.
> +#
> +# @book: book level.  The @books option in SMPConfiguration is used
> +#     to configure the topology of this level.
> +#
> +# @drawer: drawer level.  The @drawers option in SMPConfiguration is
> +#     used to configure the topology of this level.
> +#
> +# @default: default level.  Some architectures will have default
> +#     topology settings (e.g., cache topology), and this special
> +#     level means following the architecture-specific settings.
> +#
> +# Since: 9.1
> +##
> +{ 'enum': 'CpuTopologyLevel',
> +  'prefix': 'CPU_TOPO_LEVEL',

Why set a 'prefix'?

> +  'data': [ 'invalid', 'thread', 'core', 'module', 'cluster',
> +            'die', 'socket', 'book', 'drawer', 'default' ] }

[...]

Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object

[Markus Armbruster]

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

> Introduce smp-cache object so that user could define cache properties.
>
> In smp-cache object, define cache topology based on CPU topology level
> with 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, enumerate the L1 D-cache, L1 I-cache, L2 cache and L3 cache
> with smp-cache object to add the basic cache topology support.
>
> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>

[...]

> diff --git a/qapi/machine-common.json b/qapi/machine-common.json
> index 82413c668bdb..8b8c0e9eeb86 100644
> --- a/qapi/machine-common.json
> +++ b/qapi/machine-common.json
> @@ -64,3 +64,53 @@
>    'prefix': 'CPU_TOPO_LEVEL',
>    'data': [ 'invalid', 'thread', 'core', 'module', 'cluster',
>              'die', 'socket', 'book', 'drawer', 'default' ] }
> +
> +##
> +# @SMPCacheName:

Why the SMP in this name?  Because it's currently only used by SMP
stuff?  Or is there another reason I'm missing?

The more idiomatic QAPI name would be SmpCacheName.  Likewise for the
other type names below.

> +#
> +# An enumeration of cache for SMP systems.  The cache name here is
> +# a combination of cache level and cache type.

The first sentence feels awkward.  Maybe

   # Caches an SMP system may have.

> +#
> +# @l1d: L1 data cache.
> +#
> +# @l1i: L1 instruction cache.
> +#
> +# @l2: L2 (unified) cache.
> +#
> +# @l3: L3 (unified) cache
> +#
> +# Since: 9.1
> +##

This assumes the L1 cache is split, and L2 and L3 are unified.

If we model a system with say a unified L1 cache, we'd simply extend
this enum.  No real difference to extending it for additional levels.
Correct?

> +{ 'enum': 'SMPCacheName',
> +  'prefix': 'SMP_CACHE',

Why not call it SmpCache, and ditch 'prefix'?

> +  'data': [ 'l1d', 'l1i', 'l2', 'l3' ] }

> +
> +##
> +# @SMPCacheProperty:

Sure we want to call this "property" (singular) and not "properties"?
What if we add members to this type?

> +#
> +# Cache information for SMP systems.
> +#
> +# @name: Cache name.
> +#
> +# @topo: Cache topology level.  It accepts the CPU topology
> +#     enumeration as the parameter, i.e., CPUs in the same
> +#     topology container share the same cache.
> +#
> +# Since: 9.1
> +##
> +{ 'struct': 'SMPCacheProperty',
> +  'data': {
> +  'name': 'SMPCacheName',
> +  'topo': 'CpuTopologyLevel' } }

We tend to avoid abbreviations in the QAPI schema.  Please consider
naming this 'topology'.

> +
> +##
> +# @SMPCacheProperties:
> +#
> +# List wrapper of SMPCacheProperty.
> +#
> +# @caches: the SMPCacheProperty list.
> +#
> +# Since 9.1
> +##
> +{ 'struct': 'SMPCacheProperties',
> +  'data': { 'caches': ['SMPCacheProperty'] } }

Ah, now I see why you used the singular above!

However, this type holds the properties of call caches.  It is a list
where each element holds the properties of a single cache.  Calling the
former "cache property" and the latter "cache properties" is confusing.

SmpCachesProperties and SmpCacheProperties would put the singular
vs. plural where it belongs.  Sounds a bit awkward to me, though.
Naming is hard.

Other ideas, anybody?

> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> index b1581988e4eb..25394f2cda50 100644
> --- a/qapi/qapi-schema.json
> +++ b/qapi/qapi-schema.json
> @@ -64,11 +64,11 @@
>  { 'include': 'compat.json' }
>  { 'include': 'control.json' }
>  { 'include': 'introspect.json' }
> -{ 'include': 'qom.json' }
> -{ 'include': 'qdev.json' }
>  { 'include': 'machine-common.json' }
>  { 'include': 'machine.json' }
>  { 'include': 'machine-target.json' }
> +{ 'include': 'qom.json' }
> +{ 'include': 'qdev.json' }
>  { 'include': 'replay.json' }
>  { 'include': 'yank.json' }
>  { 'include': 'misc.json' }

Worth explaining in the commit message, I think.

> diff --git a/qapi/qom.json b/qapi/qom.json
> index 8bd299265e39..797dd58a61f5 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -8,6 +8,7 @@
>  { 'include': 'block-core.json' }
>  { 'include': 'common.json' }
>  { 'include': 'crypto.json' }
> +{ 'include': 'machine-common.json' }
>  
>  ##
>  # = QEMU Object Model (QOM)
> @@ -1064,6 +1065,7 @@
>        'if': 'CONFIG_SECRET_KEYRING' },
>      'sev-guest',
>      'sev-snp-guest',
> +    'smp-cache',
>      'thread-context',
>      's390-pv-guest',
>      'throttle-group',
> @@ -1135,6 +1137,7 @@
>                                        'if': 'CONFIG_SECRET_KEYRING' },
>        'sev-guest':                  'SevGuestProperties',
>        'sev-snp-guest':              'SevSnpGuestProperties',
> +      'smp-cache':                  'SMPCacheProperties',
>        'thread-context':             'ThreadContextProperties',
>        'throttle-group':             'ThrottleGroupProperties',
>        'tls-creds-anon':             'TlsCredsAnonProperties',

Re: [PATCH 8/8] qemu-options: Add the description of smp-cache object

[Markus Armbruster]

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

> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>

This patch is just documentation.  The code got added in some previous
patch.  Would it make sense to squash this patch into that previous
patch?

> ---
> Changes since RFC v2:
>  * Rewrote the document of smp-cache object.
>
> 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 8ca7f34ef0c8..4b84f4508a6e 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -159,6 +159,15 @@ SRST
>          ::
>  
>              -machine cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=128G,cxl-fmw.0.interleave-granularity=512
> +
> +    ``smp-cache='id'``
> +        Allows to configure cache property (now only the cache topology level).
> +
> +        For example:
> +        ::
> +
> +            -object '{"qom-type":"smp-cache","id":"cache","caches":[{"name":"l1d","topo":"core"},{"name":"l1i","topo":"core"},{"name":"l2","topo":"module"},{"name":"l3","topo":"die"}]}'
> +            -machine smp-cache=cache
>  ERST
>  
>  DEF("M", HAS_ARG, QEMU_OPTION_M,
> @@ -5871,6 +5880,55 @@ SRST
>          ::
>  
>              (qemu) qom-set /objects/iothread1 poll-max-ns 100000
> +
> +    ``-object '{"qom-type":"smp-cache","id":id,"caches":[{"name":cache_name,"topo":cache_topo}]}'``
> +        Create an smp-cache object that configures machine's cache
> +        property. Currently, cache property only include cache topology
> +        level.
> +
> +        This option must be written in JSON format to support JSON list.

Why?

> +
> +        The ``caches`` parameter accepts a list of cache property in JSON
> +        format.
> +
> +        A list element requires the cache name to be specified in the
> +        ``name`` parameter (currently ``l1d``, ``l1i``, ``l2`` and ``l3``
> +        are supported). ``topo`` parameter accepts CPU topology levels
> +        including ``thread``, ``core``, ``module``, ``cluster``, ``die``,
> +        ``socket``, ``book``, ``drawer`` and ``default``. The ``topo``
> +        parameter indicates CPUs winthin the same CPU topology container
> +        are sharing the same cache.
> +
> +        Some machines may have their own cache topology model, and this
> +        object may override the machine-specific cache topology setting
> +        by specifying smp-cache object in the -machine. When specifying
> +        the cache topology level of ``default``, it will honor the default
> +        machine-specific cache topology setting. For other topology levels,
> +        they will override the default setting.
> +
> +        An example list of caches to configure the cache model (l1d cache
> +        per core, l1i cache per core, l2 cache per module and l3 cache per
> +        socket) supported by PC machine might look like:
> +
> +        ::
> +
> +              {
> +                "caches": [
> +                   { "name": "l1d", "topo": "core" },
> +                   { "name": "l1i", "topo": "core" },
> +                   { "name": "l2", "topo": "module" },
> +                   { "name": "l3", "topo": "socket" },
> +                ]
> +              }
> +
> +        An example smp-cache object would look like:()
> +
> +        .. parsed-literal::
> +
> +             # |qemu_system| \\
> +                 ... \\
> +                 -object '{"qom-type":"smp-cache","id":id,"caches":[{"name":cache_name,"topo":cache_topo}]}' \\
> +                 ...
>  ERST

Re: [PATCH 1/8] hw/core: Make CPU topology enumeration arch-agnostic

[Zhao Liu]

Hi Markus,

On Mon, Jul 22, 2024 at 03:24:24PM +0200, Markus Armbruster wrote:
> Date: Mon, 22 Jul 2024 15:24:24 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH 1/8] hw/core: Make CPU topology enumeration
>  arch-agnostic
> 
> One little thing...
> 
> 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
> > a CPU_TOPO_LEVEL_DEFAULT to help future smp-cache object de-compatibilize
> > arch-specific cache topology settings.
> >
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> 
> [...]
> 
> > diff --git a/qapi/machine-common.json b/qapi/machine-common.json
> > index fa6bd71d1280..82413c668bdb 100644
> > --- a/qapi/machine-common.json
> > +++ b/qapi/machine-common.json
> > @@ -5,7 +5,7 @@
> >  # See the COPYING file in the top-level directory.
> >  
> >  ##
> > -# = Machines S390 data types
> > +# = Common machine types
> >  ##
> >  
> >  ##
> > @@ -19,3 +19,48 @@
> >  { 'enum': 'CpuS390Entitlement',
> >    'prefix': 'S390_CPU_ENTITLEMENT',
> >    'data': [ 'auto', 'low', 'medium', 'high' ] }
> > +
> > +##
> > +# @CpuTopologyLevel:
> > +#
> > +# 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
> > +#     SMPConfiguration is used to configure the topology of this
> > +#     level.
> > +#
> > +# @core: core level.  The @cores option in SMPConfiguration is used
> > +#     to configure the topology of this level.
> > +#
> > +# @module: module level.  The @modules option in SMPConfiguration is
> > +#     used to configure the topology of this level.
> > +#
> > +# @cluster: cluster level.  The @clusters option in SMPConfiguration
> > +#     is used to configure the topology of this level.
> > +#
> > +# @die: die level.  The @dies option in SMPConfiguration is used to
> > +#     configure the topology of this level.
> > +#
> > +# @socket: socket level, which would also be called package level.
> > +#     The @sockets option in SMPConfiguration is used to configure
> > +#     the topology of this level.
> > +#
> > +# @book: book level.  The @books option in SMPConfiguration is used
> > +#     to configure the topology of this level.
> > +#
> > +# @drawer: drawer level.  The @drawers option in SMPConfiguration is
> > +#     used to configure the topology of this level.
> > +#
> > +# @default: default level.  Some architectures will have default
> > +#     topology settings (e.g., cache topology), and this special
> > +#     level means following the architecture-specific settings.
> > +#
> > +# Since: 9.1
> > +##
> > +{ 'enum': 'CpuTopologyLevel',
> > +  'prefix': 'CPU_TOPO_LEVEL',
> 
> Why set a 'prefix'?
> 

Because my previous i386 commit 6ddeb0ec8c29 ("i386/cpu: Introduce bitmap
to cache available CPU topology levels") introduced the level
enumeration with such prefix. For naming consistency, and to shorten the
length of the name, I've used the same prefix here as well.

I've sensed that you don't like the TOPO abbreviation and I'll remove the
prefix :-).

Thanks,
Zhao

Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object

[Zhao Liu]

Hi Markus,

On Mon, Jul 22, 2024 at 03:33:13PM +0200, Markus Armbruster wrote:
> Date: Mon, 22 Jul 2024 15:33:13 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object
> 
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > Introduce smp-cache object so that user could define cache properties.
> >
> > In smp-cache object, define cache topology based on CPU topology level
> > with 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, enumerate the L1 D-cache, L1 I-cache, L2 cache and L3 cache
> > with smp-cache object to add the basic cache topology support.
> >
> > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> 
> [...]
> 
> > diff --git a/qapi/machine-common.json b/qapi/machine-common.json
> > index 82413c668bdb..8b8c0e9eeb86 100644
> > --- a/qapi/machine-common.json
> > +++ b/qapi/machine-common.json
> > @@ -64,3 +64,53 @@
> >    'prefix': 'CPU_TOPO_LEVEL',
> >    'data': [ 'invalid', 'thread', 'core', 'module', 'cluster',
> >              'die', 'socket', 'book', 'drawer', 'default' ] }
> > +
> > +##
> > +# @SMPCacheName:
> 
> Why the SMP in this name?  Because it's currently only used by SMP
> stuff?  Or is there another reason I'm missing?

Yes, I suppose it can only be used in SMP case.

Because Intel's heterogeneous CPUs have different topologies for cache,
for example, Alderlake's L2, for P core, L2 is per P-core, but for E
core, L2 is per module (4 E cores per module). Thus I would like to keep
the topology semantics of this object and -smp as consistent as possible.

Do you agree?

> The more idiomatic QAPI name would be SmpCacheName.  Likewise for the
> other type names below.

I hesitated here as well, but considering that SMPConfiguration is "SMP"
and not "Smp", it has that name. I'll change to SmpCacheName for strict
initial capitalization.

> > +#
> > +# An enumeration of cache for SMP systems.  The cache name here is
> > +# a combination of cache level and cache type.
> 
> The first sentence feels awkward.  Maybe
> 
>    # Caches an SMP system may have.
> 
> > +#
> > +# @l1d: L1 data cache.
> > +#
> > +# @l1i: L1 instruction cache.
> > +#
> > +# @l2: L2 (unified) cache.
> > +#
> > +# @l3: L3 (unified) cache
> > +#
> > +# Since: 9.1
> > +##
> 
> This assumes the L1 cache is split, and L2 and L3 are unified.
> 
> If we model a system with say a unified L1 cache, we'd simply extend
> this enum.  No real difference to extending it for additional levels.
> Correct?

Yes. For unified L1, we just need add a "l1" which is opposed to l1i/l1d.

> > +{ 'enum': 'SMPCacheName',
> > +  'prefix': 'SMP_CACHE',
> 
> Why not call it SmpCache, and ditch 'prefix'?

Because the SMPCache structure in smp_cache.h uses the similar name:

+#define TYPE_SMP_CACHE "smp-cache"
+OBJECT_DECLARE_SIMPLE_TYPE(SMPCache, SMP_CACHE)
+
+struct SMPCache {
+    Object parent_obj;
+
+    SMPCacheProperty props[SMP_CACHE__MAX];
+};

Naming is always difficult, so I would use Smpcache here if you feel that
SmpCache is sufficient to distinguish it from SMPCache, or I would also
rename the SMPCache structure to SMPCacheState in smp_cache.h.

Which way do you prefer?

> > +  'data': [ 'l1d', 'l1i', 'l2', 'l3' ] }
> 
> > +
> > +##
> > +# @SMPCacheProperty:
> 
> Sure we want to call this "property" (singular) and not "properties"?
> What if we add members to this type?
> 
> > +#
> > +# Cache information for SMP systems.
> > +#
> > +# @name: Cache name.
> > +#
> > +# @topo: Cache topology level.  It accepts the CPU topology
> > +#     enumeration as the parameter, i.e., CPUs in the same
> > +#     topology container share the same cache.
> > +#
> > +# Since: 9.1
> > +##
> > +{ 'struct': 'SMPCacheProperty',
> > +  'data': {
> > +  'name': 'SMPCacheName',
> > +  'topo': 'CpuTopologyLevel' } }
> 
> We tend to avoid abbreviations in the QAPI schema.  Please consider
> naming this 'topology'.

Sure!

> > +
> > +##
> > +# @SMPCacheProperties:
> > +#
> > +# List wrapper of SMPCacheProperty.
> > +#
> > +# @caches: the SMPCacheProperty list.
> > +#
> > +# Since 9.1
> > +##
> > +{ 'struct': 'SMPCacheProperties',
> > +  'data': { 'caches': ['SMPCacheProperty'] } }
> 
> Ah, now I see why you used the singular above!
> 
> However, this type holds the properties of call caches.  It is a list
> where each element holds the properties of a single cache.  Calling the
> former "cache property" and the latter "cache properties" is confusing.

Yes...

> SmpCachesProperties and SmpCacheProperties would put the singular
> vs. plural where it belongs.  Sounds a bit awkward to me, though.
> Naming is hard.

For SmpCachesProperties, it's easy to overlook the first "s".

> Other ideas, anybody?

Maybe SmpCacheOptions or SmpCachesPropertyWrapper?

> > diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> > index b1581988e4eb..25394f2cda50 100644
> > --- a/qapi/qapi-schema.json
> > +++ b/qapi/qapi-schema.json
> > @@ -64,11 +64,11 @@
> >  { 'include': 'compat.json' }
> >  { 'include': 'control.json' }
> >  { 'include': 'introspect.json' }
> > -{ 'include': 'qom.json' }
> > -{ 'include': 'qdev.json' }
> >  { 'include': 'machine-common.json' }
> >  { 'include': 'machine.json' }
> >  { 'include': 'machine-target.json' }
> > +{ 'include': 'qom.json' }
> > +{ 'include': 'qdev.json' }
> >  { 'include': 'replay.json' }
> >  { 'include': 'yank.json' }
> >  { 'include': 'misc.json' }
> 
> Worth explaining in the commit message, I think.

Because of the include relationship between the json files, I need to
change the order. I had a "crazy" proposal to clean up this:
https://lore.kernel.org/qemu-devel/20240517062748.782366-1-zhao1.liu@intel.com/

Thanks,
Zhao

Re: [PATCH 8/8] qemu-options: Add the description of smp-cache object

[Zhao Liu]

Hi Markus,

On Mon, Jul 22, 2024 at 03:37:43PM +0200, Markus Armbruster wrote:
> Date: Mon, 22 Jul 2024 15:37:43 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
>  object
> 
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> 
> This patch is just documentation.  The code got added in some previous
> patch.  Would it make sense to squash this patch into that previous
> patch?

OK, I'll merge them.

> > ---
> > Changes since RFC v2:
> >  * Rewrote the document of smp-cache object.
> >
> > 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 58 insertions(+)
> >
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 8ca7f34ef0c8..4b84f4508a6e 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -159,6 +159,15 @@ SRST
> >          ::
> >  
> >              -machine cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=128G,cxl-fmw.0.interleave-granularity=512
> > +
> > +    ``smp-cache='id'``
> > +        Allows to configure cache property (now only the cache topology level).
> > +
> > +        For example:
> > +        ::
> > +
> > +            -object '{"qom-type":"smp-cache","id":"cache","caches":[{"name":"l1d","topo":"core"},{"name":"l1i","topo":"core"},{"name":"l2","topo":"module"},{"name":"l3","topo":"die"}]}'
> > +            -machine smp-cache=cache
> >  ERST
> >  
> >  DEF("M", HAS_ARG, QEMU_OPTION_M,
> > @@ -5871,6 +5880,55 @@ SRST
> >          ::
> >  
> >              (qemu) qom-set /objects/iothread1 poll-max-ns 100000
> > +
> > +    ``-object '{"qom-type":"smp-cache","id":id,"caches":[{"name":cache_name,"topo":cache_topo}]}'``
> > +        Create an smp-cache object that configures machine's cache
> > +        property. Currently, cache property only include cache topology
> > +        level.
> > +
> > +        This option must be written in JSON format to support JSON list.
> 
> Why?

I'm not familiar with this, so I hope you could educate me if I'm wrong.

All I know so far is for -object that defining a list can only be done in
JSON format and not with a numeric index like a keyval based option, like:

-object smp-cache,id=cache0,caches.0.name=l1i,caches.0.topo=core: Parameter 'caches' is missing

the above doesn't work.

Is there any other way to specify a list in command line?

> > +
> > +        The ``caches`` parameter accepts a list of cache property in JSON
> > +        format.
> > +
> > +        A list element requires the cache name to be specified in the
> > +        ``name`` parameter (currently ``l1d``, ``l1i``, ``l2`` and ``l3``
> > +        are supported). ``topo`` parameter accepts CPU topology levels
> > +        including ``thread``, ``core``, ``module``, ``cluster``, ``die``,
> > +        ``socket``, ``book``, ``drawer`` and ``default``. The ``topo``
> > +        parameter indicates CPUs winthin the same CPU topology container
> > +        are sharing the same cache.
> > +
> > +        Some machines may have their own cache topology model, and this
> > +        object may override the machine-specific cache topology setting
> > +        by specifying smp-cache object in the -machine. When specifying
> > +        the cache topology level of ``default``, it will honor the default
> > +        machine-specific cache topology setting. For other topology levels,
> > +        they will override the default setting.
> > +
> > +        An example list of caches to configure the cache model (l1d cache
> > +        per core, l1i cache per core, l2 cache per module and l3 cache per
> > +        socket) supported by PC machine might look like:
> > +
> > +        ::
> > +
> > +              {
> > +                "caches": [
> > +                   { "name": "l1d", "topo": "core" },
> > +                   { "name": "l1i", "topo": "core" },
> > +                   { "name": "l2", "topo": "module" },
> > +                   { "name": "l3", "topo": "socket" },
> > +                ]
> > +              }
> > +
> > +        An example smp-cache object would look like:()
> > +
> > +        .. parsed-literal::
> > +
> > +             # |qemu_system| \\
> > +                 ... \\
> > +                 -object '{"qom-type":"smp-cache","id":id,"caches":[{"name":cache_name,"topo":cache_topo}]}' \\
> > +                 ...
> >  ERST
> 

Re: [PATCH 1/8] hw/core: Make CPU topology enumeration arch-agnostic

[Markus Armbruster]

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

> Hi Markus,
>
> On Mon, Jul 22, 2024 at 03:24:24PM +0200, Markus Armbruster wrote:
>> Date: Mon, 22 Jul 2024 15:24:24 +0200
>> From: Markus Armbruster <armbru@redhat.com>
>> Subject: Re: [PATCH 1/8] hw/core: Make CPU topology enumeration
>>  arch-agnostic
>> 
>> One little thing...
>> 
>> 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
>> > a CPU_TOPO_LEVEL_DEFAULT to help future smp-cache object de-compatibilize
>> > arch-specific cache topology settings.
>> >
>> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
>> 
>> [...]
>> 
>> > diff --git a/qapi/machine-common.json b/qapi/machine-common.json
>> > index fa6bd71d1280..82413c668bdb 100644
>> > --- a/qapi/machine-common.json
>> > +++ b/qapi/machine-common.json
>> > @@ -5,7 +5,7 @@
>> >  # See the COPYING file in the top-level directory.
>> >  
>> >  ##
>> > -# = Machines S390 data types
>> > +# = Common machine types
>> >  ##
>> >  
>> >  ##
>> > @@ -19,3 +19,48 @@
>> >  { 'enum': 'CpuS390Entitlement',
>> >    'prefix': 'S390_CPU_ENTITLEMENT',
>> >    'data': [ 'auto', 'low', 'medium', 'high' ] }
>> > +
>> > +##
>> > +# @CpuTopologyLevel:
>> > +#
>> > +# 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
>> > +#     SMPConfiguration is used to configure the topology of this
>> > +#     level.
>> > +#
>> > +# @core: core level.  The @cores option in SMPConfiguration is used
>> > +#     to configure the topology of this level.
>> > +#
>> > +# @module: module level.  The @modules option in SMPConfiguration is
>> > +#     used to configure the topology of this level.
>> > +#
>> > +# @cluster: cluster level.  The @clusters option in SMPConfiguration
>> > +#     is used to configure the topology of this level.
>> > +#
>> > +# @die: die level.  The @dies option in SMPConfiguration is used to
>> > +#     configure the topology of this level.
>> > +#
>> > +# @socket: socket level, which would also be called package level.
>> > +#     The @sockets option in SMPConfiguration is used to configure
>> > +#     the topology of this level.
>> > +#
>> > +# @book: book level.  The @books option in SMPConfiguration is used
>> > +#     to configure the topology of this level.
>> > +#
>> > +# @drawer: drawer level.  The @drawers option in SMPConfiguration is
>> > +#     used to configure the topology of this level.
>> > +#
>> > +# @default: default level.  Some architectures will have default
>> > +#     topology settings (e.g., cache topology), and this special
>> > +#     level means following the architecture-specific settings.
>> > +#
>> > +# Since: 9.1
>> > +##
>> > +{ 'enum': 'CpuTopologyLevel',
>> > +  'prefix': 'CPU_TOPO_LEVEL',
>> 
>> Why set a 'prefix'?
>> 
>
> Because my previous i386 commit 6ddeb0ec8c29 ("i386/cpu: Introduce bitmap
> to cache available CPU topology levels") introduced the level
> enumeration with such prefix. For naming consistency, and to shorten the
> length of the name, I've used the same prefix here as well.
>
> I've sensed that you don't like the TOPO abbreviation and I'll remove the
> prefix :-).

Consistency is good, but I'd rather achieve it by consistently using
"topology".

I never liked the 'prefix' feature much.  We have it because the mapping
from camel case to upper case with underscores is heuristical, and can
result in something undesirable.  See commit 351d36e454c (qapi: allow
override of default enum prefix naming).  Using it just to shorten
generated identifiers is a bad idea.

Re: [PATCH 1/8] hw/core: Make CPU topology enumeration arch-agnostic

[Zhao Liu]

On Tue, Jul 23, 2024 at 12:14:30PM +0200, Markus Armbruster wrote:
> Date: Tue, 23 Jul 2024 12:14:30 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH 1/8] hw/core: Make CPU topology enumeration
>  arch-agnostic
> 
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > Hi Markus,
> >
> > On Mon, Jul 22, 2024 at 03:24:24PM +0200, Markus Armbruster wrote:
> >> Date: Mon, 22 Jul 2024 15:24:24 +0200
> >> From: Markus Armbruster <armbru@redhat.com>
> >> Subject: Re: [PATCH 1/8] hw/core: Make CPU topology enumeration
> >>  arch-agnostic
> >> 
> >> One little thing...
> >> 
> >> 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
> >> > a CPU_TOPO_LEVEL_DEFAULT to help future smp-cache object de-compatibilize
> >> > arch-specific cache topology settings.
> >> >
> >> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> >> 
> >> [...]
> >> 
> >> > diff --git a/qapi/machine-common.json b/qapi/machine-common.json
> >> > index fa6bd71d1280..82413c668bdb 100644
> >> > --- a/qapi/machine-common.json
> >> > +++ b/qapi/machine-common.json
> >> > @@ -5,7 +5,7 @@
> >> >  # See the COPYING file in the top-level directory.
> >> >  
> >> >  ##
> >> > -# = Machines S390 data types
> >> > +# = Common machine types
> >> >  ##
> >> >  
> >> >  ##
> >> > @@ -19,3 +19,48 @@
> >> >  { 'enum': 'CpuS390Entitlement',
> >> >    'prefix': 'S390_CPU_ENTITLEMENT',
> >> >    'data': [ 'auto', 'low', 'medium', 'high' ] }
> >> > +
> >> > +##
> >> > +# @CpuTopologyLevel:
> >> > +#
> >> > +# 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
> >> > +#     SMPConfiguration is used to configure the topology of this
> >> > +#     level.
> >> > +#
> >> > +# @core: core level.  The @cores option in SMPConfiguration is used
> >> > +#     to configure the topology of this level.
> >> > +#
> >> > +# @module: module level.  The @modules option in SMPConfiguration is
> >> > +#     used to configure the topology of this level.
> >> > +#
> >> > +# @cluster: cluster level.  The @clusters option in SMPConfiguration
> >> > +#     is used to configure the topology of this level.
> >> > +#
> >> > +# @die: die level.  The @dies option in SMPConfiguration is used to
> >> > +#     configure the topology of this level.
> >> > +#
> >> > +# @socket: socket level, which would also be called package level.
> >> > +#     The @sockets option in SMPConfiguration is used to configure
> >> > +#     the topology of this level.
> >> > +#
> >> > +# @book: book level.  The @books option in SMPConfiguration is used
> >> > +#     to configure the topology of this level.
> >> > +#
> >> > +# @drawer: drawer level.  The @drawers option in SMPConfiguration is
> >> > +#     used to configure the topology of this level.
> >> > +#
> >> > +# @default: default level.  Some architectures will have default
> >> > +#     topology settings (e.g., cache topology), and this special
> >> > +#     level means following the architecture-specific settings.
> >> > +#
> >> > +# Since: 9.1
> >> > +##
> >> > +{ 'enum': 'CpuTopologyLevel',
> >> > +  'prefix': 'CPU_TOPO_LEVEL',
> >> 
> >> Why set a 'prefix'?
> >> 
> >
> > Because my previous i386 commit 6ddeb0ec8c29 ("i386/cpu: Introduce bitmap
> > to cache available CPU topology levels") introduced the level
> > enumeration with such prefix. For naming consistency, and to shorten the
> > length of the name, I've used the same prefix here as well.
> >
> > I've sensed that you don't like the TOPO abbreviation and I'll remove the
> > prefix :-).
> 
> Consistency is good, but I'd rather achieve it by consistently using
> "topology".
> 
> I never liked the 'prefix' feature much.  We have it because the mapping
> from camel case to upper case with underscores is heuristical, and can
> result in something undesirable.  See commit 351d36e454c (qapi: allow
> override of default enum prefix naming).  Using it just to shorten
> generated identifiers is a bad idea.

Thanks for your clarification! I see, I will drop the prefix.

Regards,
Zhao

Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object

[Markus Armbruster]

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

> Hi Markus,
>
> On Mon, Jul 22, 2024 at 03:33:13PM +0200, Markus Armbruster wrote:
>> Date: Mon, 22 Jul 2024 15:33:13 +0200
>> From: Markus Armbruster <armbru@redhat.com>
>> Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object
>> 
>> Zhao Liu <zhao1.liu@intel.com> writes:
>> 
>> > Introduce smp-cache object so that user could define cache properties.
>> >
>> > In smp-cache object, define cache topology based on CPU topology level
>> > with 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, enumerate the L1 D-cache, L1 I-cache, L2 cache and L3 cache
>> > with smp-cache object to add the basic cache topology support.
>> >
>> > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
>> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
>> 
>> [...]
>> 
>> > diff --git a/qapi/machine-common.json b/qapi/machine-common.json
>> > index 82413c668bdb..8b8c0e9eeb86 100644
>> > --- a/qapi/machine-common.json
>> > +++ b/qapi/machine-common.json
>> > @@ -64,3 +64,53 @@
>> >    'prefix': 'CPU_TOPO_LEVEL',
>> >    'data': [ 'invalid', 'thread', 'core', 'module', 'cluster',
>> >              'die', 'socket', 'book', 'drawer', 'default' ] }
>> > +
>> > +##
>> > +# @SMPCacheName:
>> 
>> Why the SMP in this name?  Because it's currently only used by SMP
>> stuff?  Or is there another reason I'm missing?
>
> Yes, I suppose it can only be used in SMP case.
>
> Because Intel's heterogeneous CPUs have different topologies for cache,
> for example, Alderlake's L2, for P core, L2 is per P-core, but for E
> core, L2 is per module (4 E cores per module). Thus I would like to keep
> the topology semantics of this object and -smp as consistent as possible.
>
> Do you agree?

I don't know enough to meaningfully agree or disagree.  I know just
enough to annoy you with questions :)

This series adds a way to configure caches.

Structure of the configuration data: a list

    [{"name": N, "topo": T}, ...]

where N can be "l1d", "l1i", "l2", or "l3",
  and T can be "invalid", "thread", "core", "module", "cluster",
               "die", "socket", "book", "drawer", or "default".

What's the use case?  The commit messages don't tell.

Why does that use case make no sense without SMP?

Can the same @name occur multiple times?  Documentation doesn't tell.
If yes, what does that mean?

Say we later add value "l1" for unified level 1 cache.  Would "l1" then
conflict with "l1d" and "l1u"?

May @topo be "invalid"?  Documentation doesn't tell.  If yes, what does
that mean?

>> The more idiomatic QAPI name would be SmpCacheName.  Likewise for the
>> other type names below.
>
> I hesitated here as well, but considering that SMPConfiguration is "SMP"
> and not "Smp", it has that name. I'll change to SmpCacheName for strict
> initial capitalization.
>
>> > +#
>> > +# An enumeration of cache for SMP systems.  The cache name here is
>> > +# a combination of cache level and cache type.
>> 
>> The first sentence feels awkward.  Maybe
>> 
>>    # Caches an SMP system may have.
>> 
>> > +#
>> > +# @l1d: L1 data cache.
>> > +#
>> > +# @l1i: L1 instruction cache.
>> > +#
>> > +# @l2: L2 (unified) cache.
>> > +#
>> > +# @l3: L3 (unified) cache
>> > +#
>> > +# Since: 9.1
>> > +##
>> 
>> This assumes the L1 cache is split, and L2 and L3 are unified.
>> 
>> If we model a system with say a unified L1 cache, we'd simply extend
>> this enum.  No real difference to extending it for additional levels.
>> Correct?
>
> Yes. For unified L1, we just need add a "l1" which is opposed to l1i/l1d.
>
>> > +{ 'enum': 'SMPCacheName',
>> > +  'prefix': 'SMP_CACHE',
>> 
>> Why not call it SmpCache, and ditch 'prefix'?
>
> Because the SMPCache structure in smp_cache.h uses the similar name:
>
> +#define TYPE_SMP_CACHE "smp-cache"
> +OBJECT_DECLARE_SIMPLE_TYPE(SMPCache, SMP_CACHE)
> +
> +struct SMPCache {
> +    Object parent_obj;
> +
> +    SMPCacheProperty props[SMP_CACHE__MAX];
> +};
>
> Naming is always difficult,

Oh yes.

>                             so I would use Smpcache here if you feel that
> SmpCache is sufficient to distinguish it from SMPCache, or I would also
> rename the SMPCache structure to SMPCacheState in smp_cache.h.
>
> Which way do you prefer?

Having both QAPI enum SmpCache and handwritten type SMPCache is clearly
undesirable.

I retract my suggestion to name the enum SmpCache.  The thing clearly is
not a cache.  SmpCacheName is better.

If you drop 'prefix', the generated C enum values look like
SMP_CACHE_NAME_FOO.  Would that work for you?

The "name" part bothers me a bit.  A name doesn't define what something
is.  A type does.  SmpCacheType?

>> > +  'data': [ 'l1d', 'l1i', 'l2', 'l3' ] }
>> 
>> > +
>> > +##
>> > +# @SMPCacheProperty:
>> 
>> Sure we want to call this "property" (singular) and not "properties"?
>> What if we add members to this type?
>> 
>> > +#
>> > +# Cache information for SMP systems.
>> > +#
>> > +# @name: Cache name.
>> > +#
>> > +# @topo: Cache topology level.  It accepts the CPU topology
>> > +#     enumeration as the parameter, i.e., CPUs in the same
>> > +#     topology container share the same cache.
>> > +#
>> > +# Since: 9.1
>> > +##
>> > +{ 'struct': 'SMPCacheProperty',
>> > +  'data': {
>> > +  'name': 'SMPCacheName',
>> > +  'topo': 'CpuTopologyLevel' } }
>> 
>> We tend to avoid abbreviations in the QAPI schema.  Please consider
>> naming this 'topology'.
>
> Sure!
>
>> > +
>> > +##
>> > +# @SMPCacheProperties:
>> > +#
>> > +# List wrapper of SMPCacheProperty.
>> > +#
>> > +# @caches: the SMPCacheProperty list.
>> > +#
>> > +# Since 9.1
>> > +##
>> > +{ 'struct': 'SMPCacheProperties',
>> > +  'data': { 'caches': ['SMPCacheProperty'] } }
>> 
>> Ah, now I see why you used the singular above!
>> 
>> However, this type holds the properties of call caches.  It is a list

"of all caches" (can't type).

>> where each element holds the properties of a single cache.  Calling the
>> former "cache property" and the latter "cache properties" is confusing.
>
> Yes...
>
>> SmpCachesProperties and SmpCacheProperties would put the singular
>> vs. plural where it belongs.  Sounds a bit awkward to me, though.
>> Naming is hard.
>
> For SmpCachesProperties, it's easy to overlook the first "s".
>
>> Other ideas, anybody?
>
> Maybe SmpCacheOptions or SmpCachesPropertyWrapper?

I wonder why we have a single QOM object to configure all caches, and
not one QOM object per cache.

>> > diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
>> > index b1581988e4eb..25394f2cda50 100644
>> > --- a/qapi/qapi-schema.json
>> > +++ b/qapi/qapi-schema.json
>> > @@ -64,11 +64,11 @@
>> >  { 'include': 'compat.json' }
>> >  { 'include': 'control.json' }
>> >  { 'include': 'introspect.json' }
>> > -{ 'include': 'qom.json' }
>> > -{ 'include': 'qdev.json' }
>> >  { 'include': 'machine-common.json' }
>> >  { 'include': 'machine.json' }
>> >  { 'include': 'machine-target.json' }
>> > +{ 'include': 'qom.json' }
>> > +{ 'include': 'qdev.json' }
>> >  { 'include': 'replay.json' }
>> >  { 'include': 'yank.json' }
>> >  { 'include': 'misc.json' }
>> 
>> Worth explaining in the commit message, I think.
>
> Because of the include relationship between the json files, I need to
> change the order. I had a "crazy" proposal to clean up this:
> https://lore.kernel.org/qemu-devel/20240517062748.782366-1-zhao1.liu@intel.com/
>
> Thanks,
> Zhao

Re: [PATCH 8/8] qemu-options: Add the description of smp-cache object

[Markus Armbruster]

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

> Hi Markus,
>
> On Mon, Jul 22, 2024 at 03:37:43PM +0200, Markus Armbruster wrote:
>> Date: Mon, 22 Jul 2024 15:37:43 +0200
>> From: Markus Armbruster <armbru@redhat.com>
>> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
>>  object
>> 
>> Zhao Liu <zhao1.liu@intel.com> writes:
>> 
>> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
>> 
>> This patch is just documentation.  The code got added in some previous
>> patch.  Would it make sense to squash this patch into that previous
>> patch?
>
> OK, I'll merge them.
>
>> > ---
>> > Changes since RFC v2:
>> >  * Rewrote the document of smp-cache object.
>> >
>> > 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 58 insertions(+)
>> >
>> > diff --git a/qemu-options.hx b/qemu-options.hx
>> > index 8ca7f34ef0c8..4b84f4508a6e 100644
>> > --- a/qemu-options.hx
>> > +++ b/qemu-options.hx
>> > @@ -159,6 +159,15 @@ SRST
>> >          ::
>> >  
>> >              -machine cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=128G,cxl-fmw.0.interleave-granularity=512
>> > +
>> > +    ``smp-cache='id'``
>> > +        Allows to configure cache property (now only the cache topology level).
>> > +
>> > +        For example:
>> > +        ::
>> > +
>> > +            -object '{"qom-type":"smp-cache","id":"cache","caches":[{"name":"l1d","topo":"core"},{"name":"l1i","topo":"core"},{"name":"l2","topo":"module"},{"name":"l3","topo":"die"}]}'
>> > +            -machine smp-cache=cache
>> >  ERST
>> >  
>> >  DEF("M", HAS_ARG, QEMU_OPTION_M,
>> > @@ -5871,6 +5880,55 @@ SRST
>> >          ::
>> >  
>> >              (qemu) qom-set /objects/iothread1 poll-max-ns 100000
>> > +
>> > +    ``-object '{"qom-type":"smp-cache","id":id,"caches":[{"name":cache_name,"topo":cache_topo}]}'``
>> > +        Create an smp-cache object that configures machine's cache
>> > +        property. Currently, cache property only include cache topology
>> > +        level.
>> > +
>> > +        This option must be written in JSON format to support JSON list.
>> 
>> Why?
>
> I'm not familiar with this, so I hope you could educate me if I'm wrong.
>
> All I know so far is for -object that defining a list can only be done in
> JSON format and not with a numeric index like a keyval based option, like:
>
> -object smp-cache,id=cache0,caches.0.name=l1i,caches.0.topo=core: Parameter 'caches' is missing
>
> the above doesn't work.
>
> Is there any other way to specify a list in command line?

The command line is a big, sprawling mess :)

-object supports either a JSON or a QemuOpts argument.  *Not* keyval!

Both QemuOpts and keyval parse something like KEY=VALUE,...  Keyval
supports arrays and objects via dotted keys.  QemuOpts doesn't natively
support arrays and objects, but its users can hack around that
limitation in various ways.  -object doesn't.  So you're right, it's
JSON or bust here.

However, if we used one object per cache instead, we could get something
like

    -object smp-cache,name=l1d,...
    -object smp-cache,name=l1u,...
    -object smp-cache,name=l2,...
    ...

>> > +
>> > +        The ``caches`` parameter accepts a list of cache property in JSON
>> > +        format.
>> > +
>> > +        A list element requires the cache name to be specified in the
>> > +        ``name`` parameter (currently ``l1d``, ``l1i``, ``l2`` and ``l3``
>> > +        are supported). ``topo`` parameter accepts CPU topology levels
>> > +        including ``thread``, ``core``, ``module``, ``cluster``, ``die``,
>> > +        ``socket``, ``book``, ``drawer`` and ``default``. The ``topo``
>> > +        parameter indicates CPUs winthin the same CPU topology container
>> > +        are sharing the same cache.
>> > +
>> > +        Some machines may have their own cache topology model, and this
>> > +        object may override the machine-specific cache topology setting
>> > +        by specifying smp-cache object in the -machine. When specifying
>> > +        the cache topology level of ``default``, it will honor the default
>> > +        machine-specific cache topology setting. For other topology levels,
>> > +        they will override the default setting.
>> > +
>> > +        An example list of caches to configure the cache model (l1d cache
>> > +        per core, l1i cache per core, l2 cache per module and l3 cache per
>> > +        socket) supported by PC machine might look like:
>> > +
>> > +        ::
>> > +
>> > +              {
>> > +                "caches": [
>> > +                   { "name": "l1d", "topo": "core" },
>> > +                   { "name": "l1i", "topo": "core" },
>> > +                   { "name": "l2", "topo": "module" },
>> > +                   { "name": "l3", "topo": "socket" },
>> > +                ]
>> > +              }
>> > +
>> > +        An example smp-cache object would look like:()
>> > +
>> > +        .. parsed-literal::
>> > +
>> > +             # |qemu_system| \\
>> > +                 ... \\
>> > +                 -object '{"qom-type":"smp-cache","id":id,"caches":[{"name":cache_name,"topo":cache_topo}]}' \\
>> > +                 ...
>> >  ERST
>> 

Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object

[Daniel P. Berrangé]

On Wed, Jul 24, 2024 at 01:35:17PM +0200, Markus Armbruster wrote:
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > Hi Markus,
> >> SmpCachesProperties and SmpCacheProperties would put the singular
> >> vs. plural where it belongs.  Sounds a bit awkward to me, though.
> >> Naming is hard.
> >
> > For SmpCachesProperties, it's easy to overlook the first "s".
> >
> >> Other ideas, anybody?
> >
> > Maybe SmpCacheOptions or SmpCachesPropertyWrapper?
> 
> I wonder why we have a single QOM object to configure all caches, and
> not one QOM object per cache.

Previous versions of this series were augmenting the existing
-smp command line. Now the design has switched to use -object,
I agree that it'd be simplest to just have one -object flag
added per cache level we want to defnie.


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: [PATCH 2/8] qapi/qom: Introduce smp-cache object

[Zhao Liu]

On Wed, Jul 24, 2024 at 01:47:16PM +0100, Daniel P. Berrangé wrote:
> Date: Wed, 24 Jul 2024 13:47:16 +0100
> From: "Daniel P. Berrangé" <berrange@redhat.com>
> Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object
> 
> On Wed, Jul 24, 2024 at 01:35:17PM +0200, Markus Armbruster wrote:
> > Zhao Liu <zhao1.liu@intel.com> writes:
> > 
> > > Hi Markus,
> > >> SmpCachesProperties and SmpCacheProperties would put the singular
> > >> vs. plural where it belongs.  Sounds a bit awkward to me, though.
> > >> Naming is hard.
> > >
> > > For SmpCachesProperties, it's easy to overlook the first "s".
> > >
> > >> Other ideas, anybody?
> > >
> > > Maybe SmpCacheOptions or SmpCachesPropertyWrapper?
> > 
> > I wonder why we have a single QOM object to configure all caches, and
> > not one QOM object per cache.
> 
> Previous versions of this series were augmenting the existing
> -smp command line.

Ah, yes, since -smp, as a sugar option of -machine, doesn't support
JSON. In -smp, we need to use keyval's style to configure as:

-smp caches.0.name=l1i,caches.0.topo=core

I think JSON is the more elegant way to go, so I chose -object.

> Now the design has switched to use -object,
> I agree that it'd be simplest to just have one -object flag
> added per cache level we want to defnie.
> 

OK.

Thanks,
Zhao

Re: [PATCH 8/8] qemu-options: Add the description of smp-cache object

[Zhao Liu]

Hi Markus and Daniel,

I have the questions about the -object per cache implementation:

On Wed, Jul 24, 2024 at 02:39:29PM +0200, Markus Armbruster wrote:
> Date: Wed, 24 Jul 2024 14:39:29 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
>  object
> 
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > Hi Markus,
> >
> > On Mon, Jul 22, 2024 at 03:37:43PM +0200, Markus Armbruster wrote:
> >> Date: Mon, 22 Jul 2024 15:37:43 +0200
> >> From: Markus Armbruster <armbru@redhat.com>
> >> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
> >>  object
> >> 
> >> Zhao Liu <zhao1.liu@intel.com> writes:
> >> 
> >> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> >> 
> >> This patch is just documentation.  The code got added in some previous
> >> patch.  Would it make sense to squash this patch into that previous
> >> patch?
> >
> > OK, I'll merge them.
> >
> >> > ---
> >> > Changes since RFC v2:
> >> >  * Rewrote the document of smp-cache object.
> >> >
> >> > 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
> >> >  1 file changed, 58 insertions(+)
> >> >
> >> > diff --git a/qemu-options.hx b/qemu-options.hx
> >> > index 8ca7f34ef0c8..4b84f4508a6e 100644
> >> > --- a/qemu-options.hx
> >> > +++ b/qemu-options.hx
> >> > @@ -159,6 +159,15 @@ SRST
> >> >          ::
> >> >  
> >> >              -machine cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=128G,cxl-fmw.0.interleave-granularity=512
> >> > +
> >> > +    ``smp-cache='id'``
> >> > +        Allows to configure cache property (now only the cache topology level).
> >> > +
> >> > +        For example:
> >> > +        ::
> >> > +
> >> > +            -object '{"qom-type":"smp-cache","id":"cache","caches":[{"name":"l1d","topo":"core"},{"name":"l1i","topo":"core"},{"name":"l2","topo":"module"},{"name":"l3","topo":"die"}]}'
> >> > +            -machine smp-cache=cache
> >> >  ERST
> >> >  
> >> >  DEF("M", HAS_ARG, QEMU_OPTION_M,
> >> > @@ -5871,6 +5880,55 @@ SRST
> >> >          ::
> >> >  
> >> >              (qemu) qom-set /objects/iothread1 poll-max-ns 100000
> >> > +
> >> > +    ``-object '{"qom-type":"smp-cache","id":id,"caches":[{"name":cache_name,"topo":cache_topo}]}'``
> >> > +        Create an smp-cache object that configures machine's cache
> >> > +        property. Currently, cache property only include cache topology
> >> > +        level.
> >> > +
> >> > +        This option must be written in JSON format to support JSON list.
> >> 
> >> Why?
> >
> > I'm not familiar with this, so I hope you could educate me if I'm wrong.
> >
> > All I know so far is for -object that defining a list can only be done in
> > JSON format and not with a numeric index like a keyval based option, like:
> >
> > -object smp-cache,id=cache0,caches.0.name=l1i,caches.0.topo=core: Parameter 'caches' is missing
> >
> > the above doesn't work.
> >
> > Is there any other way to specify a list in command line?
> 
> The command line is a big, sprawling mess :)
> 
> -object supports either a JSON or a QemuOpts argument.  *Not* keyval!
> 
> Both QemuOpts and keyval parse something like KEY=VALUE,...  Keyval
> supports arrays and objects via dotted keys.  QemuOpts doesn't natively
> support arrays and objects, but its users can hack around that
> limitation in various ways.  -object doesn't.  So you're right, it's
> JSON or bust here.
> 
> However, if we used one object per cache instead, we could get something
> like
> 
>     -object smp-cache,name=l1d,...
>     -object smp-cache,name=l1u,...
>     -object smp-cache,name=l2,...
>     ...

Current, I use -object to create a smp_cache object, and link it to
MachineState by -machine,smp-cache=obj_id.

Then for the objects per cache, how could I link them to machine?

Is it possible that I create something static in smp_cache.c and expose
all the cache information to machine through some interface?

Additionally, I would like to consider for the long term heterogeneous
cache, as asked before in [1], does the object per cache conflict with
the cache device I'm considering? Considering cache device is further
because I want to create CPU/cache topology via -device and build a
topology tree.

[1]: https://lore.kernel.org/qemu-devel/Zl88DYwLE3ScDF5F@intel.com/

I think this is becoming a nightmare I can't get around. Naming is
difficult, and sorting out interface design I think is also a difficult
task.

If you feel that there is indeed a conflict, then I'm also willing
to fall back to -smp again and do it based on keyval's list, as originally
suggested by Daniel. Sorry for the repetition on thoughts/design, I hope
that discussion with you I can make sense of the current and subsequent
paths without getting out of hand!

Best Regards,
Zhao

Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object

[Zhao Liu]

Hi Markus,

I realized I should reply this mail first...

On Wed, Jul 24, 2024 at 01:35:17PM +0200, Markus Armbruster wrote:
> Date: Wed, 24 Jul 2024 13:35:17 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object
> 
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > Hi Markus,
> >
> > On Mon, Jul 22, 2024 at 03:33:13PM +0200, Markus Armbruster wrote:
> >> Date: Mon, 22 Jul 2024 15:33:13 +0200
> >> From: Markus Armbruster <armbru@redhat.com>
> >> Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object
> >> 
> >> Zhao Liu <zhao1.liu@intel.com> writes:
> >> 
> >> > Introduce smp-cache object so that user could define cache properties.
> >> >
> >> > In smp-cache object, define cache topology based on CPU topology level
> >> > with 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, enumerate the L1 D-cache, L1 I-cache, L2 cache and L3 cache
> >> > with smp-cache object to add the basic cache topology support.
> >> >
> >> > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> >> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> >> 
> >> [...]
> >> 
> >> > diff --git a/qapi/machine-common.json b/qapi/machine-common.json
> >> > index 82413c668bdb..8b8c0e9eeb86 100644
> >> > --- a/qapi/machine-common.json
> >> > +++ b/qapi/machine-common.json
> >> > @@ -64,3 +64,53 @@
> >> >    'prefix': 'CPU_TOPO_LEVEL',
> >> >    'data': [ 'invalid', 'thread', 'core', 'module', 'cluster',
> >> >              'die', 'socket', 'book', 'drawer', 'default' ] }
> >> > +
> >> > +##
> >> > +# @SMPCacheName:
> >> 
> >> Why the SMP in this name?  Because it's currently only used by SMP
> >> stuff?  Or is there another reason I'm missing?
> >
> > Yes, I suppose it can only be used in SMP case.
> >
> > Because Intel's heterogeneous CPUs have different topologies for cache,
> > for example, Alderlake's L2, for P core, L2 is per P-core, but for E
> > core, L2 is per module (4 E cores per module). Thus I would like to keep
> > the topology semantics of this object and -smp as consistent as possible.
> >
> > Do you agree?
> 
> I don't know enough to meaningfully agree or disagree.  I know just
> enough to annoy you with questions :)

Welcome and no problem!

> This series adds a way to configure caches.
> 
> Structure of the configuration data: a list
> 
>     [{"name": N, "topo": T}, ...]
> 
> where N can be "l1d", "l1i", "l2", or "l3",
>   and T can be "invalid", "thread", "core", "module", "cluster",
>                "die", "socket", "book", "drawer", or "default".
> 
> What's the use case?  The commit messages don't tell.

i386 has the default cache topology model: l1 per core/l2 per core/l3
per die.

Cache topology affects scheduler performance, e.g., kernel's cluster
scheduling.

Of course I can hardcode some cache topology model in the specific cpu
model that corresponds to the actual hardware, but for -cpu host/max,
the default i386 cache topology model has no flexibility, and the
host-cpu-cache option doesn't have enough fine-grained control over the
cache topology.

So I want to provide a way to allow user create more fleasible cache
topology. Just like cpu topology.

> Why does that use case make no sense without SMP?

As the example I mentioned, for Intel hyrbid architecture, P cores has
l2 per core and E cores has l2 per module. Then either setting the l2
topology level as core nor module, can emulate the real case.

Even considering the more extreme case of Intel 14th MTL CPU, where
some E cores have L3 and some don't even have L3. As well as the last
time you and Daniel mentioned that in the future we could consider
covering more cache properties such as cache size. But the l3 size can
be different in the same system, like AMD's x3D technology. So
generally configuring properties for @name in a list can't take into
account the differences of heterogeneous caches with the same @name.

Hope my poor english explains the problem well. :-)

> Can the same @name occur multiple times?  Documentation doesn't tell.
> If yes, what does that mean?

Yes, this means the later one will override the previous one with the same
name.

> Say we later add value "l1" for unified level 1 cache.  Would "l1" then
> conflict with "l1d" and "l1u"?

Yes, we should check in smp/machine code and ban l1 and l1i/l1d at the
same time. This check I suppose is easy to add.

> May @topo be "invalid"?  Documentation doesn't tell.  If yes, what does
> that mean?

Yes, just follow the intel's spec, invalid means the current topology
information is invalid, which is used to encode x86 CPUIDs. So when I
move this level to qapi, I just keeped this. Otherwise, I need to
re-implement the i386 specific invalid level.

> >> The more idiomatic QAPI name would be SmpCacheName.  Likewise for the
> >> other type names below.
> >
> > I hesitated here as well, but considering that SMPConfiguration is "SMP"
> > and not "Smp", it has that name. I'll change to SmpCacheName for strict
> > initial capitalization.
> >
> >> > +#
> >> > +# An enumeration of cache for SMP systems.  The cache name here is
> >> > +# a combination of cache level and cache type.
> >> 
> >> The first sentence feels awkward.  Maybe
> >> 
> >>    # Caches an SMP system may have.
> >> 
> >> > +#
> >> > +# @l1d: L1 data cache.
> >> > +#
> >> > +# @l1i: L1 instruction cache.
> >> > +#
> >> > +# @l2: L2 (unified) cache.
> >> > +#
> >> > +# @l3: L3 (unified) cache
> >> > +#
> >> > +# Since: 9.1
> >> > +##
> >> 
> >> This assumes the L1 cache is split, and L2 and L3 are unified.
> >> 
> >> If we model a system with say a unified L1 cache, we'd simply extend
> >> this enum.  No real difference to extending it for additional levels.
> >> Correct?
> >
> > Yes. For unified L1, we just need add a "l1" which is opposed to l1i/l1d.
> >
> >> > +{ 'enum': 'SMPCacheName',
> >> > +  'prefix': 'SMP_CACHE',
> >> 
> >> Why not call it SmpCache, and ditch 'prefix'?
> >
> > Because the SMPCache structure in smp_cache.h uses the similar name:
> >
> > +#define TYPE_SMP_CACHE "smp-cache"
> > +OBJECT_DECLARE_SIMPLE_TYPE(SMPCache, SMP_CACHE)
> > +
> > +struct SMPCache {
> > +    Object parent_obj;
> > +
> > +    SMPCacheProperty props[SMP_CACHE__MAX];
> > +};
> >
> > Naming is always difficult,
> 
> Oh yes.
> 
> >                             so I would use Smpcache here if you feel that
> > SmpCache is sufficient to distinguish it from SMPCache, or I would also
> > rename the SMPCache structure to SMPCacheState in smp_cache.h.
> >
> > Which way do you prefer?
> 
> Having both QAPI enum SmpCache and handwritten type SMPCache is clearly
> undesirable.
> 
> I retract my suggestion to name the enum SmpCache.  The thing clearly is
> not a cache.  SmpCacheName is better.
> 
> If you drop 'prefix', the generated C enum values look like
> SMP_CACHE_NAME_FOO.  Would that work for you?

I think the SmpCacheName is ok, since there's no other better names.

> The "name" part bothers me a bit.  A name doesn't define what something
> is.  A type does.  SmpCacheType?

Ah, I also considerred this. I didn't use "type" because people usually
uses cache type to indicate INSTRUCTION/DATA/UNIFIED and cache level to
indicate LEVEL 1/LEVEL 2/LEVEL 3. The enumeration here is a combination of
type+level. So I think it's better to avoid the type term.

> >> > +  'data': [ 'l1d', 'l1i', 'l2', 'l3' ] }
> >> 
> >> > +
> >> > +##
> >> > +# @SMPCacheProperty:
> >> 
> >> Sure we want to call this "property" (singular) and not "properties"?
> >> What if we add members to this type?
> >> 
> >> > +#
> >> > +# Cache information for SMP systems.
> >> > +#
> >> > +# @name: Cache name.
> >> > +#
> >> > +# @topo: Cache topology level.  It accepts the CPU topology
> >> > +#     enumeration as the parameter, i.e., CPUs in the same
> >> > +#     topology container share the same cache.
> >> > +#
> >> > +# Since: 9.1
> >> > +##
> >> > +{ 'struct': 'SMPCacheProperty',
> >> > +  'data': {
> >> > +  'name': 'SMPCacheName',
> >> > +  'topo': 'CpuTopologyLevel' } }
> >> 
> >> We tend to avoid abbreviations in the QAPI schema.  Please consider
> >> naming this 'topology'.
> >
> > Sure!
> >
> >> > +
> >> > +##
> >> > +# @SMPCacheProperties:
> >> > +#
> >> > +# List wrapper of SMPCacheProperty.
> >> > +#
> >> > +# @caches: the SMPCacheProperty list.
> >> > +#
> >> > +# Since 9.1
> >> > +##
> >> > +{ 'struct': 'SMPCacheProperties',
> >> > +  'data': { 'caches': ['SMPCacheProperty'] } }
> >> 
> >> Ah, now I see why you used the singular above!
> >> 
> >> However, this type holds the properties of call caches.  It is a list
> 
> "of all caches" (can't type).

Sorry I didn't get your point?

> >> where each element holds the properties of a single cache.  Calling the
> >> former "cache property" and the latter "cache properties" is confusing.
> >
> > Yes...
> >
> >> SmpCachesProperties and SmpCacheProperties would put the singular
> >> vs. plural where it belongs.  Sounds a bit awkward to me, though.
> >> Naming is hard.
> >
> > For SmpCachesProperties, it's easy to overlook the first "s".
> >
> >> Other ideas, anybody?
> >
> > Maybe SmpCacheOptions or SmpCachesPropertyWrapper?
> 
> I wonder why we have a single QOM object to configure all caches, and
> not one QOM object per cache.

I have the thoughts and questions here:

https://lore.kernel.org/qemu-devel/20240704031603.1744546-1-zhao1.liu@intel.com/T/#m8adba8ba14ebac0c9935fbf45983cc71e53ccf45

We could discuss this issue in that thread :-).

> >> > diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> >> > index b1581988e4eb..25394f2cda50 100644
> >> > --- a/qapi/qapi-schema.json
> >> > +++ b/qapi/qapi-schema.json
> >> > @@ -64,11 +64,11 @@
> >> >  { 'include': 'compat.json' }
> >> >  { 'include': 'control.json' }
> >> >  { 'include': 'introspect.json' }
> >> > -{ 'include': 'qom.json' }
> >> > -{ 'include': 'qdev.json' }
> >> >  { 'include': 'machine-common.json' }
> >> >  { 'include': 'machine.json' }
> >> >  { 'include': 'machine-target.json' }
> >> > +{ 'include': 'qom.json' }
> >> > +{ 'include': 'qdev.json' }
> >> >  { 'include': 'replay.json' }
> >> >  { 'include': 'yank.json' }
> >> >  { 'include': 'misc.json' }
> >> 
> >> Worth explaining in the commit message, I think.
> >
> > Because of the include relationship between the json files, I need to
> > change the order. I had a "crazy" proposal to clean up this:
> > https://lore.kernel.org/qemu-devel/20240517062748.782366-1-zhao1.liu@intel.com/
> >
> > Thanks,
> > Zhao
> 

Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object

[Zhao Liu]

Hi Daniel,

On Wed, Jul 24, 2024 at 10:03:02PM +0800, Zhao Liu wrote:
> Date: Wed, 24 Jul 2024 22:03:02 +0800
> From: Zhao Liu <zhao1.liu@intel.com>
> Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object
> 
> On Wed, Jul 24, 2024 at 01:47:16PM +0100, Daniel P. Berrang? wrote:
> > Date: Wed, 24 Jul 2024 13:47:16 +0100
> > From: "Daniel P. Berrang?" <berrange@redhat.com>
> > Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object
> > 
> > On Wed, Jul 24, 2024 at 01:35:17PM +0200, Markus Armbruster wrote:
> > > Zhao Liu <zhao1.liu@intel.com> writes:
> > > 
> > > > Hi Markus,
> > > >> SmpCachesProperties and SmpCacheProperties would put the singular
> > > >> vs. plural where it belongs.  Sounds a bit awkward to me, though.
> > > >> Naming is hard.
> > > >
> > > > For SmpCachesProperties, it's easy to overlook the first "s".
> > > >
> > > >> Other ideas, anybody?
> > > >
> > > > Maybe SmpCacheOptions or SmpCachesPropertyWrapper?
> > > 
> > > I wonder why we have a single QOM object to configure all caches, and
> > > not one QOM object per cache.
> > 
> > Previous versions of this series were augmenting the existing
> > -smp command line.
> 
> Ah, yes, since -smp, as a sugar option of -machine, doesn't support
> JSON. In -smp, we need to use keyval's style to configure as:
> 
> -smp caches.0.name=l1i,caches.0.topo=core
> 
> I think JSON is the more elegant way to go, so I chose -object.

I may have to retract this assertion considering more issues, I could
fall back to -smp and support it in keyval format, I think it's also ok
for me if you also like keyval format, sorry for my repetition, we can
discuss this in this thread:

https://lore.kernel.org/qemu-devel/20240704031603.1744546-1-zhao1.liu@intel.com/T/#m8adba8ba14ebac0c9935fbf45983cc71e53ccf45

Thanks,
Zhao

Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object

[Markus Armbruster]

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

> Hi Markus,
>
> I realized I should reply this mail first...
>
> On Wed, Jul 24, 2024 at 01:35:17PM +0200, Markus Armbruster wrote:
>> Date: Wed, 24 Jul 2024 13:35:17 +0200
>> From: Markus Armbruster <armbru@redhat.com>
>> Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object
>> 
>> Zhao Liu <zhao1.liu@intel.com> writes:
>> 
>> > Hi Markus,
>> >
>> > On Mon, Jul 22, 2024 at 03:33:13PM +0200, Markus Armbruster wrote:
>> >> Date: Mon, 22 Jul 2024 15:33:13 +0200
>> >> From: Markus Armbruster <armbru@redhat.com>
>> >> Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object
>> >> 
>> >> Zhao Liu <zhao1.liu@intel.com> writes:
>> >> 
>> >> > Introduce smp-cache object so that user could define cache properties.
>> >> >
>> >> > In smp-cache object, define cache topology based on CPU topology level
>> >> > with 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, enumerate the L1 D-cache, L1 I-cache, L2 cache and L3 cache
>> >> > with smp-cache object to add the basic cache topology support.
>> >> >
>> >> > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
>> >> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
>> >> 
>> >> [...]
>> >> 
>> >> > diff --git a/qapi/machine-common.json b/qapi/machine-common.json
>> >> > index 82413c668bdb..8b8c0e9eeb86 100644
>> >> > --- a/qapi/machine-common.json
>> >> > +++ b/qapi/machine-common.json
>> >> > @@ -64,3 +64,53 @@
>> >> >    'prefix': 'CPU_TOPO_LEVEL',
>> >> >    'data': [ 'invalid', 'thread', 'core', 'module', 'cluster',
>> >> >              'die', 'socket', 'book', 'drawer', 'default' ] }
>> >> > +
>> >> > +##
>> >> > +# @SMPCacheName:
>> >> 
>> >> Why the SMP in this name?  Because it's currently only used by SMP
>> >> stuff?  Or is there another reason I'm missing?
>> >
>> > Yes, I suppose it can only be used in SMP case.
>> >
>> > Because Intel's heterogeneous CPUs have different topologies for cache,
>> > for example, Alderlake's L2, for P core, L2 is per P-core, but for E
>> > core, L2 is per module (4 E cores per module). Thus I would like to keep
>> > the topology semantics of this object and -smp as consistent as possible.
>> >
>> > Do you agree?
>> 
>> I don't know enough to meaningfully agree or disagree.  I know just
>> enough to annoy you with questions :)
>
> Welcome and no problem!
>
>> This series adds a way to configure caches.
>> 
>> Structure of the configuration data: a list
>> 
>>     [{"name": N, "topo": T}, ...]
>> 
>> where N can be "l1d", "l1i", "l2", or "l3",
>>   and T can be "invalid", "thread", "core", "module", "cluster",
>>                "die", "socket", "book", "drawer", or "default".
>> 
>> What's the use case?  The commit messages don't tell.
>
> i386 has the default cache topology model: l1 per core/l2 per core/l3
> per die.
>
> Cache topology affects scheduler performance, e.g., kernel's cluster
> scheduling.
>
> Of course I can hardcode some cache topology model in the specific cpu
> model that corresponds to the actual hardware, but for -cpu host/max,
> the default i386 cache topology model has no flexibility, and the
> host-cpu-cache option doesn't have enough fine-grained control over the
> cache topology.
>
> So I want to provide a way to allow user create more fleasible cache
> topology. Just like cpu topology.


So the use case is exposing a configurable cache topology to the guest
in order to increase performance.  Performance can increase when the
configured virtual topology is closer to the physical topology than a
default topology would be.  This can be the case with CPU host or max.

Correct?

>> Why does that use case make no sense without SMP?
>
> As the example I mentioned, for Intel hyrbid architecture, P cores has
> l2 per core and E cores has l2 per module. Then either setting the l2
> topology level as core nor module, can emulate the real case.
>
> Even considering the more extreme case of Intel 14th MTL CPU, where
> some E cores have L3 and some don't even have L3. As well as the last
> time you and Daniel mentioned that in the future we could consider
> covering more cache properties such as cache size. But the l3 size can
> be different in the same system, like AMD's x3D technology. So
> generally configuring properties for @name in a list can't take into
> account the differences of heterogeneous caches with the same @name.
>
> Hope my poor english explains the problem well. :-)

I think I understand why you want to configure caches.  My question was
about the connection to SMP.

Say we run a guest with a single core, no SMP.  Could configuring caches
still be useful then?

>> Can the same @name occur multiple times?  Documentation doesn't tell.
>> If yes, what does that mean?
>
> Yes, this means the later one will override the previous one with the same
> name.

Needs documenting.

If you make it an error, you don't have to document it :)

>> Say we later add value "l1" for unified level 1 cache.  Would "l1" then
>> conflict with "l1d" and "l1u"?
>
> Yes, we should check in smp/machine code and ban l1 and l1i/l1d at the
> same time. This check I suppose is easy to add.
>
>> May @topo be "invalid"?  Documentation doesn't tell.  If yes, what does
>> that mean?
>
> Yes, just follow the intel's spec, invalid means the current topology
> information is invalid, which is used to encode x86 CPUIDs. So when I
> move this level to qapi, I just keeped this. Otherwise, I need to
> re-implement the i386 specific invalid level.

I'm afraid I don't understand what is supposed to happen when I tell
QEMU to make a cache's topology invalid.

>> >> The more idiomatic QAPI name would be SmpCacheName.  Likewise for the
>> >> other type names below.
>> >
>> > I hesitated here as well, but considering that SMPConfiguration is "SMP"
>> > and not "Smp", it has that name. I'll change to SmpCacheName for strict
>> > initial capitalization.
>> >
>> >> > +#
>> >> > +# An enumeration of cache for SMP systems.  The cache name here is
>> >> > +# a combination of cache level and cache type.
>> >> 
>> >> The first sentence feels awkward.  Maybe
>> >> 
>> >>    # Caches an SMP system may have.
>> >> 
>> >> > +#
>> >> > +# @l1d: L1 data cache.
>> >> > +#
>> >> > +# @l1i: L1 instruction cache.
>> >> > +#
>> >> > +# @l2: L2 (unified) cache.
>> >> > +#
>> >> > +# @l3: L3 (unified) cache
>> >> > +#
>> >> > +# Since: 9.1
>> >> > +##
>> >> 
>> >> This assumes the L1 cache is split, and L2 and L3 are unified.
>> >> 
>> >> If we model a system with say a unified L1 cache, we'd simply extend
>> >> this enum.  No real difference to extending it for additional levels.
>> >> Correct?
>> >
>> > Yes. For unified L1, we just need add a "l1" which is opposed to l1i/l1d.
>> >
>> >> > +{ 'enum': 'SMPCacheName',
>> >> > +  'prefix': 'SMP_CACHE',
>> >> 
>> >> Why not call it SmpCache, and ditch 'prefix'?
>> >
>> > Because the SMPCache structure in smp_cache.h uses the similar name:
>> >
>> > +#define TYPE_SMP_CACHE "smp-cache"
>> > +OBJECT_DECLARE_SIMPLE_TYPE(SMPCache, SMP_CACHE)
>> > +
>> > +struct SMPCache {
>> > +    Object parent_obj;
>> > +
>> > +    SMPCacheProperty props[SMP_CACHE__MAX];
>> > +};
>> >
>> > Naming is always difficult,
>> 
>> Oh yes.
>> 
>> >                             so I would use Smpcache here if you feel that
>> > SmpCache is sufficient to distinguish it from SMPCache, or I would also
>> > rename the SMPCache structure to SMPCacheState in smp_cache.h.
>> >
>> > Which way do you prefer?
>> 
>> Having both QAPI enum SmpCache and handwritten type SMPCache is clearly
>> undesirable.
>> 
>> I retract my suggestion to name the enum SmpCache.  The thing clearly is
>> not a cache.  SmpCacheName is better.
>> 
>> If you drop 'prefix', the generated C enum values look like
>> SMP_CACHE_NAME_FOO.  Would that work for you?
>
> I think the SmpCacheName is ok, since there's no other better names.
>
>> The "name" part bothers me a bit.  A name doesn't define what something
>> is.  A type does.  SmpCacheType?
>
> Ah, I also considerred this. I didn't use "type" because people usually
> uses cache type to indicate INSTRUCTION/DATA/UNIFIED and cache level to
> indicate LEVEL 1/LEVEL 2/LEVEL 3. The enumeration here is a combination of
> type+level. So I think it's better to avoid the type term.

SmpCacheLevelAndType is quite a mouthful.

>> >> > +  'data': [ 'l1d', 'l1i', 'l2', 'l3' ] }
>> >> 
>> >> > +
>> >> > +##
>> >> > +# @SMPCacheProperty:
>> >> 
>> >> Sure we want to call this "property" (singular) and not "properties"?
>> >> What if we add members to this type?
>> >> 
>> >> > +#
>> >> > +# Cache information for SMP systems.
>> >> > +#
>> >> > +# @name: Cache name.
>> >> > +#
>> >> > +# @topo: Cache topology level.  It accepts the CPU topology
>> >> > +#     enumeration as the parameter, i.e., CPUs in the same
>> >> > +#     topology container share the same cache.
>> >> > +#
>> >> > +# Since: 9.1
>> >> > +##
>> >> > +{ 'struct': 'SMPCacheProperty',
>> >> > +  'data': {
>> >> > +  'name': 'SMPCacheName',
>> >> > +  'topo': 'CpuTopologyLevel' } }
>> >> 
>> >> We tend to avoid abbreviations in the QAPI schema.  Please consider
>> >> naming this 'topology'.
>> >
>> > Sure!
>> >
>> >> > +
>> >> > +##
>> >> > +# @SMPCacheProperties:
>> >> > +#
>> >> > +# List wrapper of SMPCacheProperty.
>> >> > +#
>> >> > +# @caches: the SMPCacheProperty list.
>> >> > +#
>> >> > +# Since 9.1
>> >> > +##
>> >> > +{ 'struct': 'SMPCacheProperties',
>> >> > +  'data': { 'caches': ['SMPCacheProperty'] } }
>> >> 
>> >> Ah, now I see why you used the singular above!
>> >> 
>> >> However, this type holds the properties of call caches.  It is a list
>> 
>> "of all caches" (can't type).
>
> Sorry I didn't get your point?

I had typoed "of call caches", which makes no sense, so I corrected it.

>> >> where each element holds the properties of a single cache.  Calling the
>> >> former "cache property" and the latter "cache properties" is confusing.
>> >
>> > Yes...
>> >
>> >> SmpCachesProperties and SmpCacheProperties would put the singular
>> >> vs. plural where it belongs.  Sounds a bit awkward to me, though.
>> >> Naming is hard.
>> >
>> > For SmpCachesProperties, it's easy to overlook the first "s".
>> >
>> >> Other ideas, anybody?
>> >
>> > Maybe SmpCacheOptions or SmpCachesPropertyWrapper?
>> 
>> I wonder why we have a single QOM object to configure all caches, and
>> not one QOM object per cache.
>
> I have the thoughts and questions here:
>
> https://lore.kernel.org/qemu-devel/20240704031603.1744546-1-zhao1.liu@intel.com/T/#m8adba8ba14ebac0c9935fbf45983cc71e53ccf45
>
> We could discuss this issue in that thread :-).

Okay.

[...]

Re: [PATCH 8/8] qemu-options: Add the description of smp-cache object

[Markus Armbruster]

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

> Hi Markus and Daniel,
>
> I have the questions about the -object per cache implementation:
>
> On Wed, Jul 24, 2024 at 02:39:29PM +0200, Markus Armbruster wrote:
>> Date: Wed, 24 Jul 2024 14:39:29 +0200
>> From: Markus Armbruster <armbru@redhat.com>
>> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
>>  object
>> 
>> Zhao Liu <zhao1.liu@intel.com> writes:
>> 
>> > Hi Markus,
>> >
>> > On Mon, Jul 22, 2024 at 03:37:43PM +0200, Markus Armbruster wrote:
>> >> Date: Mon, 22 Jul 2024 15:37:43 +0200
>> >> From: Markus Armbruster <armbru@redhat.com>
>> >> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
>> >>  object
>> >> 
>> >> Zhao Liu <zhao1.liu@intel.com> writes:
>> >> 
>> >> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
>> >> 
>> >> This patch is just documentation.  The code got added in some previous
>> >> patch.  Would it make sense to squash this patch into that previous
>> >> patch?
>> >
>> > OK, I'll merge them.
>> >
>> >> > ---
>> >> > Changes since RFC v2:
>> >> >  * Rewrote the document of smp-cache object.
>> >> >
>> >> > 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
>> >> >  1 file changed, 58 insertions(+)
>> >> >
>> >> > diff --git a/qemu-options.hx b/qemu-options.hx
>> >> > index 8ca7f34ef0c8..4b84f4508a6e 100644
>> >> > --- a/qemu-options.hx
>> >> > +++ b/qemu-options.hx
>> >> > @@ -159,6 +159,15 @@ SRST
>> >> >          ::
>> >> >  
>> >> >              -machine cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=128G,cxl-fmw.0.interleave-granularity=512
>> >> > +
>> >> > +    ``smp-cache='id'``
>> >> > +        Allows to configure cache property (now only the cache topology level).
>> >> > +
>> >> > +        For example:
>> >> > +        ::
>> >> > +
>> >> > +            -object '{"qom-type":"smp-cache","id":"cache","caches":[{"name":"l1d","topo":"core"},{"name":"l1i","topo":"core"},{"name":"l2","topo":"module"},{"name":"l3","topo":"die"}]}'
>> >> > +            -machine smp-cache=cache
>> >> >  ERST
>> >> >  
>> >> >  DEF("M", HAS_ARG, QEMU_OPTION_M,
>> >> > @@ -5871,6 +5880,55 @@ SRST
>> >> >          ::
>> >> >  
>> >> >              (qemu) qom-set /objects/iothread1 poll-max-ns 100000
>> >> > +
>> >> > +    ``-object '{"qom-type":"smp-cache","id":id,"caches":[{"name":cache_name,"topo":cache_topo}]}'``
>> >> > +        Create an smp-cache object that configures machine's cache
>> >> > +        property. Currently, cache property only include cache topology
>> >> > +        level.
>> >> > +
>> >> > +        This option must be written in JSON format to support JSON list.
>> >> 
>> >> Why?
>> >
>> > I'm not familiar with this, so I hope you could educate me if I'm wrong.
>> >
>> > All I know so far is for -object that defining a list can only be done in
>> > JSON format and not with a numeric index like a keyval based option, like:
>> >
>> > -object smp-cache,id=cache0,caches.0.name=l1i,caches.0.topo=core: Parameter 'caches' is missing
>> >
>> > the above doesn't work.
>> >
>> > Is there any other way to specify a list in command line?
>> 
>> The command line is a big, sprawling mess :)
>> 
>> -object supports either a JSON or a QemuOpts argument.  *Not* keyval!
>> 
>> Both QemuOpts and keyval parse something like KEY=VALUE,...  Keyval
>> supports arrays and objects via dotted keys.  QemuOpts doesn't natively
>> support arrays and objects, but its users can hack around that
>> limitation in various ways.  -object doesn't.  So you're right, it's
>> JSON or bust here.
>> 
>> However, if we used one object per cache instead, we could get something
>> like
>> 
>>     -object smp-cache,name=l1d,...
>>     -object smp-cache,name=l1u,...
>>     -object smp-cache,name=l2,...
>>     ...
>
> Current, I use -object to create a smp_cache object, and link it to
> MachineState by -machine,smp-cache=obj_id.
>
> Then for the objects per cache, how could I link them to machine?
>
> Is it possible that I create something static in smp_cache.c and expose
> all the cache information to machine through some interface?

Good questions.  However, before we head deeper into the weeds here, I
feel we should discuss the things below.  And before we do that, I need
a clear understanding of the use case.  Elsewhere in this thread, I just
described the use case as I understand it.  Please reply there.  I'll
then come back to this message.

[...]

Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object

[Jonathan Cameron via]

On Thu, 25 Jul 2024 11:50:59 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

Resending as this bounced due (I think) to an address typo.

> Hi Markus, Zhao Liu
> 
> From the ARM server side this is something I want to see as well.
> So I can comment on why we care.
> 
> > >> This series adds a way to configure caches.
> > >> 
> > >> Structure of the configuration data: a list
> > >> 
> > >>     [{"name": N, "topo": T}, ...]
> > >> 
> > >> where N can be "l1d", "l1i", "l2", or "l3",
> > >>   and T can be "invalid", "thread", "core", "module", "cluster",
> > >>                "die", "socket", "book", "drawer", or "default".
> > >> 
> > >> What's the use case?  The commit messages don't tell.    
> > >
> > > i386 has the default cache topology model: l1 per core/l2 per core/l3
> > > per die.
> > >
> > > Cache topology affects scheduler performance, e.g., kernel's cluster
> > > scheduling.
> > >
> > > Of course I can hardcode some cache topology model in the specific cpu
> > > model that corresponds to the actual hardware, but for -cpu host/max,
> > > the default i386 cache topology model has no flexibility, and the
> > > host-cpu-cache option doesn't have enough fine-grained control over the
> > > cache topology.
> > >
> > > So I want to provide a way to allow user create more fleasible cache
> > > topology. Just like cpu topology.    
> > 
> > 
> > So the use case is exposing a configurable cache topology to the guest
> > in order to increase performance.  Performance can increase when the
> > configured virtual topology is closer to the physical topology than a
> > default topology would be.  This can be the case with CPU host or max.
> > 
> > Correct?  
> 
> That is definitely why we want it on arm64 where this info fills in
> the topology we can't get from the CPU registers.
> (we should have patches on top of this to send out shortly).
> 
> As a side note we also need this for MPAM emulation for TCG
> (any maybe eventually paravirtualized MPAM) as this is needed
> to build the right PPTT to describe the caches which we then
> query to figure out association of MPAM controls with particularly
> caches.
> 
> Size configuration is something we'll need down the line (presenting
> only part of an L3 may make sense if it's shared by multiple VMs
> or partitioned with MPAM) but that's a future question.
> 
> 
> >   
> > >> Why does that use case make no sense without SMP?    
> > >
> > > As the example I mentioned, for Intel hyrbid architecture, P cores has
> > > l2 per core and E cores has l2 per module. Then either setting the l2
> > > topology level as core nor module, can emulate the real case.
> > >
> > > Even considering the more extreme case of Intel 14th MTL CPU, where
> > > some E cores have L3 and some don't even have L3. As well as the last
> > > time you and Daniel mentioned that in the future we could consider
> > > covering more cache properties such as cache size. But the l3 size can
> > > be different in the same system, like AMD's x3D technology. So
> > > generally configuring properties for @name in a list can't take into
> > > account the differences of heterogeneous caches with the same @name.
> > >
> > > Hope my poor english explains the problem well. :-)    
> > 
> > I think I understand why you want to configure caches.  My question was
> > about the connection to SMP.
> > 
> > Say we run a guest with a single core, no SMP.  Could configuring caches
> > still be useful then?  
> 
> Probably not useful to configure topology (sizes are a separate question)
> - any sensible default should be fine.
> 
> Jonathan
> 
> 

Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object

[Zhao Liu]

Hi Markus,

On Thu, Jul 25, 2024 at 10:51:49AM +0200, Markus Armbruster wrote:

[snip]

> >> What's the use case?  The commit messages don't tell.
> >
> > i386 has the default cache topology model: l1 per core/l2 per core/l3
> > per die.
> >
> > Cache topology affects scheduler performance, e.g., kernel's cluster
> > scheduling.
> >
> > Of course I can hardcode some cache topology model in the specific cpu
> > model that corresponds to the actual hardware, but for -cpu host/max,
> > the default i386 cache topology model has no flexibility, and the
> > host-cpu-cache option doesn't have enough fine-grained control over the
> > cache topology.
> >
> > So I want to provide a way to allow user create more fleasible cache
> > topology. Just like cpu topology.
> 
> 
> So the use case is exposing a configurable cache topology to the guest
> in order to increase performance.  Performance can increase when the
> configured virtual topology is closer to the physical topology than a
> default topology would be.  This can be the case with CPU host or max.
> 
> Correct?

Yes! That's x86 use case. Jonathan also helped me explain his ARM use case.

> >> Why does that use case make no sense without SMP?
> >
> > As the example I mentioned, for Intel hyrbid architecture, P cores has
> > l2 per core and E cores has l2 per module. Then either setting the l2
> > topology level as core nor module, can emulate the real case.
> >
> > Even considering the more extreme case of Intel 14th MTL CPU, where
> > some E cores have L3 and some don't even have L3. As well as the last
> > time you and Daniel mentioned that in the future we could consider
> > covering more cache properties such as cache size. But the l3 size can
> > be different in the same system, like AMD's x3D technology. So
> > generally configuring properties for @name in a list can't take into
> > account the differences of heterogeneous caches with the same @name.
> >
> > Hope my poor english explains the problem well. :-)
> 
> I think I understand why you want to configure caches.  My question was
> about the connection to SMP.
> 
> Say we run a guest with a single core, no SMP.  Could configuring caches
> still be useful then?

No, for this case the CPU topology (of x86) would be 1 core per module, 1
module per die, 1 die per socket.

Then this core actually owns the l1/l2/l3.

> >> Can the same @name occur multiple times?  Documentation doesn't tell.
> >> If yes, what does that mean?
> >
> > Yes, this means the later one will override the previous one with the same
> > name.
> 
> Needs documenting.
> 
> If you make it an error, you don't have to document it :)

OK!

> >> Say we later add value "l1" for unified level 1 cache.  Would "l1" then
> >> conflict with "l1d" and "l1u"?
> >
> > Yes, we should check in smp/machine code and ban l1 and l1i/l1d at the
> > same time. This check I suppose is easy to add.
> >
> >> May @topo be "invalid"?  Documentation doesn't tell.  If yes, what does
> >> that mean?
> >
> > Yes, just follow the intel's spec, invalid means the current topology
> > information is invalid, which is used to encode x86 CPUIDs. So when I
> > move this level to qapi, I just keeped this. Otherwise, I need to
> > re-implement the i386 specific invalid level.
> 
> I'm afraid I don't understand what is supposed to happen when I tell
> QEMU to make a cache's topology invalid.

Currently this series doesn't allow users to set invalid, if they do, QEMU
reports an error.

So this invalid is just for QEMU internal use. Do you think it's okay?

[snip]

> > Ah, I also considerred this. I didn't use "type" because people usually
> > uses cache type to indicate INSTRUCTION/DATA/UNIFIED and cache level to
> > indicate LEVEL 1/LEVEL 2/LEVEL 3. The enumeration here is a combination of
> > type+level. So I think it's better to avoid the type term.
> 
> SmpCacheLevelAndType is quite a mouthful.

Better name! Thanks!

Regards,
Zhao

Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object

[Zhao Liu]

Thanks Jonathon!

On Thu, Jul 25, 2024 at 11:59:02AM +0100, Jonathan Cameron wrote:

[snip]

> > > I think I understand why you want to configure caches.  My question was
> > > about the connection to SMP.
> > > 
> > > Say we run a guest with a single core, no SMP.  Could configuring caches
> > > still be useful then?  
> > 
> > Probably not useful to configure topology (sizes are a separate question)
> > - any sensible default should be fine.
> >

Yes, I agree.

Regards,
Zhao

Re: [PATCH 8/8] qemu-options: Add the description of smp-cache object

[Zhao Liu]

On Thu, Jul 25, 2024 at 11:07:12AM +0200, Markus Armbruster wrote:
> Date: Thu, 25 Jul 2024 11:07:12 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
>  object
> 
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > Hi Markus and Daniel,
> >
> > I have the questions about the -object per cache implementation:
> >
> > On Wed, Jul 24, 2024 at 02:39:29PM +0200, Markus Armbruster wrote:
> >> Date: Wed, 24 Jul 2024 14:39:29 +0200
> >> From: Markus Armbruster <armbru@redhat.com>
> >> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
> >>  object
> >> 
> >> Zhao Liu <zhao1.liu@intel.com> writes:
> >> 
> >> > Hi Markus,
> >> >
> >> > On Mon, Jul 22, 2024 at 03:37:43PM +0200, Markus Armbruster wrote:
> >> >> Date: Mon, 22 Jul 2024 15:37:43 +0200
> >> >> From: Markus Armbruster <armbru@redhat.com>
> >> >> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
> >> >>  object
> >> >> 
> >> >> Zhao Liu <zhao1.liu@intel.com> writes:
> >> >> 
> >> >> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> >> >> 
> >> >> This patch is just documentation.  The code got added in some previous
> >> >> patch.  Would it make sense to squash this patch into that previous
> >> >> patch?
> >> >
> >> > OK, I'll merge them.
> >> >
> >> >> > ---
> >> >> > Changes since RFC v2:
> >> >> >  * Rewrote the document of smp-cache object.
> >> >> >
> >> >> > 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
> >> >> >  1 file changed, 58 insertions(+)
> >> >> >
> >> >> > diff --git a/qemu-options.hx b/qemu-options.hx
> >> >> > index 8ca7f34ef0c8..4b84f4508a6e 100644
> >> >> > --- a/qemu-options.hx
> >> >> > +++ b/qemu-options.hx
> >> >> > @@ -159,6 +159,15 @@ SRST
> >> >> >          ::
> >> >> >  
> >> >> >              -machine cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=128G,cxl-fmw.0.interleave-granularity=512
> >> >> > +
> >> >> > +    ``smp-cache='id'``
> >> >> > +        Allows to configure cache property (now only the cache topology level).
> >> >> > +
> >> >> > +        For example:
> >> >> > +        ::
> >> >> > +
> >> >> > +            -object '{"qom-type":"smp-cache","id":"cache","caches":[{"name":"l1d","topo":"core"},{"name":"l1i","topo":"core"},{"name":"l2","topo":"module"},{"name":"l3","topo":"die"}]}'
> >> >> > +            -machine smp-cache=cache
> >> >> >  ERST
> >> >> >  
> >> >> >  DEF("M", HAS_ARG, QEMU_OPTION_M,
> >> >> > @@ -5871,6 +5880,55 @@ SRST
> >> >> >          ::
> >> >> >  
> >> >> >              (qemu) qom-set /objects/iothread1 poll-max-ns 100000
> >> >> > +
> >> >> > +    ``-object '{"qom-type":"smp-cache","id":id,"caches":[{"name":cache_name,"topo":cache_topo}]}'``
> >> >> > +        Create an smp-cache object that configures machine's cache
> >> >> > +        property. Currently, cache property only include cache topology
> >> >> > +        level.
> >> >> > +
> >> >> > +        This option must be written in JSON format to support JSON list.
> >> >> 
> >> >> Why?
> >> >
> >> > I'm not familiar with this, so I hope you could educate me if I'm wrong.
> >> >
> >> > All I know so far is for -object that defining a list can only be done in
> >> > JSON format and not with a numeric index like a keyval based option, like:
> >> >
> >> > -object smp-cache,id=cache0,caches.0.name=l1i,caches.0.topo=core: Parameter 'caches' is missing
> >> >
> >> > the above doesn't work.
> >> >
> >> > Is there any other way to specify a list in command line?
> >> 
> >> The command line is a big, sprawling mess :)
> >> 
> >> -object supports either a JSON or a QemuOpts argument.  *Not* keyval!
> >> 
> >> Both QemuOpts and keyval parse something like KEY=VALUE,...  Keyval
> >> supports arrays and objects via dotted keys.  QemuOpts doesn't natively
> >> support arrays and objects, but its users can hack around that
> >> limitation in various ways.  -object doesn't.  So you're right, it's
> >> JSON or bust here.
> >> 
> >> However, if we used one object per cache instead, we could get something
> >> like
> >> 
> >>     -object smp-cache,name=l1d,...
> >>     -object smp-cache,name=l1u,...
> >>     -object smp-cache,name=l2,...
> >>     ...
> >
> > Current, I use -object to create a smp_cache object, and link it to
> > MachineState by -machine,smp-cache=obj_id.
> >
> > Then for the objects per cache, how could I link them to machine?
> >
> > Is it possible that I create something static in smp_cache.c and expose
> > all the cache information to machine through some interface?
> 
> Good questions.  However, before we head deeper into the weeds here, I
> feel we should discuss the things below.  And before we do that, I need
> a clear understanding of the use case.  Elsewhere in this thread, I just
> described the use case as I understand it.  Please reply there.  I'll
> then come back to this message.
> 
> [...]

Jonathan and I provided different use cases for x86 and Arm. Could we
come back here to continue the discussion? :)

Thanks,
Zhao

Re: [PATCH 8/8] qemu-options: Add the description of smp-cache object

[Markus Armbruster]

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

> On Thu, Jul 25, 2024 at 11:07:12AM +0200, Markus Armbruster wrote:
>> Date: Thu, 25 Jul 2024 11:07:12 +0200
>> From: Markus Armbruster <armbru@redhat.com>
>> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
>>  object
>> 
>> Zhao Liu <zhao1.liu@intel.com> writes:
>> 
>> > Hi Markus and Daniel,
>> >
>> > I have the questions about the -object per cache implementation:
>> >
>> > On Wed, Jul 24, 2024 at 02:39:29PM +0200, Markus Armbruster wrote:
>> >> Date: Wed, 24 Jul 2024 14:39:29 +0200
>> >> From: Markus Armbruster <armbru@redhat.com>
>> >> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
>> >>  object
>> >> 
>> >> Zhao Liu <zhao1.liu@intel.com> writes:
>> >> 
>> >> > Hi Markus,
>> >> >
>> >> > On Mon, Jul 22, 2024 at 03:37:43PM +0200, Markus Armbruster wrote:
>> >> >> Date: Mon, 22 Jul 2024 15:37:43 +0200
>> >> >> From: Markus Armbruster <armbru@redhat.com>
>> >> >> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
>> >> >>  object
>> >> >> 
>> >> >> Zhao Liu <zhao1.liu@intel.com> writes:
>> >> >> 
>> >> >> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
>> >> >> 
>> >> >> This patch is just documentation.  The code got added in some previous
>> >> >> patch.  Would it make sense to squash this patch into that previous
>> >> >> patch?
>> >> >
>> >> > OK, I'll merge them.
>> >> >
>> >> >> > ---
>> >> >> > Changes since RFC v2:
>> >> >> >  * Rewrote the document of smp-cache object.
>> >> >> >
>> >> >> > 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
>> >> >> >  1 file changed, 58 insertions(+)
>> >> >> >
>> >> >> > diff --git a/qemu-options.hx b/qemu-options.hx
>> >> >> > index 8ca7f34ef0c8..4b84f4508a6e 100644
>> >> >> > --- a/qemu-options.hx
>> >> >> > +++ b/qemu-options.hx
>> >> >> > @@ -159,6 +159,15 @@ SRST
>> >> >> >          ::
>> >> >> >  
>> >> >> >              -machine cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=128G,cxl-fmw.0.interleave-granularity=512
>> >> >> > +
>> >> >> > +    ``smp-cache='id'``
>> >> >> > +        Allows to configure cache property (now only the cache topology level).
>> >> >> > +
>> >> >> > +        For example:
>> >> >> > +        ::
>> >> >> > +
>> >> >> > +            -object '{"qom-type":"smp-cache","id":"cache","caches":[{"name":"l1d","topo":"core"},{"name":"l1i","topo":"core"},{"name":"l2","topo":"module"},{"name":"l3","topo":"die"}]}'
>> >> >> > +            -machine smp-cache=cache
>> >> >> >  ERST
>> >> >> >  
>> >> >> >  DEF("M", HAS_ARG, QEMU_OPTION_M,
>> >> >> > @@ -5871,6 +5880,55 @@ SRST
>> >> >> >          ::
>> >> >> >  
>> >> >> >              (qemu) qom-set /objects/iothread1 poll-max-ns 100000
>> >> >> > +
>> >> >> > +    ``-object '{"qom-type":"smp-cache","id":id,"caches":[{"name":cache_name,"topo":cache_topo}]}'``
>> >> >> > +        Create an smp-cache object that configures machine's cache
>> >> >> > +        property. Currently, cache property only include cache topology
>> >> >> > +        level.
>> >> >> > +
>> >> >> > +        This option must be written in JSON format to support JSON list.
>> >> >> 
>> >> >> Why?
>> >> >
>> >> > I'm not familiar with this, so I hope you could educate me if I'm wrong.
>> >> >
>> >> > All I know so far is for -object that defining a list can only be done in
>> >> > JSON format and not with a numeric index like a keyval based option, like:
>> >> >
>> >> > -object smp-cache,id=cache0,caches.0.name=l1i,caches.0.topo=core: Parameter 'caches' is missing
>> >> >
>> >> > the above doesn't work.
>> >> >
>> >> > Is there any other way to specify a list in command line?
>> >> 
>> >> The command line is a big, sprawling mess :)
>> >> 
>> >> -object supports either a JSON or a QemuOpts argument.  *Not* keyval!
>> >> 
>> >> Both QemuOpts and keyval parse something like KEY=VALUE,...  Keyval
>> >> supports arrays and objects via dotted keys.  QemuOpts doesn't natively
>> >> support arrays and objects, but its users can hack around that
>> >> limitation in various ways.  -object doesn't.  So you're right, it's
>> >> JSON or bust here.
>> >> 
>> >> However, if we used one object per cache instead, we could get something
>> >> like
>> >> 
>> >>     -object smp-cache,name=l1d,...
>> >>     -object smp-cache,name=l1u,...
>> >>     -object smp-cache,name=l2,...
>> >>     ...
>> >
>> > Current, I use -object to create a smp_cache object, and link it to
>> > MachineState by -machine,smp-cache=obj_id.
>> >
>> > Then for the objects per cache, how could I link them to machine?
>> >
>> > Is it possible that I create something static in smp_cache.c and expose
>> > all the cache information to machine through some interface?
>> 
>> Good questions.  However, before we head deeper into the weeds here, I
>> feel we should discuss the things below.  And before we do that, I need
>> a clear understanding of the use case.  Elsewhere in this thread, I just
>> described the use case as I understand it.  Please reply there.  I'll
>> then come back to this message.
>> 
>> [...]
>
> Jonathan and I provided different use cases for x86 and Arm. Could we
> come back here to continue the discussion? :)

Can you provide a brief summary of the design alternatives that have
been proposed so far?  Because I've lost track.

Re: [PATCH 8/8] qemu-options: Add the description of smp-cache object

[Zhao Liu]

On Thu, Aug 01, 2024 at 01:28:27PM +0200, Markus Armbruster wrote:
> Date: Thu, 01 Aug 2024 13:28:27 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
>  object
> 
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > On Thu, Jul 25, 2024 at 11:07:12AM +0200, Markus Armbruster wrote:
> >> Date: Thu, 25 Jul 2024 11:07:12 +0200
> >> From: Markus Armbruster <armbru@redhat.com>
> >> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
> >>  object
> >> 
> >> Zhao Liu <zhao1.liu@intel.com> writes:
> >> 
> >> > Hi Markus and Daniel,
> >> >
> >> > I have the questions about the -object per cache implementation:
> >> >
> >> > On Wed, Jul 24, 2024 at 02:39:29PM +0200, Markus Armbruster wrote:
> >> >> Date: Wed, 24 Jul 2024 14:39:29 +0200
> >> >> From: Markus Armbruster <armbru@redhat.com>
> >> >> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
> >> >>  object
> >> >> 
> >> >> Zhao Liu <zhao1.liu@intel.com> writes:
> >> >> 
> >> >> > Hi Markus,
> >> >> >
> >> >> > On Mon, Jul 22, 2024 at 03:37:43PM +0200, Markus Armbruster wrote:
> >> >> >> Date: Mon, 22 Jul 2024 15:37:43 +0200
> >> >> >> From: Markus Armbruster <armbru@redhat.com>
> >> >> >> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
> >> >> >>  object
> >> >> >> 
> >> >> >> Zhao Liu <zhao1.liu@intel.com> writes:
> >> >> >> 
> >> >> >> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> >> >> >> 
> >> >> >> This patch is just documentation.  The code got added in some previous
> >> >> >> patch.  Would it make sense to squash this patch into that previous
> >> >> >> patch?
> >> >> >
> >> >> > OK, I'll merge them.
> >> >> >
> >> >> >> > ---
> >> >> >> > Changes since RFC v2:
> >> >> >> >  * Rewrote the document of smp-cache object.
> >> >> >> >
> >> >> >> > 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
> >> >> >> >  1 file changed, 58 insertions(+)
> >> >> >> >
> >> >> >> > diff --git a/qemu-options.hx b/qemu-options.hx
> >> >> >> > index 8ca7f34ef0c8..4b84f4508a6e 100644
> >> >> >> > --- a/qemu-options.hx
> >> >> >> > +++ b/qemu-options.hx
> >> >> >> > @@ -159,6 +159,15 @@ SRST
> >> >> >> >          ::
> >> >> >> >  
> >> >> >> >              -machine cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=128G,cxl-fmw.0.interleave-granularity=512
> >> >> >> > +
> >> >> >> > +    ``smp-cache='id'``
> >> >> >> > +        Allows to configure cache property (now only the cache topology level).
> >> >> >> > +
> >> >> >> > +        For example:
> >> >> >> > +        ::
> >> >> >> > +
> >> >> >> > +            -object '{"qom-type":"smp-cache","id":"cache","caches":[{"name":"l1d","topo":"core"},{"name":"l1i","topo":"core"},{"name":"l2","topo":"module"},{"name":"l3","topo":"die"}]}'
> >> >> >> > +            -machine smp-cache=cache
> >> >> >> >  ERST
> >> >> >> >  
> >> >> >> >  DEF("M", HAS_ARG, QEMU_OPTION_M,
> >> >> >> > @@ -5871,6 +5880,55 @@ SRST
> >> >> >> >          ::
> >> >> >> >  
> >> >> >> >              (qemu) qom-set /objects/iothread1 poll-max-ns 100000
> >> >> >> > +
> >> >> >> > +    ``-object '{"qom-type":"smp-cache","id":id,"caches":[{"name":cache_name,"topo":cache_topo}]}'``
> >> >> >> > +        Create an smp-cache object that configures machine's cache
> >> >> >> > +        property. Currently, cache property only include cache topology
> >> >> >> > +        level.
> >> >> >> > +
> >> >> >> > +        This option must be written in JSON format to support JSON list.
> >> >> >> 
> >> >> >> Why?
> >> >> >
> >> >> > I'm not familiar with this, so I hope you could educate me if I'm wrong.
> >> >> >
> >> >> > All I know so far is for -object that defining a list can only be done in
> >> >> > JSON format and not with a numeric index like a keyval based option, like:
> >> >> >
> >> >> > -object smp-cache,id=cache0,caches.0.name=l1i,caches.0.topo=core: Parameter 'caches' is missing
> >> >> >
> >> >> > the above doesn't work.
> >> >> >
> >> >> > Is there any other way to specify a list in command line?
> >> >> 
> >> >> The command line is a big, sprawling mess :)
> >> >> 
> >> >> -object supports either a JSON or a QemuOpts argument.  *Not* keyval!
> >> >> 
> >> >> Both QemuOpts and keyval parse something like KEY=VALUE,...  Keyval
> >> >> supports arrays and objects via dotted keys.  QemuOpts doesn't natively
> >> >> support arrays and objects, but its users can hack around that
> >> >> limitation in various ways.  -object doesn't.  So you're right, it's
> >> >> JSON or bust here.
> >> >> 
> >> >> However, if we used one object per cache instead, we could get something
> >> >> like
> >> >> 
> >> >>     -object smp-cache,name=l1d,...
> >> >>     -object smp-cache,name=l1u,...
> >> >>     -object smp-cache,name=l2,...
> >> >>     ...
> >> >
> >> > Current, I use -object to create a smp_cache object, and link it to
> >> > MachineState by -machine,smp-cache=obj_id.
> >> >
> >> > Then for the objects per cache, how could I link them to machine?
> >> >
> >> > Is it possible that I create something static in smp_cache.c and expose
> >> > all the cache information to machine through some interface?
> >> 
> >> Good questions.  However, before we head deeper into the weeds here, I
> >> feel we should discuss the things below.  And before we do that, I need
> >> a clear understanding of the use case.  Elsewhere in this thread, I just
> >> described the use case as I understand it.  Please reply there.  I'll
> >> then come back to this message.
> >> 
> >> [...]
> >
> > Jonathan and I provided different use cases for x86 and Arm. Could we
> > come back here to continue the discussion? :)
> 
> Can you provide a brief summary of the design alternatives that have
> been proposed so far?  Because I've lost track.

No problem!

Currently, we have the following options:

* 1st: The first one is just to configure cache topology with several
  options in -smp:

  -smp l1i-cache-topo=core,l1d-cache-topo-core

  This one lacks scalability to support the cache size that ARM will
  need in the future.


* 2nd: The cache list object in -smp.

  The idea was to use JSON to configure the cache list. However, the
  underlying implementation of -smp at the moment is keyval parsing,
  which is not compatible with JSON.

  If we can not insist on JSON format, then cache lists can also be
  implemented in the following way:
  
  -smp caches.0.name=l1i,caches.0.topo=core,\
       caches.1.name=l1d,caches.1.topo=core


* 3rd: The cache list object linked in -machine.

  Considering that -object is JSON-compatible so that defining lists via
  JSON is more friendly, I implemented the caches list via -object and
  linked it to MachineState:

  -object '{"qom-type":"smp-cache","id":"obj","caches":[{"name":"l1d","topo":"core"},{"name":"l1i","topo":"core"}]}'
  -machine smp-caches=obj


* 4th: The per cache object without any list:

  -object smp-cache,id=cache0,name=l1i,topo=core \
  -object smp-cache,id=cache1,name=l1d,topo=core

  This proposal is clearer, but there are a few opens:
  - I plan to push qom-topo forward, which would abstract CPU related
    topology levels and cache to "device" instead of object. Is there a
    conflict here?

  - Multiple cache objects can't be linked to the machine on the command
    line, so I maintain a static cache list in smp_cache.c and expose
    the cache information to the machine through some interface. is this
    way acceptable?


In summary, the 4th proposal was the most up in the air, as it looked to
be conflict with the hybrid topology I wanted to do (and while hybrid
topology may not be accepted by the community either, I thought it would
be best for the two work to be in the same direction).

The difference between 2nd and 3rd is about the JSON requirement, if JSON
is mandatory for now then it's 3rd, if it's not mandatory (or accept to
make -machine/-smp support JSON in the future), 2nd looks cleaner, which
puts the caches list in -smp.

Regards,
Zhao

Re: [PATCH 8/8] qemu-options: Add the description of smp-cache object

[Zhao Liu]

Hi Markus,

Just a kindly ping. Hopefully we can continue this discussion when
you're free.

Regards,
Zhao

On Fri, Aug 02, 2024 at 03:58:02PM +0800, Zhao Liu wrote:
> Date: Fri, 2 Aug 2024 15:58:02 +0800
> From: Zhao Liu <zhao1.liu@intel.com>
> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
>  object
> 
> On Thu, Aug 01, 2024 at 01:28:27PM +0200, Markus Armbruster wrote:
> > Date: Thu, 01 Aug 2024 13:28:27 +0200
> > From: Markus Armbruster <armbru@redhat.com>
> > Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
> >  object
> > 
> > Zhao Liu <zhao1.liu@intel.com> writes:
> > 
> > > On Thu, Jul 25, 2024 at 11:07:12AM +0200, Markus Armbruster wrote:
> > >> Date: Thu, 25 Jul 2024 11:07:12 +0200
> > >> From: Markus Armbruster <armbru@redhat.com>
> > >> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
> > >>  object
> > >> 
> > >> Zhao Liu <zhao1.liu@intel.com> writes:
> > >> 
> > >> > Hi Markus and Daniel,
> > >> >
> > >> > I have the questions about the -object per cache implementation:
> > >> >
> > >> > On Wed, Jul 24, 2024 at 02:39:29PM +0200, Markus Armbruster wrote:
> > >> >> Date: Wed, 24 Jul 2024 14:39:29 +0200
> > >> >> From: Markus Armbruster <armbru@redhat.com>
> > >> >> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
> > >> >>  object
> > >> >> 
> > >> >> Zhao Liu <zhao1.liu@intel.com> writes:
> > >> >> 
> > >> >> > Hi Markus,
> > >> >> >
> > >> >> > On Mon, Jul 22, 2024 at 03:37:43PM +0200, Markus Armbruster wrote:
> > >> >> >> Date: Mon, 22 Jul 2024 15:37:43 +0200
> > >> >> >> From: Markus Armbruster <armbru@redhat.com>
> > >> >> >> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
> > >> >> >>  object
> > >> >> >> 
> > >> >> >> Zhao Liu <zhao1.liu@intel.com> writes:
> > >> >> >> 
> > >> >> >> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > >> >> >> 
> > >> >> >> This patch is just documentation.  The code got added in some previous
> > >> >> >> patch.  Would it make sense to squash this patch into that previous
> > >> >> >> patch?
> > >> >> >
> > >> >> > OK, I'll merge them.
> > >> >> >
> > >> >> >> > ---
> > >> >> >> > Changes since RFC v2:
> > >> >> >> >  * Rewrote the document of smp-cache object.
> > >> >> >> >
> > >> >> >> > 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >> >> >> >  1 file changed, 58 insertions(+)
> > >> >> >> >
> > >> >> >> > diff --git a/qemu-options.hx b/qemu-options.hx
> > >> >> >> > index 8ca7f34ef0c8..4b84f4508a6e 100644
> > >> >> >> > --- a/qemu-options.hx
> > >> >> >> > +++ b/qemu-options.hx
> > >> >> >> > @@ -159,6 +159,15 @@ SRST
> > >> >> >> >          ::
> > >> >> >> >  
> > >> >> >> >              -machine cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=128G,cxl-fmw.0.interleave-granularity=512
> > >> >> >> > +
> > >> >> >> > +    ``smp-cache='id'``
> > >> >> >> > +        Allows to configure cache property (now only the cache topology level).
> > >> >> >> > +
> > >> >> >> > +        For example:
> > >> >> >> > +        ::
> > >> >> >> > +
> > >> >> >> > +            -object '{"qom-type":"smp-cache","id":"cache","caches":[{"name":"l1d","topo":"core"},{"name":"l1i","topo":"core"},{"name":"l2","topo":"module"},{"name":"l3","topo":"die"}]}'
> > >> >> >> > +            -machine smp-cache=cache
> > >> >> >> >  ERST
> > >> >> >> >  
> > >> >> >> >  DEF("M", HAS_ARG, QEMU_OPTION_M,
> > >> >> >> > @@ -5871,6 +5880,55 @@ SRST
> > >> >> >> >          ::
> > >> >> >> >  
> > >> >> >> >              (qemu) qom-set /objects/iothread1 poll-max-ns 100000
> > >> >> >> > +
> > >> >> >> > +    ``-object '{"qom-type":"smp-cache","id":id,"caches":[{"name":cache_name,"topo":cache_topo}]}'``
> > >> >> >> > +        Create an smp-cache object that configures machine's cache
> > >> >> >> > +        property. Currently, cache property only include cache topology
> > >> >> >> > +        level.
> > >> >> >> > +
> > >> >> >> > +        This option must be written in JSON format to support JSON list.
> > >> >> >> 
> > >> >> >> Why?
> > >> >> >
> > >> >> > I'm not familiar with this, so I hope you could educate me if I'm wrong.
> > >> >> >
> > >> >> > All I know so far is for -object that defining a list can only be done in
> > >> >> > JSON format and not with a numeric index like a keyval based option, like:
> > >> >> >
> > >> >> > -object smp-cache,id=cache0,caches.0.name=l1i,caches.0.topo=core: Parameter 'caches' is missing
> > >> >> >
> > >> >> > the above doesn't work.
> > >> >> >
> > >> >> > Is there any other way to specify a list in command line?
> > >> >> 
> > >> >> The command line is a big, sprawling mess :)
> > >> >> 
> > >> >> -object supports either a JSON or a QemuOpts argument.  *Not* keyval!
> > >> >> 
> > >> >> Both QemuOpts and keyval parse something like KEY=VALUE,...  Keyval
> > >> >> supports arrays and objects via dotted keys.  QemuOpts doesn't natively
> > >> >> support arrays and objects, but its users can hack around that
> > >> >> limitation in various ways.  -object doesn't.  So you're right, it's
> > >> >> JSON or bust here.
> > >> >> 
> > >> >> However, if we used one object per cache instead, we could get something
> > >> >> like
> > >> >> 
> > >> >>     -object smp-cache,name=l1d,...
> > >> >>     -object smp-cache,name=l1u,...
> > >> >>     -object smp-cache,name=l2,...
> > >> >>     ...
> > >> >
> > >> > Current, I use -object to create a smp_cache object, and link it to
> > >> > MachineState by -machine,smp-cache=obj_id.
> > >> >
> > >> > Then for the objects per cache, how could I link them to machine?
> > >> >
> > >> > Is it possible that I create something static in smp_cache.c and expose
> > >> > all the cache information to machine through some interface?
> > >> 
> > >> Good questions.  However, before we head deeper into the weeds here, I
> > >> feel we should discuss the things below.  And before we do that, I need
> > >> a clear understanding of the use case.  Elsewhere in this thread, I just
> > >> described the use case as I understand it.  Please reply there.  I'll
> > >> then come back to this message.
> > >> 
> > >> [...]
> > >
> > > Jonathan and I provided different use cases for x86 and Arm. Could we
> > > come back here to continue the discussion? :)
> > 
> > Can you provide a brief summary of the design alternatives that have
> > been proposed so far?  Because I've lost track.
> 
> No problem!
> 
> Currently, we have the following options:
> 
> * 1st: The first one is just to configure cache topology with several
>   options in -smp:
> 
>   -smp l1i-cache-topo=core,l1d-cache-topo-core
> 
>   This one lacks scalability to support the cache size that ARM will
>   need in the future.
> 
> 
> * 2nd: The cache list object in -smp.
> 
>   The idea was to use JSON to configure the cache list. However, the
>   underlying implementation of -smp at the moment is keyval parsing,
>   which is not compatible with JSON.
> 
>   If we can not insist on JSON format, then cache lists can also be
>   implemented in the following way:
>   
>   -smp caches.0.name=l1i,caches.0.topo=core,\
>        caches.1.name=l1d,caches.1.topo=core
> 
> 
> * 3rd: The cache list object linked in -machine.
> 
>   Considering that -object is JSON-compatible so that defining lists via
>   JSON is more friendly, I implemented the caches list via -object and
>   linked it to MachineState:
> 
>   -object '{"qom-type":"smp-cache","id":"obj","caches":[{"name":"l1d","topo":"core"},{"name":"l1i","topo":"core"}]}'
>   -machine smp-caches=obj
> 
> 
> * 4th: The per cache object without any list:
> 
>   -object smp-cache,id=cache0,name=l1i,topo=core \
>   -object smp-cache,id=cache1,name=l1d,topo=core
> 
>   This proposal is clearer, but there are a few opens:
>   - I plan to push qom-topo forward, which would abstract CPU related
>     topology levels and cache to "device" instead of object. Is there a
>     conflict here?
> 
>   - Multiple cache objects can't be linked to the machine on the command
>     line, so I maintain a static cache list in smp_cache.c and expose
>     the cache information to the machine through some interface. is this
>     way acceptable?
> 
> 
> In summary, the 4th proposal was the most up in the air, as it looked to
> be conflict with the hybrid topology I wanted to do (and while hybrid
> topology may not be accepted by the community either, I thought it would
> be best for the two work to be in the same direction).
> 
> The difference between 2nd and 3rd is about the JSON requirement, if JSON
> is mandatory for now then it's 3rd, if it's not mandatory (or accept to
> make -machine/-smp support JSON in the future), 2nd looks cleaner, which
> puts the caches list in -smp.
> 
> Regards,
> Zhao
> 
> 
> 

Re: [PATCH 8/8] qemu-options: Add the description of smp-cache object

[Markus Armbruster]

I apologize for the delay.

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

> On Thu, Aug 01, 2024 at 01:28:27PM +0200, Markus Armbruster wrote:

[...]

>> Can you provide a brief summary of the design alternatives that have
>> been proposed so far?  Because I've lost track.
>
> No problem!
>
> Currently, we have the following options:
>
> * 1st: The first one is just to configure cache topology with several
>   options in -smp:
>
>   -smp l1i-cache-topo=core,l1d-cache-topo-core
>
>   This one lacks scalability to support the cache size that ARM will
>   need in the future.

-smp sets machine property "smp" of QAPI type SMPConfiguration.

So this one adds members l1i-cache-topo, l1d-cache-topo, ... to
SMPConfiguration.

> * 2nd: The cache list object in -smp.
>
>   The idea was to use JSON to configure the cache list. However, the
>   underlying implementation of -smp at the moment is keyval parsing,
>   which is not compatible with JSON.

Keyval is a variation of the QEMU's traditional KEY=VALUE,... syntax
that can serve as an alternative to JSON, with certain restrictions.
Ideally, we provide both JSON and keyval syntax on the command line.

Example: -blockdev supports both JSON and keyval.
    JSON:   -blockdev '{"driver": "null-co", "node-name": "node0"}'
    keyval: -blockdev null-co,node-name=node0

Unfortunately, we have many old interfaces that still lack JSON support.

>   If we can not insist on JSON format, then cache lists can also be
>   implemented in the following way:
>   
>   -smp caches.0.name=l1i,caches.0.topo=core,\
>        caches.1.name=l1d,caches.1.topo=core

This one adds a single member caches to SMPConfiguration.  It is an
array of objects.

> * 3rd: The cache list object linked in -machine.
>
>   Considering that -object is JSON-compatible so that defining lists via
>   JSON is more friendly, I implemented the caches list via -object and
>   linked it to MachineState:
>
>   -object '{"qom-type":"smp-cache","id":"obj","caches":[{"name":"l1d","topo":"core"},{"name":"l1i","topo":"core"}]}'
>   -machine smp-caches=obj

This one wraps the same array of objects in a new user-creatable object,
then sets machine property "smp-caches" to that object.

We can set machine properties directly with -machine.  But -machine
doesn't support JSON, yet.

Wrapping in an object moves the configuration to -object, which does
support JSON.

Half way between 2nd and 3rd:

  * Cache list object in machine

    -machine caches.0.name=l1i,caches.0.topo=core,\
             caches.1.name=l1d,caches.1.topo=core

> * 4th: The per cache object without any list:
>
>   -object smp-cache,id=cache0,name=l1i,topo=core \
>   -object smp-cache,id=cache1,name=l1d,topo=core
>
>   This proposal is clearer, but there are a few opens:
>   - I plan to push qom-topo forward, which would abstract CPU related
>     topology levels and cache to "device" instead of object. Is there a
>     conflict here?

Can't say, since I don't understand where you want to go.

Looks like your trying to design an interface for what you want to do
now, and are wondering whether it could evolve to accomodate what you
want to do later.

It's often better to design the interface for everything you already
know you want to do, then take out the parts you want to do later.

>   - Multiple cache objects can't be linked to the machine on the command
>     line, so I maintain a static cache list in smp_cache.c and expose
>     the cache information to the machine through some interface. is this
>     way acceptable?
>
>
> In summary, the 4th proposal was the most up in the air, as it looked to
> be conflict with the hybrid topology I wanted to do (and while hybrid
> topology may not be accepted by the community either, I thought it would
> be best for the two work to be in the same direction).
>
> The difference between 2nd and 3rd is about the JSON requirement, if JSON
> is mandatory for now then it's 3rd, if it's not mandatory (or accept to
> make -machine/-smp support JSON in the future), 2nd looks cleaner, which
> puts the caches list in -smp.

I'd rather not let syntactic limitations of our CLI dictate the
structure of our configuration data.  Design the structure *first*.
Only then start to think about CLI.  Our CLI is an unholy mess, and
thinking about it too early risks getting lost in the weeds.  I fear
this is what happened to you.

If I forcibly ignore all the considerations related to concrete syntax
in your message, a structure seems to emerge: there's a set of caches
identified by name (l1i, l1d, ...), and for each cache, we have a number
of configurable properties (topology level, ...).  Makes sense?

What else will you need to configure in the future?

By the way, extending -machine to support JSON looks feasible to me at a
glance.

Re: [PATCH 8/8] qemu-options: Add the description of smp-cache object

[Zhao Liu]

Hi Markus,

On Fri, Aug 09, 2024 at 02:24:48PM +0200, Markus Armbruster wrote:
> Date: Fri, 09 Aug 2024 14:24:48 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH 8/8] qemu-options: Add the description of smp-cache
>  object
> 
> I apologize for the delay.

You're welcome! I appreciate your time, guidance and feedback.

> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > On Thu, Aug 01, 2024 at 01:28:27PM +0200, Markus Armbruster wrote:
> 
> [...]
> 
> >> Can you provide a brief summary of the design alternatives that have
> >> been proposed so far?  Because I've lost track.
> >
> > No problem!
> >
> > Currently, we have the following options:
> >
> > * 1st: The first one is just to configure cache topology with several
> >   options in -smp:
> >
> >   -smp l1i-cache-topo=core,l1d-cache-topo-core
> >
> >   This one lacks scalability to support the cache size that ARM will
> >   need in the future.
> 
> -smp sets machine property "smp" of QAPI type SMPConfiguration.
> 
> So this one adds members l1i-cache-topo, l1d-cache-topo, ... to
> SMPConfiguration.

Yes.

> > * 2nd: The cache list object in -smp.
> >
> >   The idea was to use JSON to configure the cache list. However, the
> >   underlying implementation of -smp at the moment is keyval parsing,
> >   which is not compatible with JSON.
> 
> Keyval is a variation of the QEMU's traditional KEY=VALUE,... syntax
> that can serve as an alternative to JSON, with certain restrictions.
> Ideally, we provide both JSON and keyval syntax on the command line.

I see. It's the ideal state of the CLI, and -machine and -smp haven't
arrived here yet.

> Example: -blockdev supports both JSON and keyval.
>     JSON:   -blockdev '{"driver": "null-co", "node-name": "node0"}'
>     keyval: -blockdev null-co,node-name=node0
> 
> Unfortunately, we have many old interfaces that still lack JSON support.
> 
> >   If we can not insist on JSON format, then cache lists can also be
> >   implemented in the following way:
> >   
> >   -smp caches.0.name=l1i,caches.0.topo=core,\
> >        caches.1.name=l1d,caches.1.topo=core
> 
> This one adds a single member caches to SMPConfiguration.  It is an
> array of objects.

Yes.

> > * 3rd: The cache list object linked in -machine.
> >
> >   Considering that -object is JSON-compatible so that defining lists via
> >   JSON is more friendly, I implemented the caches list via -object and
> >   linked it to MachineState:
> >
> >   -object '{"qom-type":"smp-cache","id":"obj","caches":[{"name":"l1d","topo":"core"},{"name":"l1i","topo":"core"}]}'
> >   -machine smp-caches=obj
> 
> This one wraps the same array of objects in a new user-creatable object,
> then sets machine property "smp-caches" to that object.
> 
> We can set machine properties directly with -machine.  But -machine
> doesn't support JSON, yet.
> 
> Wrapping in an object moves the configuration to -object, which does
> support JSON.
> 
> Half way between 2nd and 3rd:
> 
>   * Cache list object in machine
> 
>     -machine caches.0.name=l1i,caches.0.topo=core,\
>              caches.1.name=l1d,caches.1.topo=core

I got your point, and putting the array in -machine does align with the
design of the other machine options nowadays.

> > * 4th: The per cache object without any list:
> >
> >   -object smp-cache,id=cache0,name=l1i,topo=core \
> >   -object smp-cache,id=cache1,name=l1d,topo=core
> >
> >   This proposal is clearer, but there are a few opens:
> >   - I plan to push qom-topo forward, which would abstract CPU related
> >     topology levels and cache to "device" instead of object. Is there a
> >     conflict here?
> 
> Can't say, since I don't understand where you want to go.
> 
> Looks like your trying to design an interface for what you want to do
> now, and are wondering whether it could evolve to accomodate what you
> want to do later.
> 
> It's often better to design the interface for everything you already
> know you want to do, then take out the parts you want to do later.

Thanks! From this point of view, then per cache of objects does not meet
my needs.

> >   - Multiple cache objects can't be linked to the machine on the command
> >     line, so I maintain a static cache list in smp_cache.c and expose
> >     the cache information to the machine through some interface. is this
> >     way acceptable?
> >
> >
> > In summary, the 4th proposal was the most up in the air, as it looked to
> > be conflict with the hybrid topology I wanted to do (and while hybrid
> > topology may not be accepted by the community either, I thought it would
> > be best for the two work to be in the same direction).
> >
> > The difference between 2nd and 3rd is about the JSON requirement, if JSON
> > is mandatory for now then it's 3rd, if it's not mandatory (or accept to
> > make -machine/-smp support JSON in the future), 2nd looks cleaner, which
> > puts the caches list in -smp.
> 
> I'd rather not let syntactic limitations of our CLI dictate the
> structure of our configuration data.  Design the structure *first*.
> Only then start to think about CLI.  Our CLI is an unholy mess, and
> thinking about it too early risks getting lost in the weeds.  I fear
> this is what happened to you.

Indeed, that's my dilemma, lost in the world of CLIs.

> If I forcibly ignore all the considerations related to concrete syntax
> in your message, a structure seems to emerge: there's a set of caches
> identified by name (l1i, l1d, ...), and for each cache, we have a number
> of configurable properties (topology level, ...).  Makes sense?

Yes, you're right!

> What else will you need to configure in the future?

Maybe cache size, as Jonathan mentioned for Arm side.

> By the way, extending -machine to support JSON looks feasible to me at a
> glance.
 
Thanks again! Then I made it clear that it would be most appropriate to
place the cache array in -machine, i.e., it's extensible and consistent
with the design of the rest of the machine's properties, as well as
consistent with my long-term needs.

Later on, if -machine is able to support JSON, it will also benefit from
it. If I have time, I will also think about how -machine can support
JSON.

Regards,
Zhao