[Qemu-devel] [PATCH v3 0/5] spapr/xics: fix migration of older machine types

[Qemu-devel] [PATCH v3 0/5] spapr/xics: fix migration of older machine types

[Greg Kurz]

v3: - preparatory cleanup in pnv (patch 1)
    - rework ICPState realization and vmstate registration (patches 2,3,4)
    - fix migration using dummy icp/server entries (patch 5)

v2: - some patches from v1 are already merged in ppc-for-2.10
    - added a new fix to a potential memory leak (patch 1)
    - consolidate dt_id computation (patch 3)
    - see individual changelogs for patch 2 and 4

This series is based on:

https://github.com/dgibson/qemu.git ppc-for-2.10

This was lightly tested.

--
Greg

---

Greg Kurz (5):
      pnv_core: drop reference on ICPState object during CPU realization
      xics: add reset() handler to ICPStateClass
      xics: setup cpu at realize time
      xics: directly register ICPState objects to vmstate
      spapr: fix migration of ICPState objects from/to older QEMU


 hw/intc/xics.c          |   86 ++++++++++++++++++++++------------------------
 hw/intc/xics_kvm.c      |   27 +-------------
 hw/ppc/pnv_core.c       |   15 ++++----
 hw/ppc/spapr.c          |   88 ++++++++++++++++++++++++++++++++++++++++++++++-
 hw/ppc/spapr_cpu_core.c |   21 +++++------
 include/hw/ppc/spapr.h  |    2 +
 include/hw/ppc/xics.h   |    3 +-
 7 files changed, 148 insertions(+), 94 deletions(-)

[Qemu-devel] [PATCH v3 1/5] pnv_core: drop reference on ICPState object during CPU realization

[Greg Kurz]

Similarly to what was done to spapr with commit 249127d0dfeb, this patch
ensures that we don't keep an extra reference on the ICPState object. Also
since the object was just created and not reparented yet, the call to
object_property_add_child() should never fail: let's pass &error_abort to
make this clear.

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

diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index 1b7ec70f033d..e8a9a94d5a24 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -119,7 +119,8 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
     Object *obj;
 
     obj = object_new(TYPE_PNV_ICP);
-    object_property_add_child(OBJECT(cpu), "icp", obj, NULL);
+    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
+    object_unref(obj);
     object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
     object_property_set_bool(obj, true, "realized", &local_err);
     if (local_err) {

Re: [Qemu-devel] [PATCH v3 1/5] pnv_core: drop reference on ICPState object during CPU realization

[Cédric Le Goater]

On 06/07/2017 07:16 PM, Greg Kurz wrote:
> Similarly to what was done to spapr with commit 249127d0dfeb, this patch
> ensures that we don't keep an extra reference on the ICPState object. Also
> since the object was just created and not reparented yet, the call to
> object_property_add_child() should never fail: let's pass &error_abort to
> make this clear.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C. 

> ---
>  hw/ppc/pnv_core.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index 1b7ec70f033d..e8a9a94d5a24 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -119,7 +119,8 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
>      Object *obj;
>  
>      obj = object_new(TYPE_PNV_ICP);
> -    object_property_add_child(OBJECT(cpu), "icp", obj, NULL);
> +    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> +    object_unref(obj);
>      object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
>      object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
> 

Re: [Qemu-devel] [PATCH v3 1/5] pnv_core: drop reference on ICPState object during CPU realization

[David Gibson]

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

On Wed, Jun 07, 2017 at 07:49:14PM +0200, Cédric Le Goater wrote:
> On 06/07/2017 07:16 PM, Greg Kurz wrote:
> > Similarly to what was done to spapr with commit 249127d0dfeb, this patch
> > ensures that we don't keep an extra reference on the ICPState object. Also
> > since the object was just created and not reparented yet, the call to
> > object_property_add_child() should never fail: let's pass &error_abort to
> > make this clear.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>

Applied to ppc-for-2.10, thanks.


> 
> Thanks,
> 
> C. 
> 
> > ---
> >  hw/ppc/pnv_core.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> > index 1b7ec70f033d..e8a9a94d5a24 100644
> > --- a/hw/ppc/pnv_core.c
> > +++ b/hw/ppc/pnv_core.c
> > @@ -119,7 +119,8 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
> >      Object *obj;
> >  
> >      obj = object_new(TYPE_PNV_ICP);
> > -    object_property_add_child(OBJECT(cpu), "icp", obj, NULL);
> > +    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> > +    object_unref(obj);
> >      object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
> >      object_property_set_bool(obj, true, "realized", &local_err);
> >      if (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: 819 bytes --]

[Qemu-devel] [PATCH v3 2/5] xics: add reset() handler to ICPStateClass

[Greg Kurz]

Taking into account that qemu_set_irq() returns immediatly if its first
argument is NULL, icp_kvm_reset() largely duplicates icp_reset().

This patch introduces a reset() handler, so that the common logic can
be implemented in icp_reset() only.

While there we can also drop icp_kvm_realize() and icp_kvm_unrealize(). This
causes icp-kvm to be realized in icp_realize(), which sets icp->xics, but
it has no impact.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/xics.c        |    5 +++++
 hw/intc/xics_kvm.c    |   27 ++-------------------------
 include/hw/ppc/xics.h |    1 +
 3 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index ea3516794af7..ec73f02144c9 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -325,6 +325,7 @@ static const VMStateDescription vmstate_icp_server = {
 static void icp_reset(void *dev)
 {
     ICPState *icp = ICP(dev);
+    ICPStateClass *icpc = ICP_GET_CLASS(icp);
 
     icp->xirr = 0;
     icp->pending_priority = 0xff;
@@ -332,6 +333,10 @@ static void icp_reset(void *dev)
 
     /* Make all outputs are deasserted */
     qemu_set_irq(icp->output, 0);
+
+    if (icpc->reset) {
+        icpc->reset(icp);
+    }
 }
 
 static void icp_realize(DeviceState *dev, Error **errp)
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 14b8f6f6e478..45bf110b51e6 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -110,19 +110,8 @@ static int icp_set_kvm_state(ICPState *icp, int version_id)
     return 0;
 }
 
-static void icp_kvm_reset(void *dev)
+static void icp_kvm_reset(ICPState *icp)
 {
-    ICPState *icp = ICP(dev);
-
-    icp->xirr = 0;
-    icp->pending_priority = 0xff;
-    icp->mfrr = 0xff;
-
-    /* Make all outputs as deasserted only if the CPU thread is in use */
-    if (icp->output) {
-        qemu_set_irq(icp->output, 0);
-    }
-
     icp_set_kvm_state(icp, 1);
 }
 
@@ -159,26 +148,14 @@ static void icp_kvm_cpu_setup(ICPState *icp, PowerPCCPU *cpu)
     QLIST_INSERT_HEAD(&kvm_enabled_icps, enabled_icp, node);
 }
 
-static void icp_kvm_realize(DeviceState *dev, Error **errp)
-{
-    qemu_register_reset(icp_kvm_reset, dev);
-}
-
-static void icp_kvm_unrealize(DeviceState *dev, Error **errp)
-{
-    qemu_unregister_reset(icp_kvm_reset, dev);
-}
-
 static void icp_kvm_class_init(ObjectClass *klass, void *data)
 {
-    DeviceClass *dc = DEVICE_CLASS(klass);
     ICPStateClass *icpc = ICP_CLASS(klass);
 
-    dc->realize = icp_kvm_realize;
-    dc->unrealize = icp_kvm_unrealize;
     icpc->pre_save = icp_get_kvm_state;
     icpc->post_load = icp_set_kvm_state;
     icpc->cpu_setup = icp_kvm_cpu_setup;
+    icpc->reset = icp_kvm_reset;
 }
 
 static const TypeInfo icp_kvm_info = {
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index a3073f90533a..40a506eacfb4 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -69,6 +69,7 @@ struct ICPStateClass {
     void (*pre_save)(ICPState *s);
     int (*post_load)(ICPState *s, int version_id);
     void (*cpu_setup)(ICPState *icp, PowerPCCPU *cpu);
+    void (*reset)(ICPState *icp);
 };
 
 struct ICPState {

Re: [Qemu-devel] [PATCH v3 2/5] xics: add reset() handler to ICPStateClass

[Cédric Le Goater]

On 06/07/2017 07:17 PM, Greg Kurz wrote:
> Taking into account that qemu_set_irq() returns immediatly if its first
> argument is NULL, icp_kvm_reset() largely duplicates icp_reset().
> 
> This patch introduces a reset() handler, so that the common logic can
> be implemented in icp_reset() only.
> 
> While there we can also drop icp_kvm_realize() and icp_kvm_unrealize(). This
> causes icp-kvm to be realized in icp_realize(), which sets icp->xics, but
> it has no impact.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>


Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/intc/xics.c        |    5 +++++
>  hw/intc/xics_kvm.c    |   27 ++-------------------------
>  include/hw/ppc/xics.h |    1 +
>  3 files changed, 8 insertions(+), 25 deletions(-)

another good cleanup !

Thanks,

C. 

> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index ea3516794af7..ec73f02144c9 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -325,6 +325,7 @@ static const VMStateDescription vmstate_icp_server = {
>  static void icp_reset(void *dev)
>  {
>      ICPState *icp = ICP(dev);
> +    ICPStateClass *icpc = ICP_GET_CLASS(icp);
>  
>      icp->xirr = 0;
>      icp->pending_priority = 0xff;
> @@ -332,6 +333,10 @@ static void icp_reset(void *dev)
>  
>      /* Make all outputs are deasserted */
>      qemu_set_irq(icp->output, 0);
> +
> +    if (icpc->reset) {
> +        icpc->reset(icp);
> +    }
>  }
>  
>  static void icp_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 14b8f6f6e478..45bf110b51e6 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -110,19 +110,8 @@ static int icp_set_kvm_state(ICPState *icp, int version_id)
>      return 0;
>  }
>  
> -static void icp_kvm_reset(void *dev)
> +static void icp_kvm_reset(ICPState *icp)
>  {
> -    ICPState *icp = ICP(dev);
> -
> -    icp->xirr = 0;
> -    icp->pending_priority = 0xff;
> -    icp->mfrr = 0xff;
> -
> -    /* Make all outputs as deasserted only if the CPU thread is in use */
> -    if (icp->output) {
> -        qemu_set_irq(icp->output, 0);
> -    }
> -
>      icp_set_kvm_state(icp, 1);
>  }
>  
> @@ -159,26 +148,14 @@ static void icp_kvm_cpu_setup(ICPState *icp, PowerPCCPU *cpu)
>      QLIST_INSERT_HEAD(&kvm_enabled_icps, enabled_icp, node);
>  }
>  
> -static void icp_kvm_realize(DeviceState *dev, Error **errp)
> -{
> -    qemu_register_reset(icp_kvm_reset, dev);
> -}
> -
> -static void icp_kvm_unrealize(DeviceState *dev, Error **errp)
> -{
> -    qemu_unregister_reset(icp_kvm_reset, dev);
> -}
> -
>  static void icp_kvm_class_init(ObjectClass *klass, void *data)
>  {
> -    DeviceClass *dc = DEVICE_CLASS(klass);
>      ICPStateClass *icpc = ICP_CLASS(klass);
>  
> -    dc->realize = icp_kvm_realize;
> -    dc->unrealize = icp_kvm_unrealize;
>      icpc->pre_save = icp_get_kvm_state;
>      icpc->post_load = icp_set_kvm_state;
>      icpc->cpu_setup = icp_kvm_cpu_setup;
> +    icpc->reset = icp_kvm_reset;
>  }
>  
>  static const TypeInfo icp_kvm_info = {
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index a3073f90533a..40a506eacfb4 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -69,6 +69,7 @@ struct ICPStateClass {
>      void (*pre_save)(ICPState *s);
>      int (*post_load)(ICPState *s, int version_id);
>      void (*cpu_setup)(ICPState *icp, PowerPCCPU *cpu);
> +    void (*reset)(ICPState *icp);
>  };
>  
>  struct ICPState {
> 

Re: [Qemu-devel] [PATCH v3 2/5] xics: add reset() handler to ICPStateClass

[David Gibson]

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

On Wed, Jun 07, 2017 at 07:47:04PM +0200, Cédric Le Goater wrote:
1;4602;0c> On 06/07/2017 07:17 PM, Greg Kurz wrote:
> > Taking into account that qemu_set_irq() returns immediatly if its first
> > argument is NULL, icp_kvm_reset() largely duplicates icp_reset().
> > 
> > This patch introduces a reset() handler, so that the common logic can
> > be implemented in icp_reset() only.
> > 
> > While there we can also drop icp_kvm_realize() and icp_kvm_unrealize(). This
> > causes icp-kvm to be realized in icp_realize(), which sets icp->xics, but
> > it has no impact.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> > ---
> >  hw/intc/xics.c        |    5 +++++
> >  hw/intc/xics_kvm.c    |   27 ++-------------------------
> >  include/hw/ppc/xics.h |    1 +
> >  3 files changed, 8 insertions(+), 25 deletions(-)
> 
> another good cleanup !

I concur.  Applied to ppc-for-2.10.

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

[Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time

[Greg Kurz]

Until recently, spapr used to allocate ICPState objects for the lifetime
of the machine. They would only be associated to vCPUs in xics_cpu_setup()
when plugging a CPU core.

Now that ICPState objects have the same lifecycle as vCPUs, it is
possible to associate them during realization.

This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
is passed as a property. Note that vCPU now needs to be realized first
for the IRQs to be allocated. It also needs to resetted before ICPState
realization in order to synchronize with KVM.

Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't
needed anymore and can be safely dropped.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/xics.c          |   76 ++++++++++++++++++++---------------------------
 hw/ppc/pnv_core.c       |   16 ++++------
 hw/ppc/spapr_cpu_core.c |   21 ++++++-------
 include/hw/ppc/xics.h   |    2 -
 4 files changed, 48 insertions(+), 67 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index ec73f02144c9..330441ff7fe8 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -38,50 +38,6 @@
 #include "monitor/monitor.h"
 #include "hw/intc/intc.h"
 
-void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
-{
-    CPUState *cs = CPU(cpu);
-    ICPState *icp = ICP(cpu->intc);
-
-    assert(icp);
-    assert(cs == icp->cs);
-
-    icp->output = NULL;
-    icp->cs = NULL;
-}
-
-void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp)
-{
-    CPUState *cs = CPU(cpu);
-    CPUPPCState *env = &cpu->env;
-    ICPStateClass *icpc;
-
-    assert(icp);
-
-    cpu->intc = OBJECT(icp);
-    icp->cs = cs;
-
-    icpc = ICP_GET_CLASS(icp);
-    if (icpc->cpu_setup) {
-        icpc->cpu_setup(icp, cpu);
-    }
-
-    switch (PPC_INPUT(env)) {
-    case PPC_FLAGS_INPUT_POWER7:
-        icp->output = env->irq_inputs[POWER7_INPUT_INT];
-        break;
-
-    case PPC_FLAGS_INPUT_970:
-        icp->output = env->irq_inputs[PPC970_INPUT_INT];
-        break;
-
-    default:
-        error_report("XICS interrupt controller does not support this CPU "
-                     "bus model");
-        abort();
-    }
-}
-
 void icp_pic_print_info(ICPState *icp, Monitor *mon)
 {
     int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
@@ -343,6 +299,8 @@ static void icp_realize(DeviceState *dev, Error **errp)
 {
     ICPState *icp = ICP(dev);
     ICPStateClass *icpc = ICP_GET_CLASS(dev);
+    PowerPCCPU *cpu;
+    CPUPPCState *env;
     Object *obj;
     Error *err = NULL;
 
@@ -355,6 +313,36 @@ static void icp_realize(DeviceState *dev, Error **errp)
 
     icp->xics = XICS_FABRIC(obj);
 
+    obj = object_property_get_link(OBJECT(dev), "cs", &err);
+    if (!obj) {
+        error_setg(errp, "%s: required link 'xics' not found: %s",
+                   __func__, error_get_pretty(err));
+        return;
+    }
+
+    cpu = POWERPC_CPU(obj);
+    cpu->intc = OBJECT(icp);
+    icp->cs = CPU(obj);
+
+    if (icpc->cpu_setup) {
+        icpc->cpu_setup(icp, cpu);
+    }
+
+    env = &cpu->env;
+    switch (PPC_INPUT(env)) {
+    case PPC_FLAGS_INPUT_POWER7:
+        icp->output = env->irq_inputs[POWER7_INPUT_INT];
+        break;
+
+    case PPC_FLAGS_INPUT_970:
+        icp->output = env->irq_inputs[PPC970_INPUT_INT];
+        break;
+
+    default:
+        error_setg(errp, "XICS interrupt controller does not support this CPU bus model");
+        return;
+    }
+
     if (icpc->realize) {
         icpc->realize(dev, errp);
     }
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index e8a9a94d5a24..1393005e76f3 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -118,19 +118,19 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     Object *obj;
 
-    obj = object_new(TYPE_PNV_ICP);
-    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
-    object_unref(obj);
-    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
-    object_property_set_bool(obj, true, "realized", &local_err);
+    object_property_set_bool(child, true, "realized", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }
 
-    object_property_set_bool(child, true, "realized", &local_err);
+    obj = object_new(TYPE_PNV_ICP);
+    object_property_add_child(child, "icp", obj, NULL);
+    object_unref(obj);
+    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
+    object_property_add_const_link(obj, "cs", child, &error_abort);
+    object_property_set_bool(obj, true, "realized", &local_err);
     if (local_err) {
-        object_unparent(obj);
         error_propagate(errp, local_err);
         return;
     }
@@ -141,8 +141,6 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
         error_propagate(errp, local_err);
         return;
     }
-
-    xics_cpu_setup(xi, cpu, ICP(obj));
 }
 
 static void pnv_core_realize(DeviceState *dev, Error **errp)
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 029a14120edd..9a6259525953 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -53,9 +53,6 @@ static void spapr_cpu_reset(void *opaque)
 
 static void spapr_cpu_destroy(PowerPCCPU *cpu)
 {
-    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
-
-    xics_cpu_destroy(XICS_FABRIC(spapr), cpu);
     qemu_unregister_reset(spapr_cpu_reset, cpu);
 }
 
@@ -140,28 +137,28 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     CPUState *cs = CPU(child);
     PowerPCCPU *cpu = POWERPC_CPU(cs);
-    Object *obj;
+    Object *obj = NULL;
 
-    obj = object_new(spapr->icp_type);
-    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
-    object_unref(obj);
-    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
-    object_property_set_bool(obj, true, "realized", &local_err);
+    object_property_set_bool(child, true, "realized", &local_err);
     if (local_err) {
         goto error;
     }
 
-    object_property_set_bool(child, true, "realized", &local_err);
+    spapr_cpu_init(spapr, cpu, &local_err);
     if (local_err) {
         goto error;
     }
 
-    spapr_cpu_init(spapr, cpu, &local_err);
+    obj = object_new(spapr->icp_type);
+    object_property_add_child(child, "icp", obj, &error_abort);
+    object_unref(obj);
+    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
+    object_property_add_const_link(obj, "cs", child, &error_abort);
+    object_property_set_bool(obj, true, "realized", &local_err);
     if (local_err) {
         goto error;
     }
 
-    xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
     return;
 
 error:
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 40a506eacfb4..05767a15be9a 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -183,8 +183,6 @@ void spapr_dt_xics(int nr_servers, void *fdt, uint32_t phandle);
 
 qemu_irq xics_get_qirq(XICSFabric *xi, int irq);
 ICPState *xics_icp_get(XICSFabric *xi, int server);
-void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp);
-void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu);
 
 /* Internal XICS interfaces */
 void icp_set_cppr(ICPState *icp, uint8_t cppr);

Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time

[Cédric Le Goater]

On 06/07/2017 07:17 PM, Greg Kurz wrote:
> Until recently, spapr used to allocate ICPState objects for the lifetime
> of the machine. They would only be associated to vCPUs in xics_cpu_setup()
> when plugging a CPU core.
> 
> Now that ICPState objects have the same lifecycle as vCPUs, it is
> possible to associate them during realization.
> 
> This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
> is passed as a property. Note that vCPU now needs to be realized first
> for the IRQs to be allocated. It also needs to resetted before ICPState
> realization in order to synchronize with KVM.
> 
> Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't
> needed anymore and can be safely dropped.

I like the idea but I think the assignment of ->cs attribute should be 
moved under icp_kvm_cpu_setup(), as it is only used under xics_kvm by 
the kvm_vcpu_ioctl() calls. 

Ideally, we should also introduce :

	struct KvmState {
	    ICPState parent_obj;
	
	    CPUState *cs;
	};

That would be cleaner, but that might introduce some migration compat 
issues  again ...

Some minor comments below,

Thanks,

C. 

> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/intc/xics.c          |   76 ++++++++++++++++++++---------------------------
>  hw/ppc/pnv_core.c       |   16 ++++------
>  hw/ppc/spapr_cpu_core.c |   21 ++++++-------
>  include/hw/ppc/xics.h   |    2 -
>  4 files changed, 48 insertions(+), 67 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index ec73f02144c9..330441ff7fe8 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -38,50 +38,6 @@
>  #include "monitor/monitor.h"
>  #include "hw/intc/intc.h"
>  
> -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
> -{
> -    CPUState *cs = CPU(cpu);
> -    ICPState *icp = ICP(cpu->intc);
> -
> -    assert(icp);
> -    assert(cs == icp->cs);
> -
> -    icp->output = NULL;
> -    icp->cs = NULL;
> -}
> -
> -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp)
> -{
> -    CPUState *cs = CPU(cpu);
> -    CPUPPCState *env = &cpu->env;
> -    ICPStateClass *icpc;
> -
> -    assert(icp);
> -
> -    cpu->intc = OBJECT(icp);
> -    icp->cs = cs;
> -
> -    icpc = ICP_GET_CLASS(icp);
> -    if (icpc->cpu_setup) {
> -        icpc->cpu_setup(icp, cpu);
> -    }
> -
> -    switch (PPC_INPUT(env)) {
> -    case PPC_FLAGS_INPUT_POWER7:
> -        icp->output = env->irq_inputs[POWER7_INPUT_INT];
> -        break;
> -
> -    case PPC_FLAGS_INPUT_970:
> -        icp->output = env->irq_inputs[PPC970_INPUT_INT];
> -        break;
> -
> -    default:
> -        error_report("XICS interrupt controller does not support this CPU "
> -                     "bus model");
> -        abort();
> -    }
> -}
> -
>  void icp_pic_print_info(ICPState *icp, Monitor *mon)
>  {
>      int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
> @@ -343,6 +299,8 @@ static void icp_realize(DeviceState *dev, Error **errp)
>  {
>      ICPState *icp = ICP(dev);
>      ICPStateClass *icpc = ICP_GET_CLASS(dev);
> +    PowerPCCPU *cpu;
> +    CPUPPCState *env;
>      Object *obj;
>      Error *err = NULL;
>  
> @@ -355,6 +313,36 @@ static void icp_realize(DeviceState *dev, Error **errp)
>  
>      icp->xics = XICS_FABRIC(obj);
>  
> +    obj = object_property_get_link(OBJECT(dev), "cs", &err);
> +    if (!obj) {
> +        error_setg(errp, "%s: required link 'xics' not found: %s",
> +                   __func__, error_get_pretty(err));
> +        return;
> +    }
> +
> +    cpu = POWERPC_CPU(obj);
> +    cpu->intc = OBJECT(icp);
> +    icp->cs = CPU(obj);

only xics_kvm needs ->cs. 

> +
> +    if (icpc->cpu_setup) {
> +        icpc->cpu_setup(icp, cpu);
> +    }
> +
> +    env = &cpu->env;
> +    switch (PPC_INPUT(env)) {
> +    case PPC_FLAGS_INPUT_POWER7:
> +        icp->output = env->irq_inputs[POWER7_INPUT_INT];
> +        break;
> +
> +    case PPC_FLAGS_INPUT_970:
> +        icp->output = env->irq_inputs[PPC970_INPUT_INT];
> +        break;
> +
> +    default:
> +        error_setg(errp, "XICS interrupt controller does not support this CPU bus model");
> +        return;
> +    }
> +
>      if (icpc->realize) {
>          icpc->realize(dev, errp);
>      }
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index e8a9a94d5a24..1393005e76f3 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -118,19 +118,19 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      Object *obj;
>  
> -    obj = object_new(TYPE_PNV_ICP);
> -    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> -    object_unref(obj);
> -    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
> -    object_property_set_bool(obj, true, "realized", &local_err);
> +    object_property_set_bool(child, true, "realized", &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>  
> -    object_property_set_bool(child, true, "realized", &local_err);
> +    obj = object_new(TYPE_PNV_ICP);
> +    object_property_add_child(child, "icp", obj, NULL);
> +    object_unref(obj);
> +    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
> +    object_property_add_const_link(obj, "cs", child, &error_abort);

may be link the cpu object instead ? 

> +    object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
> -        object_unparent(obj);
>          error_propagate(errp, local_err);
>          return;
>      }
> @@ -141,8 +141,6 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
>          error_propagate(errp, local_err);
>          return;
>      }
> -
> -    xics_cpu_setup(xi, cpu, ICP(obj));
>  }
>  
>  static void pnv_core_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 029a14120edd..9a6259525953 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -53,9 +53,6 @@ static void spapr_cpu_reset(void *opaque)
>  
>  static void spapr_cpu_destroy(PowerPCCPU *cpu)
>  {
> -    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> -
> -    xics_cpu_destroy(XICS_FABRIC(spapr), cpu);
>      qemu_unregister_reset(spapr_cpu_reset, cpu);
>  }
>  
> @@ -140,28 +137,28 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      CPUState *cs = CPU(child);
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> -    Object *obj;
> +    Object *obj = NULL;
>  
> -    obj = object_new(spapr->icp_type);
> -    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> -    object_unref(obj);
> -    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> -    object_property_set_bool(obj, true, "realized", &local_err);
> +    object_property_set_bool(child, true, "realized", &local_err);
>      if (local_err) {
>          goto error;
>      }
>  
> -    object_property_set_bool(child, true, "realized", &local_err);
> +    spapr_cpu_init(spapr, cpu, &local_err);
>      if (local_err) {
>          goto error;
>      }
>  
> -    spapr_cpu_init(spapr, cpu, &local_err);
> +    obj = object_new(spapr->icp_type);
> +    object_property_add_child(child, "icp", obj, &error_abort);
> +    object_unref(obj);
> +    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> +    object_property_add_const_link(obj, "cs", child, &error_abort);

same here.

> +    object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
>          goto error;
>      }
>  
> -    xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
>      return;
>  
>  error:
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 40a506eacfb4..05767a15be9a 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -183,8 +183,6 @@ void spapr_dt_xics(int nr_servers, void *fdt, uint32_t phandle);
>  
>  qemu_irq xics_get_qirq(XICSFabric *xi, int irq);
>  ICPState *xics_icp_get(XICSFabric *xi, int server);
> -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp);
> -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu);
>  
>  /* Internal XICS interfaces */
>  void icp_set_cppr(ICPState *icp, uint8_t cppr);
> 

Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time

[Greg Kurz]

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

On Wed, 7 Jun 2017 20:11:38 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 06/07/2017 07:17 PM, Greg Kurz wrote:
> > Until recently, spapr used to allocate ICPState objects for the lifetime
> > of the machine. They would only be associated to vCPUs in xics_cpu_setup()
> > when plugging a CPU core.
> > 
> > Now that ICPState objects have the same lifecycle as vCPUs, it is
> > possible to associate them during realization.
> > 
> > This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
> > is passed as a property. Note that vCPU now needs to be realized first
> > for the IRQs to be allocated. It also needs to resetted before ICPState
> > realization in order to synchronize with KVM.
> > 
> > Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't
> > needed anymore and can be safely dropped.  
> 
> I like the idea but I think the assignment of ->cs attribute should be 
> moved under icp_kvm_cpu_setup(), as it is only used under xics_kvm by 
> the kvm_vcpu_ioctl() calls. 
> 

Well, cs->cpu_index is also used for traces and to implement the 'info pic'
monitor command.

> Ideally, we should also introduce :
> 
> 	struct KvmState {
> 	    ICPState parent_obj;
> 	
> 	    CPUState *cs;
> 	};
> 
> That would be cleaner, but that might introduce some migration compat 
> issues  again ...
> 

No, as long as it doesn't change state, we're good. My concern is more
about how to pass cs to xics_kvm and the cs->cpu_index to xics.

> Some minor comments below,
> 
> Thanks,
> 
> C. 
> 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/intc/xics.c          |   76 ++++++++++++++++++++---------------------------
> >  hw/ppc/pnv_core.c       |   16 ++++------
> >  hw/ppc/spapr_cpu_core.c |   21 ++++++-------
> >  include/hw/ppc/xics.h   |    2 -
> >  4 files changed, 48 insertions(+), 67 deletions(-)
> > 
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index ec73f02144c9..330441ff7fe8 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -38,50 +38,6 @@
> >  #include "monitor/monitor.h"
> >  #include "hw/intc/intc.h"
> >  
> > -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
> > -{
> > -    CPUState *cs = CPU(cpu);
> > -    ICPState *icp = ICP(cpu->intc);
> > -
> > -    assert(icp);
> > -    assert(cs == icp->cs);
> > -
> > -    icp->output = NULL;
> > -    icp->cs = NULL;
> > -}
> > -
> > -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp)
> > -{
> > -    CPUState *cs = CPU(cpu);
> > -    CPUPPCState *env = &cpu->env;
> > -    ICPStateClass *icpc;
> > -
> > -    assert(icp);
> > -
> > -    cpu->intc = OBJECT(icp);
> > -    icp->cs = cs;
> > -
> > -    icpc = ICP_GET_CLASS(icp);
> > -    if (icpc->cpu_setup) {
> > -        icpc->cpu_setup(icp, cpu);
> > -    }
> > -
> > -    switch (PPC_INPUT(env)) {
> > -    case PPC_FLAGS_INPUT_POWER7:
> > -        icp->output = env->irq_inputs[POWER7_INPUT_INT];
> > -        break;
> > -
> > -    case PPC_FLAGS_INPUT_970:
> > -        icp->output = env->irq_inputs[PPC970_INPUT_INT];
> > -        break;
> > -
> > -    default:
> > -        error_report("XICS interrupt controller does not support this CPU "
> > -                     "bus model");
> > -        abort();
> > -    }
> > -}
> > -
> >  void icp_pic_print_info(ICPState *icp, Monitor *mon)
> >  {
> >      int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
> > @@ -343,6 +299,8 @@ static void icp_realize(DeviceState *dev, Error **errp)
> >  {
> >      ICPState *icp = ICP(dev);
> >      ICPStateClass *icpc = ICP_GET_CLASS(dev);
> > +    PowerPCCPU *cpu;
> > +    CPUPPCState *env;
> >      Object *obj;
> >      Error *err = NULL;
> >  
> > @@ -355,6 +313,36 @@ static void icp_realize(DeviceState *dev, Error **errp)
> >  
> >      icp->xics = XICS_FABRIC(obj);
> >  
> > +    obj = object_property_get_link(OBJECT(dev), "cs", &err);
> > +    if (!obj) {
> > +        error_setg(errp, "%s: required link 'xics' not found: %s",
> > +                   __func__, error_get_pretty(err));
> > +        return;
> > +    }
> > +
> > +    cpu = POWERPC_CPU(obj);
> > +    cpu->intc = OBJECT(icp);
> > +    icp->cs = CPU(obj);  
> 
> only xics_kvm needs ->cs. 
> 

yeah, maybe the base class should only have a cpu_index field:

    icp->cpu_index = CPU(obj)->cpu_index;

> > +
> > +    if (icpc->cpu_setup) {
> > +        icpc->cpu_setup(icp, cpu);
> > +    }
> > +
> > +    env = &cpu->env;
> > +    switch (PPC_INPUT(env)) {
> > +    case PPC_FLAGS_INPUT_POWER7:
> > +        icp->output = env->irq_inputs[POWER7_INPUT_INT];
> > +        break;
> > +
> > +    case PPC_FLAGS_INPUT_970:
> > +        icp->output = env->irq_inputs[PPC970_INPUT_INT];
> > +        break;
> > +
> > +    default:
> > +        error_setg(errp, "XICS interrupt controller does not support this CPU bus model");
> > +        return;
> > +    }
> > +
> >      if (icpc->realize) {
> >          icpc->realize(dev, errp);
> >      }
> > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> > index e8a9a94d5a24..1393005e76f3 100644
> > --- a/hw/ppc/pnv_core.c
> > +++ b/hw/ppc/pnv_core.c
> > @@ -118,19 +118,19 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
> >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> >      Object *obj;
> >  
> > -    obj = object_new(TYPE_PNV_ICP);
> > -    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> > -    object_unref(obj);
> > -    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
> > -    object_property_set_bool(obj, true, "realized", &local_err);
> > +    object_property_set_bool(child, true, "realized", &local_err);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> >          return;
> >      }
> >  
> > -    object_property_set_bool(child, true, "realized", &local_err);
> > +    obj = object_new(TYPE_PNV_ICP);
> > +    object_property_add_child(child, "icp", obj, NULL);
> > +    object_unref(obj);
> > +    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
> > +    object_property_add_const_link(obj, "cs", child, &error_abort);  
> 
> may be link the cpu object instead ? 
> 

I'm not sure to understand... isn't child supposed to be a CPU index here ?

> > +    object_property_set_bool(obj, true, "realized", &local_err);
> >      if (local_err) {
> > -        object_unparent(obj);
> >          error_propagate(errp, local_err);
> >          return;
> >      }
> > @@ -141,8 +141,6 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
> >          error_propagate(errp, local_err);
> >          return;
> >      }
> > -
> > -    xics_cpu_setup(xi, cpu, ICP(obj));
> >  }
> >  
> >  static void pnv_core_realize(DeviceState *dev, Error **errp)
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 029a14120edd..9a6259525953 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -53,9 +53,6 @@ static void spapr_cpu_reset(void *opaque)
> >  
> >  static void spapr_cpu_destroy(PowerPCCPU *cpu)
> >  {
> > -    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > -
> > -    xics_cpu_destroy(XICS_FABRIC(spapr), cpu);
> >      qemu_unregister_reset(spapr_cpu_reset, cpu);
> >  }
> >  
> > @@ -140,28 +137,28 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >      CPUState *cs = CPU(child);
> >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> > -    Object *obj;
> > +    Object *obj = NULL;
> >  
> > -    obj = object_new(spapr->icp_type);
> > -    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> > -    object_unref(obj);
> > -    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> > -    object_property_set_bool(obj, true, "realized", &local_err);
> > +    object_property_set_bool(child, true, "realized", &local_err);
> >      if (local_err) {
> >          goto error;
> >      }
> >  
> > -    object_property_set_bool(child, true, "realized", &local_err);
> > +    spapr_cpu_init(spapr, cpu, &local_err);
> >      if (local_err) {
> >          goto error;
> >      }
> >  
> > -    spapr_cpu_init(spapr, cpu, &local_err);
> > +    obj = object_new(spapr->icp_type);
> > +    object_property_add_child(child, "icp", obj, &error_abort);
> > +    object_unref(obj);
> > +    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> > +    object_property_add_const_link(obj, "cs", child, &error_abort);  
> 
> same here.
> 
> > +    object_property_set_bool(obj, true, "realized", &local_err);
> >      if (local_err) {
> >          goto error;
> >      }
> >  
> > -    xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
> >      return;
> >  
> >  error:
> > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> > index 40a506eacfb4..05767a15be9a 100644
> > --- a/include/hw/ppc/xics.h
> > +++ b/include/hw/ppc/xics.h
> > @@ -183,8 +183,6 @@ void spapr_dt_xics(int nr_servers, void *fdt, uint32_t phandle);
> >  
> >  qemu_irq xics_get_qirq(XICSFabric *xi, int irq);
> >  ICPState *xics_icp_get(XICSFabric *xi, int server);
> > -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp);
> > -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu);
> >  
> >  /* Internal XICS interfaces */
> >  void icp_set_cppr(ICPState *icp, uint8_t cppr);
> >   
> 


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

Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time

[David Gibson]

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

On Wed, Jun 07, 2017 at 10:55:07PM +0200, Greg Kurz wrote:
> On Wed, 7 Jun 2017 20:11:38 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
> > On 06/07/2017 07:17 PM, Greg Kurz wrote:
> > > Until recently, spapr used to allocate ICPState objects for the lifetime
> > > of the machine. They would only be associated to vCPUs in xics_cpu_setup()
> > > when plugging a CPU core.
> > > 
> > > Now that ICPState objects have the same lifecycle as vCPUs, it is
> > > possible to associate them during realization.
> > > 
> > > This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
> > > is passed as a property. Note that vCPU now needs to be realized first
> > > for the IRQs to be allocated. It also needs to resetted before ICPState
> > > realization in order to synchronize with KVM.
> > > 
> > > Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't
> > > needed anymore and can be safely dropped.  
> > 
> > I like the idea but I think the assignment of ->cs attribute should be 
> > moved under icp_kvm_cpu_setup(), as it is only used under xics_kvm by 
> > the kvm_vcpu_ioctl() calls. 
> > 
> 
> Well, cs->cpu_index is also used for traces and to implement the 'info pic'
> monitor command.

Right.  I think it makes sense for the ICP to have a handle on it's
associated CPU, even if we don't actually use it in all cases right
now.  So I have no problem with the property being in all ICPs; I
think that will be cleaner than special casing xics_kvm.  Especially
if we have to un-special-case it sometime in future because we need
to access the CPU object for some reason we haven't thought of yet.

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

Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time

[Greg Kurz]

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

On Thu, 8 Jun 2017 11:53:29 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jun 07, 2017 at 10:55:07PM +0200, Greg Kurz wrote:
> > On Wed, 7 Jun 2017 20:11:38 +0200
> > Cédric Le Goater <clg@kaod.org> wrote:
> >   
> > > On 06/07/2017 07:17 PM, Greg Kurz wrote:  
> > > > Until recently, spapr used to allocate ICPState objects for the lifetime
> > > > of the machine. They would only be associated to vCPUs in xics_cpu_setup()
> > > > when plugging a CPU core.
> > > > 
> > > > Now that ICPState objects have the same lifecycle as vCPUs, it is
> > > > possible to associate them during realization.
> > > > 
> > > > This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
> > > > is passed as a property. Note that vCPU now needs to be realized first
> > > > for the IRQs to be allocated. It also needs to resetted before ICPState
> > > > realization in order to synchronize with KVM.
> > > > 
> > > > Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't
> > > > needed anymore and can be safely dropped.    
> > > 
> > > I like the idea but I think the assignment of ->cs attribute should be 
> > > moved under icp_kvm_cpu_setup(), as it is only used under xics_kvm by 
> > > the kvm_vcpu_ioctl() calls. 
> > >   
> > 
> > Well, cs->cpu_index is also used for traces and to implement the 'info pic'
> > monitor command.  
> 
> Right.  I think it makes sense for the ICP to have a handle on it's
> associated CPU, even if we don't actually use it in all cases right
> now.  So I have no problem with the property being in all ICPs; I
> think that will be cleaner than special casing xics_kvm.  Especially
> if we have to un-special-case it sometime in future because we need
> to access the CPU object for some reason we haven't thought of yet.
> 

We had discussed with Cedric about that actually but when I started
to write code, I had the impression that I would have to do convoluted
things to get rid of the CPU handle in ICP.

Thanks for confirming this.

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

Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time

[Cédric Le Goater]

On 06/08/2017 11:14 AM, Greg Kurz wrote:
> On Thu, 8 Jun 2017 11:53:29 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
>> On Wed, Jun 07, 2017 at 10:55:07PM +0200, Greg Kurz wrote:
>>> On Wed, 7 Jun 2017 20:11:38 +0200
>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>   
>>>> On 06/07/2017 07:17 PM, Greg Kurz wrote:  
>>>>> Until recently, spapr used to allocate ICPState objects for the lifetime
>>>>> of the machine. They would only be associated to vCPUs in xics_cpu_setup()
>>>>> when plugging a CPU core.
>>>>>
>>>>> Now that ICPState objects have the same lifecycle as vCPUs, it is
>>>>> possible to associate them during realization.
>>>>>
>>>>> This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
>>>>> is passed as a property. Note that vCPU now needs to be realized first
>>>>> for the IRQs to be allocated. It also needs to resetted before ICPState
>>>>> realization in order to synchronize with KVM.
>>>>>
>>>>> Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't
>>>>> needed anymore and can be safely dropped.    
>>>>
>>>> I like the idea but I think the assignment of ->cs attribute should be 
>>>> moved under icp_kvm_cpu_setup(), as it is only used under xics_kvm by 
>>>> the kvm_vcpu_ioctl() calls. 
>>>>   
>>>
>>> Well, cs->cpu_index is also used for traces and to implement the 'info pic'
>>> monitor command.  
>>
>> Right.  I think it makes sense for the ICP to have a handle on it's
>> associated CPU, even if we don't actually use it in all cases right
>> now.  So I have no problem with the property being in all ICPs; I
>> think that will be cleaner than special casing xics_kvm.  Especially
>> if we have to un-special-case it sometime in future because we need
>> to access the CPU object for some reason we haven't thought of yet.
>>
> 
> We had discussed with Cedric about that actually but when I started
> to write code, I had the impression that I would have to do convoluted
> things to get rid of the CPU handle in ICP.

There is nothing complex and it would surely simplify cpu_setup(). 

But, the main argument is that it might be useful to other platforms.   
So there is not much value in removing it. I am OK with that. I am less
OK with using the index, even if it is useful for the migration state 
of ICP objects today. 

We could be tempted to use more of cpu_index, like we have done in 
the past, and that was really complex to untangle. Let's be cautious.

C.

Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time

[Greg Kurz]

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

On Thu, 8 Jun 2017 11:25:52 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 06/08/2017 11:14 AM, Greg Kurz wrote:
> > On Thu, 8 Jun 2017 11:53:29 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> >> On Wed, Jun 07, 2017 at 10:55:07PM +0200, Greg Kurz wrote:  
> >>> On Wed, 7 Jun 2017 20:11:38 +0200
> >>> Cédric Le Goater <clg@kaod.org> wrote:
> >>>     
> >>>> On 06/07/2017 07:17 PM, Greg Kurz wrote:    
> >>>>> Until recently, spapr used to allocate ICPState objects for the lifetime
> >>>>> of the machine. They would only be associated to vCPUs in xics_cpu_setup()
> >>>>> when plugging a CPU core.
> >>>>>
> >>>>> Now that ICPState objects have the same lifecycle as vCPUs, it is
> >>>>> possible to associate them during realization.
> >>>>>
> >>>>> This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
> >>>>> is passed as a property. Note that vCPU now needs to be realized first
> >>>>> for the IRQs to be allocated. It also needs to resetted before ICPState
> >>>>> realization in order to synchronize with KVM.
> >>>>>
> >>>>> Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't
> >>>>> needed anymore and can be safely dropped.      
> >>>>
> >>>> I like the idea but I think the assignment of ->cs attribute should be 
> >>>> moved under icp_kvm_cpu_setup(), as it is only used under xics_kvm by 
> >>>> the kvm_vcpu_ioctl() calls. 
> >>>>     
> >>>
> >>> Well, cs->cpu_index is also used for traces and to implement the 'info pic'
> >>> monitor command.    
> >>
> >> Right.  I think it makes sense for the ICP to have a handle on it's
> >> associated CPU, even if we don't actually use it in all cases right
> >> now.  So I have no problem with the property being in all ICPs; I
> >> think that will be cleaner than special casing xics_kvm.  Especially
> >> if we have to un-special-case it sometime in future because we need
> >> to access the CPU object for some reason we haven't thought of yet.
> >>  
> > 
> > We had discussed with Cedric about that actually but when I started
> > to write code, I had the impression that I would have to do convoluted
> > things to get rid of the CPU handle in ICP.  
> 
> There is nothing complex and it would surely simplify cpu_setup(). 
> 
> But, the main argument is that it might be useful to other platforms.   
> So there is not much value in removing it. I am OK with that. I am less
> OK with using the index, even if it is useful for the migration state 
> of ICP objects today. 
> 
> We could be tempted to use more of cpu_index, like we have done in 
> the past, and that was really complex to untangle. Let's be cautious.
> 

I agree. Maybe this could be hidden in a icpc->get_index() handler ?

> C.

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

Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time

[Cédric Le Goater]

On 06/07/2017 10:55 PM, Greg Kurz wrote:
> On Wed, 7 Jun 2017 20:11:38 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> On 06/07/2017 07:17 PM, Greg Kurz wrote:
>>> Until recently, spapr used to allocate ICPState objects for the lifetime
>>> of the machine. They would only be associated to vCPUs in xics_cpu_setup()
>>> when plugging a CPU core.
>>>
>>> Now that ICPState objects have the same lifecycle as vCPUs, it is
>>> possible to associate them during realization.
>>>
>>> This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
>>> is passed as a property. Note that vCPU now needs to be realized first
>>> for the IRQs to be allocated. It also needs to resetted before ICPState
>>> realization in order to synchronize with KVM.
>>>
>>> Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't
>>> needed anymore and can be safely dropped.  
>>
>> I like the idea but I think the assignment of ->cs attribute should be 
>> moved under icp_kvm_cpu_setup(), as it is only used under xics_kvm by 
>> the kvm_vcpu_ioctl() calls. 
>>
> 
> Well, cs->cpu_index is also used for traces and to implement the 'info pic'
> monitor command.

yes. But, these are just printfs.

>> Ideally, we should also introduce :
>>
>> 	struct KvmState {
>> 	    ICPState parent_obj;
>> 	
>> 	    CPUState *cs;
>> 	};
>>
>> That would be cleaner, but that might introduce some migration compat 
>> issues  again ...
>>
> 
> No, as long as it doesn't change state, we're good. My concern is more
> about how to pass cs to xics_kvm 

That can be done in the cpu_setup handler.

> and the cs->cpu_index to xics.

The printfs are interesting to have but not vital. 

David has a good point for keeping ->cs. So let's abandon the idea.  

>> Some minor comments below,
>>
>> Thanks,
>>
>> C. 
>>
>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>> ---
>>>  hw/intc/xics.c          |   76 ++++++++++++++++++++---------------------------
>>>  hw/ppc/pnv_core.c       |   16 ++++------
>>>  hw/ppc/spapr_cpu_core.c |   21 ++++++-------
>>>  include/hw/ppc/xics.h   |    2 -
>>>  4 files changed, 48 insertions(+), 67 deletions(-)
>>>
>>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>>> index ec73f02144c9..330441ff7fe8 100644
>>> --- a/hw/intc/xics.c
>>> +++ b/hw/intc/xics.c
>>> @@ -38,50 +38,6 @@
>>>  #include "monitor/monitor.h"
>>>  #include "hw/intc/intc.h"
>>>  
>>> -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
>>> -{
>>> -    CPUState *cs = CPU(cpu);
>>> -    ICPState *icp = ICP(cpu->intc);
>>> -
>>> -    assert(icp);
>>> -    assert(cs == icp->cs);
>>> -
>>> -    icp->output = NULL;
>>> -    icp->cs = NULL;
>>> -}
>>> -
>>> -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp)
>>> -{
>>> -    CPUState *cs = CPU(cpu);
>>> -    CPUPPCState *env = &cpu->env;
>>> -    ICPStateClass *icpc;
>>> -
>>> -    assert(icp);
>>> -
>>> -    cpu->intc = OBJECT(icp);
>>> -    icp->cs = cs;
>>> -
>>> -    icpc = ICP_GET_CLASS(icp);
>>> -    if (icpc->cpu_setup) {
>>> -        icpc->cpu_setup(icp, cpu);
>>> -    }
>>> -
>>> -    switch (PPC_INPUT(env)) {
>>> -    case PPC_FLAGS_INPUT_POWER7:
>>> -        icp->output = env->irq_inputs[POWER7_INPUT_INT];
>>> -        break;
>>> -
>>> -    case PPC_FLAGS_INPUT_970:
>>> -        icp->output = env->irq_inputs[PPC970_INPUT_INT];
>>> -        break;
>>> -
>>> -    default:
>>> -        error_report("XICS interrupt controller does not support this CPU "
>>> -                     "bus model");
>>> -        abort();
>>> -    }
>>> -}
>>> -
>>>  void icp_pic_print_info(ICPState *icp, Monitor *mon)
>>>  {
>>>      int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
>>> @@ -343,6 +299,8 @@ static void icp_realize(DeviceState *dev, Error **errp)
>>>  {
>>>      ICPState *icp = ICP(dev);
>>>      ICPStateClass *icpc = ICP_GET_CLASS(dev);
>>> +    PowerPCCPU *cpu;
>>> +    CPUPPCState *env;
>>>      Object *obj;
>>>      Error *err = NULL;
>>>  
>>> @@ -355,6 +313,36 @@ static void icp_realize(DeviceState *dev, Error **errp)
>>>  
>>>      icp->xics = XICS_FABRIC(obj);
>>>  
>>> +    obj = object_property_get_link(OBJECT(dev), "cs", &err);
>>> +    if (!obj) {
>>> +        error_setg(errp, "%s: required link 'xics' not found: %s",
>>> +                   __func__, error_get_pretty(err));
>>> +        return;
>>> +    }
>>> +
>>> +    cpu = POWERPC_CPU(obj);
>>> +    cpu->intc = OBJECT(icp);
>>> +    icp->cs = CPU(obj);  
>>
>> only xics_kvm needs ->cs. 
>>
> 
> yeah, maybe the base class should only have a cpu_index field:
> 
>     icp->cpu_index = CPU(obj)->cpu_index;

arg, no. please, let's not add another cpu index :)

>>> +
>>> +    if (icpc->cpu_setup) {
>>> +        icpc->cpu_setup(icp, cpu);
>>> +    }
>>> +
>>> +    env = &cpu->env;
>>> +    switch (PPC_INPUT(env)) {
>>> +    case PPC_FLAGS_INPUT_POWER7:
>>> +        icp->output = env->irq_inputs[POWER7_INPUT_INT];
>>> +        break;
>>> +
>>> +    case PPC_FLAGS_INPUT_970:
>>> +        icp->output = env->irq_inputs[PPC970_INPUT_INT];
>>> +        break;
>>> +
>>> +    default:
>>> +        error_setg(errp, "XICS interrupt controller does not support this CPU bus model");
>>> +        return;
>>> +    }
>>> +
>>>      if (icpc->realize) {
>>>          icpc->realize(dev, errp);
>>>      }
>>> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
>>> index e8a9a94d5a24..1393005e76f3 100644
>>> --- a/hw/ppc/pnv_core.c
>>> +++ b/hw/ppc/pnv_core.c
>>> @@ -118,19 +118,19 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
>>>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>>>      Object *obj;
>>>  
>>> -    obj = object_new(TYPE_PNV_ICP);
>>> -    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
>>> -    object_unref(obj);
>>> -    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
>>> -    object_property_set_bool(obj, true, "realized", &local_err);
>>> +    object_property_set_bool(child, true, "realized", &local_err);
>>>      if (local_err) {
>>>          error_propagate(errp, local_err);
>>>          return;
>>>      }
>>>  
>>> -    object_property_set_bool(child, true, "realized", &local_err);
>>> +    obj = object_new(TYPE_PNV_ICP);
>>> +    object_property_add_child(child, "icp", obj, NULL);
>>> +    object_unref(obj);
>>> +    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
>>> +    object_property_add_const_link(obj, "cs", child, &error_abort);  
>>
>> may be link the cpu object instead ? 
>>
> 
> I'm not sure to understand... isn't child supposed to be a CPU index here ?

I meant linking the 'PowerPCCPU *cpu' object and not the CPUState.
That's minor.

>>> +    object_property_set_bool(obj, true, "realized", &local_err);
>>>      if (local_err) {
>>> -        object_unparent(obj);
>>>          error_propagate(errp, local_err);
>>>          return;
>>>      }
>>> @@ -141,8 +141,6 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
>>>          error_propagate(errp, local_err);
>>>          return;
>>>      }
>>> -
>>> -    xics_cpu_setup(xi, cpu, ICP(obj));
>>>  }
>>>  
>>>  static void pnv_core_realize(DeviceState *dev, Error **errp)
>>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>>> index 029a14120edd..9a6259525953 100644
>>> --- a/hw/ppc/spapr_cpu_core.c
>>> +++ b/hw/ppc/spapr_cpu_core.c
>>> @@ -53,9 +53,6 @@ static void spapr_cpu_reset(void *opaque)
>>>  
>>>  static void spapr_cpu_destroy(PowerPCCPU *cpu)
>>>  {
>>> -    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>>> -
>>> -    xics_cpu_destroy(XICS_FABRIC(spapr), cpu);
>>>      qemu_unregister_reset(spapr_cpu_reset, cpu);
>>>  }
>>>  
>>> @@ -140,28 +137,28 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
>>>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>>>      CPUState *cs = CPU(child);
>>>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>>> -    Object *obj;
>>> +    Object *obj = NULL;
>>>  
>>> -    obj = object_new(spapr->icp_type);
>>> -    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
>>> -    object_unref(obj);
>>> -    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
>>> -    object_property_set_bool(obj, true, "realized", &local_err);
>>> +    object_property_set_bool(child, true, "realized", &local_err);
>>>      if (local_err) {
>>>          goto error;
>>>      }
>>>  
>>> -    object_property_set_bool(child, true, "realized", &local_err);
>>> +    spapr_cpu_init(spapr, cpu, &local_err);
>>>      if (local_err) {
>>>          goto error;
>>>      }
>>>  
>>> -    spapr_cpu_init(spapr, cpu, &local_err);
>>> +    obj = object_new(spapr->icp_type);
>>> +    object_property_add_child(child, "icp", obj, &error_abort);
>>> +    object_unref(obj);
>>> +    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
>>> +    object_property_add_const_link(obj, "cs", child, &error_abort);  
>>
>> same here.
>>
>>> +    object_property_set_bool(obj, true, "realized", &local_err);
>>>      if (local_err) {
>>>          goto error;
>>>      }
>>>  
>>> -    xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
>>>      return;
>>>  
>>>  error:
>>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>>> index 40a506eacfb4..05767a15be9a 100644
>>> --- a/include/hw/ppc/xics.h
>>> +++ b/include/hw/ppc/xics.h
>>> @@ -183,8 +183,6 @@ void spapr_dt_xics(int nr_servers, void *fdt, uint32_t phandle);
>>>  
>>>  qemu_irq xics_get_qirq(XICSFabric *xi, int irq);
>>>  ICPState *xics_icp_get(XICSFabric *xi, int server);
>>> -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp);
>>> -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu);
>>>  
>>>  /* Internal XICS interfaces */
>>>  void icp_set_cppr(ICPState *icp, uint8_t cppr);
>>>   
>>
> 

Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time

[Greg Kurz]

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

On Thu, 8 Jun 2017 07:50:08 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 06/07/2017 10:55 PM, Greg Kurz wrote:
> [...]
> >>> +    obj = object_new(TYPE_PNV_ICP);
> >>> +    object_property_add_child(child, "icp", obj, NULL);
> >>> +    object_unref(obj);
> >>> +    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
> >>> +    object_property_add_const_link(obj, "cs", child, &error_abort);    
> >>
> >> may be link the cpu object instead ? 
> >>  
> > 
> > I'm not sure to understand... isn't child supposed to be a CPU index here ?  
> 
> I meant linking the 'PowerPCCPU *cpu' object and not the CPUState.
> That's minor.
> 

Well, cpu == cs == child and object_property_add_const_link() expects
an Object *. Passing child avoids a not-really-useful OBJECT() dynamic
cast.

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

Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time

[David Gibson]

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

On Wed, Jun 07, 2017 at 07:17:09PM +0200, Greg Kurz wrote:
> Until recently, spapr used to allocate ICPState objects for the lifetime
> of the machine. They would only be associated to vCPUs in xics_cpu_setup()
> when plugging a CPU core.
> 
> Now that ICPState objects have the same lifecycle as vCPUs, it is
> possible to associate them during realization.
> 
> This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
> is passed as a property. Note that vCPU now needs to be realized first
> for the IRQs to be allocated. It also needs to resetted before ICPState
> realization in order to synchronize with KVM.

Ok, what enforces those ordering constraints?

> Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't
> needed anymore and can be safely dropped.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>


Looks pretty good, though a couple nits below.

> ---
>  hw/intc/xics.c          |   76 ++++++++++++++++++++---------------------------
>  hw/ppc/pnv_core.c       |   16 ++++------
>  hw/ppc/spapr_cpu_core.c |   21 ++++++-------
>  include/hw/ppc/xics.h   |    2 -
>  4 files changed, 48 insertions(+), 67 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index ec73f02144c9..330441ff7fe8 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -38,50 +38,6 @@
>  #include "monitor/monitor.h"
>  #include "hw/intc/intc.h"
>  
> -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
> -{
> -    CPUState *cs = CPU(cpu);
> -    ICPState *icp = ICP(cpu->intc);
> -
> -    assert(icp);
> -    assert(cs == icp->cs);
> -
> -    icp->output = NULL;
> -    icp->cs = NULL;
> -}
> -
> -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp)
> -{
> -    CPUState *cs = CPU(cpu);
> -    CPUPPCState *env = &cpu->env;
> -    ICPStateClass *icpc;
> -
> -    assert(icp);
> -
> -    cpu->intc = OBJECT(icp);
> -    icp->cs = cs;
> -
> -    icpc = ICP_GET_CLASS(icp);
> -    if (icpc->cpu_setup) {
> -        icpc->cpu_setup(icp, cpu);
> -    }
> -
> -    switch (PPC_INPUT(env)) {
> -    case PPC_FLAGS_INPUT_POWER7:
> -        icp->output = env->irq_inputs[POWER7_INPUT_INT];
> -        break;
> -
> -    case PPC_FLAGS_INPUT_970:
> -        icp->output = env->irq_inputs[PPC970_INPUT_INT];
> -        break;
> -
> -    default:
> -        error_report("XICS interrupt controller does not support this CPU "
> -                     "bus model");
> -        abort();
> -    }
> -}
> -
>  void icp_pic_print_info(ICPState *icp, Monitor *mon)
>  {
>      int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
> @@ -343,6 +299,8 @@ static void icp_realize(DeviceState *dev, Error **errp)
>  {
>      ICPState *icp = ICP(dev);
>      ICPStateClass *icpc = ICP_GET_CLASS(dev);
> +    PowerPCCPU *cpu;
> +    CPUPPCState *env;
>      Object *obj;
>      Error *err = NULL;
>  
> @@ -355,6 +313,36 @@ static void icp_realize(DeviceState *dev, Error **errp)
>  
>      icp->xics = XICS_FABRIC(obj);
>  
> +    obj = object_property_get_link(OBJECT(dev), "cs", &err);

I don't like the name "cs" for an externally visible property.  "cpu"
would be better.

> +    if (!obj) {
> +        error_setg(errp, "%s: required link 'xics' not found: %s",

Copy/paste mistake in this message.

> +                   __func__, error_get_pretty(err));
> +        return;
> +    }
> +
> +    cpu = POWERPC_CPU(obj);
> +    cpu->intc = OBJECT(icp);
> +    icp->cs = CPU(obj);
> +
> +    if (icpc->cpu_setup) {
> +        icpc->cpu_setup(icp, cpu);
> +    }
> +
> +    env = &cpu->env;
> +    switch (PPC_INPUT(env)) {
> +    case PPC_FLAGS_INPUT_POWER7:
> +        icp->output = env->irq_inputs[POWER7_INPUT_INT];
> +        break;
> +
> +    case PPC_FLAGS_INPUT_970:
> +        icp->output = env->irq_inputs[PPC970_INPUT_INT];
> +        break;
> +
> +    default:
> +        error_setg(errp, "XICS interrupt controller does not support this CPU bus model");
> +        return;
> +    }
> +
>      if (icpc->realize) {
>          icpc->realize(dev, errp);
>      }
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index e8a9a94d5a24..1393005e76f3 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -118,19 +118,19 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      Object *obj;
>  
> -    obj = object_new(TYPE_PNV_ICP);
> -    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> -    object_unref(obj);
> -    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
> -    object_property_set_bool(obj, true, "realized", &local_err);
> +    object_property_set_bool(child, true, "realized", &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>  
> -    object_property_set_bool(child, true, "realized", &local_err);
> +    obj = object_new(TYPE_PNV_ICP);
> +    object_property_add_child(child, "icp", obj, NULL);
> +    object_unref(obj);
> +    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
> +    object_property_add_const_link(obj, "cs", child, &error_abort);
> +    object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
> -        object_unparent(obj);
>          error_propagate(errp, local_err);
>          return;
>      }
> @@ -141,8 +141,6 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
>          error_propagate(errp, local_err);
>          return;
>      }
> -
> -    xics_cpu_setup(xi, cpu, ICP(obj));
>  }
>  
>  static void pnv_core_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 029a14120edd..9a6259525953 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -53,9 +53,6 @@ static void spapr_cpu_reset(void *opaque)
>  
>  static void spapr_cpu_destroy(PowerPCCPU *cpu)
>  {
> -    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> -
> -    xics_cpu_destroy(XICS_FABRIC(spapr), cpu);
>      qemu_unregister_reset(spapr_cpu_reset, cpu);
>  }
>  
> @@ -140,28 +137,28 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      CPUState *cs = CPU(child);
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> -    Object *obj;
> +    Object *obj = NULL;
>  
> -    obj = object_new(spapr->icp_type);
> -    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> -    object_unref(obj);
> -    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> -    object_property_set_bool(obj, true, "realized", &local_err);
> +    object_property_set_bool(child, true, "realized", &local_err);
>      if (local_err) {
>          goto error;
>      }
>  
> -    object_property_set_bool(child, true, "realized", &local_err);
> +    spapr_cpu_init(spapr, cpu, &local_err);
>      if (local_err) {
>          goto error;
>      }
>  
> -    spapr_cpu_init(spapr, cpu, &local_err);
> +    obj = object_new(spapr->icp_type);
> +    object_property_add_child(child, "icp", obj, &error_abort);
> +    object_unref(obj);
> +    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> +    object_property_add_const_link(obj, "cs", child, &error_abort);
> +    object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
>          goto error;
>      }
>  
> -    xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
>      return;
>  
>  error:
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 40a506eacfb4..05767a15be9a 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -183,8 +183,6 @@ void spapr_dt_xics(int nr_servers, void *fdt, uint32_t phandle);
>  
>  qemu_irq xics_get_qirq(XICSFabric *xi, int irq);
>  ICPState *xics_icp_get(XICSFabric *xi, int server);
> -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp);
> -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu);
>  
>  /* Internal XICS interfaces */
>  void icp_set_cppr(ICPState *icp, uint8_t cppr);
> 

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

Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time

[Greg Kurz]

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

On Thu, 8 Jun 2017 12:01:12 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jun 07, 2017 at 07:17:09PM +0200, Greg Kurz wrote:
> > Until recently, spapr used to allocate ICPState objects for the lifetime
> > of the machine. They would only be associated to vCPUs in xics_cpu_setup()
> > when plugging a CPU core.
> > 
> > Now that ICPState objects have the same lifecycle as vCPUs, it is
> > possible to associate them during realization.
> > 
> > This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
> > is passed as a property. Note that vCPU now needs to be realized first
> > for the IRQs to be allocated. It also needs to resetted before ICPState
> > realization in order to synchronize with KVM.  
> 
> Ok, what enforces those ordering constraints?
> 

I'm not sure about what you're asking... I had to re-order because
xics_cpu_setup() used to be called after the vCPU is realized and
put in PAPR mode.

Especially, we have these lines:

    switch (PPC_INPUT(env)) {
    case PPC_FLAGS_INPUT_POWER7:
        icp->output = env->irq_inputs[POWER7_INPUT_INT];
        break;

    case PPC_FLAGS_INPUT_970:
        icp->output = env->irq_inputs[PPC970_INPUT_INT];
        break;

They depend on env->irq_inputs being allocated. This happens on the

spapr_cpu_core_realize_child()
{
    ...
    object_property_set_bool(child, true, "realized", &local_err);
     object_property_set_bool()
      object_property_set_qobject()
       object_property_set()
        property_set_bool()
         device_set_realized()
          ppc_cpu_realizefn()
           init_ppc_proc()
            init_proc_POWER8()
             ppcPOWER7_irq_init()

and, when using xics_kvm:

icp_kvm_cpu_setup()
{
    ...
    ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0, kernel_xics_fd, vcpu_id);

This ioctl returns EINVAL because it fails the kvmppc_sanity_check() in KVM.

It depends on QEMU enabling KVM_CAP_PPC_PAPR for the vCPU, which happens in:

spapr_cpu_core_realize_child()
{
    ...
    spapr_cpu_init(spapr, cpu, &local_err);
     cpu_ppc_set_papr()
      kvmppc_set_papr()

> > Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't
> > needed anymore and can be safely dropped.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> 
> Looks pretty good, though a couple nits below.
> 

Thanks. I'm also thinking about going one step further and converting
xics_kvm to use the icpc->realize() handler instead of icpc->cpu_setup().
Makes sense ?

> > ---
> >  hw/intc/xics.c          |   76 ++++++++++++++++++++---------------------------
> >  hw/ppc/pnv_core.c       |   16 ++++------
> >  hw/ppc/spapr_cpu_core.c |   21 ++++++-------
> >  include/hw/ppc/xics.h   |    2 -
> >  4 files changed, 48 insertions(+), 67 deletions(-)
> > 
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index ec73f02144c9..330441ff7fe8 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -38,50 +38,6 @@
> >  #include "monitor/monitor.h"
> >  #include "hw/intc/intc.h"
> >  
> > -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
> > -{
> > -    CPUState *cs = CPU(cpu);
> > -    ICPState *icp = ICP(cpu->intc);
> > -
> > -    assert(icp);
> > -    assert(cs == icp->cs);
> > -
> > -    icp->output = NULL;
> > -    icp->cs = NULL;
> > -}
> > -
> > -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp)
> > -{
> > -    CPUState *cs = CPU(cpu);
> > -    CPUPPCState *env = &cpu->env;
> > -    ICPStateClass *icpc;
> > -
> > -    assert(icp);
> > -
> > -    cpu->intc = OBJECT(icp);
> > -    icp->cs = cs;
> > -
> > -    icpc = ICP_GET_CLASS(icp);
> > -    if (icpc->cpu_setup) {
> > -        icpc->cpu_setup(icp, cpu);
> > -    }
> > -
> > -    switch (PPC_INPUT(env)) {
> > -    case PPC_FLAGS_INPUT_POWER7:
> > -        icp->output = env->irq_inputs[POWER7_INPUT_INT];
> > -        break;
> > -
> > -    case PPC_FLAGS_INPUT_970:
> > -        icp->output = env->irq_inputs[PPC970_INPUT_INT];
> > -        break;
> > -
> > -    default:
> > -        error_report("XICS interrupt controller does not support this CPU "
> > -                     "bus model");
> > -        abort();
> > -    }
> > -}
> > -
> >  void icp_pic_print_info(ICPState *icp, Monitor *mon)
> >  {
> >      int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
> > @@ -343,6 +299,8 @@ static void icp_realize(DeviceState *dev, Error **errp)
> >  {
> >      ICPState *icp = ICP(dev);
> >      ICPStateClass *icpc = ICP_GET_CLASS(dev);
> > +    PowerPCCPU *cpu;
> > +    CPUPPCState *env;
> >      Object *obj;
> >      Error *err = NULL;
> >  
> > @@ -355,6 +313,36 @@ static void icp_realize(DeviceState *dev, Error **errp)
> >  
> >      icp->xics = XICS_FABRIC(obj);
> >  
> > +    obj = object_property_get_link(OBJECT(dev), "cs", &err);  
> 
> I don't like the name "cs" for an externally visible property.  "cpu"
> would be better.
> 

Right. I'll fix this.

> > +    if (!obj) {
> > +        error_setg(errp, "%s: required link 'xics' not found: %s",  
> 
> Copy/paste mistake in this message.
> 

Oops :)

> > +                   __func__, error_get_pretty(err));
> > +        return;
> > +    }
> > +
> > +    cpu = POWERPC_CPU(obj);
> > +    cpu->intc = OBJECT(icp);
> > +    icp->cs = CPU(obj);
> > +
> > +    if (icpc->cpu_setup) {
> > +        icpc->cpu_setup(icp, cpu);
> > +    }
> > +
> > +    env = &cpu->env;
> > +    switch (PPC_INPUT(env)) {
> > +    case PPC_FLAGS_INPUT_POWER7:
> > +        icp->output = env->irq_inputs[POWER7_INPUT_INT];
> > +        break;
> > +
> > +    case PPC_FLAGS_INPUT_970:
> > +        icp->output = env->irq_inputs[PPC970_INPUT_INT];
> > +        break;
> > +
> > +    default:
> > +        error_setg(errp, "XICS interrupt controller does not support this CPU bus model");
> > +        return;
> > +    }
> > +
> >      if (icpc->realize) {
> >          icpc->realize(dev, errp);
> >      }
> > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> > index e8a9a94d5a24..1393005e76f3 100644
> > --- a/hw/ppc/pnv_core.c
> > +++ b/hw/ppc/pnv_core.c
> > @@ -118,19 +118,19 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
> >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> >      Object *obj;
> >  
> > -    obj = object_new(TYPE_PNV_ICP);
> > -    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> > -    object_unref(obj);
> > -    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
> > -    object_property_set_bool(obj, true, "realized", &local_err);
> > +    object_property_set_bool(child, true, "realized", &local_err);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> >          return;
> >      }
> >  
> > -    object_property_set_bool(child, true, "realized", &local_err);
> > +    obj = object_new(TYPE_PNV_ICP);
> > +    object_property_add_child(child, "icp", obj, NULL);
> > +    object_unref(obj);
> > +    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
> > +    object_property_add_const_link(obj, "cs", child, &error_abort);
> > +    object_property_set_bool(obj, true, "realized", &local_err);
> >      if (local_err) {
> > -        object_unparent(obj);
> >          error_propagate(errp, local_err);
> >          return;
> >      }
> > @@ -141,8 +141,6 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
> >          error_propagate(errp, local_err);
> >          return;
> >      }
> > -
> > -    xics_cpu_setup(xi, cpu, ICP(obj));
> >  }
> >  
> >  static void pnv_core_realize(DeviceState *dev, Error **errp)
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 029a14120edd..9a6259525953 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -53,9 +53,6 @@ static void spapr_cpu_reset(void *opaque)
> >  
> >  static void spapr_cpu_destroy(PowerPCCPU *cpu)
> >  {
> > -    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > -
> > -    xics_cpu_destroy(XICS_FABRIC(spapr), cpu);
> >      qemu_unregister_reset(spapr_cpu_reset, cpu);
> >  }
> >  
> > @@ -140,28 +137,28 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >      CPUState *cs = CPU(child);
> >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> > -    Object *obj;
> > +    Object *obj = NULL;
> >  
> > -    obj = object_new(spapr->icp_type);
> > -    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> > -    object_unref(obj);
> > -    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> > -    object_property_set_bool(obj, true, "realized", &local_err);
> > +    object_property_set_bool(child, true, "realized", &local_err);
> >      if (local_err) {
> >          goto error;
> >      }
> >  
> > -    object_property_set_bool(child, true, "realized", &local_err);
> > +    spapr_cpu_init(spapr, cpu, &local_err);
> >      if (local_err) {
> >          goto error;
> >      }
> >  
> > -    spapr_cpu_init(spapr, cpu, &local_err);
> > +    obj = object_new(spapr->icp_type);
> > +    object_property_add_child(child, "icp", obj, &error_abort);
> > +    object_unref(obj);
> > +    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> > +    object_property_add_const_link(obj, "cs", child, &error_abort);
> > +    object_property_set_bool(obj, true, "realized", &local_err);
> >      if (local_err) {
> >          goto error;
> >      }
> >  
> > -    xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
> >      return;
> >  
> >  error:
> > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> > index 40a506eacfb4..05767a15be9a 100644
> > --- a/include/hw/ppc/xics.h
> > +++ b/include/hw/ppc/xics.h
> > @@ -183,8 +183,6 @@ void spapr_dt_xics(int nr_servers, void *fdt, uint32_t phandle);
> >  
> >  qemu_irq xics_get_qirq(XICSFabric *xi, int irq);
> >  ICPState *xics_icp_get(XICSFabric *xi, int server);
> > -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp);
> > -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu);
> >  
> >  /* Internal XICS interfaces */
> >  void icp_set_cppr(ICPState *icp, uint8_t cppr);
> >   
> 


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

Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time

[Cédric Le Goater]

On 06/08/2017 10:45 AM, Greg Kurz wrote:
> On Thu, 8 Jun 2017 12:01:12 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
>> On Wed, Jun 07, 2017 at 07:17:09PM +0200, Greg Kurz wrote:
>>> Until recently, spapr used to allocate ICPState objects for the lifetime
>>> of the machine. They would only be associated to vCPUs in xics_cpu_setup()
>>> when plugging a CPU core.
>>>
>>> Now that ICPState objects have the same lifecycle as vCPUs, it is
>>> possible to associate them during realization.
>>>
>>> This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
>>> is passed as a property. Note that vCPU now needs to be realized first
>>> for the IRQs to be allocated. It also needs to resetted before ICPState
>>> realization in order to synchronize with KVM.  
>>
>> Ok, what enforces those ordering constraints?
>>
> 
> I'm not sure about what you're asking... I had to re-order because
> xics_cpu_setup() used to be called after the vCPU is realized and
> put in PAPR mode.
> 
> Especially, we have these lines:
> 
>     switch (PPC_INPUT(env)) {
>     case PPC_FLAGS_INPUT_POWER7:
>         icp->output = env->irq_inputs[POWER7_INPUT_INT];
>         break;
> 
>     case PPC_FLAGS_INPUT_970:
>         icp->output = env->irq_inputs[PPC970_INPUT_INT];
>         break;
> 
> They depend on env->irq_inputs being allocated. This happens on the
> 
> spapr_cpu_core_realize_child()
> {
>     ...
>     object_property_set_bool(child, true, "realized", &local_err);
>      object_property_set_bool()
>       object_property_set_qobject()
>        object_property_set()
>         property_set_bool()
>          device_set_realized()
>           ppc_cpu_realizefn()
>            init_ppc_proc()
>             init_proc_POWER8()
>              ppcPOWER7_irq_init()
> 
> and, when using xics_kvm:
> 
> icp_kvm_cpu_setup()
> {
>     ...
>     ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0, kernel_xics_fd, vcpu_id);
> 
> This ioctl returns EINVAL because it fails the kvmppc_sanity_check() in KVM.
> 
> It depends on QEMU enabling KVM_CAP_PPC_PAPR for the vCPU, which happens in:
> 
> spapr_cpu_core_realize_child()
> {
>     ...
>     spapr_cpu_init(spapr, cpu, &local_err);
>      cpu_ppc_set_papr()
>       kvmppc_set_papr()
> 
>>> Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't
>>> needed anymore and can be safely dropped.
>>>
>>> Signed-off-by: Greg Kurz <groug@kaod.org>  
>>
>>
>> Looks pretty good, though a couple nits below.
>>
> 
> Thanks. I'm also thinking about going one step further and converting
> xics_kvm to use the icpc->realize() handler instead of icpc->cpu_setup().
> Makes sense ?

It should be fine I think, now that a reset handler is defined.

C.


>>> ---
>>>  hw/intc/xics.c          |   76 ++++++++++++++++++++---------------------------
>>>  hw/ppc/pnv_core.c       |   16 ++++------
>>>  hw/ppc/spapr_cpu_core.c |   21 ++++++-------
>>>  include/hw/ppc/xics.h   |    2 -
>>>  4 files changed, 48 insertions(+), 67 deletions(-)
>>>
>>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>>> index ec73f02144c9..330441ff7fe8 100644
>>> --- a/hw/intc/xics.c
>>> +++ b/hw/intc/xics.c
>>> @@ -38,50 +38,6 @@
>>>  #include "monitor/monitor.h"
>>>  #include "hw/intc/intc.h"
>>>  
>>> -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
>>> -{
>>> -    CPUState *cs = CPU(cpu);
>>> -    ICPState *icp = ICP(cpu->intc);
>>> -
>>> -    assert(icp);
>>> -    assert(cs == icp->cs);
>>> -
>>> -    icp->output = NULL;
>>> -    icp->cs = NULL;
>>> -}
>>> -
>>> -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp)
>>> -{
>>> -    CPUState *cs = CPU(cpu);
>>> -    CPUPPCState *env = &cpu->env;
>>> -    ICPStateClass *icpc;
>>> -
>>> -    assert(icp);
>>> -
>>> -    cpu->intc = OBJECT(icp);
>>> -    icp->cs = cs;
>>> -
>>> -    icpc = ICP_GET_CLASS(icp);
>>> -    if (icpc->cpu_setup) {
>>> -        icpc->cpu_setup(icp, cpu);
>>> -    }
>>> -
>>> -    switch (PPC_INPUT(env)) {
>>> -    case PPC_FLAGS_INPUT_POWER7:
>>> -        icp->output = env->irq_inputs[POWER7_INPUT_INT];
>>> -        break;
>>> -
>>> -    case PPC_FLAGS_INPUT_970:
>>> -        icp->output = env->irq_inputs[PPC970_INPUT_INT];
>>> -        break;
>>> -
>>> -    default:
>>> -        error_report("XICS interrupt controller does not support this CPU "
>>> -                     "bus model");
>>> -        abort();
>>> -    }
>>> -}
>>> -
>>>  void icp_pic_print_info(ICPState *icp, Monitor *mon)
>>>  {
>>>      int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
>>> @@ -343,6 +299,8 @@ static void icp_realize(DeviceState *dev, Error **errp)
>>>  {
>>>      ICPState *icp = ICP(dev);
>>>      ICPStateClass *icpc = ICP_GET_CLASS(dev);
>>> +    PowerPCCPU *cpu;
>>> +    CPUPPCState *env;
>>>      Object *obj;
>>>      Error *err = NULL;
>>>  
>>> @@ -355,6 +313,36 @@ static void icp_realize(DeviceState *dev, Error **errp)
>>>  
>>>      icp->xics = XICS_FABRIC(obj);
>>>  
>>> +    obj = object_property_get_link(OBJECT(dev), "cs", &err);  
>>
>> I don't like the name "cs" for an externally visible property.  "cpu"
>> would be better.
>>
> 
> Right. I'll fix this.
> 
>>> +    if (!obj) {
>>> +        error_setg(errp, "%s: required link 'xics' not found: %s",  
>>
>> Copy/paste mistake in this message.
>>
> 
> Oops :)
> 
>>> +                   __func__, error_get_pretty(err));
>>> +        return;
>>> +    }
>>> +
>>> +    cpu = POWERPC_CPU(obj);
>>> +    cpu->intc = OBJECT(icp);
>>> +    icp->cs = CPU(obj);
>>> +
>>> +    if (icpc->cpu_setup) {
>>> +        icpc->cpu_setup(icp, cpu);
>>> +    }
>>> +
>>> +    env = &cpu->env;
>>> +    switch (PPC_INPUT(env)) {
>>> +    case PPC_FLAGS_INPUT_POWER7:
>>> +        icp->output = env->irq_inputs[POWER7_INPUT_INT];
>>> +        break;
>>> +
>>> +    case PPC_FLAGS_INPUT_970:
>>> +        icp->output = env->irq_inputs[PPC970_INPUT_INT];
>>> +        break;
>>> +
>>> +    default:
>>> +        error_setg(errp, "XICS interrupt controller does not support this CPU bus model");
>>> +        return;
>>> +    }
>>> +
>>>      if (icpc->realize) {
>>>          icpc->realize(dev, errp);
>>>      }
>>> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
>>> index e8a9a94d5a24..1393005e76f3 100644
>>> --- a/hw/ppc/pnv_core.c
>>> +++ b/hw/ppc/pnv_core.c
>>> @@ -118,19 +118,19 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
>>>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>>>      Object *obj;
>>>  
>>> -    obj = object_new(TYPE_PNV_ICP);
>>> -    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
>>> -    object_unref(obj);
>>> -    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
>>> -    object_property_set_bool(obj, true, "realized", &local_err);
>>> +    object_property_set_bool(child, true, "realized", &local_err);
>>>      if (local_err) {
>>>          error_propagate(errp, local_err);
>>>          return;
>>>      }
>>>  
>>> -    object_property_set_bool(child, true, "realized", &local_err);
>>> +    obj = object_new(TYPE_PNV_ICP);
>>> +    object_property_add_child(child, "icp", obj, NULL);
>>> +    object_unref(obj);
>>> +    object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
>>> +    object_property_add_const_link(obj, "cs", child, &error_abort);
>>> +    object_property_set_bool(obj, true, "realized", &local_err);
>>>      if (local_err) {
>>> -        object_unparent(obj);
>>>          error_propagate(errp, local_err);
>>>          return;
>>>      }
>>> @@ -141,8 +141,6 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
>>>          error_propagate(errp, local_err);
>>>          return;
>>>      }
>>> -
>>> -    xics_cpu_setup(xi, cpu, ICP(obj));
>>>  }
>>>  
>>>  static void pnv_core_realize(DeviceState *dev, Error **errp)
>>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>>> index 029a14120edd..9a6259525953 100644
>>> --- a/hw/ppc/spapr_cpu_core.c
>>> +++ b/hw/ppc/spapr_cpu_core.c
>>> @@ -53,9 +53,6 @@ static void spapr_cpu_reset(void *opaque)
>>>  
>>>  static void spapr_cpu_destroy(PowerPCCPU *cpu)
>>>  {
>>> -    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>>> -
>>> -    xics_cpu_destroy(XICS_FABRIC(spapr), cpu);
>>>      qemu_unregister_reset(spapr_cpu_reset, cpu);
>>>  }
>>>  
>>> @@ -140,28 +137,28 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
>>>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>>>      CPUState *cs = CPU(child);
>>>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>>> -    Object *obj;
>>> +    Object *obj = NULL;
>>>  
>>> -    obj = object_new(spapr->icp_type);
>>> -    object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
>>> -    object_unref(obj);
>>> -    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
>>> -    object_property_set_bool(obj, true, "realized", &local_err);
>>> +    object_property_set_bool(child, true, "realized", &local_err);
>>>      if (local_err) {
>>>          goto error;
>>>      }
>>>  
>>> -    object_property_set_bool(child, true, "realized", &local_err);
>>> +    spapr_cpu_init(spapr, cpu, &local_err);
>>>      if (local_err) {
>>>          goto error;
>>>      }
>>>  
>>> -    spapr_cpu_init(spapr, cpu, &local_err);
>>> +    obj = object_new(spapr->icp_type);
>>> +    object_property_add_child(child, "icp", obj, &error_abort);
>>> +    object_unref(obj);
>>> +    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
>>> +    object_property_add_const_link(obj, "cs", child, &error_abort);
>>> +    object_property_set_bool(obj, true, "realized", &local_err);
>>>      if (local_err) {
>>>          goto error;
>>>      }
>>>  
>>> -    xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
>>>      return;
>>>  
>>>  error:
>>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>>> index 40a506eacfb4..05767a15be9a 100644
>>> --- a/include/hw/ppc/xics.h
>>> +++ b/include/hw/ppc/xics.h
>>> @@ -183,8 +183,6 @@ void spapr_dt_xics(int nr_servers, void *fdt, uint32_t phandle);
>>>  
>>>  qemu_irq xics_get_qirq(XICSFabric *xi, int irq);
>>>  ICPState *xics_icp_get(XICSFabric *xi, int server);
>>> -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp);
>>> -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu);
>>>  
>>>  /* Internal XICS interfaces */
>>>  void icp_set_cppr(ICPState *icp, uint8_t cppr);
>>>   
>>
> 

Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time

[David Gibson]

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

On Thu, Jun 08, 2017 at 10:45:30AM +0200, Greg Kurz wrote:
> On Thu, 8 Jun 2017 12:01:12 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Jun 07, 2017 at 07:17:09PM +0200, Greg Kurz wrote:
> > > Until recently, spapr used to allocate ICPState objects for the lifetime
> > > of the machine. They would only be associated to vCPUs in xics_cpu_setup()
> > > when plugging a CPU core.
> > > 
> > > Now that ICPState objects have the same lifecycle as vCPUs, it is
> > > possible to associate them during realization.
> > > 
> > > This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
> > > is passed as a property. Note that vCPU now needs to be realized first
> > > for the IRQs to be allocated. It also needs to resetted before ICPState
> > > realization in order to synchronize with KVM.  
> > 
> > Ok, what enforces those ordering constraints?
> > 
> 
> I'm not sure about what you're asking... I had to re-order because
> xics_cpu_setup() used to be called after the vCPU is realized and
> put in PAPR mode.

Duh, sorry, I wasn't thinking to ask about realize order, since that's
manual and you've re-ordered it to be correct.

You also mention that reset order matters, and I'm less clear on what
guarantees that the reset handlers for the components get called in
the right order.

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

Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time

[Greg Kurz]

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

On Fri, 9 Jun 2017 12:24:52 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jun 08, 2017 at 10:45:30AM +0200, Greg Kurz wrote:
> > On Thu, 8 Jun 2017 12:01:12 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Wed, Jun 07, 2017 at 07:17:09PM +0200, Greg Kurz wrote:  
> > > > Until recently, spapr used to allocate ICPState objects for the lifetime
> > > > of the machine. They would only be associated to vCPUs in xics_cpu_setup()
> > > > when plugging a CPU core.
> > > > 
> > > > Now that ICPState objects have the same lifecycle as vCPUs, it is
> > > > possible to associate them during realization.
> > > > 
> > > > This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
> > > > is passed as a property. Note that vCPU now needs to be realized first
> > > > for the IRQs to be allocated. It also needs to resetted before ICPState
> > > > realization in order to synchronize with KVM.    
> > > 
> > > Ok, what enforces those ordering constraints?
> > >   
> > 
> > I'm not sure about what you're asking... I had to re-order because
> > xics_cpu_setup() used to be called after the vCPU is realized and
> > put in PAPR mode.  
> 
> Duh, sorry, I wasn't thinking to ask about realize order, since that's
> manual and you've re-ordered it to be correct.
> 
> You also mention that reset order matters, and I'm less clear on what
> guarantees that the reset handlers for the components get called in
> the right order.
> 

Oops... my bad, this is a mistake in the changelog. The KVM error I was
seing isn't related to CPU reset as I was thinking first but to
cpu_ppc_set_papr()... :-\

The last sentence should rather be something like:

"We also need to call spapr_cpu_init() before ICPState realization in
 order to enable PAPR mode in KVM."

Can you fix this in your tree or should I send an updated version ?

Cheers,

--
Greg

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

Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time

[David Gibson]

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

On Fri, Jun 09, 2017 at 08:45:50AM +0200, Greg Kurz wrote:
> On Fri, 9 Jun 2017 12:24:52 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Jun 08, 2017 at 10:45:30AM +0200, Greg Kurz wrote:
> > > On Thu, 8 Jun 2017 12:01:12 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Wed, Jun 07, 2017 at 07:17:09PM +0200, Greg Kurz wrote:  
> > > > > Until recently, spapr used to allocate ICPState objects for the lifetime
> > > > > of the machine. They would only be associated to vCPUs in xics_cpu_setup()
> > > > > when plugging a CPU core.
> > > > > 
> > > > > Now that ICPState objects have the same lifecycle as vCPUs, it is
> > > > > possible to associate them during realization.
> > > > > 
> > > > > This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
> > > > > is passed as a property. Note that vCPU now needs to be realized first
> > > > > for the IRQs to be allocated. It also needs to resetted before ICPState
> > > > > realization in order to synchronize with KVM.    
> > > > 
> > > > Ok, what enforces those ordering constraints?
> > > >   
> > > 
> > > I'm not sure about what you're asking... I had to re-order because
> > > xics_cpu_setup() used to be called after the vCPU is realized and
> > > put in PAPR mode.  
> > 
> > Duh, sorry, I wasn't thinking to ask about realize order, since that's
> > manual and you've re-ordered it to be correct.
> > 
> > You also mention that reset order matters, and I'm less clear on what
> > guarantees that the reset handlers for the components get called in
> > the right order.
> > 
> 
> Oops... my bad, this is a mistake in the changelog. The KVM error I was
> seing isn't related to CPU reset as I was thinking first but to
> cpu_ppc_set_papr()... :-\
> 
> The last sentence should rather be something like:
> 
> "We also need to call spapr_cpu_init() before ICPState realization in
>  order to enable PAPR mode in KVM."
> 
> Can you fix this in your tree or should I send an updated version ?

Uh.. too late, I already sent a pull request.

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

[Qemu-devel] [PATCH v3 4/5] xics: directly register ICPState objects to vmstate

[Greg Kurz]

The ICPState objects are currently registered to vmstate as qdev objects.
Their instance ids are hence computed automatically in the migration code,
and thus depends on the order the CPU cores were plugged.

If the destination had its CPU cores plugged in a different order than the
source, then ICPState objects will have different instance_ids and load
the wrong state.

Since CPU objects have a reliable cpu_index which is already used as
instance_id in vmstate, let's use it for ICPState as well.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/xics.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 330441ff7fe8..3d76b43876c5 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -348,10 +348,14 @@ static void icp_realize(DeviceState *dev, Error **errp)
     }
 
     qemu_register_reset(icp_reset, dev);
+    vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
 }
 
 static void icp_unrealize(DeviceState *dev, Error **errp)
 {
+    ICPState *icp = ICP(dev);
+
+    vmstate_unregister(NULL, &vmstate_icp_server, icp);
     qemu_unregister_reset(icp_reset, dev);
 }
 
@@ -359,7 +363,6 @@ static void icp_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    dc->vmsd = &vmstate_icp_server;
     dc->realize = icp_realize;
     dc->unrealize = icp_unrealize;
 }

Re: [Qemu-devel] [PATCH v3 4/5] xics: directly register ICPState objects to vmstate

[Cédric Le Goater]

On 06/07/2017 07:17 PM, Greg Kurz wrote:
> The ICPState objects are currently registered to vmstate as qdev objects.
> Their instance ids are hence computed automatically in the migration code,
> and thus depends on the order the CPU cores were plugged.
> 
> If the destination had its CPU cores plugged in a different order than the
> source, then ICPState objects will have different instance_ids and load
> the wrong state.
> 
> Since CPU objects have a reliable cpu_index which is already used as
> instance_id in vmstate, let's use it for ICPState as well.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/intc/xics.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 330441ff7fe8..3d76b43876c5 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -348,10 +348,14 @@ static void icp_realize(DeviceState *dev, Error **errp)
>      }
>  
>      qemu_register_reset(icp_reset, dev);
> +    vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);

so, what I just said in the previous email regarding ->cs would break this 
patch. Forget about it. 


Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

>  }
>  
>  static void icp_unrealize(DeviceState *dev, Error **errp)
>  {
> +    ICPState *icp = ICP(dev);
> +
> +    vmstate_unregister(NULL, &vmstate_icp_server, icp);
>      qemu_unregister_reset(icp_reset, dev);
>  }
>  
> @@ -359,7 +363,6 @@ static void icp_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -    dc->vmsd = &vmstate_icp_server;
>      dc->realize = icp_realize;
>      dc->unrealize = icp_unrealize;
>  }
> 

Re: [Qemu-devel] [PATCH v3 4/5] xics: directly register ICPState objects to vmstate

[Greg Kurz]

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

On Wed, 7 Jun 2017 20:14:01 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 06/07/2017 07:17 PM, Greg Kurz wrote:
> > The ICPState objects are currently registered to vmstate as qdev objects.
> > Their instance ids are hence computed automatically in the migration code,
> > and thus depends on the order the CPU cores were plugged.
> > 
> > If the destination had its CPU cores plugged in a different order than the
> > source, then ICPState objects will have different instance_ids and load
> > the wrong state.
> > 
> > Since CPU objects have a reliable cpu_index which is already used as
> > instance_id in vmstate, let's use it for ICPState as well.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/intc/xics.c |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index 330441ff7fe8..3d76b43876c5 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -348,10 +348,14 @@ static void icp_realize(DeviceState *dev, Error **errp)
> >      }
> >  
> >      qemu_register_reset(icp_reset, dev);
> > +    vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);  
> 
> so, what I just said in the previous email regarding ->cs would break this 
> patch. Forget about it. 
> 

Unless we have an icp->cpu_index like I mentioned in the other mail.

> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks,
> 
> C.
> 
> >  }
> >  
> >  static void icp_unrealize(DeviceState *dev, Error **errp)
> >  {
> > +    ICPState *icp = ICP(dev);
> > +
> > +    vmstate_unregister(NULL, &vmstate_icp_server, icp);
> >      qemu_unregister_reset(icp_reset, dev);
> >  }
> >  
> > @@ -359,7 +363,6 @@ static void icp_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >  
> > -    dc->vmsd = &vmstate_icp_server;
> >      dc->realize = icp_realize;
> >      dc->unrealize = icp_unrealize;
> >  }
> >   
> 


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

Re: [Qemu-devel] [PATCH v3 4/5] xics: directly register ICPState objects to vmstate

[David Gibson]

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

On Wed, Jun 07, 2017 at 07:17:17PM +0200, Greg Kurz wrote:
> The ICPState objects are currently registered to vmstate as qdev objects.
> Their instance ids are hence computed automatically in the migration code,
> and thus depends on the order the CPU cores were plugged.
> 
> If the destination had its CPU cores plugged in a different order than the
> source, then ICPState objects will have different instance_ids and load
> the wrong state.
> 
> Since CPU objects have a reliable cpu_index which is already used as
> instance_id in vmstate, let's use it for ICPState as well.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/intc/xics.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

So, this is definitely a good idea w.r.t. future compatibility.  But,
won't it break migration compat as a once off, since the devices will
be found under their new id instead of the qdev id?

> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 330441ff7fe8..3d76b43876c5 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -348,10 +348,14 @@ static void icp_realize(DeviceState *dev, Error **errp)
>      }
>  
>      qemu_register_reset(icp_reset, dev);
> +    vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
>  }
>  
>  static void icp_unrealize(DeviceState *dev, Error **errp)
>  {
> +    ICPState *icp = ICP(dev);
> +
> +    vmstate_unregister(NULL, &vmstate_icp_server, icp);
>      qemu_unregister_reset(icp_reset, dev);
>  }
>  
> @@ -359,7 +363,6 @@ static void icp_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -    dc->vmsd = &vmstate_icp_server;
>      dc->realize = icp_realize;
>      dc->unrealize = icp_unrealize;
>  }
> 

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

Re: [Qemu-devel] [PATCH v3 4/5] xics: directly register ICPState objects to vmstate

[Greg Kurz]

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

On Thu, 8 Jun 2017 13:59:21 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jun 07, 2017 at 07:17:17PM +0200, Greg Kurz wrote:
> > The ICPState objects are currently registered to vmstate as qdev objects.
> > Their instance ids are hence computed automatically in the migration code,
> > and thus depends on the order the CPU cores were plugged.
> > 
> > If the destination had its CPU cores plugged in a different order than the
> > source, then ICPState objects will have different instance_ids and load
> > the wrong state.
> > 
> > Since CPU objects have a reliable cpu_index which is already used as
> > instance_id in vmstate, let's use it for ICPState as well.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/intc/xics.c |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)  
> 
> So, this is definitely a good idea w.r.t. future compatibility.  But,
> won't it break migration compat as a once off, since the devices will
> be found under their new id instead of the qdev id?
> 

Older machine types used to allocate/realize all ICPState objects
at machine init time and never release them. The qdev ids are thus
0,1,2... nr_servers and happen to map to cpu_index.

> > 
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index 330441ff7fe8..3d76b43876c5 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -348,10 +348,14 @@ static void icp_realize(DeviceState *dev, Error **errp)
> >      }
> >  
> >      qemu_register_reset(icp_reset, dev);
> > +    vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
> >  }
> >  
> >  static void icp_unrealize(DeviceState *dev, Error **errp)
> >  {
> > +    ICPState *icp = ICP(dev);
> > +
> > +    vmstate_unregister(NULL, &vmstate_icp_server, icp);
> >      qemu_unregister_reset(icp_reset, dev);
> >  }
> >  
> > @@ -359,7 +363,6 @@ static void icp_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >  
> > -    dc->vmsd = &vmstate_icp_server;
> >      dc->realize = icp_realize;
> >      dc->unrealize = icp_unrealize;
> >  }
> >   
> 


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

[Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU

[Greg Kurz]

Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
sPAPRCPUCore") moved ICPState objects from the machine to CPU cores.
This is an improvement since we no longer allocate ICPState objects
that will never be used. But it has the side-effect of breaking
migration of older machine types from older QEMU versions.

This patch allows spapr to register dummy "icp/server" entries to vmstate.
These entries use a dedicated VMStateDescription that can swallow and
discard state of an incoming migration stream, and that don't send anything
on outgoing migration.

As for real ICPState objects, the instance_id is the cpu_index of the
corresponding vCPU, which happens to be equal to the generated instance_id
of older machine types.

The machine can unregister/register these entries when CPUs are dynamically
plugged/unplugged.

This is only available for pseries-2.9 and older machines, thanks to a
compat property.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
v3: - new logic entirely implemented in hw/ppc/spapr.c
---
 hw/ppc/spapr.c         |   88 +++++++++++++++++++++++++++++++++++++++++++++++-
 include/hw/ppc/spapr.h |    2 +
 2 files changed, 88 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 9b7ae28939a8..c15b604978f0 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -124,9 +124,52 @@ error:
     return NULL;
 }
 
+static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
+{
+    return false;
+}
+
+static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
+    .name = "icp/server",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = pre_2_10_vmstate_dummy_icp_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UNUSED(4), /* uint32_t xirr */
+        VMSTATE_UNUSED(1), /* uint8_t pending_priority */
+        VMSTATE_UNUSED(1), /* uint8_t mfrr */
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr, int i)
+{
+    bool *flag = &spapr->pre_2_10_ignore_icp[i];
+
+    g_assert(!*flag);
+    vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp, flag);
+    *flag = true;
+}
+
+static void pre_2_10_vmstate_unregister_dummy_icp(sPAPRMachineState *spapr,
+                                                  int i)
+{
+    bool *flag = &spapr->pre_2_10_ignore_icp[i];
+
+    g_assert(*flag);
+    vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp, flag);
+    *flag = false;
+}
+
+static inline int xics_nr_servers(void)
+{
+    return DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(), smp_threads);
+}
+
 static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
 
     if (kvm_enabled()) {
         if (machine_kernel_irqchip_allowed(machine) &&
@@ -148,6 +191,15 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
             return;
         }
     }
+
+    if (smc->pre_2_10_has_unused_icps) {
+        int i;
+
+        spapr->pre_2_10_ignore_icp = g_malloc(xics_nr_servers());
+        for (i = 0; i < xics_nr_servers(); i++) {
+            pre_2_10_vmstate_register_dummy_icp(spapr, i);
+        }
+    }
 }
 
 static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
@@ -976,7 +1028,6 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
     void *fdt;
     sPAPRPHBState *phb;
     char *buf;
-    int smt = kvmppc_smt_threads();
 
     fdt = g_malloc0(FDT_MAX_SIZE);
     _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
@@ -1016,7 +1067,7 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
     _FDT(fdt_setprop_cell(fdt, 0, "#size-cells", 2));
 
     /* /interrupt controller */
-    spapr_dt_xics(DIV_ROUND_UP(max_cpus * smt, smp_threads), fdt, PHANDLE_XICP);
+    spapr_dt_xics(xics_nr_servers(), fdt, PHANDLE_XICP);
 
     ret = spapr_populate_memory(spapr, fdt);
     if (ret < 0) {
@@ -2800,9 +2851,24 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp)
 {
     MachineState *ms = MACHINE(qdev_get_machine());
+    sPAPRMachineState *spapr = SPAPR_MACHINE(ms);
     CPUCore *cc = CPU_CORE(dev);
     CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);
 
+    if (spapr->pre_2_10_ignore_icp) {
+        sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
+        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
+        const char *typename = object_class_get_name(scc->cpu_class);
+        size_t size = object_type_get_instance_size(typename);
+        int i;
+
+        for (i = 0; i < cc->nr_threads; i++) {
+            CPUState *cs = CPU(sc->threads + i * size);
+
+            pre_2_10_vmstate_register_dummy_icp(spapr, cs->cpu_index);
+        }
+    }
+
     assert(core_slot);
     core_slot->cpu = NULL;
     object_unparent(OBJECT(dev));
@@ -2912,6 +2978,21 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         }
     }
     core_slot->cpu = OBJECT(dev);
+
+    if (spapr->pre_2_10_ignore_icp) {
+        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
+        const char *typename = object_class_get_name(scc->cpu_class);
+        size_t size = object_type_get_instance_size(typename);
+        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);
+            pre_2_10_vmstate_unregister_dummy_icp(spapr, cs->cpu_index);
+        }
+    }
 }
 
 static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
@@ -3361,9 +3442,12 @@ static void spapr_machine_2_9_instance_options(MachineState *machine)
 
 static void spapr_machine_2_9_class_options(MachineClass *mc)
 {
+    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
+
     spapr_machine_2_10_class_options(mc);
     SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_9);
     mc->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
+    smc->pre_2_10_has_unused_icps = true;
 }
 
 DEFINE_SPAPR_MACHINE(2_9, "2.9", false);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index f973b0284596..64382623199d 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -53,6 +53,7 @@ struct sPAPRMachineClass {
     bool dr_lmb_enabled;       /* enable dynamic-reconfig/hotplug of LMBs */
     bool use_ohci_by_default;  /* use USB-OHCI instead of XHCI */
     const char *tcg_default_cpu; /* which (TCG) CPU to simulate by default */
+    bool pre_2_10_has_unused_icps;
     void (*phb_placement)(sPAPRMachineState *spapr, uint32_t index,
                           uint64_t *buid, hwaddr *pio, 
                           hwaddr *mmio32, hwaddr *mmio64,
@@ -90,6 +91,7 @@ struct sPAPRMachineState {
     sPAPROptionVector *ov5_cas;     /* negotiated (via CAS) option vectors */
     bool cas_reboot;
     bool cas_legacy_guest_workaround;
+    bool *pre_2_10_ignore_icp;
 
     Notifier epow_notifier;
     QTAILQ_HEAD(, sPAPREventLogEntry) pending_events;

Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU

[David Gibson]

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

On Wed, Jun 07, 2017 at 07:17:26PM +0200, Greg Kurz wrote:
> Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> sPAPRCPUCore") moved ICPState objects from the machine to CPU cores.
> This is an improvement since we no longer allocate ICPState objects
> that will never be used. But it has the side-effect of breaking
> migration of older machine types from older QEMU versions.
> 
> This patch allows spapr to register dummy "icp/server" entries to vmstate.
> These entries use a dedicated VMStateDescription that can swallow and
> discard state of an incoming migration stream, and that don't send anything
> on outgoing migration.
> 
> As for real ICPState objects, the instance_id is the cpu_index of the
> corresponding vCPU, which happens to be equal to the generated instance_id
> of older machine types.
> 
> The machine can unregister/register these entries when CPUs are dynamically
> plugged/unplugged.
> 
> This is only available for pseries-2.9 and older machines, thanks to a
> compat property.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> v3: - new logic entirely implemented in hw/ppc/spapr.c
> ---
>  hw/ppc/spapr.c         |   88 +++++++++++++++++++++++++++++++++++++++++++++++-
>  include/hw/ppc/spapr.h |    2 +
>  2 files changed, 88 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 9b7ae28939a8..c15b604978f0 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -124,9 +124,52 @@ error:
>      return NULL;
>  }
>  
> +static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
> +{
> +    return false;
> +}

Uh.. the needed function always returns false, how can that work?

> +
> +static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> +    .name = "icp/server",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = pre_2_10_vmstate_dummy_icp_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UNUSED(4), /* uint32_t xirr */
> +        VMSTATE_UNUSED(1), /* uint8_t pending_priority */
> +        VMSTATE_UNUSED(1), /* uint8_t mfrr */
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr, int i)
> +{
> +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> +
> +    g_assert(!*flag);

Apart from this assert(), you never seem to test the values in the
pre_2_10_ignore_icp() array, so it seems a bit pointless.

> +    vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp, flag);
> +    *flag = true;
> +}
> +
> +static void pre_2_10_vmstate_unregister_dummy_icp(sPAPRMachineState *spapr,
> +                                                  int i)
> +{
> +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> +
> +    g_assert(*flag);
> +    vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp, flag);
> +    *flag = false;
> +}
> +
> +static inline int xics_nr_servers(void)

Maybe a different name to emphasise that this is only used for the
backwards compat logic.

> +{
> +    return DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(), smp_threads);
> +}
> +
>  static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
>  
>      if (kvm_enabled()) {
>          if (machine_kernel_irqchip_allowed(machine) &&
> @@ -148,6 +191,15 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
>              return;
>          }
>      }
> +
> +    if (smc->pre_2_10_has_unused_icps) {
> +        int i;
> +
> +        spapr->pre_2_10_ignore_icp = g_malloc(xics_nr_servers());
> +        for (i = 0; i < xics_nr_servers(); i++) {
> +            pre_2_10_vmstate_register_dummy_icp(spapr, i);

This registers a dummy ICP for every slot, some of which will have
real cpus / icps.  That doesn't seem right.

> +        }
> +    }
>  }
>  
>  static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
> @@ -976,7 +1028,6 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
>      void *fdt;
>      sPAPRPHBState *phb;
>      char *buf;
> -    int smt = kvmppc_smt_threads();
>  
>      fdt = g_malloc0(FDT_MAX_SIZE);
>      _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
> @@ -1016,7 +1067,7 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
>      _FDT(fdt_setprop_cell(fdt, 0, "#size-cells", 2));
>  
>      /* /interrupt controller */
> -    spapr_dt_xics(DIV_ROUND_UP(max_cpus * smt, smp_threads), fdt, PHANDLE_XICP);
> +    spapr_dt_xics(xics_nr_servers(), fdt, PHANDLE_XICP);
>  
>      ret = spapr_populate_memory(spapr, fdt);
>      if (ret < 0) {
> @@ -2800,9 +2851,24 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                Error **errp)
>  {
>      MachineState *ms = MACHINE(qdev_get_machine());
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(ms);
>      CPUCore *cc = CPU_CORE(dev);
>      CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);
>  
> +    if (spapr->pre_2_10_ignore_icp) {
> +        sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> +        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
> +        const char *typename = object_class_get_name(scc->cpu_class);
> +        size_t size = object_type_get_instance_size(typename);
> +        int i;
> +
> +        for (i = 0; i < cc->nr_threads; i++) {
> +            CPUState *cs = CPU(sc->threads + i * size);
> +
> +            pre_2_10_vmstate_register_dummy_icp(spapr, cs->cpu_index);
> +        }
> +    }
> +
>      assert(core_slot);
>      core_slot->cpu = NULL;
>      object_unparent(OBJECT(dev));
> @@ -2912,6 +2978,21 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          }
>      }
>      core_slot->cpu = OBJECT(dev);
> +
> +    if (spapr->pre_2_10_ignore_icp) {
> +        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
> +        const char *typename = object_class_get_name(scc->cpu_class);
> +        size_t size = object_type_get_instance_size(typename);
> +        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);
> +            pre_2_10_vmstate_unregister_dummy_icp(spapr, cs->cpu_index);
> +        }
> +    }
>  }
>  
>  static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> @@ -3361,9 +3442,12 @@ static void spapr_machine_2_9_instance_options(MachineState *machine)
>  
>  static void spapr_machine_2_9_class_options(MachineClass *mc)
>  {
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> +
>      spapr_machine_2_10_class_options(mc);
>      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_9);
>      mc->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
> +    smc->pre_2_10_has_unused_icps = true;
>  }
>  
>  DEFINE_SPAPR_MACHINE(2_9, "2.9", false);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index f973b0284596..64382623199d 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -53,6 +53,7 @@ struct sPAPRMachineClass {
>      bool dr_lmb_enabled;       /* enable dynamic-reconfig/hotplug of LMBs */
>      bool use_ohci_by_default;  /* use USB-OHCI instead of XHCI */
>      const char *tcg_default_cpu; /* which (TCG) CPU to simulate by default */
> +    bool pre_2_10_has_unused_icps;
>      void (*phb_placement)(sPAPRMachineState *spapr, uint32_t index,
>                            uint64_t *buid, hwaddr *pio, 
>                            hwaddr *mmio32, hwaddr *mmio64,
> @@ -90,6 +91,7 @@ struct sPAPRMachineState {
>      sPAPROptionVector *ov5_cas;     /* negotiated (via CAS) option vectors */
>      bool cas_reboot;
>      bool cas_legacy_guest_workaround;
> +    bool *pre_2_10_ignore_icp;
>  
>      Notifier epow_notifier;
>      QTAILQ_HEAD(, sPAPREventLogEntry) pending_events;
> 

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

Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU

[Greg Kurz]

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

On Thu, 8 Jun 2017 14:08:57 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jun 07, 2017 at 07:17:26PM +0200, Greg Kurz wrote:
> > Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> > sPAPRCPUCore") moved ICPState objects from the machine to CPU cores.
> > This is an improvement since we no longer allocate ICPState objects
> > that will never be used. But it has the side-effect of breaking
> > migration of older machine types from older QEMU versions.
> > 
> > This patch allows spapr to register dummy "icp/server" entries to vmstate.
> > These entries use a dedicated VMStateDescription that can swallow and
> > discard state of an incoming migration stream, and that don't send anything
> > on outgoing migration.
> > 
> > As for real ICPState objects, the instance_id is the cpu_index of the
> > corresponding vCPU, which happens to be equal to the generated instance_id
> > of older machine types.
> > 
> > The machine can unregister/register these entries when CPUs are dynamically
> > plugged/unplugged.
> > 
> > This is only available for pseries-2.9 and older machines, thanks to a
> > compat property.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > v3: - new logic entirely implemented in hw/ppc/spapr.c
> > ---
> >  hw/ppc/spapr.c         |   88 +++++++++++++++++++++++++++++++++++++++++++++++-
> >  include/hw/ppc/spapr.h |    2 +
> >  2 files changed, 88 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 9b7ae28939a8..c15b604978f0 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -124,9 +124,52 @@ error:
> >      return NULL;
> >  }
> >  
> > +static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
> > +{
> > +    return false;
> > +}  
> 
> Uh.. the needed function always returns false, how can that work?
> 

The needed function is used for outgoing migration only:

bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque)
{
    if (vmsd->needed && !vmsd->needed(opaque)) {
        /* optional section not needed */
        return false;
    }
    return true;
}

The idea is that all ICPState objects that were created but not associated
to a vCPU by pre-2.10 machine types don't need to be migrated at all, as
their state hasn't changed.

We don't even create these unneeded ICPState objects here, but simply
register their ids to vmstate.

> > +
> > +static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> > +    .name = "icp/server",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .needed = pre_2_10_vmstate_dummy_icp_needed,

Outgoing migration:
- machine in older QEMU have unused ICPState objects (default state)
- machine in QEMU 2.10 doesn't even have extra ICPState objects

=> don't send anything

> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UNUSED(4), /* uint32_t xirr */
> > +        VMSTATE_UNUSED(1), /* uint8_t pending_priority */
> > +        VMSTATE_UNUSED(1), /* uint8_t mfrr */

Incoming migration from older QEMU: we don't have the extra ICPState objects.

=> accept the state and discard it

> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +};
> > +
> > +static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr, int i)
> > +{
> > +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > +
> > +    g_assert(!*flag);  
> 
> Apart from this assert(), you never seem to test the values in the
> pre_2_10_ignore_icp() array, so it seems a bit pointless.
> 

There's the opposite check in pre_2_10_vmstate_unregister_dummy_icp().
But I agree it isn't really useful... but more because of paranoia :)

> > +    vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp, flag);
> > +    *flag = true;
> > +}
> > +
> > +static void pre_2_10_vmstate_unregister_dummy_icp(sPAPRMachineState *spapr,
> > +                                                  int i)
> > +{
> > +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > +
> > +    g_assert(*flag);
> > +    vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp, flag);
> > +    *flag = false;
> > +}
> > +
> > +static inline int xics_nr_servers(void)  
> 
> Maybe a different name to emphasise that this is only used for the
> backwards compat logic.
> 

It is also used to compute the "ibm,interrupt-server-ranges" DT prop.

    /* /interrupt controller */
    spapr_dt_xics(xics_nr_servers(), fdt, PHANDLE_XICP);


> > +{
> > +    return DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(), smp_threads);
> > +}
> > +
> >  static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> >  {
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> >  
> >      if (kvm_enabled()) {
> >          if (machine_kernel_irqchip_allowed(machine) &&
> > @@ -148,6 +191,15 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> >              return;
> >          }
> >      }
> > +
> > +    if (smc->pre_2_10_has_unused_icps) {
> > +        int i;
> > +
> > +        spapr->pre_2_10_ignore_icp = g_malloc(xics_nr_servers());
> > +        for (i = 0; i < xics_nr_servers(); i++) {
> > +            pre_2_10_vmstate_register_dummy_icp(spapr, i);  
> 
> This registers a dummy ICP for every slot, some of which will have
> real cpus / icps.  That doesn't seem right.
> 

This is initialization, before we even have actual CPUs. We start with
dummy ICPs for every slot, but they get replaced by real ICPs when we
plug CPU cores...... (see below)

> > +        }
> > +    }
> >  }
> >  
> >  static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
> > @@ -976,7 +1028,6 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
> >      void *fdt;
> >      sPAPRPHBState *phb;
> >      char *buf;
> > -    int smt = kvmppc_smt_threads();
> >  
> >      fdt = g_malloc0(FDT_MAX_SIZE);
> >      _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
> > @@ -1016,7 +1067,7 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
> >      _FDT(fdt_setprop_cell(fdt, 0, "#size-cells", 2));
> >  
> >      /* /interrupt controller */
> > -    spapr_dt_xics(DIV_ROUND_UP(max_cpus * smt, smp_threads), fdt, PHANDLE_XICP);
> > +    spapr_dt_xics(xics_nr_servers(), fdt, PHANDLE_XICP);
> >  
> >      ret = spapr_populate_memory(spapr, fdt);
> >      if (ret < 0) {
> > @@ -2800,9 +2851,24 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >                                Error **errp)
> >  {
> >      MachineState *ms = MACHINE(qdev_get_machine());
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(ms);
> >      CPUCore *cc = CPU_CORE(dev);
> >      CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);
> >  
> > +    if (spapr->pre_2_10_ignore_icp) {
> > +        sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > +        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
> > +        const char *typename = object_class_get_name(scc->cpu_class);
> > +        size_t size = object_type_get_instance_size(typename);
> > +        int i;
> > +
> > +        for (i = 0; i < cc->nr_threads; i++) {
> > +            CPUState *cs = CPU(sc->threads + i * size);
> > +
> > +            pre_2_10_vmstate_register_dummy_icp(spapr, cs->cpu_index);
> > +        }
> > +    }
> > +
> >      assert(core_slot);
> >      core_slot->cpu = NULL;
> >      object_unparent(OBJECT(dev));
> > @@ -2912,6 +2978,21 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >          }
> >      }
> >      core_slot->cpu = OBJECT(dev);
> > +
> > +    if (spapr->pre_2_10_ignore_icp) {
> > +        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
> > +        const char *typename = object_class_get_name(scc->cpu_class);
> > +        size_t size = object_type_get_instance_size(typename);
> > +        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);
> > +            pre_2_10_vmstate_unregister_dummy_icp(spapr, cs->cpu_index);

...... here.

The opposite happens in spapr_core_unplug().

> > +        }
> > +    }
> >  }
> >  
> >  static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > @@ -3361,9 +3442,12 @@ static void spapr_machine_2_9_instance_options(MachineState *machine)
> >  
> >  static void spapr_machine_2_9_class_options(MachineClass *mc)
> >  {
> > +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> > +
> >      spapr_machine_2_10_class_options(mc);
> >      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_9);
> >      mc->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
> > +    smc->pre_2_10_has_unused_icps = true;
> >  }
> >  
> >  DEFINE_SPAPR_MACHINE(2_9, "2.9", false);
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index f973b0284596..64382623199d 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -53,6 +53,7 @@ struct sPAPRMachineClass {
> >      bool dr_lmb_enabled;       /* enable dynamic-reconfig/hotplug of LMBs */
> >      bool use_ohci_by_default;  /* use USB-OHCI instead of XHCI */
> >      const char *tcg_default_cpu; /* which (TCG) CPU to simulate by default */
> > +    bool pre_2_10_has_unused_icps;
> >      void (*phb_placement)(sPAPRMachineState *spapr, uint32_t index,
> >                            uint64_t *buid, hwaddr *pio, 
> >                            hwaddr *mmio32, hwaddr *mmio64,
> > @@ -90,6 +91,7 @@ struct sPAPRMachineState {
> >      sPAPROptionVector *ov5_cas;     /* negotiated (via CAS) option vectors */
> >      bool cas_reboot;
> >      bool cas_legacy_guest_workaround;
> > +    bool *pre_2_10_ignore_icp;
> >  
> >      Notifier epow_notifier;
> >      QTAILQ_HEAD(, sPAPREventLogEntry) pending_events;
> >   
> 


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

Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU

[David Gibson]

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

On Thu, Jun 08, 2017 at 11:54:10AM +0200, Greg Kurz wrote:
> On Thu, 8 Jun 2017 14:08:57 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Jun 07, 2017 at 07:17:26PM +0200, Greg Kurz wrote:
> > > Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> > > sPAPRCPUCore") moved ICPState objects from the machine to CPU cores.
> > > This is an improvement since we no longer allocate ICPState objects
> > > that will never be used. But it has the side-effect of breaking
> > > migration of older machine types from older QEMU versions.
> > > 
> > > This patch allows spapr to register dummy "icp/server" entries to vmstate.
> > > These entries use a dedicated VMStateDescription that can swallow and
> > > discard state of an incoming migration stream, and that don't send anything
> > > on outgoing migration.
> > > 
> > > As for real ICPState objects, the instance_id is the cpu_index of the
> > > corresponding vCPU, which happens to be equal to the generated instance_id
> > > of older machine types.
> > > 
> > > The machine can unregister/register these entries when CPUs are dynamically
> > > plugged/unplugged.
> > > 
> > > This is only available for pseries-2.9 and older machines, thanks to a
> > > compat property.
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > > v3: - new logic entirely implemented in hw/ppc/spapr.c
> > > ---
> > >  hw/ppc/spapr.c         |   88 +++++++++++++++++++++++++++++++++++++++++++++++-
> > >  include/hw/ppc/spapr.h |    2 +
> > >  2 files changed, 88 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 9b7ae28939a8..c15b604978f0 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -124,9 +124,52 @@ error:
> > >      return NULL;
> > >  }
> > >  
> > > +static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
> > > +{
> > > +    return false;
> > > +}  
> > 
> > Uh.. the needed function always returns false, how can that work?
> > 
> 
> The needed function is used for outgoing migration only:
> 
> bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque)
> {
>     if (vmsd->needed && !vmsd->needed(opaque)) {
>         /* optional section not needed */
>         return false;
>     }
>     return true;
> }
> 
> The idea is that all ICPState objects that were created but not associated
> to a vCPU by pre-2.10 machine types don't need to be migrated at all, as
> their state hasn't changed.
> 
> We don't even create these unneeded ICPState objects here, but simply
> register their ids to vmstate.
> 
> > > +
> > > +static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> > > +    .name = "icp/server",
> > > +    .version_id = 1,
> > > +    .minimum_version_id = 1,
> > > +    .needed = pre_2_10_vmstate_dummy_icp_needed,
> 
> Outgoing migration:
> - machine in older QEMU have unused ICPState objects (default state)
> - machine in QEMU 2.10 doesn't even have extra ICPState objects
> 
> => don't send anything
> 
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_UNUSED(4), /* uint32_t xirr */
> > > +        VMSTATE_UNUSED(1), /* uint8_t pending_priority */
> > > +        VMSTATE_UNUSED(1), /* uint8_t mfrr */
> 
> Incoming migration from older QEMU: we don't have the extra ICPState objects.
> 
> => accept the state and discard it
> 
> > > +        VMSTATE_END_OF_LIST()
> > > +    },
> > > +};
> > > +
> > > +static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr, int i)
> > > +{
> > > +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > > +
> > > +    g_assert(!*flag);  
> > 
> > Apart from this assert(), you never seem to test the values in the
> > pre_2_10_ignore_icp() array, so it seems a bit pointless.
> > 
> 
> There's the opposite check in pre_2_10_vmstate_unregister_dummy_icp().
> But I agree it isn't really useful... but more because of paranoia :)

I'm all for paranoid assert()s if they can be made using data readily
to hand.  Adding a data structure just for the purpose of making an
assert() later, not so much.

> > > +    vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp, flag);
> > > +    *flag = true;
> > > +}
> > > +
> > > +static void pre_2_10_vmstate_unregister_dummy_icp(sPAPRMachineState *spapr,
> > > +                                                  int i)
> > > +{
> > > +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > > +
> > > +    g_assert(*flag);
> > > +    vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp, flag);
> > > +    *flag = false;
> > > +}
> > > +
> > > +static inline int xics_nr_servers(void)  
> > 
> > Maybe a different name to emphasise that this is only used for the
> > backwards compat logic.
> > 
> 
> It is also used to compute the "ibm,interrupt-server-ranges" DT prop.
> 
>     /* /interrupt controller */
>     spapr_dt_xics(xics_nr_servers(), fdt, PHANDLE_XICP);

Ah, good point.  Maybe rename to "max server number" or something,
since "nr_servers" isn't really accurate any more.

> > > +{
> > > +    return DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(), smp_threads);
> > > +}
> > > +
> > >  static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> > >  {
> > >      sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > > +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> > >  
> > >      if (kvm_enabled()) {
> > >          if (machine_kernel_irqchip_allowed(machine) &&
> > > @@ -148,6 +191,15 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> > >              return;
> > >          }
> > >      }
> > > +
> > > +    if (smc->pre_2_10_has_unused_icps) {
> > > +        int i;
> > > +
> > > +        spapr->pre_2_10_ignore_icp = g_malloc(xics_nr_servers());
> > > +        for (i = 0; i < xics_nr_servers(); i++) {
> > > +            pre_2_10_vmstate_register_dummy_icp(spapr, i);  
> > 
> > This registers a dummy ICP for every slot, some of which will have
> > real cpus / icps.  That doesn't seem right.
> > 
> 
> This is initialization, before we even have actual CPUs. We start with
> dummy ICPs for every slot, but they get replaced by real ICPs when we
> plug CPU cores...... (see below)
> 
> > > +        }
> > > +    }
> > >  }
> > >  
> > >  static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
> > > @@ -976,7 +1028,6 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
> > >      void *fdt;
> > >      sPAPRPHBState *phb;
> > >      char *buf;
> > > -    int smt = kvmppc_smt_threads();
> > >  
> > >      fdt = g_malloc0(FDT_MAX_SIZE);
> > >      _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
> > > @@ -1016,7 +1067,7 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
> > >      _FDT(fdt_setprop_cell(fdt, 0, "#size-cells", 2));
> > >  
> > >      /* /interrupt controller */
> > > -    spapr_dt_xics(DIV_ROUND_UP(max_cpus * smt, smp_threads), fdt, PHANDLE_XICP);
> > > +    spapr_dt_xics(xics_nr_servers(), fdt, PHANDLE_XICP);
> > >  
> > >      ret = spapr_populate_memory(spapr, fdt);
> > >      if (ret < 0) {
> > > @@ -2800,9 +2851,24 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >                                Error **errp)
> > >  {
> > >      MachineState *ms = MACHINE(qdev_get_machine());
> > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(ms);
> > >      CPUCore *cc = CPU_CORE(dev);
> > >      CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);
> > >  
> > > +    if (spapr->pre_2_10_ignore_icp) {
> > > +        sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > > +        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
> > > +        const char *typename = object_class_get_name(scc->cpu_class);
> > > +        size_t size = object_type_get_instance_size(typename);
> > > +        int i;
> > > +
> > > +        for (i = 0; i < cc->nr_threads; i++) {
> > > +            CPUState *cs = CPU(sc->threads + i * size);
> > > +
> > > +            pre_2_10_vmstate_register_dummy_icp(spapr, cs->cpu_index);
> > > +        }
> > > +    }
> > > +
> > >      assert(core_slot);
> > >      core_slot->cpu = NULL;
> > >      object_unparent(OBJECT(dev));
> > > @@ -2912,6 +2978,21 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >          }
> > >      }
> > >      core_slot->cpu = OBJECT(dev);
> > > +
> > > +    if (spapr->pre_2_10_ignore_icp) {
> > > +        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
> > > +        const char *typename = object_class_get_name(scc->cpu_class);
> > > +        size_t size = object_type_get_instance_size(typename);
> > > +        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);
> > > +            pre_2_10_vmstate_unregister_dummy_icp(spapr, cs->cpu_index);
> 
> ...... here.
> 
> The opposite happens in spapr_core_unplug().
> 
> > > +        }
> > > +    }
> > >  }
> > >  
> > >  static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > @@ -3361,9 +3442,12 @@ static void spapr_machine_2_9_instance_options(MachineState *machine)
> > >  
> > >  static void spapr_machine_2_9_class_options(MachineClass *mc)
> > >  {
> > > +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> > > +
> > >      spapr_machine_2_10_class_options(mc);
> > >      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_9);
> > >      mc->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
> > > +    smc->pre_2_10_has_unused_icps = true;
> > >  }
> > >  
> > >  DEFINE_SPAPR_MACHINE(2_9, "2.9", false);
> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > index f973b0284596..64382623199d 100644
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -53,6 +53,7 @@ struct sPAPRMachineClass {
> > >      bool dr_lmb_enabled;       /* enable dynamic-reconfig/hotplug of LMBs */
> > >      bool use_ohci_by_default;  /* use USB-OHCI instead of XHCI */
> > >      const char *tcg_default_cpu; /* which (TCG) CPU to simulate by default */
> > > +    bool pre_2_10_has_unused_icps;
> > >      void (*phb_placement)(sPAPRMachineState *spapr, uint32_t index,
> > >                            uint64_t *buid, hwaddr *pio, 
> > >                            hwaddr *mmio32, hwaddr *mmio64,
> > > @@ -90,6 +91,7 @@ struct sPAPRMachineState {
> > >      sPAPROptionVector *ov5_cas;     /* negotiated (via CAS) option vectors */
> > >      bool cas_reboot;
> > >      bool cas_legacy_guest_workaround;
> > > +    bool *pre_2_10_ignore_icp;
> > >  
> > >      Notifier epow_notifier;
> > >      QTAILQ_HEAD(, sPAPREventLogEntry) pending_events;
> > >   
> > 
> 



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

Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU

[Greg Kurz]

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

On Mon, 12 Jun 2017 22:24:56 +0800
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jun 08, 2017 at 11:54:10AM +0200, Greg Kurz wrote:
> > On Thu, 8 Jun 2017 14:08:57 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Wed, Jun 07, 2017 at 07:17:26PM +0200, Greg Kurz wrote:  
> > > > Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> > > > sPAPRCPUCore") moved ICPState objects from the machine to CPU cores.
> > > > This is an improvement since we no longer allocate ICPState objects
> > > > that will never be used. But it has the side-effect of breaking
> > > > migration of older machine types from older QEMU versions.
> > > > 
> > > > This patch allows spapr to register dummy "icp/server" entries to vmstate.
> > > > These entries use a dedicated VMStateDescription that can swallow and
> > > > discard state of an incoming migration stream, and that don't send anything
> > > > on outgoing migration.
> > > > 
> > > > As for real ICPState objects, the instance_id is the cpu_index of the
> > > > corresponding vCPU, which happens to be equal to the generated instance_id
> > > > of older machine types.
> > > > 
> > > > The machine can unregister/register these entries when CPUs are dynamically
> > > > plugged/unplugged.
> > > > 
> > > > This is only available for pseries-2.9 and older machines, thanks to a
> > > > compat property.
> > > > 
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > ---
> > > > v3: - new logic entirely implemented in hw/ppc/spapr.c
> > > > ---
> > > >  hw/ppc/spapr.c         |   88 +++++++++++++++++++++++++++++++++++++++++++++++-
> > > >  include/hw/ppc/spapr.h |    2 +
> > > >  2 files changed, 88 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 9b7ae28939a8..c15b604978f0 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -124,9 +124,52 @@ error:
> > > >      return NULL;
> > > >  }
> > > >  
> > > > +static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
> > > > +{
> > > > +    return false;
> > > > +}    
> > > 
> > > Uh.. the needed function always returns false, how can that work?
> > >   
> > 
> > The needed function is used for outgoing migration only:
> > 
> > bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque)
> > {
> >     if (vmsd->needed && !vmsd->needed(opaque)) {
> >         /* optional section not needed */
> >         return false;
> >     }
> >     return true;
> > }
> > 
> > The idea is that all ICPState objects that were created but not associated
> > to a vCPU by pre-2.10 machine types don't need to be migrated at all, as
> > their state hasn't changed.
> > 
> > We don't even create these unneeded ICPState objects here, but simply
> > register their ids to vmstate.
> >   
> > > > +
> > > > +static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> > > > +    .name = "icp/server",
> > > > +    .version_id = 1,
> > > > +    .minimum_version_id = 1,
> > > > +    .needed = pre_2_10_vmstate_dummy_icp_needed,  
> > 
> > Outgoing migration:
> > - machine in older QEMU have unused ICPState objects (default state)
> > - machine in QEMU 2.10 doesn't even have extra ICPState objects
> >   
> > => don't send anything  
> >   
> > > > +    .fields = (VMStateField[]) {
> > > > +        VMSTATE_UNUSED(4), /* uint32_t xirr */
> > > > +        VMSTATE_UNUSED(1), /* uint8_t pending_priority */
> > > > +        VMSTATE_UNUSED(1), /* uint8_t mfrr */  
> > 
> > Incoming migration from older QEMU: we don't have the extra ICPState objects.
> >   
> > => accept the state and discard it  
> >   
> > > > +        VMSTATE_END_OF_LIST()
> > > > +    },
> > > > +};
> > > > +
> > > > +static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr, int i)
> > > > +{
> > > > +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > > > +
> > > > +    g_assert(!*flag);    
> > > 
> > > Apart from this assert(), you never seem to test the values in the
> > > pre_2_10_ignore_icp() array, so it seems a bit pointless.
> > >   
> > 
> > There's the opposite check in pre_2_10_vmstate_unregister_dummy_icp().
> > But I agree it isn't really useful... but more because of paranoia :)  
> 
> I'm all for paranoid assert()s if they can be made using data readily
> to hand.  Adding a data structure just for the purpose of making an
> assert() later, not so much.
> 

It is also passed as opaque argument to vmstate_register(), where it is
used as a key when calling vmstate_unregister(). I could possibly pass
(void *) i instead, but I'm not a big fan of hijacking pointer arguments
to pass numbers.

> > > > +    vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp, flag);
> > > > +    *flag = true;
> > > > +}
> > > > +
> > > > +static void pre_2_10_vmstate_unregister_dummy_icp(sPAPRMachineState *spapr,
> > > > +                                                  int i)
> > > > +{
> > > > +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > > > +
> > > > +    g_assert(*flag);
> > > > +    vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp, flag);
> > > > +    *flag = false;
> > > > +}
> > > > +
> > > > +static inline int xics_nr_servers(void)    
> > > 
> > > Maybe a different name to emphasise that this is only used for the
> > > backwards compat logic.
> > >   
> > 
> > It is also used to compute the "ibm,interrupt-server-ranges" DT prop.
> > 
> >     /* /interrupt controller */
> >     spapr_dt_xics(xics_nr_servers(), fdt, PHANDLE_XICP);  
> 
> Ah, good point.  Maybe rename to "max server number" or something,
> since "nr_servers" isn't really accurate any more.
> 

Sure, I'll do that.

> > > > +{
> > > > +    return DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(), smp_threads);
> > > > +}
> > > > +
> > > >  static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> > > >  {
> > > >      sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > > > +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> > > >  
> > > >      if (kvm_enabled()) {
> > > >          if (machine_kernel_irqchip_allowed(machine) &&
> > > > @@ -148,6 +191,15 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> > > >              return;
> > > >          }
> > > >      }
> > > > +
> > > > +    if (smc->pre_2_10_has_unused_icps) {
> > > > +        int i;
> > > > +
> > > > +        spapr->pre_2_10_ignore_icp = g_malloc(xics_nr_servers());
> > > > +        for (i = 0; i < xics_nr_servers(); i++) {
> > > > +            pre_2_10_vmstate_register_dummy_icp(spapr, i);    
> > > 
> > > This registers a dummy ICP for every slot, some of which will have
> > > real cpus / icps.  That doesn't seem right.
> > >   
> > 
> > This is initialization, before we even have actual CPUs. We start with
> > dummy ICPs for every slot, but they get replaced by real ICPs when we
> > plug CPU cores...... (see below)
> >   
> > > > +        }
> > > > +    }
> > > >  }
> > > >  
> > > >  static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
> > > > @@ -976,7 +1028,6 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
> > > >      void *fdt;
> > > >      sPAPRPHBState *phb;
> > > >      char *buf;
> > > > -    int smt = kvmppc_smt_threads();
> > > >  
> > > >      fdt = g_malloc0(FDT_MAX_SIZE);
> > > >      _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
> > > > @@ -1016,7 +1067,7 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
> > > >      _FDT(fdt_setprop_cell(fdt, 0, "#size-cells", 2));
> > > >  
> > > >      /* /interrupt controller */
> > > > -    spapr_dt_xics(DIV_ROUND_UP(max_cpus * smt, smp_threads), fdt, PHANDLE_XICP);
> > > > +    spapr_dt_xics(xics_nr_servers(), fdt, PHANDLE_XICP);
> > > >  
> > > >      ret = spapr_populate_memory(spapr, fdt);
> > > >      if (ret < 0) {
> > > > @@ -2800,9 +2851,24 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > >                                Error **errp)
> > > >  {
> > > >      MachineState *ms = MACHINE(qdev_get_machine());
> > > > +    sPAPRMachineState *spapr = SPAPR_MACHINE(ms);
> > > >      CPUCore *cc = CPU_CORE(dev);
> > > >      CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);
> > > >  
> > > > +    if (spapr->pre_2_10_ignore_icp) {
> > > > +        sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > > > +        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
> > > > +        const char *typename = object_class_get_name(scc->cpu_class);
> > > > +        size_t size = object_type_get_instance_size(typename);
> > > > +        int i;
> > > > +
> > > > +        for (i = 0; i < cc->nr_threads; i++) {
> > > > +            CPUState *cs = CPU(sc->threads + i * size);
> > > > +
> > > > +            pre_2_10_vmstate_register_dummy_icp(spapr, cs->cpu_index);
> > > > +        }
> > > > +    }
> > > > +
> > > >      assert(core_slot);
> > > >      core_slot->cpu = NULL;
> > > >      object_unparent(OBJECT(dev));
> > > > @@ -2912,6 +2978,21 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > >          }
> > > >      }
> > > >      core_slot->cpu = OBJECT(dev);
> > > > +
> > > > +    if (spapr->pre_2_10_ignore_icp) {
> > > > +        sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(OBJECT(cc));
> > > > +        const char *typename = object_class_get_name(scc->cpu_class);
> > > > +        size_t size = object_type_get_instance_size(typename);
> > > > +        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);
> > > > +            pre_2_10_vmstate_unregister_dummy_icp(spapr, cs->cpu_index);  
> > 
> > ...... here.
> > 
> > The opposite happens in spapr_core_unplug().
> >   
> > > > +        }
> > > > +    }
> > > >  }
> > > >  
> > > >  static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > > @@ -3361,9 +3442,12 @@ static void spapr_machine_2_9_instance_options(MachineState *machine)
> > > >  
> > > >  static void spapr_machine_2_9_class_options(MachineClass *mc)
> > > >  {
> > > > +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> > > > +
> > > >      spapr_machine_2_10_class_options(mc);
> > > >      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_9);
> > > >      mc->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
> > > > +    smc->pre_2_10_has_unused_icps = true;
> > > >  }
> > > >  
> > > >  DEFINE_SPAPR_MACHINE(2_9, "2.9", false);
> > > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > > index f973b0284596..64382623199d 100644
> > > > --- a/include/hw/ppc/spapr.h
> > > > +++ b/include/hw/ppc/spapr.h
> > > > @@ -53,6 +53,7 @@ struct sPAPRMachineClass {
> > > >      bool dr_lmb_enabled;       /* enable dynamic-reconfig/hotplug of LMBs */
> > > >      bool use_ohci_by_default;  /* use USB-OHCI instead of XHCI */
> > > >      const char *tcg_default_cpu; /* which (TCG) CPU to simulate by default */
> > > > +    bool pre_2_10_has_unused_icps;
> > > >      void (*phb_placement)(sPAPRMachineState *spapr, uint32_t index,
> > > >                            uint64_t *buid, hwaddr *pio, 
> > > >                            hwaddr *mmio32, hwaddr *mmio64,
> > > > @@ -90,6 +91,7 @@ struct sPAPRMachineState {
> > > >      sPAPROptionVector *ov5_cas;     /* negotiated (via CAS) option vectors */
> > > >      bool cas_reboot;
> > > >      bool cas_legacy_guest_workaround;
> > > > +    bool *pre_2_10_ignore_icp;
> > > >  
> > > >      Notifier epow_notifier;
> > > >      QTAILQ_HEAD(, sPAPREventLogEntry) pending_events;
> > > >     
> > >   
> >   
> 
> 
> 


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

Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU

[David Gibson]

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

On Tue, Jun 13, 2017 at 09:33:59AM +0200, Greg Kurz wrote:
> On Mon, 12 Jun 2017 22:24:56 +0800
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Jun 08, 2017 at 11:54:10AM +0200, Greg Kurz wrote:
> > > On Thu, 8 Jun 2017 14:08:57 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Wed, Jun 07, 2017 at 07:17:26PM +0200, Greg Kurz wrote:  
> > > > > Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> > > > > sPAPRCPUCore") moved ICPState objects from the machine to CPU cores.
> > > > > This is an improvement since we no longer allocate ICPState objects
> > > > > that will never be used. But it has the side-effect of breaking
> > > > > migration of older machine types from older QEMU versions.
> > > > > 
> > > > > This patch allows spapr to register dummy "icp/server" entries to vmstate.
> > > > > These entries use a dedicated VMStateDescription that can swallow and
> > > > > discard state of an incoming migration stream, and that don't send anything
> > > > > on outgoing migration.
> > > > > 
> > > > > As for real ICPState objects, the instance_id is the cpu_index of the
> > > > > corresponding vCPU, which happens to be equal to the generated instance_id
> > > > > of older machine types.
> > > > > 
> > > > > The machine can unregister/register these entries when CPUs are dynamically
> > > > > plugged/unplugged.
> > > > > 
> > > > > This is only available for pseries-2.9 and older machines, thanks to a
> > > > > compat property.
> > > > > 
> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > > ---
> > > > > v3: - new logic entirely implemented in hw/ppc/spapr.c
> > > > > ---
> > > > >  hw/ppc/spapr.c         |   88 +++++++++++++++++++++++++++++++++++++++++++++++-
> > > > >  include/hw/ppc/spapr.h |    2 +
> > > > >  2 files changed, 88 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > index 9b7ae28939a8..c15b604978f0 100644
> > > > > --- a/hw/ppc/spapr.c
> > > > > +++ b/hw/ppc/spapr.c
> > > > > @@ -124,9 +124,52 @@ error:
> > > > >      return NULL;
> > > > >  }
> > > > >  
> > > > > +static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
> > > > > +{
> > > > > +    return false;
> > > > > +}    
> > > > 
> > > > Uh.. the needed function always returns false, how can that work?
> > > >   
> > > 
> > > The needed function is used for outgoing migration only:
> > > 
> > > bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque)
> > > {
> > >     if (vmsd->needed && !vmsd->needed(opaque)) {
> > >         /* optional section not needed */
> > >         return false;
> > >     }
> > >     return true;
> > > }
> > > 
> > > The idea is that all ICPState objects that were created but not associated
> > > to a vCPU by pre-2.10 machine types don't need to be migrated at all, as
> > > their state hasn't changed.
> > > 
> > > We don't even create these unneeded ICPState objects here, but simply
> > > register their ids to vmstate.
> > >   
> > > > > +
> > > > > +static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> > > > > +    .name = "icp/server",
> > > > > +    .version_id = 1,
> > > > > +    .minimum_version_id = 1,
> > > > > +    .needed = pre_2_10_vmstate_dummy_icp_needed,  
> > > 
> > > Outgoing migration:
> > > - machine in older QEMU have unused ICPState objects (default state)
> > > - machine in QEMU 2.10 doesn't even have extra ICPState objects
> > >   
> > > => don't send anything  
> > >   
> > > > > +    .fields = (VMStateField[]) {
> > > > > +        VMSTATE_UNUSED(4), /* uint32_t xirr */
> > > > > +        VMSTATE_UNUSED(1), /* uint8_t pending_priority */
> > > > > +        VMSTATE_UNUSED(1), /* uint8_t mfrr */  
> > > 
> > > Incoming migration from older QEMU: we don't have the extra ICPState objects.
> > >   
> > > => accept the state and discard it  
> > >   
> > > > > +        VMSTATE_END_OF_LIST()
> > > > > +    },
> > > > > +};
> > > > > +
> > > > > +static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr, int i)
> > > > > +{
> > > > > +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > > > > +
> > > > > +    g_assert(!*flag);    
> > > > 
> > > > Apart from this assert(), you never seem to test the values in the
> > > > pre_2_10_ignore_icp() array, so it seems a bit pointless.
> > > >   
> > > 
> > > There's the opposite check in pre_2_10_vmstate_unregister_dummy_icp().
> > > But I agree it isn't really useful... but more because of paranoia :)  
> > 
> > I'm all for paranoid assert()s if they can be made using data readily
> > to hand.  Adding a data structure just for the purpose of making an
> > assert() later, not so much.
> > 
> 
> It is also passed as opaque argument to vmstate_register(), where it is
> used as a key when calling vmstate_unregister(). I could possibly pass
> (void *) i instead, but I'm not a big fan of hijacking pointer arguments
> to pass numbers.

Ah, I see your point.  Creating an array, purely to generate arbitrary
pointers is also kind of ugly, though.  Really the cpu_index / XICS
server number makes sense to identify the vmstate, but it looks like
vmstate_unregister() doesn't take that.

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

Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU

[Greg Kurz]

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

On Tue, 13 Jun 2017 16:06:31 +0800
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Jun 13, 2017 at 09:33:59AM +0200, Greg Kurz wrote:
[...]
> > > > > > +static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr, int i)
> > > > > > +{
> > > > > > +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > > > > > +
> > > > > > +    g_assert(!*flag);      
> > > > > 
> > > > > Apart from this assert(), you never seem to test the values in the
> > > > > pre_2_10_ignore_icp() array, so it seems a bit pointless.
> > > > >     
> > > > 
> > > > There's the opposite check in pre_2_10_vmstate_unregister_dummy_icp().
> > > > But I agree it isn't really useful... but more because of paranoia :)    
> > > 
> > > I'm all for paranoid assert()s if they can be made using data readily
> > > to hand.  Adding a data structure just for the purpose of making an
> > > assert() later, not so much.
> > >   
> > 
> > It is also passed as opaque argument to vmstate_register(), where it is
> > used as a key when calling vmstate_unregister(). I could possibly pass
> > (void *) i instead, but I'm not a big fan of hijacking pointer arguments
> > to pass numbers.  
> 
> Ah, I see your point.  Creating an array, purely to generate arbitrary
> pointers is also kind of ugly, though.  Really the cpu_index / XICS
> server number makes sense to identify the vmstate, but it looks like
> vmstate_unregister() doesn't take that.
> 

Indeed... what about adding a vmstate_unregister_by_instance_id() then ?

Cc'ing Juan and David.

--
Greg

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

Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU

[Dr. David Alan Gilbert]

* Greg Kurz (groug@kaod.org) wrote:
> On Tue, 13 Jun 2017 16:06:31 +0800
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, Jun 13, 2017 at 09:33:59AM +0200, Greg Kurz wrote:
> [...]
> > > > > > > +static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr, int i)
> > > > > > > +{
> > > > > > > +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > > > > > > +
> > > > > > > +    g_assert(!*flag);      
> > > > > > 
> > > > > > Apart from this assert(), you never seem to test the values in the
> > > > > > pre_2_10_ignore_icp() array, so it seems a bit pointless.
> > > > > >     
> > > > > 
> > > > > There's the opposite check in pre_2_10_vmstate_unregister_dummy_icp().
> > > > > But I agree it isn't really useful... but more because of paranoia :)    
> > > > 
> > > > I'm all for paranoid assert()s if they can be made using data readily
> > > > to hand.  Adding a data structure just for the purpose of making an
> > > > assert() later, not so much.
> > > >   
> > > 
> > > It is also passed as opaque argument to vmstate_register(), where it is
> > > used as a key when calling vmstate_unregister(). I could possibly pass
> > > (void *) i instead, but I'm not a big fan of hijacking pointer arguments
> > > to pass numbers.  
> > 
> > Ah, I see your point.  Creating an array, purely to generate arbitrary
> > pointers is also kind of ugly, though.  Really the cpu_index / XICS
> > server number makes sense to identify the vmstate, but it looks like
> > vmstate_unregister() doesn't take that.
> > 
> 
> Indeed... what about adding a vmstate_unregister_by_instance_id() then ?
> 
> Cc'ing Juan and David.

So what's the problem with a (void *)i ? It's simple, as long as you're
not actually using the opaque anywhere it's easy.

Note from a quick glance at your patch;  will this work migrating
from this 2.10 -> 2.9 ?  Are your dummy vmstate's really good enough for
the 2.9 ?

Dave


> --
> Greg


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU

[Greg Kurz]

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

On Tue, 13 Jun 2017 10:00:03 +0100
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Greg Kurz (groug@kaod.org) wrote:
> > On Tue, 13 Jun 2017 16:06:31 +0800
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Tue, Jun 13, 2017 at 09:33:59AM +0200, Greg Kurz wrote:  
> > [...]  
> > > > > > > > +static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr, int i)
> > > > > > > > +{
> > > > > > > > +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > > > > > > > +
> > > > > > > > +    g_assert(!*flag);        
> > > > > > > 
> > > > > > > Apart from this assert(), you never seem to test the values in the
> > > > > > > pre_2_10_ignore_icp() array, so it seems a bit pointless.
> > > > > > >       
> > > > > > 
> > > > > > There's the opposite check in pre_2_10_vmstate_unregister_dummy_icp().
> > > > > > But I agree it isn't really useful... but more because of paranoia :)      
> > > > > 
> > > > > I'm all for paranoid assert()s if they can be made using data readily
> > > > > to hand.  Adding a data structure just for the purpose of making an
> > > > > assert() later, not so much.
> > > > >     
> > > > 
> > > > It is also passed as opaque argument to vmstate_register(), where it is
> > > > used as a key when calling vmstate_unregister(). I could possibly pass
> > > > (void *) i instead, but I'm not a big fan of hijacking pointer arguments
> > > > to pass numbers.    
> > > 
> > > Ah, I see your point.  Creating an array, purely to generate arbitrary
> > > pointers is also kind of ugly, though.  Really the cpu_index / XICS
> > > server number makes sense to identify the vmstate, but it looks like
> > > vmstate_unregister() doesn't take that.
> > >   
> > 
> > Indeed... what about adding a vmstate_unregister_by_instance_id() then ?
> > 
> > Cc'ing Juan and David.  
> 
> So what's the problem with a (void *)i ?

https://stackoverflow.com/questions/8618637/what-does-it-mean-to-convert-int-to-void-or-vice-versa

> It's simple, as long as you're
> not actually using the opaque anywhere it's easy.
> 

but as you say, since the opaque isn't used anywhere, it is probably
okay to pass (void *) i.


> Note from a quick glance at your patch;  will this work migrating
> from this 2.10 -> 2.9 ?  Are your dummy vmstate's really good enough for
> the 2.9 ?
> 

Yeah but I need to add some comments as David suggested.

The idea is that 2.9 used to create a bunch of objects at machine init,
that only get used when CPUs are plugged. With 2.10, these objects are
now created under the CPUs. When migrating from 2.10 to 2.9, we only need
to send the real objects. The dummy vmstate entries don't send anything
(.needed always returns false) since the corresponding objects in 2.9 aren't
being used and still have their default state.

> Dave
> 
> 
> > --
> > Greg  
> 
> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


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

Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU

[Dr. David Alan Gilbert]

* Greg Kurz (groug@kaod.org) wrote:
> On Tue, 13 Jun 2017 10:00:03 +0100
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> > * Greg Kurz (groug@kaod.org) wrote:
> > > On Tue, 13 Jun 2017 16:06:31 +0800
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Tue, Jun 13, 2017 at 09:33:59AM +0200, Greg Kurz wrote:  
> > > [...]  
> > > > > > > > > +static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr, int i)
> > > > > > > > > +{
> > > > > > > > > +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > > > > > > > > +
> > > > > > > > > +    g_assert(!*flag);        
> > > > > > > > 
> > > > > > > > Apart from this assert(), you never seem to test the values in the
> > > > > > > > pre_2_10_ignore_icp() array, so it seems a bit pointless.
> > > > > > > >       
> > > > > > > 
> > > > > > > There's the opposite check in pre_2_10_vmstate_unregister_dummy_icp().
> > > > > > > But I agree it isn't really useful... but more because of paranoia :)      
> > > > > > 
> > > > > > I'm all for paranoid assert()s if they can be made using data readily
> > > > > > to hand.  Adding a data structure just for the purpose of making an
> > > > > > assert() later, not so much.
> > > > > >     
> > > > > 
> > > > > It is also passed as opaque argument to vmstate_register(), where it is
> > > > > used as a key when calling vmstate_unregister(). I could possibly pass
> > > > > (void *) i instead, but I'm not a big fan of hijacking pointer arguments
> > > > > to pass numbers.    
> > > > 
> > > > Ah, I see your point.  Creating an array, purely to generate arbitrary
> > > > pointers is also kind of ugly, though.  Really the cpu_index / XICS
> > > > server number makes sense to identify the vmstate, but it looks like
> > > > vmstate_unregister() doesn't take that.
> > > >   
> > > 
> > > Indeed... what about adding a vmstate_unregister_by_instance_id() then ?
> > > 
> > > Cc'ing Juan and David.  
> > 
> > So what's the problem with a (void *)i ?
> 
> https://stackoverflow.com/questions/8618637/what-does-it-mean-to-convert-int-to-void-or-vice-versa
>
> > It's simple, as long as you're
> > not actually using the opaque anywhere it's easy.
> > 
> 
> but as you say, since the opaque isn't used anywhere, it is probably
> okay to pass (void *) i.

Yes, I don't think we're ever casting back from the (void *) to an int
so it feels pretty safe to me.

> 
> > Note from a quick glance at your patch;  will this work migrating
> > from this 2.10 -> 2.9 ?  Are your dummy vmstate's really good enough for
> > the 2.9 ?
> > 
> 
> Yeah but I need to add some comments as David suggested.
> 
> The idea is that 2.9 used to create a bunch of objects at machine init,
> that only get used when CPUs are plugged. With 2.10, these objects are
> now created under the CPUs. When migrating from 2.10 to 2.9, we only need
> to send the real objects. The dummy vmstate entries don't send anything
> (.needed always returns false) since the corresponding objects in 2.9 aren't
> being used and still have their default state.
> 

OK, that'll probably work.

Dave

> > Dave
> > 
> > 
> > > --
> > > Greg  
> > 
> > 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU

[Greg Kurz]

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

On Tue, 13 Jun 2017 10:55:50 +0100
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Greg Kurz (groug@kaod.org) wrote:
> > On Tue, 13 Jun 2017 10:00:03 +0100
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> >   
> > > * Greg Kurz (groug@kaod.org) wrote:  
> > > > On Tue, 13 Jun 2017 16:06:31 +0800
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >     
> > > > > On Tue, Jun 13, 2017 at 09:33:59AM +0200, Greg Kurz wrote:    
> > > > [...]    
> > > > > > > > > > +static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr, int i)
> > > > > > > > > > +{
> > > > > > > > > > +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > > > > > > > > > +
> > > > > > > > > > +    g_assert(!*flag);          
> > > > > > > > > 
> > > > > > > > > Apart from this assert(), you never seem to test the values in the
> > > > > > > > > pre_2_10_ignore_icp() array, so it seems a bit pointless.
> > > > > > > > >         
> > > > > > > > 
> > > > > > > > There's the opposite check in pre_2_10_vmstate_unregister_dummy_icp().
> > > > > > > > But I agree it isn't really useful... but more because of paranoia :)        
> > > > > > > 
> > > > > > > I'm all for paranoid assert()s if they can be made using data readily
> > > > > > > to hand.  Adding a data structure just for the purpose of making an
> > > > > > > assert() later, not so much.
> > > > > > >       
> > > > > > 
> > > > > > It is also passed as opaque argument to vmstate_register(), where it is
> > > > > > used as a key when calling vmstate_unregister(). I could possibly pass
> > > > > > (void *) i instead, but I'm not a big fan of hijacking pointer arguments
> > > > > > to pass numbers.      
> > > > > 
> > > > > Ah, I see your point.  Creating an array, purely to generate arbitrary
> > > > > pointers is also kind of ugly, though.  Really the cpu_index / XICS
> > > > > server number makes sense to identify the vmstate, but it looks like
> > > > > vmstate_unregister() doesn't take that.
> > > > >     
> > > > 
> > > > Indeed... what about adding a vmstate_unregister_by_instance_id() then ?
> > > > 
> > > > Cc'ing Juan and David.    
> > > 
> > > So what's the problem with a (void *)i ?  
> > 
> > https://stackoverflow.com/questions/8618637/what-does-it-mean-to-convert-int-to-void-or-vice-versa
> >  
> > > It's simple, as long as you're
> > > not actually using the opaque anywhere it's easy.
> > >   
> > 
> > but as you say, since the opaque isn't used anywhere, it is probably
> > okay to pass (void *) i.  
> 
> Yes, I don't think we're ever casting back from the (void *) to an int
> so it feels pretty safe to me.
> 

Just one nit, gcc doesn't like it on 64-bit hosts:

hw/ppc/spapr.c: In function ‘pre_2_10_vmstate_register_dummy_icp’:
hw/ppc/spapr.c:148:60: error: cast to pointer from integer of different
 size [-Werror=int-to-pointer-cast]
     vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp, (void *) i);

I need to cast to (long) first.

> >   
> > > Note from a quick glance at your patch;  will this work migrating
> > > from this 2.10 -> 2.9 ?  Are your dummy vmstate's really good enough for
> > > the 2.9 ?
> > >   
> > 
> > Yeah but I need to add some comments as David suggested.
> > 
> > The idea is that 2.9 used to create a bunch of objects at machine init,
> > that only get used when CPUs are plugged. With 2.10, these objects are
> > now created under the CPUs. When migrating from 2.10 to 2.9, we only need
> > to send the real objects. The dummy vmstate entries don't send anything
> > (.needed always returns false) since the corresponding objects in 2.9 aren't
> > being used and still have their default state.
> >   
> 
> OK, that'll probably work.
> 

Thanks for this confirmation.

> Dave
> 
> > > Dave
> > > 
> > >   
> > > > --
> > > > Greg    
> > > 
> > > 
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK  
> >   
> 
> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


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

Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU

[Dr. David Alan Gilbert]

* Greg Kurz (groug@kaod.org) wrote:
> On Tue, 13 Jun 2017 10:55:50 +0100
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> > * Greg Kurz (groug@kaod.org) wrote:
> > > On Tue, 13 Jun 2017 10:00:03 +0100
> > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > >   
> > > > * Greg Kurz (groug@kaod.org) wrote:  
> > > > > On Tue, 13 Jun 2017 16:06:31 +0800
> > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > >     
> > > > > > On Tue, Jun 13, 2017 at 09:33:59AM +0200, Greg Kurz wrote:    
> > > > > [...]    
> > > > > > > > > > > +static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr, int i)
> > > > > > > > > > > +{
> > > > > > > > > > > +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > > > > > > > > > > +
> > > > > > > > > > > +    g_assert(!*flag);          
> > > > > > > > > > 
> > > > > > > > > > Apart from this assert(), you never seem to test the values in the
> > > > > > > > > > pre_2_10_ignore_icp() array, so it seems a bit pointless.
> > > > > > > > > >         
> > > > > > > > > 
> > > > > > > > > There's the opposite check in pre_2_10_vmstate_unregister_dummy_icp().
> > > > > > > > > But I agree it isn't really useful... but more because of paranoia :)        
> > > > > > > > 
> > > > > > > > I'm all for paranoid assert()s if they can be made using data readily
> > > > > > > > to hand.  Adding a data structure just for the purpose of making an
> > > > > > > > assert() later, not so much.
> > > > > > > >       
> > > > > > > 
> > > > > > > It is also passed as opaque argument to vmstate_register(), where it is
> > > > > > > used as a key when calling vmstate_unregister(). I could possibly pass
> > > > > > > (void *) i instead, but I'm not a big fan of hijacking pointer arguments
> > > > > > > to pass numbers.      
> > > > > > 
> > > > > > Ah, I see your point.  Creating an array, purely to generate arbitrary
> > > > > > pointers is also kind of ugly, though.  Really the cpu_index / XICS
> > > > > > server number makes sense to identify the vmstate, but it looks like
> > > > > > vmstate_unregister() doesn't take that.
> > > > > >     
> > > > > 
> > > > > Indeed... what about adding a vmstate_unregister_by_instance_id() then ?
> > > > > 
> > > > > Cc'ing Juan and David.    
> > > > 
> > > > So what's the problem with a (void *)i ?  
> > > 
> > > https://stackoverflow.com/questions/8618637/what-does-it-mean-to-convert-int-to-void-or-vice-versa
> > >  
> > > > It's simple, as long as you're
> > > > not actually using the opaque anywhere it's easy.
> > > >   
> > > 
> > > but as you say, since the opaque isn't used anywhere, it is probably
> > > okay to pass (void *) i.  
> > 
> > Yes, I don't think we're ever casting back from the (void *) to an int
> > so it feels pretty safe to me.
> > 
> 
> Just one nit, gcc doesn't like it on 64-bit hosts:
> 
> hw/ppc/spapr.c: In function ‘pre_2_10_vmstate_register_dummy_icp’:
> hw/ppc/spapr.c:148:60: error: cast to pointer from integer of different
>  size [-Werror=int-to-pointer-cast]
>      vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp, (void *) i);
> 
> I need to cast to (long) first.

Try casting to a uintptr_t first.

Dave

> > >   
> > > > Note from a quick glance at your patch;  will this work migrating
> > > > from this 2.10 -> 2.9 ?  Are your dummy vmstate's really good enough for
> > > > the 2.9 ?
> > > >   
> > > 
> > > Yeah but I need to add some comments as David suggested.
> > > 
> > > The idea is that 2.9 used to create a bunch of objects at machine init,
> > > that only get used when CPUs are plugged. With 2.10, these objects are
> > > now created under the CPUs. When migrating from 2.10 to 2.9, we only need
> > > to send the real objects. The dummy vmstate entries don't send anything
> > > (.needed always returns false) since the corresponding objects in 2.9 aren't
> > > being used and still have their default state.
> > >   
> > 
> > OK, that'll probably work.
> > 
> 
> Thanks for this confirmation.
> 
> > Dave
> > 
> > > > Dave
> > > > 
> > > >   
> > > > > --
> > > > > Greg    
> > > > 
> > > > 
> > > > --
> > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK  
> > >   
> > 
> > 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU

[Greg Kurz]

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

On Tue, 13 Jun 2017 11:12:17 +0100
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
[...]
> > > > > 
> > > > > So what's the problem with a (void *)i ?    
> > > > 
> > > > https://stackoverflow.com/questions/8618637/what-does-it-mean-to-convert-int-to-void-or-vice-versa
> > > >    
> > > > > It's simple, as long as you're
> > > > > not actually using the opaque anywhere it's easy.
> > > > >     
> > > > 
> > > > but as you say, since the opaque isn't used anywhere, it is probably
> > > > okay to pass (void *) i.    
> > > 
> > > Yes, I don't think we're ever casting back from the (void *) to an int
> > > so it feels pretty safe to me.
> > >   
> > 
> > Just one nit, gcc doesn't like it on 64-bit hosts:
> > 
> > hw/ppc/spapr.c: In function ‘pre_2_10_vmstate_register_dummy_icp’:
> > hw/ppc/spapr.c:148:60: error: cast to pointer from integer of different
> >  size [-Werror=int-to-pointer-cast]
> >      vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp, (void *) i);
> > 
> > I need to cast to (long) first.  
> 
> Try casting to a uintptr_t first.
> 

Yeah, you're right. The standard doesn't guarantees whether pointers
have the size of int or long. I'll use intptr_t.

Thanks for the suggestion.

> Dave
> 
> > > >     
> > > > > Note from a quick glance at your patch;  will this work migrating
> > > > > from this 2.10 -> 2.9 ?  Are your dummy vmstate's really good enough for
> > > > > the 2.9 ?
> > > > >     
> > > > 
> > > > Yeah but I need to add some comments as David suggested.
> > > > 
> > > > The idea is that 2.9 used to create a bunch of objects at machine init,
> > > > that only get used when CPUs are plugged. With 2.10, these objects are
> > > > now created under the CPUs. When migrating from 2.10 to 2.9, we only need
> > > > to send the real objects. The dummy vmstate entries don't send anything
> > > > (.needed always returns false) since the corresponding objects in 2.9 aren't
> > > > being used and still have their default state.
> > > >     
> > > 
> > > OK, that'll probably work.
> > >   
> > 
> > Thanks for this confirmation.
> >   
> > > Dave
> > >   
> > > > > Dave
> > > > > 
> > > > >     
> > > > > > --
> > > > > > Greg      
> > > > > 
> > > > > 
> > > > > --
> > > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK    
> > > >     
> > > 
> > > 
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK  
> >   
> 
> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


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

Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU

[David Gibson]

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

On Tue, Jun 13, 2017 at 12:05:52PM +0200, Greg Kurz wrote:
> On Tue, 13 Jun 2017 10:55:50 +0100
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> > * Greg Kurz (groug@kaod.org) wrote:
> > > On Tue, 13 Jun 2017 10:00:03 +0100
> > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > >   
> > > > * Greg Kurz (groug@kaod.org) wrote:  
> > > > > On Tue, 13 Jun 2017 16:06:31 +0800
> > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > >     
> > > > > > On Tue, Jun 13, 2017 at 09:33:59AM +0200, Greg Kurz wrote:    
> > > > > [...]    
> > > > > > > > > > > +static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr, int i)
> > > > > > > > > > > +{
> > > > > > > > > > > +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > > > > > > > > > > +
> > > > > > > > > > > +    g_assert(!*flag);          
> > > > > > > > > > 
> > > > > > > > > > Apart from this assert(), you never seem to test the values in the
> > > > > > > > > > pre_2_10_ignore_icp() array, so it seems a bit pointless.
> > > > > > > > > >         
> > > > > > > > > 
> > > > > > > > > There's the opposite check in pre_2_10_vmstate_unregister_dummy_icp().
> > > > > > > > > But I agree it isn't really useful... but more because of paranoia :)        
> > > > > > > > 
> > > > > > > > I'm all for paranoid assert()s if they can be made using data readily
> > > > > > > > to hand.  Adding a data structure just for the purpose of making an
> > > > > > > > assert() later, not so much.
> > > > > > > >       
> > > > > > > 
> > > > > > > It is also passed as opaque argument to vmstate_register(), where it is
> > > > > > > used as a key when calling vmstate_unregister(). I could possibly pass
> > > > > > > (void *) i instead, but I'm not a big fan of hijacking pointer arguments
> > > > > > > to pass numbers.      
> > > > > > 
> > > > > > Ah, I see your point.  Creating an array, purely to generate arbitrary
> > > > > > pointers is also kind of ugly, though.  Really the cpu_index / XICS
> > > > > > server number makes sense to identify the vmstate, but it looks like
> > > > > > vmstate_unregister() doesn't take that.
> > > > > >     
> > > > > 
> > > > > Indeed... what about adding a vmstate_unregister_by_instance_id() then ?
> > > > > 
> > > > > Cc'ing Juan and David.    
> > > > 
> > > > So what's the problem with a (void *)i ?  
> > > 
> > > https://stackoverflow.com/questions/8618637/what-does-it-mean-to-convert-int-to-void-or-vice-versa
> > >  
> > > > It's simple, as long as you're
> > > > not actually using the opaque anywhere it's easy.
> > > >   
> > > 
> > > but as you say, since the opaque isn't used anywhere, it is probably
> > > okay to pass (void *) i.  
> > 
> > Yes, I don't think we're ever casting back from the (void *) to an int
> > so it feels pretty safe to me.
> > 
> 
> Just one nit, gcc doesn't like it on 64-bit hosts:
> 
> hw/ppc/spapr.c: In function ‘pre_2_10_vmstate_register_dummy_icp’:
> hw/ppc/spapr.c:148:60: error: cast to pointer from integer of different
>  size [-Werror=int-to-pointer-cast]
>      vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp, (void *) i);
> 
> I need to cast to (long) first.

No, you need to cast to uintptr_t first.

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

Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU

[David Gibson]

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

On Tue, Jun 13, 2017 at 11:21:50AM +0200, Greg Kurz wrote:
> On Tue, 13 Jun 2017 10:00:03 +0100
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> > * Greg Kurz (groug@kaod.org) wrote:
> > > On Tue, 13 Jun 2017 16:06:31 +0800
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Tue, Jun 13, 2017 at 09:33:59AM +0200, Greg Kurz wrote:  
> > > [...]  
> > > > > > > > > +static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr, int i)
> > > > > > > > > +{
> > > > > > > > > +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > > > > > > > > +
> > > > > > > > > +    g_assert(!*flag);        
> > > > > > > > 
> > > > > > > > Apart from this assert(), you never seem to test the values in the
> > > > > > > > pre_2_10_ignore_icp() array, so it seems a bit pointless.
> > > > > > > >       
> > > > > > > 
> > > > > > > There's the opposite check in pre_2_10_vmstate_unregister_dummy_icp().
> > > > > > > But I agree it isn't really useful... but more because of paranoia :)      
> > > > > > 
> > > > > > I'm all for paranoid assert()s if they can be made using data readily
> > > > > > to hand.  Adding a data structure just for the purpose of making an
> > > > > > assert() later, not so much.
> > > > > >     
> > > > > 
> > > > > It is also passed as opaque argument to vmstate_register(), where it is
> > > > > used as a key when calling vmstate_unregister(). I could possibly pass
> > > > > (void *) i instead, but I'm not a big fan of hijacking pointer arguments
> > > > > to pass numbers.    
> > > > 
> > > > Ah, I see your point.  Creating an array, purely to generate arbitrary
> > > > pointers is also kind of ugly, though.  Really the cpu_index / XICS
> > > > server number makes sense to identify the vmstate, but it looks like
> > > > vmstate_unregister() doesn't take that.
> > > >   
> > > 
> > > Indeed... what about adding a vmstate_unregister_by_instance_id() then ?
> > > 
> > > Cc'ing Juan and David.  
> > 
> > So what's the problem with a (void *)i ?
> 
> https://stackoverflow.com/questions/8618637/what-does-it-mean-to-convert-int-to-void-or-vice-versa
> 
> > It's simple, as long as you're
> > not actually using the opaque anywhere it's easy.
> > 
> 
> but as you say, since the opaque isn't used anywhere, it is probably
> okay to pass (void *) i.

Right, that's probably the right short term approach.


Incidentally, for a somewhat safer-than-standard approach to this
common problem, see https://ccodearchive.net/info/ptrint.html

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

Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU

[Greg Kurz]

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

On Tue, 13 Jun 2017 18:01:46 +0800
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Jun 13, 2017 at 11:21:50AM +0200, Greg Kurz wrote:
> > On Tue, 13 Jun 2017 10:00:03 +0100
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> >   
> > > * Greg Kurz (groug@kaod.org) wrote:  
> > > > On Tue, 13 Jun 2017 16:06:31 +0800
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >     
> > > > > On Tue, Jun 13, 2017 at 09:33:59AM +0200, Greg Kurz wrote:    
> > > > [...]    
> > > > > > > > > > +static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr, int i)
> > > > > > > > > > +{
> > > > > > > > > > +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > > > > > > > > > +
> > > > > > > > > > +    g_assert(!*flag);          
> > > > > > > > > 
> > > > > > > > > Apart from this assert(), you never seem to test the values in the
> > > > > > > > > pre_2_10_ignore_icp() array, so it seems a bit pointless.
> > > > > > > > >         
> > > > > > > > 
> > > > > > > > There's the opposite check in pre_2_10_vmstate_unregister_dummy_icp().
> > > > > > > > But I agree it isn't really useful... but more because of paranoia :)        
> > > > > > > 
> > > > > > > I'm all for paranoid assert()s if they can be made using data readily
> > > > > > > to hand.  Adding a data structure just for the purpose of making an
> > > > > > > assert() later, not so much.
> > > > > > >       
> > > > > > 
> > > > > > It is also passed as opaque argument to vmstate_register(), where it is
> > > > > > used as a key when calling vmstate_unregister(). I could possibly pass
> > > > > > (void *) i instead, but I'm not a big fan of hijacking pointer arguments
> > > > > > to pass numbers.      
> > > > > 
> > > > > Ah, I see your point.  Creating an array, purely to generate arbitrary
> > > > > pointers is also kind of ugly, though.  Really the cpu_index / XICS
> > > > > server number makes sense to identify the vmstate, but it looks like
> > > > > vmstate_unregister() doesn't take that.
> > > > >     
> > > > 
> > > > Indeed... what about adding a vmstate_unregister_by_instance_id() then ?
> > > > 
> > > > Cc'ing Juan and David.    
> > > 
> > > So what's the problem with a (void *)i ?  
> > 
> > https://stackoverflow.com/questions/8618637/what-does-it-mean-to-convert-int-to-void-or-vice-versa
> >   
> > > It's simple, as long as you're
> > > not actually using the opaque anywhere it's easy.
> > >   
> > 
> > but as you say, since the opaque isn't used anywhere, it is probably
> > okay to pass (void *) i.  
> 
> Right, that's probably the right short term approach.
> 

I'm currently heading towards (void *)(intptr_t) i

> 
> Incidentally, for a somewhat safer-than-standard approach to this
> common problem, see https://ccodearchive.net/info/ptrint.html
> 

Heh, your code :)

IIUC it is licensed under the terms of CC0, which I believe to be GPL
compatible. Why not using it in QEMU ?

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

Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU

[David Gibson]

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

On Tue, Jun 13, 2017 at 05:24:28PM +0200, Greg Kurz wrote:
> On Tue, 13 Jun 2017 18:01:46 +0800
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, Jun 13, 2017 at 11:21:50AM +0200, Greg Kurz wrote:
> > > On Tue, 13 Jun 2017 10:00:03 +0100
> > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > >   
> > > > * Greg Kurz (groug@kaod.org) wrote:  
> > > > > On Tue, 13 Jun 2017 16:06:31 +0800
> > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > >     
> > > > > > On Tue, Jun 13, 2017 at 09:33:59AM +0200, Greg Kurz wrote:    
> > > > > [...]    
> > > > > > > > > > > +static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr, int i)
> > > > > > > > > > > +{
> > > > > > > > > > > +    bool *flag = &spapr->pre_2_10_ignore_icp[i];
> > > > > > > > > > > +
> > > > > > > > > > > +    g_assert(!*flag);          
> > > > > > > > > > 
> > > > > > > > > > Apart from this assert(), you never seem to test the values in the
> > > > > > > > > > pre_2_10_ignore_icp() array, so it seems a bit pointless.
> > > > > > > > > >         
> > > > > > > > > 
> > > > > > > > > There's the opposite check in pre_2_10_vmstate_unregister_dummy_icp().
> > > > > > > > > But I agree it isn't really useful... but more because of paranoia :)        
> > > > > > > > 
> > > > > > > > I'm all for paranoid assert()s if they can be made using data readily
> > > > > > > > to hand.  Adding a data structure just for the purpose of making an
> > > > > > > > assert() later, not so much.
> > > > > > > >       
> > > > > > > 
> > > > > > > It is also passed as opaque argument to vmstate_register(), where it is
> > > > > > > used as a key when calling vmstate_unregister(). I could possibly pass
> > > > > > > (void *) i instead, but I'm not a big fan of hijacking pointer arguments
> > > > > > > to pass numbers.      
> > > > > > 
> > > > > > Ah, I see your point.  Creating an array, purely to generate arbitrary
> > > > > > pointers is also kind of ugly, though.  Really the cpu_index / XICS
> > > > > > server number makes sense to identify the vmstate, but it looks like
> > > > > > vmstate_unregister() doesn't take that.
> > > > > >     
> > > > > 
> > > > > Indeed... what about adding a vmstate_unregister_by_instance_id() then ?
> > > > > 
> > > > > Cc'ing Juan and David.    
> > > > 
> > > > So what's the problem with a (void *)i ?  
> > > 
> > > https://stackoverflow.com/questions/8618637/what-does-it-mean-to-convert-int-to-void-or-vice-versa
> > >   
> > > > It's simple, as long as you're
> > > > not actually using the opaque anywhere it's easy.
> > > >   
> > > 
> > > but as you say, since the opaque isn't used anywhere, it is probably
> > > okay to pass (void *) i.  
> > 
> > Right, that's probably the right short term approach.
> > 
> 
> I'm currently heading towards (void *)(intptr_t) i
> 
> > 
> > Incidentally, for a somewhat safer-than-standard approach to this
> > common problem, see https://ccodearchive.net/info/ptrint.html
> > 
> 
> Heh, your code :)
> 
> IIUC it is licensed under the terms of CC0, which I believe to be GPL
> compatible. Why not using it in QEMU ?

Well, we could.  I just haven't bothered to import it and convince
people to use it.

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