target-arm: Extend PMCCNTR for ARMv8
v2 cover target-arm: Extend PMCCNTR for ARMv8
This patch series continues on from my original PMCCNTR patch work to extend the counter to be 64-bit and support for the PMCCFILTR_EL0 register which allows the counter to be disabled based on the current EL
V2:
-Fix some typos identified by Christopher Covington -Convert the CCNT_ENABLED macro to the arm_ccnt_enabled
function
Alistair Francis (7):
target-arm: Make the ARM PMCCNTR register 64-bit
target-arm: Implement PMCCNTR_EL0 and related registers
target-arm: Add arm_ccnt_enabled function
target-arm: Implement pmccntr_sync function
target-arm: Remove old code and replace with new functions
target-arm: Implement pmccfiltr_write function
target-arm: Call the pmccntr_sync function when swapping ELs
target-arm/cpu.h | 14 ++++- target-arm/helper-a64.c | 5 ++ target-arm/helper.c | 150 ++++++++++++++++++++++++++++++++++++++--------- target-arm/op_helper.c | 6 ++ 4 files changed, 146 insertions(+), 29 deletions(-)
PATCH v2 1/7 target-arm: Make the ARM PMCCNTR register 64-bit
This makes the PMCCNTR register 64-bit to allow for the 64-bit ARMv8 version
---
target-arm/cpu.h | 2 +- target-arm/helper.c | 19 +++++++++---------- 2 files changed, 10 insertions(+), 11 deletions(-)
target-arm/cpu.h +1 -1
@@ -223,7 +223,7 @@ typedef struct CPUARMState {/* If the counter is enabled, this stores the last time the counter * was reset. Otherwise it stores the counter value */ -uint32_t c15_ccnt; +uint64_t c15_ccnt; } cp15;struct {
target-arm/helper.c +9 -10
@@ -550,11 +550,10 @@ static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri)static void pmcr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { -/* Don't computer the number of ticks in user mode */ -uint32_t temp_ticks; +uint64_t temp_ticks; - temp_ticks =qemu_clock_get_us (QEMU_CLOCK_VIRTUAL ) * -get_ticks_per_sec () /1000000 ; + temp_ticks =muldiv64 (qemu_clock_get_us (QEMU_CLOCK_VIRTUAL ), +get_ticks_per_sec (),1000000 );if (env->cp15.c9_pmcr &PMCRE ) {/* If the counter is enabled */
@@ -586,15 +585,15 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,static uint64_t pmccntr_read (CPUARMState *env,const ARMCPRegInfo *ri) { -uint32_t total_ticks; +uint64_t total_ticks;if (!(env->cp15.c9_pmcr &PMCRE )) {/* Counter is disabled, do not change value */ return env->cp15.c15_ccnt; } - total_ticks =qemu_clock_get_us (QEMU_CLOCK_VIRTUAL ) * -get_ticks_per_sec () /1000000 ; + total_ticks =muldiv64 (qemu_clock_get_us (QEMU_CLOCK_VIRTUAL ), +get_ticks_per_sec (),1000000 );if (env->cp15.c9_pmcr &PMCRD ) {/* Increment once every 64 processor clock cycles */
@@ -606,7 +605,7 @@ static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)static void pmccntr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { -uint32_t total_ticks; +uint64_t total_ticks;if (!(env->cp15.c9_pmcr &PMCRE )) {/* Counter is disabled, set the absolute value */
@@ -614,8 +613,8 @@ static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,return ; } - total_ticks =qemu_clock_get_us (QEMU_CLOCK_VIRTUAL ) * -get_ticks_per_sec () /1000000 ; + total_ticks =muldiv64 (qemu_clock_get_us (QEMU_CLOCK_VIRTUAL ), +get_ticks_per_sec (),1000000 );if (env->cp15.c9_pmcr &PMCRD ) {/* Increment once every 64 processor clock cycles */
Signature · 1 line
1.7.1
PATCH v2 2/7 target-arm: Implement PMCCNTR_EL0 and related registers
In reply to Alistair FrancisThis patch adds support for the ARMv8 version of the PMCCNTR and related registers. It also starts to implement the PMCCFILTR_EL0 register.
---
target-arm/cpu.h | 1 + target-arm/helper.c | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 0 deletions(-)
target-arm/cpu.h +1 -0
@@ -224,6 +224,7 @@ typedef struct CPUARMState {* was reset. Otherwise it stores the counter value */ uint64_t c15_ccnt; +uint64_t pmccfiltr_el0;/* Performance Monitor Filter Register */ } cp15;struct {
target-arm/helper.c +39 -0
@@ -738,7 +738,22 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .writefn = pmcntenset_write, .accessfn = pmreg_access, .raw_writefn = raw_write }, + { .name = "PMCNTENSET_EL0", .crn = 9, .crm = 12, .opc0 = 3, .opc1 = 3, + .opc2 = 1, .access = PL0_RW, .resetvalue = 0, + .state = ARM_CP_STATE_AA64, + .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten), + .writefn = pmcntenset_write, + .accessfn = pmreg_access, + .raw_writefn = raw_write }, { .name = "PMCNTENCLR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 2, + .access = PL0_RW, + .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten), + .accessfn = pmreg_access, + .writefn = pmcntenclr_write, + .type = ARM_CP_NO_MIGRATE }, + { .name = "PMCNTENCLR_EL0", .crn = 9, .crm = 12, .opc0 = 3, .opc1 = 3, + .opc2 = 2, + .state = ARM_CP_STATE_AA64, .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten), .accessfn = pmreg_access, .writefn = pmcntenclr_write,
@@ -762,11 +777,25 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO, .readfn = pmccntr_read, .writefn = pmccntr_write, .accessfn = pmreg_access }, + { .name = "PMCCNTR_EL0", .cp = 15, .crn = 9, .crm = 13, .opc1 = 3, + .opc2 = 0, + .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO, + .state = ARM_CP_STATE_AA64, + .readfn = pmccntr_read, .writefn = pmccntr_write, + .accessfn = pmreg_access }, #endif + { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64, + .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7, + .access = PL0_RW, .accessfn = pmreg_access, + .state = ARM_CP_STATE_AA64, + .resetvalue = 0, + .type = ARM_CP_IO, + .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0), }, { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1, .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper), .accessfn = pmreg_access, .writefn = pmxevtyper_write, + .resetvalue = 0, .raw_writefn = raw_write }, /* Unimplemented, RAZ/WI. */ { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2,
@@ -2324,6 +2353,16 @@ void register_cp_regs_for_features(ARMCPU *cpu) .raw_writefn = raw_write, };define_one_arm_cp_reg (cpu, &pmcr); +ARMCPRegInfo pmcr64 = { + .name ="PMCR_EL0" , .crn =9 , .crm =12 , .opc0 =3 , .opc1 =3 , + .opc2 =0 , + .state =ARM_CP_STATE_AA64 , + .access =PL0_RW , .resetvalue = cpu->midr &0xff000000 , + .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr), + .accessfn = pmreg_access, .writefn = pmcr_write, + .raw_writefn = raw_write, + }; +define_one_arm_cp_reg (cpu, &pmcr64);#endif ARMCPRegInfo clidr = { .name ="CLIDR" , .state =ARM_CP_STATE_BOTH ,
Signature · 1 line
1.7.1
Re: [Qemu-devel] [PATCH v2 2/7] target-arm: Implement PMCCNTR_EL0 and related registers
Quoted 29 lines · Alistair Francis
This patch adds support for the ARMv8 version of the PMCCNTR and related registers. It also starts to implement the
PMCCFILTR_EL0register.Signed-off-by Peter Crosthwaite <[email protected]>Signed-off-by Alistair Francis <[email protected]>---
target-arm/cpu.h | 1 + target-arm/helper.c | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 0 deletions(-)target-arm/cpu.h +1
-0@@ -224,6 +224,7 @@ typedef struct CPUARMState {* was reset. Otherwise it stores the counter value */ uint64_t c15_ccnt; +uint64_t pmccfiltr_el0;/* Performance Monitor Filter Register */ } cp15;struct {target-arm/helper.c +0
-0
target-arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 744
Quoted 7 lines
target-arm/helper.c +4
-0@@ -738,7 +738,22 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .writefn = pmcntenset_write, .accessfn = pmreg_access, .raw_writefn = raw_write }, + { .name = "PMCNTENSET_EL0", .crn = 9, .crm = 12, .opc0 = 3, .opc1 = 3, + .opc2 = 1, .access = PL0_RW, .resetvalue = 0, + .state = ARM_CP_STATE_AA64, + .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
fieldoffset for an AArch64 register has to point at a uint64_t; this is going to read the state next to cp15.c9_pmcnten as well, resulting in garbage in half the register. You need to widen that field to uint64_t and change the AArch32 accessor to use offsetoflow32().
target-arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 747
Quoted 3 lines
target-arm/helper.c +3
-0+ .writefn = pmcntenset_write, + .accessfn = pmreg_access, + .raw_writefn = raw_write },
This is a pretty random order for the fields in a reginfo struct (though existing code is not great here either). Preferred is to put the .name first, then .state, then the encoding in the same order as the v8 ARM ARM:
.cp [if needed], .opc0, .opc1, .crn, .crm, .opc2
then .access and .accessfn
then .fieldoffset and .resetvalue, then read and writefns.
Quoted 13 lines
target-arm/helper.c +8
-0{ .name = "PMCNTENCLR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 2, + .access = PL0_RW, + .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten), + .accessfn = pmreg_access, + .writefn = pmcntenclr_write, + .type = ARM_CP_NO_MIGRATE }, + { .name = "PMCNTENCLR_EL0", .crn = 9, .crm = 12, .opc0 = 3, .opc1 = 3, + .opc2 = 2, + .state = ARM_CP_STATE_AA64, .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten), .accessfn = pmreg_access, .writefn = pmcntenclr_write,
target-arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 785
Quoted 9 lines
target-arm/helper.c +6
-0@@ -762,11 +777,25 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO, .readfn = pmccntr_read, .writefn = pmccntr_write, .accessfn = pmreg_access }, + { .name = "PMCCNTR_EL0", .cp = 15, .crn = 9, .crm = 13, .opc1 = 3, + .opc2 = 0, + .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO, + .state = ARM_CP_STATE_AA64, + .readfn = pmccntr_read, .writefn = pmccntr_write, + .accessfn = pmreg_access },
The AArch32 reginfo now needs a NO_MIGRATE marker, or we'll migrate the underlying state twice.
You don't need to specify a .cp for a STATE_AA64 only reginfo.
The ARM ARM says the semantics of a 32 bit write to PMCCNTR are to write the lower 32 bits and not touch the high bits. I suspect your code will zero them instead. (Maybe this should have been a comment on patch 1...)
target-arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 798
Quoted 13 lines
target-arm/helper.c +8
-0#endif + { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64, + .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7, + .access = PL0_RW, .accessfn = pmreg_access, + .state = ARM_CP_STATE_AA64, + .resetvalue = 0, + .type = ARM_CP_IO, + .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0), }, { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1, .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper), .accessfn = pmreg_access, .writefn = pmxevtyper_write, + .resetvalue = 0,
This seems like a stray unnecessary change.
Quoted 6 lines
target-arm/helper.c +0
-0.raw_writefn = raw_write },/* Unimplemented, RAZ/WI. */ { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2,@@ -2324,6 +2353,16 @@ void register_cp_regs_for_features(ARMCPU *cpu) .raw_writefn = raw_write, };
target-arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 2364
Quoted 10 lines
target-arm/helper.c +9
-0define_one_arm_cp_reg (cpu, &pmcr); +ARMCPRegInfo pmcr64 = { + .name ="PMCR_EL0" , .crn =9 , .crm =12 , .opc0 =3 , .opc1 =3 , + .opc2 =0 , + .state =ARM_CP_STATE_AA64 , + .access =PL0_RW , .resetvalue = cpu->midr &0xff000000 , + .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr), + .accessfn = pmreg_access, .writefn = pmcr_write, + .raw_writefn = raw_write, + };
Don't put variable definitions in the middle of code blocks, please.
Same remarks as above about need for NO_MIGRATE and need for widening a uint32_t field apply here.
Quoted 7 lines
target-arm/helper.c +1
-1+define_one_arm_cp_reg (cpu, &pmcr64);#endif ARMCPRegInfo clidr = { .name ="CLIDR" , .state =ARM_CP_STATE_BOTH , --1.7.1
thanks
-- PMM
Re: [Qemu-devel] [PATCH v2 2/7] target-arm: Implement PMCCNTR_EL0 and related registers
Quoted 35 lines · Peter Maydell
Quoted 34 lines · Alistair Francis
This patch adds support for the ARMv8 version of the PMCCNTR and related registers. It also starts to implement the
PMCCFILTR_EL0register.Signed-off-by Peter Crosthwaite <[email protected]>Signed-off-by Alistair Francis <[email protected]>---
target-arm/cpu.h | 1 + target-arm/helper.c | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 0 deletions(-)target-arm/cpu.h +1
-0@@ -224,6 +224,7 @@ typedef struct CPUARMState {* was reset. Otherwise it stores the counter value */ uint64_t c15_ccnt; +uint64_t pmccfiltr_el0;/* Performance Monitor Filter Register */ } cp15;struct {target-arm/helper.c +2
-0@@ -738,7 +738,22 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .writefn = pmcntenset_write, .accessfn = pmreg_access, .raw_writefn = raw_write }, + { .name = "PMCNTENSET_EL0", .crn = 9, .crm = 12, .opc0 = 3, .opc1 = 3, + .opc2 = 1, .access = PL0_RW, .resetvalue = 0,
Quoted 9 lines
target-arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 744
Quoted 2 lines
target-arm/helper.c +2
-0+ .state = ARM_CP_STATE_AA64, + .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),fieldoffset for an AArch64 register has to point at a
uint64_t; this is going to read the state next to cp15.c9_pmcnten as well, resulting in garbage in half the register. You need to widen that field touint64_tand change the AArch32 accessor to useoffsetoflow32().
Fixed.
Quoted 4 lines
target-arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 747
Quoted 3 lines
target-arm/helper.c +3
-0+ .writefn = pmcntenset_write, + .accessfn = pmreg_access, + .raw_writefn = raw_write },
Quoted 8 lines
This is a pretty random order for the fields in a reginfo struct (though existing code is not great here either). Preferred is to put the .name first, then .state, then the encoding in the same order as the v8 ARM ARM:
.cp [if needed], .opc0, .opc1, .crn, .crm, .opc2then .access and .accessfn
then .fieldoffset and .resetvalue, then read and writefns.
Done. I like the new scheme:
792 { .name = "PMCNTENSET_EL0", .state = ARM_CP_STATE_AA64,
793 .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12,.opc2 = 1, 794 .access = PL0_RW, .accessfn = pmreg_access, 795 .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
.resetvalue = 0,
796 .writefn = pmcntenset_write, .raw_writefn = raw_write },
For completeness where does .type fit in?
Quoted 17 lines
Quoted 17 lines
target-arm/helper.c +9
-0{ .name = "PMCNTENCLR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 2, + .access = PL0_RW, + .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten), + .accessfn = pmreg_access, + .writefn = pmcntenclr_write, + .type = ARM_CP_NO_MIGRATE }, + { .name = "PMCNTENCLR_EL0", .crn = 9, .crm = 12, .opc0 = 3, .opc1 = 3, + .opc2 = 2, + .state = ARM_CP_STATE_AA64, .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten), .accessfn = pmreg_access, .writefn = pmcntenclr_write,@@ -762,11 +777,25 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO, .readfn = pmccntr_read, .writefn = pmccntr_write, .accessfn = pmreg_access }, + { .name = "PMCCNTR_EL0", .cp = 15, .crn = 9, .crm = 13, .opc1 = 3,
Quoted 9 lines
target-arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 785
Quoted 5 lines
target-arm/helper.c +5
-0+ .opc2 = 0, + .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO, + .state = ARM_CP_STATE_AA64, + .readfn = pmccntr_read, .writefn = pmccntr_write, + .accessfn = pmreg_access },The AArch32 reginfo now needs a
NO_MIGRATEmarker, or we'll migrate the underlying state twice.
Yep. And ditto for CNTENSET I guess.
Quoted 7 lines
You don't need to specify a .cp for a
STATE_AA64only reginfo.The ARM ARM says the semantics of a 32 bit write to PMCCNTR are to write the lower 32 bits and not touch the high bits. I suspect your code will zero them instead. (Maybe this should have been a comment on patch 1...)
Hmm slightly tricky. Added the RMW logic to do this.
Quoted 14 lines
target-arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 798
Quoted 13 lines
target-arm/helper.c +8
-0#endif + { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64, + .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7, + .access = PL0_RW, .accessfn = pmreg_access, + .state = ARM_CP_STATE_AA64, + .resetvalue = 0, + .type = ARM_CP_IO, + .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0), }, { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1, .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper), .accessfn = pmreg_access, .writefn = pmxevtyper_write, + .resetvalue = 0,
Quoted 2 lines
This seems like a stray unnecessary change.
Dropped.
Quoted 10 lines
Quoted 10 lines
target-arm/helper.c +3
-0.raw_writefn = raw_write },/* Unimplemented, RAZ/WI. */ { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2,@@ -2324,6 +2353,16 @@ void register_cp_regs_for_features(ARMCPU *cpu) .raw_writefn = raw_write, };define_one_arm_cp_reg (cpu, &pmcr); +ARMCPRegInfo pmcr64 = { + .name ="PMCR_EL0" , .crn =9 , .crm =12 , .opc0 =3 , .opc1 =3 , + .opc2 =0 ,
Quoted 9 lines
target-arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 2364
Quoted 6 lines
target-arm/helper.c +6
-0+ .state = ARM_CP_STATE_AA64, + .access = PL0_RW, .resetvalue = cpu->midr & 0xff000000, + .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr), + .accessfn = pmreg_access, .writefn = pmcr_write, + .raw_writefn = raw_write, + };Don't put variable definitions in the middle of code blocks, please.
Fixed. Although we have that problem from CLIDR in the context below.
Quoted 3 lines
Same remarks as above about need for
NO_MIGRATEand need for widening auint32_tfield apply here.
Fixed.
Thanks,
Regards,
Peter
Quoted 11 lines
Quoted 7 lines
target-arm/helper.c +1
-1+define_one_arm_cp_reg (cpu, &pmcr64);#endif ARMCPRegInfo clidr = { .name ="CLIDR" , .state =ARM_CP_STATE_BOTH , --1.7.1
thanks
-- PMM
Re: [Qemu-devel] [PATCH v2 2/7] target-arm: Implement PMCCNTR_EL0 and related registers
Quoted 12 lines · Peter Crosthwaite
Quoted 8 lines · Peter Maydell
This is a pretty random order for the fields in a reginfo struct (though existing code is not great here either). Preferred is to put the .name first, then .state, then the encoding in the same order as the v8 ARM ARM:
.cp [if needed], .opc0, .opc1, .crn, .crm, .opc2then .access and .accessfn
then .fieldoffset and .resetvalue, then read and writefns.Done. I like the new scheme:
Quoted 8 lines
792 { .name = "
PMCNTENSET_EL0", .state =ARM_CP_STATE_AA64,793 .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12,.opc2 = 1, 794 .access = PL0_RW, .accessfn = pmreg_access, 795 .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),.resetvalue = 0,
796 .writefn =pmcntenset_write, .raw_writefn =raw_write},For completeness where does .type fit in?
After .access and .accessfn, I think. (There's also a fair amount of existing code which puts it just before those two, which doesn't look too awful, but after is slightly more logical.)
-- PMM
PATCH v2 3/7 target-arm: Add arm_ccnt_enabled function
In reply to Alistair FrancisInclude a helper function to determine if the CCNT counter is enabled as well as the constants used to mask the pmccfiltr_el0 and c9_pmxevtyper registers.
---
target-arm/helper.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 files changed, 40 insertions(+), 0 deletions(-)
target-arm/helper.c +40 -0
@@ -547,6 +547,46 @@ static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri) }#ifndef CONFIG_USER_ONLY +#define PMCCFILTR_NSH 0x8000000 +#define PMCCFILTR_P 0x80000000 +#define PMCCFILTR_U 0x40000000 + +#define PMXEVTYPER_P 0x80000000 +#define PMXEVTYPER_U 0x40000000 + +static bool arm_ccnt_enabled (CPUARMState *env) +{ +/* This does not support checking for the secure/non-secure +* components of the PMCCFILTR_EL0 register +*/ + +if (!(env->cp15.c9_pmcr &PMCRE )) { +return 0 ; + } + +if (arm_current_pl (env) ==2 ) { +if (!(env->cp15.pmccfiltr_el0 &PMCCFILTR_NSH )) { +return 0 ; + } + }else if (arm_current_pl (env) ==1 ) { +if (env->cp15.pmccfiltr_el0 &PMCCFILTR_P ) { +return 0 ; + }else if (env->cp15.c9_pmxevtyper &PMXEVTYPER_P ) { +return 0 ; + } + }else if (arm_current_pl (env) ==0 ) { +if (env->cp15.pmccfiltr_el0 &PMCCFILTR_U ) { +return 0 ; + }else if (env->cp15.c9_pmxevtyper &PMXEVTYPER_U ) { +return 0 ; + } + } + +return 1 ; +} +#endif + +#ifndef CONFIG_USER_ONLY static void pmcr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) {
Signature · 1 line
1.7.1
Re: [Qemu-devel] [PATCH v2 3/7] target-arm: Add arm_ccnt_enabled function
Quoted 15 lines · Alistair Francis
Include a helper function to determine if the CCNT counter is enabled as well as the constants used to mask the
pmccfiltr_el0andc9_pmxevtyperregisters.Signed-off-by Alistair Francis <[email protected]>---
target-arm/helper.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 files changed, 40 insertions(+), 0 deletions(-)target-arm/helper.c +0
-0
target-arm/helper.c static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri) · line 549
Quoted 3 lines
target-arm/helper.c +0
-0@@ -547,6 +547,46 @@ static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri) }#ifndef CONFIG_USER_ONLY
Blank line here for readability.
Quoted 8 lines
target-arm/helper.c +8
-0+#define PMCCFILTR_NSH 0x8000000 +#define PMCCFILTR_P 0x80000000 +#define PMCCFILTR_U 0x40000000 + +#define PMXEVTYPER_P 0x80000000 +#define PMXEVTYPER_U 0x40000000 + +static bool arm_ccnt_enabled (CPUARMState *env)
target-arm/helper.c static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri) · line 567
Quoted 10 lines
target-arm/helper.c +10
-0+{ +/* This does not support checking for the secure/non-secure +* components of the PMCCFILTR_EL0 register +*/ + +if (!(env->cp15.c9_pmcr &PMCRE )) { +return 0 ; + } + +if (arm_current_pl (env) ==2 ) {
switch(arm_current_pl(env))
target-arm/helper.c static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri) · line 569
Quoted 2 lines
target-arm/helper.c +2
-0+if (!(env->cp15.pmccfiltr_el0 &PMCCFILTR_NSH )) { +return 0 ;
Use "true" and "false" for boolean function return values.
target-arm/helper.c static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri) · line 574
Quoted 5 lines
target-arm/helper.c +5
-0+ } + }else if (arm_current_pl (env) ==1 ) { +if (env->cp15.pmccfiltr_el0 &PMCCFILTR_P ) { +return 0 ; + }else if (env->cp15.c9_pmxevtyper &PMXEVTYPER_P ) {
use an || to merge the two if branches with same function body.
Quoted 5 lines
target-arm/helper.c +5
-0+return 0 ; + } + }else if (arm_current_pl (env) ==0 ) { +if (env->cp15.pmccfiltr_el0 &PMCCFILTR_U ) { +return 0 ;
target-arm/helper.c static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri) · line 589
Quoted 10 lines
target-arm/helper.c +10
-0+ } else if (env->cp15.c9_pmxevtyper & PMXEVTYPER_U) { +return 0 ; + } + } + +return 1 ; +} +#endif + +#ifndef CONFIG_USER_ONLY
Just drop the extra #endif #ifndef CONFIG_USER_ONLY.
Regards,
Peter
Quoted 7 lines
target-arm/helper.c +0
-1static void pmcr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { --1.7.1
Re: [Qemu-devel] [PATCH v2 3/7] target-arm: Add arm_ccnt_enabled function
Quoted 66 lines · Peter Crosthwaite
Quoted 15 lines · Alistair Francis
Include a helper function to determine if the CCNT counter is enabled as well as the constants used to mask the
pmccfiltr_el0andc9_pmxevtyperregisters.Signed-off-by Alistair Francis <[email protected]>---
target-arm/helper.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 files changed, 40 insertions(+), 0 deletions(-)target-arm/helper.c +0
-0target-arm/helper.c static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri) · line 549
Quoted 3 lines
target-arm/helper.c +0
-0@@ -547,6 +547,46 @@ static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri) }#ifndef CONFIG_USER_ONLY Blank line here for readability.
Quoted 8 lines
target-arm/helper.c +8
-0+#define PMCCFILTR_NSH 0x8000000 +#define PMCCFILTR_P 0x80000000 +#define PMCCFILTR_U 0x40000000 + +#define PMXEVTYPER_P 0x80000000 +#define PMXEVTYPER_U 0x40000000 + +static bool arm_ccnt_enabled (CPUARMState *env)target-arm/helper.c static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri) · line 567
Quoted 10 lines
target-arm/helper.c +10
-0+{ +/* This does not support checking for the secure/non-secure +* components of the PMCCFILTR_EL0 register +*/ + +if (!(env->cp15.c9_pmcr &PMCRE )) { +return 0 ; + } + +if (arm_current_pl (env) ==2 ) {switch(arm_current_pl(env))
target-arm/helper.c static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri) · line 569
Quoted 2 lines
target-arm/helper.c +2
-0+if (!(env->cp15.pmccfiltr_el0 &PMCCFILTR_NSH )) { +return 0 ;Use "true" and "false" for boolean function return values.
target-arm/helper.c static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri) · line 574
Quoted 5 lines
target-arm/helper.c +5
-0+ } + }else if (arm_current_pl (env) ==1 ) { +if (env->cp15.pmccfiltr_el0 &PMCCFILTR_P ) { +return 0 ; + }else if (env->cp15.c9_pmxevtyper &PMXEVTYPER_P ) {use an || to merge the two if branches with same function body.
Quoted 9 lines
target-arm/helper.c +9
-0+return 0 ; + } + }else if (arm_current_pl (env) ==0 ) { +if (env->cp15.pmccfiltr_el0 &PMCCFILTR_U ) { +return 0 ; + }else if (env->cp15.c9_pmxevtyper &PMXEVTYPER_U ) { +return 0 ; + } + }
Quoted 9 lines
target-arm/helper.c static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri) · line 589
Quoted 6 lines
target-arm/helper.c +6
-0+ + return 1; +} +#endif + +#ifndef CONFIG_USER_ONLY Just drop the extra #endif #ifndef
CONFIG_USER_ONLY.
All fixed (in a respun version I got from Alistair).
Regards,
Peter
Quoted 10 lines
Regards,
PeterQuoted 7 lines
target-arm/helper.c +0
-1static void pmcr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { --1.7.1
PATCH v2 4/7 target-arm: Implement pmccntr_sync function
In reply to Alistair FrancisThis is used to synchronise the PMCCNTR counter and swap its state between enabled and disabled if required. It must always be called twice, both before and after any logic that could change the state of the PMCCNTR counter.
---
Remembering that the c15_ccnt register stores the last time the counter was reset if enabled. If disabled it stores the counter value (when it was disabled).
The three use cases are as below:
--
Starts enabled/disabled and is staying enabled/disabled --
The two calls to pmccntr_sync cancel each other out. Each call implements this logic:env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt
Which expands to:env->cp15.c15_ccnt = temp_ticks -
(temp_ticks - env->cp15.c15_ccnt)
env->cp15.c15_ccnt = env->cp15.c15_ccnt
--
Starts enabled, gets disabled
--
The logic is run during the first call while during the second call it is not. That means that c15_ccnt changes from storing the last time the counter was reset, to storing the absolute value of the counter.
--
Starts disabled, gets enabled
--
During the fist call no changes are made, while during the second call the register is changed. This changes it from storing the absolute value to storing the last time the counter was reset.
target-arm/cpu.h | 11 +++++++++++ target-arm/helper.c | 19 +++++++++++++++++++ 2 files changed, 30 insertions(+), 0 deletions(-)
target-arm/cpu.h +11 -0
@@ -1032,6 +1032,17 @@ static inline bool cp_access_ok(int current_pl, }/** +* pmccntr_sync +* @cpu: ARMCPU +* +* Syncronises the counter in the PMCCNTR. This must always be called twice, +* once before any action that might effect the timer and again afterwards. +* The function is used to swap the state of the register if required. +* This only happens when not in user mode (!CONFIG_USER_ONLY) +*/ +void pmccntr_sync (CPUARMState *env); + +/** * write_list_to_cpustate * @cpu: ARMCPU *
target-arm/helper.c +19 -0
@@ -586,6 +586,25 @@ static bool arm_ccnt_enabled(CPUARMState *env) }#endif +void pmccntr_sync (CPUARMState *env) +{ +#ifndef CONFIG_USER_ONLY +uint64_t temp_ticks; + + temp_ticks =muldiv64 (qemu_clock_get_us (QEMU_CLOCK_VIRTUAL ), +get_ticks_per_sec (),1000000 ); + +if (env->cp15.c9_pmcr &PMCRD ) { +/* Increment once every 64 processor clock cycles */ + temp_ticks /=64 ; + } + +if (arm_ccnt_enabled (env)) { + env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt; + } +#endif +} +#ifndef CONFIG_USER_ONLY static void pmcr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value)
Signature · 1 line
1.7.1
PATCH v2 5/7 target-arm: Remove old code and replace with new functions
In reply to Alistair FrancisRemove the old PMCCNTR code and replace it with calls to the new pmccntr_sync() function and the CCNT_ENABLED macro
---
target-arm/helper.c | 27 ++++----------------------- 1 files changed, 4 insertions(+), 23 deletions(-)
target-arm/helper.c +4 -23
@@ -609,20 +609,7 @@ void pmccntr_sync(CPUARMState *env)static void pmcr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { -uint64_t temp_ticks; - - temp_ticks =muldiv64 (qemu_clock_get_us (QEMU_CLOCK_VIRTUAL ), -get_ticks_per_sec (),1000000 ); - -if (env->cp15.c9_pmcr &PMCRE ) { -/* If the counter is enabled */ -if (env->cp15.c9_pmcr &PMCRD ) { -/* Increment once every 64 processor clock cycles */ - env->cp15.c15_ccnt = (temp_ticks/64 ) - env->cp15.c15_ccnt; - }else { - env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt; - } - } +pmccntr_sync (env);if (value &PMCRC ) {/* The counter has been reset */
@@ -633,20 +620,14 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri, env->cp15.c9_pmcr &= ~0x39 ; env->cp15.c9_pmcr |= (value &0x39 ); -if (env->cp15.c9_pmcr &PMCRE ) { -if (env->cp15.c9_pmcr &PMCRD ) { -/* Increment once every 64 processor clock cycles */ - temp_ticks /=64 ; - } - env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt; - } +pmccntr_sync (env); }static uint64_t pmccntr_read (CPUARMState *env,const ARMCPRegInfo *ri) {uint64_t total_ticks; -if (!(env->cp15.c9_pmcr &PMCRE )) { +if (!arm_ccnt_enabled (env)) {/* Counter is disabled, do not change value */ return env->cp15.c15_ccnt; }
@@ -666,7 +647,7 @@ static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri, {uint64_t total_ticks; -if (!(env->cp15.c9_pmcr &PMCRE )) { +if (!arm_ccnt_enabled (env)) {/* Counter is disabled, set the absolute value */ env->cp15.c15_ccnt = value;return ;
Signature · 1 line
1.7.1
Re: [Qemu-devel] [PATCH v2 5/7] target-arm: Remove old code and replace with new functions
Quoted 2 lines · Alistair Francis
Remove the old PMCCNTR code and replace it with calls to the new
pmccntr_sync()function and theCCNT_ENABLEDmacro
arm_ccnt_enabled() function.
Quoted 57 lines
Signed-off-by Alistair Francis <[email protected]>---
target-arm/helper.c | 27 ++++----------------------- 1 files changed, 4 insertions(+), 23 deletions(-)target-arm/helper.c +3
-22@@ -609,20 +609,7 @@ void pmccntr_sync(CPUARMState *env)static void pmcr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { -uint64_t temp_ticks; - - temp_ticks =muldiv64 (qemu_clock_get_us (QEMU_CLOCK_VIRTUAL ), -get_ticks_per_sec (),1000000 ); - -if (env->cp15.c9_pmcr &PMCRE ) { -/* If the counter is enabled */ -if (env->cp15.c9_pmcr &PMCRD ) { -/* Increment once every 64 processor clock cycles */ - env->cp15.c15_ccnt = (temp_ticks/64 ) - env->cp15.c15_ccnt; - }else { - env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt; - } - } +pmccntr_sync (env);if (value &PMCRC ) {/* The counter has been reset */ @@ -633,20 +620,14 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri, env->cp15.c9_pmcr &= ~0x39 ; env->cp15.c9_pmcr |= (value &0x39 ); -if (env->cp15.c9_pmcr &PMCRE ) { -if (env->cp15.c9_pmcr &PMCRD ) { -/* Increment once every 64 processor clock cycles */ - temp_ticks /=64 ; - } - env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt; - } +pmccntr_sync (env); }static uint64_t pmccntr_read (CPUARMState *env,const ARMCPRegInfo *ri) {uint64_t total_ticks; -if (!(env->cp15.c9_pmcr &PMCRE )) { +if (!arm_ccnt_enabled (env)) {/* Counter is disabled, do not change value */ return env->cp15.c15_ccnt; }
target-arm/helper.c static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri, · line 650
Quoted 5 lines
target-arm/helper.c +1
-1@@ -666,7 +647,7 @@ static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri, {uint64_t total_ticks; -if (!(env->cp15.c9_pmcr &PMCRE )) { +if (!arm_ccnt_enabled (env)) {
Is it valid to write to the running counter?
If so you can sync(), update sync() to implement easily now and drop the if.
Regards,
Peter
Quoted 7 lines
target-arm/helper.c +0
-1/* Counter is disabled, set the absolute value */ env->cp15.c15_ccnt = value;return ; --1.7.1
Re: [Qemu-devel] [PATCH v2 5/7] target-arm: Remove old code and replace with new functions
Quoted 67 lines · Peter Crosthwaite
Quoted 2 lines · Alistair Francis
Remove the old PMCCNTR code and replace it with calls to the new
pmccntr_sync()function and theCCNT_ENABLEDmacro
arm_ccnt_enabled()function.Quoted 60 lines
Signed-off-by Alistair Francis <[email protected]>---
target-arm/helper.c | 27 ++++----------------------- 1 files changed, 4 insertions(+), 23 deletions(-)target-arm/helper.c +3
-22@@ -609,20 +609,7 @@ void pmccntr_sync(CPUARMState *env)static void pmcr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { -uint64_t temp_ticks; - - temp_ticks =muldiv64 (qemu_clock_get_us (QEMU_CLOCK_VIRTUAL ), -get_ticks_per_sec (),1000000 ); - -if (env->cp15.c9_pmcr &PMCRE ) { -/* If the counter is enabled */ -if (env->cp15.c9_pmcr &PMCRD ) { -/* Increment once every 64 processor clock cycles */ - env->cp15.c15_ccnt = (temp_ticks/64 ) - env->cp15.c15_ccnt; - }else { - env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt; - } - } +pmccntr_sync (env);if (value &PMCRC ) {/* The counter has been reset */ @@ -633,20 +620,14 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri, env->cp15.c9_pmcr &= ~0x39 ; env->cp15.c9_pmcr |= (value &0x39 ); -if (env->cp15.c9_pmcr &PMCRE ) { -if (env->cp15.c9_pmcr &PMCRD ) { -/* Increment once every 64 processor clock cycles */ - temp_ticks /=64 ; - } - env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt; - } +pmccntr_sync (env); }static uint64_t pmccntr_read (CPUARMState *env,const ARMCPRegInfo *ri) {uint64_t total_ticks; -if (!(env->cp15.c9_pmcr &PMCRE )) { +if (!arm_ccnt_enabled (env)) {/* Counter is disabled, do not change value */ return env->cp15.c15_ccnt; }@@ -666,7 +647,7 @@ static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri, {uint64_t total_ticks;
Quoted 4 lines
target-arm/helper.c static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri, · line 650
Quoted 2 lines
target-arm/helper.c +1
-1-if (!(env->cp15.c9_pmcr &PMCRE )) { +if (!arm_ccnt_enabled (env)) {Is it valid to write to the running counter?
It is a RW register, so I'm assuming it can be set
Quoted 2 lines
If so you can
sync(), updatesync()to implement easily now and drop the if.
It's not the same process as the other syncs. This logic includes the value that is being written, which the sync function doesn't. It would require changes to the
sync function, with values to be passed in. It doesn't seem like it's worth doing
Quoted 12 lines
Regards,
PeterQuoted 7 lines
target-arm/helper.c +0
-1/* Counter is disabled, set the absolute value */ env->cp15.c15_ccnt = value;return ; --1.7.1
PATCH v2 6/7 target-arm: Implement pmccfiltr_write function
In reply to Alistair FrancisThis is the function that is called when writing to the PMCCFILTR_EL0 register
---
target-arm/helper.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-)
target-arm/helper.c +9 -0
@@ -664,6 +664,14 @@ static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri, }#endif +static void pmccfiltr_write (CPUARMState *env,const ARMCPRegInfo *ri, +uint64_t value) +{ +pmccntr_sync (env); + env->cp15.pmccfiltr_el0 = value &0x7E000000 ; +pmccntr_sync (env); +} +static void pmcntenset_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) {
@@ -826,6 +834,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {#endif { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7, + .writefn = pmccfiltr_write, .access = PL0_RW, .accessfn = pmreg_access, .state = ARM_CP_STATE_AA64, .resetvalue = 0,
Signature · 1 line
1.7.1
PATCH v2 7/7 target-arm: Call the pmccntr_sync function when swapping ELs
In reply to Alistair FrancisCall the new pmccntr_sync() function when there is a possibility of swapping ELs (I.E. when there is an exception)
---
target-arm/helper-a64.c | 5 +++++ target-arm/helper.c | 7 +++++++ target-arm/op_helper.c | 6 ++++++ 3 files changed, 18 insertions(+), 0 deletions(-)
target-arm/helper-a64.c +5 -0
@@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)target_ulong addr = env->cp15.vbar_el[1 ];int i; +pmccntr_sync (env); +if (arm_current_pl (env) ==0 ) {if (env->aarch64) { addr +=0x400 ;
@@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs) addr +=0x100 ;break ;default : +pmccntr_sync (env);cpu_abort (cs,"Unhandled exception 0x%x\n" , cs->exception_index); }
@@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs) env->pc = addr; cs->interrupt_request |=CPU_INTERRUPT_EXITTB ; + +pmccntr_sync (env); }
target-arm/helper.c +7 -0
@@ -3457,6 +3457,8 @@ void arm_cpu_do_interrupt(CPUState *cs)assert (!IS_M (env)); +pmccntr_sync (env); +arm_log_exception (cs->exception_index);/* TODO: Vectored interrupt controller. */
@@ -3487,6 +3489,7 @@ void arm_cpu_do_interrupt(CPUState *cs) && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR) { env->regs[0 ] =do_arm_semihosting (env);qemu_log_mask (CPU_LOG_INT ,"...handled as semihosting call\n" ); +pmccntr_sync (env);return ; } }
@@ -3505,6 +3508,7 @@ void arm_cpu_do_interrupt(CPUState *cs) env->regs[15 ] +=2 ; env->regs[0 ] =do_arm_semihosting (env);qemu_log_mask (CPU_LOG_INT ,"...handled as semihosting call\n" ); +pmccntr_sync (env);return ; } }
@@ -3548,6 +3552,7 @@ void arm_cpu_do_interrupt(CPUState *cs) offset =4 ;break ;default : +pmccntr_sync (env);cpu_abort (cs,"Unhandled exception 0x%x\n" , cs->exception_index);return ;/* Never happens. Keep compiler happy. */ }
@@ -3580,6 +3585,8 @@ void arm_cpu_do_interrupt(CPUState *cs) env->regs[14 ] = env->regs[15 ] + offset; env->regs[15 ] = addr; cs->interrupt_request |=CPU_INTERRUPT_EXITTB ; + +pmccntr_sync (env); }/* Check section/page access permissions.
target-arm/op_helper.c +6 -0
@@ -376,6 +376,8 @@ void HELPER(exception_return)(CPUARMState *env)uint32_t spsr = env->banked_spsr[spsr_idx];int new_el, i; +pmccntr_sync (env); +if (env->pstate &PSTATE_SP ) { env->sp_el[cur_el] = env->xregs[31 ]; }else {
@@ -418,6 +420,8 @@ void HELPER(exception_return)(CPUARMState *env) env->pc = env->elr_el[cur_el]; } +pmccntr_sync (env); +return ; illegal_return:
@@ -433,6 +437,8 @@ illegal_return: spsr &=PSTATE_NZCV |PSTATE_DAIF ; spsr |=pstate_read (env) & ~(PSTATE_NZCV |PSTATE_DAIF );pstate_write (env, spsr); + +pmccntr_sync (env); }/* ??? Flag setting arithmetic is awkward because we need to do comparisons.
Signature · 1 line
1.7.1
Re: [Qemu-devel] [PATCH v2 7/7] target-arm: Call the pmccntr_sync function when swapping ELs
Quoted 108 lines · Alistair Francis
Call the new
pmccntr_sync()function when there is a possibility of swapping ELs (I.E. when there is an exception)Signed-off-by Alistair Francis <[email protected]>---
target-arm/helper-a64.c | 5 +++++ target-arm/helper.c | 7 +++++++ target-arm/op_helper.c | 6 ++++++ 3 files changed, 18 insertions(+), 0 deletions(-)target-arm/helper-a64.c +5
-0@@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)target_ulong addr = env->cp15.vbar_el[1 ];int i; +pmccntr_sync (env); +if (arm_current_pl (env) ==0 ) {if (env->aarch64) { addr +=0x400 ;@@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs) addr +=0x100 ;break ;default : +pmccntr_sync (env);cpu_abort (cs,"Unhandled exception 0x%x\n" , cs->exception_index); }@@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs) env->pc = addr; cs->interrupt_request |=CPU_INTERRUPT_EXITTB ; + +pmccntr_sync (env); }target-arm/helper.c +7
-0@@ -3457,6 +3457,8 @@ void arm_cpu_do_interrupt(CPUState *cs)assert (!IS_M (env)); +pmccntr_sync (env); +arm_log_exception (cs->exception_index);/* TODO: Vectored interrupt controller. */ @@ -3487,6 +3489,7 @@ void arm_cpu_do_interrupt(CPUState *cs) && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR) { env->regs[0 ] =do_arm_semihosting (env);qemu_log_mask (CPU_LOG_INT ,"...handled as semihosting call\n" ); +pmccntr_sync (env);return ; } }@@ -3505,6 +3508,7 @@ void arm_cpu_do_interrupt(CPUState *cs) env->regs[15 ] +=2 ; env->regs[0 ] =do_arm_semihosting (env);qemu_log_mask (CPU_LOG_INT ,"...handled as semihosting call\n" ); +pmccntr_sync (env);return ; } }@@ -3548,6 +3552,7 @@ void arm_cpu_do_interrupt(CPUState *cs) offset =4 ;break ;default : +pmccntr_sync (env);cpu_abort (cs,"Unhandled exception 0x%x\n" , cs->exception_index);return ;/* Never happens. Keep compiler happy. */ }@@ -3580,6 +3585,8 @@ void arm_cpu_do_interrupt(CPUState *cs) env->regs[14 ] = env->regs[15 ] + offset; env->regs[15 ] = addr; cs->interrupt_request |=CPU_INTERRUPT_EXITTB ; + +pmccntr_sync (env); }/* Check section/page access permissions. target-arm/op_helper.c +4
-0@@ -376,6 +376,8 @@ void HELPER(exception_return)(CPUARMState *env)uint32_t spsr = env->banked_spsr[spsr_idx];int new_el, i; +pmccntr_sync (env); +if (env->pstate &PSTATE_SP ) { env->sp_el[cur_el] = env->xregs[31 ]; }else {@@ -418,6 +420,8 @@ void HELPER(exception_return)(CPUARMState *env) env->pc = env->elr_el[cur_el]; } +pmccntr_sync (env); +return ; illegal_return:
target-arm/op_helper.c illegal_return: · line 442
Quoted 6 lines
target-arm/op_helper.c +2
-0@@ -433,6 +437,8 @@ illegal_return: spsr &=PSTATE_NZCV |PSTATE_DAIF ; spsr |=pstate_read (env) & ~(PSTATE_NZCV |PSTATE_DAIF );pstate_write (env, spsr); + +pmccntr_sync (env); }
This feels way too fragile to me to be scattering these sync calls all over the place like this. We need to find a better approach than this.
thanks
-- PMM
Re: [Qemu-devel] [PATCH v2 7/7] target-arm: Call the pmccntr_sync function when swapping ELs
Quoted 107 lines · Peter Maydell
Quoted 106 lines · Alistair Francis
Call the new
pmccntr_sync()function when there is a possibility of swapping ELs (I.E. when there is an exception)Signed-off-by Alistair Francis <[email protected]>---
target-arm/helper-a64.c | 5 +++++ target-arm/helper.c | 7 +++++++ target-arm/op_helper.c | 6 ++++++ 3 files changed, 18 insertions(+), 0 deletions(-)target-arm/helper-a64.c +5
-0@@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)target_ulong addr = env->cp15.vbar_el[1 ];int i; +pmccntr_sync (env); +if (arm_current_pl (env) ==0 ) {if (env->aarch64) { addr +=0x400 ;@@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs) addr +=0x100 ;break ;default : +pmccntr_sync (env);cpu_abort (cs,"Unhandled exception 0x%x\n" , cs->exception_index); }@@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs) env->pc = addr; cs->interrupt_request |=CPU_INTERRUPT_EXITTB ; + +pmccntr_sync (env); }target-arm/helper.c +7
-0@@ -3457,6 +3457,8 @@ void arm_cpu_do_interrupt(CPUState *cs)assert (!IS_M (env)); +pmccntr_sync (env); +arm_log_exception (cs->exception_index);/* TODO: Vectored interrupt controller. */ @@ -3487,6 +3489,7 @@ void arm_cpu_do_interrupt(CPUState *cs) && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR) { env->regs[0 ] =do_arm_semihosting (env);qemu_log_mask (CPU_LOG_INT ,"...handled as semihosting call\n" ); +pmccntr_sync (env);return ; } }@@ -3505,6 +3508,7 @@ void arm_cpu_do_interrupt(CPUState *cs) env->regs[15 ] +=2 ; env->regs[0 ] =do_arm_semihosting (env);qemu_log_mask (CPU_LOG_INT ,"...handled as semihosting call\n" ); +pmccntr_sync (env);return ; } }@@ -3548,6 +3552,7 @@ void arm_cpu_do_interrupt(CPUState *cs) offset =4 ;break ;default : +pmccntr_sync (env);cpu_abort (cs,"Unhandled exception 0x%x\n" , cs->exception_index);return ;/* Never happens. Keep compiler happy. */ }@@ -3580,6 +3585,8 @@ void arm_cpu_do_interrupt(CPUState *cs) env->regs[14 ] = env->regs[15 ] + offset; env->regs[15 ] = addr; cs->interrupt_request |=CPU_INTERRUPT_EXITTB ; + +pmccntr_sync (env); }/* Check section/page access permissions. target-arm/op_helper.c +4
-0@@ -376,6 +376,8 @@ void HELPER(exception_return)(CPUARMState *env)uint32_t spsr = env->banked_spsr[spsr_idx];int new_el, i; +pmccntr_sync (env); +if (env->pstate &PSTATE_SP ) { env->sp_el[cur_el] = env->xregs[31 ]; }else {@@ -418,6 +420,8 @@ void HELPER(exception_return)(CPUARMState *env) env->pc = env->elr_el[cur_el]; } +pmccntr_sync (env); +return ;
Quoted 13 lines
target-arm/op_helper.c illegal_return: · line 442
Quoted 8 lines
target-arm/op_helper.c +2
-0illegal_return:@@ -433,6 +437,8 @@ illegal_return: spsr &=PSTATE_NZCV |PSTATE_DAIF ; spsr |=pstate_read (env) & ~(PSTATE_NZCV |PSTATE_DAIF );pstate_write (env, spsr); + +pmccntr_sync (env); }This feels way too fragile to me to be scattering these sync calls all over the place like this. We need to find a better approach than this.
If we could consolidate all the code paths that actually switch el (both ways) into a helper fn we could then add the pmccntr el-switching side effects (i.e. the sync) to that helper once. Would you accept such an approach?
Regards,
Peter
Quoted 3 lines
thanks
-- PMM