|
From: | Marc Zyngier |
Subject: | Re: [PATCH 3/3] target/arm: Handle trapping to EL2 of AArch32 VMRS instructions |
Date: | Thu, 28 Nov 2019 17:49:55 +0000 |
User-agent: | Roundcube Webmail/0.7.2 |
Hi Peter, Thanks for having a look at this. On 2019-11-28 16:43, Peter Maydell wrote:
On Thu, 28 Nov 2019 at 16:17, Marc Zyngier <address@hidden> 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.cindex 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);Since the syndrome value depends only on these two things, you might as well generate the full syndrome value at translate time rather than doing it at runtime; then you only need to pass one thing through to the helper rather than two.
OK. This means that the register check in check_hcr_el2_trap will need to extract the register value from the syndrome. Not a big deal, but maybe slightly less readable.
+ gen_helper_check_hcr_el2_trap(cpu_env, tcg_rt, tcg_reg);This helper call is potentially going to throw an exception at runtime. QEMU's JIT doesn't write back all the state of the CPU to the CPU state structure fields for helper calls, so to avoid losing non-written-back state there are two possible approaches: (1) manually write back the state before the call; for aarch32 this looks like gen_set_condexec(s); gen_set_pc_im(s, s->pc_curr); (you can see this done before we call the access_check_cp_reg() helper, for instance) (2) in the helper function, instead of raise_exception(), call raise_exception_ra(..., GETPC()) This says "when we take the exception, also re-sync the CPU state by looking at the host PC value in the JITted code (ie the address of the callsite of the helper) and looking through a table for this translation block that cross-references the host PC against the guest PC and condexec values for that point in execution". Option 1 is better if the expectation is that the trap will be taken always, often or usually; option 2 is what we use if the trap is unlikely (it's how we handle exceptions on guest load/store insns, which are the main reason we have the mechanism at all). Since it's unlikely that guest code will be doing ID register accesses in hot codepaths, I'd go with option 1, mostly just for consistency with how we do coprocessor register access-check function calls.
Ah, very interesting stuff. There is a lot of "magic" happening in QEMU, and I wondered about the emulated state at some point, until I forgot about it! On a vaguely tangential subject, how are conditional instructions JIT-ed? I could perfectly imagine a conditional VMRS instruction, but none of the code I looked at seem to care about it. Or is that done before the access itself is actually emitted?
+ /* 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; + } + + switch (reg) { + case ARM_VFP_MVFR0: + case ARM_VFP_MVFR1: + case ARM_VFP_MVFR2: + if (!(arm_hcr_el2_eff(env) & HCR_TID3)) { + return; + } + break; + case ARM_VFP_FPSID: + if (!(arm_hcr_el2_eff(env) & HCR_TID0)) { + return; + } + break; + default: + /* Shouldn't be here... */ + return;We usually write 'impossible' default cases as: g_assert_not_reached();
Noted, thanks. I'll wait a bit for additional reviews (if any), and then repost the series with these fixes in. Thanks, M. -- Jazz is not dead. It just smells funny...
[Prev in Thread] | Current Thread | [Next in Thread] |