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

[Peter Maydell]

On 28 January 2014 02:09, Christoffer Dall <address@hidden> wrote:
> On Sun, Jan 19, 2014 at 07:49:58PM +0000, Peter Maydell wrote:
>> 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)
>
> ok, it's probably cleaner code-wise, but should produce the same result
> as far as I can see.  I guess I see the pending field as the result of
> the final or-gate (possibly because I'm too tied to the way the
> in-kernel emulation works where we don't have a separate 'level' state).
> Anyway, I'll respin the patch according to your directions (my only
> concern with that is that it feels a bit unlogical to have a bitfield
> called pending where some of the bits are not set despite the
> corresponding interrupts being pending, but we'll see if the overall
> code looks more clean with this approach).

The key thing is that we definitely shouldn't be keeping more
state than the hardware does; that's always a bad sign.

thanks
-- PMM