diff mbox series

[08/10] target/arm: Permit accesses to ELR_Hyp from Hyp mode via MSR/MRS (banked)

Message ID 20180814124254.5229-9-peter.maydell@linaro.org
State Superseded
Headers show
Series target/arm: Some pieces of support for 32-bit Hyp mode | expand

Commit Message

Peter Maydell Aug. 14, 2018, 12:42 p.m. UTC 
The MSR (banked) and MRS (banked) instructions allow accesses to ELR_Hyp
from either Monitor or Hyp mode. Our translate time check
was overly strict and only permitted access from Monitor mode.

The runtime check wo do in msr_mrs_banked_exc_checks() had the
correct code in it, but never got there because of the earlier
"currmode == tgtmode" check. Special case ELR_Hyp.

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

---
 target/arm/op_helper.c | 22 +++++++++++-----------
 target/arm/translate.c | 10 +++++++---
 2 files changed, 18 insertions(+), 14 deletions(-)

-- 
2.18.0

Comments

Edgar E. Iglesias Aug. 14, 2018, 3:07 p.m. UTC | #1 
On Tue, Aug 14, 2018 at 01:42:52PM +0100, Peter Maydell wrote:
> The MSR (banked) and MRS (banked) instructions allow accesses to ELR_Hyp

> from either Monitor or Hyp mode. Our translate time check

> was overly strict and only permitted access from Monitor mode.

> 

> The runtime check wo do in msr_mrs_banked_exc_checks() had the

> correct code in it, but never got there because of the earlier

> "currmode == tgtmode" check. Special case ELR_Hyp.


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



> 

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

> ---

>  target/arm/op_helper.c | 22 +++++++++++-----------

>  target/arm/translate.c | 10 +++++++---

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

> 

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

> index d550978b5b9..952b8d122b7 100644

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

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

> @@ -611,6 +611,14 @@ static void msr_mrs_banked_exc_checks(CPUARMState *env, uint32_t tgtmode,

>       */

>      int curmode = env->uncached_cpsr & CPSR_M;

>  

> +    if (regno == 17) {

> +        /* ELR_Hyp: a special case because access from tgtmode is OK */

> +        if (curmode != ARM_CPU_MODE_HYP && curmode != ARM_CPU_MODE_MON) {

> +            goto undef;

> +        }

> +        return;

> +    }

> +

>      if (curmode == tgtmode) {

>          goto undef;

>      }

> @@ -638,17 +646,9 @@ static void msr_mrs_banked_exc_checks(CPUARMState *env, uint32_t tgtmode,

>      }

>  

>      if (tgtmode == ARM_CPU_MODE_HYP) {

> -        switch (regno) {

> -        case 17: /* ELR_Hyp */

> -            if (curmode != ARM_CPU_MODE_HYP && curmode != ARM_CPU_MODE_MON) {

> -                goto undef;

> -            }

> -            break;

> -        default:

> -            if (curmode != ARM_CPU_MODE_MON) {

> -                goto undef;

> -            }

> -            break;

> +        /* SPSR_Hyp, r13_hyp: accessible from Monitor mode only */

> +        if (curmode != ARM_CPU_MODE_MON) {

> +            goto undef;

>          }

>      }

>  

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

> index f845da7c638..3f5751d4826 100644

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

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

> @@ -4506,10 +4506,14 @@ static bool msr_banked_access_decode(DisasContext *s, int r, int sysm, int rn,

>          }

>          break;

>      case ARM_CPU_MODE_HYP:

> -        /* Note that we can forbid accesses from EL2 here because they

> -         * must be from Hyp mode itself

> +        /*

> +         * SPSR_hyp and r13_hyp can only be accessed from Monitor mode

> +         * (and so we can forbid accesses from EL2 or below). elr_hyp

> +         * can be accessed also from Hyp mode, so forbid accesses from

> +         * EL0 or EL1.

>           */

> -        if (!arm_dc_feature(s, ARM_FEATURE_EL2) || s->current_el < 3) {

> +        if (!arm_dc_feature(s, ARM_FEATURE_EL2) || s->current_el < 2 ||

> +            (s->current_el < 3 && *regno != 17)) {

>              goto undef;

>          }

>          break;

> -- 

> 2.18.0

>
Luc Michel Aug. 16, 2018, 7:58 a.m. UTC | #2 
On 8/14/18 2:42 PM, Peter Maydell wrote:
> The MSR (banked) and MRS (banked) instructions allow accesses to ELR_Hyp

> from either Monitor or Hyp mode. Our translate time check

> was overly strict and only permitted access from Monitor mode.

> 

> The runtime check wo do in msr_mrs_banked_exc_checks() had the

"we"
> correct code in it, but never got there because of the earlier

> "currmode == tgtmode" check. Special case ELR_Hyp.

> 

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


Reviewed-By: Luc Michel <luc.michel@greensocs.com>


> ---

>  target/arm/op_helper.c | 22 +++++++++++-----------

>  target/arm/translate.c | 10 +++++++---

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

> 

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

> index d550978b5b9..952b8d122b7 100644

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

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

> @@ -611,6 +611,14 @@ static void msr_mrs_banked_exc_checks(CPUARMState *env, uint32_t tgtmode,

>       */

>      int curmode = env->uncached_cpsr & CPSR_M;

>  

> +    if (regno == 17) {

> +        /* ELR_Hyp: a special case because access from tgtmode is OK */

> +        if (curmode != ARM_CPU_MODE_HYP && curmode != ARM_CPU_MODE_MON) {

> +            goto undef;

> +        }

> +        return;

> +    }

> +

>      if (curmode == tgtmode) {

>          goto undef;

>      }

> @@ -638,17 +646,9 @@ static void msr_mrs_banked_exc_checks(CPUARMState *env, uint32_t tgtmode,

>      }

>  

>      if (tgtmode == ARM_CPU_MODE_HYP) {

> -        switch (regno) {

> -        case 17: /* ELR_Hyp */

> -            if (curmode != ARM_CPU_MODE_HYP && curmode != ARM_CPU_MODE_MON) {

> -                goto undef;

> -            }

> -            break;

> -        default:

> -            if (curmode != ARM_CPU_MODE_MON) {

> -                goto undef;

> -            }

> -            break;

> +        /* SPSR_Hyp, r13_hyp: accessible from Monitor mode only */

> +        if (curmode != ARM_CPU_MODE_MON) {

> +            goto undef;

>          }

>      }

>  

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

> index f845da7c638..3f5751d4826 100644

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

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

> @@ -4506,10 +4506,14 @@ static bool msr_banked_access_decode(DisasContext *s, int r, int sysm, int rn,

>          }

>          break;

>      case ARM_CPU_MODE_HYP:

> -        /* Note that we can forbid accesses from EL2 here because they

> -         * must be from Hyp mode itself

> +        /*

> +         * SPSR_hyp and r13_hyp can only be accessed from Monitor mode

> +         * (and so we can forbid accesses from EL2 or below). elr_hyp

> +         * can be accessed also from Hyp mode, so forbid accesses from

> +         * EL0 or EL1.

>           */

> -        if (!arm_dc_feature(s, ARM_FEATURE_EL2) || s->current_el < 3) {

> +        if (!arm_dc_feature(s, ARM_FEATURE_EL2) || s->current_el < 2 ||

> +            (s->current_el < 3 && *regno != 17)) {

>              goto undef;

>          }

>          break;

>
diff mbox series

Patch

diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index d550978b5b9..952b8d122b7 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -611,6 +611,14 @@  static void msr_mrs_banked_exc_checks(CPUARMState *env, uint32_t tgtmode,
      */
     int curmode = env->uncached_cpsr & CPSR_M;
 
+    if (regno == 17) {
+        /* ELR_Hyp: a special case because access from tgtmode is OK */
+        if (curmode != ARM_CPU_MODE_HYP && curmode != ARM_CPU_MODE_MON) {
+            goto undef;
+        }
+        return;
+    }
+
     if (curmode == tgtmode) {
         goto undef;
     }
@@ -638,17 +646,9 @@  static void msr_mrs_banked_exc_checks(CPUARMState *env, uint32_t tgtmode,
     }
 
     if (tgtmode == ARM_CPU_MODE_HYP) {
-        switch (regno) {
-        case 17: /* ELR_Hyp */
-            if (curmode != ARM_CPU_MODE_HYP && curmode != ARM_CPU_MODE_MON) {
-                goto undef;
-            }
-            break;
-        default:
-            if (curmode != ARM_CPU_MODE_MON) {
-                goto undef;
-            }
-            break;
+        /* SPSR_Hyp, r13_hyp: accessible from Monitor mode only */
+        if (curmode != ARM_CPU_MODE_MON) {
+            goto undef;
         }
     }
 
diff --git a/target/arm/translate.c b/target/arm/translate.c
index f845da7c638..3f5751d4826 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -4506,10 +4506,14 @@  static bool msr_banked_access_decode(DisasContext *s, int r, int sysm, int rn,
         }
         break;
     case ARM_CPU_MODE_HYP:
-        /* Note that we can forbid accesses from EL2 here because they
-         * must be from Hyp mode itself
+        /*
+         * SPSR_hyp and r13_hyp can only be accessed from Monitor mode
+         * (and so we can forbid accesses from EL2 or below). elr_hyp
+         * can be accessed also from Hyp mode, so forbid accesses from
+         * EL0 or EL1.
          */
-        if (!arm_dc_feature(s, ARM_FEATURE_EL2) || s->current_el < 3) {
+        if (!arm_dc_feature(s, ARM_FEATURE_EL2) || s->current_el < 2 ||
+            (s->current_el < 3 && *regno != 17)) {
             goto undef;
         }
         break;