Skip to contentSkip to conversation

target-arm: Extend PMCCNTR for ARMv8

PATCH Superseded Alistair Francis 4 participants 21 messages Jun 24, 2014 – Jun 26, 2014

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

cover target-arm: Extend PMCCNTR for ARMv8

This patch series continues on from my original PMCCNTR patch work to extend the counter to be 64-bit and support for the PMCCFILTR_EL0 register which allows the counter to be disabled based on the current EL

Alistair Francis (7):
target-arm: Make the ARM PMCCNTR register 64-bit
target-arm: Implement PMCCNTR_EL0 and related registers
target-arm: Add helper macros and defines for CCNT register
target-arm: Implement pmccntr_sync function
target-arm: Remove old code and replace with new functions
target-arm: Implement pmccfiltr_write function
target-arm: Call the pmccntr_sync function when swapping ELs

 target-arm/cpu.h        |   33 +++++++++++++-
 target-arm/helper-a64.c |    5 ++
 target-arm/helper.c     |  114 ++++++++++++++++++++++++++++++++++------------
 target-arm/op_helper.c  |    6 +++
 4 files changed, 127 insertions(+), 31 deletions(-)
#1 lore raw

PATCH 1/7 target-arm: Make the ARM PMCCNTR register 64-bit

This makes the PMCCNTR register 64-bit to allow for the 64-bit ARMv8 version

---

 target-arm/cpu.h    |    2 +-
 target-arm/helper.c |   19 +++++++++----------
 2 files changed, 10 insertions(+), 11 deletions(-)
target-arm/cpu.h +1 -1
index 369d472..cd1c7b6 100644
@@ -223,7 +223,7 @@ typedef struct CPUARMState {
         /* If the counter is enabled, this stores the last time the counter
          * was reset. Otherwise it stores the counter value
          */
-        uint32_t c15_ccnt;
+        uint64_t c15_ccnt;
     } cp15;
 
     struct {
target-arm/helper.c +9 -10
index ed4d2bb..ac10564 100644
@@ -550,11 +550,10 @@ static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri)
 static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                        uint64_t value)
 {
-    /* Don't computer the number of ticks in user mode */
-    uint32_t temp_ticks;
+    uint64_t temp_ticks;
 
-    temp_ticks = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) *
-                  get_ticks_per_sec() / 1000000;
+    temp_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
+                          get_ticks_per_sec(), 1000000);
 
     if (env->cp15.c9_pmcr & PMCRE) {
         /* If the counter is enabled */
@@ -586,15 +585,15 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 
 static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
-    uint32_t total_ticks;
+    uint64_t total_ticks;
 
     if (!(env->cp15.c9_pmcr & PMCRE)) {
         /* Counter is disabled, do not change value */
         return env->cp15.c15_ccnt;
     }
 
-    total_ticks = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) *
-                  get_ticks_per_sec() / 1000000;
+    total_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
+                  get_ticks_per_sec(), 1000000);
 
     if (env->cp15.c9_pmcr & PMCRD) {
         /* Increment once every 64 processor clock cycles */
@@ -606,7 +605,7 @@ static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                         uint64_t value)
 {
-    uint32_t total_ticks;
+    uint64_t total_ticks;
 
     if (!(env->cp15.c9_pmcr & PMCRE)) {
         /* Counter is disabled, set the absolute value */
@@ -614,8 +613,8 @@ static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
         return;
     }
 
-    total_ticks = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) *
-                  get_ticks_per_sec() / 1000000;
+    total_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
+                  get_ticks_per_sec(), 1000000);
 
     if (env->cp15.c9_pmcr & PMCRD) {
         /* Increment once every 64 processor clock cycles */
Signature · 1 line
1.7.1
#2 lore raw

PATCH 2/7 target-arm: Implement PMCCNTR_EL0 and related registers

In reply to Alistair Francis

This patch adds support for the ARMv8 version of the PMCCNTR and related registers. It also starts to implement the PMCCFILTR_EL0 register.

---

 target-arm/cpu.h    |    1 +
 target-arm/helper.c |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 0 deletions(-)
target-arm/cpu.h +1 -0
index cd1c7b6..6a2efd8 100644
@@ -224,6 +224,7 @@ typedef struct CPUARMState {
          * was reset. Otherwise it stores the counter value
          */
         uint64_t c15_ccnt;
+        uint64_t pmccfiltr_el0; /* Performance Monitor Filter Register */
     } cp15;
 
     struct {
target-arm/helper.c +39 -0
index ac10564..ce986ee 100644
@@ -738,7 +738,22 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .writefn = pmcntenset_write,
       .accessfn = pmreg_access,
       .raw_writefn = raw_write },
+    { .name = "PMCNTENSET_EL0", .crn = 9, .crm = 12, .opc0 = 3, .opc1 = 3,
+      .opc2 = 1, .access = PL0_RW, .resetvalue = 0,
+      .state = ARM_CP_STATE_AA64,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
+      .writefn = pmcntenset_write,
+      .accessfn = pmreg_access,
+      .raw_writefn = raw_write },
     { .name = "PMCNTENCLR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 2,
+      .access = PL0_RW,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
+      .accessfn = pmreg_access,
+      .writefn = pmcntenclr_write,
+      .type = ARM_CP_NO_MIGRATE },
+    { .name = "PMCNTENCLR_EL0", .crn = 9, .crm = 12, .opc0 = 3, .opc1 = 3,
+      .opc2 = 2,
+      .state = ARM_CP_STATE_AA64,
       .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
       .accessfn = pmreg_access,
       .writefn = pmcntenclr_write,
@@ -762,11 +777,25 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
       .readfn = pmccntr_read, .writefn = pmccntr_write,
       .accessfn = pmreg_access },
+    { .name = "PMCCNTR_EL0", .cp = 15, .crn = 9, .crm = 13, .opc1 = 3,
+      .opc2 = 0,
+      .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
+      .state = ARM_CP_STATE_AA64,
+      .readfn = pmccntr_read, .writefn = pmccntr_write,
+      .accessfn = pmreg_access },
 #endif
+    { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7,
+      .access = PL0_RW, .accessfn = pmreg_access,
+      .state = ARM_CP_STATE_AA64,
+      .resetvalue = 0,
+      .type = ARM_CP_IO,
+      .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0), },
     { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
       .access = PL0_RW,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
       .accessfn = pmreg_access, .writefn = pmxevtyper_write,
+      .resetvalue = 0,
       .raw_writefn = raw_write },
     /* Unimplemented, RAZ/WI. */
     { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2,
@@ -2324,6 +2353,16 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             .raw_writefn = raw_write,
         };
         define_one_arm_cp_reg(cpu, &pmcr);
+        ARMCPRegInfo pmcr64 = {
+            .name = "PMCR_EL0", .crn = 9, .crm = 12, .opc0 = 3, .opc1 = 3,
+            .opc2 = 0,
+            .state = ARM_CP_STATE_AA64,
+            .access = PL0_RW, .resetvalue = cpu->midr & 0xff000000,
+            .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr),
+            .accessfn = pmreg_access, .writefn = pmcr_write,
+            .raw_writefn = raw_write,
+        };
+        define_one_arm_cp_reg(cpu, &pmcr64);
 #endif
         ARMCPRegInfo clidr = {
             .name = "CLIDR", .state = ARM_CP_STATE_BOTH,
Signature · 1 line
1.7.1
#3 lore raw

PATCH 3/7 target-arm: Add helper macros and defines for CCNT register

In reply to Alistair Francis

Include a helper function to determine if the CCNT counter is enabled as well as the constants used to mask the pmccfiltr_el0 register.

---

 target-arm/cpu.h |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)
target-arm/cpu.h +19 -0
index 6a2efd8..31aa09c 100644
@@ -111,6 +111,25 @@ typedef struct ARMGenericTimer {
 #define GTIMER_VIRT 1
 #define NUM_GTIMERS 2
 
+#ifndef CONFIG_USER_ONLY
+    /* Definitions for the PMCCFILTR_EL0 and PMXEVTYPER registers */
+    #define PMCP 0x80000000
+    #define PMCU 0x40000000
+
+    /* This implements the PMCCFILTR_EL0:P and U bits; the PMXEVTYPER:P and U
+     * bits and the c9_pmcr:E bit.
+     *
+     * It does not suppor the secure/non-secure componenets of the
+     * PMCCFILTR_EL0 register
+     */
+    #define CCNT_ENABLED(env) \
+        ((env->cp15.c9_pmcr & PMCRE) && \
+        !(env->cp15.pmccfiltr_el0 & PMCP && arm_current_pl(env) == 1) && \
+        !(env->cp15.pmccfiltr_el0 & PMCU && arm_current_pl(env) == 0) && \
+        !(env->cp15.c9_pmxevtyper & PMCP && arm_current_pl(env) == 1) && \
+        !(env->cp15.c9_pmxevtyper & PMCU && arm_current_pl(env) == 0))
+#endif
+
 typedef struct CPUARMState {
     /* Regs for current mode.  */
     uint32_t regs[16];
Signature · 1 line
1.7.1
#4 lore raw

Re: [Qemu-devel] [PATCH v1 3/7] target-arm: Add helper macros and defines for CCNT register

Hi Alistair,

Quoted 15 lines · Alistair Francis

Include a helper function to determine if the CCNT counter is enabled as well as the constants used to mask the pmccfiltr_el0 register.

---

 target-arm/cpu.h |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)
target-arm/cpu.h +0 -0
index 6a2efd8..31aa09c 100644

target-arm/cpu.h typedef struct ARMGenericTimer { · line 117

Quoted 7 lines
target-arm/cpu.h +4 -0
@@ -111,6 +111,25 @@ typedef struct ARMGenericTimer {
 #define GTIMER_VIRT 1
 #define NUM_GTIMERS 2
 
+#ifndef CONFIG_USER_ONLY
+    /* Definitions for the PMCCFILTR_EL0 and PMXEVTYPER registers */
+    #define PMCP 0x80000000
+    #define PMCU 0x40000000

These names are very similar to what one might use for the PMCR, which has its P bit somewhere completely different. A prefix derived from PMXEVTYPER might be clearer.

target-arm/cpu.h typedef struct ARMGenericTimer { · line 122

Quoted 4 lines
target-arm/cpu.h +4 -0
+    /* This implements the PMCCFILTR_EL0:P and U bits; the PMXEVTYPER:P and U
+     * bits and the c9_pmcr:E bit.
+     *
+     * It does not suppor the secure/non-secure componenets of the

Nit: support, components

Quoted 4 lines
target-arm/cpu.h +3 -0
+     * PMCCFILTR_EL0 register
+     */
+    #define CCNT_ENABLED(env) \

target-arm/cpu.h typedef struct ARMGenericTimer { · line 135

Quoted 10 lines
target-arm/cpu.h +7 -0
+        ((env->cp15.c9_pmcr & PMCRE) && \
+        !(env->cp15.pmccfiltr_el0 & PMCP && arm_current_pl(env) == 1) && \
+        !(env->cp15.pmccfiltr_el0 & PMCU && arm_current_pl(env) == 0) && \
+        !(env->cp15.c9_pmxevtyper & PMCP && arm_current_pl(env) == 1) && \
+        !(env->cp15.c9_pmxevtyper & PMCU && arm_current_pl(env) == 0))
+#endif
+
 typedef struct CPUARMState {
     /* Regs for current mode.  */
     uint32_t regs[16];

Christopher

Signature · 3 lines
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.
#5 lore raw

Re: [Qemu-devel] [PATCH v1 3/7] target-arm: Add helper macros and defines for CCNT register

Quoted 21 lines · Christopher Covington

Hi Alistair,

Quoted 18 lines · Alistair Francis

Include a helper function to determine if the CCNT counter is enabled as well as the constants used to mask the pmccfiltr_el0 register.

---

 target-arm/cpu.h |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)
target-arm/cpu.h +0 -0
index 6a2efd8..31aa09c 100644
@@ -111,6 +111,25 @@ typedef struct ARMGenericTimer {
 #define GTIMER_VIRT 1
 #define NUM_GTIMERS 2
 
Quoted 8 lines

target-arm/cpu.h typedef struct ARMGenericTimer { · line 117

Quoted 4 lines
target-arm/cpu.h +4 -0
+#ifndef CONFIG_USER_ONLY
+    /* Definitions for the PMCCFILTR_EL0 and PMXEVTYPER registers */
+    #define PMCP 0x80000000
+    #define PMCU 0x40000000

These names are very similar to what one might use for the PMCR, which has its P bit somewhere completely different. A prefix derived from PMXEVTYPER might be clearer.

Yep, can do

Quoted 7 lines

target-arm/cpu.h typedef struct ARMGenericTimer { · line 122

Quoted 5 lines
target-arm/cpu.h +4 -0
+    /* This implements the PMCCFILTR_EL0:P and U bits; the PMXEVTYPER:P and U
+     * bits and the c9_pmcr:E bit.
+     *
+     * It does not suppor the secure/non-secure componenets of the

Nit: support, components

Thanks, will fix in next version

Quoted 23 lines
Quoted 5 lines
target-arm/cpu.h +3 -0
+     * PMCCFILTR_EL0 register
+     */
+    #define CCNT_ENABLED(env) \

target-arm/cpu.h typedef struct ARMGenericTimer { · line 135

Quoted 10 lines
target-arm/cpu.h +7 -0
+        ((env->cp15.c9_pmcr & PMCRE) && \
+        !(env->cp15.pmccfiltr_el0 & PMCP && arm_current_pl(env) == 1) && \
+        !(env->cp15.pmccfiltr_el0 & PMCU && arm_current_pl(env) == 0) && \
+        !(env->cp15.c9_pmxevtyper & PMCP && arm_current_pl(env) == 1) && \
+        !(env->cp15.c9_pmxevtyper & PMCU && arm_current_pl(env) == 0))
+#endif
+
 typedef struct CPUARMState {
     /* Regs for current mode.  */
     uint32_t regs[16];

Christopher

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation.

#6 lore raw

Re: [Qemu-devel] [PATCH v1 3/7] target-arm: Add helper macros and defines for CCNT register

In reply to Alistair Francis

target-arm/cpu.h typedef struct ARMGenericTimer { · line 131

Quoted 13 lines · Alistair Francis
target-arm/cpu.h +13 -0
+    /* This implements the PMCCFILTR_EL0:P and U bits; the PMXEVTYPER:P and U
+     * bits and the c9_pmcr:E bit.
+     *
+     * It does not suppor the secure/non-secure componenets of the
+     * PMCCFILTR_EL0 register
+     */
+    #define CCNT_ENABLED(env) \
+        ((env->cp15.c9_pmcr & PMCRE) && \
+        !(env->cp15.pmccfiltr_el0 & PMCP && arm_current_pl(env) == 1) && \
+        !(env->cp15.pmccfiltr_el0 & PMCU && arm_current_pl(env) == 0) && \
+        !(env->cp15.c9_pmxevtyper & PMCP && arm_current_pl(env) == 1) && \
+        !(env->cp15.c9_pmxevtyper & PMCU && arm_current_pl(env) == 0))
+#endif

This really should be a function, not a macro. (And it probably ought to live in internals.h.)

Can you write the check so that it won't blow up and need fixing when arm_current_pl() returns 2 or 3 in the future?

thanks
-- PMM

#7 lore raw

Re: [Qemu-devel] [PATCH v1 3/7] target-arm: Add helper macros and defines for CCNT register

Quoted 15 lines · Peter Maydell

target-arm/cpu.h typedef struct ARMGenericTimer { · line 131

Quoted 13 lines · Alistair Francis
target-arm/cpu.h +13 -0
+    /* This implements the PMCCFILTR_EL0:P and U bits; the PMXEVTYPER:P and U
+     * bits and the c9_pmcr:E bit.
+     *
+     * It does not suppor the secure/non-secure componenets of the
+     * PMCCFILTR_EL0 register
+     */
+    #define CCNT_ENABLED(env) \
+        ((env->cp15.c9_pmcr & PMCRE) && \
+        !(env->cp15.pmccfiltr_el0 & PMCP && arm_current_pl(env) == 1) && \
+        !(env->cp15.pmccfiltr_el0 & PMCU && arm_current_pl(env) == 0) && \
+        !(env->cp15.c9_pmxevtyper & PMCP && arm_current_pl(env) == 1) && \
+        !(env->cp15.c9_pmxevtyper & PMCU && arm_current_pl(env) == 0))
+#endif
Quoted 5 lines

This really should be a function, not a macro. (And it probably ought to live in internals.h.)

Can you write the check so that it won't blow up and need fixing when arm_current_pl() returns 2 or 3 in the future?

Ok, I can move it to a function and it shouldn't be too hard to allow it to work with EL 2 or 3 (from memory there isn't a specific check for EL2, just the secure/non-secure stuff)

Quoted 4 lines

thanks
-- PMM

#8 lore raw

PATCH 4/7 target-arm: Implement pmccntr_sync function

In reply to Alistair Francis

This is used to synchronise the PMCCNTR counter and swap its state between enabled and disabled if required. It must always be called twice, both before and after any logic that could change the state of the PMCCNTR counter.

---
Remembering that the c15_ccnt register stores the last time the counter was reset if enabled. If disabled it stores the counter value (when it was disabled).

The three use cases are as below:
--
Starts enabled/disabled and is staying enabled/disabled --
The two calls to pmccntr_sync cancel each other out. Each call implements this logic:
env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt

Which expands to:
env->cp15.c15_ccnt = temp_ticks -

                     (temp_ticks - env->cp15.c15_ccnt)

env->cp15.c15_ccnt = env->cp15.c15_ccnt

--
Starts enabled, gets disabled
--
The logic is run during the first call while during the second call it is not. That means that c15_ccnt changes from storing the last time the counter was reset, to storing the absolute value of the counter.

--
Starts disabled, gets enabled
--
During the fist call no changes are made, while during the second call the register is changed. This changes it from storing the absolute value to storing the last time the counter was reset.

 target-arm/cpu.h    |   11 +++++++++++
 target-arm/helper.c |   19 +++++++++++++++++++
 2 files changed, 30 insertions(+), 0 deletions(-)
target-arm/cpu.h +11 -0
index 31aa09c..0984eda 100644
@@ -1051,6 +1051,17 @@ static inline bool cp_access_ok(int current_pl,
 }
 
 /**
+ * pmccntr_sync
+ * @cpu: ARMCPU
+ *
+ * Syncronises the counter in the PMCCNTR. This must always be called twice,
+ * once before any action that might effect the timer and again afterwards.
+ * The fucntion is used to swap the state of the register if required.
+ * This only happens when not in user mode (!CONFIG_USER_ONLY)
+ */
+void pmccntr_sync(CPUARMState *env);
+
+/**
  * write_list_to_cpustate
  * @cpu: ARMCPU
  *
target-arm/helper.c +19 -0
index ce986ee..15169c4 100644
@@ -546,6 +546,25 @@ static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri)
     return CP_ACCESS_OK;
 }
 
+void pmccntr_sync(CPUARMState *env)
+{
+#ifndef CONFIG_USER_ONLY
+    uint64_t temp_ticks;
+
+    temp_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
+                          get_ticks_per_sec(), 1000000);
+
+    if (env->cp15.c9_pmcr & PMCRD) {
+        /* Increment once every 64 processor clock cycles */
+        temp_ticks /= 64;
+    }
+
+    if (CCNT_ENABLED(env)) {
+        env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
+    }
+#endif
+}
+
 #ifndef CONFIG_USER_ONLY
 static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                        uint64_t value)
Signature · 1 line
1.7.1
#9 lore raw

Re: [Qemu-devel] [PATCH v1 4/7] target-arm: Implement pmccntr_sync function

Quoted 10 lines · Alistair Francis

This is used to synchronise the PMCCNTR counter and swap its state between enabled and disabled if required. It must always be called twice, both before and after any logic that could change the state of the PMCCNTR counter.

target-arm/cpu.h +0 -0
index 31aa09c..0984eda 100644

target-arm/cpu.h static inline bool cp_access_ok(int current_pl, · line 1059

Quoted 9 lines
target-arm/cpu.h +6 -0
@@ -1051,6 +1051,17 @@ static inline bool cp_access_ok(int current_pl,
 }
 
 /**
+ * pmccntr_sync
+ * @cpu: ARMCPU
+ *
+ * Syncronises the counter in the PMCCNTR. This must always be called twice,
+ * once before any action that might effect the timer and again afterwards.
+ * The fucntion is used to swap the state of the register if required.

Nit: function

Signature · 3 lines
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.
#10 lore raw

Re: [Qemu-devel] [PATCH v1 4/7] target-arm: Implement pmccntr_sync function

Quoted 13 lines · Christopher Covington
Quoted 12 lines · Alistair Francis

This is used to synchronise the PMCCNTR counter and swap its state between enabled and disabled if required. It must always be called twice, both before and after any logic that could change the state of the PMCCNTR counter.

target-arm/cpu.h +0 -0
index 31aa09c..0984eda 100644
@@ -1051,6 +1051,17 @@ static inline bool cp_access_ok(int current_pl,
 }
 
Quoted 9 lines

target-arm/cpu.h static inline bool cp_access_ok(int current_pl, · line 1059

Quoted 7 lines
target-arm/cpu.h +6 -0
 /**
+ * pmccntr_sync
+ * @cpu: ARMCPU
+ *
+ * Syncronises the counter in the PMCCNTR. This must always be called twice,
+ * once before any action that might effect the timer and again afterwards.
+ * The fucntion is used to swap the state of the register if required.

Nit: function

Good catch, will fix

Quoted 6 lines

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation.

#11 lore raw

PATCH 5/7 target-arm: Remove old code and replace with new functions

In reply to Alistair Francis

Remove the old PMCCNTR code and replace it with calls to the new pmccntr_sync() function and the CCNT_ENABLED macro

---

 target-arm/helper.c |   27 ++++-----------------------
 1 files changed, 4 insertions(+), 23 deletions(-)
target-arm/helper.c +4 -23
index 15169c4..bbd4b23 100644
@@ -569,20 +569,7 @@ void pmccntr_sync(CPUARMState *env)
 static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                        uint64_t value)
 {
-    uint64_t temp_ticks;
-
-    temp_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
-                          get_ticks_per_sec(), 1000000);
-
-    if (env->cp15.c9_pmcr & PMCRE) {
-        /* If the counter is enabled */
-        if (env->cp15.c9_pmcr & PMCRD) {
-            /* Increment once every 64 processor clock cycles */
-            env->cp15.c15_ccnt = (temp_ticks/64) - env->cp15.c15_ccnt;
-        } else {
-            env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
-        }
-    }
+    pmccntr_sync(env);
 
     if (value & PMCRC) {
         /* The counter has been reset */
@@ -593,20 +580,14 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     env->cp15.c9_pmcr &= ~0x39;
     env->cp15.c9_pmcr |= (value & 0x39);
 
-    if (env->cp15.c9_pmcr & PMCRE) {
-        if (env->cp15.c9_pmcr & PMCRD) {
-            /* Increment once every 64 processor clock cycles */
-            temp_ticks /= 64;
-        }
-        env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
-    }
+    pmccntr_sync(env);
 }
 
 static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
     uint64_t total_ticks;
 
-    if (!(env->cp15.c9_pmcr & PMCRE)) {
+    if (!CCNT_ENABLED(env)) {
         /* Counter is disabled, do not change value */
         return env->cp15.c15_ccnt;
     }
@@ -626,7 +607,7 @@ static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     uint64_t total_ticks;
 
-    if (!(env->cp15.c9_pmcr & PMCRE)) {
+    if (!CCNT_ENABLED(env)) {
         /* Counter is disabled, set the absolute value */
         env->cp15.c15_ccnt = value;
         return;
Signature · 1 line
1.7.1
#12 lore raw

PATCH 6/7 target-arm: Implement pmccfiltr_write function

In reply to Alistair Francis

This is the function that is called when writing to the PMCCFILTR_EL0 register

---

 target-arm/helper.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)
target-arm/helper.c +9 -0
index bbd4b23..3e0a9a1 100644
@@ -624,6 +624,14 @@ static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 }
 #endif
 
+static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                            uint64_t value)
+{
+    pmccntr_sync(env);
+    env->cp15.pmccfiltr_el0 = value & 0x7E000000;
+    pmccntr_sync(env);
+}
+
 static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
                             uint64_t value)
 {
@@ -786,6 +794,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
 #endif
     { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7,
+      .writefn = pmccfiltr_write,
       .access = PL0_RW, .accessfn = pmreg_access,
       .state = ARM_CP_STATE_AA64,
       .resetvalue = 0,
Signature · 1 line
1.7.1
#13 lore raw

PATCH 7/7 target-arm: Call the pmccntr_sync function when swapping ELs

In reply to Alistair Francis

Call the new pmccntr_sync() function when there is a possibility of swapping ELs (I.E. when there is an exception)

---

 target-arm/helper-a64.c |    5 +++++
 target-arm/helper.c     |    7 +++++++
 target-arm/op_helper.c  |    6 ++++++
 3 files changed, 18 insertions(+), 0 deletions(-)
target-arm/helper-a64.c +5 -0
index 2b4ce6a..b61174f 100644
@@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
     target_ulong addr = env->cp15.vbar_el[1];
     int i;
 
+    pmccntr_sync(env);
+
     if (arm_current_pl(env) == 0) {
         if (env->aarch64) {
             addr += 0x400;
@@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
         addr += 0x100;
         break;
     default:
+        pmccntr_sync(env);
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
     }
 
@@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
 
     env->pc = addr;
     cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+    pmccntr_sync(env);
 }
target-arm/helper.c +7 -0
index 3e0a9a1..7c0a57f 100644
@@ -3417,6 +3417,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
 
     assert(!IS_M(env));
 
+    pmccntr_sync(env);
+
     arm_log_exception(cs->exception_index);
 
     /* TODO: Vectored interrupt controller.  */
@@ -3447,6 +3449,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
                   && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR) {
                 env->regs[0] = do_arm_semihosting(env);
                 qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n");
+                pmccntr_sync(env);
                 return;
             }
         }
@@ -3465,6 +3468,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
                 env->regs[15] += 2;
                 env->regs[0] = do_arm_semihosting(env);
                 qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n");
+                pmccntr_sync(env);
                 return;
             }
         }
@@ -3508,6 +3512,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
         offset = 4;
         break;
     default:
+        pmccntr_sync(env);
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
         return; /* Never happens.  Keep compiler happy.  */
     }
@@ -3540,6 +3545,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
     env->regs[14] = env->regs[15] + offset;
     env->regs[15] = addr;
     cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+    pmccntr_sync(env);
 }
 
 /* Check section/page access permissions.
target-arm/op_helper.c +6 -0
index 9c1ef52..07ab30b 100644
@@ -376,6 +376,8 @@ void HELPER(exception_return)(CPUARMState *env)
     uint32_t spsr = env->banked_spsr[spsr_idx];
     int new_el, i;
 
+    pmccntr_sync(env);
+
     if (env->pstate & PSTATE_SP) {
         env->sp_el[cur_el] = env->xregs[31];
     } else {
@@ -418,6 +420,8 @@ void HELPER(exception_return)(CPUARMState *env)
         env->pc = env->elr_el[cur_el];
     }
 
+    pmccntr_sync(env);
+
     return;
 
 illegal_return:
@@ -433,6 +437,8 @@ illegal_return:
     spsr &= PSTATE_NZCV | PSTATE_DAIF;
     spsr |= pstate_read(env) & ~(PSTATE_NZCV | PSTATE_DAIF);
     pstate_write(env, spsr);
+
+    pmccntr_sync(env);
 }
 
 /* ??? Flag setting arithmetic is awkward because we need to do comparisons.
Signature · 1 line
1.7.1
#14 lore raw

Re: [Qemu-devel] [PATCH v1 7/7] target-arm: Call the pmccntr_sync function when swapping ELs

Quoted 33 lines · Alistair Francis

Call the new pmccntr_sync() function when there is a possibility of swapping ELs (I.E. when there is an exception)

---

 target-arm/helper-a64.c |    5 +++++
 target-arm/helper.c     |    7 +++++++
 target-arm/op_helper.c  |    6 ++++++
 3 files changed, 18 insertions(+), 0 deletions(-)
target-arm/helper-a64.c +3 -0
index 2b4ce6a..b61174f 100644
@@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
     target_ulong addr = env->cp15.vbar_el[1];
     int i;
 
+    pmccntr_sync(env);
+
     if (arm_current_pl(env) == 0) {
         if (env->aarch64) {
             addr += 0x400;
@@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
         addr += 0x100;
         break;
     default:
+        pmccntr_sync(env);
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
     }
 

target-arm/helper-a64.c void aarch64_cpu_do_interrupt(CPUState *cs) · line 519

Quoted 6 lines
target-arm/helper-a64.c +2 -0
@@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
 
     env->pc = addr;
     cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+    pmccntr_sync(env);
 }

The double calls seem unwieldy. I think it could be made into a single function call if there was access, perhaps as a second parameter or maybe as a static variable, to both the previous and current state so the function could tell whether there is no transition, enable->disable, or disable->enable.

Christopher

Signature · 3 lines
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.
#15 lore raw

Re: [Qemu-devel] [PATCH v1 7/7] target-arm: Call the pmccntr_sync function when swapping ELs

Quoted 35 lines · Christopher Covington
Quoted 34 lines · Alistair Francis

Call the new pmccntr_sync() function when there is a possibility of swapping ELs (I.E. when there is an exception)

---

 target-arm/helper-a64.c |    5 +++++
 target-arm/helper.c     |    7 +++++++
 target-arm/op_helper.c  |    6 ++++++
 3 files changed, 18 insertions(+), 0 deletions(-)
target-arm/helper-a64.c +3 -0
index 2b4ce6a..b61174f 100644
@@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
     target_ulong addr = env->cp15.vbar_el[1];
     int i;
 
+    pmccntr_sync(env);
+
     if (arm_current_pl(env) == 0) {
         if (env->aarch64) {
             addr += 0x400;
@@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
         addr += 0x100;
         break;
     default:
+        pmccntr_sync(env);
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
     }
 
@@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
 
Quoted 11 lines

target-arm/helper-a64.c void aarch64_cpu_do_interrupt(CPUState *cs) · line 519

Quoted 5 lines
target-arm/helper-a64.c +2 -0
     env->pc = addr;
     cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+    pmccntr_sync(env);
 }

The double calls seem unwieldy. I think it could be made into a single function call if there was access, perhaps as a second parameter or maybe as a static variable, to both the previous and current state so the function could tell whether there is no transition, enable->disable, or disable->enable.

The problem with a parameter is that the state of the enabled register needs to be saved at the start of any code that will enable/disable the register. So it ends up being just as messy.

Static variables won't work if there are multiple CPUs. I guess an array of statics
could work, but I don't like that method

I feel that just calling the function twice ends up being neat and works pretty well

Quoted 7 lines

Christopher

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation.

#16 lore raw

Re: [Qemu-devel] [PATCH v1 7/7] target-arm: Call the pmccntr_sync function when swapping ELs

Quoted 22 lines · Alistair Francis
Quoted 20 lines · Christopher Covington
Quoted 19 lines · Alistair Francis

Call the new pmccntr_sync() function when there is a possibility of swapping ELs (I.E. when there is an exception)

---

 target-arm/helper-a64.c |    5 +++++
 target-arm/helper.c     |    7 +++++++
 target-arm/op_helper.c  |    6 ++++++
 3 files changed, 18 insertions(+), 0 deletions(-)
target-arm/helper-a64.c +0 -0
index 2b4ce6a..b61174f 100644
@@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
     target_ulong addr = env->cp15.vbar_el[1];
     int i;
 
Quoted 11 lines
Quoted 11 lines

target-arm/helper-a64.c void aarch64_cpu_do_interrupt(CPUState *cs) · line 490

Quoted 11 lines
target-arm/helper-a64.c +3 -0
+    pmccntr_sync(env);
+
     if (arm_current_pl(env) == 0) {
         if (env->aarch64) {
             addr += 0x400;
@@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
         addr += 0x100;
         break;
     default:
+        pmccntr_sync(env);
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);

Not sure you need this, there is not much point in causing your side effects right before an assertion (via cpu_abort).

Quoted 20 lines
Quoted 4 lines
Quoted 4 lines
target-arm/helper-a64.c +0 -0
     }
 
@@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
 
Quoted 11 lines

target-arm/helper-a64.c void aarch64_cpu_do_interrupt(CPUState *cs) · line 519

Quoted 5 lines
target-arm/helper-a64.c +2 -0
     env->pc = addr;
     cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+    pmccntr_sync(env);
 }

The double calls seem unwieldy. I think it could be made into a single function call if there was access, perhaps as a second parameter or maybe as a static variable, to both the previous and current state so the function could tell whether there is no transition, enable->disable, or disable->enable.

The problem with a parameter is that the state of the enabled register needs to be saved at the start of any code that will enable/disable the register. So it ends up being just as messy.

Quoted 7 lines

Static variables won't work if there are multiple CPUs. I guess an array of statics
could work, but I don't like that method

I feel that just calling the function twice ends up being neat and works pretty well

Theres a third option. Create a new function that explicitly changes EL:

arm_change_el(int new) {
    sync();
    env->el = new;
    sync();
}

And update the interrupt path functions to use it instead of direct env manipulation.

The advantage of this, is others can also add el switching side effects in one place. I doubt this is the last time we will want to trap EL changes for system level side effects.

Regards,
Peter

Quoted 8 lines
Quoted 7 lines

Christopher

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation.

#17 lore raw

Re: [Qemu-devel] [PATCH v1 7/7] target-arm: Call the pmccntr_sync function when swapping ELs

Quoted 24 lines · Peter Crosthwaite
Quoted 22 lines · Alistair Francis
Quoted 20 lines · Christopher Covington
Quoted 19 lines · Alistair Francis

Call the new pmccntr_sync() function when there is a possibility of swapping ELs (I.E. when there is an exception)

---

 target-arm/helper-a64.c |    5 +++++
 target-arm/helper.c     |    7 +++++++
 target-arm/op_helper.c  |    6 ++++++
 3 files changed, 18 insertions(+), 0 deletions(-)
target-arm/helper-a64.c +0 -0
index 2b4ce6a..b61174f 100644
@@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
     target_ulong addr = env->cp15.vbar_el[1];
     int i;
 
Quoted 14 lines
Quoted 11 lines
Quoted 11 lines

target-arm/helper-a64.c void aarch64_cpu_do_interrupt(CPUState *cs) · line 490

Quoted 11 lines
target-arm/helper-a64.c +3 -0
+    pmccntr_sync(env);
+
     if (arm_current_pl(env) == 0) {
         if (env->aarch64) {
             addr += 0x400;
@@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
         addr += 0x100;
         break;
     default:
+        pmccntr_sync(env);
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);

Not sure you need this, there is not much point in causing your side effects right before an assertion (via cpu_abort).

I figured it doesn't really matter. Plus it could make debugging easier if someone
was monitoring the registers?

Quoted 37 lines
Quoted 21 lines
Quoted 5 lines
Quoted 5 lines
target-arm/helper-a64.c +0 -0
     }
 
@@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
 
Quoted 11 lines

target-arm/helper-a64.c void aarch64_cpu_do_interrupt(CPUState *cs) · line 519

Quoted 5 lines
target-arm/helper-a64.c +2 -0
     env->pc = addr;
     cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+    pmccntr_sync(env);
 }

The double calls seem unwieldy. I think it could be made into a single function call if there was access, perhaps as a second parameter or maybe as a static variable, to both the previous and current state so the function could tell whether there is no transition, enable->disable, or disable->enable.

The problem with a parameter is that the state of the enabled register needs to be saved at the start of any code that will enable/disable the register. So it ends up being just as messy.

Quoted 7 lines

Static variables won't work if there are multiple CPUs. I guess an array of statics
could work, but I don't like that method

I feel that just calling the function twice ends up being neat and works pretty well

Theres a third option. Create a new function that explicitly changes EL:

arm_change_el(int new) {
    sync();
    env->el = new;
    sync();
}
Quoted 6 lines

And update the interrupt path functions to use it instead of direct env manipulation.

The advantage of this, is others can also add el switching side effects in one place. I doubt this is the last time we will want to trap EL changes for system level side effects.

That doesn't really fix the problem, because the sync function will still need to exist as there are other places where the counter can be enabled/disabled. So it just moves it to a different place. I also feel that that's pretty much what the
*cpu_do_interrupt() functions already do

Could that also interfere with the work that is being done to support EL2 and 3?

Quoted 13 lines

Regards,
Peter

Quoted 8 lines
Quoted 7 lines

Christopher

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation.

#18 lore raw

Re: [Qemu-devel] [PATCH v1 7/7] target-arm: Call the pmccntr_sync function when swapping ELs

In reply to Alistair Francis
Quoted 25 lines · Alistair Francis

Call the new pmccntr_sync() function when there is a possibility of swapping ELs (I.E. when there is an exception)

---

 target-arm/helper-a64.c |    5 +++++
 target-arm/helper.c     |    7 +++++++
 target-arm/op_helper.c  |    6 ++++++
 3 files changed, 18 insertions(+), 0 deletions(-)
target-arm/helper-a64.c +2 -0
index 2b4ce6a..b61174f 100644
@@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
     target_ulong addr = env->cp15.vbar_el[1];
     int i;
 
+    pmccntr_sync(env);
+
     if (arm_current_pl(env) == 0) {
         if (env->aarch64) {
             addr += 0x400;

target-arm/helper-a64.c void aarch64_cpu_do_interrupt(CPUState *cs) · line 489

Quoted 4 lines
target-arm/helper-a64.c +1 -0
@@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
         addr += 0x100;
         break;
     default:
+        pmccntr_sync(env);

Can you localise just the one sync->sync pair around the el change logic itself to avoid having to catch all the return paths?

Regards,
Peter

Quoted 91 lines
target-arm/helper-a64.c +2 -0
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
     }
 
@@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
 
     env->pc = addr;
     cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+    pmccntr_sync(env);
 }
target-arm/helper.c +7 -0
index 3e0a9a1..7c0a57f 100644
@@ -3417,6 +3417,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
 
     assert(!IS_M(env));
 
+    pmccntr_sync(env);
+
     arm_log_exception(cs->exception_index);
 
     /* TODO: Vectored interrupt controller.  */
@@ -3447,6 +3449,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
                   && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR) {
                 env->regs[0] = do_arm_semihosting(env);
                 qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n");
+                pmccntr_sync(env);
                 return;
             }
         }
@@ -3465,6 +3468,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
                 env->regs[15] += 2;
                 env->regs[0] = do_arm_semihosting(env);
                 qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n");
+                pmccntr_sync(env);
                 return;
             }
         }
@@ -3508,6 +3512,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
         offset = 4;
         break;
     default:
+        pmccntr_sync(env);
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
         return; /* Never happens.  Keep compiler happy.  */
     }
@@ -3540,6 +3545,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
     env->regs[14] = env->regs[15] + offset;
     env->regs[15] = addr;
     cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+    pmccntr_sync(env);
 }
 
 /* Check section/page access permissions.
target-arm/op_helper.c +6 -1
index 9c1ef52..07ab30b 100644
@@ -376,6 +376,8 @@ void HELPER(exception_return)(CPUARMState *env)
     uint32_t spsr = env->banked_spsr[spsr_idx];
     int new_el, i;
 
+    pmccntr_sync(env);
+
     if (env->pstate & PSTATE_SP) {
         env->sp_el[cur_el] = env->xregs[31];
     } else {
@@ -418,6 +420,8 @@ void HELPER(exception_return)(CPUARMState *env)
         env->pc = env->elr_el[cur_el];
     }
 
+    pmccntr_sync(env);
+
     return;
 
 illegal_return:
@@ -433,6 +437,8 @@ illegal_return:
     spsr &= PSTATE_NZCV | PSTATE_DAIF;
     spsr |= pstate_read(env) & ~(PSTATE_NZCV | PSTATE_DAIF);
     pstate_write(env, spsr);
+
+    pmccntr_sync(env);
 }
 
 /* ??? Flag setting arithmetic is awkward because we need to do comparisons.
--

1.7.1

#19 lore raw

Re: [Qemu-devel] [PATCH v1 7/7] target-arm: Call the pmccntr_sync function when swapping ELs

Quoted 21 lines · Peter Crosthwaite
Quoted 19 lines · Alistair Francis

Call the new pmccntr_sync() function when there is a possibility of swapping ELs (I.E. when there is an exception)

---

 target-arm/helper-a64.c |    5 +++++
 target-arm/helper.c     |    7 +++++++
 target-arm/op_helper.c  |    6 ++++++
 3 files changed, 18 insertions(+), 0 deletions(-)
target-arm/helper-a64.c +0 -0
index 2b4ce6a..b61174f 100644
@@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
     target_ulong addr = env->cp15.vbar_el[1];
     int i;
 
Quoted 14 lines

target-arm/helper-a64.c void aarch64_cpu_do_interrupt(CPUState *cs) · line 489

Quoted 10 lines
target-arm/helper-a64.c +3 -0
+    pmccntr_sync(env);
+
     if (arm_current_pl(env) == 0) {
         if (env->aarch64) {
             addr += 0x400;
@@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
         addr += 0x100;
         break;
     default:
+        pmccntr_sync(env);

Can you localise just the one sync->sync pair around the el change logic itself to avoid having to catch all the return paths?

Doh,

reviewed the wrong version.

Sry for the noise.

Regards,
Peter

Quoted 94 lines

Regards,
Peter

Quoted 91 lines
target-arm/helper-a64.c +2 -0
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
     }
 
@@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
 
     env->pc = addr;
     cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+    pmccntr_sync(env);
 }
target-arm/helper.c +7 -0
index 3e0a9a1..7c0a57f 100644
@@ -3417,6 +3417,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
 
     assert(!IS_M(env));
 
+    pmccntr_sync(env);
+
     arm_log_exception(cs->exception_index);
 
     /* TODO: Vectored interrupt controller.  */
@@ -3447,6 +3449,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
                   && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR) {
                 env->regs[0] = do_arm_semihosting(env);
                 qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n");
+                pmccntr_sync(env);
                 return;
             }
         }
@@ -3465,6 +3468,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
                 env->regs[15] += 2;
                 env->regs[0] = do_arm_semihosting(env);
                 qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n");
+                pmccntr_sync(env);
                 return;
             }
         }
@@ -3508,6 +3512,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
         offset = 4;
         break;
     default:
+        pmccntr_sync(env);
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
         return; /* Never happens.  Keep compiler happy.  */
     }
@@ -3540,6 +3545,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
     env->regs[14] = env->regs[15] + offset;
     env->regs[15] = addr;
     cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+    pmccntr_sync(env);
 }
 
 /* Check section/page access permissions.
target-arm/op_helper.c +6 -1
index 9c1ef52..07ab30b 100644
@@ -376,6 +376,8 @@ void HELPER(exception_return)(CPUARMState *env)
     uint32_t spsr = env->banked_spsr[spsr_idx];
     int new_el, i;
 
+    pmccntr_sync(env);
+
     if (env->pstate & PSTATE_SP) {
         env->sp_el[cur_el] = env->xregs[31];
     } else {
@@ -418,6 +420,8 @@ void HELPER(exception_return)(CPUARMState *env)
         env->pc = env->elr_el[cur_el];
     }
 
+    pmccntr_sync(env);
+
     return;
 
 illegal_return:
@@ -433,6 +437,8 @@ illegal_return:
     spsr &= PSTATE_NZCV | PSTATE_DAIF;
     spsr |= pstate_read(env) & ~(PSTATE_NZCV | PSTATE_DAIF);
     pstate_write(env, spsr);
+
+    pmccntr_sync(env);
 }
 
 /* ??? Flag setting arithmetic is awkward because we need to do comparisons.
--

1.7.1

#20 lore raw
In reply to Alistair Francis
Quoted 5 lines · Alistair Francis

This patch series continues on from my original PMCCNTR patch work to extend the counter to be 64-bit and support for the PMCCFILTR_EL0 register which allows the counter to be disabled based on the current EL

Quoted 8 lines

Alistair Francis (7):
target-arm: Make the ARM PMCCNTR register 64-bit
target-arm: Implement PMCCNTR_EL0 and related registers
target-arm: Add helper macros and defines for CCNT register
target-arm: Implement pmccntr_sync function
target-arm: Remove old code and replace with new functions
target-arm: Implement pmccfiltr_write function
target-arm: Call the pmccntr_sync function when swapping ELs

Hi. This is still on my must-review queue but I think it is too late for 2.1.

thanks
-- PMM