[PATCH 0/3] target/arm: More HCR_EL2.TIDx fixes

[PATCH 0/3] target/arm: More HCR_EL2.TIDx fixes

[Marc Zyngier]

I started looking the rest of the missing TIDx handling,
and this resulted in the following patches.

There is still one thing I'm a bit puzzled by though:

HCR_EL2.TID0 mandates trapping of the AArch32 JIDR
register, but I couldn't find a trace of it in the QEMU
code, and trying to read it seems to generate an exception.

It isn't like anyone is going to miss it, but I wonder if
it should be implemented... It could also be that I'm missing
the obvious and that my testing is broken! ;-)

Marc Zyngier (3):
  target/arm: Honor HCR_EL2.TID2 trapping requirements
  target/arm: Honor HCR_EL2.TID1 trapping requirements
  target/arm: Handle trapping to EL2 of AArch32 VMRS instructions

 target/arm/helper-a64.h        |  2 ++
 target/arm/helper.c            | 64 ++++++++++++++++++++++++++++++----
 target/arm/internals.h         |  8 +++++
 target/arm/translate-vfp.inc.c | 12 +++++--
 target/arm/vfp_helper.c        | 27 ++++++++++++++
 5 files changed, 103 insertions(+), 10 deletions(-)

-- 
2.20.1

[PATCH 1/3] target/arm: Honor HCR_EL2.TID2 trapping requirements

[Marc Zyngier]

HCR_EL2.TID2 mandates that access from EL1 to CTR_EL0, CCSIDR_EL1,
CCSIDR2_EL1, CLIDR_EL1, CSSELR_EL1 are trapped to EL2, and QEMU
completely ignores it, making impossible for hypervisors to
virtualize the cache hierarchy.

Do the right thing by trapping to EL2 if HCR_EL2.TID2 is set.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 target/arm/helper.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 0bf8f53d4b..0b6887b100 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1910,6 +1910,17 @@ static void scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
     raw_write(env, ri, value);
 }
 
+static CPAccessResult access_aa64_tid2(CPUARMState *env,
+                                       const ARMCPRegInfo *ri,
+                                       bool isread)
+{
+    if (arm_current_el(env) == 1 && (arm_hcr_el2_eff(env) & HCR_TID2)) {
+        return CP_ACCESS_TRAP_EL2;
+    }
+
+    return CP_ACCESS_OK;
+}
+
 static uint64_t ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
     ARMCPU *cpu = env_archcpu(env);
@@ -2110,10 +2121,14 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .writefn = pmintenclr_write },
     { .name = "CCSIDR", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0,
-      .access = PL1_R, .readfn = ccsidr_read, .type = ARM_CP_NO_RAW },
+      .access = PL1_R,
+      .accessfn = access_aa64_tid2,
+      .readfn = ccsidr_read, .type = ARM_CP_NO_RAW },
     { .name = "CSSELR", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 2, .opc2 = 0,
-      .access = PL1_RW, .writefn = csselr_write, .resetvalue = 0,
+      .access = PL1_RW,
+      .accessfn = access_aa64_tid2,
+      .writefn = csselr_write, .resetvalue = 0,
       .bank_fieldoffsets = { offsetof(CPUARMState, cp15.csselr_s),
                              offsetof(CPUARMState, cp15.csselr_ns) } },
     /* Auxiliary ID register: this actually has an IMPDEF value but for now
@@ -5204,6 +5219,11 @@ static CPAccessResult ctr_el0_access(CPUARMState *env, const ARMCPRegInfo *ri,
     if (arm_current_el(env) == 0 && !(env->cp15.sctlr_el[1] & SCTLR_UCT)) {
         return CP_ACCESS_TRAP;
     }
+
+    if (arm_hcr_el2_eff(env) & HCR_TID2) {
+        return CP_ACCESS_TRAP_EL2;
+    }
+
     return CP_ACCESS_OK;
 }
 
@@ -6184,7 +6204,9 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         ARMCPRegInfo clidr = {
             .name = "CLIDR", .state = ARM_CP_STATE_BOTH,
             .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 1,
-            .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->clidr
+            .access = PL1_R, .type = ARM_CP_CONST,
+            .accessfn = access_aa64_tid2,
+            .resetvalue = cpu->clidr
         };
         define_one_arm_cp_reg(cpu, &clidr);
         define_arm_cp_regs(cpu, v7_cp_reginfo);
-- 
2.20.1

[PATCH 2/3] target/arm: Honor HCR_EL2.TID1 trapping requirements

[Marc Zyngier]

HCR_EL2.TID1 mandates that access from EL1 to REVIDR_EL1, AIDR_EL1
(and their 32bit equivalents) as well as TCMTR, TLBTR are trapped
to EL2. QEMU ignores it, naking it harder for a hypervisor to
virtualize the HW (though to be fair, no known hypervisor actually
cares).

Do the right thing by trapping to EL2 if HCR_EL2.TID1 is set.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 target/arm/helper.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 0b6887b100..9bff769692 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1973,6 +1973,26 @@ static uint64_t isr_read(CPUARMState *env, const ARMCPRegInfo *ri)
     return ret;
 }
 
+static CPAccessResult access_aa64_tid1(CPUARMState *env, const ARMCPRegInfo *ri,
+                                       bool isread)
+{
+    if (arm_hcr_el2_eff(env) & HCR_TID1) {
+        return CP_ACCESS_TRAP_EL2;
+    }
+
+    return CP_ACCESS_OK;
+}
+
+static CPAccessResult access_aa32_tid1(CPUARMState *env, const ARMCPRegInfo *ri,
+                                       bool isread)
+{
+    if (arm_feature(env, ARM_FEATURE_V8)) {
+        return access_aa64_tid1(env, ri, isread);
+    }
+
+    return CP_ACCESS_OK;
+}
+
 static const ARMCPRegInfo v7_cp_reginfo[] = {
     /* the old v6 WFI, UNPREDICTABLE in v7 but we choose to NOP */
     { .name = "NOP", .cp = 15, .crn = 7, .crm = 0, .opc1 = 0, .opc2 = 4,
@@ -2136,7 +2156,9 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
      */
     { .name = "AIDR", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 1, .crn = 0, .crm = 0, .opc2 = 7,
-      .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
+      .access = PL1_R, .type = ARM_CP_CONST,
+      .accessfn = access_aa64_tid1,
+      .resetvalue = 0 },
     /* Auxiliary fault status registers: these also are IMPDEF, and we
      * choose to RAZ/WI for all cores.
      */
@@ -6732,7 +6754,9 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .access = PL1_R, .resetvalue = cpu->midr },
             { .name = "REVIDR_EL1", .state = ARM_CP_STATE_BOTH,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 0, .opc2 = 6,
-              .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->revidr },
+              .access = PL1_R,
+              .accessfn = access_aa64_tid1,
+              .type = ARM_CP_CONST, .resetvalue = cpu->revidr },
             REGINFO_SENTINEL
         };
         ARMCPRegInfo id_cp_reginfo[] = {
@@ -6747,14 +6771,18 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             /* TCMTR and TLBTR exist in v8 but have no 64-bit versions */
             { .name = "TCMTR",
               .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 2,
-              .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
+              .access = PL1_R,
+              .accessfn = access_aa32_tid1,
+              .type = ARM_CP_CONST, .resetvalue = 0 },
             REGINFO_SENTINEL
         };
         /* TLBTR is specific to VMSA */
         ARMCPRegInfo id_tlbtr_reginfo = {
               .name = "TLBTR",
               .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 3,
-              .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0,
+              .access = PL1_R,
+              .accessfn = access_aa32_tid1,
+              .type = ARM_CP_CONST, .resetvalue = 0,
         };
         /* MPUIR is specific to PMSA V6+ */
         ARMCPRegInfo id_mpuir_reginfo = {
-- 
2.20.1

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

[Marc Zyngier]

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 <maz@kernel.org>
---
 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;
+    }
+
+    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;
+    }
+
+    raise_exception(env, EXCP_HYP_TRAP, syn_vmrs_trap(rt, reg), 2);
+}
+
 #endif
-- 
2.20.1

Re: [PATCH 0/3] target/arm: More HCR_EL2.TIDx fixes

[Peter Maydell]

On Thu, 28 Nov 2019 at 16:17, Marc Zyngier <maz@kernel.org> wrote:
>
> I started looking the rest of the missing TIDx handling,
> and this resulted in the following patches.
>
> There is still one thing I'm a bit puzzled by though:
>
> HCR_EL2.TID0 mandates trapping of the AArch32 JIDR
> register, but I couldn't find a trace of it in the QEMU
> code, and trying to read it seems to generate an exception.
>
> It isn't like anyone is going to miss it, but I wonder if
> it should be implemented... It could also be that I'm missing
> the obvious and that my testing is broken! ;-)

Hmm, I was under the impression that we correctly implemented
'trivial Jazelle', but we obviously missed some of it
(we do have the handling of BXJ insns).
We should, yes, ideally, have RAZ/WI implementations
of JIDR, JMCR and JOSCR.

We also I think don't get right the fiddly detail about
attempting an exception return with SPSR.J set, but that's
not worth messing about with IMHO.

thanks
-- PMM

Re: [PATCH 0/3] target/arm: More HCR_EL2.TIDx fixes

[Marc Zyngier]

On 2019-11-28 16:30, Peter Maydell wrote:
> On Thu, 28 Nov 2019 at 16:17, Marc Zyngier <maz@kernel.org> wrote:
>>
>> I started looking the rest of the missing TIDx handling,
>> and this resulted in the following patches.
>>
>> There is still one thing I'm a bit puzzled by though:
>>
>> HCR_EL2.TID0 mandates trapping of the AArch32 JIDR
>> register, but I couldn't find a trace of it in the QEMU
>> code, and trying to read it seems to generate an exception.
>>
>> It isn't like anyone is going to miss it, but I wonder if
>> it should be implemented... It could also be that I'm missing
>> the obvious and that my testing is broken! ;-)
>
> Hmm, I was under the impression that we correctly implemented
> 'trivial Jazelle', but we obviously missed some of it
> (we do have the handling of BXJ insns).
> We should, yes, ideally, have RAZ/WI implementations
> of JIDR, JMCR and JOSCR.

OK, I'll have a look at this, and plumb the handling of TID0
in JIDR.

> We also I think don't get right the fiddly detail about
> attempting an exception return with SPSR.J set, but that's
> not worth messing about with IMHO.

Indeed. The less we hear about Jazelle, the better... ;-)

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

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

[Peter Maydell]

On Thu, 28 Nov 2019 at 16:17, Marc Zyngier <maz@kernel.org> 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 <maz@kernel.org>
> ---
>  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);

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.

> +            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.

> +            /* 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();

> +    }
> +
> +    raise_exception(env, EXCP_HYP_TRAP, syn_vmrs_trap(rt, reg), 2);
> +}
> +
>  #endif

thanks
-- PMM

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

[Marc Zyngier]

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 <maz@kernel.org> 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 <maz@kernel.org>
>> ---
>>  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);
>
> 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...

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

[Peter Maydell]

On Thu, 28 Nov 2019 at 17:49, Marc Zyngier <maz@kernel.org> wrote:
>
> 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 <maz@kernel.org> 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 <maz@kernel.org>

> > 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.

Oops, I hadn't noticed that we were switching on reg.
Yeah, you might as well leave it as is. (We could have
a separate helper for each of TID0 and TID3 but that
seems like overkill.)

> 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?

Arm conditional instructions are handled at a pretty
high level in the decode, because they all work the same way.
In disas_arm_insn() we have:

    if (cond != 0xe) {
        /* if not always execute, we generate a conditional jump to
           next instruction */
        arm_skip_unless(s, cond);
    }

and there's something similar in thumb_tr_translate_insn()
which puts in a branch based on the thumb condexec bits.
The target of the branch is a label whose position is
set either in arm_post_translate_insn() after the code for the
insn is emitted, or in arm_tr_tb_stop() if the insn is
the last in the TB (always true for branch or trap insns).

thanks
-- PMM

Re: [PATCH 1/3] target/arm: Honor HCR_EL2.TID2 trapping requirements

[Edgar E. Iglesias]

On Thu, Nov 28, 2019 at 04:17:16PM +0000, Marc Zyngier wrote:
> HCR_EL2.TID2 mandates that access from EL1 to CTR_EL0, CCSIDR_EL1,
> CCSIDR2_EL1, CLIDR_EL1, CSSELR_EL1 are trapped to EL2, and QEMU
> completely ignores it, making impossible for hypervisors to

Nit: "making it impossible"


> virtualize the cache hierarchy.
> 
> Do the right thing by trapping to EL2 if HCR_EL2.TID2 is set.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  target/arm/helper.c | 28 +++++++++++++++++++++++++---
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 0bf8f53d4b..0b6887b100 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1910,6 +1910,17 @@ static void scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>      raw_write(env, ri, value);
>  }
>  
> +static CPAccessResult access_aa64_tid2(CPUARMState *env,
> +                                       const ARMCPRegInfo *ri,
> +                                       bool isread)
> +{
> +    if (arm_current_el(env) == 1 && (arm_hcr_el2_eff(env) & HCR_TID2)) {
> +        return CP_ACCESS_TRAP_EL2;
> +    }
> +
> +    return CP_ACCESS_OK;
> +}
> +
>  static uint64_t ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>  {
>      ARMCPU *cpu = env_archcpu(env);
> @@ -2110,10 +2121,14 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .writefn = pmintenclr_write },
>      { .name = "CCSIDR", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0,
> -      .access = PL1_R, .readfn = ccsidr_read, .type = ARM_CP_NO_RAW },
> +      .access = PL1_R,
> +      .accessfn = access_aa64_tid2,
> +      .readfn = ccsidr_read, .type = ARM_CP_NO_RAW },
>      { .name = "CSSELR", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 2, .opc2 = 0,
> -      .access = PL1_RW, .writefn = csselr_write, .resetvalue = 0,
> +      .access = PL1_RW,
> +      .accessfn = access_aa64_tid2,
> +      .writefn = csselr_write, .resetvalue = 0,
>        .bank_fieldoffsets = { offsetof(CPUARMState, cp15.csselr_s),
>                               offsetof(CPUARMState, cp15.csselr_ns) } },
>      /* Auxiliary ID register: this actually has an IMPDEF value but for now
> @@ -5204,6 +5219,11 @@ static CPAccessResult ctr_el0_access(CPUARMState *env, const ARMCPRegInfo *ri,
>      if (arm_current_el(env) == 0 && !(env->cp15.sctlr_el[1] & SCTLR_UCT)) {
>          return CP_ACCESS_TRAP;
>      }
> +
> +    if (arm_hcr_el2_eff(env) & HCR_TID2) {
> +        return CP_ACCESS_TRAP_EL2;
> +    }


Shouldn't we also be checking that we're in EL < 2 when trapping?

Also, I think we need to somehow hook in access_aa64_tid2() for the AArch32
view of CTR, don't we?

Cheers,
Edgar


> +
>      return CP_ACCESS_OK;
>  }
>  
> @@ -6184,7 +6204,9 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>          ARMCPRegInfo clidr = {
>              .name = "CLIDR", .state = ARM_CP_STATE_BOTH,
>              .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 1,
> -            .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->clidr
> +            .access = PL1_R, .type = ARM_CP_CONST,
> +            .accessfn = access_aa64_tid2,
> +            .resetvalue = cpu->clidr
>          };
>          define_one_arm_cp_reg(cpu, &clidr);
>          define_arm_cp_regs(cpu, v7_cp_reginfo);
> -- 
> 2.20.1
> 
> 

Re: [PATCH 2/3] target/arm: Honor HCR_EL2.TID1 trapping requirements

[Edgar E. Iglesias]

On Thu, Nov 28, 2019 at 04:17:17PM +0000, Marc Zyngier wrote:
> HCR_EL2.TID1 mandates that access from EL1 to REVIDR_EL1, AIDR_EL1
> (and their 32bit equivalents) as well as TCMTR, TLBTR are trapped
> to EL2. QEMU ignores it, naking it harder for a hypervisor to

Typo: "making it harder"


> virtualize the HW (though to be fair, no known hypervisor actually
> cares).
> 
> Do the right thing by trapping to EL2 if HCR_EL2.TID1 is set.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  target/arm/helper.c | 36 ++++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 0b6887b100..9bff769692 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1973,6 +1973,26 @@ static uint64_t isr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>      return ret;
>  }
>  
> +static CPAccessResult access_aa64_tid1(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                       bool isread)
> +{
> +    if (arm_hcr_el2_eff(env) & HCR_TID1) {
> +        return CP_ACCESS_TRAP_EL2;
> +    }

I think we need to check for EL1 here?

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

Cheers,
Edgar


> +
> +    return CP_ACCESS_OK;
> +}
> +
> +static CPAccessResult access_aa32_tid1(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                       bool isread)
> +{
> +    if (arm_feature(env, ARM_FEATURE_V8)) {
> +        return access_aa64_tid1(env, ri, isread);
> +    }
> +
> +    return CP_ACCESS_OK;
> +}
> +
>  static const ARMCPRegInfo v7_cp_reginfo[] = {
>      /* the old v6 WFI, UNPREDICTABLE in v7 but we choose to NOP */
>      { .name = "NOP", .cp = 15, .crn = 7, .crm = 0, .opc1 = 0, .opc2 = 4,
> @@ -2136,7 +2156,9 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>       */
>      { .name = "AIDR", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 1, .crn = 0, .crm = 0, .opc2 = 7,
> -      .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
> +      .access = PL1_R, .type = ARM_CP_CONST,
> +      .accessfn = access_aa64_tid1,
> +      .resetvalue = 0 },
>      /* Auxiliary fault status registers: these also are IMPDEF, and we
>       * choose to RAZ/WI for all cores.
>       */
> @@ -6732,7 +6754,9 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>                .access = PL1_R, .resetvalue = cpu->midr },
>              { .name = "REVIDR_EL1", .state = ARM_CP_STATE_BOTH,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 0, .opc2 = 6,
> -              .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->revidr },
> +              .access = PL1_R,
> +              .accessfn = access_aa64_tid1,
> +              .type = ARM_CP_CONST, .resetvalue = cpu->revidr },
>              REGINFO_SENTINEL
>          };
>          ARMCPRegInfo id_cp_reginfo[] = {
> @@ -6747,14 +6771,18 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>              /* TCMTR and TLBTR exist in v8 but have no 64-bit versions */
>              { .name = "TCMTR",
>                .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 2,
> -              .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
> +              .access = PL1_R,
> +              .accessfn = access_aa32_tid1,
> +              .type = ARM_CP_CONST, .resetvalue = 0 },
>              REGINFO_SENTINEL
>          };
>          /* TLBTR is specific to VMSA */
>          ARMCPRegInfo id_tlbtr_reginfo = {
>                .name = "TLBTR",
>                .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 3,
> -              .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0,
> +              .access = PL1_R,
> +              .accessfn = access_aa32_tid1,
> +              .type = ARM_CP_CONST, .resetvalue = 0,
>          };
>          /* MPUIR is specific to PMSA V6+ */
>          ARMCPRegInfo id_mpuir_reginfo = {
> -- 
> 2.20.1
> 
> 

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

[Edgar E. Iglesias]

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 <maz@kernel.org>
> ---
>  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?

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



> +
> +    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;
> +    }
> +
> +    raise_exception(env, EXCP_HYP_TRAP, syn_vmrs_trap(rt, reg), 2);
> +}
> +
>  #endif
> -- 
> 2.20.1
> 
> 

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

[Marc Zyngier]

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 <maz@kernel.org>
>> ---
>>  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?

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

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 <maz@kernel.org>
> > > ---
> > >  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

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

[Peter Maydell]

On Fri, 29 Nov 2019 at 09:45, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> IIRC, the current EL was always known at translation time but I've
> not been tracking recent changes.

Yes, it's known at translate time, in dc->current_el.
(The code is structured to make it difficult to accidentally
use info that's not known at translate-time: most translate.c
code only has access to the DisasContext struct, and that
struct only has info that is safe to use.)

We need to know the EL at translate time anyway because we
need to generate the right kind of guest load/store, where
the code generated is different for different ELs (they
get looked up in different TLBs because the access
permissions can differ).

thanks
-- PMM