[PATCH 0/4] spapr: Assorted minor cleanups

[PATCH 0/4] spapr: Assorted minor cleanups

[David Gibson]

Here's a handful of cleanups that came out of larger bits of work but
which aren't intimately tied to those.  For one reason or another they
got forgotten for a while, but I've now dug them out, polished them a
bit and hope to get them in just in time for the qemu-5.0 soft freeze.

David Gibson (4):
  spapr: Move creation of ibm,dynamic-reconfiguration-memory dt node
  spapr: Move creation of ibm,architecture-vec-5 property
  spapr: Rename DT functions to newer naming convention
  spapr: Fold spapr_node0_size() into its only caller

 hw/ppc/spapr.c              | 597 ++++++++++++++++++------------------
 hw/ppc/spapr_ovec.c         |   4 +-
 include/hw/ppc/spapr_ovec.h |   4 +-
 3 files changed, 294 insertions(+), 311 deletions(-)

-- 
2.24.1

[PATCH 1/4] spapr: Move creation of ibm, dynamic-reconfiguration-memory dt node

[David Gibson]

Currently this node with information about hotpluggable memory is created
from spapr_dt_cas_updates().  But that's just a hangover from when we
created it only as a diff to the device tree at CAS time.  Now that we
fully rebuild the DT as CAS time, it makes more sense to create this along
with the rest of the memory information in the device tree.

So, move it to spapr_populate_memory().  The patch is huge, but it's nearly
all just code motion.

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 64bc8b83e9..66289ffef5 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -341,257 +341,6 @@ static int spapr_populate_memory_node(void *fdt, int nodeid, hwaddr start,
     return off;
 }
 
-static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
-{
-    MachineState *machine = MACHINE(spapr);
-    hwaddr mem_start, node_size;
-    int i, nb_nodes = machine->numa_state->num_nodes;
-    NodeInfo *nodes = machine->numa_state->nodes;
-
-    for (i = 0, mem_start = 0; i < nb_nodes; ++i) {
-        if (!nodes[i].node_mem) {
-            continue;
-        }
-        if (mem_start >= machine->ram_size) {
-            node_size = 0;
-        } else {
-            node_size = nodes[i].node_mem;
-            if (node_size > machine->ram_size - mem_start) {
-                node_size = machine->ram_size - mem_start;
-            }
-        }
-        if (!mem_start) {
-            /* spapr_machine_init() checks for rma_size <= node0_size
-             * already */
-            spapr_populate_memory_node(fdt, i, 0, spapr->rma_size);
-            mem_start += spapr->rma_size;
-            node_size -= spapr->rma_size;
-        }
-        for ( ; node_size; ) {
-            hwaddr sizetmp = pow2floor(node_size);
-
-            /* mem_start != 0 here */
-            if (ctzl(mem_start) < ctzl(sizetmp)) {
-                sizetmp = 1ULL << ctzl(mem_start);
-            }
-
-            spapr_populate_memory_node(fdt, i, mem_start, sizetmp);
-            node_size -= sizetmp;
-            mem_start += sizetmp;
-        }
-    }
-
-    return 0;
-}
-
-static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
-                                  SpaprMachineState *spapr)
-{
-    MachineState *ms = MACHINE(spapr);
-    PowerPCCPU *cpu = POWERPC_CPU(cs);
-    CPUPPCState *env = &cpu->env;
-    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
-    int index = spapr_get_vcpu_id(cpu);
-    uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40),
-                       0xffffffff, 0xffffffff};
-    uint32_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq()
-        : SPAPR_TIMEBASE_FREQ;
-    uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() : 1000000000;
-    uint32_t page_sizes_prop[64];
-    size_t page_sizes_prop_size;
-    unsigned int smp_threads = ms->smp.threads;
-    uint32_t vcpus_per_socket = smp_threads * ms->smp.cores;
-    uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
-    int compat_smt = MIN(smp_threads, ppc_compat_max_vthreads(cpu));
-    SpaprDrc *drc;
-    int drc_index;
-    uint32_t radix_AP_encodings[PPC_PAGE_SIZES_MAX_SZ];
-    int i;
-
-    drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index);
-    if (drc) {
-        drc_index = spapr_drc_index(drc);
-        _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)));
-    }
-
-    _FDT((fdt_setprop_cell(fdt, offset, "reg", index)));
-    _FDT((fdt_setprop_string(fdt, offset, "device_type", "cpu")));
-
-    _FDT((fdt_setprop_cell(fdt, offset, "cpu-version", env->spr[SPR_PVR])));
-    _FDT((fdt_setprop_cell(fdt, offset, "d-cache-block-size",
-                           env->dcache_line_size)));
-    _FDT((fdt_setprop_cell(fdt, offset, "d-cache-line-size",
-                           env->dcache_line_size)));
-    _FDT((fdt_setprop_cell(fdt, offset, "i-cache-block-size",
-                           env->icache_line_size)));
-    _FDT((fdt_setprop_cell(fdt, offset, "i-cache-line-size",
-                           env->icache_line_size)));
-
-    if (pcc->l1_dcache_size) {
-        _FDT((fdt_setprop_cell(fdt, offset, "d-cache-size",
-                               pcc->l1_dcache_size)));
-    } else {
-        warn_report("Unknown L1 dcache size for cpu");
-    }
-    if (pcc->l1_icache_size) {
-        _FDT((fdt_setprop_cell(fdt, offset, "i-cache-size",
-                               pcc->l1_icache_size)));
-    } else {
-        warn_report("Unknown L1 icache size for cpu");
-    }
-
-    _FDT((fdt_setprop_cell(fdt, offset, "timebase-frequency", tbfreq)));
-    _FDT((fdt_setprop_cell(fdt, offset, "clock-frequency", cpufreq)));
-    _FDT((fdt_setprop_cell(fdt, offset, "slb-size", cpu->hash64_opts->slb_size)));
-    _FDT((fdt_setprop_cell(fdt, offset, "ibm,slb-size", cpu->hash64_opts->slb_size)));
-    _FDT((fdt_setprop_string(fdt, offset, "status", "okay")));
-    _FDT((fdt_setprop(fdt, offset, "64-bit", NULL, 0)));
-
-    if (env->spr_cb[SPR_PURR].oea_read) {
-        _FDT((fdt_setprop_cell(fdt, offset, "ibm,purr", 1)));
-    }
-    if (env->spr_cb[SPR_SPURR].oea_read) {
-        _FDT((fdt_setprop_cell(fdt, offset, "ibm,spurr", 1)));
-    }
-
-    if (ppc_hash64_has(cpu, PPC_HASH64_1TSEG)) {
-        _FDT((fdt_setprop(fdt, offset, "ibm,processor-segment-sizes",
-                          segs, sizeof(segs))));
-    }
-
-    /* Advertise VSX (vector extensions) if available
-     *   1               == VMX / Altivec available
-     *   2               == VSX available
-     *
-     * Only CPUs for which we create core types in spapr_cpu_core.c
-     * are possible, and all of those have VMX */
-    if (spapr_get_cap(spapr, SPAPR_CAP_VSX) != 0) {
-        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 2)));
-    } else {
-        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 1)));
-    }
-
-    /* Advertise DFP (Decimal Floating Point) if available
-     *   0 / no property == no DFP
-     *   1               == DFP available */
-    if (spapr_get_cap(spapr, SPAPR_CAP_DFP) != 0) {
-        _FDT((fdt_setprop_cell(fdt, offset, "ibm,dfp", 1)));
-    }
-
-    page_sizes_prop_size = ppc_create_page_sizes_prop(cpu, page_sizes_prop,
-                                                      sizeof(page_sizes_prop));
-    if (page_sizes_prop_size) {
-        _FDT((fdt_setprop(fdt, offset, "ibm,segment-page-sizes",
-                          page_sizes_prop, page_sizes_prop_size)));
-    }
-
-    spapr_populate_pa_features(spapr, cpu, fdt, offset);
-
-    _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
-                           cs->cpu_index / vcpus_per_socket)));
-
-    _FDT((fdt_setprop(fdt, offset, "ibm,pft-size",
-                      pft_size_prop, sizeof(pft_size_prop))));
-
-    if (ms->numa_state->num_nodes > 1) {
-        _FDT(spapr_fixup_cpu_numa_dt(fdt, offset, cpu));
-    }
-
-    _FDT(spapr_fixup_cpu_smt_dt(fdt, offset, cpu, compat_smt));
-
-    if (pcc->radix_page_info) {
-        for (i = 0; i < pcc->radix_page_info->count; i++) {
-            radix_AP_encodings[i] =
-                cpu_to_be32(pcc->radix_page_info->entries[i]);
-        }
-        _FDT((fdt_setprop(fdt, offset, "ibm,processor-radix-AP-encodings",
-                          radix_AP_encodings,
-                          pcc->radix_page_info->count *
-                          sizeof(radix_AP_encodings[0]))));
-    }
-
-    /*
-     * We set this property to let the guest know that it can use the large
-     * decrementer and its width in bits.
-     */
-    if (spapr_get_cap(spapr, SPAPR_CAP_LARGE_DECREMENTER) != SPAPR_CAP_OFF)
-        _FDT((fdt_setprop_u32(fdt, offset, "ibm,dec-bits",
-                              pcc->lrg_decr_bits)));
-}
-
-static void spapr_populate_cpus_dt_node(void *fdt, SpaprMachineState *spapr)
-{
-    CPUState **rev;
-    CPUState *cs;
-    int n_cpus;
-    int cpus_offset;
-    char *nodename;
-    int i;
-
-    cpus_offset = fdt_add_subnode(fdt, 0, "cpus");
-    _FDT(cpus_offset);
-    _FDT((fdt_setprop_cell(fdt, cpus_offset, "#address-cells", 0x1)));
-    _FDT((fdt_setprop_cell(fdt, cpus_offset, "#size-cells", 0x0)));
-
-    /*
-     * We walk the CPUs in reverse order to ensure that CPU DT nodes
-     * created by fdt_add_subnode() end up in the right order in FDT
-     * for the guest kernel the enumerate the CPUs correctly.
-     *
-     * The CPU list cannot be traversed in reverse order, so we need
-     * to do extra work.
-     */
-    n_cpus = 0;
-    rev = NULL;
-    CPU_FOREACH(cs) {
-        rev = g_renew(CPUState *, rev, n_cpus + 1);
-        rev[n_cpus++] = cs;
-    }
-
-    for (i = n_cpus - 1; i >= 0; i--) {
-        CPUState *cs = rev[i];
-        PowerPCCPU *cpu = POWERPC_CPU(cs);
-        int index = spapr_get_vcpu_id(cpu);
-        DeviceClass *dc = DEVICE_GET_CLASS(cs);
-        int offset;
-
-        if (!spapr_is_thread0_in_vcore(spapr, cpu)) {
-            continue;
-        }
-
-        nodename = g_strdup_printf("%s@%x", dc->fw_name, index);
-        offset = fdt_add_subnode(fdt, cpus_offset, nodename);
-        g_free(nodename);
-        _FDT(offset);
-        spapr_populate_cpu_dt(cs, fdt, offset, spapr);
-    }
-
-    g_free(rev);
-}
-
-static int spapr_rng_populate_dt(void *fdt)
-{
-    int node;
-    int ret;
-
-    node = qemu_fdt_add_subnode(fdt, "/ibm,platform-facilities");
-    if (node <= 0) {
-        return -1;
-    }
-    ret = fdt_setprop_string(fdt, node, "device_type",
-                             "ibm,platform-facilities");
-    ret |= fdt_setprop_cell(fdt, node, "#address-cells", 0x1);
-    ret |= fdt_setprop_cell(fdt, node, "#size-cells", 0x0);
-
-    node = fdt_add_subnode(fdt, node, "ibm,random-v1");
-    if (node <= 0) {
-        return -1;
-    }
-    ret |= fdt_setprop_string(fdt, node, "compatible", "ibm,random");
-
-    return ret ? -1 : 0;
-}
-
 static uint32_t spapr_pc_dimm_node(MemoryDeviceInfoList *list, ram_addr_t addr)
 {
     MemoryDeviceInfoList *info;
@@ -877,14 +626,51 @@ static int spapr_populate_drconf_memory(SpaprMachineState *spapr, void *fdt)
     return ret;
 }
 
-static int spapr_dt_cas_updates(SpaprMachineState *spapr, void *fdt,
-                                SpaprOptionVector *ov5_updates)
+static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
 {
+    MachineState *machine = MACHINE(spapr);
     SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
-    int ret = 0, offset;
+    hwaddr mem_start, node_size;
+    int i, nb_nodes = machine->numa_state->num_nodes;
+    NodeInfo *nodes = machine->numa_state->nodes;
+
+    for (i = 0, mem_start = 0; i < nb_nodes; ++i) {
+        if (!nodes[i].node_mem) {
+            continue;
+        }
+        if (mem_start >= machine->ram_size) {
+            node_size = 0;
+        } else {
+            node_size = nodes[i].node_mem;
+            if (node_size > machine->ram_size - mem_start) {
+                node_size = machine->ram_size - mem_start;
+            }
+        }
+        if (!mem_start) {
+            /* spapr_machine_init() checks for rma_size <= node0_size
+             * already */
+            spapr_populate_memory_node(fdt, i, 0, spapr->rma_size);
+            mem_start += spapr->rma_size;
+            node_size -= spapr->rma_size;
+        }
+        for ( ; node_size; ) {
+            hwaddr sizetmp = pow2floor(node_size);
+
+            /* mem_start != 0 here */
+            if (ctzl(mem_start) < ctzl(sizetmp)) {
+                sizetmp = 1ULL << ctzl(mem_start);
+            }
+
+            spapr_populate_memory_node(fdt, i, mem_start, sizetmp);
+            node_size -= sizetmp;
+            mem_start += sizetmp;
+        }
+    }
 
     /* Generate ibm,dynamic-reconfiguration-memory node if required */
-    if (spapr_ovec_test(ov5_updates, OV5_DRCONF_MEMORY)) {
+    if (spapr_ovec_test(spapr->ov5_cas, OV5_DRCONF_MEMORY)) {
+        int ret;
+
         g_assert(smc->dr_lmb_enabled);
         ret = spapr_populate_drconf_memory(spapr, fdt);
         if (ret) {
@@ -892,6 +678,222 @@ static int spapr_dt_cas_updates(SpaprMachineState *spapr, void *fdt,
         }
     }
 
+    return 0;
+}
+
+static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
+                                  SpaprMachineState *spapr)
+{
+    MachineState *ms = MACHINE(spapr);
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
+    int index = spapr_get_vcpu_id(cpu);
+    uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40),
+                       0xffffffff, 0xffffffff};
+    uint32_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq()
+        : SPAPR_TIMEBASE_FREQ;
+    uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() : 1000000000;
+    uint32_t page_sizes_prop[64];
+    size_t page_sizes_prop_size;
+    unsigned int smp_threads = ms->smp.threads;
+    uint32_t vcpus_per_socket = smp_threads * ms->smp.cores;
+    uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
+    int compat_smt = MIN(smp_threads, ppc_compat_max_vthreads(cpu));
+    SpaprDrc *drc;
+    int drc_index;
+    uint32_t radix_AP_encodings[PPC_PAGE_SIZES_MAX_SZ];
+    int i;
+
+    drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index);
+    if (drc) {
+        drc_index = spapr_drc_index(drc);
+        _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)));
+    }
+
+    _FDT((fdt_setprop_cell(fdt, offset, "reg", index)));
+    _FDT((fdt_setprop_string(fdt, offset, "device_type", "cpu")));
+
+    _FDT((fdt_setprop_cell(fdt, offset, "cpu-version", env->spr[SPR_PVR])));
+    _FDT((fdt_setprop_cell(fdt, offset, "d-cache-block-size",
+                           env->dcache_line_size)));
+    _FDT((fdt_setprop_cell(fdt, offset, "d-cache-line-size",
+                           env->dcache_line_size)));
+    _FDT((fdt_setprop_cell(fdt, offset, "i-cache-block-size",
+                           env->icache_line_size)));
+    _FDT((fdt_setprop_cell(fdt, offset, "i-cache-line-size",
+                           env->icache_line_size)));
+
+    if (pcc->l1_dcache_size) {
+        _FDT((fdt_setprop_cell(fdt, offset, "d-cache-size",
+                               pcc->l1_dcache_size)));
+    } else {
+        warn_report("Unknown L1 dcache size for cpu");
+    }
+    if (pcc->l1_icache_size) {
+        _FDT((fdt_setprop_cell(fdt, offset, "i-cache-size",
+                               pcc->l1_icache_size)));
+    } else {
+        warn_report("Unknown L1 icache size for cpu");
+    }
+
+    _FDT((fdt_setprop_cell(fdt, offset, "timebase-frequency", tbfreq)));
+    _FDT((fdt_setprop_cell(fdt, offset, "clock-frequency", cpufreq)));
+    _FDT((fdt_setprop_cell(fdt, offset, "slb-size", cpu->hash64_opts->slb_size)));
+    _FDT((fdt_setprop_cell(fdt, offset, "ibm,slb-size", cpu->hash64_opts->slb_size)));
+    _FDT((fdt_setprop_string(fdt, offset, "status", "okay")));
+    _FDT((fdt_setprop(fdt, offset, "64-bit", NULL, 0)));
+
+    if (env->spr_cb[SPR_PURR].oea_read) {
+        _FDT((fdt_setprop_cell(fdt, offset, "ibm,purr", 1)));
+    }
+    if (env->spr_cb[SPR_SPURR].oea_read) {
+        _FDT((fdt_setprop_cell(fdt, offset, "ibm,spurr", 1)));
+    }
+
+    if (ppc_hash64_has(cpu, PPC_HASH64_1TSEG)) {
+        _FDT((fdt_setprop(fdt, offset, "ibm,processor-segment-sizes",
+                          segs, sizeof(segs))));
+    }
+
+    /* Advertise VSX (vector extensions) if available
+     *   1               == VMX / Altivec available
+     *   2               == VSX available
+     *
+     * Only CPUs for which we create core types in spapr_cpu_core.c
+     * are possible, and all of those have VMX */
+    if (spapr_get_cap(spapr, SPAPR_CAP_VSX) != 0) {
+        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 2)));
+    } else {
+        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 1)));
+    }
+
+    /* Advertise DFP (Decimal Floating Point) if available
+     *   0 / no property == no DFP
+     *   1               == DFP available */
+    if (spapr_get_cap(spapr, SPAPR_CAP_DFP) != 0) {
+        _FDT((fdt_setprop_cell(fdt, offset, "ibm,dfp", 1)));
+    }
+
+    page_sizes_prop_size = ppc_create_page_sizes_prop(cpu, page_sizes_prop,
+                                                      sizeof(page_sizes_prop));
+    if (page_sizes_prop_size) {
+        _FDT((fdt_setprop(fdt, offset, "ibm,segment-page-sizes",
+                          page_sizes_prop, page_sizes_prop_size)));
+    }
+
+    spapr_populate_pa_features(spapr, cpu, fdt, offset);
+
+    _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
+                           cs->cpu_index / vcpus_per_socket)));
+
+    _FDT((fdt_setprop(fdt, offset, "ibm,pft-size",
+                      pft_size_prop, sizeof(pft_size_prop))));
+
+    if (ms->numa_state->num_nodes > 1) {
+        _FDT(spapr_fixup_cpu_numa_dt(fdt, offset, cpu));
+    }
+
+    _FDT(spapr_fixup_cpu_smt_dt(fdt, offset, cpu, compat_smt));
+
+    if (pcc->radix_page_info) {
+        for (i = 0; i < pcc->radix_page_info->count; i++) {
+            radix_AP_encodings[i] =
+                cpu_to_be32(pcc->radix_page_info->entries[i]);
+        }
+        _FDT((fdt_setprop(fdt, offset, "ibm,processor-radix-AP-encodings",
+                          radix_AP_encodings,
+                          pcc->radix_page_info->count *
+                          sizeof(radix_AP_encodings[0]))));
+    }
+
+    /*
+     * We set this property to let the guest know that it can use the large
+     * decrementer and its width in bits.
+     */
+    if (spapr_get_cap(spapr, SPAPR_CAP_LARGE_DECREMENTER) != SPAPR_CAP_OFF)
+        _FDT((fdt_setprop_u32(fdt, offset, "ibm,dec-bits",
+                              pcc->lrg_decr_bits)));
+}
+
+static void spapr_populate_cpus_dt_node(void *fdt, SpaprMachineState *spapr)
+{
+    CPUState **rev;
+    CPUState *cs;
+    int n_cpus;
+    int cpus_offset;
+    char *nodename;
+    int i;
+
+    cpus_offset = fdt_add_subnode(fdt, 0, "cpus");
+    _FDT(cpus_offset);
+    _FDT((fdt_setprop_cell(fdt, cpus_offset, "#address-cells", 0x1)));
+    _FDT((fdt_setprop_cell(fdt, cpus_offset, "#size-cells", 0x0)));
+
+    /*
+     * We walk the CPUs in reverse order to ensure that CPU DT nodes
+     * created by fdt_add_subnode() end up in the right order in FDT
+     * for the guest kernel the enumerate the CPUs correctly.
+     *
+     * The CPU list cannot be traversed in reverse order, so we need
+     * to do extra work.
+     */
+    n_cpus = 0;
+    rev = NULL;
+    CPU_FOREACH(cs) {
+        rev = g_renew(CPUState *, rev, n_cpus + 1);
+        rev[n_cpus++] = cs;
+    }
+
+    for (i = n_cpus - 1; i >= 0; i--) {
+        CPUState *cs = rev[i];
+        PowerPCCPU *cpu = POWERPC_CPU(cs);
+        int index = spapr_get_vcpu_id(cpu);
+        DeviceClass *dc = DEVICE_GET_CLASS(cs);
+        int offset;
+
+        if (!spapr_is_thread0_in_vcore(spapr, cpu)) {
+            continue;
+        }
+
+        nodename = g_strdup_printf("%s@%x", dc->fw_name, index);
+        offset = fdt_add_subnode(fdt, cpus_offset, nodename);
+        g_free(nodename);
+        _FDT(offset);
+        spapr_populate_cpu_dt(cs, fdt, offset, spapr);
+    }
+
+    g_free(rev);
+}
+
+static int spapr_rng_populate_dt(void *fdt)
+{
+    int node;
+    int ret;
+
+    node = qemu_fdt_add_subnode(fdt, "/ibm,platform-facilities");
+    if (node <= 0) {
+        return -1;
+    }
+    ret = fdt_setprop_string(fdt, node, "device_type",
+                             "ibm,platform-facilities");
+    ret |= fdt_setprop_cell(fdt, node, "#address-cells", 0x1);
+    ret |= fdt_setprop_cell(fdt, node, "#size-cells", 0x0);
+
+    node = fdt_add_subnode(fdt, node, "ibm,random-v1");
+    if (node <= 0) {
+        return -1;
+    }
+    ret |= fdt_setprop_string(fdt, node, "compatible", "ibm,random");
+
+    return ret ? -1 : 0;
+}
+
+static int spapr_dt_cas_updates(SpaprMachineState *spapr, void *fdt,
+                                SpaprOptionVector *ov5_updates)
+{
+    int offset;
+
     offset = fdt_path_offset(fdt, "/chosen");
     if (offset < 0) {
         offset = fdt_add_subnode(fdt, 0, "chosen");
-- 
2.24.1

Re: [PATCH 1/4] spapr: Move creation of ibm, dynamic-reconfiguration-memory dt node

[Greg Kurz]

On Fri, 13 Mar 2020 15:05:36 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Currently this node with information about hotpluggable memory is created
> from spapr_dt_cas_updates().  But that's just a hangover from when we
> created it only as a diff to the device tree at CAS time.  Now that we
> fully rebuild the DT as CAS time, it makes more sense to create this along
> with the rest of the memory information in the device tree.
> 
> So, move it to spapr_populate_memory().  The patch is huge, but it's nearly
> all just code motion.
> 

The change looks good but it may be possible to improve the diffstat,
see some comments below.

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 512 +++++++++++++++++++++++++------------------------
>  1 file changed, 257 insertions(+), 255 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 64bc8b83e9..66289ffef5 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -341,257 +341,6 @@ static int spapr_populate_memory_node(void *fdt, int nodeid, hwaddr start,
>      return off;
>  }
>  
> -static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
> -{
> -    MachineState *machine = MACHINE(spapr);
> -    hwaddr mem_start, node_size;
> -    int i, nb_nodes = machine->numa_state->num_nodes;
> -    NodeInfo *nodes = machine->numa_state->nodes;
> -
> -    for (i = 0, mem_start = 0; i < nb_nodes; ++i) {
> -        if (!nodes[i].node_mem) {
> -            continue;
> -        }
> -        if (mem_start >= machine->ram_size) {
> -            node_size = 0;
> -        } else {
> -            node_size = nodes[i].node_mem;
> -            if (node_size > machine->ram_size - mem_start) {
> -                node_size = machine->ram_size - mem_start;
> -            }
> -        }
> -        if (!mem_start) {
> -            /* spapr_machine_init() checks for rma_size <= node0_size
> -             * already */
> -            spapr_populate_memory_node(fdt, i, 0, spapr->rma_size);
> -            mem_start += spapr->rma_size;
> -            node_size -= spapr->rma_size;
> -        }
> -        for ( ; node_size; ) {
> -            hwaddr sizetmp = pow2floor(node_size);
> -
> -            /* mem_start != 0 here */
> -            if (ctzl(mem_start) < ctzl(sizetmp)) {
> -                sizetmp = 1ULL << ctzl(mem_start);
> -            }
> -
> -            spapr_populate_memory_node(fdt, i, mem_start, sizetmp);
> -            node_size -= sizetmp;
> -            mem_start += sizetmp;
> -        }
> -    }
> -
> -    return 0;
> -}
> -
> -static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
> -                                  SpaprMachineState *spapr)
> -{
> -    MachineState *ms = MACHINE(spapr);
> -    PowerPCCPU *cpu = POWERPC_CPU(cs);
> -    CPUPPCState *env = &cpu->env;
> -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
> -    int index = spapr_get_vcpu_id(cpu);
> -    uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40),
> -                       0xffffffff, 0xffffffff};
> -    uint32_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq()
> -        : SPAPR_TIMEBASE_FREQ;
> -    uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() : 1000000000;
> -    uint32_t page_sizes_prop[64];
> -    size_t page_sizes_prop_size;
> -    unsigned int smp_threads = ms->smp.threads;
> -    uint32_t vcpus_per_socket = smp_threads * ms->smp.cores;
> -    uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
> -    int compat_smt = MIN(smp_threads, ppc_compat_max_vthreads(cpu));
> -    SpaprDrc *drc;
> -    int drc_index;
> -    uint32_t radix_AP_encodings[PPC_PAGE_SIZES_MAX_SZ];
> -    int i;
> -
> -    drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index);
> -    if (drc) {
> -        drc_index = spapr_drc_index(drc);
> -        _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)));
> -    }
> -
> -    _FDT((fdt_setprop_cell(fdt, offset, "reg", index)));
> -    _FDT((fdt_setprop_string(fdt, offset, "device_type", "cpu")));
> -
> -    _FDT((fdt_setprop_cell(fdt, offset, "cpu-version", env->spr[SPR_PVR])));
> -    _FDT((fdt_setprop_cell(fdt, offset, "d-cache-block-size",
> -                           env->dcache_line_size)));
> -    _FDT((fdt_setprop_cell(fdt, offset, "d-cache-line-size",
> -                           env->dcache_line_size)));
> -    _FDT((fdt_setprop_cell(fdt, offset, "i-cache-block-size",
> -                           env->icache_line_size)));
> -    _FDT((fdt_setprop_cell(fdt, offset, "i-cache-line-size",
> -                           env->icache_line_size)));
> -
> -    if (pcc->l1_dcache_size) {
> -        _FDT((fdt_setprop_cell(fdt, offset, "d-cache-size",
> -                               pcc->l1_dcache_size)));
> -    } else {
> -        warn_report("Unknown L1 dcache size for cpu");
> -    }
> -    if (pcc->l1_icache_size) {
> -        _FDT((fdt_setprop_cell(fdt, offset, "i-cache-size",
> -                               pcc->l1_icache_size)));
> -    } else {
> -        warn_report("Unknown L1 icache size for cpu");
> -    }
> -
> -    _FDT((fdt_setprop_cell(fdt, offset, "timebase-frequency", tbfreq)));
> -    _FDT((fdt_setprop_cell(fdt, offset, "clock-frequency", cpufreq)));
> -    _FDT((fdt_setprop_cell(fdt, offset, "slb-size", cpu->hash64_opts->slb_size)));
> -    _FDT((fdt_setprop_cell(fdt, offset, "ibm,slb-size", cpu->hash64_opts->slb_size)));
> -    _FDT((fdt_setprop_string(fdt, offset, "status", "okay")));
> -    _FDT((fdt_setprop(fdt, offset, "64-bit", NULL, 0)));
> -
> -    if (env->spr_cb[SPR_PURR].oea_read) {
> -        _FDT((fdt_setprop_cell(fdt, offset, "ibm,purr", 1)));
> -    }
> -    if (env->spr_cb[SPR_SPURR].oea_read) {
> -        _FDT((fdt_setprop_cell(fdt, offset, "ibm,spurr", 1)));
> -    }
> -
> -    if (ppc_hash64_has(cpu, PPC_HASH64_1TSEG)) {
> -        _FDT((fdt_setprop(fdt, offset, "ibm,processor-segment-sizes",
> -                          segs, sizeof(segs))));
> -    }
> -
> -    /* Advertise VSX (vector extensions) if available
> -     *   1               == VMX / Altivec available
> -     *   2               == VSX available
> -     *
> -     * Only CPUs for which we create core types in spapr_cpu_core.c
> -     * are possible, and all of those have VMX */
> -    if (spapr_get_cap(spapr, SPAPR_CAP_VSX) != 0) {
> -        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 2)));
> -    } else {
> -        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 1)));
> -    }
> -
> -    /* Advertise DFP (Decimal Floating Point) if available
> -     *   0 / no property == no DFP
> -     *   1               == DFP available */
> -    if (spapr_get_cap(spapr, SPAPR_CAP_DFP) != 0) {
> -        _FDT((fdt_setprop_cell(fdt, offset, "ibm,dfp", 1)));
> -    }
> -
> -    page_sizes_prop_size = ppc_create_page_sizes_prop(cpu, page_sizes_prop,
> -                                                      sizeof(page_sizes_prop));
> -    if (page_sizes_prop_size) {
> -        _FDT((fdt_setprop(fdt, offset, "ibm,segment-page-sizes",
> -                          page_sizes_prop, page_sizes_prop_size)));
> -    }
> -
> -    spapr_populate_pa_features(spapr, cpu, fdt, offset);
> -
> -    _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
> -                           cs->cpu_index / vcpus_per_socket)));
> -
> -    _FDT((fdt_setprop(fdt, offset, "ibm,pft-size",
> -                      pft_size_prop, sizeof(pft_size_prop))));
> -
> -    if (ms->numa_state->num_nodes > 1) {
> -        _FDT(spapr_fixup_cpu_numa_dt(fdt, offset, cpu));
> -    }
> -
> -    _FDT(spapr_fixup_cpu_smt_dt(fdt, offset, cpu, compat_smt));
> -
> -    if (pcc->radix_page_info) {
> -        for (i = 0; i < pcc->radix_page_info->count; i++) {
> -            radix_AP_encodings[i] =
> -                cpu_to_be32(pcc->radix_page_info->entries[i]);
> -        }
> -        _FDT((fdt_setprop(fdt, offset, "ibm,processor-radix-AP-encodings",
> -                          radix_AP_encodings,
> -                          pcc->radix_page_info->count *
> -                          sizeof(radix_AP_encodings[0]))));
> -    }
> -
> -    /*
> -     * We set this property to let the guest know that it can use the large
> -     * decrementer and its width in bits.
> -     */
> -    if (spapr_get_cap(spapr, SPAPR_CAP_LARGE_DECREMENTER) != SPAPR_CAP_OFF)
> -        _FDT((fdt_setprop_u32(fdt, offset, "ibm,dec-bits",
> -                              pcc->lrg_decr_bits)));
> -}
> -
> -static void spapr_populate_cpus_dt_node(void *fdt, SpaprMachineState *spapr)
> -{
> -    CPUState **rev;
> -    CPUState *cs;
> -    int n_cpus;
> -    int cpus_offset;
> -    char *nodename;
> -    int i;
> -
> -    cpus_offset = fdt_add_subnode(fdt, 0, "cpus");
> -    _FDT(cpus_offset);
> -    _FDT((fdt_setprop_cell(fdt, cpus_offset, "#address-cells", 0x1)));
> -    _FDT((fdt_setprop_cell(fdt, cpus_offset, "#size-cells", 0x0)));
> -
> -    /*
> -     * We walk the CPUs in reverse order to ensure that CPU DT nodes
> -     * created by fdt_add_subnode() end up in the right order in FDT
> -     * for the guest kernel the enumerate the CPUs correctly.
> -     *
> -     * The CPU list cannot be traversed in reverse order, so we need
> -     * to do extra work.
> -     */
> -    n_cpus = 0;
> -    rev = NULL;
> -    CPU_FOREACH(cs) {
> -        rev = g_renew(CPUState *, rev, n_cpus + 1);
> -        rev[n_cpus++] = cs;
> -    }
> -
> -    for (i = n_cpus - 1; i >= 0; i--) {
> -        CPUState *cs = rev[i];
> -        PowerPCCPU *cpu = POWERPC_CPU(cs);
> -        int index = spapr_get_vcpu_id(cpu);
> -        DeviceClass *dc = DEVICE_GET_CLASS(cs);
> -        int offset;
> -
> -        if (!spapr_is_thread0_in_vcore(spapr, cpu)) {
> -            continue;
> -        }
> -
> -        nodename = g_strdup_printf("%s@%x", dc->fw_name, index);
> -        offset = fdt_add_subnode(fdt, cpus_offset, nodename);
> -        g_free(nodename);
> -        _FDT(offset);
> -        spapr_populate_cpu_dt(cs, fdt, offset, spapr);
> -    }
> -
> -    g_free(rev);
> -}
> -
> -static int spapr_rng_populate_dt(void *fdt)

I'm not quite sure why this function needs to move...

> -{
> -    int node;
> -    int ret;
> -
> -    node = qemu_fdt_add_subnode(fdt, "/ibm,platform-facilities");
> -    if (node <= 0) {
> -        return -1;
> -    }
> -    ret = fdt_setprop_string(fdt, node, "device_type",
> -                             "ibm,platform-facilities");
> -    ret |= fdt_setprop_cell(fdt, node, "#address-cells", 0x1);
> -    ret |= fdt_setprop_cell(fdt, node, "#size-cells", 0x0);
> -
> -    node = fdt_add_subnode(fdt, node, "ibm,random-v1");
> -    if (node <= 0) {
> -        return -1;
> -    }
> -    ret |= fdt_setprop_string(fdt, node, "compatible", "ibm,random");
> -
> -    return ret ? -1 : 0;
> -}
> -
>  static uint32_t spapr_pc_dimm_node(MemoryDeviceInfoList *list, ram_addr_t addr)
>  {
>      MemoryDeviceInfoList *info;
> @@ -877,14 +626,51 @@ static int spapr_populate_drconf_memory(SpaprMachineState *spapr, void *fdt)
>      return ret;
>  }
>  
> -static int spapr_dt_cas_updates(SpaprMachineState *spapr, void *fdt,
> -                                SpaprOptionVector *ov5_updates)
> +static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
>  {
> +    MachineState *machine = MACHINE(spapr);
>      SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> -    int ret = 0, offset;
> +    hwaddr mem_start, node_size;
> +    int i, nb_nodes = machine->numa_state->num_nodes;
> +    NodeInfo *nodes = machine->numa_state->nodes;
> +
> +    for (i = 0, mem_start = 0; i < nb_nodes; ++i) {
> +        if (!nodes[i].node_mem) {
> +            continue;
> +        }
> +        if (mem_start >= machine->ram_size) {
> +            node_size = 0;
> +        } else {
> +            node_size = nodes[i].node_mem;
> +            if (node_size > machine->ram_size - mem_start) {
> +                node_size = machine->ram_size - mem_start;
> +            }
> +        }
> +        if (!mem_start) {
> +            /* spapr_machine_init() checks for rma_size <= node0_size
> +             * already */
> +            spapr_populate_memory_node(fdt, i, 0, spapr->rma_size);
> +            mem_start += spapr->rma_size;
> +            node_size -= spapr->rma_size;
> +        }
> +        for ( ; node_size; ) {
> +            hwaddr sizetmp = pow2floor(node_size);
> +
> +            /* mem_start != 0 here */
> +            if (ctzl(mem_start) < ctzl(sizetmp)) {
> +                sizetmp = 1ULL << ctzl(mem_start);
> +            }
> +
> +            spapr_populate_memory_node(fdt, i, mem_start, sizetmp);
> +            node_size -= sizetmp;
> +            mem_start += sizetmp;
> +        }
> +    }
>  
>      /* Generate ibm,dynamic-reconfiguration-memory node if required */
> -    if (spapr_ovec_test(ov5_updates, OV5_DRCONF_MEMORY)) {
> +    if (spapr_ovec_test(spapr->ov5_cas, OV5_DRCONF_MEMORY)) {
> +        int ret;
> +
>          g_assert(smc->dr_lmb_enabled);
>          ret = spapr_populate_drconf_memory(spapr, fdt);
>          if (ret) {
> @@ -892,6 +678,222 @@ static int spapr_dt_cas_updates(SpaprMachineState *spapr, void *fdt,
>          }
>      }
>  
> +    return 0;
> +}
> +
> +static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
> +                                  SpaprMachineState *spapr)
> +{
> +    MachineState *ms = MACHINE(spapr);
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
> +    int index = spapr_get_vcpu_id(cpu);
> +    uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40),
> +                       0xffffffff, 0xffffffff};
> +    uint32_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq()
> +        : SPAPR_TIMEBASE_FREQ;
> +    uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() : 1000000000;
> +    uint32_t page_sizes_prop[64];
> +    size_t page_sizes_prop_size;
> +    unsigned int smp_threads = ms->smp.threads;
> +    uint32_t vcpus_per_socket = smp_threads * ms->smp.cores;
> +    uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
> +    int compat_smt = MIN(smp_threads, ppc_compat_max_vthreads(cpu));
> +    SpaprDrc *drc;
> +    int drc_index;
> +    uint32_t radix_AP_encodings[PPC_PAGE_SIZES_MAX_SZ];
> +    int i;
> +
> +    drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index);
> +    if (drc) {
> +        drc_index = spapr_drc_index(drc);
> +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)));
> +    }
> +
> +    _FDT((fdt_setprop_cell(fdt, offset, "reg", index)));
> +    _FDT((fdt_setprop_string(fdt, offset, "device_type", "cpu")));
> +
> +    _FDT((fdt_setprop_cell(fdt, offset, "cpu-version", env->spr[SPR_PVR])));
> +    _FDT((fdt_setprop_cell(fdt, offset, "d-cache-block-size",
> +                           env->dcache_line_size)));
> +    _FDT((fdt_setprop_cell(fdt, offset, "d-cache-line-size",
> +                           env->dcache_line_size)));
> +    _FDT((fdt_setprop_cell(fdt, offset, "i-cache-block-size",
> +                           env->icache_line_size)));
> +    _FDT((fdt_setprop_cell(fdt, offset, "i-cache-line-size",
> +                           env->icache_line_size)));
> +
> +    if (pcc->l1_dcache_size) {
> +        _FDT((fdt_setprop_cell(fdt, offset, "d-cache-size",
> +                               pcc->l1_dcache_size)));
> +    } else {
> +        warn_report("Unknown L1 dcache size for cpu");
> +    }
> +    if (pcc->l1_icache_size) {
> +        _FDT((fdt_setprop_cell(fdt, offset, "i-cache-size",
> +                               pcc->l1_icache_size)));
> +    } else {
> +        warn_report("Unknown L1 icache size for cpu");
> +    }
> +
> +    _FDT((fdt_setprop_cell(fdt, offset, "timebase-frequency", tbfreq)));
> +    _FDT((fdt_setprop_cell(fdt, offset, "clock-frequency", cpufreq)));
> +    _FDT((fdt_setprop_cell(fdt, offset, "slb-size", cpu->hash64_opts->slb_size)));
> +    _FDT((fdt_setprop_cell(fdt, offset, "ibm,slb-size", cpu->hash64_opts->slb_size)));
> +    _FDT((fdt_setprop_string(fdt, offset, "status", "okay")));
> +    _FDT((fdt_setprop(fdt, offset, "64-bit", NULL, 0)));
> +
> +    if (env->spr_cb[SPR_PURR].oea_read) {
> +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,purr", 1)));
> +    }
> +    if (env->spr_cb[SPR_SPURR].oea_read) {
> +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,spurr", 1)));
> +    }
> +
> +    if (ppc_hash64_has(cpu, PPC_HASH64_1TSEG)) {
> +        _FDT((fdt_setprop(fdt, offset, "ibm,processor-segment-sizes",
> +                          segs, sizeof(segs))));
> +    }
> +
> +    /* Advertise VSX (vector extensions) if available
> +     *   1               == VMX / Altivec available
> +     *   2               == VSX available
> +     *
> +     * Only CPUs for which we create core types in spapr_cpu_core.c
> +     * are possible, and all of those have VMX */
> +    if (spapr_get_cap(spapr, SPAPR_CAP_VSX) != 0) {
> +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 2)));
> +    } else {
> +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 1)));
> +    }
> +
> +    /* Advertise DFP (Decimal Floating Point) if available
> +     *   0 / no property == no DFP
> +     *   1               == DFP available */
> +    if (spapr_get_cap(spapr, SPAPR_CAP_DFP) != 0) {
> +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,dfp", 1)));
> +    }
> +
> +    page_sizes_prop_size = ppc_create_page_sizes_prop(cpu, page_sizes_prop,
> +                                                      sizeof(page_sizes_prop));
> +    if (page_sizes_prop_size) {
> +        _FDT((fdt_setprop(fdt, offset, "ibm,segment-page-sizes",
> +                          page_sizes_prop, page_sizes_prop_size)));
> +    }
> +
> +    spapr_populate_pa_features(spapr, cpu, fdt, offset);
> +
> +    _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
> +                           cs->cpu_index / vcpus_per_socket)));
> +
> +    _FDT((fdt_setprop(fdt, offset, "ibm,pft-size",
> +                      pft_size_prop, sizeof(pft_size_prop))));
> +
> +    if (ms->numa_state->num_nodes > 1) {
> +        _FDT(spapr_fixup_cpu_numa_dt(fdt, offset, cpu));
> +    }
> +
> +    _FDT(spapr_fixup_cpu_smt_dt(fdt, offset, cpu, compat_smt));
> +
> +    if (pcc->radix_page_info) {
> +        for (i = 0; i < pcc->radix_page_info->count; i++) {
> +            radix_AP_encodings[i] =
> +                cpu_to_be32(pcc->radix_page_info->entries[i]);
> +        }
> +        _FDT((fdt_setprop(fdt, offset, "ibm,processor-radix-AP-encodings",
> +                          radix_AP_encodings,
> +                          pcc->radix_page_info->count *
> +                          sizeof(radix_AP_encodings[0]))));
> +    }
> +
> +    /*
> +     * We set this property to let the guest know that it can use the large
> +     * decrementer and its width in bits.
> +     */
> +    if (spapr_get_cap(spapr, SPAPR_CAP_LARGE_DECREMENTER) != SPAPR_CAP_OFF)
> +        _FDT((fdt_setprop_u32(fdt, offset, "ibm,dec-bits",
> +                              pcc->lrg_decr_bits)));
> +}
> +
> +static void spapr_populate_cpus_dt_node(void *fdt, SpaprMachineState *spapr)
> +{
> +    CPUState **rev;
> +    CPUState *cs;
> +    int n_cpus;
> +    int cpus_offset;
> +    char *nodename;
> +    int i;
> +
> +    cpus_offset = fdt_add_subnode(fdt, 0, "cpus");
> +    _FDT(cpus_offset);
> +    _FDT((fdt_setprop_cell(fdt, cpus_offset, "#address-cells", 0x1)));
> +    _FDT((fdt_setprop_cell(fdt, cpus_offset, "#size-cells", 0x0)));
> +
> +    /*
> +     * We walk the CPUs in reverse order to ensure that CPU DT nodes
> +     * created by fdt_add_subnode() end up in the right order in FDT
> +     * for the guest kernel the enumerate the CPUs correctly.
> +     *
> +     * The CPU list cannot be traversed in reverse order, so we need
> +     * to do extra work.
> +     */
> +    n_cpus = 0;
> +    rev = NULL;
> +    CPU_FOREACH(cs) {
> +        rev = g_renew(CPUState *, rev, n_cpus + 1);
> +        rev[n_cpus++] = cs;
> +    }
> +
> +    for (i = n_cpus - 1; i >= 0; i--) {
> +        CPUState *cs = rev[i];
> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> +        int index = spapr_get_vcpu_id(cpu);
> +        DeviceClass *dc = DEVICE_GET_CLASS(cs);
> +        int offset;
> +
> +        if (!spapr_is_thread0_in_vcore(spapr, cpu)) {
> +            continue;
> +        }
> +
> +        nodename = g_strdup_printf("%s@%x", dc->fw_name, index);
> +        offset = fdt_add_subnode(fdt, cpus_offset, nodename);
> +        g_free(nodename);
> +        _FDT(offset);
> +        spapr_populate_cpu_dt(cs, fdt, offset, spapr);
> +    }
> +
> +    g_free(rev);
> +}
> +
> +static int spapr_rng_populate_dt(void *fdt)
> +{
> +    int node;
> +    int ret;
> +
> +    node = qemu_fdt_add_subnode(fdt, "/ibm,platform-facilities");
> +    if (node <= 0) {
> +        return -1;
> +    }
> +    ret = fdt_setprop_string(fdt, node, "device_type",
> +                             "ibm,platform-facilities");
> +    ret |= fdt_setprop_cell(fdt, node, "#address-cells", 0x1);
> +    ret |= fdt_setprop_cell(fdt, node, "#size-cells", 0x0);
> +
> +    node = fdt_add_subnode(fdt, node, "ibm,random-v1");
> +    if (node <= 0) {
> +        return -1;
> +    }
> +    ret |= fdt_setprop_string(fdt, node, "compatible", "ibm,random");
> +
> +    return ret ? -1 : 0;
> +}
> +
> +static int spapr_dt_cas_updates(SpaprMachineState *spapr, void *fdt,
> +                                SpaprOptionVector *ov5_updates)
> +{
> +    int offset;
> +
>      offset = fdt_path_offset(fdt, "/chosen");
>      if (offset < 0) {
>          offset = fdt_add_subnode(fdt, 0, "chosen");

The final code is thus:

static int spapr_dt_cas_updates(SpaprMachineState *spapr, void *fdt,
                                SpaprOptionVector *ov5_updates)
{
    int offset;

    offset = fdt_path_offset(fdt, "/chosen");
    if (offset < 0) {
        offset = fdt_add_subnode(fdt, 0, "chosen");
        if (offset < 0) {
            return offset;
        }
    }
    return spapr_ovec_populate_dt(fdt, offset, spapr->ov5_cas,
                                  "ibm,architecture-vec-5");
}

ie. ov5_updates has no more users. It wasn't very useful BTW
since the unique caller of spapr_dt_cas_updates() already
passes spapr->ov5_cas. Maybe kill it ?

Anyway, in case you prefer to fix this up later:

Reviewed-by: Greg Kurz <groug@kaod.org>

Re: [PATCH 1/4] spapr: Move creation of ibm, dynamic-reconfiguration-memory dt node

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1211 bytes --]

On Fri, Mar 13, 2020 at 12:30:51PM +0100, Greg Kurz wrote:
> On Fri, 13 Mar 2020 15:05:36 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Currently this node with information about hotpluggable memory is created
> > from spapr_dt_cas_updates().  But that's just a hangover from when we
> > created it only as a diff to the device tree at CAS time.  Now that we
> > fully rebuild the DT as CAS time, it makes more sense to create this along
> > with the rest of the memory information in the device tree.
> > 
> > So, move it to spapr_populate_memory().  The patch is huge, but it's nearly
> > all just code motion.
> > 
> 
> The change looks good but it may be possible to improve the diffstat,
> see some comments below.

[snip]
> > -
> > -static int spapr_rng_populate_dt(void *fdt)
> 
> I'm not quite sure why this function needs to move...

Eh, I guess it didn't.  I'm disinclined to change it now though, so I
can get this in for tomorrows last-PR-before-soft-freeze.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/4] spapr: Move creation of ibm,architecture-vec-5 property

[David Gibson]

This is currently called from spapr_dt_cas_updates() which is a hang over
from when we created this only as a diff to the DT at CAS time.  Now that
we fully rebuild the DT at CAS time, just create it alon with the rest
of the properties in /chosen.

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 66289ffef5..fc28d9df25 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -889,22 +889,6 @@ static int spapr_rng_populate_dt(void *fdt)
     return ret ? -1 : 0;
 }
 
-static int spapr_dt_cas_updates(SpaprMachineState *spapr, void *fdt,
-                                SpaprOptionVector *ov5_updates)
-{
-    int offset;
-
-    offset = fdt_path_offset(fdt, "/chosen");
-    if (offset < 0) {
-        offset = fdt_add_subnode(fdt, 0, "chosen");
-        if (offset < 0) {
-            return offset;
-        }
-    }
-    return spapr_ovec_populate_dt(fdt, offset, spapr->ov5_cas,
-                                  "ibm,architecture-vec-5");
-}
-
 static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
 {
     MachineState *ms = MACHINE(spapr);
@@ -1115,6 +1099,9 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt)
 
     spapr_dt_ov5_platform_support(spapr, fdt, chosen);
 
+    _FDT(spapr_ovec_populate_dt(fdt, offset, spapr->ov5_cas,
+                                "ibm,architecture-vec-5"));
+
     g_free(stdout_path);
     g_free(bootlist);
 }
@@ -1263,13 +1250,6 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
         }
     }
 
-    /* ibm,client-architecture-support updates */
-    ret = spapr_dt_cas_updates(spapr, fdt, spapr->ov5_cas);
-    if (ret < 0) {
-        error_report("couldn't setup CAS properties fdt");
-        exit(1);
-    }
-
     if (smc->dr_phb_enabled) {
         ret = spapr_dt_drc(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_PHB);
         if (ret < 0) {
-- 
2.24.1

Re: [PATCH 2/4] spapr: Move creation of ibm,architecture-vec-5 property

[Greg Kurz]

On Fri, 13 Mar 2020 15:05:37 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> This is currently called from spapr_dt_cas_updates() which is a hang over
> from when we created this only as a diff to the DT at CAS time.  Now that
> we fully rebuild the DT at CAS time, just create it alon with the rest

s/alon/along

> of the properties in /chosen.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 26 +++-----------------------
>  1 file changed, 3 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 66289ffef5..fc28d9df25 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -889,22 +889,6 @@ static int spapr_rng_populate_dt(void *fdt)
>      return ret ? -1 : 0;
>  }
>  
> -static int spapr_dt_cas_updates(SpaprMachineState *spapr, void *fdt,
> -                                SpaprOptionVector *ov5_updates)

Heh I should have looked at patch 2 before commenting on patch 1 :)

Reviewed-by: Greg Kurz <groug@kaod.org>

> -{
> -    int offset;
> -
> -    offset = fdt_path_offset(fdt, "/chosen");
> -    if (offset < 0) {
> -        offset = fdt_add_subnode(fdt, 0, "chosen");
> -        if (offset < 0) {
> -            return offset;
> -        }
> -    }
> -    return spapr_ovec_populate_dt(fdt, offset, spapr->ov5_cas,
> -                                  "ibm,architecture-vec-5");
> -}
> -
>  static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
>  {
>      MachineState *ms = MACHINE(spapr);
> @@ -1115,6 +1099,9 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt)
>  
>      spapr_dt_ov5_platform_support(spapr, fdt, chosen);
>  
> +    _FDT(spapr_ovec_populate_dt(fdt, offset, spapr->ov5_cas,
> +                                "ibm,architecture-vec-5"));
> +
>      g_free(stdout_path);
>      g_free(bootlist);
>  }
> @@ -1263,13 +1250,6 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
>          }
>      }
>  
> -    /* ibm,client-architecture-support updates */
> -    ret = spapr_dt_cas_updates(spapr, fdt, spapr->ov5_cas);
> -    if (ret < 0) {
> -        error_report("couldn't setup CAS properties fdt");
> -        exit(1);
> -    }
> -
>      if (smc->dr_phb_enabled) {
>          ret = spapr_dt_drc(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_PHB);
>          if (ret < 0) {

Re: [PATCH 2/4] spapr: Move creation of ibm,architecture-vec-5 property

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2843 bytes --]

On Fri, Mar 13, 2020 at 12:40:03PM +0100, Greg Kurz wrote:
> On Fri, 13 Mar 2020 15:05:37 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > This is currently called from spapr_dt_cas_updates() which is a hang over
> > from when we created this only as a diff to the DT at CAS time.  Now that
> > we fully rebuild the DT at CAS time, just create it alon with the rest
> 
> s/alon/along

Corrected, thanks.

> 
> > of the properties in /chosen.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c | 26 +++-----------------------
> >  1 file changed, 3 insertions(+), 23 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 66289ffef5..fc28d9df25 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -889,22 +889,6 @@ static int spapr_rng_populate_dt(void *fdt)
> >      return ret ? -1 : 0;
> >  }
> >  
> > -static int spapr_dt_cas_updates(SpaprMachineState *spapr, void *fdt,
> > -                                SpaprOptionVector *ov5_updates)
> 
> Heh I should have looked at patch 2 before commenting on patch 1 :)
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> > -{
> > -    int offset;
> > -
> > -    offset = fdt_path_offset(fdt, "/chosen");
> > -    if (offset < 0) {
> > -        offset = fdt_add_subnode(fdt, 0, "chosen");
> > -        if (offset < 0) {
> > -            return offset;
> > -        }
> > -    }
> > -    return spapr_ovec_populate_dt(fdt, offset, spapr->ov5_cas,
> > -                                  "ibm,architecture-vec-5");
> > -}
> > -
> >  static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
> >  {
> >      MachineState *ms = MACHINE(spapr);
> > @@ -1115,6 +1099,9 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt)
> >  
> >      spapr_dt_ov5_platform_support(spapr, fdt, chosen);
> >  
> > +    _FDT(spapr_ovec_populate_dt(fdt, offset, spapr->ov5_cas,
> > +                                "ibm,architecture-vec-5"));
> > +
> >      g_free(stdout_path);
> >      g_free(bootlist);
> >  }
> > @@ -1263,13 +1250,6 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
> >          }
> >      }
> >  
> > -    /* ibm,client-architecture-support updates */
> > -    ret = spapr_dt_cas_updates(spapr, fdt, spapr->ov5_cas);
> > -    if (ret < 0) {
> > -        error_report("couldn't setup CAS properties fdt");
> > -        exit(1);
> > -    }
> > -
> >      if (smc->dr_phb_enabled) {
> >          ret = spapr_dt_drc(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_PHB);
> >          if (ret < 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/4] spapr: Rename DT functions to newer naming convention

[David Gibson]

In the spapr code we've been gradually moving towards a convention that
functions which create pieces of the device tree are called spapr_dt_*().
This patch speeds that along by renaming most of the things that don't yet
match that so that they do.

For now we leave the *_dt_populate() functions which are actual methods
used in the DRCClass::dt_populate method.

While we're there we remove a few comments that don't really say anything
useful.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c              | 62 +++++++++++++++++--------------------
 hw/ppc/spapr_ovec.c         |  4 +--
 include/hw/ppc/spapr_ovec.h |  4 +--
 3 files changed, 33 insertions(+), 37 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index fc28d9df25..6c32ec3c0a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -217,10 +217,9 @@ static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, PowerPCCPU *cpu)
                           sizeof(associativity));
 }
 
-/* Populate the "ibm,pa-features" property */
-static void spapr_populate_pa_features(SpaprMachineState *spapr,
-                                       PowerPCCPU *cpu,
-                                       void *fdt, int offset)
+static void spapr_dt_pa_features(SpaprMachineState *spapr,
+                                 PowerPCCPU *cpu,
+                                 void *fdt, int offset)
 {
     uint8_t pa_features_206[] = { 6, 0,
         0xf6, 0x1f, 0xc7, 0x00, 0x80, 0xc0 };
@@ -315,8 +314,8 @@ static void add_str(GString *s, const gchar *s1)
     g_string_append_len(s, s1, strlen(s1) + 1);
 }
 
-static int spapr_populate_memory_node(void *fdt, int nodeid, hwaddr start,
-                                       hwaddr size)
+static int spapr_dt_memory_node(void *fdt, int nodeid, hwaddr start,
+                                hwaddr size)
 {
     uint32_t associativity[] = {
         cpu_to_be32(0x4), /* length */
@@ -391,9 +390,8 @@ spapr_get_drconf_cell(uint32_t seq_lmbs, uint64_t base_addr,
     return elem;
 }
 
-/* ibm,dynamic-memory-v2 */
-static int spapr_populate_drmem_v2(SpaprMachineState *spapr, void *fdt,
-                                   int offset, MemoryDeviceInfoList *dimms)
+static int spapr_dt_dynamic_memory_v2(SpaprMachineState *spapr, void *fdt,
+                                      int offset, MemoryDeviceInfoList *dimms)
 {
     MachineState *machine = MACHINE(spapr);
     uint8_t *int_buf, *cur_index;
@@ -484,8 +482,7 @@ static int spapr_populate_drmem_v2(SpaprMachineState *spapr, void *fdt,
     return 0;
 }
 
-/* ibm,dynamic-memory */
-static int spapr_populate_drmem_v1(SpaprMachineState *spapr, void *fdt,
+static int spapr_dt_dynamic_memory(SpaprMachineState *spapr, void *fdt,
                                    int offset, MemoryDeviceInfoList *dimms)
 {
     MachineState *machine = MACHINE(spapr);
@@ -554,7 +551,8 @@ static int spapr_populate_drmem_v1(SpaprMachineState *spapr, void *fdt,
  * Refer to docs/specs/ppc-spapr-hotplug.txt for the documentation
  * of this device tree node.
  */
-static int spapr_populate_drconf_memory(SpaprMachineState *spapr, void *fdt)
+static int spapr_dt_dynamic_reconfiguration_memory(SpaprMachineState *spapr,
+                                                   void *fdt)
 {
     MachineState *machine = MACHINE(spapr);
     int nb_numa_nodes = machine->numa_state->num_nodes;
@@ -593,9 +591,9 @@ static int spapr_populate_drconf_memory(SpaprMachineState *spapr, void *fdt)
     /* ibm,dynamic-memory or ibm,dynamic-memory-v2 */
     dimms = qmp_memory_device_list();
     if (spapr_ovec_test(spapr->ov5_cas, OV5_DRMEM_V2)) {
-        ret = spapr_populate_drmem_v2(spapr, fdt, offset, dimms);
+        ret = spapr_dt_dynamic_memory_v2(spapr, fdt, offset, dimms);
     } else {
-        ret = spapr_populate_drmem_v1(spapr, fdt, offset, dimms);
+        ret = spapr_dt_dynamic_memory(spapr, fdt, offset, dimms);
     }
     qapi_free_MemoryDeviceInfoList(dimms);
 
@@ -626,7 +624,7 @@ static int spapr_populate_drconf_memory(SpaprMachineState *spapr, void *fdt)
     return ret;
 }
 
-static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
+static int spapr_dt_memory(SpaprMachineState *spapr, void *fdt)
 {
     MachineState *machine = MACHINE(spapr);
     SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
@@ -649,7 +647,7 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
         if (!mem_start) {
             /* spapr_machine_init() checks for rma_size <= node0_size
              * already */
-            spapr_populate_memory_node(fdt, i, 0, spapr->rma_size);
+            spapr_dt_memory_node(fdt, i, 0, spapr->rma_size);
             mem_start += spapr->rma_size;
             node_size -= spapr->rma_size;
         }
@@ -661,7 +659,7 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
                 sizetmp = 1ULL << ctzl(mem_start);
             }
 
-            spapr_populate_memory_node(fdt, i, mem_start, sizetmp);
+            spapr_dt_memory_node(fdt, i, mem_start, sizetmp);
             node_size -= sizetmp;
             mem_start += sizetmp;
         }
@@ -672,7 +670,7 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
         int ret;
 
         g_assert(smc->dr_lmb_enabled);
-        ret = spapr_populate_drconf_memory(spapr, fdt);
+        ret = spapr_dt_dynamic_reconfiguration_memory(spapr, fdt);
         if (ret) {
             return ret;
         }
@@ -681,8 +679,8 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
     return 0;
 }
 
-static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
-                                  SpaprMachineState *spapr)
+static void spapr_dt_cpu(CPUState *cs, void *fdt, int offset,
+                         SpaprMachineState *spapr)
 {
     MachineState *ms = MACHINE(spapr);
     PowerPCCPU *cpu = POWERPC_CPU(cs);
@@ -782,7 +780,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
                           page_sizes_prop, page_sizes_prop_size)));
     }
 
-    spapr_populate_pa_features(spapr, cpu, fdt, offset);
+    spapr_dt_pa_features(spapr, cpu, fdt, offset);
 
     _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
                            cs->cpu_index / vcpus_per_socket)));
@@ -816,7 +814,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
                               pcc->lrg_decr_bits)));
 }
 
-static void spapr_populate_cpus_dt_node(void *fdt, SpaprMachineState *spapr)
+static void spapr_dt_cpus(void *fdt, SpaprMachineState *spapr)
 {
     CPUState **rev;
     CPUState *cs;
@@ -860,13 +858,13 @@ static void spapr_populate_cpus_dt_node(void *fdt, SpaprMachineState *spapr)
         offset = fdt_add_subnode(fdt, cpus_offset, nodename);
         g_free(nodename);
         _FDT(offset);
-        spapr_populate_cpu_dt(cs, fdt, offset, spapr);
+        spapr_dt_cpu(cs, fdt, offset, spapr);
     }
 
     g_free(rev);
 }
 
-static int spapr_rng_populate_dt(void *fdt)
+static int spapr_dt_rng(void *fdt)
 {
     int node;
     int ret;
@@ -1099,8 +1097,7 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt)
 
     spapr_dt_ov5_platform_support(spapr, fdt, chosen);
 
-    _FDT(spapr_ovec_populate_dt(fdt, offset, spapr->ov5_cas,
-                                "ibm,architecture-vec-5"));
+    _FDT(spapr_dt_ovec(fdt, chosen, spapr->ov5_cas, "ibm,architecture-vec-5"));
 
     g_free(stdout_path);
     g_free(bootlist);
@@ -1181,7 +1178,7 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
     /* /interrupt controller */
     spapr_irq_dt(spapr, spapr_max_server_number(spapr), fdt, PHANDLE_INTC);
 
-    ret = spapr_populate_memory(spapr, fdt);
+    ret = spapr_dt_memory(spapr, fdt);
     if (ret < 0) {
         error_report("couldn't setup memory nodes in fdt");
         exit(1);
@@ -1191,7 +1188,7 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
     spapr_dt_vdevice(spapr->vio_bus, fdt);
 
     if (object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)) {
-        ret = spapr_rng_populate_dt(fdt);
+        ret = spapr_dt_rng(fdt);
         if (ret < 0) {
             error_report("could not set up rng device in the fdt");
             exit(1);
@@ -1206,8 +1203,7 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
         }
     }
 
-    /* cpus */
-    spapr_populate_cpus_dt_node(fdt, spapr);
+    spapr_dt_cpus(fdt, spapr);
 
     if (smc->dr_lmb_enabled) {
         _FDT(spapr_dt_drc(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB));
@@ -3400,8 +3396,8 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
     addr = spapr_drc_index(drc) * SPAPR_MEMORY_BLOCK_SIZE;
     node = object_property_get_uint(OBJECT(drc->dev), PC_DIMM_NODE_PROP,
                                     &error_abort);
-    *fdt_start_offset = spapr_populate_memory_node(fdt, node, addr,
-                                                   SPAPR_MEMORY_BLOCK_SIZE);
+    *fdt_start_offset = spapr_dt_memory_node(fdt, node, addr,
+                                             SPAPR_MEMORY_BLOCK_SIZE);
     return 0;
 }
 
@@ -3802,7 +3798,7 @@ int spapr_core_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
     offset = fdt_add_subnode(fdt, 0, nodename);
     g_free(nodename);
 
-    spapr_populate_cpu_dt(cs, fdt, offset, spapr);
+    spapr_dt_cpu(cs, fdt, offset, spapr);
 
     *fdt_start_offset = offset;
     return 0;
diff --git a/hw/ppc/spapr_ovec.c b/hw/ppc/spapr_ovec.c
index 0ff6d1aeae..dd003f1763 100644
--- a/hw/ppc/spapr_ovec.c
+++ b/hw/ppc/spapr_ovec.c
@@ -200,8 +200,8 @@ SpaprOptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector)
     return ov;
 }
 
-int spapr_ovec_populate_dt(void *fdt, int fdt_offset,
-                           SpaprOptionVector *ov, const char *name)
+int spapr_dt_ovec(void *fdt, int fdt_offset,
+                  SpaprOptionVector *ov, const char *name)
 {
     uint8_t vec[OV_MAXBYTES + 1];
     uint16_t vec_len;
diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
index 2bed517a2b..d4dee9e06a 100644
--- a/include/hw/ppc/spapr_ovec.h
+++ b/include/hw/ppc/spapr_ovec.h
@@ -72,8 +72,8 @@ void spapr_ovec_set(SpaprOptionVector *ov, long bitnr);
 void spapr_ovec_clear(SpaprOptionVector *ov, long bitnr);
 bool spapr_ovec_test(SpaprOptionVector *ov, long bitnr);
 SpaprOptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector);
-int spapr_ovec_populate_dt(void *fdt, int fdt_offset,
-                           SpaprOptionVector *ov, const char *name);
+int spapr_dt_ovec(void *fdt, int fdt_offset,
+                  SpaprOptionVector *ov, const char *name);
 
 /* migration */
 extern const VMStateDescription vmstate_spapr_ovec;
-- 
2.24.1

Re: [PATCH 3/4] spapr: Rename DT functions to newer naming convention

[Cédric Le Goater]

On 3/13/20 5:05 AM, David Gibson wrote:
> In the spapr code we've been gradually moving towards a convention that
> functions which create pieces of the device tree are called spapr_dt_*().
> This patch speeds that along by renaming most of the things that don't yet
> match that so that they do.
> 
> For now we leave the *_dt_populate() functions which are actual methods
> used in the DRCClass::dt_populate method.
> 
> While we're there we remove a few comments that don't really say anything
> useful.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/ppc/spapr.c              | 62 +++++++++++++++++--------------------
>  hw/ppc/spapr_ovec.c         |  4 +--
>  include/hw/ppc/spapr_ovec.h |  4 +--
>  3 files changed, 33 insertions(+), 37 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index fc28d9df25..6c32ec3c0a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -217,10 +217,9 @@ static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, PowerPCCPU *cpu)
>                            sizeof(associativity));
>  }
>  
> -/* Populate the "ibm,pa-features" property */
> -static void spapr_populate_pa_features(SpaprMachineState *spapr,
> -                                       PowerPCCPU *cpu,
> -                                       void *fdt, int offset)
> +static void spapr_dt_pa_features(SpaprMachineState *spapr,
> +                                 PowerPCCPU *cpu,
> +                                 void *fdt, int offset)
>  {
>      uint8_t pa_features_206[] = { 6, 0,
>          0xf6, 0x1f, 0xc7, 0x00, 0x80, 0xc0 };
> @@ -315,8 +314,8 @@ static void add_str(GString *s, const gchar *s1)
>      g_string_append_len(s, s1, strlen(s1) + 1);
>  }
>  
> -static int spapr_populate_memory_node(void *fdt, int nodeid, hwaddr start,
> -                                       hwaddr size)
> +static int spapr_dt_memory_node(void *fdt, int nodeid, hwaddr start,
> +                                hwaddr size)
>  {
>      uint32_t associativity[] = {
>          cpu_to_be32(0x4), /* length */
> @@ -391,9 +390,8 @@ spapr_get_drconf_cell(uint32_t seq_lmbs, uint64_t base_addr,
>      return elem;
>  }
>  
> -/* ibm,dynamic-memory-v2 */
> -static int spapr_populate_drmem_v2(SpaprMachineState *spapr, void *fdt,
> -                                   int offset, MemoryDeviceInfoList *dimms)
> +static int spapr_dt_dynamic_memory_v2(SpaprMachineState *spapr, void *fdt,
> +                                      int offset, MemoryDeviceInfoList *dimms)
>  {
>      MachineState *machine = MACHINE(spapr);
>      uint8_t *int_buf, *cur_index;
> @@ -484,8 +482,7 @@ static int spapr_populate_drmem_v2(SpaprMachineState *spapr, void *fdt,
>      return 0;
>  }
>  
> -/* ibm,dynamic-memory */
> -static int spapr_populate_drmem_v1(SpaprMachineState *spapr, void *fdt,
> +static int spapr_dt_dynamic_memory(SpaprMachineState *spapr, void *fdt,
>                                     int offset, MemoryDeviceInfoList *dimms)
>  {
>      MachineState *machine = MACHINE(spapr);
> @@ -554,7 +551,8 @@ static int spapr_populate_drmem_v1(SpaprMachineState *spapr, void *fdt,
>   * Refer to docs/specs/ppc-spapr-hotplug.txt for the documentation
>   * of this device tree node.
>   */
> -static int spapr_populate_drconf_memory(SpaprMachineState *spapr, void *fdt)
> +static int spapr_dt_dynamic_reconfiguration_memory(SpaprMachineState *spapr,
> +                                                   void *fdt)
>  {
>      MachineState *machine = MACHINE(spapr);
>      int nb_numa_nodes = machine->numa_state->num_nodes;
> @@ -593,9 +591,9 @@ static int spapr_populate_drconf_memory(SpaprMachineState *spapr, void *fdt)
>      /* ibm,dynamic-memory or ibm,dynamic-memory-v2 */
>      dimms = qmp_memory_device_list();
>      if (spapr_ovec_test(spapr->ov5_cas, OV5_DRMEM_V2)) {
> -        ret = spapr_populate_drmem_v2(spapr, fdt, offset, dimms);
> +        ret = spapr_dt_dynamic_memory_v2(spapr, fdt, offset, dimms);
>      } else {
> -        ret = spapr_populate_drmem_v1(spapr, fdt, offset, dimms);
> +        ret = spapr_dt_dynamic_memory(spapr, fdt, offset, dimms);
>      }
>      qapi_free_MemoryDeviceInfoList(dimms);
>  
> @@ -626,7 +624,7 @@ static int spapr_populate_drconf_memory(SpaprMachineState *spapr, void *fdt)
>      return ret;
>  }
>  
> -static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
> +static int spapr_dt_memory(SpaprMachineState *spapr, void *fdt)
>  {
>      MachineState *machine = MACHINE(spapr);
>      SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> @@ -649,7 +647,7 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
>          if (!mem_start) {
>              /* spapr_machine_init() checks for rma_size <= node0_size
>               * already */
> -            spapr_populate_memory_node(fdt, i, 0, spapr->rma_size);
> +            spapr_dt_memory_node(fdt, i, 0, spapr->rma_size);
>              mem_start += spapr->rma_size;
>              node_size -= spapr->rma_size;
>          }
> @@ -661,7 +659,7 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
>                  sizetmp = 1ULL << ctzl(mem_start);
>              }
>  
> -            spapr_populate_memory_node(fdt, i, mem_start, sizetmp);
> +            spapr_dt_memory_node(fdt, i, mem_start, sizetmp);
>              node_size -= sizetmp;
>              mem_start += sizetmp;
>          }
> @@ -672,7 +670,7 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
>          int ret;
>  
>          g_assert(smc->dr_lmb_enabled);
> -        ret = spapr_populate_drconf_memory(spapr, fdt);
> +        ret = spapr_dt_dynamic_reconfiguration_memory(spapr, fdt);
>          if (ret) {
>              return ret;
>          }
> @@ -681,8 +679,8 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
>      return 0;
>  }
>  
> -static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
> -                                  SpaprMachineState *spapr)
> +static void spapr_dt_cpu(CPUState *cs, void *fdt, int offset,
> +                         SpaprMachineState *spapr)
>  {
>      MachineState *ms = MACHINE(spapr);
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> @@ -782,7 +780,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>                            page_sizes_prop, page_sizes_prop_size)));
>      }
>  
> -    spapr_populate_pa_features(spapr, cpu, fdt, offset);
> +    spapr_dt_pa_features(spapr, cpu, fdt, offset);
>  
>      _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
>                             cs->cpu_index / vcpus_per_socket)));
> @@ -816,7 +814,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>                                pcc->lrg_decr_bits)));
>  }
>  
> -static void spapr_populate_cpus_dt_node(void *fdt, SpaprMachineState *spapr)
> +static void spapr_dt_cpus(void *fdt, SpaprMachineState *spapr)
>  {
>      CPUState **rev;
>      CPUState *cs;
> @@ -860,13 +858,13 @@ static void spapr_populate_cpus_dt_node(void *fdt, SpaprMachineState *spapr)
>          offset = fdt_add_subnode(fdt, cpus_offset, nodename);
>          g_free(nodename);
>          _FDT(offset);
> -        spapr_populate_cpu_dt(cs, fdt, offset, spapr);
> +        spapr_dt_cpu(cs, fdt, offset, spapr);
>      }
>  
>      g_free(rev);
>  }
>  
> -static int spapr_rng_populate_dt(void *fdt)
> +static int spapr_dt_rng(void *fdt)
>  {
>      int node;
>      int ret;
> @@ -1099,8 +1097,7 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt)
>  
>      spapr_dt_ov5_platform_support(spapr, fdt, chosen);
>  
> -    _FDT(spapr_ovec_populate_dt(fdt, offset, spapr->ov5_cas,
> -                                "ibm,architecture-vec-5"));
> +    _FDT(spapr_dt_ovec(fdt, chosen, spapr->ov5_cas, "ibm,architecture-vec-5"));
>  
>      g_free(stdout_path);
>      g_free(bootlist);
> @@ -1181,7 +1178,7 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
>      /* /interrupt controller */
>      spapr_irq_dt(spapr, spapr_max_server_number(spapr), fdt, PHANDLE_INTC);
>  
> -    ret = spapr_populate_memory(spapr, fdt);
> +    ret = spapr_dt_memory(spapr, fdt);
>      if (ret < 0) {
>          error_report("couldn't setup memory nodes in fdt");
>          exit(1);
> @@ -1191,7 +1188,7 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
>      spapr_dt_vdevice(spapr->vio_bus, fdt);
>  
>      if (object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)) {
> -        ret = spapr_rng_populate_dt(fdt);
> +        ret = spapr_dt_rng(fdt);
>          if (ret < 0) {
>              error_report("could not set up rng device in the fdt");
>              exit(1);
> @@ -1206,8 +1203,7 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
>          }
>      }
>  
> -    /* cpus */
> -    spapr_populate_cpus_dt_node(fdt, spapr);
> +    spapr_dt_cpus(fdt, spapr);
>  
>      if (smc->dr_lmb_enabled) {
>          _FDT(spapr_dt_drc(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB));
> @@ -3400,8 +3396,8 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
>      addr = spapr_drc_index(drc) * SPAPR_MEMORY_BLOCK_SIZE;
>      node = object_property_get_uint(OBJECT(drc->dev), PC_DIMM_NODE_PROP,
>                                      &error_abort);
> -    *fdt_start_offset = spapr_populate_memory_node(fdt, node, addr,
> -                                                   SPAPR_MEMORY_BLOCK_SIZE);
> +    *fdt_start_offset = spapr_dt_memory_node(fdt, node, addr,
> +                                             SPAPR_MEMORY_BLOCK_SIZE);
>      return 0;
>  }
>  
> @@ -3802,7 +3798,7 @@ int spapr_core_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
>      offset = fdt_add_subnode(fdt, 0, nodename);
>      g_free(nodename);
>  
> -    spapr_populate_cpu_dt(cs, fdt, offset, spapr);
> +    spapr_dt_cpu(cs, fdt, offset, spapr);
>  
>      *fdt_start_offset = offset;
>      return 0;
> diff --git a/hw/ppc/spapr_ovec.c b/hw/ppc/spapr_ovec.c
> index 0ff6d1aeae..dd003f1763 100644
> --- a/hw/ppc/spapr_ovec.c
> +++ b/hw/ppc/spapr_ovec.c
> @@ -200,8 +200,8 @@ SpaprOptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector)
>      return ov;
>  }
>  
> -int spapr_ovec_populate_dt(void *fdt, int fdt_offset,
> -                           SpaprOptionVector *ov, const char *name)
> +int spapr_dt_ovec(void *fdt, int fdt_offset,
> +                  SpaprOptionVector *ov, const char *name)
>  {
>      uint8_t vec[OV_MAXBYTES + 1];
>      uint16_t vec_len;
> diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
> index 2bed517a2b..d4dee9e06a 100644
> --- a/include/hw/ppc/spapr_ovec.h
> +++ b/include/hw/ppc/spapr_ovec.h
> @@ -72,8 +72,8 @@ void spapr_ovec_set(SpaprOptionVector *ov, long bitnr);
>  void spapr_ovec_clear(SpaprOptionVector *ov, long bitnr);
>  bool spapr_ovec_test(SpaprOptionVector *ov, long bitnr);
>  SpaprOptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector);
> -int spapr_ovec_populate_dt(void *fdt, int fdt_offset,
> -                           SpaprOptionVector *ov, const char *name);
> +int spapr_dt_ovec(void *fdt, int fdt_offset,
> +                  SpaprOptionVector *ov, const char *name);
>  
>  /* migration */
>  extern const VMStateDescription vmstate_spapr_ovec;
> 

Re: [PATCH 3/4] spapr: Rename DT functions to newer naming convention

[Greg Kurz]

On Fri, 13 Mar 2020 15:05:38 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> In the spapr code we've been gradually moving towards a convention that
> functions which create pieces of the device tree are called spapr_dt_*().
> This patch speeds that along by renaming most of the things that don't yet
> match that so that they do.
> 
> For now we leave the *_dt_populate() functions which are actual methods
> used in the DRCClass::dt_populate method.
> 
> While we're there we remove a few comments that don't really say anything
> useful.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

$ git grep _populate hw/ppc/spapr* | grep -v drc
hw/ppc/spapr_ovec.c:            trace_spapr_ovec_populate_dt(i, vec_len, vec[i]);

This one needs fixing since spapr_ovec_populate_dt() is renamed.

hw/ppc/spapr_pci.c:    spapr_phb_nvgpu_populate_pcidev_dt(dev, fdt, offset, sphb);
hw/ppc/spapr_pci.c:    spapr_phb_nvgpu_populate_dt(phb, fdt, bus_off, &err);
hw/ppc/spapr_pci.c:    spapr_phb_nvgpu_ram_populate_dt(phb, fdt);
hw/ppc/spapr_pci_nvlink2.c:void spapr_phb_nvgpu_populate_dt(SpaprPhbState *sphb, void *fdt, int bus_off,
hw/ppc/spapr_pci_nvlink2.c:void spapr_phb_nvgpu_ram_populate_dt(SpaprPhbState *sphb, void *fdt)
hw/ppc/spapr_pci_nvlink2.c:void spapr_phb_nvgpu_populate_pcidev_dt(PCIDevice *dev, void *fdt, int offset,

These look like good candidates since they are not DRC methods.

>  hw/ppc/spapr.c              | 62 +++++++++++++++++--------------------
>  hw/ppc/spapr_ovec.c         |  4 +--
>  include/hw/ppc/spapr_ovec.h |  4 +--
>  3 files changed, 33 insertions(+), 37 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index fc28d9df25..6c32ec3c0a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -217,10 +217,9 @@ static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, PowerPCCPU *cpu)
>                            sizeof(associativity));
>  }
>  
> -/* Populate the "ibm,pa-features" property */
> -static void spapr_populate_pa_features(SpaprMachineState *spapr,
> -                                       PowerPCCPU *cpu,
> -                                       void *fdt, int offset)
> +static void spapr_dt_pa_features(SpaprMachineState *spapr,
> +                                 PowerPCCPU *cpu,
> +                                 void *fdt, int offset)
>  {
>      uint8_t pa_features_206[] = { 6, 0,
>          0xf6, 0x1f, 0xc7, 0x00, 0x80, 0xc0 };
> @@ -315,8 +314,8 @@ static void add_str(GString *s, const gchar *s1)
>      g_string_append_len(s, s1, strlen(s1) + 1);
>  }
>  
> -static int spapr_populate_memory_node(void *fdt, int nodeid, hwaddr start,
> -                                       hwaddr size)
> +static int spapr_dt_memory_node(void *fdt, int nodeid, hwaddr start,
> +                                hwaddr size)
>  {
>      uint32_t associativity[] = {
>          cpu_to_be32(0x4), /* length */
> @@ -391,9 +390,8 @@ spapr_get_drconf_cell(uint32_t seq_lmbs, uint64_t base_addr,
>      return elem;
>  }
>  
> -/* ibm,dynamic-memory-v2 */
> -static int spapr_populate_drmem_v2(SpaprMachineState *spapr, void *fdt,
> -                                   int offset, MemoryDeviceInfoList *dimms)
> +static int spapr_dt_dynamic_memory_v2(SpaprMachineState *spapr, void *fdt,
> +                                      int offset, MemoryDeviceInfoList *dimms)
>  {
>      MachineState *machine = MACHINE(spapr);
>      uint8_t *int_buf, *cur_index;
> @@ -484,8 +482,7 @@ static int spapr_populate_drmem_v2(SpaprMachineState *spapr, void *fdt,
>      return 0;
>  }
>  
> -/* ibm,dynamic-memory */
> -static int spapr_populate_drmem_v1(SpaprMachineState *spapr, void *fdt,
> +static int spapr_dt_dynamic_memory(SpaprMachineState *spapr, void *fdt,
>                                     int offset, MemoryDeviceInfoList *dimms)
>  {
>      MachineState *machine = MACHINE(spapr);
> @@ -554,7 +551,8 @@ static int spapr_populate_drmem_v1(SpaprMachineState *spapr, void *fdt,
>   * Refer to docs/specs/ppc-spapr-hotplug.txt for the documentation
>   * of this device tree node.
>   */
> -static int spapr_populate_drconf_memory(SpaprMachineState *spapr, void *fdt)
> +static int spapr_dt_dynamic_reconfiguration_memory(SpaprMachineState *spapr,
> +                                                   void *fdt)
>  {
>      MachineState *machine = MACHINE(spapr);
>      int nb_numa_nodes = machine->numa_state->num_nodes;
> @@ -593,9 +591,9 @@ static int spapr_populate_drconf_memory(SpaprMachineState *spapr, void *fdt)
>      /* ibm,dynamic-memory or ibm,dynamic-memory-v2 */
>      dimms = qmp_memory_device_list();
>      if (spapr_ovec_test(spapr->ov5_cas, OV5_DRMEM_V2)) {
> -        ret = spapr_populate_drmem_v2(spapr, fdt, offset, dimms);
> +        ret = spapr_dt_dynamic_memory_v2(spapr, fdt, offset, dimms);
>      } else {
> -        ret = spapr_populate_drmem_v1(spapr, fdt, offset, dimms);
> +        ret = spapr_dt_dynamic_memory(spapr, fdt, offset, dimms);
>      }
>      qapi_free_MemoryDeviceInfoList(dimms);
>  
> @@ -626,7 +624,7 @@ static int spapr_populate_drconf_memory(SpaprMachineState *spapr, void *fdt)
>      return ret;
>  }
>  
> -static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
> +static int spapr_dt_memory(SpaprMachineState *spapr, void *fdt)
>  {
>      MachineState *machine = MACHINE(spapr);
>      SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> @@ -649,7 +647,7 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
>          if (!mem_start) {
>              /* spapr_machine_init() checks for rma_size <= node0_size
>               * already */
> -            spapr_populate_memory_node(fdt, i, 0, spapr->rma_size);
> +            spapr_dt_memory_node(fdt, i, 0, spapr->rma_size);
>              mem_start += spapr->rma_size;
>              node_size -= spapr->rma_size;
>          }
> @@ -661,7 +659,7 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
>                  sizetmp = 1ULL << ctzl(mem_start);
>              }
>  
> -            spapr_populate_memory_node(fdt, i, mem_start, sizetmp);
> +            spapr_dt_memory_node(fdt, i, mem_start, sizetmp);
>              node_size -= sizetmp;
>              mem_start += sizetmp;
>          }
> @@ -672,7 +670,7 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
>          int ret;
>  
>          g_assert(smc->dr_lmb_enabled);
> -        ret = spapr_populate_drconf_memory(spapr, fdt);
> +        ret = spapr_dt_dynamic_reconfiguration_memory(spapr, fdt);
>          if (ret) {
>              return ret;
>          }
> @@ -681,8 +679,8 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
>      return 0;
>  }
>  
> -static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
> -                                  SpaprMachineState *spapr)
> +static void spapr_dt_cpu(CPUState *cs, void *fdt, int offset,
> +                         SpaprMachineState *spapr)
>  {
>      MachineState *ms = MACHINE(spapr);
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> @@ -782,7 +780,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>                            page_sizes_prop, page_sizes_prop_size)));
>      }
>  
> -    spapr_populate_pa_features(spapr, cpu, fdt, offset);
> +    spapr_dt_pa_features(spapr, cpu, fdt, offset);
>  
>      _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
>                             cs->cpu_index / vcpus_per_socket)));
> @@ -816,7 +814,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>                                pcc->lrg_decr_bits)));
>  }
>  
> -static void spapr_populate_cpus_dt_node(void *fdt, SpaprMachineState *spapr)
> +static void spapr_dt_cpus(void *fdt, SpaprMachineState *spapr)
>  {
>      CPUState **rev;
>      CPUState *cs;
> @@ -860,13 +858,13 @@ static void spapr_populate_cpus_dt_node(void *fdt, SpaprMachineState *spapr)
>          offset = fdt_add_subnode(fdt, cpus_offset, nodename);
>          g_free(nodename);
>          _FDT(offset);
> -        spapr_populate_cpu_dt(cs, fdt, offset, spapr);
> +        spapr_dt_cpu(cs, fdt, offset, spapr);
>      }
>  
>      g_free(rev);
>  }
>  
> -static int spapr_rng_populate_dt(void *fdt)
> +static int spapr_dt_rng(void *fdt)
>  {
>      int node;
>      int ret;
> @@ -1099,8 +1097,7 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt)
>  
>      spapr_dt_ov5_platform_support(spapr, fdt, chosen);
>  
> -    _FDT(spapr_ovec_populate_dt(fdt, offset, spapr->ov5_cas,
> -                                "ibm,architecture-vec-5"));
> +    _FDT(spapr_dt_ovec(fdt, chosen, spapr->ov5_cas, "ibm,architecture-vec-5"));
>  
>      g_free(stdout_path);
>      g_free(bootlist);
> @@ -1181,7 +1178,7 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
>      /* /interrupt controller */
>      spapr_irq_dt(spapr, spapr_max_server_number(spapr), fdt, PHANDLE_INTC);
>  
> -    ret = spapr_populate_memory(spapr, fdt);
> +    ret = spapr_dt_memory(spapr, fdt);
>      if (ret < 0) {
>          error_report("couldn't setup memory nodes in fdt");
>          exit(1);
> @@ -1191,7 +1188,7 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
>      spapr_dt_vdevice(spapr->vio_bus, fdt);
>  
>      if (object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)) {
> -        ret = spapr_rng_populate_dt(fdt);
> +        ret = spapr_dt_rng(fdt);
>          if (ret < 0) {
>              error_report("could not set up rng device in the fdt");
>              exit(1);
> @@ -1206,8 +1203,7 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
>          }
>      }
>  
> -    /* cpus */
> -    spapr_populate_cpus_dt_node(fdt, spapr);
> +    spapr_dt_cpus(fdt, spapr);
>  
>      if (smc->dr_lmb_enabled) {
>          _FDT(spapr_dt_drc(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB));
> @@ -3400,8 +3396,8 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
>      addr = spapr_drc_index(drc) * SPAPR_MEMORY_BLOCK_SIZE;
>      node = object_property_get_uint(OBJECT(drc->dev), PC_DIMM_NODE_PROP,
>                                      &error_abort);
> -    *fdt_start_offset = spapr_populate_memory_node(fdt, node, addr,
> -                                                   SPAPR_MEMORY_BLOCK_SIZE);
> +    *fdt_start_offset = spapr_dt_memory_node(fdt, node, addr,
> +                                             SPAPR_MEMORY_BLOCK_SIZE);
>      return 0;
>  }
>  
> @@ -3802,7 +3798,7 @@ int spapr_core_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
>      offset = fdt_add_subnode(fdt, 0, nodename);
>      g_free(nodename);
>  
> -    spapr_populate_cpu_dt(cs, fdt, offset, spapr);
> +    spapr_dt_cpu(cs, fdt, offset, spapr);
>  
>      *fdt_start_offset = offset;
>      return 0;
> diff --git a/hw/ppc/spapr_ovec.c b/hw/ppc/spapr_ovec.c
> index 0ff6d1aeae..dd003f1763 100644
> --- a/hw/ppc/spapr_ovec.c
> +++ b/hw/ppc/spapr_ovec.c
> @@ -200,8 +200,8 @@ SpaprOptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector)
>      return ov;
>  }
>  
> -int spapr_ovec_populate_dt(void *fdt, int fdt_offset,
> -                           SpaprOptionVector *ov, const char *name)
> +int spapr_dt_ovec(void *fdt, int fdt_offset,
> +                  SpaprOptionVector *ov, const char *name)
>  {
>      uint8_t vec[OV_MAXBYTES + 1];
>      uint16_t vec_len;
> diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
> index 2bed517a2b..d4dee9e06a 100644
> --- a/include/hw/ppc/spapr_ovec.h
> +++ b/include/hw/ppc/spapr_ovec.h
> @@ -72,8 +72,8 @@ void spapr_ovec_set(SpaprOptionVector *ov, long bitnr);
>  void spapr_ovec_clear(SpaprOptionVector *ov, long bitnr);
>  bool spapr_ovec_test(SpaprOptionVector *ov, long bitnr);
>  SpaprOptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector);
> -int spapr_ovec_populate_dt(void *fdt, int fdt_offset,
> -                           SpaprOptionVector *ov, const char *name);
> +int spapr_dt_ovec(void *fdt, int fdt_offset,
> +                  SpaprOptionVector *ov, const char *name);
>  
>  /* migration */
>  extern const VMStateDescription vmstate_spapr_ovec;

Re: [PATCH 3/4] spapr: Rename DT functions to newer naming convention

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 13469 bytes --]

On Fri, Mar 13, 2020 at 12:54:37PM +0100, Greg Kurz wrote:
> On Fri, 13 Mar 2020 15:05:38 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > In the spapr code we've been gradually moving towards a convention that
> > functions which create pieces of the device tree are called spapr_dt_*().
> > This patch speeds that along by renaming most of the things that don't yet
> > match that so that they do.
> > 
> > For now we leave the *_dt_populate() functions which are actual methods
> > used in the DRCClass::dt_populate method.
> > 
> > While we're there we remove a few comments that don't really say anything
> > useful.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> 
> $ git grep _populate hw/ppc/spapr* | grep -v drc
> hw/ppc/spapr_ovec.c:            trace_spapr_ovec_populate_dt(i, vec_len, vec[i]);
> 
> This one needs fixing since spapr_ovec_populate_dt() is renamed.
> 
> hw/ppc/spapr_pci.c:    spapr_phb_nvgpu_populate_pcidev_dt(dev, fdt, offset, sphb);
> hw/ppc/spapr_pci.c:    spapr_phb_nvgpu_populate_dt(phb, fdt, bus_off, &err);
> hw/ppc/spapr_pci.c:    spapr_phb_nvgpu_ram_populate_dt(phb, fdt);
> hw/ppc/spapr_pci_nvlink2.c:void spapr_phb_nvgpu_populate_dt(SpaprPhbState *sphb, void *fdt, int bus_off,
> hw/ppc/spapr_pci_nvlink2.c:void spapr_phb_nvgpu_ram_populate_dt(SpaprPhbState *sphb, void *fdt)
> hw/ppc/spapr_pci_nvlink2.c:void spapr_phb_nvgpu_populate_pcidev_dt(PCIDevice *dev, void *fdt, int offset,
> 
> These look like good candidates since they are not DRC methods.

Eh, true.  I don't really want to go through another review and test
cycle before my PR, so I think I'll merge as is and we can fix those
ones up later.

> 
> >  hw/ppc/spapr.c              | 62 +++++++++++++++++--------------------
> >  hw/ppc/spapr_ovec.c         |  4 +--
> >  include/hw/ppc/spapr_ovec.h |  4 +--
> >  3 files changed, 33 insertions(+), 37 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index fc28d9df25..6c32ec3c0a 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -217,10 +217,9 @@ static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, PowerPCCPU *cpu)
> >                            sizeof(associativity));
> >  }
> >  
> > -/* Populate the "ibm,pa-features" property */
> > -static void spapr_populate_pa_features(SpaprMachineState *spapr,
> > -                                       PowerPCCPU *cpu,
> > -                                       void *fdt, int offset)
> > +static void spapr_dt_pa_features(SpaprMachineState *spapr,
> > +                                 PowerPCCPU *cpu,
> > +                                 void *fdt, int offset)
> >  {
> >      uint8_t pa_features_206[] = { 6, 0,
> >          0xf6, 0x1f, 0xc7, 0x00, 0x80, 0xc0 };
> > @@ -315,8 +314,8 @@ static void add_str(GString *s, const gchar *s1)
> >      g_string_append_len(s, s1, strlen(s1) + 1);
> >  }
> >  
> > -static int spapr_populate_memory_node(void *fdt, int nodeid, hwaddr start,
> > -                                       hwaddr size)
> > +static int spapr_dt_memory_node(void *fdt, int nodeid, hwaddr start,
> > +                                hwaddr size)
> >  {
> >      uint32_t associativity[] = {
> >          cpu_to_be32(0x4), /* length */
> > @@ -391,9 +390,8 @@ spapr_get_drconf_cell(uint32_t seq_lmbs, uint64_t base_addr,
> >      return elem;
> >  }
> >  
> > -/* ibm,dynamic-memory-v2 */
> > -static int spapr_populate_drmem_v2(SpaprMachineState *spapr, void *fdt,
> > -                                   int offset, MemoryDeviceInfoList *dimms)
> > +static int spapr_dt_dynamic_memory_v2(SpaprMachineState *spapr, void *fdt,
> > +                                      int offset, MemoryDeviceInfoList *dimms)
> >  {
> >      MachineState *machine = MACHINE(spapr);
> >      uint8_t *int_buf, *cur_index;
> > @@ -484,8 +482,7 @@ static int spapr_populate_drmem_v2(SpaprMachineState *spapr, void *fdt,
> >      return 0;
> >  }
> >  
> > -/* ibm,dynamic-memory */
> > -static int spapr_populate_drmem_v1(SpaprMachineState *spapr, void *fdt,
> > +static int spapr_dt_dynamic_memory(SpaprMachineState *spapr, void *fdt,
> >                                     int offset, MemoryDeviceInfoList *dimms)
> >  {
> >      MachineState *machine = MACHINE(spapr);
> > @@ -554,7 +551,8 @@ static int spapr_populate_drmem_v1(SpaprMachineState *spapr, void *fdt,
> >   * Refer to docs/specs/ppc-spapr-hotplug.txt for the documentation
> >   * of this device tree node.
> >   */
> > -static int spapr_populate_drconf_memory(SpaprMachineState *spapr, void *fdt)
> > +static int spapr_dt_dynamic_reconfiguration_memory(SpaprMachineState *spapr,
> > +                                                   void *fdt)
> >  {
> >      MachineState *machine = MACHINE(spapr);
> >      int nb_numa_nodes = machine->numa_state->num_nodes;
> > @@ -593,9 +591,9 @@ static int spapr_populate_drconf_memory(SpaprMachineState *spapr, void *fdt)
> >      /* ibm,dynamic-memory or ibm,dynamic-memory-v2 */
> >      dimms = qmp_memory_device_list();
> >      if (spapr_ovec_test(spapr->ov5_cas, OV5_DRMEM_V2)) {
> > -        ret = spapr_populate_drmem_v2(spapr, fdt, offset, dimms);
> > +        ret = spapr_dt_dynamic_memory_v2(spapr, fdt, offset, dimms);
> >      } else {
> > -        ret = spapr_populate_drmem_v1(spapr, fdt, offset, dimms);
> > +        ret = spapr_dt_dynamic_memory(spapr, fdt, offset, dimms);
> >      }
> >      qapi_free_MemoryDeviceInfoList(dimms);
> >  
> > @@ -626,7 +624,7 @@ static int spapr_populate_drconf_memory(SpaprMachineState *spapr, void *fdt)
> >      return ret;
> >  }
> >  
> > -static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
> > +static int spapr_dt_memory(SpaprMachineState *spapr, void *fdt)
> >  {
> >      MachineState *machine = MACHINE(spapr);
> >      SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> > @@ -649,7 +647,7 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
> >          if (!mem_start) {
> >              /* spapr_machine_init() checks for rma_size <= node0_size
> >               * already */
> > -            spapr_populate_memory_node(fdt, i, 0, spapr->rma_size);
> > +            spapr_dt_memory_node(fdt, i, 0, spapr->rma_size);
> >              mem_start += spapr->rma_size;
> >              node_size -= spapr->rma_size;
> >          }
> > @@ -661,7 +659,7 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
> >                  sizetmp = 1ULL << ctzl(mem_start);
> >              }
> >  
> > -            spapr_populate_memory_node(fdt, i, mem_start, sizetmp);
> > +            spapr_dt_memory_node(fdt, i, mem_start, sizetmp);
> >              node_size -= sizetmp;
> >              mem_start += sizetmp;
> >          }
> > @@ -672,7 +670,7 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
> >          int ret;
> >  
> >          g_assert(smc->dr_lmb_enabled);
> > -        ret = spapr_populate_drconf_memory(spapr, fdt);
> > +        ret = spapr_dt_dynamic_reconfiguration_memory(spapr, fdt);
> >          if (ret) {
> >              return ret;
> >          }
> > @@ -681,8 +679,8 @@ static int spapr_populate_memory(SpaprMachineState *spapr, void *fdt)
> >      return 0;
> >  }
> >  
> > -static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
> > -                                  SpaprMachineState *spapr)
> > +static void spapr_dt_cpu(CPUState *cs, void *fdt, int offset,
> > +                         SpaprMachineState *spapr)
> >  {
> >      MachineState *ms = MACHINE(spapr);
> >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> > @@ -782,7 +780,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
> >                            page_sizes_prop, page_sizes_prop_size)));
> >      }
> >  
> > -    spapr_populate_pa_features(spapr, cpu, fdt, offset);
> > +    spapr_dt_pa_features(spapr, cpu, fdt, offset);
> >  
> >      _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
> >                             cs->cpu_index / vcpus_per_socket)));
> > @@ -816,7 +814,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
> >                                pcc->lrg_decr_bits)));
> >  }
> >  
> > -static void spapr_populate_cpus_dt_node(void *fdt, SpaprMachineState *spapr)
> > +static void spapr_dt_cpus(void *fdt, SpaprMachineState *spapr)
> >  {
> >      CPUState **rev;
> >      CPUState *cs;
> > @@ -860,13 +858,13 @@ static void spapr_populate_cpus_dt_node(void *fdt, SpaprMachineState *spapr)
> >          offset = fdt_add_subnode(fdt, cpus_offset, nodename);
> >          g_free(nodename);
> >          _FDT(offset);
> > -        spapr_populate_cpu_dt(cs, fdt, offset, spapr);
> > +        spapr_dt_cpu(cs, fdt, offset, spapr);
> >      }
> >  
> >      g_free(rev);
> >  }
> >  
> > -static int spapr_rng_populate_dt(void *fdt)
> > +static int spapr_dt_rng(void *fdt)
> >  {
> >      int node;
> >      int ret;
> > @@ -1099,8 +1097,7 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt)
> >  
> >      spapr_dt_ov5_platform_support(spapr, fdt, chosen);
> >  
> > -    _FDT(spapr_ovec_populate_dt(fdt, offset, spapr->ov5_cas,
> > -                                "ibm,architecture-vec-5"));
> > +    _FDT(spapr_dt_ovec(fdt, chosen, spapr->ov5_cas, "ibm,architecture-vec-5"));
> >  
> >      g_free(stdout_path);
> >      g_free(bootlist);
> > @@ -1181,7 +1178,7 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
> >      /* /interrupt controller */
> >      spapr_irq_dt(spapr, spapr_max_server_number(spapr), fdt, PHANDLE_INTC);
> >  
> > -    ret = spapr_populate_memory(spapr, fdt);
> > +    ret = spapr_dt_memory(spapr, fdt);
> >      if (ret < 0) {
> >          error_report("couldn't setup memory nodes in fdt");
> >          exit(1);
> > @@ -1191,7 +1188,7 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
> >      spapr_dt_vdevice(spapr->vio_bus, fdt);
> >  
> >      if (object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)) {
> > -        ret = spapr_rng_populate_dt(fdt);
> > +        ret = spapr_dt_rng(fdt);
> >          if (ret < 0) {
> >              error_report("could not set up rng device in the fdt");
> >              exit(1);
> > @@ -1206,8 +1203,7 @@ void *spapr_build_fdt(SpaprMachineState *spapr, bool reset, size_t space)
> >          }
> >      }
> >  
> > -    /* cpus */
> > -    spapr_populate_cpus_dt_node(fdt, spapr);
> > +    spapr_dt_cpus(fdt, spapr);
> >  
> >      if (smc->dr_lmb_enabled) {
> >          _FDT(spapr_dt_drc(fdt, 0, NULL, SPAPR_DR_CONNECTOR_TYPE_LMB));
> > @@ -3400,8 +3396,8 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
> >      addr = spapr_drc_index(drc) * SPAPR_MEMORY_BLOCK_SIZE;
> >      node = object_property_get_uint(OBJECT(drc->dev), PC_DIMM_NODE_PROP,
> >                                      &error_abort);
> > -    *fdt_start_offset = spapr_populate_memory_node(fdt, node, addr,
> > -                                                   SPAPR_MEMORY_BLOCK_SIZE);
> > +    *fdt_start_offset = spapr_dt_memory_node(fdt, node, addr,
> > +                                             SPAPR_MEMORY_BLOCK_SIZE);
> >      return 0;
> >  }
> >  
> > @@ -3802,7 +3798,7 @@ int spapr_core_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
> >      offset = fdt_add_subnode(fdt, 0, nodename);
> >      g_free(nodename);
> >  
> > -    spapr_populate_cpu_dt(cs, fdt, offset, spapr);
> > +    spapr_dt_cpu(cs, fdt, offset, spapr);
> >  
> >      *fdt_start_offset = offset;
> >      return 0;
> > diff --git a/hw/ppc/spapr_ovec.c b/hw/ppc/spapr_ovec.c
> > index 0ff6d1aeae..dd003f1763 100644
> > --- a/hw/ppc/spapr_ovec.c
> > +++ b/hw/ppc/spapr_ovec.c
> > @@ -200,8 +200,8 @@ SpaprOptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector)
> >      return ov;
> >  }
> >  
> > -int spapr_ovec_populate_dt(void *fdt, int fdt_offset,
> > -                           SpaprOptionVector *ov, const char *name)
> > +int spapr_dt_ovec(void *fdt, int fdt_offset,
> > +                  SpaprOptionVector *ov, const char *name)
> >  {
> >      uint8_t vec[OV_MAXBYTES + 1];
> >      uint16_t vec_len;
> > diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
> > index 2bed517a2b..d4dee9e06a 100644
> > --- a/include/hw/ppc/spapr_ovec.h
> > +++ b/include/hw/ppc/spapr_ovec.h
> > @@ -72,8 +72,8 @@ void spapr_ovec_set(SpaprOptionVector *ov, long bitnr);
> >  void spapr_ovec_clear(SpaprOptionVector *ov, long bitnr);
> >  bool spapr_ovec_test(SpaprOptionVector *ov, long bitnr);
> >  SpaprOptionVector *spapr_ovec_parse_vector(target_ulong table_addr, int vector);
> > -int spapr_ovec_populate_dt(void *fdt, int fdt_offset,
> > -                           SpaprOptionVector *ov, const char *name);
> > +int spapr_dt_ovec(void *fdt, int fdt_offset,
> > +                  SpaprOptionVector *ov, const char *name);
> >  
> >  /* migration */
> >  extern const VMStateDescription vmstate_spapr_ovec;
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 4/4] spapr: Fold spapr_node0_size() into its only caller

[David Gibson]

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 6c32ec3c0a..6a42c0f1c9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -295,20 +295,6 @@ static void spapr_dt_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);
@@ -2631,10 +2617,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) {
+                hwaddr node_size = machine->numa_state->nodes[i].node_mem;
+                rma_size = MIN(rma_size, pow2floor(node_size));
+                break;
+            }
+        }
+    }
 
     /*
      * VRMA access is via a special 1TiB SLB mapping, so the RMA can
@@ -2651,6 +2651,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)",
-- 
2.24.1

Re: [PATCH 4/4] spapr: Fold spapr_node0_size() into its only caller

[Greg Kurz]

On Fri, 13 Mar 2020 15:05:39 +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 6c32ec3c0a..6a42c0f1c9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -295,20 +295,6 @@ static void spapr_dt_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);
> @@ -2631,10 +2617,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) {
> +                hwaddr node_size = machine->numa_state->nodes[i].node_mem;
> +                rma_size = MIN(rma_size, pow2floor(node_size));
> +                break;
> +            }
> +        }
> +    }
>  
>      /*
>       * VRMA access is via a special 1TiB SLB mapping, so the RMA can
> @@ -2651,6 +2651,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);
> +

The patch is identical to the last spin, for which I had
a comment already:

-----------------------------------------------------------------------
On Wed, 4 Mar 2020 12:25:55 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Tue, Mar 03, 2020 at 11:32:49AM +0100, Greg Kurz wrote:

[...]

> > 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 still think that the rounding introduced by this patch deserves
some explanations in the changelog...

>      if (rma_size < MIN_RMA_SLOF) {
>          error_setg(errp,
>  "pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area memory)",

Re: [PATCH 4/4] spapr: Fold spapr_node0_size() into its only caller

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5116 bytes --]

On Fri, Mar 13, 2020 at 10:33:30AM +0100, Greg Kurz wrote:
> On Fri, 13 Mar 2020 15:05:39 +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 6c32ec3c0a..6a42c0f1c9 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -295,20 +295,6 @@ static void spapr_dt_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);
> > @@ -2631,10 +2617,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) {
> > +                hwaddr node_size = machine->numa_state->nodes[i].node_mem;
> > +                rma_size = MIN(rma_size, pow2floor(node_size));
> > +                break;
> > +            }
> > +        }
> > +    }
> >  
> >      /*
> >       * VRMA access is via a special 1TiB SLB mapping, so the RMA can
> > @@ -2651,6 +2651,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);
> > +
> 
> The patch is identical to the last spin, for which I had
> a comment already:

Ah, dangit.  It's actually not identical.  I put the pow2floor() back
in the node loop, but forgot to remove it from the bottom here.  I'll
put this one off for now.

> -----------------------------------------------------------------------
> On Wed, 4 Mar 2020 12:25:55 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Tue, Mar 03, 2020 at 11:32:49AM +0100, Greg Kurz wrote:
> 
> [...]
> 
> > > 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 still think that the rounding introduced by this patch deserves
> some explanations in the changelog...

Yeah.. turns out this is more complicated than you'd think.  It is
indeed related to that block splitting you noticed.  Except that
AFAICT that block splitting is irrelevant in most cases, and broken
for most of the remaining cases.  It seems to have been based on a
misunderstanding of how the kernel's early memory block handling
works.

My intention had been to make this patch do just the cleanup without
the rounding change, then address the rounding change another day.
Except I messed up the first part.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]