On Wed, Apr 11, 2018 at 02:45:58PM +1000, David Gibson wrote: > On Mon, Apr 09, 2018 at 11:55:38AM +0530, Bharata B Rao wrote: > > The new property ibm,dynamic-memory-v2 allows memory to be represented > > in a more compact manner in device tree. > > > > Signed-off-by: Bharata B Rao <bhar...@linux.vnet.ibm.com> > > --- > > v1: https://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg01788.html > > Changes in v1: > > - Minor cleanups in the error paths > > - Rebased on top of ppc-for-2.13 > > > > docs/specs/ppc-spapr-hotplug.txt | 19 +++ > > hw/ppc/spapr.c | 257 > > ++++++++++++++++++++++++++++++++------- > > include/hw/ppc/spapr.h | 1 + > > include/hw/ppc/spapr_ovec.h | 1 + > > 4 files changed, 233 insertions(+), 45 deletions(-) > > > > > diff --git a/docs/specs/ppc-spapr-hotplug.txt > > b/docs/specs/ppc-spapr-hotplug.txt > > index f57e2a09c6..cc7833108e 100644 > > --- a/docs/specs/ppc-spapr-hotplug.txt > > +++ b/docs/specs/ppc-spapr-hotplug.txt > > @@ -387,4 +387,23 @@ Each LMB list entry consists of the following elements: > > - A 32bit flags word. The bit at bit position 0x00000008 defines whether > > the LMB is assigned to the the partition as of boot time. > > > > +ibm,dynamic-memory-v2 > > + > > +This property describes the dynamically reconfigurable memory. This is > > +an alternate and newer way to describe dyanamically reconfigurable memory. > > +It is a property encoded array that has an integer N (the number of > > +LMB set entries) followed by N LMB set entries. There is an LMB set entry > > +for each sequential group of LMBs that share common attributes. > > + > > +Each LMB set entry consists of the following elements: > > + > > +- Number of sequential LMBs in the entry represented by a 32bit integer. > > +- Logical address of the first LMB in the set encoded as a 64bit integer. > > +- DRC index of the first LMB in the set. > > +- Associativity list index that is used as an index into > > + ibm,associativity-lookup-arrays property described earlier. This > > + is used to retrieve the right associativity list to be used for all > > + the LMBs in this set. > > +- A 32bit flags word that applies to all the LMBs in the set. > > + > > [1] > > http://thread.gmane.org/gmane.linux.ports.ppc.embedded/75350/focus=106867 > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 3ffadd6ac7..4a24fac38c 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -669,63 +669,139 @@ static uint32_t > > spapr_pc_dimm_node(MemoryDeviceInfoList *list, ram_addr_t addr) > > return -1; > > } > > > > -/* > > - * Adds ibm,dynamic-reconfiguration-memory node. > > - * 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) > > +struct of_drconf_cell_v2 { > > qemu convention is to use CamelCase for types.
Ok. > > > + uint32_t seq_lmbs; > > + uint64_t base_addr; > > + uint32_t drc_index; > > + uint32_t aa_index; > > + uint32_t flags; > > +} __attribute__((packed)); > > + > > +#define SPAPR_DRCONF_CELL_SIZE 6 > > Define this using a sizeof() for safety. Ok. > > > +/* ibm,dynamic-memory-v2 */ > > +static int spapr_populate_drmem_v2(sPAPRMachineState *spapr, void *fdt, > > + int offset, MemoryDeviceInfoList *dimms) > > { > > - MachineState *machine = MACHINE(spapr); > > - int ret, i, offset; > > - uint64_t lmb_size = SPAPR_MEMORY_BLOCK_SIZE; > > - uint32_t prop_lmb_size[] = {0, cpu_to_be32(lmb_size)}; > > - uint32_t hotplug_lmb_start = spapr->hotplug_memory.base / lmb_size; > > - uint32_t nr_lmbs = (spapr->hotplug_memory.base + > > - memory_region_size(&spapr->hotplug_memory.mr)) / > > - lmb_size; > > uint32_t *int_buf, *cur_index, buf_len; > > - int nr_nodes = nb_numa_nodes ? nb_numa_nodes : 1; > > - MemoryDeviceInfoList *dimms = NULL; > > + int ret; > > + uint64_t lmb_size = SPAPR_MEMORY_BLOCK_SIZE; > > + uint64_t addr, cur_addr, size; > > + uint32_t nr_boot_lmbs = (spapr->hotplug_memory.base / lmb_size); > > + uint64_t mem_end = spapr->hotplug_memory.base + > > + memory_region_size(&spapr->hotplug_memory.mr); > > + uint32_t node, nr_entries = 0; > > + sPAPRDRConnector *drc; > > + typedef struct drconf_cell_queue { > > + struct of_drconf_cell_v2 cell; > > + QSIMPLEQ_ENTRY(drconf_cell_queue) entry; > > + } drconf_cell_queue; > > Likewise CamelCase here. Ok. > > > + QSIMPLEQ_HEAD(, drconf_cell_queue) drconf_queue > > + = QSIMPLEQ_HEAD_INITIALIZER(drconf_queue); > > + drconf_cell_queue *elem, *next; > > + MemoryDeviceInfoList *info; > > > > - /* > > - * Don't create the node if there is no hotpluggable memory > > - */ > > - if (machine->ram_size == machine->maxram_size) { > > - return 0; > > - } > > + /* Entry to cover RAM and the gap area */ > > + elem = g_malloc0(sizeof(drconf_cell_queue)); > > Please use sizeof(*elem) - it's more robust in case you need to change > types around. Ok. > > > + elem->cell.seq_lmbs = cpu_to_be32(nr_boot_lmbs); > > + elem->cell.base_addr = cpu_to_be64(0); > > + elem->cell.drc_index = cpu_to_be32(0); > > + elem->cell.aa_index = cpu_to_be32(-1); > > + elem->cell.flags = cpu_to_be32(SPAPR_LMB_FLAGS_RESERVED | > > + SPAPR_LMB_FLAGS_DRC_INVALID); > > A helper for allocating, populating and queuing a new element might be > useful. I almost did that but required the list head to be global which I didn't like, but me give it another attempt. > > > + QSIMPLEQ_INSERT_TAIL(&drconf_queue, elem, entry); > > + nr_entries++; > > + > > + cur_addr = spapr->hotplug_memory.base; > > + for (info = dimms; info; info = info->next) { > > + PCDIMMDeviceInfo *di = info->value->u.dimm.data; > > + > > + addr = di->addr; > > + size = di->size; > > + node = di->node; > > + > > + /* Entry for hot-pluggable area */ > > + if (cur_addr < addr) { > > + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB, cur_addr / lmb_size); > > + g_assert(drc); > > > > - /* > > - * Allocate enough buffer size to fit in ibm,dynamic-memory > > - * or ibm,associativity-lookup-arrays > > - */ > > - buf_len = MAX(nr_lmbs * SPAPR_DR_LMB_LIST_ENTRY_SIZE + 1, nr_nodes * 4 > > + 2) > > - * sizeof(uint32_t); > > - cur_index = int_buf = g_malloc0(buf_len); > > + elem = g_malloc0(sizeof(drconf_cell_queue)); > > + elem->cell.seq_lmbs = cpu_to_be32((addr - cur_addr) / > > lmb_size); > > + elem->cell.base_addr = cpu_to_be64(cur_addr); > > + elem->cell.drc_index = cpu_to_be32(spapr_drc_index(drc)); > > + elem->cell.aa_index = cpu_to_be32(-1); > > + elem->cell.flags = cpu_to_be32(0); > > + QSIMPLEQ_INSERT_TAIL(&drconf_queue, elem, entry); > > + nr_entries++; > > + } > > > > - offset = fdt_add_subnode(fdt, 0, "ibm,dynamic-reconfiguration-memory"); > > + /* Entry for DIMM */ > > + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB, addr / lmb_size); > > + g_assert(drc); > > > > - ret = fdt_setprop(fdt, offset, "ibm,lmb-size", prop_lmb_size, > > - sizeof(prop_lmb_size)); > > - if (ret < 0) { > > - goto out; > > + elem = g_malloc0(sizeof(drconf_cell_queue)); > > + elem->cell.seq_lmbs = cpu_to_be32(size / lmb_size); > > + elem->cell.base_addr = cpu_to_be64(addr); > > + elem->cell.drc_index = cpu_to_be32(spapr_drc_index(drc)); > > + elem->cell.aa_index = cpu_to_be32(node); > > + elem->cell.flags = cpu_to_be32(SPAPR_LMB_FLAGS_ASSIGNED); > > + QSIMPLEQ_INSERT_TAIL(&drconf_queue, elem, entry); > > + nr_entries++; > > + cur_addr = addr + size; > > } > > > > - ret = fdt_setprop_cell(fdt, offset, "ibm,memory-flags-mask", 0xff); > > - if (ret < 0) { > > - goto out; > > + /* Entry for remaining hotpluggable area */ > > + if (cur_addr < mem_end) { > > + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB, cur_addr / lmb_size); > > + g_assert(drc); > > + > > + elem = g_malloc0(sizeof(drconf_cell_queue)); > > + elem->cell.seq_lmbs = cpu_to_be32((mem_end - cur_addr) / lmb_size); > > + elem->cell.base_addr = cpu_to_be64(cur_addr); > > + elem->cell.drc_index = cpu_to_be32(spapr_drc_index(drc)); > > + elem->cell.aa_index = cpu_to_be32(-1); > > + elem->cell.flags = cpu_to_be32(0); > > + QSIMPLEQ_INSERT_TAIL(&drconf_queue, elem, entry); > > + nr_entries++; > > } > > > > - ret = fdt_setprop_cell(fdt, offset, "ibm,memory-preservation-time", > > 0x0); > > - if (ret < 0) { > > - goto out; > > + buf_len = (nr_entries * SPAPR_DRCONF_CELL_SIZE + 1) * sizeof(uint32_t); > > + int_buf = cur_index = g_malloc0(buf_len); > > + int_buf[0] = cpu_to_be32(nr_entries); > > + cur_index++; > > + QSIMPLEQ_FOREACH_SAFE(elem, &drconf_queue, entry, next) { > > + memcpy(cur_index, &elem->cell, > > + SPAPR_DRCONF_CELL_SIZE * sizeof(uint32_t)); > > + cur_index += SPAPR_DRCONF_CELL_SIZE; > > + QSIMPLEQ_REMOVE(&drconf_queue, elem, drconf_cell_queue, entry); > > + g_free(elem); > > } > > > > - if (hotplug_lmb_start) { > > - dimms = qmp_pc_dimm_device_list(); > > + ret = fdt_setprop(fdt, offset, "ibm,dynamic-memory-v2", int_buf, > > buf_len); > > + g_free(int_buf); > > + if (ret < 0) { > > + return -1; > > } > > + return 0; > > +} > > > > - /* ibm,dynamic-memory */ > > +/* ibm,dynamic-memory */ > > +static int spapr_populate_drmem_v1(sPAPRMachineState *spapr, void *fdt, > > + int offset, MemoryDeviceInfoList *dimms) > > +{ > > + int i, ret; > > + uint64_t lmb_size = SPAPR_MEMORY_BLOCK_SIZE; > > + uint32_t hotplug_lmb_start = spapr->hotplug_memory.base / lmb_size; > > + uint32_t nr_lmbs = (spapr->hotplug_memory.base + > > + memory_region_size(&spapr->hotplug_memory.mr)) / > > + lmb_size; > > + uint32_t *int_buf, *cur_index, buf_len; > > + > > + /* > > + * Allocate enough buffer size to fit in ibm,dynamic-memory > > + */ > > + buf_len = (nr_lmbs * SPAPR_DR_LMB_LIST_ENTRY_SIZE + 1) * > > sizeof(uint32_t); > > + cur_index = int_buf = g_malloc0(buf_len); > > int_buf[0] = cpu_to_be32(nr_lmbs); > > cur_index++; > > for (i = 0; i < nr_lmbs; i++) { > > @@ -765,13 +841,71 @@ static int > > spapr_populate_drconf_memory(sPAPRMachineState *spapr, void *fdt) > > > > cur_index += SPAPR_DR_LMB_LIST_ENTRY_SIZE; > > } > > - qapi_free_MemoryDeviceInfoList(dimms); > > ret = fdt_setprop(fdt, offset, "ibm,dynamic-memory", int_buf, buf_len); > > + g_free(int_buf); > > + if (ret < 0) { > > + return -1; > > + } > > + return 0; > > +} > > + > > +/* > > + * Adds ibm,dynamic-reconfiguration-memory node. > > + * 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) > > +{ > > + MachineState *machine = MACHINE(spapr); > > + int ret, i, offset; > > + uint64_t lmb_size = SPAPR_MEMORY_BLOCK_SIZE; > > + uint32_t prop_lmb_size[] = {0, cpu_to_be32(lmb_size)}; > > + uint32_t *int_buf, *cur_index, buf_len; > > + int nr_nodes = nb_numa_nodes ? nb_numa_nodes : 1; > > + MemoryDeviceInfoList *dimms = NULL; > > + > > + /* > > + * Don't create the node if there is no hotpluggable memory > > + */ > > + if (machine->ram_size == machine->maxram_size) { > > + return 0; > > + } > > + > > + offset = fdt_add_subnode(fdt, 0, "ibm,dynamic-reconfiguration-memory"); > > + > > + ret = fdt_setprop(fdt, offset, "ibm,lmb-size", prop_lmb_size, > > + sizeof(prop_lmb_size)); > > + if (ret < 0) { > > + goto out; > > + } > > + > > + ret = fdt_setprop_cell(fdt, offset, "ibm,memory-flags-mask", 0xff); > > + if (ret < 0) { > > + goto out; > > + } > > + > > + ret = fdt_setprop_cell(fdt, offset, "ibm,memory-preservation-time", > > 0x0); > > + if (ret < 0) { > > + goto out; > > + } > > + > > + /* ibm,dynamic-memory or ibm,dynamic-memory-v2 */ > > + dimms = qmp_pc_dimm_device_list(); > > + if (spapr_ovec_test(spapr->ov5_cas, OV5_DRMEM_V2)) { > > + ret = spapr_populate_drmem_v2(spapr, fdt, offset, dimms); > > + } else { > > + ret = spapr_populate_drmem_v1(spapr, fdt, offset, dimms); > > + } > > + qapi_free_MemoryDeviceInfoList(dimms); > > + > > if (ret < 0) { > > goto out; > > } > > > > /* ibm,associativity-lookup-arrays */ > > + buf_len = (nr_nodes * 4 + 2) * sizeof(uint32_t); > > + cur_index = int_buf = g_malloc0(buf_len); > > + > > cur_index = int_buf; > > int_buf[0] = cpu_to_be32(nr_nodes); > > int_buf[1] = cpu_to_be32(4); /* Number of entries per associativity > > list */ > > @@ -788,8 +922,9 @@ static int > > spapr_populate_drconf_memory(sPAPRMachineState *spapr, void *fdt) > > } > > ret = fdt_setprop(fdt, offset, "ibm,associativity-lookup-arrays", > > int_buf, > > (cur_index - int_buf) * sizeof(uint32_t)); > > -out: > > g_free(int_buf); > > + > > +out: > > No point to this label if all you do after is a plain return - you can > just use return directly instead of gotos. Yes, cleaned up error path which made this label pointless, but missed removing it. > > > return ret; > > } > > > > @@ -2500,6 +2635,11 @@ static void spapr_machine_init(MachineState *machine) > > spapr_ovec_set(spapr->ov5, OV5_HPT_RESIZE); > > } > > > > + /* advertise support for ibm,dyamic-memory-v2 */ > > + if (spapr->use_ibm_dynamic_memory_v2) { > > + spapr_ovec_set(spapr->ov5, OV5_DRMEM_V2); > > + } > > + > > /* init CPUs */ > > spapr_init_cpus(spapr); > > > > @@ -2907,12 +3047,27 @@ static void spapr_set_vsmt(Object *obj, Visitor *v, > > const char *name, > > visit_type_uint32(v, name, (uint32_t *)opaque, errp); > > } > > > > +static bool spapr_get_drmem_v2(Object *obj, Error **errp) > > +{ > > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > + > > + return spapr->use_ibm_dynamic_memory_v2; > > +} > > + > > +static void spapr_set_drmem_v2(Object *obj, bool value, Error **errp) > > +{ > > + sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > + > > + spapr->use_ibm_dynamic_memory_v2 = value; > > +} > > + > > static void spapr_instance_init(Object *obj) > > { > > sPAPRMachineState *spapr = SPAPR_MACHINE(obj); > > > > spapr->htab_fd = -1; > > spapr->use_hotplug_event_source = true; > > + spapr->use_ibm_dynamic_memory_v2 = true; > > object_property_add_str(obj, "kvm-type", > > spapr_get_kvm_type, spapr_set_kvm_type, NULL); > > object_property_set_description(obj, "kvm-type", > > @@ -2927,6 +3082,15 @@ static void spapr_instance_init(Object *obj) > > " place of standard EPOW events when > > possible" > > " (required for memory hot-unplug > > support)", > > NULL); > > + object_property_add_bool(obj, "drmem-v2", > > + spapr_get_drmem_v2, > > + spapr_set_drmem_v2, > > + NULL); > > + object_property_set_description(obj, "ibm-dynamic-memory-v2", > > + "Use ibm-dynamic-memory-v2 > > representation" > > + " in place of ibm-dynamic-memory when" > > + " possible", > > + NULL); > > I don't really see any point to making this a user configurable > option. Why not just always enable it if the guest says it can > support it. Makes sense. While thinking about it, I wonder why other properties like modern-hotplug-events remain user configurable. Thanks for the review, will post next version with your suggested changes. Regards, Bharata.