hw/intc/arm_gicv3: Update cached state after LPI state changes

The logic of gicv3_redist_update() is as follows:
 * it must be called in any code path that changes the state of
   (only) redistributor interrupts
 * if it finds a redistributor interrupt that is (now) higher
   priority than the previous highest-priority pending interrupt,
   then this must be the new highest-priority pending interrupt
 * if it does *not* find a better redistributor interrupt, then:
    - if the previous state was "no interrupts pending" then
      the new state is still "no interrupts pending"
    - if the previous best interrupt was not a redistributor
      interrupt then that remains the best interrupt
    - if the previous best interrupt *was* a redistributor interrupt,
      then the new best interrupt must be some non-redistributor
      interrupt, but we don't know which so must do a full scan

In commit 17fb5e36aa we effectively added the LPI interrupts
as a kind of "redistributor interrupt" for this purpose, by adding
cs->hpplpi to the set of things that gicv3_redist_update() considers
before it gives up and decides to do a full scan of distributor
interrupts. However we didn't quite get this right:
 * the condition check for "was the previous best interrupt a
   redistributor interrupt" must be updated to include LPIs
   in what it considers to be redistributor interrupts
 * every code path which updates the LPI state which
   gicv3_redist_update() checks must also call gicv3_redist_update():
   this is cs->hpplpi and the GICR_CTLR ENABLE_LPIS bit

This commit fixes this by:
 * correcting the test on cs->hppi.irq in gicv3_redist_update()
 * making gicv3_redist_update_lpi() always call gicv3_redist_update()
 * introducing a new gicv3_redist_update_lpi_only() for the one
   callsite (the post-load hook) which must not call
   gicv3_redist_update()
 * making gicv3_redist_lpi_pending() always call gicv3_redist_update(),
   either directly or via gicv3_redist_update_lpi()
 * removing a couple of now-unnecessary calls to gicv3_redist_update()
   from some callers of those two functions
 * calling gicv3_redist_update() when the GICR_CTLR ENABLE_LPIS
   bit is cleared

(This means that the not-file-local gicv3_redist_* LPI related
functions now all take care of the updates of internally cached
GICv3 information, in the same way the older functions
gicv3_redist_set_irq() and gicv3_redist_send_sgi() do.)

The visible effect of this bug was that when the guest acknowledged
an LPI by reading ICC_IAR1_EL1, we marked it as not pending in the
LPI data structure but still left it in cs->hppi so we would offer it
to the guest again.  In particular for setups using an emulated GICv3
and ITS and using devices which use LPIs (ie PCI devices) a Linux
guest would complain "irq 54: nobody cared" and then hang.  (The hang
was intermittent, presumably depending on the timing between
different interrupts arriving and being completed.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Tested-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Message-id: 20211124202005.989935-1-peter.maydell@linaro.org
This commit is contained in:
Peter Maydell 2021-11-24 20:20:05 +00:00
parent 2f459cd1a8
commit 101f27f3c8
3 changed files with 31 additions and 6 deletions

View file

@ -256,9 +256,10 @@ static MemTxResult gicr_writel(GICv3CPUState *cs, hwaddr offset,
cs->gicr_ctlr |= GICR_CTLR_ENABLE_LPIS;
/* Check for any pending interr in pending table */
gicv3_redist_update_lpi(cs);
gicv3_redist_update(cs);
} else {
cs->gicr_ctlr &= ~GICR_CTLR_ENABLE_LPIS;
/* cs->hppi might have been an LPI; recalculate */
gicv3_redist_update(cs);
}
}
return MEMTX_OK;
@ -571,7 +572,7 @@ static void gicv3_redist_check_lpi_priority(GICv3CPUState *cs, int irq)
}
}
void gicv3_redist_update_lpi(GICv3CPUState *cs)
void gicv3_redist_update_lpi_only(GICv3CPUState *cs)
{
/*
* This function scans the LPI pending table and for each pending
@ -614,6 +615,12 @@ void gicv3_redist_update_lpi(GICv3CPUState *cs)
}
}
void gicv3_redist_update_lpi(GICv3CPUState *cs)
{
gicv3_redist_update_lpi_only(cs);
gicv3_redist_update(cs);
}
void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int level)
{
/*
@ -651,6 +658,7 @@ void gicv3_redist_lpi_pending(GICv3CPUState *cs, int irq, int level)
*/
if (level) {
gicv3_redist_check_lpi_priority(cs, irq);
gicv3_redist_update(cs);
} else {
if (irq == cs->hpplpi.irq) {
gicv3_redist_update_lpi(cs);
@ -673,8 +681,6 @@ void gicv3_redist_process_lpi(GICv3CPUState *cs, int irq, int level)
/* set/clear the pending bit for this irq */
gicv3_redist_lpi_pending(cs, irq, level);
gicv3_redist_update(cs);
}
void gicv3_redist_set_irq(GICv3CPUState *cs, int irq, int level)