Re: Help: NVIC, level-triggered interrupts and interrupt pending

[Igor Kotrasiński]

On 30.05.2022 12:49, Peter Maydell wrote:
> On Mon, 30 May 2022 at 10:33, Igor Kotrasiński <i.kotrasinsk@samsung.com> 
> wrote:
>>
>> Hi,
>>
>> I've been hacking on QEMU recently, adding support for a custom Cortex-M
>> board. One of the devices that I'm emulating is using level-triggered
>> interrupts, for which I assume qemu_irq_raise/lower is the right tool.
>> However, I'm having trouble receiving interrupts for my device which I
>> *think* are caused by interaction with setting/clearing
>> interrupt-pending status.
>>
>> The way I'm handling my interrupt currently is as follows:
>>
>> 1. The device raises an interrupt with qemu_irq_raise.
>> 2. In interrupt handler, I set the clear-enable nvic register and queue
>> a handler.
>> 3. In handler, I read data from the device and set a register that
>> lowers its interrupt with qemu_irq_lower.
>> 4. Then I set the clear-pending nvic register.
>> 5. Finally I set the set-enable nvic register.
> 
> You don't say in this sequence where "return from the interrupt
> exception handler" happens, which is an important part of the
> sequence. (That is, I'm not sure whether your design has only
> step 2 in the CPU exception handler with the "queued" handler
> being run after exception-return, or whether all of 2-5 are
> in the CPU exception handler for the interrupt.)
> 

Sorry, I lied a bit here. I'm running someone else's code and I misread 
it a bit. The exact sequence here is:

1. The device raises the interrupt.
2. Interrupt handler sets the set-pending register, then the 
clear-enable register, then returns.
3. A handler running outside exception context reads data from the 
device and lowers the irq.
3.5. The device raises the irq again.
4. The handler sets the clear-pending register, then the set-enable 
register.

I hope it'll be useful for reproducing the issue.

> It is also weird that you are clearing the pending state by
> manually writing to the NVIC register -- generally the expected
> design is that the NVIC itself will mark the interrupt as no
> longer pending when it takes the interrupt (ie at step 2).
> Also, it's implementation-defined whether the NVIC even lets
> you control the pending state via the software registers, so
> a design like this isn't guaranteed to work on all hardware.
> 
>> The problem happens when the device re-raises the interrupt between
>> steps 3 and 4. I would expect the interrupt to stay pending after
>> clearing its pending status, and random ARM docs I found on the net seem
>> to confirm that:
>>
>>       A pending interrupt remains pending until one of the following:
>>       ...
>>       * Software writes to the corresponding interrupt clear-pending
>> register bit. For a level-sensitive interrupt, if the interrupt signal
>> is still asserted, the state of the interrupt does not change.
>>
>>
>> https://developer.arm.com/documentation/dui0497/a/cortex-m0-peripherals/nested-vectored-interrupt-controller/level-sensitive-and-pulse-interrupts
> 
> This is a explanatory document; its explanations might be helpful,
> or might not. If you're in doubt about the precise detail of how
> the CPU and the NVIC work you should be looking at the Armv8M
> Architecture Reference Manual (or Armv7M, possibly, depending on
> what CPU you're using).
> 
>> On QEMU however, pending status is cleared and I lose the interrupt. I
>> tried to hack in the behaviour described in the docs quoted above, like so:
>>
>> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
>> index 773e311754..4914b1217f 100644
>> --- a/hw/intc/armv7m_nvic.c
>> +++ b/hw/intc/armv7m_nvic.c
>> @@ -2393,6 +2393,12 @@ static MemTxResult nvic_sysreg_write(void
>> *opaque, hwaddr addr,
>>            for (i = 0, end = size * 8; i < end && startvec + i <
>> s->num_irq; i++) {
>>                if (value & (1 << i) &&
>>                    (attrs.secure || s->itns[startvec + i])) {
>> +                /* If the interrupt signal is still asserted, the state
>> of the
>> +                 * interrupt does not change.
>> +                 */
>> +                if (!setval && s->vectors[i + NVIC_FIRST_IRQ].level) {
>> +                    continue;
>> +                }
>>                    s->vectors[startvec + i].pending = setval;
>>                }
>>            }
>>
>> This fixed the issue I was having. However, I'm not sure if it's the
>> right solution. One, this affects all devices connected to nvic, so this
>> might randomly break them. Two, there's a confusing comment in
>> armv7m_nvic.c, in set_irq_level, that seems to imply that all external
>> interrupts are edge-triggered:
>>
>>       /* The pending status of an external interrupt is
>>        * latched on rising edge and exception handler return.
>>        *
>>        * Pulsing the IRQ will always run the handler
>>        * once, and the handler will re-run until the
>>        * level is low when the handler completes.
>>        */
> 
> The NVIC doesn't make a distinction between "edge triggered" and
> "level triggered" interrupt types. Instead it provides behaviour
> which generally speaking is compatible with both kinds of device:
> an interrupt becomes pending if either:
>   * the input IRQ line is high and the interrupt is not Active
>   * the input IRQ line transitions from low to high and the
>     interrupt is Active
> 
> (this is rule R_CVJS in the v8M Arm ARM, and the implications
> for both level and pulse sensitive interrupts are explained in
> I_WTFS). The code associated with that comment is doing the second
> of those -- if the level transitions low-to-high we call
> armv7m_nvic_set_pending(), which will mark the interrupt pending.
> Checking the pending status on exception handler return is part
> of the first bullet point -- it's a transition from interrupt
> Active to not-Active, so if the IRQ line is high at that point
> then we should make the interrupt Pending again.
> 
>> I'd be very thankful for any help with this. I *think* I found a bug,
>> but I'm not 100% sure.
> 
> I suspect you're right that there's a bug lurking here -- I'll think
> a bit more about it and dig into the details of what the architecture
> specifies -- but I suspect also that your software design is doing
> something a bit odd that is why it's running into this corner case
> in the first place.
> 
> thanks
> -- PMM
>