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

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

[Greg Kurz]

Recent work on XICS broke migration of older machine types. The target
fails with:

qemu-system-ppc64: Unknown savevm section or instance 'icp/server' 1
qemu-system-ppc64: load of migration failed: Invalid argument

This happens because the current code no longer pre-allocates ICP objects
at machine init time.

This series restore the previous behavior for older machine types (patch 6).
Since the fix adds an error path to cover the case when realization of the
ICP objects fails, the series first does some cleanup on the way errors are
handled (patch 1-5).

I could successfully migrate older machines back and forth with QEMU 2.9:
- pseries-2.9
- pseries-2.9 with hotplugged CPUs
- pseries-2.6 (pre CPU hotplug support)

This series is based on:

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

--
Greg

---

Greg Kurz (6):
      ppc/xics: simplify prototype of xics_spapr_init()
      spapr: fix error path of required kernel-irqchip
      spapr: fix error reporting in xics_system_init()
      spapr: sanitize error handling in spapr_ics_create()
      spapr-cpu-core: release ICP object when realization fails
      spapr: fix migration of ICP objects from/to older QEMU


 hw/intc/xics_spapr.c    |    3 +-
 hw/ppc/spapr.c          |   75 ++++++++++++++++++++++++++++++++++++++---------
 hw/ppc/spapr_cpu_core.c |   40 ++++++++++++++++---------
 include/hw/ppc/spapr.h  |    2 +
 include/hw/ppc/xics.h   |    2 +
 5 files changed, 90 insertions(+), 32 deletions(-)

[Qemu-devel] [PATCH 1/6] ppc/xics: simplify prototype of xics_spapr_init()

[Greg Kurz]

This function only does hypercall and RTAS-call registration, and thus
never returns an error. This patch adapt the prototype to reflect that.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/xics_spapr.c  |    3 +--
 hw/ppc/spapr.c        |    2 +-
 include/hw/ppc/xics.h |    2 +-
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index f05308b897f2..d98ea8b13068 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -229,7 +229,7 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     rtas_st(rets, 0, RTAS_OUT_SUCCESS);
 }
 
-int xics_spapr_init(sPAPRMachineState *spapr, Error **errp)
+void xics_spapr_init(sPAPRMachineState *spapr)
 {
     /* Registration of global state belongs into realize */
     spapr_rtas_register(RTAS_IBM_SET_XIVE, "ibm,set-xive", rtas_set_xive);
@@ -243,7 +243,6 @@ int xics_spapr_init(sPAPRMachineState *spapr, Error **errp)
     spapr_register_hypercall(H_XIRR_X, h_xirr_x);
     spapr_register_hypercall(H_EOI, h_eoi);
     spapr_register_hypercall(H_IPOLL, h_ipoll);
-    return 0;
 }
 
 #define ICS_IRQ_FREE(ics, srcno)   \
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 1b7cadab0cdf..abfb99b71b7d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -139,7 +139,7 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
     }
 
     if (!spapr->ics) {
-        xics_spapr_init(spapr, errp);
+        xics_spapr_init(spapr);
         spapr->icp_type = TYPE_ICP;
         spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
     }
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 05e6acbb3533..d6cb51f3ad5d 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -206,6 +206,6 @@ void icp_resend(ICPState *ss);
 typedef struct sPAPRMachineState sPAPRMachineState;
 
 int xics_kvm_init(sPAPRMachineState *spapr, Error **errp);
-int xics_spapr_init(sPAPRMachineState *spapr, Error **errp);
+void xics_spapr_init(sPAPRMachineState *spapr);
 
 #endif /* XICS_H */

Re: [Qemu-devel] [PATCH 1/6] ppc/xics: simplify prototype of xics_spapr_init()

[Cédric Le Goater]

On 05/15/2017 01:39 PM, Greg Kurz wrote:
> This function only does hypercall and RTAS-call registration, and thus
> never returns an error. This patch adapt the prototype to reflect that.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

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

> ---
>  hw/intc/xics_spapr.c  |    3 +--
>  hw/ppc/spapr.c        |    2 +-
>  include/hw/ppc/xics.h |    2 +-
>  3 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index f05308b897f2..d98ea8b13068 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -229,7 +229,7 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>  }
>  
> -int xics_spapr_init(sPAPRMachineState *spapr, Error **errp)
> +void xics_spapr_init(sPAPRMachineState *spapr)
>  {
>      /* Registration of global state belongs into realize */
>      spapr_rtas_register(RTAS_IBM_SET_XIVE, "ibm,set-xive", rtas_set_xive);
> @@ -243,7 +243,6 @@ int xics_spapr_init(sPAPRMachineState *spapr, Error **errp)
>      spapr_register_hypercall(H_XIRR_X, h_xirr_x);
>      spapr_register_hypercall(H_EOI, h_eoi);
>      spapr_register_hypercall(H_IPOLL, h_ipoll);
> -    return 0;
>  }
>  
>  #define ICS_IRQ_FREE(ics, srcno)   \
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 1b7cadab0cdf..abfb99b71b7d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -139,7 +139,7 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
>      }
>  
>      if (!spapr->ics) {
> -        xics_spapr_init(spapr, errp);
> +        xics_spapr_init(spapr);
>          spapr->icp_type = TYPE_ICP;
>          spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
>      }
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 05e6acbb3533..d6cb51f3ad5d 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -206,6 +206,6 @@ void icp_resend(ICPState *ss);
>  typedef struct sPAPRMachineState sPAPRMachineState;
>  
>  int xics_kvm_init(sPAPRMachineState *spapr, Error **errp);
> -int xics_spapr_init(sPAPRMachineState *spapr, Error **errp);
> +void xics_spapr_init(sPAPRMachineState *spapr);
>  
>  #endif /* XICS_H */
> 

Re: [Qemu-devel] [PATCH 1/6] ppc/xics: simplify prototype of xics_spapr_init()

[Philippe Mathieu-Daudé]

On 05/15/2017 08:39 AM, Greg Kurz wrote:
> This function only does hypercall and RTAS-call registration, and thus
> never returns an error. This patch adapt the prototype to reflect that.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/intc/xics_spapr.c  |    3 +--
>  hw/ppc/spapr.c        |    2 +-
>  include/hw/ppc/xics.h |    2 +-
>  3 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index f05308b897f2..d98ea8b13068 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -229,7 +229,7 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>  }
>
> -int xics_spapr_init(sPAPRMachineState *spapr, Error **errp)
> +void xics_spapr_init(sPAPRMachineState *spapr)
>  {
>      /* Registration of global state belongs into realize */
>      spapr_rtas_register(RTAS_IBM_SET_XIVE, "ibm,set-xive", rtas_set_xive);
> @@ -243,7 +243,6 @@ int xics_spapr_init(sPAPRMachineState *spapr, Error **errp)
>      spapr_register_hypercall(H_XIRR_X, h_xirr_x);
>      spapr_register_hypercall(H_EOI, h_eoi);
>      spapr_register_hypercall(H_IPOLL, h_ipoll);
> -    return 0;
>  }
>
>  #define ICS_IRQ_FREE(ics, srcno)   \
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 1b7cadab0cdf..abfb99b71b7d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -139,7 +139,7 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
>      }
>
>      if (!spapr->ics) {
> -        xics_spapr_init(spapr, errp);
> +        xics_spapr_init(spapr);
>          spapr->icp_type = TYPE_ICP;
>          spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
>      }
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 05e6acbb3533..d6cb51f3ad5d 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -206,6 +206,6 @@ void icp_resend(ICPState *ss);
>  typedef struct sPAPRMachineState sPAPRMachineState;
>
>  int xics_kvm_init(sPAPRMachineState *spapr, Error **errp);
> -int xics_spapr_init(sPAPRMachineState *spapr, Error **errp);
> +void xics_spapr_init(sPAPRMachineState *spapr);
>
>  #endif /* XICS_H */
>
>

Re: [Qemu-devel] [PATCH 1/6] ppc/xics: simplify prototype of xics_spapr_init()

[David Gibson]

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

On Mon, May 15, 2017 at 01:39:16PM +0200, Greg Kurz wrote:
> This function only does hypercall and RTAS-call registration, and thus
> never returns an error. This patch adapt the prototype to reflect that.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-2.10, thanks.

> ---
>  hw/intc/xics_spapr.c  |    3 +--
>  hw/ppc/spapr.c        |    2 +-
>  include/hw/ppc/xics.h |    2 +-
>  3 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index f05308b897f2..d98ea8b13068 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -229,7 +229,7 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>  }
>  
> -int xics_spapr_init(sPAPRMachineState *spapr, Error **errp)
> +void xics_spapr_init(sPAPRMachineState *spapr)
>  {
>      /* Registration of global state belongs into realize */
>      spapr_rtas_register(RTAS_IBM_SET_XIVE, "ibm,set-xive", rtas_set_xive);
> @@ -243,7 +243,6 @@ int xics_spapr_init(sPAPRMachineState *spapr, Error **errp)
>      spapr_register_hypercall(H_XIRR_X, h_xirr_x);
>      spapr_register_hypercall(H_EOI, h_eoi);
>      spapr_register_hypercall(H_IPOLL, h_ipoll);
> -    return 0;
>  }
>  
>  #define ICS_IRQ_FREE(ics, srcno)   \
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 1b7cadab0cdf..abfb99b71b7d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -139,7 +139,7 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
>      }
>  
>      if (!spapr->ics) {
> -        xics_spapr_init(spapr, errp);
> +        xics_spapr_init(spapr);
>          spapr->icp_type = TYPE_ICP;
>          spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
>      }
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 05e6acbb3533..d6cb51f3ad5d 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -206,6 +206,6 @@ void icp_resend(ICPState *ss);
>  typedef struct sPAPRMachineState sPAPRMachineState;
>  
>  int xics_kvm_init(sPAPRMachineState *spapr, Error **errp);
> -int xics_spapr_init(sPAPRMachineState *spapr, Error **errp);
> +void xics_spapr_init(sPAPRMachineState *spapr);
>  
>  #endif /* XICS_H */
> 

-- 
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 2/6] spapr: fix error path of required kernel-irqchip

[Greg Kurz]

QEMU should exit if the user explicitely asked for kernel-irqchip support
and "xics-kvm" initialization fails.

The changelog of commit 34f2af3d3edf ("spapr: Clean up misuse of qdev_init()
in xics-kvm creation") reads:

    While there, improve the error message when we can't satisfy an
    explicit user request for "xics-kvm", and exit(1) instead of abort().
    Simplify the abort when we can't create "xics".

This patch adds the missing call to exit().

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index abfb99b71b7d..f477d7b8a210 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -133,6 +133,7 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
         if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
             error_reportf_err(err,
                               "kernel_irqchip requested but unavailable: ");
+            exit(EXIT_FAILURE);
         } else {
             error_free(err);
         }

Re: [Qemu-devel] [PATCH 2/6] spapr: fix error path of required kernel-irqchip

[Cédric Le Goater]

On 05/15/2017 01:39 PM, Greg Kurz wrote:
> QEMU should exit if the user explicitely asked for kernel-irqchip support
> and "xics-kvm" initialization fails.
> 
> The changelog of commit 34f2af3d3edf ("spapr: Clean up misuse of qdev_init()
> in xics-kvm creation") reads:
> 
>     While there, improve the error message when we can't satisfy an
>     explicit user request for "xics-kvm", and exit(1) instead of abort().
>     Simplify the abort when we can't create "xics".
> 
> This patch adds the missing call to exit().
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>


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

> ---
>  hw/ppc/spapr.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index abfb99b71b7d..f477d7b8a210 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -133,6 +133,7 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
>          if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
>              error_reportf_err(err,
>                                "kernel_irqchip requested but unavailable: ");
> +            exit(EXIT_FAILURE);
>          } else {
>              error_free(err);
>          }
> 

Re: [Qemu-devel] [PATCH 2/6] spapr: fix error path of required kernel-irqchip

[David Gibson]

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

On Mon, May 15, 2017 at 01:39:26PM +0200, Greg Kurz wrote:
> QEMU should exit if the user explicitely asked for kernel-irqchip support
> and "xics-kvm" initialization fails.
> 
> The changelog of commit 34f2af3d3edf ("spapr: Clean up misuse of qdev_init()
> in xics-kvm creation") reads:
> 
>     While there, improve the error message when we can't satisfy an
>     explicit user request for "xics-kvm", and exit(1) instead of abort().
>     Simplify the abort when we can't create "xics".
> 
> This patch adds the missing call to exit().
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index abfb99b71b7d..f477d7b8a210 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -133,6 +133,7 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
>          if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
>              error_reportf_err(err,
>                                "kernel_irqchip requested but unavailable: ");
> +            exit(EXIT_FAILURE);
>          } else {
>              error_free(err);
>          }
> 


This doesn't look right.  We have an errp pointer in the caller.  So
on failure we should error_propagate(), rather than deciding for
ourselves that exiting is the right course of action.

-- 
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 2/6] spapr: fix error path of required kernel-irqchip

[Greg Kurz]

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

On Tue, 16 May 2017 14:35:55 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, May 15, 2017 at 01:39:26PM +0200, Greg Kurz wrote:
> > QEMU should exit if the user explicitely asked for kernel-irqchip support
> > and "xics-kvm" initialization fails.
> > 
> > The changelog of commit 34f2af3d3edf ("spapr: Clean up misuse of qdev_init()
> > in xics-kvm creation") reads:
> > 
> >     While there, improve the error message when we can't satisfy an
> >     explicit user request for "xics-kvm", and exit(1) instead of abort().
> >     Simplify the abort when we can't create "xics".
> > 
> > This patch adds the missing call to exit().
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/ppc/spapr.c |    1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index abfb99b71b7d..f477d7b8a210 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -133,6 +133,7 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> >          if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> >              error_reportf_err(err,
> >                                "kernel_irqchip requested but unavailable: ");
> > +            exit(EXIT_FAILURE);
> >          } else {
> >              error_free(err);
> >          }
> >   
> 
> 
> This doesn't look right.  We have an errp pointer in the caller.  So
> on failure we should error_propagate(), rather than deciding for
> ourselves that exiting is the right course of action.
> 

I generally agree with that but can the caller cope with the fact that
the user passed -machine accel=kvm,kernel_irqchip=on and this cannot
be satisfied ?

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

Re: [Qemu-devel] [PATCH 2/6] spapr: fix error path of required kernel-irqchip

[David Gibson]

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

On Tue, May 16, 2017 at 07:56:05AM +0200, Greg Kurz wrote:
> On Tue, 16 May 2017 14:35:55 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Mon, May 15, 2017 at 01:39:26PM +0200, Greg Kurz wrote:
> > > QEMU should exit if the user explicitely asked for kernel-irqchip support
> > > and "xics-kvm" initialization fails.
> > > 
> > > The changelog of commit 34f2af3d3edf ("spapr: Clean up misuse of qdev_init()
> > > in xics-kvm creation") reads:
> > > 
> > >     While there, improve the error message when we can't satisfy an
> > >     explicit user request for "xics-kvm", and exit(1) instead of abort().
> > >     Simplify the abort when we can't create "xics".
> > > 
> > > This patch adds the missing call to exit().
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > >  hw/ppc/spapr.c |    1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index abfb99b71b7d..f477d7b8a210 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -133,6 +133,7 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> > >          if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> > >              error_reportf_err(err,
> > >                                "kernel_irqchip requested but unavailable: ");
> > > +            exit(EXIT_FAILURE);
> > >          } else {
> > >              error_free(err);
> > >          }
> > >   
> > 
> > 
> > This doesn't look right.  We have an errp pointer in the caller.  So
> > on failure we should error_propagate(), rather than deciding for
> > ourselves that exiting is the right course of action.
> > 
> 
> I generally agree with that but can the caller cope with the fact that
> the user passed -machine accel=kvm,kernel_irqchip=on and this cannot
> be satisfied ?

AFAICT the only caller passes &error_fatal, so, yes..

-- 
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 2/6] spapr: fix error path of required kernel-irqchip

[Greg Kurz]

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

On Tue, 16 May 2017 16:08:46 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, May 16, 2017 at 07:56:05AM +0200, Greg Kurz wrote:
> > On Tue, 16 May 2017 14:35:55 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Mon, May 15, 2017 at 01:39:26PM +0200, Greg Kurz wrote:  
> > > > QEMU should exit if the user explicitely asked for kernel-irqchip support
> > > > and "xics-kvm" initialization fails.
> > > > 
> > > > The changelog of commit 34f2af3d3edf ("spapr: Clean up misuse of qdev_init()
> > > > in xics-kvm creation") reads:
> > > > 
> > > >     While there, improve the error message when we can't satisfy an
> > > >     explicit user request for "xics-kvm", and exit(1) instead of abort().
> > > >     Simplify the abort when we can't create "xics".
> > > > 
> > > > This patch adds the missing call to exit().
> > > > 
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > ---
> > > >  hw/ppc/spapr.c |    1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index abfb99b71b7d..f477d7b8a210 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -133,6 +133,7 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> > > >          if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> > > >              error_reportf_err(err,
> > > >                                "kernel_irqchip requested but unavailable: ");
> > > > +            exit(EXIT_FAILURE);
> > > >          } else {
> > > >              error_free(err);
> > > >          }
> > > >     
> > > 
> > > 
> > > This doesn't look right.  We have an errp pointer in the caller.  So
> > > on failure we should error_propagate(), rather than deciding for
> > > ourselves that exiting is the right course of action.
> > >   
> > 
> > I generally agree with that but can the caller cope with the fact that
> > the user passed -machine accel=kvm,kernel_irqchip=on and this cannot
> > be satisfied ?  
> 
> AFAICT the only caller passes &error_fatal, so, yes..
> 

Ok, then I'll rewrite xics_system_init() so that it propagates the error
message instead of printing it to the monitor.

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

[Qemu-devel] [PATCH 3/6] spapr: fix error reporting in xics_system_init()

[Greg Kurz]

The xics_system_init() function passes its errp argument to xics_kvm_init().
If the call fails and the user requested in-kernel irqchip, it then ends up
passing a NULL Error * to error_reportf_err() and the error message is
silently dropped.

Passing an errp argument is generally wrong, unless you really just need
to convey an error to the caller.

This patch converts xics_system_init() to *only* pass a pointer to its own
Error * and then to propagate it with error_propagate(), as recommended
in error.h.

The local_err name is used for consistency with the rest of the code.

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f477d7b8a210..44f7dc7f40e9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -121,29 +121,32 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
 static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
+    Error *local_err = NULL;
 
     if (kvm_enabled()) {
-        Error *err = NULL;
-
         if (machine_kernel_irqchip_allowed(machine) &&
-            !xics_kvm_init(spapr, errp)) {
+            !xics_kvm_init(spapr, &local_err)) {
             spapr->icp_type = TYPE_KVM_ICP;
-            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, &err);
+            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
+                                          &local_err);
         }
         if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
-            error_reportf_err(err,
+            error_reportf_err(local_err,
                               "kernel_irqchip requested but unavailable: ");
             exit(EXIT_FAILURE);
         } else {
-            error_free(err);
+            error_free(local_err);
         }
     }
 
     if (!spapr->ics) {
         xics_spapr_init(spapr);
         spapr->icp_type = TYPE_ICP;
-        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
+        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
+                                      &local_err);
     }
+
+    error_propagate(errp, local_err);
 }
 
 static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,

Re: [Qemu-devel] [PATCH 3/6] spapr: fix error reporting in xics_system_init()

[Cédric Le Goater]

On 05/15/2017 01:39 PM, Greg Kurz wrote:
> The xics_system_init() function passes its errp argument to xics_kvm_init().
> If the call fails and the user requested in-kernel irqchip, it then ends up
> passing a NULL Error * to error_reportf_err() and the error message is
> silently dropped.
> 
> Passing an errp argument is generally wrong, unless you really just need
> to convey an error to the caller.
> 
> This patch converts xics_system_init() to *only* pass a pointer to its own
> Error * and then to propagate it with error_propagate(), as recommended
> in error.h.
> 
> The local_err name is used for consistency with the rest of the code.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Thanks for the fix.

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

C.

> ---
>  hw/ppc/spapr.c |   17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f477d7b8a210..44f7dc7f40e9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -121,29 +121,32 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
>  static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> +    Error *local_err = NULL;
>  
>      if (kvm_enabled()) {
> -        Error *err = NULL;
> -
>          if (machine_kernel_irqchip_allowed(machine) &&
> -            !xics_kvm_init(spapr, errp)) {
> +            !xics_kvm_init(spapr, &local_err)) {
>              spapr->icp_type = TYPE_KVM_ICP;
> -            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, &err);
> +            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
> +                                          &local_err);
>          }
>          if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> -            error_reportf_err(err,
> +            error_reportf_err(local_err,
>                                "kernel_irqchip requested but unavailable: ");
>              exit(EXIT_FAILURE);
>          } else {
> -            error_free(err);
> +            error_free(local_err);
>          }
>      }
>  
>      if (!spapr->ics) {
>          xics_spapr_init(spapr);
>          spapr->icp_type = TYPE_ICP;
> -        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
> +        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
> +                                      &local_err);
>      }
> +
> +    error_propagate(errp, local_err);
>  }
>  
>  static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
> 

Re: [Qemu-devel] [PATCH 3/6] spapr: fix error reporting in xics_system_init()

[David Gibson]

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

On Mon, May 15, 2017 at 01:39:36PM +0200, Greg Kurz wrote:
> The xics_system_init() function passes its errp argument to xics_kvm_init().
> If the call fails and the user requested in-kernel irqchip, it then ends up
> passing a NULL Error * to error_reportf_err() and the error message is
> silently dropped.
> 
> Passing an errp argument is generally wrong, unless you really just need
> to convey an error to the caller.
> 
> This patch converts xics_system_init() to *only* pass a pointer to its own
> Error * and then to propagate it with error_propagate(), as recommended
> in error.h.
> 
> The local_err name is used for consistency with the rest of the code.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

The change is good, but will need updating to work with the problem in
the previous patch.

> ---
>  hw/ppc/spapr.c |   17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f477d7b8a210..44f7dc7f40e9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -121,29 +121,32 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
>  static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> +    Error *local_err = NULL;
>  
>      if (kvm_enabled()) {
> -        Error *err = NULL;
> -
>          if (machine_kernel_irqchip_allowed(machine) &&
> -            !xics_kvm_init(spapr, errp)) {
> +            !xics_kvm_init(spapr, &local_err)) {
>              spapr->icp_type = TYPE_KVM_ICP;
> -            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs, &err);
> +            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
> +                                          &local_err);
>          }
>          if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> -            error_reportf_err(err,
> +            error_reportf_err(local_err,
>                                "kernel_irqchip requested but unavailable: ");
>              exit(EXIT_FAILURE);
>          } else {
> -            error_free(err);
> +            error_free(local_err);
>          }
>      }
>  
>      if (!spapr->ics) {
>          xics_spapr_init(spapr);
>          spapr->icp_type = TYPE_ICP;
> -        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs, errp);
> +        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
> +                                      &local_err);
>      }
> +
> +    error_propagate(errp, local_err);
>  }
>  
>  static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
> 

-- 
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 4/6] spapr: sanitize error handling in spapr_ics_create()

[Greg Kurz]

The spapr_ics_create() function handles errors in a rather convoluted
way, with two local Error * variables. Moreover, failing to parent the
ICS object to the machine should be considered as a bug but it is
currently ignored.

This patch addresses both issues.

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

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 44f7dc7f40e9..c53989bb10b1 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -101,21 +101,26 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
                                   const char *type_ics,
                                   int nr_irqs, Error **errp)
 {
-    Error *err = NULL, *local_err = NULL;
+    Error *local_err = NULL;
     Object *obj;
 
     obj = object_new(type_ics);
-    object_property_add_child(OBJECT(spapr), "ics", obj, NULL);
+    object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
     object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
-    object_property_set_int(obj, nr_irqs, "nr-irqs", &err);
+    object_property_set_int(obj, nr_irqs, "nr-irqs", &local_err);
+    if (local_err) {
+        goto error;
+    }
     object_property_set_bool(obj, true, "realized", &local_err);
-    error_propagate(&err, local_err);
-    if (err) {
-        error_propagate(errp, err);
-        return NULL;
+    if (local_err) {
+        goto error;
     }
 
     return ICS_SIMPLE(obj);
+
+error:
+    error_propagate(errp, local_err);
+    return NULL;
 }
 
 static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)

Re: [Qemu-devel] [PATCH 4/6] spapr: sanitize error handling in spapr_ics_create()

[Cédric Le Goater]

On 05/15/2017 01:39 PM, Greg Kurz wrote:
> The spapr_ics_create() function handles errors in a rather convoluted
> way, with two local Error * variables. Moreover, failing to parent the
> ICS object to the machine should be considered as a bug but it is
> currently ignored.

I am not sure what should be done for object_property_add_child()
errors but QEMU generally uses NULL for 'Error **'. It might be 
wrong though.

As for the local error handling, it is following what is described in 
qapi/error.h. Isn't it ?

Cheers,

C. 

 
> This patch addresses both issues.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr.c |   19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 44f7dc7f40e9..c53989bb10b1 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -101,21 +101,26 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
>                                    const char *type_ics,
>                                    int nr_irqs, Error **errp)
>  {
> -    Error *err = NULL, *local_err = NULL;
> +    Error *local_err = NULL;
>      Object *obj;
>  
>      obj = object_new(type_ics);
> -    object_property_add_child(OBJECT(spapr), "ics", obj, NULL);
> +    object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
>      object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> -    object_property_set_int(obj, nr_irqs, "nr-irqs", &err);
> +    object_property_set_int(obj, nr_irqs, "nr-irqs", &local_err);
> +    if (local_err) {
> +        goto error;
> +    }
>      object_property_set_bool(obj, true, "realized", &local_err);
> -    error_propagate(&err, local_err);
> -    if (err) {
> -        error_propagate(errp, err);
> -        return NULL;
> +    if (local_err) {
> +        goto error;
>      }
>  
>      return ICS_SIMPLE(obj);
> +
> +error:
> +    error_propagate(errp, local_err);
> +    return NULL;
>  }
>  
>  static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> 

Re: [Qemu-devel] [PATCH 4/6] spapr: sanitize error handling in spapr_ics_create()

[Greg Kurz]

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

On Mon, 15 May 2017 13:59:33 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 05/15/2017 01:39 PM, Greg Kurz wrote:
> > The spapr_ics_create() function handles errors in a rather convoluted
> > way, with two local Error * variables. Moreover, failing to parent the
> > ICS object to the machine should be considered as a bug but it is
> > currently ignored.  
> 
> I am not sure what should be done for object_property_add_child()
> errors but QEMU generally uses NULL for 'Error **'. It might be 
> wrong though.
> 
> As for the local error handling, it is following what is described in 
> qapi/error.h. Isn't it ?
> 

Yes, it does follow the "Receive and accumulate multiple errors" recommandation,
but does it make sense to realize the ICS object if we failed to set nr-irqs ?

> Cheers,
> 
> C. 
> 
>  
> > This patch addresses both issues.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/ppc/spapr.c |   19 ++++++++++++-------
> >  1 file changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 44f7dc7f40e9..c53989bb10b1 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -101,21 +101,26 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
> >                                    const char *type_ics,
> >                                    int nr_irqs, Error **errp)
> >  {
> > -    Error *err = NULL, *local_err = NULL;
> > +    Error *local_err = NULL;
> >      Object *obj;
> >  
> >      obj = object_new(type_ics);
> > -    object_property_add_child(OBJECT(spapr), "ics", obj, NULL);
> > +    object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
> >      object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> > -    object_property_set_int(obj, nr_irqs, "nr-irqs", &err);
> > +    object_property_set_int(obj, nr_irqs, "nr-irqs", &local_err);
> > +    if (local_err) {
> > +        goto error;
> > +    }
> >      object_property_set_bool(obj, true, "realized", &local_err);
> > -    error_propagate(&err, local_err);
> > -    if (err) {
> > -        error_propagate(errp, err);
> > -        return NULL;
> > +    if (local_err) {
> > +        goto error;
> >      }
> >  
> >      return ICS_SIMPLE(obj);
> > +
> > +error:
> > +    error_propagate(errp, local_err);
> > +    return NULL;
> >  }
> >  
> >  static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> >   
> 


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

Re: [Qemu-devel] [PATCH 4/6] spapr: sanitize error handling in spapr_ics_create()

[David Gibson]

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

On Mon, May 15, 2017 at 02:06:18PM +0200, Greg Kurz wrote:
> On Mon, 15 May 2017 13:59:33 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
> > On 05/15/2017 01:39 PM, Greg Kurz wrote:
> > > The spapr_ics_create() function handles errors in a rather convoluted
> > > way, with two local Error * variables. Moreover, failing to parent the
> > > ICS object to the machine should be considered as a bug but it is
> > > currently ignored.  
> > 
> > I am not sure what should be done for object_property_add_child()
> > errors but QEMU generally uses NULL for 'Error **'. It might be 
> > wrong though.
> > 
> > As for the local error handling, it is following what is described in 
> > qapi/error.h. Isn't it ?
> > 
> 
> Yes, it does follow the "Receive and accumulate multiple errors" recommandation,
> but does it make sense to realize the ICS object if we failed to set
> nr-irqs ?

Nor is it necessary to have two different local error variables.

-- 
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 4/6] spapr: sanitize error handling in spapr_ics_create()

[David Gibson]

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

On Mon, May 15, 2017 at 01:39:45PM +0200, Greg Kurz wrote:
> The spapr_ics_create() function handles errors in a rather convoluted
> way, with two local Error * variables. Moreover, failing to parent the
> ICS object to the machine should be considered as a bug but it is
> currently ignored.
> 
> This patch addresses both issues.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-2.10

> ---
>  hw/ppc/spapr.c |   19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 44f7dc7f40e9..c53989bb10b1 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -101,21 +101,26 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
>                                    const char *type_ics,
>                                    int nr_irqs, Error **errp)
>  {
> -    Error *err = NULL, *local_err = NULL;
> +    Error *local_err = NULL;
>      Object *obj;
>  
>      obj = object_new(type_ics);
> -    object_property_add_child(OBJECT(spapr), "ics", obj, NULL);
> +    object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
>      object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> -    object_property_set_int(obj, nr_irqs, "nr-irqs", &err);
> +    object_property_set_int(obj, nr_irqs, "nr-irqs", &local_err);
> +    if (local_err) {
> +        goto error;
> +    }
>      object_property_set_bool(obj, true, "realized", &local_err);
> -    error_propagate(&err, local_err);
> -    if (err) {
> -        error_propagate(errp, err);
> -        return NULL;
> +    if (local_err) {
> +        goto error;
>      }
>  
>      return ICS_SIMPLE(obj);
> +
> +error:
> +    error_propagate(errp, local_err);
> +    return NULL;
>  }
>  
>  static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> 

-- 
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 5/6] spapr-cpu-core: release ICP object when realization fails

[Greg Kurz]

While here we introduce a single error path to avoid code duplication.

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

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 4389ef4c2aef..63d160f7e010 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -147,25 +147,25 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
     object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
     object_property_set_bool(obj, true, "realized", &local_err);
     if (local_err) {
-        error_propagate(errp, local_err);
-        return;
+        goto error;
     }
 
     object_property_set_bool(child, true, "realized", &local_err);
     if (local_err) {
-        object_unparent(obj);
-        error_propagate(errp, local_err);
-        return;
+        goto error;
     }
 
     spapr_cpu_init(spapr, cpu, &local_err);
     if (local_err) {
-        object_unparent(obj);
-        error_propagate(errp, local_err);
-        return;
+        goto error;
     }
 
     xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
+    return;
+
+error:
+    object_unparent(obj);
+    error_propagate(errp, local_err);
 }
 
 static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)

Re: [Qemu-devel] [PATCH 5/6] spapr-cpu-core: release ICP object when realization fails

[Cédric Le Goater]

On 05/15/2017 01:39 PM, Greg Kurz wrote:
> While here we introduce a single error path to avoid code duplication.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr_cpu_core.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 4389ef4c2aef..63d160f7e010 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -147,25 +147,25 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
>      object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
>      object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> +        goto error;

the error: statement below adds an object_unparent(). is that ok ? 

C. 

>      }
>  
>      object_property_set_bool(child, true, "realized", &local_err);
>      if (local_err) {
> -        object_unparent(obj);
> -        error_propagate(errp, local_err);
> -        return;
> +        goto error;
>      }
>  
>      spapr_cpu_init(spapr, cpu, &local_err);
>      if (local_err) {
> -        object_unparent(obj);
> -        error_propagate(errp, local_err);
> -        return;
> +        goto error;
>      }
>  
>      xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
> +    return;
> +
> +error:
> +    object_unparent(obj);
> +    error_propagate(errp, local_err);
>  }
>  
>  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> 

Re: [Qemu-devel] [PATCH 5/6] spapr-cpu-core: release ICP object when realization fails

[Greg Kurz]

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

On Mon, 15 May 2017 14:02:34 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 05/15/2017 01:39 PM, Greg Kurz wrote:
> > While here we introduce a single error path to avoid code duplication.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/ppc/spapr_cpu_core.c |   16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 4389ef4c2aef..63d160f7e010 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -147,25 +147,25 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
> >      object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> >      object_property_set_bool(obj, true, "realized", &local_err);
> >      if (local_err) {
> > -        error_propagate(errp, local_err);
> > -        return;
> > +        goto error;  
> 
> the error: statement below adds an object_unparent(). is that ok ? 
> 

Yes, this is the purpose of the patch actually.

The ICP object gets parented to the CPU a few lines above, so I guess we
should unparent in this error path as well.

Maybe I should rename the patch to "unparent ICP object when realization fails" ?

> C. 
> 
> >      }
> >  
> >      object_property_set_bool(child, true, "realized", &local_err);
> >      if (local_err) {
> > -        object_unparent(obj);
> > -        error_propagate(errp, local_err);
> > -        return;
> > +        goto error;
> >      }
> >  
> >      spapr_cpu_init(spapr, cpu, &local_err);
> >      if (local_err) {
> > -        object_unparent(obj);
> > -        error_propagate(errp, local_err);
> > -        return;
> > +        goto error;
> >      }
> >  
> >      xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
> > +    return;
> > +
> > +error:
> > +    object_unparent(obj);
> > +    error_propagate(errp, local_err);
> >  }
> >  
> >  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >   
> 


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

Re: [Qemu-devel] [PATCH 5/6] spapr-cpu-core: release ICP object when realization fails

[David Gibson]

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

On Mon, May 15, 2017 at 01:39:55PM +0200, Greg Kurz wrote:
> While here we introduce a single error path to avoid code duplication.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc=for-2.10.

> ---
>  hw/ppc/spapr_cpu_core.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 4389ef4c2aef..63d160f7e010 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -147,25 +147,25 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
>      object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
>      object_property_set_bool(obj, true, "realized", &local_err);
>      if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> +        goto error;
>      }
>  
>      object_property_set_bool(child, true, "realized", &local_err);
>      if (local_err) {
> -        object_unparent(obj);
> -        error_propagate(errp, local_err);
> -        return;
> +        goto error;
>      }
>  
>      spapr_cpu_init(spapr, cpu, &local_err);
>      if (local_err) {
> -        object_unparent(obj);
> -        error_propagate(errp, local_err);
> -        return;
> +        goto error;
>      }
>  
>      xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj));
> +    return;
> +
> +error:
> +    object_unparent(obj);
> +    error_propagate(errp, local_err);
>  }
>  
>  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> 

-- 
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 6/6] spapr: fix migration of ICP objects from/to older QEMU

[Greg Kurz]

Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
sPAPRCPUCore") moved ICP objects from the machine to CPU cores. This
is an improvement since we no longer allocate ICP 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 introduces a compat flag in the sPAPR machine class so
that all pseries machine up to 2.9 go on with the previous behavior
of pre-allocating ICP objects.

While here, we also ensure that object_property_add_child() errors cause
QEMU to abort for newer machines.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c          |   36 ++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr_cpu_core.c |   28 ++++++++++++++++++++--------
 include/hw/ppc/spapr.h  |    2 ++
 3 files changed, 58 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index c53989bb10b1..ab3683bcd677 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -126,6 +126,7 @@ error:
 static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
     Error *local_err = NULL;
 
     if (kvm_enabled()) {
@@ -151,6 +152,38 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
                                       &local_err);
     }
 
+    if (!spapr->ics) {
+        goto out;
+    }
+
+    if (smc->must_pre_allocate_icps) {
+        int smt = kvmppc_smt_threads();
+        int nr_servers = DIV_ROUND_UP(max_cpus * smt, smp_threads);
+        int i;
+
+        spapr->legacy_icps = g_malloc0(nr_servers * sizeof(ICPState));
+
+        for (i = 0; i < nr_servers; i++) {
+            void* obj = &spapr->legacy_icps[i];
+
+            object_initialize(obj, sizeof(ICPState), spapr->icp_type);
+            object_property_add_child(OBJECT(spapr), "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);
+            if (local_err) {
+                while (i--) {
+                    object_unparent(obj);
+                }
+                g_free(spapr->legacy_icps);
+                break;
+            }
+        }
+    }
+
+out:
     error_propagate(errp, local_err);
 }
 
@@ -3256,8 +3289,11 @@ 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);
+    smc->must_pre_allocate_icps = true;
 }
 
 DEFINE_SPAPR_MACHINE(2_9, "2.9", false);
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 63d160f7e010..5476647efa06 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -119,6 +119,7 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
     size_t size = object_type_get_instance_size(typename);
     CPUCore *cc = CPU_CORE(dev);
     int i;
+    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
 
     for (i = 0; i < cc->nr_threads; i++) {
         void *obj = sc->threads + i * size;
@@ -127,7 +128,9 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
         PowerPCCPU *cpu = POWERPC_CPU(cs);
 
         spapr_cpu_destroy(cpu);
-        object_unparent(cpu->intc);
+        if (!spapr->legacy_icps) {
+            object_unparent(cpu->intc);
+        }
         cpu_remove_sync(cs);
         object_unparent(obj);
     }
@@ -142,12 +145,19 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     Object *obj;
 
-    obj = object_new(spapr->icp_type);
-    object_property_add_child(OBJECT(cpu), "icp", obj, NULL);
-    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
-    object_property_set_bool(obj, true, "realized", &local_err);
-    if (local_err) {
-        goto error;
+    if (spapr->legacy_icps) {
+        int index = cpu->parent_obj.cpu_index;
+
+        obj = OBJECT(&spapr->legacy_icps[index]);
+    } else {
+        obj = object_new(spapr->icp_type);
+        object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
+        object_property_add_const_link(obj, "xics", OBJECT(spapr),
+                                       &error_abort);
+        object_property_set_bool(obj, true, "realized", &local_err);
+        if (local_err) {
+            goto error;
+        }
     }
 
     object_property_set_bool(child, true, "realized", &local_err);
@@ -164,7 +174,9 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
     return;
 
 error:
-    object_unparent(obj);
+    if (!spapr->legacy_icps) {
+        object_unparent(obj);
+    }
     error_propagate(errp, local_err);
 }
 
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 5802f888c39d..72cd5af2679b 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 must_pre_allocate_icps; /* only for pseries-2.9 and older */
     void (*phb_placement)(sPAPRMachineState *spapr, uint32_t index,
                           uint64_t *buid, hwaddr *pio, 
                           hwaddr *mmio32, hwaddr *mmio64,
@@ -109,6 +110,7 @@ struct sPAPRMachineState {
     MemoryHotplugState hotplug_memory;
 
     const char *icp_type;
+    ICPState *legacy_icps;
 };
 
 #define H_SUCCESS         0

Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU

[Cédric Le Goater]

On 05/15/2017 01:40 PM, Greg Kurz wrote:
> Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> sPAPRCPUCore") moved ICP objects from the machine to CPU cores. This
> is an improvement since we no longer allocate ICP 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 introduces a compat flag in the sPAPR machine class so
> that all pseries machine up to 2.9 go on with the previous behavior
> of pre-allocating ICP objects.

I think this is a quite elegant way to a handle the migration 
regression. Thanks for taking care of it.

Have you tried to simply reparent the ICPs objects to OBJECT(spapr) 
instead of the OBJECT(cpu)  ? 

See some minor comments below.

> While here, we also ensure that object_property_add_child() errors cause
> QEMU to abort for newer machines.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr.c          |   36 ++++++++++++++++++++++++++++++++++++
>  hw/ppc/spapr_cpu_core.c |   28 ++++++++++++++++++++--------
>  include/hw/ppc/spapr.h  |    2 ++
>  3 files changed, 58 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index c53989bb10b1..ab3683bcd677 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -126,6 +126,7 @@ error:
>  static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>      Error *local_err = NULL;
>  
>      if (kvm_enabled()) {
> @@ -151,6 +152,38 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
>                                        &local_err);
>      }
>  
> +    if (!spapr->ics) {
> +        goto out;
> +    }
> +
> +    if (smc->must_pre_allocate_icps) {

I am not sure I like 'must', I think 'pre_allocate_icps' should be enough ? 
or simply 'allocate_legacy_icps' ?

> +        int smt = kvmppc_smt_threads();
> +        int nr_servers = DIV_ROUND_UP(max_cpus * smt, smp_threads);

may be we should reintroduce nr_servers at the machine level ? 

> +        int i;
> +
> +        spapr->legacy_icps = g_malloc0(nr_servers * sizeof(ICPState));
> +
> +        for (i = 0; i < nr_servers; i++) {
> +            void* obj = &spapr->legacy_icps[i];

'void *'

> +
> +            object_initialize(obj, sizeof(ICPState), spapr->icp_type);
> +            object_property_add_child(OBJECT(spapr), "icp[*]", obj,
> +                                      &error_abort);

David does not like the "icp[*]" syntax.

> +            object_unref(obj);
> +            object_property_add_const_link(obj, "xics", OBJECT(spapr),
> +                                           &error_abort);
> +            object_property_set_bool(obj, true, "realized", &local_err);
> +            if (local_err) {
> +                while (i--) {
> +                    object_unparent(obj);
> +                }
> +                g_free(spapr->legacy_icps);
> +                break;
> +            }
> +        }
> +    }
> +
> +out:
>      error_propagate(errp, local_err);
>  }
>  
> @@ -3256,8 +3289,11 @@ 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);
> +    smc->must_pre_allocate_icps = true;
>  }
>  
>  DEFINE_SPAPR_MACHINE(2_9, "2.9", false);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 63d160f7e010..5476647efa06 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -119,6 +119,7 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
>      size_t size = object_type_get_instance_size(typename);
>      CPUCore *cc = CPU_CORE(dev);
>      int i;
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>  
>      for (i = 0; i < cc->nr_threads; i++) {
>          void *obj = sc->threads + i * size;
> @@ -127,7 +128,9 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
>          spapr_cpu_destroy(cpu);
> -        object_unparent(cpu->intc);
> +        if (!spapr->legacy_icps) {
> +            object_unparent(cpu->intc);
> +        }
>          cpu_remove_sync(cs);
>          object_unparent(obj);
>      }
> @@ -142,12 +145,19 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      Object *obj;
>  
> -    obj = object_new(spapr->icp_type);
> -    object_property_add_child(OBJECT(cpu), "icp", obj, NULL);
> -    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> -    object_property_set_bool(obj, true, "realized", &local_err);
> -    if (local_err) {
> -        goto error;
> +    if (spapr->legacy_icps) {
> +        int index = cpu->parent_obj.cpu_index;
> +
> +        obj = OBJECT(&spapr->legacy_icps[index]);
> +    } else {
> +        obj = object_new(spapr->icp_type);
> +        object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> +        object_property_add_const_link(obj, "xics", OBJECT(spapr),
> +                                       &error_abort);
> +        object_property_set_bool(obj, true, "realized", &local_err);
> +        if (local_err) {
> +            goto error;
> +        }
>      }
>  
>      object_property_set_bool(child, true, "realized", &local_err);
> @@ -164,7 +174,9 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
>      return;
>  
>  error:
> -    object_unparent(obj);
> +    if (!spapr->legacy_icps) {
> +        object_unparent(obj);
> +    }
>      error_propagate(errp, local_err);
>  }
>  
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 5802f888c39d..72cd5af2679b 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 must_pre_allocate_icps; /* only for pseries-2.9 and older */
>      void (*phb_placement)(sPAPRMachineState *spapr, uint32_t index,
>                            uint64_t *buid, hwaddr *pio, 
>                            hwaddr *mmio32, hwaddr *mmio64,
> @@ -109,6 +110,7 @@ struct sPAPRMachineState {
>      MemoryHotplugState hotplug_memory;
>  
>      const char *icp_type;
> +    ICPState *legacy_icps;
>  };
>  
>  #define H_SUCCESS         0
> 

Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU

[Greg Kurz]

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

On Mon, 15 May 2017 14:22:32 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 05/15/2017 01:40 PM, Greg Kurz wrote:
> > Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> > sPAPRCPUCore") moved ICP objects from the machine to CPU cores. This
> > is an improvement since we no longer allocate ICP 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 introduces a compat flag in the sPAPR machine class so
> > that all pseries machine up to 2.9 go on with the previous behavior
> > of pre-allocating ICP objects.  
> 
> I think this is a quite elegant way to a handle the migration 
> regression. Thanks for taking care of it.
> 
> Have you tried to simply reparent the ICPs objects to OBJECT(spapr) 
> instead of the OBJECT(cpu)  ? 
> 

Do you mean to reparent unconditionally to OBJECT(spapr) for all
machine versions ? I'm not sure this would be beneficial, but I
might be missing something...

> See some minor comments below.
> 
> > While here, we also ensure that object_property_add_child() errors cause
> > QEMU to abort for newer machines.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/ppc/spapr.c          |   36 ++++++++++++++++++++++++++++++++++++
> >  hw/ppc/spapr_cpu_core.c |   28 ++++++++++++++++++++--------
> >  include/hw/ppc/spapr.h  |    2 ++
> >  3 files changed, 58 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index c53989bb10b1..ab3683bcd677 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -126,6 +126,7 @@ error:
> >  static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> >  {
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> >      Error *local_err = NULL;
> >  
> >      if (kvm_enabled()) {
> > @@ -151,6 +152,38 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> >                                        &local_err);
> >      }
> >  
> > +    if (!spapr->ics) {
> > +        goto out;
> > +    }
> > +
> > +    if (smc->must_pre_allocate_icps) {  
> 
> I am not sure I like 'must', I think 'pre_allocate_icps' should be enough ? 
> or simply 'allocate_legacy_icps' ?
> 

Ok. I'll go for the latter if it's ok with David.

> > +        int smt = kvmppc_smt_threads();
> > +        int nr_servers = DIV_ROUND_UP(max_cpus * smt, smp_threads);  
> 
> may be we should reintroduce nr_servers at the machine level ? 
> 

I had reintroduced it but then I realized it was only used in this
function.

> > +        int i;
> > +
> > +        spapr->legacy_icps = g_malloc0(nr_servers * sizeof(ICPState));
> > +
> > +        for (i = 0; i < nr_servers; i++) {
> > +            void* obj = &spapr->legacy_icps[i];  
> 
> 'void *'
> 

Yeah, patchew already yelled at me :)

> > +
> > +            object_initialize(obj, sizeof(ICPState), spapr->icp_type);
> > +            object_property_add_child(OBJECT(spapr), "icp[*]", obj,
> > +                                      &error_abort);  
> 
> David does not like the "icp[*]" syntax.
> 

Ah... I wasn't aware of that. But I agree that I should probably create
the object names based on 'i', rather than relying on the more complex
logic in object_property_add().

> > +            object_unref(obj);
> > +            object_property_add_const_link(obj, "xics", OBJECT(spapr),
> > +                                           &error_abort);
> > +            object_property_set_bool(obj, true, "realized", &local_err);
> > +            if (local_err) {
> > +                while (i--) {
> > +                    object_unparent(obj);
> > +                }
> > +                g_free(spapr->legacy_icps);
> > +                break;
> > +            }
> > +        }
> > +    }
> > +
> > +out:
> >      error_propagate(errp, local_err);
> >  }
> >  
> > @@ -3256,8 +3289,11 @@ 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);
> > +    smc->must_pre_allocate_icps = true;
> >  }
> >  
> >  DEFINE_SPAPR_MACHINE(2_9, "2.9", false);
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 63d160f7e010..5476647efa06 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -119,6 +119,7 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
> >      size_t size = object_type_get_instance_size(typename);
> >      CPUCore *cc = CPU_CORE(dev);
> >      int i;
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >  
> >      for (i = 0; i < cc->nr_threads; i++) {
> >          void *obj = sc->threads + i * size;
> > @@ -127,7 +128,9 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
> >          PowerPCCPU *cpu = POWERPC_CPU(cs);
> >  
> >          spapr_cpu_destroy(cpu);
> > -        object_unparent(cpu->intc);
> > +        if (!spapr->legacy_icps) {
> > +            object_unparent(cpu->intc);
> > +        }
> >          cpu_remove_sync(cs);
> >          object_unparent(obj);
> >      }
> > @@ -142,12 +145,19 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
> >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> >      Object *obj;
> >  
> > -    obj = object_new(spapr->icp_type);
> > -    object_property_add_child(OBJECT(cpu), "icp", obj, NULL);
> > -    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> > -    object_property_set_bool(obj, true, "realized", &local_err);
> > -    if (local_err) {
> > -        goto error;
> > +    if (spapr->legacy_icps) {
> > +        int index = cpu->parent_obj.cpu_index;
> > +
> > +        obj = OBJECT(&spapr->legacy_icps[index]);
> > +    } else {
> > +        obj = object_new(spapr->icp_type);
> > +        object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> > +        object_property_add_const_link(obj, "xics", OBJECT(spapr),
> > +                                       &error_abort);
> > +        object_property_set_bool(obj, true, "realized", &local_err);
> > +        if (local_err) {
> > +            goto error;
> > +        }
> >      }
> >  
> >      object_property_set_bool(child, true, "realized", &local_err);
> > @@ -164,7 +174,9 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
> >      return;
> >  
> >  error:
> > -    object_unparent(obj);
> > +    if (!spapr->legacy_icps) {
> > +        object_unparent(obj);
> > +    }
> >      error_propagate(errp, local_err);
> >  }
> >  
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 5802f888c39d..72cd5af2679b 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 must_pre_allocate_icps; /* only for pseries-2.9 and older */
> >      void (*phb_placement)(sPAPRMachineState *spapr, uint32_t index,
> >                            uint64_t *buid, hwaddr *pio, 
> >                            hwaddr *mmio32, hwaddr *mmio64,
> > @@ -109,6 +110,7 @@ struct sPAPRMachineState {
> >      MemoryHotplugState hotplug_memory;
> >  
> >      const char *icp_type;
> > +    ICPState *legacy_icps;
> >  };
> >  
> >  #define H_SUCCESS         0
> >   
> 


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

Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU

[Cédric Le Goater]

On 05/15/2017 03:16 PM, Greg Kurz wrote:
> On Mon, 15 May 2017 14:22:32 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> On 05/15/2017 01:40 PM, Greg Kurz wrote:
>>> Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
>>> sPAPRCPUCore") moved ICP objects from the machine to CPU cores. This
>>> is an improvement since we no longer allocate ICP 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 introduces a compat flag in the sPAPR machine class so
>>> that all pseries machine up to 2.9 go on with the previous behavior
>>> of pre-allocating ICP objects.  
>>
>> I think this is a quite elegant way to a handle the migration 
>> regression. Thanks for taking care of it.
>>
>> Have you tried to simply reparent the ICPs objects to OBJECT(spapr) 
>> instead of the OBJECT(cpu)  ? 
>>
> 
> Do you mean to reparent unconditionally to OBJECT(spapr) for all
> machine versions ? 

only in the case of smc->must_pre_allocate_icps

> I'm not sure this would be beneficial, but I might be missing 
> something...

I think that we would not need to allocate the legacy_icps array. 
Parenting the icp object to the spapr machine should be enough. 
I might be wrong. my expertise on the migration stream is very 
basic.

C.
 

Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU

[Greg Kurz]

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

On Mon, 15 May 2017 18:09:04 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 05/15/2017 03:16 PM, Greg Kurz wrote:
> > On Mon, 15 May 2017 14:22:32 +0200
> > Cédric Le Goater <clg@kaod.org> wrote:
> >   
> >> On 05/15/2017 01:40 PM, Greg Kurz wrote:  
> >>> Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> >>> sPAPRCPUCore") moved ICP objects from the machine to CPU cores. This
> >>> is an improvement since we no longer allocate ICP 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 introduces a compat flag in the sPAPR machine class so
> >>> that all pseries machine up to 2.9 go on with the previous behavior
> >>> of pre-allocating ICP objects.    
> >>
> >> I think this is a quite elegant way to a handle the migration 
> >> regression. Thanks for taking care of it.
> >>
> >> Have you tried to simply reparent the ICPs objects to OBJECT(spapr) 
> >> instead of the OBJECT(cpu)  ? 
> >>  
> > 
> > Do you mean to reparent unconditionally to OBJECT(spapr) for all
> > machine versions ?   
> 
> only in the case of smc->must_pre_allocate_icps
> 
> > I'm not sure this would be beneficial, but I might be missing 
> > something...  
> 
> I think that we would not need to allocate the legacy_icps array. 
> Parenting the icp object to the spapr machine should be enough. 
> I might be wrong. my expertise on the migration stream is very 
> basic.
> 

I don't think this would work because an older QEMU would still
send state for objects that don't exist in the destination.

> C.
>  


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

Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU

[David Gibson]

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

On Mon, May 15, 2017 at 06:20:06PM +0200, Greg Kurz wrote:
> On Mon, 15 May 2017 18:09:04 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
> > On 05/15/2017 03:16 PM, Greg Kurz wrote:
> > > On Mon, 15 May 2017 14:22:32 +0200
> > > Cédric Le Goater <clg@kaod.org> wrote:
> > >   
> > >> On 05/15/2017 01:40 PM, Greg Kurz wrote:  
> > >>> Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> > >>> sPAPRCPUCore") moved ICP objects from the machine to CPU cores. This
> > >>> is an improvement since we no longer allocate ICP 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 introduces a compat flag in the sPAPR machine class so
> > >>> that all pseries machine up to 2.9 go on with the previous behavior
> > >>> of pre-allocating ICP objects.    
> > >>
> > >> I think this is a quite elegant way to a handle the migration 
> > >> regression. Thanks for taking care of it.
> > >>
> > >> Have you tried to simply reparent the ICPs objects to OBJECT(spapr) 
> > >> instead of the OBJECT(cpu)  ? 
> > >>  
> > > 
> > > Do you mean to reparent unconditionally to OBJECT(spapr) for all
> > > machine versions ?   
> > 
> > only in the case of smc->must_pre_allocate_icps
> > 
> > > I'm not sure this would be beneficial, but I might be missing 
> > > something...  
> > 
> > I think that we would not need to allocate the legacy_icps array. 
> > Parenting the icp object to the spapr machine should be enough. 
> > I might be wrong. my expertise on the migration stream is very 
> > basic.
> > 
> 
> I don't think this would work because an older QEMU would still
> send state for objects that don't exist in the destination.

Right.  We could create "dummy" objects that receive the ICP data,
then discard it.  But it's probably more trouble than it's worty.

-- 
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 6/6] spapr: fix migration of ICP objects from/to older QEMU

[Cédric Le Goater]

>>> +        int smt = kvmppc_smt_threads();
>>> +        int nr_servers = DIV_ROUND_UP(max_cpus * smt, smp_threads);  
>>
>> may be we should reintroduce nr_servers at the machine level ? 
>>
> 
> I had reintroduced it but then I realized it was only used in this
> function.

nr_servers is also used when the device tree is populated with the 
interrupt controller nodes. No big deal.

C. 

Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU

[Greg Kurz]

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

On Mon, 15 May 2017 18:11:27 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> >>> +        int smt = kvmppc_smt_threads();
> >>> +        int nr_servers = DIV_ROUND_UP(max_cpus * smt, smp_threads);    
> >>
> >> may be we should reintroduce nr_servers at the machine level ? 
> >>  
> > 
> > I had reintroduced it but then I realized it was only used in this
> > function.  
> 
> nr_servers is also used when the device tree is populated with the 
> interrupt controller nodes. No big deal.
> 

Ah yes you're right. Maybe this can be factored out in a followup
patch.

> C. 
> 


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

Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU

[David Gibson]

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

On Mon, May 15, 2017 at 06:11:27PM +0200, Cédric Le Goater wrote:
> >>> +        int smt = kvmppc_smt_threads();
> >>> +        int nr_servers = DIV_ROUND_UP(max_cpus * smt, smp_threads);  
> >>
> >> may be we should reintroduce nr_servers at the machine level ? 
> >>
> > 
> > I had reintroduced it but then I realized it was only used in this
> > function.
> 
> nr_servers is also used when the device tree is populated with the 
> interrupt controller nodes. No big deal.

Which is guest visible, so we should really make that stay the same
for older machine types.  I'd like to avoid re-introducing nr_servers
as a property if we can, but maybe we can't.

-- 
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 6/6] spapr: fix migration of ICP objects from/to older QEMU

[Greg Kurz]

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

On Wed, 17 May 2017 14:18:16 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, May 15, 2017 at 06:11:27PM +0200, Cédric Le Goater wrote:
> > >>> +        int smt = kvmppc_smt_threads();
> > >>> +        int nr_servers = DIV_ROUND_UP(max_cpus * smt, smp_threads);
> > >>
> > >> may be we should reintroduce nr_servers at the machine level ? 
> > >>  
> > > 
> > > I had reintroduced it but then I realized it was only used in this
> > > function.  
> > 
> > nr_servers is also used when the device tree is populated with the 
> > interrupt controller nodes. No big deal.  
> 
> Which is guest visible, so we should really make that stay the same
> for older machine types.  I'd like to avoid re-introducing nr_servers
> as a property if we can, but maybe we can't.
> 

Yes we can :) or at least maybe, if you can shed light on a guest
visible change introduced by this commit in 2.8:

commit 9b9a19080a6e548b91420ce7925f2ac81ef63ae8
Author: David Gibson <david@gibson.dropbear.id.au>
Date:   Thu Oct 20 16:07:56 2016 +1100

    pseries: Move construction of /interrupt-controller fdt node


It changes the "ibm,interrupt-server-ranges" property in the device
tree from

    {0, cpu_to_be32(max_cpus)}

to

    {0, cpu_to_be32(xics->nr_servers)}

ie, {0, cpu_to_be32(DIV_ROUND_UP(max_cpus * smt, smp_threads))}

And indeed, if I start QEMU with

 -smp cores=2,threads=4,maxcpus=16 -machine type=pseries-2.7,accel=kvm

the following is exposed to the guest with 2.7:

ibm,interrupt-server-ranges
                 00000000 00000010

and with 2.8 we get:

ibm,interrupt-server-ranges
                 00000000 00000020

LoPAPR B.6.9.1.1 says that the range (ie, the second number) "shall be the
number of contiguous server#s supported by the unit (this also corresponds
to the number of “reg” entries)". I'm inclined to think this maps to max_cpus
but I may be wrong... any clues ?

Cheers,

--
Greg

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

Re: [Qemu-devel] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU

[David Gibson]

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

On Wed, May 17, 2017 at 10:33:44PM +0200, Greg Kurz wrote:
> On Wed, 17 May 2017 14:18:16 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Mon, May 15, 2017 at 06:11:27PM +0200, Cédric Le Goater wrote:
> > > >>> +        int smt = kvmppc_smt_threads();
> > > >>> +        int nr_servers = DIV_ROUND_UP(max_cpus * smt, smp_threads);
> > > >>
> > > >> may be we should reintroduce nr_servers at the machine level ? 
> > > >>  
> > > > 
> > > > I had reintroduced it but then I realized it was only used in this
> > > > function.  
> > > 
> > > nr_servers is also used when the device tree is populated with the 
> > > interrupt controller nodes. No big deal.  
> > 
> > Which is guest visible, so we should really make that stay the same
> > for older machine types.  I'd like to avoid re-introducing nr_servers
> > as a property if we can, but maybe we can't.
> > 
> 
> Yes we can :) or at least maybe, if you can shed light on a guest
> visible change introduced by this commit in 2.8:
> 
> commit 9b9a19080a6e548b91420ce7925f2ac81ef63ae8
> Author: David Gibson <david@gibson.dropbear.id.au>
> Date:   Thu Oct 20 16:07:56 2016 +1100
> 
>     pseries: Move construction of /interrupt-controller fdt node
> 
> 
> It changes the "ibm,interrupt-server-ranges" property in the device
> tree from
> 
>     {0, cpu_to_be32(max_cpus)}
> 
> to
> 
>     {0, cpu_to_be32(xics->nr_servers)}
> 
> ie, {0, cpu_to_be32(DIV_ROUND_UP(max_cpus * smt, smp_threads))}
> 
> And indeed, if I start QEMU with
> 
>  -smp cores=2,threads=4,maxcpus=16 -machine type=pseries-2.7,accel=kvm
> 
> the following is exposed to the guest with 2.7:
> 
> ibm,interrupt-server-ranges
>                  00000000 00000010
> 
> and with 2.8 we get:
> 
> ibm,interrupt-server-ranges
>                  00000000 00000020
> 
> LoPAPR B.6.9.1.1 says that the range (ie, the second number) "shall be the
> number of contiguous server#s supported by the unit (this also corresponds
> to the number of “reg” entries)". I'm inclined to think this maps to max_cpus
> but I may be wrong... any clues ?

So, yes, it's a guest visible change in some configurations, but in
those configurations the previous behaviour was sufficiently broken
that I think that's preferable to not changing it.

Basically the ibm,interrupt-server-ranges property gives the total
range of valid server IDs.  The server IDs assigned to each thread are
based on the dt_id, and are spaced by the number of host threads, not
the number of guest threads.  That means that with 2.7, the last cpus
would have advertised ibm,ppc-interrupt-server#s properties with
values outside the range given by ibm,interrupt-server-ranages, which
is definitely wrong.

-- 
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 6/6] spapr: fix migration of ICP objects from/to older QEMU

[David Gibson]

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

On Mon, May 15, 2017 at 03:16:02PM +0200, Greg Kurz wrote:
> On Mon, 15 May 2017 14:22:32 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
> > On 05/15/2017 01:40 PM, Greg Kurz wrote:
[snip]
> > > +
> > > +            object_initialize(obj, sizeof(ICPState), spapr->icp_type);
> > > +            object_property_add_child(OBJECT(spapr), "icp[*]", obj,
> > > +                                      &error_abort);  
> > 
> > David does not like the "icp[*]" syntax.
> > 
> 
> Ah... I wasn't aware of that. But I agree that I should probably create
> the object names based on 'i', rather than relying on the more complex
> logic in object_property_add().

Right, the non-obviousness of what the indicies will end up being is
exactly why I dislike the "[*]" syntax.  Especially here, where it's
clearly important that the QOM paths end up exactly like in older qemu
versions, I think it's better to be explicit.

-- 
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 6/6] spapr: fix migration of ICP objects from/to older QEMU

[David Gibson]

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

On Mon, May 15, 2017 at 02:22:32PM +0200, Cédric Le Goater wrote:
> On 05/15/2017 01:40 PM, Greg Kurz wrote:
> > Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> > sPAPRCPUCore") moved ICP objects from the machine to CPU cores. This
> > is an improvement since we no longer allocate ICP 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 introduces a compat flag in the sPAPR machine class so
> > that all pseries machine up to 2.9 go on with the previous behavior
> > of pre-allocating ICP objects.
> 
> I think this is a quite elegant way to a handle the migration 
> regression. Thanks for taking care of it.
> 
> Have you tried to simply reparent the ICPs objects to OBJECT(spapr) 
> instead of the OBJECT(cpu)  ? 

I actually kind of hate changing the QOM tree structure based on
machine type compatibility.  Unfortunately, since we're matching up
the migration state based (essentially) on QOM path, I don't see any
easy alternative.  I really wish there was a mechanism for defining
"alias paths" or something to handle this kind of migration
compatibility shim.

> See some minor comments below.
> 
> > While here, we also ensure that object_property_add_child() errors cause
> > QEMU to abort for newer machines.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/ppc/spapr.c          |   36 ++++++++++++++++++++++++++++++++++++
> >  hw/ppc/spapr_cpu_core.c |   28 ++++++++++++++++++++--------
> >  include/hw/ppc/spapr.h  |    2 ++
> >  3 files changed, 58 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index c53989bb10b1..ab3683bcd677 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -126,6 +126,7 @@ error:
> >  static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> >  {
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> >      Error *local_err = NULL;
> >  
> >      if (kvm_enabled()) {
> > @@ -151,6 +152,38 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> >                                        &local_err);
> >      }
> >  
> > +    if (!spapr->ics) {
> > +        goto out;
> > +    }
> > +
> > +    if (smc->must_pre_allocate_icps) {
> 
> I am not sure I like 'must', I think 'pre_allocate_icps' should be enough ? 
> or simply 'allocate_legacy_icps' ?

I'd actually prefer to make it explicit that this is a migration
compatibility shim and call it something like
'pre_2_10_icp_allocation'.

> > +        int smt = kvmppc_smt_threads();
> > +        int nr_servers = DIV_ROUND_UP(max_cpus * smt, smp_threads);
> 
> may be we should reintroduce nr_servers at the machine level ? 
> 
> > +        int i;
> > +
> > +        spapr->legacy_icps = g_malloc0(nr_servers * sizeof(ICPState));

This isn't technically safe, although you'll probably get away with
it.  spapr->icp_type is parameterized, which means it could be a
sub-class with a larger state structure than base ICPState.

> > +        for (i = 0; i < nr_servers; i++) {
> > +            void* obj = &spapr->legacy_icps[i];
> 
> 'void *'
> 
> > +
> > +            object_initialize(obj, sizeof(ICPState), spapr->icp_type);
> > +            object_property_add_child(OBJECT(spapr), "icp[*]", obj,
> > +                                      &error_abort);
> 
> David does not like the "icp[*]" syntax.
> 
> > +            object_unref(obj);
> > +            object_property_add_const_link(obj, "xics", OBJECT(spapr),
> > +                                           &error_abort);
> > +            object_property_set_bool(obj, true, "realized", &local_err);
> > +            if (local_err) {
> > +                while (i--) {
> > +                    object_unparent(obj);
> > +                }
> > +                g_free(spapr->legacy_icps);
> > +                break;
> > +            }
> > +        }
> > +    }
> > +
> > +out:
> >      error_propagate(errp, local_err);
> >  }
> >  
> > @@ -3256,8 +3289,11 @@ 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);
> > +    smc->must_pre_allocate_icps = true;
> >  }
> >  
> >  DEFINE_SPAPR_MACHINE(2_9, "2.9", false);
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 63d160f7e010..5476647efa06 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -119,6 +119,7 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
> >      size_t size = object_type_get_instance_size(typename);
> >      CPUCore *cc = CPU_CORE(dev);
> >      int i;
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >  
> >      for (i = 0; i < cc->nr_threads; i++) {
> >          void *obj = sc->threads + i * size;
> > @@ -127,7 +128,9 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
> >          PowerPCCPU *cpu = POWERPC_CPU(cs);
> >  
> >          spapr_cpu_destroy(cpu);
> > -        object_unparent(cpu->intc);
> > +        if (!spapr->legacy_icps) {
> > +            object_unparent(cpu->intc);
> > +        }
> >          cpu_remove_sync(cs);
> >          object_unparent(obj);
> >      }
> > @@ -142,12 +145,19 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
> >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> >      Object *obj;
> >  
> > -    obj = object_new(spapr->icp_type);
> > -    object_property_add_child(OBJECT(cpu), "icp", obj, NULL);
> > -    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> > -    object_property_set_bool(obj, true, "realized", &local_err);
> > -    if (local_err) {
> > -        goto error;
> > +    if (spapr->legacy_icps) {
> > +        int index = cpu->parent_obj.cpu_index;
> > +
> > +        obj = OBJECT(&spapr->legacy_icps[index]);
> > +    } else {
> > +        obj = object_new(spapr->icp_type);
> > +        object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> > +        object_property_add_const_link(obj, "xics", OBJECT(spapr),
> > +                                       &error_abort);
> > +        object_property_set_bool(obj, true, "realized", &local_err);
> > +        if (local_err) {
> > +            goto error;
> > +        }
> >      }
> >  
> >      object_property_set_bool(child, true, "realized", &local_err);
> > @@ -164,7 +174,9 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
> >      return;
> >  
> >  error:
> > -    object_unparent(obj);
> > +    if (!spapr->legacy_icps) {
> > +        object_unparent(obj);
> > +    }
> >      error_propagate(errp, local_err);
> >  }
> >  
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 5802f888c39d..72cd5af2679b 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 must_pre_allocate_icps; /* only for pseries-2.9 and older */
> >      void (*phb_placement)(sPAPRMachineState *spapr, uint32_t index,
> >                            uint64_t *buid, hwaddr *pio, 
> >                            hwaddr *mmio32, hwaddr *mmio64,
> > @@ -109,6 +110,7 @@ struct sPAPRMachineState {
> >      MemoryHotplugState hotplug_memory;
> >  
> >      const char *icp_type;
> > +    ICPState *legacy_icps;
> >  };
> >  
> >  #define H_SUCCESS         0
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 6/6] spapr: fix migration of ICP objects from/to older QEMU

[Greg Kurz]

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

On Mon, 15 May 2017 13:40:04 +0200
Greg Kurz <groug@kaod.org> wrote:

> Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> sPAPRCPUCore") moved ICP objects from the machine to CPU cores. This
> is an improvement since we no longer allocate ICP 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 introduces a compat flag in the sPAPR machine class so
> that all pseries machine up to 2.9 go on with the previous behavior
> of pre-allocating ICP objects.
> 
> While here, we also ensure that object_property_add_child() errors cause
> QEMU to abort for newer machines.
> 

Looking at the code again...

> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr.c          |   36 ++++++++++++++++++++++++++++++++++++
>  hw/ppc/spapr_cpu_core.c |   28 ++++++++++++++++++++--------
>  include/hw/ppc/spapr.h  |    2 ++
>  3 files changed, 58 insertions(+), 8 deletions(-)
[...]
> @@ -142,12 +145,19 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      Object *obj;
>  
> -    obj = object_new(spapr->icp_type);
> -    object_property_add_child(OBJECT(cpu), "icp", obj, NULL);

... I now realize that we're lacking

       object_unref(obj);

and the ref count of the ICP object can never go down to zero...

I'll fix that in a preparatory patch for v2.

> -    object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort);
> -    object_property_set_bool(obj, true, "realized", &local_err);
> -    if (local_err) {
> -        goto error;
> +    if (spapr->legacy_icps) {
> +        int index = cpu->parent_obj.cpu_index;
> +
> +        obj = OBJECT(&spapr->legacy_icps[index]);
> +    } else {
> +        obj = object_new(spapr->icp_type);
> +        object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> +        object_property_add_const_link(obj, "xics", OBJECT(spapr),
> +                                       &error_abort);
> +        object_property_set_bool(obj, true, "realized", &local_err);
> +        if (local_err) {
> +            goto error;
> +        }
>      }
>  
>      object_property_set_bool(child, true, "realized", &local_err);
> @@ -164,7 +174,9 @@ static void spapr_cpu_core_realize_child(Object *child, Error **errp)
>      return;
>  
>  error:
> -    object_unparent(obj);
> +    if (!spapr->legacy_icps) {
> +        object_unparent(obj);
> +    }
>      error_propagate(errp, local_err);
>  }
>  
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 5802f888c39d..72cd5af2679b 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 must_pre_allocate_icps; /* only for pseries-2.9 and older */
>      void (*phb_placement)(sPAPRMachineState *spapr, uint32_t index,
>                            uint64_t *buid, hwaddr *pio, 
>                            hwaddr *mmio32, hwaddr *mmio64,
> @@ -109,6 +110,7 @@ struct sPAPRMachineState {
>      MemoryHotplugState hotplug_memory;
>  
>      const char *icp_type;
> +    ICPState *legacy_icps;
>  };
>  
>  #define H_SUCCESS         0
> 
> 


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

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

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH 0/6] spapr/xics: fix migration of older machine types
Message-id: 149484833874.20089.4164801378197848306.stgit@bahia.lan
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/149484833874.20089.4164801378197848306.stgit@bahia.lan -> patchew/149484833874.20089.4164801378197848306.stgit@bahia.lan
Switched to a new branch 'test'
3c3824a spapr: fix migration of ICP objects from/to older QEMU
280164f spapr-cpu-core: release ICP object when realization fails
5bc72eb spapr: sanitize error handling in spapr_ics_create()
fa15d00 spapr: fix error reporting in xics_system_init()
ecbe234 spapr: fix error path of required kernel-irqchip
b7cf73d ppc/xics: simplify prototype of xics_spapr_init()

=== OUTPUT BEGIN ===
Checking PATCH 1/6: ppc/xics: simplify prototype of xics_spapr_init()...
Checking PATCH 2/6: spapr: fix error path of required kernel-irqchip...
Checking PATCH 3/6: spapr: fix error reporting in xics_system_init()...
Checking PATCH 4/6: spapr: sanitize error handling in spapr_ics_create()...
Checking PATCH 5/6: spapr-cpu-core: release ICP object when realization fails...
Checking PATCH 6/6: spapr: fix migration of ICP objects from/to older QEMU...
ERROR: "foo* bar" should be "foo *bar"
#50: FILE: hw/ppc/spapr.c:167:
+            void* obj = &spapr->legacy_icps[i];

total: 1 errors, 0 warnings, 122 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

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

[Greg Kurz]

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

On Mon, 15 May 2017 05:13:34 -0700 (PDT)
no-reply@patchew.org wrote:

> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Subject: [Qemu-devel] [PATCH 0/6] spapr/xics: fix migration of older machine types
> Message-id: 149484833874.20089.4164801378197848306.stgit@bahia.lan
> Type: series
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>         failed=1
>         echo
>     fi
>     n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>  - [tag update]      patchew/149484833874.20089.4164801378197848306.stgit@bahia.lan -> patchew/149484833874.20089.4164801378197848306.stgit@bahia.lan
> Switched to a new branch 'test'
> 3c3824a spapr: fix migration of ICP objects from/to older QEMU
> 280164f spapr-cpu-core: release ICP object when realization fails
> 5bc72eb spapr: sanitize error handling in spapr_ics_create()
> fa15d00 spapr: fix error reporting in xics_system_init()
> ecbe234 spapr: fix error path of required kernel-irqchip
> b7cf73d ppc/xics: simplify prototype of xics_spapr_init()
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/6: ppc/xics: simplify prototype of xics_spapr_init()...
> Checking PATCH 2/6: spapr: fix error path of required kernel-irqchip...
> Checking PATCH 3/6: spapr: fix error reporting in xics_system_init()...
> Checking PATCH 4/6: spapr: sanitize error handling in spapr_ics_create()...
> Checking PATCH 5/6: spapr-cpu-core: release ICP object when realization fails...
> Checking PATCH 6/6: spapr: fix migration of ICP objects from/to older QEMU...
> ERROR: "foo* bar" should be "foo *bar"
> #50: FILE: hw/ppc/spapr.c:167:
> +            void* obj = &spapr->legacy_icps[i];
> 

Oops, I'll fix that.

> total: 1 errors, 0 warnings, 122 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@freelists.org

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