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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2828 bytes --]

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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]