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

[David Gibson]

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

> ---
>  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 */

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature