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

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

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;
> > +}
> > +
> 
>