Re: [RFC 4/8] hw/core: Add cache topology options in -smp

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Tue, 27 Feb 2024 17:20:25 +0800
Zhao Liu <zhao1.liu@linux.intel.com> wrote:

> Hi Jonathan,
> 
> > Hi Zhao Liu
> > 
> > I like the scheme.  Strikes a good balance between complexity of description
> > and systems that actually exist. Sure there are systems with more cache
> > levels etc but they are rare and support can be easily added later
> > if people want to model them.  
> 
> Thanks for your support!
> 
> [snip]
> 
> > > +static int smp_cache_string_to_topology(MachineState *ms,  
> > 
> > Not a good name for a function that does rather more than that.  
> 
> What about "smp_cache_get_valid_topology()"?

Looking again, could we return the CPUTopoLevel? I think returning
CPU_TOPO_LEVEL_INVALID will replace -1/0 returns and this can just
be smp_cache_string_to_topology() as you have it in this version.

The check on the return value becomes a little more more complex
and I think you want to squash CPU_TOPO_LEVEL_MAX down so we only
have one invalid value to check at callers..  E.g.

static CPUTopoLevel smp_cache_string_to_topolgy(MachineState *ms,
                                                char *top_str,
                                                Error **errp)
{
    CPUTopoLevel topo = string_to_cpu_topo(topo_str);

    if (topo == CPU_TOPO_LEVEL_MAX || topo == CPU_TOP?O_LEVEL_INVALID) {
        return CPU_TOPO_LEVEL_INVALID;
    }

    if (!machine_check_topo_support(ms, topo) {
        error_setg(errp,
                   "Invalid cache topology level: %s. "
                   "The cache topology should match the CPU topology level",
//Break string like this to make it as grep-able as possible!
                   topo_str);
        return CPU_TOPO_LEVEL_INVALID;
    }

    return topo;

}                


The checks then become != CPU_TOPO_LEVEL_INVALID at each callsite.

Jonathan