On Tue, Sep 17, 2024 at 09:56:12AM +0100, Jonathan Cameron wrote:
> Date: Tue, 17 Sep 2024 09:56:12 +0100
> From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> Subject: Re: [PATCH v2 4/7] hw/core: Check smp cache topology support for
> machine
> X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32)
>
> On Sun, 8 Sep 2024 20:59:17 +0800
> Zhao Liu <zhao1.liu@xxxxxxxxx> wrote:
>
> > 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@xxxxxxxxx>
> > Tested-by: Yongwei Ma <yongwei.ma@xxxxxxxxx>
>
> Just a few trivial comments inline.
>
> FWIW with or without those changes.
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
Thanks!
[snip]
> > + /*
> > + * Allow setting "default" topology level even though the cache
> > + * isn't supported by machine.
> I'd flip the comment as the condition is doing the opposite.
OK, it's more intuitive.
> * 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;
> > + }
> > + }
> > +
> > + if (smp_cache_topo_cmp(&ms->smp_cache,
> > + CACHE_LEVEL_AND_TYPE_L1D,
>
> Short line wrap. Maybe combine the two lines above and similar
> cases.
Like this?
smp_cache_topo_cmp(&ms->smp_cache, CACHE_LEVEL_AND_TYPE_L1D,
CACHE_LEVEL_AND_TYPE_L2)
> Up to you though, I don't feel that strongly.
>
> > + CACHE_LEVEL_AND_TYPE_L2) ||
> > + smp_cache_topo_cmp(&ms->smp_cache,
> > + CACHE_LEVEL_AND_TYPE_L1I,
> > + CACHE_LEVEL_AND_TYPE_L2)) {
> > + error_setg(errp,
> > + "Invalid smp cache topology. "
> > + "L2 cache topology level shouldn't be lower than L1 cache");
> > + return false;
> > + }
> > +
Regards,
Zhao
