Re: [Qemu-ppc] [PATCH v3 3/5] xics: setup cpu at realize time

[Greg Kurz]

On Thu, 8 Jun 2017 12:01:12 +1000
David Gibson <address@hidden> 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 <address@hidden>  
> 
> 
> 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);
> >   
>

pgphP16k6kzt_.pgp
Description: OpenPGP digital signature