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

[Edgar E. Iglesias]

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 <address@hidden>


> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  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
>