More fully implement ARM PMUv3
v2 cover More fully implement ARM PMUv3
The ARM PMU implementation currently contains a basic cycle counter, but it is often useful to gather counts of other events and filter them based on execution mode. These patches flesh out the implementations of various PMU registers including PM[X]EVCNTR and PM[X]EVTYPER, add a struct definition to represent arbitrary counter types, implement mode filtering, and add instruction, cycle, and software increment events.
I am particularly interested in feedback on the following two patches because I think I'm likely Doing It Wrong:
[1] target/arm: Filter cycle counter based on PMCCFILTR_EL0
[2] target/arm: PMU: Add instruction and cycle events
In order to implement mode filtering in an event-driven way, [1] adds a pair of calls to pmu_sync() surrounding every update to a register/variable which may affect whether any counter is currently filtered. These pmu_sync() calls ultimately call cpu_get_icount_raw() for enabled instruction and cycle counters when using icount. Unfortunately, cpu->can_do_io may otherwise be zero for these calls so the current implementation in [2] temporarily sets can_do_io to
1. I haven't see any ill side effects from this in my testing, but it doesn't seem like the right way to handle this.
I would like to eventually add sending interrupts on counter overflow. Suggestions for the best direction to handle this are most welcome.
Thanks for any feedback,
Aaron
Aaron Lindsay (13):
target/arm: A53: Initialize PMCEID[0]
target/arm: Check PMCNTEN for whether PMCCNTR is enabled target/arm: Reorganize PMCCNTR read, write, sync target/arm: Mask PMU register writes based on PMCR_EL0.N target/arm: Allow AArch32 access for PMCCFILTR target/arm: Filter cycle counter based on PMCCFILTR_EL0 target/arm: Implement PMOVSSET
target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled target/arm: Add array for supported PMU events, generate PMCEID[01] target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER target/arm: PMU: Add instruction and cycle events target/arm: PMU: Set PMCR.N to 4
target/arm: Implement PMSWINC
target/arm/cpu.c | 10 +- target/arm/cpu.h | 34 +++- target/arm/cpu64.c | 2 + target/arm/helper.c | 535 +++++++++++++++++++++++++++++++++++++++++-------- target/arm/kvm64.c | 2 + target/arm/machine.c | 2 + target/arm/op_helper.c | 4 + 7 files changed, 500 insertions(+), 89 deletions(-)
Signature · 3 lines
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v2 1/13 target/arm: A53: Initialize PMCEID[0]
A53 advertises ARM_FEATURE_PMU, but wasn't initializing pmceid[01]
--- target/arm/cpu.c | 2 +- target/arm/cpu64.c | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-)
target/arm/cpu.c +1 -1
@@ -1402,7 +1402,7 @@ static void cortex_a15_initfn(Object *obj) cpu->id_pfr0 =0x00001131 ; cpu->id_pfr1 =0x00011011 ; cpu->id_dfr0 =0x02010555 ; - cpu->pmceid0 =0x0000000 ; + cpu->pmceid0 =0x00000000 ; cpu->pmceid1 =0x00000000 ; cpu->id_afr0 =0x00000000 ; cpu->id_mmfr0 =0x10201105 ;
target/arm/cpu64.c +2 -0
@@ -198,6 +198,8 @@ static void aarch64_a53_initfn(Object *obj) cpu->id_isar5 =0x00011121 ; cpu->id_aa64pfr0 =0x00002222 ; cpu->id_aa64dfr0 =0x10305106 ; + cpu->pmceid0 =0x00000000 ; + cpu->pmceid1 =0x00000000 ; cpu->id_aa64isar0 =0x00011120 ; cpu->id_aa64mmfr0 =0x00001122 ;/* 40 bit physical addr */ cpu->dbgdidr =0x3516d000 ;
Signature · 1 line
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.\nQualcomm Technologies, Inc. is a member of the\nCode Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v2 2/13 target/arm: Check PMCNTEN for whether PMCCNTR is enabled
In reply to Aaron Lindsay--- target/arm/helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
target/arm/helper.c +1 -1
@@ -964,7 +964,7 @@ static inline bool arm_ccnt_enabled(CPUARMState *env) {/* This does not support checking PMCCFILTR_EL0 register */ -if (!(env->cp15.c9_pmcr &PMCRE )) { +if (!(env->cp15.c9_pmcr &PMCRE ) || !(env->cp15.c9_pmcnten & (1 <<31 ))) {return false; }
Signature · 1 line
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.\nQualcomm Technologies, Inc. is a member of the\nCode Aurora Forum, a Linux Foundation Collaborative Project.
Re: [Qemu-devel] [PATCH 02/13] target/arm: Check PMCNTEN for whether PMCCNTR is enabled
Quoted 10 lines · Aaron Lindsay
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)target/arm/helper.c +0
-0
target/arm/helper.c static inline bool arm_ccnt_enabled(CPUARMState *env) · line 970
Quoted 8 lines
target/arm/helper.c +1
-1@@ -964,7 +964,7 @@ static inline bool arm_ccnt_enabled(CPUARMState *env) {/* This does not support checking PMCCFILTR_EL0 register */ -if (!(env->cp15.c9_pmcr &PMCRE )) { +if (!(env->cp15.c9_pmcr &PMCRE ) || !(env->cp15.c9_pmcnten & (1 <<31 ))) {return false; }
I keep replying to your v1 patchset by mistake -- can you use the git format-patch options to put make the subject prefix be "[PATCH v3]" for the next round in all the patchmails to help keep the versions distinct, please?
thanks
-- PMM
Re: [Qemu-arm] [PATCH 02/13] target/arm: Check PMCNTEN for whether PMCCNTR is enabled
Quoted 22 lines · Peter Maydell
Quoted 10 lines · Aaron Lindsay
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)target/arm/helper.c +0
-0target/arm/helper.c static inline bool arm_ccnt_enabled(CPUARMState *env) · line 970
Quoted 8 lines
target/arm/helper.c +1
-1@@ -964,7 +964,7 @@ static inline bool arm_ccnt_enabled(CPUARMState *env) {/* This does not support checking PMCCFILTR_EL0 register */ -if (!(env->cp15.c9_pmcr &PMCRE )) { +if (!(env->cp15.c9_pmcr &PMCRE ) || !(env->cp15.c9_pmcnten & (1 <<31 ))) {return false; }Reviewed-by Peter Maydell <[email protected]>
Quoted 4 lines
I keep replying to your v1 patchset by mistake -- can you use the git format-patch options to put make the subject prefix be "[PATCH v3]" for the next round in all the patchmails to help keep the versions distinct, please?
Yes, and I apologize for the nuisance. Also, thanks - I didn't know about the -v, --reroll-count option and must have gotten distracted between updating the subject to be 'v2' for the cover letter and the patches.
-Aaron
Signature · 3 lines
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [Qemu-arm] [PATCH 02/13] target/arm: Check PMCNTEN for whether PMCCNTR is enabled
Quoted 9 lines · Aaron Lindsay
Quoted 4 lines · Peter Maydell
I keep replying to your v1 patchset by mistake -- can you use the git format-patch options to put make the subject prefix be "[PATCH v3]" for the next round in all the patchmails to help keep the versions distinct, please?
Yes, and I apologize for the nuisance. Also, thanks - I didn't know about the
-v, --reroll-countoption and must have gotten distracted between updating the subject to be 'v2' for the cover letter and the patches.
There's also --subject-prefix if you need more flexibility than -v.
thanks
-- PMM
PATCH v2 3/13 target/arm: Reorganize PMCCNTR read, write, sync
In reply to Aaron Lindsaypmccntr_read and pmccntr_write contained duplicate code that was already being handled by pmccntr_sync. This also moves the calls to get the clock inside the 'if' statement so they are not executed if not needed.
--- target/arm/helper.c | 55 ++++++++++++++++------------------------------------- 1 file changed, 16 insertions(+), 39 deletions(-)
target/arm/helper.c +16 -39
@@ -973,17 +973,18 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)void pmccntr_sync (CPUARMState *env) { -uint64_t temp_ticks; +if (arm_ccnt_enabled (env) && + !pmu_counter_filtered (env, env->cp15.pmccfiltr_el0)) { +uint64_t temp_ticks; - temp_ticks =muldiv64 (qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ), -ARM_CPU_FREQ ,NANOSECONDS_PER_SECOND ); + temp_ticks =muldiv64 (qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ), +ARM_CPU_FREQ ,NANOSECONDS_PER_SECOND ); -if (env->cp15.c9_pmcr &PMCRD ) { -/* Increment once every 64 processor clock cycles */ - temp_ticks /=64 ; - } +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; } }
@@ -1007,21 +1008,11 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,static uint64_t pmccntr_read (CPUARMState *env,const ARMCPRegInfo *ri) { -uint64_t total_ticks; - -if (!arm_ccnt_enabled (env)) { -/* Counter is disabled, do not change value */ -return env->cp15.c15_ccnt; - } - - total_ticks =muldiv64 (qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ), -ARM_CPU_FREQ ,NANOSECONDS_PER_SECOND ); - -if (env->cp15.c9_pmcr &PMCRD ) { -/* Increment once every 64 processor clock cycles */ - total_ticks /=64 ; - } -return total_ticks - env->cp15.c15_ccnt; +uint64_t ret; +pmccntr_sync (env); + ret = env->cp15.c15_ccnt; +pmccntr_sync (env); +return ret; } static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1038,22 +1029,8 @@ static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri,static void pmccntr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { -uint64_t total_ticks; - -if (!arm_ccnt_enabled (env)) { -/* Counter is disabled, set the absolute value */ - env->cp15.c15_ccnt = value; -return ; - } - - total_ticks =muldiv64 (qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ), -ARM_CPU_FREQ ,NANOSECONDS_PER_SECOND ); - -if (env->cp15.c9_pmcr &PMCRD ) { -/* Increment once every 64 processor clock cycles */ - total_ticks /=64 ; - } - env->cp15.c15_ccnt = total_ticks - value; + env->cp15.c15_ccnt = value; +pmccntr_sync (env); } static void pmccntr_write32(CPUARMState *env, const ARMCPRegInfo *ri,
Signature · 1 line
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.\nQualcomm Technologies, Inc. is a member of the\nCode Aurora Forum, a Linux Foundation Collaborative Project.
Re: [Qemu-arm] [PATCH 03/13] target/arm: Reorganize PMCCNTR read, write, sync
Quoted 3 lines · Aaron Lindsay
pmccntr_readandpmccntr_writecontained duplicate code that was already being handled bypmccntr_sync. This also moves the calls to get the clock inside the 'if' statement so they are not executed if not needed.
It is duplicate code, yes, but I also find it a bit confusing, because it's the same code doing two different operations:
(1) if (as is usual when the counter is running) c15_ccnt contains a delta between the clock ticks and the register value, pmccntr_sync() sets c15_ccnt to the current guest-visible register value
(2) if c15_ccnt contains a guest-visible register value and the clock is running, pmccntr_sync() sets c15_ccnt to the ticks-to-value delta
and we use this in a couple of places like:
pmccntr_sync();
do stuff that operates on the guest register values,
or maybe turns off the counter, etc;
pmccntr_sync();
But that's wrong really -- we'll slightly lose time because the nanosecond clock advances between the two calls to qemu_clock_get_ns(). (It only works at all because it happens that f(x) = C - x is a self-inverse function.) It's also a confusing API because it looks like something you only need to call once but actually in most cases you need to call it twice, before and after whatever it is you're doing with the counter.
So I think we should refactor this so that we have a pair of functions, something like:
uint64_t clocknow = pmccntr_op_start(env);
/* Now c15_ccnt is the guest visible value, and
* we can do things like change PMCRD, enable bits, etc
*/
[...]
/* convert c15_ccnt back to the clock-to-value delta,
* passing it the tick count we used when we did the
* delta-to-value conversion earlier.
*/
pmccntr_op_finish(env, clocknow);(clocknow here should be the output of the muldiv64(), not the divided-by-64 version.)
Some more specific review comments below, though the suggested refactoring above might render some of them moot.
Quoted 10 lines
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/helper.c | 55 ++++++++++++++++------------------------------------- 1 file changed, 16 insertions(+), 39 deletions(-)target/arm/helper.c +0
-0
target/arm/helper.c static inline bool arm_ccnt_enabled(CPUARMState *env) · line 977
Quoted 6 lines
target/arm/helper.c +2
-1@@ -973,17 +973,18 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)void pmccntr_sync (CPUARMState *env) { -uint64_t temp_ticks; +if (arm_ccnt_enabled (env) && + !pmu_counter_filtered (env, env->cp15.pmccfiltr_el0)) {
This seems to be adding an extra condition that the commit message doesn't mention.
Quoted 34 lines
target/arm/helper.c +7
-17+uint64_t temp_ticks; - temp_ticks =muldiv64 (qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ), -ARM_CPU_FREQ ,NANOSECONDS_PER_SECOND ); + temp_ticks =muldiv64 (qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ), +ARM_CPU_FREQ ,NANOSECONDS_PER_SECOND ); -if (env->cp15.c9_pmcr &PMCRD ) { -/* Increment once every 64 processor clock cycles */ - temp_ticks /=64 ; - } +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; } }@@ -1007,21 +1008,11 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,static uint64_t pmccntr_read (CPUARMState *env,const ARMCPRegInfo *ri) { -uint64_t total_ticks; - -if (!arm_ccnt_enabled (env)) { -/* Counter is disabled, do not change value */ -return env->cp15.c15_ccnt; - } - - total_ticks =muldiv64 (qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ), -ARM_CPU_FREQ ,NANOSECONDS_PER_SECOND ); -
target/arm/helper.c static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri, · line 1015
Quoted 10 lines
target/arm/helper.c +5
-5-if (env->cp15.c9_pmcr &PMCRD ) { -/* Increment once every 64 processor clock cycles */ - total_ticks /=64 ; - } -return total_ticks - env->cp15.c15_ccnt; +uint64_t ret; +pmccntr_sync (env); + ret = env->cp15.c15_ccnt; +pmccntr_sync (env); +return ret;
This change means that as a result of this refactoring we now do the 'sync' operations (muldiv etc) twice, whereas previously we did them only once.
Quoted 16 lines
target/arm/helper.c +0
-9} static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri,@@ -1038,22 +1029,8 @@ static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri,static void pmccntr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { -uint64_t total_ticks; - -if (!arm_ccnt_enabled (env)) { -/* Counter is disabled, set the absolute value */ - env->cp15.c15_ccnt = value; -return ; - } - - total_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
target/arm/helper.c static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri, · line 1034
Quoted 10 lines
target/arm/helper.c +2
-7- ARM_CPU_FREQ, NANOSECONDS_PER_SECOND); - -if (env->cp15.c9_pmcr &PMCRD ) { -/* Increment once every 64 processor clock cycles */ - total_ticks /=64 ; - } - env->cp15.c15_ccnt = total_ticks - value; + env->cp15.c15_ccnt = value; +pmccntr_sync (env); }
thanks
-- PMM
Re: [Qemu-arm] [PATCH 03/13] target/arm: Reorganize PMCCNTR read, write, sync
Quoted 23 lines · Peter Maydell
Quoted 3 lines · Aaron Lindsay
pmccntr_readandpmccntr_writecontained duplicate code that was already being handled bypmccntr_sync. This also moves the calls to get the clock inside the 'if' statement so they are not executed if not needed.It is duplicate code, yes, but I also find it a bit confusing, because it's the same code doing two different operations:
(1) if (as is usual when the counter is running)
c15_ccntcontains a delta between the clock ticks and the register value,pmccntr_sync()setsc15_ccntto the current guest-visible register value(2) if
c15_ccntcontains a guest-visible register value and the clock is running,pmccntr_sync()setsc15_ccntto the ticks-to-value deltaand we use this in a couple of places like:
pmccntr_sync(); do stuff that operates on the guest register values, or maybe turns off the counter, etc; pmccntr_sync();
Quoted 8 lines
But that's wrong really -- we'll slightly lose time because the nanosecond clock advances between the two calls to
qemu_clock_get_ns(). (It only works at all because it happens that f(x) = C - x is a self-inverse function.) It's also a confusing API because it looks like something you only need to call once but actually in most cases you need to call it twice, before and after whatever it is you're doing with the counter.
Interesting, I hadn't thought about the loss of time issue.
Quoted 13 lines
So I think we should refactor this so that we have a pair of functions, something like:
uint64_t clocknow = pmccntr_op_start(env); /* Now c15_ccnt is the guest visible value, and * we can do things like change PMCRD, enable bits, etc */ [...] /* convert c15_ccnt back to the clock-to-value delta, * passing it the tick count we used when we did the * delta-to-value conversion earlier. */ pmccntr_op_finish(env, clocknow);
Quoted 5 lines
(clocknow here should be the output of the
muldiv64(), not the divided-by-64 version.)Some more specific review comments below, though the suggested refactoring above might render some of them moot.
I agree that what you describe would be cleaner. I'll work towards that in v3.
Quoted 11 lines
Quoted 11 lines
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/helper.c | 55 ++++++++++++++++------------------------------------- 1 file changed, 16 insertions(+), 39 deletions(-)target/arm/helper.c +0
-0@@ -973,17 +973,18 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
Quoted 8 lines
target/arm/helper.c static inline bool arm_ccnt_enabled(CPUARMState *env) · line 977
Quoted 5 lines
target/arm/helper.c +2
-1void pmccntr_sync (CPUARMState *env) { -uint64_t temp_ticks; +if (arm_ccnt_enabled (env) && + !pmu_counter_filtered (env, env->cp15.pmccfiltr_el0)) {This seems to be adding an extra condition that the commit message doesn't mention.
Eek. This slipped the cracks through during a rebase and belongs in 'target/arm: Filter cycle counter based on PMCCFILTR_EL0'.
Quoted 70 lines
Quoted 34 lines
target/arm/helper.c +7
-17+uint64_t temp_ticks; - temp_ticks =muldiv64 (qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ), -ARM_CPU_FREQ ,NANOSECONDS_PER_SECOND ); + temp_ticks =muldiv64 (qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ), +ARM_CPU_FREQ ,NANOSECONDS_PER_SECOND ); -if (env->cp15.c9_pmcr &PMCRD ) { -/* Increment once every 64 processor clock cycles */ - temp_ticks /=64 ; - } +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; } }@@ -1007,21 +1008,11 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,static uint64_t pmccntr_read (CPUARMState *env,const ARMCPRegInfo *ri) { -uint64_t total_ticks; - -if (!arm_ccnt_enabled (env)) { -/* Counter is disabled, do not change value */ -return env->cp15.c15_ccnt; - } - - total_ticks =muldiv64 (qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ), -ARM_CPU_FREQ ,NANOSECONDS_PER_SECOND ); -target/arm/helper.c static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri, · line 1015
Quoted 10 lines
target/arm/helper.c +5
-5-if (env->cp15.c9_pmcr &PMCRD ) { -/* Increment once every 64 processor clock cycles */ - total_ticks /=64 ; - } -return total_ticks - env->cp15.c15_ccnt; +uint64_t ret; +pmccntr_sync (env); + ret = env->cp15.c15_ccnt; +pmccntr_sync (env); +return ret;This change means that as a result of this refactoring we now do the 'sync' operations (muldiv etc) twice, whereas previously we did them only once.
Quoted 21 lines
target/arm/helper.c +0
-14} static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri,@@ -1038,22 +1029,8 @@ static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri,static void pmccntr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { -uint64_t total_ticks; - -if (!arm_ccnt_enabled (env)) { -/* Counter is disabled, set the absolute value */ - env->cp15.c15_ccnt = value; -return ; - } - - total_ticks =muldiv64 (qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ), -ARM_CPU_FREQ ,NANOSECONDS_PER_SECOND ); - -if (env->cp15.c9_pmcr &PMCRD ) { -/* Increment once every 64 processor clock cycles */ - total_ticks /=64 ;
Quoted 8 lines
target/arm/helper.c static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri, · line 1034
Quoted 5 lines
target/arm/helper.c +2
-2- } - env->cp15.c15_ccnt = total_ticks - value; + env->cp15.c15_ccnt = value; +pmccntr_sync (env); }thanks
-- PMM
-Aaron
Signature · 3 lines
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v2 4/13 target/arm: Mask PMU register writes based on PMCR_EL0.N
In reply to Aaron LindsayThis is in preparation for enabling counters other than PMCCNTR
--- target/arm/helper.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)
target/arm/helper.c +15 -9
@@ -30,11 +30,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot, target_ulong *page_size_ptr, uint32_t *fsr, ARMMMUFaultInfo *fi); - -/* Definitions for the PMCCNTR and PMCR registers */ -#define PMCRD 0x8 -#define PMCRC 0x4 -#define PMCRE 0x1#endif static int vfp_gdb_get_reg (CPUARMState *env,uint8_t *buf,int reg)
@@ -876,6 +871,17 @@ static const ARMCPRegInfo v6_cp_reginfo[] = { REGINFO_SENTINEL }; +/* Definitions for the PMU registers */ +#define PMCRN 0xf800 +#define PMCRN_SHIFT 11 +#define PMCRD 0x8 +#define PMCRC 0x4 +#define PMCRE 0x1 + +#define PMU_NUM_COUNTERS (env) ((env->cp15.c9_pmcr & PMCRN) >> PMCRN_SHIFT) +/* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */ +#define PMU_COUNTER_MASK (env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 1)) +static CPAccessResult pmreg_access (CPUARMState *env,const ARMCPRegInfo *ri,bool isread) {
@@ -1060,14 +1066,14 @@ static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,static void pmcntenset_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { - value &= (1 <<31 ); + value &= (PMU_COUNTER_MASK (env) | (1 <<31 )); env->cp15.c9_pmcnten |= value; }static void pmcntenclr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { - value &= (1 <<31 ); + value &= (PMU_COUNTER_MASK (env) | (1 <<31 )); env->cp15.c9_pmcnten &= ~value; }
@@ -1115,14 +1121,14 @@ static void pmintenset_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) {/* We have no event counters so only the C bit can be changed */ - value &= (1 <<31 ); + value &=PMU_COUNTER_MASK (env); env->cp15.c9_pminten |= value; }static void pmintenclr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { - value &= (1 <<31 ); + value &=PMU_COUNTER_MASK (env); env->cp15.c9_pminten &= ~value; }
Signature · 1 line
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.\nQualcomm Technologies, Inc. is a member of the\nCode Aurora Forum, a Linux Foundation Collaborative Project.
Re: [Qemu-devel] [PATCH 04/13] target/arm: Mask PMU register writes based on PMCR_EL0.N
Quoted 24 lines · Aaron Lindsay
This is in preparation for enabling counters other than PMCCNTR
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/helper.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)target/arm/helper.c +0
-5@@ -30,11 +30,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot, target_ulong *page_size_ptr, uint32_t *fsr, ARMMMUFaultInfo *fi); - -/* Definitions for the PMCCNTR and PMCR registers */ -#define PMCRD 0x8 -#define PMCRC 0x4 -#define PMCRE 0x1#endif static int vfp_gdb_get_reg (CPUARMState *env,uint8_t *buf,int reg)
target/arm/helper.c static const ARMCPRegInfo v6_cp_reginfo[] = { · line 875
Quoted 5 lines
target/arm/helper.c +2
-0@@ -876,6 +871,17 @@ static const ARMCPRegInfo v6_cp_reginfo[] = { REGINFO_SENTINEL }; +/* Definitions for the PMU registers */ +#define PMCRN 0xf800
We usually name this kind of whole-field mask value something like PMCRN_MASK.
(We also have a FIELD() macro in hw/registerfields.h for defining mask/length/shift constants; though it can be a bit verbose in the length of the names it uses, so I don't insist on its use if you don't like the results. I'm on the fence myself about it.)
Quoted 13 lines
target/arm/helper.c +9
-0+#define PMCRN_SHIFT 11 +#define PMCRD 0x8 +#define PMCRC 0x4 +#define PMCRE 0x1 + +#define PMU_NUM_COUNTERS (env) ((env->cp15.c9_pmcr & PMCRN) >> PMCRN_SHIFT) +/* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */ +#define PMU_COUNTER_MASK (env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 1)) +static CPAccessResult pmreg_access (CPUARMState *env,const ARMCPRegInfo *ri,bool isread) {
target/arm/helper.c static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri, · line 1069
Quoted 5 lines
target/arm/helper.c +1
-1@@ -1060,14 +1066,14 @@ static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,static void pmcntenset_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { - value &= (1 <<31 ); + value &= (PMU_COUNTER_MASK (env) | (1 <<31 ));
PMU_COUNTER_MASK always contains bit 31, so why do we manually OR it in again here?
Quoted 29 lines
target/arm/helper.c +3
-4env->cp15.c9_pmcnten |= value; }static void pmcntenclr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { - value &= (1 <<31 ); + value &= (PMU_COUNTER_MASK (env) | (1 <<31 )); env->cp15.c9_pmcnten &= ~value; }@@ -1115,14 +1121,14 @@ static void pmintenset_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) {/* We have no event counters so only the C bit can be changed */ - value &= (1 <<31 ); + value &=PMU_COUNTER_MASK (env); env->cp15.c9_pminten |= value; }static void pmintenclr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { - value &= (1 <<31 ); + value &=PMU_COUNTER_MASK (env); env->cp15.c9_pminten &= ~value; } --
Quoted 1 line
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.\nQualcomm Technologies, Inc. is a member of the\nCode Aurora Forum, a Linux Foundation Collaborative Project.
Shouldn't these '\n' in your sig be literal line breaks? :-)
thanks
-- PMM
Re: [Qemu-arm] [PATCH 04/13] target/arm: Mask PMU register writes based on PMCR_EL0.N
Quoted 33 lines · Peter Maydell
Quoted 24 lines · Aaron Lindsay
This is in preparation for enabling counters other than PMCCNTR
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/helper.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)target/arm/helper.c +0
-5@@ -30,11 +30,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address, hwaddr *phys_ptr, MemTxAttrs *txattrs, int *prot, target_ulong *page_size_ptr, uint32_t *fsr, ARMMMUFaultInfo *fi); - -/* Definitions for the PMCCNTR and PMCR registers */ -#define PMCRD 0x8 -#define PMCRC 0x4 -#define PMCRE 0x1#endif static int vfp_gdb_get_reg (CPUARMState *env,uint8_t *buf,int reg)target/arm/helper.c static const ARMCPRegInfo v6_cp_reginfo[] = { · line 875
Quoted 5 lines
target/arm/helper.c +2
-0@@ -876,6 +871,17 @@ static const ARMCPRegInfo v6_cp_reginfo[] = { REGINFO_SENTINEL }; +/* Definitions for the PMU registers */ +#define PMCRN 0xf800We usually name this kind of whole-field mask value something like
PMCRN_MASK.
Quoted 4 lines
(We also have a
FIELD()macro inhw/registerfields.hfor defining mask/length/shift constants; though it can be a bit verbose in the length of the names it uses, so I don't insist on its use if you don't like the results. I'm on the fence myself about it.)
Will do.
Quoted 13 lines
Quoted 13 lines
target/arm/helper.c +9
-0+#define PMCRN_SHIFT 11 +#define PMCRD 0x8 +#define PMCRC 0x4 +#define PMCRE 0x1 + +#define PMU_NUM_COUNTERS (env) ((env->cp15.c9_pmcr & PMCRN) >> PMCRN_SHIFT) +/* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */ +#define PMU_COUNTER_MASK (env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 1)) +static CPAccessResult pmreg_access (CPUARMState *env,const ARMCPRegInfo *ri,bool isread) {@@ -1060,14 +1066,14 @@ static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
Quoted 8 lines
target/arm/helper.c static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri, · line 1069
Quoted 5 lines
target/arm/helper.c +1
-1static void pmcntenset_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { - value &= (1 <<31 ); + value &= (PMU_COUNTER_MASK (env) | (1 <<31 ));
PMU_COUNTER_MASKalways contains bit 31, so why do we manually OR it in again here?
I forgot I'd also included the cycle counter in PMU_COUNTER_MASK... I'll remove the duplication here and in pmcntenclr_write below.
Quoted 28 lines
Quoted 28 lines
target/arm/helper.c +3
-3env->cp15.c9_pmcnten |= value; }static void pmcntenclr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { - value &= (1 <<31 ); + value &= (PMU_COUNTER_MASK (env) | (1 <<31 )); env->cp15.c9_pmcnten &= ~value; }@@ -1115,14 +1121,14 @@ static void pmintenset_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) {/* We have no event counters so only the C bit can be changed */ - value &= (1 <<31 ); + value &=PMU_COUNTER_MASK (env); env->cp15.c9_pminten |= value; }static void pmintenclr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { - value &= (1 <<31 ); + value &=PMU_COUNTER_MASK (env); env->cp15.c9_pminten &= ~value; }
Quoted 4 lines
Quoted 2 lines
--
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.\nQualcomm Technologies, Inc. is a member of the\nCode Aurora Forum, a Linux Foundation Collaborative Project.Shouldn't these '\n' in your sig be literal line breaks? :-)
Yes :( ...apparently git config doesn't interpret them as I expected.
Quoted 2 lines
thanks
-- PMM
Signature · 3 lines
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v2 5/13 target/arm: Allow AArch32 access for PMCCFILTR
In reply to Aaron LindsayAlso fix the existing bitmask for writes.
--- target/arm/helper.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)
target/arm/helper.c +22 -1
@@ -1059,10 +1059,25 @@ static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) {pmccntr_sync (env); - env->cp15.pmccfiltr_el0 = value &0x7E000000 ; + env->cp15.pmccfiltr_el0 = value &0xfc000000 ;pmccntr_sync (env); } +static void pmccfiltr_write_a32 (CPUARMState *env,const ARMCPRegInfo *ri, +uint64_t value) +{ +pmccntr_sync (env); + env->cp15.pmccfiltr_el0 = (env->cp15.pmccfiltr_el0 &0x04000000 ) | + (value &0xf8000000 );/* M is not visible in AArch32 */ +pmccntr_sync (env); +} + +static uint64_t pmccfiltr_read_a32 (CPUARMState *env,const ARMCPRegInfo *ri) +{ +/* M is not visible in AArch32 */ +return env->cp15.pmccfiltr_el0 &0xf8000000 ; +} +static void pmcntenset_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) {
@@ -1280,6 +1295,12 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .type = ARM_CP_IO, .readfn = pmccntr_read, .writefn = pmccntr_write, },#endif + { .name = "PMCCFILTR", .cp = 15, .opc1 = 0, .crn = 14, .crm = 15, .opc2 = 7, + .writefn = pmccfiltr_write_a32, .readfn = pmccfiltr_read_a32, + .access = PL0_RW, .accessfn = pmreg_access, + .type = ARM_CP_ALIAS, + .fieldoffset = offsetoflow32(CPUARMState, cp15.pmccfiltr_el0), + .resetvalue = 0, }, { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7, .writefn = pmccfiltr_write,
Signature · 1 line
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.\nQualcomm Technologies, Inc. is a member of the\nCode Aurora Forum, a Linux Foundation Collaborative Project.
Re: [Qemu-devel] [PATCH 05/13] target/arm: Allow AArch32 access for PMCCFILTR
Quoted 1 line · Aaron Lindsay
Also fix the existing bitmask for writes.
"Also" in a commit message can often be translated as "This should really go in a separate commit, but" :-)
Quoted 14 lines
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/helper.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)target/arm/helper.c +0
-1@@ -1059,10 +1059,25 @@ static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) {pmccntr_sync (env); - env->cp15.pmccfiltr_el0 = value &0x7E000000 ;
target/arm/helper.c static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri, · line 1071
Quoted 10 lines
target/arm/helper.c +7
-0+ env->cp15.pmccfiltr_el0 = value &0xfc000000 ;pmccntr_sync (env); } +static void pmccfiltr_write_a32 (CPUARMState *env,const ARMCPRegInfo *ri, +uint64_t value) +{ +pmccntr_sync (env); + env->cp15.pmccfiltr_el0 = (env->cp15.pmccfiltr_el0 &0x04000000 ) | + (value &0xf8000000 );/* M is not visible in AArch32 */
I think the comment would look better on its own line.
target/arm/helper.c static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri, · line 1073
Quoted 2 lines
target/arm/helper.c +2
-0+pmccntr_sync (env); +}
I think we could probably benefit from defining constants for the bit names in this register.
Quoted 13 lines
target/arm/helper.c +7
-0+ +static uint64_t pmccfiltr_read_a32 (CPUARMState *env,const ARMCPRegInfo *ri) +{ +/* M is not visible in AArch32 */ +return env->cp15.pmccfiltr_el0 &0xf8000000 ; +} +static void pmcntenset_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) {@@ -1280,6 +1295,12 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .type = ARM_CP_IO, .readfn = pmccntr_read, .writefn = pmccntr_write, },
target/arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 1306
Quoted 10 lines
target/arm/helper.c +6
-0#endif + { .name = "PMCCFILTR", .cp = 15, .opc1 = 0, .crn = 14, .crm = 15, .opc2 = 7, + .writefn = pmccfiltr_write_a32, .readfn = pmccfiltr_read_a32, + .access = PL0_RW, .accessfn = pmreg_access, + .type = ARM_CP_ALIAS, + .fieldoffset = offsetoflow32(CPUARMState, cp15.pmccfiltr_el0), + .resetvalue = 0, }, { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7, .writefn = pmccfiltr_write,
If you're defining both a .readfn and a .writefn for a register you don't need to provide a .fieldoffset.
thanks
-- PMM
PATCH v2 6/13 target/arm: Filter cycle counter based on PMCCFILTR_EL0
In reply to Aaron LindsayThe pmu_counter_filtered and pmu_sync functions are generic (as opposed to PMCCNTR-specific) to allow for the implementation of other events.
RFC: I know that many of the locations of the calls to pmu_sync are problematic when icount is enabled because can_do_io will not be set. The documentation says that for deterministic execution, IO must only be performed by the last instruction of a thread block. Because cpu_handle_interrupt() and cpu_handle_exception() are actually made outside of a thread block, is it safe to set can_do_io=1 for them to allow this to succeed? Is there a better mechanism for handling this?
--- target/arm/cpu.c | 4 +++ target/arm/cpu.h | 15 +++++++++++ target/arm/helper.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++--- target/arm/kvm64.c | 2 ++ target/arm/machine.c | 2 ++ target/arm/op_helper.c | 4 +++ 6 files changed, 97 insertions(+), 3 deletions(-)
target/arm/cpu.c +4 -0
@@ -139,6 +139,8 @@ static void arm_cpu_reset(CPUState *s) env->iwmmxt.cregs[ARM_IWMMXT_wCID] =0x69051000 |'Q' ; } +pmu_sync (env);/* Surround writes to uncached_cpsr, pstate, and aarch64 */ +if (arm_feature (env,ARM_FEATURE_AARCH64 )) {/* 64 bit CPUs always start in 64 bit mode */ env->aarch64 =1 ;
@@ -180,6 +182,8 @@ static void arm_cpu_reset(CPUState *s) env->uncached_cpsr =ARM_CPU_MODE_SVC ; env->daif =PSTATE_D |PSTATE_A |PSTATE_I |PSTATE_F ; +pmu_sync (env);/* Surround writes to uncached_cpsr, pstate, and aarch64 */ +if (arm_feature (env,ARM_FEATURE_M )) {uint32_t initial_msp;/* Loaded from 0x0 */ uint32_t initial_pc;/* Loaded from 0x4 */
target/arm/cpu.h +15 -0
@@ -822,6 +822,19 @@ int cpu_arm_signal_handler(int host_signum, void *pinfo,*/ void pmccntr_sync (CPUARMState *env); +/** +* pmu_sync +* @env: CPUARMState +* +* Synchronises all PMU counters. This must always be called twice, once before +* any action that might affect the filtering of all counters and again +* afterwards. The function is used to swap the state of the registers if +* required. This only happens when not in user mode (!CONFIG_USER_ONLY). Any +* writes to env's aarch64, pstate, uncached_cpsr, cp15.scr_el3, or +* cp15.hcr_el2 must be protected by calls to this function. +*/ +void pmu_sync (CPUARMState *env); +/* SCTLR bit meanings. Several bits have been reused in newer * versions of the architecture; in that case we define constants * for both old and new bit meanings. Code which tests against those
@@ -1018,7 +1031,9 @@ static inline void pstate_write(CPUARMState *env, uint32_t val) env->CF = (val >>29 ) &1 ; env->VF = (val <<3 ) &0x80000000 ; env->daif = val &PSTATE_DAIF ; +pmu_sync (env); env->pstate = val & ~CACHED_PSTATE_BITS ; +pmu_sync (env); }/* Return the current CPSR value. */
target/arm/helper.c +70 -3
152 lines not on this page · raw message
target/arm/kvm64.c +2 -0
@@ -783,7 +783,9 @@ int kvm_arch_get_registers(CPUState *cs)return ret; } +pmu_sync (env); env->aarch64 = ((val & PSTATE_nRW) ==0 ); +pmu_sync (env);if (is_a64 (env)) {pstate_write (env, val); }else {
target/arm/machine.c +2 -0
@@ -325,7 +325,9 @@ static int get_cpsr(QEMUFile *f, void *opaque, size_t size,return 0 ; } +pmu_sync (env); env->aarch64 = ((val & PSTATE_nRW) ==0 ); +pmu_sync (env);if (is_a64 (env)) {pstate_write (env, val);
target/arm/op_helper.c +4 -0
@@ -1063,7 +1063,9 @@ void HELPER(exception_return)(CPUARMState *env) }if (!return_to_aa64) { +pmu_sync (env); env->aarch64 =0 ; +pmu_sync (env);/* We do a raw CPSR write because aarch64_sync_64_to_32() * will sort the register banks out for us, and we've already * caught all the bad-mode cases in el_from_spsr().
@@ -1083,7 +1085,9 @@ void HELPER(exception_return)(CPUARMState *env)"AArch32 EL%d PC 0x%" PRIx32"\n" , cur_el, new_el, env->regs[15]); }else { +pmu_sync (env); env->aarch64 =1 ; +pmu_sync (env);pstate_write (env, spsr);if (!arm_singlestep_active (env)) { env->pstate &= ~PSTATE_SS ;
Signature · 1 line
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.\nQualcomm Technologies, Inc. is a member of the\nCode Aurora Forum, a Linux Foundation Collaborative Project.
Re: [Qemu-arm] [PATCH 06/13] target/arm: Filter cycle counter based on PMCCFILTR_EL0
Quoted 7 lines · Aaron Lindsay
The
pmu_counter_filteredandpmu_syncfunctions are generic (as opposed to PMCCNTR-specific) to allow for the implementation of other events.RFC: I know that many of the locations of the calls to
pmu_syncare problematic when icount is enabled becausecan_do_iowill not be set. The documentation says that for deterministic execution, IO must only be performed by the last instruction of a thread block.
Yes. You need to arrange that gen_io_start() and gen_io_end() are called around the generation of code for operations that might do IO or care about the state of the clock, and that we end the TB after gen_io_end().
Quoted 6 lines
Because
cpu_handle_interrupt()andcpu_handle_exception()are actually made outside of a thread block, is it safe to set can_do_io=1 for them to allow this to succeed? Is there a better mechanism for handling this?From my reading of the code,
can_do_ioshould already be 1
when these functions are called. It's only set to 0 just before we call tcg_qemu_tb_exec() and then set back to 1 immediately after (see cpu_tb_exec()).
In general, the approach you have here looks like it's going to be pretty invasive and also hard to keep working right. I think we can come up with something a bit better.
Specifically, the filtering criteria effectively only change when we change exception level, right? (since you can only change security state as part of an exception level change). We already have a mechanism for getting a callback when the EL changes -- arm_register_el_change_hook(). (We would need to upgrade it to support more than one hook function.)
You still need to get the io-count handling right, but there are many fewer places that need to change (just the codegen for calls to exception-return helpers, I think) and they already end the TB, so you don't have the complexity of trying to end the TB in places you didn't before, you just need the gen_io_start/end.
thanks
-- PMM
Re: [Qemu-arm] [PATCH 06/13] target/arm: Filter cycle counter based on PMCCFILTR_EL0
Quoted 19 lines · Peter Maydell
Quoted 7 lines · Aaron Lindsay
The
pmu_counter_filteredandpmu_syncfunctions are generic (as opposed to PMCCNTR-specific) to allow for the implementation of other events.RFC: I know that many of the locations of the calls to
pmu_syncare problematic when icount is enabled becausecan_do_iowill not be set. The documentation says that for deterministic execution, IO must only be performed by the last instruction of a thread block.Yes. You need to arrange that
gen_io_start()andgen_io_end()are called around the generation of code for operations that might do IO or care about the state of the clock, and that we end the TB aftergen_io_end().Quoted 4 lines
Because
cpu_handle_interrupt()andcpu_handle_exception()are actually made outside of a thread block, is it safe to set can_do_io=1 for them to allow this to succeed? Is there a better mechanism for handling this?
Quoted 8 lines
From my reading of the code,
can_do_ioshould already be 1 when these functions are called. It's only set to 0 just before we calltcg_qemu_tb_exec()and then set back to 1 immediately after (seecpu_tb_exec()).In general, the approach you have here looks like it's going to be pretty invasive and also hard to keep working right. I think we can come up with something a bit better.
Yes, I am hoping so.
Quoted 7 lines
Specifically, the filtering criteria effectively only change when we change exception level, right? (since you can only change security state as part of an exception level change). We already have a mechanism for getting a callback when the EL changes --
arm_register_el_change_hook(). (We would need to upgrade it to support more than one hook function.)
Quoted 5 lines
You still need to get the io-count handling right, but there are many fewer places that need to change (just the codegen for calls to exception-return helpers, I think) and they already end the TB, so you don't have the complexity of trying to end the TB in places you didn't before, you just need the gen_io_start/end.
Using hooks/callbacks is clearly a better solution. I shied away from using *el_change_hook in this patchset because A) it seemed to be marked explicitly for GICv3 emulation, B) the one-hook limitation you mentioned, and C) it doesn't currently provide you with which EL you're coming from.
If everyone is amenable to changing how the hook is structured, I don't think any of those reasons stand. My initial thought is that the best way to fix C) is to add a pre_el_change_hook to be called at the top of the exception_return and cpsr_write_eret helpers in target/arm/op_helper.c to drive the first call to pmu_sync() (or whatever I rename the first half of that pair to based on your suggestions on that patch).
If I don't receive any additional feedback before then, I'll do my best to adapt the existing hooks to allow for a cleaner approach to filtering for v3.
-Aaron
Signature · 3 lines
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v2 7/13 target/arm: Implement PMOVSSET
In reply to Aaron LindsayAlso modify it to be stored as a uint64_t
--- target/arm/cpu.h | 2 +- target/arm/helper.c | 27 ++++++++++++++++++++++++--- 2 files changed, 25 insertions(+), 4 deletions(-)
target/arm/cpu.h +1 -1
@@ -325,7 +325,7 @@ typedef struct CPUARMState {uint32_t c9_data;uint64_t c9_pmcr;/* performance monitor control register */ uint64_t c9_pmcnten;/* perf monitor counter enables */ -uint32_t c9_pmovsr;/* perf monitor overflow status */ +uint64_t c9_pmovsr;/* perf monitor overflow status */ uint32_t c9_pmuserenr;/* perf monitor user enable */ uint64_t c9_pmselr;/* perf monitor counter selection register */ uint64_t c9_pminten;/* perf monitor interrupt enables */
target/arm/helper.c +24 -3
@@ -1150,9 +1150,17 @@ static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,static void pmovsr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { + value &=PMU_COUNTER_MASK (env); env->cp15.c9_pmovsr &= ~value; } +static void pmovsset_write (CPUARMState *env,const ARMCPRegInfo *ri, +uint64_t value) +{ + value &=PMU_COUNTER_MASK (env); + env->cp15.c9_pmovsr |= value; +} +static void pmxevtyper_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) {
@@ -1317,10 +1325,10 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten), .writefn = pmcntenclr_write }, { .name = "PMOVSR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 3, - .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr), - .accessfn = pmreg_access, + .access = PL0_RW, .accessfn = pmreg_access, + .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr), .writefn = pmovsr_write, - .raw_writefn = raw_write }, + .raw_writefn = raw_write, .resetvalue = 0 }, { .name = "PMOVSCLR_EL0", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 3, .access = PL0_RW, .accessfn = pmreg_access,
@@ -1328,6 +1336,19 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr), .writefn = pmovsr_write, .raw_writefn = raw_write }, + { .name = "PMOVSSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 3, + .access = PL0_RW, .accessfn = pmreg_access, + .type = ARM_CP_ALIAS, + .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr), + .writefn = pmovsset_write, + .raw_writefn = raw_write }, + { .name = "PMOVSSET_EL0", .state = ARM_CP_STATE_AA64, + .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 14, .opc2 = 3, + .access = PL0_RW, .accessfn = pmreg_access, + .type = ARM_CP_ALIAS, + .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr), + .writefn = pmovsset_write, + .raw_writefn = raw_write }, /* Unimplemented so WI. */ { .name = "PMSWINC", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 4, .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NOP },
Signature · 1 line
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.\nQualcomm Technologies, Inc. is a member of the\nCode Aurora Forum, a Linux Foundation Collaborative Project.
Re: [Qemu-arm] [PATCH 07/13] target/arm: Implement PMOVSSET
Quoted 13 lines · Aaron Lindsay
Also modify it to be stored as a
uint64_tSigned-off-by Aaron Lindsay <[email protected]>--- target/arm/cpu.h | 2 +- target/arm/helper.c | 27 ++++++++++++++++++++++++--- 2 files changed, 25 insertions(+), 4 deletions(-)target/arm/cpu.h +0
-0
target/arm/cpu.h typedef struct CPUARMState { · line 328
Quoted 5 lines
target/arm/cpu.h +1
-1@@ -325,7 +325,7 @@ typedef struct CPUARMState {uint32_t c9_data;uint64_t c9_pmcr;/* performance monitor control register */ uint64_t c9_pmcnten;/* perf monitor counter enables */ -uint32_t c9_pmovsr;/* perf monitor overflow status */ +uint64_t c9_pmovsr;/* perf monitor overflow status */
This is a bug fix, so it should go in its own patch. Specifically, we already have an AArch64 sysreg PMOVSCLR_EL0 which has
.fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
so we should have made the CPUARMState field 64 bits when we added it. (I think without this bugfix reads of the AArch64 reg will return data from the adjoining field in the struct.)
Quoted 26 lines
target/arm/cpu.h +0
-0uint32_t c9_pmuserenr;/* perf monitor user enable */ uint64_t c9_pmselr;/* perf monitor counter selection register */ uint64_t c9_pminten;/* perf monitor interrupt enables */ target/arm/helper.c +8
-0@@ -1150,9 +1150,17 @@ static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,static void pmovsr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { + value &=PMU_COUNTER_MASK (env); env->cp15.c9_pmovsr &= ~value; } +static void pmovsset_write (CPUARMState *env,const ARMCPRegInfo *ri, +uint64_t value) +{ + value &=PMU_COUNTER_MASK (env); + env->cp15.c9_pmovsr |= value; +} +static void pmxevtyper_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) {
target/arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 1329
Quoted 7 lines
target/arm/helper.c +2
-2@@ -1317,10 +1325,10 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten), .writefn = pmcntenclr_write }, { .name = "PMOVSR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 3, - .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr), - .accessfn = pmreg_access, + .access = PL0_RW, .accessfn = pmreg_access, + .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
Is this just reshuffling the order of .field initializers?
target/arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 1331
Quoted 3 lines
target/arm/helper.c +1
-1.writefn = pmovsr_write, - .raw_writefn = raw_write }, + .raw_writefn = raw_write, .resetvalue = 0 },
.resetvalue 0 is the default, but it doesn't hurt to specify it explicitly I guess.
Quoted 10 lines
target/arm/helper.c +3
-0{ .name = "PMOVSCLR_EL0", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 3, .access = PL0_RW, .accessfn = pmreg_access,@@ -1328,6 +1336,19 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr), .writefn = pmovsr_write, .raw_writefn = raw_write }, + { .name = "PMOVSSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 3, + .access = PL0_RW, .accessfn = pmreg_access, + .type = ARM_CP_ALIAS,
target/arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 1351
Quoted 10 lines
target/arm/helper.c +10
-0+ .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr), + .writefn = pmovsset_write, + .raw_writefn = raw_write }, + { .name = "PMOVSSET_EL0", .state = ARM_CP_STATE_AA64, + .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 14, .opc2 = 3, + .access = PL0_RW, .accessfn = pmreg_access, + .type = ARM_CP_ALIAS, + .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr), + .writefn = pmovsset_write, + .raw_writefn = raw_write },
You can implement these using a single STATE_BOTH regdef, I think. Also, there's a standard order for the fields which goes .cp .opc0 .opc1 .crn .crm .opc2
(you can often omit the .cp as the default is 15).
We need to be a bit careful here, because the AArch32 PMMOVSET register isn't implemented in ARMv7 until v7VE. So we need to put the regdef somewhere else...
thanks
-- PMM
Re: [Qemu-arm] [PATCH 07/13] target/arm: Implement PMOVSSET
Quoted 18 lines · Peter Maydell
Quoted 17 lines · Aaron Lindsay
Also modify it to be stored as a
uint64_tSigned-off-by Aaron Lindsay <[email protected]>--- target/arm/cpu.h | 2 +- target/arm/helper.c | 27 ++++++++++++++++++++++++--- 2 files changed, 25 insertions(+), 4 deletions(-)target/arm/cpu.h +0
-1@@ -325,7 +325,7 @@ typedef struct CPUARMState {uint32_t c9_data;uint64_t c9_pmcr;/* performance monitor control register */ uint64_t c9_pmcnten;/* perf monitor counter enables */ -uint32_t c9_pmovsr;/* perf monitor overflow status */
Quoted 8 lines
target/arm/cpu.h typedef struct CPUARMState { · line 328
Quoted 1 line
target/arm/cpu.h +1
-0+uint64_t c9_pmovsr;/* perf monitor overflow status */ This is a bug fix, so it should go in its own patch. Specifically, we already have an AArch64 sysreg
PMOVSCLR_EL0which has
.fieldoffset =offsetof(CPUARMState, cp15.c9_pmovsr),
so we should have made the CPUARMState field 64 bits when we added it. (I think without this bugfix reads of the AArch64 reg will return data from the adjoining field in the struct.)
Okay, I'll split it off in v3.
Quoted 28 lines
Quoted 28 lines
target/arm/cpu.h +0
-0uint32_t c9_pmuserenr;/* perf monitor user enable */ uint64_t c9_pmselr;/* perf monitor counter selection register */ uint64_t c9_pminten;/* perf monitor interrupt enables */ target/arm/helper.c +8
-0@@ -1150,9 +1150,17 @@ static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,static void pmovsr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) { + value &=PMU_COUNTER_MASK (env); env->cp15.c9_pmovsr &= ~value; } +static void pmovsset_write (CPUARMState *env,const ARMCPRegInfo *ri, +uint64_t value) +{ + value &=PMU_COUNTER_MASK (env); + env->cp15.c9_pmovsr |= value; +} +static void pmxevtyper_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) {@@ -1317,10 +1325,10 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
Quoted 8 lines
target/arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 1329
Quoted 6 lines
target/arm/helper.c +2
-2.writefn = pmcntenclr_write }, { .name = "PMOVSR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 3, - .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr), - .accessfn = pmreg_access, + .access = PL0_RW, .accessfn = pmreg_access, + .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),Is this just reshuffling the order of .field initializers?
Also updated offsetof -> offsetoflow32. IIRC, I shuffled the order to match that of the surrounding registers and to appease my sense of order. Would you prefer that I don't shuffle them, or split that off as a separate patch?
Quoted 33 lines
target/arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 1331
Quoted 3 lines
target/arm/helper.c +1
-1.writefn = pmovsr_write, - .raw_writefn = raw_write }, + .raw_writefn = raw_write, .resetvalue = 0 },.resetvalue 0 is the default, but it doesn't hurt to specify it explicitly I guess.
Quoted 10 lines
target/arm/helper.c +3
-0{ .name = "PMOVSCLR_EL0", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 3, .access = PL0_RW, .accessfn = pmreg_access,@@ -1328,6 +1336,19 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr), .writefn = pmovsr_write, .raw_writefn = raw_write }, + { .name = "PMOVSSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 3, + .access = PL0_RW, .accessfn = pmreg_access, + .type = ARM_CP_ALIAS,target/arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 1351
Quoted 10 lines
target/arm/helper.c +10
-0+ .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr), + .writefn = pmovsset_write, + .raw_writefn = raw_write }, + { .name = "PMOVSSET_EL0", .state = ARM_CP_STATE_AA64, + .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 14, .opc2 = 3, + .access = PL0_RW, .accessfn = pmreg_access, + .type = ARM_CP_ALIAS, + .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr), + .writefn = pmovsset_write, + .raw_writefn = raw_write },You can implement these using a single
STATE_BOTHregdef, I think. Also, there's a standard order for the fields which goes .cp .opc0 .opc1 .crn .crm .opc2
(you can often omit the .cp as the default is 15).
Quoted 3 lines
We need to be a bit careful here, because the AArch32 PMMOVSET register isn't implemented in ARMv7 until v7VE. So we need to put the regdef somewhere else...
Hmmm, what does it need to be protected by? I assume ARM_FEATURE_V8 || (ARM_FEATURE_V7 && something), but I'm not familiar enough with v7 that it's immediately obvious from looking around what 'something' is.
-Aaron
Quoted 4 lines
thanks
-- PMM
Signature · 3 lines
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v2 8/13 target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled
In reply to Aaron Lindsay--- target/arm/helper.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
target/arm/helper.c +8 -3
@@ -975,17 +975,22 @@ static CPAccessResult pmreg_access_ccntr(CPUARMState *env,return pmreg_access (env, ri, isread); } -static inline bool arm_ccnt_enabled (CPUARMState *env) +static inline bool pmu_counter_enabled (CPUARMState *env,uint8_t counter) {/* Does not check PMCCFILTR_EL0, which is handled by pmu_counter_filtered */ - -if (!(env->cp15.c9_pmcr &PMCRE ) || !(env->cp15.c9_pmcnten & (1 <<31 ))) { +if (!(env->cp15.c9_pmcr &PMCRE ) || + !(env->cp15.c9_pmcnten & (1 << counter))) {return false; }return true; } +static inline bool arm_ccnt_enabled (CPUARMState *env) +{ +return pmu_counter_enabled (env,31 ); +} +/* Returns true if the counter corresponding to the passed-in pmevtyper or * pmccfiltr value is filtered using the current state */ static inline bool pmu_counter_filtered (CPUARMState *env,uint64_t pmxevtyper)
Signature · 1 line
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.\nQualcomm Technologies, Inc. is a member of the\nCode Aurora Forum, a Linux Foundation Collaborative Project.
Re: [Qemu-arm] [PATCH 08/13] target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled
Quoted 22 lines · Aaron Lindsay
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/helper.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)target/arm/helper.c +3
-3@@ -975,17 +975,22 @@ static CPAccessResult pmreg_access_ccntr(CPUARMState *env,return pmreg_access (env, ri, isread); } -static inline bool arm_ccnt_enabled (CPUARMState *env) +static inline bool pmu_counter_enabled (CPUARMState *env,uint8_t counter) {/* Does not check PMCCFILTR_EL0, which is handled by pmu_counter_filtered */ - -if (!(env->cp15.c9_pmcr &PMCRE ) || !(env->cp15.c9_pmcnten & (1 <<31 ))) { +if (!(env->cp15.c9_pmcr &PMCRE ) || + !(env->cp15.c9_pmcnten & (1 << counter))) {return false;
target/arm/helper.c static CPAccessResult pmreg_access_ccntr(CPUARMState *env, · line 993
Quoted 10 lines
target/arm/helper.c +5
-0}return true; } +static inline bool arm_ccnt_enabled (CPUARMState *env) +{ +return pmu_counter_enabled (env,31 ); +} +
thanks
-- PMM
PATCH v2 9/13 target/arm: Add array for supported PMU events, generate PMCEID[01]
In reply to Aaron LindsayThis commit doesn't add any supported events, but provides the framework for adding them. We store the pm_event structs in a simple array, and provide the mapping from the event numbers to array indexes in the supported_event_map array.
--- target/arm/cpu.c | 4 ++++ target/arm/cpu.h | 10 ++++++++++ target/arm/helper.c | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+)
target/arm/cpu.c +4 -0
@@ -830,6 +830,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)if (!cpu->has_pmu) {unset_feature (env,ARM_FEATURE_PMU ); cpu->id_aa64dfr0 &= ~0xf00 ; + }else { +uint64_t pmceid =get_pmceid (&cpu->env); + cpu->pmceid0 = pmceid &0xffffffff ; + cpu->pmceid1 = (pmceid >>32 ) &0xffffffff ; }if (!arm_feature (env,ARM_FEATURE_EL2 )) {
target/arm/cpu.h +10 -0
@@ -835,6 +835,16 @@ void pmccntr_sync(CPUARMState *env);*/ void pmu_sync (CPUARMState *env); +/* +* get_pmceid +* @env: CPUARMState +* +* Return the PMCEID[01] register values corresponding to the counters which +* are supported given the current configuration (0 is low 32, 1 is high 32 +* bits) +*/ +uint64_t get_pmceid (CPUARMState *env); +/* SCTLR bit meanings. Several bits have been reused in newer * versions of the architecture; in that case we define constants * for both old and new bit meanings. Code which tests against those
target/arm/helper.c +37 -0
@@ -891,6 +891,43 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {/* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */ #define PMU_COUNTER_MASK (env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 1)) +typedef struct pm_event { +uint16_t number;/* PMEVTYPER.evtCount is 10 bits wide */ +/* If the event is supported on this CPU (used to generate PMCEID[01]) */ +bool (*supported)(CPUARMState *); +/* Retrieve the current count of the underlying event. The programmed +* counters hold a difference from the return value from this function */ +uint64_t (*get_count)(CPUARMState *); +}pm_event ; + +#define SUPPORTED_EVENT_SENTINEL UINT16_MAX +static const pm_event pm_events[] = { + { .number =SUPPORTED_EVENT_SENTINEL } +}; +static uint16_t supported_event_map[0x3f ]; + +/* +* Called upon initialization to build PMCEID0 (low 32 bits) and PMCEID1 (high +* 32). We also use it to build a map of ARM event numbers to indices in +* our pm_events array. +*/ +uint64_t get_pmceid (CPUARMState *env) +{ +uint64_t pmceid =0 ; +unsigned int i =0 ; +while (pm_events[i].number !=SUPPORTED_EVENT_SENTINEL ) { +const pm_event *cnt = &pm_events[i]; +if (cnt->number <0x3f && cnt->supported (env)) { + pmceid |= (1 << cnt->number); + supported_event_map[cnt->number] = i; + }else { + supported_event_map[cnt->number] =SUPPORTED_EVENT_SENTINEL ; + } + i++; + } +return pmceid; +} +static CPAccessResult pmreg_access (CPUARMState *env,const ARMCPRegInfo *ri,bool isread) {
Signature · 1 line
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.\nQualcomm Technologies, Inc. is a member of the\nCode Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v2 10/13 target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER
In reply to Aaron LindsayAdd arrays to hold the registers, the definitions themselves, access functions, and add logic to reset counters when PMCR.P is set.
--- target/arm/cpu.h | 7 +- target/arm/helper.c | 187 ++++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 179 insertions(+), 15 deletions(-)
target/arm/cpu.h +5 -2
@@ -412,10 +412,13 @@ typedef struct CPUARMState {uint64_t oslsr_el1;/* OS Lock Status */ uint64_t mdcr_el2;uint64_t mdcr_el3; -/* If the counter is enabled, this stores the last time the counter -* was reset. Otherwise it stores the counter value +/* If the pmccntr and pmevcntr counters are enabled, they store the +* offset the last time the counter was reset. Otherwise they store the +* counter value. */ uint64_t c15_ccnt; +uint64_t c14_pmevcntr[31 ]; +uint64_t c14_pmevtyper[31 ];uint64_t pmccfiltr_el0;/* Performance Monitor Filter Register */ uint64_t vpidr_el2;/* Virtualization Processor ID Register */ uint64_t vmpidr_el2;/* Virtualization Multiprocessor ID Register */
target/arm/helper.c +174 -13
259 lines not on this page · raw message
Signature · 1 line
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.\nQualcomm Technologies, Inc. is a member of the\nCode Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v2 11/13 target/arm: PMU: Add instruction and cycle events
In reply to Aaron LindsayThe instruction event is only enabled when icount is used, cycles are always supported.
Note: Setting can_do_io=1 should not be done here. It is ugly and wrong, but I am not sure of the proper way to handle this (See 'target/arm:
Filter cycle counter based on PMCCFILTR_EL0')
--- target/arm/helper.c | 75 ++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 54 insertions(+), 21 deletions(-)
target/arm/helper.c +54 -21
@@ -14,6 +14,7 @@#include "arm_ldst.h" #include <zlib.h> /* For crc32 */ #include "exec/semihost.h" +#include "sysemu/cpus.h" #include "sysemu/kvm.h" #define ARM_CPU_FREQ 1000000000/* FIXME: 1 GHz, should be configurable */
@@ -901,8 +902,57 @@ typedef struct pm_event {uint64_t (*get_count)(CPUARMState *); } pm_event; +static bool event_always_supported (CPUARMState *env) +{ +return true; +} + +#ifndef CONFIG_USER_ONLY +static uint64_t cycles_get_count (CPUARMState *env) +{ +uint64_t ret; +CPUState *cpu =ENV_GET_CPU (env); +uint32_t saved_can_do_io = cpu->can_do_io; + cpu->can_do_io =1 ; + + ret =muldiv64 (qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ), +ARM_CPU_FREQ ,NANOSECONDS_PER_SECOND ); + + cpu->can_do_io = saved_can_do_io; +return ret; +} + +static bool instructions_supported (CPUARMState *env) +{ +return use_icount ==1 /* Precise instruction counting */ ; +} + +static uint64_t instructions_get_count (CPUARMState *env) +{ +uint64_t ret; +CPUState *cpu =ENV_GET_CPU (env); +uint32_t saved_can_do_io = cpu->can_do_io; + cpu->can_do_io =1 ; + + ret = (uint64_t )cpu_get_icount_raw (); + + cpu->can_do_io = saved_can_do_io; +return ret; +} +#endif +#define SUPPORTED_EVENT_SENTINEL UINT16_MAXstatic const pm_event pm_events[] = { +#ifndef CONFIG_USER_ONLY + { .number =0x008 ,/* INST_RETIRED */ + .supported = instructions_supported, + .get_count = instructions_get_count + }, + { .number =0x011 ,/* CPU_CYCLES */ + .supported = event_always_supported, + .get_count = cycles_get_count + }, +#endif { .number =SUPPORTED_EVENT_SENTINEL } };static uint16_t supported_event_map[0x3f ];
@@ -982,8 +1032,6 @@ static CPAccessResult pmreg_access_swinc(CPUARMState *env,return pmreg_access (env, ri, isread); } -#ifndef CONFIG_USER_ONLY -static CPAccessResult pmreg_access_selr (CPUARMState *env,const ARMCPRegInfo *ri,bool isread)
@@ -1085,10 +1133,11 @@ void pmccntr_sync(CPUARMState *env) {if (arm_ccnt_enabled (env) && !pmu_counter_filtered (env, env->cp15.pmccfiltr_el0)) { -uint64_t temp_ticks; +uint64_t temp_ticks =0 ; - temp_ticks =muldiv64 (qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ), -ARM_CPU_FREQ ,NANOSECONDS_PER_SECOND ); +#ifndef CONFIG_USER_ONLY + temp_ticks =cycles_get_count (env); +#endif if (env->cp15.c9_pmcr &PMCRD ) {/* Increment once every 64 processor clock cycles */
@@ -1181,18 +1230,6 @@ static void pmccntr_write32(CPUARMState *env, const ARMCPRegInfo *ri,pmccntr_write (env, ri,deposit64 (cur_val,0 ,32 , value)); } -#else /* CONFIG_USER_ONLY */ - -void pmccntr_sync (CPUARMState *env) -{ -} - -void pmu_sync (CPUARMState *env) -{ -} - -#endif -static void pmccfiltr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) {
@@ -1517,7 +1554,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {/* Unimplemented so WI. */ { .name = "PMSWINC", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 4, .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NOP }, -#ifndef CONFIG_USER_ONLY { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5, .access = PL0_RW, .type = ARM_CP_ALIAS, .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmselr),
@@ -1537,7 +1573,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .access = PL0_RW, .accessfn = pmreg_access_ccntr, .type = ARM_CP_IO, .readfn = pmccntr_read, .writefn = pmccntr_write, }, -#endif { .name = "PMCCFILTR", .cp = 15, .opc1 = 0, .crn = 14, .crm = 15, .opc2 = 7, .writefn = pmccfiltr_write_a32, .readfn = pmccfiltr_read_a32, .access = PL0_RW, .accessfn = pmreg_access,
@@ -4877,7 +4912,6 @@ void register_cp_regs_for_features(ARMCPU *cpu)* field as main ID register, and we implement only the cycle * count register. */ -#ifndef CONFIG_USER_ONLY ARMCPRegInfo pmcr = { .name ="PMCR" , .cp =15 , .crn =9 , .crm =12 , .opc1 =0 , .opc2 =0 , .access =PL0_RW ,
@@ -4931,7 +4965,6 @@ void register_cp_regs_for_features(ARMCPU *cpu)g_free (pmevtyper_name);g_free (pmevtyper_el0_name); } -#endif ARMCPRegInfo clidr = { .name ="CLIDR" , .state =ARM_CP_STATE_BOTH , .opc0 =3 , .crn =0 , .crm =0 , .opc1 =1 , .opc2 =1 ,
Signature · 1 line
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.\nQualcomm Technologies, Inc. is a member of the\nCode Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v2 12/13 target/arm: PMU: Set PMCR.N to 4
In reply to Aaron LindsayThis both advertises that we support four counters and adds them to the implementation because the PMU_NUM_COUNTERS macro reads this value from the PMCR.
--- target/arm/helper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
target/arm/helper.c +2 -1
@@ -4926,7 +4926,8 @@ void register_cp_regs_for_features(ARMCPU *cpu) .access = PL0_RW, .accessfn = pmreg_access, .type = ARM_CP_IO, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr), - .resetvalue = cpu->midr & 0xff000000, + /* 4 counters enabled */ + .resetvalue = (cpu->midr & 0xff000000) | (0x4 << PMCRN_SHIFT), .writefn = pmcr_write, .raw_writefn = raw_write, };define_one_arm_cp_reg (cpu, &pmcr);
Signature · 1 line
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.\nQualcomm Technologies, Inc. is a member of the\nCode Aurora Forum, a Linux Foundation Collaborative Project.
PATCH v2 13/13 target/arm: Implement PMSWINC
In reply to Aaron Lindsay--- target/arm/helper.c | 40 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-)
target/arm/helper.c +38 -2
@@ -907,6 +907,15 @@ static bool event_always_supported(CPUARMState *env)return true; } +static uint64_t swinc_get_count (CPUARMState *env) +{ +/* +* SW_INCR events are written directly to the pmevcntr's by writes to +* PMSWINC, so don't do anything here... +*/ +return 0 ; +} +#ifndef CONFIG_USER_ONLY static uint64_t cycles_get_count (CPUARMState *env) {
@@ -943,6 +952,10 @@ static uint64_t instructions_get_count(CPUARMState *env)#define SUPPORTED_EVENT_SENTINEL UINT16_MAXstatic const pm_event pm_events[] = { + { .number =0x000 ,/* SW_INCR */ + .supported = event_always_supported, + .get_count = swinc_get_count + }, #ifndef CONFIG_USER_ONLY { .number =0x008 ,/* INST_RETIRED */ .supported = instructions_supported,
@@ -1195,6 +1208,25 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,pmu_sync (env); } +static void pmswinc_write (CPUARMState *env,const ARMCPRegInfo *ri, +uint64_t value) +{ +unsigned int i; +for (i =0 ; i <PMU_NUM_COUNTERS (env); i++) { +/* Increment a counter's count iff: */ +if ((value & (1 << i)) &&/* counter's bit is set */ +/* counter is enabled and not filtered */ +pmu_counter_enabled (env, i) && + !pmu_counter_filtered (env, env->cp15.c14_pmevtyper[i]) && +/* counter is SW_INCR */ + (env->cp15.c14_pmevtyper[i] &PMXEVTYPER_EVTCOUNT ) ==0x0 ) { +pmu_sync_counter (env, i); + env->cp15.c14_pmevcntr[i]++; +pmu_sync_counter (env, i); + } + } +} +static uint64_t pmccntr_read (CPUARMState *env,const ARMCPRegInfo *ri) {uint64_t ret;
@@ -1551,9 +1583,13 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr), .writefn = pmovsset_write, .raw_writefn = raw_write }, -/* Unimplemented so WI. */ { .name = "PMSWINC", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 4, - .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NOP }, + .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NO_RAW, + .writefn = pmswinc_write }, + { .name = "PMSWINC_EL0", .state = ARM_CP_STATE_AA64, + .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 4, + .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NO_RAW, + .writefn = pmswinc_write }, { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5, .access = PL0_RW, .type = ARM_CP_ALIAS, .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmselr),
Signature · 1 line
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.\nQualcomm Technologies, Inc. is a member of the\nCode Aurora Forum, a Linux Foundation Collaborative Project.
Ping!
Unfortunately I'm not sure who to add other than the current recipients, but I'm eager for feedback and would love to work this into something that will allow for using the full ARM PMU.
I've also updated Peter Crosthwaite's email since the xilinx one appears to be stale.
-Aaron
Quoted 56 lines · Aaron Lindsay
The ARM PMU implementation currently contains a basic cycle counter, but it is often useful to gather counts of other events and filter them based on execution mode. These patches flesh out the implementations of various PMU registers including PM[X]EVCNTR and PM[X]EVTYPER, add a struct definition to represent arbitrary counter types, implement mode filtering, and add instruction, cycle, and software increment events.
I am particularly interested in feedback on the following two patches because I think I'm likely Doing It Wrong:
[1] target/arm: Filter cycle counter based onPMCCFILTR_EL0
[2] target/arm: PMU: Add instruction and cycle eventsIn order to implement mode filtering in an event-driven way, [1] adds a pair of calls to
pmu_sync()surrounding every update to a register/variable which may affect whether any counter is currently filtered. Thesepmu_sync()calls ultimately callcpu_get_icount_raw()for enabled instruction and cycle counters when using icount. Unfortunately,cpu->can_do_iomay otherwise be zero for these calls so the current implementation in [2] temporarily setscan_do_ioto
1. I haven't see any ill side effects from this in my testing, but it doesn't seem like the right way to handle this.I would like to eventually add sending interrupts on counter overflow. Suggestions for the best direction to handle this are most welcome.
Thanks for any feedback,
AaronAaron Lindsay (13):
target/arm: A53: Initialize PMCEID[0]
target/arm: Check PMCNTEN for whether PMCCNTR is enabled target/arm: Reorganize PMCCNTR read, write, sync target/arm: Mask PMU register writes based on PMCR_EL0.N target/arm: Allow AArch32 access for PMCCFILTR target/arm: Filter cycle counter based onPMCCFILTR_EL0target/arm: Implement PMOVSSET
target/arm: Splitarm_ccnt_enabledinto genericpmu_counter_enabledtarget/arm: Add array for supported PMU events, generate PMCEID[01] target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER target/arm: PMU: Add instruction and cycle events target/arm: PMU: Set PMCR.N to 4
target/arm: Implement PMSWINCtarget/arm/cpu.c | 10 +- target/arm/cpu.h | 34 +++- target/arm/cpu64.c | 2 + target/arm/helper.c | 535 +++++++++++++++++++++++++++++++++++++++++-------- target/arm/kvm64.c | 2 + target/arm/machine.c | 2 + target/arm/op_helper.c | 4 + 7 files changed, 500 insertions(+), 89 deletions(-)Signature · 3 lines
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Signature · 3 lines
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 3 lines · Aaron Lindsay
Unfortunately I'm not sure who to add other than the current recipients, but I'm eager for feedback and would love to work this into something that will allow for using the full ARM PMU.
Hi -- I do have this on my review queue, but unfortunately it's sitting behind some other fairly chunky hard-to-review patchsets.
As a first quick "is this going in the right direction" review based pretty much only on the cover letter:
What extra events do you want to try to support in the emulated PMU? Part of the reason we only support the cycle counter is because
(1) a lot of the events in a real PMU would be hard to support
(2) it's not clear to me that exposing events to the guest would be very useful to it anyway -- performance profiling of guest code running under emulation is fraught with difficulty
Giving more of an idea of what your use case is would help in evaluating these patches.
Some of what you're doing looks like it's fixing bugs in our current implementation, which is definitely fine in principle.
I haven't looked at the icount related stuff (and I can never remember how it works either) but fiddling with can_do_io does sound like it's not the right approach...
thanks
-- PMM
Quoted 7 lines · Peter Maydell
Quoted 3 lines · Aaron Lindsay
Unfortunately I'm not sure who to add other than the current recipients, but I'm eager for feedback and would love to work this into something that will allow for using the full ARM PMU.
Hi -- I do have this on my review queue, but unfortunately it's sitting behind some other fairly chunky hard-to-review patchsets.
No problem - I'll wait my turn.
Quoted 10 lines
As a first quick "is this going in the right direction" review based pretty much only on the cover letter:
What extra events do you want to try to support in the emulated PMU? Part of the reason we only support the cycle counter is because
(1) a lot of the events in a real PMU would be hard to support
(2) it's not clear to me that exposing events to the guest would be very useful to it anyway -- performance profiling of guest code running under emulation is fraught with difficulty
Quoted 2 lines
Giving more of an idea of what your use case is would help in evaluating these patches.
My goal isn't to expose events concerned with microarchitectural performance, but rather those that can help characterize architectural behavior (initially instructions and maybe branches, but perhaps anything in ARM ARM D5.10.4: "Common architectural event numbers"). We use a number of platforms at different points along the accuracy/speed trade-off continuum, and it is convenient to use self-hosted tools that we can expect to work on all of them.
For instance, implementing the instruction counter allows us to stop processes after executing prescribed numbers of instructions and resume them on other platforms for further study (using CRIU). It can also be useful to get a perf profile based on instruction count to get a rough idea of where an application is spending its time.
Quoted 2 lines
Some of what you're doing looks like it's fixing bugs in our current implementation, which is definitely fine in principle.
Yes, I believe patches 1-5 are fairly straightforward fixes, with the later patches getting into the more invasive changes.
Quoted 3 lines
I haven't looked at the icount related stuff (and I can never remember how it works either) but fiddling with
can_do_iodoes sound like it's not the right approach...
Agreed. Perhaps part of the same offense as the pmu_sync() calls scattered around - I was unable to find a better way to drive the mode filtering, and am more than glad to pursue a different approach if pointed in the right direction.
-Aaron
Signature · 3 lines
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 12 lines · Aaron Lindsay
The ARM PMU implementation currently contains a basic cycle counter, but it is often useful to gather counts of other events and filter them based on execution mode. These patches flesh out the implementations of various PMU registers including PM[X]EVCNTR and PM[X]EVTYPER, add a struct definition to represent arbitrary counter types, implement mode filtering, and add instruction, cycle, and software increment events.
I am particularly interested in feedback on the following two patches because I think I'm likely Doing It Wrong:
[1] target/arm: Filter cycle counter based onPMCCFILTR_EL0
[2] target/arm: PMU: Add instruction and cycle events
Quoted 8 lines
In order to implement mode filtering in an event-driven way, [1] adds a pair of calls to
pmu_sync()surrounding every update to a register/variable which may affect whether any counter is currently filtered. Thesepmu_sync()calls ultimately callcpu_get_icount_raw()for enabled instruction and cycle counters when using icount. Unfortunately,cpu->can_do_iomay otherwise be zero for these calls so the current implementation in [2] temporarily setscan_do_ioto
1. I haven't see any ill side effects from this in my testing, but it doesn't seem like the right way to handle this.
I've now reviewed the early stuff and provided what I hope is a useful direction for the mode-filtering, so I'm not going to look at the patches at the tail end on this version of the series.
Quoted 2 lines
I would like to eventually add sending interrupts on counter overflow. Suggestions for the best direction to handle this are most welcome.
Check out how the helper.c timer interrupts are wired up (the CPU object exposes outbound irq lines, which then get wired up by the board to the GIC.)
thanks
-- PMM
Quoted 22 lines · Peter Maydell
Quoted 12 lines · Aaron Lindsay
The ARM PMU implementation currently contains a basic cycle counter, but it is often useful to gather counts of other events and filter them based on execution mode. These patches flesh out the implementations of various PMU registers including PM[X]EVCNTR and PM[X]EVTYPER, add a struct definition to represent arbitrary counter types, implement mode filtering, and add instruction, cycle, and software increment events.
I am particularly interested in feedback on the following two patches because I think I'm likely Doing It Wrong:
[1] target/arm: Filter cycle counter based onPMCCFILTR_EL0
[2] target/arm: PMU: Add instruction and cycle eventsQuoted 8 lines
In order to implement mode filtering in an event-driven way, [1] adds a pair of calls to
pmu_sync()surrounding every update to a register/variable which may affect whether any counter is currently filtered. Thesepmu_sync()calls ultimately callcpu_get_icount_raw()for enabled instruction and cycle counters when using icount. Unfortunately,cpu->can_do_iomay otherwise be zero for these calls so the current implementation in [2] temporarily setscan_do_ioto
1. I haven't see any ill side effects from this in my testing, but it doesn't seem like the right way to handle this.
Quoted 3 lines
I've now reviewed the early stuff and provided what I hope is a useful direction for the mode-filtering, so I'm not going to look at the patches at the tail end on this version of the series.
Thank you, your review has been very helpful - the next pass of the mode-filtering patch will be more maintainable.
Quoted 6 lines
Quoted 2 lines
I would like to eventually add sending interrupts on counter overflow. Suggestions for the best direction to handle this are most welcome.
Check out how the
helper.ctimer interrupts are wired up (the CPU object exposes outbound irq lines, which then get wired up by the board to the GIC.)
Thanks for the pointer - I'll take a look.
-Aaron
Signature · 3 lines
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.