qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH 11/18] armv7m: fix I and F flag handling
Date: Fri, 20 Nov 2015 13:47:43 +0000

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.

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

> +    /* 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.

> +{
> +    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.

> +    } 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).

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

> +}
> +
>  /* 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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]