| Message ID | 149685582923.12025.13700165807436904935.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
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. 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 ... Some minor comments below, Thanks, C. > Signed-off-by: Greg Kurz <groug@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. > + > + 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 ? > + 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); >
On Wed, 7 Jun 2017 20:11:38 +0200 Cédric Le Goater <clg@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. > 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 and the cs->cpu_index to xics. > Some minor comments below, > > Thanks, > > C. > > > Signed-off-by: Greg Kurz <groug@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; > > + > > + 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 ? > > + 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); > > >
On Wed, Jun 07, 2017 at 10:55:07PM +0200, Greg Kurz wrote: > On Wed, 7 Jun 2017 20:11:38 +0200 > Cédric Le Goater <clg@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. Right. I think it makes sense for the ICP to have a handle on it's associated CPU, even if we don't actually use it in all cases right now. So I have no problem with the property being in all ICPs; I think that will be cleaner than special casing xics_kvm. Especially if we have to un-special-case it sometime in future because we need to access the CPU object for some reason we haven't thought of yet.
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 <groug@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); >
On 06/07/2017 10:55 PM, Greg Kurz wrote: > On Wed, 7 Jun 2017 20:11:38 +0200 > Cédric Le Goater <clg@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 <groug@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); >>> >> >
On Thu, 8 Jun 2017 12:01:12 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > 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? > I'm not sure about what you're asking... I had to re-order because xics_cpu_setup() used to be called after the vCPU is realized and put in PAPR mode. Especially, we have these lines: 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; They depend on env->irq_inputs being allocated. This happens on the spapr_cpu_core_realize_child() { ... object_property_set_bool(child, true, "realized", &local_err); object_property_set_bool() object_property_set_qobject() object_property_set() property_set_bool() device_set_realized() ppc_cpu_realizefn() init_ppc_proc() init_proc_POWER8() ppcPOWER7_irq_init() and, when using xics_kvm: icp_kvm_cpu_setup() { ... ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0, kernel_xics_fd, vcpu_id); This ioctl returns EINVAL because it fails the kvmppc_sanity_check() in KVM. It depends on QEMU enabling KVM_CAP_PPC_PAPR for the vCPU, which happens in: spapr_cpu_core_realize_child() { ... spapr_cpu_init(spapr, cpu, &local_err); cpu_ppc_set_papr() kvmppc_set_papr() > > Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't > > needed anymore and can be safely dropped. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > Looks pretty good, though a couple nits below. > Thanks. I'm also thinking about going one step further and converting xics_kvm to use the icpc->realize() handler instead of icpc->cpu_setup(). Makes sense ? > > --- > > 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. > Right. I'll fix this. > > + if (!obj) { > > + error_setg(errp, "%s: required link 'xics' not found: %s", > > Copy/paste mistake in this message. > Oops :) > > + __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); > > >
On Thu, 8 Jun 2017 07:50:08 +0200 Cédric Le Goater <clg@kaod.org> wrote: > On 06/07/2017 10:55 PM, Greg Kurz wrote: > [...] > >>> + 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. > Well, cpu == cs == child and object_property_add_const_link() expects an Object *. Passing child avoids a not-really-useful OBJECT() dynamic cast.
On Thu, 8 Jun 2017 11:53:29 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Wed, Jun 07, 2017 at 10:55:07PM +0200, Greg Kurz wrote: > > On Wed, 7 Jun 2017 20:11:38 +0200 > > Cédric Le Goater <clg@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. > > Right. I think it makes sense for the ICP to have a handle on it's > associated CPU, even if we don't actually use it in all cases right > now. So I have no problem with the property being in all ICPs; I > think that will be cleaner than special casing xics_kvm. Especially > if we have to un-special-case it sometime in future because we need > to access the CPU object for some reason we haven't thought of yet. > We had discussed with Cedric about that actually but when I started to write code, I had the impression that I would have to do convoluted things to get rid of the CPU handle in ICP. Thanks for confirming this.
On 06/08/2017 11:14 AM, Greg Kurz wrote: > On Thu, 8 Jun 2017 11:53:29 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > >> On Wed, Jun 07, 2017 at 10:55:07PM +0200, Greg Kurz wrote: >>> On Wed, 7 Jun 2017 20:11:38 +0200 >>> Cédric Le Goater <clg@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. >> >> Right. I think it makes sense for the ICP to have a handle on it's >> associated CPU, even if we don't actually use it in all cases right >> now. So I have no problem with the property being in all ICPs; I >> think that will be cleaner than special casing xics_kvm. Especially >> if we have to un-special-case it sometime in future because we need >> to access the CPU object for some reason we haven't thought of yet. >> > > We had discussed with Cedric about that actually but when I started > to write code, I had the impression that I would have to do convoluted > things to get rid of the CPU handle in ICP. There is nothing complex and it would surely simplify cpu_setup(). But, the main argument is that it might be useful to other platforms. So there is not much value in removing it. I am OK with that. I am less OK with using the index, even if it is useful for the migration state of ICP objects today. We could be tempted to use more of cpu_index, like we have done in the past, and that was really complex to untangle. Let's be cautious. C.
On 06/08/2017 10:45 AM, Greg Kurz wrote: > On Thu, 8 Jun 2017 12:01:12 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > >> 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? >> > > I'm not sure about what you're asking... I had to re-order because > xics_cpu_setup() used to be called after the vCPU is realized and > put in PAPR mode. > > Especially, we have these lines: > > 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; > > They depend on env->irq_inputs being allocated. This happens on the > > spapr_cpu_core_realize_child() > { > ... > object_property_set_bool(child, true, "realized", &local_err); > object_property_set_bool() > object_property_set_qobject() > object_property_set() > property_set_bool() > device_set_realized() > ppc_cpu_realizefn() > init_ppc_proc() > init_proc_POWER8() > ppcPOWER7_irq_init() > > and, when using xics_kvm: > > icp_kvm_cpu_setup() > { > ... > ret = kvm_vcpu_enable_cap(cs, KVM_CAP_IRQ_XICS, 0, kernel_xics_fd, vcpu_id); > > This ioctl returns EINVAL because it fails the kvmppc_sanity_check() in KVM. > > It depends on QEMU enabling KVM_CAP_PPC_PAPR for the vCPU, which happens in: > > spapr_cpu_core_realize_child() > { > ... > spapr_cpu_init(spapr, cpu, &local_err); > cpu_ppc_set_papr() > kvmppc_set_papr() > >>> Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't >>> needed anymore and can be safely dropped. >>> >>> Signed-off-by: Greg Kurz <groug@kaod.org> >> >> >> Looks pretty good, though a couple nits below. >> > > Thanks. I'm also thinking about going one step further and converting > xics_kvm to use the icpc->realize() handler instead of icpc->cpu_setup(). > Makes sense ? It should be fine I think, now that a reset handler is defined. C. >>> --- >>> 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. >> > > Right. I'll fix this. > >>> + if (!obj) { >>> + error_setg(errp, "%s: required link 'xics' not found: %s", >> >> Copy/paste mistake in this message. >> > > Oops :) > >>> + __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); >>> >> >
On Thu, 8 Jun 2017 11:25:52 +0200 Cédric Le Goater <clg@kaod.org> wrote: > On 06/08/2017 11:14 AM, Greg Kurz wrote: > > On Thu, 8 Jun 2017 11:53:29 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > >> On Wed, Jun 07, 2017 at 10:55:07PM +0200, Greg Kurz wrote: > >>> On Wed, 7 Jun 2017 20:11:38 +0200 > >>> Cédric Le Goater <clg@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. > >> > >> Right. I think it makes sense for the ICP to have a handle on it's > >> associated CPU, even if we don't actually use it in all cases right > >> now. So I have no problem with the property being in all ICPs; I > >> think that will be cleaner than special casing xics_kvm. Especially > >> if we have to un-special-case it sometime in future because we need > >> to access the CPU object for some reason we haven't thought of yet. > >> > > > > We had discussed with Cedric about that actually but when I started > > to write code, I had the impression that I would have to do convoluted > > things to get rid of the CPU handle in ICP. > > There is nothing complex and it would surely simplify cpu_setup(). > > But, the main argument is that it might be useful to other platforms. > So there is not much value in removing it. I am OK with that. I am less > OK with using the index, even if it is useful for the migration state > of ICP objects today. > > We could be tempted to use more of cpu_index, like we have done in > the past, and that was really complex to untangle. Let's be cautious. > I agree. Maybe this could be hidden in a icpc->get_index() handler ? > C.
On Thu, Jun 08, 2017 at 10:45:30AM +0200, Greg Kurz wrote: > On Thu, 8 Jun 2017 12:01:12 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > 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? > > > > I'm not sure about what you're asking... I had to re-order because > xics_cpu_setup() used to be called after the vCPU is realized and > put in PAPR mode. Duh, sorry, I wasn't thinking to ask about realize order, since that's manual and you've re-ordered it to be correct. You also mention that reset order matters, and I'm less clear on what guarantees that the reset handlers for the components get called in the right order.
On Fri, 9 Jun 2017 12:24:52 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Thu, Jun 08, 2017 at 10:45:30AM +0200, Greg Kurz wrote: > > On Thu, 8 Jun 2017 12:01:12 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > 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? > > > > > > > I'm not sure about what you're asking... I had to re-order because > > xics_cpu_setup() used to be called after the vCPU is realized and > > put in PAPR mode. > > Duh, sorry, I wasn't thinking to ask about realize order, since that's > manual and you've re-ordered it to be correct. > > You also mention that reset order matters, and I'm less clear on what > guarantees that the reset handlers for the components get called in > the right order. > Oops... my bad, this is a mistake in the changelog. The KVM error I was seing isn't related to CPU reset as I was thinking first but to cpu_ppc_set_papr()... :-\ The last sentence should rather be something like: "We also need to call spapr_cpu_init() before ICPState realization in order to enable PAPR mode in KVM." Can you fix this in your tree or should I send an updated version ? Cheers, -- Greg
On Fri, Jun 09, 2017 at 08:45:50AM +0200, Greg Kurz wrote: > On Fri, 9 Jun 2017 12:24:52 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Thu, Jun 08, 2017 at 10:45:30AM +0200, Greg Kurz wrote: > > > On Thu, 8 Jun 2017 12:01:12 +1000 > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > 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? > > > > > > > > > > I'm not sure about what you're asking... I had to re-order because > > > xics_cpu_setup() used to be called after the vCPU is realized and > > > put in PAPR mode. > > > > Duh, sorry, I wasn't thinking to ask about realize order, since that's > > manual and you've re-ordered it to be correct. > > > > You also mention that reset order matters, and I'm less clear on what > > guarantees that the reset handlers for the components get called in > > the right order. > > > > Oops... my bad, this is a mistake in the changelog. The KVM error I was > seing isn't related to CPU reset as I was thinking first but to > cpu_ppc_set_papr()... :-\ > > The last sentence should rather be something like: > > "We also need to call spapr_cpu_init() before ICPState realization in > order to enable PAPR mode in KVM." > > Can you fix this in your tree or should I send an updated version ? Uh.. too late, I already sent a pull request.
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); + + 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);
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. Signed-off-by: Greg Kurz <groug@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(-)