Re: [Qemu-devel] [PATCH v2 05/26] armv7m: add armv7m_excp_running_prio()

[Peter Maydell]

On 27 December 2015 at 20:56, Michael Davidsaver <address@hidden> wrote:
> On 12/17/2015 09:36 AM, Peter Maydell wrote:
>> On 3 December 2015 at 00:18, Michael Davidsaver <address@hidden> wrote:
>>> Implements v7m exception priority algorithm
>>> using FAULTMASK, PRIMASK, BASEPRI, and the highest
>>> priority active exception.
>>>
>>> The number returned is the current execution priority
>>> which may be in the range [-2,0x7f] when an exception is active
>>> or 0x100 when no exception is active.
>>> ---
>>>  hw/intc/armv7m_nvic.c | 25 +++++++++++++++++++++++++
>>>  target-arm/cpu.h      |  1 +
>>>  2 files changed, 26 insertions(+)
>>>
>>> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
>>> index 6fc167e..0145ca7 100644
>>> --- a/hw/intc/armv7m_nvic.c
>>> +++ b/hw/intc/armv7m_nvic.c
>>> @@ -18,6 +18,8 @@
>>>
>>>  typedef struct {
>>>      GICState gic;
>>> +    uint8_t prigroup;
>>> +
>>>      struct {
>>>          uint32_t control;
>>>          uint32_t reload;
>>> @@ -116,6 +118,29 @@ static void systick_reset(nvic_state *s)
>>>      timer_del(s->systick.timer);
>>>  }
>>>
>>> +/* @returns the active (running) exception priority.
>>> + *    only a higher (numerically lower) priority can preempt.
>>> + */
>>> +int armv7m_excp_running_prio(ARMCPU *cpu)
>>> +{
>>> +    CPUARMState *env = &cpu->env;
>>> +    nvic_state *s = env->nvic;
>>> +    int running;
>>> +
>>> +    if (env->daif & PSTATE_F) { /* FAULTMASK */
>>> +        running = -1;
>>> +    } else if (env->daif & PSTATE_I) { /* PRIMASK */
>>> +        running = 0;
>>> +    } else if (env->v7m.basepri > 0) {
>>> +        /* BASEPRI==1 -> masks [1,255] (not same as PRIMASK==1) */
>>> +        running = env->v7m.basepri >> (s->prigroup+1);
>> This isn't right -- the effect of PRIGROUP is that we mask
>> out the lower (subgroup) bits, but the upper group bits stay
>> where they are rather than shifting down.
>>
>> So you want env->v7m.basepri & ~((1 << (s->prigroup + 1)) - 1);
>>
>> (the same mask you need to get the group priority for
>> an interrupt).
>
> I don't know about "right", but it is consistent with how the
> .prio_group field is now handled in the nvic.  So I think the final
> behavior is as specified.

The v7M ARM ARM is the authoritative source for "what is the
right behaviour", and the pseudocode is I think fairly clear
about how BASEPRI affects the running priority. If the current
NVIC code is handling BASEPRI wrongly that's a bug in the NVIC
code.

> There is no functional reason that I do this.  I just think it makes the
> DPRINTF messages easier to interpret.  If you feel strongly I can change
> this.

You need to get the right value here because otherwise we won't
be implementing the spec correctly.

thanks
-- PMM