Re: [PATCH v2 4/7] hw/core: Check smp cache topology support for machine

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

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@Huawei.com>
> 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@intel.com> 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@intel.com>
> > Tested-by: Yongwei Ma <yongwei.ma@intel.com>
> 
> Just a few trivial comments inline.
> 
> FWIW with or without those changes.
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

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