More fully implement ARM PMUv3
v5 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, send interrupts on counter overflow, and add instruction, cycle, and software increment events.
Since v4 I've added improved V7VE handling with Peter's direction, fixed up a few patch staging issues, and fixed a bug causing cycle counter overflow to be checked every instruction.
-Aaron
Aaron Lindsay (13):
target/arm: Reorganize PMCCNTR accesses
target/arm: Filter cycle counter based on PMCCFILTR_EL0 target/arm: Allow AArch32 access for PMCCFILTR target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions target/arm: Remove redundant DIV detection for KVM target/arm: Implement PMOVSSET
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: Mark PMINTENSET accesses as possibly doing IO target/arm: Send interrupts on PMU counter overflow
target/arm/cpu.c | 49 +++- target/arm/cpu.h | 71 +++++- target/arm/cpu64.c | 2 - target/arm/helper.c | 702 +++++++++++++++++++++++++++++++++++++++++++++------- target/arm/kvm32.c | 27 +- 5 files changed, 720 insertions(+), 131 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 v5 1/13 target/arm: Reorganize PMCCNTR accesses
pmccntr_read and pmccntr_write contained duplicate code that was already being handled by pmccntr_sync. Consolidate the duplicated code into two
functions: pmccntr_op_start and pmccntr_op_finish. Add a companion to c15_ccnt in CPUARMState so that we can simultaneously save both the architectural register value and the last underlying cycle count - this ensure time isn't lost and will also allow us to access the 'old' architectural register value in order to detect overflows in later patches.
--- target/arm/cpu.h | 28 ++++++++++----- target/arm/helper.c | 100 ++++++++++++++++++++++++++++------------------------ 2 files changed, 73 insertions(+), 55 deletions(-)
target/arm/cpu.h +20 -8
@@ -467,10 +467,20 @@ 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 +/* Stores the architectural value of the counter *the last time it was +* updated* by pmccntr_op_start. Accesses should always be surrounded +* by pmccntr_op_start/pmccntr_op_finish to guarantee the latest +* architecturally-corect value is being read/set. */ uint64_t c15_ccnt; +/* Stores the delta between the architectural value and the underlying +* cycle count during normal operation. It is used to update c15_ccnt +* to be the correct architectural value before accesses. During +* accesses, c15_ccnt_delta contains the underlying count being used +* for the access, after which it reverts to the delta value in +* pmccntr_op_finish. +*/ +uint64_t c15_ccnt_delta;uint64_t pmccfiltr_el0;/* Performance Monitor Filter Register */ uint64_t vpidr_el2;/* Virtualization Processor ID Register */ uint64_t vmpidr_el2;/* Virtualization Multiprocessor ID Register */
@@ -924,15 +934,17 @@ int cpu_arm_signal_handler(int host_signum, void *pinfo, void *puc);/** -* pmccntr_sync +* pmccntr_op_start/finish * @env: CPUARMState * -* Synchronises the counter in the PMCCNTR. This must always be called twice, -* once before any action that might affect 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) +* Convert the counter in the PMCCNTR between its delta form (the typical mode +* when it's enabled) and the guest-visible value. These two calls must always +* surround any action which might affect the counter, and the return value +* from pmccntr_op_start must be supplied as the second argument to +* pmccntr_op_finish. */ -void pmccntr_sync (CPUARMState *env); +void pmccntr_op_start (CPUARMState *env); +void pmccntr_op_finish (CPUARMState *env);/* SCTLR bit meanings. Several bits have been reused in newer * versions of the architecture; in that case we define constants
target/arm/helper.c +53 -47
145 lines not on this page · raw message
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 v5 01/13] target/arm: Reorganize PMCCNTR accesses
Quoted 20 lines · Aaron Lindsay
pmccntr_readandpmccntr_writecontained duplicate code that was already being handled bypmccntr_sync. Consolidate the duplicated code into two
functions:pmccntr_op_startandpmccntr_op_finish. Add a companion toc15_ccntin CPUARMState so that we can simultaneously save both the architectural register value and the last underlying cycle count - this ensure time isn't lost and will also allow us to access the 'old' architectural register value in order to detect overflows in later patches.Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/cpu.h | 28 ++++++++++----- target/arm/helper.c | 100 ++++++++++++++++++++++++++++------------------------ 2 files changed, 73 insertions(+), 55 deletions(-)target/arm/cpu.h +0
-0
target/arm/cpu.h typedef struct CPUARMState { · line 473
Quoted 9 lines
target/arm/cpu.h +4
-2@@ -467,10 +467,20 @@ 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 +/* Stores the architectural value of the counter *the last time it was +* updated* by pmccntr_op_start. Accesses should always be surrounded +* by pmccntr_op_start/pmccntr_op_finish to guarantee the latest +* architecturally-corect value is being read/set.
"correct"
target/arm/cpu.h typedef struct CPUARMState { · line 483
Quoted 10 lines
target/arm/cpu.h +8
-0*/ uint64_t c15_ccnt; +/* Stores the delta between the architectural value and the underlying +* cycle count during normal operation. It is used to update c15_ccnt +* to be the correct architectural value before accesses. During +* accesses, c15_ccnt_delta contains the underlying count being used +* for the access, after which it reverts to the delta value in +* pmccntr_op_finish. +*/ +uint64_t c15_ccnt_delta;
I had a look through the patchset, and I still can't see anything here which indicates how we're handling migration.
If this field is only ever used as a temporary while we're between op_start and op_finish, then we can just return it from start and take it as an argument to finish, and we're fine (migration of the PMCCNTR_EL0 register will be done by calling its readfn on the source and its writefn on the destination).
If there's a reason for having this be a field in the state struct (I think you said counter overflow)? then we need to be sure that migration is going to do the right thing and that we don't for instance lose overflow indications during a migration.
Quoted 14 lines
target/arm/cpu.h +1
-4uint64_t pmccfiltr_el0;/* Performance Monitor Filter Register */ uint64_t vpidr_el2;/* Virtualization Processor ID Register */ uint64_t vmpidr_el2;/* Virtualization Multiprocessor ID Register */ @@ -924,15 +934,17 @@ int cpu_arm_signal_handler(int host_signum, void *pinfo, void *puc);/** -* pmccntr_sync +* pmccntr_op_start/finish * @env: CPUARMState * -* Synchronises the counter in the PMCCNTR. This must always be called twice, -* once before any action that might affect the timer and again afterwards. -* The function is used to swap the state of the register if required.
target/arm/cpu.h int cpu_arm_signal_handler(int host_signum, void *pinfo, · line 947
Quoted 10 lines
target/arm/cpu.h +7
-2-* This only happens when not in user mode (!CONFIG_USER_ONLY) +* Convert the counter in the PMCCNTR between its delta form (the typical mode +* when it's enabled) and the guest-visible value. These two calls must always +* surround any action which might affect the counter, and the return value +* from pmccntr_op_start must be supplied as the second argument to +* pmccntr_op_finish. */ -void pmccntr_sync (CPUARMState *env); +void pmccntr_op_start (CPUARMState *env); +void pmccntr_op_finish (CPUARMState *env);
The comment says pmccntr_op_start() returns a value but the prototype says it doesn't...
thanks
-- PMM
Re: [Qemu-arm] [PATCH v5 01/13] target/arm: Reorganize PMCCNTR accesses
Quoted 17 lines · Peter Maydell
Quoted 5 lines · Aaron Lindsay
target/arm/cpu.h +0
-0target/arm/cpu.h typedef struct CPUARMState { · line 473
Quoted 9 lines
target/arm/cpu.h +4
-2@@ -467,10 +467,20 @@ 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 +/* Stores the architectural value of the counter *the last time it was +* updated* by pmccntr_op_start. Accesses should always be surrounded +* by pmccntr_op_start/pmccntr_op_finish to guarantee the latest +* architecturally-corect value is being read/set. "correct"
Thanks - fixed for v6.
Quoted 11 lines
target/arm/cpu.h typedef struct CPUARMState { · line 483
Quoted 10 lines
target/arm/cpu.h +8
-0*/ uint64_t c15_ccnt; +/* Stores the delta between the architectural value and the underlying +* cycle count during normal operation. It is used to update c15_ccnt +* to be the correct architectural value before accesses. During +* accesses, c15_ccnt_delta contains the underlying count being used +* for the access, after which it reverts to the delta value in +* pmccntr_op_finish. +*/ +uint64_t c15_ccnt_delta;
Quoted 2 lines
I had a look through the patchset, and I still can't see anything here which indicates how we're handling migration.
I'll admit that I had not previously thought through everything exhaustively, but I think I've convinced myself that *almost* everything is already handled by calling .readfn/writefn on the registers for migration... more below.
Quoted 10 lines
If this field is only ever used as a temporary while we're between
op_startandop_finish, then we can just return it from start and take it as an argument to finish, and we're fine (migration of thePMCCNTR_EL0register will be done by calling its readfn on the source and its writefn on the destination).If there's a reason for having this be a field in the state struct (I think you said counter overflow)? then we need to be sure that migration is going to do the right thing and that we don't for instance lose overflow indications during a migration.
I think we do need additional state for supporting counter overflow during normal operation (more than just between op_start and op_finish), but I don't think sending additional state over the wire is necessary for migration. Let me lay out my reasoning to see if you think it makes sense:
I observe that we seem to deal mostly with the following values with respect to PMU counters, and I think it might be easier to discuss this in terms of values rather than variables:
AC = current architectural value of PMCCNTR
AL = architectural value of PMCCNTR when last observed/updated by
the OS
UC = current underlying counter value
UL = underlying counter value when PMCCNTR last observed/updated by
OS
D = difference between underlying counter value and architectural
value
At any given point in time, the following should then be true during normal execution (i.e. between OS accesses to the counter):
UL - AL == D == UC - AC
The approach taken previous to my patchset was to store D in cp15.c15_ccnt most of the time, updating that variable to hold AC when read (by doing c15_ccnt = UC - c15_ccnt... an implementation using AC = UC - D, which is a different way to write D = UC - AC from above). This operation was reversed after a write completes, so that c15_ccnt again holds a value corresponding to D.
Only storing D works well and minimizes memory usage if you don't care about detecting overflows. However, without some frame of reference for what the last observed architectural value was relative to its current value, we have no way to know if the counter has overflowed in the intervening time. In other words, we need to know both AC and AL to know if the counter has overflowed. Assuming we store D and can obtain UC we can calculate AC, but we need to store at least one additional piece of information to obtain AL (either AL itself or UL).
In my patchset so far, I'm storing AL in c15_ccnt and D in c15_ccnt_delta during normal operation and AL/AC in c15_ccnt, UL/UC in c15_ccnt_delta between op_start and op_finish (note that AL == AC, UL == UC, and D == 0 during this time). Also note that UL/UC are only stored between op_start and op_finish calls to avoid losing time, and are not needed to maintain architectural state across migrations.
Now for migration - The read/writefn's already marshal/unmarshal both the counter value and overflow state to their architectural values in PMCCNTR and PMOVSCLR. I think this would work on its own if we made two big (and wrong) assumptions:
1. PMOVSCLR is read after PMCCNTR on save (ensures all overflows which have occurred are recorded in PMOVSCLR) and written before it on load (ensures any overflow bits in PMOVSCLR cause the interrupt line to be raised)
2. Time (as measured by qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)) does not change during the process of saving or loading CP registers (ensures PMCCNTR can't have overflowed between when it was saved and when PMOVSCLR was later saved).
One possible solution for this is to add calls to op_start in both cpu_pre_load and cpu_pre_save, add calls to op_finish in cpu_post_load and cpu_post_save, and ensure that all CP registers which would call op_start/finish on their own have raw_read/writefn's which assume they're between calls to op_start and op_finish.
Thoughts? (and I apologize for the wall of text)
-Aaron
Re: [Qemu-devel] [PATCH v5 01/13] target/arm: Reorganize PMCCNTR accesses
Quoted 8 lines · Aaron Lindsay
Quoted 2 lines · Peter Maydell
I had a look through the patchset, and I still can't see anything here which indicates how we're handling migration.
I'll admit that I had not previously thought through everything exhaustively, but I think I've convinced myself that *almost* everything is already handled by calling .readfn/writefn on the registers for migration... more below.
Thanks for writing up this explanation; it's very helpful. Sorry it's taken me a while to get to it -- you unfortunately sent it just as I was going on holiday...
Quoted 41 lines
Quoted 10 lines
If this field is only ever used as a temporary while we're between
op_startandop_finish, then we can just return it from start and take it as an argument to finish, and we're fine (migration of thePMCCNTR_EL0register will be done by calling its readfn on the source and its writefn on the destination).If there's a reason for having this be a field in the state struct (I think you said counter overflow)? then we need to be sure that migration is going to do the right thing and that we don't for instance lose overflow indications during a migration.
I think we do need additional state for supporting counter overflow during normal operation (more than just between
op_startandop_finish), but I don't think sending additional state over the wire is necessary for migration. Let me lay out my reasoning to see if you think it makes sense:I observe that we seem to deal mostly with the following values with respect to PMU counters, and I think it might be easier to discuss this in terms of values rather than variables:
AC = current architectural value of PMCCNTR
AL = architectural value of PMCCNTR when last observed/updated by
the OS
UC = current underlying counter value
UL = underlying counter value when PMCCNTR last observed/updated by
OS
D = difference between underlying counter value and architectural
valueAt any given point in time, the following should then be true during normal execution (i.e. between OS accesses to the counter):
UL - AL == D == UC - ACThe approach taken previous to my patchset was to store D in cp15.c15_ccnt most of the time, updating that variable to hold AC when read (by doing
c15_ccnt = UC - c15_ccnt... an implementation usingAC = UC - D, which is a different way to writeD = UC - ACfrom above). This operation was reversed after a write completes, so thatc15_ccntagain holds a value corresponding to D.
Quoted 8 lines
Only storing D works well and minimizes memory usage if you don't care about detecting overflows. However, without some frame of reference for what the last observed architectural value was relative to its current value, we have no way to know if the counter has overflowed in the intervening time. In other words, we need to know both AC and AL to know if the counter has overflowed. Assuming we store D and can obtain UC we can calculate AC, but we need to store at least one additional piece of information to obtain AL (either AL itself or UL).
In this scheme, how do you tell the difference between "AC > AL because it hasn't overflowed" and "AC > AL because it has overflowed and run all the way round past AL again" ? That is, I'm not sure that storing AL helps you to reliably detect overflow.
Quoted 7 lines
In my patchset so far, I'm storing AL in
c15_ccntand D inc15_ccnt_deltaduring normal operation and AL/AC inc15_ccnt, UL/UC inc15_ccnt_deltabetweenop_startandop_finish(note that AL == AC, UL == UC, and D == 0 during this time). Also note that UL/UC are only stored betweenop_startandop_finishcalls to avoid losing time, and are not needed to maintain architectural state across migrations.
Quoted 8 lines
Now for migration - The read/writefn's already marshal/unmarshal both the counter value and overflow state to their architectural values in PMCCNTR and PMOVSCLR. I think this would work on its own if we made two big (and wrong) assumptions:
1. PMOVSCLR is read after PMCCNTR on save (ensures all overflows which have occurred are recorded in PMOVSCLR) and written before it on load (ensures any overflow bits in PMOVSCLR cause the interrupt line to be raised)
There is indeed no guaranteed ordering in which registers are saved/loaded. Also, the load function mustn't raise interrupt lines: it has to transfer the state just of this device, and trust that the device on the other end restores its state into one that matches the current level of the interrupt line.
Quoted 4 lines
2. Time (as measured by
qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)) does not change during the process of saving or loading CP registers (ensures PMCCNTR can't have overflowed between when it was saved and when PMOVSCLR was later saved).
I think this assumption is true, isn't it? (VM migration happens while the VM is stopped, and do_vm_stop() calls cpu_disable_ticks() which means QEMU_CLOCK_VIRTUAL won't advance.)
thanks
-- PMM
Re: [Qemu-arm] [PATCH v5 01/13] target/arm: Reorganize PMCCNTR accesses
Quoted 5 lines · Peter Maydell
Quoted 4 lines · Aaron Lindsay
Quoted 2 lines · Peter Maydell
I had a look through the patchset, and I still can't see anything here which indicates how we're handling migration.
Quoted 8 lines
Quoted 4 lines
I'll admit that I had not previously thought through everything exhaustively, but I think I've convinced myself that *almost* everything is already handled by calling .readfn/writefn on the registers for migration... more below.
Thanks for writing up this explanation; it's very helpful. Sorry it's taken me a while to get to it -- you unfortunately sent it just as I was going on holiday...
No problem - I noticed you went quiet on the list right when I sent it and figured I scared you away!
I've got a v6 staged - If you don't come up with significant design changes based on my response below, I'll plan to send it in the next few days.
Quoted 50 lines
Quoted 41 lines
Quoted 10 lines
If this field is only ever used as a temporary while we're between
op_startandop_finish, then we can just return it from start and take it as an argument to finish, and we're fine (migration of thePMCCNTR_EL0register will be done by calling its readfn on the source and its writefn on the destination).If there's a reason for having this be a field in the state struct (I think you said counter overflow)? then we need to be sure that migration is going to do the right thing and that we don't for instance lose overflow indications during a migration.
I think we do need additional state for supporting counter overflow during normal operation (more than just between
op_startandop_finish), but I don't think sending additional state over the wire is necessary for migration. Let me lay out my reasoning to see if you think it makes sense:I observe that we seem to deal mostly with the following values with respect to PMU counters, and I think it might be easier to discuss this in terms of values rather than variables:
AC = current architectural value of PMCCNTR
AL = architectural value of PMCCNTR when last observed/updated by
the OS
UC = current underlying counter value
UL = underlying counter value when PMCCNTR last observed/updated by
OS
D = difference between underlying counter value and architectural
valueAt any given point in time, the following should then be true during normal execution (i.e. between OS accesses to the counter):
UL - AL == D == UC - ACThe approach taken previous to my patchset was to store D in cp15.c15_ccnt most of the time, updating that variable to hold AC when read (by doing
c15_ccnt = UC - c15_ccnt... an implementation usingAC = UC - D, which is a different way to writeD = UC - ACfrom above). This operation was reversed after a write completes, so thatc15_ccntagain holds a value corresponding to D.Quoted 8 lines
Only storing D works well and minimizes memory usage if you don't care about detecting overflows. However, without some frame of reference for what the last observed architectural value was relative to its current value, we have no way to know if the counter has overflowed in the intervening time. In other words, we need to know both AC and AL to know if the counter has overflowed. Assuming we store D and can obtain UC we can calculate AC, but we need to store at least one additional piece of information to obtain AL (either AL itself or UL).
Quoted 4 lines
In this scheme, how do you tell the difference between "AC > AL because it hasn't overflowed" and "AC > AL because it has overflowed and run all the way round past AL again" ? That is, I'm not sure that storing AL helps you to reliably detect overflow.
I wrestled with how to accomplish generic overflow detection in a foolproof way that fits with the event-driven design. My current implementation (see [1]) adds a function called ns_per_count for each counter type, which is used to setup a timer to overflow at the earliest time a given counter is expected to overflow. My thinking is that even if this isn't perfect (i.e. if there's a little 'skid'), it is likely to be close enough to prevent the type of wrap-around double-overflow you describe. I'm not wedded to this design - please don't hesitate to let me know if you think of something better.
Quoted 16 lines
Quoted 7 lines
In my patchset so far, I'm storing AL in
c15_ccntand D inc15_ccnt_deltaduring normal operation and AL/AC inc15_ccnt, UL/UC inc15_ccnt_deltabetweenop_startandop_finish(note that AL == AC, UL == UC, and D == 0 during this time). Also note that UL/UC are only stored betweenop_startandop_finishcalls to avoid losing time, and are not needed to maintain architectural state across migrations.Quoted 8 lines
Now for migration - The read/writefn's already marshal/unmarshal both the counter value and overflow state to their architectural values in PMCCNTR and PMOVSCLR. I think this would work on its own if we made two big (and wrong) assumptions:
1. PMOVSCLR is read after PMCCNTR on save (ensures all overflows which have occurred are recorded in PMOVSCLR) and written before it on load (ensures any overflow bits in PMOVSCLR cause the interrupt line to be raised)
Quoted 5 lines
There is indeed no guaranteed ordering in which registers are saved/loaded. Also, the load function mustn't raise interrupt lines: it has to transfer the state just of this device, and trust that the device on the other end restores its state into one that matches the current level of the interrupt line.
Okay, got it.
Quoted 8 lines
Quoted 4 lines
2. Time (as measured by
qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)) does not change during the process of saving or loading CP registers (ensures PMCCNTR can't have overflowed between when it was saved and when PMOVSCLR was later saved).I think this assumption is true, isn't it? (VM migration happens while the VM is stopped, and
do_vm_stop()callscpu_disable_ticks()which meansQEMU_CLOCK_VIRTUALwon't advance.)
Ah, okay. I wasn't aware of this detail.
-Aaron
[1] - http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg06843.html
PATCH v5 2/13 target/arm: Filter cycle counter based on PMCCFILTR_EL0
In reply to Aaron LindsayThe pmu_counter_enabled and pmu_op_start/finish functions are generic (as opposed to PMCCNTR-specific) to allow for the implementation of other events.
--- target/arm/cpu.c | 3 ++ target/arm/cpu.h | 22 +++++++++- target/arm/helper.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++----- 3 files changed, 129 insertions(+), 10 deletions(-)
target/arm/cpu.c +3 -0
@@ -916,6 +916,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)if (!cpu->has_pmu) {unset_feature (env,ARM_FEATURE_PMU ); cpu->id_aa64dfr0 &= ~0xf00 ; + }else if (!kvm_enabled ()) { +arm_register_pre_el_change_hook (cpu, &pmu_pre_el_change,0 ); +arm_register_el_change_hook (cpu, &pmu_post_el_change,0 ); }if (!arm_feature (env,ARM_FEATURE_EL2 )) {
target/arm/cpu.h +21 -1
@@ -946,6 +946,24 @@ int cpu_arm_signal_handler(int host_signum, void *pinfo,void pmccntr_op_start (CPUARMState *env);void pmccntr_op_finish (CPUARMState *env); +/** +* pmu_op_start/finish +* @env: CPUARMState +* +* Convert all PMU counters between their delta form (the typical mode when +* they are enabled) and the guest-visible values. These two calls must +* surround any action which might affect the counters, and the return value +* from pmu_op_start must be supplied as the second argument to pmu_op_finish. +*/ +void pmu_op_start (CPUARMState *env); +void pmu_op_finish (CPUARMState *env); + +/** +* Functions to register as EL change hooks for PMU mode filtering +*/ +void pmu_pre_el_change (ARMCPU *cpu,void *ignored); +void pmu_post_el_change (ARMCPU *cpu,void *ignored); +/* 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
@@ -1007,7 +1025,8 @@ void pmccntr_op_finish(CPUARMState *env);#define MDCR_EPMAD (1U << 21)#define MDCR_EDAD (1U << 20) -#define MDCR_SPME (1U << 17) +#define MDCR_SPME (1U << 17)/* MDCR_EL3 */ +#define MDCR_HPMD (1U << 17)/* MDCR_EL2 */ #define MDCR_SDD (1U << 16)#define MDCR_SPD (3U << 14)#define MDCR_TDRA (1U << 11)
@@ -1017,6 +1036,7 @@ void pmccntr_op_finish(CPUARMState *env);#define MDCR_HPME (1U << 7)#define MDCR_TPM (1U << 6)#define MDCR_TPMCR (1U << 5) +#define MDCR_HPMN (0x1fU)/* Not all of the MDCR_EL3 bits are present in the 32-bit SDCR */ #define SDCR_VALID_MASK (MDCR_EPMAD | MDCR_EDAD | MDCR_SPME | MDCR_SPD)
target/arm/helper.c +105 -9
168 lines not on this page · raw message
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 v5 02/13] target/arm: Filter cycle counter based on PMCCFILTR_EL0
Quoted 5 lines · Aaron Lindsay
The
pmu_counter_enabledand pmu_op_start/finish functions are generic (as opposed to PMCCNTR-specific) to allow for the implementation of other events.Signed-off-by Aaron Lindsay <[email protected]>
thanks
-- PMM
PATCH v5 3/13 target/arm: Allow AArch32 access for PMCCFILTR
In reply to Aaron Lindsay--- target/arm/helper.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)
target/arm/helper.c +26 -1
@@ -949,6 +949,10 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {#define PMXEVTYPER_MT 0x02000000#define PMXEVTYPER_EVTCOUNT 0x000003ff +#define PMCCFILTR 0xf8000000 +#define PMCCFILTR_M PMXEVTYPER_M +#define PMCCFILTR_EL0 (PMCCFILTR | PMCCFILTR_M) +static inline uint32_t pmu_num_counters (CPUARMState *env) {return (env->cp15.c9_pmcr &PMCRN_MASK ) >>PMCRN_SHIFT ;
@@ -1252,10 +1256,26 @@ static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) {pmccntr_op_start (env); - env->cp15.pmccfiltr_el0 = value &0xfc000000 ; + env->cp15.pmccfiltr_el0 = value &PMCCFILTR_EL0 ;pmccntr_op_finish (env); } +static void pmccfiltr_write_a32 (CPUARMState *env,const ARMCPRegInfo *ri, +uint64_t value) +{ +pmccntr_op_start (env); +/* M is not accessible from AArch32 */ + env->cp15.pmccfiltr_el0 = (env->cp15.pmccfiltr_el0 &PMCCFILTR_M ) | + (value &PMCCFILTR ); +pmccntr_op_finish (env); +} + +static uint64_t pmccfiltr_read_a32 (CPUARMState *env,const ARMCPRegInfo *ri) +{ +/* M is not visible in AArch32 */ +return env->cp15.pmccfiltr_el0 &PMCCFILTR ; +} +static void pmcntenset_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) {
@@ -1474,6 +1494,11 @@ 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 | ARM_CP_IO, + .resetvalue = 0, }, { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7, .writefn = pmccfiltr_write,
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 v5 03/13] target/arm: Allow AArch32 access for PMCCFILTR
Quoted 5 lines · Aaron Lindsay
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/helper.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)
thanks
-- PMM
PATCH v5 4/13 target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions
In reply to Aaron Lindsay--- target/arm/cpu.c | 21 ++++++++++++++------- target/arm/cpu.h | 1 + target/arm/kvm32.c | 8 ++++---- 3 files changed, 19 insertions(+), 11 deletions(-)
target/arm/cpu.c +14 -7
@@ -793,9 +793,20 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)/* Some features automatically imply others: */ if (arm_feature (env,ARM_FEATURE_V8 )) { -set_feature (env,ARM_FEATURE_V7 ); +set_feature (env,ARM_FEATURE_V7VE ); + } +if (arm_feature (env,ARM_FEATURE_V7VE )) { +/* v7 Virtualization Extensions. In real hardware this implies +* EL2 and also the presence of the Security Extensions. +* For QEMU, for backwards-compatibility we implement some +* CPUs or CPU configs which have no actual EL2 or EL3 but do +* include the various other features that V7VE implies. +* Presence of EL2 itself is ARM_FEATURE_EL2, and of the +* Security Extensions is ARM_FEATURE_EL3. +*/ set_feature (env,ARM_FEATURE_ARM_DIV );set_feature (env,ARM_FEATURE_LPAE ); +set_feature (env,ARM_FEATURE_V7 ); }if (arm_feature (env,ARM_FEATURE_V7 )) {set_feature (env,ARM_FEATURE_VAPA );
@@ -1509,15 +1520,13 @@ static void cortex_a7_initfn(Object *obj)ARMCPU *cpu =ARM_CPU (obj); cpu->dtb_compatible ="arm,cortex-a7" ; -set_feature (&cpu->env,ARM_FEATURE_V7 ); +set_feature (&cpu->env,ARM_FEATURE_V7VE );set_feature (&cpu->env,ARM_FEATURE_VFP4 );set_feature (&cpu->env,ARM_FEATURE_NEON );set_feature (&cpu->env,ARM_FEATURE_THUMB2EE ); -set_feature (&cpu->env,ARM_FEATURE_ARM_DIV );set_feature (&cpu->env,ARM_FEATURE_GENERIC_TIMER );set_feature (&cpu->env,ARM_FEATURE_DUMMY_C15_REGS );set_feature (&cpu->env,ARM_FEATURE_CBAR_RO ); -set_feature (&cpu->env,ARM_FEATURE_LPAE );set_feature (&cpu->env,ARM_FEATURE_EL3 ); cpu->kvm_target =QEMU_KVM_ARM_TARGET_CORTEX_A7 ; cpu->midr =0x410fc075 ;
@@ -1554,15 +1563,13 @@ static void cortex_a15_initfn(Object *obj)ARMCPU *cpu =ARM_CPU (obj); cpu->dtb_compatible ="arm,cortex-a15" ; -set_feature (&cpu->env,ARM_FEATURE_V7 ); +set_feature (&cpu->env,ARM_FEATURE_V7VE );set_feature (&cpu->env,ARM_FEATURE_VFP4 );set_feature (&cpu->env,ARM_FEATURE_NEON );set_feature (&cpu->env,ARM_FEATURE_THUMB2EE ); -set_feature (&cpu->env,ARM_FEATURE_ARM_DIV );set_feature (&cpu->env,ARM_FEATURE_GENERIC_TIMER );set_feature (&cpu->env,ARM_FEATURE_DUMMY_C15_REGS );set_feature (&cpu->env,ARM_FEATURE_CBAR_RO ); -set_feature (&cpu->env,ARM_FEATURE_LPAE );set_feature (&cpu->env,ARM_FEATURE_EL3 ); cpu->kvm_target =QEMU_KVM_ARM_TARGET_CORTEX_A15 ; cpu->midr =0x412fc0f1 ;
target/arm/cpu.h +1 -0
@@ -1474,6 +1474,7 @@ enum arm_features {ARM_FEATURE_OMAPCP ,/* OMAP specific CP15 ops handling. */ ARM_FEATURE_THUMB2EE ,ARM_FEATURE_V7MP ,/* v7 Multiprocessing Extensions */ +ARM_FEATURE_V7VE ,/* v7 Virtualization Extensions (non-EL2 parts) */ ARM_FEATURE_V4T ,ARM_FEATURE_V5 , ARM_FEATURE_STRONGARM,
target/arm/kvm32.c +4 -4
@@ -98,12 +98,12 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)/* Now we've retrieved all the register information we can * set the feature bits based on the ID register fields. * We can assume any KVM supporting CPU is at least a v7 -* with VFPv3, LPAE and the generic timers; this in turn implies -* most of the other feature bits, but a few must be tested. +* with VFPv3, virtualization extensions, and the generic +* timers; this in turn implies most of the other feature +* bits, but a few must be tested. */ -set_feature (&features,ARM_FEATURE_V7 ); +set_feature (&features,ARM_FEATURE_V7VE );set_feature (&features,ARM_FEATURE_VFP3 ); -set_feature (&features,ARM_FEATURE_LPAE );set_feature (&features,ARM_FEATURE_GENERIC_TIMER );switch (extract32 (id_isar0,24 ,4 )) {
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 v5 04/13] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions
Quoted 6 lines · Aaron Lindsay
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/cpu.c | 21 ++++++++++++++------- target/arm/cpu.h | 1 + target/arm/kvm32.c | 8 ++++---- 3 files changed, 19 insertions(+), 11 deletions(-)
thanks
-- PMM
PATCH v5 5/13 target/arm: Remove redundant DIV detection for KVM
In reply to Aaron LindsayKVM implies V7VE, which implies ARM_DIV and THUMB_DIV. The conditional detection here is therefore unnecessary. Because V7VE is already unconditionally specified for all KVM hosts, ARM_DIV and THUMB_DIV are already indirectly specified and do not need to be included here at all.
--- target/arm/kvm32.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-)
target/arm/kvm32.c +1 -18
@@ -36,7 +36,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)* and then query that CPU for the relevant ID registers. */ int i, ret, fdarray[3 ]; -uint32_t midr, id_pfr0, id_isar0, mvfr1; +uint32_t midr, id_pfr0, mvfr1;uint64_t features =0 ;/* Old kernels may not know about the PREFERRED_TARGET ioctl: however * we know these will only support creating one kind of guest CPU,
@@ -60,11 +60,6 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) }, { .id = KVM_REG_ARM | KVM_REG_SIZE_U32 - | ENCODE_CP_REG(15, 0, 0, 0, 2, 0, 0), - .addr = (uintptr_t)&id_isar0, - }, - { - .id = KVM_REG_ARM | KVM_REG_SIZE_U32 | KVM_REG_ARM_VFP | KVM_REG_ARM_VFP_MVFR1, .addr = (uintptr_t)&mvfr1, },
@@ -106,18 +101,6 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)set_feature (&features,ARM_FEATURE_VFP3 );set_feature (&features,ARM_FEATURE_GENERIC_TIMER ); -switch (extract32 (id_isar0,24 ,4 )) { -case 1 : -set_feature (&features,ARM_FEATURE_THUMB_DIV ); -break ; -case 2 : -set_feature (&features,ARM_FEATURE_ARM_DIV ); -set_feature (&features,ARM_FEATURE_THUMB_DIV ); -break ; -default : -break ; - } -if (extract32 (id_pfr0,12 ,4 ) ==1 ) {set_feature (&features,ARM_FEATURE_THUMB2EE ); }
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 v5 05/13] target/arm: Remove redundant DIV detection for KVM
Quoted 7 lines · Aaron Lindsay
KVM implies V7VE, which implies
ARM_DIVandTHUMB_DIV. The conditional detection here is therefore unnecessary. Because V7VE is already unconditionally specified for all KVM hosts,ARM_DIVandTHUMB_DIVare already indirectly specified and do not need to be included here at all.Signed-off-by Aaron Lindsay <[email protected]>---
thanks
-- PMM
PATCH v5 6/13 target/arm: Implement PMOVSSET
In reply to Aaron LindsayAdd an array for PMOVSSET so we only define it for v7ve+ platforms
--- target/arm/helper.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
target/arm/helper.c +28 -0
@@ -1293,9 +1293,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) {
@@ -1645,6 +1653,23 @@ static const ARMCPRegInfo v7mp_cp_reginfo[] = {REGINFO_SENTINEL }; +static const ARMCPRegInfo pmovsset_cp_reginfo[] = { +/* PMOVSSET is not implemented in v7 before v7ve */ + { .name ="PMOVSSET" , .cp =15 , .opc1 =0 , .crn =9 , .crm =14 , .opc2 =3 , + .access =PL0_RW , .accessfn = pmreg_access, + .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 }, +REGINFO_SENTINEL +}; +static void teecr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) {
@@ -4996,6 +5021,9 @@ void register_cp_regs_for_features(ARMCPU *cpu) !arm_feature(env, ARM_FEATURE_PMSA)) {define_arm_cp_regs (cpu, v7mp_cp_reginfo); } +if (arm_feature (env,ARM_FEATURE_V7VE )) { +define_arm_cp_regs (cpu, pmovsset_cp_reginfo); + }if (arm_feature (env,ARM_FEATURE_V7 )) {/* v7 performance monitor control register: same implementor * field as main ID register, and we implement only the cycle
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 v5 06/13] target/arm: Implement PMOVSSET
Quoted 12 lines · Aaron Lindsay
Add an array for PMOVSSET so we only define it for v7ve+ platforms
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/helper.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)target/arm/helper.c +0
-0
target/arm/helper.c static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri, · line 1298
Quoted 6 lines
target/arm/helper.c +1
-0@@ -1293,9 +1293,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; }
This change doesn't look like it should be in this patch ?
Quoted 12 lines
target/arm/helper.c +7
-0+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 v7mp_cp_reginfo[] = { · line 1662
Quoted 10 lines
target/arm/helper.c +7
-0@@ -1645,6 +1653,23 @@ static const ARMCPRegInfo v7mp_cp_reginfo[] = {REGINFO_SENTINEL }; +static const ARMCPRegInfo pmovsset_cp_reginfo[] = { +/* PMOVSSET is not implemented in v7 before v7ve */ + { .name ="PMOVSSET" , .cp =15 , .opc1 =0 , .crn =9 , .crm =14 , .opc2 =3 , + .access =PL0_RW , .accessfn = pmreg_access, + .fieldoffset =offsetoflow32 (CPUARMState, cp15.c9_pmovsr), + .writefn = pmovsset_write, + .raw_writefn = raw_write },
This should be marked ARM_CP_ALIAS, beacuse its underlying state in c9_pmovsr is just an alias into PMOVSR, and that register is handling reset and migration.
Quoted 14 lines
target/arm/helper.c +10
-0+ { .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 }, + REGINFO_SENTINEL +}; +static void teecr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) {
target/arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu)
Quoted 10 lines
target/arm/helper.c +3
-1@@ -4996,6 +5021,9 @@ void register_cp_regs_for_features(ARMCPU *cpu) !arm_feature(env, ARM_FEATURE_PMSA)) {define_arm_cp_regs (cpu, v7mp_cp_reginfo); } +if (arm_feature (env,ARM_FEATURE_V7VE )) { +define_arm_cp_regs (cpu, pmovsset_cp_reginfo); + }if (arm_feature (env,ARM_FEATURE_V7 )) {/* v7 performance monitor control register: same implementor * field as main ID register, and we implement only the cycle --
thanks
-- PMM
Re: [Qemu-devel] [PATCH v5 06/13] target/arm: Implement PMOVSSET
Quoted 8 lines · Peter Maydell
Quoted 7 lines · Aaron Lindsay
Add an array for PMOVSSET so we only define it for v7ve+ platforms
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/helper.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
Quoted 13 lines
target/arm/helper.c static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri, · line 1298
Quoted 11 lines
target/arm/helper.c +1
-0@@ -1293,9 +1293,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; }This change doesn't look like it should be in this patch ?
This has been appropriately split off into a separate patch for v6. I must've seen "pmovsr" and staged it here by accident.
Quoted 15 lines
Quoted 15 lines
target/arm/helper.c +7
-0+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) {@@ -1645,6 +1653,23 @@ static const ARMCPRegInfo v7mp_cp_reginfo[] = { REGINFO_SENTINEL };
Quoted 11 lines
target/arm/helper.c static const ARMCPRegInfo v7mp_cp_reginfo[] = { · line 1662
Quoted 7 lines
target/arm/helper.c +7
-0+static const ARMCPRegInfo pmovsset_cp_reginfo[] = { +/* PMOVSSET is not implemented in v7 before v7ve */ + { .name ="PMOVSSET" , .cp =15 , .opc1 =0 , .crn =9 , .crm =14 , .opc2 =3 , + .access =PL0_RW , .accessfn = pmreg_access, + .fieldoffset =offsetoflow32 (CPUARMState, cp15.c9_pmovsr), + .writefn = pmovsset_write, + .raw_writefn = raw_write },This should be marked
ARM_CP_ALIAS, beacuse its underlying state inc9_pmovsris just an alias into PMOVSR, and that register is handling reset and migration.
Thanks for catching this, too!
-Aaron
PATCH v5 7/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. Because the value of PMCEID[01] depends upon which events are supported at runtime, generate it dynamically.
--- target/arm/cpu.c | 20 +++++++++++++------- target/arm/cpu.h | 10 ++++++++++ target/arm/cpu64.c | 2 -- target/arm/helper.c | 37 +++++++++++++++++++++++++++++++++++++ 4 files changed, 60 insertions(+), 9 deletions(-)
target/arm/cpu.c +13 -7
@@ -927,9 +927,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)if (!cpu->has_pmu) {unset_feature (env,ARM_FEATURE_PMU ); cpu->id_aa64dfr0 &= ~0xf00 ; - }else if (!kvm_enabled ()) { -arm_register_pre_el_change_hook (cpu, &pmu_pre_el_change,0 ); -arm_register_el_change_hook (cpu, &pmu_post_el_change,0 ); + } +if (arm_feature (env,ARM_FEATURE_PMU )) { +uint64_t pmceid =get_pmceid (&cpu->env); + cpu->pmceid0 = pmceid &0xffffffff ; + cpu->pmceid1 = (pmceid >>32 ) &0xffffffff ; + +if (!kvm_enabled ()) { +arm_register_pre_el_change_hook (cpu, &pmu_pre_el_change,0 ); +arm_register_el_change_hook (cpu, &pmu_post_el_change,0 ); + } + }else { + cpu->pmceid0 =0x00000000 ; + cpu->pmceid1 =0x00000000 ; }if (!arm_feature (env,ARM_FEATURE_EL2 )) {
@@ -1538,8 +1548,6 @@ static void cortex_a7_initfn(Object *obj) cpu->id_pfr0 =0x00001131 ; cpu->id_pfr1 =0x00011011 ; cpu->id_dfr0 =0x02010555 ; - cpu->pmceid0 =0x00000000 ; - cpu->pmceid1 =0x00000000 ; cpu->id_afr0 =0x00000000 ; cpu->id_mmfr0 =0x10101105 ; cpu->id_mmfr1 =0x40000000 ;
@@ -1581,8 +1589,6 @@ static void cortex_a15_initfn(Object *obj) cpu->id_pfr0 =0x00001131 ; cpu->id_pfr1 =0x00011011 ; cpu->id_dfr0 =0x02010555 ; - cpu->pmceid0 =0x0000000 ; - cpu->pmceid1 =0x00000000 ; cpu->id_afr0 =0x00000000 ; cpu->id_mmfr0 =0x10201105 ; cpu->id_mmfr1 =0x20000000 ;
target/arm/cpu.h +10 -0
@@ -964,6 +964,16 @@ void pmu_op_finish(CPUARMState *env);void pmu_pre_el_change (ARMCPU *cpu,void *ignored);void pmu_post_el_change (ARMCPU *cpu,void *ignored); +/* +* 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/cpu64.c +0 -2
@@ -141,8 +141,6 @@ static void aarch64_a57_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 =0x00001124 ; cpu->dbgdidr =0x3516d000 ;
target/arm/helper.c +37 -0
@@ -964,6 +964,43 @@ static inline uint64_t pmu_counter_mask(CPUARMState *env)return (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 · 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 v5 07/13] target/arm: Add array for supported PMU events, generate PMCEID[01]
Quoted 28 lines · Aaron Lindsay
This commit doesn't add any supported events, but provides the framework for adding them. We store the
pm_eventstructs in a simple array, and provide the mapping from the event numbers to array indexes in thesupported_event_maparray. Because the value of PMCEID[01] depends upon which events are supported at runtime, generate it dynamically.Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/cpu.c | 20 +++++++++++++------- target/arm/cpu.h | 10 ++++++++++ target/arm/cpu64.c | 2 -- target/arm/helper.c | 37 +++++++++++++++++++++++++++++++++++++ 4 files changed, 60 insertions(+), 9 deletions(-)target/arm/cpu.c +3
-3@@ -927,9 +927,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)if (!cpu->has_pmu) {unset_feature (env,ARM_FEATURE_PMU ); cpu->id_aa64dfr0 &= ~0xf00 ; - }else if (!kvm_enabled ()) { -arm_register_pre_el_change_hook (cpu, &pmu_pre_el_change,0 ); -arm_register_el_change_hook (cpu, &pmu_post_el_change,0 ); + } +if (arm_feature (env,ARM_FEATURE_PMU )) { +uint64_t pmceid =get_pmceid (&cpu->env);
target/arm/cpu.c static void arm_cpu_realizefn(DeviceState *dev, Error **errp) · line 942
Quoted 10 lines
target/arm/cpu.c +10
-0+ cpu->pmceid0 = pmceid &0xffffffff ; + cpu->pmceid1 = (pmceid >>32 ) &0xffffffff ; + +if (!kvm_enabled ()) { +arm_register_pre_el_change_hook (cpu, &pmu_pre_el_change,0 ); +arm_register_el_change_hook (cpu, &pmu_post_el_change,0 ); + } + }else { + cpu->pmceid0 =0x00000000 ; + cpu->pmceid1 =0x00000000 ;
This is ok for now, but we really need to sort out what we're doing for the interplay between feature bits and ID register values more generally (notably for '-cpu max'), so depending on when we do that work this might end up needing to change a bit to fit in.
Quoted 60 lines
target/arm/cpu.c +0
-4}if (!arm_feature (env,ARM_FEATURE_EL2 )) {@@ -1538,8 +1548,6 @@ static void cortex_a7_initfn(Object *obj) cpu->id_pfr0 =0x00001131 ; cpu->id_pfr1 =0x00011011 ; cpu->id_dfr0 =0x02010555 ; - cpu->pmceid0 =0x00000000 ; - cpu->pmceid1 =0x00000000 ; cpu->id_afr0 =0x00000000 ; cpu->id_mmfr0 =0x10101105 ; cpu->id_mmfr1 =0x40000000 ;@@ -1581,8 +1589,6 @@ static void cortex_a15_initfn(Object *obj) cpu->id_pfr0 =0x00001131 ; cpu->id_pfr1 =0x00011011 ; cpu->id_dfr0 =0x02010555 ; - cpu->pmceid0 =0x0000000 ; - cpu->pmceid1 =0x00000000 ; cpu->id_afr0 =0x00000000 ; cpu->id_mmfr0 =0x10201105 ; cpu->id_mmfr1 =0x20000000 ;target/arm/cpu.h +10
-0@@ -964,6 +964,16 @@ void pmu_op_finish(CPUARMState *env);void pmu_pre_el_change (ARMCPU *cpu,void *ignored);void pmu_post_el_change (ARMCPU *cpu,void *ignored); +/* +* 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/cpu64.c +0
-2@@ -141,8 +141,6 @@ static void aarch64_a57_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 =0x00001124 ; cpu->dbgdidr =0x3516d000 ;target/arm/helper.c +0
-0
target/arm/helper.c static inline uint64_t pmu_counter_mask(CPUARMState *env) · line 972
Quoted 9 lines
target/arm/helper.c +6
-0@@ -964,6 +964,43 @@ static inline uint64_t pmu_counter_mask(CPUARMState *env)return (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 */
Don't leave the */ dangling on the RHS of a multiline comment. (CODING_STYLE now recommends Linux-kernel style.)
target/arm/helper.c static inline uint64_t pmu_counter_mask(CPUARMState *env) · line 980
Quoted 8 lines
target/arm/helper.c +8
-0+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 ];
What is this hardcoded 0x3f ?
target/arm/helper.c static inline uint64_t pmu_counter_mask(CPUARMState *env) · line 991
Quoted 11 lines
target/arm/helper.c +11
-0+ +/* +* 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 ) {
Are we ever going to run through some other array than the constant pm_events[] ? We could just use ARRAY_SIZE rather than requiring a sentinel member.
target/arm/helper.c static inline uint64_t pmu_counter_mask(CPUARMState *env) · line 993
Quoted 2 lines
target/arm/helper.c +2
-0+const pm_event *cnt = &pm_events[i]; +if (cnt->number <0x3f && cnt->supported (env)) {
...another 0x3f. Are we ever really going to see an out-of-range .number field, or should we just assert() that it's in range ?
target/arm/helper.c static inline uint64_t pmu_counter_mask(CPUARMState *env) · line 997
Quoted 4 lines
target/arm/helper.c +4
-0+ pmceid |= (1 << cnt->number); + supported_event_map[cnt->number] = i; + }else { + supported_event_map[cnt->number] =SUPPORTED_EVENT_SENTINEL ;
This will leave us with supported_event_map[] entries in one of three states:
* SUPPORTED_EVENT_SENTINEL if they have a specific pm_events[] entry marked as not supported
* an index if they are supported
* zero if there is no pm_events[] entry
Don't the ones in the last category get confused with the real index 0?
Maybe this should initialize the whole array first to the sentinel value and then just fill in the supported ones with their index values?
target/arm/helper.c static inline uint64_t pmu_counter_mask(CPUARMState *env) · line 1003
Quoted 6 lines
target/arm/helper.c +6
-0+ } + i++; + } +return pmceid; +} +
thanks
-- PMM
Re: [Qemu-arm] [PATCH v5 07/13] target/arm: Add array for supported PMU events, generate PMCEID[01]
Quoted 37 lines · Peter Maydell
Quoted 36 lines · Aaron Lindsay
This commit doesn't add any supported events, but provides the framework for adding them. We store the
pm_eventstructs in a simple array, and provide the mapping from the event numbers to array indexes in thesupported_event_maparray. Because the value of PMCEID[01] depends upon which events are supported at runtime, generate it dynamically.Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/cpu.c | 20 +++++++++++++------- target/arm/cpu.h | 10 ++++++++++ target/arm/cpu64.c | 2 -- target/arm/helper.c | 37 +++++++++++++++++++++++++++++++++++++ 4 files changed, 60 insertions(+), 9 deletions(-)target/arm/cpu.c +11
-3@@ -927,9 +927,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)if (!cpu->has_pmu) {unset_feature (env,ARM_FEATURE_PMU ); cpu->id_aa64dfr0 &= ~0xf00 ; - }else if (!kvm_enabled ()) { -arm_register_pre_el_change_hook (cpu, &pmu_pre_el_change,0 ); -arm_register_el_change_hook (cpu, &pmu_post_el_change,0 ); + } +if (arm_feature (env,ARM_FEATURE_PMU )) { +uint64_t pmceid =get_pmceid (&cpu->env); + cpu->pmceid0 = pmceid &0xffffffff ; + cpu->pmceid1 = (pmceid >>32 ) &0xffffffff ; + +if (!kvm_enabled ()) { +arm_register_pre_el_change_hook (cpu, &pmu_pre_el_change,0 ); +arm_register_el_change_hook (cpu, &pmu_post_el_change,0 ); + } + }else {
Quoted 8 lines
target/arm/cpu.c static void arm_cpu_realizefn(DeviceState *dev, Error **errp) · line 942
Quoted 2 lines
target/arm/cpu.c +2
-0+ cpu->pmceid0 =0x00000000 ; + cpu->pmceid1 =0x00000000 ;This is ok for now, but we really need to sort out what we're doing for the interplay between feature bits and ID register values more generally (notably for '-cpu max'), so depending on when we do that work this might end up needing to change a bit to fit in.
Okay, I'll try to be on the lookout for this work.
Quoted 73 lines
Quoted 60 lines
target/arm/cpu.c +0
-4}if (!arm_feature (env,ARM_FEATURE_EL2 )) {@@ -1538,8 +1548,6 @@ static void cortex_a7_initfn(Object *obj) cpu->id_pfr0 =0x00001131 ; cpu->id_pfr1 =0x00011011 ; cpu->id_dfr0 =0x02010555 ; - cpu->pmceid0 =0x00000000 ; - cpu->pmceid1 =0x00000000 ; cpu->id_afr0 =0x00000000 ; cpu->id_mmfr0 =0x10101105 ; cpu->id_mmfr1 =0x40000000 ;@@ -1581,8 +1589,6 @@ static void cortex_a15_initfn(Object *obj) cpu->id_pfr0 =0x00001131 ; cpu->id_pfr1 =0x00011011 ; cpu->id_dfr0 =0x02010555 ; - cpu->pmceid0 =0x0000000 ; - cpu->pmceid1 =0x00000000 ; cpu->id_afr0 =0x00000000 ; cpu->id_mmfr0 =0x10201105 ; cpu->id_mmfr1 =0x20000000 ;target/arm/cpu.h +10
-0@@ -964,6 +964,16 @@ void pmu_op_finish(CPUARMState *env);void pmu_pre_el_change (ARMCPU *cpu,void *ignored);void pmu_post_el_change (ARMCPU *cpu,void *ignored); +/* +* 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/cpu64.c +0
-2@@ -141,8 +141,6 @@ static void aarch64_a57_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 =0x00001124 ; cpu->dbgdidr =0x3516d000 ;target/arm/helper.c +0
-0target/arm/helper.c static inline uint64_t pmu_counter_mask(CPUARMState *env) · line 972
Quoted 9 lines
target/arm/helper.c +6
-0@@ -964,6 +964,43 @@ static inline uint64_t pmu_counter_mask(CPUARMState *env)return (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 */Don't leave the */ dangling on the RHS of a multiline comment. (
CODING_STYLEnow recommends Linux-kernel style.)
Quoted 10 lines
target/arm/helper.c static inline uint64_t pmu_counter_mask(CPUARMState *env) · line 980
Quoted 8 lines
target/arm/helper.c +8
-0+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 ];What is this hardcoded
0x3f?
This was the maximum common event ID (i.e. bits in PMCEID[01]) as of ARMv8.0. Obviously it should be a macro instead of a magic constant. I'm also debating whether this should be the maximum supported by the architecture or just the maximum supported by QEMU (particularly because later ARM versions raise this number significantly).
Quoted 7 lines
Quoted 7 lines
target/arm/helper.c +7
-0+ +/* +* 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)
Quoted 8 lines
target/arm/helper.c static inline uint64_t pmu_counter_mask(CPUARMState *env) · line 991
Quoted 4 lines
target/arm/helper.c +4
-0+{ +uint64_t pmceid =0 ; +unsigned int i =0 ; +while (pm_events[i].number !=SUPPORTED_EVENT_SENTINEL ) {Are we ever going to run through some other array than the constant pm_events[] ? We could just use
ARRAY_SIZErather than requiring a sentinel member.
No, I don't think so.
Quoted 7 lines
target/arm/helper.c static inline uint64_t pmu_counter_mask(CPUARMState *env) · line 993
Quoted 3 lines
target/arm/helper.c +2
-0+const pm_event *cnt = &pm_events[i]; +if (cnt->number <0x3f && cnt->supported (env)) {...another
0x3f. Are we ever really going to see an out-of-range .number field, or should we justassert()that it's in range ?
In some places this can be set by user code via PMEVTYPER.evtCount, but I think in this code an assert is the right way to handle it.
Quoted 13 lines
target/arm/helper.c static inline uint64_t pmu_counter_mask(CPUARMState *env) · line 997
Quoted 5 lines
target/arm/helper.c +4
-0+ pmceid |= (1 << cnt->number); + supported_event_map[cnt->number] = i; + }else { + supported_event_map[cnt->number] =SUPPORTED_EVENT_SENTINEL ;This will leave us with supported_event_map[] entries in one of three states:
*SUPPORTED_EVENT_SENTINELif they have a specific pm_events[] entry marked as not supported
* an index if they are supported
* zero if there is no pm_events[] entry
Quoted 6 lines
Don't the ones in the last category get confused with the real index 0?
Maybe this should initialize the whole array first to the sentinel value and then just fill in the supported ones with their index values?
I've re-written this code based on your feedback for v6, eliminating the sentinel and initializing the entirety of supported_event_map.
-Aaron
PATCH v5 8/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 logic to reset counters when PMCR.P is set. Update filtering code to support counters other than PMCCNTR.
--- target/arm/cpu.h | 3 + target/arm/helper.c | 224 +++++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 209 insertions(+), 18 deletions(-)
target/arm/cpu.h +3 -0
@@ -481,6 +481,9 @@ typedef struct CPUARMState {* pmccntr_op_finish. */ uint64_t c15_ccnt_delta; +uint64_t c14_pmevcntr[31 ]; +uint64_t c14_pmevcntr_delta[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 +206 -18
318 lines not on this page · raw message
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 v5 08/13] target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER
Quoted 52 lines · Aaron Lindsay
Add arrays to hold the registers, the definitions themselves, access functions, and logic to reset counters when PMCR.P is set. Update filtering code to support counters other than PMCCNTR.
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/cpu.h | 3 + target/arm/helper.c | 224 +++++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 209 insertions(+), 18 deletions(-)target/arm/cpu.h +3
-0@@ -481,6 +481,9 @@ typedef struct CPUARMState {* pmccntr_op_finish. */ uint64_t c15_ccnt_delta; +uint64_t c14_pmevcntr[31 ]; +uint64_t c14_pmevcntr_delta[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 +6
-3@@ -938,6 +938,7 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {#define PMCRDP 0x10#define PMCRD 0x8#define PMCRC 0x4 +#define PMCRP 0x2#define PMCRE 0x1#define PMXEVTYPER_P 0x80000000@@ -1120,9 +1121,11 @@ static inline bool pmu_counter_enabled(CPUARMState *env, uint8_t counter) prohibited = env->cp15.c9_pmcr &PMCRDP ; } -/* TODO Remove assert, set filter to correct PMEVTYPER */ -assert (counter ==31 ); - filter = env->cp15.pmccfiltr_el0; +if (counter ==31 ) { + filter = env->cp15.pmccfiltr_el0; + }else { + filter = env->cp15.c14_pmevtyper[counter]; + } p = filter &PMXEVTYPER_P ; u = filter &PMXEVTYPER_U ;
target/arm/helper.c static inline bool pmu_counter_enabled(CPUARMState *env, uint8_t counter) · line 1150
Quoted 6 lines
target/arm/helper.c +3
-0@@ -1142,6 +1145,21 @@ static inline bool pmu_counter_enabled(CPUARMState *env, uint8_t counter) filtered = m != p; } +if (counter !=31 ) { +/* If not checking PMCCNTR, ensure the counter is setup to an event we +* support */
Please use the right format for multiline comments (here and below).
Quoted 78 lines
target/arm/helper.c +39
-6static void pmuserenr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) {@@ -1552,16 +1698,23 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0), .resetvalue = 0, }, { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1, - .access = PL0_RW, .type = ARM_CP_NO_RAW, .accessfn = pmreg_access, + .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO, + .accessfn = pmreg_access, .writefn = pmxevtyper_write, .readfn = pmxevtyper_read }, { .name = "PMXEVTYPER_EL0", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 1, - .access = PL0_RW, .type = ARM_CP_NO_RAW, .accessfn = pmreg_access, + .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO, + .accessfn = pmreg_access, .writefn = pmxevtyper_write, .readfn = pmxevtyper_read }, - /* Unimplemented, RAZ/WI. */ { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2, - .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0, - .accessfn = pmreg_access_xevcntr }, + .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO, + .accessfn = pmreg_access_xevcntr, + .writefn = pmxevcntr_write, .readfn = pmxevcntr_read }, + { .name = "PMXEVCNTR_EL0", .state = ARM_CP_STATE_AA64, + .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 2, + .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO, + .accessfn = pmreg_access_xevcntr, + .writefn = pmxevcntr_write, .readfn = pmxevcntr_read }, { .name = "PMUSERENR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 0, .access = PL0_R | PL1_RW, .accessfn = access_tpm, .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmuserenr),@@ -4250,7 +4403,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {#endif /* The only field of MDCR_EL2 that has a defined architectural reset value * is MDCR_EL2.HPMN which should reset to the value of PMCR_EL0.N; but we -* don't impelment any PMU event counters, so using zero as a reset +* don't implement any PMU event counters, so using zero as a reset * value for MDCR_EL2 is okay */ { .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH,@@ -5062,6 +5215,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)define_arm_cp_regs (cpu, pmovsset_cp_reginfo); }if (arm_feature (env,ARM_FEATURE_V7 )) { +unsigned int i;/* v7 performance monitor control register: same implementor * field as main ID register, and we implement only the cycle * count register. @@ -5086,6 +5240,40 @@ void register_cp_regs_for_features(ARMCPU *cpu) };define_one_arm_cp_reg (cpu, &pmcr);define_one_arm_cp_reg (cpu, &pmcr64); +for (i =0 ; i <31 ; i++) { +char *pmevcntr_name =g_strdup_printf ("PMEVCNTR%d" , i); +char *pmevcntr_el0_name =g_strdup_printf ("PMEVCNTR%d_EL0" , i); +char *pmevtyper_name =g_strdup_printf ("PMEVTYPER%d" , i); +char *pmevtyper_el0_name =g_strdup_printf ("PMEVTYPER%d_EL0" , i); +ARMCPRegInfo pmev_regs[] = { + { .name = pmevcntr_name, .cp =15 , .crn =15 , + .crm =8 | (3 & (i >>3 )), .opc1 =0 , .opc2 = i &7 , + .access =PL0_RW , .type =ARM_CP_NO_RAW |ARM_CP_IO , + .readfn = pmevcntr_readfn, .writefn = pmevcntr_writefn, + .accessfn = pmreg_access }, + { .name = pmevcntr_el0_name, .state =ARM_CP_STATE_AA64 , + .opc0 =3 , .opc1 =3 , .crn =15 , .crm =8 | (3 & (i >>3 )), + .opc2 = i &7 , .access =PL0_RW , .accessfn = pmreg_access, + .type =ARM_CP_NO_RAW |ARM_CP_IO , + .readfn = pmevcntr_readfn, .writefn = pmevcntr_writefn }, + { .name = pmevtyper_name, .cp =15 , .crn =15 , + .crm =12 | (3 & (i >>3 )), .opc1 =0 , .opc2 = i &7 , + .access =PL0_RW , .type =ARM_CP_NO_RAW |ARM_CP_IO , + .readfn = pmevtyper_readfn, .writefn = pmevtyper_writefn, + .accessfn = pmreg_access }, + { .name = pmevtyper_el0_name, .state =ARM_CP_STATE_AA64 , + .opc0 =3 , .opc1 =3 , .crn =15 , .crm =12 | (3 & (i >>3 )), + .opc2 = i &7 , .access =PL0_RW , .accessfn = pmreg_access, + .type =ARM_CP_NO_RAW |ARM_CP_IO ,
target/arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 5277
Quoted 10 lines
target/arm/helper.c +9
-0+ .readfn = pmevtyper_readfn, .writefn = pmevtyper_writefn }, +REGINFO_SENTINEL + }; +define_arm_cp_regs (cpu, pmev_regs); +g_free (pmevcntr_name); +g_free (pmevcntr_el0_name); +g_free (pmevtyper_name); +g_free (pmevtyper_el0_name); + }#endif
Because all these registers are marked as NO_RAW we're going to fail to migrate their state, I think.
This patchset in general needs to address how the extra CPU state involved with the PMU is going to be migrated.
thanks
-- PMM
Re: [Qemu-arm] [PATCH v5 08/13] target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER
Quoted 56 lines · Peter Maydell
Quoted 55 lines · Aaron Lindsay
Add arrays to hold the registers, the definitions themselves, access functions, and logic to reset counters when PMCR.P is set. Update filtering code to support counters other than PMCCNTR.
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/cpu.h | 3 + target/arm/helper.c | 224 +++++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 209 insertions(+), 18 deletions(-)target/arm/cpu.h +3
-0@@ -481,6 +481,9 @@ typedef struct CPUARMState {* pmccntr_op_finish. */ uint64_t c15_ccnt_delta; +uint64_t c14_pmevcntr[31 ]; +uint64_t c14_pmevcntr_delta[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 +6
-3@@ -938,6 +938,7 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {#define PMCRDP 0x10#define PMCRD 0x8#define PMCRC 0x4 +#define PMCRP 0x2#define PMCRE 0x1#define PMXEVTYPER_P 0x80000000@@ -1120,9 +1121,11 @@ static inline bool pmu_counter_enabled(CPUARMState *env, uint8_t counter) prohibited = env->cp15.c9_pmcr &PMCRDP ; } -/* TODO Remove assert, set filter to correct PMEVTYPER */ -assert (counter ==31 ); - filter = env->cp15.pmccfiltr_el0; +if (counter ==31 ) { + filter = env->cp15.pmccfiltr_el0; + }else { + filter = env->cp15.c14_pmevtyper[counter]; + } p = filter &PMXEVTYPER_P ; u = filter &PMXEVTYPER_U ;@@ -1142,6 +1145,21 @@ static inline bool pmu_counter_enabled(CPUARMState *env, uint8_t counter) filtered = m != p; }
Quoted 5 lines
target/arm/helper.c static inline bool pmu_counter_enabled(CPUARMState *env, uint8_t counter) · line 1150
Quoted 3 lines
target/arm/helper.c +3
-0+if (counter !=31 ) { +/* If not checking PMCCNTR, ensure the counter is setup to an event we +* support */ Please use the right format for multiline comments (here and below).
Fixed.
Quoted 84 lines
Quoted 84 lines
target/arm/helper.c +44
-6static void pmuserenr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) {@@ -1552,16 +1698,23 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0), .resetvalue = 0, }, { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1, - .access = PL0_RW, .type = ARM_CP_NO_RAW, .accessfn = pmreg_access, + .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO, + .accessfn = pmreg_access, .writefn = pmxevtyper_write, .readfn = pmxevtyper_read }, { .name = "PMXEVTYPER_EL0", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 1, - .access = PL0_RW, .type = ARM_CP_NO_RAW, .accessfn = pmreg_access, + .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO, + .accessfn = pmreg_access, .writefn = pmxevtyper_write, .readfn = pmxevtyper_read }, - /* Unimplemented, RAZ/WI. */ { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2, - .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0, - .accessfn = pmreg_access_xevcntr }, + .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO, + .accessfn = pmreg_access_xevcntr, + .writefn = pmxevcntr_write, .readfn = pmxevcntr_read }, + { .name = "PMXEVCNTR_EL0", .state = ARM_CP_STATE_AA64, + .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 2, + .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO, + .accessfn = pmreg_access_xevcntr, + .writefn = pmxevcntr_write, .readfn = pmxevcntr_read }, { .name = "PMUSERENR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 0, .access = PL0_R | PL1_RW, .accessfn = access_tpm, .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmuserenr),@@ -4250,7 +4403,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {#endif /* The only field of MDCR_EL2 that has a defined architectural reset value * is MDCR_EL2.HPMN which should reset to the value of PMCR_EL0.N; but we -* don't impelment any PMU event counters, so using zero as a reset +* don't implement any PMU event counters, so using zero as a reset * value for MDCR_EL2 is okay */ { .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH,@@ -5062,6 +5215,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)define_arm_cp_regs (cpu, pmovsset_cp_reginfo); }if (arm_feature (env,ARM_FEATURE_V7 )) { +unsigned int i;/* v7 performance monitor control register: same implementor * field as main ID register, and we implement only the cycle * count register. @@ -5086,6 +5240,40 @@ void register_cp_regs_for_features(ARMCPU *cpu) };define_one_arm_cp_reg (cpu, &pmcr);define_one_arm_cp_reg (cpu, &pmcr64); +for (i =0 ; i <31 ; i++) { +char *pmevcntr_name =g_strdup_printf ("PMEVCNTR%d" , i); +char *pmevcntr_el0_name =g_strdup_printf ("PMEVCNTR%d_EL0" , i); +char *pmevtyper_name =g_strdup_printf ("PMEVTYPER%d" , i); +char *pmevtyper_el0_name =g_strdup_printf ("PMEVTYPER%d_EL0" , i); +ARMCPRegInfo pmev_regs[] = { + { .name = pmevcntr_name, .cp =15 , .crn =15 , + .crm =8 | (3 & (i >>3 )), .opc1 =0 , .opc2 = i &7 , + .access =PL0_RW , .type =ARM_CP_NO_RAW |ARM_CP_IO , + .readfn = pmevcntr_readfn, .writefn = pmevcntr_writefn, + .accessfn = pmreg_access }, + { .name = pmevcntr_el0_name, .state =ARM_CP_STATE_AA64 , + .opc0 =3 , .opc1 =3 , .crn =15 , .crm =8 | (3 & (i >>3 )), + .opc2 = i &7 , .access =PL0_RW , .accessfn = pmreg_access, + .type =ARM_CP_NO_RAW |ARM_CP_IO , + .readfn = pmevcntr_readfn, .writefn = pmevcntr_writefn }, + { .name = pmevtyper_name, .cp =15 , .crn =15 , + .crm =12 | (3 & (i >>3 )), .opc1 =0 , .opc2 = i &7 , + .access =PL0_RW , .type =ARM_CP_NO_RAW |ARM_CP_IO , + .readfn = pmevtyper_readfn, .writefn = pmevtyper_writefn, + .accessfn = pmreg_access }, + { .name = pmevtyper_el0_name, .state =ARM_CP_STATE_AA64 , + .opc0 =3 , .opc1 =3 , .crn =15 , .crm =12 | (3 & (i >>3 )), + .opc2 = i &7 , .access =PL0_RW , .accessfn = pmreg_access, + .type =ARM_CP_NO_RAW |ARM_CP_IO , + .readfn = pmevtyper_readfn, .writefn = pmevtyper_writefn }, +REGINFO_SENTINEL + }; +define_arm_cp_regs (cpu, pmev_regs); +g_free (pmevcntr_name);
Quoted 8 lines
target/arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 5277
Quoted 5 lines
target/arm/helper.c +4
-0+g_free (pmevcntr_el0_name); +g_free (pmevtyper_name); + g_free(pmevtyper_el0_name); + }#endif Because all these registers are marked as
NO_RAWwe're going to fail to migrate their state, I think.
A bad copy-paste job from the PMXEVCNTR variants, I'll warrant. Fixed now.
Quoted 2 lines
This patchset in general needs to address how the extra CPU state involved with the PMU is going to be migrated.
I've now commented elsewhere about general ideas for migration, but I'm wondering if there is a good way to set up unit tests for this code because I don't trust myself to get migration right (in particular because it's not my use case, so I won't be "naturally" testing it on my own).
-Aaron
PATCH v5 9/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. Always defining get_cycle_count (but altering its behavior depending on CONFIG_USER_ONLY) allows us to remove some CONFIG_USER_ONLY #defines throughout the rest of the code.
--- target/arm/helper.c | 88 ++++++++++++++++++++++++++--------------------------- 1 file changed, 43 insertions(+), 45 deletions(-)
target/arm/helper.c +43 -45
@@ -15,6 +15,7 @@#include "arm_ldst.h" #include <zlib.h> /* For crc32 */ #include "exec/semihost.h" +#include "sysemu/cpus.h" #include "sysemu/kvm.h" #include "fpu/softfloat.h"
@@ -974,8 +975,49 @@ typedef struct pm_event {uint64_t (*get_count)(CPUARMState *); } pm_event; +static bool event_always_supported (CPUARMState *env) +{ +return true; +} + +/* +* Return the underlying cycle count for the PMU cycle counters. If we're in +* usermode, simply return 0. +*/ +static uint64_t cycles_get_count (CPUARMState *env) +{ +#ifndef CONFIG_USER_ONLY +return muldiv64 (qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ), +ARM_CPU_FREQ ,NANOSECONDS_PER_SECOND ); +#else +return 0 ; +#endif +} + +#ifndef CONFIG_USER_ONLY +static bool instructions_supported (CPUARMState *env) +{ +return use_icount ==1 /* Precise instruction counting */ ; +} + +static uint64_t instructions_get_count (CPUARMState *env) +{ +return (uint64_t )cpu_get_icount_raw (); +} +#endif +#define SUPPORTED_EVENT_SENTINEL UINT16_MAXstatic const pm_event pm_events[] = { +#ifndef CONFIG_USER_ONLY + { .number =0x008 ,/* INST_RETIRED, Instruction architecturally executed */ + .supported = instructions_supported, + .get_count = instructions_get_count + }, + { .number =0x011 ,/* CPU_CYCLES, Cycle */ + .supported = event_always_supported, + .get_count = cycles_get_count + }, +#endif { .number =SUPPORTED_EVENT_SENTINEL } };static uint16_t supported_event_map[0x3f ];
@@ -1055,8 +1097,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)
@@ -1171,9 +1211,7 @@ static inline bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)*/ void pmccntr_op_start (CPUARMState *env) { -uint64_t cycles =0 ; - cycles =muldiv64 (qemu_clock_get_ns (QEMU_CLOCK_VIRTUAL ), -ARM_CPU_FREQ ,NANOSECONDS_PER_SECOND ); +uint64_t cycles =cycles_get_count (env);if (pmu_counter_enabled (env,31 )) {uint64_t eff_cycles = cycles;
@@ -1316,42 +1354,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_op_start (CPUARMState *env) -{ -} - -void pmccntr_op_finish (CPUARMState *env) -{ -} - -void pmevcntr_op_start (CPUARMState *env,uint8_t i) -{ -} - -void pmevcntr_op_finish (CPUARMState *env,uint8_t i) -{ -} - -void pmu_op_start (CPUARMState *env) -{ -} - -void pmu_op_finish (CPUARMState *env) -{ -} - -void pmu_pre_el_change (ARMCPU *cpu,void *ignored) -{ -} - -void pmu_post_el_change (ARMCPU *cpu,void *ignored) -{ -} - -#endif -static void pmccfiltr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) {
@@ -1664,7 +1666,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),
@@ -1684,7 +1685,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,
@@ -5220,7 +5220,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 ,
@@ -5274,7 +5273,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 · 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 v5 09/13] target/arm: PMU: Add instruction and cycle events
Quoted 6 lines · Aaron Lindsay
The instruction event is only enabled when icount is used, cycles are always supported. Always defining
get_cycle_count(but altering its behavior depending onCONFIG_USER_ONLY) allows us to remove someCONFIG_USER_ONLY#defines throughout the rest of the code.Signed-off-by Aaron Lindsay <[email protected]>
thanks
-- PMM
PATCH v5 10/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 | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
target/arm/helper.c +3 -2
@@ -1618,7 +1618,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .access = PL1_W, .type = ARM_CP_NOP },/* Performance monitors are implementation defined in v7, * but with an ARM recommended set of registers, which we -* follow (although we don't actually implement any counters) +* follow. * * Performance registers fall into three categories: * (a) always UNDEF in PL0, RW in PL1 (PMINTENSET, PMINTENCLR)
@@ -5234,7 +5234,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 · 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 v5 11/13 target/arm: Implement PMSWINC
In reply to Aaron Lindsay--- target/arm/helper.c | 39 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-)
target/arm/helper.c +37 -2
@@ -980,6 +980,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 there is no underlying count maintained by the PMU itself +*/ +return 0 ; +} +/* * Return the underlying cycle count for the PMU cycle counters. If we're in * usermode, simply return 0.
@@ -1008,6 +1017,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, Instruction architecturally executed */ .supported = instructions_supported,
@@ -1318,6 +1331,24 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,pmu_op_finish (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) && +/* counter is SW_INCR */ + (env->cp15.c14_pmevtyper[i] &PMXEVTYPER_EVTCOUNT ) ==0x0 ) { +pmevcntr_op_start (env, i); + env->cp15.c14_pmevcntr[i]++; +pmevcntr_op_finish (env, i); + } + } +} +static uint64_t pmccntr_read (CPUARMState *env,const ARMCPRegInfo *ri) {uint64_t ret;
@@ -1663,9 +1694,13 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr), .writefn = pmovsr_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 · 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 v5 12/13 target/arm: Mark PMINTENSET accesses as possibly doing IO
In reply to Aaron LindsayThis makes it match its AArch64 equivalent, PMINTENSET_EL1
--- target/arm/helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
target/arm/helper.c +1 -1
@@ -1763,7 +1763,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .writefn = pmuserenr_write, .raw_writefn = raw_write }, { .name = "PMINTENSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 1, .access = PL1_RW, .accessfn = access_tpm, - .type = ARM_CP_ALIAS, + .type = ARM_CP_ALIAS | ARM_CP_IO, .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pminten), .resetvalue = 0, .writefn = pmintenset_write, .raw_writefn = raw_write },
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 v5 12/13] target/arm: Mark PMINTENSET accesses as possibly doing IO
Quoted 12 lines · Aaron Lindsay
This makes it match its AArch64 equivalent,
PMINTENSET_EL1Signed-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 const ARMCPRegInfo v7_cp_reginfo[] = { · line 1769
Quoted 8 lines
target/arm/helper.c +1
-1@@ -1763,7 +1763,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .writefn = pmuserenr_write, .raw_writefn = raw_write }, { .name = "PMINTENSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 1, .access = PL1_RW, .accessfn = access_tpm, - .type = ARM_CP_ALIAS, + .type = ARM_CP_ALIAS | ARM_CP_IO, .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pminten), .resetvalue = 0, .writefn = pmintenset_write, .raw_writefn = raw_write },
Shouldn't PMINTENCLR and PMINTENCLR_EL1 also be ARM_CP_IO ?
thanks
-- PMM
Re: [Qemu-devel] [PATCH v5 12/13] target/arm: Mark PMINTENSET accesses as possibly doing IO
Quoted 1 line · Peter Maydell
Shouldn't PMINTENCLR and
PMINTENCLR_EL1also beARM_CP_IO?
Yes, that was an oversight - I've added a patch addressing this for v6.
-Aaron
PATCH v5 13/13 target/arm: Send interrupts on PMU counter overflow
In reply to Aaron LindsaySetup a QEMUTimer to get a callback when we expect counters to next overflow and trigger an interrupt at that time.
--- target/arm/cpu.c | 11 +++++ target/arm/cpu.h | 7 +++ target/arm/helper.c | 132 ++++++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 141 insertions(+), 9 deletions(-)
target/arm/cpu.c +11 -0
@@ -743,6 +743,12 @@ static void arm_cpu_finalizefn(Object *obj)QLIST_REMOVE (hook, node);g_free (hook); } +#ifndef CONFIG_USER_ONLY +if (arm_feature (&cpu->env,ARM_FEATURE_PMU )) { +timer_deinit (cpu->pmu_timer); +timer_free (cpu->pmu_timer); + } +#endif }static void arm_cpu_realizefn (DeviceState *dev,Error **errp)
@@ -937,6 +943,11 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)arm_register_pre_el_change_hook (cpu, &pmu_pre_el_change,0 );arm_register_el_change_hook (cpu, &pmu_post_el_change,0 ); } + +#ifndef CONFIG_USER_ONLY + cpu->pmu_timer =timer_new (QEMU_CLOCK_VIRTUAL ,1 , arm_pmu_timer_cb, + cpu); +#endif }else { cpu->pmceid0 =0x00000000 ; cpu->pmceid1 =0x00000000 ;
target/arm/cpu.h +7 -0
@@ -720,6 +720,8 @@ struct ARMCPU {/* Timers used by the generic (architected) timer */ QEMUTimer *gt_timer[NUM_GTIMERS ]; +/* Timer used by the PMU */ +QEMUTimer *pmu_timer;/* GPIO outputs for generic timer */ qemu_irq gt_timer_outputs[NUM_GTIMERS ];/* GPIO output for GICv3 maintenance interrupt signal */
@@ -962,6 +964,11 @@ void pmu_op_start(CPUARMState *env);void pmu_op_finish (CPUARMState *env);/** +* Called when a PMU counter is due to overflow +*/ +void arm_pmu_timer_cb (void *opaque); + +/** * Functions to register as EL change hooks for PMU mode filtering */ void pmu_pre_el_change (ARMCPU *cpu,void *ignored);
target/arm/helper.c +123 -9
264 lines not on this page · raw message
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 v5 13/13] target/arm: Send interrupts on PMU counter overflow
Quoted 28 lines · Aaron Lindsay
Setup a QEMUTimer to get a callback when we expect counters to next overflow and trigger an interrupt at that time.
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/cpu.c | 11 +++++ target/arm/cpu.h | 7 +++ target/arm/helper.c | 132 ++++++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 141 insertions(+), 9 deletions(-)target/arm/cpu.c +6
-0@@ -743,6 +743,12 @@ static void arm_cpu_finalizefn(Object *obj)QLIST_REMOVE (hook, node);g_free (hook); } +#ifndef CONFIG_USER_ONLY +if (arm_feature (&cpu->env,ARM_FEATURE_PMU )) { +timer_deinit (cpu->pmu_timer); +timer_free (cpu->pmu_timer); + } +#endif }static void arm_cpu_realizefn (DeviceState *dev,Error **errp)
target/arm/cpu.c static void arm_cpu_realizefn(DeviceState *dev, Error **errp) · line 949
Quoted 7 lines
target/arm/cpu.c +4
-0@@ -937,6 +943,11 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)arm_register_pre_el_change_hook (cpu, &pmu_pre_el_change,0 );arm_register_el_change_hook (cpu, &pmu_post_el_change,0 ); } + +#ifndef CONFIG_USER_ONLY + cpu->pmu_timer =timer_new (QEMU_CLOCK_VIRTUAL ,1 , arm_pmu_timer_cb, + cpu);
This is a confusing way to write timer_new_ns().
target/arm/cpu.c
Quoted 9 lines
target/arm/cpu.c +1
-0+#endif #define SUPPORTED_EVENT_SENTINEL UINT16_MAXstatic const pm_event pm_events[] = {{ .number =
0x000, /*SW_INCR*/
.supported =event_always_supported,target/arm/cpu.c +2
-1- .get_count = swinc_get_count + .get_count = swinc_get_count, + .ns_per_count = swinc_ns_per
If you always put trailing commas on the end of the last line in a struct initializer, then you don't need to modify the preceding line in later patches which add a new line to the initializer.
The meat of the patch is beyond me at this time of the afternoon :-)
thanks
-- PMM
Quoted 10 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, send interrupts on counter overflow, and add instruction, cycle, and software increment events.
Since v4 I've added improved V7VE handling with Peter's direction, fixed up a few patch staging issues, and fixed a bug causing cycle counter overflow to be checked every instruction.
Hi; I've reviewed some of the simpler parts of this, but I suspect I'm not going to get to the rest of it for a bit (the softfreeze deadline is next Tuesday, so it's a busy time of the release cycle for me). For the moment I'm going to take patches 4, 5 and 12 into target-arm.next, which should at least reduce the size of the patchset for next time.
thanks
-- PMM