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@xxxxxxxxxx>
> 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@xxxxxxxxx> 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@xxxxxxxxx>
> > Tested-by: Yongwei Ma <yongwei.ma@xxxxxxxxx>
>
> 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@xxxxxxxxxx>
>
> > ---
> > 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;
> > +}
> > +
>
>
