| Message ID | 20180814124254.5229-9-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Some pieces of support for 32-bit Hyp mode | expand |
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 >
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 --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;
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(-)