Re: [PATCH] hw/intc/armv7m_nvic: ICPRn must not unpend an IRQ that is being held high

[Peter Maydell]

Ping for code review, please?

thanks
-- PMM

On Tue, 28 Jun 2022 at 16:47, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In the M-profile Arm ARM, rule R_CVJS defines when an interrupt should
> be set to the Pending state:
>  A) when the input line is high and the interrupt is not Active
>  B) when the input line transitions from low to high and the interrupt
>     is Active
> (Note that the first of these is an ongoing condition, and the
> second is a point-in-time event.)
>
> This can be rephrased as:
>  1 when the line goes from low to high, set Pending
>  2 when Active goes from 1 to 0, if line is high then set Pending
>  3 ignore attempts to clear Pending when the line is high
>    and Active is 0
>
> where 1 covers both B and one of the "transition into condition A"
> cases, 2 deals with the other "transition into condition A"
> possibility, and 3 is "don't drop Pending if we're already in
> condition A".  Transitions out of condition A don't affect Pending
> state.
>
> We handle case 1 in set_irq_level(). For an interrupt (as opposed
> to other kinds of exception) the only place where we clear Active
> is in armv7m_nvic_complete_irq(), where we handle case 2 by
> checking for whether we need to re-pend the exception. For case 3,
> the only places where we clear Pending state on an interrupt are in
> armv7m_nvic_acknowledge_irq() (where we are setting Active so it
> doesn't count) and for writes to NVIC_CPSRn.
>
> It is the "write to NVIC_ICPRn" case that we missed: we must ignore
> this if the input line is high and the interrupt is not Active.
> (This required behaviour is differently and perhaps more clearly
> stated in the v7M Arm ARM, which has pseudocode in section B3.4.1
> that implies it.)
>
> Reported-by: Igor Kotrasiński <i.kotrasinsk@samsung.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Simple change, commit message long because I included all
> the analysis that we haven't forgotten any other cases.
> This is essentially the change Igor suggested in the qemu-arm
> thread, but it took me a while to find time to audit the code
> to confirm that was the only change we needed here.
> ---
>  hw/intc/armv7m_nvic.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 13df002ce4d..1f7763964c3 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -2389,8 +2389,15 @@ static MemTxResult nvic_sysreg_write(void *opaque, 
> hwaddr addr,
>          startvec = 8 * (offset - 0x280) + NVIC_FIRST_IRQ; /* vector # */
>
>          for (i = 0, end = size * 8; i < end && startvec + i < s->num_irq; 
> i++) {
> +            /*
> +             * Note that if the input line is still held high and the 
> interrupt
> +             * is not active then rule R_CVJS requires that the Pending state
> +             * remains set; in that case we mustn't let it be cleared.
> +             */
>              if (value & (1 << i) &&
> -                (attrs.secure || s->itns[startvec + i])) {
> +                (attrs.secure || s->itns[startvec + i]) &&
> +                !(setval == 0 && s->vectors[startvec + i].level &&
> +                  !s->vectors[startvec + i].active)) {
>                  s->vectors[startvec + i].pending = setval;
>              }
>          }
> --
> 2.25.1