On Tue, Feb 27, 2024 at 10:51:45AM +0000, Jonathan Cameron wrote: > Date: Tue, 27 Feb 2024 10:51:45 +0000 > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > Subject: Re: [RFC 4/8] hw/core: Add cache topology options in -smp > X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32) > > 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. > Good idea! This makes the code clearer. Let me try this way. Thanks!