Re: [for-5.0 3/4] spapr: Clean up RMA size calculation

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 29/11/2019 12:35, 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)
>   * The 16GiB limit we apply is only correct for POWER8, but there is also
>     a 1TiB limit that applies on POWER9.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 57 +++++++++++++++++++++++++++++++-------------------
>  1 file changed, 35 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 52c39daa99..7efd4f2b85 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2664,6 +2664,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);
> +
> +    /*
> +     * RMA size is controlled in hardware by LPCR[RMLS].  On POWER8


RMA is controlled by LPCR on P8 but the RMLS bits on P9 are reserved
(also reserved in PowerISA 3.0).


> +     * the largest RMA that can be specified there is 16GiB


The P8 user manual says:
===
The following RMO sizes are available for the POWER8 processor.
The RMLS[34:37] field in the LPCR defines the RMO sizes, as described below.
1000 - 32 MB
0011 - 64 MB
0111 - 128 MB
0100 - 256 MB
0010 - 1 GB
0001 - 16 GB
0000 - 256 GB
===

The maximum seems to be 256GiB.


> +     */
> +    if (!ppc_type_check_compat(machine->cpu_type, CPU_POWERPC_LOGICAL_3_00,
> +                               0, spapr->max_compat_pvr)) {
> +        rma_size = MIN(rma_size, 16 * GiB);
> +    }
> +
> +    if (rma_size < (MIN_RMA_SLOF * MiB)) {


nit: it is time to redefine MIN_RMA_SLOF to use MiBs imho :)


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

Something went wrong with formatting here.

Otherwise looks good. Thanks,



> +        return -1;
> +    }
> +
> +    return rma_size;
> +}
> +
>  /* pSeries LPAR / sPAPR hardware init */
>  static void spapr_machine_init(MachineState *machine)
>  {
> @@ -2675,7 +2709,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;
> @@ -2715,20 +2748,7 @@ static void spapr_machine_init(MachineState *machine)
>          exit(1);
>      }
>  
> -    spapr->rma_size = node0_size;
> -
> -    /* Actually we don't support unbounded RMA anymore since we added
> -     * proper emulation of HV mode. The max we can get is 16G which
> -     * also happens to be what we configure for PAPR mode so make sure
> -     * we don't do anything bigger than that
> -     */
> -    spapr->rma_size = MIN(spapr->rma_size, 0x400000000ull);
> -
> -    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;
> @@ -2956,13 +2976,6 @@ static void spapr_machine_init(MachineState *machine)
>          }
>      }
>  
> -    if (spapr->rma_size < (MIN_RMA_SLOF * MiB)) {
> -        error_report(
> -            "pSeries SLOF firmware requires >= %ldM guest RMA (Real Mode Area memory)",
> -            MIN_RMA_SLOF);
> -        exit(1);
> -    }
> -
>      if (kernel_filename) {
>          uint64_t lowaddr = 0;
>  
> 

-- 
Alexey