On Tue, 27 Feb 2024 17:20:25 +0800
Zhao Liu <zhao1.liu@xxxxxxxxxxxxxxx> 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
