More fully implement ARM PMUv3
v6 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, filter them based on execution mode, and/or be notified on counter overflow. 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 v5 [1] I have:
* Taken a first pass at addressing migration
* Restructured the list of supported events, and ensured they're all initialized
* Fixed aliasing for PMOVSSET
* Added ARM_CP_IO for PMINTENCLR and PMINTENCLR_EL1
* Addressed a few non-code issues (comment style, patch staging, spelling, etc.)
[1] - https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg06830.html
Aaron Lindsay (14):
target/arm: Mark PMINTENCLR and PMINTENCLR_EL1 accesses as possibly
doing IO
target/arm: Mask PMOVSR writes based on supported counters
migration: Add post_save function to VMStateDescription target/arm: Swap PMU values before/after migrations target/arm: Reorganize PMCCNTR accesses
target/arm: Filter cycle counter based on PMCCFILTR_EL0 target/arm: Allow AArch32 access for PMCCFILTR 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: Send interrupts on PMU counter overflow
docs/devel/migration.rst | 9 +- include/migration/vmstate.h | 1 + migration/vmstate.c | 10 +- target/arm/cpu.c | 28 +- target/arm/cpu.h | 68 +++- target/arm/cpu64.c | 2 - target/arm/helper.c | 781 ++++++++++++++++++++++++++++++++---- target/arm/machine.c | 19 + 8 files changed, 817 insertions(+), 101 deletions(-)
Signature · 1 line
2.19.1
PATCH v6 1/14 target/arm: Mark PMINTENCLR and PMINTENCLR_EL1 accesses as possibly doing IO
I previously fixed this for PMINTENSET_EL1, but missed these.
--- target/arm/helper.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
target/arm/helper.c +4 -2
@@ -1423,12 +1423,14 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .writefn = pmintenset_write, .raw_writefn = raw_write, .resetvalue = 0x0 }, { .name = "PMINTENCLR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 2, - .access = PL1_RW, .accessfn = access_tpm, .type = ARM_CP_ALIAS, + .access = PL1_RW, .accessfn = access_tpm, + .type = ARM_CP_ALIAS | ARM_CP_IO, .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten), .writefn = pmintenclr_write, }, { .name = "PMINTENCLR_EL1", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 2, - .access = PL1_RW, .accessfn = access_tpm, .type = ARM_CP_ALIAS, + .access = PL1_RW, .accessfn = access_tpm, + .type = ARM_CP_ALIAS | ARM_CP_IO, .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten), .writefn = pmintenclr_write }, { .name = "CCSIDR", .state = ARM_CP_STATE_BOTH,
Signature · 1 line
2.19.1
Re: [Qemu-devel] [PATCH v6 01/14] target/arm: Mark PMINTENCLR and PMINTENCLR_EL1 accesses as possibly doing IO
Quoted 7 lines · Aaron Lindsay
I previously fixed this for
PMINTENSET_EL1, but missed these.Signed-off-by Aaron Lindsay <[email protected]>Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/helper.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
r~
PATCH v6 2/14 target/arm: Mask PMOVSR writes based on supported counters
In reply to Aaron LindsayThis is an amendment to my earlier patch:
commit 7ece99b17e832065236c07a158dfac62619ef99b
Author: Aaron Lindsay <[email protected]>
Date: Thu Apr 26 11:04:39 2018 +0100
target/arm: Mask PMU register writes based on PMCR_EL0.N
--- target/arm/helper.c | 1 + 1 file changed, 1 insertion(+)
target/arm/helper.c +1 -0
@@ -1179,6 +1179,7 @@ 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; }
Signature · 1 line
2.19.1
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 02/14] target/arm: Mask PMOVSR writes based on supported counters
Quoted 11 lines · Aaron Lindsay
This is an amendment to my earlier patch:
commit7ece99b17e832065236c07a158dfac62619ef99b
Author: Aaron Lindsay <[email protected]>
Date: Thu Apr 26 11:04:39 2018 +0100target/arm: Mask PMU register writes based on PMCR_EL0.N
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/helper.c | 1 + 1 file changed, 1 insertion(+)
r~
PATCH v6 3/14 migration: Add post_save function to VMStateDescription
In reply to Aaron LindsayIn some cases it may be helpful to modify state before saving it for migration, and then modify the state back after it has been saved. The existing pre_save function provides half of this functionality. This patch adds a post_save function to provide the second half.
--- docs/devel/migration.rst | 9 +++++++-- include/migration/vmstate.h | 1 + migration/vmstate.c | 10 +++++++++- 3 files changed, 17 insertions(+), 3 deletions(-)
docs/devel/migration.rst +7 -2
@@ -419,8 +419,13 @@ The functions to do that are inside a vmstate definition, and are called: This function is called before we save the state of one device. -Example: You can look at hpet.c, that uses the three function to -massage the state that is transferred. +- ``void (*post_save)(void *opaque);`` + + This function is called after we save the state of one device + (even upon failure, unless the call to pre_save returned and error). + +Example: You can look at hpet.c, that uses the first three functions +to massage the state that is transferred. The ``VMSTATE_WITH_TMP`` macro may be useful when the migration data doesn't match the stored device data well; it allows an
include/migration/vmstate.h +1 -0
@@ -185,6 +185,7 @@ struct VMStateDescription {int (*pre_load)(void *opaque);int (*post_load)(void *opaque,int version_id);int (*pre_save)(void *opaque); +void (*post_save)(void *opaque);bool (*needed)(void *opaque);VMStateField *fields;const VMStateDescription **subsections;
migration/vmstate.c +9 -1
@@ -387,6 +387,9 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,if (ret) {error_report ("Save of field %s/%s failed" , vmsd->name, field->name); +if (vmsd->post_save) { + vmsd->post_save (opaque); + }return ret; }
@@ -412,7 +415,12 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,json_end_array (vmdesc); } -return vmstate_subsection_save (f, vmsd, opaque, vmdesc); + ret =vmstate_subsection_save (f, vmsd, opaque, vmdesc); + +if (vmsd->post_save) { + vmsd->post_save (opaque); + } +return ret; } static const VMStateDescription *
Signature · 1 line
2.19.1
Re: [Qemu-devel] [PATCH v6 03/14] migration: Add post_save function to VMStateDescription
Quoted 11 lines · Aaron Lindsay
In some cases it may be helpful to modify state before saving it for migration, and then modify the state back after it has been saved. The existing
pre_savefunction provides half of this functionality. This patch adds apost_savefunction to provide the second half.Signed-off-by Aaron Lindsay <[email protected]>--- docs/devel/migration.rst | 9 +++++++-- include/migration/vmstate.h | 1 + migration/vmstate.c | 10 +++++++++- 3 files changed, 17 insertions(+), 3 deletions(-)
Hmm, maybe. I believe the common practice is for pre_save to copy state into a separate member on the side, so that conversion back isn't necessary.
Ccing in the migration maintainers for a second opinion.
r~
Quoted 62 lines
docs/devel/migration.rst +7
-2@@ -419,8 +419,13 @@ The functions to do that are inside a vmstate definition, and are called: This function is called before we save the state of one device. -Example: You can look at hpet.c, that uses the three function to -massage the state that is transferred. +- ``void (*post_save)(void *opaque);`` + + This function is called after we save the state of one device + (even upon failure, unless the call to pre_save returned and error). + +Example: You can look at hpet.c, that uses the first three functions +to massage the state that is transferred. The ``VMSTATE_WITH_TMP`` macro may be useful when the migration data doesn't match the stored device data well; it allows aninclude/migration/vmstate.h +1
-0@@ -185,6 +185,7 @@ struct VMStateDescription {int (*pre_load)(void *opaque);int (*post_load)(void *opaque,int version_id);int (*pre_save)(void *opaque); +void (*post_save)(void *opaque);bool (*needed)(void *opaque);VMStateField *fields;const VMStateDescription **subsections;migration/vmstate.c +9
-1@@ -387,6 +387,9 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,if (ret) {error_report ("Save of field %s/%s failed" , vmsd->name, field->name); +if (vmsd->post_save) { + vmsd->post_save (opaque); + }return ret; }@@ -412,7 +415,12 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,json_end_array (vmdesc); } -return vmstate_subsection_save (f, vmsd, opaque, vmdesc); + ret =vmstate_subsection_save (f, vmsd, opaque, vmdesc); + +if (vmsd->post_save) { + vmsd->post_save (opaque); + } +return ret; } static const VMStateDescription *
Re: [Qemu-devel] [PATCH v6 03/14] migration: Add post_save function to VMStateDescription
Quoted 13 lines · * Richard Henderson ([email protected]) wrote:
Quoted 11 lines · Aaron Lindsay
In some cases it may be helpful to modify state before saving it for migration, and then modify the state back after it has been saved. The existing
pre_savefunction provides half of this functionality. This patch adds apost_savefunction to provide the second half.Signed-off-by Aaron Lindsay <[email protected]>--- docs/devel/migration.rst | 9 +++++++-- include/migration/vmstate.h | 1 + migration/vmstate.c | 10 +++++++++- 3 files changed, 17 insertions(+), 3 deletions(-)
Quoted 4 lines
Hmm, maybe. I believe the common practice is for
pre_saveto copy state into a separate member on the side, so that conversion back isn't necessary.Ccing in the migration maintainers for a second opinion.
It is common to copy stuff into a separate member; however we do occasionally think that post_save would be a useful addition; so I think we should take it (if nothing else it actually makes stuff symmetric!).
Please make it return 'int' in the same way that pre_save/pre_load does, so that it can fail and stop the migration.
Dave
Quoted 64 lines
r~
Quoted 60 lines
docs/devel/migration.rst +7
-2@@ -419,8 +419,13 @@ The functions to do that are inside a vmstate definition, and are called: This function is called before we save the state of one device. -Example: You can look at hpet.c, that uses the three function to -massage the state that is transferred. +- ``void (*post_save)(void *opaque);`` + + This function is called after we save the state of one device + (even upon failure, unless the call to pre_save returned and error). + +Example: You can look at hpet.c, that uses the first three functions +to massage the state that is transferred. The ``VMSTATE_WITH_TMP`` macro may be useful when the migration data doesn't match the stored device data well; it allows aninclude/migration/vmstate.h +1
-0@@ -185,6 +185,7 @@ struct VMStateDescription {int (*pre_load)(void *opaque);int (*post_load)(void *opaque,int version_id);int (*pre_save)(void *opaque); +void (*post_save)(void *opaque);bool (*needed)(void *opaque);VMStateField *fields;const VMStateDescription **subsections;migration/vmstate.c +9
-1@@ -387,6 +387,9 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,if (ret) {error_report ("Save of field %s/%s failed" , vmsd->name, field->name); +if (vmsd->post_save) { + vmsd->post_save (opaque); + }return ret; }@@ -412,7 +415,12 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,json_end_array (vmdesc); } -return vmstate_subsection_save (f, vmsd, opaque, vmdesc); + ret =vmstate_subsection_save (f, vmsd, opaque, vmdesc); + +if (vmsd->post_save) { + vmsd->post_save (opaque); + } +return ret; }
Quoted 3 lines
migration/vmstate.c int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd, · line 426
Quoted 2 lines
migration/vmstate.c +0
-0static const VMStateDescription *
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [Qemu-devel] [PATCH v6 03/14] migration: Add post_save function to VMStateDescription
Quoted 17 lines · Dr. David Alan Gilbert
Quoted 16 lines · * Richard Henderson ([email protected]) wrote:
Quoted 11 lines · Aaron Lindsay
In some cases it may be helpful to modify state before saving it for migration, and then modify the state back after it has been saved. The existing
pre_savefunction provides half of this functionality. This patch adds apost_savefunction to provide the second half.Signed-off-by Aaron Lindsay <[email protected]>--- docs/devel/migration.rst | 9 +++++++-- include/migration/vmstate.h | 1 + migration/vmstate.c | 10 +++++++++- 3 files changed, 17 insertions(+), 3 deletions(-)Hmm, maybe. I believe the common practice is for
pre_saveto copy state into a separate member on the side, so that conversion back isn't necessary.
Quoted 8 lines
Quoted 1 line
Ccing in the migration maintainers for a second opinion.
It is common to copy stuff into a separate member; however we do occasionally think that
post_savewould be a useful addition; so I think we should take it (if nothing else it actually makes stuff symmetric!).Please make it return 'int' in the same way that pre_save/pre_load does, so that it can fail and stop the migration.
This patch calls post_save *even if the save operation fails*. My reasoning was that I didn't want a failed migration to leave a still-running original QEMU instance in an invalid state. Was this misguided?
If it was not, which error do you prefer to be returned from vmstate_save_state_v() in the case that both the save operation itself and the post_save call returned errors?
-Aaron
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 03/14] migration: Add post_save function to VMStateDescription
Quoted 24 lines · * Aaron Lindsay ([email protected]) wrote:
Quoted 23 lines · Dr. David Alan Gilbert
Quoted 13 lines · * Richard Henderson ([email protected]) wrote:
Quoted 11 lines · Aaron Lindsay
In some cases it may be helpful to modify state before saving it for migration, and then modify the state back after it has been saved. The existing
pre_savefunction provides half of this functionality. This patch adds apost_savefunction to provide the second half.Signed-off-by Aaron Lindsay <[email protected]>--- docs/devel/migration.rst | 9 +++++++-- include/migration/vmstate.h | 1 + migration/vmstate.c | 10 +++++++++- 3 files changed, 17 insertions(+), 3 deletions(-)Quoted 4 lines
Hmm, maybe. I believe the common practice is for
pre_saveto copy state into a separate member on the side, so that conversion back isn't necessary.Ccing in the migration maintainers for a second opinion.
It is common to copy stuff into a separate member; however we do occasionally think that
post_savewould be a useful addition; so I think we should take it (if nothing else it actually makes stuff symmetric!).
Quoted 7 lines
Quoted 2 lines
Please make it return 'int' in the same way that pre_save/pre_load does, so that it can fail and stop the migration.
This patch calls
post_save*even if the save operation fails*. My reasoning was that I didn't want a failed migration to leave a still-running original QEMU instance in an invalid state. Was this misguided?
That's fine - my only issue is that I want post_save to be able to fail itself even if pre_save failed.
Quoted 3 lines
If it was not, which error do you prefer to be returned from
vmstate_save_state_v()in the case that both the save operation itself and thepost_savecall returned errors?
The return value from the save operation.
I did wonder about suggesting that you pass the return value from the save operation as a parameter to post_save.
Dave
Quoted 1 line
-Aaron
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 03/14] migration: Add post_save function to VMStateDescription
Quoted 2 lines · Dr. David Alan Gilbert
I did wonder about suggesting that you pass the return value from the save operation as a parameter to
post_save.
I feel like having that information in post_save may be useful, though I'm having trouble coming up with any concrete uses for it. But, let me know if you decide that you prefer it and I can add that for the next revision.
-Aaron
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 03/14] migration: Add post_save function to VMStateDescription
Quoted 8 lines · * Aaron Lindsay ([email protected]) wrote:
Quoted 2 lines · Dr. David Alan Gilbert
I did wonder about suggesting that you pass the return value from the save operation as a parameter to
post_save.I feel like having that information in
post_savemay be useful, though I'm having trouble coming up with any concrete uses for it. But, let me know if you decide that you prefer it and I can add that for the next revision.
I'm happy either way with the input value; it sounded useful to me but not enough to ask you for, but if you agree then throw it in.
Dave
Quoted 1 line
-Aaron
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [Qemu-arm] [PATCH v6 03/14] migration: Add post_save function to VMStateDescription
In reply to Dr. David Alan GilbertQuoted 37 lines · Dr. David Alan Gilbert
Quoted 25 lines · * Aaron Lindsay ([email protected]) wrote:
Quoted 24 lines · Dr. David Alan Gilbert
Quoted 13 lines · * Richard Henderson ([email protected]) wrote:
Quoted 11 lines · Aaron Lindsay
In some cases it may be helpful to modify state before saving it for migration, and then modify the state back after it has been saved. The existing
pre_savefunction provides half of this functionality. This patch adds apost_savefunction to provide the second half.Signed-off-by Aaron Lindsay <[email protected]>--- docs/devel/migration.rst | 9 +++++++-- include/migration/vmstate.h | 1 + migration/vmstate.c | 10 +++++++++- 3 files changed, 17 insertions(+), 3 deletions(-)Quoted 5 lines
Hmm, maybe. I believe the common practice is for
pre_saveto copy state into a
separate member on the side, so that conversion back isn't necessary.Ccing in the migration maintainers for a second opinion.
It is common to copy stuff into a separate member; however we do occasionally think that
post_savewould be a useful addition; so I think we should take it (if nothing else it actually makes stuff symmetric!).Quoted 7 lines
Quoted 2 lines
Please make it return 'int' in the same way that pre_save/pre_load does, so that it can fail and stop the migration.
This patch calls
post_save*even if the save operation fails*. My reasoning was that I didn't want a failed migration to leave a still-running original QEMU instance in an invalid state. Was this misguided?That's fine - my only issue is that I want
post_saveto be able to fail itself even ifpre_savefailed.
Quoted 7 lines
Quoted 3 lines
If it was not, which error do you prefer to be returned from
vmstate_save_state_v()in the case that both the save operation itself and thepost_savecall returned errors?The return value from the save operation.
I did wonder about suggesting that you pass the return value from the save operation as a parameter topost_save.
The one from save. In general, this one shouldn't be called, and if it gets an error, we are really in big trouble, no? By big treauble I mean that we can basically only stop the guest?
Later, Juan.
Quoted 6 lines
Dave
Quoted 1 line
-Aaron
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [Qemu-arm] [PATCH v6 03/14] migration: Add post_save function to VMStateDescription
In reply to Richard HendersonQuoted 13 lines · Richard Henderson
Quoted 11 lines · Aaron Lindsay
In some cases it may be helpful to modify state before saving it for migration, and then modify the state back after it has been saved. The existing
pre_savefunction provides half of this functionality. This patch adds apost_savefunction to provide the second half.Signed-off-by Aaron Lindsay <[email protected]>--- docs/devel/migration.rst | 9 +++++++-- include/migration/vmstate.h | 1 + migration/vmstate.c | 10 +++++++++- 3 files changed, 17 insertions(+), 3 deletions(-)
Quoted 2 lines
Hmm, maybe. I believe the common practice is for
pre_saveto copy state into a separate member on the side, so that conversion back isn't necessary.
Hi
Originally we have that function. We removed it because we had not remaining uses for it on tree. I am not againt getting it if you need it.
Once told that, I think you can add a return value as David saide.
And once there, it ia good thing that you document it if it is called "before" or "after" the subsections_save. There are arguments for doing it either way, just document it.
Thanks, Juan.
PATCH v6 4/14 target/arm: Swap PMU values before/after migrations
In reply to Aaron LindsayBecause of the PMU's design, many register accesses have side effects which are inter-related, meaning that the normal method of saving CP registers can result in inconsistent state. These side-effects are largely handled in *op_start and *op_finish functions which can be called globally once before and after the state is saved/restored. By doing this and adding raw read/write functions for the affected registers, we avoid such inconsistencies.
--- target/arm/helper.c | 6 ++++-- target/arm/machine.c | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-)
target/arm/helper.c +4 -2
@@ -1379,11 +1379,13 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 0, .access = PL0_RW, .accessfn = pmreg_access_ccntr, .type = ARM_CP_IO, - .readfn = pmccntr_read, .writefn = pmccntr_write, }, + .fieldoffset = offsetof(CPUARMState, cp15.c15_ccnt), + .readfn = pmccntr_read, .writefn = pmccntr_write, + .raw_readfn = raw_read, .raw_writefn = raw_write, },#endif { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7, - .writefn = pmccfiltr_write, + .writefn = pmccfiltr_write, .raw_writefn = raw_write, .access = PL0_RW, .accessfn = pmreg_access, .type = ARM_CP_IO, .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0),
target/arm/machine.c +19 -0
@@ -584,6 +584,8 @@ static int cpu_pre_save(void *opaque) {ARMCPU *cpu = opaque; +pmccntr_sync (&cpu->env); +if (kvm_enabled ()) {if (!write_kvmstate_to_list (cpu)) {/* This should never fail */
@@ -605,6 +607,19 @@ static int cpu_pre_save(void *opaque)return 0 ; } +static void cpu_post_save (void *opaque) +{ +ARMCPU *cpu = opaque; +pmccntr_sync (&cpu->env); +} + +static int cpu_pre_load (void *opaque) +{ +ARMCPU *cpu = opaque; +pmccntr_sync (&cpu->env); +return 0 ; +} +static int cpu_post_load (void *opaque,int version_id) {ARMCPU *cpu = opaque;
@@ -652,6 +667,8 @@ static int cpu_post_load(void *opaque, int version_id)hw_breakpoint_update_all (cpu);hw_watchpoint_update_all (cpu); +pmccntr_sync (&cpu->env); +return 0 ; }
@@ -660,6 +677,8 @@ const VMStateDescription vmstate_arm_cpu = { .version_id = 22, .minimum_version_id = 22, .pre_save = cpu_pre_save, + .post_save = cpu_post_save, + .pre_load = cpu_pre_load, .post_load = cpu_post_load, .fields = (VMStateField []) {VMSTATE_UINT32_ARRAY (env.regs,ARMCPU ,16 ),
Signature · 1 line
2.19.1
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 04/14] target/arm: Swap PMU values before/after migrations
target/arm/machine.c static int cpu_pre_save(void *opaque) · line 614
Quoted 5 lines · Aaron Lindsay
target/arm/machine.c +5
-0+static void cpu_post_save (void *opaque) +{ +ARMCPU *cpu = opaque; +pmccntr_sync (&cpu->env); +}
I'm confused about the need for this.
Can you explain the sequence of events that requires it?
r~
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 04/14] target/arm: Swap PMU values before/after migrations
Quoted 9 lines · Richard Henderson
target/arm/machine.c static int cpu_pre_save(void *opaque) · line 614
Quoted 5 lines · Aaron Lindsay
target/arm/machine.c +5
-0+static void cpu_post_save (void *opaque) +{ +ARMCPU *cpu = opaque; +pmccntr_sync (&cpu->env); +}I'm confused about the need for this.
Can you explain the sequence of events that requires it?
It sounds like you're more okay with this now based on your comments on my later patch, but for others...
The PMU implementation is event-driven - the counters and other associated PMU/sysreg state are only updated when needed to ensure the architectural state observed by the system is correct (i.e. on system register reads). Otherwise, the counters are stored as differences from the underlying counts rather than the raw counter values themselves. Upon migration, we want to convert the counters and other state to their architectural versions, but then want to swap it all back to the differential versions again when we're done saving/loading.
-Aaron
PATCH v6 5/14 target/arm: Reorganize PMCCNTR accesses
In reply to Aaron Lindsaypmccntr_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 ensures 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 | 26 ++++++++---- target/arm/helper.c | 96 +++++++++++++++++++++++--------------------- target/arm/machine.c | 8 ++-- 3 files changed, 73 insertions(+), 57 deletions(-)
target/arm/cpu.h +18 -8
@@ -468,10 +468,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-correct 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 */
@@ -937,15 +947,15 @@ 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. */ -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 +51 -45
143 lines not on this page · raw message
target/arm/machine.c +4 -4
@@ -584,7 +584,7 @@ static int cpu_pre_save(void *opaque) {ARMCPU *cpu = opaque; -pmccntr_sync (&cpu->env); +pmccntr_op_start (&cpu->env);if (kvm_enabled ()) {if (!write_kvmstate_to_list (cpu)) {
@@ -610,13 +610,13 @@ static int cpu_pre_save(void *opaque)static void cpu_post_save (void *opaque) {ARMCPU *cpu = opaque; -pmccntr_sync (&cpu->env); +pmccntr_op_finish (&cpu->env); }static int cpu_pre_load (void *opaque) {ARMCPU *cpu = opaque; -pmccntr_sync (&cpu->env); +pmccntr_op_start (&cpu->env);return 0 ; }
@@ -667,7 +667,7 @@ static int cpu_post_load(void *opaque, int version_id)hw_breakpoint_update_all (cpu);hw_watchpoint_update_all (cpu); -pmccntr_sync (&cpu->env); +pmccntr_op_finish (&cpu->env);return 0 ; }
Signature · 1 line
2.19.1
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 05/14] target/arm: Reorganize PMCCNTR accesses
Quoted 9 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 ensures time isn't lost and will also allow us to access the 'old' architectural register value in order to detect overflows in later patches.
Quoted 6 lines
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/cpu.h | 26 ++++++++---- target/arm/helper.c | 96 +++++++++++++++++++++++--------------------- target/arm/machine.c | 8 ++-- 3 files changed, 73 insertions(+), 57 deletions(-)
Ok, looking at this follow-up makes more sense than the previous patch. Would it make sense to squash these two together?
It also makes sense why you'd need the post_save hook.
r~
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 05/14] target/arm: Reorganize PMCCNTR accesses
Quoted 2 lines · Richard Henderson
Ok, looking at this follow-up makes more sense than the previous patch. Would it make sense to squash these two together?
Or, rather, split into two different patches: the first splits pmccntr_sync and updates all of the existing uses, and the second which adds the migration hooks.
r~
Re: [Qemu-devel] [PATCH v6 05/14] target/arm: Reorganize PMCCNTR accesses
Quoted 6 lines · Richard Henderson
Quoted 2 lines · Richard Henderson
Ok, looking at this follow-up makes more sense than the previous patch. Would it make sense to squash these two together?
Or, rather, split into two different patches: the first splits
pmccntr_syncand updates all of the existing uses, and the second which adds the migration hooks.
To make sure I understand, you're suggesting essentially reversing the order of patches 4 and 5 (and fixing everything up to be consistent)?
-Aaron
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 05/14] target/arm: Reorganize PMCCNTR accesses
Quoted 10 lines · Aaron Lindsay
Quoted 6 lines · Richard Henderson
Quoted 2 lines · Richard Henderson
Ok, looking at this follow-up makes more sense than the previous patch. Would it make sense to squash these two together?
Or, rather, split into two different patches: the first splits
pmccntr_syncand updates all of the existing uses, and the second which adds the migration hooks.To make sure I understand, you're suggesting essentially reversing the order of patches 4 and 5 (and fixing everything up to be consistent)?
Yes.
r~
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 05/14] target/arm: Reorganize PMCCNTR accesses
In reply to Richard HendersonQuoted 17 lines · Richard Henderson
Quoted 9 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 ensures time isn't lost and will also allow us to access the 'old' architectural register value in order to detect overflows in later patches.Quoted 6 lines
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/cpu.h | 26 ++++++++---- target/arm/helper.c | 96 +++++++++++++++++++++++--------------------- target/arm/machine.c | 8 ++-- 3 files changed, 73 insertions(+), 57 deletions(-)
Quoted 2 lines
Ok, looking at this follow-up makes more sense than the previous patch. Would it make sense to squash these two together?
I was attempting to keep the migration plumbing separate from the PMU implementation details, but I'm not particularly partial to this staging.
Quoted 1 line
It also makes sense why you'd need the
post_savehook.
Okay. I attempted to describe this in the commit message in a way that communicated the need for the hook without being overly verbose - but suggestions in that area are very welcome if you think a different commit message would help.
-Aaron
PATCH v6 6/14 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 | 118 +++++++++++++++++++++++++++++++++++++++---- target/arm/machine.c | 8 +-- 4 files changed, 137 insertions(+), 14 deletions(-)
target/arm/cpu.c +3 -0
@@ -948,6 +948,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
@@ -957,6 +957,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
@@ -1018,7 +1036,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)
@@ -1028,6 +1047,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 +109 -9
172 lines not on this page · raw message
target/arm/machine.c +4 -4
@@ -584,7 +584,7 @@ static int cpu_pre_save(void *opaque) {ARMCPU *cpu = opaque; -pmccntr_op_start (&cpu->env); +pmu_op_start (&cpu->env);if (kvm_enabled ()) {if (!write_kvmstate_to_list (cpu)) {
@@ -610,13 +610,13 @@ static int cpu_pre_save(void *opaque)static void cpu_post_save (void *opaque) {ARMCPU *cpu = opaque; -pmccntr_op_finish (&cpu->env); +pmu_op_finish (&cpu->env); }static int cpu_pre_load (void *opaque) {ARMCPU *cpu = opaque; -pmccntr_op_start (&cpu->env); +pmu_op_start (&cpu->env);return 0 ; }
@@ -667,7 +667,7 @@ static int cpu_post_load(void *opaque, int version_id)hw_breakpoint_update_all (cpu);hw_watchpoint_update_all (cpu); -pmccntr_op_finish (&cpu->env); +pmu_op_finish (&cpu->env);return 0 ; }
Signature · 1 line
2.19.1
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 06/14] target/arm: Filter cycle counter based on PMCCFILTR_EL0
target/arm/machine.c static int cpu_pre_save(void *opaque) · line 587
Quoted 8 lines · Aaron Lindsay
target/arm/machine.c +1
-1@@ -584,7 +584,7 @@ static int cpu_pre_save(void *opaque) {ARMCPU *cpu = opaque; -pmccntr_op_start (&cpu->env); +pmu_op_start (&cpu->env);
Does it make sense to move this patch earlier so that these hooks are modified once? No big deal if not.
target/arm/machine.c static int cpu_pre_save(void *opaque) · line 588
Quoted 1 line
target/arm/machine.c +1
-0+static inline bool pmu_counter_enabled (CPUARMState *env,uint8_t counter)
Drop the inline. This function is pretty big; we should let the compiler choose.
Otherwise,
r~
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 06/14] target/arm: Filter cycle counter based on PMCCFILTR_EL0
Quoted 8 lines · Aaron Lindsay
Quoted 7 lines · Richard Henderson
Quoted 6 lines · Aaron Lindsay
target/arm/machine.c +0
-0@@ -584,7 +584,7 @@ static int cpu_pre_save(void *opaque) {ARMCPU *cpu = opaque;
Quoted 5 lines
Quoted 5 lines
target/arm/machine.c static int cpu_pre_save(void *opaque) · line 587
Quoted 2 lines
target/arm/machine.c +1
-1-pmccntr_op_start (&cpu->env); +pmu_op_start (&cpu->env);Does it make sense to move this patch earlier so that these hooks are modified once? No big deal if not.
I took another look at this, and I think it makes sense to move the pmccntr_op -> pmu_op changes into "target/arm: Reorganize PMCCNTR accesses", independent of the filtering changes.
-Aaron
PATCH v6 7/14 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
@@ -961,6 +961,10 @@ static const ARMCPRegInfo v6_cp_reginfo[] = { PMXEVTYPER_M | PMXEVTYPER_MT | \ PMXEVTYPER_EVTCOUNT) +#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 ;
@@ -1264,10 +1268,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) {
@@ -1489,6 +1509,11 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .readfn = pmccntr_read, .writefn = pmccntr_write, .raw_readfn = raw_read, .raw_writefn = raw_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, .raw_writefn = raw_write,
Signature · 1 line
2.19.1
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 07/14] target/arm: Allow AArch32 access for PMCCFILTR
Quoted 5 lines · Aaron Lindsay
Signed-off-by Aaron Lindsay <[email protected]>Reviewed-by Peter Maydell <[email protected]>--- target/arm/helper.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)
r~
PATCH v6 8/14 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
@@ -1309,6 +1309,13 @@ static void pmovsr_write(CPUARMState *env, const ARMCPRegInfo *ri, 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) {
@@ -1662,6 +1669,24 @@ 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, + .type =ARM_CP_ALIAS , + .fieldoffset =offsetoflow32 (CPUARMState, cp15.c9_pmovsr), + .writefn = pmovsset_write, + .raw_writefn = raw_write }, + { .name ="PMOVSSET_EL0" , .state =ARM_CP_STATE_AA64 , + .opc0 =3 , .opc1 =3 , .crn =9 , .crm =14 , .opc2 =3 , + .access =PL0_RW , .accessfn = pmreg_access, + .type =ARM_CP_ALIAS , + .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr), + .writefn = pmovsset_write, + .raw_writefn = raw_write }, +REGINFO_SENTINEL +}; +static void teecr_write (CPUARMState *env,const ARMCPRegInfo *ri,uint64_t value) {
@@ -5116,6 +5141,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 · 1 line
2.19.1
Re: [Qemu-devel] [PATCH v6 08/14] target/arm: Implement PMOVSSET
Quoted 6 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(+)
r~
PATCH v6 9/14 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 | 42 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 65 insertions(+), 9 deletions(-)
target/arm/cpu.c +13 -7
@@ -948,9 +948,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 )) {
@@ -1583,8 +1593,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 ;
@@ -1626,8 +1634,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
@@ -975,6 +975,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
@@ -143,8 +143,6 @@ static void aarch64_a57_initfn(Object *obj) cpu->id_isar6 =0 ; 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 +42 -0
48 lines not on this page · raw message
Signature · 1 line
2.19.1
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 09/14] target/arm: Add array for supported PMU events, generate PMCEID[01]
target/arm/cpu.c static void arm_cpu_realizefn(DeviceState *dev, Error **errp) · line 955
Quoted 2 lines · Aaron Lindsay
target/arm/cpu.c +2
-0+ cpu->pmceid0 = pmceid &0xffffffff ; + cpu->pmceid1 = (pmceid >>32 ) &0xffffffff ;
extract64(pmceid, 0, 32) and extract64(pmceid, 32, 32).
target/arm/helper.c static inline uint64_t pmu_counter_mask(CPUARMState *env) · line 992
Quoted 3 lines
target/arm/helper.c +3
-0+static const pm_event pm_events[] = { +}; +#define MAX_EVENT_ID 0x0
Is this going to be ARRAY_SIZE(pm_events) - 1 in the end?
Quoted 1 line
+static
uint16_tsupported_event_map[MAX_EVENT_ID + 1];
Better as ARRAY_SIZE(pm_events)?
target/arm/helper.c static inline uint64_t pmu_counter_mask(CPUARMState *env) · line 1008
Quoted 3 lines
target/arm/helper.c +3
-0+for (i =0 ; i <=MAX_EVENT_ID ; i++) { + supported_event_map[i] =UNSUPPORTED_EVENT ; + }
I think maybe
for (i = 0; i < ARRAY_SIZE(supported_map_event); ++i) {would be better.
r~
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 09/14] target/arm: Add array for supported PMU events, generate PMCEID[01]
Quoted 6 lines · Richard Henderson
target/arm/helper.c static inline uint64_t pmu_counter_mask(CPUARMState *env) · line 992
Quoted 3 lines · Aaron Lindsay
target/arm/helper.c +3
-0+static const pm_event pm_events[] = { +}; +#define MAX_EVENT_ID 0x0Is this going to be
ARRAY_SIZE(pm_events)- 1 in the end?
No, this ends up being a 'sparse' array. It maps the ARM architected PMU event numbers to their implementations. So, MAX_EVENT_ID ends up being the maximum architected event number we implement when my later patches add those.
-Aaron
PATCH v6 10/14 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. Support migration with raw read/write functions.
--- target/arm/cpu.h | 3 + target/arm/helper.c | 296 +++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 282 insertions(+), 17 deletions(-)
target/arm/cpu.h +3 -0
@@ -482,6 +482,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 +279 -17
397 lines not on this page · raw message
Signature · 1 line
2.19.1
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 10/14] target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER
Quoted 11 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. Support migration with raw read/write functions.
Signed-off-by Aaron Lindsay <[email protected]>Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/cpu.h | 3 + target/arm/helper.c | 296 +++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 282 insertions(+), 17 deletions(-)
r~
PATCH v6 11/14 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 | 90 ++++++++++++++++++++++----------------------- 1 file changed, 44 insertions(+), 46 deletions(-)
target/arm/helper.c +44 -46
@@ -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" #include "qemu/range.h"
@@ -988,9 +989,50 @@ 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 +static 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 }; -#define MAX_EVENT_ID 0x0 +#define MAX_EVENT_ID 0x11#define UNSUPPORTED_EVENT UINT16_MAXstatic uint16_t supported_event_map[MAX_EVENT_ID +1 ];
@@ -1083,8 +1125,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)
@@ -1195,9 +1235,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;
@@ -1343,42 +1381,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) {
@@ -1752,7 +1754,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),
@@ -1774,7 +1775,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = { .fieldoffset = offsetof(CPUARMState, cp15.c15_ccnt), .readfn = pmccntr_read, .writefn = pmccntr_write, .raw_readfn = raw_read, .raw_writefn = raw_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,
@@ -5416,7 +5416,6 @@ void register_cp_regs_for_features(ARMCPU *cpu)* count register. */ unsigned int i, pmcrn =4 ; -#ifndef CONFIG_USER_ONLY ARMCPRegInfo pmcr = { .name ="PMCR" , .cp =15 , .crn =9 , .crm =12 , .opc1 =0 , .opc2 =0 , .access =PL0_RW ,
@@ -5473,7 +5472,6 @@ void register_cp_regs_for_features(ARMCPU *cpu)g_free (pmevtyper_name);g_free (pmevtyper_el0_name); } -#endif ARMCPRegInfo clidr = { .name ="CLIDR" , .state =ARM_CP_STATE_BOTH , .opc0 =3 , .crn =0 , .crm =0 , .opc1 =1 , .opc2 =1 ,
Signature · 1 line
2.19.1
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 11/14] target/arm: PMU: Add instruction and cycle events
target/arm/helper.c typedef struct pm_event { · line 1009
Quoted 12 lines · Aaron Lindsay
+ * Return the underlying cycle count for the PMU cycle counters. If we're in
+ * usermode, simply return 0.
+ */target/arm/helper.c +9
-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 +}
Usually we pass through the host cycle counter.
See cpu_get_host_ticks().
r~
Re: [Qemu-devel] [PATCH v6 11/14] target/arm: PMU: Add instruction and cycle events
Quoted 16 lines · Richard Henderson
target/arm/helper.c typedef struct pm_event { · line 1009
Quoted 12 lines · Aaron Lindsay
+ * Return the underlying cycle count for the PMU cycle counters. If we're in
+ * usermode, simply return 0.
+ */target/arm/helper.c +9
-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 +}Usually we pass through the host cycle counter.
Seecpu_get_host_ticks().
Why do you prefer cpu_get_host_ticks()? And are you suggesting this for just user-mode, or both system and user?
PMCCNTR used this same qemu_clock_get_ns() call previous to my patch (see where this patch replaces that call with one to cycles_get_count() in pmccntr_op_start()). Of course, we could keep the preexisting PMCCNTR behavior while making the new cycle counter use cpu_get_host_ticks(), but having two ways through the same interface which count cycles differently feels wrong.
-Aaron
Re: [Qemu-devel] [PATCH v6 11/14] target/arm: PMU: Add instruction and cycle events
Quoted 18 lines · Aaron Lindsay
Quoted 16 lines · Richard Henderson
target/arm/helper.c typedef struct pm_event { · line 1009
Quoted 12 lines · Aaron Lindsay
+ * Return the underlying cycle count for the PMU cycle counters. If we're in
+ * usermode, simply return 0.
+ */target/arm/helper.c +9
-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 +}Usually we pass through the host cycle counter.
Seecpu_get_host_ticks().
Quoted 2 lines
Why do you prefer
cpu_get_host_ticks()? And are you suggesting this for just user-mode, or both system and user?
Just user-mode. Providing a clock with unknown scaling is more useful than a constant 0.
r~
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 11/14] target/arm: PMU: Add instruction and cycle events
Quoted 19 lines · Richard Henderson
Quoted 18 lines · Aaron Lindsay
Quoted 16 lines · Richard Henderson
target/arm/helper.c typedef struct pm_event { · line 1009
Quoted 12 lines · Aaron Lindsay
+ * Return the underlying cycle count for the PMU cycle counters. If we're in
+ * usermode, simply return 0.
+ */target/arm/helper.c +9
-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 +}Usually we pass through the host cycle counter.
Seecpu_get_host_ticks().
Quoted 5 lines
Quoted 2 lines
Why do you prefer
cpu_get_host_ticks()? And are you suggesting this for just user-mode, or both system and user?Just user-mode. Providing a clock with unknown scaling is more useful than a constant 0.
Okay, that makes sense I think. I slightly dislike the fact that the behavior is silently different between user and system mode, but perhaps some documentation could alleviate my concern - is there an appropriate place to document this difference?
It occurs to me that one argument for always returning 0 is that it's immediately obvious that the behavior is different from system mode, eliminating the possibility of a user assuming results from user and system mode are comparable.
-Aaron
PATCH v6 12/14 target/arm: PMU: Set PMCR.N to 4
In reply to Aaron LindsayThis both advertises that we support four counters and enables them because the pmu_num_counters() reads this value from PMCR.
--- target/arm/helper.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
target/arm/helper.c +4 -4
@@ -1706,7 +1706,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)
@@ -5412,8 +5412,8 @@ void register_cp_regs_for_features(ARMCPU *cpu) }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 -* count register. +* field as main ID register, and we implement four counters in +* addition to the cycle count register. */ unsigned int i, pmcrn =4 ;ARMCPRegInfo pmcr = {
@@ -5430,7 +5430,7 @@ 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, + .resetvalue = (cpu->midr & 0xff000000) | (pmcrn << PMCRN_SHIFT), .writefn = pmcr_write, .raw_writefn = raw_write, };define_one_arm_cp_reg (cpu, &pmcr);
Signature · 1 line
2.19.1
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 12/14] target/arm: PMU: Set PMCR.N to 4
Quoted 7 lines · Aaron Lindsay
This both advertises that we support four counters and enables them because the
pmu_num_counters()reads this value from PMCR.Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/helper.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
It looks like this should have been folded into patch 10, or patch 10 split to include the code changes that go with these comment changes.
r~
Re: [Qemu-devel] [PATCH v6 12/14] target/arm: PMU: Set PMCR.N to 4
Quoted 4 lines · Richard Henderson
Quoted 3 lines · Aaron Lindsay
This both advertises that we support four counters and enables them because the
pmu_num_counters()reads this value from PMCR.
Quoted 8 lines
Quoted 4 lines
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/helper.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)It looks like this should have been folded into patch 10, or patch 10 split to include the code changes that go with these comment changes.
By setting PMCR.N to 4, this commit is what causes all the previous implementation of non-PMCCNTR counters to become usable by the guest - both because the OS can now detect their presence in PMCR, and because the system registers used to setup/access the counters are gated on pmu_num_counters(), which uses PMCR.N to determine if a counter is implemented.
That said, I'm not against merging the two patches if you feel that is cleaner.
-Aaron
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 12/14] target/arm: PMU: Set PMCR.N to 4
Quoted 14 lines · Aaron Lindsay
Quoted 4 lines · Richard Henderson
Quoted 3 lines · Aaron Lindsay
This both advertises that we support four counters and enables them because the
pmu_num_counters()reads this value from PMCR.Quoted 8 lines
Quoted 4 lines
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/helper.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)It looks like this should have been folded into patch 10, or patch 10 split to include the code changes that go with these comment changes.
Quoted 6 lines
By setting PMCR.N to 4, this commit is what causes all the previous implementation of non-PMCCNTR counters to become usable by the guest - both because the OS can now detect their presence in PMCR, and because the system registers used to setup/access the counters are gated on
pmu_num_counters(), which uses PMCR.N to determine if a counter is implemented.
But e.g.
target/arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 5419
Quoted 11 lines
target/arm/helper.c +2
-2@@ -5412,8 +5412,8 @@ void register_cp_regs_for_features(ARMCPU *cpu) }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 -* count register. +* field as main ID register, and we implement four counters in +* addition to the cycle count register. */ unsigned int i, pmcrn =4 ;ARMCPRegInfo pmcr = {
clearly belongs with the previous patch, since pmcrn is already 4.
target/arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 1712
Quoted 9 lines
target/arm/helper.c +1
-1@@ -1706,7 +1706,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)
And this is out of date as well, and probably belonged elsewhere. Which leaves only
target/arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 5435
Quoted 8 lines
target/arm/helper.c +1
-1@@ -5430,7 +5430,7 @@ 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, + .resetvalue = (cpu->midr & 0xff000000) | (pmcrn << PMCRN_SHIFT), .writefn = pmcr_write, .raw_writefn = raw_write, };
which suggests that this one line belonged to the patch that set pmcrn in the first place.
r~
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 12/14] target/arm: PMU: Set PMCR.N to 4
Quoted 24 lines · Richard Henderson
Quoted 14 lines · Aaron Lindsay
Quoted 4 lines · Richard Henderson
Quoted 3 lines · Aaron Lindsay
This both advertises that we support four counters and enables them because the
pmu_num_counters()reads this value from PMCR.Quoted 8 lines
Quoted 4 lines
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/helper.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)It looks like this should have been folded into patch 10, or patch 10 split to include the code changes that go with these comment changes.
Quoted 6 lines
By setting PMCR.N to 4, this commit is what causes all the previous implementation of non-PMCCNTR counters to become usable by the guest - both because the OS can now detect their presence in PMCR, and because the system registers used to setup/access the counters are gated on
pmu_num_counters(), which uses PMCR.N to determine if a counter is implemented.But e.g.
Quoted 13 lines
target/arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 5419
Quoted 11 lines
target/arm/helper.c +2
-2@@ -5412,8 +5412,8 @@ void register_cp_regs_for_features(ARMCPU *cpu) }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 -* count register. +* field as main ID register, and we implement four counters in +* addition to the cycle count register. */ unsigned int i, pmcrn =4 ;ARMCPRegInfo pmcr = {clearly belongs with the previous patch, since pmcrn is already 4.
I suppose pmcrn (the local variable) should've been set to 0 before this patch and updated here to be 4. However, even though it was already set to 4 my amendment to the comment was still not true until this patch, because it wasn't written into PMCR.N (the CP register).
Quoted 4 lines
Quoted 4 lines
target/arm/helper.c +0
-0@@ -1706,7 +1706,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
Quoted 8 lines
target/arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 1712
Quoted 5 lines
target/arm/helper.c +1
-1-* 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) And this is out of date as well, and probably belonged elsewhere. Which leaves only
Why is this out of date? Because PMCR.N still contained zero, no counters were implemented from the guest's standpoint until this patch.
Quoted 11 lines
target/arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 5435
Quoted 8 lines
target/arm/helper.c +1
-1@@ -5430,7 +5430,7 @@ 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, + .resetvalue = (cpu->midr & 0xff000000) | (pmcrn << PMCRN_SHIFT), .writefn = pmcr_write, .raw_writefn = raw_write, };which suggests that this one line belonged to the patch that set pmcrn in the first place.
I get the feeling we may be talking past each other/misunderstanding somehow, but I'm not sure why/how. Maybe I'm thinking about whether or not counters are implemented from the guest's standpoint and you're thinking about it from the standpoint of whether the code to implement them exists in QEMU?
Anyway, by splitting these patches apart, my intention was to separate the plumbing work from turning on the water. If you believe it is better to do both in the same patch, I'll make that change for v7.
-Aaron
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 12/14] target/arm: PMU: Set PMCR.N to 4
Quoted 2 lines · Aaron Lindsay
I suppose pmcrn (the local variable) should've been set to 0 before this patch and updated here to be 4.
That's plausible.
Quoted 3 lines
Anyway, by splitting these patches apart, my intention was to separate the plumbing work from turning on the water. If you believe it is better to do both in the same patch, I'll make that change for v7.
Fair. I'll let Peter weigh in for final decision.
r~
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 12/14] target/arm: PMU: Set PMCR.N to 4
Quoted 11 lines · Richard Henderson
Quoted 2 lines · Aaron Lindsay
I suppose pmcrn (the local variable) should've been set to 0 before this patch and updated here to be 4.
That's plausible.
Quoted 3 lines
Anyway, by splitting these patches apart, my intention was to separate the plumbing work from turning on the water. If you believe it is better to do both in the same patch, I'll make that change for v7.
Fair. I'll let Peter weigh in for final decision.
No strong opinion -- rth has reviewed this set, not me.
thanks
-- PMM
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 12/14] target/arm: PMU: Set PMCR.N to 4
In reply to Richard HendersonQuoted 11 lines · Richard Henderson
Quoted 2 lines · Aaron Lindsay
I suppose pmcrn (the local variable) should've been set to 0 before this patch and updated here to be 4.
That's plausible.
Quoted 3 lines
Anyway, by splitting these patches apart, my intention was to separate the plumbing work from turning on the water. If you believe it is better to do both in the same patch, I'll make that change for v7.
Fair. I'll let Peter weigh in for final decision.
At a minimum, I've updated my local tree so that pmcrn is initially set to 0 in 'target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER' and updated to 4 in this patch. And I'll squash them instead if that ends up being the decision.
-Aaron
PATCH v6 13/14 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
@@ -994,6 +994,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.
@@ -1021,6 +1030,10 @@ static uint64_t instructions_get_count(CPUARMState *env)#endif static 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,
@@ -1345,6 +1358,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;
@@ -1751,9 +1782,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 · 1 line
2.19.1
Re: [Qemu-arm] [Qemu-devel] [PATCH v6 13/14] target/arm: Implement PMSWINC
Quoted 4 lines · Aaron Lindsay
Signed-off-by Aaron Lindsay <[email protected]>--- target/arm/helper.c | 39 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-)
r~
PATCH v6 14/14 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 | 126 +++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 138 insertions(+), 6 deletions(-)
target/arm/cpu.c +11 -0
@@ -764,6 +764,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)
@@ -958,6 +964,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_ns (QEMU_CLOCK_VIRTUAL , arm_pmu_timer_cb, + cpu); +#endif }else { cpu->pmceid0 =0x00000000 ; cpu->pmceid1 =0x00000000 ;
target/arm/cpu.h +7 -0
@@ -721,6 +721,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 */
@@ -972,6 +974,11 @@ void pmccntr_op_finish(CPUARMState *env);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 */
target/arm/helper.c +120 -6
260 lines not on this page · raw message
Signature · 1 line
2.19.1
Quoted 18 lines · Aaron Lindsay
The ARM PMU implementation currently contains a basic cycle counter, but it is often useful to gather counts of other events, filter them based on execution mode, and/or be notified on counter overflow. 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 v5 [1] I have:
* Taken a first pass at addressing migration
* Restructured the list of supported events, and ensured they're all initialized
* Fixed aliasing for PMOVSSET
* AddedARM_CP_IOfor PMINTENCLR andPMINTENCLR_EL1
* Addressed a few non-code issues (comment style, patch staging, spelling, etc.)
Quoted 6 lines
[1] - https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg06830.html
Aaron Lindsay (14):
target/arm: Mark PMINTENCLR andPMINTENCLR_EL1accesses as possibly
doing IO
target/arm: Mask PMOVSR writes based on supported counters
Hi; Richard has reviewed most of this series and suggested some changes (thanks!); I'll just take these first two patches into target-arm.next, since they're simple fixes that have been reviewed.
thanks
-- PMM
Quoted 21 lines · Peter Maydell
Quoted 20 lines · Aaron Lindsay
The ARM PMU implementation currently contains a basic cycle counter, but it is often useful to gather counts of other events, filter them based on execution mode, and/or be notified on counter overflow. 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 v5 [1] I have:
* Taken a first pass at addressing migration
* Restructured the list of supported events, and ensured they're all initialized
* Fixed aliasing for PMOVSSET
* AddedARM_CP_IOfor PMINTENCLR andPMINTENCLR_EL1
* Addressed a few non-code issues (comment style, patch staging, spelling, etc.)[1] - https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg06830.html
Quoted 8 lines
Quoted 4 lines
Aaron Lindsay (14):
target/arm: Mark PMINTENCLR andPMINTENCLR_EL1accesses as possibly
doing IO
target/arm: Mask PMOVSR writes based on supported countersHi; Richard has reviewed most of this series and suggested some changes (thanks!); I'll just take these first two patches into target-arm.next, since they're simple fixes that have been reviewed.
Thanks, Peter and Richard!
Is anyone willing to take a glance at the final patch in this series, "target/arm: Send interrupts on PMU counter overflow", before my next iteration? I'm particularly interested in a review of the approach I took for detecting overflow.
-Aaron
Quoted 4 lines · Aaron Lindsay
Is anyone willing to take a glance at the final patch in this series, "target/arm: Send interrupts on PMU counter overflow", before my next iteration? I'm particularly interested in a review of the approach I took for detecting overflow.
I intend to finish reviewing the patch series today.
r~