On 06/07/2017 10:55 PM, Greg Kurz wrote: > On Wed, 7 Jun 2017 20:11:38 +0200 > Cédric Le Goater <c...@kaod.org> wrote: > >> On 06/07/2017 07:17 PM, 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. >>> >>> Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't >>> needed anymore and can be safely dropped. >> >> I like the idea but I think the assignment of ->cs attribute should be >> moved under icp_kvm_cpu_setup(), as it is only used under xics_kvm by >> the kvm_vcpu_ioctl() calls. >> > > Well, cs->cpu_index is also used for traces and to implement the 'info pic' > monitor command.
yes. But, these are just printfs. >> Ideally, we should also introduce : >> >> struct KvmState { >> ICPState parent_obj; >> >> CPUState *cs; >> }; >> >> That would be cleaner, but that might introduce some migration compat >> issues again ... >> > > No, as long as it doesn't change state, we're good. My concern is more > about how to pass cs to xics_kvm That can be done in the cpu_setup handler. > and the cs->cpu_index to xics. The printfs are interesting to have but not vital. David has a good point for keeping ->cs. So let's abandon the idea. >> Some minor comments below, >> >> Thanks, >> >> C. >> >>> Signed-off-by: Greg Kurz <gr...@kaod.org> >>> --- >>> 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); >>> + if (!obj) { >>> + error_setg(errp, "%s: required link 'xics' not found: %s", >>> + __func__, error_get_pretty(err)); >>> + return; >>> + } >>> + >>> + cpu = POWERPC_CPU(obj); >>> + cpu->intc = OBJECT(icp); >>> + icp->cs = CPU(obj); >> >> only xics_kvm needs ->cs. >> > > yeah, maybe the base class should only have a cpu_index field: > > icp->cpu_index = CPU(obj)->cpu_index; arg, no. please, let's not add another cpu index :) >>> + >>> + 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); >> >> may be link the cpu object instead ? >> > > I'm not sure to understand... isn't child supposed to be a CPU index here ? I meant linking the 'PowerPCCPU *cpu' object and not the CPUState. That's minor. >>> + 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); >> >> same here. >> >>> + 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); >>> >> >