Re: [PATCH v6 17/18] spapr: Clean up RMA size calculation

[David Gibson]

On Wed, Feb 26, 2020 at 02:37:51PM +0100, Greg Kurz wrote:
> On Tue, 25 Feb 2020 10:37:23 +1100
> David Gibson <address@hidden> 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 <address@hidden>
> > ---
> >  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;
> >  
> 

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

signature.asc
Description: PGP signature