Re: [Qemu-arm] [PATCH 06/18] armv7m: new NVIC utility functions

[Peter Maydell]

On 9 November 2015 at 01:11, Michael Davidsaver <address@hidden> wrote:
> Internal functions for operations previously done
> by GIC internals.
>
> nvic_irq_update() recalculates highest pending/active
> exceptions.
>
> armv7m_nvic_set_pending() include exception escalation
> logic.
>
> armv7m_nvic_acknowledge_irq() and nvic_irq_update()
> update ARMCPU fields.

Hi; I have a lot of review comments on this patch set, but that's
really because v7M exception logic is pretty complicated and
our current code is a long way away from correct. You might
find it helpful to separate out "restructure the NVIC code
into its own device" into separate patches from "and now add
functionality like exception escalation", perhaps.

> Signed-off-by: Michael Davidsaver <address@hidden>
> ---
>  hw/intc/armv7m_nvic.c | 250 
> +++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 235 insertions(+), 15 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 487a09a..ebb4d4e 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -140,36 +140,256 @@ static void systick_reset(nvic_state *s)
>      timer_del(s->systick.timer);
>  }
>
> -/* The external routines use the hardware vector numbering, ie. the first
> -   IRQ is #16.  The internal GIC routines use #32 as the first IRQ.  */
> +/* caller must call nvic_irq_update() after this */
> +static
> +void set_prio(nvic_state *s, unsigned irq, uint8_t prio)
> +{
> +    unsigned submask = (1<<(s->prigroup+1))-1;
> +
> +    assert(irq > 3); /* only use for configurable prios */
> +    assert(irq < NVIC_MAX_VECTORS);
> +
> +    s->vectors[irq].raw_prio = prio;
> +    s->vectors[irq].prio_group = (prio>>(s->prigroup+1));
> +    s->vectors[irq].prio_sub = irq + (prio&submask)*NVIC_MAX_VECTORS;

Precalculating the group priority seems a bit odd, since it
will change later anyway if the guest writes to PRIGROUP.

> +
> +    DPRINTF(0, "Set %u priority grp %d sub %u\n", irq,
> +            s->vectors[irq].prio_group, s->vectors[irq].prio_sub);
> +}
> +
> +/* recompute highest pending */
> +static
> +void nvic_irq_update(nvic_state *s, int update_active)
> +{
> +    unsigned i;
> +    int lvl;
> +    CPUARMState *env = &s->cpu->env;
> +    int16_t act_group = 0x100, pend_group = 0x100;
> +    uint16_t act_sub = 0, pend_sub = 0;
> +    uint16_t act_irq = 0, pend_irq = 0;
> +
> +    /* find highest priority */
> +    for (i = 1; i < s->num_irq; i++) {
> +        vec_info *I = &s->vectors[i];

Minor style issue: we prefer lower case for variable names,
and CamelCase for structure type names (see CODING_STYLE).
So this would be VecInfo *vec; or something similar.

> +
> +        DPRINTF(2, " VECT %d %d:%u\n", i, I->prio_group, I->prio_sub);
> +
> +        if (I->active && ((I->prio_group < act_group)
> +                || (I->prio_group == act_group && I->prio_sub < act_sub)))

Because the group priority and sub priority are just two contiguous
parts of the raw priority, you get the same effect here by just
doing a comparison on the raw value: "I->raw_prio <  act_raw_prio".

Note however that the running priority is actually only the
group priority part (see the pseudocode ExecutionPriority function)
and so you want something more like:

    highestpri = 0x100;
    for (each vector) {
        if (vector->active && vector->raw_prio < highestpri) {
            highestpri = vector->raw_prio & prigroup_mask;
            act_sub = vector->raw_prio & ~prigroup_mask; // if you need it
        }
    }

(this is why it's not worth precalculating the group and
subpriorities -- it's cheap enough to do at this point.)

> +        {
> +            act_group = I->prio_group;
> +            act_sub = I->prio_sub;
> +            act_irq = i;
> +        }
> +
> +        if (I->enabled && I->pending && ((I->prio_group < pend_group)
> +                || (I->prio_group == pend_group && I->prio_sub < pend_sub)))
> +        {
> +            pend_group = I->prio_group;
> +            pend_sub = I->prio_sub;
> +            pend_irq = i;
> +        }
> +    }
> +
> +    env->v7m.pending = pend_irq;
> +    env->v7m.pending_prio = pend_group;
> +
> +    if (update_active) {
> +        env->v7m.exception = act_irq;
> +        env->v7m.exception_prio = act_group;
> +    }
> +
> +    /* Raise NVIC output even if pend_group is masked.
> +     * This is necessary as we get no notification
> +     * when PRIMASK et al. are changed.
> +     * As long as our output is high cpu_exec() will call
> +     * into arm_v7m_cpu_exec_interrupt() frequently, which
> +     * then tests to see if the pending exception
> +     * is permitted.
> +     */

This is bad -- we should instead arrange to update our idea
of the current running priority when PRIMASK &c are updated.
(Also it's not clear why we need to have an outbound qemu_irq
just to tell the core there's a pending exception. I think
we should just be able to call cpu_interrupt().)

> +    lvl = pend_irq > 0;
> +    DPRINTF(1, "highest pending %d %d:%u\n", pend_irq, pend_group, pend_sub);
> +    DPRINTF(1, "highest active  %d %d:%u\n", act_irq, act_group, act_sub);
> +
> +    DPRINTF(0, "IRQ %c highest act %d pend %d\n",
> +            lvl ? 'X' : '_', act_irq, pend_irq);
> +
> +    qemu_set_irq(s->excpout, lvl);
> +}
> +
> +static
> +void armv7m_nvic_clear_pending(void *opaque, int irq)
> +{
> +    nvic_state *s = (nvic_state *)opaque;
> +    vec_info *I;
> +
> +    assert(irq >= 0);
> +    assert(irq < NVIC_MAX_VECTORS);
> +
> +    I = &s->vectors[irq];
> +    if (I->pending) {
> +        I->pending = 0;
> +        nvic_irq_update(s, 0);
> +    }
> +}
> +
>  void armv7m_nvic_set_pending(void *opaque, int irq)
>  {
>      nvic_state *s = (nvic_state *)opaque;
> -    if (irq >= 16)
> -        irq += 16;
> -    gic_set_pending_private(&s->gic, 0, irq);
> +    CPUARMState *env = &s->cpu->env;
> +    vec_info *I;
> +    int active = s->cpu->env.v7m.exception;
> +
> +    assert(irq > 0);
> +    assert(irq < NVIC_MAX_VECTORS);
> +
> +    I = &s->vectors[irq];
> +
> +    if (irq < ARMV7M_EXCP_SYSTICK && irq != ARMV7M_EXCP_DEBUG) {

This will include NMI, which is wrong (NMI doesn't escalate
to HardFault because it's already higher priority than HardFault).
It also includes PendSV, which is classified as an interrupt
(like SysTick) and so also doesn't get escalated.

Also worth noting in a comment somewhere that for QEMU
BusFault is always synchronous and so we have no asynchronous
faults. (Async faults don't escalate.)

> +        int runnable = armv7m_excp_unmasked(s->cpu);

"running_prio" might be a better name for this variable, since
it's the current execution (running) priority.

> +        /* test for exception escalation for vectors other than:
> +         * Debug (12), SysTick (15), and all external IRQs (>=16)
> +         */

This isn't really getting the DebugMonitor fault case right:
DebugMonitor exceptions caused by executing a BKPT insn must be
escalated if the running priority is too low; other DebugMonitor
exceptions are just ignored (*not* set Pending).

> +        unsigned escalate = 0;
> +        if (I->active) {
> +            /* trying to pend an active fault (possibly nested).
> +             * eg. UsageFault in UsageFault handler
> +             */
> +            escalate = 1;
> +            DPRINTF(0, " Escalate, active\n");

You don't need to explicitly check for this case, because it
will be caught by the "is the running priority too high" check
(the current running priority must be at least as high as the
priority of an active fault handler). If you look at the ARM ARM
section on priority escalation you'll see that "undef in a
UsageFault handler" is listed in the "examples of things that
cause priority escalation" bullet list, not the "definition of
what causes escalation" list.

> +        } else if (!I->enabled) {
> +            /* trying to pend a disabled fault
> +             * eg. UsageFault while USGFAULTENA in SHCSR is clear.
> +             */
> +            escalate = 1;
> +            DPRINTF(0, " Escalate, not enabled\n");
> +        } else if (I->prio_group > runnable) {

This should be <= : (a) equal priority to current execution
priority gets escalated and (b) prio values are "low numbers
are higher priorities".

> +            /* trying to pend a fault which is not immediately
> +             * runnable due to masking by PRIMASK, FAULTMASK, BASEPRI,
> +             * or the priority of the active exception
> +             */
> +            DPRINTF(0, " Escalate, mask %d >= %d\n",
> +                    I->prio_group, runnable);

The condition printed in the debug log doesn't match the
condition tested by the code.

> +            escalate = 1;
> +        }
> +
> +        if (escalate) {
> +#ifdef DEBUG_NVIC
> +            int oldirq = irq;
> +#endif
> +            if (runnable < -1) {
> +                /* TODO: actual unrecoverable exception actions */
> +                cpu_abort(&s->cpu->parent_obj,
> +                          "%d in %d escalates to unrecoverable exception\n",
> +                          irq, active);
> +            } else {
> +                irq = ARMV7M_EXCP_HARD;
> +            }
> +            I = &s->vectors[irq];
> +
> +            DPRINTF(0, "Escalate %d to %d\n", oldirq, irq);

Faults escalated to HardFault retain the ReturnAddress
behaviour of the original fault (ie what return-address
is saved to the stack as part of the exception stacking and
so whether we resume execution on the faulting insn or the
next one). So it's not sufficient to just say "treat it
exactly as if it were a HardFault" like this.

> +        }
> +    }
> +
> +    I->pending = 1;
> +    if (I->enabled && (I->prio_group < env->v7m.pending_prio)) {
> +        env->v7m.pending_prio = I->prio_group;
> +        env->v7m.pending = irq;
> +        qemu_set_irq(s->excpout, irq > 0);
> +    }
> +    DPRINTF(0, "Pending %d at %d%s runnable %d\n",
> +            irq, I->prio_group,
> +            env->v7m.pending == irq ? " (highest)" : "",
> +            armv7m_excp_unmasked(s->cpu));
>  }

thanks
-- PMM