On Tue, Feb 27, 2024 at 05:20:25PM +0800, Zhao Liu 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()"?
>
> >
> > > + char *topo_str,
> > > + CPUTopoLevel *topo,
> > > + Error **errp)
> > > +{
> > > + *topo = string_to_cpu_topo(topo_str);
> > > +
> > > + if (*topo == CPU_TOPO_LEVEL_MAX || *topo == CPU_TOPO_LEVEL_INVALID) {
> > > + error_setg(errp, "Invalid cache topology level: %s. The cache "
> > > + "topology should match the CPU topology level",
> > > topo_str);
> > > + return -1;
> > > + }
> > > +
> > > + if (!machine_check_topo_support(ms, *topo)) {
> > > + error_setg(errp, "Invalid cache topology level: %s. The topology
> > > "
> > > + "level is not supported by this machine", topo_str);
> > > + return -1;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void machine_parse_smp_cache_config(MachineState *ms,
> > > + const SMPConfiguration
> > > *config,
> > > + Error **errp)
> > > +{
> > > + MachineClass *mc = MACHINE_GET_CLASS(ms);
> > > +
> > > + if (config->l1d_cache) {
> > > + if (!mc->smp_props.l1_separated_cache_supported) {
> > > + error_setg(errp, "L1 D-cache topology not "
> > > + "supported by this machine");
> > > + return;
> > > + }
> > > +
> > > + if (smp_cache_string_to_topology(ms, config->l1d_cache,
> > > + &ms->smp_cache.l1d, errp)) {
> >
> > Indent is to wrong opening bracket.
> > Same for other cases.
>
> Could you please educate me about the correct style here?
> I'm unsure if it should be indented by 4 spaces.
It needs to look like this:
if (smp_cache_string_to_topology(ms, config->l1d_cache,
&ms->smp_cache.l1d, errp)) {
so func parameters are aligned to the function calls' opening bracket,
not the 'if' statement's opening bracket.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
