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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 7 Oct 2024 19:12:29 +0800
Zhao Liu <zhao1.liu@intel.com> wrote:

> 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)
yes - that's what I had in mind.

> 
> > 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
> 
>