Re: [Qemu-arm] [PATCH 2/7] target/arm: Split "get pending exception info" from "acknowledge it"

[Philippe Mathieu-Daudé]

On 01/30/2018 12:02 PM, Peter Maydell wrote:
> Currently armv7m_nvic_acknowledge_irq() does three things:
>  * make the current highest priority pending interrupt active
>  * return a bool indicating whether that interrupt is targeting
>    Secure or NonSecure state
>  * implicitly tell the caller which is the highest priority
>    pending interrupt by setting env->v7m.exception
> 
> We need to split these jobs, because v7m_exception_taken()
> needs to know whether the pending interrupt targets Secure so
> it can choose to stack callee-saves registers or not, but it
> must not make the interrupt active until after it has done
> that stacking, in case the stacking causes a derived exception.
> Similarly, it needs to know the number of the pending interrupt
> so it can read the correct vector table entry before the
> interrupt is made active, because vector table reads might
> also cause a derived exception.
> 
> Create a new armv7m_nvic_get_pending_irq_info() function which simply
> returns information about the highest priority pending interrupt, and
> use it to rearrange the v7m_exception_taken() code so we don't
> acknowledge the exception until we've done all the things which could
> possibly cause a derived exception.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  target/arm/cpu.h      | 19 ++++++++++++++++---
>  hw/intc/armv7m_nvic.c | 30 +++++++++++++++++++++++-------
>  target/arm/helper.c   | 16 ++++++++++++----
>  hw/intc/trace-events  |  3 ++-
>  4 files changed, 53 insertions(+), 15 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 9ed03e6..f21f68e 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1519,16 +1519,29 @@ void armv7m_nvic_set_pending(void *opaque, int irq, 
> bool secure);
>   */
>  void armv7m_nvic_set_pending_derived(void *opaque, int irq, bool secure);
>  /**
> + * armv7m_nvic_get_pending_irq_info: return highest priority pending
> + *    exception, and whether it targets Secure state
> + * @opaque: the NVIC
> + * @pirq: set to pending exception number
> + * @ptargets_secure: set to whether pending exception targets Secure
> + *
> + * This function writes the number of the highest priority pending
> + * exception (the one which would be made active by
> + * armv7m_nvic_acknowledge_irq()) to @pirq, and sets @ptargets_secure
> + * to true if the current highest priority pending exception should
> + * be taken to Secure state, false for NS.
> + */
> +void armv7m_nvic_get_pending_irq_info(void *opaque, int *pirq,
> +                                      bool *ptargets_secure);
> +/**
>   * armv7m_nvic_acknowledge_irq: make highest priority pending exception 
> active
>   * @opaque: the NVIC
>   *
>   * Move the current highest priority pending exception from the pending
>   * state to the active state, and update v7m.exception to indicate that
>   * it is the exception currently being handled.
> - *
> - * Returns: true if exception should be taken to Secure state, false for NS
>   */
> -bool armv7m_nvic_acknowledge_irq(void *opaque);
> +void armv7m_nvic_acknowledge_irq(void *opaque);
>  /**
>   * armv7m_nvic_complete_irq: complete specified interrupt or exception
>   * @opaque: the NVIC
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index b4a6e7c..360889d 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -650,24 +650,20 @@ void armv7m_nvic_set_pending_derived(void *opaque, int 
> irq, bool secure)
>  }
>  
>  /* Make pending IRQ active.  */
> -bool armv7m_nvic_acknowledge_irq(void *opaque)
> +void armv7m_nvic_acknowledge_irq(void *opaque)
>  {
>      NVICState *s = (NVICState *)opaque;
>      CPUARMState *env = &s->cpu->env;
>      const int pending = s->vectpending;
>      const int running = nvic_exec_prio(s);
>      VecInfo *vec;
> -    bool targets_secure;
>  
>      assert(pending > ARMV7M_EXCP_RESET && pending < s->num_irq);
>  
>      if (s->vectpending_is_s_banked) {
>          vec = &s->sec_vectors[pending];
> -        targets_secure = true;
>      } else {
>          vec = &s->vectors[pending];
> -        targets_secure = !exc_is_banked(s->vectpending) &&
> -            exc_targets_secure(s, s->vectpending);
>      }
>  
>      assert(vec->enabled);
> @@ -675,7 +671,7 @@ bool armv7m_nvic_acknowledge_irq(void *opaque)
>  
>      assert(s->vectpending_prio < running);
>  
> -    trace_nvic_acknowledge_irq(pending, s->vectpending_prio, targets_secure);
> +    trace_nvic_acknowledge_irq(pending, s->vectpending_prio);
>  
>      vec->active = 1;
>      vec->pending = 0;
> @@ -683,8 +679,28 @@ bool armv7m_nvic_acknowledge_irq(void *opaque)
>      write_v7m_exception(env, s->vectpending);
>  
>      nvic_irq_update(s);
> +}
> +
> +void armv7m_nvic_get_pending_irq_info(void *opaque,
> +                                      int *pirq, bool *ptargets_secure)
> +{
> +    NVICState *s = (NVICState *)opaque;
> +    const int pending = s->vectpending;
> +    bool targets_secure;
> +
> +    assert(pending > ARMV7M_EXCP_RESET && pending < s->num_irq);
> +
> +    if (s->vectpending_is_s_banked) {
> +        targets_secure = true;
> +    } else {
> +        targets_secure = !exc_is_banked(pending) &&
> +            exc_targets_secure(s, pending);
> +    }
> +
> +    trace_nvic_get_pending_irq_info(pending, targets_secure);
>  
> -    return targets_secure;

maybe:

assert(ptargets_secure && pirq);

(function is externally callable)

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> +    *ptargets_secure = targets_secure;
> +    *pirq = pending;
>  }
>  
>  int armv7m_nvic_complete_irq(void *opaque, int irq, bool secure)
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index bfce096..6062f38 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6395,12 +6395,12 @@ static uint32_t *get_v7m_sp_ptr(CPUARMState *env, 
> bool secure, bool threadmode,
>      }
>  }
>  
> -static uint32_t arm_v7m_load_vector(ARMCPU *cpu, bool targets_secure)
> +static uint32_t arm_v7m_load_vector(ARMCPU *cpu, int exc, bool 
> targets_secure)
>  {
>      CPUState *cs = CPU(cpu);
>      CPUARMState *env = &cpu->env;
>      MemTxResult result;
> -    hwaddr vec = env->v7m.vecbase[targets_secure] + env->v7m.exception * 4;
> +    hwaddr vec = env->v7m.vecbase[targets_secure] + exc * 4;
>      uint32_t addr;
>  
>      addr = address_space_ldl(cs->as, vec,
> @@ -6462,8 +6462,9 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t 
> lr, bool dotailchain)
>      CPUARMState *env = &cpu->env;
>      uint32_t addr;
>      bool targets_secure;
> +    int exc;
>  
> -    targets_secure = armv7m_nvic_acknowledge_irq(env->nvic);
> +    armv7m_nvic_get_pending_irq_info(env->nvic, &exc, &targets_secure);
>  
>      if (arm_feature(env, ARM_FEATURE_V8)) {
>          if (arm_feature(env, ARM_FEATURE_M_SECURITY) &&
> @@ -6531,6 +6532,14 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t 
> lr, bool dotailchain)
>          }
>      }
>  
> +    addr = arm_v7m_load_vector(cpu, exc, targets_secure);
> +
> +    /* Now we've done everything that might cause a derived exception
> +     * we can go ahead and activate whichever exception we're going to
> +     * take (which might now be the derived exception).
> +     */
> +    armv7m_nvic_acknowledge_irq(env->nvic);
> +
>      /* Switch to target security state -- must do this before writing SPSEL 
> */
>      switch_v7m_security_state(env, targets_secure);
>      write_v7m_control_spsel(env, 0);
> @@ -6538,7 +6547,6 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t 
> lr, bool dotailchain)
>      /* Clear IT bits */
>      env->condexec_bits = 0;
>      env->regs[14] = lr;
> -    addr = arm_v7m_load_vector(cpu, targets_secure);
>      env->regs[15] = addr & 0xfffffffe;
>      env->thumb = addr & 1;
>  }
> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> index 09e87d1..4092d28 100644
> --- a/hw/intc/trace-events
> +++ b/hw/intc/trace-events
> @@ -180,7 +180,8 @@ nvic_escalate_disabled(int irq) "NVIC escalating irq %d 
> to HardFault: disabled"
>  nvic_set_pending(int irq, bool secure, bool derived, int en, int prio) "NVIC 
> set pending irq %d secure-bank %d derived %d (enabled: %d priority %d)"
>  nvic_clear_pending(int irq, bool secure, int en, int prio) "NVIC clear 
> pending irq %d secure-bank %d (enabled: %d priority %d)"
>  nvic_set_pending_level(int irq) "NVIC set pending: irq %d higher prio than 
> vectpending: setting irq line to 1"
> -nvic_acknowledge_irq(int irq, int prio, bool targets_secure) "NVIC 
> acknowledge IRQ: %d now active (prio %d targets_secure %d)"
> +nvic_acknowledge_irq(int irq, int prio) "NVIC acknowledge IRQ: %d now active 
> (prio %d)"
> +nvic_get_pending_irq_info(int irq, bool secure) "NVIC next IRQ %d: 
> targets_secure: %d"
>  nvic_complete_irq(int irq, bool secure) "NVIC complete IRQ %d (secure %d)"
>  nvic_set_irq_level(int irq, int level) "NVIC external irq %d level set to %d"
>  nvic_sysreg_read(uint64_t addr, uint32_t value, unsigned size) "NVIC sysreg 
> read addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
>

signature.asc
Description: OpenPGP digital signature