diff mbox series

[PULL,10/29] hw/core: Check smp cache topology support for machine

Message ID 20241105224727.53059-11-philmd@linaro.org
State New
Headers show
Series [PULL,01/29] target/microblaze: Rename CPU endianness property as 'little-endian' | expand

Commit Message

Philippe Mathieu-Daudé Nov. 5, 2024, 10:47 p.m. UTC 
From: Zhao Liu <zhao1.liu@intel.com>

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 <zhao1.liu@intel.com>
Tested-by: Yongwei Ma <yongwei.ma@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Message-ID: <20241101083331.340178-5-zhao1.liu@intel.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/boards.h   |  3 +++
 hw/core/machine-smp.c | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

Comments

Peter Maydell Nov. 8, 2024, 7:16 p.m. UTC | #1 
On Tue, 5 Nov 2024 at 22:49, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> From: Zhao Liu <zhao1.liu@intel.com>
>
> 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().

Hi; Coverity points out an issue in this code (CID 1565391):

>  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);
>
> @@ -283,6 +305,25 @@ bool machine_parse_smp_cache(MachineState *ms,
>          set_bit(node->value->cache, caches_bitmap);
>      }
>
> +    for (int i = 0; i < CACHE_LEVEL_AND_TYPE__MAX; i++) {
> +        const SmpCacheProperties *props = &ms->smp_cache.props[i];
> +
> +        /*
> +         * 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));

This error message looks at "node", but "node" was the iteration
variable in the first loop in this function, so generally at
this point it will be NULL because that is the loop termination
condition.

The code looks like it ought to be reporting an error relating
to the 'props' variable, not 'node' ?

> +            return false;
> +        }
> +
> +        if (!machine_check_topo_support(ms, props->topology, errp)) {
> +            return false;
> +        }
> +    }
>      return true;
>  }

thanks
-- PMM
Zhao Liu Nov. 10, 2024, 2:12 p.m. UTC | #2 
Hi Peter,

On Fri, Nov 08, 2024 at 07:16:50PM +0000, Peter Maydell wrote:
> Date: Fri, 8 Nov 2024 19:16:50 +0000
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: Re: [PULL 10/29] hw/core: Check smp cache topology support for
>  machine
> 
> On Tue, 5 Nov 2024 at 22:49, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >
> > From: Zhao Liu <zhao1.liu@intel.com>
> >
> > 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().
> 
> Hi; Coverity points out an issue in this code (CID 1565391):
> 
> >  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);
> >
> > @@ -283,6 +305,25 @@ bool machine_parse_smp_cache(MachineState *ms,
> >          set_bit(node->value->cache, caches_bitmap);
> >      }
> >
> > +    for (int i = 0; i < CACHE_LEVEL_AND_TYPE__MAX; i++) {
> > +        const SmpCacheProperties *props = &ms->smp_cache.props[i];
> > +
> > +        /*
> > +         * 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));
> 
> This error message looks at "node", but "node" was the iteration
> variable in the first loop in this function, so generally at
> this point it will be NULL because that is the loop termination
> condition.
> 
> The code looks like it ought to be reporting an error relating
> to the 'props' variable, not 'node' ?

Thank you! My fault and you're right, here I should use "props->cache"
instead of "node->value->cache". I'll submit a patch to fix this.

Regards,
Zhao
diff mbox series

Patch

diff --git a/include/hw/boards.h b/include/hw/boards.h
index b3a8064ccc9..edf1a8ca1c4 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -153,6 +153,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 (l1d, l1i, l2 and l3) configuration are
+ *                    supported by the machine
  */
 typedef struct {
     bool prefer_sockets;
@@ -162,6 +164,7 @@  typedef struct {
     bool books_supported;
     bool drawers_supported;
     bool modules_supported;
+    bool cache_supported[CACHE_LEVEL_AND_TYPE__MAX];
 } SMPCompatProps;
 
 /**
diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index c6d90cd6d41..ebb7a134a7b 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -261,10 +261,32 @@  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;
+}
+
 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);
 
@@ -283,6 +305,25 @@  bool machine_parse_smp_cache(MachineState *ms,
         set_bit(node->value->cache, caches_bitmap);
     }
 
+    for (int i = 0; i < CACHE_LEVEL_AND_TYPE__MAX; i++) {
+        const SmpCacheProperties *props = &ms->smp_cache.props[i];
+
+        /*
+         * 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;
+        }
+    }
     return true;
 }