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

[Cédric Le Goater]

On 06/08/2017 10:45 AM, Greg Kurz wrote:
> 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 ?

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);
>>>   
>>
>