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

["Daniel P. Berrangé" <berrange@redhat.com>]

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 :|