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

[David Gibson <david@gibson.dropbear.id.au>]

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

On Mon, May 22, 2017 at 09:20:42AM +0200, Cédric Le Goater wrote:
> On 05/22/2017 04:30 AM, David Gibson wrote:
> > On Fri, May 19, 2017 at 12:32:27PM +0200, 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.
> >>
> >> Signed-off-by: Greg Kurz <groug@kaod.org>
> >> ---
> >> v2: - s/void* /void * in xics_system_init()
> >>     - don't use "[*]" in the ICP object name
> >>     - use pre_2_10_ prefix in field names
> >>     - added xics_nr_servers() helper
> >> ---
> >>  hw/ppc/spapr.c          |   40 +++++++++++++++++++++++++++++++++++++++-
> >>  hw/ppc/spapr_cpu_core.c |   29 ++++++++++++++++++++---------
> >>  include/hw/ppc/spapr.h  |    2 ++
> >>  3 files changed, 61 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 1bb05a9a6b07..182262257c60 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -123,9 +123,15 @@ error:
> >>      return NULL;
> >>  }
> >>  
> >> +static inline int xics_nr_servers(void)
> >> +{
> >> +    return ppc_cpu_dt_id_from_index(max_cpus);
> >> +}
> >> +
> >>  static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> >>  {
> >>      sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> >> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> >>  
> >>      if (kvm_enabled()) {
> >>          if (machine_kernel_irqchip_allowed(machine) &&
> >> @@ -147,6 +153,35 @@ static void xics_system_init(MachineState *machine, int nr_irqs, Error **errp)
> >>              return;
> >>          }
> >>      }
> >> +
> >> +    if (smc->pre_2_10_icp_allocation) {
> >> +        int nr_servers = xics_nr_servers();
> >> +        Error *local_err = NULL;
> >> +        int i;
> >> +
> >> +        spapr->pre_2_10_icps = g_malloc0(nr_servers * sizeof(ICPState));
> >> +
> >> +        for (i = 0; i < nr_servers; i++) {
> >> +            void *obj = &spapr->pre_2_10_icps[i];
> >> +            char *name = g_strdup_printf("icp[%d]", i);
> >> +
> >> +            object_initialize(obj, sizeof(ICPState), spapr->icp_type);
> >> +            object_property_add_child(OBJECT(spapr), name, obj, &error_abort);
> >> +            g_free(name);
> >> +            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->pre_2_10_icps);
> >> +                error_propagate(errp, local_err);
> >> +                break;
> >> +            }
> >> +        }
> >> +    }
> >>  }
> >>  
> >>  static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
> >> @@ -1020,7 +1055,7 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
> >>      _FDT(fdt_setprop_cell(fdt, 0, "#size-cells", 2));
> >>  
> >>      /* /interrupt controller */
> >> -    spapr_dt_xics(ppc_cpu_dt_id_from_index(max_cpus), fdt, PHANDLE_XICP);
> >> +    spapr_dt_xics(xics_nr_servers(), fdt, PHANDLE_XICP);
> >>  
> >>      ret = spapr_populate_memory(spapr, fdt);
> >>      if (ret < 0) {
> >> @@ -3286,9 +3321,12 @@ static void spapr_machine_2_9_instance_options(MachineState *machine)
> >>  
> >>  static void spapr_machine_2_9_class_options(MachineClass *mc)
> >>  {
> >> +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> >> +
> >>      spapr_machine_2_10_class_options(mc);
> >>      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_9);
> >>      mc->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
> >> +    smc->pre_2_10_icp_allocation = 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 ff7058ecc00e..13c4916aa5e6 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->pre_2_10_icps) {
> > 
> > Hrm.  I dislike code for the core object directly reaching into the
> > machine to check the compat flag here (and a bunch of other places
> > below).  I can think of a few possible ways of avoiding this:
> > 
> > 1) The most direct is to make another compat flag in the cpu core
> > object, set by the machine.  Straightforward, but ugly.
> > 
> > 2) Use a property to optionally pass a reference to the ICP array into
> > the core object.  If set it will give the cpu objects ICPs from that
> > array (compat mode), if not it will allocate them (new style mode).
> 
> for 2) we can just use a object_property_add_const_link() like
> we do to pass the 'xics' object which is needed by the ICSes.

Yes.  Though you'll need to pass one for each thread down to the core
object, which will be fiddly.

> > 3) (Preferred, if it works)  Always have the core allocate ICPs for
> > each CPU.  For compat mode instead of directly allocating ICPs, the
> > machine sets up an array of pointers to the existing ICPs for each
> > CPU.  The "extra" slots that don't have ICPs in new-style allocation
> > get references to dummy ICP objects (maybe even all the same one).
> > Only the dummy ICP(s) are allocated by the machine, the rest remain
> > owned by the cpu.
> 
> I like this solution too as it should isolate the compat handling under
> the machine, maybe even in a single routine.

Right, that's the hope.  Plus it means we reduce the difference in
runtime QOM structure between the two modes, which is best when
possible.

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