diff mbox series

[v2,3/7] hw/core: Add smp cache topology for machine

Message ID 20240908125920.1160236-4-zhao1.liu@intel.com (mailing list archive)
State New, archived
Headers show
Series Introduce SMP Cache Topology | expand

Commit Message

Zhao Liu Sept. 8, 2024, 12:59 p.m. UTC 
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>
Tested-by: Yongwei Ma <yongwei.ma@intel.com>
---
Changes since Patch v1:
 * Integrated cache properties list into MachineState and used -machine
   to configure SMP cache properties. (Markus)

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 | 41 ++++++++++++++++++++++++++++++++++++++++
 hw/core/machine.c     | 44 +++++++++++++++++++++++++++++++++++++++++++
 include/hw/boards.h   | 10 ++++++++++
 3 files changed, 95 insertions(+)

Comments

Zhao Liu Oct. 7, 2024, 11:02 a.m. UTC | #1 
On Tue, Sep 17, 2024 at 10:00:48AM +0100, Jonathan Cameron wrote:
> Date: Tue, 17 Sep 2024 10:00:48 +0100
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Subject: Re: [PATCH v2 3/7] hw/core: Add smp cache topology for machine
> X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32)
> 
> On Sun,  8 Sep 2024 20:59:16 +0800
> Zhao Liu <zhao1.liu@intel.com> wrote:
> 
> > 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>
> > Tested-by: Yongwei Ma <yongwei.ma@intel.com>
>
> Minor stuff. The property stuff is something I seems to mostly get wrong
> so needs more eyes but fwiw looks fine to me.

Yes and thank you!

> With the tweaks suggested below.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> > ---
> > Changes since Patch v1:
> >  * Integrated cache properties list into MachineState and used -machine
> >    to configure SMP cache properties. (Markus)
> > 
> > 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 | 41 ++++++++++++++++++++++++++++++++++++++++
> >  hw/core/machine.c     | 44 +++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/boards.h   | 10 ++++++++++
> >  3 files changed, 95 insertions(+)
> > 
> > diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> > index 5d8d7edcbd3f..b517c3471d1a 100644
> > --- a/hw/core/machine-smp.c
> > +++ b/hw/core/machine-smp.c
> > @@ -261,6 +261,41 @@ void machine_parse_smp_config(MachineState *ms,
> >      }
> >  }
> >  
> > +bool machine_parse_smp_cache(MachineState *ms,
> > +                             const SmpCachePropertiesList *caches,
> > +                             Error **errp)
> > +{
> > +    const SmpCachePropertiesList *node;
> > +    DECLARE_BITMAP(caches_bitmap, CACHE_LEVEL_AND_TYPE__MAX);
> > +
> > +    for (node = caches; node; node = node->next) {
> > +        /* Prohibit users from setting the cache topology level to invalid. */
> > +        if (node->value->topology == CPU_TOPOLOGY_LEVEL_INVALID) {
> > +            error_setg(errp,
> > +                       "Invalid cache topology level: %s. "
> > +                       "The topology should match the "
> > +                       "valid CPU topology level",
> 
> I think that's too much wrapping for an error message. Makes them hard
> to grep for.

I understand you mean the last sentence should not be on separate lines
but should be continuous in one line, right?

> > +                       CpuTopologyLevel_str(node->value->topology));
> > +            return false;
> > +        }
> > +
> > +        /* Prohibit users from repeating settings. */
> > +        if (test_bit(node->value->cache, caches_bitmap)) {
> > +            error_setg(errp,
> > +                       "Invalid cache properties: %s. "
> > +                       "The cache properties are duplicated",
> > +                       CacheLevelAndType_str(node->value->cache));
> > +            return false;
> > +        } else {
> 
> returned anyway in the above path, so can drop the else and reduce
> indent a little.

Sure.

Thanks,
Zhao

> > +            ms->smp_cache.props[node->value->cache].topology =
> > +                node->value->topology;
> > +            set_bit(node->value->cache, caches_bitmap);
> > +        }
> > +    }
> > +
> > +    return true;
> > +}
> > +
> 
>
diff mbox series

Patch

diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 5d8d7edcbd3f..b517c3471d1a 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -261,6 +261,41 @@  void machine_parse_smp_config(MachineState *ms,
     }
 }
 
+bool machine_parse_smp_cache(MachineState *ms,
+                             const SmpCachePropertiesList *caches,
+                             Error **errp)
+{
+    const SmpCachePropertiesList *node;
+    DECLARE_BITMAP(caches_bitmap, CACHE_LEVEL_AND_TYPE__MAX);
+
+    for (node = caches; node; node = node->next) {
+        /* Prohibit users from setting the cache topology level to invalid. */
+        if (node->value->topology == CPU_TOPOLOGY_LEVEL_INVALID) {
+            error_setg(errp,
+                       "Invalid cache topology level: %s. "
+                       "The topology should match the "
+                       "valid CPU topology level",
+                       CpuTopologyLevel_str(node->value->topology));
+            return false;
+        }
+
+        /* Prohibit users from repeating settings. */
+        if (test_bit(node->value->cache, caches_bitmap)) {
+            error_setg(errp,
+                       "Invalid cache properties: %s. "
+                       "The cache properties are duplicated",
+                       CacheLevelAndType_str(node->value->cache));
+            return false;
+        } else {
+            ms->smp_cache.props[node->value->cache].topology =
+                node->value->topology;
+            set_bit(node->value->cache, caches_bitmap);
+        }
+    }
+
+    return true;
+}
+
 unsigned int machine_topo_get_cores_per_socket(const MachineState *ms)
 {
     return ms->smp.cores * ms->smp.modules * ms->smp.clusters * ms->smp.dies;
@@ -270,3 +305,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,
+                                              CacheLevelAndType cache)
+{
+    return ms->smp_cache.props[cache].topology;
+}
diff --git a/hw/core/machine.c b/hw/core/machine.c
index adaba17ebac1..518beb9f883a 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -932,6 +932,40 @@  static void machine_set_smp(Object *obj, Visitor *v, const char *name,
     machine_parse_smp_config(ms, config, errp);
 }
 
+static void machine_get_smp_cache(Object *obj, Visitor *v, const char *name,
+                                  void *opaque, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+    SmpCache *cache = &ms->smp_cache;
+    SmpCachePropertiesList *head = NULL;
+    SmpCachePropertiesList **tail = &head;
+
+    for (int i = 0; i < CACHE_LEVEL_AND_TYPE__MAX; i++) {
+        SmpCacheProperties *node = g_new(SmpCacheProperties, 1);
+
+        node->cache = cache->props[i].cache;
+        node->topology = cache->props[i].topology;
+        QAPI_LIST_APPEND(tail, node);
+    }
+
+    visit_type_SmpCachePropertiesList(v, name, &head, errp);
+    qapi_free_SmpCachePropertiesList(head);
+}
+
+static void machine_set_smp_cache(Object *obj, Visitor *v, const char *name,
+                                  void *opaque, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+    SmpCachePropertiesList *caches;
+
+    if (!visit_type_SmpCachePropertiesList(v, name, &caches, errp)) {
+        return;
+    }
+
+    machine_parse_smp_cache(ms, caches, errp);
+    qapi_free_SmpCachePropertiesList(caches);
+}
+
 static void machine_get_boot(Object *obj, Visitor *v, const char *name,
                             void *opaque, Error **errp)
 {
@@ -1057,6 +1091,11 @@  static void machine_class_init(ObjectClass *oc, void *data)
     object_class_property_set_description(oc, "smp",
         "CPU topology");
 
+    object_class_property_add(oc, "smp-cache", "SmpCachePropertiesWrapper",
+        machine_get_smp_cache, machine_set_smp_cache, NULL, NULL);
+    object_class_property_set_description(oc, "smp-cache",
+        "Cache properties list for SMP machine");
+
     object_class_property_add(oc, "phandle-start", "int",
         machine_get_phandle_start, machine_set_phandle_start,
         NULL, NULL);
@@ -1195,6 +1234,11 @@  static void machine_initfn(Object *obj)
     ms->smp.cores = 1;
     ms->smp.threads = 1;
 
+    for (int i = 0; i < CACHE_LEVEL_AND_TYPE__MAX; i++) {
+        ms->smp_cache.props[i].cache = (CacheLevelAndType)i;
+        ms->smp_cache.props[i].topology = CPU_TOPOLOGY_LEVEL_DEFAULT;
+    }
+
     machine_copy_boot_config(ms, &(BootConfiguration){ 0 });
 }
 
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 9a492770cbb9..64439dc7da2c 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -43,8 +43,13 @@  void machine_set_cpu_numa_node(MachineState *machine,
                                Error **errp);
 void machine_parse_smp_config(MachineState *ms,
                               const SMPConfiguration *config, Error **errp);
+bool machine_parse_smp_cache(MachineState *ms,
+                             const SmpCachePropertiesList *caches,
+                             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,
+                                              CacheLevelAndType cache);
 void machine_memory_devices_init(MachineState *ms, hwaddr base, uint64_t size);
 
 /**
@@ -363,6 +368,10 @@  typedef struct CpuTopology {
     unsigned int max_cpus;
 } CpuTopology;
 
+typedef struct SmpCache {
+    SmpCacheProperties props[CACHE_LEVEL_AND_TYPE__MAX];
+} SmpCache;
+
 /**
  * MachineState:
  */
@@ -413,6 +422,7 @@  struct MachineState {
     AccelState *accelerator;
     CPUArchIdList *possible_cpus;
     CpuTopology smp;
+    SmpCache smp_cache;
     struct NVDIMMState *nvdimms_state;
     struct NumaState *numa_state;
 };