[v2,4/7] hw/core: Check smp cache topology support for machine

Commit Message

Zhao Liu Sept. 8, 2024, 12:59 p.m. UTC

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

And check the compatibility of the cache properties with the
machine support in machine_parse_smp_cache().

Signed-off-by: Zhao Liu <[e-mail]>
Tested-by: Yongwei Ma <[e-mail]>
---
Changes since Patch v1:
 * Dropped machine_check_smp_cache_support() and did the check when
   -machine parses smp-cache in machine_parse_smp_cache().

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 | 78 +++++++++++++++++++++++++++++++++++++++++++
 include/hw/boards.h   |  3 ++
 2 files changed, 81 insertions(+)

Comments

Zhao Liu Oct. 7, 2024, 11:12 a.m. UTC | #1

On Tue, Sep 17, 2024 at 09:56:12AM +0100, Jonathan Cameron wrote:
> Date: Tue, 17 Sep 2024 09:56:12 +0100
> From: Jonathan Cameron <[e-mail]>
> Subject: Re: [PATCH v2 4/7] hw/core: Check smp cache topology support for
>  machine
> X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32)
> 
> On Sun,  8 Sep 2024 20:59:17 +0800
> Zhao Liu <[e-mail]> wrote:
> 
> > Add cache_supported flags in SMPCompatProps to allow machines to
> > configure various caches support.
> > 
> > And check the compatibility of the cache properties with the
> > machine support in machine_parse_smp_cache().
> > 
> > Signed-off-by: Zhao Liu <[e-mail]>
> > Tested-by: Yongwei Ma <[e-mail]>
> 
> Just a few trivial comments inline.
> 
> FWIW with or without those changes.
> Reviewed-by: Jonathan Cameron <[e-mail]>

Thanks!

[snip]

> > +        /*
> > +         * Allow setting "default" topology level even though the cache
> > +         * isn't supported by machine.
> I'd flip the comment as the condition is doing the opposite.

OK, it's more intuitive.

>             * Reject non "default" topology level if the cache isn't
>             * supported by the machine.
> > +         */
> > +        if (props->topology != CPU_TOPOLOGY_LEVEL_DEFAULT &&
> > +            !mc->smp_props.cache_supported[props->cache]) {
> > +            error_setg(errp,
> > +                       "%s cache topology not supported by this machine",
> > +                       CacheLevelAndType_str(node->value->cache));
> > +            return false;
> > +        }
> > +
> > +        if (!machine_check_topo_support(ms, props->topology, errp)) {
> > +            return false;
> > +        }
> > +    }
> > +
> > +    if (smp_cache_topo_cmp(&ms->smp_cache,
> > +                           CACHE_LEVEL_AND_TYPE_L1D,
> 
> Short line wrap.  Maybe combine the two lines above and similar
> cases.

Like this?

smp_cache_topo_cmp(&ms->smp_cache, CACHE_LEVEL_AND_TYPE_L1D,
                   CACHE_LEVEL_AND_TYPE_L2)

> Up to you though, I don't feel that strongly.
> 
> > +                           CACHE_LEVEL_AND_TYPE_L2) ||
> > +        smp_cache_topo_cmp(&ms->smp_cache,
> > +                           CACHE_LEVEL_AND_TYPE_L1I,
> > +                           CACHE_LEVEL_AND_TYPE_L2)) {
> > +        error_setg(errp,
> > +                   "Invalid smp cache topology. "
> > +                   "L2 cache topology level shouldn't be lower than L1 cache");
> > +        return false;
> > +    }
> > +

Regards,
Zhao

Patch

diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index b517c3471d1a..9a281946762f 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -261,10 +261,47 @@  void machine_parse_smp_config(MachineState *ms,
     }
 }
 
+static bool machine_check_topo_support(MachineState *ms,
+                                       CpuTopologyLevel topo,
+                                       Error **errp)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+
+    if ((topo == CPU_TOPOLOGY_LEVEL_MODULE && !mc->smp_props.modules_supported) ||
+        (topo == CPU_TOPOLOGY_LEVEL_CLUSTER && !mc->smp_props.clusters_supported) ||
+        (topo == CPU_TOPOLOGY_LEVEL_DIE && !mc->smp_props.dies_supported) ||
+        (topo == CPU_TOPOLOGY_LEVEL_BOOK && !mc->smp_props.books_supported) ||
+        (topo == CPU_TOPOLOGY_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,
+                               CacheLevelAndType cache1,
+                               CacheLevelAndType cache2)
+{
+    if (smp_cache->props[cache1].topology != CPU_TOPOLOGY_LEVEL_DEFAULT &&
+        smp_cache->props[cache1].topology > smp_cache->props[cache2].topology) {
+        return true;
+    }
+    return false;
+}
+
 bool machine_parse_smp_cache(MachineState *ms,
                              const SmpCachePropertiesList *caches,
                              Error **errp)
 {
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
     const SmpCachePropertiesList *node;
     DECLARE_BITMAP(caches_bitmap, CACHE_LEVEL_AND_TYPE__MAX);
 
@@ -293,6 +330,47 @@  bool machine_parse_smp_cache(MachineState *ms,
         }
     }
 
+    for (int i = 0; i < CACHE_LEVEL_AND_TYPE__MAX; i++) {
+        const SmpCacheProperties *props = &ms->smp_cache.props[i];
+
+        /*
+         * Allow setting "default" topology level even though the cache
+         * isn't supported by machine.
+         */
+        if (props->topology != CPU_TOPOLOGY_LEVEL_DEFAULT &&
+            !mc->smp_props.cache_supported[props->cache]) {
+            error_setg(errp,
+                       "%s cache topology not supported by this machine",
+                       CacheLevelAndType_str(node->value->cache));
+            return false;
+        }
+
+        if (!machine_check_topo_support(ms, props->topology, errp)) {
+            return false;
+        }
+    }
+
+    if (smp_cache_topo_cmp(&ms->smp_cache,
+                           CACHE_LEVEL_AND_TYPE_L1D,
+                           CACHE_LEVEL_AND_TYPE_L2) ||
+        smp_cache_topo_cmp(&ms->smp_cache,
+                           CACHE_LEVEL_AND_TYPE_L1I,
+                           CACHE_LEVEL_AND_TYPE_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(&ms->smp_cache,
+                           CACHE_LEVEL_AND_TYPE_L2,
+                           CACHE_LEVEL_AND_TYPE_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/include/hw/boards.h b/include/hw/boards.h
index 64439dc7da2c..6c3cdfa15f50 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -150,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;
@@ -159,6 +161,7 @@  typedef struct {
     bool books_supported;
     bool drawers_supported;
     bool modules_supported;
+    bool cache_supported[CACHE_LEVEL_AND_TYPE__MAX];
 } SMPCompatProps;
 
 /**