Skip to contentSkip to conversation

Extend the performance monitoring registers

PATCH Superseded Alistair Francis 4 participants 17 messages Feb 6, 2016 – Feb 16, 2016

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Alistair Francis

author
#0 lore raw

v2 cover Extend the performance monitoring registers

This patch set is based on the patch sent by Christopher Covington and written by Aaron Lindsay which was sent as an RFC (Implement remaining PMU functionality).

It adds a few performance monitoring related registers.

V2:
- Add Aaron to the signed off lines
- Add the tested-by lines from Nathan
- Add two more patches from Chris

Alistair Francis (5):
target-arm: Add the pmceid0 and pmceid1 registers
target-arm: Add Some of the performance monitor registers
target-arm: Add the pmovsclr_el0 and pmintenclr_el1 registers
target-arm: Add PMUSERENR_EL0 register
target-arm: Unmask PMU bits in debug feature register

 target-arm/cpu-qom.h |   2 +
 target-arm/cpu.c     |   2 +
 target-arm/cpu.h     |   6 +++
 target-arm/cpu64.c   |   2 +
 target-arm/helper.c  | 122 ++++++++++++++++++++++++++++++++++++++++++++-------
 5 files changed, 117 insertions(+), 17 deletions(-)
Signature · 1 line
2.5.0
author
#1 lore raw

PATCH v2 1/5 target-arm: Add the pmceid0 and pmceid1 registers

Signed-off-by Aaron Lindsay <[email protected]>
Signed-off-by Alistair Francis <[email protected]>
Tested-by Nathan Rossi <[email protected]>

---

 target-arm/cpu-qom.h | 2 ++
 target-arm/cpu.c     | 2 ++
 target-arm/cpu64.c   | 2 ++
 target-arm/helper.c  | 8 ++++++++
 4 files changed, 14 insertions(+)
target-arm/cpu-qom.h +2 -0
index 07c0a71..1cc4502 100644
@@ -148,6 +148,8 @@ typedef struct ARMCPU {
     uint32_t id_pfr0;
     uint32_t id_pfr1;
     uint32_t id_dfr0;
+    uint32_t pmceid0;
+    uint32_t pmceid1;
     uint32_t id_afr0;
     uint32_t id_mmfr0;
     uint32_t id_mmfr1;
target-arm/cpu.c +2 -0
index 7ddbf3d..937f845 100644
@@ -1156,6 +1156,8 @@ static void cortex_a15_initfn(Object *obj)
     cpu->id_pfr0 = 0x00001131;
     cpu->id_pfr1 = 0x00011011;
     cpu->id_dfr0 = 0x02010555;
+    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */
+    cpu->pmceid1 = 0x00000000;
     cpu->id_afr0 = 0x00000000;
     cpu->id_mmfr0 = 0x10201105;
     cpu->id_mmfr1 = 0x20000000;
target-arm/cpu64.c +2 -0
index c847513..8c4b6fd 100644
@@ -134,6 +134,8 @@ static void aarch64_a57_initfn(Object *obj)
     cpu->id_isar5 = 0x00011121;
     cpu->id_aa64pfr0 = 0x00002222;
     cpu->id_aa64dfr0 = 0x10305106;
+    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */
+    cpu->pmceid1 = 0x00000000;
     cpu->id_aa64isar0 = 0x00011120;
     cpu->id_aa64mmfr0 = 0x00001124;
     cpu->dbgdidr = 0x3516d000;
target-arm/helper.c +8 -0
index 5ea507f..66aa406 100644
@@ -4192,6 +4192,14 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 1,
               .access = PL1_R, .type = ARM_CP_CONST,
               .resetvalue = cpu->id_aa64dfr1 },
+            { .name = "PMCEID0_EL0", .state = ARM_CP_STATE_AA64,
+              .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 6,
+              .access = PL1_R, .type = ARM_CP_CONST,
+              .resetvalue = cpu->pmceid0},
+            { .name = "PMCEID1_EL0", .state = ARM_CP_STATE_AA64,
+              .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 7,
+              .access = PL1_R, .type = ARM_CP_CONST,
+              .resetvalue = cpu->pmceid1},
             { .name = "ID_AA64AFR0_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 4,
               .access = PL1_R, .type = ARM_CP_CONST,
Signature · 1 line
2.5.0
#2 lore raw

Re: [Qemu-devel] [PATCH v2 1/5] target-arm: Add the pmceid0 and pmceid1 registers

Quoted 29 lines · Alistair Francis
Signed-off-by Aaron Lindsay <[email protected]>
Signed-off-by Alistair Francis <[email protected]>
Tested-by Nathan Rossi <[email protected]>

---

 target-arm/cpu-qom.h | 2 ++
 target-arm/cpu.c     | 2 ++
 target-arm/cpu64.c   | 2 ++
 target-arm/helper.c  | 8 ++++++++
 4 files changed, 14 insertions(+)
target-arm/cpu-qom.h +2 -0
index 07c0a71..1cc4502 100644
@@ -148,6 +148,8 @@ typedef struct ARMCPU {
     uint32_t id_pfr0;
     uint32_t id_pfr1;
     uint32_t id_dfr0;
+    uint32_t pmceid0;
+    uint32_t pmceid1;
     uint32_t id_afr0;
     uint32_t id_mmfr0;
     uint32_t id_mmfr1;
target-arm/cpu.c +0 -0
index 7ddbf3d..937f845 100644

target-arm/cpu.c static void cortex_a15_initfn(Object *obj) · line 1159

Quoted 4 lines
target-arm/cpu.c +1 -0
@@ -1156,6 +1156,8 @@ static void cortex_a15_initfn(Object *obj)
     cpu->id_pfr0 = 0x00001131;
     cpu->id_pfr1 = 0x00011011;
     cpu->id_dfr0 = 0x02010555;
+    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */

These are:
SW_INCR # insn architecturally executed, cc pass, software increment
INST_RETIRED # insn architecturally executed
CPU_CYCLES # cycle

However we don't actually implement any of these, so should we be advertising them?

Quoted 22 lines
target-arm/cpu.c +1 -0
+    cpu->pmceid1 = 0x00000000;
     cpu->id_afr0 = 0x00000000;
     cpu->id_mmfr0 = 0x10201105;
     cpu->id_mmfr1 = 0x20000000;
target-arm/cpu64.c +2 -0
index c847513..8c4b6fd 100644
@@ -134,6 +134,8 @@ static void aarch64_a57_initfn(Object *obj)
     cpu->id_isar5 = 0x00011121;
     cpu->id_aa64pfr0 = 0x00002222;
     cpu->id_aa64dfr0 = 0x10305106;
+    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */
+    cpu->pmceid1 = 0x00000000;
     cpu->id_aa64isar0 = 0x00011120;
     cpu->id_aa64mmfr0 = 0x00001124;
     cpu->dbgdidr = 0x3516d000;
target-arm/helper.c +0 -0
index 5ea507f..66aa406 100644

target-arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 4198

Quoted 7 lines
target-arm/helper.c +4 -0
@@ -4192,6 +4192,14 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 1,
               .access = PL1_R, .type = ARM_CP_CONST,
               .resetvalue = cpu->id_aa64dfr1 },
+            { .name = "PMCEID0_EL0", .state = ARM_CP_STATE_AA64,
+              .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 6,
+              .access = PL1_R, .type = ARM_CP_CONST,
+              .resetvalue = cpu->pmceid0},

These have 32-bit versions from v8 and up (sadly not with the right opc values to use STATE_BOTH, so second stanza needed).

These are configurably RO from EL0, controlled by PMUSERENR_EL0.EN, so you want
.access = PL0_R, .accessfn = pmreg_access

Space before final "}", please.

Can we move these down so they're not placed right in the middle of the ID_AA64* registers ?

target-arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 4202

Quoted 4 lines
target-arm/helper.c +4 -0
+            { .name = "PMCEID1_EL0", .state = ARM_CP_STATE_AA64,
+              .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 7,
+              .access = PL1_R, .type = ARM_CP_CONST,
+              .resetvalue = cpu->pmceid1},

Ditto.

Quoted 5 lines
target-arm/helper.c +0 -1
             { .name = "ID_AA64AFR0_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 4,
               .access = PL1_R, .type = ARM_CP_CONST,
--

2.5.0

thanks
-- PMM

#3 lore raw

Re: [Qemu-devel] [PATCH v2 1/5] target-arm: Add the pmceid0 and pmceid1 registers

Quoted 41 lines · Peter Maydell
Quoted 29 lines · Alistair Francis
Signed-off-by Aaron Lindsay <[email protected]>
Signed-off-by Alistair Francis <[email protected]>
Tested-by Nathan Rossi <[email protected]>

---

 target-arm/cpu-qom.h | 2 ++
 target-arm/cpu.c     | 2 ++
 target-arm/cpu64.c   | 2 ++
 target-arm/helper.c  | 8 ++++++++
 4 files changed, 14 insertions(+)
target-arm/cpu-qom.h +2 -0
index 07c0a71..1cc4502 100644
@@ -148,6 +148,8 @@ typedef struct ARMCPU {
     uint32_t id_pfr0;
     uint32_t id_pfr1;
     uint32_t id_dfr0;
+    uint32_t pmceid0;
+    uint32_t pmceid1;
     uint32_t id_afr0;
     uint32_t id_mmfr0;
     uint32_t id_mmfr1;
target-arm/cpu.c +0 -0
index 7ddbf3d..937f845 100644

target-arm/cpu.c static void cortex_a15_initfn(Object *obj) · line 1159

Quoted 4 lines
target-arm/cpu.c +1 -0
@@ -1156,6 +1156,8 @@ static void cortex_a15_initfn(Object *obj)
     cpu->id_pfr0 = 0x00001131;
     cpu->id_pfr1 = 0x00011011;
     cpu->id_dfr0 = 0x02010555;
+    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */

These are:
SW_INCR # insn architecturally executed, cc pass, software increment
INST_RETIRED # insn architecturally executed
CPU_CYCLES # cycle

Quoted 2 lines

However we don't actually implement any of these, so should we be advertising them?

Perhaps I'm missing something, but I was under the impression that CPU cycle accounting was implemented as pmccntr_read/write in target-arm/helper.c.

The instruction count event may need a wrapper around cpu_get_icount().

SWINC is pretty trivial, but I don't think we actually use it, other than for some testing (but unfortunately not yet part of the kvm-unit-tests patchset).

Thanks,
Cov

Signature · 3 lines
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
#4 lore raw

Re: [Qemu-devel] [PATCH v2 1/5] target-arm: Add the pmceid0 and pmceid1 registers

Quoted 42 lines · Christopher Covington
Quoted 41 lines · Peter Maydell
Quoted 29 lines · Alistair Francis
Signed-off-by Aaron Lindsay <[email protected]>
Signed-off-by Alistair Francis <[email protected]>
Tested-by Nathan Rossi <[email protected]>

---

 target-arm/cpu-qom.h | 2 ++
 target-arm/cpu.c     | 2 ++
 target-arm/cpu64.c   | 2 ++
 target-arm/helper.c  | 8 ++++++++
 4 files changed, 14 insertions(+)
target-arm/cpu-qom.h +2 -0
index 07c0a71..1cc4502 100644
@@ -148,6 +148,8 @@ typedef struct ARMCPU {
     uint32_t id_pfr0;
     uint32_t id_pfr1;
     uint32_t id_dfr0;
+    uint32_t pmceid0;
+    uint32_t pmceid1;
     uint32_t id_afr0;
     uint32_t id_mmfr0;
     uint32_t id_mmfr1;
target-arm/cpu.c +0 -0
index 7ddbf3d..937f845 100644

target-arm/cpu.c static void cortex_a15_initfn(Object *obj) · line 1159

Quoted 4 lines
target-arm/cpu.c +1 -0
@@ -1156,6 +1156,8 @@ static void cortex_a15_initfn(Object *obj)
     cpu->id_pfr0 = 0x00001131;
     cpu->id_pfr1 = 0x00011011;
     cpu->id_dfr0 = 0x02010555;
+    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */

These are:
SW_INCR # insn architecturally executed, cc pass, software increment
INST_RETIRED # insn architecturally executed
CPU_CYCLES # cycle

Quoted 6 lines
Quoted 2 lines

However we don't actually implement any of these, so should we be advertising them?

Perhaps I'm missing something, but I was under the impression that CPU cycle accounting was implemented as pmccntr_read/write in target-arm/helper.c.

Yes, but that's not the same as implementing it as an event visible and controllable via the PM[X]EVCNTR and PM[X]EVTYPER register interface. (The cycle count has its own set of dedicated registers.)

Quoted 5 lines

The instruction count event may need a wrapper around cpu_get_icount().

SWINC is pretty trivial, but I don't think we actually use it, other than for some testing (but unfortunately not yet part of the kvm-unit-tests patchset).

Yep, so we could implement an event or two. The question is
(a) is it useful to do so? (quite possibly, though trying to get any kind of legitimate perf data out of a model is at best dicey)
(b) is it useful to advertise their support in this patchset but not actually implement the events? (much less clear)

If we want to add real events I think that should probably be its own patchset, and for this patchset we should stick to bringing the 64-bit PMU implementation into line with our existing 32-bit implementation (which is a "we support no event counters, only the separate cycle counter" implementation, which I think is architecturally permitted: PMCR_EL0.N == 0)

Similarly in a later patch I don't think we should implement the non-trivial PM[X]EVCNTR/PM[X]EVTYPER registers and the selectors until we actually implement some real events to count, ie we should postpone that to that future patchset, not put it in this one.

thanks
-- PMM

author
#5 lore raw

Re: [Qemu-devel] [PATCH v2 1/5] target-arm: Add the pmceid0 and pmceid1 registers

In reply to Peter Maydell
Quoted 41 lines · Peter Maydell
Quoted 29 lines · Alistair Francis
Signed-off-by Aaron Lindsay <[email protected]>
Signed-off-by Alistair Francis <[email protected]>
Tested-by Nathan Rossi <[email protected]>

---

 target-arm/cpu-qom.h | 2 ++
 target-arm/cpu.c     | 2 ++
 target-arm/cpu64.c   | 2 ++
 target-arm/helper.c  | 8 ++++++++
 4 files changed, 14 insertions(+)
target-arm/cpu-qom.h +2 -0
index 07c0a71..1cc4502 100644
@@ -148,6 +148,8 @@ typedef struct ARMCPU {
     uint32_t id_pfr0;
     uint32_t id_pfr1;
     uint32_t id_dfr0;
+    uint32_t pmceid0;
+    uint32_t pmceid1;
     uint32_t id_afr0;
     uint32_t id_mmfr0;
     uint32_t id_mmfr1;
target-arm/cpu.c +0 -0
index 7ddbf3d..937f845 100644

target-arm/cpu.c static void cortex_a15_initfn(Object *obj) · line 1159

Quoted 4 lines
target-arm/cpu.c +1 -0
@@ -1156,6 +1156,8 @@ static void cortex_a15_initfn(Object *obj)
     cpu->id_pfr0 = 0x00001131;
     cpu->id_pfr1 = 0x00011011;
     cpu->id_dfr0 = 0x02010555;
+    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */

These are:
SW_INCR # insn architecturally executed, cc pass, software increment
INST_RETIRED # insn architecturally executed
CPU_CYCLES # cycle

Quoted 2 lines

However we don't actually implement any of these, so should we be advertising them?

So this part I took directly from Chris's RFC. I'm happy to take it out if you would like.

Quoted 25 lines
Quoted 25 lines
target-arm/cpu.c +1 -0
+    cpu->pmceid1 = 0x00000000;
     cpu->id_afr0 = 0x00000000;
     cpu->id_mmfr0 = 0x10201105;
     cpu->id_mmfr1 = 0x20000000;
target-arm/cpu64.c +2 -0
index c847513..8c4b6fd 100644
@@ -134,6 +134,8 @@ static void aarch64_a57_initfn(Object *obj)
     cpu->id_isar5 = 0x00011121;
     cpu->id_aa64pfr0 = 0x00002222;
     cpu->id_aa64dfr0 = 0x10305106;
+    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */
+    cpu->pmceid1 = 0x00000000;
     cpu->id_aa64isar0 = 0x00011120;
     cpu->id_aa64mmfr0 = 0x00001124;
     cpu->dbgdidr = 0x3516d000;
target-arm/helper.c +0 -0
index 5ea507f..66aa406 100644
@@ -4192,6 +4192,14 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 1,
               .access = PL1_R, .type = ARM_CP_CONST,
Quoted 8 lines

target-arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 4198

Quoted 5 lines
target-arm/helper.c +4 -0
               .resetvalue = cpu->id_aa64dfr1 },
+            { .name = "PMCEID0_EL0", .state = ARM_CP_STATE_AA64,
+              .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 6,
+              .access = PL1_R, .type = ARM_CP_CONST,
+              .resetvalue = cpu->pmceid0},

These have 32-bit versions from v8 and up (sadly not with the right opc values to use STATE_BOTH, so second stanza needed).

Ok, I have added PMCEID0 and PMCEID1.

Quoted 9 lines

These are configurably RO from EL0, controlled by PMUSERENR_EL0.EN, so you want
.access = PL0_R, .accessfn = pmreg_access

Space before final "}", please.

Can we move these down so they're not placed right in the middle of the ID_AA64* registers ?

Fixed the rest.

Thanks,

Alistair

Quoted 17 lines

target-arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 4202

Quoted 5 lines
target-arm/helper.c +4 -0
+            { .name = "PMCEID1_EL0", .state = ARM_CP_STATE_AA64,
+              .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 7,
+              .access = PL1_R, .type = ARM_CP_CONST,
+              .resetvalue = cpu->pmceid1},

Ditto.

Quoted 5 lines
target-arm/helper.c +0 -1
             { .name = "ID_AA64AFR0_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 4,
               .access = PL1_R, .type = ARM_CP_CONST,
--

2.5.0

thanks
-- PMM

#6 lore raw

Re: [Qemu-devel] [PATCH v2 1/5] target-arm: Add the pmceid0 and pmceid1 registers

Quoted 42 lines · Alistair Francis
Quoted 41 lines · Peter Maydell
Quoted 29 lines · Alistair Francis
Signed-off-by Aaron Lindsay <[email protected]>
Signed-off-by Alistair Francis <[email protected]>
Tested-by Nathan Rossi <[email protected]>

---

 target-arm/cpu-qom.h | 2 ++
 target-arm/cpu.c     | 2 ++
 target-arm/cpu64.c   | 2 ++
 target-arm/helper.c  | 8 ++++++++
 4 files changed, 14 insertions(+)
target-arm/cpu-qom.h +2 -0
index 07c0a71..1cc4502 100644
@@ -148,6 +148,8 @@ typedef struct ARMCPU {
     uint32_t id_pfr0;
     uint32_t id_pfr1;
     uint32_t id_dfr0;
+    uint32_t pmceid0;
+    uint32_t pmceid1;
     uint32_t id_afr0;
     uint32_t id_mmfr0;
     uint32_t id_mmfr1;
target-arm/cpu.c +0 -0
index 7ddbf3d..937f845 100644

target-arm/cpu.c static void cortex_a15_initfn(Object *obj) · line 1159

Quoted 4 lines
target-arm/cpu.c +1 -0
@@ -1156,6 +1156,8 @@ static void cortex_a15_initfn(Object *obj)
     cpu->id_pfr0 = 0x00001131;
     cpu->id_pfr1 = 0x00011011;
     cpu->id_dfr0 = 0x02010555;
+    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */

These are:
SW_INCR # insn architecturally executed, cc pass, software increment
INST_RETIRED # insn architecturally executed
CPU_CYCLES # cycle

Quoted 5 lines
Quoted 2 lines

However we don't actually implement any of these, so should we be advertising them?

So this part I took directly from Chris's RFC. I'm happy to take it out if you would like.

I think removing the PMCEID0 change makes sense since these patches don't implement the advertised counters. We have other patches which do implement them, but they need some more work, so we can make this change if/when they're actually implemented.

-Aaron

Quoted 70 lines
Quoted 26 lines
Quoted 26 lines
target-arm/cpu.c +1 -0
+    cpu->pmceid1 = 0x00000000;
     cpu->id_afr0 = 0x00000000;
     cpu->id_mmfr0 = 0x10201105;
     cpu->id_mmfr1 = 0x20000000;
target-arm/cpu64.c +2 -0
index c847513..8c4b6fd 100644
@@ -134,6 +134,8 @@ static void aarch64_a57_initfn(Object *obj)
     cpu->id_isar5 = 0x00011121;
     cpu->id_aa64pfr0 = 0x00002222;
     cpu->id_aa64dfr0 = 0x10305106;
+    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */
+    cpu->pmceid1 = 0x00000000;
     cpu->id_aa64isar0 = 0x00011120;
     cpu->id_aa64mmfr0 = 0x00001124;
     cpu->dbgdidr = 0x3516d000;
target-arm/helper.c +0 -0
index 5ea507f..66aa406 100644
@@ -4192,6 +4192,14 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 1,
               .access = PL1_R, .type = ARM_CP_CONST,
Quoted 8 lines

target-arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 4198

Quoted 5 lines
target-arm/helper.c +4 -0
               .resetvalue = cpu->id_aa64dfr1 },
+            { .name = "PMCEID0_EL0", .state = ARM_CP_STATE_AA64,
+              .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 6,
+              .access = PL1_R, .type = ARM_CP_CONST,
+              .resetvalue = cpu->pmceid0},

These have 32-bit versions from v8 and up (sadly not with the right opc values to use STATE_BOTH, so second stanza needed).

Ok, I have added PMCEID0 and PMCEID1.

Quoted 9 lines

These are configurably RO from EL0, controlled by PMUSERENR_EL0.EN, so you want
.access = PL0_R, .accessfn = pmreg_access

Space before final "}", please.

Can we move these down so they're not placed right in the middle of the ID_AA64* registers ?

Fixed the rest.

Thanks,

Alistair

Quoted 17 lines

target-arm/helper.c void register_cp_regs_for_features(ARMCPU *cpu) · line 4202

Quoted 5 lines
target-arm/helper.c +4 -0
+            { .name = "PMCEID1_EL0", .state = ARM_CP_STATE_AA64,
+              .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 7,
+              .access = PL1_R, .type = ARM_CP_CONST,
+              .resetvalue = cpu->pmceid1},

Ditto.

Quoted 5 lines
target-arm/helper.c +0 -1
             { .name = "ID_AA64AFR0_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 4,
               .access = PL1_R, .type = ARM_CP_CONST,
--

2.5.0

thanks
-- PMM

#7 lore raw

Re: [Qemu-devel] [PATCH v2 1/5] target-arm: Add the pmceid0 and pmceid1 registers

Quoted 22 lines · Aaron Lindsay
Quoted 21 lines · Alistair Francis
Quoted 19 lines · Peter Maydell

target-arm/cpu.c static void cortex_a15_initfn(Object *obj) · line 1159

Quoted 9 lines · Alistair Francis
target-arm/cpu.c +1 -0
index 7ddbf3d..937f845 100644
@@ -1156,6 +1156,8 @@ static void cortex_a15_initfn(Object *obj)
     cpu->id_pfr0 = 0x00001131;
     cpu->id_pfr1 = 0x00011011;
     cpu->id_dfr0 = 0x02010555;
+    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */

These are:
SW_INCR # insn architecturally executed, cc pass, software increment
INST_RETIRED # insn architecturally executed
CPU_CYCLES # cycle

However we don't actually implement any of these, so should we be advertising them?

Quoted 7 lines
Quoted 2 lines

So this part I took directly from Chris's RFC. I'm happy to take it out if you would like.

I think removing the PMCEID0 change makes sense since these patches don't implement the advertised counters. We have other patches which do implement them, but they need some more work, so we can make this change if/when they're actually implemented.

I agree, so I propose to take Alistair's v3 series into target-arm.next with the following change:

target-arm/cpu.c +1 -1
index 1203783..e95b030 100644
@@ -1156,7 +1156,7 @@ static void cortex_a15_initfn(Object *obj)
     cpu->id_pfr0 = 0x00001131;
     cpu->id_pfr1 = 0x00011011;
     cpu->id_dfr0 = 0x02010555;
-    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */
+    cpu->pmceid0 = 0x0000000;
     cpu->pmceid1 = 0x00000000;
     cpu->id_afr0 = 0x00000000;
     cpu->id_mmfr0 = 0x10201105;
target-arm/cpu64.c +1 -1
index fc336e1..fa5eda2 100644
@@ -135,7 +135,7 @@ static void aarch64_a57_initfn(Object *obj)
     cpu->id_isar5 = 0x00011121;
     cpu->id_aa64pfr0 = 0x00002222;
     cpu->id_aa64dfr0 = 0x10305106;
-    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */
+    cpu->pmceid0 = 0x00000000;
     cpu->pmceid1 = 0x00000000;
     cpu->id_aa64isar0 = 0x00011120;
     cpu->id_aa64mmfr0 = 0x00001124;

If anybody disagrees let me know; otherwise this will go into a pullreq later this week.

thanks
-- PMM

author
#8 lore raw

PATCH v2 2/5 target-arm: Add Some of the performance monitor registers

In reply to Alistair Francis

This patch adds the following registers including read and write functions:
PMSELR, PMSELR_EL0, PMXEVCNTR, PMXEVCNTR_EL0, PMXEVTYPER and PMXEVTYPER_EL0.

Signed-off-by Aaron Lindsay <[email protected]>
Signed-off-by Alistair Francis <[email protected]>
Tested-by Nathan Rossi <[email protected]>

---

 target-arm/cpu.h    |  6 ++++
 target-arm/helper.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 84 insertions(+), 11 deletions(-)
target-arm/cpu.h +6 -0
index b8b3364..5c31c56 100644
@@ -117,6 +117,8 @@ typedef struct ARMGenericTimer {
 #define GTIMER_SEC  3
 #define NUM_GTIMERS 4
 
+#define NUM_PMU_COUNTERS 4
+
 typedef struct {
     uint64_t raw_tcr;
     uint32_t mask;

@@ -300,6 +302,7 @@ typedef struct CPUARMState {
         uint32_t c9_pmxevtyper; /* perf monitor event type */
         uint32_t c9_pmuserenr; /* perf monitor user enable */
         uint32_t c9_pminten; /* perf monitor interrupt enables */
+        uint32_t c9_pmselr; /* perf monitor event counter selection */
         union { /* Memory attribute redirection */
             struct {
 #ifdef HOST_WORDS_BIGENDIAN

@@ -361,6 +364,9 @@ typedef struct CPUARMState {
             uint64_t tpidruro_ns;
             uint64_t tpidrro_el[1];
         };
+        uint32_t c14_pmccfiltr; /* Performance Monitor Filter Register */
+        uint32_t c14_pmevcntr[NUM_PMU_COUNTERS];
+        uint32_t c14_pmevtyper[NUM_PMU_COUNTERS];
         uint64_t c14_cntfrq; /* Counter Frequency register */
         uint64_t c14_cntkctl; /* Timer Control register */
         uint32_t cnthctl_el2; /* Counter/Timer Hyp Control register */
target-arm/helper.c +78 -11

107 lines not on this page · raw message

Signature · 1 line
2.5.0
#9 lore raw

Re: [Qemu-devel] [PATCH v2 2/5] target-arm: Add Some of the performance monitor registers

Quoted 6 lines · Alistair Francis

This patch adds the following registers including read and write functions:
PMSELR, PMSELR_EL0, PMXEVCNTR, PMXEVCNTR_EL0, PMXEVTYPER and PMXEVTYPER_EL0.

Signed-off-by Aaron Lindsay <[email protected]>
Signed-off-by Alistair Francis <[email protected]>
Tested-by Nathan Rossi <[email protected]>

Is there any benefit to implementing all the type and counter registers and the multiplex selection, when we don't actually support any counters yet?

Quoted 60 lines

---

 target-arm/cpu.h    |  6 ++++
 target-arm/helper.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 84 insertions(+), 11 deletions(-)
target-arm/cpu.h +6 -0
index b8b3364..5c31c56 100644
@@ -117,6 +117,8 @@ typedef struct ARMGenericTimer {
 #define GTIMER_SEC  3
 #define NUM_GTIMERS 4
 
+#define NUM_PMU_COUNTERS 4
+
 typedef struct {
     uint64_t raw_tcr;
     uint32_t mask;

@@ -300,6 +302,7 @@ typedef struct CPUARMState {
         uint32_t c9_pmxevtyper; /* perf monitor event type */
         uint32_t c9_pmuserenr; /* perf monitor user enable */
         uint32_t c9_pminten; /* perf monitor interrupt enables */
+        uint32_t c9_pmselr; /* perf monitor event counter selection */
         union { /* Memory attribute redirection */
             struct {
 #ifdef HOST_WORDS_BIGENDIAN

@@ -361,6 +364,9 @@ typedef struct CPUARMState {
             uint64_t tpidruro_ns;
             uint64_t tpidrro_el[1];
         };
+        uint32_t c14_pmccfiltr; /* Performance Monitor Filter Register */
+        uint32_t c14_pmevcntr[NUM_PMU_COUNTERS];
+        uint32_t c14_pmevtyper[NUM_PMU_COUNTERS];
         uint64_t c14_cntfrq; /* Counter Frequency register */
         uint64_t c14_cntkctl; /* Timer Control register */
         uint32_t cnthctl_el2; /* Counter/Timer Hyp Control register */
target-arm/helper.c +15 -0
index 66aa406..164853f 100644
@@ -807,6 +807,58 @@ void pmccntr_sync(CPUARMState *env)
 
 #endif
 
+static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                           uint64_t value)
+{
+    env->cp15.c9_pmselr = value & 31;
+}
+
+static uint64_t pmevcntr_read(CPUARMState *env, const ARMCPRegInfo *ri,
+                              const uint8_t idx)
+{
+    if (idx >= NUM_PMU_COUNTERS) {
+        return arm_cp_read_zero(env, ri);
+    }
+    return env->cp15.c14_pmevcntr[idx];
+}
+

target-arm/helper.c void pmccntr_sync(CPUARMState *env) · line 834

Quoted 10 lines
target-arm/helper.c +10 -0
+static uint64_t pmxevcntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    return pmevcntr_read(env, ri, env->cp15.c9_pmselr & 31);
+}
+
+static void pmevcntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                             uint64_t value, const uint8_t idx)
+{
+    if (idx >= NUM_PMU_COUNTERS) {
+        return arm_cp_write_ignore(env, ri, value);

arm_cp_write_ignore() does nothing, as you might expect from the name, so why call it?

You might mention that this is CONSTRAINED UNPREDICTABLE and we are choosing to RAZ/WI.

Quoted 5 lines
target-arm/helper.c +5 -0
+    }
+    env->cp15.c14_pmevcntr[idx] = value;
+}
+
+static void pmxevcntr_write(CPUARMState *env, const ARMCPRegInfo *ri,

target-arm/helper.c void pmccntr_sync(CPUARMState *env) · line 849

Quoted 10 lines
target-arm/helper.c +10 -0
+                            uint64_t value)
+{
+    pmevcntr_write(env, ri, value, env->cp15.c9_pmselr & 31);
+}
+
+static uint64_t pmevtyper_read(CPUARMState *env, const ARMCPRegInfo *ri,
+                               const uint8_t idx)
+{
+    if (idx >= NUM_PMU_COUNTERS) {
+        return arm_cp_read_zero(env, ri);

You could just return 0.

Quoted 37 lines
target-arm/helper.c +28 -2
+    }
+    return env->cp15.c14_pmevtyper[idx];
+}
+
+static uint64_t pmxevtyper_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    if (!!(env->cp15.c9_pmselr & 31)) {
+        return env->cp15.c14_pmccfiltr;
+    }
+    return pmevtyper_read(env, ri, env->cp15.c9_pmselr & 31);
+}
+
 static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                             uint64_t value)
 {

@@ -986,8 +1038,32 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
      * We choose to RAZ/WI.
      */
     { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
-      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
-      .accessfn = pmreg_access },
+      .access = PL0_RW, .type = ARM_CP_ALIAS,
+      .accessfn = pmreg_access, .writefn = pmselr_write,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr) },
+    { .name = "PMSELR_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 5,
+      .access = PL0_RW, .accessfn = pmreg_access,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr),
+      .writefn = pmselr_write, .resetvalue = 0 },
+    { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2,
+      .access = PL0_RW, .type = ARM_CP_ALIAS,
+      .accessfn = pmreg_access, .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_ALIAS,
+      .accessfn = pmreg_access, .writefn = pmxevcntr_write,

target-arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 1066

Quoted 10 lines
target-arm/helper.c +10 -0
+      .readfn = pmxevcntr_read },
+    { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
+      .access = PL0_RW, .type = ARM_CP_ALIAS,
+      .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_ALIAS,
+      .accessfn = pmreg_access, .writefn = pmxevtyper_write,
+      .readfn = pmxevtyper_read },

This isn't going to handle migration correctly, because there are a whole pile of data values behind these registers.

It may be easiest to implement the PMEVTYPER<n>_EL0 and PMEVCNTR<n>_EL0 registers (which are PMUv3 only and provide un-multiplexed access to the type and counter registers), gate them so they're present but not guest-accessible on PMUv2 and below, and use them for migrating the state. Then the X multiplexed registers can be simple CP_ALIASes as you have them here.

Quoted 20 lines
target-arm/helper.c +0 -10
 #ifndef CONFIG_USER_ONLY
     { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
       .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,

@@ -1006,15 +1082,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .type = ARM_CP_IO,
       .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0),
       .resetvalue = 0, },
-    { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
-      .access = PL0_RW,
-      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
-      .accessfn = pmreg_access, .writefn = pmxevtyper_write,
-      .raw_writefn = raw_write },
-    /* 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 },
     { .name = "PMUSERENR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 0,
       .access = PL0_R | PL1_RW,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
--
Quoted 2 lines

2.5.0

thanks
-- PMM

author
#10 lore raw

Re: [Qemu-devel] [PATCH v2 2/5] target-arm: Add Some of the performance monitor registers

Quoted 5 lines · Peter Maydell
Quoted 3 lines · Alistair Francis

This patch adds the following registers including read and write functions:
PMSELR, PMSELR_EL0, PMXEVCNTR, PMXEVCNTR_EL0, PMXEVTYPER and PMXEVTYPER_EL0.

Quoted 7 lines
Quoted 3 lines
Signed-off-by Aaron Lindsay <[email protected]>
Signed-off-by Alistair Francis <[email protected]>
Tested-by Nathan Rossi <[email protected]>

Is there any benefit to implementing all the type and counter registers and the multiplex selection, when we don't actually support any counters yet?

Thanks for reviewing these. The others (patch 1, 3 and 4) are more important to me, so I'm sending them out so they can be applied then I'll look at these after.

Thanks,

Alistair

Quoted 180 lines
Quoted 61 lines

---

 target-arm/cpu.h    |  6 ++++
 target-arm/helper.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 84 insertions(+), 11 deletions(-)
target-arm/cpu.h +6 -0
index b8b3364..5c31c56 100644
@@ -117,6 +117,8 @@ typedef struct ARMGenericTimer {
 #define GTIMER_SEC  3
 #define NUM_GTIMERS 4
 
+#define NUM_PMU_COUNTERS 4
+
 typedef struct {
     uint64_t raw_tcr;
     uint32_t mask;

@@ -300,6 +302,7 @@ typedef struct CPUARMState {
         uint32_t c9_pmxevtyper; /* perf monitor event type */
         uint32_t c9_pmuserenr; /* perf monitor user enable */
         uint32_t c9_pminten; /* perf monitor interrupt enables */
+        uint32_t c9_pmselr; /* perf monitor event counter selection */
         union { /* Memory attribute redirection */
             struct {
 #ifdef HOST_WORDS_BIGENDIAN

@@ -361,6 +364,9 @@ typedef struct CPUARMState {
             uint64_t tpidruro_ns;
             uint64_t tpidrro_el[1];
         };
+        uint32_t c14_pmccfiltr; /* Performance Monitor Filter Register */
+        uint32_t c14_pmevcntr[NUM_PMU_COUNTERS];
+        uint32_t c14_pmevtyper[NUM_PMU_COUNTERS];
         uint64_t c14_cntfrq; /* Counter Frequency register */
         uint64_t c14_cntkctl; /* Timer Control register */
         uint32_t cnthctl_el2; /* Counter/Timer Hyp Control register */
target-arm/helper.c +15 -0
index 66aa406..164853f 100644
@@ -807,6 +807,58 @@ void pmccntr_sync(CPUARMState *env)
 
 #endif
 
+static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                           uint64_t value)
+{
+    env->cp15.c9_pmselr = value & 31;
+}
+
+static uint64_t pmevcntr_read(CPUARMState *env, const ARMCPRegInfo *ri,
+                              const uint8_t idx)
+{
+    if (idx >= NUM_PMU_COUNTERS) {
+        return arm_cp_read_zero(env, ri);
+    }
+    return env->cp15.c14_pmevcntr[idx];
+}
+

target-arm/helper.c void pmccntr_sync(CPUARMState *env) · line 834

Quoted 10 lines
target-arm/helper.c +10 -0
+static uint64_t pmxevcntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    return pmevcntr_read(env, ri, env->cp15.c9_pmselr & 31);
+}
+
+static void pmevcntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                             uint64_t value, const uint8_t idx)
+{
+    if (idx >= NUM_PMU_COUNTERS) {
+        return arm_cp_write_ignore(env, ri, value);

arm_cp_write_ignore() does nothing, as you might expect from the name, so why call it?

You might mention that this is CONSTRAINED UNPREDICTABLE and we are choosing to RAZ/WI.

Quoted 5 lines
target-arm/helper.c +5 -0
+    }
+    env->cp15.c14_pmevcntr[idx] = value;
+}
+
+static void pmxevcntr_write(CPUARMState *env, const ARMCPRegInfo *ri,

target-arm/helper.c void pmccntr_sync(CPUARMState *env) · line 849

Quoted 10 lines
target-arm/helper.c +10 -0
+                            uint64_t value)
+{
+    pmevcntr_write(env, ri, value, env->cp15.c9_pmselr & 31);
+}
+
+static uint64_t pmevtyper_read(CPUARMState *env, const ARMCPRegInfo *ri,
+                               const uint8_t idx)
+{
+    if (idx >= NUM_PMU_COUNTERS) {
+        return arm_cp_read_zero(env, ri);

You could just return 0.

Quoted 37 lines
target-arm/helper.c +28 -2
+    }
+    return env->cp15.c14_pmevtyper[idx];
+}
+
+static uint64_t pmxevtyper_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    if (!!(env->cp15.c9_pmselr & 31)) {
+        return env->cp15.c14_pmccfiltr;
+    }
+    return pmevtyper_read(env, ri, env->cp15.c9_pmselr & 31);
+}
+
 static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                             uint64_t value)
 {

@@ -986,8 +1038,32 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
      * We choose to RAZ/WI.
      */
     { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
-      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
-      .accessfn = pmreg_access },
+      .access = PL0_RW, .type = ARM_CP_ALIAS,
+      .accessfn = pmreg_access, .writefn = pmselr_write,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr) },
+    { .name = "PMSELR_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 5,
+      .access = PL0_RW, .accessfn = pmreg_access,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr),
+      .writefn = pmselr_write, .resetvalue = 0 },
+    { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2,
+      .access = PL0_RW, .type = ARM_CP_ALIAS,
+      .accessfn = pmreg_access, .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_ALIAS,
+      .accessfn = pmreg_access, .writefn = pmxevcntr_write,

target-arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 1066

Quoted 10 lines
target-arm/helper.c +10 -0
+      .readfn = pmxevcntr_read },
+    { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
+      .access = PL0_RW, .type = ARM_CP_ALIAS,
+      .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_ALIAS,
+      .accessfn = pmreg_access, .writefn = pmxevtyper_write,
+      .readfn = pmxevtyper_read },

This isn't going to handle migration correctly, because there are a whole pile of data values behind these registers.

It may be easiest to implement the PMEVTYPER<n>_EL0 and PMEVCNTR<n>_EL0 registers (which are PMUv3 only and provide un-multiplexed access to the type and counter registers), gate them so they're present but not guest-accessible on PMUv2 and below, and use them for migrating the state. Then the X multiplexed registers can be simple CP_ALIASes as you have them here.

Quoted 20 lines
target-arm/helper.c +0 -10
 #ifndef CONFIG_USER_ONLY
     { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
       .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,

@@ -1006,15 +1082,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .type = ARM_CP_IO,
       .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0),
       .resetvalue = 0, },
-    { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
-      .access = PL0_RW,
-      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
-      .accessfn = pmreg_access, .writefn = pmxevtyper_write,
-      .raw_writefn = raw_write },
-    /* 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 },
     { .name = "PMUSERENR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 0,
       .access = PL0_R | PL1_RW,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
--
Quoted 2 lines

2.5.0

thanks
-- PMM

author
#11 lore raw

PATCH v2 3/5 target-arm: Add the pmovsclr_el0 and pmintenclr_el1 registers

In reply to Alistair Francis
Signed-off-by Aaron Lindsay <[email protected]>
Signed-off-by Alistair Francis <[email protected]>
Tested-by Nathan Rossi <[email protected]>

---

 target-arm/helper.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
target-arm/helper.c +12 -0
index 164853f..8d401c6 100644
@@ -1031,6 +1031,13 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .accessfn = pmreg_access,
       .writefn = pmovsr_write,
       .raw_writefn = raw_write },
+    { .name = "PMOVSCLR_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 3,
+      .access = PL0_RW, .accessfn = pmreg_access,
+      .type = ARM_CP_ALIAS,
+      .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, .type = ARM_CP_NOP },

@@ -1096,6 +1103,11 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .access = PL1_RW, .type = ARM_CP_ALIAS,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
       .writefn = pmintenclr_write, },
+    { .name = "PMINTENCLR_EL1", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 2,
+      .access = PL1_RW, .type = ARM_CP_ALIAS,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
+      .writefn = pmintenclr_write },
     { .name = "VBAR", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
       .access = PL1_RW, .writefn = vbar_write,
Signature · 1 line
2.5.0
author
#13 lore raw

PATCH v2 4/5 target-arm: Add PMUSERENR_EL0 register

In reply to Alistair Francis

The Linux kernel accesses this register early in its setup.

Signed-off-by Christopher Covington <[email protected]>
Signed-off-by Alistair Francis <[email protected]>

---

 target-arm/helper.c | 6 ++++++
 1 file changed, 6 insertions(+)
target-arm/helper.c +6 -0
index 8d401c6..b4bf6fa 100644
@@ -1094,6 +1094,12 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
       .resetvalue = 0,
       .writefn = pmuserenr_write, .raw_writefn = raw_write },
+    { .name = "PMUSERENR_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 14, .opc2 = 0,
+      .access = PL0_R | PL1_RW,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
+      .resetvalue = 0,
+      .writefn = pmuserenr_write, .raw_writefn = raw_write },
     { .name = "PMINTENSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 1,
       .access = PL1_RW,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
Signature · 1 line
2.5.0
Reviewed-by on v2 4/5
#14 lore raw

Re: [Qemu-devel] [PATCH v2 4/5] target-arm: Add PMUSERENR_EL0 register

Quoted 14 lines · Alistair Francis

The Linux kernel accesses this register early in its setup.

Signed-off-by Christopher Covington <[email protected]>
Signed-off-by Alistair Francis <[email protected]>

---

 target-arm/helper.c | 6 ++++++
 1 file changed, 6 insertions(+)
target-arm/helper.c +0 -0
index 8d401c6..b4bf6fa 100644

target-arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = { · line 1102

Quoted 9 lines
target-arm/helper.c +6 -0
@@ -1094,6 +1094,12 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
       .resetvalue = 0,
       .writefn = pmuserenr_write, .raw_writefn = raw_write },
+    { .name = "PMUSERENR_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 14, .opc2 = 0,
+      .access = PL0_R | PL1_RW,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
+      .resetvalue = 0,
+      .writefn = pmuserenr_write, .raw_writefn = raw_write },

Either this or the 32-bit version need to be marked ARM_CP_ALIAS.

Otherwise

Reviewed-by Peter Maydell <[email protected]>

thanks
-- PMM

author
#15 lore raw

PATCH v2 5/5 target-arm: Unmask PMU bits in debug feature register

In reply to Alistair Francis

The previously missing registers are now present in QEMU.

Signed-off-by Christopher Covington <[email protected]>
Signed-off-by Alistair Francis <[email protected]>

---

 target-arm/helper.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)
target-arm/helper.c +1 -6
index b4bf6fa..1775768 100644
@@ -4267,12 +4267,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             { .name = "ID_AA64DFR0_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 0,
               .access = PL1_R, .type = ARM_CP_CONST,
-              /* We mask out the PMUVer field, because we don't currently
-               * implement the PMU. Not advertising it prevents the guest
-               * from trying to use it and getting UNDEFs on registers we
-               * don't implement.
-               */
-              .resetvalue = cpu->id_aa64dfr0 & ~0xf00 },
+              .resetvalue = cpu->id_aa64dfr0 },
             { .name = "ID_AA64DFR1_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 1,
               .access = PL1_R, .type = ARM_CP_CONST,
Signature · 1 line
2.5.0
#16 lore raw

Re: [Qemu-devel] [PATCH v2 5/5] target-arm: Unmask PMU bits in debug feature register

Quoted 1 line · Alistair Francis

The previously missing registers are now present in QEMU.

I think we're still missing some:

32 bit version present, 64 bit version missing:
PMSWINC
PMINTENSET

not implemented in 32 bit either:
PMOVSCLR (I think this is only in PMUv3 so needs care as to
which CPUs we define it on)

Quoted 26 lines
Signed-off-by Christopher Covington <[email protected]>
Signed-off-by Alistair Francis <[email protected]>

---

 target-arm/helper.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)
target-arm/helper.c +1 -7
index b4bf6fa..1775768 100644
@@ -4267,12 +4267,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             { .name = "ID_AA64DFR0_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 0,
               .access = PL1_R, .type = ARM_CP_CONST,
-              /* We mask out the PMUVer field, because we don't currently
-               * implement the PMU. Not advertising it prevents the guest
-               * from trying to use it and getting UNDEFs on registers we
-               * don't implement.
-               */
-              .resetvalue = cpu->id_aa64dfr0 & ~0xf00 },
+              .resetvalue = cpu->id_aa64dfr0 },
             { .name = "ID_AA64DFR1_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 1,
               .access = PL1_R, .type = ARM_CP_CONST,
--
Quoted 2 lines

2.5.0

thanks
-- PMM