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

[Greg Kurz]

On Wed, 7 Jun 2017 20:11:38 +0200
Cédric Le Goater <address@hidden> 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 <address@hidden>
> > ---
> >  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);
> >   
>

pgpjEtj2C9uFA.pgp
Description: OpenPGP digital signature