Re: [Qemu-devel] [RFC PATCH v4 3/8] arm_gic: Fix GIC pending behavior

[Peter Maydell]

On 21 December 2013 06:09, Christoffer Dall <address@hidden> wrote:
> The existing implementation of the pending behavior in gic_set_irq,
> gic_acknowledge_irq, gic_complete_irq, and the distributor pending
> set/clear registers does not follow the semantics of the GICv2.0 specs,
> but may implement the 11MPCore support.  Therefore, maintain the
> existing semantics for 11MPCore and change the behavior to be in
> accordance with the GICv2.0 specs for "generic implementations"
> (s->revision == 1 || s->revision == 2).
>
> Generic implementations also mandate that level-triggered interrupts
> that are pending due to a write to GICD_ISPENDR stay pending until the
> interrupt is acknowledged or until there's a write to the GICD_ICPENDR.
> Similarly, only clear the pending state on writes to GICD_ICPENDR if the
> interrupt signal is not asserted.  This requires an extra state variable
> to keep track of writes to the set/clear registers, corresponding to the
> D-flip flop in the GICv2.0 specs Figure 4-10.

So this means we now have two lots of pending state, the pending
and sw_pending fields. I think this is not in fact correct, and there
is only one lot of state in a real h/w GIC (it's that flipflop). AIUI the
correct behaviour is:

  * GIC_TEST_PENDING() should read:
    ((s->irq_state[irq].pending & (cm) != 0) ||
       (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_LEVEL(irq, cm))
     -- this corresponds to the OR gate in fig 4-10; this is the value
    to be used in reads from ICPENDR/ISPENDR and other "is this
    interrupt pending" checks like the one in gic_update()
  * in gic_set_irq(), if level is 1 (rising edge) we GIC_SET_PENDING
     only if the interrupt is edge-triggered (pending status isn't latched
     for level triggered interrupts). We always GIC_SET_LEVEL(irq, 1).
  * in gic_set_irq(), if level is 0 (falling edge) we don't touch the pending
    state, but just GIC_SET_LEVEL(irq, 0)
  * gic_acknowledge_irq() always does a GIC_CLEAR_PENDING()
    (this won't actually make a level-triggered interrupt be not pending
    because of the second clause of our updated GIC_TEST_PENDING)
  * writes to the I[CS]PENDR registers always result in GIC_SET_PENDING
    or GIC_CLEAR_PENDING calls (updating the latch state)

(The above applies to GIC v1/v2 only; 11mpcore wants to keep the
current GIC_TEST_PENDING definition, etc, but I figured it would
be clearer to just describe the v1/v2 behaviour without peppering it
with "not 11mpcore" caveats.)

The v7M NVIC (ie s->rev ==REV_NVIC) should behave like the
11MPcore GIC. (At least, the v7M specified behaviour for the
pending bits is that they are cleared on exception entry and then
the interrupt re-pends if the signal is still asserted on exception
exit. I suspect that there are cases where v7M diverges from
11MPCore GIC behaviour, but that's what we're currently treating
it like, so leaving it that way will not be a regression.)

I haven't thought too hard about how this should interact with the
KVM GIC save/restore interface, but I think that it should be OK
to just do a simple "save/load the value of the I*PENDR registers",
because the only case where this isn't just a simple replication
of the source state to the destination is:
 * the source has a 1 bit in the pending register because of a
   level triggered interrupt whose input is currently asserted,
   but its pending latch is not set
 * in migration we write a 1 bit into the destination pending
   register, which causes the destination's pending latch to be
   set even though it was clear on the source
 * however this is fairly harmless, because when the guest takes
   the interrupt after migration (which it would have done anyway
   because of the asserted input) the interrupt acknowlege process
   will clear the pending latch and we'll be back in sync with
   where we should be
I think it's technically possible to write a guest which can detect
the difference (it would have to disable interrupts, prod the device
to assert its interrupt, clear the interrupt status on the device and
then reenable interrupts again, at which point if we've migrated
in the middle of that we get a single spurious interrupt). But I would
be entirely unsurprised if the guest saw the same behaviour if
it was running on bare metal and there was a power management
CPU & GIC power-down/power-up transition, since the power
management code also has to do GIC state save and restore...
In any case any non-perverse guest should never notice.

Apologies for the "you need to rewrite this" review at this late
stage, but I didn't find the necessary information about the
behaviour here until the very tail end of last year.

> @@ -423,25 +470,38 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>          irq = (offset - 0x200) * 8 + GIC_BASE_IRQ;
>          if (irq >= s->num_irq)
>              goto bad_reg;
> -        if (irq < 16)
> -          irq = 0;
> +        if (irq < GIC_NR_SGIS)
> +          value = 0;

This change is correct but it should be its own patch since it's a
standalone bugfix.

Minor style note: coding style demands braces on all if() statements,
even one-liners like this one (and even if you're just making a slight
tweak to code that was already present and not coding-standard
compliant). scripts/checkpatch.pl ought to spot this.

> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -27,6 +27,7 @@
>  #define GIC_MAXIRQ 1020
>  /* First 32 are private to each CPU (SGIs and PPIs). */
>  #define GIC_INTERNAL 32
> +#define GIC_NR_SGIS 16

If we're going to introduce this (and it seems like a good idea) we should
use it more consistently in the places that currently have a hardcoded "16".
(You might want to make that a separate patch.)

thanks
-- PMM