[Qemu-devel] [RFC PATCH v0 0/5] spapr-next: Memory hotplug updates

[Qemu-devel] [RFC PATCH v0 0/5] spapr-next: Memory hotplug updates

[Bharata B Rao]

This patchset includes some updates to sPAPR memory hotplug code that
currently resides in spapr-next branch of David Gibson's tree.

The main change here is to use drc-count hotplug identifier type for memory
hotplug. Currently we use drc-index type of hotplug identifier for
memory, which means that we send one EPOW event to guest on hotplug
of each LMB. Guest kernel has limited buffer space to hold these events
and we can easily overflow this buffer leading to missing of memory
hotplug events. After we switch to drc-count type of hotplug identifier,
we will send only one event to guest on hotplugging each pc-dimm device
thereby reducing the chances of guest rtas buffer overflow. This change
allows us to hotplug huge amounts of memory (>16G) in one pc-dimm
hotplug request.

Another change here is to not include information about memory specified
using -m option in ibm,drc-names, ibm,drc-indexes and
ibm,dynamic-memory-reconfiguration nodes thus having only truly DR capable
memory in these DT nodes/properties.

Bharata B Rao (5):
  spapr: Provide better error message when slots exceed max allowed
  spapr: Populate ibm,associativity-lookup-arrays correctly for non-NUMA
  spapr: Revert to memory@XXXX representation for non-hotplugged memory
  spapr: Support hotplug by specifying DRC count
  spapr: Move memory hotplug to RTAS_LOG_V6_HP_ID_DRC_COUNT type

 hw/ppc/spapr.c         | 58 +++++++++++++-------------------------------------
 hw/ppc/spapr_events.c  | 47 ++++++++++++++++++++++++++++++++--------
 hw/ppc/spapr_pci.c     |  4 ++--
 include/hw/ppc/spapr.h |  8 +++++--
 4 files changed, 61 insertions(+), 56 deletions(-)

-- 
2.1.0

[Qemu-devel] [RFC PATCH v0 1/5] spapr: Provide better error message when slots exceed max allowed

[Bharata B Rao]

Currently when user specifies more slots than allowed max of
SPAPR_MAX_RAM_SLOTS (32), we error out like this:

qemu-system-ppc64: unsupported amount of memory slots: 64

Let the user know about the max allowed slots like this:

qemu-system-ppc64: Specified number of memory slots 64 exceeds max supported 32

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6e24bf9..6d9cbd9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1777,8 +1777,8 @@ static void ppc_spapr_init(MachineState *machine)
         ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
 
         if (machine->ram_slots > SPAPR_MAX_RAM_SLOTS) {
-            error_report("unsupported amount of memory slots: %"PRIu64,
-                          machine->ram_slots);
+            error_report("Specified number of memory slots %"PRIu64" exceeds max supported %d\n",
+                         machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
             exit(EXIT_FAILURE);
         }
 
-- 
2.1.0

[Qemu-devel] [RFC PATCH v0 2/5] spapr: Populate ibm, associativity-lookup-arrays correctly for non-NUMA

[Bharata B Rao]

When NUMA isn't configured explicitly, assume node 0 is present for
the purpose of creating ibm,associativity-lookup-arrays property
under ibm,dynamic-reconfiguration-memory DT node. This ensures that
the associativity index property is correctly updated in ibm,dynamic-memory
for the LMB that is hotplugged.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6d9cbd9..7f94f97 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -728,6 +728,7 @@ static int spapr_populate_drconf_memory(sPAPRMachineState *spapr, void *fdt)
     uint32_t nr_lmbs = machine->maxram_size/lmb_size - nr_rma_lmbs;
     uint32_t nr_assigned_lmbs = machine->ram_size/lmb_size - nr_rma_lmbs;
     uint32_t *int_buf, *cur_index, buf_len;
+    int nr_nodes = nb_numa_nodes ? nb_numa_nodes : 1;
 
     /* Allocate enough buffer size to fit in ibm,dynamic-memory */
     buf_len = nr_lmbs * SPAPR_DR_LMB_LIST_ENTRY_SIZE * sizeof(uint32_t) +
@@ -793,10 +794,10 @@ static int spapr_populate_drconf_memory(sPAPRMachineState *spapr, void *fdt)
 
     /* ibm,associativity-lookup-arrays */
     cur_index = int_buf;
-    int_buf[0] = cpu_to_be32(nb_numa_nodes);
+    int_buf[0] = cpu_to_be32(nr_nodes);
     int_buf[1] = cpu_to_be32(4); /* Number of entries per associativity list */
     cur_index += 2;
-    for (i = 0; i < nb_numa_nodes; i++) {
+    for (i = 0; i < nr_nodes; i++) {
         uint32_t associativity[] = {
             cpu_to_be32(0x0),
             cpu_to_be32(0x0),
-- 
2.1.0

[Qemu-devel] [RFC PATCH v0 3/5] spapr: Revert to memory@XXXX representation for non-hotplugged memory

[Bharata B Rao]

Don't represent non-hotluggable memory under drconf node. With this
we don't have to create DRC objects for them.

The effect of this patch is that we revert back to memory@XXXX representation
for all the memory specified with -m option and represent the cold
plugged memory and hot-pluggable memory under
ibm,dynamic-reconfiguration-memory.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 47 +++++++++--------------------------------------
 1 file changed, 9 insertions(+), 38 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 7f94f97..669dc43 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -562,6 +562,7 @@ static int spapr_populate_memory(sPAPRMachineState *spapr, void *fdt)
         }
         if (!mem_start) {
             /* ppc_spapr_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;
         }
@@ -724,9 +725,7 @@ static int spapr_populate_drconf_memory(sPAPRMachineState *spapr, void *fdt)
     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 nr_rma_lmbs = spapr->rma_size/lmb_size;
-    uint32_t nr_lmbs = machine->maxram_size/lmb_size - nr_rma_lmbs;
-    uint32_t nr_assigned_lmbs = machine->ram_size/lmb_size - nr_rma_lmbs;
+    uint32_t nr_lmbs = (machine->maxram_size - machine->ram_size)/lmb_size;
     uint32_t *int_buf, *cur_index, buf_len;
     int nr_nodes = nb_numa_nodes ? nb_numa_nodes : 1;
 
@@ -759,15 +758,9 @@ static int spapr_populate_drconf_memory(sPAPRMachineState *spapr, void *fdt)
     for (i = 0; i < nr_lmbs; i++) {
         sPAPRDRConnector *drc;
         sPAPRDRConnectorClass *drck;
-        uint64_t addr;
+        uint64_t addr = i * lmb_size + spapr->hotplug_memory.base;;
         uint32_t *dynamic_memory = cur_index;
 
-        if (i < nr_assigned_lmbs) {
-            addr = (i + nr_rma_lmbs) * lmb_size;
-        } else {
-            addr = (i - nr_assigned_lmbs) * lmb_size +
-                    spapr->hotplug_memory.base;
-        }
         drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
                                        addr/lmb_size);
         g_assert(drc);
@@ -842,8 +835,6 @@ int spapr_h_cas_compose_response(sPAPRMachineState *spapr,
     /* Generate memory nodes or ibm,dynamic-reconfiguration-memory node */
     if (memory_update && smc->dr_lmb_enabled) {
         _FDT((spapr_populate_drconf_memory(spapr, fdt)));
-    } else {
-        _FDT((spapr_populate_memory(spapr, fdt)));
     }
 
     /* Pack resulting tree */
@@ -881,23 +872,10 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
     /* open out the base tree into a temp buffer for the final tweaks */
     _FDT((fdt_open_into(spapr->fdt_skel, fdt, FDT_MAX_SIZE)));
 
-    /*
-     * Add memory@0 node to represent RMA. Rest of the memory is either
-     * represented by memory nodes or ibm,dynamic-reconfiguration-memory
-     * node later during ibm,client-architecture-support call.
-     *
-     * If NUMA is configured, ensure that memory@0 ends up in the
-     * first memory-less node.
-     */
-    if (nb_numa_nodes) {
-        for (i = 0; i < nb_numa_nodes; ++i) {
-            if (numa_info[i].node_mem) {
-                spapr_populate_memory_node(fdt, i, 0, spapr->rma_size);
-                break;
-            }
-        }
-    } else {
-        spapr_populate_memory_node(fdt, 0, 0, spapr->rma_size);
+    ret = spapr_populate_memory(spapr, fdt);
+    if (ret < 0) {
+        fprintf(stderr, "couldn't setup memory nodes in fdt\n");
+        exit(1);
     }
 
     ret = spapr_populate_vdevice(spapr->vio_bus, fdt);
@@ -1604,21 +1582,14 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
 {
     MachineState *machine = MACHINE(spapr);
     uint64_t lmb_size = SPAPR_MEMORY_BLOCK_SIZE;
-    uint32_t nr_rma_lmbs = spapr->rma_size/lmb_size;
-    uint32_t nr_lmbs = machine->maxram_size/lmb_size - nr_rma_lmbs;
-    uint32_t nr_assigned_lmbs = machine->ram_size/lmb_size - nr_rma_lmbs;
+    uint32_t nr_lmbs = (machine->maxram_size - machine->ram_size)/lmb_size;
     int i;
 
     for (i = 0; i < nr_lmbs; i++) {
         sPAPRDRConnector *drc;
         uint64_t addr;
 
-        if (i < nr_assigned_lmbs) {
-            addr = (i + nr_rma_lmbs) * lmb_size;
-        } else {
-            addr = (i - nr_assigned_lmbs) * lmb_size +
-                    spapr->hotplug_memory.base;
-        }
+        addr = i * lmb_size + spapr->hotplug_memory.base;
         drc = spapr_dr_connector_new(OBJECT(spapr), SPAPR_DR_CONNECTOR_TYPE_LMB,
                                      addr/lmb_size);
         qemu_register_reset(spapr_drc_reset, drc);
-- 
2.1.0

[Qemu-devel] [RFC PATCH v0 4/5] spapr: Support hotplug by specifying DRC count

[Bharata B Rao]

Support hotplug identifier type RTAS_LOG_V6_HP_ID_DRC_COUNT that allows
hotplugging of DRCs by specifying the DRC count.

While we are here, rename

spapr_hotplug_req_add_event() to spapr_hotplug_req_add_by_index()
spapr_hotplug_req_remove_event() to spapr_hotplug_req_remove_by_index()

so that they match with spapr_hotplug_req_add_by_count().

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c         |  2 +-
 hw/ppc/spapr_events.c  | 47 ++++++++++++++++++++++++++++++++++++++---------
 hw/ppc/spapr_pci.c     |  4 ++--
 include/hw/ppc/spapr.h |  8 ++++++--
 4 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 669dc43..13af9be 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2072,7 +2072,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr, uint64_t size,
 
         drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
         drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
-        spapr_hotplug_req_add_event(drc);
+        spapr_hotplug_req_add_by_index(drc);
         addr += SPAPR_MEMORY_BLOCK_SIZE;
     }
 }
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 98bf7ae..744ea62 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -386,7 +386,9 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
     qemu_irq_pulse(xics_get_qirq(spapr->icp, spapr->check_exception_irq));
 }
 
-static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
+static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
+                                    sPAPRDRConnectorType drc_type,
+                                    uint32_t drc)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     struct hp_log_full *new_hp;
@@ -395,8 +397,6 @@ static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
     struct rtas_event_log_v6_maina *maina;
     struct rtas_event_log_v6_mainb *mainb;
     struct rtas_event_log_v6_hp *hp;
-    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-    sPAPRDRConnectorType drc_type = drck->get_type(drc);
 
     new_hp = g_malloc0(sizeof(struct hp_log_full));
     hdr = &new_hp->hdr;
@@ -427,8 +427,7 @@ static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
     hp->hdr.section_length = cpu_to_be16(sizeof(*hp));
     hp->hdr.section_version = 1; /* includes extended modifier */
     hp->hotplug_action = hp_action;
-    hp->drc.index = cpu_to_be32(drck->get_index(drc));
-    hp->hotplug_identifier = RTAS_LOG_V6_HP_ID_DRC_INDEX;
+    hp->hotplug_identifier = hp_id;
 
     switch (drc_type) {
     case SPAPR_DR_CONNECTOR_TYPE_PCI:
@@ -445,19 +444,49 @@ static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
         return;
     }
 
+    if (hp_id == RTAS_LOG_V6_HP_ID_DRC_COUNT) {
+        hp->drc.count = cpu_to_be32(drc);
+    } else if (hp_id == RTAS_LOG_V6_HP_ID_DRC_INDEX) {
+        hp->drc.index = cpu_to_be32(drc);
+    }
+
     rtas_event_log_queue(RTAS_LOG_TYPE_HOTPLUG, new_hp, true);
 
     qemu_irq_pulse(xics_get_qirq(spapr->icp, spapr->check_exception_irq));
 }
 
-void spapr_hotplug_req_add_event(sPAPRDRConnector *drc)
+void spapr_hotplug_req_add_by_index(sPAPRDRConnector *drc)
+{
+    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+    sPAPRDRConnectorType drc_type = drck->get_type(drc);
+    uint32 index = drck->get_index(drc);
+
+    spapr_hotplug_req_event(RTAS_LOG_V6_HP_ID_DRC_INDEX,
+                            RTAS_LOG_V6_HP_ACTION_ADD, drc_type, index);
+}
+
+void spapr_hotplug_req_remove_by_index(sPAPRDRConnector *drc)
+{
+    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+    sPAPRDRConnectorType drc_type = drck->get_type(drc);
+    uint32 index = drck->get_index(drc);
+
+    spapr_hotplug_req_event(RTAS_LOG_V6_HP_ID_DRC_INDEX,
+                            RTAS_LOG_V6_HP_ACTION_REMOVE, drc_type, index);
+}
+
+void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
+                                       uint32_t count)
 {
-    spapr_hotplug_req_event(drc, RTAS_LOG_V6_HP_ACTION_ADD);
+    spapr_hotplug_req_event(RTAS_LOG_V6_HP_ID_DRC_COUNT,
+                            RTAS_LOG_V6_HP_ACTION_ADD, drc_type, count);
 }
 
-void spapr_hotplug_req_remove_event(sPAPRDRConnector *drc)
+void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
+                                          uint32_t count)
 {
-    spapr_hotplug_req_event(drc, RTAS_LOG_V6_HP_ACTION_REMOVE);
+    spapr_hotplug_req_event(RTAS_LOG_V6_HP_ID_DRC_COUNT,
+                            RTAS_LOG_V6_HP_ACTION_REMOVE, drc_type, count);
 }
 
 static void check_exception(PowerPCCPU *cpu, sPAPRMachineState *spapr,
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index cfd3b7b..9d41060 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1179,7 +1179,7 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
         return;
     }
     if (plugged_dev->hotplugged) {
-        spapr_hotplug_req_add_event(drc);
+        spapr_hotplug_req_add_by_index(drc);
     }
 }
 
@@ -1207,7 +1207,7 @@ static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
             error_propagate(errp, local_err);
             return;
         }
-        spapr_hotplug_req_remove_event(drc);
+        spapr_hotplug_req_remove_by_index(drc);
     }
 }
 
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index b6cb0d0..d87c6d4 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -588,8 +588,12 @@ int spapr_dma_dt(void *fdt, int node_off, const char *propname,
 int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
                       sPAPRTCETable *tcet);
 void spapr_pci_switch_vga(bool big_endian);
-void spapr_hotplug_req_add_event(sPAPRDRConnector *drc);
-void spapr_hotplug_req_remove_event(sPAPRDRConnector *drc);
+void spapr_hotplug_req_add_by_index(sPAPRDRConnector *drc);
+void spapr_hotplug_req_remove_by_index(sPAPRDRConnector *drc);
+void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
+                                       uint32_t count);
+void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
+                                          uint32_t count);
 
 /* rtas-configure-connector state */
 struct sPAPRConfigureConnectorState {
-- 
2.1.0

[Qemu-devel] [RFC PATCH v0 5/5] spapr: Move memory hotplug to RTAS_LOG_V6_HP_ID_DRC_COUNT type

[Bharata B Rao]

Till now memory hotplug used RTAS_LOG_V6_HP_ID_DRC_INDEX hotplug type
which meant that we generated one hotplug type of EPOW event for every
256MB (SPAPR_MEMORY_BLOCK_SIZE). This quickly overruns the kernel
rtas log buffer thus resulting in loss of memory hotplug events. Switch
to RTAS_LOG_V6_HP_ID_DRC_COUNT hotplug type for memory so that we
generate only one event per hotplug request.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 13af9be..2426345 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2072,9 +2072,9 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr, uint64_t size,
 
         drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
         drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
-        spapr_hotplug_req_add_by_index(drc);
         addr += SPAPR_MEMORY_BLOCK_SIZE;
     }
+    spapr_hotplug_req_add_by_count(SPAPR_DR_CONNECTOR_TYPE_LMB, nr_lmbs);
 }
 
 static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
-- 
2.1.0

Re: [Qemu-devel] [RFC PATCH v0 1/5] spapr: Provide better error message when slots exceed max allowed

[David Gibson]

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

On Mon, Aug 03, 2015 at 11:05:39AM +0530, Bharata B Rao wrote:
> Currently when user specifies more slots than allowed max of
> SPAPR_MAX_RAM_SLOTS (32), we error out like this:
> 
> qemu-system-ppc64: unsupported amount of memory slots: 64
> 
> Let the user know about the max allowed slots like this:
> 
> qemu-system-ppc64: Specified number of memory slots 64 exceeds max supported 32
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

So, this is good as far as it goes.  There's another problem here
though.

At present even requesting 32 slots will not work properly, because
KVM only supports 32 slots and a couple of those are already used for
the non-hotplug RAM.  I think fixing this requires a kernel change to
advertise the available KVM memory slots.

> ---
>  hw/ppc/spapr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6e24bf9..6d9cbd9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1777,8 +1777,8 @@ static void ppc_spapr_init(MachineState *machine)
>          ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
>  
>          if (machine->ram_slots > SPAPR_MAX_RAM_SLOTS) {
> -            error_report("unsupported amount of memory slots: %"PRIu64,
> -                          machine->ram_slots);
> +            error_report("Specified number of memory slots %"PRIu64" exceeds max supported %d\n",
> +                         machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
>              exit(EXIT_FAILURE);
>          }
>  

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH v0 4/5] spapr: Support hotplug by specifying DRC count

[David Gibson]

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

On Mon, Aug 03, 2015 at 11:05:42AM +0530, Bharata B Rao wrote:
> Support hotplug identifier type RTAS_LOG_V6_HP_ID_DRC_COUNT that allows
> hotplugging of DRCs by specifying the DRC count.
> 
> While we are here, rename
> 
> spapr_hotplug_req_add_event() to spapr_hotplug_req_add_by_index()
> spapr_hotplug_req_remove_event() to spapr_hotplug_req_remove_by_index()
> 
> so that they match with spapr_hotplug_req_add_by_count().
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c         |  2 +-
>  hw/ppc/spapr_events.c  | 47 ++++++++++++++++++++++++++++++++++++++---------
>  hw/ppc/spapr_pci.c     |  4 ++--
>  include/hw/ppc/spapr.h |  8 ++++++--
>  4 files changed, 47 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 669dc43..13af9be 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2072,7 +2072,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr, uint64_t size,
>  
>          drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>          drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
> -        spapr_hotplug_req_add_event(drc);
> +        spapr_hotplug_req_add_by_index(drc);
>          addr += SPAPR_MEMORY_BLOCK_SIZE;
>      }
>  }
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 98bf7ae..744ea62 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -386,7 +386,9 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
>      qemu_irq_pulse(xics_get_qirq(spapr->icp, spapr->check_exception_irq));
>  }
>  
> -static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
> +static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
> +                                    sPAPRDRConnectorType drc_type,
> +                                    uint32_t drc)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      struct hp_log_full *new_hp;
> @@ -395,8 +397,6 @@ static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
>      struct rtas_event_log_v6_maina *maina;
>      struct rtas_event_log_v6_mainb *mainb;
>      struct rtas_event_log_v6_hp *hp;
> -    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -    sPAPRDRConnectorType drc_type = drck->get_type(drc);
>  
>      new_hp = g_malloc0(sizeof(struct hp_log_full));
>      hdr = &new_hp->hdr;
> @@ -427,8 +427,7 @@ static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
>      hp->hdr.section_length = cpu_to_be16(sizeof(*hp));
>      hp->hdr.section_version = 1; /* includes extended modifier */
>      hp->hotplug_action = hp_action;
> -    hp->drc.index = cpu_to_be32(drck->get_index(drc));
> -    hp->hotplug_identifier = RTAS_LOG_V6_HP_ID_DRC_INDEX;
> +    hp->hotplug_identifier = hp_id;
>  
>      switch (drc_type) {
>      case SPAPR_DR_CONNECTOR_TYPE_PCI:
> @@ -445,19 +444,49 @@ static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
>          return;
>      }
>  
> +    if (hp_id == RTAS_LOG_V6_HP_ID_DRC_COUNT) {
> +        hp->drc.count = cpu_to_be32(drc);

I'm a bit confused as to how this message can work with *only* a
count and not some sort of base index.

> +    } else if (hp_id == RTAS_LOG_V6_HP_ID_DRC_INDEX) {
> +        hp->drc.index = cpu_to_be32(drc);
> +    }
> +
>      rtas_event_log_queue(RTAS_LOG_TYPE_HOTPLUG, new_hp, true);
>  
>      qemu_irq_pulse(xics_get_qirq(spapr->icp, spapr->check_exception_irq));
>  }
>  
> -void spapr_hotplug_req_add_event(sPAPRDRConnector *drc)
> +void spapr_hotplug_req_add_by_index(sPAPRDRConnector *drc)
> +{
> +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +    sPAPRDRConnectorType drc_type = drck->get_type(drc);
> +    uint32 index = drck->get_index(drc);
> +
> +    spapr_hotplug_req_event(RTAS_LOG_V6_HP_ID_DRC_INDEX,
> +                            RTAS_LOG_V6_HP_ACTION_ADD, drc_type, index);
> +}
> +
> +void spapr_hotplug_req_remove_by_index(sPAPRDRConnector *drc)
> +{
> +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +    sPAPRDRConnectorType drc_type = drck->get_type(drc);
> +    uint32 index = drck->get_index(drc);
> +
> +    spapr_hotplug_req_event(RTAS_LOG_V6_HP_ID_DRC_INDEX,
> +                            RTAS_LOG_V6_HP_ACTION_REMOVE, drc_type, index);
> +}
> +
> +void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
> +                                       uint32_t count)
>  {
> -    spapr_hotplug_req_event(drc, RTAS_LOG_V6_HP_ACTION_ADD);
> +    spapr_hotplug_req_event(RTAS_LOG_V6_HP_ID_DRC_COUNT,
> +                            RTAS_LOG_V6_HP_ACTION_ADD, drc_type, count);
>  }
>  
> -void spapr_hotplug_req_remove_event(sPAPRDRConnector *drc)
> +void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
> +                                          uint32_t count)
>  {
> -    spapr_hotplug_req_event(drc, RTAS_LOG_V6_HP_ACTION_REMOVE);
> +    spapr_hotplug_req_event(RTAS_LOG_V6_HP_ID_DRC_COUNT,
> +                            RTAS_LOG_V6_HP_ACTION_REMOVE, drc_type, count);
>  }
>  
>  static void check_exception(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index cfd3b7b..9d41060 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1179,7 +1179,7 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
>          return;
>      }
>      if (plugged_dev->hotplugged) {
> -        spapr_hotplug_req_add_event(drc);
> +        spapr_hotplug_req_add_by_index(drc);
>      }
>  }
>  
> @@ -1207,7 +1207,7 @@ static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
>              error_propagate(errp, local_err);
>              return;
>          }
> -        spapr_hotplug_req_remove_event(drc);
> +        spapr_hotplug_req_remove_by_index(drc);
>      }
>  }
>  
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index b6cb0d0..d87c6d4 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -588,8 +588,12 @@ int spapr_dma_dt(void *fdt, int node_off, const char *propname,
>  int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
>                        sPAPRTCETable *tcet);
>  void spapr_pci_switch_vga(bool big_endian);
> -void spapr_hotplug_req_add_event(sPAPRDRConnector *drc);
> -void spapr_hotplug_req_remove_event(sPAPRDRConnector *drc);
> +void spapr_hotplug_req_add_by_index(sPAPRDRConnector *drc);
> +void spapr_hotplug_req_remove_by_index(sPAPRDRConnector *drc);
> +void spapr_hotplug_req_add_by_count(sPAPRDRConnectorType drc_type,
> +                                       uint32_t count);
> +void spapr_hotplug_req_remove_by_count(sPAPRDRConnectorType drc_type,
> +                                          uint32_t count);
>  
>  /* rtas-configure-connector state */
>  struct sPAPRConfigureConnectorState {

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH v0 4/5] spapr: Support hotplug by specifying DRC count

[Bharata B Rao]

On Mon, Aug 03, 2015 at 04:55:01PM +1000, David Gibson wrote:
> On Mon, Aug 03, 2015 at 11:05:42AM +0530, Bharata B Rao wrote:
> > Support hotplug identifier type RTAS_LOG_V6_HP_ID_DRC_COUNT that allows
> > hotplugging of DRCs by specifying the DRC count.
> > 
> > While we are here, rename
> > 
> > spapr_hotplug_req_add_event() to spapr_hotplug_req_add_by_index()
> > spapr_hotplug_req_remove_event() to spapr_hotplug_req_remove_by_index()
> > 
> > so that they match with spapr_hotplug_req_add_by_count().
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr.c         |  2 +-
> >  hw/ppc/spapr_events.c  | 47 ++++++++++++++++++++++++++++++++++++++---------
> >  hw/ppc/spapr_pci.c     |  4 ++--
> >  include/hw/ppc/spapr.h |  8 ++++++--
> >  4 files changed, 47 insertions(+), 14 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 669dc43..13af9be 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2072,7 +2072,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr, uint64_t size,
> >  
> >          drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> >          drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
> > -        spapr_hotplug_req_add_event(drc);
> > +        spapr_hotplug_req_add_by_index(drc);
> >          addr += SPAPR_MEMORY_BLOCK_SIZE;
> >      }
> >  }
> > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> > index 98bf7ae..744ea62 100644
> > --- a/hw/ppc/spapr_events.c
> > +++ b/hw/ppc/spapr_events.c
> > @@ -386,7 +386,9 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
> >      qemu_irq_pulse(xics_get_qirq(spapr->icp, spapr->check_exception_irq));
> >  }
> >  
> > -static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
> > +static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
> > +                                    sPAPRDRConnectorType drc_type,
> > +                                    uint32_t drc)
> >  {
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >      struct hp_log_full *new_hp;
> > @@ -395,8 +397,6 @@ static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
> >      struct rtas_event_log_v6_maina *maina;
> >      struct rtas_event_log_v6_mainb *mainb;
> >      struct rtas_event_log_v6_hp *hp;
> > -    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > -    sPAPRDRConnectorType drc_type = drck->get_type(drc);
> >  
> >      new_hp = g_malloc0(sizeof(struct hp_log_full));
> >      hdr = &new_hp->hdr;
> > @@ -427,8 +427,7 @@ static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
> >      hp->hdr.section_length = cpu_to_be16(sizeof(*hp));
> >      hp->hdr.section_version = 1; /* includes extended modifier */
> >      hp->hotplug_action = hp_action;
> > -    hp->drc.index = cpu_to_be32(drck->get_index(drc));
> > -    hp->hotplug_identifier = RTAS_LOG_V6_HP_ID_DRC_INDEX;
> > +    hp->hotplug_identifier = hp_id;
> >  
> >      switch (drc_type) {
> >      case SPAPR_DR_CONNECTOR_TYPE_PCI:
> > @@ -445,19 +444,49 @@ static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
> >          return;
> >      }
> >  
> > +    if (hp_id == RTAS_LOG_V6_HP_ID_DRC_COUNT) {
> > +        hp->drc.count = cpu_to_be32(drc);
> 
> I'm a bit confused as to how this message can work with *only* a
> count and not some sort of base index.

Right and this can be an issue when we start supporting memory removal
and the currrent userspace drmgr tool will just go ahead and remove
'count' number of LMBs that have been marked for removal and there is
really no association b/n these LMBs to the LMBs that make up the
pc-dimm device that is being removed.

One solution is to extend the rtas event header and include the base
drc-index (with count type of identifier)  so that we know exactly which
set of LMBs to remove for the given pc-dimm device.

Michael has some thoughts about alternative ways in thich we can achieve
removal of correct LMBs without needing to pass the base drc-index.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v0 4/5] spapr: Support hotplug by specifying DRC count

[Michael Roth]

Quoting Bharata B Rao (2015-08-03 02:53:02)
> On Mon, Aug 03, 2015 at 04:55:01PM +1000, David Gibson wrote:
> > On Mon, Aug 03, 2015 at 11:05:42AM +0530, Bharata B Rao wrote:
> > > Support hotplug identifier type RTAS_LOG_V6_HP_ID_DRC_COUNT that allows
> > > hotplugging of DRCs by specifying the DRC count.
> > > 
> > > While we are here, rename
> > > 
> > > spapr_hotplug_req_add_event() to spapr_hotplug_req_add_by_index()
> > > spapr_hotplug_req_remove_event() to spapr_hotplug_req_remove_by_index()
> > > 
> > > so that they match with spapr_hotplug_req_add_by_count().
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/spapr.c         |  2 +-
> > >  hw/ppc/spapr_events.c  | 47 ++++++++++++++++++++++++++++++++++++++---------
> > >  hw/ppc/spapr_pci.c     |  4 ++--
> > >  include/hw/ppc/spapr.h |  8 ++++++--
> > >  4 files changed, 47 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 669dc43..13af9be 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -2072,7 +2072,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr, uint64_t size,
> > >  
> > >          drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > >          drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
> > > -        spapr_hotplug_req_add_event(drc);
> > > +        spapr_hotplug_req_add_by_index(drc);
> > >          addr += SPAPR_MEMORY_BLOCK_SIZE;
> > >      }
> > >  }
> > > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> > > index 98bf7ae..744ea62 100644
> > > --- a/hw/ppc/spapr_events.c
> > > +++ b/hw/ppc/spapr_events.c
> > > @@ -386,7 +386,9 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
> > >      qemu_irq_pulse(xics_get_qirq(spapr->icp, spapr->check_exception_irq));
> > >  }
> > >  
> > > -static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
> > > +static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
> > > +                                    sPAPRDRConnectorType drc_type,
> > > +                                    uint32_t drc)
> > >  {
> > >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > >      struct hp_log_full *new_hp;
> > > @@ -395,8 +397,6 @@ static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
> > >      struct rtas_event_log_v6_maina *maina;
> > >      struct rtas_event_log_v6_mainb *mainb;
> > >      struct rtas_event_log_v6_hp *hp;
> > > -    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > -    sPAPRDRConnectorType drc_type = drck->get_type(drc);
> > >  
> > >      new_hp = g_malloc0(sizeof(struct hp_log_full));
> > >      hdr = &new_hp->hdr;
> > > @@ -427,8 +427,7 @@ static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
> > >      hp->hdr.section_length = cpu_to_be16(sizeof(*hp));
> > >      hp->hdr.section_version = 1; /* includes extended modifier */
> > >      hp->hotplug_action = hp_action;
> > > -    hp->drc.index = cpu_to_be32(drck->get_index(drc));
> > > -    hp->hotplug_identifier = RTAS_LOG_V6_HP_ID_DRC_INDEX;
> > > +    hp->hotplug_identifier = hp_id;
> > >  
> > >      switch (drc_type) {
> > >      case SPAPR_DR_CONNECTOR_TYPE_PCI:
> > > @@ -445,19 +444,49 @@ static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
> > >          return;
> > >      }
> > >  
> > > +    if (hp_id == RTAS_LOG_V6_HP_ID_DRC_COUNT) {
> > > +        hp->drc.count = cpu_to_be32(drc);
> > 
> > I'm a bit confused as to how this message can work with *only* a
> > count and not some sort of base index.

Talked a bit with Nathan for some more insight on *why* it works:

drc-count is actually how hotplug add/remove is done for pHyp cpu/mem
resources, drc-index is used in special situations like memory/cpu
failures, and for other drc types like pci.

So in general, drc-count with no drc-index is supposed to work fine
with existing guests/tools.

As far as the *how*:

guests just attempt to add LMBs until they fulfill the request, if
there's a failure they abort/rewind and try the next LMB.

There are some complications with QEMU though. Namely, our LMBs are
hotplugged in the form of DIMM-backed LMBs, as opposed to pHyp where
we hotplug from a logical pool of LMBs where there's no DIMM-level
modeling of resources. So, with both pHyp and pKVM, LMB hotplug/unplug
can fail for various reasons, but in the case of QEMU we have the
unique no-dimm-backing-this-lmb failure. We fail this at the DRC
level: if a guest attempts to set DRC/LMB allocation state to USED,
and QEMU decides there's no DIMM backing this memory, it simply reports
an error. This is in line with PAPR+. And due to the
try/abort/rewind/next handling this all still works.

Memory *unplug* is a little hairier:

All the above applies, but prior to getting to the point of QEMU failing
the DRC/LMB removal (via set-isolation-state:ISOLATED,
set-allocation-state:UNUSED returning failure), the guest kernel will
offline the LMB, migrating the memory if need be, so when the guest hits
the DRC failure it has to bring the LMB back online. This unecessary
churn and memory migration is obviously not desirable, but we've yet
to look into the performance aspects of it.

I think a drc-index being provided as a hint/starting point would be
the right way to address this potential performance issue, but I'm hoping
the approach in this series is sufficient for mem hotplug/unplug
for existing guests that wouldn't support such a hint.

This drc-index hint would likely be introduced in tandem with a new
ibm,client-set-architecture flag so QEMU can gracefully switch over for
newer guests. Nathan is working the PAPR+ spec for this in the process
of his work to move memory hotplug handling into guest kernel.

> 
> Right and this can be an issue when we start supporting memory removal
> and the currrent userspace drmgr tool will just go ahead and remove
> 'count' number of LMBs that have been marked for removal and there is
> really no association b/n these LMBs to the LMBs that make up the
> pc-dimm device that is being removed.
> 
> One solution is to extend the rtas event header and include the base
> drc-index (with count type of identifier)  so that we know exactly which
> set of LMBs to remove for the given pc-dimm device.
> 
> Michael has some thoughts about alternative ways in thich we can achieve
> removal of correct LMBs without needing to pass the base drc-index.
> 
> Regards,
> Bharata.
> 

Re: [Qemu-devel] [RFC PATCH v0 4/5] spapr: Support hotplug by specifying DRC count

[David Gibson]

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

On Mon, Aug 03, 2015 at 05:32:43PM -0500, Michael Roth wrote:
> Quoting Bharata B Rao (2015-08-03 02:53:02)
> > On Mon, Aug 03, 2015 at 04:55:01PM +1000, David Gibson wrote:
> > > On Mon, Aug 03, 2015 at 11:05:42AM +0530, Bharata B Rao wrote:
> > > > Support hotplug identifier type RTAS_LOG_V6_HP_ID_DRC_COUNT that allows
> > > > hotplugging of DRCs by specifying the DRC count.
> > > > 
> > > > While we are here, rename
> > > > 
> > > > spapr_hotplug_req_add_event() to spapr_hotplug_req_add_by_index()
> > > > spapr_hotplug_req_remove_event() to spapr_hotplug_req_remove_by_index()
> > > > 
> > > > so that they match with spapr_hotplug_req_add_by_count().
> > > > 
> > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > ---
> > > >  hw/ppc/spapr.c         |  2 +-
> > > >  hw/ppc/spapr_events.c  | 47 ++++++++++++++++++++++++++++++++++++++---------
> > > >  hw/ppc/spapr_pci.c     |  4 ++--
> > > >  include/hw/ppc/spapr.h |  8 ++++++--
> > > >  4 files changed, 47 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 669dc43..13af9be 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -2072,7 +2072,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr, uint64_t size,
> > > >  
> > > >          drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > >          drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
> > > > -        spapr_hotplug_req_add_event(drc);
> > > > +        spapr_hotplug_req_add_by_index(drc);
> > > >          addr += SPAPR_MEMORY_BLOCK_SIZE;
> > > >      }
> > > >  }
> > > > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> > > > index 98bf7ae..744ea62 100644
> > > > --- a/hw/ppc/spapr_events.c
> > > > +++ b/hw/ppc/spapr_events.c
> > > > @@ -386,7 +386,9 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
> > > >      qemu_irq_pulse(xics_get_qirq(spapr->icp, spapr->check_exception_irq));
> > > >  }
> > > >  
> > > > -static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
> > > > +static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
> > > > +                                    sPAPRDRConnectorType drc_type,
> > > > +                                    uint32_t drc)
> > > >  {
> > > >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > >      struct hp_log_full *new_hp;
> > > > @@ -395,8 +397,6 @@ static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
> > > >      struct rtas_event_log_v6_maina *maina;
> > > >      struct rtas_event_log_v6_mainb *mainb;
> > > >      struct rtas_event_log_v6_hp *hp;
> > > > -    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > > -    sPAPRDRConnectorType drc_type = drck->get_type(drc);
> > > >  
> > > >      new_hp = g_malloc0(sizeof(struct hp_log_full));
> > > >      hdr = &new_hp->hdr;
> > > > @@ -427,8 +427,7 @@ static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
> > > >      hp->hdr.section_length = cpu_to_be16(sizeof(*hp));
> > > >      hp->hdr.section_version = 1; /* includes extended modifier */
> > > >      hp->hotplug_action = hp_action;
> > > > -    hp->drc.index = cpu_to_be32(drck->get_index(drc));
> > > > -    hp->hotplug_identifier = RTAS_LOG_V6_HP_ID_DRC_INDEX;
> > > > +    hp->hotplug_identifier = hp_id;
> > > >  
> > > >      switch (drc_type) {
> > > >      case SPAPR_DR_CONNECTOR_TYPE_PCI:
> > > > @@ -445,19 +444,49 @@ static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
> > > >          return;
> > > >      }
> > > >  
> > > > +    if (hp_id == RTAS_LOG_V6_HP_ID_DRC_COUNT) {
> > > > +        hp->drc.count = cpu_to_be32(drc);
> > > 
> > > I'm a bit confused as to how this message can work with *only* a
> > > count and not some sort of base index.
> 
> Talked a bit with Nathan for some more insight on *why* it works:
> 
> drc-count is actually how hotplug add/remove is done for pHyp cpu/mem
> resources, drc-index is used in special situations like memory/cpu
> failures, and for other drc types like pci.
> 
> So in general, drc-count with no drc-index is supposed to work fine
> with existing guests/tools.
> 
> As far as the *how*:
> 
> guests just attempt to add LMBs until they fulfill the request, if
> there's a failure they abort/rewind and try the next LMB.

Huh, ok.

> There are some complications with QEMU though. Namely, our LMBs are
> hotplugged in the form of DIMM-backed LMBs, as opposed to pHyp where
> we hotplug from a logical pool of LMBs where there's no DIMM-level
> modeling of resources. So, with both pHyp and pKVM, LMB hotplug/unplug
> can fail for various reasons, but in the case of QEMU we have the
> unique no-dimm-backing-this-lmb failure. We fail this at the DRC
> level: if a guest attempts to set DRC/LMB allocation state to USED,
> and QEMU decides there's no DIMM backing this memory, it simply reports
> an error. This is in line with PAPR+. And due to the
> try/abort/rewind/next handling this all still works.
> 
> Memory *unplug* is a little hairier:
> 
> All the above applies, but prior to getting to the point of QEMU failing
> the DRC/LMB removal (via set-isolation-state:ISOLATED,
> set-allocation-state:UNUSED returning failure), the guest kernel will
> offline the LMB, migrating the memory if need be, so when the guest hits
> the DRC failure it has to bring the LMB back online. This unecessary
> churn and memory migration is obviously not desirable, but we've yet
> to look into the performance aspects of it.

Ah, and presumably this migrate shuffle could continue for a long
time, as it works its way though each LMB until reaching the ones
connected to the removed DIMM.

> I think a drc-index being provided as a hint/starting point would be
> the right way to address this potential performance issue, but I'm hoping
> the approach in this series is sufficient for mem hotplug/unplug
> for existing guests that wouldn't support such a hint.

I guess so.  Although it does make me wonder if using the PC derived
DIMM model was really what we wanted for PAPR guests.  I guess
implementing the phyp LMB-pool model probably would have required a
lot of messing with the core qemu hotplug model, though.

> This drc-index hint would likely be introduced in tandem with a new
> ibm,client-set-architecture flag so QEMU can gracefully switch over for
> newer guests. Nathan is working the PAPR+ spec for this in the process
> of his work to move memory hotplug handling into guest kernel.
> 
> > 
> > Right and this can be an issue when we start supporting memory removal
> > and the currrent userspace drmgr tool will just go ahead and remove
> > 'count' number of LMBs that have been marked for removal and there is
> > really no association b/n these LMBs to the LMBs that make up the
> > pc-dimm device that is being removed.
> > 
> > One solution is to extend the rtas event header and include the base
> > drc-index (with count type of identifier)  so that we know exactly which
> > set of LMBs to remove for the given pc-dimm device.
> > 
> > Michael has some thoughts about alternative ways in thich we can achieve
> > removal of correct LMBs without needing to pass the base drc-index.
> > 
> > Regards,
> > Bharata.
> > 
> 

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH v0 3/5] spapr: Revert to memory@XXXX representation for non-hotplugged memory

[Nathan Fontenot]

On 08/03/2015 12:35 AM, Bharata B Rao wrote:
> Don't represent non-hotluggable memory under drconf node. With this
> we don't have to create DRC objects for them.
> 
> The effect of this patch is that we revert back to memory@XXXX representation
> for all the memory specified with -m option and represent the cold
> plugged memory and hot-pluggable memory under
> ibm,dynamic-reconfiguration-memory.
>

I was looking through this and looking at the kernel code that inits memory
for power systems and I wanted to make sure this is really working and
you are seeing all the memory you expect to see in the guest.

Looking through the memory init code (powerpc/kerne/prom.c) it appears that
the additional memory@XXX would get initialized very early in boot, the same
time we currently init the memory@0 node. Then later in boot we would init
the rest of memory, lmbs in the dynamic-reconfiguration property.

Just wanting to make sure I'm understanding how this is working.

Also, since the memory specified in the memory@XXX nodes is not removable this
should not break any of the userspace tools.

-Nathan


 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c | 47 +++++++++--------------------------------------
>  1 file changed, 9 insertions(+), 38 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7f94f97..669dc43 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -562,6 +562,7 @@ static int spapr_populate_memory(sPAPRMachineState *spapr, void *fdt)
>          }
>          if (!mem_start) {
>              /* ppc_spapr_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;
>          }
> @@ -724,9 +725,7 @@ static int spapr_populate_drconf_memory(sPAPRMachineState *spapr, void *fdt)
>      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 nr_rma_lmbs = spapr->rma_size/lmb_size;
> -    uint32_t nr_lmbs = machine->maxram_size/lmb_size - nr_rma_lmbs;
> -    uint32_t nr_assigned_lmbs = machine->ram_size/lmb_size - nr_rma_lmbs;
> +    uint32_t nr_lmbs = (machine->maxram_size - machine->ram_size)/lmb_size;
>      uint32_t *int_buf, *cur_index, buf_len;
>      int nr_nodes = nb_numa_nodes ? nb_numa_nodes : 1;
> 
> @@ -759,15 +758,9 @@ static int spapr_populate_drconf_memory(sPAPRMachineState *spapr, void *fdt)
>      for (i = 0; i < nr_lmbs; i++) {
>          sPAPRDRConnector *drc;
>          sPAPRDRConnectorClass *drck;
> -        uint64_t addr;
> +        uint64_t addr = i * lmb_size + spapr->hotplug_memory.base;;
>          uint32_t *dynamic_memory = cur_index;
> 
> -        if (i < nr_assigned_lmbs) {
> -            addr = (i + nr_rma_lmbs) * lmb_size;
> -        } else {
> -            addr = (i - nr_assigned_lmbs) * lmb_size +
> -                    spapr->hotplug_memory.base;
> -        }
>          drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
>                                         addr/lmb_size);
>          g_assert(drc);
> @@ -842,8 +835,6 @@ int spapr_h_cas_compose_response(sPAPRMachineState *spapr,
>      /* Generate memory nodes or ibm,dynamic-reconfiguration-memory node */
>      if (memory_update && smc->dr_lmb_enabled) {
>          _FDT((spapr_populate_drconf_memory(spapr, fdt)));
> -    } else {
> -        _FDT((spapr_populate_memory(spapr, fdt)));
>      }
> 
>      /* Pack resulting tree */
> @@ -881,23 +872,10 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
>      /* open out the base tree into a temp buffer for the final tweaks */
>      _FDT((fdt_open_into(spapr->fdt_skel, fdt, FDT_MAX_SIZE)));
> 
> -    /*
> -     * Add memory@0 node to represent RMA. Rest of the memory is either
> -     * represented by memory nodes or ibm,dynamic-reconfiguration-memory
> -     * node later during ibm,client-architecture-support call.
> -     *
> -     * If NUMA is configured, ensure that memory@0 ends up in the
> -     * first memory-less node.
> -     */
> -    if (nb_numa_nodes) {
> -        for (i = 0; i < nb_numa_nodes; ++i) {
> -            if (numa_info[i].node_mem) {
> -                spapr_populate_memory_node(fdt, i, 0, spapr->rma_size);
> -                break;
> -            }
> -        }
> -    } else {
> -        spapr_populate_memory_node(fdt, 0, 0, spapr->rma_size);
> +    ret = spapr_populate_memory(spapr, fdt);
> +    if (ret < 0) {
> +        fprintf(stderr, "couldn't setup memory nodes in fdt\n");
> +        exit(1);
>      }
> 
>      ret = spapr_populate_vdevice(spapr->vio_bus, fdt);
> @@ -1604,21 +1582,14 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
>  {
>      MachineState *machine = MACHINE(spapr);
>      uint64_t lmb_size = SPAPR_MEMORY_BLOCK_SIZE;
> -    uint32_t nr_rma_lmbs = spapr->rma_size/lmb_size;
> -    uint32_t nr_lmbs = machine->maxram_size/lmb_size - nr_rma_lmbs;
> -    uint32_t nr_assigned_lmbs = machine->ram_size/lmb_size - nr_rma_lmbs;
> +    uint32_t nr_lmbs = (machine->maxram_size - machine->ram_size)/lmb_size;
>      int i;
> 
>      for (i = 0; i < nr_lmbs; i++) {
>          sPAPRDRConnector *drc;
>          uint64_t addr;
> 
> -        if (i < nr_assigned_lmbs) {
> -            addr = (i + nr_rma_lmbs) * lmb_size;
> -        } else {
> -            addr = (i - nr_assigned_lmbs) * lmb_size +
> -                    spapr->hotplug_memory.base;
> -        }
> +        addr = i * lmb_size + spapr->hotplug_memory.base;
>          drc = spapr_dr_connector_new(OBJECT(spapr), SPAPR_DR_CONNECTOR_TYPE_LMB,
>                                       addr/lmb_size);
>          qemu_register_reset(spapr_drc_reset, drc);
> 

Re: [Qemu-devel] [RFC PATCH v0 3/5] spapr: Revert to memory@XXXX representation for non-hotplugged memory

[Bharata B Rao]

On Tue, Aug 04, 2015 at 09:33:56AM -0500, Nathan Fontenot wrote:
> On 08/03/2015 12:35 AM, Bharata B Rao wrote:
> > Don't represent non-hotluggable memory under drconf node. With this
> > we don't have to create DRC objects for them.
> > 
> > The effect of this patch is that we revert back to memory@XXXX representation
> > for all the memory specified with -m option and represent the cold
> > plugged memory and hot-pluggable memory under
> > ibm,dynamic-reconfiguration-memory.
> >
> 
> I was looking through this and looking at the kernel code that inits memory
> for power systems and I wanted to make sure this is really working and
> you are seeing all the memory you expect to see in the guest.
> 
> Looking through the memory init code (powerpc/kerne/prom.c) it appears that
> the additional memory@XXX would get initialized very early in boot, the same
> time we currently init the memory@0 node. Then later in boot we would init
> the rest of memory, lmbs in the dynamic-reconfiguration property.
> 
> Just wanting to make sure I'm understanding how this is working.
> 
> Also, since the memory specified in the memory@XXX nodes is not removable this
> should not break any of the userspace tools.

Thanks for confirming this. Initially I went for only memory@0 and
rest of the memory as part of ibm,dynamic-reconfiguration-memory because
that's how it was in a couple of PowerVM boxes that I checked.

Regards,
Bharata.

Re: [Qemu-devel] [RFC PATCH v0 0/5] spapr-next: Memory hotplug updates

[David Gibson]

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

On Mon, Aug 03, 2015 at 11:05:38AM +0530, Bharata B Rao wrote:
> This patchset includes some updates to sPAPR memory hotplug code that
> currently resides in spapr-next branch of David Gibson's tree.
> 
> The main change here is to use drc-count hotplug identifier type for memory
> hotplug. Currently we use drc-index type of hotplug identifier for
> memory, which means that we send one EPOW event to guest on hotplug
> of each LMB. Guest kernel has limited buffer space to hold these events
> and we can easily overflow this buffer leading to missing of memory
> hotplug events. After we switch to drc-count type of hotplug identifier,
> we will send only one event to guest on hotplugging each pc-dimm device
> thereby reducing the chances of guest rtas buffer overflow. This change
> allows us to hotplug huge amounts of memory (>16G) in one pc-dimm
> hotplug request.
> 
> Another change here is to not include information about memory specified
> using -m option in ibm,drc-names, ibm,drc-indexes and
> ibm,dynamic-memory-reconfiguration nodes thus having only truly DR capable
> memory in these DT nodes/properties.
> 
> Bharata B Rao (5):
>   spapr: Provide better error message when slots exceed max allowed
>   spapr: Populate ibm,associativity-lookup-arrays correctly for non-NUMA
>   spapr: Revert to memory@XXXX representation for non-hotplugged memory
>   spapr: Support hotplug by specifying DRC count
>   spapr: Move memory hotplug to RTAS_LOG_V6_HP_ID_DRC_COUNT type
> 
>  hw/ppc/spapr.c         | 58 +++++++++++++-------------------------------------
>  hw/ppc/spapr_events.c  | 47 ++++++++++++++++++++++++++++++++--------
>  hw/ppc/spapr_pci.c     |  4 ++--
>  include/hw/ppc/spapr.h |  8 +++++--
>  4 files changed, 61 insertions(+), 56 deletions(-)

I've merged patches 1..3/5 into spapr-next.  4 & 5 I need to think
about a bit more.

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]