diff mbox series

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

Message ID 20200303034351.333043-18-david@gibson.dropbear.id.au
State New, archived
Headers show
Series target/ppc: Correct some errors with real mode handling | expand

Commit Message

David Gibson March 3, 2020, 3:43 a.m. UTC
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(-)

Comments

Greg Kurz March 3, 2020, 10:32 a.m. UTC | #1
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 ?

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

>      if (rma_size < MIN_RMA_SLOF) {
>          error_setg(errp,
>  "pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area memory)",
David Gibson March 4, 2020, 1:25 a.m. UTC | #2
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)",
>
diff mbox series

Patch

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);
+
     if (rma_size < MIN_RMA_SLOF) {
         error_setg(errp,
 "pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area memory)",