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

[Cédric Le Goater]

On 06/07/2017 10:55 PM, Greg Kurz wrote:
> 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.

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

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