Ping for code review, please?
thanks
-- PMM
On Tue, 28 Jun 2022 at 16:47, Peter Maydell <peter.mayd...@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.kotrasi...@samsung.com>
> Signed-off-by: Peter Maydell <peter.mayd...@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
