[Qemu-devel] [PATCH v2 0/2] Implement MDCR_EL3.TPM and MDCR_EL2.TPM traps

[Qemu-devel] [PATCH v2 0/2] Implement MDCR_EL3.TPM and MDCR_EL2.TPM traps

[Peter Maydell]

This patchset implements the MDCR_EL3 and MDCR_EL2 trap bits
to trap the performance monitor registers.

Patch 2 is the same as I sent out earlier today as a standalone
patch. Patch 1 fixes a couple of bugs in our SDCR handling, and
in particular imposes a mask so that if a guest has booted with
EL3 in AArch32 then it cannot set bits like TPM which are RES0
for the 32-bit version of the register.

Peter Maydell (2):
  target-arm: Fix handling of SDCR for 32-bit code
  target-arm: Implement MDCR_EL3.TPM and MDCR_EL2.TPM traps

 target-arm/cpu.h    |  4 ++++
 target-arm/helper.c | 66 +++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 55 insertions(+), 15 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 1/2] target-arm: Fix handling of SDCR for 32-bit code

[Peter Maydell]

Fix two issues with our implementation of the SDCR:
 * it is only present from ARMv8 onwards
 * it does not contain several of the trap bits present in its 64-bit
   counterpart the MDCR_EL3

Put the register description in the right place so that it does not
get enabled for ARMv7 and earlier, and give it a write function so that
we can mask out the bits which should not be allowed to have an effect
if EL3 is 32-bit.

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

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index e72e33b..a729ae0 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -599,6 +599,7 @@ void pmccntr_sync(CPUARMState *env);
 #define MDCR_EDAD     (1U << 20)
 #define MDCR_SPME     (1U << 17)
 #define MDCR_SDD      (1U << 16)
+#define MDCR_SPD      (3U << 14)
 #define MDCR_TDRA     (1U << 11)
 #define MDCR_TDOSA    (1U << 10)
 #define MDCR_TDA      (1U << 9)
@@ -607,6 +608,9 @@ void pmccntr_sync(CPUARMState *env);
 #define MDCR_TPM      (1U << 6)
 #define MDCR_TPMCR    (1U << 5)
 
+/* Not all of the MDCR_EL3 bits are present in the 32-bit SDCR */
+#define SDCR_VALID_MASK (MDCR_EPMAD | MDCR_EDAD | MDCR_SPME | MDCR_SPD)
+
 #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 3d7fda1..e9b89e6 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3037,6 +3037,12 @@ static CPAccessResult fpexc32_access(CPUARMState *env, const ARMCPRegInfo *ri,
     return CP_ACCESS_OK;
 }
 
+static void sdcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                       uint64_t value)
+{
+    env->cp15.mdcr_el3 = value & SDCR_VALID_MASK;
+}
+
 static const ARMCPRegInfo v8_cp_reginfo[] = {
     /* Minimal set of EL0-visible registers. This will need to be expanded
      * significantly for system emulation of AArch64 CPUs.
@@ -3331,6 +3337,15 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
       .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 3, .opc2 = 3,
       .access = PL2_RW,
       .fieldoffset = offsetof(CPUARMState, banked_spsr[BANK_FIQ]) },
+    { .name = "MDCR_EL3", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 3, .opc2 = 1,
+      .resetvalue = 0,
+      .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el3) },
+    { .name = "SDCR", .type = ARM_CP_ALIAS,
+      .cp = 15, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 1,
+      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
+      .writefn = sdcr_write,
+      .fieldoffset = offsetoflow32(CPUARMState, cp15.mdcr_el3) },
     REGINFO_SENTINEL
 };
 
@@ -3688,14 +3703,6 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
       .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
       .fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3),
       .writefn = scr_write },
-    { .name = "MDCR_EL3", .state = ARM_CP_STATE_AA64,
-      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 3, .opc2 = 1,
-      .resetvalue = 0,
-      .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el3) },
-    { .name = "SDCR", .type = ARM_CP_ALIAS,
-      .cp = 15, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 1,
-      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
-      .fieldoffset = offsetoflow32(CPUARMState, cp15.mdcr_el3) },
     { .name = "SDER32_EL3", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 1,
       .access = PL3_RW, .resetvalue = 0,
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 1/2] target-arm: Fix handling of SDCR for 32-bit code

[Sergey Fedorov]

On 19.02.2016 17:39, Peter Maydell wrote:
> Fix two issues with our implementation of the SDCR:
>  * it is only present from ARMv8 onwards
>  * it does not contain several of the trap bits present in its 64-bit
>    counterpart the MDCR_EL3
>
> Put the register description in the right place so that it does not
> get enabled for ARMv7 and earlier, and give it a write function so that
> we can mask out the bits which should not be allowed to have an effect
> if EL3 is 32-bit.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/cpu.h    |  4 ++++
>  target-arm/helper.c | 23 +++++++++++++++--------
>  2 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index e72e33b..a729ae0 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -599,6 +599,7 @@ void pmccntr_sync(CPUARMState *env);
>  #define MDCR_EDAD     (1U << 20)
>  #define MDCR_SPME     (1U << 17)
>  #define MDCR_SDD      (1U << 16)
> +#define MDCR_SPD      (3U << 14)
>  #define MDCR_TDRA     (1U << 11)
>  #define MDCR_TDOSA    (1U << 10)
>  #define MDCR_TDA      (1U << 9)
> @@ -607,6 +608,9 @@ void pmccntr_sync(CPUARMState *env);
>  #define MDCR_TPM      (1U << 6)
>  #define MDCR_TPMCR    (1U << 5)
>  
> +/* Not all of the MDCR_EL3 bits are present in the 32-bit SDCR */
> +#define SDCR_VALID_MASK (MDCR_EPMAD | MDCR_EDAD | MDCR_SPME | MDCR_SPD)
> +
>  #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 3d7fda1..e9b89e6 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3037,6 +3037,12 @@ static CPAccessResult fpexc32_access(CPUARMState *env, const ARMCPRegInfo *ri,
>      return CP_ACCESS_OK;
>  }
>  
> +static void sdcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                       uint64_t value)
> +{
> +    env->cp15.mdcr_el3 = value & SDCR_VALID_MASK;
> +}
> +

Just one comment. As soon as we cannot have both of MDCR_EL3 in SDCR in
a specific CPU configuration (EL3 is either AArch64 or AArch32), the
RES0 bitfields of SDCR are "RES0 in all contexts". Thus we can choose
"The bit is hardwired to 0" behaviour as we do here. We could also
choose another behaviour "The bit can be written" and check for "EL3 is
AArch64" case before trying to interpret those bits. Anyway:

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

>  static const ARMCPRegInfo v8_cp_reginfo[] = {
>      /* Minimal set of EL0-visible registers. This will need to be expanded
>       * significantly for system emulation of AArch64 CPUs.
> @@ -3331,6 +3337,15 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
>        .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 3, .opc2 = 3,
>        .access = PL2_RW,
>        .fieldoffset = offsetof(CPUARMState, banked_spsr[BANK_FIQ]) },
> +    { .name = "MDCR_EL3", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 3, .opc2 = 1,
> +      .resetvalue = 0,
> +      .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el3) },
> +    { .name = "SDCR", .type = ARM_CP_ALIAS,
> +      .cp = 15, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 1,
> +      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
> +      .writefn = sdcr_write,
> +      .fieldoffset = offsetoflow32(CPUARMState, cp15.mdcr_el3) },
>      REGINFO_SENTINEL
>  };
>  
> @@ -3688,14 +3703,6 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>        .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
>        .fieldoffset = offsetoflow32(CPUARMState, cp15.scr_el3),
>        .writefn = scr_write },
> -    { .name = "MDCR_EL3", .state = ARM_CP_STATE_AA64,
> -      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 3, .opc2 = 1,
> -      .resetvalue = 0,
> -      .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el3) },
> -    { .name = "SDCR", .type = ARM_CP_ALIAS,
> -      .cp = 15, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 1,
> -      .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
> -      .fieldoffset = offsetoflow32(CPUARMState, cp15.mdcr_el3) },
>      { .name = "SDER32_EL3", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 1,
>        .access = PL3_RW, .resetvalue = 0,

Re: [Qemu-devel] [PATCH v2 1/2] target-arm: Fix handling of SDCR for 32-bit code

[Peter Maydell]

On 19 February 2016 at 16:31, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> On 19.02.2016 17:39, Peter Maydell wrote:
>> +static void sdcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> +                       uint64_t value)
>> +{
>> +    env->cp15.mdcr_el3 = value & SDCR_VALID_MASK;
>> +}
>> +
>
> Just one comment. As soon as we cannot have both of MDCR_EL3 in SDCR in
> a specific CPU configuration (EL3 is either AArch64 or AArch32), the
> RES0 bitfields of SDCR are "RES0 in all contexts". Thus we can choose
> "The bit is hardwired to 0" behaviour as we do here. We could also
> choose another behaviour "The bit can be written" and check for "EL3 is
> AArch64" case before trying to interpret those bits.

Yes, as you say we could do either (and we have examples of both in
QEMU currently, as well as examples of "we don't do either and if
the guest writes in a bit it should not it will get a feature it
shouldn't in theory have"). Masking on write seemed simpler here.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/2] target-arm: Fix handling of SDCR for 32-bit code

[Alistair Francis]

On Fri, Feb 19, 2016 at 6:39 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Fix two issues with our implementation of the SDCR:
>  * it is only present from ARMv8 onwards
>  * it does not contain several of the trap bits present in its 64-bit
>    counterpart the MDCR_EL3
>
> Put the register description in the right place so that it does not
> get enabled for ARMv7 and earlier, and give it a write function so that
> we can mask out the bits which should not be allowed to have an effect
> if EL3 is 32-bit.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Acked-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

> ---
>  target-arm/cpu.h    |  4 ++++
>  target-arm/helper.c | 23 +++++++++++++++--------
>  2 files changed, 19 insertions(+), 8 deletions(-)
>

[Qemu-devel] [PATCH v2 2/2] target-arm: Implement MDCR_EL3.TPM and MDCR_EL2.TPM traps

[Peter Maydell]

Implement the performance monitor register traps controlled
by MDCR_EL3.TPM and MDCR_EL2.TPM. Most of the performance
registers already have an access function to deal with the
user-enable bit, and the TPM checks can be added there. We
also need a new access function which only implements the
TPM checks for use by the few not-EL0-accessible registers
and by PMUSERENR_EL0 (which is always EL0-readable).

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

diff --git a/target-arm/helper.c b/target-arm/helper.c
index e9b89e6..ef3f1ce 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -439,6 +439,24 @@ static CPAccessResult access_tda(CPUARMState *env, const ARMCPRegInfo *ri,
     return CP_ACCESS_OK;
 }
 
+/* Check for traps to performance monitor registers, which are controlled
+ * by MDCR_EL2.TPM for EL2 and MDCR_EL3.TPM for EL3.
+ */
+static CPAccessResult access_tpm(CPUARMState *env, const ARMCPRegInfo *ri,
+                                 bool isread)
+{
+    int el = arm_current_el(env);
+
+    if (el < 2 && (env->cp15.mdcr_el2 & MDCR_TPM)
+        && !arm_is_secure_below_el3(env)) {
+        return CP_ACCESS_TRAP_EL2;
+    }
+    if (el < 3 && (env->cp15.mdcr_el3 & MDCR_TPM)) {
+        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);
@@ -774,11 +792,22 @@ static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
                                    bool isread)
 {
     /* Performance monitor registers user accessibility is controlled
-     * by PMUSERENR.
+     * by PMUSERENR. MDCR_EL2.TPM and MDCR_EL3.TPM allow configurable
+     * trapping to EL2 or EL3 for other accesses.
      */
-    if (arm_current_el(env) == 0 && !env->cp15.c9_pmuserenr) {
+    int el = arm_current_el(env);
+
+    if (el == 0 && !env->cp15.c9_pmuserenr) {
         return CP_ACCESS_TRAP;
     }
+    if (el < 2 && (env->cp15.mdcr_el2 & MDCR_TPM)
+        && !arm_is_secure_below_el3(env)) {
+        return CP_ACCESS_TRAP_EL2;
+    }
+    if (el < 3 && (env->cp15.mdcr_el3 & MDCR_TPM)) {
+        return CP_ACCESS_TRAP_EL3;
+    }
+
     return CP_ACCESS_OK;
 }
 
@@ -1101,28 +1130,28 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
       .accessfn = pmreg_access },
     { .name = "PMUSERENR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 0,
-      .access = PL0_R | PL1_RW,
+      .access = PL0_R | PL1_RW, .accessfn = access_tpm,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
       .resetvalue = 0,
       .writefn = pmuserenr_write, .raw_writefn = raw_write },
     { .name = "PMUSERENR_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 14, .opc2 = 0,
-      .access = PL0_R | PL1_RW, .type = ARM_CP_ALIAS,
+      .access = PL0_R | PL1_RW, .accessfn = access_tpm, .type = ARM_CP_ALIAS,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
       .resetvalue = 0,
       .writefn = pmuserenr_write, .raw_writefn = raw_write },
     { .name = "PMINTENSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 1,
-      .access = PL1_RW,
+      .access = PL1_RW, .accessfn = access_tpm,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
       .resetvalue = 0,
       .writefn = pmintenset_write, .raw_writefn = raw_write },
     { .name = "PMINTENCLR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 2,
-      .access = PL1_RW, .type = ARM_CP_ALIAS,
+      .access = PL1_RW, .accessfn = access_tpm, .type = ARM_CP_ALIAS,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
       .writefn = pmintenclr_write, },
     { .name = "PMINTENCLR_EL1", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 2,
-      .access = PL1_RW, .type = ARM_CP_ALIAS,
+      .access = PL1_RW, .accessfn = access_tpm, .type = ARM_CP_ALIAS,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
       .writefn = pmintenclr_write },
     { .name = "VBAR", .state = ARM_CP_STATE_BOTH,
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 2/2] target-arm: Implement MDCR_EL3.TPM and MDCR_EL2.TPM traps

[Sergey Fedorov]

On 19.02.2016 17:39, Peter Maydell wrote:
> Implement the performance monitor register traps controlled
> by MDCR_EL3.TPM and MDCR_EL2.TPM. Most of the performance
> registers already have an access function to deal with the
> user-enable bit, and the TPM checks can be added there. We
> also need a new access function which only implements the
> TPM checks for use by the few not-EL0-accessible registers
> and by PMUSERENR_EL0 (which is always EL0-readable).
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  target-arm/helper.c | 43 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index e9b89e6..ef3f1ce 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -439,6 +439,24 @@ static CPAccessResult access_tda(CPUARMState *env, const ARMCPRegInfo *ri,
>      return CP_ACCESS_OK;
>  }
>  
> +/* Check for traps to performance monitor registers, which are controlled
> + * by MDCR_EL2.TPM for EL2 and MDCR_EL3.TPM for EL3.
> + */
> +static CPAccessResult access_tpm(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                 bool isread)
> +{
> +    int el = arm_current_el(env);
> +
> +    if (el < 2 && (env->cp15.mdcr_el2 & MDCR_TPM)
> +        && !arm_is_secure_below_el3(env)) {
> +        return CP_ACCESS_TRAP_EL2;
> +    }
> +    if (el < 3 && (env->cp15.mdcr_el3 & MDCR_TPM)) {
> +        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);
> @@ -774,11 +792,22 @@ static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
>                                     bool isread)
>  {
>      /* Performance monitor registers user accessibility is controlled
> -     * by PMUSERENR.
> +     * by PMUSERENR. MDCR_EL2.TPM and MDCR_EL3.TPM allow configurable
> +     * trapping to EL2 or EL3 for other accesses.
>       */
> -    if (arm_current_el(env) == 0 && !env->cp15.c9_pmuserenr) {
> +    int el = arm_current_el(env);
> +
> +    if (el == 0 && !env->cp15.c9_pmuserenr) {
>          return CP_ACCESS_TRAP;
>      }
> +    if (el < 2 && (env->cp15.mdcr_el2 & MDCR_TPM)
> +        && !arm_is_secure_below_el3(env)) {
> +        return CP_ACCESS_TRAP_EL2;
> +    }
> +    if (el < 3 && (env->cp15.mdcr_el3 & MDCR_TPM)) {
> +        return CP_ACCESS_TRAP_EL3;
> +    }
> +
>      return CP_ACCESS_OK;
>  }
>  
> @@ -1101,28 +1130,28 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
>        .accessfn = pmreg_access },
>      { .name = "PMUSERENR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 0,
> -      .access = PL0_R | PL1_RW,
> +      .access = PL0_R | PL1_RW, .accessfn = access_tpm,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
>        .resetvalue = 0,
>        .writefn = pmuserenr_write, .raw_writefn = raw_write },
>      { .name = "PMUSERENR_EL0", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 14, .opc2 = 0,
> -      .access = PL0_R | PL1_RW, .type = ARM_CP_ALIAS,
> +      .access = PL0_R | PL1_RW, .accessfn = access_tpm, .type = ARM_CP_ALIAS,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
>        .resetvalue = 0,
>        .writefn = pmuserenr_write, .raw_writefn = raw_write },
>      { .name = "PMINTENSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 1,
> -      .access = PL1_RW,
> +      .access = PL1_RW, .accessfn = access_tpm,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
>        .resetvalue = 0,
>        .writefn = pmintenset_write, .raw_writefn = raw_write },
>      { .name = "PMINTENCLR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 2,
> -      .access = PL1_RW, .type = ARM_CP_ALIAS,
> +      .access = PL1_RW, .accessfn = access_tpm, .type = ARM_CP_ALIAS,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
>        .writefn = pmintenclr_write, },
>      { .name = "PMINTENCLR_EL1", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 2,
> -      .access = PL1_RW, .type = ARM_CP_ALIAS,
> +      .access = PL1_RW, .accessfn = access_tpm, .type = ARM_CP_ALIAS,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
>        .writefn = pmintenclr_write },
>      { .name = "VBAR", .state = ARM_CP_STATE_BOTH,

Re: [Qemu-devel] [PATCH v2 2/2] target-arm: Implement MDCR_EL3.TPM and MDCR_EL2.TPM traps

[Alistair Francis]

On Fri, Feb 19, 2016 at 6:39 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Implement the performance monitor register traps controlled
> by MDCR_EL3.TPM and MDCR_EL2.TPM. Most of the performance
> registers already have an access function to deal with the
> user-enable bit, and the TPM checks can be added there. We
> also need a new access function which only implements the
> TPM checks for use by the few not-EL0-accessible registers
> and by PMUSERENR_EL0 (which is always EL0-readable).
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/helper.c | 43 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index e9b89e6..ef3f1ce 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -439,6 +439,24 @@ static CPAccessResult access_tda(CPUARMState *env, const ARMCPRegInfo *ri,
>      return CP_ACCESS_OK;
>  }
>
> +/* Check for traps to performance monitor registers, which are controlled
> + * by MDCR_EL2.TPM for EL2 and MDCR_EL3.TPM for EL3.
> + */
> +static CPAccessResult access_tpm(CPUARMState *env, const ARMCPRegInfo *ri,
> +                                 bool isread)
> +{
> +    int el = arm_current_el(env);
> +
> +    if (el < 2 && (env->cp15.mdcr_el2 & MDCR_TPM)
> +        && !arm_is_secure_below_el3(env)) {
> +        return CP_ACCESS_TRAP_EL2;
> +    }
> +    if (el < 3 && (env->cp15.mdcr_el3 & MDCR_TPM)) {
> +        return CP_ACCESS_TRAP_EL3;
> +    }

Hey Peter,

Why not use else if?

Also, I'll hopefully be able to have another look at the PMU patches
next week. I'll make sure to rebase them on top of this.

> +    return CP_ACCESS_OK;
> +}
> +
>  static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>  {
>      ARMCPU *cpu = arm_env_get_cpu(env);
> @@ -774,11 +792,22 @@ static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
>                                     bool isread)
>  {
>      /* Performance monitor registers user accessibility is controlled
> -     * by PMUSERENR.
> +     * by PMUSERENR. MDCR_EL2.TPM and MDCR_EL3.TPM allow configurable
> +     * trapping to EL2 or EL3 for other accesses.
>       */
> -    if (arm_current_el(env) == 0 && !env->cp15.c9_pmuserenr) {
> +    int el = arm_current_el(env);
> +
> +    if (el == 0 && !env->cp15.c9_pmuserenr) {
>          return CP_ACCESS_TRAP;
>      }
> +    if (el < 2 && (env->cp15.mdcr_el2 & MDCR_TPM)
> +        && !arm_is_secure_below_el3(env)) {
> +        return CP_ACCESS_TRAP_EL2;
> +    }
> +    if (el < 3 && (env->cp15.mdcr_el3 & MDCR_TPM)) {
> +        return CP_ACCESS_TRAP_EL3;
> +    }

Same here

Thanks,

Alistair

> +
>      return CP_ACCESS_OK;
>  }
>
> @@ -1101,28 +1130,28 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
>        .accessfn = pmreg_access },
>      { .name = "PMUSERENR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 0,
> -      .access = PL0_R | PL1_RW,
> +      .access = PL0_R | PL1_RW, .accessfn = access_tpm,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
>        .resetvalue = 0,
>        .writefn = pmuserenr_write, .raw_writefn = raw_write },
>      { .name = "PMUSERENR_EL0", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 14, .opc2 = 0,
> -      .access = PL0_R | PL1_RW, .type = ARM_CP_ALIAS,
> +      .access = PL0_R | PL1_RW, .accessfn = access_tpm, .type = ARM_CP_ALIAS,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
>        .resetvalue = 0,
>        .writefn = pmuserenr_write, .raw_writefn = raw_write },
>      { .name = "PMINTENSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 1,
> -      .access = PL1_RW,
> +      .access = PL1_RW, .accessfn = access_tpm,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
>        .resetvalue = 0,
>        .writefn = pmintenset_write, .raw_writefn = raw_write },
>      { .name = "PMINTENCLR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 2,
> -      .access = PL1_RW, .type = ARM_CP_ALIAS,
> +      .access = PL1_RW, .accessfn = access_tpm, .type = ARM_CP_ALIAS,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
>        .writefn = pmintenclr_write, },
>      { .name = "PMINTENCLR_EL1", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 2,
> -      .access = PL1_RW, .type = ARM_CP_ALIAS,
> +      .access = PL1_RW, .accessfn = access_tpm, .type = ARM_CP_ALIAS,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
>        .writefn = pmintenclr_write },
>      { .name = "VBAR", .state = ARM_CP_STATE_BOTH,
> --
> 1.9.1
>
>

Re: [Qemu-devel] [PATCH v2 2/2] target-arm: Implement MDCR_EL3.TPM and MDCR_EL2.TPM traps

[Peter Maydell]

On 19 February 2016 at 19:38, Alistair Francis <alistair23@gmail.com> wrote:
> On Fri, Feb 19, 2016 at 6:39 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> +/* Check for traps to performance monitor registers, which are controlled
>> + * by MDCR_EL2.TPM for EL2 and MDCR_EL3.TPM for EL3.
>> + */
>> +static CPAccessResult access_tpm(CPUARMState *env, const ARMCPRegInfo *ri,
>> +                                 bool isread)
>> +{
>> +    int el = arm_current_el(env);
>> +
>> +    if (el < 2 && (env->cp15.mdcr_el2 & MDCR_TPM)
>> +        && !arm_is_secure_below_el3(env)) {
>> +        return CP_ACCESS_TRAP_EL2;
>> +    }
>> +    if (el < 3 && (env->cp15.mdcr_el3 & MDCR_TPM)) {
>> +        return CP_ACCESS_TRAP_EL3;
>> +    }
>
> Hey Peter,
>
> Why not use else if?

I generally tend not to use else-if ladders if the thing
in the conditional returns unconditionally, just as a
personal style preference. "if () { X } else if () { Y } Z"
implies a possible control flow path of "take the if
branch so run X, then skip Y, and continue after to run Z",
and if X returns unconditionally that can't happen.
It also matches up with the usual approach of
   if (something) {
       early return;
   }
   main body of function;

which you wouldn't want to write as
   if (something) {
      early return;
   } else {
      main body;
   }

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 2/2] target-arm: Implement MDCR_EL3.TPM and MDCR_EL2.TPM traps

[Alistair Francis]

On Sat, Feb 20, 2016 at 3:28 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 19 February 2016 at 19:38, Alistair Francis <alistair23@gmail.com> wrote:
>> On Fri, Feb 19, 2016 at 6:39 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> +/* Check for traps to performance monitor registers, which are controlled
>>> + * by MDCR_EL2.TPM for EL2 and MDCR_EL3.TPM for EL3.
>>> + */
>>> +static CPAccessResult access_tpm(CPUARMState *env, const ARMCPRegInfo *ri,
>>> +                                 bool isread)
>>> +{
>>> +    int el = arm_current_el(env);
>>> +
>>> +    if (el < 2 && (env->cp15.mdcr_el2 & MDCR_TPM)
>>> +        && !arm_is_secure_below_el3(env)) {
>>> +        return CP_ACCESS_TRAP_EL2;
>>> +    }
>>> +    if (el < 3 && (env->cp15.mdcr_el3 & MDCR_TPM)) {
>>> +        return CP_ACCESS_TRAP_EL3;
>>> +    }
>>
>> Hey Peter,
>>
>> Why not use else if?
>
> I generally tend not to use else-if ladders if the thing
> in the conditional returns unconditionally, just as a
> personal style preference. "if () { X } else if () { Y } Z"
> implies a possible control flow path of "take the if
> branch so run X, then skip Y, and continue after to run Z",
> and if X returns unconditionally that can't happen.
> It also matches up with the usual approach of
>    if (something) {
>        early return;
>    }
>    main body of function;
>
> which you wouldn't want to write as
>    if (something) {
>       early return;
>    } else {
>       main body;
>    }

Fair point. I always thought of it the other way around. If you can't
go into the other branches then it should be if () { return } else if
() { return } to make it more explict.

It doesn't matter though.

Acked-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

>
> thanks
> -- PMM