| Message ID | 1455892784-11328-3-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
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,
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 > >
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
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 --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,
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(-)