diff mbox series

[15/20] target/arm: Fix calculation of secure mm_idx values

Message ID 1506092407-26985-16-git-send-email-peter.maydell@linaro.org
State Superseded
Headers show
Series ARM v8M: exception entry, exit and security | expand

Commit Message

Peter Maydell Sept. 22, 2017, 3 p.m. UTC
In cpu_mmu_index() we try to do this:
        if (env->v7m.secure) {
            mmu_idx += ARMMMUIdx_MSUser;
        }
but it will give the wrong answer, because ARMMMUIdx_MSUser
includes the 0x40 ARM_MMU_IDX_M field, and so does the
mmu_idx we're adding to, and we'll end up with 0x8n rather
than 0x4n. This error is then nullified by the call to
arm_to_core_mmu_idx() which masks out the high part, but
we're about to factor out the code that calculates the
ARMMMUIdx values so it can be used without passing it through
arm_to_core_mmu_idx(), so fix this bug first.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
 target/arm/cpu.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

-- 
2.7.4

Comments

Philippe Mathieu-Daudé Oct. 5, 2017, 4:46 a.m. UTC | #1
On 09/22/2017 12:00 PM, Peter Maydell wrote:
> In cpu_mmu_index() we try to do this:

>         if (env->v7m.secure) {

>             mmu_idx += ARMMMUIdx_MSUser;

>         }

> but it will give the wrong answer, because ARMMMUIdx_MSUser

> includes the 0x40 ARM_MMU_IDX_M field, and so does the

> mmu_idx we're adding to, and we'll end up with 0x8n rather

> than 0x4n. This error is then nullified by the call to

> arm_to_core_mmu_idx() which masks out the high part, but

> we're about to factor out the code that calculates the

> ARMMMUIdx values so it can be used without passing it through

> arm_to_core_mmu_idx(), so fix this bug first.

> 

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


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


> ---

>  target/arm/cpu.h | 12 +++++++-----

>  1 file changed, 7 insertions(+), 5 deletions(-)

> 

> diff --git a/target/arm/cpu.h b/target/arm/cpu.h

> index 441e584..70c1f85 100644

> --- a/target/arm/cpu.h

> +++ b/target/arm/cpu.h

> @@ -2335,14 +2335,16 @@ static inline int cpu_mmu_index(CPUARMState *env, bool ifetch)

>      int el = arm_current_el(env);

>  

>      if (arm_feature(env, ARM_FEATURE_M)) {

> -        ARMMMUIdx mmu_idx = el == 0 ? ARMMMUIdx_MUser : ARMMMUIdx_MPriv;

> +        ARMMMUIdx mmu_idx;

>  

> -        if (armv7m_nvic_neg_prio_requested(env->nvic, env->v7m.secure)) {

> -            mmu_idx = ARMMMUIdx_MNegPri;

> +        if (el == 0) {

> +            mmu_idx = env->v7m.secure ? ARMMMUIdx_MSUser : ARMMMUIdx_MUser;

> +        } else {

> +            mmu_idx = env->v7m.secure ? ARMMMUIdx_MSPriv : ARMMMUIdx_MPriv;

>          }

>  

> -        if (env->v7m.secure) {

> -            mmu_idx += ARMMMUIdx_MSUser;

> +        if (armv7m_nvic_neg_prio_requested(env->nvic, env->v7m.secure)) {

> +            mmu_idx = env->v7m.secure ? ARMMMUIdx_MSNegPri : ARMMMUIdx_MNegPri;

>          }

>  

>          return arm_to_core_mmu_idx(mmu_idx);

>
Richard Henderson Oct. 5, 2017, 6:41 p.m. UTC | #2
On 09/22/2017 11:00 AM, Peter Maydell wrote:
> In cpu_mmu_index() we try to do this:

>         if (env->v7m.secure) {

>             mmu_idx += ARMMMUIdx_MSUser;

>         }

> but it will give the wrong answer, because ARMMMUIdx_MSUser

> includes the 0x40 ARM_MMU_IDX_M field, and so does the

> mmu_idx we're adding to, and we'll end up with 0x8n rather

> than 0x4n. This error is then nullified by the call to

> arm_to_core_mmu_idx() which masks out the high part, but

> we're about to factor out the code that calculates the

> ARMMMUIdx values so it can be used without passing it through

> arm_to_core_mmu_idx(), so fix this bug first.

> 

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

>  target/arm/cpu.h | 12 +++++++-----

>  1 file changed, 7 insertions(+), 5 deletions(-)


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



r~
diff mbox series

Patch

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 441e584..70c1f85 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2335,14 +2335,16 @@  static inline int cpu_mmu_index(CPUARMState *env, bool ifetch)
     int el = arm_current_el(env);
 
     if (arm_feature(env, ARM_FEATURE_M)) {
-        ARMMMUIdx mmu_idx = el == 0 ? ARMMMUIdx_MUser : ARMMMUIdx_MPriv;
+        ARMMMUIdx mmu_idx;
 
-        if (armv7m_nvic_neg_prio_requested(env->nvic, env->v7m.secure)) {
-            mmu_idx = ARMMMUIdx_MNegPri;
+        if (el == 0) {
+            mmu_idx = env->v7m.secure ? ARMMMUIdx_MSUser : ARMMMUIdx_MUser;
+        } else {
+            mmu_idx = env->v7m.secure ? ARMMMUIdx_MSPriv : ARMMMUIdx_MPriv;
         }
 
-        if (env->v7m.secure) {
-            mmu_idx += ARMMMUIdx_MSUser;
+        if (armv7m_nvic_neg_prio_requested(env->nvic, env->v7m.secure)) {
+            mmu_idx = env->v7m.secure ? ARMMMUIdx_MSNegPri : ARMMMUIdx_MNegPri;
         }
 
         return arm_to_core_mmu_idx(mmu_idx);