diff mbox series

[03/15] target/arm: Consolidate PMSA handling in get_phys_addr()

Message ID 1501692241-23310-4-git-send-email-peter.maydell@linaro.org
State Superseded
Headers show
Series v7M: cleanups and bugfixes prior to v8M | expand

Commit Message

Peter Maydell Aug. 2, 2017, 4:43 p.m. UTC 
Currently get_phys_addr() has PMSAv7 handling before the
"is translation disabled?" check, and then PMSAv5 after it.
Tidy this up by making the PMSAv5 code handle the "MPU disabled"
case itself, so that we have all the PMSA code in one place.
This will make adding the PMSAv8 code slightly cleaner, and
also means that pre-v7 PMSA cores benefit from the MPU lookup
logging that the PMSAv7 codepath had.

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

---
 target/arm/helper.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

-- 
2.7.4

Comments

Edgar E. Iglesias Aug. 2, 2017, 5:40 p.m. UTC | #1 
On Wed, Aug 02, 2017 at 05:43:49PM +0100, Peter Maydell wrote:
> Currently get_phys_addr() has PMSAv7 handling before the

> "is translation disabled?" check, and then PMSAv5 after it.

> Tidy this up by making the PMSAv5 code handle the "MPU disabled"

> case itself, so that we have all the PMSA code in one place.

> This will make adding the PMSAv8 code slightly cleaner, and

> also means that pre-v7 PMSA cores benefit from the MPU lookup

> logging that the PMSAv7 codepath had.

> 

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


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



> ---

>  target/arm/helper.c | 38 ++++++++++++++++++++++----------------

>  1 file changed, 22 insertions(+), 16 deletions(-)

> 

> diff --git a/target/arm/helper.c b/target/arm/helper.c

> index b78d277..fd83a21 100644

> --- a/target/arm/helper.c

> +++ b/target/arm/helper.c

> @@ -8423,6 +8423,13 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,

>      uint32_t base;

>      bool is_user = regime_is_user(env, mmu_idx);

>  

> +    if (regime_translation_disabled(env, mmu_idx)) {

> +        /* MPU disabled.  */

> +        *phys_ptr = address;

> +        *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;

> +        return false;

> +    }

> +

>      *phys_ptr = address;

>      for (n = 7; n >= 0; n--) {

>          base = env->cp15.c6_region[n];

> @@ -8572,16 +8579,20 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,

>          }

>      }

>  

> -    /* pmsav7 has special handling for when MPU is disabled so call it before

> -     * the common MMU/MPU disabled check below.

> -     */

> -    if (arm_feature(env, ARM_FEATURE_PMSA) &&

> -        arm_feature(env, ARM_FEATURE_V7)) {

> +    if (arm_feature(env, ARM_FEATURE_PMSA)) {

>          bool ret;

>          *page_size = TARGET_PAGE_SIZE;

> -        ret = get_phys_addr_pmsav7(env, address, access_type, mmu_idx,

> -                                   phys_ptr, prot, fsr);

> -        qemu_log_mask(CPU_LOG_MMU, "PMSAv7 MPU lookup for %s at 0x%08" PRIx32

> +

> +        if (arm_feature(env, ARM_FEATURE_V7)) {

> +            /* PMSAv7 */

> +            ret = get_phys_addr_pmsav7(env, address, access_type, mmu_idx,

> +                                       phys_ptr, prot, fsr);

> +        } else {

> +            /* Pre-v7 MPU */

> +            ret = get_phys_addr_pmsav5(env, address, access_type, mmu_idx,

> +                                       phys_ptr, prot, fsr);

> +        }

> +        qemu_log_mask(CPU_LOG_MMU, "PMSA MPU lookup for %s at 0x%08" PRIx32

>                        " mmu_idx %u -> %s (prot %c%c%c)\n",

>                        access_type == MMU_DATA_LOAD ? "reading" :

>                        (access_type == MMU_DATA_STORE ? "writing" : "execute"),

> @@ -8594,21 +8605,16 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,

>          return ret;

>      }

>  

> +    /* Definitely a real MMU, not an MPU */

> +

>      if (regime_translation_disabled(env, mmu_idx)) {

> -        /* MMU/MPU disabled.  */

> +        /* MMU disabled. */

>          *phys_ptr = address;

>          *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;

>          *page_size = TARGET_PAGE_SIZE;

>          return 0;

>      }

>  

> -    if (arm_feature(env, ARM_FEATURE_PMSA)) {

> -        /* Pre-v7 MPU */

> -        *page_size = TARGET_PAGE_SIZE;

> -        return get_phys_addr_pmsav5(env, address, access_type, mmu_idx,

> -                                    phys_ptr, prot, fsr);

> -    }

> -

>      if (regime_using_lpae_format(env, mmu_idx)) {

>          return get_phys_addr_lpae(env, address, access_type, mmu_idx, phys_ptr,

>                                    attrs, prot, page_size, fsr, fi);

> -- 

> 2.7.4

> 

>
Philippe Mathieu-Daudé Aug. 2, 2017, 9:50 p.m. UTC | #2 
On 08/02/2017 01:43 PM, Peter Maydell wrote:
> Currently get_phys_addr() has PMSAv7 handling before the

> "is translation disabled?" check, and then PMSAv5 after it.

> Tidy this up by making the PMSAv5 code handle the "MPU disabled"

> case itself, so that we have all the PMSA code in one place.

> This will make adding the PMSAv8 code slightly cleaner, and

> also means that pre-v7 PMSA cores benefit from the MPU lookup

> logging that the PMSAv7 codepath had.

> 

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


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


> ---

>   target/arm/helper.c | 38 ++++++++++++++++++++++----------------

>   1 file changed, 22 insertions(+), 16 deletions(-)

> 

> diff --git a/target/arm/helper.c b/target/arm/helper.c

> index b78d277..fd83a21 100644

> --- a/target/arm/helper.c

> +++ b/target/arm/helper.c

> @@ -8423,6 +8423,13 @@ static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,

>       uint32_t base;

>       bool is_user = regime_is_user(env, mmu_idx);

>   

> +    if (regime_translation_disabled(env, mmu_idx)) {

> +        /* MPU disabled.  */

> +        *phys_ptr = address;

> +        *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;

> +        return false;

> +    }

> +

>       *phys_ptr = address;

>       for (n = 7; n >= 0; n--) {

>           base = env->cp15.c6_region[n];

> @@ -8572,16 +8579,20 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,

>           }

>       }

>   

> -    /* pmsav7 has special handling for when MPU is disabled so call it before

> -     * the common MMU/MPU disabled check below.

> -     */

> -    if (arm_feature(env, ARM_FEATURE_PMSA) &&

> -        arm_feature(env, ARM_FEATURE_V7)) {

> +    if (arm_feature(env, ARM_FEATURE_PMSA)) {

>           bool ret;

>           *page_size = TARGET_PAGE_SIZE;

> -        ret = get_phys_addr_pmsav7(env, address, access_type, mmu_idx,

> -                                   phys_ptr, prot, fsr);

> -        qemu_log_mask(CPU_LOG_MMU, "PMSAv7 MPU lookup for %s at 0x%08" PRIx32

> +

> +        if (arm_feature(env, ARM_FEATURE_V7)) {

> +            /* PMSAv7 */

> +            ret = get_phys_addr_pmsav7(env, address, access_type, mmu_idx,

> +                                       phys_ptr, prot, fsr);

> +        } else {

> +            /* Pre-v7 MPU */

> +            ret = get_phys_addr_pmsav5(env, address, access_type, mmu_idx,

> +                                       phys_ptr, prot, fsr);

> +        }

> +        qemu_log_mask(CPU_LOG_MMU, "PMSA MPU lookup for %s at 0x%08" PRIx32

>                         " mmu_idx %u -> %s (prot %c%c%c)\n",

>                         access_type == MMU_DATA_LOAD ? "reading" :

>                         (access_type == MMU_DATA_STORE ? "writing" : "execute"),

> @@ -8594,21 +8605,16 @@ static bool get_phys_addr(CPUARMState *env, target_ulong address,

>           return ret;

>       }

>   

> +    /* Definitely a real MMU, not an MPU */

> +

>       if (regime_translation_disabled(env, mmu_idx)) {

> -        /* MMU/MPU disabled.  */

> +        /* MMU disabled. */

>           *phys_ptr = address;

>           *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;

>           *page_size = TARGET_PAGE_SIZE;

>           return 0;

>       }

>   

> -    if (arm_feature(env, ARM_FEATURE_PMSA)) {

> -        /* Pre-v7 MPU */

> -        *page_size = TARGET_PAGE_SIZE;

> -        return get_phys_addr_pmsav5(env, address, access_type, mmu_idx,

> -                                    phys_ptr, prot, fsr);

> -    }

> -

>       if (regime_using_lpae_format(env, mmu_idx)) {

>           return get_phys_addr_lpae(env, address, access_type, mmu_idx, phys_ptr,

>                                     attrs, prot, page_size, fsr, fi);

>
Richard Henderson Aug. 3, 2017, 8:33 p.m. UTC | #3 
On 08/02/2017 09:43 AM, Peter Maydell wrote:
> Currently get_phys_addr() has PMSAv7 handling before the

> "is translation disabled?" check, and then PMSAv5 after it.

> Tidy this up by making the PMSAv5 code handle the "MPU disabled"

> case itself, so that we have all the PMSA code in one place.

> This will make adding the PMSAv8 code slightly cleaner, and

> also means that pre-v7 PMSA cores benefit from the MPU lookup

> logging that the PMSAv7 codepath had.

> 

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

> ---

>  target/arm/helper.c | 38 ++++++++++++++++++++++----------------

>  1 file changed, 22 insertions(+), 16 deletions(-)



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



r~
diff mbox series

Patch

diff --git a/target/arm/helper.c b/target/arm/helper.c
index b78d277..fd83a21 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8423,6 +8423,13 @@  static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
     uint32_t base;
     bool is_user = regime_is_user(env, mmu_idx);
 
+    if (regime_translation_disabled(env, mmu_idx)) {
+        /* MPU disabled.  */
+        *phys_ptr = address;
+        *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
+        return false;
+    }
+
     *phys_ptr = address;
     for (n = 7; n >= 0; n--) {
         base = env->cp15.c6_region[n];
@@ -8572,16 +8579,20 @@  static bool get_phys_addr(CPUARMState *env, target_ulong address,
         }
     }
 
-    /* pmsav7 has special handling for when MPU is disabled so call it before
-     * the common MMU/MPU disabled check below.
-     */
-    if (arm_feature(env, ARM_FEATURE_PMSA) &&
-        arm_feature(env, ARM_FEATURE_V7)) {
+    if (arm_feature(env, ARM_FEATURE_PMSA)) {
         bool ret;
         *page_size = TARGET_PAGE_SIZE;
-        ret = get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
-                                   phys_ptr, prot, fsr);
-        qemu_log_mask(CPU_LOG_MMU, "PMSAv7 MPU lookup for %s at 0x%08" PRIx32
+
+        if (arm_feature(env, ARM_FEATURE_V7)) {
+            /* PMSAv7 */
+            ret = get_phys_addr_pmsav7(env, address, access_type, mmu_idx,
+                                       phys_ptr, prot, fsr);
+        } else {
+            /* Pre-v7 MPU */
+            ret = get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
+                                       phys_ptr, prot, fsr);
+        }
+        qemu_log_mask(CPU_LOG_MMU, "PMSA MPU lookup for %s at 0x%08" PRIx32
                       " mmu_idx %u -> %s (prot %c%c%c)\n",
                       access_type == MMU_DATA_LOAD ? "reading" :
                       (access_type == MMU_DATA_STORE ? "writing" : "execute"),
@@ -8594,21 +8605,16 @@  static bool get_phys_addr(CPUARMState *env, target_ulong address,
         return ret;
     }
 
+    /* Definitely a real MMU, not an MPU */
+
     if (regime_translation_disabled(env, mmu_idx)) {
-        /* MMU/MPU disabled.  */
+        /* MMU disabled. */
         *phys_ptr = address;
         *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
         *page_size = TARGET_PAGE_SIZE;
         return 0;
     }
 
-    if (arm_feature(env, ARM_FEATURE_PMSA)) {
-        /* Pre-v7 MPU */
-        *page_size = TARGET_PAGE_SIZE;
-        return get_phys_addr_pmsav5(env, address, access_type, mmu_idx,
-                                    phys_ptr, prot, fsr);
-    }
-
     if (regime_using_lpae_format(env, mmu_idx)) {
         return get_phys_addr_lpae(env, address, access_type, mmu_idx, phys_ptr,
                                   attrs, prot, page_size, fsr, fi);