On Tue, 25 Feb 2020 10:37:24 +1100
David Gibson <da...@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 <da...@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 f0354b699d..9ba645c9cb 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);
> @@ -2652,10 +2638,24 @@ 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);
> + 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
> @@ -2672,6 +2672,11 @@ static hwaddr spapr_rma_size(SpaprMachineState *spapr,
> Error **errp)
> spapr->rma_size = MIN(spapr->rma_size, smc->rma_limit);
> }
>
> + /*
> + * RMA size must be a power of 2
> + */
> + rma_size = pow2floor(rma_size);
> +
This slightly changes semantics. In a scenario without NUMA,
we used to get:
rma_size == machine->ram_size
but with this patch we now get:
rma_size == pow2floor(machine->ram_size)
Maybe mention it in the changelog ?
Anyway,
Reviewed-by: Greg Kurz <gr...@kaod.org>
> if (rma_size < (MIN_RMA_SLOF * MiB)) {
> error_setg(errp,
> "pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area)",
