Re: [PATCH 11/20] spapr: Fix indexing of XICS irqs

[Greg Kurz]

On Thu, 26 Sep 2019 21:32:18 +1000
David Gibson <address@hidden> wrote:

> On Thu, Sep 26, 2019 at 09:21:41AM +0200, Greg Kurz wrote:
> > 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 ?
> 
> Ah, I see what you mean.  So, the reason srcno appears here is that
> ics_set_irq() is arguably an internal xics function that we shouldn't
> be calling directly from here, but do because of.. reasons.
> 
> As it happens, I have another patch in the works which will move this
> function back into xics_spapr.c and bind it more tightly to the xics
> code, so I think that will address your concern.
> 

Great ! :)

pgpiXUtnrBm5b.pgp
Description: OpenPGP digital signature