Re: [PATCH v4 2/5] spapr_numa.c: split FORM1 code into helpers

[Daniel Henrique Barboza]

On 9/6/21 9:30 PM, David Gibson wrote:
> On Fri, Aug 27, 2021 at 06:24:52AM -0300, Daniel Henrique Barboza wrote:
> > The upcoming FORM2 NUMA affinity will support asymmetric NUMA topologies
> > and doesn't need be concerned with all the legacy support for older
> > pseries FORM1 guests.
> >
> > We're also not going to calculate associativity domains based on numa
> > distance (via spapr_numa_define_associativity_domains) since the
> > distances will be written directly into new DT properties.
> >
> > Let's split FORM1 code into its own functions to allow for easier
> > insertion of FORM2 logic later on.
> >
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > ---
> >   hw/ppc/spapr_numa.c | 61 +++++++++++++++++++++++++++++----------------
> >   1 file changed, 39 insertions(+), 22 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
> > index 779f18b994..04a86f9b5b 100644
> > --- a/hw/ppc/spapr_numa.c
> > +++ b/hw/ppc/spapr_numa.c
> > @@ -155,6 +155,32 @@ static void 
> > spapr_numa_define_associativity_domains(SpaprMachineState *spapr)
> >   
> >   }
> >   
> > +/*
> > + * Set NUMA machine state data based on FORM1 affinity semantics.
> > + */
> > +static void spapr_numa_FORM1_affinity_init(SpaprMachineState *spapr,
> > +                                           MachineState *machine)
> > +{
> > +    bool using_legacy_numa = spapr_machine_using_legacy_numa(spapr);
> > +
> > +    /*
> > +     * Legacy NUMA guests (pseries-5.1 and older, or guests with only
> > +     * 1 NUMA node) will not benefit from anything we're going to do
> > +     * after this point.
> > +     */
> > +    if (using_legacy_numa) {
> > +        return;
> > +    }
>
> My only concern with this patch is that handling the
> "using_legacy_numa" case might logically belong outside the FORM1
> code.  I mean, I'm pretty sure using_legacy_numa implies FORM1 in
> practice, but conceptually it seems like a more fundamental question
> than the DT encoding of the NUMA information.

I'll carry on this suggestion for the next spin, v6, given that the v5 I sent
a few minutes ago is also verifying legacy numa in FORM1 code.


Thanks,


Daniel

> > +
> > +    if (!spapr_numa_is_symmetrical(machine)) {
> > +        error_report("Asymmetrical NUMA topologies aren't supported "
> > +                     "in the pSeries machine");
> > +        exit(EXIT_FAILURE);
> > +    }
> > +
> > +    spapr_numa_define_associativity_domains(spapr);
> > +}
> > +
> >   void spapr_numa_associativity_init(SpaprMachineState *spapr,
> >                                      MachineState *machine)
> >   {
> > @@ -210,22 +236,7 @@ void spapr_numa_associativity_init(SpaprMachineState 
> > *spapr,
> >           spapr->numa_assoc_array[i][MAX_DISTANCE_REF_POINTS] = cpu_to_be32(i);
> >       }
> >   
> > -    /*
> > -     * Legacy NUMA guests (pseries-5.1 and older, or guests with only
> > -     * 1 NUMA node) will not benefit from anything we're going to do
> > -     * after this point.
> > -     */
> > -    if (using_legacy_numa) {
> > -        return;
> > -    }
> > -
> > -    if (!spapr_numa_is_symmetrical(machine)) {
> > -        error_report("Asymmetrical NUMA topologies aren't supported "
> > -                     "in the pSeries machine");
> > -        exit(EXIT_FAILURE);
> > -    }
> > -
> > -    spapr_numa_define_associativity_domains(spapr);
> > +    spapr_numa_FORM1_affinity_init(spapr, machine);
> >   }
> >   
> >   void spapr_numa_write_associativity_dt(SpaprMachineState *spapr, void *fdt,
> > @@ -302,12 +313,8 @@ int spapr_numa_write_assoc_lookup_arrays(SpaprMachineState 
> > *spapr, void *fdt,
> >       return ret;
> >   }
> >   
> > -/*
> > - * Helper that writes ibm,associativity-reference-points and
> > - * max-associativity-domains in the RTAS pointed by @rtas
> > - * in the DT @fdt.
> > - */
> > -void spapr_numa_write_rtas_dt(SpaprMachineState *spapr, void *fdt, int rtas)
> > +static void spapr_numa_FORM1_write_rtas_dt(SpaprMachineState *spapr,
> > +                                           void *fdt, int rtas)
> >   {
> >       MachineState *ms = MACHINE(spapr);
> >       SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> > @@ -365,6 +372,16 @@ void spapr_numa_write_rtas_dt(SpaprMachineState *spapr, 
> > void *fdt, int rtas)
> >                        maxdomains, sizeof(maxdomains)));
> >   }
> >   
> > +/*
> > + * Helper that writes ibm,associativity-reference-points and
> > + * max-associativity-domains in the RTAS pointed by @rtas
> > + * in the DT @fdt.
> > + */
> > +void spapr_numa_write_rtas_dt(SpaprMachineState *spapr, void *fdt, int rtas)
> > +{
> > +    spapr_numa_FORM1_write_rtas_dt(spapr, fdt, rtas);
> > +}
> > +
> >   static target_ulong h_home_node_associativity(PowerPCCPU *cpu,
> >                                                 SpaprMachineState *spapr,
> >                                                 target_ulong opcode,