Skip to contentSkip to conversation

spapr: machine hotplug handler cleanups

PATCH Applied David Hildenbrand 3 participants 10 messages Jun 8, 2018 – Jun 9, 2018

lore
Share & tools
lore

David Hildenbrand

author
#0 lore raw

v2 cover spapr: machine hotplug handler cleanups

I'll be messing with machine hotplug handlers of pc/spapr/s390x in the context of
[PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers

So this is a spin-off of the cleanup patches in the context of hotplug handlers.

v1 -> v2:
- dropped the three "local_err" patches
- tweaked some patch descriptions
- Split "spapr: move all DIMM checks into spapr_memory_plug" up -- Move the memory hotplug check into the pre_plug handler
- Use &error_abort instead of NULL

David Hildenbrand (6):
spapr: no need to verify the node
spapr: move lookup of the node into spapr_memory_plug()
spapr: move memory hotplug support check into spapr_memory_pre_plug()
spapr: introduce machine unplug handler
spapr: handle pc-dimm unplug via hotplug handler chain
spapr: handle cpu core unplug via hotplug handler chain

 hw/ppc/spapr.c | 67 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 24 deletions(-)
Signature · 1 line
2.17.0
author
#1 lore raw

PATCH v2 1/6 spapr: no need to verify the node

The node property can always be queried and the value has already been verified in pc_dimm_realize().

Acked-by David Gibson <[email protected]>
Reviewed-by Greg Kurz <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
---
 hw/ppc/spapr.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)
hw/ppc/spapr.c +2 -8
index 2375cbee12..f16a0b2870 100644
@@ -3578,14 +3578,8 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
             error_setg(errp, "Memory hotplug not supported for this machine");
             return;
         }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp);
-        if (*errp) {
-            return;
-        }
-        if (node < 0 || node >= MAX_NODES) {
-            error_setg(errp, "Invaild node %d", node);
-            return;
-        }
+        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP,
+                                        &error_abort);
 
         spapr_memory_plug(hotplug_dev, dev, node, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
Signature · 1 line
2.17.0
author
#2 lore raw

PATCH v2 2/6 spapr: move lookup of the node into spapr_memory_plug()

In reply to David Hildenbrand

Let's clean the hotplug handler up by moving lookup of the node into the function where it is actually being used.

Signed-off-by David Hildenbrand <[email protected]>
---
 hw/ppc/spapr.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
hw/ppc/spapr.c +5 -7
index f16a0b2870..1f577b274b 100644
@@ -3136,7 +3136,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
 }
 
 static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
-                              uint32_t node, Error **errp)
+                              Error **errp)
 {
     Error *local_err = NULL;
     sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);

@@ -3144,6 +3144,7 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *mr;
     uint64_t align, size, addr;
+    uint32_t node;
 
     mr = ddc->get_memory_region(dimm, &local_err);
     if (local_err) {

@@ -3163,6 +3164,8 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         goto out_unplug;
     }
 
+    node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP,
+                                    &error_abort);
     spapr_add_lmbs(dev, addr, size, node,
                    spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
                    &local_err);

@@ -3572,16 +3575,11 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        int node;
-
         if (!smc->dr_lmb_enabled) {
             error_setg(errp, "Memory hotplug not supported for this machine");
             return;
         }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP,
-                                        &error_abort);
-
-        spapr_memory_plug(hotplug_dev, dev, node, errp);
+        spapr_memory_plug(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
         spapr_core_plug(hotplug_dev, dev, errp);
     }
Signature · 1 line
2.17.0
Reviewed-by on v2 2/6
#3 lore raw

Re: [Qemu-devel] [PATCH v2 2/6] spapr: move lookup of the node into spapr_memory_plug()

Quoted 5 lines · On Fri, 8 Jun 2018 14:48:12 +0200 David Hildenbrand <[email protected]> wrote:

Let's clean the hotplug handler up by moving lookup of the node into the function where it is actually being used.

Signed-off-by David Hildenbrand <[email protected]>

---

Reviewed-by Greg Kurz <[email protected]>
Quoted 51 lines
 hw/ppc/spapr.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
hw/ppc/spapr.c +5 -7
index f16a0b2870..1f577b274b 100644
@@ -3136,7 +3136,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
 }
 
 static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
-                              uint32_t node, Error **errp)
+                              Error **errp)
 {
     Error *local_err = NULL;
     sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);

@@ -3144,6 +3144,7 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *mr;
     uint64_t align, size, addr;
+    uint32_t node;
 
     mr = ddc->get_memory_region(dimm, &local_err);
     if (local_err) {

@@ -3163,6 +3164,8 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         goto out_unplug;
     }
 
+    node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP,
+                                    &error_abort);
     spapr_add_lmbs(dev, addr, size, node,
                    spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
                    &local_err);

@@ -3572,16 +3575,11 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        int node;
-
         if (!smc->dr_lmb_enabled) {
             error_setg(errp, "Memory hotplug not supported for this machine");
             return;
         }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP,
-                                        &error_abort);
-
-        spapr_memory_plug(hotplug_dev, dev, node, errp);
+        spapr_memory_plug(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
         spapr_core_plug(hotplug_dev, dev, errp);
     }
author
#4 lore raw

PATCH v2 3/6 spapr: move memory hotplug support check into spapr_memory_pre_plug()

In reply to David Hildenbrand

Let's finish cleaning up the hotplug handler. This check can be performed in the pre_plug code as the very first thing.

Signed-off-by David Hildenbrand <[email protected]>
---
 hw/ppc/spapr.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)
hw/ppc/spapr.c +6 -7
index 1f577b274b..9b8b4068b1 100644
@@ -3184,12 +3184,18 @@ out:
 static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                   Error **errp)
 {
+    const sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *mr;
     uint64_t size;
     char *mem_dev;
 
+    if (!smc->dr_lmb_enabled) {
+        error_setg(errp, "Memory hotplug not supported for this machine");
+        return;
+    }
+
     mr = ddc->get_memory_region(dimm, errp);
     if (!mr) {
         return;

@@ -3571,14 +3577,7 @@ out:
 static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
-    MachineState *ms = MACHINE(hotplug_dev);
-    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
-
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        if (!smc->dr_lmb_enabled) {
-            error_setg(errp, "Memory hotplug not supported for this machine");
-            return;
-        }
         spapr_memory_plug(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
         spapr_core_plug(hotplug_dev, dev, errp);
Signature · 1 line
2.17.0
Reviewed-by on v2 3/6
#5 lore raw

Re: [Qemu-devel] [PATCH v2 3/6] spapr: move memory hotplug support check into spapr_memory_pre_plug()

Quoted 13 lines · On Fri, 8 Jun 2018 14:48:13 +0200 David Hildenbrand <[email protected]> wrote:

Let's finish cleaning up the hotplug handler. This check can be performed in the pre_plug code as the very first thing.

Signed-off-by David Hildenbrand <[email protected]>
---
 hw/ppc/spapr.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)
hw/ppc/spapr.c +0 -0
index 1f577b274b..9b8b4068b1 100644

hw/ppc/spapr.c out: · line 3187

Quoted 4 lines
hw/ppc/spapr.c +1 -0
@@ -3184,12 +3184,18 @@ out:
 static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                   Error **errp)
 {
+    const sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);

Even if const is valid here, it doesn't seem to be widely used in similar situations:

$ git grep 'Class.* = .*GET_CLASS' | grep const | wc -l
14
$ git grep 'Class.* = .*GET_CLASS' | grep -v const | wc -l
642

hw/ppc/spapr.c out: · line 3189

Quoted 2 lines
hw/ppc/spapr.c +0 -0
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);

... like here for example. This inconsistency is a bit weird.

The patch is good anyway so:

Reviewed-by Greg Kurz <[email protected]>
Quoted 27 lines
hw/ppc/spapr.c +5 -7
     MemoryRegion *mr;
     uint64_t size;
     char *mem_dev;
 
+    if (!smc->dr_lmb_enabled) {
+        error_setg(errp, "Memory hotplug not supported for this machine");
+        return;
+    }
+
     mr = ddc->get_memory_region(dimm, errp);
     if (!mr) {
         return;

@@ -3571,14 +3577,7 @@ out:
 static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
-    MachineState *ms = MACHINE(hotplug_dev);
-    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
-
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        if (!smc->dr_lmb_enabled) {
-            error_setg(errp, "Memory hotplug not supported for this machine");
-            return;
-        }
         spapr_memory_plug(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
         spapr_core_plug(hotplug_dev, dev, errp);
author
#6 lore raw

PATCH v2 4/6 spapr: introduce machine unplug handler

In reply to David Hildenbrand

We'll be handling unplug of e.g. CPUs and PCDIMMs via the general hotplug handler soon, so let's add that handler function.

Acked-by David Gibson <[email protected]>
Reviewed-by Greg Kurz <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
---
 hw/ppc/spapr.c | 6 ++++++
 1 file changed, 6 insertions(+)
hw/ppc/spapr.c +6 -0
index 9b8b4068b1..c45f8bc75b 100644
@@ -3584,6 +3584,11 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
     }
 }
 
+static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
+                                        DeviceState *dev, Error **errp)
+{
+}
+
 static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
                                                 DeviceState *dev, Error **errp)
 {

@@ -3978,6 +3983,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->get_default_cpu_node_id = spapr_get_default_cpu_node_id;
     mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids;
     hc->unplug_request = spapr_machine_device_unplug_request;
+    hc->unplug = spapr_machine_device_unplug;
 
     smc->dr_lmb_enabled = true;
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");
Signature · 1 line
2.17.0
author
#7 lore raw

PATCH v2 5/6 spapr: handle pc-dimm unplug via hotplug handler chain

In reply to David Hildenbrand

Factor out memory unplug into separate function from spapr_lmb_release(). Then use generic hotplug_handler_unplug() to trigger memory unplug, which will call spapr_machine_device_unplug() -> spapr_memory_unplug() in the end.

This way unplug operation is not buried in lmb internals and located in the same place like in other targets, following similar logic/call chain across targets.

Acked-by David Gibson <[email protected]>
Reviewed-by Greg Kurz <[email protected]>
Reviewed-by Igor Mammedov <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
---
 hw/ppc/spapr.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)
hw/ppc/spapr.c +15 -3
index c45f8bc75b..404d887f4e 100644
@@ -3299,7 +3299,8 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
 /* Callback to be called during DRC release. */
 void spapr_lmb_release(DeviceState *dev)
 {
-    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
+    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
+    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl);
     sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
 
     /* This information will get lost if a migration occurs

@@ -3317,9 +3318,17 @@ void spapr_lmb_release(DeviceState *dev)
 
     /*
      * Now that all the LMBs have been removed by the guest, call the
-     * pc-dimm unplug handler to cleanup up the pc-dimm device.
+     * unplug handler chain. This can never fail.
      */
-    pc_dimm_memory_unplug(dev, MACHINE(spapr));
+    hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
+}
+
+static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
+{
+    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
+    sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
+
+    pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev));
     object_unparent(OBJECT(dev));
     spapr_pending_dimm_unplugs_remove(spapr, ds);
 }

@@ -3587,6 +3596,9 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
 static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        spapr_memory_unplug(hotplug_dev, dev);
+    }
 }
 
 static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
Signature · 1 line
2.17.0
author
#8 lore raw

PATCH v2 6/6 spapr: handle cpu core unplug via hotplug handler chain

In reply to David Hildenbrand

Factor out cpu core unplug into separate function from spapr_core_release(). Then use generic hotplug_handler_unplug() to trigger cpu core unplug, which would call spapr_machine_device_unplug() -> spapr_core_unplug() in the end.

This way unplug operation is not buried in spapr internals and located in the same place like in other targets, following similar logic/call chain across targets.

Acked-by Igor Mammedov <[email protected]>
Acked-by David Gibson <[email protected]>
Reviewed-by Greg Kurz <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
---
 hw/ppc/spapr.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
hw/ppc/spapr.c +11 -1
index 404d887f4e..f59999daac 100644
@@ -3416,7 +3416,15 @@ static void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
 /* Callback to be called during DRC release. */
 void spapr_core_release(DeviceState *dev)
 {
-    MachineState *ms = MACHINE(qdev_get_hotplug_handler(dev));
+    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
+
+    /* Call the unplug handler chain. This can never fail. */
+    hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
+}
+
+static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
+{
+    MachineState *ms = MACHINE(hotplug_dev);
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
     CPUCore *cc = CPU_CORE(dev);
     CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);

@@ -3598,6 +3606,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         spapr_memory_unplug(hotplug_dev, dev);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
+        spapr_core_unplug(hotplug_dev, dev);
     }
 }
Signature · 1 line
2.17.0
Applied
#9 lore raw
In reply to David Hildenbrand
Quoted 6 lines · David Hildenbrand

I'll be messing with machine hotplug handlers of pc/spapr/s390x in the context of
[PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers

So this is a spin-off of the cleanup patches in the context of hotplug handlers.

Applied to ppc-for-3.0, thanks.

Quoted 19 lines

v1 -> v2:
- dropped the three "local_err" patches
- tweaked some patch descriptions
- Split "spapr: move all DIMM checks into spapr_memory_plug" up -- Move the memory hotplug check into the pre_plug handler
- Use &error_abort instead of NULL

David Hildenbrand (6):
spapr: no need to verify the node
spapr: move lookup of the node into spapr_memory_plug()
spapr: move memory hotplug support check into spapr_memory_pre_plug()
spapr: introduce machine unplug handler
spapr: handle pc-dimm unplug via hotplug handler chain
spapr: handle cpu core unplug via hotplug handler chain

 hw/ppc/spapr.c | 67 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 24 deletions(-)
Signature · 4 lines
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