[Qemu-devel] [PATCH] spapr_cpu_core: instantiate CPUs separately

[Qemu-devel] [PATCH] spapr_cpu_core: instantiate CPUs separately

[Greg Kurz]

The current code assumes that only the CPU core object holds a
reference on each individual CPU object, and happily frees their
allocated memory when the core is unrealized. This is dangerous
as some other code can legitimely keep a pointer to a CPU if it
calls object_ref(), but it would end up with a dangling pointer.

Let's allocate all CPUs with object_new() and let QOM frees them
when their reference count reaches zero. This greatly simplify the
code as we don't have to fiddle with the instance size anymore.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
v2: - mention code simplification in changelog
    - use PowerPCCPU * and Object * instead of void *
---
 hw/ppc/spapr.c                  |   11 +++--------
 hw/ppc/spapr_cpu_core.c         |   19 +++++++------------
 include/hw/ppc/spapr_cpu_core.h |    2 +-
 3 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index fd9813bde82f..d9555a3677be 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3153,12 +3153,10 @@ void spapr_core_release(DeviceState *dev)
 
     if (smc->pre_2_10_has_unused_icps) {
         sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
-        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
-        size_t size = object_type_get_instance_size(scc->cpu_type);
         int i;
 
         for (i = 0; i < cc->nr_threads; i++) {
-            CPUState *cs = CPU(sc->threads + i * size);
+            CPUState *cs = CPU(sc->threads[i]);
 
             pre_2_10_vmstate_register_dummy_icp(cs->cpu_index);
         }
@@ -3204,7 +3202,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
     sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
     CPUCore *cc = CPU_CORE(dev);
-    CPUState *cs = CPU(core->threads);
+    CPUState *cs = CPU(core->threads[0]);
     sPAPRDRConnector *drc;
     Error *local_err = NULL;
     int smt = kvmppc_smt_threads();
@@ -3249,15 +3247,12 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     core_slot->cpu = OBJECT(dev);
 
     if (smc->pre_2_10_has_unused_icps) {
-        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
-        size_t size = object_type_get_instance_size(scc->cpu_type);
         int i;
 
         for (i = 0; i < cc->nr_threads; i++) {
             sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
-            void *obj = sc->threads + i * size;
 
-            cs = CPU(obj);
+            cs = CPU(sc->threads[i]);
             pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
         }
     }
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 3a4c17401226..588f9b45714a 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -79,13 +79,11 @@ const char *spapr_get_cpu_core_type(const char *cpu_type)
 static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
 {
     sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
-    sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev));
-    size_t size = object_type_get_instance_size(scc->cpu_type);
     CPUCore *cc = CPU_CORE(dev);
     int i;
 
     for (i = 0; i < cc->nr_threads; i++) {
-        void *obj = sc->threads + i * size;
+        Object *obj = OBJECT(sc->threads[i]);
         DeviceState *dev = DEVICE(obj);
         CPUState *cs = CPU(dev);
         PowerPCCPU *cpu = POWERPC_CPU(cs);
@@ -146,9 +144,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
     sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
     sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev));
     CPUCore *cc = CPU_CORE(OBJECT(dev));
-    size_t size;
     Error *local_err = NULL;
-    void *obj;
+    Object *obj;
     int i, j;
 
     if (!spapr) {
@@ -156,18 +153,16 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    size = object_type_get_instance_size(scc->cpu_type);
-    sc->threads = g_malloc0(size * cc->nr_threads);
+    sc->threads = g_new(PowerPCCPU *, cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
         char id[32];
         CPUState *cs;
         PowerPCCPU *cpu;
 
-        obj = sc->threads + i * size;
+        obj = object_new(scc->cpu_type);
 
-        object_initialize(obj, size, scc->cpu_type);
         cs = CPU(obj);
-        cpu = POWERPC_CPU(cs);
+        cpu = sc->threads[i] = POWERPC_CPU(obj);
         cs->cpu_index = cc->core_id + i;
         cpu->vcpu_id = (cc->core_id * spapr->vsmt / smp_threads) + i;
         if (kvm_enabled() && !kvm_vcpu_id_is_valid(cpu->vcpu_id)) {
@@ -192,7 +187,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
     }
 
     for (j = 0; j < cc->nr_threads; j++) {
-        obj = sc->threads + j * size;
+        obj = OBJECT(sc->threads[j]);
 
         spapr_cpu_core_realize_child(obj, spapr, &local_err);
         if (local_err) {
@@ -203,7 +198,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
 
 err:
     while (--i >= 0) {
-        obj = sc->threads + i * size;
+        obj = OBJECT(sc->threads[i]);
         object_unparent(obj);
     }
     g_free(sc->threads);
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index f2d48d6a6786..1129f344aa0c 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -28,7 +28,7 @@ typedef struct sPAPRCPUCore {
     CPUCore parent_obj;
 
     /*< public >*/
-    void *threads;
+    PowerPCCPU **threads;
     int node_id;
 } sPAPRCPUCore;
 

Re: [Qemu-devel] [PATCH] spapr_cpu_core: instantiate CPUs separately

[David Gibson]

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

On Fri, Oct 13, 2017 at 01:31:44PM +0200, Greg Kurz wrote:
> The current code assumes that only the CPU core object holds a
> reference on each individual CPU object, and happily frees their
> allocated memory when the core is unrealized. This is dangerous
> as some other code can legitimely keep a pointer to a CPU if it
> calls object_ref(), but it would end up with a dangling pointer.
> 
> Let's allocate all CPUs with object_new() and let QOM frees them
> when their reference count reaches zero. This greatly simplify the
> code as we don't have to fiddle with the instance size anymore.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

So, I'm pretty sure my first drafts of the core stuff did things this
waym and it got nacked, for QOM lifetime reasons that I never really
understood.

> ---
> v2: - mention code simplification in changelog
>     - use PowerPCCPU * and Object * instead of void *
> ---
>  hw/ppc/spapr.c                  |   11 +++--------
>  hw/ppc/spapr_cpu_core.c         |   19 +++++++------------
>  include/hw/ppc/spapr_cpu_core.h |    2 +-
>  3 files changed, 11 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index fd9813bde82f..d9555a3677be 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3153,12 +3153,10 @@ void spapr_core_release(DeviceState *dev)
>  
>      if (smc->pre_2_10_has_unused_icps) {
>          sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> -        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
> -        size_t size = object_type_get_instance_size(scc->cpu_type);
>          int i;
>  
>          for (i = 0; i < cc->nr_threads; i++) {
> -            CPUState *cs = CPU(sc->threads + i * size);
> +            CPUState *cs = CPU(sc->threads[i]);
>  
>              pre_2_10_vmstate_register_dummy_icp(cs->cpu_index);
>          }
> @@ -3204,7 +3202,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
>      sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
>      CPUCore *cc = CPU_CORE(dev);
> -    CPUState *cs = CPU(core->threads);
> +    CPUState *cs = CPU(core->threads[0]);
>      sPAPRDRConnector *drc;
>      Error *local_err = NULL;
>      int smt = kvmppc_smt_threads();
> @@ -3249,15 +3247,12 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      core_slot->cpu = OBJECT(dev);
>  
>      if (smc->pre_2_10_has_unused_icps) {
> -        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
> -        size_t size = object_type_get_instance_size(scc->cpu_type);
>          int i;
>  
>          for (i = 0; i < cc->nr_threads; i++) {
>              sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
> -            void *obj = sc->threads + i * size;
>  
> -            cs = CPU(obj);
> +            cs = CPU(sc->threads[i]);
>              pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
>          }
>      }
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 3a4c17401226..588f9b45714a 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -79,13 +79,11 @@ const char *spapr_get_cpu_core_type(const char *cpu_type)
>  static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
>  {
>      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> -    sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev));
> -    size_t size = object_type_get_instance_size(scc->cpu_type);
>      CPUCore *cc = CPU_CORE(dev);
>      int i;
>  
>      for (i = 0; i < cc->nr_threads; i++) {
> -        void *obj = sc->threads + i * size;
> +        Object *obj = OBJECT(sc->threads[i]);
>          DeviceState *dev = DEVICE(obj);
>          CPUState *cs = CPU(dev);
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
> @@ -146,9 +144,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
>      sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev));
>      CPUCore *cc = CPU_CORE(OBJECT(dev));
> -    size_t size;
>      Error *local_err = NULL;
> -    void *obj;
> +    Object *obj;
>      int i, j;
>  
>      if (!spapr) {
> @@ -156,18 +153,16 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    size = object_type_get_instance_size(scc->cpu_type);
> -    sc->threads = g_malloc0(size * cc->nr_threads);
> +    sc->threads = g_new(PowerPCCPU *, cc->nr_threads);
>      for (i = 0; i < cc->nr_threads; i++) {
>          char id[32];
>          CPUState *cs;
>          PowerPCCPU *cpu;
>  
> -        obj = sc->threads + i * size;
> +        obj = object_new(scc->cpu_type);
>  
> -        object_initialize(obj, size, scc->cpu_type);
>          cs = CPU(obj);
> -        cpu = POWERPC_CPU(cs);
> +        cpu = sc->threads[i] = POWERPC_CPU(obj);
>          cs->cpu_index = cc->core_id + i;
>          cpu->vcpu_id = (cc->core_id * spapr->vsmt / smp_threads) + i;
>          if (kvm_enabled() && !kvm_vcpu_id_is_valid(cpu->vcpu_id)) {
> @@ -192,7 +187,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>      }
>  
>      for (j = 0; j < cc->nr_threads; j++) {
> -        obj = sc->threads + j * size;
> +        obj = OBJECT(sc->threads[j]);
>  
>          spapr_cpu_core_realize_child(obj, spapr, &local_err);
>          if (local_err) {
> @@ -203,7 +198,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>  
>  err:
>      while (--i >= 0) {
> -        obj = sc->threads + i * size;
> +        obj = OBJECT(sc->threads[i]);
>          object_unparent(obj);
>      }
>      g_free(sc->threads);
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index f2d48d6a6786..1129f344aa0c 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -28,7 +28,7 @@ typedef struct sPAPRCPUCore {
>      CPUCore parent_obj;
>  
>      /*< public >*/
> -    void *threads;
> +    PowerPCCPU **threads;
>      int node_id;
>  } sPAPRCPUCore;
>  
> 

-- 
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] spapr_cpu_core: instantiate CPUs separately

[Greg Kurz]

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

On Sat, 14 Oct 2017 20:33:37 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Oct 13, 2017 at 01:31:44PM +0200, Greg Kurz wrote:
> > The current code assumes that only the CPU core object holds a
> > reference on each individual CPU object, and happily frees their
> > allocated memory when the core is unrealized. This is dangerous
> > as some other code can legitimely keep a pointer to a CPU if it
> > calls object_ref(), but it would end up with a dangling pointer.
> > 
> > Let's allocate all CPUs with object_new() and let QOM frees them
> > when their reference count reaches zero. This greatly simplify the
> > code as we don't have to fiddle with the instance size anymore.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> So, I'm pretty sure my first drafts of the core stuff did things this
> waym and it got nacked, for QOM lifetime reasons that I never really
> understood.
> 

Oh... well I'll go through the archives and try to find the thread then.
On the other end, the motivation for this patch was to add reference
counting of CPUs in the monitor code but Igor nacked the approach and
suggested to fix the bug differently. I didn't find any other place where
we would end up with a dangling CPU pointer, so I guess this patch wouldn't
fix an actual issue.

> > ---
> > v2: - mention code simplification in changelog
> >     - use PowerPCCPU * and Object * instead of void *
> > ---
> >  hw/ppc/spapr.c                  |   11 +++--------
> >  hw/ppc/spapr_cpu_core.c         |   19 +++++++------------
> >  include/hw/ppc/spapr_cpu_core.h |    2 +-
> >  3 files changed, 11 insertions(+), 21 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index fd9813bde82f..d9555a3677be 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -3153,12 +3153,10 @@ void spapr_core_release(DeviceState *dev)
> >  
> >      if (smc->pre_2_10_has_unused_icps) {
> >          sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > -        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
> > -        size_t size = object_type_get_instance_size(scc->cpu_type);
> >          int i;
> >  
> >          for (i = 0; i < cc->nr_threads; i++) {
> > -            CPUState *cs = CPU(sc->threads + i * size);
> > +            CPUState *cs = CPU(sc->threads[i]);
> >  
> >              pre_2_10_vmstate_register_dummy_icp(cs->cpu_index);
> >          }
> > @@ -3204,7 +3202,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> >      sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> >      CPUCore *cc = CPU_CORE(dev);
> > -    CPUState *cs = CPU(core->threads);
> > +    CPUState *cs = CPU(core->threads[0]);
> >      sPAPRDRConnector *drc;
> >      Error *local_err = NULL;
> >      int smt = kvmppc_smt_threads();
> > @@ -3249,15 +3247,12 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      core_slot->cpu = OBJECT(dev);
> >  
> >      if (smc->pre_2_10_has_unused_icps) {
> > -        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
> > -        size_t size = object_type_get_instance_size(scc->cpu_type);
> >          int i;
> >  
> >          for (i = 0; i < cc->nr_threads; i++) {
> >              sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
> > -            void *obj = sc->threads + i * size;
> >  
> > -            cs = CPU(obj);
> > +            cs = CPU(sc->threads[i]);
> >              pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
> >          }
> >      }
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 3a4c17401226..588f9b45714a 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -79,13 +79,11 @@ const char *spapr_get_cpu_core_type(const char *cpu_type)
> >  static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
> >  {
> >      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > -    sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev));
> > -    size_t size = object_type_get_instance_size(scc->cpu_type);
> >      CPUCore *cc = CPU_CORE(dev);
> >      int i;
> >  
> >      for (i = 0; i < cc->nr_threads; i++) {
> > -        void *obj = sc->threads + i * size;
> > +        Object *obj = OBJECT(sc->threads[i]);
> >          DeviceState *dev = DEVICE(obj);
> >          CPUState *cs = CPU(dev);
> >          PowerPCCPU *cpu = POWERPC_CPU(cs);
> > @@ -146,9 +144,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> >      sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev));
> >      CPUCore *cc = CPU_CORE(OBJECT(dev));
> > -    size_t size;
> >      Error *local_err = NULL;
> > -    void *obj;
> > +    Object *obj;
> >      int i, j;
> >  
> >      if (!spapr) {
> > @@ -156,18 +153,16 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >          return;
> >      }
> >  
> > -    size = object_type_get_instance_size(scc->cpu_type);
> > -    sc->threads = g_malloc0(size * cc->nr_threads);
> > +    sc->threads = g_new(PowerPCCPU *, cc->nr_threads);
> >      for (i = 0; i < cc->nr_threads; i++) {
> >          char id[32];
> >          CPUState *cs;
> >          PowerPCCPU *cpu;
> >  
> > -        obj = sc->threads + i * size;
> > +        obj = object_new(scc->cpu_type);
> >  
> > -        object_initialize(obj, size, scc->cpu_type);
> >          cs = CPU(obj);
> > -        cpu = POWERPC_CPU(cs);
> > +        cpu = sc->threads[i] = POWERPC_CPU(obj);
> >          cs->cpu_index = cc->core_id + i;
> >          cpu->vcpu_id = (cc->core_id * spapr->vsmt / smp_threads) + i;
> >          if (kvm_enabled() && !kvm_vcpu_id_is_valid(cpu->vcpu_id)) {
> > @@ -192,7 +187,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >      }
> >  
> >      for (j = 0; j < cc->nr_threads; j++) {
> > -        obj = sc->threads + j * size;
> > +        obj = OBJECT(sc->threads[j]);
> >  
> >          spapr_cpu_core_realize_child(obj, spapr, &local_err);
> >          if (local_err) {
> > @@ -203,7 +198,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >  
> >  err:
> >      while (--i >= 0) {
> > -        obj = sc->threads + i * size;
> > +        obj = OBJECT(sc->threads[i]);
> >          object_unparent(obj);
> >      }
> >      g_free(sc->threads);
> > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> > index f2d48d6a6786..1129f344aa0c 100644
> > --- a/include/hw/ppc/spapr_cpu_core.h
> > +++ b/include/hw/ppc/spapr_cpu_core.h
> > @@ -28,7 +28,7 @@ typedef struct sPAPRCPUCore {
> >      CPUCore parent_obj;
> >  
> >      /*< public >*/
> > -    void *threads;
> > +    PowerPCCPU **threads;
> >      int node_id;
> >  } sPAPRCPUCore;
> >  
> >   
> 


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

Re: [Qemu-devel] [PATCH] spapr_cpu_core: instantiate CPUs separately

[Igor Mammedov]

On Sat, 14 Oct 2017 20:33:37 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Oct 13, 2017 at 01:31:44PM +0200, Greg Kurz wrote:
> > The current code assumes that only the CPU core object holds a
> > reference on each individual CPU object, and happily frees their
> > allocated memory when the core is unrealized. This is dangerous
> > as some other code can legitimely keep a pointer to a CPU if it
> > calls object_ref(), but it would end up with a dangling pointer.
> > 
> > Let's allocate all CPUs with object_new() and let QOM frees them
> > when their reference count reaches zero. This greatly simplify the
> > code as we don't have to fiddle with the instance size anymore.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> So, I'm pretty sure my first drafts of the core stuff did things this
> waym and it got nacked, for QOM lifetime reasons that I never really
> understood.
From what I remember, Andreas would like to see composite CPU object
allocated in one go and then its children initialized with object_initialize()
so that no more allocation were needed.
That potentially would benefit hotplug, since we could gracefully
fail object creation early if there is not enough memory.
But the way it's implemented currently doesn't really match that initial
goal as array for threads is dynamically allocated later
and then we need to dance around it with pointer arithmetic.

BTW: almost any allocation failure in qemu currently
is fatal so whether we fail on array alloc or on individual
object_new() won't make any difference.

I'd rather see this clean up merged as it simplifies code
in these case.


> 
> > ---
> > v2: - mention code simplification in changelog
> >     - use PowerPCCPU * and Object * instead of void *
> > ---
> >  hw/ppc/spapr.c                  |   11 +++--------
> >  hw/ppc/spapr_cpu_core.c         |   19 +++++++------------
> >  include/hw/ppc/spapr_cpu_core.h |    2 +-
> >  3 files changed, 11 insertions(+), 21 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index fd9813bde82f..d9555a3677be 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -3153,12 +3153,10 @@ void spapr_core_release(DeviceState *dev)
> >  
> >      if (smc->pre_2_10_has_unused_icps) {
> >          sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > -        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
> > -        size_t size = object_type_get_instance_size(scc->cpu_type);
> >          int i;
> >  
> >          for (i = 0; i < cc->nr_threads; i++) {
> > -            CPUState *cs = CPU(sc->threads + i * size);
> > +            CPUState *cs = CPU(sc->threads[i]);
> >  
> >              pre_2_10_vmstate_register_dummy_icp(cs->cpu_index);
> >          }
> > @@ -3204,7 +3202,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> >      sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> >      CPUCore *cc = CPU_CORE(dev);
> > -    CPUState *cs = CPU(core->threads);
> > +    CPUState *cs = CPU(core->threads[0]);
> >      sPAPRDRConnector *drc;
> >      Error *local_err = NULL;
> >      int smt = kvmppc_smt_threads();
> > @@ -3249,15 +3247,12 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      core_slot->cpu = OBJECT(dev);
> >  
> >      if (smc->pre_2_10_has_unused_icps) {
> > -        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
> > -        size_t size = object_type_get_instance_size(scc->cpu_type);
> >          int i;
> >  
> >          for (i = 0; i < cc->nr_threads; i++) {
> >              sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
> > -            void *obj = sc->threads + i * size;
> >  
> > -            cs = CPU(obj);
> > +            cs = CPU(sc->threads[i]);
> >              pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
> >          }
> >      }
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 3a4c17401226..588f9b45714a 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -79,13 +79,11 @@ const char *spapr_get_cpu_core_type(const char *cpu_type)
> >  static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
> >  {
> >      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > -    sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev));
> > -    size_t size = object_type_get_instance_size(scc->cpu_type);
> >      CPUCore *cc = CPU_CORE(dev);
> >      int i;
> >  
> >      for (i = 0; i < cc->nr_threads; i++) {
> > -        void *obj = sc->threads + i * size;
> > +        Object *obj = OBJECT(sc->threads[i]);
> >          DeviceState *dev = DEVICE(obj);
> >          CPUState *cs = CPU(dev);
> >          PowerPCCPU *cpu = POWERPC_CPU(cs);
> > @@ -146,9 +144,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> >      sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(dev));
> >      CPUCore *cc = CPU_CORE(OBJECT(dev));
> > -    size_t size;
> >      Error *local_err = NULL;
> > -    void *obj;
> > +    Object *obj;
> >      int i, j;
> >  
> >      if (!spapr) {
> > @@ -156,18 +153,16 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >          return;
> >      }
> >  
> > -    size = object_type_get_instance_size(scc->cpu_type);
> > -    sc->threads = g_malloc0(size * cc->nr_threads);
> > +    sc->threads = g_new(PowerPCCPU *, cc->nr_threads);
> >      for (i = 0; i < cc->nr_threads; i++) {
> >          char id[32];
> >          CPUState *cs;
> >          PowerPCCPU *cpu;
> >  
> > -        obj = sc->threads + i * size;
> > +        obj = object_new(scc->cpu_type);
> >  
> > -        object_initialize(obj, size, scc->cpu_type);
> >          cs = CPU(obj);
> > -        cpu = POWERPC_CPU(cs);
> > +        cpu = sc->threads[i] = POWERPC_CPU(obj);
> >          cs->cpu_index = cc->core_id + i;
> >          cpu->vcpu_id = (cc->core_id * spapr->vsmt / smp_threads) + i;
> >          if (kvm_enabled() && !kvm_vcpu_id_is_valid(cpu->vcpu_id)) {
> > @@ -192,7 +187,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >      }
> >  
> >      for (j = 0; j < cc->nr_threads; j++) {
> > -        obj = sc->threads + j * size;
> > +        obj = OBJECT(sc->threads[j]);
> >  
> >          spapr_cpu_core_realize_child(obj, spapr, &local_err);
> >          if (local_err) {
> > @@ -203,7 +198,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >  
> >  err:
> >      while (--i >= 0) {
> > -        obj = sc->threads + i * size;
> > +        obj = OBJECT(sc->threads[i]);
> >          object_unparent(obj);
> >      }
> >      g_free(sc->threads);
> > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> > index f2d48d6a6786..1129f344aa0c 100644
> > --- a/include/hw/ppc/spapr_cpu_core.h
> > +++ b/include/hw/ppc/spapr_cpu_core.h
> > @@ -28,7 +28,7 @@ typedef struct sPAPRCPUCore {
> >      CPUCore parent_obj;
> >  
> >      /*< public >*/
> > -    void *threads;
> > +    PowerPCCPU **threads;
> >      int node_id;
> >  } sPAPRCPUCore;
> >  
> >   
> 

Re: [Qemu-devel] [PATCH] spapr_cpu_core: instantiate CPUs separately

[David Gibson]

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

On Mon, Oct 16, 2017 at 10:26:38AM +0200, Igor Mammedov wrote:
> On Sat, 14 Oct 2017 20:33:37 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Oct 13, 2017 at 01:31:44PM +0200, Greg Kurz wrote:
> > > The current code assumes that only the CPU core object holds a
> > > reference on each individual CPU object, and happily frees their
> > > allocated memory when the core is unrealized. This is dangerous
> > > as some other code can legitimely keep a pointer to a CPU if it
> > > calls object_ref(), but it would end up with a dangling pointer.
> > > 
> > > Let's allocate all CPUs with object_new() and let QOM frees them
> > > when their reference count reaches zero. This greatly simplify the
> > > code as we don't have to fiddle with the instance size anymore.
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>  
> > 
> > So, I'm pretty sure my first drafts of the core stuff did things this
> > waym and it got nacked, for QOM lifetime reasons that I never really
> > understood.
> From what I remember, Andreas would like to see composite CPU object
> allocated in one go and then its children initialized with object_initialize()
> so that no more allocation were needed.

Ah, ok.

> That potentially would benefit hotplug, since we could gracefully
> fail object creation early if there is not enough memory.

Yeah, it sounds nice, but I don't see how we can do it.  In order to
do that the core object has to have enough space for all the threads,
which means we need both the size of each thread object and the number
of them.  The size we have (and will be easier to handle after Igor's
cleanups).  The number, we don't.

> But the way it's implemented currently doesn't really match that initial
> goal as array for threads is dynamically allocated later
> and then we need to dance around it with pointer arithmetic.
> 
> BTW: almost any allocation failure in qemu currently
> is fatal so whether we fail on array alloc or on individual
> object_new() won't make any difference.
> 
> I'd rather see this clean up merged as it simplifies code
> in these case.

Ok, works for me.

-- 
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] spapr_cpu_core: instantiate CPUs separately

[Greg Kurz]

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

On Tue, 17 Oct 2017 17:16:09 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Oct 16, 2017 at 10:26:38AM +0200, Igor Mammedov wrote:
> > On Sat, 14 Oct 2017 20:33:37 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Fri, Oct 13, 2017 at 01:31:44PM +0200, Greg Kurz wrote:  
> > > > The current code assumes that only the CPU core object holds a
> > > > reference on each individual CPU object, and happily frees their
> > > > allocated memory when the core is unrealized. This is dangerous
> > > > as some other code can legitimely keep a pointer to a CPU if it
> > > > calls object_ref(), but it would end up with a dangling pointer.
> > > > 
> > > > Let's allocate all CPUs with object_new() and let QOM frees them
> > > > when their reference count reaches zero. This greatly simplify the
> > > > code as we don't have to fiddle with the instance size anymore.
> > > > 
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>    
> > > 
> > > So, I'm pretty sure my first drafts of the core stuff did things this
> > > waym and it got nacked, for QOM lifetime reasons that I never really
> > > understood.  
> > From what I remember, Andreas would like to see composite CPU object
> > allocated in one go and then its children initialized with object_initialize()
> > so that no more allocation were needed.  
> 
> Ah, ok.
> 
> > That potentially would benefit hotplug, since we could gracefully
> > fail object creation early if there is not enough memory.  
> 
> Yeah, it sounds nice, but I don't see how we can do it.  In order to
> do that the core object has to have enough space for all the threads,
> which means we need both the size of each thread object and the number
> of them.  The size we have (and will be easier to handle after Igor's
> cleanups).  The number, we don't.
> 
> > But the way it's implemented currently doesn't really match that initial
> > goal as array for threads is dynamically allocated later
> > and then we need to dance around it with pointer arithmetic.
> > 
> > BTW: almost any allocation failure in qemu currently
> > is fatal so whether we fail on array alloc or on individual
> > object_new() won't make any difference.
> > 
> > I'd rather see this clean up merged as it simplifies code
> > in these case.  
> 
> Ok, works for me.
> 

Since we're in soft freeze already, I guess this won't go to 2.11. Maybe it's
time to create ppc-for-2.12 and apply it there ?

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

Re: [Qemu-devel] [PATCH] spapr_cpu_core: instantiate CPUs separately

[David Gibson]

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

On Mon, Nov 06, 2017 at 04:03:07PM +0100, Greg Kurz wrote:
> On Tue, 17 Oct 2017 17:16:09 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Mon, Oct 16, 2017 at 10:26:38AM +0200, Igor Mammedov wrote:
> > > On Sat, 14 Oct 2017 20:33:37 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Fri, Oct 13, 2017 at 01:31:44PM +0200, Greg Kurz wrote:  
> > > > > The current code assumes that only the CPU core object holds a
> > > > > reference on each individual CPU object, and happily frees their
> > > > > allocated memory when the core is unrealized. This is dangerous
> > > > > as some other code can legitimely keep a pointer to a CPU if it
> > > > > calls object_ref(), but it would end up with a dangling pointer.
> > > > > 
> > > > > Let's allocate all CPUs with object_new() and let QOM frees them
> > > > > when their reference count reaches zero. This greatly simplify the
> > > > > code as we don't have to fiddle with the instance size anymore.
> > > > > 
> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>    
> > > > 
> > > > So, I'm pretty sure my first drafts of the core stuff did things this
> > > > waym and it got nacked, for QOM lifetime reasons that I never really
> > > > understood.  
> > > From what I remember, Andreas would like to see composite CPU object
> > > allocated in one go and then its children initialized with object_initialize()
> > > so that no more allocation were needed.  
> > 
> > Ah, ok.
> > 
> > > That potentially would benefit hotplug, since we could gracefully
> > > fail object creation early if there is not enough memory.  
> > 
> > Yeah, it sounds nice, but I don't see how we can do it.  In order to
> > do that the core object has to have enough space for all the threads,
> > which means we need both the size of each thread object and the number
> > of them.  The size we have (and will be easier to handle after Igor's
> > cleanups).  The number, we don't.
> > 
> > > But the way it's implemented currently doesn't really match that initial
> > > goal as array for threads is dynamically allocated later
> > > and then we need to dance around it with pointer arithmetic.
> > > 
> > > BTW: almost any allocation failure in qemu currently
> > > is fatal so whether we fail on array alloc or on individual
> > > object_new() won't make any difference.
> > > 
> > > I'd rather see this clean up merged as it simplifies code
> > > in these case.  
> > 
> > Ok, works for me.
> > 
> 
> Since we're in soft freeze already, I guess this won't go to 2.11. Maybe it's
> time to create ppc-for-2.12 and apply it there ?

Yeah, sounds like a plan.

-- 
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] [Qemu-ppc] [PATCH] spapr_cpu_core: instantiate CPUs separately

[Greg Kurz]

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

On Tue, 7 Nov 2017 06:04:55 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Nov 06, 2017 at 04:03:07PM +0100, Greg Kurz wrote:
> > On Tue, 17 Oct 2017 17:16:09 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Mon, Oct 16, 2017 at 10:26:38AM +0200, Igor Mammedov wrote:  
> > > > On Sat, 14 Oct 2017 20:33:37 +1100
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >     
> > > > > On Fri, Oct 13, 2017 at 01:31:44PM +0200, Greg Kurz wrote:    
> > > > > > The current code assumes that only the CPU core object holds a
> > > > > > reference on each individual CPU object, and happily frees their
> > > > > > allocated memory when the core is unrealized. This is dangerous
> > > > > > as some other code can legitimely keep a pointer to a CPU if it
> > > > > > calls object_ref(), but it would end up with a dangling pointer.
> > > > > > 
> > > > > > Let's allocate all CPUs with object_new() and let QOM frees them
> > > > > > when their reference count reaches zero. This greatly simplify the
> > > > > > code as we don't have to fiddle with the instance size anymore.
> > > > > > 
> > > > > > Signed-off-by: Greg Kurz <groug@kaod.org>      
> > > > > 
> > > > > So, I'm pretty sure my first drafts of the core stuff did things this
> > > > > waym and it got nacked, for QOM lifetime reasons that I never really
> > > > > understood.    
> > > > From what I remember, Andreas would like to see composite CPU object
> > > > allocated in one go and then its children initialized with object_initialize()
> > > > so that no more allocation were needed.    
> > > 
> > > Ah, ok.
> > >   
> > > > That potentially would benefit hotplug, since we could gracefully
> > > > fail object creation early if there is not enough memory.    
> > > 
> > > Yeah, it sounds nice, but I don't see how we can do it.  In order to
> > > do that the core object has to have enough space for all the threads,
> > > which means we need both the size of each thread object and the number
> > > of them.  The size we have (and will be easier to handle after Igor's
> > > cleanups).  The number, we don't.
> > >   
> > > > But the way it's implemented currently doesn't really match that initial
> > > > goal as array for threads is dynamically allocated later
> > > > and then we need to dance around it with pointer arithmetic.
> > > > 
> > > > BTW: almost any allocation failure in qemu currently
> > > > is fatal so whether we fail on array alloc or on individual
> > > > object_new() won't make any difference.
> > > > 
> > > > I'd rather see this clean up merged as it simplifies code
> > > > in these case.    
> > > 
> > > Ok, works for me.
> > >   
> > 
> > Since we're in soft freeze already, I guess this won't go to 2.11. Maybe it's
> > time to create ppc-for-2.12 and apply it there ?  
> 
> Yeah, sounds like a plan.
> 

Friendly reminder: can you push this to ppc-for-2.12 so that it doesn't fall
through the cracks ? :)

Cheers,

--
Greg

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH] spapr_cpu_core: instantiate CPUs separately

[David Gibson]

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

On Tue, Nov 14, 2017 at 08:59:43AM +0100, Greg Kurz wrote:
> On Tue, 7 Nov 2017 06:04:55 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Mon, Nov 06, 2017 at 04:03:07PM +0100, Greg Kurz wrote:
> > > On Tue, 17 Oct 2017 17:16:09 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Mon, Oct 16, 2017 at 10:26:38AM +0200, Igor Mammedov wrote:  
> > > > > On Sat, 14 Oct 2017 20:33:37 +1100
> > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > >     
> > > > > > On Fri, Oct 13, 2017 at 01:31:44PM +0200, Greg Kurz wrote:    
> > > > > > > The current code assumes that only the CPU core object holds a
> > > > > > > reference on each individual CPU object, and happily frees their
> > > > > > > allocated memory when the core is unrealized. This is dangerous
> > > > > > > as some other code can legitimely keep a pointer to a CPU if it
> > > > > > > calls object_ref(), but it would end up with a dangling pointer.
> > > > > > > 
> > > > > > > Let's allocate all CPUs with object_new() and let QOM frees them
> > > > > > > when their reference count reaches zero. This greatly simplify the
> > > > > > > code as we don't have to fiddle with the instance size anymore.
> > > > > > > 
> > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org>      
> > > > > > 
> > > > > > So, I'm pretty sure my first drafts of the core stuff did things this
> > > > > > waym and it got nacked, for QOM lifetime reasons that I never really
> > > > > > understood.    
> > > > > From what I remember, Andreas would like to see composite CPU object
> > > > > allocated in one go and then its children initialized with object_initialize()
> > > > > so that no more allocation were needed.    
> > > > 
> > > > Ah, ok.
> > > >   
> > > > > That potentially would benefit hotplug, since we could gracefully
> > > > > fail object creation early if there is not enough memory.    
> > > > 
> > > > Yeah, it sounds nice, but I don't see how we can do it.  In order to
> > > > do that the core object has to have enough space for all the threads,
> > > > which means we need both the size of each thread object and the number
> > > > of them.  The size we have (and will be easier to handle after Igor's
> > > > cleanups).  The number, we don't.
> > > >   
> > > > > But the way it's implemented currently doesn't really match that initial
> > > > > goal as array for threads is dynamically allocated later
> > > > > and then we need to dance around it with pointer arithmetic.
> > > > > 
> > > > > BTW: almost any allocation failure in qemu currently
> > > > > is fatal so whether we fail on array alloc or on individual
> > > > > object_new() won't make any difference.
> > > > > 
> > > > > I'd rather see this clean up merged as it simplifies code
> > > > > in these case.    
> > > > 
> > > > Ok, works for me.
> > > >   
> > > 
> > > Since we're in soft freeze already, I guess this won't go to 2.11. Maybe it's
> > > time to create ppc-for-2.12 and apply it there ?  
> > 
> > Yeah, sounds like a plan.
> > 
> 
> Friendly reminder: can you push this to ppc-for-2.12 so that it doesn't fall
> through the cracks ? :)

Uh.. sorry.  I thought there was another spin of this coming.  Can you
resend, and I'll apply to ppc-for-2.12.

-- 
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] [Qemu-ppc] [PATCH] spapr_cpu_core: instantiate CPUs separately

[Greg Kurz]

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

On Mon, 20 Nov 2017 10:17:51 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Nov 14, 2017 at 08:59:43AM +0100, Greg Kurz wrote:
> > On Tue, 7 Nov 2017 06:04:55 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Mon, Nov 06, 2017 at 04:03:07PM +0100, Greg Kurz wrote:  
> > > > On Tue, 17 Oct 2017 17:16:09 +1100
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >     
> > > > > On Mon, Oct 16, 2017 at 10:26:38AM +0200, Igor Mammedov wrote:    
> > > > > > On Sat, 14 Oct 2017 20:33:37 +1100
> > > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > > >       
> > > > > > > On Fri, Oct 13, 2017 at 01:31:44PM +0200, Greg Kurz wrote:      
> > > > > > > > The current code assumes that only the CPU core object holds a
> > > > > > > > reference on each individual CPU object, and happily frees their
> > > > > > > > allocated memory when the core is unrealized. This is dangerous
> > > > > > > > as some other code can legitimely keep a pointer to a CPU if it
> > > > > > > > calls object_ref(), but it would end up with a dangling pointer.
> > > > > > > > 
> > > > > > > > Let's allocate all CPUs with object_new() and let QOM frees them
> > > > > > > > when their reference count reaches zero. This greatly simplify the
> > > > > > > > code as we don't have to fiddle with the instance size anymore.
> > > > > > > > 
> > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org>        
> > > > > > > 
> > > > > > > So, I'm pretty sure my first drafts of the core stuff did things this
> > > > > > > waym and it got nacked, for QOM lifetime reasons that I never really
> > > > > > > understood.      
> > > > > > From what I remember, Andreas would like to see composite CPU object
> > > > > > allocated in one go and then its children initialized with object_initialize()
> > > > > > so that no more allocation were needed.      
> > > > > 
> > > > > Ah, ok.
> > > > >     
> > > > > > That potentially would benefit hotplug, since we could gracefully
> > > > > > fail object creation early if there is not enough memory.      
> > > > > 
> > > > > Yeah, it sounds nice, but I don't see how we can do it.  In order to
> > > > > do that the core object has to have enough space for all the threads,
> > > > > which means we need both the size of each thread object and the number
> > > > > of them.  The size we have (and will be easier to handle after Igor's
> > > > > cleanups).  The number, we don't.
> > > > >     
> > > > > > But the way it's implemented currently doesn't really match that initial
> > > > > > goal as array for threads is dynamically allocated later
> > > > > > and then we need to dance around it with pointer arithmetic.
> > > > > > 
> > > > > > BTW: almost any allocation failure in qemu currently
> > > > > > is fatal so whether we fail on array alloc or on individual
> > > > > > object_new() won't make any difference.
> > > > > > 
> > > > > > I'd rather see this clean up merged as it simplifies code
> > > > > > in these case.      
> > > > > 
> > > > > Ok, works for me.
> > > > >     
> > > > 
> > > > Since we're in soft freeze already, I guess this won't go to 2.11. Maybe it's
> > > > time to create ppc-for-2.12 and apply it there ?    
> > > 
> > > Yeah, sounds like a plan.
> > >   
> > 
> > Friendly reminder: can you push this to ppc-for-2.12 so that it doesn't fall
> > through the cracks ? :)  
> 
> Uh.. sorry.  I thought there was another spin of this coming.  Can you
> resend, and I'll apply to ppc-for-2.12.
> 

Heh, I didn't resend because the patch still applies flawlessly. I'll repost
right away for your convenience.

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