[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 11/20] spapr: Fix indexing of XICS irqs
From: |
Greg Kurz |
Subject: |
Re: [PATCH 11/20] spapr: Fix indexing of XICS irqs |
Date: |
Thu, 26 Sep 2019 09:21:41 +0200 |
On Thu, 26 Sep 2019 11:31:48 +1000
David Gibson <address@hidden> wrote:
> On Wed, Sep 25, 2019 at 10:17:46PM +0200, Greg Kurz wrote:
> > On Wed, 25 Sep 2019 16:45:25 +1000
> > David Gibson <address@hidden> wrote:
> >
> > > spapr global irq numbers are different from the source numbers on the ICS
> > > when using XICS - they're offset by XICS_IRQ_BASE (0x1000). But
> > > spapr_irq_set_irq_xics() was passing through the global irq number to
> > > the ICS code unmodified.
> > >
> > > We only got away with this because of a counteracting bug - we were
> > > incorrectly adjusting the qemu_irq we returned for a requested global irq
> > > number.
> > >
> > > That approach mostly worked but is very confusing, incorrectly relies on
> > > the way the qemu_irq array is allocated, and undermines the intention of
> > > having the global array of qemu_irqs for spapr have a consistent meaning
> > > regardless of irq backend.
> > >
> > > So, fix both set_irq and qemu_irq indexing. We rename some parameters at
> > > the same time to make it clear that they are referring to spapr global
> > > irq numbers.
> > >
> > > Signed-off-by: David Gibson <address@hidden>
> > > ---
> >
> > Reviewed-by: Greg Kurz <address@hidden>
> >
> > Further cleanup could be to have the XICS backend to only take global
> > irq numbers and to convert them to ICS source numbers internally. This
> > would put an end to the confusion between srcno/irq in the frontend
> > code.
>
> Yeah, maybe. But the local srcnos do actually make sense from within
> the perspective of ICS, so I'm not all that keen to do that.
>
Not sure to understand what you mean by "within the perspective of ICS".
My concern is actually to get rid of ics->offset users in spapr_irq.c.
eg,
static void spapr_irq_set_irq_xics(void *opaque, int irq, int val)
{
SpaprMachineState *spapr = opaque;
uint32_t srcno = irq - spapr->ics->offset;
ics_set_irq(spapr->ics, srcno, val);
}
It looks like we should do something like:
static void spapr_irq_set_irq_xics(void *opaque, int irq, int val)
{
SpaprMachineState *spapr = opaque;
ics_set_irq(spapr->ics, irq, val);
}
and have ics_set_irq() do:
uint32_t srcno = irq - spapr->ics->offset;
Are you inferring that it is better to keep the irq to srcno conversions
in spapr_irq.c ?
pgpswrvSv8jC1.pgp
Description: OpenPGP digital signature
- Re: [PATCH 14/20] spapr: Remove unhelpful tracepoints from spapr_irq_free_xics(), (continued)
[PATCH 16/20] spapr, xics, xive: Better use of assert()s on irq claim/free paths, David Gibson, 2019/09/25
[PATCH 09/20] spapr: Clarify and fix handling of nr_irqs, David Gibson, 2019/09/25