[Qemu-devel] [PATCH 0/7] ARMv8M: some bugfixes and prep. cleanup

[Qemu-devel] [PATCH 0/7] ARMv8M: some bugfixes and prep. cleanup

[Peter Maydell]

Seven simple patches off the top of my v8M work in progress:
three bugfixes which are also relevant for v7M:
 * need to clear exclusive monitor on exception entry/exit
 * wrong BFSR bits set on aborts
 * negative exception priorities were being wrongly handled
and four code cleanup patches, including one which is a change
suggested in review of my last series which I forgot to make.

thanks
-- PMM

Peter Maydell (7):
  target/arm: Use M_REG_NUM_BANKS rather than hardcoding 2
  target/arm: Clear exclusive monitor on v7M reset, exception entry/exit
  target/arm: Get PRECISERR and IBUSERR the right way round
  nvic: Don't apply group priority mask to negative priorities
  target/arm: Remove unnecessary '| 0xf0000000' from
    do_v7m_exception_exit()
  target/arm: Add and use defines for EXCRET constants
  target/arm: Rename 'type' to 'excret' in do_v7m_exception_exit()

 target/arm/cpu.h       | 35 +++++++++++++++++++----------------
 target/arm/internals.h | 20 ++++++++++++++++++++
 hw/intc/armv7m_nvic.c  | 11 +++++++++--
 target/arm/cpu.c       |  6 ++++++
 target/arm/helper.c    | 43 +++++++++++++++++++++++++------------------
 target/arm/op_helper.c |  2 +-
 6 files changed, 80 insertions(+), 37 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/7] target/arm: Use M_REG_NUM_BANKS rather than hardcoding 2

[Peter Maydell]

Use a symbolic constant M_REG_NUM_BANKS for the array size for
registers which are banked by M profile security state, rather
than hardcoding lots of 2s.

Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Philippe suggested this in review on the last round of
patches but I forgot about it :-(
---
 target/arm/cpu.h | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 98b9b26..5a1f957 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -81,8 +81,11 @@
  * accessed via env->registerfield[env->v7m.secure] (whether the security
  * extension is implemented or not).
  */
-#define M_REG_NS 0
-#define M_REG_S 1
+enum {
+    M_REG_NS = 0,
+    M_REG_S = 1,
+    M_REG_NUM_BANKS = 2,
+};
 
 /* ARM-specific interrupt pending bits.  */
 #define CPU_INTERRUPT_FIQ   CPU_INTERRUPT_TGT_EXT_1
@@ -433,19 +436,19 @@ typedef struct CPUARMState {
         uint32_t other_sp;
         uint32_t other_ss_msp;
         uint32_t other_ss_psp;
-        uint32_t vecbase[2];
-        uint32_t basepri[2];
-        uint32_t control[2];
-        uint32_t ccr[2]; /* Configuration and Control */
-        uint32_t cfsr[2]; /* Configurable Fault Status */
+        uint32_t vecbase[M_REG_NUM_BANKS];
+        uint32_t basepri[M_REG_NUM_BANKS];
+        uint32_t control[M_REG_NUM_BANKS];
+        uint32_t ccr[M_REG_NUM_BANKS]; /* Configuration and Control */
+        uint32_t cfsr[M_REG_NUM_BANKS]; /* Configurable Fault Status */
         uint32_t hfsr; /* HardFault Status */
         uint32_t dfsr; /* Debug Fault Status Register */
-        uint32_t mmfar[2]; /* MemManage Fault Address */
+        uint32_t mmfar[M_REG_NUM_BANKS]; /* MemManage Fault Address */
         uint32_t bfar; /* BusFault Address */
-        unsigned mpu_ctrl[2]; /* MPU_CTRL */
+        unsigned mpu_ctrl[M_REG_NUM_BANKS]; /* MPU_CTRL */
         int exception;
-        uint32_t primask[2];
-        uint32_t faultmask[2];
+        uint32_t primask[M_REG_NUM_BANKS];
+        uint32_t faultmask[M_REG_NUM_BANKS];
         uint32_t secure; /* Is CPU in Secure state? (not guest visible) */
     } v7m;
 
@@ -546,7 +549,7 @@ typedef struct CPUARMState {
         uint32_t *drbar;
         uint32_t *drsr;
         uint32_t *dracr;
-        uint32_t rnr[2];
+        uint32_t rnr[M_REG_NUM_BANKS];
     } pmsav7;
 
     /* PMSAv8 MPU */
@@ -556,10 +559,10 @@ typedef struct CPUARMState {
          *  pmsav7.rnr (region number register)
          *  pmsav7_dregion (number of configured regions)
          */
-        uint32_t *rbar[2];
-        uint32_t *rlar[2];
-        uint32_t mair0[2];
-        uint32_t mair1[2];
+        uint32_t *rbar[M_REG_NUM_BANKS];
+        uint32_t *rlar[M_REG_NUM_BANKS];
+        uint32_t mair0[M_REG_NUM_BANKS];
+        uint32_t mair1[M_REG_NUM_BANKS];
     } pmsav8;
 
     void *nvic;
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/7] target/arm: Use M_REG_NUM_BANKS rather than hardcoding 2

[Philippe Mathieu-Daudé]

On 09/11/2017 10:52 AM, Peter Maydell wrote:
> Use a symbolic constant M_REG_NUM_BANKS for the array size for
> registers which are banked by M profile security state, rather
> than hardcoding lots of 2s.
> 
> Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
> Philippe suggested this in review on the last round of
> patches but I forgot about it :-(

Thank you, it eases review and looks cleaner :)

> ---
>   target/arm/cpu.h | 35 +++++++++++++++++++----------------
>   1 file changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 98b9b26..5a1f957 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -81,8 +81,11 @@
>    * accessed via env->registerfield[env->v7m.secure] (whether the security
>    * extension is implemented or not).
>    */
> -#define M_REG_NS 0
> -#define M_REG_S 1
> +enum {
> +    M_REG_NS = 0,
> +    M_REG_S = 1,
> +    M_REG_NUM_BANKS = 2,
> +};
>   
>   /* ARM-specific interrupt pending bits.  */
>   #define CPU_INTERRUPT_FIQ   CPU_INTERRUPT_TGT_EXT_1
> @@ -433,19 +436,19 @@ typedef struct CPUARMState {
>           uint32_t other_sp;
>           uint32_t other_ss_msp;
>           uint32_t other_ss_psp;
> -        uint32_t vecbase[2];
> -        uint32_t basepri[2];
> -        uint32_t control[2];
> -        uint32_t ccr[2]; /* Configuration and Control */
> -        uint32_t cfsr[2]; /* Configurable Fault Status */
> +        uint32_t vecbase[M_REG_NUM_BANKS];
> +        uint32_t basepri[M_REG_NUM_BANKS];
> +        uint32_t control[M_REG_NUM_BANKS];
> +        uint32_t ccr[M_REG_NUM_BANKS]; /* Configuration and Control */
> +        uint32_t cfsr[M_REG_NUM_BANKS]; /* Configurable Fault Status */
>           uint32_t hfsr; /* HardFault Status */
>           uint32_t dfsr; /* Debug Fault Status Register */
> -        uint32_t mmfar[2]; /* MemManage Fault Address */
> +        uint32_t mmfar[M_REG_NUM_BANKS]; /* MemManage Fault Address */
>           uint32_t bfar; /* BusFault Address */
> -        unsigned mpu_ctrl[2]; /* MPU_CTRL */
> +        unsigned mpu_ctrl[M_REG_NUM_BANKS]; /* MPU_CTRL */
>           int exception;
> -        uint32_t primask[2];
> -        uint32_t faultmask[2];
> +        uint32_t primask[M_REG_NUM_BANKS];
> +        uint32_t faultmask[M_REG_NUM_BANKS];
>           uint32_t secure; /* Is CPU in Secure state? (not guest visible) */
>       } v7m;
>   
> @@ -546,7 +549,7 @@ typedef struct CPUARMState {
>           uint32_t *drbar;
>           uint32_t *drsr;
>           uint32_t *dracr;
> -        uint32_t rnr[2];
> +        uint32_t rnr[M_REG_NUM_BANKS];
>       } pmsav7;
>   
>       /* PMSAv8 MPU */
> @@ -556,10 +559,10 @@ typedef struct CPUARMState {
>            *  pmsav7.rnr (region number register)
>            *  pmsav7_dregion (number of configured regions)
>            */
> -        uint32_t *rbar[2];
> -        uint32_t *rlar[2];
> -        uint32_t mair0[2];
> -        uint32_t mair1[2];
> +        uint32_t *rbar[M_REG_NUM_BANKS];
> +        uint32_t *rlar[M_REG_NUM_BANKS];
> +        uint32_t mair0[M_REG_NUM_BANKS];
> +        uint32_t mair1[M_REG_NUM_BANKS];
>       } pmsav8;
>   
>       void *nvic;
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/7] target/arm: Use M_REG_NUM_BANKS rather than hardcoding 2

[Alistair Francis]

On Mon, Sep 11, 2017 at 7:59 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 09/11/2017 10:52 AM, Peter Maydell wrote:
>>
>> Use a symbolic constant M_REG_NUM_BANKS for the array size for
>> registers which are banked by M profile security state, rather
>> than hardcoding lots of 2s.
>>
>> Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,
Alistair

>
>> ---
>> Philippe suggested this in review on the last round of
>> patches but I forgot about it :-(
>
>
> Thank you, it eases review and looks cleaner :)
>
>
>> ---
>>   target/arm/cpu.h | 35 +++++++++++++++++++----------------
>>   1 file changed, 19 insertions(+), 16 deletions(-)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index 98b9b26..5a1f957 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -81,8 +81,11 @@
>>    * accessed via env->registerfield[env->v7m.secure] (whether the
>> security
>>    * extension is implemented or not).
>>    */
>> -#define M_REG_NS 0
>> -#define M_REG_S 1
>> +enum {
>> +    M_REG_NS = 0,
>> +    M_REG_S = 1,
>> +    M_REG_NUM_BANKS = 2,
>> +};
>>     /* ARM-specific interrupt pending bits.  */
>>   #define CPU_INTERRUPT_FIQ   CPU_INTERRUPT_TGT_EXT_1
>> @@ -433,19 +436,19 @@ typedef struct CPUARMState {
>>           uint32_t other_sp;
>>           uint32_t other_ss_msp;
>>           uint32_t other_ss_psp;
>> -        uint32_t vecbase[2];
>> -        uint32_t basepri[2];
>> -        uint32_t control[2];
>> -        uint32_t ccr[2]; /* Configuration and Control */
>> -        uint32_t cfsr[2]; /* Configurable Fault Status */
>> +        uint32_t vecbase[M_REG_NUM_BANKS];
>> +        uint32_t basepri[M_REG_NUM_BANKS];
>> +        uint32_t control[M_REG_NUM_BANKS];
>> +        uint32_t ccr[M_REG_NUM_BANKS]; /* Configuration and Control */
>> +        uint32_t cfsr[M_REG_NUM_BANKS]; /* Configurable Fault Status */
>>           uint32_t hfsr; /* HardFault Status */
>>           uint32_t dfsr; /* Debug Fault Status Register */
>> -        uint32_t mmfar[2]; /* MemManage Fault Address */
>> +        uint32_t mmfar[M_REG_NUM_BANKS]; /* MemManage Fault Address */
>>           uint32_t bfar; /* BusFault Address */
>> -        unsigned mpu_ctrl[2]; /* MPU_CTRL */
>> +        unsigned mpu_ctrl[M_REG_NUM_BANKS]; /* MPU_CTRL */
>>           int exception;
>> -        uint32_t primask[2];
>> -        uint32_t faultmask[2];
>> +        uint32_t primask[M_REG_NUM_BANKS];
>> +        uint32_t faultmask[M_REG_NUM_BANKS];
>>           uint32_t secure; /* Is CPU in Secure state? (not guest visible)
>> */
>>       } v7m;
>>   @@ -546,7 +549,7 @@ typedef struct CPUARMState {
>>           uint32_t *drbar;
>>           uint32_t *drsr;
>>           uint32_t *dracr;
>> -        uint32_t rnr[2];
>> +        uint32_t rnr[M_REG_NUM_BANKS];
>>       } pmsav7;
>>         /* PMSAv8 MPU */
>> @@ -556,10 +559,10 @@ typedef struct CPUARMState {
>>            *  pmsav7.rnr (region number register)
>>            *  pmsav7_dregion (number of configured regions)
>>            */
>> -        uint32_t *rbar[2];
>> -        uint32_t *rlar[2];
>> -        uint32_t mair0[2];
>> -        uint32_t mair1[2];
>> +        uint32_t *rbar[M_REG_NUM_BANKS];
>> +        uint32_t *rlar[M_REG_NUM_BANKS];
>> +        uint32_t mair0[M_REG_NUM_BANKS];
>> +        uint32_t mair1[M_REG_NUM_BANKS];
>>       } pmsav8;
>>         void *nvic;
>>
>

[Qemu-devel] [PATCH 2/7] target/arm: Clear exclusive monitor on v7M reset, exception entry/exit

[Peter Maydell]

For M profile we must clear the exclusive monitor on reset, exception
entry and exception exit.  We weren't doing any of these things; fix
this bug.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/internals.h | 10 ++++++++++
 target/arm/cpu.c       |  6 ++++++
 target/arm/helper.c    |  2 ++
 target/arm/op_helper.c |  2 +-
 4 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 5d7f24c..a315354 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -444,6 +444,16 @@ void arm_handle_psci_call(ARMCPU *cpu);
 #endif
 
 /**
+ * arm_clear_exclusive: clear the exclusive monitor
+ * @env: CPU env
+ * Clear the CPU's exclusive monitor, like the guest CLREX instruction.
+ */
+static inline void arm_clear_exclusive(CPUARMState *env)
+{
+    env->exclusive_addr = -1;
+}
+
+/**
  * ARMMMUFaultInfo: Information describing an ARM MMU Fault
  * @s2addr: Address that caused a fault at stage 2
  * @stage2: True if we faulted at stage 2
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index a1acce3..412e94c 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -235,6 +235,12 @@ static void arm_cpu_reset(CPUState *s)
         env->regs[15] = 0xFFFF0000;
     }
 
+    /* M profile requires that reset clears the exclusive monitor;
+     * A profile does not, but clearing it makes more sense than having it
+     * set with an exclusive access on address zero.
+     */
+    arm_clear_exclusive(env);
+
     env->vfp.xregs[ARM_VFP_FPEXC] = 0;
 #endif
 
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 329e517..668e367 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6175,6 +6175,7 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr)
 
     armv7m_nvic_acknowledge_irq(env->nvic);
     switch_v7m_sp(env, 0);
+    arm_clear_exclusive(env);
     /* Clear IT bits */
     env->condexec_bits = 0;
     env->regs[14] = lr;
@@ -6354,6 +6355,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
     }
 
     /* Otherwise, we have a successful exception exit. */
+    arm_clear_exclusive(env);
     qemu_log_mask(CPU_LOG_INT, "...successful exception return\n");
 }
 
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index d1bca46..6a60464 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -1022,7 +1022,7 @@ void HELPER(exception_return)(CPUARMState *env)
 
     aarch64_save_sp(env, cur_el);
 
-    env->exclusive_addr = -1;
+    arm_clear_exclusive(env);
 
     /* We must squash the PSTATE.SS bit to zero unless both of the
      * following hold:
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/7] target/arm: Clear exclusive monitor on v7M reset, exception entry/exit

[Alistair Francis]

On Mon, Sep 11, 2017 at 6:52 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> For M profile we must clear the exclusive monitor on reset, exception
> entry and exception exit.  We weren't doing any of these things; fix
> this bug.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,
Alistair

> ---
>  target/arm/internals.h | 10 ++++++++++
>  target/arm/cpu.c       |  6 ++++++
>  target/arm/helper.c    |  2 ++
>  target/arm/op_helper.c |  2 +-
>  4 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 5d7f24c..a315354 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -444,6 +444,16 @@ void arm_handle_psci_call(ARMCPU *cpu);
>  #endif
>
>  /**
> + * arm_clear_exclusive: clear the exclusive monitor
> + * @env: CPU env
> + * Clear the CPU's exclusive monitor, like the guest CLREX instruction.
> + */
> +static inline void arm_clear_exclusive(CPUARMState *env)
> +{
> +    env->exclusive_addr = -1;
> +}
> +
> +/**
>   * ARMMMUFaultInfo: Information describing an ARM MMU Fault
>   * @s2addr: Address that caused a fault at stage 2
>   * @stage2: True if we faulted at stage 2
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index a1acce3..412e94c 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -235,6 +235,12 @@ static void arm_cpu_reset(CPUState *s)
>          env->regs[15] = 0xFFFF0000;
>      }
>
> +    /* M profile requires that reset clears the exclusive monitor;
> +     * A profile does not, but clearing it makes more sense than having it
> +     * set with an exclusive access on address zero.
> +     */
> +    arm_clear_exclusive(env);
> +
>      env->vfp.xregs[ARM_VFP_FPEXC] = 0;
>  #endif
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 329e517..668e367 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6175,6 +6175,7 @@ static void v7m_exception_taken(ARMCPU *cpu, uint32_t lr)
>
>      armv7m_nvic_acknowledge_irq(env->nvic);
>      switch_v7m_sp(env, 0);
> +    arm_clear_exclusive(env);
>      /* Clear IT bits */
>      env->condexec_bits = 0;
>      env->regs[14] = lr;
> @@ -6354,6 +6355,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>      }
>
>      /* Otherwise, we have a successful exception exit. */
> +    arm_clear_exclusive(env);
>      qemu_log_mask(CPU_LOG_INT, "...successful exception return\n");
>  }
>
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index d1bca46..6a60464 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -1022,7 +1022,7 @@ void HELPER(exception_return)(CPUARMState *env)
>
>      aarch64_save_sp(env, cur_el);
>
> -    env->exclusive_addr = -1;
> +    arm_clear_exclusive(env);
>
>      /* We must squash the PSTATE.SS bit to zero unless both of the
>       * following hold:
> --
> 2.7.4
>
>

Re: [Qemu-devel] [PATCH 2/7] target/arm: Clear exclusive monitor on v7M reset, exception entry/exit

[Richard Henderson]

On 09/11/2017 06:52 AM, Peter Maydell wrote:
> For M profile we must clear the exclusive monitor on reset, exception
> entry and exception exit.  We weren't doing any of these things; fix
> this bug.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/internals.h | 10 ++++++++++
>  target/arm/cpu.c       |  6 ++++++
>  target/arm/helper.c    |  2 ++
>  target/arm/op_helper.c |  2 +-
>  4 files changed, 19 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 3/7] target/arm: Get PRECISERR and IBUSERR the right way round

[Peter Maydell]

For a bus fault, the M profile BFSR bit PRECISERR means a bus
fault on a data access, and IBUSERR means a bus fault on an
instruction access. We had these the wrong way around; fix this.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 668e367..1741e0d 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6430,15 +6430,15 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
         case 0x8: /* External Abort */
             switch (cs->exception_index) {
             case EXCP_PREFETCH_ABORT:
-                env->v7m.cfsr[M_REG_NS] |= R_V7M_CFSR_PRECISERR_MASK;
-                qemu_log_mask(CPU_LOG_INT, "...with CFSR.PRECISERR\n");
+                env->v7m.cfsr[M_REG_NS] |= R_V7M_CFSR_IBUSERR_MASK;
+                qemu_log_mask(CPU_LOG_INT, "...with CFSR.IBUSERR\n");
                 break;
             case EXCP_DATA_ABORT:
                 env->v7m.cfsr[M_REG_NS] |=
-                    (R_V7M_CFSR_IBUSERR_MASK | R_V7M_CFSR_BFARVALID_MASK);
+                    (R_V7M_CFSR_PRECISERR_MASK | R_V7M_CFSR_BFARVALID_MASK);
                 env->v7m.bfar = env->exception.vaddress;
                 qemu_log_mask(CPU_LOG_INT,
-                              "...with CFSR.IBUSERR and BFAR 0x%x\n",
+                              "...with CFSR.PRECISERR and BFAR 0x%x\n",
                               env->v7m.bfar);
                 break;
             }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 3/7] target/arm: Get PRECISERR and IBUSERR the right way round

[Alistair Francis]

On Mon, Sep 11, 2017 at 6:52 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> For a bus fault, the M profile BFSR bit PRECISERR means a bus
> fault on a data access, and IBUSERR means a bus fault on an
> instruction access. We had these the wrong way around; fix this.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,
Alistair

> ---
>  target/arm/helper.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 668e367..1741e0d 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6430,15 +6430,15 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>          case 0x8: /* External Abort */
>              switch (cs->exception_index) {
>              case EXCP_PREFETCH_ABORT:
> -                env->v7m.cfsr[M_REG_NS] |= R_V7M_CFSR_PRECISERR_MASK;
> -                qemu_log_mask(CPU_LOG_INT, "...with CFSR.PRECISERR\n");
> +                env->v7m.cfsr[M_REG_NS] |= R_V7M_CFSR_IBUSERR_MASK;
> +                qemu_log_mask(CPU_LOG_INT, "...with CFSR.IBUSERR\n");
>                  break;
>              case EXCP_DATA_ABORT:
>                  env->v7m.cfsr[M_REG_NS] |=
> -                    (R_V7M_CFSR_IBUSERR_MASK | R_V7M_CFSR_BFARVALID_MASK);
> +                    (R_V7M_CFSR_PRECISERR_MASK | R_V7M_CFSR_BFARVALID_MASK);
>                  env->v7m.bfar = env->exception.vaddress;
>                  qemu_log_mask(CPU_LOG_INT,
> -                              "...with CFSR.IBUSERR and BFAR 0x%x\n",
> +                              "...with .PRECISERR and BFAR 0x%x\n",
>                                env->v7m.bfar);
>                  break;
>              }
> --
> 2.7.4
>
>

Re: [Qemu-devel] [PATCH 3/7] target/arm: Get PRECISERR and IBUSERR the right way round

[Richard Henderson]

On 09/11/2017 06:52 AM, Peter Maydell wrote:
> For a bus fault, the M profile BFSR bit PRECISERR means a bus
> fault on a data access, and IBUSERR means a bus fault on an
> instruction access. We had these the wrong way around; fix this.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 4/7] nvic: Don't apply group priority mask to negative priorities

[Peter Maydell]

In several places we were unconditionally applying the
nvic_gprio_mask() to a priority value. This is incorrect
if the priority is one of the fixed negative priority
values (for NMI and HardFault), so don't do it.

This bug would have caused both NMI and HardFault to be
considered as the same priority and so NMI wouldn't
correctly preempt HardFault.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/armv7m_nvic.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 1fecfd6..d3e2056 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -152,8 +152,12 @@ static void nvic_recompute_state(NVICState *s)
         }
     }
 
+    if (active_prio > 0) {
+        active_prio &= nvic_gprio_mask(s);
+    }
+
     s->vectpending = pend_irq;
-    s->exception_prio = active_prio & nvic_gprio_mask(s);
+    s->exception_prio = active_prio;
 
     trace_nvic_recompute_state(s->vectpending, s->exception_prio);
 }
@@ -329,7 +333,10 @@ void armv7m_nvic_acknowledge_irq(void *opaque)
     assert(vec->enabled);
     assert(vec->pending);
 
-    pendgroupprio = vec->prio & nvic_gprio_mask(s);
+    pendgroupprio = vec->prio;
+    if (pendgroupprio > 0) {
+        pendgroupprio &= nvic_gprio_mask(s);
+    }
     assert(pendgroupprio < running);
 
     trace_nvic_acknowledge_irq(pending, vec->prio);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 4/7] nvic: Don't apply group priority mask to negative priorities

[Richard Henderson]

On 09/11/2017 06:52 AM, Peter Maydell wrote:
> In several places we were unconditionally applying the
> nvic_gprio_mask() to a priority value. This is incorrect
> if the priority is one of the fixed negative priority
> values (for NMI and HardFault), so don't do it.
> 
> This bug would have caused both NMI and HardFault to be
> considered as the same priority and so NMI wouldn't
> correctly preempt HardFault.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/intc/armv7m_nvic.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 5/7] target/arm: Remove unnecessary '| 0xf0000000' from do_v7m_exception_exit()

[Peter Maydell]

In do_v7m_exception_exit(), there's no need to force the high 4
bits of 'type' to 1 when calling v7m_exception_taken(), because
we know that they're always 1 or we could not have got to this
"handle return to magic exception return address" code. Remove
the unnecessary ORs.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 1741e0d..fdd5cc6 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6306,7 +6306,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
          */
         env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVPC_MASK;
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
-        v7m_exception_taken(cpu, type | 0xf0000000);
+        v7m_exception_taken(cpu, type);
         qemu_log_mask(CPU_LOG_INT, "...taking UsageFault on existing "
                       "stackframe: failed exception return integrity check\n");
         return;
@@ -6348,7 +6348,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
         env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVPC_MASK;
         v7m_push_stack(cpu);
-        v7m_exception_taken(cpu, type | 0xf0000000);
+        v7m_exception_taken(cpu, type);
         qemu_log_mask(CPU_LOG_INT, "...taking UsageFault on new stackframe: "
                       "failed exception return integrity check\n");
         return;
-- 
2.7.4

Re: [Qemu-devel] [PATCH 5/7] target/arm: Remove unnecessary '| 0xf0000000' from do_v7m_exception_exit()

[Alistair Francis]

On Mon, Sep 11, 2017 at 6:52 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> In do_v7m_exception_exit(), there's no need to force the high 4
> bits of 'type' to 1 when calling v7m_exception_taken(), because
> we know that they're always 1 or we could not have got to this
> "handle return to magic exception return address" code. Remove
> the unnecessary ORs.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Acked-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,
Alistair

> ---
>  target/arm/helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 1741e0d..fdd5cc6 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6306,7 +6306,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>           */
>          env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVPC_MASK;
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
> -        v7m_exception_taken(cpu, type | 0xf0000000);
> +        v7m_exception_taken(cpu, type);
>          qemu_log_mask(CPU_LOG_INT, "...taking UsageFault on existing "
>                        "stackframe: failed exception return integrity check\n");
>          return;
> @@ -6348,7 +6348,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>          armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
>          env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVPC_MASK;
>          v7m_push_stack(cpu);
> -        v7m_exception_taken(cpu, type | 0xf0000000);
> +        v7m_exception_taken(cpu, type);
>          qemu_log_mask(CPU_LOG_INT, "...taking UsageFault on new stackframe: "
>                        "failed exception return integrity check\n");
>          return;
> --
> 2.7.4
>
>

Re: [Qemu-devel] [PATCH 5/7] target/arm: Remove unnecessary '| 0xf0000000' from do_v7m_exception_exit()

[Richard Henderson]

On 09/11/2017 06:52 AM, Peter Maydell wrote:
> In do_v7m_exception_exit(), there's no need to force the high 4
> bits of 'type' to 1 when calling v7m_exception_taken(), because
> we know that they're always 1 or we could not have got to this
> "handle return to magic exception return address" code. Remove
> the unnecessary ORs.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 6/7] target/arm: Add and use defines for EXCRET constants

[Peter Maydell]

The exception-return magic values get some new bits in v8M, which
makes some bit definitions for them worthwhile.

We don't use the bit definitions for the switch on the low bits
which checks the return type for v7M, because this is defined
in the v7M ARM ARM as a set of valid values rather than via
per-bit checks.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/internals.h | 10 ++++++++++
 target/arm/helper.c    | 14 +++++++++-----
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index a315354..18be370 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -61,6 +61,16 @@ FIELD(V7M_CONTROL, NPRIV, 0, 1)
 FIELD(V7M_CONTROL, SPSEL, 1, 1)
 FIELD(V7M_CONTROL, FPCA, 2, 1)
 
+/* Bit definitions for v7M exception return payload */
+FIELD(V7M_EXCRET, ES, 0, 1)
+FIELD(V7M_EXCRET, RES0, 1, 1)
+FIELD(V7M_EXCRET, SPSEL, 2, 1)
+FIELD(V7M_EXCRET, MODE, 3, 1)
+FIELD(V7M_EXCRET, FTYPE, 4, 1)
+FIELD(V7M_EXCRET, DCRS, 5, 1)
+FIELD(V7M_EXCRET, S, 6, 1)
+FIELD(V7M_EXCRET, RES1, 7, 25) /* including the must-be-1 prefix */
+
 /*
  * For AArch64, map a given EL to an index in the banked_spsr array.
  * Note that this mapping and the AArch32 mapping defined in bank_number()
diff --git a/target/arm/helper.c b/target/arm/helper.c
index fdd5cc6..a502e4e 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6242,7 +6242,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
                   " previous exception %d\n",
                   type, env->v7m.exception);
 
-    if (extract32(type, 5, 23) != extract32(-1, 5, 23)) {
+    if ((type & R_V7M_EXCRET_RES1_MASK) != R_V7M_EXCRET_RES1_MASK) {
         qemu_log_mask(LOG_GUEST_ERROR, "M profile: zero high bits in exception "
                       "exit PC value 0x%" PRIx32 " are UNPREDICTABLE\n", type);
     }
@@ -6255,7 +6255,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
          * which security state's faultmask to clear. (v8M ARM ARM R_KBNF.)
          */
         if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
-            int es = type & 1;
+            int es = type & R_V7M_EXCRET_ES_MASK;
             if (armv7m_nvic_raw_execution_priority(env->nvic) >= 0) {
                 env->v7m.faultmask[es] = 0;
             }
@@ -6491,12 +6491,16 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
         return; /* Never happens.  Keep compiler happy.  */
     }
 
-    lr = 0xfffffff1;
+    lr = R_V7M_EXCRET_RES1_MASK |
+        R_V7M_EXCRET_S_MASK |
+        R_V7M_EXCRET_DCRS_MASK |
+        R_V7M_EXCRET_FTYPE_MASK |
+        R_V7M_EXCRET_ES_MASK;
     if (env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_SPSEL_MASK) {
-        lr |= 4;
+        lr |= R_V7M_EXCRET_SPSEL_MASK;
     }
     if (!arm_v7m_is_handler_mode(env)) {
-        lr |= 8;
+        lr |= R_V7M_EXCRET_MODE_MASK;
     }
 
     v7m_push_stack(cpu);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 6/7] target/arm: Add and use defines for EXCRET constants

[Alistair Francis]

On Mon, Sep 11, 2017 at 6:52 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> The exception-return magic values get some new bits in v8M, which
> makes some bit definitions for them worthwhile.
>
> We don't use the bit definitions for the switch on the low bits
> which checks the return type for v7M, because this is defined
> in the v7M ARM ARM as a set of valid values rather than via
> per-bit checks.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,
Alistair

> ---
>  target/arm/internals.h | 10 ++++++++++
>  target/arm/helper.c    | 14 +++++++++-----
>  2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index a315354..18be370 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -61,6 +61,16 @@ FIELD(V7M_CONTROL, NPRIV, 0, 1)
>  FIELD(V7M_CONTROL, SPSEL, 1, 1)
>  FIELD(V7M_CONTROL, FPCA, 2, 1)
>
> +/* Bit definitions for v7M exception return payload */
> +FIELD(V7M_EXCRET, ES, 0, 1)
> +FIELD(V7M_EXCRET, RES0, 1, 1)
> +FIELD(V7M_EXCRET, SPSEL, 2, 1)
> +FIELD(V7M_EXCRET, MODE, 3, 1)
> +FIELD(V7M_EXCRET, FTYPE, 4, 1)
> +FIELD(V7M_EXCRET, DCRS, 5, 1)
> +FIELD(V7M_EXCRET, S, 6, 1)
> +FIELD(V7M_EXCRET, RES1, 7, 25) /* including the must-be-1 prefix */
> +
>  /*
>   * For AArch64, map a given EL to an index in the banked_spsr array.
>   * Note that this mapping and the AArch32 mapping defined in bank_number()
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index fdd5cc6..a502e4e 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6242,7 +6242,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>                    " previous exception %d\n",
>                    type, env->v7m.exception);
>
> -    if (extract32(type, 5, 23) != extract32(-1, 5, 23)) {
> +    if ((type & R_V7M_EXCRET_RES1_MASK) != R_V7M_EXCRET_RES1_MASK) {
>          qemu_log_mask(LOG_GUEST_ERROR, "M profile: zero high bits in exception "
>                        "exit PC value 0x%" PRIx32 " are UNPREDICTABLE\n", type);
>      }
> @@ -6255,7 +6255,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>           * which security state's faultmask to clear. (v8M ARM ARM R_KBNF.)
>           */
>          if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
> -            int es = type & 1;
> +            int es = type & R_V7M_EXCRET_ES_MASK;
>              if (armv7m_nvic_raw_execution_priority(env->nvic) >= 0) {
>                  env->v7m.faultmask[es] = 0;
>              }
> @@ -6491,12 +6491,16 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>          return; /* Never happens.  Keep compiler happy.  */
>      }
>
> -    lr = 0xfffffff1;
> +    lr = R_V7M_EXCRET_RES1_MASK |
> +        R_V7M_EXCRET_S_MASK |
> +        R_V7M_EXCRET_DCRS_MASK |
> +        R_V7M_EXCRET_FTYPE_MASK |
> +        R_V7M_EXCRET_ES_MASK;
>      if (env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_SPSEL_MASK) {
> -        lr |= 4;
> +        lr |= R_V7M_EXCRET_SPSEL_MASK;
>      }
>      if (!arm_v7m_is_handler_mode(env)) {
> -        lr |= 8;
> +        lr |= R_V7M_EXCRET_MODE_MASK;
>      }
>
>      v7m_push_stack(cpu);
> --
> 2.7.4
>
>

[Qemu-devel] [PATCH 7/7] target/arm: Rename 'type' to 'excret' in do_v7m_exception_exit()

[Peter Maydell]

In the v7M and v8M ARM ARM, the magic exception return values are
referred to as EXC_RETURN values, and in QEMU we use V7M_EXCRET_*
constants to define bits within them. Rename the 'type' variable
which holds the exception return value in do_v7m_exception_exit()
to excret, making it clearer that it does hold an EXC_RETURN value.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index a502e4e..4f41841 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6212,7 +6212,7 @@ static void v7m_push_stack(ARMCPU *cpu)
 static void do_v7m_exception_exit(ARMCPU *cpu)
 {
     CPUARMState *env = &cpu->env;
-    uint32_t type;
+    uint32_t excret;
     uint32_t xpsr;
     bool ufault = false;
     bool return_to_sp_process = false;
@@ -6233,18 +6233,19 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
      * the target value up between env->regs[15] and env->thumb in
      * gen_bx(). Reconstitute it.
      */
-    type = env->regs[15];
+    excret = env->regs[15];
     if (env->thumb) {
-        type |= 1;
+        excret |= 1;
     }
 
     qemu_log_mask(CPU_LOG_INT, "Exception return: magic PC %" PRIx32
                   " previous exception %d\n",
-                  type, env->v7m.exception);
+                  excret, env->v7m.exception);
 
-    if ((type & R_V7M_EXCRET_RES1_MASK) != R_V7M_EXCRET_RES1_MASK) {
+    if ((excret & R_V7M_EXCRET_RES1_MASK) != R_V7M_EXCRET_RES1_MASK) {
         qemu_log_mask(LOG_GUEST_ERROR, "M profile: zero high bits in exception "
-                      "exit PC value 0x%" PRIx32 " are UNPREDICTABLE\n", type);
+                      "exit PC value 0x%" PRIx32 " are UNPREDICTABLE\n",
+                      excret);
     }
 
     if (env->v7m.exception != ARMV7M_EXCP_NMI) {
@@ -6255,7 +6256,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
          * which security state's faultmask to clear. (v8M ARM ARM R_KBNF.)
          */
         if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
-            int es = type & R_V7M_EXCRET_ES_MASK;
+            int es = excret & R_V7M_EXCRET_ES_MASK;
             if (armv7m_nvic_raw_execution_priority(env->nvic) >= 0) {
                 env->v7m.faultmask[es] = 0;
             }
@@ -6283,7 +6284,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
         g_assert_not_reached();
     }
 
-    switch (type & 0xf) {
+    switch (excret & 0xf) {
     case 1: /* Return to Handler */
         return_to_handler = true;
         break;
@@ -6306,7 +6307,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
          */
         env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVPC_MASK;
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
-        v7m_exception_taken(cpu, type);
+        v7m_exception_taken(cpu, excret);
         qemu_log_mask(CPU_LOG_INT, "...taking UsageFault on existing "
                       "stackframe: failed exception return integrity check\n");
         return;
@@ -6341,14 +6342,14 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
 
     /* The restored xPSR exception field will be zero if we're
      * resuming in Thread mode. If that doesn't match what the
-     * exception return type specified then this is a UsageFault.
+     * exception return excret specified then this is a UsageFault.
      */
     if (return_to_handler != arm_v7m_is_handler_mode(env)) {
         /* Take an INVPC UsageFault by pushing the stack again. */
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
         env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVPC_MASK;
         v7m_push_stack(cpu);
-        v7m_exception_taken(cpu, type);
+        v7m_exception_taken(cpu, excret);
         qemu_log_mask(CPU_LOG_INT, "...taking UsageFault on new stackframe: "
                       "failed exception return integrity check\n");
         return;
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 7/7] target/arm: Rename 'type' to 'excret' in do_v7m_exception_exit()

[Philippe Mathieu-Daudé]

On 09/11/2017 10:52 AM, Peter Maydell wrote:
> In the v7M and v8M ARM ARM, the magic exception return values are
> referred to as EXC_RETURN values, and in QEMU we use V7M_EXCRET_*
> constants to define bits within them. Rename the 'type' variable
> which holds the exception return value in do_v7m_exception_exit()
> to excret, making it clearer that it does hold an EXC_RETURN value.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>   target/arm/helper.c | 23 ++++++++++++-----------
>   1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index a502e4e..4f41841 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6212,7 +6212,7 @@ static void v7m_push_stack(ARMCPU *cpu)
>   static void do_v7m_exception_exit(ARMCPU *cpu)
>   {
>       CPUARMState *env = &cpu->env;
> -    uint32_t type;
> +    uint32_t excret;
>       uint32_t xpsr;
>       bool ufault = false;
>       bool return_to_sp_process = false;
> @@ -6233,18 +6233,19 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>        * the target value up between env->regs[15] and env->thumb in
>        * gen_bx(). Reconstitute it.
>        */
> -    type = env->regs[15];
> +    excret = env->regs[15];
>       if (env->thumb) {
> -        type |= 1;
> +        excret |= 1;
>       }
>   
>       qemu_log_mask(CPU_LOG_INT, "Exception return: magic PC %" PRIx32
>                     " previous exception %d\n",
> -                  type, env->v7m.exception);
> +                  excret, env->v7m.exception);
>   
> -    if ((type & R_V7M_EXCRET_RES1_MASK) != R_V7M_EXCRET_RES1_MASK) {
> +    if ((excret & R_V7M_EXCRET_RES1_MASK) != R_V7M_EXCRET_RES1_MASK) {
>           qemu_log_mask(LOG_GUEST_ERROR, "M profile: zero high bits in exception "
> -                      "exit PC value 0x%" PRIx32 " are UNPREDICTABLE\n", type);
> +                      "exit PC value 0x%" PRIx32 " are UNPREDICTABLE\n",
> +                      excret);
>       }
>   
>       if (env->v7m.exception != ARMV7M_EXCP_NMI) {
> @@ -6255,7 +6256,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>            * which security state's faultmask to clear. (v8M ARM ARM R_KBNF.)
>            */
>           if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
> -            int es = type & R_V7M_EXCRET_ES_MASK;
> +            int es = excret & R_V7M_EXCRET_ES_MASK;
>               if (armv7m_nvic_raw_execution_priority(env->nvic) >= 0) {
>                   env->v7m.faultmask[es] = 0;
>               }
> @@ -6283,7 +6284,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>           g_assert_not_reached();
>       }
>   
> -    switch (type & 0xf) {
> +    switch (excret & 0xf) {
>       case 1: /* Return to Handler */
>           return_to_handler = true;
>           break;
> @@ -6306,7 +6307,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>            */
>           env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVPC_MASK;
>           armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
> -        v7m_exception_taken(cpu, type);
> +        v7m_exception_taken(cpu, excret);
>           qemu_log_mask(CPU_LOG_INT, "...taking UsageFault on existing "
>                         "stackframe: failed exception return integrity check\n");
>           return;
> @@ -6341,14 +6342,14 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>   
>       /* The restored xPSR exception field will be zero if we're
>        * resuming in Thread mode. If that doesn't match what the
> -     * exception return type specified then this is a UsageFault.
> +     * exception return excret specified then this is a UsageFault.
>        */
>       if (return_to_handler != arm_v7m_is_handler_mode(env)) {
>           /* Take an INVPC UsageFault by pushing the stack again. */
>           armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
>           env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVPC_MASK;
>           v7m_push_stack(cpu);
> -        v7m_exception_taken(cpu, type);
> +        v7m_exception_taken(cpu, excret);
>           qemu_log_mask(CPU_LOG_INT, "...taking UsageFault on new stackframe: "
>                         "failed exception return integrity check\n");
>           return;
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 7/7] target/arm: Rename 'type' to 'excret' in do_v7m_exception_exit()

[Alistair Francis]

On Mon, Sep 11, 2017 at 8:03 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 09/11/2017 10:52 AM, Peter Maydell wrote:
>>
>> In the v7M and v8M ARM ARM, the magic exception return values are
>> referred to as EXC_RETURN values, and in QEMU we use V7M_EXCRET_*
>> constants to define bits within them. Rename the 'type' variable
>> which holds the exception return value in do_v7m_exception_exit()
>> to excret, making it clearer that it does hold an EXC_RETURN value.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,
Alistair

>
>
>> ---
>>   target/arm/helper.c | 23 ++++++++++++-----------
>>   1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index a502e4e..4f41841 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -6212,7 +6212,7 @@ static void v7m_push_stack(ARMCPU *cpu)
>>   static void do_v7m_exception_exit(ARMCPU *cpu)
>>   {
>>       CPUARMState *env = &cpu->env;
>> -    uint32_t type;
>> +    uint32_t excret;
>>       uint32_t xpsr;
>>       bool ufault = false;
>>       bool return_to_sp_process = false;
>> @@ -6233,18 +6233,19 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>>        * the target value up between env->regs[15] and env->thumb in
>>        * gen_bx(). Reconstitute it.
>>        */
>> -    type = env->regs[15];
>> +    excret = env->regs[15];
>>       if (env->thumb) {
>> -        type |= 1;
>> +        excret |= 1;
>>       }
>>         qemu_log_mask(CPU_LOG_INT, "Exception return: magic PC %" PRIx32
>>                     " previous exception %d\n",
>> -                  type, env->v7m.exception);
>> +                  excret, env->v7m.exception);
>>   -    if ((type & R_V7M_EXCRET_RES1_MASK) != R_V7M_EXCRET_RES1_MASK) {
>> +    if ((excret & R_V7M_EXCRET_RES1_MASK) != R_V7M_EXCRET_RES1_MASK) {
>>           qemu_log_mask(LOG_GUEST_ERROR, "M profile: zero high bits in
>> exception "
>> -                      "exit PC value 0x%" PRIx32 " are UNPREDICTABLE\n",
>> type);
>> +                      "exit PC value 0x%" PRIx32 " are UNPREDICTABLE\n",
>> +                      excret);
>>       }
>>         if (env->v7m.exception != ARMV7M_EXCP_NMI) {
>> @@ -6255,7 +6256,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>>            * which security state's faultmask to clear. (v8M ARM ARM
>> R_KBNF.)
>>            */
>>           if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
>> -            int es = type & R_V7M_EXCRET_ES_MASK;
>> +            int es = excret & R_V7M_EXCRET_ES_MASK;
>>               if (armv7m_nvic_raw_execution_priority(env->nvic) >= 0) {
>>                   env->v7m.faultmask[es] = 0;
>>               }
>> @@ -6283,7 +6284,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>>           g_assert_not_reached();
>>       }
>>   -    switch (type & 0xf) {
>> +    switch (excret & 0xf) {
>>       case 1: /* Return to Handler */
>>           return_to_handler = true;
>>           break;
>> @@ -6306,7 +6307,7 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>>            */
>>           env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVPC_MASK;
>>           armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
>> -        v7m_exception_taken(cpu, type);
>> +        v7m_exception_taken(cpu, excret);
>>           qemu_log_mask(CPU_LOG_INT, "...taking UsageFault on existing "
>>                         "stackframe: failed exception return integrity
>> check\n");
>>           return;
>> @@ -6341,14 +6342,14 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
>>         /* The restored xPSR exception field will be zero if we're
>>        * resuming in Thread mode. If that doesn't match what the
>> -     * exception return type specified then this is a UsageFault.
>> +     * exception return excret specified then this is a UsageFault.
>>        */
>>       if (return_to_handler != arm_v7m_is_handler_mode(env)) {
>>           /* Take an INVPC UsageFault by pushing the stack again. */
>>           armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_USAGE);
>>           env->v7m.cfsr[env->v7m.secure] |= R_V7M_CFSR_INVPC_MASK;
>>           v7m_push_stack(cpu);
>> -        v7m_exception_taken(cpu, type);
>> +        v7m_exception_taken(cpu, excret);
>>           qemu_log_mask(CPU_LOG_INT, "...taking UsageFault on new
>> stackframe: "
>>                         "failed exception return integrity check\n");
>>           return;
>>
>

Re: [Qemu-devel] [PATCH 7/7] target/arm: Rename 'type' to 'excret' in do_v7m_exception_exit()

[Richard Henderson]

On 09/11/2017 06:52 AM, Peter Maydell wrote:
> In the v7M and v8M ARM ARM, the magic exception return values are
> referred to as EXC_RETURN values, and in QEMU we use V7M_EXCRET_*
> constants to define bits within them. Rename the 'type' variable
> which holds the exception return value in do_v7m_exception_exit()
> to excret, making it clearer that it does hold an EXC_RETURN value.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~