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

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(-)

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.

Should read "NVIC_ICPRn"...

> 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.)

-- PMM

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

On 28/6/22 17:47, Peter Maydell 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;
>               }
>           }

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>