Message ID | 20210827092455.125411-3-danielhb413@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | pSeries FORM2 affinity support | expand |
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. > + > + 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,
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, >
On Mon, Sep 06, 2021 at 09:50:36PM -0300, Daniel Henrique Barboza wrote: > > > 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. Ok. I should note that I'm not saying what you have now is definitely wrong, it just looks a bit odd to me. If you have a rationale for doing it this way, go ahead and tell me, rather than changing it.
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; + } + + 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,