Re: [PULL 09/29] qapi/qom: Define cache enumeration and properties for machine

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

Hi Peter,

On Fri, Nov 08, 2024 at 07:10:42PM +0000, Peter Maydell wrote:
> Date: Fri, 8 Nov 2024 19:10:42 +0000
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: Re: [PULL 09/29] qapi/qom: Define cache enumeration and properties
>  for machine
> 
> On Tue, 5 Nov 2024 at 22:49, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >
> > From: Zhao Liu <zhao1.liu@intel.com>
> >
> > The x86 and ARM need to allow user to configure cache properties
> > (current only topology):
> >  * For x86, the default cache topology model (of max/host CPU) does not
> >    always match the Host's real physical cache topology. Performance can
> >    increase when the configured virtual topology is closer to the
> >    physical topology than a default topology would be.
> >  * For ARM, QEMU can't get the cache topology information from the CPU
> >    registers, then user configuration is necessary. Additionally, the
> >    cache information is also needed for MPAM emulation (for TCG) to
> >    build the right PPTT.
> >
> 
> Hi; Coverity points out an issue with this change (CID 1565389):
> 
> > diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> > index 5d8d7edcbd3..c6d90cd6d41 100644
> > --- a/hw/core/machine-smp.c
> > +++ b/hw/core/machine-smp.c
> > @@ -261,6 +261,31 @@ 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);
> 
> DECLARE_BITMAP() defines the caches_bitmap bitmap, but it
> does not initialize it...

Yes, I missed this...

> > +
> > +    for (node = caches; node; node = node->next) {
> > +        /* Prohibit users from repeating settings. */
> > +        if (test_bit(node->value->cache, caches_bitmap)) {
> 
> ...so here we are reading the variable when it is uninitialized.
> 
> If you want to zero-initialize the bitmap you can use
>    bitmap_zero(caches_bitmap, CACHE_LEVEL_AND_TYPE__MAX);

Thank you for your advice! I'll submit a patch to fix this!

Regards,
Zhao