Re: [Qemu-devel] [Qemu-arm] [PATCH 1/2] target/arm: Hyp mode R14 is shared with User and System

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Fri, Nov 09, 2018 at 06:15:20PM +0000, Peter Maydell wrote:
> On 9 November 2018 at 17:35, Peter Maydell <peter.maydell@linaro.org> wrote:
> > Hyp mode is an exception to the general rule that each AArch32
> > mode has its own r13, r14 and SPSR -- it has a banked r13 and
> > SPSR but shares its r14 with User and System mode. We were
> > incorrectly implementing it as banked, which meant that on
> > entry to Hyp mode r14 was 0 rather than the USR/SYS r14.
> >
> > We provide a new function r14_bank_number() which is like
> > the existing bank_number() but provides the index into
> > env->banked_r14[]; bank_number() provides the index to use
> > for env->banked_r13[] and env->banked_cpsr[].
> >
> > All the points in the code that were using bank_number()
> > to index into env->banked_r14[] are updated for consintency:
> >  * switch_mode() -- this is the only place where we fix
> >    an actual bug
> >  * aarch64_sync_32_to_64() and aarch64_sync_64_to_32():
> >    no behavioural change as we already special-cased Hyp R14
> >  * kvm32.c: no behavioural change since the guest can't ever
> >    be in Hyp mode, but conceptually the right thing to do
> >  * msr_banked()/mrs_banked(): we can never get to the case
> >    that accesses banked_r14[] with tgtmode == ARM_CPU_MODE_HYP,
> >    so no behavioural change
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  target/arm/internals.h | 16 ++++++++++++++++
> >  target/arm/helper.c    | 29 +++++++++++++++--------------
> >  target/arm/kvm32.c     |  4 ++--
> >  target/arm/op_helper.c |  2 +-
> >  4 files changed, 34 insertions(+), 17 deletions(-)
> 
> Rats, this bit accidentally didn't make it into this patch:
> 
> diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
> index 2b62c53f5b5..eb6fb82fb81 100644
> --- a/target/arm/op_helper.c
> +++ b/target/arm/op_helper.c
> @@ -725,7 +725,7 @@ uint32_t HELPER(mrs_banked)(CPUARMState *env,
> uint32_t tgtmode, uint32_t regno)
>      case 13:
>          return env->banked_r13[bank_number(tgtmode)];
>      case 14:
> -        return env->banked_r14[bank_number(tgtmode)];
> +        return env->banked_r14[r14_bank_number(tgtmode)];
>      case 8 ... 12:
>          switch (tgtmode) {
>          case ARM_CPU_MODE_USR:
> 
> 
> (it's one of the "no behavioural change" bits).
> 


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