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

[Zhao Liu <zhao1.liu@intel.com>]

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