Re: [PATCH 3/3] target/arm: Handle trapping to EL2 of AArch32 VMRS instructions

[Edgar E. Iglesias]

On Fri, Nov 29, 2019 at 09:24:37AM +0000, Marc Zyngier wrote:
> On 2019-11-29 08:28, Edgar E. Iglesias wrote:
> > On Thu, Nov 28, 2019 at 04:17:18PM +0000, Marc Zyngier wrote:
> > > HCR_EL2.TID3 requires that AArch32 reads of MVFR[012] are trapped to
> > > EL2, and that HCR_EL2.TID0 does the same for reads of FPSID.
> > > In order to handle this, introduce a new TCG helper function that
> > > checks for these control bits before executing the VMRC instruction.
> > > 
> > > Tested with a hacked-up version of KVM/arm64 that sets the control
> > > bits for 32bit guests.
> > > 
> > > Signed-off-by: Marc Zyngier <address@hidden>
> > > ---
> > >  target/arm/helper-a64.h        |  2 ++
> > >  target/arm/internals.h         |  8 ++++++++
> > >  target/arm/translate-vfp.inc.c | 12 +++++++++---
> > >  target/arm/vfp_helper.c        | 27 +++++++++++++++++++++++++++
> > >  4 files changed, 46 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/target/arm/helper-a64.h b/target/arm/helper-a64.h
> > > index a915c1247f..311ced44e6 100644
> > > --- a/target/arm/helper-a64.h
> > > +++ b/target/arm/helper-a64.h
> > > @@ -102,3 +102,5 @@ DEF_HELPER_FLAGS_3(autda, TCG_CALL_NO_WG, i64,
> > > env, i64, i64)
> > >  DEF_HELPER_FLAGS_3(autdb, TCG_CALL_NO_WG, i64, env, i64, i64)
> > >  DEF_HELPER_FLAGS_2(xpaci, TCG_CALL_NO_RWG_SE, i64, env, i64)
> > >  DEF_HELPER_FLAGS_2(xpacd, TCG_CALL_NO_RWG_SE, i64, env, i64)
> > > +
> > > +DEF_HELPER_3(check_hcr_el2_trap, void, env, int, int)
> > > diff --git a/target/arm/internals.h b/target/arm/internals.h
> > > index f5313dd3d4..5a55e960de 100644
> > > --- a/target/arm/internals.h
> > > +++ b/target/arm/internals.h
> > > @@ -430,6 +430,14 @@ static inline uint32_t syn_simd_access_trap(int
> > > cv, int cond, bool is_16bit)
> > >          | (cv << 24) | (cond << 20) | (1 << 5);
> > >  }
> > > 
> > > +static inline uint32_t syn_vmrs_trap(int rt, int reg)
> > > +{
> > > +    return (EC_FPIDTRAP << ARM_EL_EC_SHIFT)
> > > +        | ARM_EL_IL
> > > +        | (1 << 24) | (0xe << 20) | (7 << 14)
> > > +        | (reg << 10) | (rt << 5) | 1;
> > > +}
> > > +
> > >  static inline uint32_t syn_sve_access_trap(void)
> > >  {
> > >      return EC_SVEACCESSTRAP << ARM_EL_EC_SHIFT;
> > > diff --git a/target/arm/translate-vfp.inc.c
> > > b/target/arm/translate-vfp.inc.c
> > > index 85c5ef897b..4c435b6c35 100644
> > > --- a/target/arm/translate-vfp.inc.c
> > > +++ b/target/arm/translate-vfp.inc.c
> > > @@ -759,15 +759,21 @@ static bool trans_VMSR_VMRS(DisasContext *s,
> > > arg_VMSR_VMRS *a)
> > >      }
> > > 
> > >      if (a->l) {
> > > +        TCGv_i32 tcg_rt, tcg_reg;
> > > +
> > >          /* VMRS, move VFP special register to gp register */
> > >          switch (a->reg) {
> > > +        case ARM_VFP_MVFR0:
> > > +        case ARM_VFP_MVFR1:
> > > +        case ARM_VFP_MVFR2:
> > >          case ARM_VFP_FPSID:
> > > +            tcg_rt = tcg_const_i32(a->rt);
> > > +            tcg_reg = tcg_const_i32(a->reg);
> > > +            gen_helper_check_hcr_el2_trap(cpu_env, tcg_rt,
> > > tcg_reg);
> > > +            /* fall through */
> > >          case ARM_VFP_FPEXC:
> > >          case ARM_VFP_FPINST:
> > >          case ARM_VFP_FPINST2:
> > > -        case ARM_VFP_MVFR0:
> > > -        case ARM_VFP_MVFR1:
> > > -        case ARM_VFP_MVFR2:
> > >              tmp = load_cpu_field(vfp.xregs[a->reg]);
> > >              break;
> > >          case ARM_VFP_FPSCR:
> > > diff --git a/target/arm/vfp_helper.c b/target/arm/vfp_helper.c
> > > index 9710ef1c3e..44e538e51c 100644
> > > --- a/target/arm/vfp_helper.c
> > > +++ b/target/arm/vfp_helper.c
> > > @@ -1322,4 +1322,31 @@ float64 HELPER(frint64_d)(float64 f, void
> > > *fpst)
> > >      return frint_d(f, fpst, 64);
> > >  }
> > > 
> > > +void HELPER(check_hcr_el2_trap)(CPUARMState *env, int rt, int reg)
> > > +{
> > > +    if (arm_current_el(env) != 1) {
> > > +        return;
> > > +    }
> > 
> > I think we could move the EL1 check to translation time, couldn't we?
> 
> I think that depends whether the translated code is tagged by EL
> or not, or if an exception entry (and exception return) invalidates
> the JIT-ed code (and it this case it would have to be CPU-private).
> 
> I can perfectly imagine the same piece of code being executed both
> at EL0 and EL1, and this would fail if it was executed using the
> same JIT-ed code.
> 
> So if QEMU gives us the above as a guarantee, we're good. Otherwise,
> we need this check. How can I find out?

Hi Marc,

IIRC, the current EL was always known at translation time but I've
not been tracking recent changes.

There are several ways to check this, one way is to look in
cpu_get_tb_cpu_state() and see if the state needed to extract the
the EL goes into the TB-flags.

Another way is to look in arm_tr_init_disas_context() and see what gets
extracted from the tb_flags just before translating a block.

>From arm_tr_init_disas_context():
    dc->current_el = arm_mmu_idx_to_el(dc->mmu_idx);

Unless I'm missing something it's still there, so I think this could be
done at translation time. Peter?

Cheers,
Edgar