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!
