Re: [Qemu-devel] [PATCH v1 06/11] ppc/xics: Make the ICSState a list

[David Gibson]

On Mon, Jun 27, 2016 at 10:26:42AM +0530, Nikunj A Dadhania wrote:
> David Gibson <address@hidden> writes:
> 
> > [ Unknown signature status ]
> > On Thu, Jun 23, 2016 at 11:17:25PM +0530, Nikunj A Dadhania wrote:
> >> From: Benjamin Herrenschmidt <address@hidden>
> >> 
> >> Instead of an array of fixed sized blocks, use a list, as we will need
> >> to have sources with variable number of interrupts. SPAPR only uses
> >> a single entry. Native will create more. If performance becomes an
> >> issue we can add some hashed lookup but for now this will do fine.
> >> 
> >> Signed-off-by: Benjamin Herrenschmidt <address@hidden>
> >> Signed-off-by: Nikunj A Dadhania <address@hidden>
> >> ---
> >>  hw/intc/xics.c        | 78 
> >> +++++++++++++++++++++++++++---------------------
> >>  hw/intc/xics_kvm.c    | 29 +++++++++++++-----
> >>  hw/intc/xics_spapr.c  | 82 
> >> +++++++++++++++++++++++++++++----------------------
> >>  hw/ppc/spapr_events.c |  2 +-
> >>  hw/ppc/spapr_pci.c    |  5 ++--
> >>  hw/ppc/spapr_vio.c    |  2 +-
> >>  include/hw/ppc/xics.h | 13 ++++----
> >>  7 files changed, 124 insertions(+), 87 deletions(-)
> >> 
> >> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> >> index 38e51fc..ef2a1e4 100644
> >> --- a/hw/intc/xics.c
> >> +++ b/hw/intc/xics.c
> >> @@ -96,13 +96,16 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
> >>  static void xics_common_reset(DeviceState *d)
> >>  {
> >>      XICSState *xics = XICS_COMMON(d);
> >> +    ICSState *ics;
> >>      int i;
> >>  
> >>      for (i = 0; i < xics->nr_servers; i++) {
> >>          device_reset(DEVICE(&xics->ss[i]));
> >>      }
> >>  
> >> -    device_reset(DEVICE(xics->ics));
> >> +    QLIST_FOREACH(ics, &xics->ics, list) {
> >> +        device_reset(DEVICE(ics));
> >> +    }
> >>  }
> >>  
> >>  static void xics_prop_get_nr_irqs(Object *obj, Visitor *v, const char 
> >> *name,
> >> @@ -134,7 +137,6 @@ static void xics_prop_set_nr_irqs(Object *obj, Visitor 
> >> *v, const char *name,
> >>      }
> >>  
> >>      assert(info->set_nr_irqs);
> >> -    assert(xics->ics);
> >>      info->set_nr_irqs(xics, value, errp);
> >>  }
> >>  
> >> @@ -212,33 +214,35 @@ static void ics_reject(ICSState *ics, int nr);
> >>  static void ics_resend(ICSState *ics);
> >>  static void ics_eoi(ICSState *ics, int nr);
> >>  
> >> -static void icp_check_ipi(XICSState *xics, int server)
> >> +static void icp_check_ipi(ICPState *ss, int server)
> >
> > Since you're now passing ICPState, you don't need the server
> > parameter, since its only purpose is to locate the right ICPState in
> > the XICSState.
> 
> Right, i had retained this as there was a trace function using it. Maybe
> we can then move the trace to the caller.

Or just change the trace template.

> >> @@ -361,13 +375,16 @@ int xics_spapr_alloc(XICSState *xics, int src, int 
> >> irq_hint, bool lsi,
> >>   * Allocate block of consecutive IRQs, and return the number of the first 
> >> IRQ in
> >>   * the block. If align==true, aligns the first IRQ number to num.
> >>   */
> >> -int xics_spapr_alloc_block(XICSState *xics, int src, int num, bool lsi,
> >> -                           bool align, Error **errp)
> >> +int xics_spapr_alloc_block(XICSState *xics, int num, bool lsi, bool align,
> >> +                           Error **errp)
> >>  {
> >> +    ICSState *ics = QLIST_FIRST(&xics->ics);
> >>      int i, first = -1;
> >> -    ICSState *ics = &xics->ics[src];
> >
> > Ouch.. AFAICT this would have SEGVed if it was ever called with src != 0.
> >
> >>  
> >> -    assert(src == 0);
> >> +    if (!ics) {
> >> +        return -1;
> >> +    }
> >> +
> >>      /*
> >>       * MSIMesage::data is used for storing VIRQ so
> >>       * it has to be aligned to num to support multiple
> >> @@ -394,7 +411,7 @@ int xics_spapr_alloc_block(XICSState *xics, int src, 
> >> int num, bool lsi,
> >>      }
> >>      first += ics->offset;
> >>  
> >> -    trace_xics_alloc_block(src, first, num, lsi, align);
> >> +    trace_xics_alloc_block(0, first, num, lsi, align);
> >
> > You should remove the src from the trave definition since it has to be
> > 0.
> 
> Sure.
> 
> >>  
> >>      return first;
> >>  }
> >> @@ -405,7 +422,7 @@ static void ics_free(ICSState *ics, int srcno, int num)
> >>  
> >>      for (i = srcno; i < srcno + num; ++i) {
> >>          if (ICS_IRQ_FREE(ics, i)) {
> >> -            trace_xics_ics_free_warn(ics - ics->xics->ics, i + 
> >> ics->offset);
> >> +            trace_xics_ics_free_warn(0, i + ics->offset);
> >
> > Likewise here you should remove the always 0 parameter from the trace
> > definition.
> 
> Sure.
> 
> Regards
> Nikunj
> 

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