diff mbox

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

Message ID 1455892784-11328-3-git-send-email-peter.maydell@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Maydell Feb. 19, 2016, 2:39 p.m. UTC 
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(-)

Comments

Sergey Fedorov Feb. 19, 2016, 4:42 p.m. UTC | #1 
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,
Alistair Francis Feb. 19, 2016, 7:38 p.m. UTC | #2 
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
>
>
Peter Maydell Feb. 20, 2016, 11:28 a.m. UTC | #3 
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
Alistair Francis Feb. 22, 2016, 6:06 p.m. UTC | #4 
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
diff mbox

Patch

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,