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

[Peter Maydell <peter.maydell@linaro.org>]

On 2 December 2015 at 23:18, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> On 11/20/2015 08:25 AM, Peter Maydell wrote:
>> 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.
>
> I'd certainly find this helpful :) I'm just not sure how to accomplish
> this and still make every patch compile.

The idea would be to have a patch which is just moving (copying)
the old code into the new code structure, so you get the old behaviour
but in the new device. Then you have patches which change the old
behaviour to the desired new behaviour.

>>> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
>>> ---
>>>  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.
>
> I've kept the pre-calculation as the alternative comparison code is
> no simpler when tie breaking with exception number is done.

Precalculation means you end up either (a) migrating state which
isn't really device state, or (b) having to re-calculate the
precalculations on migration load. (a) isn't really a good idea
since it turns internal details of the implementation into ABI
we might eventually have to support for backward compatibility
of migrations, and (b) is extra code.

>>> +
>>> +    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.
>
> Done.
>
>>> +
>>> +        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".
>
> Not quite since it's possible that I->raw_prio == act_raw_prio.  As
> I read the ARM this situation should be avoided by using the exception
> number to break the tie.  This way no two priorities are ever equal.
>  This is the reason that act_sub is "irq + (prio&submask)*NVIC_MAX_VECTORS".

You should use the exception number to break the tie, yes, but
I don't see why that requires you to encode the irq number into
anything, because you can arrange that you get that behaviour
by coding the loop suitably. The code I had isn't quite right,
though...

>> 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
>>         }
>>     }

...try:

   highestpri = current_running_priority;
   /* find highest priority active vector; this will correctly
    * handle group and subgroup priority fields, and in the case
    * of a tie we'll get the vector with the lowest exception
    * number, because the comparison is '<' and we start from 0.
    */
   for (each vector starting from 0) {
       if (vector->active && vector->raw_prio < highestpri) {
           highestpri = vector->raw_prio;
       }
   }
   if ((highestpri & prigroup_mask) < current_running_priority) {
       /* this interrupt should actually preempt */
   }

(I think we also have similar loop-through-all-interrupts logic in
the "return current running priority" function, though -- we should
only do the calculation in one place if we can.)

>>> +        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.
>
> Good point.  On re-reading this I think I understand better how
> priority changes take effect (immediately).  I've move this test
> last and made it a hw_error() since it should only be hit if
> some bug results in inconsistency between the NVIC and ARMCPU
> priorities fields.

It can happen for synchronous exceptions, like the example
case of UsageFault in a UsageFault handler (eg executing an
undefined insn in the handler).

>>> +            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'm confused.  From my testing it seems like the PC has already
> been adjusted by this point and no further changes are needed.
> Am I missing something?

You're right -- for arm_v7m_cpu_do_interrupt() we expect the
PC to have already been adjusted as required (this is unlike
the A/R profile do_interrupt function which does the adjustment
of the LR according to type of exception, which is why I thought
there was a problem.)

In that case we should just have a comment here that says
that since env->regs[15] has already been adjusted appropriately
for the original fault type we can just change the irq number
here and still get the correct ReturnAddress behaviour for
the original fault.

thanks
-- PMM