On 25/06/13 05:16, Christoffer Dall wrote:
> Hi Peter,
>
> Can you help me understand arm_vgic.c, specifically, see the quoted code
> below and my question:
>
>> /* 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.
>
>> 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?
>
> 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), so reading this registers would give a
> false impression of an edge-triggered interrupt, which is disabled, but
> not pending on any CPU interfaces.
>
> 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.
>
>> 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).
That basically means there is an OR operation between some internal
buffer representing the state of the PENDRn registers and the wire state.
> Not sure it's worth fixing - I don't think Linux is writing to these
> registers, but I haven't verified it. Marc?
No, Linux doesn't make any use of these registers, and it is very likely
that there are bugs lurking there. Clearly, the GICD_I[CS]PENDRn logic
needs some improvement, but it also requires a guest that makes use of
this feature...
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
