[Qemu-devel] [PATCH v0] spapr: Ensure thread0 of CPU core is always realized first

[Qemu-devel] [PATCH v0] spapr: Ensure thread0 of CPU core is always realized first

[Bharata B Rao]

During CPU core realization, we create all the thread objects and parent
them to the core object in a loop. However, the realization of thread
objects is done separately by walking the threads of a core using
object_child_foreach(). With this, there is no guarantee on the order
in which the child thread objects get realized. Since CPU device tree
properties are currently derived from the CPU thread object, we assume
thread0 of the core to be the representative thread of the core when
creating device tree properties for the core. If thread0 is not the
first thread that gets realized, then we would end up having an
incorrect dt_id for the core and this causes hotplug failures from
the guest.

Fix this by realizing each thread object by walking the core's thread
object list thereby ensuring that thread0 and other threads are always
realized in the correct order.

Future TODO: CPU DT nodes are per-core properties and we should
ideally base the creation of CPU DT nodes on core objects rather than
the thread objects.

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

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index a384db5..70b6b0b 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -259,9 +259,9 @@ out:
     error_propagate(errp, local_err);
 }
 
-static int spapr_cpu_core_realize_child(Object *child, void *opaque)
+static void spapr_cpu_core_realize_child(Object *child, Error **errp)
 {
-    Error **errp = opaque, *local_err = NULL;
+    Error *local_err = NULL;
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     CPUState *cs = CPU(child);
     PowerPCCPU *cpu = POWERPC_CPU(cs);
@@ -269,15 +269,14 @@ static int spapr_cpu_core_realize_child(Object *child, void *opaque)
     object_property_set_bool(child, true, "realized", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        return 1;
+        return;
     }
 
     spapr_cpu_init(spapr, cpu, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        return 1;
+        return;
     }
-    return 0;
 }
 
 static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
@@ -287,13 +286,13 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
     const char *typename = object_class_get_name(sc->cpu_class);
     size_t size = object_type_get_instance_size(typename);
     Error *local_err = NULL;
-    Object *obj;
-    int i;
+    void *obj;
+    int i, j;
 
     sc->threads = g_malloc0(size * cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
         char id[32];
-        void *obj = sc->threads + i * size;
+        obj = sc->threads + i * size;
 
         object_initialize(obj, size, typename);
         snprintf(id, sizeof(id), "thread[%d]", i);
@@ -303,12 +302,16 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
         }
         object_unref(obj);
     }
-    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &local_err);
-    if (local_err) {
-        goto err;
-    } else {
-        return;
+
+    for (j = 0; j < cc->nr_threads; j++) {
+        obj = sc->threads + j * size;
+
+        spapr_cpu_core_realize_child(obj, &local_err);
+        if (local_err) {
+            goto err;
+        }
     }
+    return;
 
 err:
     while (--i >= 0) {
-- 
2.1.0

Re: [Qemu-devel] [Qemu-ppc] [PATCH v0] spapr: Ensure thread0 of CPU core is always realized first

[Greg Kurz]

On Fri,  1 Jul 2016 10:44:39 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> During CPU core realization, we create all the thread objects and parent
> them to the core object in a loop. However, the realization of thread
> objects is done separately by walking the threads of a core using
> object_child_foreach(). With this, there is no guarantee on the order
> in which the child thread objects get realized. Since CPU device tree
> properties are currently derived from the CPU thread object, we assume
> thread0 of the core to be the representative thread of the core when
> creating device tree properties for the core. If thread0 is not the
> first thread that gets realized, then we would end up having an
> incorrect dt_id for the core and this causes hotplug failures from
> the guest.
> 
> Fix this by realizing each thread object by walking the core's thread
> object list thereby ensuring that thread0 and other threads are always
> realized in the correct order.
> 
> Future TODO: CPU DT nodes are per-core properties and we should
> ideally base the creation of CPU DT nodes on core objects rather than
> the thread objects.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---

This will also help to compute a new index to be used in place of
cs->cpu_index in my series to move cpu_dt_id generation from
target to machine code.

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

>  hw/ppc/spapr_cpu_core.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index a384db5..70b6b0b 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -259,9 +259,9 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> -static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> +static void spapr_cpu_core_realize_child(Object *child, Error **errp)
>  {
> -    Error **errp = opaque, *local_err = NULL;
> +    Error *local_err = NULL;
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      CPUState *cs = CPU(child);
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> @@ -269,15 +269,14 @@ static int spapr_cpu_core_realize_child(Object *child, void *opaque)
>      object_property_set_bool(child, true, "realized", &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> -        return 1;
> +        return;
>      }
>  
>      spapr_cpu_init(spapr, cpu, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> -        return 1;
> +        return;
>      }
> -    return 0;
>  }
>  
>  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> @@ -287,13 +286,13 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>      const char *typename = object_class_get_name(sc->cpu_class);
>      size_t size = object_type_get_instance_size(typename);
>      Error *local_err = NULL;
> -    Object *obj;
> -    int i;
> +    void *obj;
> +    int i, j;
>  
>      sc->threads = g_malloc0(size * cc->nr_threads);
>      for (i = 0; i < cc->nr_threads; i++) {
>          char id[32];
> -        void *obj = sc->threads + i * size;
> +        obj = sc->threads + i * size;
>  
>          object_initialize(obj, size, typename);
>          snprintf(id, sizeof(id), "thread[%d]", i);
> @@ -303,12 +302,16 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>          }
>          object_unref(obj);
>      }
> -    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &local_err);
> -    if (local_err) {
> -        goto err;
> -    } else {
> -        return;
> +
> +    for (j = 0; j < cc->nr_threads; j++) {
> +        obj = sc->threads + j * size;
> +
> +        spapr_cpu_core_realize_child(obj, &local_err);
> +        if (local_err) {
> +            goto err;
> +        }
>      }
> +    return;
>  
>  err:
>      while (--i >= 0) {

Re: [Qemu-devel] [PATCH v0] spapr: Ensure thread0 of CPU core is always realized first

[David Gibson]

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

On Fri, Jul 01, 2016 at 10:44:39AM +0530, Bharata B Rao wrote:
> During CPU core realization, we create all the thread objects and parent
> them to the core object in a loop. However, the realization of thread
> objects is done separately by walking the threads of a core using
> object_child_foreach(). With this, there is no guarantee on the order
> in which the child thread objects get realized. Since CPU device tree
> properties are currently derived from the CPU thread object, we assume
> thread0 of the core to be the representative thread of the core when
> creating device tree properties for the core. If thread0 is not the
> first thread that gets realized, then we would end up having an
> incorrect dt_id for the core and this causes hotplug failures from
> the guest.
> 
> Fix this by realizing each thread object by walking the core's thread
> object list thereby ensuring that thread0 and other threads are always
> realized in the correct order.
> 
> Future TODO: CPU DT nodes are per-core properties and we should
> ideally base the creation of CPU DT nodes on core objects rather than
> the thread objects.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Applied to ppc-for-2.7, thanks.

> ---
>  hw/ppc/spapr_cpu_core.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index a384db5..70b6b0b 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -259,9 +259,9 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> -static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> +static void spapr_cpu_core_realize_child(Object *child, Error **errp)
>  {
> -    Error **errp = opaque, *local_err = NULL;
> +    Error *local_err = NULL;
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      CPUState *cs = CPU(child);
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> @@ -269,15 +269,14 @@ static int spapr_cpu_core_realize_child(Object *child, void *opaque)
>      object_property_set_bool(child, true, "realized", &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> -        return 1;
> +        return;
>      }
>  
>      spapr_cpu_init(spapr, cpu, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> -        return 1;
> +        return;
>      }
> -    return 0;
>  }
>  
>  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> @@ -287,13 +286,13 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>      const char *typename = object_class_get_name(sc->cpu_class);
>      size_t size = object_type_get_instance_size(typename);
>      Error *local_err = NULL;
> -    Object *obj;
> -    int i;
> +    void *obj;
> +    int i, j;
>  
>      sc->threads = g_malloc0(size * cc->nr_threads);
>      for (i = 0; i < cc->nr_threads; i++) {
>          char id[32];
> -        void *obj = sc->threads + i * size;
> +        obj = sc->threads + i * size;
>  
>          object_initialize(obj, size, typename);
>          snprintf(id, sizeof(id), "thread[%d]", i);
> @@ -303,12 +302,16 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>          }
>          object_unref(obj);
>      }
> -    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &local_err);
> -    if (local_err) {
> -        goto err;
> -    } else {
> -        return;
> +
> +    for (j = 0; j < cc->nr_threads; j++) {
> +        obj = sc->threads + j * size;
> +
> +        spapr_cpu_core_realize_child(obj, &local_err);
> +        if (local_err) {
> +            goto err;
> +        }
>      }
> +    return;
>  
>  err:
>      while (--i >= 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: 819 bytes --]