[Qemu-devel] [PATCH v1 0/8] pc/spapr/s390x: machine hotplug handler cleanups

[Qemu-devel] [PATCH v1 0/8] pc/spapr/s390x: machine hotplug handler cleanups

[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 produced so far.

David Hildenbrand (8):
  pc: local error handling in hotplug handler functions
  spapr: no need to verify the node
  spapr: move all DIMM checks into spapr_memory_plug
  spapr: local error handling in hotplug handler functions
  spapr: introduce machine unplug handler
  spapr: handle pc-dimm unplug via hotplug handler chain
  spapr: handle cpu core unplug via hotplug handler chain
  s390x: local error handling in hotplug handler functions

 hw/i386/pc.c               | 32 +++++++++-----
 hw/ppc/spapr.c             | 89 +++++++++++++++++++++++++-------------
 hw/s390x/s390-virtio-ccw.c | 11 +++--
 3 files changed, 89 insertions(+), 43 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH v1 1/8] pc: local error handling in hotplug handler functions

[David Hildenbrand]

Let's introduce and use local error variables in the hotplug handler
functions.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/i386/pc.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f3befe6721..8075c6af15 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2006,45 +2006,57 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
 static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
                                           DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_pre_plug(hotplug_dev, dev, errp);
+        pc_cpu_pre_plug(hotplug_dev, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_plug(hotplug_dev, dev, errp);
+        pc_dimm_plug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_plug(hotplug_dev, dev, errp);
+        pc_cpu_plug(hotplug_dev, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
                                                 DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_unplug_request(hotplug_dev, dev, errp);
+        pc_dimm_unplug_request(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
+        pc_cpu_unplug_request_cb(hotplug_dev, dev, &local_err);
     } else {
-        error_setg(errp, "acpi: device unplug request for not supported device"
-                   " type: %s", object_get_typename(OBJECT(dev)));
+        error_setg(&local_err, "acpi: device unplug request for not supported"
+                   " device type: %s", object_get_typename(OBJECT(dev)));
     }
+    error_propagate(errp, local_err);
 }
 
 static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_unplug(hotplug_dev, dev, errp);
+        pc_dimm_unplug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_unplug_cb(hotplug_dev, dev, errp);
+        pc_cpu_unplug_cb(hotplug_dev, dev, &local_err);
     } else {
-        error_setg(errp, "acpi: device unplug for not supported device"
+        error_setg(&local_err, "acpi: device unplug for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
     }
+    error_propagate(errp, local_err);
 }
 
 static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,
-- 
2.17.0

[Qemu-devel] [PATCH v1 2/8] spapr: no need to verify the node

[David Hildenbrand]

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

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/ppc/spapr.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 2375cbee12..d038f3243e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3578,14 +3578,7 @@ 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, NULL);
 
         spapr_memory_plug(hotplug_dev, dev, node, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
-- 
2.17.0

[Qemu-devel] [PATCH v1 3/8] spapr: move all DIMM checks into spapr_memory_plug

[David Hildenbrand]

Let's clean the hotplug handler up by moving everything into
spapr_memory_plug().

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/ppc/spapr.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d038f3243e..a12be24ca9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3136,14 +3136,22 @@ 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);
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *mr;
     uint64_t align, size, addr;
+    uint32_t node;
+
+    if (!smc->dr_lmb_enabled) {
+        error_setg(&local_err, "Memory hotplug not supported for this machine");
+        goto out;
+    }
+    node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
 
     mr = ddc->get_memory_region(dimm, &local_err);
     if (local_err) {
@@ -3568,19 +3576,8 @@ 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)) {
-        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, NULL);
-
-        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);
     }
-- 
2.17.0

[Qemu-devel] [PATCH v1 4/8] spapr: local error handling in hotplug handler functions

[David Hildenbrand]

Let's introduce and use local error variables in the hotplug handler
functions.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/ppc/spapr.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a12be24ca9..4447cb197f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3576,11 +3576,14 @@ out:
 static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        spapr_memory_plug(hotplug_dev, dev, errp);
+        spapr_memory_plug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
-        spapr_core_plug(hotplug_dev, dev, errp);
+        spapr_core_plug(hotplug_dev, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
@@ -3588,10 +3591,11 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
 {
     sPAPRMachineState *sms = SPAPR_MACHINE(OBJECT(hotplug_dev));
     MachineClass *mc = MACHINE_GET_CLASS(sms);
+    Error *local_err = NULL;
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         if (spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT)) {
-            spapr_memory_unplug_request(hotplug_dev, dev, errp);
+            spapr_memory_unplug_request(hotplug_dev, dev, &local_err);
         } else {
             /* NOTE: this means there is a window after guest reset, prior to
              * CAS negotiation, where unplug requests will fail due to the
@@ -3599,25 +3603,32 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
              * the case with PCI unplug, where the events will be queued and
              * eventually handled by the guest after boot
              */
-            error_setg(errp, "Memory hot unplug not supported for this guest");
+            error_setg(&local_err,
+                       "Memory hot unplug not supported for this guest");
         }
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
         if (!mc->has_hotpluggable_cpus) {
-            error_setg(errp, "CPU hot unplug not supported on this machine");
-            return;
+            error_setg(&local_err,
+                       "CPU hot unplug not supported on this machine");
+            goto out;
         }
-        spapr_core_unplug_request(hotplug_dev, dev, errp);
+        spapr_core_unplug_request(hotplug_dev, dev, &local_err);
     }
+out:
+    error_propagate(errp, local_err);
 }
 
 static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
                                           DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        spapr_memory_pre_plug(hotplug_dev, dev, errp);
+        spapr_memory_pre_plug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
-        spapr_core_pre_plug(hotplug_dev, dev, errp);
+        spapr_core_pre_plug(hotplug_dev, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,
-- 
2.17.0

[Qemu-devel] [PATCH v1 5/8] spapr: introduce machine unplug handler

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

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/ppc/spapr.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 4447cb197f..bcb72d9fa7 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3586,6 +3586,11 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
     error_propagate(errp, local_err);
 }
 
+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)
 {
@@ -3988,6 +3993,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");
-- 
2.17.0

[Qemu-devel] [PATCH v1 6/8] spapr: handle pc-dimm unplug via hotplug handler chain

[David Hildenbrand]

Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/
unplug memory devices (which a pc-dimm is) later.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/ppc/spapr.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index bcb72d9fa7..0a8a3455d6 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3298,7 +3298,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
@@ -3316,9 +3317,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);
 }
@@ -3589,6 +3598,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,
-- 
2.17.0

[Qemu-devel] [PATCH v1 7/8] spapr: handle cpu core unplug via hotplug handler chain

[David Hildenbrand]

Let's handle it via hotplug_handler_unplug() to make plug/unplug code
look symmetrical.

Acked-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/ppc/spapr.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0a8a3455d6..994deea8cf 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3415,7 +3415,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);
@@ -3600,6 +3608,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);
     }
 }
 
-- 
2.17.0

[Qemu-devel] [PATCH v1 8/8] s390x: local error handling in hotplug handler functions

[David Hildenbrand]

Let's introduce and use local error variables in the hotplug handler
functions.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 7ae5fb38dd..29ea50a177 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -434,18 +434,23 @@ static void s390_machine_reset(void)
 static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
                                      DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        s390_cpu_plug(hotplug_dev, dev, errp);
+        s390_cpu_plug(hotplug_dev, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
                                                DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        error_setg(errp, "CPU hot unplug not supported on this machine");
-        return;
+        error_setg(&local_err, "CPU hot unplug not supported on this machine");
     }
+    error_propagate(errp, local_err);
 }
 
 static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,
-- 
2.17.0

Re: [Qemu-devel] [PATCH v1 2/8] spapr: no need to verify the node

[David Gibson]

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

On Thu, Jun 07, 2018 at 06:52:12PM +0200, David Hildenbrand wrote:
> The node property can always be queried and the value has already been
> verified in pc_dimm_realize().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 2375cbee12..d038f3243e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3578,14 +3578,7 @@ 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, NULL);
>  
>          spapr_memory_plug(hotplug_dev, dev, node, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {

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

Re: [Qemu-devel] [PATCH v1 3/8] spapr: move all DIMM checks into spapr_memory_plug

[David Gibson]

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

On Thu, Jun 07, 2018 at 06:52:13PM +0200, David Hildenbrand wrote:
> Let's clean the hotplug handler up by moving everything into
> spapr_memory_plug().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d038f3243e..a12be24ca9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3136,14 +3136,22 @@ 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);
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *mr;
>      uint64_t align, size, addr;
> +    uint32_t node;
> +
> +    if (!smc->dr_lmb_enabled) {
> +        error_setg(&local_err, "Memory hotplug not supported for this machine");
> +        goto out;
> +    }
> +    node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
>  
>      mr = ddc->get_memory_region(dimm, &local_err);
>      if (local_err) {
> @@ -3568,19 +3576,8 @@ 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)) {
> -        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, NULL);
> -
> -        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);
>      }

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

Re: [Qemu-devel] [PATCH v1 4/8] spapr: local error handling in hotplug handler functions

[David Gibson]

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

On Thu, Jun 07, 2018 at 06:52:14PM +0200, David Hildenbrand wrote:
> Let's introduce and use local error variables in the hotplug handler
> functions.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a12be24ca9..4447cb197f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3576,11 +3576,14 @@ out:
>  static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
> +    Error *local_err = NULL;
> +
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        spapr_memory_plug(hotplug_dev, dev, errp);
> +        spapr_memory_plug(hotplug_dev, dev, &local_err);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> -        spapr_core_plug(hotplug_dev, dev, errp);
> +        spapr_core_plug(hotplug_dev, dev, &local_err);
>      }
> +    error_propagate(errp, local_err);
>  }
>  
>  static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> @@ -3588,10 +3591,11 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>  {
>      sPAPRMachineState *sms = SPAPR_MACHINE(OBJECT(hotplug_dev));
>      MachineClass *mc = MACHINE_GET_CLASS(sms);
> +    Error *local_err = NULL;
>  
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>          if (spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT)) {
> -            spapr_memory_unplug_request(hotplug_dev, dev, errp);
> +            spapr_memory_unplug_request(hotplug_dev, dev, &local_err);
>          } else {
>              /* NOTE: this means there is a window after guest reset, prior to
>               * CAS negotiation, where unplug requests will fail due to the
> @@ -3599,25 +3603,32 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>               * the case with PCI unplug, where the events will be queued and
>               * eventually handled by the guest after boot
>               */
> -            error_setg(errp, "Memory hot unplug not supported for this guest");
> +            error_setg(&local_err,
> +                       "Memory hot unplug not supported for this guest");
>          }
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
>          if (!mc->has_hotpluggable_cpus) {
> -            error_setg(errp, "CPU hot unplug not supported on this machine");
> -            return;
> +            error_setg(&local_err,
> +                       "CPU hot unplug not supported on this machine");
> +            goto out;
>          }
> -        spapr_core_unplug_request(hotplug_dev, dev, errp);
> +        spapr_core_unplug_request(hotplug_dev, dev, &local_err);
>      }
> +out:
> +    error_propagate(errp, local_err);
>  }
>  
>  static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
>                                            DeviceState *dev, Error **errp)
>  {
> +    Error *local_err = NULL;
> +
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        spapr_memory_pre_plug(hotplug_dev, dev, errp);
> +        spapr_memory_pre_plug(hotplug_dev, dev, &local_err);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> -        spapr_core_pre_plug(hotplug_dev, dev, errp);
> +        spapr_core_pre_plug(hotplug_dev, dev, &local_err);
>      }
> +    error_propagate(errp, local_err);
>  }
>  
>  static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,

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

Re: [Qemu-devel] [PATCH v1 5/8] spapr: introduce machine unplug handler

[David Gibson]

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

On Thu, Jun 07, 2018 at 06:52:15PM +0200, David Hildenbrand wrote:
> We'll be handling unplug of e.g. CPUs and PCDIMMs  via the general
> hotplug handler soon, so let's add that handler function.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 4447cb197f..bcb72d9fa7 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3586,6 +3586,11 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>      error_propagate(errp, local_err);
>  }
>  
> +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)
>  {
> @@ -3988,6 +3993,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");

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

Re: [Qemu-devel] [PATCH v1 6/8] spapr: handle pc-dimm unplug via hotplug handler chain

[David Gibson]

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

On Thu, Jun 07, 2018 at 06:52:16PM +0200, David Hildenbrand wrote:
> Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/
> unplug memory devices (which a pc-dimm is) later.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bcb72d9fa7..0a8a3455d6 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3298,7 +3298,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
> @@ -3316,9 +3317,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);
>  }
> @@ -3589,6 +3598,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,

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

Re: [Qemu-devel] [PATCH v1 7/8] spapr: handle cpu core unplug via hotplug handler chain

[David Gibson]

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

On Thu, Jun 07, 2018 at 06:52:17PM +0200, David Hildenbrand wrote:
> Let's handle it via hotplug_handler_unplug() to make plug/unplug code
> look symmetrical.
> 
> Acked-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0a8a3455d6..994deea8cf 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3415,7 +3415,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);
> @@ -3600,6 +3608,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);
>      }
>  }
>  

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

Re: [Qemu-devel] [PATCH v1 8/8] s390x: local error handling in hotplug handler functions

[Cornelia Huck]

On Thu,  7 Jun 2018 18:52:18 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's introduce and use local error variables in the hotplug handler
> functions.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 7ae5fb38dd..29ea50a177 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -434,18 +434,23 @@ static void s390_machine_reset(void)
>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
>                                       DeviceState *dev, Error **errp)
>  {
> +    Error *local_err = NULL;
> +
>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> -        s390_cpu_plug(hotplug_dev, dev, errp);
> +        s390_cpu_plug(hotplug_dev, dev, &local_err);
>      }
> +    error_propagate(errp, local_err);
>  }
>  
>  static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>                                                 DeviceState *dev, Error **errp)
>  {
> +    Error *local_err = NULL;
> +
>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> -        error_setg(errp, "CPU hot unplug not supported on this machine");
> -        return;
> +        error_setg(&local_err, "CPU hot unplug not supported on this machine");
>      }
> +    error_propagate(errp, local_err);
>  }
>  
>  static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,

Just seeing this patch by itself, it does not really make much sense.
Even if this is a split out clean-up series, I'd prefer this to go
together with a patch that actually adds something more to the
plug/unplug functions.

Re: [Qemu-devel] [PATCH v1 8/8] s390x: local error handling in hotplug handler functions

[Christian Borntraeger]

On 06/08/2018 09:25 AM, Cornelia Huck wrote:
> On Thu,  7 Jun 2018 18:52:18 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Let's introduce and use local error variables in the hotplug handler
>> functions.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/s390x/s390-virtio-ccw.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 7ae5fb38dd..29ea50a177 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -434,18 +434,23 @@ static void s390_machine_reset(void)
>>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
>>                                       DeviceState *dev, Error **errp)
>>  {
>> +    Error *local_err = NULL;
>> +
>>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> -        s390_cpu_plug(hotplug_dev, dev, errp);
>> +        s390_cpu_plug(hotplug_dev, dev, &local_err);
>>      }
>> +    error_propagate(errp, local_err);
>>  }
>>  
>>  static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>>                                                 DeviceState *dev, Error **errp)
>>  {
>> +    Error *local_err = NULL;
>> +
>>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> -        error_setg(errp, "CPU hot unplug not supported on this machine");
>> -        return;
>> +        error_setg(&local_err, "CPU hot unplug not supported on this machine");
>>      }
>> +    error_propagate(errp, local_err);
>>  }
>>  
>>  static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,
> 
> Just seeing this patch by itself, it does not really make much sense.
> Even if this is a split out clean-up series, I'd prefer this to go
> together with a patch that actually adds something more to the
> plug/unplug functions.

+1. It is hard to see the "why". Maybe a better patch description could help here?

Re: [Qemu-devel] [PATCH v1 2/8] spapr: no need to verify the node

[Greg Kurz]

On Thu,  7 Jun 2018 18:52:12 +0200
David Hildenbrand <david@redhat.com> wrote:

> The node property can always be queried and the value has already been
> verified in pc_dimm_realize().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/ppc/spapr.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 2375cbee12..d038f3243e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3578,14 +3578,7 @@ 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) {

Good riddance :)

> -            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, NULL);

Maybe pass &error_abort ?

>  
>          spapr_memory_plug(hotplug_dev, dev, node, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {

Re: [Qemu-devel] [PATCH v1 8/8] s390x: local error handling in hotplug handler functions

[David Hildenbrand]

On 08.06.2018 09:27, Christian Borntraeger wrote:
> 
> 
> On 06/08/2018 09:25 AM, Cornelia Huck wrote:
>> On Thu,  7 Jun 2018 18:52:18 +0200
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> Let's introduce and use local error variables in the hotplug handler
>>> functions.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  hw/s390x/s390-virtio-ccw.c | 11 ++++++++---
>>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index 7ae5fb38dd..29ea50a177 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -434,18 +434,23 @@ static void s390_machine_reset(void)
>>>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
>>>                                       DeviceState *dev, Error **errp)
>>>  {
>>> +    Error *local_err = NULL;
>>> +
>>>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>> -        s390_cpu_plug(hotplug_dev, dev, errp);
>>> +        s390_cpu_plug(hotplug_dev, dev, &local_err);
>>>      }
>>> +    error_propagate(errp, local_err);
>>>  }
>>>  
>>>  static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>>>                                                 DeviceState *dev, Error **errp)
>>>  {
>>> +    Error *local_err = NULL;
>>> +
>>>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>> -        error_setg(errp, "CPU hot unplug not supported on this machine");
>>> -        return;
>>> +        error_setg(&local_err, "CPU hot unplug not supported on this machine");
>>>      }
>>> +    error_propagate(errp, local_err);
>>>  }
>>>  
>>>  static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,
>>
>> Just seeing this patch by itself, it does not really make much sense.
>> Even if this is a split out clean-up series, I'd prefer this to go
>> together with a patch that actually adds something more to the
>> plug/unplug functions.
> 
> +1. It is hard to see the "why". Maybe a better patch description could help here?
> 

When checking for an error (*errp) we should make sure that we don't
dereference the NULL pointer. I will be doing that in the future (memory
devices), but as you both don't seem to like this patch, I'll drop it
for now.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 2/8] spapr: no need to verify the node

[David Hildenbrand]

On 08.06.2018 09:34, Greg Kurz wrote:
> On Thu,  7 Jun 2018 18:52:12 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> The node property can always be queried and the value has already been
>> verified in pc_dimm_realize().
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/ppc/spapr.c | 9 +--------
>>  1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 2375cbee12..d038f3243e 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3578,14 +3578,7 @@ 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) {
> 
> Good riddance :)
> 
>> -            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, NULL);
> 
> Maybe pass &error_abort ?

I'm using the same access scheme as in hw/acpi/memory_hotplug.c

("error ignored" vs. "error leads to an abort") - but this will actually
never fail. But I can use error_abort here, does not matter.

Thanks!

> 
>>  
>>          spapr_memory_plug(hotplug_dev, dev, node, errp);
>>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 2/8] spapr: no need to verify the node

[Greg Kurz]

On Fri, 8 Jun 2018 09:42:48 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 08.06.2018 09:34, Greg Kurz wrote:
> > On Thu,  7 Jun 2018 18:52:12 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> The node property can always be queried and the value has already been
> >> verified in pc_dimm_realize().
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  hw/ppc/spapr.c | 9 +--------
> >>  1 file changed, 1 insertion(+), 8 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 2375cbee12..d038f3243e 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -3578,14 +3578,7 @@ 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) {  
> > 
> > Good riddance :)
> >   
> >> -            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, NULL);  
> > 
> > Maybe pass &error_abort ?  
> 
> I'm using the same access scheme as in hw/acpi/memory_hotplug.c
> 
> ("error ignored" vs. "error leads to an abort") - but this will actually
> never fail. But I can use error_abort here, does not matter.
> 

Heh, /me paranoid but this is David's call and he acked that already
so I guess it's okay.

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

> Thanks!
> 
> >   
> >>  
> >>          spapr_memory_plug(hotplug_dev, dev, node, errp);
> >>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {  
> >   
> 
> 

Re: [Qemu-devel] [PATCH v1 2/8] spapr: no need to verify the node

[David Hildenbrand]

On 08.06.2018 09:46, Greg Kurz wrote:
> On Fri, 8 Jun 2018 09:42:48 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 08.06.2018 09:34, Greg Kurz wrote:
>>> On Thu,  7 Jun 2018 18:52:12 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> The node property can always be queried and the value has already been
>>>> verified in pc_dimm_realize().
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>  hw/ppc/spapr.c | 9 +--------
>>>>  1 file changed, 1 insertion(+), 8 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index 2375cbee12..d038f3243e 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -3578,14 +3578,7 @@ 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) {  
>>>
>>> Good riddance :)
>>>   
>>>> -            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, NULL);  
>>>
>>> Maybe pass &error_abort ?  
>>
>> I'm using the same access scheme as in hw/acpi/memory_hotplug.c
>>
>> ("error ignored" vs. "error leads to an abort") - but this will actually
>> never fail. But I can use error_abort here, does not matter.
>>
> 
> Heh, /me paranoid but this is David's call and he acked that already
> so I guess it's okay.

NULL makes it fit into a single line :)

Thanks!

> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
>> Thanks!
>>
>>>   
>>>>  
>>>>          spapr_memory_plug(hotplug_dev, dev, node, errp);
>>>>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {  
>>>   
>>
>>
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 8/8] s390x: local error handling in hotplug handler functions

[Christian Borntraeger]

On 06/08/2018 09:40 AM, David Hildenbrand wrote:
> On 08.06.2018 09:27, Christian Borntraeger wrote:
>>
>>
>> On 06/08/2018 09:25 AM, Cornelia Huck wrote:
>>> On Thu,  7 Jun 2018 18:52:18 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>
>>>> Let's introduce and use local error variables in the hotplug handler
>>>> functions.
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>  hw/s390x/s390-virtio-ccw.c | 11 ++++++++---
>>>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>> index 7ae5fb38dd..29ea50a177 100644
>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>> @@ -434,18 +434,23 @@ static void s390_machine_reset(void)
>>>>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
>>>>                                       DeviceState *dev, Error **errp)
>>>>  {
>>>> +    Error *local_err = NULL;
>>>> +
>>>>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>>> -        s390_cpu_plug(hotplug_dev, dev, errp);
>>>> +        s390_cpu_plug(hotplug_dev, dev, &local_err);
>>>>      }
>>>> +    error_propagate(errp, local_err);
>>>>  }
>>>>  
>>>>  static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>>>>                                                 DeviceState *dev, Error **errp)
>>>>  {
>>>> +    Error *local_err = NULL;
>>>> +
>>>>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>>> -        error_setg(errp, "CPU hot unplug not supported on this machine");
>>>> -        return;
>>>> +        error_setg(&local_err, "CPU hot unplug not supported on this machine");
>>>>      }
>>>> +    error_propagate(errp, local_err);
>>>>  }
>>>>  
>>>>  static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,
>>>
>>> Just seeing this patch by itself, it does not really make much sense.
>>> Even if this is a split out clean-up series, I'd prefer this to go
>>> together with a patch that actually adds something more to the
>>> plug/unplug functions.
>>
>> +1. It is hard to see the "why". Maybe a better patch description could help here?
>>
> 
> When checking for an error (*errp) we should make sure that we don't
> dereference the NULL pointer. I will be doing that in the future (memory
> devices), but as you both don't seem to like this patch, I'll drop it
> for now.

With such a patch description (making the handler safe against NULL errp) the patch
suddenly makes sense on its own. 

Re: [Qemu-devel] [PATCH v1 0/8] pc/spapr/s390x: machine hotplug handler cleanups

[David Hildenbrand]

On 07.06.2018 18:52, David Hildenbrand wrote:
> 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 produced so far.
> 
> David Hildenbrand (8):
>   pc: local error handling in hotplug handler functions
>   spapr: no need to verify the node
>   spapr: move all DIMM checks into spapr_memory_plug
>   spapr: local error handling in hotplug handler functions
>   spapr: introduce machine unplug handler
>   spapr: handle pc-dimm unplug via hotplug handler chain
>   spapr: handle cpu core unplug via hotplug handler chain
>   s390x: local error handling in hotplug handler functions
> 
>  hw/i386/pc.c               | 32 +++++++++-----
>  hw/ppc/spapr.c             | 89 +++++++++++++++++++++++++-------------
>  hw/s390x/s390-virtio-ccw.c | 11 +++--
>  3 files changed, 89 insertions(+), 43 deletions(-)
> 

I might be dropping the "local error handling" patches (as Conny and
Christian properly asked for the reason). Although they are not wrong,
they are only useful if performing checks on error pointers (*errp) or
when relying on the error to indicate the "return value" of a function.

And as I might be handling plugging of memory devices in subfunctions
(to keep the handler short as requested by Igor), this might not be
needed on this level.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 1/8] pc: local error handling in hotplug handler functions

[Thomas Huth]

On 07.06.2018 18:52, David Hildenbrand wrote:
> Let's introduce and use local error variables in the hotplug handler
> functions.

Why? You don't check local_err in the functions, so I fail to see why
this is needed? If you need this in a later patch, I think this should
simply be part of that later patch instead.

 Thomas


> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/i386/pc.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f3befe6721..8075c6af15 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2006,45 +2006,57 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>                                            DeviceState *dev, Error **errp)
>  {
> +    Error *local_err = NULL;
> +
>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> -        pc_cpu_pre_plug(hotplug_dev, dev, errp);
> +        pc_cpu_pre_plug(hotplug_dev, dev, &local_err);
>      }
> +    error_propagate(errp, local_err);
>  }
>  
>  static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
> +    Error *local_err = NULL;
> +
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        pc_dimm_plug(hotplug_dev, dev, errp);
> +        pc_dimm_plug(hotplug_dev, dev, &local_err);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> -        pc_cpu_plug(hotplug_dev, dev, errp);
> +        pc_cpu_plug(hotplug_dev, dev, &local_err);
>      }
> +    error_propagate(errp, local_err);
>  }
>  
>  static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>                                                  DeviceState *dev, Error **errp)
>  {
> +    Error *local_err = NULL;
> +
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        pc_dimm_unplug_request(hotplug_dev, dev, errp);
> +        pc_dimm_unplug_request(hotplug_dev, dev, &local_err);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> -        pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
> +        pc_cpu_unplug_request_cb(hotplug_dev, dev, &local_err);
>      } else {
> -        error_setg(errp, "acpi: device unplug request for not supported device"
> -                   " type: %s", object_get_typename(OBJECT(dev)));
> +        error_setg(&local_err, "acpi: device unplug request for not supported"
> +                   " device type: %s", object_get_typename(OBJECT(dev)));
>      }
> +    error_propagate(errp, local_err);
>  }
>  
>  static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
>                                          DeviceState *dev, Error **errp)
>  {
> +    Error *local_err = NULL;
> +
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        pc_dimm_unplug(hotplug_dev, dev, errp);
> +        pc_dimm_unplug(hotplug_dev, dev, &local_err);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> -        pc_cpu_unplug_cb(hotplug_dev, dev, errp);
> +        pc_cpu_unplug_cb(hotplug_dev, dev, &local_err);
>      } else {
> -        error_setg(errp, "acpi: device unplug for not supported device"
> +        error_setg(&local_err, "acpi: device unplug for not supported device"
>                     " type: %s", object_get_typename(OBJECT(dev)));
>      }
> +    error_propagate(errp, local_err);
>  }
>  
>  static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,
> 

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 1/8] pc: local error handling in hotplug handler functions

[David Hildenbrand]

On 08.06.2018 10:04, Thomas Huth wrote:
> On 07.06.2018 18:52, David Hildenbrand wrote:
>> Let's introduce and use local error variables in the hotplug handler
>> functions.
> 
> Why? You don't check local_err in the functions, so I fail to see why
> this is needed? If you need this in a later patch, I think this should
> simply be part of that later patch instead.

See the mail I sent a couple of minutes ago as reply to the cover
letter. Thanks.

> 
>  Thomas
> 
> 
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/i386/pc.c | 32 ++++++++++++++++++++++----------
>>  1 file changed, 22 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index f3befe6721..8075c6af15 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -2006,45 +2006,57 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>>  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>>                                            DeviceState *dev, Error **errp)
>>  {
>> +    Error *local_err = NULL;
>> +
>>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> -        pc_cpu_pre_plug(hotplug_dev, dev, errp);
>> +        pc_cpu_pre_plug(hotplug_dev, dev, &local_err);
>>      }
>> +    error_propagate(errp, local_err);
>>  }
>>  
>>  static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>>                                        DeviceState *dev, Error **errp)
>>  {
>> +    Error *local_err = NULL;
>> +
>>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>> -        pc_dimm_plug(hotplug_dev, dev, errp);
>> +        pc_dimm_plug(hotplug_dev, dev, &local_err);
>>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> -        pc_cpu_plug(hotplug_dev, dev, errp);
>> +        pc_cpu_plug(hotplug_dev, dev, &local_err);
>>      }
>> +    error_propagate(errp, local_err);
>>  }
>>  
>>  static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>>                                                  DeviceState *dev, Error **errp)
>>  {
>> +    Error *local_err = NULL;
>> +
>>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>> -        pc_dimm_unplug_request(hotplug_dev, dev, errp);
>> +        pc_dimm_unplug_request(hotplug_dev, dev, &local_err);
>>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> -        pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
>> +        pc_cpu_unplug_request_cb(hotplug_dev, dev, &local_err);
>>      } else {
>> -        error_setg(errp, "acpi: device unplug request for not supported device"
>> -                   " type: %s", object_get_typename(OBJECT(dev)));
>> +        error_setg(&local_err, "acpi: device unplug request for not supported"
>> +                   " device type: %s", object_get_typename(OBJECT(dev)));
>>      }
>> +    error_propagate(errp, local_err);
>>  }
>>  
>>  static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
>>                                          DeviceState *dev, Error **errp)
>>  {
>> +    Error *local_err = NULL;
>> +
>>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>> -        pc_dimm_unplug(hotplug_dev, dev, errp);
>> +        pc_dimm_unplug(hotplug_dev, dev, &local_err);
>>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>> -        pc_cpu_unplug_cb(hotplug_dev, dev, errp);
>> +        pc_cpu_unplug_cb(hotplug_dev, dev, &local_err);
>>      } else {
>> -        error_setg(errp, "acpi: device unplug for not supported device"
>> +        error_setg(&local_err, "acpi: device unplug for not supported device"
>>                     " type: %s", object_get_typename(OBJECT(dev)));
>>      }
>> +    error_propagate(errp, local_err);
>>  }
>>  
>>  static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,
>>
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 3/8] spapr: move all DIMM checks into spapr_memory_plug

[Greg Kurz]

On Thu,  7 Jun 2018 18:52:13 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's clean the hotplug handler up by moving everything into
> spapr_memory_plug().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/ppc/spapr.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d038f3243e..a12be24ca9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3136,14 +3136,22 @@ 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);
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *mr;
>      uint64_t align, size, addr;
> +    uint32_t node;
> +
> +    if (!smc->dr_lmb_enabled) {
> +        error_setg(&local_err, "Memory hotplug not supported for this machine");
> +        goto out;
> +    }

Wouldn't it be more appropriate to move this check to spapr_memory_pre_plug() ?

> +    node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
>  
>      mr = ddc->get_memory_region(dimm, &local_err);
>      if (local_err) {
> @@ -3568,19 +3576,8 @@ 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)) {
> -        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, NULL);
> -
> -        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);
>      }

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[Thomas Huth]

On 08.06.2018 09:48, David Hildenbrand wrote:
> On 08.06.2018 09:46, Greg Kurz wrote:
>> On Fri, 8 Jun 2018 09:42:48 +0200
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> On 08.06.2018 09:34, Greg Kurz wrote:
>>>> On Thu,  7 Jun 2018 18:52:12 +0200
>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>   
>>>>> The node property can always be queried and the value has already been
>>>>> verified in pc_dimm_realize().
>>>>>
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>>  hw/ppc/spapr.c | 9 +--------
>>>>>  1 file changed, 1 insertion(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>> index 2375cbee12..d038f3243e 100644
>>>>> --- a/hw/ppc/spapr.c
>>>>> +++ b/hw/ppc/spapr.c
>>>>> @@ -3578,14 +3578,7 @@ 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) {  
>>>>
>>>> Good riddance :)
>>>>   
>>>>> -            return;
>>>>> -        }
>>>>> -        if (node < 0 || node >= MAX_NODES) {
>>>>> -            error_setg(errp, "Invaild node %d", node);
>>>>> -            return;
>>>>> -        }

Maybe turn that into an assert() instead? ... just for the paranoids ;-)

>>>>> +        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);  
>>>>
>>>> Maybe pass &error_abort ?  
>>>
>>> I'm using the same access scheme as in hw/acpi/memory_hotplug.c
>>>
>>> ("error ignored" vs. "error leads to an abort") - but this will actually
>>> never fail. But I can use error_abort here, does not matter.
>>>
>>
>> Heh, /me paranoid but this is David's call and he acked that already
>> so I guess it's okay.
> 
> NULL makes it fit into a single line :)

+1 for error_abort, even if it takes another line.

 Thomas

Re: [Qemu-devel] [PATCH v1 3/8] spapr: move all DIMM checks into spapr_memory_plug

[David Hildenbrand]

On 08.06.2018 10:05, Greg Kurz wrote:
> On Thu,  7 Jun 2018 18:52:13 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Let's clean the hotplug handler up by moving everything into
>> spapr_memory_plug().
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/ppc/spapr.c | 23 ++++++++++-------------
>>  1 file changed, 10 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index d038f3243e..a12be24ca9 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3136,14 +3136,22 @@ 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);
>> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
>>      PCDIMMDevice *dimm = PC_DIMM(dev);
>>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>>      MemoryRegion *mr;
>>      uint64_t align, size, addr;
>> +    uint32_t node;
>> +
>> +    if (!smc->dr_lmb_enabled) {
>> +        error_setg(&local_err, "Memory hotplug not supported for this machine");
>> +        goto out;
>> +    }
> 
> Wouldn't it be more appropriate to move this check to spapr_memory_pre_plug() ?

Think you're right (and as spapr_memory_pre_plug() already exists, it's
easy), other opinions? Thanks.

> 
>> +    node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
>>  
>>      mr = ddc->get_memory_region(dimm, &local_err);
>>      if (local_err) {
>> @@ -3568,19 +3576,8 @@ 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)) {
>> -        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, NULL);
>> -
>> -        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);
>>      }
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 2/8] spapr: no need to verify the node

[David Gibson]

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

On Fri, Jun 08, 2018 at 09:46:57AM +0200, Greg Kurz wrote:
> On Fri, 8 Jun 2018 09:42:48 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
> > On 08.06.2018 09:34, Greg Kurz wrote:
> > > On Thu,  7 Jun 2018 18:52:12 +0200
> > > David Hildenbrand <david@redhat.com> wrote:
> > >   
> > >> The node property can always be queried and the value has already been
> > >> verified in pc_dimm_realize().
> > >>
> > >> Signed-off-by: David Hildenbrand <david@redhat.com>
> > >> ---
> > >>  hw/ppc/spapr.c | 9 +--------
> > >>  1 file changed, 1 insertion(+), 8 deletions(-)
> > >>
> > >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > >> index 2375cbee12..d038f3243e 100644
> > >> --- a/hw/ppc/spapr.c
> > >> +++ b/hw/ppc/spapr.c
> > >> @@ -3578,14 +3578,7 @@ 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) {  
> > > 
> > > Good riddance :)
> > >   
> > >> -            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, NULL);  
> > > 
> > > Maybe pass &error_abort ?  
> > 
> > I'm using the same access scheme as in hw/acpi/memory_hotplug.c
> > 
> > ("error ignored" vs. "error leads to an abort") - but this will actually
> > never fail. But I can use error_abort here, does not matter.
> > 
> 
> Heh, /me paranoid but this is David's call and he acked that already
> so I guess it's okay.

Actually, I missed this - error_abort is preferable.  That's general
the right choice for things that shouldn't ever fail.  This way if
they *do* fail we get a clear error immediately.

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


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

Re: [Qemu-devel] [PATCH v1 2/8] spapr: no need to verify the node

[David Hildenbrand]

On 08.06.2018 10:20, David Gibson wrote:
> On Fri, Jun 08, 2018 at 09:46:57AM +0200, Greg Kurz wrote:
>> On Fri, 8 Jun 2018 09:42:48 +0200
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> On 08.06.2018 09:34, Greg Kurz wrote:
>>>> On Thu,  7 Jun 2018 18:52:12 +0200
>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>   
>>>>> The node property can always be queried and the value has already been
>>>>> verified in pc_dimm_realize().
>>>>>
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>>  hw/ppc/spapr.c | 9 +--------
>>>>>  1 file changed, 1 insertion(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>> index 2375cbee12..d038f3243e 100644
>>>>> --- a/hw/ppc/spapr.c
>>>>> +++ b/hw/ppc/spapr.c
>>>>> @@ -3578,14 +3578,7 @@ 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) {  
>>>>
>>>> Good riddance :)
>>>>   
>>>>> -            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, NULL);  
>>>>
>>>> Maybe pass &error_abort ?  
>>>
>>> I'm using the same access scheme as in hw/acpi/memory_hotplug.c
>>>
>>> ("error ignored" vs. "error leads to an abort") - but this will actually
>>> never fail. But I can use error_abort here, does not matter.
>>>
>>
>> Heh, /me paranoid but this is David's call and he acked that already
>> so I guess it's okay.
> 
> Actually, I missed this - error_abort is preferable.  That's general
> the right choice for things that shouldn't ever fail.  This way if
> they *do* fail we get a clear error immediately.

error_abort it is :)

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


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[Igor Mammedov]

On Fri, 8 Jun 2018 10:07:31 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 08.06.2018 09:48, David Hildenbrand wrote:
> > On 08.06.2018 09:46, Greg Kurz wrote:  
> >> On Fri, 8 Jun 2018 09:42:48 +0200
> >> David Hildenbrand <david@redhat.com> wrote:
> >>  
> >>> On 08.06.2018 09:34, Greg Kurz wrote:  
> >>>> On Thu,  7 Jun 2018 18:52:12 +0200
> >>>> David Hildenbrand <david@redhat.com> wrote:
> >>>>     
> >>>>> The node property can always be queried and the value has already been
> >>>>> verified in pc_dimm_realize().
> >>>>>
> >>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
> >>>>> ---
> >>>>>  hw/ppc/spapr.c | 9 +--------
> >>>>>  1 file changed, 1 insertion(+), 8 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>>>> index 2375cbee12..d038f3243e 100644
> >>>>> --- a/hw/ppc/spapr.c
> >>>>> +++ b/hw/ppc/spapr.c
> >>>>> @@ -3578,14 +3578,7 @@ 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) {    
> >>>>
> >>>> Good riddance :)
> >>>>     
> >>>>> -            return;
> >>>>> -        }
> >>>>> -        if (node < 0 || node >= MAX_NODES) {
> >>>>> -            error_setg(errp, "Invaild node %d", node);
> >>>>> -            return;
> >>>>> -        }  
> 
> Maybe turn that into an assert() instead? ... just for the paranoids ;-)
> 
> >>>>> +        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);    
> >>>>
> >>>> Maybe pass &error_abort ?    
> >>>
> >>> I'm using the same access scheme as in hw/acpi/memory_hotplug.c
> >>>
> >>> ("error ignored" vs. "error leads to an abort") - but this will actually
> >>> never fail. But I can use error_abort here, does not matter.
> >>>  
> >>
> >> Heh, /me paranoid but this is David's call and he acked that already
> >> so I guess it's okay.  
> > 
> > NULL makes it fit into a single line :)  
> 
> +1 for error_abort, even if it takes another line.
+1 for error_abort
call shouldn't fail, but if does it won't be silently ignored
and introduce undefined behavior.

> 
>  Thomas

Re: [Qemu-devel] [PATCH v1 4/8] spapr: local error handling in hotplug handler functions

[Greg Kurz]

On Thu,  7 Jun 2018 18:52:14 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's introduce and use local error variables in the hotplug handler
> functions.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---

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

>  hw/ppc/spapr.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a12be24ca9..4447cb197f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3576,11 +3576,14 @@ out:
>  static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
> +    Error *local_err = NULL;
> +
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        spapr_memory_plug(hotplug_dev, dev, errp);
> +        spapr_memory_plug(hotplug_dev, dev, &local_err);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> -        spapr_core_plug(hotplug_dev, dev, errp);
> +        spapr_core_plug(hotplug_dev, dev, &local_err);
>      }
> +    error_propagate(errp, local_err);
>  }
>  
>  static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> @@ -3588,10 +3591,11 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>  {
>      sPAPRMachineState *sms = SPAPR_MACHINE(OBJECT(hotplug_dev));
>      MachineClass *mc = MACHINE_GET_CLASS(sms);
> +    Error *local_err = NULL;
>  
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>          if (spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT)) {
> -            spapr_memory_unplug_request(hotplug_dev, dev, errp);
> +            spapr_memory_unplug_request(hotplug_dev, dev, &local_err);
>          } else {
>              /* NOTE: this means there is a window after guest reset, prior to
>               * CAS negotiation, where unplug requests will fail due to the
> @@ -3599,25 +3603,32 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>               * the case with PCI unplug, where the events will be queued and
>               * eventually handled by the guest after boot
>               */
> -            error_setg(errp, "Memory hot unplug not supported for this guest");
> +            error_setg(&local_err,
> +                       "Memory hot unplug not supported for this guest");
>          }
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
>          if (!mc->has_hotpluggable_cpus) {
> -            error_setg(errp, "CPU hot unplug not supported on this machine");
> -            return;
> +            error_setg(&local_err,
> +                       "CPU hot unplug not supported on this machine");
> +            goto out;
>          }
> -        spapr_core_unplug_request(hotplug_dev, dev, errp);
> +        spapr_core_unplug_request(hotplug_dev, dev, &local_err);
>      }
> +out:
> +    error_propagate(errp, local_err);
>  }
>  
>  static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
>                                            DeviceState *dev, Error **errp)
>  {
> +    Error *local_err = NULL;
> +
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        spapr_memory_pre_plug(hotplug_dev, dev, errp);
> +        spapr_memory_pre_plug(hotplug_dev, dev, &local_err);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> -        spapr_core_pre_plug(hotplug_dev, dev, errp);
> +        spapr_core_pre_plug(hotplug_dev, dev, &local_err);
>      }
> +    error_propagate(errp, local_err);
>  }
>  
>  static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[David Hildenbrand]

On 08.06.2018 10:39, Igor Mammedov wrote:
> On Fri, 8 Jun 2018 10:07:31 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 08.06.2018 09:48, David Hildenbrand wrote:
>>> On 08.06.2018 09:46, Greg Kurz wrote:  
>>>> On Fri, 8 Jun 2018 09:42:48 +0200
>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>  
>>>>> On 08.06.2018 09:34, Greg Kurz wrote:  
>>>>>> On Thu,  7 Jun 2018 18:52:12 +0200
>>>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>>>     
>>>>>>> The node property can always be queried and the value has already been
>>>>>>> verified in pc_dimm_realize().
>>>>>>>
>>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>>>> ---
>>>>>>>  hw/ppc/spapr.c | 9 +--------
>>>>>>>  1 file changed, 1 insertion(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>>>> index 2375cbee12..d038f3243e 100644
>>>>>>> --- a/hw/ppc/spapr.c
>>>>>>> +++ b/hw/ppc/spapr.c
>>>>>>> @@ -3578,14 +3578,7 @@ 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) {    
>>>>>>
>>>>>> Good riddance :)
>>>>>>     
>>>>>>> -            return;
>>>>>>> -        }
>>>>>>> -        if (node < 0 || node >= MAX_NODES) {
>>>>>>> -            error_setg(errp, "Invaild node %d", node);
>>>>>>> -            return;
>>>>>>> -        }  
>>
>> Maybe turn that into an assert() instead? ... just for the paranoids ;-)
>>
>>>>>>> +        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);    
>>>>>>
>>>>>> Maybe pass &error_abort ?    
>>>>>
>>>>> I'm using the same access scheme as in hw/acpi/memory_hotplug.c
>>>>>
>>>>> ("error ignored" vs. "error leads to an abort") - but this will actually
>>>>> never fail. But I can use error_abort here, does not matter.
>>>>>  
>>>>
>>>> Heh, /me paranoid but this is David's call and he acked that already
>>>> so I guess it's okay.  
>>>
>>> NULL makes it fit into a single line :)  
>>
>> +1 for error_abort, even if it takes another line.
> +1 for error_abort
> call shouldn't fail, but if does it won't be silently ignored
> and introduce undefined behavior.

Maybe we should fix the others that pass in NULL.

(no, not me :D - I'm already busy with your requested pre_plug handling)

> 
>>
>>  Thomas
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 3/8] spapr: move all DIMM checks into spapr_memory_plug

[Igor Mammedov]

On Fri, 8 Jun 2018 10:07:59 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 08.06.2018 10:05, Greg Kurz wrote:
> > On Thu,  7 Jun 2018 18:52:13 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> Let's clean the hotplug handler up by moving everything into
> >> spapr_memory_plug().
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  hw/ppc/spapr.c | 23 ++++++++++-------------
> >>  1 file changed, 10 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index d038f3243e..a12be24ca9 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -3136,14 +3136,22 @@ 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);
> >> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
> >>      PCDIMMDevice *dimm = PC_DIMM(dev);
> >>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> >>      MemoryRegion *mr;
> >>      uint64_t align, size, addr;
> >> +    uint32_t node;
> >> +
> >> +    if (!smc->dr_lmb_enabled) {
> >> +        error_setg(&local_err, "Memory hotplug not supported for this machine");
> >> +        goto out;
> >> +    }  
> > 
> > Wouldn't it be more appropriate to move this check to spapr_memory_pre_plug() ?  
> 
> Think you're right (and as spapr_memory_pre_plug() already exists, it's
> easy), other opinions? Thanks.
I also think that it should go to preplug

> 
> >   
> >> +    node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);
> >>  
> >>      mr = ddc->get_memory_region(dimm, &local_err);
> >>      if (local_err) {
> >> @@ -3568,19 +3576,8 @@ 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)) {
> >> -        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, NULL);
> >> -
> >> -        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);
> >>      }  
> >   
> 
> 

Re: [Qemu-devel] [PATCH v1 5/8] spapr: introduce machine unplug handler

[Igor Mammedov]

On Thu,  7 Jun 2018 18:52:15 +0200
David Hildenbrand <david@redhat.com> wrote:

> We'll be handling unplug of e.g. CPUs and PCDIMMs  via the general
> hotplug handler soon, so let's add that handler function.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/ppc/spapr.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 4447cb197f..bcb72d9fa7 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3586,6 +3586,11 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>      error_propagate(errp, local_err);
>  }
>  
> +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)
>  {
> @@ -3988,6 +3993,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");

Re: [Qemu-devel] [PATCH v1 5/8] spapr: introduce machine unplug handler

[Greg Kurz]

On Thu,  7 Jun 2018 18:52:15 +0200
David Hildenbrand <david@redhat.com> wrote:

> We'll be handling unplug of e.g. CPUs and PCDIMMs  via the general
> hotplug handler soon, so let's add that handler function.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---

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

>  hw/ppc/spapr.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 4447cb197f..bcb72d9fa7 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3586,6 +3586,11 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>      error_propagate(errp, local_err);
>  }
>  
> +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)
>  {
> @@ -3988,6 +3993,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");

Re: [Qemu-devel] [PATCH v1 6/8] spapr: handle pc-dimm unplug via hotplug handler chain

[Igor Mammedov]

On Thu,  7 Jun 2018 18:52:16 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/
> unplug memory devices (which a pc-dimm is) later.
Perhaps something like following would be better:

Factor out memory unplug into separate function from spapr_lmb_release().
Then use generic hotplug_handler_unplug() to trigger memory unplug,
which would 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.


> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/ppc/spapr.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bcb72d9fa7..0a8a3455d6 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3298,7 +3298,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
> @@ -3316,9 +3317,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);
>  }
> @@ -3589,6 +3598,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,

Re: [Qemu-devel] [PATCH v1 7/8] spapr: handle cpu core unplug via hotplug handler chain

[Igor Mammedov]

On Thu,  7 Jun 2018 18:52:17 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's handle it via hotplug_handler_unplug() to make plug/unplug code
> look symmetrical.
ditto wrt commit message as 6/8

> 
> Acked-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/ppc/spapr.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0a8a3455d6..994deea8cf 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3415,7 +3415,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);
> @@ -3600,6 +3608,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);
>      }
>  }
>  

Re: [Qemu-devel] [PATCH v1 6/8] spapr: handle pc-dimm unplug via hotplug handler chain

[Greg Kurz]

On Thu,  7 Jun 2018 18:52:16 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/
> unplug memory devices (which a pc-dimm is) later.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---

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

>  hw/ppc/spapr.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bcb72d9fa7..0a8a3455d6 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3298,7 +3298,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
> @@ -3316,9 +3317,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);
>  }
> @@ -3589,6 +3598,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,

Re: [Qemu-devel] [PATCH v1 7/8] spapr: handle cpu core unplug via hotplug handler chain

[Greg Kurz]

On Thu,  7 Jun 2018 18:52:17 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's handle it via hotplug_handler_unplug() to make plug/unplug code
> look symmetrical.
> 
> Acked-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---

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

>  hw/ppc/spapr.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0a8a3455d6..994deea8cf 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3415,7 +3415,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);
> @@ -3600,6 +3608,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);
>      }
>  }
>  

Re: [Qemu-devel] [PATCH v1 6/8] spapr: handle pc-dimm unplug via hotplug handler chain

[David Hildenbrand]

On 08.06.2018 10:56, Igor Mammedov wrote:
> On Thu,  7 Jun 2018 18:52:16 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/
>> unplug memory devices (which a pc-dimm is) later.
> Perhaps something like following would be better:
> 
> Factor out memory unplug into separate function from spapr_lmb_release().
> Then use generic hotplug_handler_unplug() to trigger memory unplug,
> which would 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.

Can this be an addon patch? Sounds like factoring out more and moving more.

> 
> 
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/ppc/spapr.c | 18 +++++++++++++++---
>>  1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index bcb72d9fa7..0a8a3455d6 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3298,7 +3298,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
>> @@ -3316,9 +3317,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);
>>  }
>> @@ -3589,6 +3598,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,
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 8/8] s390x: local error handling in hotplug handler functions

[Igor Mammedov]

On Fri, 8 Jun 2018 09:40:04 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 08.06.2018 09:27, Christian Borntraeger wrote:
> > 
> > 
> > On 06/08/2018 09:25 AM, Cornelia Huck wrote:  
> >> On Thu,  7 Jun 2018 18:52:18 +0200
> >> David Hildenbrand <david@redhat.com> wrote:
> >>  
> >>> Let's introduce and use local error variables in the hotplug handler
> >>> functions.
> >>>
> >>> Signed-off-by: David Hildenbrand <david@redhat.com>
> >>> ---
> >>>  hw/s390x/s390-virtio-ccw.c | 11 ++++++++---
> >>>  1 file changed, 8 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> >>> index 7ae5fb38dd..29ea50a177 100644
> >>> --- a/hw/s390x/s390-virtio-ccw.c
> >>> +++ b/hw/s390x/s390-virtio-ccw.c
> >>> @@ -434,18 +434,23 @@ static void s390_machine_reset(void)
> >>>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
> >>>                                       DeviceState *dev, Error **errp)
> >>>  {
> >>> +    Error *local_err = NULL;
> >>> +
> >>>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> >>> -        s390_cpu_plug(hotplug_dev, dev, errp);
> >>> +        s390_cpu_plug(hotplug_dev, dev, &local_err);
> >>>      }
> >>> +    error_propagate(errp, local_err);
> >>>  }
> >>>  
> >>>  static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
> >>>                                                 DeviceState *dev, Error **errp)
> >>>  {
> >>> +    Error *local_err = NULL;
> >>> +
> >>>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> >>> -        error_setg(errp, "CPU hot unplug not supported on this machine");
> >>> -        return;
> >>> +        error_setg(&local_err, "CPU hot unplug not supported on this machine");
> >>>      }
> >>> +    error_propagate(errp, local_err);
> >>>  }
> >>>  
> >>>  static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,  
> >>
> >> Just seeing this patch by itself, it does not really make much sense.
> >> Even if this is a split out clean-up series, I'd prefer this to go
> >> together with a patch that actually adds something more to the
> >> plug/unplug functions.  
> > 
> > +1. It is hard to see the "why". Maybe a better patch description could help here?
> >   
> 
> When checking for an error (*errp) we should make sure that we don't
> dereference the NULL pointer. I will be doing that in the future (memory
> devices), but as you both don't seem to like this patch, I'll drop it
> for now.
hotplug handlers aren't called with NULL errp, so it not really necessary.
To be on the safe side we can ensure that errp is not NULL doing something
like:

diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
index 17ac986..dc9e4bf 100644
--- a/hw/core/hotplug.c
+++ b/hw/core/hotplug.c
@@ -52,6 +52,7 @@ void hotplug_handler_unplug(HotplugHandler *plug_handler,
 {
     HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
 
+    g_assert(errp);
     if (hdc->unplug) {
         hdc->unplug(plug_handler, plugged_dev, errp);
     }

and do it for all similar wrappers in this file

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[Igor Mammedov]

On Fri, 8 Jun 2018 10:41:36 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 08.06.2018 10:39, Igor Mammedov wrote:
> > On Fri, 8 Jun 2018 10:07:31 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> On 08.06.2018 09:48, David Hildenbrand wrote:  
> >>> On 08.06.2018 09:46, Greg Kurz wrote:    
> >>>> On Fri, 8 Jun 2018 09:42:48 +0200
> >>>> David Hildenbrand <david@redhat.com> wrote:
> >>>>    
> >>>>> On 08.06.2018 09:34, Greg Kurz wrote:    
> >>>>>> On Thu,  7 Jun 2018 18:52:12 +0200
> >>>>>> David Hildenbrand <david@redhat.com> wrote:
> >>>>>>       
> >>>>>>> The node property can always be queried and the value has already been
> >>>>>>> verified in pc_dimm_realize().
> >>>>>>>
> >>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
> >>>>>>> ---
> >>>>>>>  hw/ppc/spapr.c | 9 +--------
> >>>>>>>  1 file changed, 1 insertion(+), 8 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>>>>>> index 2375cbee12..d038f3243e 100644
> >>>>>>> --- a/hw/ppc/spapr.c
> >>>>>>> +++ b/hw/ppc/spapr.c
> >>>>>>> @@ -3578,14 +3578,7 @@ 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) {      
> >>>>>>
> >>>>>> Good riddance :)
> >>>>>>       
> >>>>>>> -            return;
> >>>>>>> -        }
> >>>>>>> -        if (node < 0 || node >= MAX_NODES) {
> >>>>>>> -            error_setg(errp, "Invaild node %d", node);
> >>>>>>> -            return;
> >>>>>>> -        }    
> >>
> >> Maybe turn that into an assert() instead? ... just for the paranoids ;-)
> >>  
> >>>>>>> +        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL);      
> >>>>>>
> >>>>>> Maybe pass &error_abort ?      
> >>>>>
> >>>>> I'm using the same access scheme as in hw/acpi/memory_hotplug.c
> >>>>>
> >>>>> ("error ignored" vs. "error leads to an abort") - but this will actually
> >>>>> never fail. But I can use error_abort here, does not matter.
> >>>>>    
> >>>>
> >>>> Heh, /me paranoid but this is David's call and he acked that already
> >>>> so I guess it's okay.    
> >>>
> >>> NULL makes it fit into a single line :)    
> >>
> >> +1 for error_abort, even if it takes another line.  
> > +1 for error_abort
> > call shouldn't fail, but if does it won't be silently ignored
> > and introduce undefined behavior.  
> 
> Maybe we should fix the others that pass in NULL.
> 
> (no, not me :D - I'm already busy with your requested pre_plug handling)
Add it to wiki page for bite sized tasks?

> 
> >   
> >>
> >>  Thomas  
> >   
> 
> 

Re: [Qemu-devel] [PATCH v1 8/8] s390x: local error handling in hotplug handler functions

[David Hildenbrand]

On 08.06.2018 11:03, Igor Mammedov wrote:
> On Fri, 8 Jun 2018 09:40:04 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 08.06.2018 09:27, Christian Borntraeger wrote:
>>>
>>>
>>> On 06/08/2018 09:25 AM, Cornelia Huck wrote:  
>>>> On Thu,  7 Jun 2018 18:52:18 +0200
>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>  
>>>>> Let's introduce and use local error variables in the hotplug handler
>>>>> functions.
>>>>>
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>>  hw/s390x/s390-virtio-ccw.c | 11 ++++++++---
>>>>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>>> index 7ae5fb38dd..29ea50a177 100644
>>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>>> @@ -434,18 +434,23 @@ static void s390_machine_reset(void)
>>>>>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
>>>>>                                       DeviceState *dev, Error **errp)
>>>>>  {
>>>>> +    Error *local_err = NULL;
>>>>> +
>>>>>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>>>> -        s390_cpu_plug(hotplug_dev, dev, errp);
>>>>> +        s390_cpu_plug(hotplug_dev, dev, &local_err);
>>>>>      }
>>>>> +    error_propagate(errp, local_err);
>>>>>  }
>>>>>  
>>>>>  static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>>>>>                                                 DeviceState *dev, Error **errp)
>>>>>  {
>>>>> +    Error *local_err = NULL;
>>>>> +
>>>>>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>>>> -        error_setg(errp, "CPU hot unplug not supported on this machine");
>>>>> -        return;
>>>>> +        error_setg(&local_err, "CPU hot unplug not supported on this machine");
>>>>>      }
>>>>> +    error_propagate(errp, local_err);
>>>>>  }
>>>>>  
>>>>>  static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,  
>>>>
>>>> Just seeing this patch by itself, it does not really make much sense.
>>>> Even if this is a split out clean-up series, I'd prefer this to go
>>>> together with a patch that actually adds something more to the
>>>> plug/unplug functions.  
>>>
>>> +1. It is hard to see the "why". Maybe a better patch description could help here?
>>>   
>>
>> When checking for an error (*errp) we should make sure that we don't
>> dereference the NULL pointer. I will be doing that in the future (memory
>> devices), but as you both don't seem to like this patch, I'll drop it
>> for now.
> hotplug handlers aren't called with NULL errp, so it not really necessary.
> To be on the safe side we can ensure that errp is not NULL doing something
> like:

They are, but not on s390x :) But we should never life with such
assumptions - calling code may change. Passing NULL results right now
not in a crash, but the "return" value in case of a failure cannot be
indicated.

Maybe we should even change all users of NULL for errp to use &error_abort.

For now I dropped these "local_err" patches and kept only the spapr
cleanups.

> 
> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> index 17ac986..dc9e4bf 100644
> --- a/hw/core/hotplug.c
> +++ b/hw/core/hotplug.c
> @@ -52,6 +52,7 @@ void hotplug_handler_unplug(HotplugHandler *plug_handler,
>  {
>      HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
>  
> +    g_assert(errp);
>      if (hdc->unplug) {
>          hdc->unplug(plug_handler, plugged_dev, errp);
>      }
> 
> and do it for all similar wrappers in this file
> 
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[David Hildenbrand]

>>>> +1 for error_abort, even if it takes another line.  
>>> +1 for error_abort
>>> call shouldn't fail, but if does it won't be silently ignored
>>> and introduce undefined behavior.  
>>
>> Maybe we should fix the others that pass in NULL.
>>
>> (no, not me :D - I'm already busy with your requested pre_plug handling)
> Add it to wiki page for bite sized tasks?

Done.


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 6/8] spapr: handle pc-dimm unplug via hotplug handler chain

[Igor Mammedov]

On Fri, 8 Jun 2018 11:02:23 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 08.06.2018 10:56, Igor Mammedov wrote:
> > On Thu,  7 Jun 2018 18:52:16 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/
> >> unplug memory devices (which a pc-dimm is) later.  
> > Perhaps something like following would be better:
> > 
> > Factor out memory unplug into separate function from spapr_lmb_release().
> > Then use generic hotplug_handler_unplug() to trigger memory unplug,
> > which would 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.  
> 
> Can this be an addon patch? Sounds like factoring out more and moving more.
I've suggested ^^^ it as this patch description instead of the current one
that doesn't really makes the sense on it's own.

> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  hw/ppc/spapr.c | 18 +++++++++++++++---
> >>  1 file changed, 15 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index bcb72d9fa7..0a8a3455d6 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -3298,7 +3298,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
> >> @@ -3316,9 +3317,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);
> >>  }
> >> @@ -3589,6 +3598,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,  
> >   
> 
> 

Re: [Qemu-devel] [PATCH v1 6/8] spapr: handle pc-dimm unplug via hotplug handler chain

[David Hildenbrand]

On 08.06.2018 11:35, Igor Mammedov wrote:
> On Fri, 8 Jun 2018 11:02:23 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 08.06.2018 10:56, Igor Mammedov wrote:
>>> On Thu,  7 Jun 2018 18:52:16 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/
>>>> unplug memory devices (which a pc-dimm is) later.  
>>> Perhaps something like following would be better:
>>>
>>> Factor out memory unplug into separate function from spapr_lmb_release().
>>> Then use generic hotplug_handler_unplug() to trigger memory unplug,
>>> which would 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.  
>>
>> Can this be an addon patch? Sounds like factoring out more and moving more.
> I've suggested ^^^ it as this patch description instead of the current one
> that doesn't really makes the sense on it's own.

Okay, I was definitely misreading your comment :) Will change.


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[Greg Kurz]

On Fri, 8 Jun 2018 11:24:51 +0200
David Hildenbrand <david@redhat.com> wrote:

> >>>> +1 for error_abort, even if it takes another line.    
> >>> +1 for error_abort
> >>> call shouldn't fail, but if does it won't be silently ignored
> >>> and introduce undefined behavior.    
> >>
> >> Maybe we should fix the others that pass in NULL.
> >>
> >> (no, not me :D - I'm already busy with your requested pre_plug handling)  
> > Add it to wiki page for bite sized tasks?  
> 
> Done.
> 
> 

FWIW, I've also added a line to check and possibly fix places where we do
'if (*errp)', which would cause QEMU to crash if the caller passes NULL.

$ git grep 'if (\*errp)'
hmp.c:    if (*errp) {
hw/ipmi/isa_ipmi_bt.c:    if (*errp)
hw/ipmi/isa_ipmi_kcs.c:    if (*errp)
hw/mem/memory-device.c:    if (*errp) {
hw/mem/memory-device.c:        if (*errp) {
hw/ppc/spapr.c:        if (*errp) {
hw/s390x/event-facility.c:        if (*errp) {
include/qapi/error.h: *     if (*errp) { // WRONG!
qga/commands-posix.c:            if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:        if (*errp) {
target/s390x/cpu_models.c:            if (*errp) {
target/s390x/cpu_models.c:        if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
target/s390x/cpu_models.c:    if (*errp) {
tests/test-crypto-tlscredsx509.c:    if (*errp) {
tests/test-io-channel-tls.c:    if (*errp) {

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[David Hildenbrand]

On 08.06.2018 12:52, Greg Kurz wrote:
> On Fri, 8 Jun 2018 11:24:51 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>>>>>> +1 for error_abort, even if it takes another line.    
>>>>> +1 for error_abort
>>>>> call shouldn't fail, but if does it won't be silently ignored
>>>>> and introduce undefined behavior.    
>>>>
>>>> Maybe we should fix the others that pass in NULL.
>>>>
>>>> (no, not me :D - I'm already busy with your requested pre_plug handling)  
>>> Add it to wiki page for bite sized tasks?  
>>
>> Done.
>>
>>
> 
> FWIW, I've also added a line to check and possibly fix places where we do
> 'if (*errp)', which would cause QEMU to crash if the caller passes NULL.
> 
> $ git grep 'if (\*errp)'
> hmp.c:    if (*errp) {
> hw/ipmi/isa_ipmi_bt.c:    if (*errp)
> hw/ipmi/isa_ipmi_kcs.c:    if (*errp)
> hw/mem/memory-device.c:    if (*errp) {
> hw/mem/memory-device.c:        if (*errp) {
> hw/ppc/spapr.c:        if (*errp) {
> hw/s390x/event-facility.c:        if (*errp) {
> include/qapi/error.h: *     if (*errp) { // WRONG!
> qga/commands-posix.c:            if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> target/s390x/cpu_models.c:        if (*errp) {
> target/s390x/cpu_models.c:            if (*errp) {
> target/s390x/cpu_models.c:        if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> tests/test-crypto-tlscredsx509.c:    if (*errp) {
> tests/test-io-channel-tls.c:    if (*errp) {
> 

I think the more important part is actually looking out for people that
use NULL instead of error_abort. This way we won't silently ignore errors.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[Cornelia Huck]

On Fri, 8 Jun 2018 13:28:01 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 08.06.2018 12:52, Greg Kurz wrote:
> > On Fri, 8 Jun 2018 11:24:51 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >>>>>> +1 for error_abort, even if it takes another line.      
> >>>>> +1 for error_abort
> >>>>> call shouldn't fail, but if does it won't be silently ignored
> >>>>> and introduce undefined behavior.      
> >>>>
> >>>> Maybe we should fix the others that pass in NULL.
> >>>>
> >>>> (no, not me :D - I'm already busy with your requested pre_plug handling)    
> >>> Add it to wiki page for bite sized tasks?    
> >>
> >> Done.
> >>
> >>  
> > 
> > FWIW, I've also added a line to check and possibly fix places where we do
> > 'if (*errp)', which would cause QEMU to crash if the caller passes NULL.
> > 
> > $ git grep 'if (\*errp)'
> > hmp.c:    if (*errp) {
> > hw/ipmi/isa_ipmi_bt.c:    if (*errp)
> > hw/ipmi/isa_ipmi_kcs.c:    if (*errp)
> > hw/mem/memory-device.c:    if (*errp) {
> > hw/mem/memory-device.c:        if (*errp) {
> > hw/ppc/spapr.c:        if (*errp) {
> > hw/s390x/event-facility.c:        if (*errp) {
> > include/qapi/error.h: *     if (*errp) { // WRONG!
> > qga/commands-posix.c:            if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > target/s390x/cpu_models.c:        if (*errp) {
> > target/s390x/cpu_models.c:            if (*errp) {
> > target/s390x/cpu_models.c:        if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > tests/test-crypto-tlscredsx509.c:    if (*errp) {
> > tests/test-io-channel-tls.c:    if (*errp) {
> >   
> 
> I think the more important part is actually looking out for people that
> use NULL instead of error_abort. This way we won't silently ignore errors.

I think we can assume that the callers here all pass in !NULL. Would
probably make sense to change these anyway because (a) better safe than
sorry, and (b) make sure new code does not copy it.

Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[Greg Kurz]

On Fri, 8 Jun 2018 13:28:01 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 08.06.2018 12:52, Greg Kurz wrote:
> > On Fri, 8 Jun 2018 11:24:51 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >>>>>> +1 for error_abort, even if it takes another line.      
> >>>>> +1 for error_abort
> >>>>> call shouldn't fail, but if does it won't be silently ignored
> >>>>> and introduce undefined behavior.      
> >>>>
> >>>> Maybe we should fix the others that pass in NULL.
> >>>>
> >>>> (no, not me :D - I'm already busy with your requested pre_plug handling)    
> >>> Add it to wiki page for bite sized tasks?    
> >>
> >> Done.
> >>
> >>  
> > 
> > FWIW, I've also added a line to check and possibly fix places where we do
> > 'if (*errp)', which would cause QEMU to crash if the caller passes NULL.
> > 
> > $ git grep 'if (\*errp)'
> > hmp.c:    if (*errp) {
> > hw/ipmi/isa_ipmi_bt.c:    if (*errp)
> > hw/ipmi/isa_ipmi_kcs.c:    if (*errp)
> > hw/mem/memory-device.c:    if (*errp) {
> > hw/mem/memory-device.c:        if (*errp) {
> > hw/ppc/spapr.c:        if (*errp) {
> > hw/s390x/event-facility.c:        if (*errp) {
> > include/qapi/error.h: *     if (*errp) { // WRONG!
> > qga/commands-posix.c:            if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > target/s390x/cpu_models.c:        if (*errp) {
> > target/s390x/cpu_models.c:            if (*errp) {
> > target/s390x/cpu_models.c:        if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > target/s390x/cpu_models.c:    if (*errp) {
> > tests/test-crypto-tlscredsx509.c:    if (*errp) {
> > tests/test-io-channel-tls.c:    if (*errp) {
> >   
> 
> I think the more important part is actually looking out for people that
> use NULL instead of error_abort. This way we won't silently ignore errors.
> 

I agree that we should probably pass &error_abort in many places,
but passing NULL isn't bad per se. It means any failure in the
callee is unimportant enough that we can simply ignore it.

The error framework provides this possibility and so we should
never dereference errp.