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