Re: [Qemu-devel] arm_gic emulation

[Peter Maydell]

On 25 June 2013 05:16, Christoffer Dall <address@hidden> wrote:
> Hi Peter,
>
> Can you help me understand arm_vgic.c, specifically, see the quoted code
> below and my question:

(arm_gic.c; also, cc'd qemu-devel.)

>> /* Process a change in an external IRQ input.  */
>> static void gic_set_irq(void *opaque, int irq, int level)
>> {
>>     /* Meaning of the 'irq' parameter:
>>      *  [0..N-1] : external interrupts
>>      *  [N..N+31] : PPI (internal) interrupts for CPU 0
>>      *  [N+32..N+63] : PPI (internal interrupts for CPU 1
>>      *  ...
>>      */
>>     GICState *s = (GICState *)opaque;
>>     int cm, target;
>>     if (irq < (s->num_irq - GIC_INTERNAL)) {
>>         /* The first external input line is internal interrupt 32.  */
>>         cm = ALL_CPU_MASK;
>>         irq += GIC_INTERNAL;
>>         target = GIC_TARGET(irq);
>>     } else {
>>         int cpu;
>>         irq -= (s->num_irq - GIC_INTERNAL);
>>         cpu = irq / GIC_INTERNAL;
>>         irq %= GIC_INTERNAL;
>>         cm = 1 << cpu;
>>         target = cm;
>>     }
>>
>>     if (level == GIC_TEST_LEVEL(irq, cm)) {
>>         return;
>>     }
>>
>>     if (level) {
>>         GIC_SET_LEVEL(irq, cm);
>
> If this is an edge-triggered interrupt, is the function called with
> level==0 right after?  I assume so.

Whether the interrupt is edge or level triggered has no bearing
on what the device that sets this input line does. You can have
an IRQ configured for edge triggered where the device just takes
the input line high and leaves it there, or an IRQ configured for
level triggered where the device blips the line briefly (that
would probably indicate that the guest OS had misconfigured
something, though).

>>         if (GIC_TEST_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)) {
>
> The way I read the code is that gic_irq_state->level means the state of
> the input line to the distributor, and gic_irq_state->pending is
> what GICD_ISPENDRn tells you: that the interrupt is pending on at least
> one CPU.
>
> So the question is, why does an edge-triggered interrupt override the
> disabled state?

I suspect the code here and in gic_complete_irq() is an incorrect
attempt at implementing the logic described in figure 4.10 of the
GICv2 spec:
 * for an edge triggered interrupt, we latch the pending status
   when the interrupt is asserted (ie on the edge)
 * for a level triggered interrupt, we are supposed to follow the
   raw state of the input line
(plus in either case a write to GICD_ISPENDRn latches pending to 1)

> A possible answer could be that because disabling the interrupt doesn't
> mean that it cannot become pending (only that it doesn't get forwarded to
> the CPU) we need to remember this state for an edge-triggered interrupt,
> because we can't re-sample the line.
>
> But, looking at the emulation code for GICD_IxPENDRn contradicts this,
> because this returns a raw view of pending IRQs, and the docs clearly
> state that reads should only return 1 for IRQs that are actually pending
> on one or more CPU interfaces (I take this to mean enabled, and thus
> forwarded to the CPU interface)

This is a wrong interpretation. GICv2 spec section 3.2.2 is clear that
interrupts can become pending (and generally move through the state
diagram) whether they're enabled or not. All the 'enabled' state does is
control whether the distributor forwards them to the cpu interfaces.
By "pending on one or more CPU interfaces" the spec is referring to
the fact that there is a state machine for each (interrupt, cpu) tuple,
so IRQ 23 could be in the pending state for cpu0 but not for cpu1
(regardless of whether either of those cpus has irq 23 enabled).

> If I read the specs wrong, and pending simply is:
>  - either an edge-triggered line was raised
>  - a level triggered line is held high (assuming active-HIGH)
> and GICD_IxPENDRn returns a raw view of the above, well then I don't
> understand the second operand to the if statement, because it shouldn't
> matter if the interrupt is enabled or disabled, it's just the raw
> hardware state.

Yes, it's the level-triggered part of the logic that's broken.

>>             DPRINTF("Set %d pending mask %x\n", irq, target);
>>             GIC_SET_PENDING(irq, target);
>>         }
>>     } else {
>>         GIC_CLEAR_LEVEL(irq, cm);
>>     }
>>     gic_update(s);
>> }
>
> There's an extra technicality based on my reading of the GIC specs that
> make me think we're missing a bit of state in the kernel.  If we look
> at the handling of GICD_ICPENDRn, then we simply clear the state of the
> irq, but the docs actually specify that it should only make the
> interrupt inactive if the only reason it became pending was a write to
> GICD_ISPENDRn (implying that the hardware level-triggered line was not
> high).

Yeah, you need to have some state representing the current wire values
of your input lines (in QEMU this is the gic_irq_state 'level' field).
This isn't state in the hardware, of course, but we have to store it
as state in KVM because our API means we only notify on wire logic
level changes, and there's no way to arbitrarily ask "what's the level
of this input line right now" (which is what the hardware trivially
does).

thanks
-- PMM