On Wed, Jun 07, 2017 at 07:17:09PM +0200, Greg Kurz wrote: > Until recently, spapr used to allocate ICPState objects for the lifetime > of the machine. They would only be associated to vCPUs in xics_cpu_setup() > when plugging a CPU core. > > Now that ICPState objects have the same lifecycle as vCPUs, it is > possible to associate them during realization. > > This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU > is passed as a property. Note that vCPU now needs to be realized first > for the IRQs to be allocated. It also needs to resetted before ICPState > realization in order to synchronize with KVM.
Ok, what enforces those ordering constraints? > Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't > needed anymore and can be safely dropped. > > Signed-off-by: Greg Kurz <gr...@kaod.org> Looks pretty good, though a couple nits below. > --- > hw/intc/xics.c | 76 > ++++++++++++++++++++--------------------------- > hw/ppc/pnv_core.c | 16 ++++------ > hw/ppc/spapr_cpu_core.c | 21 ++++++------- > include/hw/ppc/xics.h | 2 - > 4 files changed, 48 insertions(+), 67 deletions(-) > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index ec73f02144c9..330441ff7fe8 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -38,50 +38,6 @@ > #include "monitor/monitor.h" > #include "hw/intc/intc.h" > > -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu) > -{ > - CPUState *cs = CPU(cpu); > - ICPState *icp = ICP(cpu->intc); > - > - assert(icp); > - assert(cs == icp->cs); > - > - icp->output = NULL; > - icp->cs = NULL; > -} > - > -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp) > -{ > - CPUState *cs = CPU(cpu); > - CPUPPCState *env = &cpu->env; > - ICPStateClass *icpc; > - > - assert(icp); > - > - cpu->intc = OBJECT(icp); > - icp->cs = cs; > - > - icpc = ICP_GET_CLASS(icp); > - if (icpc->cpu_setup) { > - icpc->cpu_setup(icp, cpu); > - } > - > - switch (PPC_INPUT(env)) { > - case PPC_FLAGS_INPUT_POWER7: > - icp->output = env->irq_inputs[POWER7_INPUT_INT]; > - break; > - > - case PPC_FLAGS_INPUT_970: > - icp->output = env->irq_inputs[PPC970_INPUT_INT]; > - break; > - > - default: > - error_report("XICS interrupt controller does not support this CPU " > - "bus model"); > - abort(); > - } > -} > - > void icp_pic_print_info(ICPState *icp, Monitor *mon) > { > int cpu_index = icp->cs ? icp->cs->cpu_index : -1; > @@ -343,6 +299,8 @@ static void icp_realize(DeviceState *dev, Error **errp) > { > ICPState *icp = ICP(dev); > ICPStateClass *icpc = ICP_GET_CLASS(dev); > + PowerPCCPU *cpu; > + CPUPPCState *env; > Object *obj; > Error *err = NULL; > > @@ -355,6 +313,36 @@ static void icp_realize(DeviceState *dev, Error **errp) > > icp->xics = XICS_FABRIC(obj); > > + obj = object_property_get_link(OBJECT(dev), "cs", &err); I don't like the name "cs" for an externally visible property. "cpu" would be better. > + if (!obj) { > + error_setg(errp, "%s: required link 'xics' not found: %s", Copy/paste mistake in this message. > + __func__, error_get_pretty(err)); > + return; > + } > + > + cpu = POWERPC_CPU(obj); > + cpu->intc = OBJECT(icp); > + icp->cs = CPU(obj); > + > + if (icpc->cpu_setup) { > + icpc->cpu_setup(icp, cpu); > + } > + > + env = &cpu->env; > + switch (PPC_INPUT(env)) { > + case PPC_FLAGS_INPUT_POWER7: > + icp->output = env->irq_inputs[POWER7_INPUT_INT]; > + break; > + > + case PPC_FLAGS_INPUT_970: > + icp->output = env->irq_inputs[PPC970_INPUT_INT]; > + break; > + > + default: > + error_setg(errp, "XICS interrupt controller does not support this > CPU bus model"); > + return; > + } > + > if (icpc->realize) { > icpc->realize(dev, errp); > } > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c > index e8a9a94d5a24..1393005e76f3 100644 > --- a/hw/ppc/pnv_core.c > +++ b/hw/ppc/pnv_core.c > @@ -118,19 +118,19 @@ static void pnv_core_realize_child(Object *child, > XICSFabric *xi, Error **errp) > PowerPCCPU *cpu = POWERPC_CPU(cs); > Object *obj; > > - obj = object_new(TYPE_PNV_ICP); > - object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort); > - object_unref(obj); > - object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort); > - object_property_set_bool(obj, true, "realized", &local_err); > + object_property_set_bool(child, true, "realized", &local_err); > if (local_err) { > error_propagate(errp, local_err); > return; > } > > - object_property_set_bool(child, true, "realized", &local_err); > + obj = object_new(TYPE_PNV_ICP); > + object_property_add_child(child, "icp", obj, NULL); > + object_unref(obj); > + object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort); > + object_property_add_const_link(obj, "cs", child, &error_abort); > + object_property_set_bool(obj, true, "realized", &local_err); > if (local_err) { > - object_unparent(obj); > error_propagate(errp, local_err); > return; > } > @@ -141,8 +141,6 @@ static void pnv_core_realize_child(Object *child, > XICSFabric *xi, Error **errp) > error_propagate(errp, local_err); > return; > } > - > - xics_cpu_setup(xi, cpu, ICP(obj)); > } > > static void pnv_core_realize(DeviceState *dev, Error **errp) > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > index 029a14120edd..9a6259525953 100644 > --- a/hw/ppc/spapr_cpu_core.c > +++ b/hw/ppc/spapr_cpu_core.c > @@ -53,9 +53,6 @@ static void spapr_cpu_reset(void *opaque) > > static void spapr_cpu_destroy(PowerPCCPU *cpu) > { > - sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > - > - xics_cpu_destroy(XICS_FABRIC(spapr), cpu); > qemu_unregister_reset(spapr_cpu_reset, cpu); > } > > @@ -140,28 +137,28 @@ static void spapr_cpu_core_realize_child(Object *child, > Error **errp) > sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > CPUState *cs = CPU(child); > PowerPCCPU *cpu = POWERPC_CPU(cs); > - Object *obj; > + Object *obj = NULL; > > - obj = object_new(spapr->icp_type); > - object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort); > - object_unref(obj); > - object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort); > - object_property_set_bool(obj, true, "realized", &local_err); > + object_property_set_bool(child, true, "realized", &local_err); > if (local_err) { > goto error; > } > > - object_property_set_bool(child, true, "realized", &local_err); > + spapr_cpu_init(spapr, cpu, &local_err); > if (local_err) { > goto error; > } > > - spapr_cpu_init(spapr, cpu, &local_err); > + obj = object_new(spapr->icp_type); > + object_property_add_child(child, "icp", obj, &error_abort); > + object_unref(obj); > + object_property_add_const_link(obj, "xics", OBJECT(spapr), &error_abort); > + object_property_add_const_link(obj, "cs", child, &error_abort); > + object_property_set_bool(obj, true, "realized", &local_err); > if (local_err) { > goto error; > } > > - xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj)); > return; > > error: > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index 40a506eacfb4..05767a15be9a 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -183,8 +183,6 @@ void spapr_dt_xics(int nr_servers, void *fdt, uint32_t > phandle); > > qemu_irq xics_get_qirq(XICSFabric *xi, int irq); > ICPState *xics_icp_get(XICSFabric *xi, int server); > -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp); > -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu); > > /* Internal XICS interfaces */ > void icp_set_cppr(ICPState *icp, uint8_t cppr); > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature