On Mon, Jun 27, 2016 at 10:26:42AM +0530, Nikunj A Dadhania wrote:
> David Gibson <da...@gibson.dropbear.id.au> writes:
>
> > [ Unknown signature status ]
> > On Thu, Jun 23, 2016 at 11:17:25PM +0530, Nikunj A Dadhania wrote:
> >> From: Benjamin Herrenschmidt <b...@kernel.crashing.org>
> >>
> >> 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 <b...@kernel.crashing.org>
> >> Signed-off-by: Nikunj A Dadhania <nik...@linux.vnet.ibm.com>
> >> ---
> >> 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
