[Qemu-devel] [PATCH v1 0/7] target-arm: Extend PMCCNTR for ARMv8

[Qemu-devel] [PATCH v1 0/7] target-arm: Extend PMCCNTR for ARMv8

[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

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(-)

[Qemu-devel] [PATCH v1 1/7] target-arm: Make the ARM PMCCNTR register 64-bit

[Alistair Francis]

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

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 target-arm/cpu.h    |    2 +-
 target-arm/helper.c |   19 +++++++++----------
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 369d472..cd1c7b6 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -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 {
diff --git a/target-arm/helper.c b/target-arm/helper.c
index ed4d2bb..ac10564 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -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 */
-- 
1.7.1

[Qemu-devel] [PATCH v1 2/7] target-arm: Implement PMCCNTR_EL0 and related registers

[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.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 target-arm/cpu.h    |    1 +
 target-arm/helper.c |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index cd1c7b6..6a2efd8 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -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 {
diff --git a/target-arm/helper.c b/target-arm/helper.c
index ac10564..ce986ee 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -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,
-- 
1.7.1

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

[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.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 target-arm/cpu.h |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 6a2efd8..31aa09c 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -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];
-- 
1.7.1

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

[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.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
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(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 31aa09c..0984eda 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -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
  *
diff --git a/target-arm/helper.c b/target-arm/helper.c
index ce986ee..15169c4 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -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)
-- 
1.7.1

[Qemu-devel] [PATCH v1 5/7] target-arm: Remove old code and replace with new functions

[Alistair Francis]

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

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 target-arm/helper.c |   27 ++++-----------------------
 1 files changed, 4 insertions(+), 23 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 15169c4..bbd4b23 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -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;
-- 
1.7.1

[Qemu-devel] [PATCH v1 6/7] target-arm: Implement pmccfiltr_write function

[Alistair Francis]

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

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 target-arm/helper.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index bbd4b23..3e0a9a1 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -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,
-- 
1.7.1

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

[Alistair Francis]

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

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 target-arm/helper-a64.c |    5 +++++
 target-arm/helper.c     |    7 +++++++
 target-arm/op_helper.c  |    6 ++++++
 3 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index 2b4ce6a..b61174f 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -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);
 }
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 3e0a9a1..7c0a57f 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -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.
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 9c1ef52..07ab30b 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -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

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

[Christopher Covington]

Hi Alistair,

On 06/23/2014 09:12 PM, Alistair Francis wrote:
> Include a helper function to determine if the CCNT counter
> is enabled as well as the constants used to mask the pmccfiltr_el0
> register.
> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> 
>  target-arm/cpu.h |   19 +++++++++++++++++++
>  1 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 6a2efd8..31aa09c 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -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.

> +    /* 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

> +     * 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];
> 

Christopher

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

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

[Christopher Covington]

On 06/23/2014 09:12 PM, Alistair Francis wrote:
> 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.

> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 31aa09c..0984eda 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -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

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

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

[Christopher Covington]

On 06/23/2014 09:12 PM, Alistair Francis wrote:
> Call the new pmccntr_sync() function when there is a possibility
> of swapping ELs (I.E. when there is an exception)
> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> 
>  target-arm/helper-a64.c |    5 +++++
>  target-arm/helper.c     |    7 +++++++
>  target-arm/op_helper.c  |    6 ++++++
>  3 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index 2b4ce6a..b61174f 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -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);
>  }

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

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

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

[Peter Maydell]

On 24 June 2014 02:12, Alistair Francis <alistair.francis@xilinx.com> wrote:
> +    /* 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

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

[Alistair Francis]

On Wed, Jun 25, 2014 at 1:35 AM, Christopher Covington
<cov@codeaurora.org> wrote:
> On 06/23/2014 09:12 PM, Alistair Francis wrote:
>> 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.
>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 31aa09c..0984eda 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -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

Good catch, will fix

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

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

[Alistair Francis]

On Wed, Jun 25, 2014 at 1:55 AM, Christopher Covington
<cov@codeaurora.org> wrote:
> On 06/23/2014 09:12 PM, Alistair Francis wrote:
>> Call the new pmccntr_sync() function when there is a possibility
>> of swapping ELs (I.E. when there is an exception)
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>>  target-arm/helper-a64.c |    5 +++++
>>  target-arm/helper.c     |    7 +++++++
>>  target-arm/op_helper.c  |    6 ++++++
>>  3 files changed, 18 insertions(+), 0 deletions(-)
>>
>> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
>> index 2b4ce6a..b61174f 100644
>> --- a/target-arm/helper-a64.c
>> +++ b/target-arm/helper-a64.c
>> @@ -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);
>>  }
>
> 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

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

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

[Alistair Francis]

On Wed, Jun 25, 2014 at 1:31 AM, Christopher Covington
<cov@codeaurora.org> wrote:
> Hi Alistair,
>
> On 06/23/2014 09:12 PM, Alistair Francis wrote:
>> Include a helper function to determine if the CCNT counter
>> is enabled as well as the constants used to mask the pmccfiltr_el0
>> register.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>>  target-arm/cpu.h |   19 +++++++++++++++++++
>>  1 files changed, 19 insertions(+), 0 deletions(-)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 6a2efd8..31aa09c 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -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.

Yep, can do

>
>> +    /* 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

>
>> +     * 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];
>>
>
> Christopher
>
> --
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by the Linux Foundation.
>

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

[Alistair Francis]

On Wed, Jun 25, 2014 at 1:56 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 24 June 2014 02:12, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> +    /* 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?

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)

>
> thanks
> -- PMM
>

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

[Peter Crosthwaite]

On Wed, Jun 25, 2014 at 8:39 AM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Wed, Jun 25, 2014 at 1:55 AM, Christopher Covington
> <cov@codeaurora.org> wrote:
>> On 06/23/2014 09:12 PM, Alistair Francis wrote:
>>> Call the new pmccntr_sync() function when there is a possibility
>>> of swapping ELs (I.E. when there is an exception)
>>>
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> ---
>>>
>>>  target-arm/helper-a64.c |    5 +++++
>>>  target-arm/helper.c     |    7 +++++++
>>>  target-arm/op_helper.c  |    6 ++++++
>>>  3 files changed, 18 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
>>> index 2b4ce6a..b61174f 100644
>>> --- a/target-arm/helper-a64.c
>>> +++ b/target-arm/helper-a64.c
>>> @@ -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);

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

>>>      }
>>>
>>> @@ -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.
>>
>
> 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
>

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

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

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

[Alistair Francis]

On Wed, Jun 25, 2014 at 9:07 AM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Wed, Jun 25, 2014 at 8:39 AM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Wed, Jun 25, 2014 at 1:55 AM, Christopher Covington
>> <cov@codeaurora.org> wrote:
>>> On 06/23/2014 09:12 PM, Alistair Francis wrote:
>>>> Call the new pmccntr_sync() function when there is a possibility
>>>> of swapping ELs (I.E. when there is an exception)
>>>>
>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>> ---
>>>>
>>>>  target-arm/helper-a64.c |    5 +++++
>>>>  target-arm/helper.c     |    7 +++++++
>>>>  target-arm/op_helper.c  |    6 ++++++
>>>>  3 files changed, 18 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
>>>> index 2b4ce6a..b61174f 100644
>>>> --- a/target-arm/helper-a64.c
>>>> +++ b/target-arm/helper-a64.c
>>>> @@ -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);
>
> 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?

>
>>>>      }
>>>>
>>>> @@ -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.
>>>
>>
>> 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
>>
>
> 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.

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?

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

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

[Peter Crosthwaite]

On Tue, Jun 24, 2014 at 11:12 AM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> Call the new pmccntr_sync() function when there is a possibility
> of swapping ELs (I.E. when there is an exception)
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
>  target-arm/helper-a64.c |    5 +++++
>  target-arm/helper.c     |    7 +++++++
>  target-arm/op_helper.c  |    6 ++++++
>  3 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index 2b4ce6a..b61174f 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -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);

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

>          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);
>  }
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 3e0a9a1..7c0a57f 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -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.
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 9c1ef52..07ab30b 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -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
>
>

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

[Peter Crosthwaite]

On Thu, Jun 26, 2014 at 9:40 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Tue, Jun 24, 2014 at 11:12 AM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> Call the new pmccntr_sync() function when there is a possibility
>> of swapping ELs (I.E. when there is an exception)
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>>  target-arm/helper-a64.c |    5 +++++
>>  target-arm/helper.c     |    7 +++++++
>>  target-arm/op_helper.c  |    6 ++++++
>>  3 files changed, 18 insertions(+), 0 deletions(-)
>>
>> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
>> index 2b4ce6a..b61174f 100644
>> --- a/target-arm/helper-a64.c
>> +++ b/target-arm/helper-a64.c
>> @@ -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);
>
> 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

> Regards,
> Peter
>
>>          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);
>>  }
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 3e0a9a1..7c0a57f 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -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.
>> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
>> index 9c1ef52..07ab30b 100644
>> --- a/target-arm/op_helper.c
>> +++ b/target-arm/op_helper.c
>> @@ -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
>>
>>

Re: [Qemu-devel] [PATCH v1 0/7] target-arm: Extend PMCCNTR for ARMv8

[Peter Maydell]

On 24 June 2014 02:11, Alistair Francis <alistair.francis@xilinx.com> wrote:
> 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

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

thanks
-- PMM

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

[Alistair Francis]

On Thu, Jun 26, 2014 at 10:37 AM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Wed, Jun 25, 2014 at 9:07 AM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> On Wed, Jun 25, 2014 at 8:39 AM, Alistair Francis
>> <alistair.francis@xilinx.com> wrote:
>>> On Wed, Jun 25, 2014 at 1:55 AM, Christopher Covington
>>> <cov@codeaurora.org> wrote:
>>>> On 06/23/2014 09:12 PM, Alistair Francis wrote:
>>>>> Call the new pmccntr_sync() function when there is a possibility
>>>>> of swapping ELs (I.E. when there is an exception)
>>>>>
>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>>> ---
>>>>>
>>>>>  target-arm/helper-a64.c |    5 +++++
>>>>>  target-arm/helper.c     |    7 +++++++
>>>>>  target-arm/op_helper.c  |    6 ++++++
>>>>>  3 files changed, 18 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
>>>>> index 2b4ce6a..b61174f 100644
>>>>> --- a/target-arm/helper-a64.c
>>>>> +++ b/target-arm/helper-a64.c
>>>>> @@ -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);
>>
>> 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?
>
>>
>>>>>      }
>>>>>
>>>>> @@ -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.
>>>>
>>>
>>> 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
>>>
>>
>> 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.
>
> 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?
>

So I sent a second version, still calling the function twice as in this version.
Still more then happy to discuss changes to stop calling the function
twice if it is an issue for anyone. Although I think it works well

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