Re: [Qemu-devel] [PATCH v4 13/20] ppc/xics: introduce helpers to find an ICP from some (CPU) index

[Cédric Le Goater]

On 10/14/2016 07:34 AM, David Gibson wrote:
> On Mon, Oct 03, 2016 at 09:24:49AM +0200, Cédric Le Goater wrote:
>> Today, the Interrupt Presentation Controllers (ICP) are stored in an
>> array under the base class XICSState. They are simply indexed using
>> the CPU index field of CPUState. This made sense for the current
>> derived classes, spapr and kvm, as the CPU index are contiguous.
>> Nevertheless some problems have started to occur with CPU hotplug.
>>
>> With the PowerNV platform CPUs, this is not true anymore. Real HW ids
>> are being used and they are not contiguous. So we need a way to
>> customize the lookups in the array. Below is a proposal for this
>> purpose. A couple of helpers are introduced to hide the nature of the
>> underlying ICP array and also a new XICSStateClass method 'find_icp'
>> to let the derived classes customize the ICP lookups.
>>
>> A followup patch would be to let the derived class decide on the ICP
>> storage. They could use a hash table for instance. We would need to
>> introduce a new class method 'get_icp' for that. Or simply, change the
>> ICP array for a hash table and let the derived class decide on the key
>> to use.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
> 
> Uuuh.. IIRC this series has now been reworked (as suggested) to keep
> cpu_index contiguous and have a separate hardware id.  Doesn't that
> mean this patch can go away (though the callers might need to use a
> hwid->cpu_index helper).

Yes. This is how it is implemented. 

The pnv machine only handles cpu hwid and it uses a custom find handler
to look for an ICP. But I see you made comments on the implementation.

Thanks,

C.   


>> ---
>>  hw/intc/xics.c        | 48 ++++++++++++++++++++++++++++++++++++------------
>>  hw/intc/xics_kvm.c    |  7 ++-----
>>  hw/intc/xics_spapr.c  | 12 ++++++------
>>  include/hw/ppc/xics.h |  2 ++
>>  4 files changed, 46 insertions(+), 23 deletions(-)
>>
>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>> index 3bbbcc847791..876c472aaa69 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -48,12 +48,32 @@ int xics_get_cpu_index_by_dt_id(int cpu_dt_id)
>>      return -1;
>>  }
>>  
>> +ICPState *xics_find_icp(XICSState *xics, int cpu_index)
>> +{
>> +    XICSStateClass *xsc = XICS_COMMON_GET_CLASS(xics);
>> +    ICPState *icp = xsc->find_icp(xics, cpu_index);
>> +
>> +    assert(icp);
>> +
>> +    return icp;
>> +}
>> +
>> +static ICPState *xics_get_icp(XICSState *xics, CPUState *cs)
>> +{
>> +    ICPState *ss;
>> +
>> +    assert(cs->cpu_index < xics->nr_servers);
>> +
>> +    ss = &xics->ss[cs->cpu_index];
>> +    ss->cs = cs;
>> +    return ss;
>> +}
>> +
>>  void xics_cpu_destroy(XICSState *xics, PowerPCCPU *cpu)
>>  {
>>      CPUState *cs = CPU(cpu);
>> -    ICPState *ss = &xics->ss[cs->cpu_index];
>> +    ICPState *ss = xics_find_icp(xics, cs->cpu_index);
>>  
>> -    assert(cs->cpu_index < xics->nr_servers);
>>      assert(cs == ss->cs);
>>  
>>      ss->output = NULL;
>> @@ -64,13 +84,9 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
>>  {
>>      CPUState *cs = CPU(cpu);
>>      CPUPPCState *env = &cpu->env;
>> -    ICPState *ss = &xics->ss[cs->cpu_index];
>> +    ICPState *ss = xics_get_icp(xics, cs);
>>      XICSStateClass *info = XICS_COMMON_GET_CLASS(xics);
>>  
>> -    assert(cs->cpu_index < xics->nr_servers);
>> -
>> -    ss->cs = cs;
>> -
>>      if (info->cpu_setup) {
>>          info->cpu_setup(xics, cpu);
>>      }
>> @@ -94,6 +110,12 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
>>  /*
>>   * XICS Common class - parent for emulated XICS and KVM-XICS
>>   */
>> +
>> +static ICPState *xics_common_find_icp(XICSState *xics, int cpu_index)
>> +{
>> +    return &xics->ss[cpu_index];
>> +}
>> +
>>  static void xics_common_reset(DeviceState *d)
>>  {
>>      XICSState *xics = XICS_COMMON(d);
>> @@ -191,8 +213,10 @@ static void xics_common_initfn(Object *obj)
>>  static void xics_common_class_init(ObjectClass *oc, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(oc);
>> +    XICSStateClass *xsc = XICS_COMMON_CLASS(oc);
>>  
>>      dc->reset = xics_common_reset;
>> +    xsc->find_icp = xics_common_find_icp;
>>  }
>>  
>>  static const TypeInfo xics_common_info = {
>> @@ -261,7 +285,7 @@ static void icp_check_ipi(ICPState *ss)
>>  
>>  static void icp_resend(XICSState *xics, int server)
>>  {
>> -    ICPState *ss = xics->ss + server;
>> +    ICPState *ss = xics_find_icp(xics, server);
>>      ICSState *ics;
>>  
>>      if (ss->mfrr < CPPR(ss)) {
>> @@ -274,7 +298,7 @@ static void icp_resend(XICSState *xics, int server)
>>  
>>  void icp_set_cppr(XICSState *xics, int server, uint8_t cppr)
>>  {
>> -    ICPState *ss = xics->ss + server;
>> +    ICPState *ss = xics_find_icp(xics, server);
>>      uint8_t old_cppr;
>>      uint32_t old_xisr;
>>  
>> @@ -301,7 +325,7 @@ void icp_set_cppr(XICSState *xics, int server, uint8_t 
>> cppr)
>>  
>>  void icp_set_mfrr(XICSState *xics, int server, uint8_t mfrr)
>>  {
>> -    ICPState *ss = xics->ss + server;
>> +    ICPState *ss = xics_find_icp(xics, server);
>>  
>>      ss->mfrr = mfrr;
>>      if (mfrr < CPPR(ss)) {
>> @@ -333,7 +357,7 @@ uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr)
>>  
>>  void icp_eoi(XICSState *xics, int server, uint32_t xirr)
>>  {
>> -    ICPState *ss = xics->ss + server;
>> +    ICPState *ss = xics_find_icp(xics, server);
>>      ICSState *ics;
>>      uint32_t irq;
>>  
>> @@ -354,7 +378,7 @@ void icp_eoi(XICSState *xics, int server, uint32_t xirr)
>>  static void icp_irq(ICSState *ics, int server, int nr, uint8_t priority)
>>  {
>>      XICSState *xics = ics->xics;
>> -    ICPState *ss = xics->ss + server;
>> +    ICPState *ss = xics_find_icp(xics, server);
>>  
>>      trace_xics_icp_irq(server, nr, priority);
>>  
>> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
>> index 9c2f198fd142..b666bb59fc24 100644
>> --- a/hw/intc/xics_kvm.c
>> +++ b/hw/intc/xics_kvm.c
>> @@ -326,14 +326,11 @@ static const TypeInfo ics_kvm_info = {
>>   */
>>  static void xics_kvm_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
>>  {
>> -    CPUState *cs;
>> -    ICPState *ss;
>> +    CPUState *cs = CPU(cpu);
>> +    ICPState *ss = xics_find_icp(xics, cs->cpu_index);
>>      KVMXICSState *xicskvm = XICS_SPAPR_KVM(xics);
>>      int ret;
>>  
>> -    cs = CPU(cpu);
>> -    ss = &xics->ss[cs->cpu_index];
>> -
>>      assert(cs->cpu_index < xics->nr_servers);
>>      if (xicskvm->kernel_xics_fd == -1) {
>>          abort();
>> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
>> index e8d0623c2cb5..af29998b1255 100644
>> --- a/hw/intc/xics_spapr.c
>> +++ b/hw/intc/xics_spapr.c
>> @@ -67,9 +67,9 @@ static target_ulong h_xirr(PowerPCCPU *cpu, 
>> sPAPRMachineState *spapr,
>>                             target_ulong opcode, target_ulong *args)
>>  {
>>      CPUState *cs = CPU(cpu);
>> -    uint32_t xirr = icp_accept(spapr->xics->ss + cs->cpu_index);
>> +    ICPState *ss = xics_find_icp(spapr->xics, cs->cpu_index);
>>  
>> -    args[0] = xirr;
>> +    args[0] = icp_accept(ss);
>>      return H_SUCCESS;
>>  }
>>  
>> @@ -77,10 +77,9 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu, 
>> sPAPRMachineState *spapr,
>>                               target_ulong opcode, target_ulong *args)
>>  {
>>      CPUState *cs = CPU(cpu);
>> -    ICPState *ss = &spapr->xics->ss[cs->cpu_index];
>> -    uint32_t xirr = icp_accept(ss);
>> +    ICPState *ss = xics_find_icp(spapr->xics, cs->cpu_index);
>>  
>> -    args[0] = xirr;
>> +    args[0] = icp_accept(ss);
>>      args[1] = cpu_get_host_ticks();
>>      return H_SUCCESS;
>>  }
>> @@ -99,8 +98,9 @@ static target_ulong h_ipoll(PowerPCCPU *cpu, 
>> sPAPRMachineState *spapr,
>>                              target_ulong opcode, target_ulong *args)
>>  {
>>      CPUState *cs = CPU(cpu);
>> +    ICPState *ss = xics_find_icp(spapr->xics, cs->cpu_index);
>>      uint32_t mfrr;
>> -    uint32_t xirr = icp_ipoll(spapr->xics->ss + cs->cpu_index, &mfrr);
>> +    uint32_t xirr = icp_ipoll(ss, &mfrr);
>>  
>>      args[0] = xirr;
>>      args[1] = mfrr;
>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>> index ca9f8da542e0..52c426d409c9 100644
>> --- a/include/hw/ppc/xics.h
>> +++ b/include/hw/ppc/xics.h
>> @@ -76,6 +76,7 @@ struct XICSStateClass {
>>      void (*cpu_setup)(XICSState *icp, PowerPCCPU *cpu);
>>      void (*set_nr_irqs)(XICSState *icp, uint32_t nr_irqs, Error **errp);
>>      void (*set_nr_servers)(XICSState *icp, uint32_t nr_servers, Error 
>> **errp);
>> +    ICPState *(*find_icp)(XICSState *xics, int cpu_index);
>>  };
>>  
>>  struct XICSState {
>> @@ -206,5 +207,6 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool 
>> lsi);
>>  ICSState *xics_find_source(XICSState *icp, int irq);
>>  
>>  void xics_hmp_info_pic(Monitor *mon, const QDict *qdict);
>> +ICPState *xics_find_icp(XICSState *xics, int cpu_index);
>>  
>>  #endif /* XICS_H */
>