[Qemu-devel] [PATCH 0/5] spapr_cpu_core: fixes and cleanups

[Qemu-devel] [PATCH 0/5] spapr_cpu_core: fixes and cleanups

[Greg Kurz]

This series is a follow-up to David's "Better handling of machine specific
per-cpu information" v3 patchset. It addresses issues mentioned at:

https://lists.nongnu.org/archive/html/qemu-ppc/2018-06/msg00514.html

This series fixes potential crashes and leaks in case of error during
CPU hotplug. It hence assumes that the machine_data pointer is set a
bit earlier as suggested at the above URL, for CPU hotplug to work.

Patch 1 is just code cleanup.

Patch 2 fixes a 2.12 regression.

Patch 3 fixes a long standing issue. It could possibly be fixed in
current master but this would require to rebase David's series on
top of it. Not sure it's worth the pain.

Patch 4 and 5 are some more cleanup.

--
Greg

---

Greg Kurz (5):
      spapr_cpu_core: convert last snprintf() to g_strdup_printf()
      spapr_cpu_core: fix potential leak in spapr_cpu_core_realize()
      spapr_cpu_core: add missing rollback on realization path
      spapr_cpu_core: introduce spapr_create_vcpu()
      spapr_cpu_core: simplify spapr_cpu_core_realize()


 hw/ppc/spapr_cpu_core.c |   93 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 60 insertions(+), 33 deletions(-)

[Qemu-devel] [PATCH 1/5] spapr_cpu_core: convert last snprintf() to g_strdup_printf()

[Greg Kurz]

Because this is the preferred practice in QEMU.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_cpu_core.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 7e3a9e78d090..27602245fd55 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -190,7 +190,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
 
     sc->threads = g_new(PowerPCCPU *, cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
-        char id[32];
+        char *id;
         CPUState *cs;
         PowerPCCPU *cpu;
 
@@ -208,8 +208,9 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
         /* Set NUMA node for the threads belonged to core  */
         cpu->node_id = sc->node_id;
 
-        snprintf(id, sizeof(id), "thread[%d]", i);
+        id = g_strdup_printf("thread[%d]", i);
         object_property_add_child(OBJECT(sc), id, obj, &local_err);
+        g_free(id);
         if (local_err) {
             goto err;
         }

[Qemu-devel] [PATCH 2/5] spapr_cpu_core: fix potential leak in spapr_cpu_core_realize()

[Greg Kurz]

Commit 94ad93bd97684 (QEMU 2.12) switched to instantiate CPUs separately
but it missed to adapt the error path accordingly. If something fails in
the CPU creation loop, then the CPU object that was just created is leaked.

The error paths in this function are a bit obfuscated, and adding
yet another label to free this CPU object makes it worse. We should
move the block of the loop to a separate function, with a proper
rollback path, but this is a bigger cleanup.

For now, let's just fix the bug by adding the missing calls to
object_unref(). This will allow easier backport to older QEMU
versions.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_cpu_core.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 27602245fd55..003c4c5a79d2 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -201,6 +201,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
         cs->cpu_index = cc->core_id + i;
         spapr_set_vcpu_id(cpu, cs->cpu_index, &local_err);
         if (local_err) {
+            object_unref(obj);
             goto err;
         }
 
@@ -212,6 +213,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
         object_property_add_child(OBJECT(sc), id, obj, &local_err);
         g_free(id);
         if (local_err) {
+            object_unref(obj);
             goto err;
         }
         object_unref(obj);

[Qemu-devel] [PATCH 3/5] spapr_cpu_core: add missing rollback on realization path

[Greg Kurz]

The spapr_realize_vcpu() function doesn't rollback in case of error.
This isn't a problem with coldplugged CPUs because the machine won't
start and QEMU will exit. Hotplug is a different story though: the
CPU thread is started under object_property_set_bool() and it assumes
it can access the CPU object.

If icp_create() fails, we return an error without unregistering the
reset handler for this CPU, and we let the underlying QEMU thread for
this CPU alive. Since spapr_cpu_core_realize() doesn't care to unrealize
already realized CPUs either, but happily frees all of them anyway, the
CPU thread crashes instantly:

(qemu) device_add host-spapr-cpu-core,core-id=1,id=gku
GKU: failing icp_create (cpu 0x11497fd0)
                             ^^^^^^^^^^
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffee3feaa0 (LWP 24725)]
0x00000000104c8374 in object_dynamic_cast_assert (obj=0x11497fd0,
                                                  ^^^^^^^^^^^^^^
                                             pointer to the CPU object
623         trace_object_dynamic_cast_assert(obj ? obj->class->type->name
(gdb) p obj->class->type
$1 = (Type) 0x0
(gdb) p * obj
$2 = {class = 0x10ea9c10, free = 0x11244620,
                                 ^^^^^^^^^^
                              should be g_free
(gdb) p g_free
$3 = {<text variable, no debug info>} 0x7ffff282bef0 <g_free>

obj is a dangling pointer to the CPU that was just destroyed in
spapr_cpu_core_realize().

This patch adds proper rollback to both spapr_realize_vcpu() and
spapr_cpu_core_realize().

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_cpu_core.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 003c4c5a79d2..04c818a6ecac 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -159,12 +159,16 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     spapr_cpu->icp = icp_create(OBJECT(cpu), spapr->icp_type,
                                 XICS_FABRIC(spapr), &local_err);
     if (local_err) {
-        goto error;
+        goto error_unregister;
     }
 
     return;
 
+error_unregister:
+    qemu_unregister_reset(spapr_cpu_reset, cpu);
+    cpu_remove_sync(CPU(cpu));
 error:
+    g_free(spapr_cpu);
     error_propagate(errp, local_err);
 }
 
@@ -222,11 +226,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
     for (j = 0; j < cc->nr_threads; j++) {
         spapr_realize_vcpu(sc->threads[j], spapr, &local_err);
         if (local_err) {
-            goto err;
+            goto err_unrealize;
         }
     }
     return;
 
+err_unrealize:
+    while (--j >= 0) {
+        spapr_unrealize_vcpu(sc->threads[i]);
+    }
 err:
     while (--i >= 0) {
         obj = OBJECT(sc->threads[i]);

[Qemu-devel] [PATCH 4/5] spapr_cpu_core: introduce spapr_create_vcpu()

[Greg Kurz]

This moves some code out from spapr_cpu_core_realize() for clarity. No
functional change.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_cpu_core.c |   73 +++++++++++++++++++++++++++++------------------
 1 file changed, 45 insertions(+), 28 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 04c818a6ecac..0ebaf804a9bc 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -172,6 +172,49 @@ error:
     error_propagate(errp, local_err);
 }
 
+static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i, Error **errp)
+{
+    sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(sc);
+    CPUCore *cc = CPU_CORE(sc);
+    Object *obj;
+    char *id;
+    CPUState *cs;
+    PowerPCCPU *cpu;
+    Error *local_err = NULL;
+
+    obj = object_new(scc->cpu_type);
+
+    cs = CPU(obj);
+    cpu = POWERPC_CPU(obj);
+    cs->cpu_index = cc->core_id + i;
+    spapr_set_vcpu_id(cpu, cs->cpu_index, &local_err);
+    if (local_err) {
+        goto err;
+    }
+
+    cpu->node_id = sc->node_id;
+
+    id = g_strdup_printf("thread[%d]", i);
+    object_property_add_child(OBJECT(sc), id, obj, &local_err);
+    g_free(id);
+    if (local_err) {
+        goto err;
+    }
+
+    object_unref(obj);
+    return cpu;
+
+err:
+    object_unref(obj);
+    error_propagate(errp, local_err);
+    return NULL;
+}
+
+static void spapr_delete_vcpu(PowerPCCPU *cpu)
+{
+    object_unparent(OBJECT(cpu));
+}
+
 static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
 {
     /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
@@ -181,10 +224,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
         (sPAPRMachineState *) object_dynamic_cast(qdev_get_machine(),
                                                   TYPE_SPAPR_MACHINE);
     sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
-    sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev));
     CPUCore *cc = CPU_CORE(OBJECT(dev));
     Error *local_err = NULL;
-    Object *obj;
     int i, j;
 
     if (!spapr) {
@@ -194,33 +235,10 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
 
     sc->threads = g_new(PowerPCCPU *, cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
-        char *id;
-        CPUState *cs;
-        PowerPCCPU *cpu;
-
-        obj = object_new(scc->cpu_type);
-
-        cs = CPU(obj);
-        cpu = sc->threads[i] = POWERPC_CPU(obj);
-        cs->cpu_index = cc->core_id + i;
-        spapr_set_vcpu_id(cpu, cs->cpu_index, &local_err);
-        if (local_err) {
-            object_unref(obj);
-            goto err;
-        }
-
-
-        /* Set NUMA node for the threads belonged to core  */
-        cpu->node_id = sc->node_id;
-
-        id = g_strdup_printf("thread[%d]", i);
-        object_property_add_child(OBJECT(sc), id, obj, &local_err);
-        g_free(id);
+        sc->threads[i] = spapr_create_vcpu(sc, i, &local_err);
         if (local_err) {
-            object_unref(obj);
             goto err;
         }
-        object_unref(obj);
     }
 
     for (j = 0; j < cc->nr_threads; j++) {
@@ -237,8 +255,7 @@ err_unrealize:
     }
 err:
     while (--i >= 0) {
-        obj = OBJECT(sc->threads[i]);
-        object_unparent(obj);
+        spapr_delete_vcpu(sc->threads[i]);
     }
     g_free(sc->threads);
     error_propagate(errp, local_err);

[Qemu-devel] [PATCH 5/5] spapr_cpu_core: simplify spapr_cpu_core_realize()

[Greg Kurz]

There's no real reason to create all CPUs in a first pass and to realize
them in a second pass. Merging these two loops makes the code simpler.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_cpu_core.c |   25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 0ebaf804a9bc..f52af20e0096 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -172,7 +172,8 @@ error:
     error_propagate(errp, local_err);
 }
 
-static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i, Error **errp)
+static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i,
+                                     sPAPRMachineState *spapr, Error **errp)
 {
     sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(sc);
     CPUCore *cc = CPU_CORE(sc);
@@ -201,9 +202,16 @@ static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i, Error **errp)
         goto err;
     }
 
+    spapr_realize_vcpu(cpu, spapr, &local_err);
+    if (local_err) {
+        goto err_unparent;
+    }
+
     object_unref(obj);
     return cpu;
 
+err_unparent:
+    object_unparent(obj);
 err:
     object_unref(obj);
     error_propagate(errp, local_err);
@@ -212,6 +220,7 @@ err:
 
 static void spapr_delete_vcpu(PowerPCCPU *cpu)
 {
+    spapr_unrealize_vcpu(cpu);
     object_unparent(OBJECT(cpu));
 }
 
@@ -226,7 +235,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
     sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
     CPUCore *cc = CPU_CORE(OBJECT(dev));
     Error *local_err = NULL;
-    int i, j;
+    int i;
 
     if (!spapr) {
         error_setg(errp, TYPE_SPAPR_CPU_CORE " needs a pseries machine");
@@ -235,24 +244,14 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
 
     sc->threads = g_new(PowerPCCPU *, cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
-        sc->threads[i] = spapr_create_vcpu(sc, i, &local_err);
+        sc->threads[i] = spapr_create_vcpu(sc, i, spapr, &local_err);
         if (local_err) {
             goto err;
         }
     }
 
-    for (j = 0; j < cc->nr_threads; j++) {
-        spapr_realize_vcpu(sc->threads[j], spapr, &local_err);
-        if (local_err) {
-            goto err_unrealize;
-        }
-    }
     return;
 
-err_unrealize:
-    while (--j >= 0) {
-        spapr_unrealize_vcpu(sc->threads[i]);
-    }
 err:
     while (--i >= 0) {
         spapr_delete_vcpu(sc->threads[i]);

Re: [Qemu-devel] [PATCH 1/5] spapr_cpu_core: convert last snprintf() to g_strdup_printf()

[David Gibson]

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

On Thu, Jun 14, 2018 at 11:50:11PM +0200, Greg Kurz wrote:
> Because this is the preferred practice in QEMU.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-3.0, thanks.

> ---
>  hw/ppc/spapr_cpu_core.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 7e3a9e78d090..27602245fd55 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -190,7 +190,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>  
>      sc->threads = g_new(PowerPCCPU *, cc->nr_threads);
>      for (i = 0; i < cc->nr_threads; i++) {
> -        char id[32];
> +        char *id;
>          CPUState *cs;
>          PowerPCCPU *cpu;
>  
> @@ -208,8 +208,9 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>          /* Set NUMA node for the threads belonged to core  */
>          cpu->node_id = sc->node_id;
>  
> -        snprintf(id, sizeof(id), "thread[%d]", i);
> +        id = g_strdup_printf("thread[%d]", i);
>          object_property_add_child(OBJECT(sc), id, obj, &local_err);
> +        g_free(id);
>          if (local_err) {
>              goto err;
>          }
> 

-- 
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 2/5] spapr_cpu_core: fix potential leak in spapr_cpu_core_realize()

[David Gibson]

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

On Thu, Jun 14, 2018 at 11:50:27PM +0200, Greg Kurz wrote:
> Commit 94ad93bd97684 (QEMU 2.12) switched to instantiate CPUs separately
> but it missed to adapt the error path accordingly. If something fails in
> the CPU creation loop, then the CPU object that was just created is leaked.
> 
> The error paths in this function are a bit obfuscated, and adding
> yet another label to free this CPU object makes it worse. We should
> move the block of the loop to a separate function, with a proper
> rollback path, but this is a bigger cleanup.
> 
> For now, let's just fix the bug by adding the missing calls to
> object_unref(). This will allow easier backport to older QEMU
> versions.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-3.0, thanks.

> ---
>  hw/ppc/spapr_cpu_core.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 27602245fd55..003c4c5a79d2 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -201,6 +201,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>          cs->cpu_index = cc->core_id + i;
>          spapr_set_vcpu_id(cpu, cs->cpu_index, &local_err);
>          if (local_err) {
> +            object_unref(obj);
>              goto err;
>          }
>  
> @@ -212,6 +213,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>          object_property_add_child(OBJECT(sc), id, obj, &local_err);
>          g_free(id);
>          if (local_err) {
> +            object_unref(obj);
>              goto err;
>          }
>          object_unref(obj);
> 

-- 
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 3/5] spapr_cpu_core: add missing rollback on realization path

[David Gibson]

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

On Thu, Jun 14, 2018 at 11:50:42PM +0200, Greg Kurz wrote:
> The spapr_realize_vcpu() function doesn't rollback in case of error.
> This isn't a problem with coldplugged CPUs because the machine won't
> start and QEMU will exit. Hotplug is a different story though: the
> CPU thread is started under object_property_set_bool() and it assumes
> it can access the CPU object.
> 
> If icp_create() fails, we return an error without unregistering the
> reset handler for this CPU, and we let the underlying QEMU thread for
> this CPU alive. Since spapr_cpu_core_realize() doesn't care to unrealize
> already realized CPUs either, but happily frees all of them anyway, the
> CPU thread crashes instantly:
> 
> (qemu) device_add host-spapr-cpu-core,core-id=1,id=gku
> GKU: failing icp_create (cpu 0x11497fd0)
>                              ^^^^^^^^^^
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7fffee3feaa0 (LWP 24725)]
> 0x00000000104c8374 in object_dynamic_cast_assert (obj=0x11497fd0,
>                                                   ^^^^^^^^^^^^^^
>                                              pointer to the CPU object
> 623         trace_object_dynamic_cast_assert(obj ? obj->class->type->name
> (gdb) p obj->class->type
> $1 = (Type) 0x0
> (gdb) p * obj
> $2 = {class = 0x10ea9c10, free = 0x11244620,
>                                  ^^^^^^^^^^
>                               should be g_free
> (gdb) p g_free
> $3 = {<text variable, no debug info>} 0x7ffff282bef0 <g_free>
> 
> obj is a dangling pointer to the CPU that was just destroyed in
> spapr_cpu_core_realize().
> 
> This patch adds proper rollback to both spapr_realize_vcpu() and
> spapr_cpu_core_realize().
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-3.0, since it definitely looks to fix some
problems.

> ---
>  hw/ppc/spapr_cpu_core.c |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 003c4c5a79d2..04c818a6ecac 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -159,12 +159,16 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      spapr_cpu->icp = icp_create(OBJECT(cpu), spapr->icp_type,
>                                  XICS_FABRIC(spapr), &local_err);
>      if (local_err) {
> -        goto error;
> +        goto error_unregister;
>      }
>  
>      return;
>  
> +error_unregister:
> +    qemu_unregister_reset(spapr_cpu_reset, cpu);
> +    cpu_remove_sync(CPU(cpu));

I'm a little unclear on exactly what init the cpu_remove_sync() is
mirroring, though.

>  error:
> +    g_free(spapr_cpu);
>      error_propagate(errp, local_err);
>  }
>  
> @@ -222,11 +226,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>      for (j = 0; j < cc->nr_threads; j++) {
>          spapr_realize_vcpu(sc->threads[j], spapr, &local_err);
>          if (local_err) {
> -            goto err;
> +            goto err_unrealize;
>          }
>      }
>      return;
>  
> +err_unrealize:
> +    while (--j >= 0) {
> +        spapr_unrealize_vcpu(sc->threads[i]);
> +    }
>  err:
>      while (--i >= 0) {
>          obj = OBJECT(sc->threads[i]);
> 

-- 
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 4/5] spapr_cpu_core: introduce spapr_create_vcpu()

[David Gibson]

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

On Thu, Jun 14, 2018 at 11:50:57PM +0200, Greg Kurz wrote:
> This moves some code out from spapr_cpu_core_realize() for clarity. No
> functional change.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-3.0, thanks.

> ---
>  hw/ppc/spapr_cpu_core.c |   73 +++++++++++++++++++++++++++++------------------
>  1 file changed, 45 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 04c818a6ecac..0ebaf804a9bc 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -172,6 +172,49 @@ error:
>      error_propagate(errp, local_err);
>  }
>  
> +static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i, Error **errp)
> +{
> +    sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(sc);
> +    CPUCore *cc = CPU_CORE(sc);
> +    Object *obj;
> +    char *id;
> +    CPUState *cs;
> +    PowerPCCPU *cpu;
> +    Error *local_err = NULL;
> +
> +    obj = object_new(scc->cpu_type);
> +
> +    cs = CPU(obj);
> +    cpu = POWERPC_CPU(obj);
> +    cs->cpu_index = cc->core_id + i;
> +    spapr_set_vcpu_id(cpu, cs->cpu_index, &local_err);
> +    if (local_err) {
> +        goto err;
> +    }
> +
> +    cpu->node_id = sc->node_id;
> +
> +    id = g_strdup_printf("thread[%d]", i);
> +    object_property_add_child(OBJECT(sc), id, obj, &local_err);
> +    g_free(id);
> +    if (local_err) {
> +        goto err;
> +    }
> +
> +    object_unref(obj);
> +    return cpu;
> +
> +err:
> +    object_unref(obj);
> +    error_propagate(errp, local_err);
> +    return NULL;
> +}
> +
> +static void spapr_delete_vcpu(PowerPCCPU *cpu)
> +{
> +    object_unparent(OBJECT(cpu));
> +}
> +
>  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>  {
>      /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
> @@ -181,10 +224,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>          (sPAPRMachineState *) object_dynamic_cast(qdev_get_machine(),
>                                                    TYPE_SPAPR_MACHINE);
>      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> -    sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev));
>      CPUCore *cc = CPU_CORE(OBJECT(dev));
>      Error *local_err = NULL;
> -    Object *obj;
>      int i, j;
>  
>      if (!spapr) {
> @@ -194,33 +235,10 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>  
>      sc->threads = g_new(PowerPCCPU *, cc->nr_threads);
>      for (i = 0; i < cc->nr_threads; i++) {
> -        char *id;
> -        CPUState *cs;
> -        PowerPCCPU *cpu;
> -
> -        obj = object_new(scc->cpu_type);
> -
> -        cs = CPU(obj);
> -        cpu = sc->threads[i] = POWERPC_CPU(obj);
> -        cs->cpu_index = cc->core_id + i;
> -        spapr_set_vcpu_id(cpu, cs->cpu_index, &local_err);
> -        if (local_err) {
> -            object_unref(obj);
> -            goto err;
> -        }
> -
> -
> -        /* Set NUMA node for the threads belonged to core  */
> -        cpu->node_id = sc->node_id;
> -
> -        id = g_strdup_printf("thread[%d]", i);
> -        object_property_add_child(OBJECT(sc), id, obj, &local_err);
> -        g_free(id);
> +        sc->threads[i] = spapr_create_vcpu(sc, i, &local_err);
>          if (local_err) {
> -            object_unref(obj);
>              goto err;
>          }
> -        object_unref(obj);
>      }
>  
>      for (j = 0; j < cc->nr_threads; j++) {
> @@ -237,8 +255,7 @@ err_unrealize:
>      }
>  err:
>      while (--i >= 0) {
> -        obj = OBJECT(sc->threads[i]);
> -        object_unparent(obj);
> +        spapr_delete_vcpu(sc->threads[i]);
>      }
>      g_free(sc->threads);
>      error_propagate(errp, local_err);
> 

-- 
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 5/5] spapr_cpu_core: simplify spapr_cpu_core_realize()

[David Gibson]

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

On Thu, Jun 14, 2018 at 11:51:11PM +0200, Greg Kurz wrote:
> There's no real reason to create all CPUs in a first pass and to realize
> them in a second pass. Merging these two loops makes the code simpler.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

I'm a bit uncertain about this one.  It's correct at the moment, but
I'm wondering if there might be things we want to do wile the cpu
objects are constructed, but not initialized.

In fact, I thought I wanted something like that for allowing earlier
initialization of the default caps values, though in the end I found a
simpler and better approach.

So, I'm holding off on this one for the time being.

> ---
>  hw/ppc/spapr_cpu_core.c |   25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 0ebaf804a9bc..f52af20e0096 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -172,7 +172,8 @@ error:
>      error_propagate(errp, local_err);
>  }
>  
> -static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i, Error **errp)
> +static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i,
> +                                     sPAPRMachineState *spapr, Error **errp)
>  {
>      sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(sc);
>      CPUCore *cc = CPU_CORE(sc);
> @@ -201,9 +202,16 @@ static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i, Error **errp)
>          goto err;
>      }
>  
> +    spapr_realize_vcpu(cpu, spapr, &local_err);
> +    if (local_err) {
> +        goto err_unparent;
> +    }
> +
>      object_unref(obj);
>      return cpu;
>  
> +err_unparent:
> +    object_unparent(obj);
>  err:
>      object_unref(obj);
>      error_propagate(errp, local_err);
> @@ -212,6 +220,7 @@ err:
>  
>  static void spapr_delete_vcpu(PowerPCCPU *cpu)
>  {
> +    spapr_unrealize_vcpu(cpu);
>      object_unparent(OBJECT(cpu));
>  }
>  
> @@ -226,7 +235,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
>      CPUCore *cc = CPU_CORE(OBJECT(dev));
>      Error *local_err = NULL;
> -    int i, j;
> +    int i;
>  
>      if (!spapr) {
>          error_setg(errp, TYPE_SPAPR_CPU_CORE " needs a pseries machine");
> @@ -235,24 +244,14 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>  
>      sc->threads = g_new(PowerPCCPU *, cc->nr_threads);
>      for (i = 0; i < cc->nr_threads; i++) {
> -        sc->threads[i] = spapr_create_vcpu(sc, i, &local_err);
> +        sc->threads[i] = spapr_create_vcpu(sc, i, spapr, &local_err);
>          if (local_err) {
>              goto err;
>          }
>      }
>  
> -    for (j = 0; j < cc->nr_threads; j++) {
> -        spapr_realize_vcpu(sc->threads[j], spapr, &local_err);
> -        if (local_err) {
> -            goto err_unrealize;
> -        }
> -    }
>      return;
>  
> -err_unrealize:
> -    while (--j >= 0) {
> -        spapr_unrealize_vcpu(sc->threads[i]);
> -    }
>  err:
>      while (--i >= 0) {
>          spapr_delete_vcpu(sc->threads[i]);
> 

-- 
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 3/5] spapr_cpu_core: add missing rollback on realization path

[David Gibson]

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

On Fri, Jun 15, 2018 at 10:02:25AM +1000, David Gibson wrote:
> On Thu, Jun 14, 2018 at 11:50:42PM +0200, Greg Kurz wrote:
> > The spapr_realize_vcpu() function doesn't rollback in case of error.
> > This isn't a problem with coldplugged CPUs because the machine won't
> > start and QEMU will exit. Hotplug is a different story though: the
> > CPU thread is started under object_property_set_bool() and it assumes
> > it can access the CPU object.
> > 
> > If icp_create() fails, we return an error without unregistering the
> > reset handler for this CPU, and we let the underlying QEMU thread for
> > this CPU alive. Since spapr_cpu_core_realize() doesn't care to unrealize
> > already realized CPUs either, but happily frees all of them anyway, the
> > CPU thread crashes instantly:
> > 
> > (qemu) device_add host-spapr-cpu-core,core-id=1,id=gku
> > GKU: failing icp_create (cpu 0x11497fd0)
> >                              ^^^^^^^^^^
> > Program received signal SIGSEGV, Segmentation fault.
> > [Switching to Thread 0x7fffee3feaa0 (LWP 24725)]
> > 0x00000000104c8374 in object_dynamic_cast_assert (obj=0x11497fd0,
> >                                                   ^^^^^^^^^^^^^^
> >                                              pointer to the CPU object
> > 623         trace_object_dynamic_cast_assert(obj ? obj->class->type->name
> > (gdb) p obj->class->type
> > $1 = (Type) 0x0
> > (gdb) p * obj
> > $2 = {class = 0x10ea9c10, free = 0x11244620,
> >                                  ^^^^^^^^^^
> >                               should be g_free
> > (gdb) p g_free
> > $3 = {<text variable, no debug info>} 0x7ffff282bef0 <g_free>
> > 
> > obj is a dangling pointer to the CPU that was just destroyed in
> > spapr_cpu_core_realize().
> > 
> > This patch adds proper rollback to both spapr_realize_vcpu() and
> > spapr_cpu_core_realize().
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> Applied to ppc-for-3.0, since it definitely looks to fix some
> problems.

Uh.. actually it has a definite bug - the first exit point will call
g_free() on an uninitialized spapr_cpu.  I fixed it up with a NULL
initialization in my tree.

> 
> > ---
> >  hw/ppc/spapr_cpu_core.c |   12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 003c4c5a79d2..04c818a6ecac 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -159,12 +159,16 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >      spapr_cpu->icp = icp_create(OBJECT(cpu), spapr->icp_type,
> >                                  XICS_FABRIC(spapr), &local_err);
> >      if (local_err) {
> > -        goto error;
> > +        goto error_unregister;
> >      }
> >  
> >      return;
> >  
> > +error_unregister:
> > +    qemu_unregister_reset(spapr_cpu_reset, cpu);
> > +    cpu_remove_sync(CPU(cpu));
> 
> I'm a little unclear on exactly what init the cpu_remove_sync() is
> mirroring, though.
> 
> >  error:
> > +    g_free(spapr_cpu);
> >      error_propagate(errp, local_err);
> >  }
> >  
> > @@ -222,11 +226,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >      for (j = 0; j < cc->nr_threads; j++) {
> >          spapr_realize_vcpu(sc->threads[j], spapr, &local_err);
> >          if (local_err) {
> > -            goto err;
> > +            goto err_unrealize;
> >          }
> >      }
> >      return;
> >  
> > +err_unrealize:
> > +    while (--j >= 0) {
> > +        spapr_unrealize_vcpu(sc->threads[i]);
> > +    }
> >  err:
> >      while (--i >= 0) {
> >          obj = OBJECT(sc->threads[i]);
> > 
> 



-- 
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 3/5] spapr_cpu_core: add missing rollback on realization path

[Greg Kurz]

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

On Fri, 15 Jun 2018 10:02:25 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jun 14, 2018 at 11:50:42PM +0200, Greg Kurz wrote:
> > The spapr_realize_vcpu() function doesn't rollback in case of error.
> > This isn't a problem with coldplugged CPUs because the machine won't
> > start and QEMU will exit. Hotplug is a different story though: the
> > CPU thread is started under object_property_set_bool() and it assumes
> > it can access the CPU object.
> > 
> > If icp_create() fails, we return an error without unregistering the
> > reset handler for this CPU, and we let the underlying QEMU thread for
> > this CPU alive. Since spapr_cpu_core_realize() doesn't care to unrealize
> > already realized CPUs either, but happily frees all of them anyway, the
> > CPU thread crashes instantly:
> > 
> > (qemu) device_add host-spapr-cpu-core,core-id=1,id=gku
> > GKU: failing icp_create (cpu 0x11497fd0)
> >                              ^^^^^^^^^^
> > Program received signal SIGSEGV, Segmentation fault.
> > [Switching to Thread 0x7fffee3feaa0 (LWP 24725)]
> > 0x00000000104c8374 in object_dynamic_cast_assert (obj=0x11497fd0,
> >                                                   ^^^^^^^^^^^^^^
> >                                              pointer to the CPU object
> > 623         trace_object_dynamic_cast_assert(obj ? obj->class->type->name
> > (gdb) p obj->class->type
> > $1 = (Type) 0x0
> > (gdb) p * obj
> > $2 = {class = 0x10ea9c10, free = 0x11244620,
> >                                  ^^^^^^^^^^
> >                               should be g_free
> > (gdb) p g_free
> > $3 = {<text variable, no debug info>} 0x7ffff282bef0 <g_free>
> > 
> > obj is a dangling pointer to the CPU that was just destroyed in
> > spapr_cpu_core_realize().
> > 
> > This patch adds proper rollback to both spapr_realize_vcpu() and
> > spapr_cpu_core_realize().
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> Applied to ppc-for-3.0, since it definitely looks to fix some
> problems.
> 
> > ---
> >  hw/ppc/spapr_cpu_core.c |   12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 003c4c5a79d2..04c818a6ecac 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -159,12 +159,16 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >      spapr_cpu->icp = icp_create(OBJECT(cpu), spapr->icp_type,
> >                                  XICS_FABRIC(spapr), &local_err);
> >      if (local_err) {
> > -        goto error;
> > +        goto error_unregister;
> >      }
> >  
> >      return;
> >  
> > +error_unregister:
> > +    qemu_unregister_reset(spapr_cpu_reset, cpu);
> > +    cpu_remove_sync(CPU(cpu));  
> 
> I'm a little unclear on exactly what init the cpu_remove_sync() is
> mirroring, though.
> 

We have the same call in spapr_unrealize_vcpu(). IIUC it is mirroring
object_property_set_bool(OBJECT(cpu), true, "realized", &local_err).

> >  error:
> > +    g_free(spapr_cpu);
> >      error_propagate(errp, local_err);
> >  }
> >  
> > @@ -222,11 +226,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >      for (j = 0; j < cc->nr_threads; j++) {
> >          spapr_realize_vcpu(sc->threads[j], spapr, &local_err);
> >          if (local_err) {
> > -            goto err;
> > +            goto err_unrealize;
> >          }
> >      }
> >      return;
> >  
> > +err_unrealize:
> > +    while (--j >= 0) {
> > +        spapr_unrealize_vcpu(sc->threads[i]);
> > +    }
> >  err:
> >      while (--i >= 0) {
> >          obj = OBJECT(sc->threads[i]);
> >   
> 


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

Re: [Qemu-devel] [PATCH 3/5] spapr_cpu_core: add missing rollback on realization path

[Greg Kurz]

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

On Fri, 15 Jun 2018 10:14:31 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jun 15, 2018 at 10:02:25AM +1000, David Gibson wrote:
> > On Thu, Jun 14, 2018 at 11:50:42PM +0200, Greg Kurz wrote:  
> > > The spapr_realize_vcpu() function doesn't rollback in case of error.
> > > This isn't a problem with coldplugged CPUs because the machine won't
> > > start and QEMU will exit. Hotplug is a different story though: the
> > > CPU thread is started under object_property_set_bool() and it assumes
> > > it can access the CPU object.
> > > 
> > > If icp_create() fails, we return an error without unregistering the
> > > reset handler for this CPU, and we let the underlying QEMU thread for
> > > this CPU alive. Since spapr_cpu_core_realize() doesn't care to unrealize
> > > already realized CPUs either, but happily frees all of them anyway, the
> > > CPU thread crashes instantly:
> > > 
> > > (qemu) device_add host-spapr-cpu-core,core-id=1,id=gku
> > > GKU: failing icp_create (cpu 0x11497fd0)
> > >                              ^^^^^^^^^^
> > > Program received signal SIGSEGV, Segmentation fault.
> > > [Switching to Thread 0x7fffee3feaa0 (LWP 24725)]
> > > 0x00000000104c8374 in object_dynamic_cast_assert (obj=0x11497fd0,
> > >                                                   ^^^^^^^^^^^^^^
> > >                                              pointer to the CPU object
> > > 623         trace_object_dynamic_cast_assert(obj ? obj->class->type->name
> > > (gdb) p obj->class->type
> > > $1 = (Type) 0x0
> > > (gdb) p * obj
> > > $2 = {class = 0x10ea9c10, free = 0x11244620,
> > >                                  ^^^^^^^^^^
> > >                               should be g_free
> > > (gdb) p g_free
> > > $3 = {<text variable, no debug info>} 0x7ffff282bef0 <g_free>
> > > 
> > > obj is a dangling pointer to the CPU that was just destroyed in
> > > spapr_cpu_core_realize().
> > > 
> > > This patch adds proper rollback to both spapr_realize_vcpu() and
> > > spapr_cpu_core_realize().
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>  
> > 
> > Applied to ppc-for-3.0, since it definitely looks to fix some
> > problems.  
> 
> Uh.. actually it has a definite bug - the first exit point will call
> g_free() on an uninitialized spapr_cpu.  I fixed it up with a NULL
> initialization in my tree.
> 

Ah... as said in the cover letter, all the series is based on machine_data
being set before the call to object_property_set_bool()... Maybe I should
have made that explicit with a preparatory patch... Sorry.

> >   
> > > ---
> > >  hw/ppc/spapr_cpu_core.c |   12 ++++++++++--
> > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index 003c4c5a79d2..04c818a6ecac 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -159,12 +159,16 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > >      spapr_cpu->icp = icp_create(OBJECT(cpu), spapr->icp_type,
> > >                                  XICS_FABRIC(spapr), &local_err);
> > >      if (local_err) {
> > > -        goto error;
> > > +        goto error_unregister;
> > >      }
> > >  
> > >      return;
> > >  
> > > +error_unregister:
> > > +    qemu_unregister_reset(spapr_cpu_reset, cpu);
> > > +    cpu_remove_sync(CPU(cpu));  
> > 
> > I'm a little unclear on exactly what init the cpu_remove_sync() is
> > mirroring, though.
> >   
> > >  error:
> > > +    g_free(spapr_cpu);
> > >      error_propagate(errp, local_err);
> > >  }
> > >  
> > > @@ -222,11 +226,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > >      for (j = 0; j < cc->nr_threads; j++) {
> > >          spapr_realize_vcpu(sc->threads[j], spapr, &local_err);
> > >          if (local_err) {
> > > -            goto err;
> > > +            goto err_unrealize;
> > >          }
> > >      }
> > >      return;
> > >  
> > > +err_unrealize:
> > > +    while (--j >= 0) {
> > > +        spapr_unrealize_vcpu(sc->threads[i]);
> > > +    }
> > >  err:
> > >      while (--i >= 0) {
> > >          obj = OBJECT(sc->threads[i]);
> > >   
> >   
> 
> 
> 


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

Re: [Qemu-devel] [PATCH 3/5] spapr_cpu_core: add missing rollback on realization path

[David Gibson]

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

On Fri, Jun 15, 2018 at 07:53:37AM +0200, Greg Kurz wrote:
> On Fri, 15 Jun 2018 10:02:25 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Jun 14, 2018 at 11:50:42PM +0200, Greg Kurz wrote:
> > > The spapr_realize_vcpu() function doesn't rollback in case of error.
> > > This isn't a problem with coldplugged CPUs because the machine won't
> > > start and QEMU will exit. Hotplug is a different story though: the
> > > CPU thread is started under object_property_set_bool() and it assumes
> > > it can access the CPU object.
> > > 
> > > If icp_create() fails, we return an error without unregistering the
> > > reset handler for this CPU, and we let the underlying QEMU thread for
> > > this CPU alive. Since spapr_cpu_core_realize() doesn't care to unrealize
> > > already realized CPUs either, but happily frees all of them anyway, the
> > > CPU thread crashes instantly:
> > > 
> > > (qemu) device_add host-spapr-cpu-core,core-id=1,id=gku
> > > GKU: failing icp_create (cpu 0x11497fd0)
> > >                              ^^^^^^^^^^
> > > Program received signal SIGSEGV, Segmentation fault.
> > > [Switching to Thread 0x7fffee3feaa0 (LWP 24725)]
> > > 0x00000000104c8374 in object_dynamic_cast_assert (obj=0x11497fd0,
> > >                                                   ^^^^^^^^^^^^^^
> > >                                              pointer to the CPU object
> > > 623         trace_object_dynamic_cast_assert(obj ? obj->class->type->name
> > > (gdb) p obj->class->type
> > > $1 = (Type) 0x0
> > > (gdb) p * obj
> > > $2 = {class = 0x10ea9c10, free = 0x11244620,
> > >                                  ^^^^^^^^^^
> > >                               should be g_free
> > > (gdb) p g_free
> > > $3 = {<text variable, no debug info>} 0x7ffff282bef0 <g_free>
> > > 
> > > obj is a dangling pointer to the CPU that was just destroyed in
> > > spapr_cpu_core_realize().
> > > 
> > > This patch adds proper rollback to both spapr_realize_vcpu() and
> > > spapr_cpu_core_realize().
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>  
> > 
> > Applied to ppc-for-3.0, since it definitely looks to fix some
> > problems.
> > 
> > > ---
> > >  hw/ppc/spapr_cpu_core.c |   12 ++++++++++--
> > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index 003c4c5a79d2..04c818a6ecac 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -159,12 +159,16 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > >      spapr_cpu->icp = icp_create(OBJECT(cpu), spapr->icp_type,
> > >                                  XICS_FABRIC(spapr), &local_err);
> > >      if (local_err) {
> > > -        goto error;
> > > +        goto error_unregister;
> > >      }
> > >  
> > >      return;
> > >  
> > > +error_unregister:
> > > +    qemu_unregister_reset(spapr_cpu_reset, cpu);
> > > +    cpu_remove_sync(CPU(cpu));  
> > 
> > I'm a little unclear on exactly what init the cpu_remove_sync() is
> > mirroring, though.
> > 
> 
> We have the same call in spapr_unrealize_vcpu(). IIUC it is mirroring
> object_property_set_bool(OBJECT(cpu), true, "realized", &local_err).

Ok.

> 
> > >  error:
> > > +    g_free(spapr_cpu);
> > >      error_propagate(errp, local_err);
> > >  }
> > >  
> > > @@ -222,11 +226,15 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > >      for (j = 0; j < cc->nr_threads; j++) {
> > >          spapr_realize_vcpu(sc->threads[j], spapr, &local_err);
> > >          if (local_err) {
> > > -            goto err;
> > > +            goto err_unrealize;
> > >          }
> > >      }
> > >      return;
> > >  
> > > +err_unrealize:
> > > +    while (--j >= 0) {
> > > +        spapr_unrealize_vcpu(sc->threads[i]);
> > > +    }
> > >  err:
> > >      while (--i >= 0) {
> > >          obj = OBJECT(sc->threads[i]);
> > >   
> > 
> 



-- 
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 3/5] spapr_cpu_core: add missing rollback on realization path

[David Gibson]

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

On Fri, Jun 15, 2018 at 07:58:05AM +0200, Greg Kurz wrote:
> On Fri, 15 Jun 2018 10:14:31 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jun 15, 2018 at 10:02:25AM +1000, David Gibson wrote:
> > > On Thu, Jun 14, 2018 at 11:50:42PM +0200, Greg Kurz wrote:  
> > > > The spapr_realize_vcpu() function doesn't rollback in case of error.
> > > > This isn't a problem with coldplugged CPUs because the machine won't
> > > > start and QEMU will exit. Hotplug is a different story though: the
> > > > CPU thread is started under object_property_set_bool() and it assumes
> > > > it can access the CPU object.
> > > > 
> > > > If icp_create() fails, we return an error without unregistering the
> > > > reset handler for this CPU, and we let the underlying QEMU thread for
> > > > this CPU alive. Since spapr_cpu_core_realize() doesn't care to unrealize
> > > > already realized CPUs either, but happily frees all of them anyway, the
> > > > CPU thread crashes instantly:
> > > > 
> > > > (qemu) device_add host-spapr-cpu-core,core-id=1,id=gku
> > > > GKU: failing icp_create (cpu 0x11497fd0)
> > > >                              ^^^^^^^^^^
> > > > Program received signal SIGSEGV, Segmentation fault.
> > > > [Switching to Thread 0x7fffee3feaa0 (LWP 24725)]
> > > > 0x00000000104c8374 in object_dynamic_cast_assert (obj=0x11497fd0,
> > > >                                                   ^^^^^^^^^^^^^^
> > > >                                              pointer to the CPU object
> > > > 623         trace_object_dynamic_cast_assert(obj ? obj->class->type->name
> > > > (gdb) p obj->class->type
> > > > $1 = (Type) 0x0
> > > > (gdb) p * obj
> > > > $2 = {class = 0x10ea9c10, free = 0x11244620,
> > > >                                  ^^^^^^^^^^
> > > >                               should be g_free
> > > > (gdb) p g_free
> > > > $3 = {<text variable, no debug info>} 0x7ffff282bef0 <g_free>
> > > > 
> > > > obj is a dangling pointer to the CPU that was just destroyed in
> > > > spapr_cpu_core_realize().
> > > > 
> > > > This patch adds proper rollback to both spapr_realize_vcpu() and
> > > > spapr_cpu_core_realize().
> > > > 
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>  
> > > 
> > > Applied to ppc-for-3.0, since it definitely looks to fix some
> > > problems.  
> > 
> > Uh.. actually it has a definite bug - the first exit point will call
> > g_free() on an uninitialized spapr_cpu.  I fixed it up with a NULL
> > initialization in my tree.
> 
> Ah... as said in the cover letter, all the series is based on machine_data
> being set before the call to object_property_set_bool()... Maybe I should
> have made that explicit with a preparatory patch... Sorry.

Ah, that makes sense.

So, I ended up having to rework a little differently, after I yanked
by intc -> machine_data patch because it broke things for clg.  I
think I've fixed it up correctly now - if you can check the latest
ppc-for-3.0 I pushed out, that would be great.

-- 
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 5/5] spapr_cpu_core: simplify spapr_cpu_core_realize()

[Greg Kurz]

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

On Fri, 15 Jun 2018 10:08:29 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jun 14, 2018 at 11:51:11PM +0200, Greg Kurz wrote:
> > There's no real reason to create all CPUs in a first pass and to realize
> > them in a second pass. Merging these two loops makes the code simpler.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> I'm a bit uncertain about this one.  It's correct at the moment, but
> I'm wondering if there might be things we want to do wile the cpu
> objects are constructed, but not initialized.
> 

Yeah, I thought about that also, but I couldn't find any case...

> In fact, I thought I wanted something like that for allowing earlier
> initialization of the default caps values, though in the end I found a
> simpler and better approach.
> 
> So, I'm holding off on this one for the time being.
> 

Sure.

> > ---
> >  hw/ppc/spapr_cpu_core.c |   25 ++++++++++++-------------
> >  1 file changed, 12 insertions(+), 13 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 0ebaf804a9bc..f52af20e0096 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -172,7 +172,8 @@ error:
> >      error_propagate(errp, local_err);
> >  }
> >  
> > -static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i, Error **errp)
> > +static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i,
> > +                                     sPAPRMachineState *spapr, Error **errp)
> >  {
> >      sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(sc);
> >      CPUCore *cc = CPU_CORE(sc);
> > @@ -201,9 +202,16 @@ static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i, Error **errp)
> >          goto err;
> >      }
> >  
> > +    spapr_realize_vcpu(cpu, spapr, &local_err);
> > +    if (local_err) {
> > +        goto err_unparent;
> > +    }
> > +
> >      object_unref(obj);
> >      return cpu;
> >  
> > +err_unparent:
> > +    object_unparent(obj);
> >  err:
> >      object_unref(obj);
> >      error_propagate(errp, local_err);
> > @@ -212,6 +220,7 @@ err:
> >  
> >  static void spapr_delete_vcpu(PowerPCCPU *cpu)
> >  {
> > +    spapr_unrealize_vcpu(cpu);
> >      object_unparent(OBJECT(cpu));
> >  }
> >  
> > @@ -226,7 +235,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> >      CPUCore *cc = CPU_CORE(OBJECT(dev));
> >      Error *local_err = NULL;
> > -    int i, j;
> > +    int i;
> >  
> >      if (!spapr) {
> >          error_setg(errp, TYPE_SPAPR_CPU_CORE " needs a pseries machine");
> > @@ -235,24 +244,14 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >  
> >      sc->threads = g_new(PowerPCCPU *, cc->nr_threads);
> >      for (i = 0; i < cc->nr_threads; i++) {
> > -        sc->threads[i] = spapr_create_vcpu(sc, i, &local_err);
> > +        sc->threads[i] = spapr_create_vcpu(sc, i, spapr, &local_err);
> >          if (local_err) {
> >              goto err;
> >          }
> >      }
> >  
> > -    for (j = 0; j < cc->nr_threads; j++) {
> > -        spapr_realize_vcpu(sc->threads[j], spapr, &local_err);
> > -        if (local_err) {
> > -            goto err_unrealize;
> > -        }
> > -    }
> >      return;
> >  
> > -err_unrealize:
> > -    while (--j >= 0) {
> > -        spapr_unrealize_vcpu(sc->threads[i]);
> > -    }
> >  err:
> >      while (--i >= 0) {
> >          spapr_delete_vcpu(sc->threads[i]);
> >   
> 


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

Re: [Qemu-devel] [PATCH 3/5] spapr_cpu_core: add missing rollback on realization path

[Greg Kurz]

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

On Fri, 15 Jun 2018 16:29:15 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jun 15, 2018 at 07:58:05AM +0200, Greg Kurz wrote:
> > On Fri, 15 Jun 2018 10:14:31 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Fri, Jun 15, 2018 at 10:02:25AM +1000, David Gibson wrote:  
> > > > On Thu, Jun 14, 2018 at 11:50:42PM +0200, Greg Kurz wrote:    
> > > > > The spapr_realize_vcpu() function doesn't rollback in case of error.
> > > > > This isn't a problem with coldplugged CPUs because the machine won't
> > > > > start and QEMU will exit. Hotplug is a different story though: the
> > > > > CPU thread is started under object_property_set_bool() and it assumes
> > > > > it can access the CPU object.
> > > > > 
> > > > > If icp_create() fails, we return an error without unregistering the
> > > > > reset handler for this CPU, and we let the underlying QEMU thread for
> > > > > this CPU alive. Since spapr_cpu_core_realize() doesn't care to unrealize
> > > > > already realized CPUs either, but happily frees all of them anyway, the
> > > > > CPU thread crashes instantly:
> > > > > 
> > > > > (qemu) device_add host-spapr-cpu-core,core-id=1,id=gku
> > > > > GKU: failing icp_create (cpu 0x11497fd0)
> > > > >                              ^^^^^^^^^^
> > > > > Program received signal SIGSEGV, Segmentation fault.
> > > > > [Switching to Thread 0x7fffee3feaa0 (LWP 24725)]
> > > > > 0x00000000104c8374 in object_dynamic_cast_assert (obj=0x11497fd0,
> > > > >                                                   ^^^^^^^^^^^^^^
> > > > >                                              pointer to the CPU object
> > > > > 623         trace_object_dynamic_cast_assert(obj ? obj->class->type->name
> > > > > (gdb) p obj->class->type
> > > > > $1 = (Type) 0x0
> > > > > (gdb) p * obj
> > > > > $2 = {class = 0x10ea9c10, free = 0x11244620,
> > > > >                                  ^^^^^^^^^^
> > > > >                               should be g_free
> > > > > (gdb) p g_free
> > > > > $3 = {<text variable, no debug info>} 0x7ffff282bef0 <g_free>
> > > > > 
> > > > > obj is a dangling pointer to the CPU that was just destroyed in
> > > > > spapr_cpu_core_realize().
> > > > > 
> > > > > This patch adds proper rollback to both spapr_realize_vcpu() and
> > > > > spapr_cpu_core_realize().
> > > > > 
> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>    
> > > > 
> > > > Applied to ppc-for-3.0, since it definitely looks to fix some
> > > > problems.    
> > > 
> > > Uh.. actually it has a definite bug - the first exit point will call
> > > g_free() on an uninitialized spapr_cpu.  I fixed it up with a NULL
> > > initialization in my tree.  
> > 
> > Ah... as said in the cover letter, all the series is based on machine_data
> > being set before the call to object_property_set_bool()... Maybe I should
> > have made that explicit with a preparatory patch... Sorry.  
> 
> Ah, that makes sense.
> 
> So, I ended up having to rework a little differently, after I yanked
> by intc -> machine_data patch because it broke things for clg.  I
> think I've fixed it up correctly now - if you can check the latest
> ppc-for-3.0 I pushed out, that would be great.
> 

I'll do this ASAP.

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

Re: [Qemu-devel] [PATCH 3/5] spapr_cpu_core: add missing rollback on realization path

[Greg Kurz]

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

On Fri, 15 Jun 2018 09:07:24 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Fri, 15 Jun 2018 16:29:15 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jun 15, 2018 at 07:58:05AM +0200, Greg Kurz wrote:  
> > > On Fri, 15 Jun 2018 10:14:31 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >     
> > > > On Fri, Jun 15, 2018 at 10:02:25AM +1000, David Gibson wrote:    
> > > > > On Thu, Jun 14, 2018 at 11:50:42PM +0200, Greg Kurz wrote:      
> > > > > > The spapr_realize_vcpu() function doesn't rollback in case of error.
> > > > > > This isn't a problem with coldplugged CPUs because the machine won't
> > > > > > start and QEMU will exit. Hotplug is a different story though: the
> > > > > > CPU thread is started under object_property_set_bool() and it assumes
> > > > > > it can access the CPU object.
> > > > > > 
> > > > > > If icp_create() fails, we return an error without unregistering the
> > > > > > reset handler for this CPU, and we let the underlying QEMU thread for
> > > > > > this CPU alive. Since spapr_cpu_core_realize() doesn't care to unrealize
> > > > > > already realized CPUs either, but happily frees all of them anyway, the
> > > > > > CPU thread crashes instantly:
> > > > > > 
> > > > > > (qemu) device_add host-spapr-cpu-core,core-id=1,id=gku
> > > > > > GKU: failing icp_create (cpu 0x11497fd0)
> > > > > >                              ^^^^^^^^^^
> > > > > > Program received signal SIGSEGV, Segmentation fault.
> > > > > > [Switching to Thread 0x7fffee3feaa0 (LWP 24725)]
> > > > > > 0x00000000104c8374 in object_dynamic_cast_assert (obj=0x11497fd0,
> > > > > >                                                   ^^^^^^^^^^^^^^
> > > > > >                                              pointer to the CPU object
> > > > > > 623         trace_object_dynamic_cast_assert(obj ? obj->class->type->name
> > > > > > (gdb) p obj->class->type
> > > > > > $1 = (Type) 0x0
> > > > > > (gdb) p * obj
> > > > > > $2 = {class = 0x10ea9c10, free = 0x11244620,
> > > > > >                                  ^^^^^^^^^^
> > > > > >                               should be g_free
> > > > > > (gdb) p g_free
> > > > > > $3 = {<text variable, no debug info>} 0x7ffff282bef0 <g_free>
> > > > > > 
> > > > > > obj is a dangling pointer to the CPU that was just destroyed in
> > > > > > spapr_cpu_core_realize().
> > > > > > 
> > > > > > This patch adds proper rollback to both spapr_realize_vcpu() and
> > > > > > spapr_cpu_core_realize().
> > > > > > 
> > > > > > Signed-off-by: Greg Kurz <groug@kaod.org>      
> > > > > 
> > > > > Applied to ppc-for-3.0, since it definitely looks to fix some
> > > > > problems.      
> > > > 
> > > > Uh.. actually it has a definite bug - the first exit point will call
> > > > g_free() on an uninitialized spapr_cpu.  I fixed it up with a NULL
> > > > initialization in my tree.    
> > > 
> > > Ah... as said in the cover letter, all the series is based on machine_data
> > > being set before the call to object_property_set_bool()... Maybe I should
> > > have made that explicit with a preparatory patch... Sorry.    
> > 
> > Ah, that makes sense.
> > 
> > So, I ended up having to rework a little differently, after I yanked
> > by intc -> machine_data patch because it broke things for clg.  I
> > think I've fixed it up correctly now - if you can check the latest
> > ppc-for-3.0 I pushed out, that would be great.
> >   
> 
> I'll do this ASAP.

Oops, I've just spotted a nit in my original patch, that causes
QEMU to crash if threads > 1... but I had only tested with single
threaded cores :)

> +err_unrealize:
> +    while (--j >= 0) {
> +        spapr_unrealize_vcpu(sc->threads[i]);
                                           ^^^
                                       should be j

Appart from that, it looks good.

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

Re: [Qemu-devel] [PATCH 3/5] spapr_cpu_core: add missing rollback on realization path

[David Gibson]

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

On Fri, Jun 15, 2018 at 10:01:47AM +0200, Greg Kurz wrote:
> On Fri, 15 Jun 2018 09:07:24 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > On Fri, 15 Jun 2018 16:29:15 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > On Fri, Jun 15, 2018 at 07:58:05AM +0200, Greg Kurz wrote:  
> > > > On Fri, 15 Jun 2018 10:14:31 +1000
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >     
> > > > > On Fri, Jun 15, 2018 at 10:02:25AM +1000, David Gibson wrote:    
> > > > > > On Thu, Jun 14, 2018 at 11:50:42PM +0200, Greg Kurz wrote:      
> > > > > > > The spapr_realize_vcpu() function doesn't rollback in case of error.
> > > > > > > This isn't a problem with coldplugged CPUs because the machine won't
> > > > > > > start and QEMU will exit. Hotplug is a different story though: the
> > > > > > > CPU thread is started under object_property_set_bool() and it assumes
> > > > > > > it can access the CPU object.
> > > > > > > 
> > > > > > > If icp_create() fails, we return an error without unregistering the
> > > > > > > reset handler for this CPU, and we let the underlying QEMU thread for
> > > > > > > this CPU alive. Since spapr_cpu_core_realize() doesn't care to unrealize
> > > > > > > already realized CPUs either, but happily frees all of them anyway, the
> > > > > > > CPU thread crashes instantly:
> > > > > > > 
> > > > > > > (qemu) device_add host-spapr-cpu-core,core-id=1,id=gku
> > > > > > > GKU: failing icp_create (cpu 0x11497fd0)
> > > > > > >                              ^^^^^^^^^^
> > > > > > > Program received signal SIGSEGV, Segmentation fault.
> > > > > > > [Switching to Thread 0x7fffee3feaa0 (LWP 24725)]
> > > > > > > 0x00000000104c8374 in object_dynamic_cast_assert (obj=0x11497fd0,
> > > > > > >                                                   ^^^^^^^^^^^^^^
> > > > > > >                                              pointer to the CPU object
> > > > > > > 623         trace_object_dynamic_cast_assert(obj ? obj->class->type->name
> > > > > > > (gdb) p obj->class->type
> > > > > > > $1 = (Type) 0x0
> > > > > > > (gdb) p * obj
> > > > > > > $2 = {class = 0x10ea9c10, free = 0x11244620,
> > > > > > >                                  ^^^^^^^^^^
> > > > > > >                               should be g_free
> > > > > > > (gdb) p g_free
> > > > > > > $3 = {<text variable, no debug info>} 0x7ffff282bef0 <g_free>
> > > > > > > 
> > > > > > > obj is a dangling pointer to the CPU that was just destroyed in
> > > > > > > spapr_cpu_core_realize().
> > > > > > > 
> > > > > > > This patch adds proper rollback to both spapr_realize_vcpu() and
> > > > > > > spapr_cpu_core_realize().
> > > > > > > 
> > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org>      
> > > > > > 
> > > > > > Applied to ppc-for-3.0, since it definitely looks to fix some
> > > > > > problems.      
> > > > > 
> > > > > Uh.. actually it has a definite bug - the first exit point will call
> > > > > g_free() on an uninitialized spapr_cpu.  I fixed it up with a NULL
> > > > > initialization in my tree.    
> > > > 
> > > > Ah... as said in the cover letter, all the series is based on machine_data
> > > > being set before the call to object_property_set_bool()... Maybe I should
> > > > have made that explicit with a preparatory patch... Sorry.    
> > > 
> > > Ah, that makes sense.
> > > 
> > > So, I ended up having to rework a little differently, after I yanked
> > > by intc -> machine_data patch because it broke things for clg.  I
> > > think I've fixed it up correctly now - if you can check the latest
> > > ppc-for-3.0 I pushed out, that would be great.
> > >   
> > 
> > I'll do this ASAP.
> 
> Oops, I've just spotted a nit in my original patch, that causes
> QEMU to crash if threads > 1... but I had only tested with single
> threaded cores :)

> 
> > +err_unrealize:
> > +    while (--j >= 0) {
> > +        spapr_unrealize_vcpu(sc->threads[i]);
>                                            ^^^
>                                        should be j

Ah, yes.  I've fixed that up in my tree.


> 
> Appart from that, it looks good.



-- 
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 3/5] spapr_cpu_core: add missing rollback on realization path

[Greg Kurz]

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

On Fri, 15 Jun 2018 22:32:44 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jun 15, 2018 at 10:01:47AM +0200, Greg Kurz wrote:
> > On Fri, 15 Jun 2018 09:07:24 +0200
> > Greg Kurz <groug@kaod.org> wrote:
> >   
> > > On Fri, 15 Jun 2018 16:29:15 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Fri, Jun 15, 2018 at 07:58:05AM +0200, Greg Kurz wrote:    
> > > > > On Fri, 15 Jun 2018 10:14:31 +1000
> > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > >       
> > > > > > On Fri, Jun 15, 2018 at 10:02:25AM +1000, David Gibson wrote:      
> > > > > > > On Thu, Jun 14, 2018 at 11:50:42PM +0200, Greg Kurz wrote:        
> > > > > > > > The spapr_realize_vcpu() function doesn't rollback in case of error.
> > > > > > > > This isn't a problem with coldplugged CPUs because the machine won't
> > > > > > > > start and QEMU will exit. Hotplug is a different story though: the
> > > > > > > > CPU thread is started under object_property_set_bool() and it assumes
> > > > > > > > it can access the CPU object.
> > > > > > > > 
> > > > > > > > If icp_create() fails, we return an error without unregistering the
> > > > > > > > reset handler for this CPU, and we let the underlying QEMU thread for
> > > > > > > > this CPU alive. Since spapr_cpu_core_realize() doesn't care to unrealize
> > > > > > > > already realized CPUs either, but happily frees all of them anyway, the
> > > > > > > > CPU thread crashes instantly:
> > > > > > > > 
> > > > > > > > (qemu) device_add host-spapr-cpu-core,core-id=1,id=gku
> > > > > > > > GKU: failing icp_create (cpu 0x11497fd0)
> > > > > > > >                              ^^^^^^^^^^
> > > > > > > > Program received signal SIGSEGV, Segmentation fault.
> > > > > > > > [Switching to Thread 0x7fffee3feaa0 (LWP 24725)]
> > > > > > > > 0x00000000104c8374 in object_dynamic_cast_assert (obj=0x11497fd0,
> > > > > > > >                                                   ^^^^^^^^^^^^^^
> > > > > > > >                                              pointer to the CPU object
> > > > > > > > 623         trace_object_dynamic_cast_assert(obj ? obj->class->type->name
> > > > > > > > (gdb) p obj->class->type
> > > > > > > > $1 = (Type) 0x0
> > > > > > > > (gdb) p * obj
> > > > > > > > $2 = {class = 0x10ea9c10, free = 0x11244620,
> > > > > > > >                                  ^^^^^^^^^^
> > > > > > > >                               should be g_free
> > > > > > > > (gdb) p g_free
> > > > > > > > $3 = {<text variable, no debug info>} 0x7ffff282bef0 <g_free>
> > > > > > > > 
> > > > > > > > obj is a dangling pointer to the CPU that was just destroyed in
> > > > > > > > spapr_cpu_core_realize().
> > > > > > > > 
> > > > > > > > This patch adds proper rollback to both spapr_realize_vcpu() and
> > > > > > > > spapr_cpu_core_realize().
> > > > > > > > 
> > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org>        
> > > > > > > 
> > > > > > > Applied to ppc-for-3.0, since it definitely looks to fix some
> > > > > > > problems.        
> > > > > > 
> > > > > > Uh.. actually it has a definite bug - the first exit point will call
> > > > > > g_free() on an uninitialized spapr_cpu.  I fixed it up with a NULL
> > > > > > initialization in my tree.      
> > > > > 
> > > > > Ah... as said in the cover letter, all the series is based on machine_data
> > > > > being set before the call to object_property_set_bool()... Maybe I should
> > > > > have made that explicit with a preparatory patch... Sorry.      
> > > > 
> > > > Ah, that makes sense.
> > > > 
> > > > So, I ended up having to rework a little differently, after I yanked
> > > > by intc -> machine_data patch because it broke things for clg.  I
> > > > think I've fixed it up correctly now - if you can check the latest
> > > > ppc-for-3.0 I pushed out, that would be great.
> > > >     
> > > 
> > > I'll do this ASAP.  
> > 
> > Oops, I've just spotted a nit in my original patch, that causes
> > QEMU to crash if threads > 1... but I had only tested with single
> > threaded cores :)  
> 
> >   
> > > +err_unrealize:
> > > +    while (--j >= 0) {
> > > +        spapr_unrealize_vcpu(sc->threads[i]);  
> >                                            ^^^
> >                                        should be j  
> 
> Ah, yes.  I've fixed that up in my tree.
> 

+        spapr_unrealize_vcpu(sc->threads[j);

Almost fixed ;)

> 
> > 
> > Appart from that, it looks good.  
> 
> 
> 


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

Re: [Qemu-devel] [PATCH 3/5] spapr_cpu_core: add missing rollback on realization path

[David Gibson]

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

On Fri, Jun 15, 2018 at 03:24:18PM +0200, Greg Kurz wrote:
> On Fri, 15 Jun 2018 22:32:44 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jun 15, 2018 at 10:01:47AM +0200, Greg Kurz wrote:
> > > On Fri, 15 Jun 2018 09:07:24 +0200
> > > Greg Kurz <groug@kaod.org> wrote:
> > >   
> > > > On Fri, 15 Jun 2018 16:29:15 +1000
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >   
> > > > > On Fri, Jun 15, 2018 at 07:58:05AM +0200, Greg Kurz wrote:    
> > > > > > On Fri, 15 Jun 2018 10:14:31 +1000
> > > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > > >       
> > > > > > > On Fri, Jun 15, 2018 at 10:02:25AM +1000, David Gibson wrote:      
> > > > > > > > On Thu, Jun 14, 2018 at 11:50:42PM +0200, Greg Kurz wrote:        
> > > > > > > > > The spapr_realize_vcpu() function doesn't rollback in case of error.
> > > > > > > > > This isn't a problem with coldplugged CPUs because the machine won't
> > > > > > > > > start and QEMU will exit. Hotplug is a different story though: the
> > > > > > > > > CPU thread is started under object_property_set_bool() and it assumes
> > > > > > > > > it can access the CPU object.
> > > > > > > > > 
> > > > > > > > > If icp_create() fails, we return an error without unregistering the
> > > > > > > > > reset handler for this CPU, and we let the underlying QEMU thread for
> > > > > > > > > this CPU alive. Since spapr_cpu_core_realize() doesn't care to unrealize
> > > > > > > > > already realized CPUs either, but happily frees all of them anyway, the
> > > > > > > > > CPU thread crashes instantly:
> > > > > > > > > 
> > > > > > > > > (qemu) device_add host-spapr-cpu-core,core-id=1,id=gku
> > > > > > > > > GKU: failing icp_create (cpu 0x11497fd0)
> > > > > > > > >                              ^^^^^^^^^^
> > > > > > > > > Program received signal SIGSEGV, Segmentation fault.
> > > > > > > > > [Switching to Thread 0x7fffee3feaa0 (LWP 24725)]
> > > > > > > > > 0x00000000104c8374 in object_dynamic_cast_assert (obj=0x11497fd0,
> > > > > > > > >                                                   ^^^^^^^^^^^^^^
> > > > > > > > >                                              pointer to the CPU object
> > > > > > > > > 623         trace_object_dynamic_cast_assert(obj ? obj->class->type->name
> > > > > > > > > (gdb) p obj->class->type
> > > > > > > > > $1 = (Type) 0x0
> > > > > > > > > (gdb) p * obj
> > > > > > > > > $2 = {class = 0x10ea9c10, free = 0x11244620,
> > > > > > > > >                                  ^^^^^^^^^^
> > > > > > > > >                               should be g_free
> > > > > > > > > (gdb) p g_free
> > > > > > > > > $3 = {<text variable, no debug info>} 0x7ffff282bef0 <g_free>
> > > > > > > > > 
> > > > > > > > > obj is a dangling pointer to the CPU that was just destroyed in
> > > > > > > > > spapr_cpu_core_realize().
> > > > > > > > > 
> > > > > > > > > This patch adds proper rollback to both spapr_realize_vcpu() and
> > > > > > > > > spapr_cpu_core_realize().
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org>        
> > > > > > > > 
> > > > > > > > Applied to ppc-for-3.0, since it definitely looks to fix some
> > > > > > > > problems.        
> > > > > > > 
> > > > > > > Uh.. actually it has a definite bug - the first exit point will call
> > > > > > > g_free() on an uninitialized spapr_cpu.  I fixed it up with a NULL
> > > > > > > initialization in my tree.      
> > > > > > 
> > > > > > Ah... as said in the cover letter, all the series is based on machine_data
> > > > > > being set before the call to object_property_set_bool()... Maybe I should
> > > > > > have made that explicit with a preparatory patch... Sorry.      
> > > > > 
> > > > > Ah, that makes sense.
> > > > > 
> > > > > So, I ended up having to rework a little differently, after I yanked
> > > > > by intc -> machine_data patch because it broke things for clg.  I
> > > > > think I've fixed it up correctly now - if you can check the latest
> > > > > ppc-for-3.0 I pushed out, that would be great.
> > > > >     
> > > > 
> > > > I'll do this ASAP.  
> > > 
> > > Oops, I've just spotted a nit in my original patch, that causes
> > > QEMU to crash if threads > 1... but I had only tested with single
> > > threaded cores :)  
> > 
> > >   
> > > > +err_unrealize:
> > > > +    while (--j >= 0) {
> > > > +        spapr_unrealize_vcpu(sc->threads[i]);  
> > >                                            ^^^
> > >                                        should be j  
> > 
> > Ah, yes.  I've fixed that up in my tree.
> > 
> 
> +        spapr_unrealize_vcpu(sc->threads[j);
> 
> Almost fixed ;)

Oops, fixed now.

> 
> > 
> > > 
> > > Appart from that, it looks good.  
> > 
> > 
> > 
> 



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