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

[Zhao Liu]

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.

Thanks,
Zhao