[02/11] target/arm: Add arm_mmu_idx_is_stage1

Message ID 20191203225333.17055-3-richard.henderson@linaro.org
State New
Headers show
Series
  • target/arm: Implement ARMv8.1-PAN + ARMv8.2-ATS1E1
Related show

Commit Message

Richard Henderson Dec. 3, 2019, 10:53 p.m.
Use a common predicate for querying stage1-ness.

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

---
 target/arm/internals.h | 11 +++++++++++
 target/arm/helper.c    |  8 +++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

-- 
2.17.1

Comments

Philippe Mathieu-Daudé Dec. 4, 2019, 3:35 p.m. | #1
On 12/3/19 11:53 PM, Richard Henderson wrote:
> Use a common predicate for querying stage1-ness.

> 

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


Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>


> ---

>   target/arm/internals.h | 11 +++++++++++

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

>   2 files changed, 14 insertions(+), 5 deletions(-)

> 

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

> index 49dac2a677..850f204f14 100644

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

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

> @@ -1034,6 +1034,17 @@ static inline ARMMMUIdx arm_stage1_mmu_idx(CPUARMState *env)

>   ARMMMUIdx arm_stage1_mmu_idx(CPUARMState *env);

>   #endif

>   

> +static inline bool arm_mmu_idx_is_stage1(ARMMMUIdx mmu_idx)

> +{

> +    switch (mmu_idx) {

> +    case ARMMMUIdx_Stage1_E0:

> +    case ARMMMUIdx_Stage1_E1:

> +        return true;

> +    default:

> +        return false;

> +    }

> +}

> +

>   /*

>    * Parameters of a given virtual address, as extracted from the

>    * translation control register (TCR) for a given regime.

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

> index f3785d5ad6..fdb86ea427 100644

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

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

> @@ -3212,8 +3212,7 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,

>           bool take_exc = false;

>   

>           if (fi.s1ptw && current_el == 1 && !arm_is_secure(env)

> -            && (mmu_idx == ARMMMUIdx_Stage1_E1

> -                || mmu_idx == ARMMMUIdx_Stage1_E0)) {

> +            && arm_mmu_idx_is_stage1(mmu_idx)) {

>               /*

>                * Synchronous stage 2 fault on an access made as part of the

>                * translation table walk for AT S1E0* or AT S1E1* insn

> @@ -9159,8 +9158,7 @@ static inline bool regime_translation_disabled(CPUARMState *env,

>           }

>       }

>   

> -    if ((env->cp15.hcr_el2 & HCR_DC) &&

> -        (mmu_idx == ARMMMUIdx_Stage1_E0 || mmu_idx == ARMMMUIdx_Stage1_E1)) {

> +    if ((env->cp15.hcr_el2 & HCR_DC) && arm_mmu_idx_is_stage1(mmu_idx)) {

>           /* HCR.DC means SCTLR_EL1.M behaves as 0 */

>           return true;

>       }

> @@ -9469,7 +9467,7 @@ static hwaddr S1_ptw_translate(CPUARMState *env, ARMMMUIdx mmu_idx,

>                                  hwaddr addr, MemTxAttrs txattrs,

>                                  ARMMMUFaultInfo *fi)

>   {

> -    if ((mmu_idx == ARMMMUIdx_Stage1_E0 || mmu_idx == ARMMMUIdx_Stage1_E1) &&

> +    if (arm_mmu_idx_is_stage1(mmu_idx) &&

>           !regime_translation_disabled(env, ARMMMUIdx_Stage2)) {

>           target_ulong s2size;

>           hwaddr s2pa;

>
Peter Maydell Dec. 6, 2019, 7 p.m. | #2
On Tue, 3 Dec 2019 at 22:53, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> Use a common predicate for querying stage1-ness.

>

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

> ---

>  target/arm/internals.h | 11 +++++++++++

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

>  2 files changed, 14 insertions(+), 5 deletions(-)

>

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

> index 49dac2a677..850f204f14 100644

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

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

> @@ -1034,6 +1034,17 @@ static inline ARMMMUIdx arm_stage1_mmu_idx(CPUARMState *env)

>  ARMMMUIdx arm_stage1_mmu_idx(CPUARMState *env);

>  #endif

>

> +static inline bool arm_mmu_idx_is_stage1(ARMMMUIdx mmu_idx)

> +{

> +    switch (mmu_idx) {

> +    case ARMMMUIdx_Stage1_E0:

> +    case ARMMMUIdx_Stage1_E1:

> +        return true;

> +    default:

> +        return false;

> +    }

> +}


This definition of 'stage 1' doesn't match the architecture's,
which has a lot more than 2 things that are stage1; eg whatever
your renaming is calling S1E2, S1E3, etc are all stage 1.
(That's why those names have 'S1' in them: they're stage 1
translation stages.)

thanks
-- PMM

Patch

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 49dac2a677..850f204f14 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1034,6 +1034,17 @@  static inline ARMMMUIdx arm_stage1_mmu_idx(CPUARMState *env)
 ARMMMUIdx arm_stage1_mmu_idx(CPUARMState *env);
 #endif
 
+static inline bool arm_mmu_idx_is_stage1(ARMMMUIdx mmu_idx)
+{
+    switch (mmu_idx) {
+    case ARMMMUIdx_Stage1_E0:
+    case ARMMMUIdx_Stage1_E1:
+        return true;
+    default:
+        return false;
+    }
+}
+
 /*
  * Parameters of a given virtual address, as extracted from the
  * translation control register (TCR) for a given regime.
diff --git a/target/arm/helper.c b/target/arm/helper.c
index f3785d5ad6..fdb86ea427 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3212,8 +3212,7 @@  static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
         bool take_exc = false;
 
         if (fi.s1ptw && current_el == 1 && !arm_is_secure(env)
-            && (mmu_idx == ARMMMUIdx_Stage1_E1
-                || mmu_idx == ARMMMUIdx_Stage1_E0)) {
+            && arm_mmu_idx_is_stage1(mmu_idx)) {
             /*
              * Synchronous stage 2 fault on an access made as part of the
              * translation table walk for AT S1E0* or AT S1E1* insn
@@ -9159,8 +9158,7 @@  static inline bool regime_translation_disabled(CPUARMState *env,
         }
     }
 
-    if ((env->cp15.hcr_el2 & HCR_DC) &&
-        (mmu_idx == ARMMMUIdx_Stage1_E0 || mmu_idx == ARMMMUIdx_Stage1_E1)) {
+    if ((env->cp15.hcr_el2 & HCR_DC) && arm_mmu_idx_is_stage1(mmu_idx)) {
         /* HCR.DC means SCTLR_EL1.M behaves as 0 */
         return true;
     }
@@ -9469,7 +9467,7 @@  static hwaddr S1_ptw_translate(CPUARMState *env, ARMMMUIdx mmu_idx,
                                hwaddr addr, MemTxAttrs txattrs,
                                ARMMMUFaultInfo *fi)
 {
-    if ((mmu_idx == ARMMMUIdx_Stage1_E0 || mmu_idx == ARMMMUIdx_Stage1_E1) &&
+    if (arm_mmu_idx_is_stage1(mmu_idx) &&
         !regime_translation_disabled(env, ARMMMUIdx_Stage2)) {
         target_ulong s2size;
         hwaddr s2pa;