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

[Greg Kurz <groug@kaod.org>]

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) {