[Qemu-devel] [PATCH 0/6] target-arm: Implement various EL3 traps

[Qemu-devel] [PATCH 0/6] target-arm: Implement various EL3 traps

[Peter Maydell]

This patchset fixes or implements a lot of traps to EL3 as
listed in the ARM ARM section D1.15.4 "EL3 configurable controls".
Most of the rest we already had implemented.

NB: where the trap I was implementing for EL3 had an
obvious equivalent in EL2 I included the EL2 check in this
series, but I haven't attempted to implement traps which are
only EL2 and not EL3.

The remaining traps I haven't done yet are:

 * MDCR_EL3.TPM -- easy (similar to TDOSA/TDRA/TDA) but will
     conflict with the perf monitor fixes currently on list,
     so I haven't done it yet
 * SRS using R13_mon
 * changing to monitor via CPS/MSR/exc return
   -- both of these require us to tighten up our handling of
      bogus mode specifications in various places in 32-bit
      code translation. I'll do this next, but it isn't really
      very similar to these patches which are mostly sysreg
      access traps, so different patchset.

The patchset assumes you've already applied the ones I have
on list at the moment (notably including the patch that adds
the isread parameter to system register access functions).

You can see the whole lot in context at
 https://git.linaro.org/people/peter.maydell/qemu-arm.git v8-tz


Peter Maydell (6):
  target-arm: correct CNTFRQ access rights
  target-arm: Fix handling of SCR.SMD
  target-arm: Implement MDCR_EL3.TDOSA and MDCR_EL2.TDOSA traps
  target-arm: Implement MDCR_EL2.TDRA traps
  target-arm: Implement MDCR_EL2.TDA and MDCR_EL2.TDA traps
  target-arm: Report correct syndrome for FPEXC32_EL2 traps

 target-arm/cpu.h       |  17 +++++++
 target-arm/helper.c    | 124 ++++++++++++++++++++++++++++++++++++++++++-------
 target-arm/op_helper.c |  25 ++++++++--
 3 files changed, 143 insertions(+), 23 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/6] target-arm: correct CNTFRQ access rights

[Peter Maydell]

Correct some corner cases we were getting wrong for
CNTFRQ access rights:
 * should UNDEF from 32-bit Secure EL1
 * only writable from the highest implemented exception level,
   which might not be EL1 now

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 7a8881a..082701a 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1217,9 +1217,34 @@ static const ARMCPRegInfo v6k_cp_reginfo[] = {
 static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri,
                                        bool isread)
 {
-    /* CNTFRQ: not visible from PL0 if both PL0PCTEN and PL0VCTEN are zero */
-    if (arm_current_el(env) == 0 && !extract32(env->cp15.c14_cntkctl, 0, 2)) {
-        return CP_ACCESS_TRAP;
+    /* CNTFRQ: not visible from PL0 if both PL0PCTEN and PL0VCTEN are zero.
+     * Writable only at the highest implemented exception level.
+     */
+    switch (arm_current_el(env)) {
+    case 0:
+        if (!extract32(env->cp15.c14_cntkctl, 0, 2)) {
+            return CP_ACCESS_TRAP;
+        }
+        /* EL0 reads are forbidden by the .access fields */
+        break;
+    case 1:
+        if (!isread && (arm_feature(env, ARM_FEATURE_EL2)
+                        || arm_feature(env, ARM_FEATURE_EL3))) {
+            return CP_ACCESS_TRAP_UNCATEGORIZED;
+        }
+        if (!isread && ri->state == ARM_CP_STATE_AA32 &&
+            arm_is_secure_below_el3(env)) {
+            /* Accesses from 32-bit Secure EL1 UNDEF (*not* trap to EL3!) */
+            return CP_ACCESS_TRAP_UNCATEGORIZED;
+        }
+        break;
+    case 2:
+        if (!isread && arm_feature(env, ARM_FEATURE_EL3)) {
+            return CP_ACCESS_TRAP_UNCATEGORIZED;
+        }
+        break;
+    case 3:
+        break;
     }
     return CP_ACCESS_OK;
 }
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/6] target-arm: correct CNTFRQ access rights

[Sergey Fedorov]

On 05.02.2016 19:44, Peter Maydell wrote:
> Correct some corner cases we were getting wrong for
> CNTFRQ access rights:
>  * should UNDEF from 32-bit Secure EL1
>  * only writable from the highest implemented exception level,
>    which might not be EL1 now
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/helper.c | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 7a8881a..082701a 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1217,9 +1217,34 @@ static const ARMCPRegInfo v6k_cp_reginfo[] = {
>  static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri,
>                                         bool isread)
>  {
> -    /* CNTFRQ: not visible from PL0 if both PL0PCTEN and PL0VCTEN are zero */
> -    if (arm_current_el(env) == 0 && !extract32(env->cp15.c14_cntkctl, 0, 2)) {
> -        return CP_ACCESS_TRAP;
> +    /* CNTFRQ: not visible from PL0 if both PL0PCTEN and PL0VCTEN are zero.
> +     * Writable only at the highest implemented exception level.
> +     */
> +    switch (arm_current_el(env)) {
> +    case 0:
> +        if (!extract32(env->cp15.c14_cntkctl, 0, 2)) {
> +            return CP_ACCESS_TRAP;
> +        }
> +        /* EL0 reads are forbidden by the .access fields */

s/reads/writes/ ?

> +        break;
> +    case 1:
> +        if (!isread && (arm_feature(env, ARM_FEATURE_EL2)
> +                        || arm_feature(env, ARM_FEATURE_EL3))) {
> +            return CP_ACCESS_TRAP_UNCATEGORIZED;
> +        }
> +        if (!isread && ri->state == ARM_CP_STATE_AA32 &&
> +            arm_is_secure_below_el3(env)) {
> +            /* Accesses from 32-bit Secure EL1 UNDEF (*not* trap to EL3!) */
> +            return CP_ACCESS_TRAP_UNCATEGORIZED;
> +        }
> +        break;
> +    case 2:
> +        if (!isread && arm_feature(env, ARM_FEATURE_EL3)) {
> +            return CP_ACCESS_TRAP_UNCATEGORIZED;
> +        }
> +        break;
> +    case 3:
> +        break;
>      }
>      return CP_ACCESS_OK;
>  }

Maybe calculating "the highest implemented exception level" could
simplify reading of the code a bit? E.g.:

    int highest_el = arm_feature(env, ARM_FEATURE_EL3) ? 3 :
                     arm_feature(env, ARM_FEATURE_EL2) ? 2 : 1;

We would probably want to have a dedicated static inline function for
this similar to HighestEL() from ARMv8 ARM pseudocode.

Kind regards,
Sergey

Re: [Qemu-devel] [PATCH 1/6] target-arm: correct CNTFRQ access rights

[Peter Maydell]

On 8 February 2016 at 15:25, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> On 05.02.2016 19:44, Peter Maydell wrote:
>> Correct some corner cases we were getting wrong for
>> CNTFRQ access rights:
>>  * should UNDEF from 32-bit Secure EL1
>>  * only writable from the highest implemented exception level,
>>    which might not be EL1 now

>> +    switch (arm_current_el(env)) {
>> +    case 0:
>> +        if (!extract32(env->cp15.c14_cntkctl, 0, 2)) {
>> +            return CP_ACCESS_TRAP;
>> +        }
>> +        /* EL0 reads are forbidden by the .access fields */
>
> s/reads/writes/ ?

Yes.

>> +        break;
>> +    case 1:
>> +        if (!isread && (arm_feature(env, ARM_FEATURE_EL2)
>> +                        || arm_feature(env, ARM_FEATURE_EL3))) {
>> +            return CP_ACCESS_TRAP_UNCATEGORIZED;
>> +        }
>> +        if (!isread && ri->state == ARM_CP_STATE_AA32 &&
>> +            arm_is_secure_below_el3(env)) {
>> +            /* Accesses from 32-bit Secure EL1 UNDEF (*not* trap to EL3!) */
>> +            return CP_ACCESS_TRAP_UNCATEGORIZED;
>> +        }
>> +        break;
>> +    case 2:
>> +        if (!isread && arm_feature(env, ARM_FEATURE_EL3)) {
>> +            return CP_ACCESS_TRAP_UNCATEGORIZED;
>> +        }
>> +        break;
>> +    case 3:
>> +        break;
>>      }
>>      return CP_ACCESS_OK;
>>  }
>
> Maybe calculating "the highest implemented exception level" could
> simplify reading of the code a bit? E.g.:
>
>     int highest_el = arm_feature(env, ARM_FEATURE_EL3) ? 3 :
>                      arm_feature(env, ARM_FEATURE_EL2) ? 2 : 1;
>
> We would probably want to have a dedicated static inline function for
> this similar to HighestEL() from ARMv8 ARM pseudocode.

Mmm, that might look neater. I'll have a play with the code.

thanks
-- PMM

[Qemu-devel] [PATCH 2/6] target-arm: Fix handling of SCR.SMD

[Peter Maydell]

We weren't quite implementing the handling of SCR.SMD correctly.
The condition governing whether the SMD bit should apply only
for NS state is "is EL3 is AArch32", not "is the current EL AArch32".
Fix the condition, and clarify the comment both to reflect this and
to expand slightly on what's going on for the v7-no-Virtualization case.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
The bit about forcing SMD to zero confused me, anyway, since I
expected it to mean "in this function", not elsewhere...
---
 target-arm/op_helper.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 313c0f8..4fedae5 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -614,12 +614,14 @@ void HELPER(pre_smc)(CPUARMState *env, uint32_t syndrome)
     int cur_el = arm_current_el(env);
     bool secure = arm_is_secure(env);
     bool smd = env->cp15.scr_el3 & SCR_SMD;
-    /* On ARMv8 AArch32, SMD only applies to NS state.
-     * On ARMv7 SMD only applies to NS state and only if EL2 is available.
-     * For ARMv7 non EL2, we force SMD to zero so we don't need to re-check
-     * the EL2 condition here.
+    /* On ARMv8 with EL3 AArch64, SMD applies to both S and NS state.
+     * On ARMv8 with EL3 AArch32, or ARMv7 with the Virtualization
+     *  extensions, SMD only applies to NS state.
+     * On ARMv7 without the Virtualization extensions, the SMD bit
+     * doesn't exist, but we forbid the guest to set it to 1 in scr_write(),
+     * so we need not special case this here.
      */
-    bool undef = is_a64(env) ? smd : (!secure && smd);
+    bool undef = arm_feature(env, ARM_FEATURE_AARCH64) ? smd : smd && !secure;
 
     if (arm_is_psci_call(cpu, EXCP_SMC)) {
         /* If PSCI is enabled and this looks like a valid PSCI call then
-- 
1.9.1

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/6] target-arm: Fix handling of SCR.SMD

[Sergey Fedorov]

On 05.02.2016 19:45, Peter Maydell wrote:
> We weren't quite implementing the handling of SCR.SMD correctly.
> The condition governing whether the SMD bit should apply only
> for NS state is "is EL3 is AArch32", not "is the current EL AArch32".
> Fix the condition, and clarify the comment both to reflect this and
> to expand slightly on what's going on for the v7-no-Virtualization case.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
> The bit about forcing SMD to zero confused me, anyway, since I
> expected it to mean "in this function", not elsewhere...
> ---
>  target-arm/op_helper.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 313c0f8..4fedae5 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -614,12 +614,14 @@ void HELPER(pre_smc)(CPUARMState *env, uint32_t syndrome)
>      int cur_el = arm_current_el(env);
>      bool secure = arm_is_secure(env);
>      bool smd = env->cp15.scr_el3 & SCR_SMD;
> -    /* On ARMv8 AArch32, SMD only applies to NS state.
> -     * On ARMv7 SMD only applies to NS state and only if EL2 is available.
> -     * For ARMv7 non EL2, we force SMD to zero so we don't need to re-check
> -     * the EL2 condition here.
> +    /* On ARMv8 with EL3 AArch64, SMD applies to both S and NS state.
> +     * On ARMv8 with EL3 AArch32, or ARMv7 with the Virtualization
> +     *  extensions, SMD only applies to NS state.
> +     * On ARMv7 without the Virtualization extensions, the SMD bit
> +     * doesn't exist, but we forbid the guest to set it to 1 in scr_write(),
> +     * so we need not special case this here.
>       */
> -    bool undef = is_a64(env) ? smd : (!secure && smd);
> +    bool undef = arm_feature(env, ARM_FEATURE_AARCH64) ? smd : smd && !secure;
>  
>      if (arm_is_psci_call(cpu, EXCP_SMC)) {
>          /* If PSCI is enabled and this looks like a valid PSCI call then

[Qemu-devel] [PATCH 3/6] target-arm: Implement MDCR_EL3.TDOSA and MDCR_EL2.TDOSA traps

[Peter Maydell]

Implement the traps to EL2 and EL3 controlled by the bits
MDCR_EL2.TDOSA MDCR_EL3.TDOSA. These can configurably trap
accesses to the "powerdown debug" registers.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu.h    | 12 ++++++++++++
 target-arm/helper.c | 23 ++++++++++++++++++++++-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 80391fa..d1d6886 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -595,6 +595,18 @@ void pmccntr_sync(CPUARMState *env);
 #define CPTR_TTA      (1U << 20)
 #define CPTR_TFP      (1U << 10)
 
+#define MDCR_EPMAD    (1U << 21)
+#define MDCR_EDAD     (1U << 20)
+#define MDCR_SPME     (1U << 17)
+#define MDCR_SDD      (1U << 16)
+#define MDCR_TDRA     (1U << 11)
+#define MDCR_TDOSA    (1U << 10)
+#define MDCR_TDA      (1U << 9)
+#define MDCR_TDE      (1U << 8)
+#define MDCR_HPME     (1U << 7)
+#define MDCR_TPM      (1U << 6)
+#define MDCR_TPMCR    (1U << 5)
+
 #define CPSR_M (0x1fU)
 #define CPSR_T (1U << 5)
 #define CPSR_F (1U << 6)
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 082701a..18e85fd 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -384,6 +384,24 @@ static CPAccessResult access_trap_aa32s_el1(CPUARMState *env,
     return CP_ACCESS_TRAP_UNCATEGORIZED;
 }
 
+/* Check for traps to "powerdown debug" registers, which are controlled
+ * by MDCR.TDOSA
+ */
+static CPAccessResult access_tdosa(CPUARMState *env, const ARMCPRegInfo *ri,
+                                   bool isread)
+{
+    int el = arm_current_el(env);
+
+    if (el < 2 && (env->cp15.mdcr_el2 & MDCR_TDOSA)
+        && !arm_is_secure_below_el3(env)) {
+        return CP_ACCESS_TRAP_EL2;
+    }
+    if (el < 3 && (env->cp15.mdcr_el3 & MDCR_TDOSA)) {
+        return CP_ACCESS_TRAP_EL3;
+    }
+    return CP_ACCESS_OK;
+}
+
 static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
     ARMCPU *cpu = arm_env_get_cpu(env);
@@ -3779,15 +3797,18 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
     { .name = "OSLAR_EL1", .state = ARM_CP_STATE_BOTH,
       .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 0, .opc2 = 4,
       .access = PL1_W, .type = ARM_CP_NO_RAW,
+      .accessfn = access_tdosa,
       .writefn = oslar_write },
     { .name = "OSLSR_EL1", .state = ARM_CP_STATE_BOTH,
       .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 4,
       .access = PL1_R, .resetvalue = 10,
+      .accessfn = access_tdosa,
       .fieldoffset = offsetof(CPUARMState, cp15.oslsr_el1) },
     /* Dummy OSDLR_EL1: 32-bit Linux will read this */
     { .name = "OSDLR_EL1", .state = ARM_CP_STATE_BOTH,
       .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 4,
-      .access = PL1_RW, .type = ARM_CP_NOP },
+      .access = PL1_RW, .accessfn = access_tdosa,
+      .type = ARM_CP_NOP },
     /* Dummy DBGVCR: Linux wants to clear this on startup, but we don't
      * implement vector catch debug events yet.
      */
-- 
1.9.1

Re: [Qemu-devel] [Qemu-arm] [PATCH 3/6] target-arm: Implement MDCR_EL3.TDOSA and MDCR_EL2.TDOSA traps

[Sergey Fedorov]

On 05.02.2016 19:45, Peter Maydell wrote:
> Implement the traps to EL2 and EL3 controlled by the bits
> MDCR_EL2.TDOSA MDCR_EL3.TDOSA. These can configurably trap
> accesses to the "powerdown debug" registers.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  target-arm/cpu.h    | 12 ++++++++++++
>  target-arm/helper.c | 23 ++++++++++++++++++++++-
>  2 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 80391fa..d1d6886 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -595,6 +595,18 @@ void pmccntr_sync(CPUARMState *env);
>  #define CPTR_TTA      (1U << 20)
>  #define CPTR_TFP      (1U << 10)
>  
> +#define MDCR_EPMAD    (1U << 21)
> +#define MDCR_EDAD     (1U << 20)
> +#define MDCR_SPME     (1U << 17)
> +#define MDCR_SDD      (1U << 16)
> +#define MDCR_TDRA     (1U << 11)
> +#define MDCR_TDOSA    (1U << 10)
> +#define MDCR_TDA      (1U << 9)
> +#define MDCR_TDE      (1U << 8)
> +#define MDCR_HPME     (1U << 7)
> +#define MDCR_TPM      (1U << 6)
> +#define MDCR_TPMCR    (1U << 5)
> +
>  #define CPSR_M (0x1fU)
>  #define CPSR_T (1U << 5)
>  #define CPSR_F (1U << 6)
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 082701a..18e85fd 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -384,6 +384,24 @@ static CPAccessResult access_trap_aa32s_el1(CPUARMState *env,
>      return CP_ACCESS_TRAP_UNCATEGORIZED;
>  }
>  
> +/* Check for traps to "powerdown debug" registers, which are controlled
> + * by MDCR.TDOSA
> + */
> +static CPAccessResult access_tdosa(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                   bool isread)
> +{
> +    int el = arm_current_el(env);
> +
> +    if (el < 2 && (env->cp15.mdcr_el2 & MDCR_TDOSA)
> +        && !arm_is_secure_below_el3(env)) {
> +        return CP_ACCESS_TRAP_EL2;
> +    }
> +    if (el < 3 && (env->cp15.mdcr_el3 & MDCR_TDOSA)) {
> +        return CP_ACCESS_TRAP_EL3;
> +    }
> +    return CP_ACCESS_OK;
> +}
> +
>  static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>  {
>      ARMCPU *cpu = arm_env_get_cpu(env);
> @@ -3779,15 +3797,18 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>      { .name = "OSLAR_EL1", .state = ARM_CP_STATE_BOTH,
>        .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 0, .opc2 = 4,
>        .access = PL1_W, .type = ARM_CP_NO_RAW,
> +      .accessfn = access_tdosa,
>        .writefn = oslar_write },
>      { .name = "OSLSR_EL1", .state = ARM_CP_STATE_BOTH,
>        .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 4,
>        .access = PL1_R, .resetvalue = 10,
> +      .accessfn = access_tdosa,
>        .fieldoffset = offsetof(CPUARMState, cp15.oslsr_el1) },
>      /* Dummy OSDLR_EL1: 32-bit Linux will read this */
>      { .name = "OSDLR_EL1", .state = ARM_CP_STATE_BOTH,
>        .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 4,
> -      .access = PL1_RW, .type = ARM_CP_NOP },
> +      .access = PL1_RW, .accessfn = access_tdosa,
> +      .type = ARM_CP_NOP },
>      /* Dummy DBGVCR: Linux wants to clear this on startup, but we don't
>       * implement vector catch debug events yet.
>       */

[Qemu-devel] [PATCH 4/6] target-arm: Implement MDCR_EL2.TDRA traps

[Peter Maydell]

Implement trapping of the "debug ROM" registers, which are controlled
by MDCR_EL2.TDRA for EL2 but by the more general MDCR_EL3.TDA for EL3.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 18e85fd..8c2adbc 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -402,6 +402,24 @@ static CPAccessResult access_tdosa(CPUARMState *env, const ARMCPRegInfo *ri,
     return CP_ACCESS_OK;
 }
 
+/* Check for traps to "debug ROM" registers, which are controlled
+ * by MDCR_EL2.TDRA for EL2 but by the more general MDCR_EL3.TDA for EL3.
+ */
+static CPAccessResult access_tdra(CPUARMState *env, const ARMCPRegInfo *ri,
+                                  bool isread)
+{
+    int el = arm_current_el(env);
+
+    if (el < 2 && (env->cp15.mdcr_el2 & MDCR_TDRA)
+        && !arm_is_secure_below_el3(env)) {
+        return CP_ACCESS_TRAP_EL2;
+    }
+    if (el < 3 && (env->cp15.mdcr_el3 & MDCR_TDA)) {
+        return CP_ACCESS_TRAP_EL3;
+    }
+    return CP_ACCESS_OK;
+}
+
 static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
     ARMCPU *cpu = arm_env_get_cpu(env);
@@ -3774,12 +3792,15 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
      * accessor.
      */
     { .name = "DBGDRAR", .cp = 14, .crn = 1, .crm = 0, .opc1 = 0, .opc2 = 0,
-      .access = PL0_R, .type = ARM_CP_CONST, .resetvalue = 0 },
+      .access = PL0_R, .accessfn = access_tdra,
+      .type = ARM_CP_CONST, .resetvalue = 0 },
     { .name = "MDRAR_EL1", .state = ARM_CP_STATE_AA64,
       .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 0, .opc2 = 0,
-      .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
+      .access = PL1_R, .accessfn = access_tdra,
+      .type = ARM_CP_CONST, .resetvalue = 0 },
     { .name = "DBGDSAR", .cp = 14, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 0,
-      .access = PL0_R, .type = ARM_CP_CONST, .resetvalue = 0 },
+      .access = PL0_R, .accessfn = access_tdra,
+      .type = ARM_CP_CONST, .resetvalue = 0 },
     /* Monitor debug system control register; the 32-bit alias is DBGDSCRext. */
     { .name = "MDSCR_EL1", .state = ARM_CP_STATE_BOTH,
       .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 2,
-- 
1.9.1

Re: [Qemu-devel] [PATCH 4/6] target-arm: Implement MDCR_EL2.TDRA traps

[Sergey Fedorov]

On 05.02.2016 19:45, Peter Maydell wrote:
> Implement trapping of the "debug ROM" registers, which are controlled
> by MDCR_EL2.TDRA for EL2 but by the more general MDCR_EL3.TDA for EL3.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  target-arm/helper.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 18e85fd..8c2adbc 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -402,6 +402,24 @@ static CPAccessResult access_tdosa(CPUARMState *env, const ARMCPRegInfo *ri,
>      return CP_ACCESS_OK;
>  }
>  
> +/* Check for traps to "debug ROM" registers, which are controlled
> + * by MDCR_EL2.TDRA for EL2 but by the more general MDCR_EL3.TDA for EL3.
> + */
> +static CPAccessResult access_tdra(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                  bool isread)
> +{
> +    int el = arm_current_el(env);
> +
> +    if (el < 2 && (env->cp15.mdcr_el2 & MDCR_TDRA)
> +        && !arm_is_secure_below_el3(env)) {
> +        return CP_ACCESS_TRAP_EL2;
> +    }
> +    if (el < 3 && (env->cp15.mdcr_el3 & MDCR_TDA)) {
> +        return CP_ACCESS_TRAP_EL3;
> +    }
> +    return CP_ACCESS_OK;
> +}
> +
>  static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>  {
>      ARMCPU *cpu = arm_env_get_cpu(env);
> @@ -3774,12 +3792,15 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>       * accessor.
>       */
>      { .name = "DBGDRAR", .cp = 14, .crn = 1, .crm = 0, .opc1 = 0, .opc2 = 0,
> -      .access = PL0_R, .type = ARM_CP_CONST, .resetvalue = 0 },
> +      .access = PL0_R, .accessfn = access_tdra,
> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>      { .name = "MDRAR_EL1", .state = ARM_CP_STATE_AA64,
>        .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 0, .opc2 = 0,
> -      .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
> +      .access = PL1_R, .accessfn = access_tdra,
> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>      { .name = "DBGDSAR", .cp = 14, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 0,
> -      .access = PL0_R, .type = ARM_CP_CONST, .resetvalue = 0 },
> +      .access = PL0_R, .accessfn = access_tdra,
> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>      /* Monitor debug system control register; the 32-bit alias is DBGDSCRext. */
>      { .name = "MDSCR_EL1", .state = ARM_CP_STATE_BOTH,
>        .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 2,

[Qemu-devel] [PATCH 5/6] target-arm: Implement MDCR_EL2.TDA and MDCR_EL2.TDA traps

[Peter Maydell]

Implement the debug register traps controlled by MDCR_EL2.TDA
and MDCR_EL3.TDA.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 8c2adbc..064b415 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -420,6 +420,24 @@ static CPAccessResult access_tdra(CPUARMState *env, const ARMCPRegInfo *ri,
     return CP_ACCESS_OK;
 }
 
+/* Check for traps to general debug registers, which are controlled
+ * by MDCR_EL2.TDA for EL2 and MDCR_EL3.TDA for EL3.
+ */
+static CPAccessResult access_tda(CPUARMState *env, const ARMCPRegInfo *ri,
+                                  bool isread)
+{
+    int el = arm_current_el(env);
+
+    if (el < 2 && (env->cp15.mdcr_el2 & MDCR_TDA)
+        && !arm_is_secure_below_el3(env)) {
+        return CP_ACCESS_TRAP_EL2;
+    }
+    if (el < 3 && (env->cp15.mdcr_el3 & MDCR_TDA)) {
+        return CP_ACCESS_TRAP_EL3;
+    }
+    return CP_ACCESS_OK;
+}
+
 static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
     ARMCPU *cpu = arm_env_get_cpu(env);
@@ -3385,7 +3403,8 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
       .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
     { .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 1,
-      .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+      .access = PL2_RW, .accessfn = access_tda,
+      .type = ARM_CP_CONST, .resetvalue = 0 },
     { .name = "HPFAR_EL2", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
       .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any,
@@ -3804,7 +3823,7 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
     /* Monitor debug system control register; the 32-bit alias is DBGDSCRext. */
     { .name = "MDSCR_EL1", .state = ARM_CP_STATE_BOTH,
       .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 2,
-      .access = PL1_RW,
+      .access = PL1_RW, .accessfn = access_tda,
       .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1),
       .resetvalue = 0 },
     /* MDCCSR_EL0, aka DBGDSCRint. This is a read-only mirror of MDSCR_EL1.
@@ -3813,7 +3832,7 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
     { .name = "MDCCSR_EL0", .state = ARM_CP_STATE_BOTH,
       .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 0,
       .type = ARM_CP_ALIAS,
-      .access = PL1_R,
+      .access = PL1_R, .accessfn = access_tda,
       .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1), },
     { .name = "OSLAR_EL1", .state = ARM_CP_STATE_BOTH,
       .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 0, .opc2 = 4,
@@ -3835,7 +3854,8 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
      */
     { .name = "DBGVCR",
       .cp = 14, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 0,
-      .access = PL1_RW, .type = ARM_CP_NOP },
+      .access = PL1_RW, .accessfn = access_tda,
+      .type = ARM_CP_NOP },
     REGINFO_SENTINEL
 };
 
@@ -4100,7 +4120,8 @@ static void define_debug_regs(ARMCPU *cpu)
     int wrps, brps, ctx_cmps;
     ARMCPRegInfo dbgdidr = {
         .name = "DBGDIDR", .cp = 14, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 0,
-        .access = PL0_R, .type = ARM_CP_CONST, .resetvalue = cpu->dbgdidr,
+        .access = PL0_R, .accessfn = access_tda,
+        .type = ARM_CP_CONST, .resetvalue = cpu->dbgdidr,
     };
 
     /* Note that all these register fields hold "number of Xs minus 1". */
@@ -4131,13 +4152,13 @@ static void define_debug_regs(ARMCPU *cpu)
         ARMCPRegInfo dbgregs[] = {
             { .name = "DBGBVR", .state = ARM_CP_STATE_BOTH,
               .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = i, .opc2 = 4,
-              .access = PL1_RW,
+              .access = PL1_RW, .accessfn = access_tda,
               .fieldoffset = offsetof(CPUARMState, cp15.dbgbvr[i]),
               .writefn = dbgbvr_write, .raw_writefn = raw_write
             },
             { .name = "DBGBCR", .state = ARM_CP_STATE_BOTH,
               .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = i, .opc2 = 5,
-              .access = PL1_RW,
+              .access = PL1_RW, .accessfn = access_tda,
               .fieldoffset = offsetof(CPUARMState, cp15.dbgbcr[i]),
               .writefn = dbgbcr_write, .raw_writefn = raw_write
             },
@@ -4150,13 +4171,13 @@ static void define_debug_regs(ARMCPU *cpu)
         ARMCPRegInfo dbgregs[] = {
             { .name = "DBGWVR", .state = ARM_CP_STATE_BOTH,
               .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = i, .opc2 = 6,
-              .access = PL1_RW,
+              .access = PL1_RW, .accessfn = access_tda,
               .fieldoffset = offsetof(CPUARMState, cp15.dbgwvr[i]),
               .writefn = dbgwvr_write, .raw_writefn = raw_write
             },
             { .name = "DBGWCR", .state = ARM_CP_STATE_BOTH,
               .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = i, .opc2 = 7,
-              .access = PL1_RW,
+              .access = PL1_RW, .accessfn = access_tda,
               .fieldoffset = offsetof(CPUARMState, cp15.dbgwcr[i]),
               .writefn = dbgwcr_write, .raw_writefn = raw_write
             },
-- 
1.9.1

Re: [Qemu-devel] [PATCH 5/6] target-arm: Implement MDCR_EL2.TDA and MDCR_EL2.TDA traps

[Sergey Fedorov]

One of the MDCR_EL2's should be MDCR_EL3 instead.

On 05.02.2016 19:45, Peter Maydell wrote:
> Implement the debug register traps controlled by MDCR_EL2.TDA
> and MDCR_EL3.TDA.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/helper.c | 39 ++++++++++++++++++++++++++++++---------
>  1 file changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 8c2adbc..064b415 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -420,6 +420,24 @@ static CPAccessResult access_tdra(CPUARMState *env, const ARMCPRegInfo *ri,
>      return CP_ACCESS_OK;
>  }
>  
> +/* Check for traps to general debug registers, which are controlled
> + * by MDCR_EL2.TDA for EL2 and MDCR_EL3.TDA for EL3.
> + */
> +static CPAccessResult access_tda(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                  bool isread)
> +{
> +    int el = arm_current_el(env);
> +
> +    if (el < 2 && (env->cp15.mdcr_el2 & MDCR_TDA)
> +        && !arm_is_secure_below_el3(env)) {
> +        return CP_ACCESS_TRAP_EL2;
> +    }
> +    if (el < 3 && (env->cp15.mdcr_el3 & MDCR_TDA)) {
> +        return CP_ACCESS_TRAP_EL3;
> +    }
> +    return CP_ACCESS_OK;
> +}
> +
>  static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>  {
>      ARMCPU *cpu = arm_env_get_cpu(env);
> @@ -3385,7 +3403,8 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>      { .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 1,
> -      .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> +      .access = PL2_RW, .accessfn = access_tda,
> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>      { .name = "HPFAR_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
>        .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any,
> @@ -3804,7 +3823,7 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>      /* Monitor debug system control register; the 32-bit alias is DBGDSCRext. */
>      { .name = "MDSCR_EL1", .state = ARM_CP_STATE_BOTH,
>        .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 2,
> -      .access = PL1_RW,
> +      .access = PL1_RW, .accessfn = access_tda,
>        .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1),
>        .resetvalue = 0 },
>      /* MDCCSR_EL0, aka DBGDSCRint. This is a read-only mirror of MDSCR_EL1.
> @@ -3813,7 +3832,7 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>      { .name = "MDCCSR_EL0", .state = ARM_CP_STATE_BOTH,
>        .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 0,
>        .type = ARM_CP_ALIAS,
> -      .access = PL1_R,
> +      .access = PL1_R, .accessfn = access_tda,

>From ARMv8 ARM rev. A.h: "If MDSCR_EL1.TDCC==1, EL0 read accesses to
this register are trapped to EL1." But it seems like we just don't
implement "Config-RO for EL0" so far. Maybe it's worth to implement a
separate function for checks controlled by MDSCR_EL1.TDCC?

>        .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1), },
>      { .name = "OSLAR_EL1", .state = ARM_CP_STATE_BOTH,
>        .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 0, .opc2 = 4,
> @@ -3835,7 +3854,8 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>       */
>      { .name = "DBGVCR",
>        .cp = 14, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 0,
> -      .access = PL1_RW, .type = ARM_CP_NOP },
> +      .access = PL1_RW, .accessfn = access_tda,
> +      .type = ARM_CP_NOP },
>      REGINFO_SENTINEL
>  };
>  
> @@ -4100,7 +4120,8 @@ static void define_debug_regs(ARMCPU *cpu)
>      int wrps, brps, ctx_cmps;
>      ARMCPRegInfo dbgdidr = {
>          .name = "DBGDIDR", .cp = 14, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 0,
> -        .access = PL0_R, .type = ARM_CP_CONST, .resetvalue = cpu->dbgdidr,
> +        .access = PL0_R, .accessfn = access_tda,
> +        .type = ARM_CP_CONST, .resetvalue = cpu->dbgdidr,

The same concern as above.

Kind regards,
Sergey

>      };
>  
>      /* Note that all these register fields hold "number of Xs minus 1". */
> @@ -4131,13 +4152,13 @@ static void define_debug_regs(ARMCPU *cpu)
>          ARMCPRegInfo dbgregs[] = {
>              { .name = "DBGBVR", .state = ARM_CP_STATE_BOTH,
>                .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = i, .opc2 = 4,
> -              .access = PL1_RW,
> +              .access = PL1_RW, .accessfn = access_tda,
>                .fieldoffset = offsetof(CPUARMState, cp15.dbgbvr[i]),
>                .writefn = dbgbvr_write, .raw_writefn = raw_write
>              },
>              { .name = "DBGBCR", .state = ARM_CP_STATE_BOTH,
>                .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = i, .opc2 = 5,
> -              .access = PL1_RW,
> +              .access = PL1_RW, .accessfn = access_tda,
>                .fieldoffset = offsetof(CPUARMState, cp15.dbgbcr[i]),
>                .writefn = dbgbcr_write, .raw_writefn = raw_write
>              },
> @@ -4150,13 +4171,13 @@ static void define_debug_regs(ARMCPU *cpu)
>          ARMCPRegInfo dbgregs[] = {
>              { .name = "DBGWVR", .state = ARM_CP_STATE_BOTH,
>                .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = i, .opc2 = 6,
> -              .access = PL1_RW,
> +              .access = PL1_RW, .accessfn = access_tda,
>                .fieldoffset = offsetof(CPUARMState, cp15.dbgwvr[i]),
>                .writefn = dbgwvr_write, .raw_writefn = raw_write
>              },
>              { .name = "DBGWCR", .state = ARM_CP_STATE_BOTH,
>                .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = i, .opc2 = 7,
> -              .access = PL1_RW,
> +              .access = PL1_RW, .accessfn = access_tda,
>                .fieldoffset = offsetof(CPUARMState, cp15.dbgwcr[i]),
>                .writefn = dbgwcr_write, .raw_writefn = raw_write
>              },

Re: [Qemu-devel] [PATCH 5/6] target-arm: Implement MDCR_EL2.TDA and MDCR_EL2.TDA traps

[Peter Maydell]

On 8 February 2016 at 16:31, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> One of the MDCR_EL2's should be MDCR_EL3 instead.

Oops, yes :-)

> On 05.02.2016 19:45, Peter Maydell wrote:
>> Implement the debug register traps controlled by MDCR_EL2.TDA
>> and MDCR_EL3.TDA.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  target-arm/helper.c | 39 ++++++++++++++++++++++++++++++---------
>>  1 file changed, 30 insertions(+), 9 deletions(-)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 8c2adbc..064b415 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -420,6 +420,24 @@ static CPAccessResult access_tdra(CPUARMState *env, const ARMCPRegInfo *ri,
>>      return CP_ACCESS_OK;
>>  }
>>
>> +/* Check for traps to general debug registers, which are controlled
>> + * by MDCR_EL2.TDA for EL2 and MDCR_EL3.TDA for EL3.
>> + */
>> +static CPAccessResult access_tda(CPUARMState *env, const ARMCPRegInfo *ri,
>> +                                  bool isread)
>> +{
>> +    int el = arm_current_el(env);
>> +
>> +    if (el < 2 && (env->cp15.mdcr_el2 & MDCR_TDA)
>> +        && !arm_is_secure_below_el3(env)) {
>> +        return CP_ACCESS_TRAP_EL2;
>> +    }
>> +    if (el < 3 && (env->cp15.mdcr_el3 & MDCR_TDA)) {
>> +        return CP_ACCESS_TRAP_EL3;
>> +    }
>> +    return CP_ACCESS_OK;
>> +}
>> +
>>  static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>>  {
>>      ARMCPU *cpu = arm_env_get_cpu(env);
>> @@ -3385,7 +3403,8 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>>      { .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH,
>>        .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 1,
>> -      .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +      .access = PL2_RW, .accessfn = access_tda,
>> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>>      { .name = "HPFAR_EL2", .state = ARM_CP_STATE_BOTH,
>>        .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
>>        .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any,
>> @@ -3804,7 +3823,7 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>>      /* Monitor debug system control register; the 32-bit alias is DBGDSCRext. */
>>      { .name = "MDSCR_EL1", .state = ARM_CP_STATE_BOTH,
>>        .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 2,
>> -      .access = PL1_RW,
>> +      .access = PL1_RW, .accessfn = access_tda,
>>        .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1),
>>        .resetvalue = 0 },
>>      /* MDCCSR_EL0, aka DBGDSCRint. This is a read-only mirror of MDSCR_EL1.
>> @@ -3813,7 +3832,7 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>>      { .name = "MDCCSR_EL0", .state = ARM_CP_STATE_BOTH,
>>        .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 0,
>>        .type = ARM_CP_ALIAS,
>> -      .access = PL1_R,
>> +      .access = PL1_R, .accessfn = access_tda,
>
> From ARMv8 ARM rev. A.h: "If MDSCR_EL1.TDCC==1, EL0 read accesses to
> this register are trapped to EL1." But it seems like we just don't
> implement "Config-RO for EL0" so far.

Yes. There's a comment about this, though it's just outside the
context region that diff has produced.

> Maybe it's worth to implement a
> separate function for checks controlled by MDSCR_EL1.TDCC?

I think that's a separate issue from the EL2/EL3 traps and
should go in its own patch. This series is just trying to get
EL3 right.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/6] target-arm: Implement MDCR_EL2.TDA and MDCR_EL2.TDA traps

[Sergey Fedorov]

On 08.02.2016 19:38, Peter Maydell wrote:
> On 8 February 2016 at 16:31, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> One of the MDCR_EL2's should be MDCR_EL3 instead.
> Oops, yes :-)
>
>> On 05.02.2016 19:45, Peter Maydell wrote:
>>> Implement the debug register traps controlled by MDCR_EL2.TDA
>>> and MDCR_EL3.TDA.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>>  target-arm/helper.c | 39 ++++++++++++++++++++++++++++++---------
>>>  1 file changed, 30 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>> index 8c2adbc..064b415 100644
>>> --- a/target-arm/helper.c
>>> +++ b/target-arm/helper.c
>>> @@ -420,6 +420,24 @@ static CPAccessResult access_tdra(CPUARMState *env, const ARMCPRegInfo *ri,
>>>      return CP_ACCESS_OK;
>>>  }
>>>
>>> +/* Check for traps to general debug registers, which are controlled
>>> + * by MDCR_EL2.TDA for EL2 and MDCR_EL3.TDA for EL3.
>>> + */
>>> +static CPAccessResult access_tda(CPUARMState *env, const ARMCPRegInfo *ri,
>>> +                                  bool isread)
>>> +{
>>> +    int el = arm_current_el(env);
>>> +
>>> +    if (el < 2 && (env->cp15.mdcr_el2 & MDCR_TDA)
>>> +        && !arm_is_secure_below_el3(env)) {
>>> +        return CP_ACCESS_TRAP_EL2;
>>> +    }
>>> +    if (el < 3 && (env->cp15.mdcr_el3 & MDCR_TDA)) {
>>> +        return CP_ACCESS_TRAP_EL3;
>>> +    }
>>> +    return CP_ACCESS_OK;
>>> +}
>>> +
>>>  static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>>>  {
>>>      ARMCPU *cpu = arm_env_get_cpu(env);
>>> @@ -3385,7 +3403,8 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>>>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>>>      { .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH,
>>>        .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 1,
>>> -      .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>>> +      .access = PL2_RW, .accessfn = access_tda,
>>> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>>>      { .name = "HPFAR_EL2", .state = ARM_CP_STATE_BOTH,
>>>        .opc0 = 3, .opc1 = 4, .crn = 6, .crm = 0, .opc2 = 4,
>>>        .access = PL2_RW, .accessfn = access_el3_aa32ns_aa64any,
>>> @@ -3804,7 +3823,7 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>>>      /* Monitor debug system control register; the 32-bit alias is DBGDSCRext. */
>>>      { .name = "MDSCR_EL1", .state = ARM_CP_STATE_BOTH,
>>>        .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 2,
>>> -      .access = PL1_RW,
>>> +      .access = PL1_RW, .accessfn = access_tda,
>>>        .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1),
>>>        .resetvalue = 0 },
>>>      /* MDCCSR_EL0, aka DBGDSCRint. This is a read-only mirror of MDSCR_EL1.
>>> @@ -3813,7 +3832,7 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>>>      { .name = "MDCCSR_EL0", .state = ARM_CP_STATE_BOTH,
>>>        .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 0,
>>>        .type = ARM_CP_ALIAS,
>>> -      .access = PL1_R,
>>> +      .access = PL1_R, .accessfn = access_tda,
>> From ARMv8 ARM rev. A.h: "If MDSCR_EL1.TDCC==1, EL0 read accesses to
>> this register are trapped to EL1." But it seems like we just don't
>> implement "Config-RO for EL0" so far.
> Yes. There's a comment about this, though it's just outside the
> context region that diff has produced.
>
>> Maybe it's worth to implement a
>> separate function for checks controlled by MDSCR_EL1.TDCC?
> I think that's a separate issue from the EL2/EL3 traps and
> should go in its own patch. This series is just trying to get
> EL3 right.

Okay, with fixed subject:

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

Kind regards,
Sergey

[Qemu-devel] [PATCH 6/6] target-arm: Report correct syndrome for FPEXC32_EL2 traps

[Peter Maydell]

If access to FPEXC32_EL2 is trapped by CPTR_EL2.TFP or CPTR_EL3.TFP,
this should be reported with a syndrome register indicating an
FP access trap, not one indicating a system register access trap.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu.h       |  5 +++++
 target-arm/helper.c    |  4 ++--
 target-arm/op_helper.c | 13 +++++++++++++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index d1d6886..a959ad6 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1322,6 +1322,11 @@ typedef enum CPAccessResult {
     /* As CP_ACCESS_UNCATEGORIZED, but for traps directly to EL2 or EL3 */
     CP_ACCESS_TRAP_UNCATEGORIZED_EL2 = 5,
     CP_ACCESS_TRAP_UNCATEGORIZED_EL3 = 6,
+    /* Access fails and results in an exception syndrome for an FP access,
+     * trapped directly to EL2 or EL3
+     */
+    CP_ACCESS_TRAP_FP_EL2 = 7,
+    CP_ACCESS_TRAP_FP_EL3 = 8,
 } CPAccessResult;
 
 /* Access functions for coprocessor registers. These cannot fail and
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 064b415..163a72a 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3012,10 +3012,10 @@ static CPAccessResult fpexc32_access(CPUARMState *env, const ARMCPRegInfo *ri,
                                      bool isread)
 {
     if ((env->cp15.cptr_el[2] & CPTR_TFP) && arm_current_el(env) == 2) {
-        return CP_ACCESS_TRAP_EL2;
+        return CP_ACCESS_TRAP_FP_EL2;
     }
     if (env->cp15.cptr_el[3] & CPTR_TFP) {
-        return CP_ACCESS_TRAP_EL3;
+        return CP_ACCESS_TRAP_FP_EL3;
     }
     return CP_ACCESS_OK;
 }
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 4fedae5..754f080 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -500,6 +500,19 @@ void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome,
         target_el = 3;
         syndrome = syn_uncategorized();
         break;
+    case CP_ACCESS_TRAP_FP_EL2:
+        target_el = 2;
+        /* Since we are an implementation that takes exceptions on a trapped
+         * conditional insn only if the insn has passed its condition code
+         * check, we take the IMPDEF choice to always report CV=1 COND=0xe
+         * (which is also the required value for AArch64 traps).
+         */
+        syndrome = syn_fp_access_trap(1, 0xe, false);
+        break;
+    case CP_ACCESS_TRAP_FP_EL3:
+        target_el = 3;
+        syndrome = syn_fp_access_trap(1, 0xe, false);
+        break;
     default:
         g_assert_not_reached();
     }
-- 
1.9.1

Re: [Qemu-devel] [PATCH 6/6] target-arm: Report correct syndrome for FPEXC32_EL2 traps

[Sergey Fedorov]

On 05.02.2016 19:45, Peter Maydell wrote:
> If access to FPEXC32_EL2 is trapped by CPTR_EL2.TFP or CPTR_EL3.TFP,
> this should be reported with a syndrome register indicating an
> FP access trap, not one indicating a system register access trap.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  target-arm/cpu.h       |  5 +++++
>  target-arm/helper.c    |  4 ++--
>  target-arm/op_helper.c | 13 +++++++++++++
>  3 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index d1d6886..a959ad6 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1322,6 +1322,11 @@ typedef enum CPAccessResult {
>      /* As CP_ACCESS_UNCATEGORIZED, but for traps directly to EL2 or EL3 */
>      CP_ACCESS_TRAP_UNCATEGORIZED_EL2 = 5,
>      CP_ACCESS_TRAP_UNCATEGORIZED_EL3 = 6,
> +    /* Access fails and results in an exception syndrome for an FP access,
> +     * trapped directly to EL2 or EL3
> +     */
> +    CP_ACCESS_TRAP_FP_EL2 = 7,
> +    CP_ACCESS_TRAP_FP_EL3 = 8,
>  } CPAccessResult;
>  
>  /* Access functions for coprocessor registers. These cannot fail and
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 064b415..163a72a 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3012,10 +3012,10 @@ static CPAccessResult fpexc32_access(CPUARMState *env, const ARMCPRegInfo *ri,
>                                       bool isread)
>  {
>      if ((env->cp15.cptr_el[2] & CPTR_TFP) && arm_current_el(env) == 2) {
> -        return CP_ACCESS_TRAP_EL2;
> +        return CP_ACCESS_TRAP_FP_EL2;
>      }
>      if (env->cp15.cptr_el[3] & CPTR_TFP) {
> -        return CP_ACCESS_TRAP_EL3;
> +        return CP_ACCESS_TRAP_FP_EL3;
>      }
>      return CP_ACCESS_OK;
>  }
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 4fedae5..754f080 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -500,6 +500,19 @@ void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome,
>          target_el = 3;
>          syndrome = syn_uncategorized();
>          break;
> +    case CP_ACCESS_TRAP_FP_EL2:
> +        target_el = 2;
> +        /* Since we are an implementation that takes exceptions on a trapped
> +         * conditional insn only if the insn has passed its condition code
> +         * check, we take the IMPDEF choice to always report CV=1 COND=0xe
> +         * (which is also the required value for AArch64 traps).
> +         */
> +        syndrome = syn_fp_access_trap(1, 0xe, false);
> +        break;
> +    case CP_ACCESS_TRAP_FP_EL3:
> +        target_el = 3;
> +        syndrome = syn_fp_access_trap(1, 0xe, false);
> +        break;
>      default:
>          g_assert_not_reached();
>      }