[Qemu-devel] [PATCH 0/2] target-arm: fix some exception-masking issues

[Qemu-devel] [PATCH 0/2] target-arm: fix some exception-masking issues

[Peter Maydell]

These patches fix a couple of issues I noticed with the existing
code in the ARM arm_excp_unmasked() function while I was reviewing
the TrustZone series:
 * we currently try to use the same function for M profile and
   A/R profile, which makes both code paths harder to read (and
   maybe one day we'll implement what M actually does, in which
   case the two code paths will be totally divergent...)
 * accidental || vs && confusion in the conditionals governing
   VIRQ and VFIQ

I think they shouldn't conflict with the TZ patches except
as minor textual conflicts.

Peter Maydell (2):
  target-arm: Separate out M profile cpu_exec_interrupt handling
  target-arm: Correct condition for taking VIRQ and VFIQ

 target-arm/cpu.c | 49 +++++++++++++++++++++++++++++++++++++++----------
 target-arm/cpu.h | 20 ++++----------------
 2 files changed, 43 insertions(+), 26 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/2] target-arm: Separate out M profile cpu_exec_interrupt handling

[Peter Maydell]

The M profile cpu_exec_interrupt handling is fairly simple
but does include an M profile specific oddity (disabling
interrupts for certain PC values). A/R profile handling
on the other hand is getting rapidly more complicated
with the support for EL2 and EL3. Split the M profile
code out into its own implementation of cpu_exec_interrupt
to keep these two things out of each others' way.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu.c | 49 +++++++++++++++++++++++++++++++++++++++----------
 target-arm/cpu.h | 16 ++--------------
 2 files changed, 41 insertions(+), 24 deletions(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index e0b82a6..5ce7350 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -203,15 +203,6 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
         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.  */
     if (interrupt_request & CPU_INTERRUPT_HARD
         && arm_excp_unmasked(cs, EXCP_IRQ)) {
         cs->exception_index = EXCP_IRQ;
@@ -234,6 +225,42 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
     return ret;
 }
 
+#if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
+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.
+     */
+    if (interrupt_request & CPU_INTERRUPT_HARD
+        && !(env->daif & PSTATE_I)
+        && (env->regs[15] < 0xfffffff0)) {
+        cs->exception_index = EXCP_IRQ;
+        cc->do_interrupt(cs);
+        ret = true;
+    }
+    return ret;
+}
+#endif
+
 #ifndef CONFIG_USER_ONLY
 static void arm_cpu_set_irq(void *opaque, int irq, int level)
 {
@@ -670,11 +697,13 @@ static void cortex_m3_initfn(Object *obj)
 
 static void arm_v7m_class_init(ObjectClass *oc, void *data)
 {
-#ifndef CONFIG_USER_ONLY
     CPUClass *cc = CPU_CLASS(oc);
 
+#ifndef CONFIG_USER_ONLY
     cc->do_interrupt = arm_v7m_cpu_do_interrupt;
 #endif
+
+    cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
 }
 
 static const ARMCPRegInfo cortexa8_cp_reginfo[] = {
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index cb6ec5c..97eaf79 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1251,18 +1251,6 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx)
     bool secure = false;
     /* If in EL1/0, Physical IRQ routing to EL2 only happens from NS state.  */
     bool irq_can_hyp = !secure && cur_el < 2 && target_el == 2;
-    /* 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.
-     */
-    bool irq_unmasked = !(env->daif & PSTATE_I)
-                        && (!IS_M(env) || env->regs[15] < 0xfffffff0);
 
     /* Don't take exceptions if they target a lower EL.  */
     if (cur_el > target_el) {
@@ -1279,7 +1267,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx)
         if (irq_can_hyp && (env->cp15.hcr_el2 & HCR_IMO)) {
             return true;
         }
-        return irq_unmasked;
+        return !(env->daif & PSTATE_I);
     case EXCP_VFIQ:
         if (!secure && !(env->cp15.hcr_el2 & HCR_FMO)) {
             /* VFIQs are only taken when hypervized and non-secure.  */
@@ -1291,7 +1279,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx)
             /* VIRQs are only taken when hypervized and non-secure.  */
             return false;
         }
-        return irq_unmasked;
+        return !(env->daif & PSTATE_I);
     default:
         g_assert_not_reached();
     }
-- 
1.9.1

[Qemu-devel] [PATCH 2/2] target-arm: Correct condition for taking VIRQ and VFIQ

[Peter Maydell]

The VIRQ and VFIQ exceptions are (as the comments say) only
taken if the CPU is in Non-secure state and the IMO/FMO bits
are set to enable virtualized interrupts. Correct the code
to actually implement this by using '||' rather than '&&'.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 97eaf79..6145403 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1269,13 +1269,13 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx)
         }
         return !(env->daif & PSTATE_I);
     case EXCP_VFIQ:
-        if (!secure && !(env->cp15.hcr_el2 & HCR_FMO)) {
+        if (!secure || !(env->cp15.hcr_el2 & HCR_FMO)) {
             /* VFIQs are only taken when hypervized and non-secure.  */
             return false;
         }
         return !(env->daif & PSTATE_F);
     case EXCP_VIRQ:
-        if (!secure && !(env->cp15.hcr_el2 & HCR_IMO)) {
+        if (!secure || !(env->cp15.hcr_el2 & HCR_IMO)) {
             /* VIRQs are only taken when hypervized and non-secure.  */
             return false;
         }
-- 
1.9.1

Re: [Qemu-devel] [PATCH 2/2] target-arm: Correct condition for taking VIRQ and VFIQ

[Edgar E. Iglesias]

On Mon, Oct 27, 2014 at 12:58:07PM +0000, Peter Maydell wrote:
> The VIRQ and VFIQ exceptions are (as the comments say) only
> taken if the CPU is in Non-secure state and the IMO/FMO bits
> are set to enable virtualized interrupts. Correct the code
> to actually implement this by using '||' rather than '&&'.

Hi Peter,

Shouldn't this be:

if (secure || ....

?

Cheers,
Edgar

> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/cpu.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 97eaf79..6145403 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1269,13 +1269,13 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx)
>          }
>          return !(env->daif & PSTATE_I);
>      case EXCP_VFIQ:
> -        if (!secure && !(env->cp15.hcr_el2 & HCR_FMO)) {
> +        if (!secure || !(env->cp15.hcr_el2 & HCR_FMO)) {
>              /* VFIQs are only taken when hypervized and non-secure.  */
>              return false;
>          }
>          return !(env->daif & PSTATE_F);
>      case EXCP_VIRQ:
> -        if (!secure && !(env->cp15.hcr_el2 & HCR_IMO)) {
> +        if (!secure || !(env->cp15.hcr_el2 & HCR_IMO)) {
>              /* VIRQs are only taken when hypervized and non-secure.  */
>              return false;
>          }
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 2/2] target-arm: Correct condition for taking VIRQ and VFIQ

[Peter Maydell]

On 27 October 2014 23:25, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> On Mon, Oct 27, 2014 at 12:58:07PM +0000, Peter Maydell wrote:
>> The VIRQ and VFIQ exceptions are (as the comments say) only
>> taken if the CPU is in Non-secure state and the IMO/FMO bits
>> are set to enable virtualized interrupts. Correct the code
>> to actually implement this by using '||' rather than '&&'.
>
> Hi Peter,
>
> Shouldn't this be:
>
> if (secure || ....
>
> ?

Heh, you're right. I didn't look closely enough...

-- PMM

Re: [Qemu-devel] [PATCH 1/2] target-arm: Separate out M profile cpu_exec_interrupt handling

[Edgar E. Iglesias]

On Mon, Oct 27, 2014 at 12:58:06PM +0000, Peter Maydell wrote:
> The M profile cpu_exec_interrupt handling is fairly simple
> but does include an M profile specific oddity (disabling
> interrupts for certain PC values). A/R profile handling
> on the other hand is getting rapidly more complicated
> with the support for EL2 and EL3. Split the M profile
> code out into its own implementation of cpu_exec_interrupt
> to keep these two things out of each others' way.

Looks good!
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/cpu.c | 49 +++++++++++++++++++++++++++++++++++++++----------
>  target-arm/cpu.h | 16 ++--------------
>  2 files changed, 41 insertions(+), 24 deletions(-)
> 
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index e0b82a6..5ce7350 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -203,15 +203,6 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>          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.  */
>      if (interrupt_request & CPU_INTERRUPT_HARD
>          && arm_excp_unmasked(cs, EXCP_IRQ)) {
>          cs->exception_index = EXCP_IRQ;
> @@ -234,6 +225,42 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>      return ret;
>  }
>  
> +#if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
> +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.
> +     */
> +    if (interrupt_request & CPU_INTERRUPT_HARD
> +        && !(env->daif & PSTATE_I)
> +        && (env->regs[15] < 0xfffffff0)) {
> +        cs->exception_index = EXCP_IRQ;
> +        cc->do_interrupt(cs);
> +        ret = true;
> +    }
> +    return ret;
> +}
> +#endif
> +
>  #ifndef CONFIG_USER_ONLY
>  static void arm_cpu_set_irq(void *opaque, int irq, int level)
>  {
> @@ -670,11 +697,13 @@ static void cortex_m3_initfn(Object *obj)
>  
>  static void arm_v7m_class_init(ObjectClass *oc, void *data)
>  {
> -#ifndef CONFIG_USER_ONLY
>      CPUClass *cc = CPU_CLASS(oc);
>  
> +#ifndef CONFIG_USER_ONLY
>      cc->do_interrupt = arm_v7m_cpu_do_interrupt;
>  #endif
> +
> +    cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
>  }
>  
>  static const ARMCPRegInfo cortexa8_cp_reginfo[] = {
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index cb6ec5c..97eaf79 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1251,18 +1251,6 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx)
>      bool secure = false;
>      /* If in EL1/0, Physical IRQ routing to EL2 only happens from NS state.  */
>      bool irq_can_hyp = !secure && cur_el < 2 && target_el == 2;
> -    /* 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.
> -     */
> -    bool irq_unmasked = !(env->daif & PSTATE_I)
> -                        && (!IS_M(env) || env->regs[15] < 0xfffffff0);
>  
>      /* Don't take exceptions if they target a lower EL.  */
>      if (cur_el > target_el) {
> @@ -1279,7 +1267,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx)
>          if (irq_can_hyp && (env->cp15.hcr_el2 & HCR_IMO)) {
>              return true;
>          }
> -        return irq_unmasked;
> +        return !(env->daif & PSTATE_I);
>      case EXCP_VFIQ:
>          if (!secure && !(env->cp15.hcr_el2 & HCR_FMO)) {
>              /* VFIQs are only taken when hypervized and non-secure.  */
> @@ -1291,7 +1279,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx)
>              /* VIRQs are only taken when hypervized and non-secure.  */
>              return false;
>          }
> -        return irq_unmasked;
> +        return !(env->daif & PSTATE_I);
>      default:
>          g_assert_not_reached();
>      }
> -- 
> 1.9.1
>