Re: [Qemu-arm] [PATCH 11/18] armv7m: fix I and F flag handling

[Michael Davidsaver]

On 11/20/2015 08:47 AM, Peter Maydell wrote:
> On 9 November 2015 at 01:11, Michael Davidsaver <address@hidden> wrote:
>> Despite having the same notation, these bits
>> have completely different meaning than -AR.
>>
>> Add armv7m_excp_unmasked()
>> to calculate the currently runable exception priority
>> taking into account masks and active handlers.
>> Use this in conjunction with the pending exception
>> priority to determine if the pending exception
>> can interrupt execution.
> 
> This function is used by code added in earlier patches in
> this series, so this patch needs to be moved earlier in the
> series, or those patches won't compile.

Should be fixed.

>> Signed-off-by: Michael Davidsaver <address@hidden>
>> ---
>>  target-arm/cpu.c | 26 +++++++-------------------
>>  target-arm/cpu.h | 27 ++++++++++++++++++++++++++-
>>  2 files changed, 33 insertions(+), 20 deletions(-)
>>
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index be026bc..5d03117 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -173,6 +173,8 @@ static void arm_cpu_reset(CPUState *s)
>>          uint32_t initial_pc; /* Loaded from 0x4 */
>>          uint8_t *rom;
>>
>> +        env->v7m.exception_prio = env->v7m.pending_prio = 0x100;
>> +
>>          env->daif &= ~PSTATE_I;
>>          rom = rom_ptr(0);
>>          if (rom) {
>> @@ -301,29 +303,15 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, 
>> int interrupt_request)
>>  {
>>      CPUClass *cc = CPU_GET_CLASS(cs);
>>      ARMCPU *cpu = ARM_CPU(cs);
>> -    CPUARMState *env = &cpu->env;
>>      bool ret = false;
>>
>> -
>> -    if (interrupt_request & CPU_INTERRUPT_FIQ
>> -        && !(env->daif & PSTATE_F)) {
>> -        cs->exception_index = EXCP_FIQ;
>> -        cc->do_interrupt(cs);
>> -        ret = true;
>> -    }
>> -    /* ARMv7-M interrupt return works by loading a magic value
>> -     * into the PC.  On real hardware the load causes the
>> -     * return to occur.  The qemu implementation performs the
>> -     * jump normally, then does the exception return when the
>> -     * CPU tries to execute code at the magic address.
>> -     * This will cause the magic PC value to be pushed to
>> -     * the stack if an interrupt occurred at the wrong time.
>> -     * We avoid this by disabling interrupts when
>> -     * pc contains a magic address.
> 
> This (removing this comment and the checks for the magic address)
> seem to be part of a separate change [probably the one in
> "armv7m: Undo armv7m.hack"] and shouldn't be in this patch.

Relocated.

>> +    /* ARMv7-M interrupt masking works differently than -A or -R.
>> +     * There is no FIQ/IRQ distinction.
>> +     * Instead of masking interrupt sources, the I and F bits
>> +     * (along with basepri) mask certain exception priority levels.
>>       */
>>      if (interrupt_request & CPU_INTERRUPT_HARD
>> -        && !(env->daif & PSTATE_I)
>> -        && (env->regs[15] < 0xfffffff0)) {
>> +            && (armv7m_excp_unmasked(cpu) >= cpu->env.v7m.pending_prio)) {
>>          cs->exception_index = EXCP_IRQ;
>>          cc->do_interrupt(cs);
>>          ret = true;
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index c193fbb..29d89ce 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -1033,9 +1033,34 @@ void arm_cpu_list(FILE *f, fprintf_function 
>> cpu_fprintf);
>>  uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
>>                                   uint32_t cur_el, bool secure);
>>
>> +/* @returns highest (numerically lowest) unmasked exception priority
>> + */
>> +static inline
>> +int armv7m_excp_unmasked(ARMCPU *cpu)
> 
> What this is really calculating is the current execution
> priority (running priority) of the CPU, so I think a better
> name would be armv7m_current_exec_priority() or
> armv7m_current_priority() or armv7m_running_priority() or similar.

Now armv7m_excp_running_prio()

>> +{
>> +    CPUARMState *env = &cpu->env;
>> +    int runnable;
>> +
>> +    /* find highest (numerically lowest) priority which could
>> +     * run based on masks
>> +     */
>> +    if (env->daif&PSTATE_F) { /* FAULTMASK */
> 
> Style issue -- operands should have spaces around them.
> 
>> +        runnable = -2;
> 
> These all seem to be off by one: FAULTMASK sets the
> running priority to -1, not -2, PRIMASK sets it to 0,
> not -1, and so on.

The off by one was due to my confusing runnable vs. running distinction, now 
gone.

>> +    } else if (env->daif&PSTATE_I) { /* PRIMASK */
>> +        runnable = -1;
>> +    } else if (env->v7m.basepri > 0) {
>> +        /* BASEPRI==1 -> runnable==-1 (same as PRIMASK==1) */
> 
> (applies to operands in comments too)
> 
>> +        runnable = env->v7m.basepri-2;
> 
> Where is this - 2 from? Also, BASEPRI values honour the
> PRIGROUP setting. (Compare the ExecutionPriority pseudocode).

The off by two for BASEPRI was my mis-reading the definition.

>> +    } else {
>> +        runnable = 0x100; /* lower than any possible priority */
>> +    }
>> +    /* consider priority of active handler */
>> +    return MIN(runnable, env->v7m.exception_prio-1);
> 
> I don't think this -1 should be here.

It is gone.

>> +}
>> +
>>  /* Interface between CPU and Interrupt controller.  */
>>  void armv7m_nvic_set_pending(void *opaque, int irq);
>> -int armv7m_nvic_acknowledge_irq(void *opaque);
>> +void armv7m_nvic_acknowledge_irq(void *opaque);
>>  void armv7m_nvic_complete_irq(void *opaque, int irq);
>>
>>  /* Interface for defining coprocessor registers.
> 
> thanks
> -- PMM
>