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

[Greg Kurz]

On Mon, 15 May 2017 13:40:04 +0200
Greg Kurz <address@hidden> 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 <address@hidden>
> ---
>  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
> 
>

pgpv_sZzqIFaa.pgp
Description: OpenPGP digital signature