Re: [PATCH v7 17/17] spapr: Fold spapr_node0_size() into its only caller

From: David Gibson <david@gibson.dropbear.id.au>
To: Greg Kurz <groug@kaod.org>
Cc: lvivier@redhat.com, Thomas Huth <thuth@redhat.com>,
	Xiao Guangrong <xiaoguangrong.eric@gmail.com>,
	farosas@linux.ibm.com, aik@ozlabs.ru,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>,
	qemu-devel@nongnu.org, Paolo Bonzini <pbonzini@redhat.com>,
	qemu-ppc@nongnu.org, clg@kaod.org,
	Igor Mammedov <imammedo@redhat.com>,
	"Edgar E. Iglesias" <edgar.iglesias@gmail.com>,
	paulus@samba.org
Subject: Re: [PATCH v7 17/17] spapr: Fold spapr_node0_size() into its only caller
Date: Wed, 4 Mar 2020 12:25:55 +1100	[thread overview]
Message-ID: <20200304012555.GH35885@umbus.fritz.box> (raw)
In-Reply-To: <20200303113012.26a79a30@bahia.home>

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

On Tue, Mar 03, 2020 at 11:32:49AM +0100, Greg Kurz wrote:
> On Tue,  3 Mar 2020 14:43:51 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > The Real Mode Area (RMA) needs to fit within the NUMA node owning memory
> > at address 0.  That's usually node 0, but can be a later one if there are
> > some nodes which have no memory (only CPUs).
> > 
> > This is currently handled by the spapr_node0_size() helper.  It has only
> > one caller, so there's not a lot of point splitting it out.  It's also
> > extremely easy to misread the code as clamping to the size of the smallest
> > node rather than the first node with any memory.
> > 
> > So, fold it into the caller, and add some commentary to make it a bit
> > clearer exactly what it's doing.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c | 37 +++++++++++++++++++++----------------
> >  1 file changed, 21 insertions(+), 16 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 2eb0d8f70d..d674a9f48f 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -296,20 +296,6 @@ static void spapr_populate_pa_features(SpaprMachineState *spapr,
> >      _FDT((fdt_setprop(fdt, offset, "ibm,pa-features", pa_features, pa_size)));
> >  }
> >  
> > -static hwaddr spapr_node0_size(MachineState *machine)
> > -{
> > -    if (machine->numa_state->num_nodes) {
> > -        int i;
> > -        for (i = 0; i < machine->numa_state->num_nodes; ++i) {
> > -            if (machine->numa_state->nodes[i].node_mem) {
> > -                return MIN(pow2floor(machine->numa_state->nodes[i].node_mem),
> > -                           machine->ram_size);
> > -            }
> > -        }
> > -    }
> > -    return machine->ram_size;
> > -}
> > -
> >  static void add_str(GString *s, const gchar *s1)
> >  {
> >      g_string_append_len(s, s1, strlen(s1) + 1);
> > @@ -2653,10 +2639,24 @@ static hwaddr spapr_rma_size(SpaprMachineState *spapr, Error **errp)
> >      MachineState *machine = MACHINE(spapr);
> >      SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> >      hwaddr rma_size = machine->ram_size;
> > -    hwaddr node0_size = spapr_node0_size(machine);
> >  
> >      /* RMA has to fit in the first NUMA node */
> > -    rma_size = MIN(rma_size, node0_size);
> > +    if (machine->numa_state->num_nodes) {
> > +        /*
> > +         * It's possible for there to be some zero-memory nodes first
> > +         * in the list.  We need the RMA to fit inside the memory of
> > +         * the first node which actually has some memory.
> > +         */
> > +        int i;
> > +
> > +        for (i = 0; i < machine->numa_state->num_nodes; ++i) {
> > +            if (machine->numa_state->nodes[i].node_mem != 0) {
> > +                rma_size = MIN(rma_size,
> > +                               machine->numa_state->nodes[i].node_mem);
> > +                break;
> > +            }
> > +        }
> > +    }
> >  
> >      /*
> >       * VRMA access is via a special 1TiB SLB mapping, so the RMA can
> > @@ -2673,6 +2673,11 @@ static hwaddr spapr_rma_size(SpaprMachineState *spapr, Error **errp)
> >          rma_size = MIN(rma_size, smc->rma_limit);
> >      }
> >  
> > +    /*
> > +     * RMA size must be a power of 2
> > +     */
> > +    rma_size = pow2floor(rma_size);
> > +
> 
> I saw somewhere else that the reason behind this might be
> related to:
> 
> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=6010818c30ce9c
> 
> commit 6010818c30ce9c796b4e22fd261fc6fea1cecbfc
> Author: Alexey Kardashevskiy <aik@ozlabs.ru>
> Date:   Thu Jul 3 13:10:05 2014 +1000
> 
>     spapr: Split memory nodes to power-of-two blocks
> 
> Is this the reason ?

Quite likely.

> In any case, it would probably help to mention somewhere
> why the rounding is introduced by this patch.

Drat.  I meant to sort out your comment on the last spin better than
this, but got part way through and forgot what I was doing.

I'm going to merge everything except this last patch into ppc-for-5.0
now, and try to sort out this one a bit later.

> 
> >      if (rma_size < MIN_RMA_SLOF) {
> >          error_setg(errp,
> >  "pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area memory)",
> 

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