On Thu, 8 Jun 2017 12:01:12 +1000 David Gibson <da...@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 <gr...@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);
> >
>
pgp386fnk5Gcd.pgp
Description: OpenPGP digital signature
