[v6,17/18] spapr: Clean up RMA size calculation
diff mbox series

Message ID 20200224233724.46415-18-david@gibson.dropbear.id.au
State New
Headers show
Series
  • [v6,01/18] pseries: Update SLOF firmware image
Related show

Commit Message

David Gibson Feb. 24, 2020, 11:37 p.m. UTC
Move the calculation of the Real Mode Area (RMA) size into a helper
function.  While we're there clean it up and correct it in a few ways:
  * Add comments making it clearer where the various constraints come from
  * Remove a pointless check that the RMA fits within Node 0 (we've just
    clamped it so that it does)

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c | 59 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 24 deletions(-)

Comments

Philippe Mathieu-Daudé Feb. 25, 2020, 11:07 a.m. UTC | #1
On 2/25/20 12:37 AM, David Gibson wrote:
> Move the calculation of the Real Mode Area (RMA) size into a helper
> function.  While we're there clean it up and correct it in a few ways:
>    * Add comments making it clearer where the various constraints come from
>    * Remove a pointless check that the RMA fits within Node 0 (we've just
>      clamped it so that it does)
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>   hw/ppc/spapr.c | 59 ++++++++++++++++++++++++++++++--------------------
>   1 file changed, 35 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6e9f15f64d..f0354b699d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2648,6 +2648,40 @@ static PCIHostState *spapr_create_default_phb(void)
>       return PCI_HOST_BRIDGE(dev);
>   }
>   
> +static hwaddr spapr_rma_size(SpaprMachineState *spapr, Error **errp)
> +{
> +    MachineState *machine = MACHINE(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);
> +
> +    /*
> +     * VRMA access is via a special 1TiB SLB mapping, so the RMA can
> +     * never exceed that
> +     */
> +    rma_size = MIN(rma_size, TiB);

Can you use '1 * TiB'? It makes review obvious.

> +
> +    /*
> +     * Clamp the RMA size based on machine type.  This is for
> +     * migration compatibility with older qemu versions, which limited
> +     * the RMA size for complicated and mostly bad reasons.
> +     */
> +    if (smc->rma_limit) {
> +        spapr->rma_size = MIN(spapr->rma_size, smc->rma_limit);
> +    }
> +
> +    if (rma_size < (MIN_RMA_SLOF * MiB)) {

This looks old copy/paste before the change "spapr: Don't use weird 
units for MIN_RMA_SLOF".

> +        error_setg(errp,
> +"pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area)",
> +                   MIN_RMA_SLOF);

Similarly, "MIN_RMA_SLOF / MiB"?

> +        return -1;

Maybe return 0 in case this function is called with errp != &error_fatal.

> +    }
> +
> +    return rma_size;
> +}
> +
>   /* pSeries LPAR / sPAPR hardware init */
>   static void spapr_machine_init(MachineState *machine)
>   {
> @@ -2660,7 +2694,6 @@ static void spapr_machine_init(MachineState *machine)
>       int i;
>       MemoryRegion *sysmem = get_system_memory();
>       MemoryRegion *ram = g_new(MemoryRegion, 1);
> -    hwaddr node0_size = spapr_node0_size(machine);
>       long load_limit, fw_size;
>       char *filename;
>       Error *resize_hpt_err = NULL;
> @@ -2700,22 +2733,7 @@ static void spapr_machine_init(MachineState *machine)
>           exit(1);
>       }
>   
> -    spapr->rma_size = node0_size;
> -
> -    /*
> -     * Clamp the RMA size based on machine type.  This is for
> -     * migration compatibility with older qemu versions, which limited
> -     * the RMA size for complicated and mostly bad reasons.
> -     */
> -    if (smc->rma_limit) {
> -        spapr->rma_size = MIN(spapr->rma_size, smc->rma_limit);
> -    }
> -
> -    if (spapr->rma_size > node0_size) {
> -        error_report("Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")",
> -                     spapr->rma_size);
> -        exit(1);
> -    }
> +    spapr->rma_size = spapr_rma_size(spapr, &error_fatal);
>   
>       /* Setup a load limit for the ramdisk leaving room for SLOF and FDT */
>       load_limit = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FW_OVERHEAD;
> @@ -2954,13 +2972,6 @@ static void spapr_machine_init(MachineState *machine)
>           }
>       }
>   
> -    if (spapr->rma_size < MIN_RMA_SLOF) {
> -        error_report(
> -            "pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area memory)",
> -            MIN_RMA_SLOF / MiB);
> -        exit(1);
> -    }
> -
>       if (kernel_filename) {
>           uint64_t lowaddr = 0;
>   
>
David Gibson Feb. 26, 2020, 1:08 a.m. UTC | #2
On Tue, Feb 25, 2020 at 12:07:29PM +0100, Philippe Mathieu-Daudé wrote:
> On 2/25/20 12:37 AM, David Gibson wrote:
> > Move the calculation of the Real Mode Area (RMA) size into a helper
> > function.  While we're there clean it up and correct it in a few ways:
> >    * Add comments making it clearer where the various constraints come from
> >    * Remove a pointless check that the RMA fits within Node 0 (we've just
> >      clamped it so that it does)
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >   hw/ppc/spapr.c | 59 ++++++++++++++++++++++++++++++--------------------
> >   1 file changed, 35 insertions(+), 24 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 6e9f15f64d..f0354b699d 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2648,6 +2648,40 @@ static PCIHostState *spapr_create_default_phb(void)
> >       return PCI_HOST_BRIDGE(dev);
> >   }
> > +static hwaddr spapr_rma_size(SpaprMachineState *spapr, Error **errp)
> > +{
> > +    MachineState *machine = MACHINE(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);
> > +
> > +    /*
> > +     * VRMA access is via a special 1TiB SLB mapping, so the RMA can
> > +     * never exceed that
> > +     */
> > +    rma_size = MIN(rma_size, TiB);
> 
> Can you use '1 * TiB'? It makes review obvious.

Done.

> > +
> > +    /*
> > +     * Clamp the RMA size based on machine type.  This is for
> > +     * migration compatibility with older qemu versions, which limited
> > +     * the RMA size for complicated and mostly bad reasons.
> > +     */
> > +    if (smc->rma_limit) {
> > +        spapr->rma_size = MIN(spapr->rma_size, smc->rma_limit);
> > +    }
> > +
> > +    if (rma_size < (MIN_RMA_SLOF * MiB)) {
> 
> This looks old copy/paste before the change "spapr: Don't use weird units
> for MIN_RMA_SLOF".
> 
> > +        error_setg(errp,
> > +"pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area)",
> > +                   MIN_RMA_SLOF);
> 
> Similarly, "MIN_RMA_SLOF / MiB"?

Ah, good catch.  I re-ordered the series at some point and forgot to
fix this up.

> > +        return -1;
> 
> Maybe return 0 in case this function is called with errp !=
> &error_fatal.

Good idea.
Greg Kurz Feb. 26, 2020, 1:37 p.m. UTC | #3
On Tue, 25 Feb 2020 10:37:23 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Move the calculation of the Real Mode Area (RMA) size into a helper
> function.  While we're there clean it up and correct it in a few ways:
>   * Add comments making it clearer where the various constraints come from
>   * Remove a pointless check that the RMA fits within Node 0 (we've just
>     clamped it so that it does)
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 59 ++++++++++++++++++++++++++++++--------------------
>  1 file changed, 35 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6e9f15f64d..f0354b699d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2648,6 +2648,40 @@ static PCIHostState *spapr_create_default_phb(void)
>      return PCI_HOST_BRIDGE(dev);
>  }
>  
> +static hwaddr spapr_rma_size(SpaprMachineState *spapr, Error **errp)
> +{
> +    MachineState *machine = MACHINE(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);
> +
> +    /*
> +     * VRMA access is via a special 1TiB SLB mapping, so the RMA can
> +     * never exceed that
> +     */
> +    rma_size = MIN(rma_size, TiB);
> +
> +    /*
> +     * Clamp the RMA size based on machine type.  This is for
> +     * migration compatibility with older qemu versions, which limited
> +     * the RMA size for complicated and mostly bad reasons.
> +     */
> +    if (smc->rma_limit) {

/home/greg/Work/qemu/qemu-ppc/hw/ppc/spapr.c: In function ‘spapr_rma_size’:
/home/greg/Work/qemu/qemu-ppc/hw/ppc/spapr.c:2671:9: error: ‘smc’ undeclared (first use in this function)
     if (smc->rma_limit) {

> +        spapr->rma_size = MIN(spapr->rma_size, smc->rma_limit);
> +    }
> +
> +    if (rma_size < (MIN_RMA_SLOF * MiB)) {
> +        error_setg(errp,
> +"pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area)",
> +                   MIN_RMA_SLOF);
> +        return -1;
> +    }
> +
> +    return rma_size;
> +}
> +
>  /* pSeries LPAR / sPAPR hardware init */
>  static void spapr_machine_init(MachineState *machine)
>  {
> @@ -2660,7 +2694,6 @@ static void spapr_machine_init(MachineState *machine)
>      int i;
>      MemoryRegion *sysmem = get_system_memory();
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
> -    hwaddr node0_size = spapr_node0_size(machine);
>      long load_limit, fw_size;
>      char *filename;
>      Error *resize_hpt_err = NULL;
> @@ -2700,22 +2733,7 @@ static void spapr_machine_init(MachineState *machine)
>          exit(1);
>      }
>  
> -    spapr->rma_size = node0_size;
> -
> -    /*
> -     * Clamp the RMA size based on machine type.  This is for
> -     * migration compatibility with older qemu versions, which limited
> -     * the RMA size for complicated and mostly bad reasons.
> -     */
> -    if (smc->rma_limit) {
> -        spapr->rma_size = MIN(spapr->rma_size, smc->rma_limit);
> -    }
> -
> -    if (spapr->rma_size > node0_size) {
> -        error_report("Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")",
> -                     spapr->rma_size);
> -        exit(1);
> -    }
> +    spapr->rma_size = spapr_rma_size(spapr, &error_fatal);
>  
>      /* Setup a load limit for the ramdisk leaving room for SLOF and FDT */
>      load_limit = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FW_OVERHEAD;
> @@ -2954,13 +2972,6 @@ static void spapr_machine_init(MachineState *machine)
>          }
>      }
>  
> -    if (spapr->rma_size < MIN_RMA_SLOF) {
> -        error_report(
> -            "pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area memory)",
> -            MIN_RMA_SLOF / MiB);
> -        exit(1);
> -    }
> -
>      if (kernel_filename) {
>          uint64_t lowaddr = 0;
>
David Gibson Feb. 27, 2020, 6:04 a.m. UTC | #4
On Wed, Feb 26, 2020 at 02:37:51PM +0100, Greg Kurz wrote:
> On Tue, 25 Feb 2020 10:37:23 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Move the calculation of the Real Mode Area (RMA) size into a helper
> > function.  While we're there clean it up and correct it in a few ways:
> >   * Add comments making it clearer where the various constraints come from
> >   * Remove a pointless check that the RMA fits within Node 0 (we've just
> >     clamped it so that it does)
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c | 59 ++++++++++++++++++++++++++++++--------------------
> >  1 file changed, 35 insertions(+), 24 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 6e9f15f64d..f0354b699d 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2648,6 +2648,40 @@ static PCIHostState *spapr_create_default_phb(void)
> >      return PCI_HOST_BRIDGE(dev);
> >  }
> >  
> > +static hwaddr spapr_rma_size(SpaprMachineState *spapr, Error **errp)
> > +{
> > +    MachineState *machine = MACHINE(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);
> > +
> > +    /*
> > +     * VRMA access is via a special 1TiB SLB mapping, so the RMA can
> > +     * never exceed that
> > +     */
> > +    rma_size = MIN(rma_size, TiB);
> > +
> > +    /*
> > +     * Clamp the RMA size based on machine type.  This is for
> > +     * migration compatibility with older qemu versions, which limited
> > +     * the RMA size for complicated and mostly bad reasons.
> > +     */
> > +    if (smc->rma_limit) {
> 
> /home/greg/Work/qemu/qemu-ppc/hw/ppc/spapr.c: In function ‘spapr_rma_size’:
> /home/greg/Work/qemu/qemu-ppc/hw/ppc/spapr.c:2671:9: error: ‘smc’ undeclared (first use in this function)
>      if (smc->rma_limit) {

Oops.  Fixed.

> 
> > +        spapr->rma_size = MIN(spapr->rma_size, smc->rma_limit);
> > +    }
> > +
> > +    if (rma_size < (MIN_RMA_SLOF * MiB)) {
> > +        error_setg(errp,
> > +"pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area)",
> > +                   MIN_RMA_SLOF);
> > +        return -1;
> > +    }
> > +
> > +    return rma_size;
> > +}
> > +
> >  /* pSeries LPAR / sPAPR hardware init */
> >  static void spapr_machine_init(MachineState *machine)
> >  {
> > @@ -2660,7 +2694,6 @@ static void spapr_machine_init(MachineState *machine)
> >      int i;
> >      MemoryRegion *sysmem = get_system_memory();
> >      MemoryRegion *ram = g_new(MemoryRegion, 1);
> > -    hwaddr node0_size = spapr_node0_size(machine);
> >      long load_limit, fw_size;
> >      char *filename;
> >      Error *resize_hpt_err = NULL;
> > @@ -2700,22 +2733,7 @@ static void spapr_machine_init(MachineState *machine)
> >          exit(1);
> >      }
> >  
> > -    spapr->rma_size = node0_size;
> > -
> > -    /*
> > -     * Clamp the RMA size based on machine type.  This is for
> > -     * migration compatibility with older qemu versions, which limited
> > -     * the RMA size for complicated and mostly bad reasons.
> > -     */
> > -    if (smc->rma_limit) {
> > -        spapr->rma_size = MIN(spapr->rma_size, smc->rma_limit);
> > -    }
> > -
> > -    if (spapr->rma_size > node0_size) {
> > -        error_report("Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")",
> > -                     spapr->rma_size);
> > -        exit(1);
> > -    }
> > +    spapr->rma_size = spapr_rma_size(spapr, &error_fatal);
> >  
> >      /* Setup a load limit for the ramdisk leaving room for SLOF and FDT */
> >      load_limit = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FW_OVERHEAD;
> > @@ -2954,13 +2972,6 @@ static void spapr_machine_init(MachineState *machine)
> >          }
> >      }
> >  
> > -    if (spapr->rma_size < MIN_RMA_SLOF) {
> > -        error_report(
> > -            "pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area memory)",
> > -            MIN_RMA_SLOF / MiB);
> > -        exit(1);
> > -    }
> > -
> >      if (kernel_filename) {
> >          uint64_t lowaddr = 0;
> >  
>

Patch
diff mbox series

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6e9f15f64d..f0354b699d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2648,6 +2648,40 @@  static PCIHostState *spapr_create_default_phb(void)
     return PCI_HOST_BRIDGE(dev);
 }
 
+static hwaddr spapr_rma_size(SpaprMachineState *spapr, Error **errp)
+{
+    MachineState *machine = MACHINE(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);
+
+    /*
+     * VRMA access is via a special 1TiB SLB mapping, so the RMA can
+     * never exceed that
+     */
+    rma_size = MIN(rma_size, TiB);
+
+    /*
+     * Clamp the RMA size based on machine type.  This is for
+     * migration compatibility with older qemu versions, which limited
+     * the RMA size for complicated and mostly bad reasons.
+     */
+    if (smc->rma_limit) {
+        spapr->rma_size = MIN(spapr->rma_size, smc->rma_limit);
+    }
+
+    if (rma_size < (MIN_RMA_SLOF * MiB)) {
+        error_setg(errp,
+"pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area)",
+                   MIN_RMA_SLOF);
+        return -1;
+    }
+
+    return rma_size;
+}
+
 /* pSeries LPAR / sPAPR hardware init */
 static void spapr_machine_init(MachineState *machine)
 {
@@ -2660,7 +2694,6 @@  static void spapr_machine_init(MachineState *machine)
     int i;
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *ram = g_new(MemoryRegion, 1);
-    hwaddr node0_size = spapr_node0_size(machine);
     long load_limit, fw_size;
     char *filename;
     Error *resize_hpt_err = NULL;
@@ -2700,22 +2733,7 @@  static void spapr_machine_init(MachineState *machine)
         exit(1);
     }
 
-    spapr->rma_size = node0_size;
-
-    /*
-     * Clamp the RMA size based on machine type.  This is for
-     * migration compatibility with older qemu versions, which limited
-     * the RMA size for complicated and mostly bad reasons.
-     */
-    if (smc->rma_limit) {
-        spapr->rma_size = MIN(spapr->rma_size, smc->rma_limit);
-    }
-
-    if (spapr->rma_size > node0_size) {
-        error_report("Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")",
-                     spapr->rma_size);
-        exit(1);
-    }
+    spapr->rma_size = spapr_rma_size(spapr, &error_fatal);
 
     /* Setup a load limit for the ramdisk leaving room for SLOF and FDT */
     load_limit = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FW_OVERHEAD;
@@ -2954,13 +2972,6 @@  static void spapr_machine_init(MachineState *machine)
         }
     }
 
-    if (spapr->rma_size < MIN_RMA_SLOF) {
-        error_report(
-            "pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area memory)",
-            MIN_RMA_SLOF / MiB);
-        exit(1);
-    }
-
     if (kernel_filename) {
         uint64_t lowaddr = 0;