[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC 4/8] hw/core: Add cache topology options in -smp
From: |
Daniel P . Berrangé |
Subject: |
Re: [RFC 4/8] hw/core: Add cache topology options in -smp |
Date: |
Tue, 27 Feb 2024 09:12:20 +0000 |
User-agent: |
Mutt/2.2.12 (2023-09-09) |
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 :|
- [RFC 1/8] hw/core: Rename CpuTopology to CPUTopology, (continued)
RE: [RFC 4/8] hw/core: Add cache topology options in -smp, JeeHeng Sia, 2024/02/28
[RFC 5/8] i386/cpu: Support thread and module level cache topology, Zhao Liu, 2024/02/20
[RFC 6/8] i386/cpu: Update cache topology with machine's configuration, Zhao Liu, 2024/02/20
[RFC 7/8] i386/pc: Support cache topology in -smp for PC machine, Zhao Liu, 2024/02/20