[Qemu-devel] [PATCH v4 15/21] target/arm: Add array for supported PMU events, generate PMCEID[01]

[Aaron Lindsay]

This commit doesn't add any supported events, but provides the framework
for adding them. We store the pm_event structs in a simple array, and
provide the mapping from the event numbers to array indexes in the
supported_event_map array. Because the value of PMCEID[01] depends upon
which events are supported at runtime, generate it dynamically.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/cpu.c| 20 +---
 target/arm/cpu.h| 10 ++
 target/arm/cpu64.c  |  2 --
 target/arm/helper.c | 37 +
 4 files changed, 60 insertions(+), 9 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 9d27ffc..22063ca 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -897,9 +897,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
**errp)
 if (!cpu->has_pmu) {
 unset_feature(env, ARM_FEATURE_PMU);
 cpu->id_aa64dfr0 &= ~0xf00;
-} else if (!kvm_enabled()) {
-arm_register_pre_el_change_hook(cpu, _pre_el_change, 0);
-arm_register_el_change_hook(cpu, _post_el_change, 0);
+}
+if (arm_feature(env, ARM_FEATURE_PMU)) {
+uint64_t pmceid = get_pmceid(>env);
+cpu->pmceid0 = pmceid & 0x;
+cpu->pmceid1 = (pmceid >> 32) & 0x;
+
+if (!kvm_enabled()) {
+arm_register_pre_el_change_hook(cpu, _pre_el_change, 0);
+arm_register_el_change_hook(cpu, _post_el_change, 0);
+}
+} else {
+cpu->pmceid0 = 0x;
+cpu->pmceid1 = 0x;
 }
 
 if (!arm_feature(env, ARM_FEATURE_EL2)) {
@@ -1511,8 +1521,6 @@ static void cortex_a7_initfn(Object *obj)
 cpu->id_pfr0 = 0x1131;
 cpu->id_pfr1 = 0x00011011;
 cpu->id_dfr0 = 0x02010555;
-cpu->pmceid0 = 0x;
-cpu->pmceid1 = 0x;
 cpu->id_afr0 = 0x;
 cpu->id_mmfr0 = 0x10101105;
 cpu->id_mmfr1 = 0x4000;
@@ -1557,8 +1565,6 @@ static void cortex_a15_initfn(Object *obj)
 cpu->id_pfr0 = 0x1131;
 cpu->id_pfr1 = 0x00011011;
 cpu->id_dfr0 = 0x02010555;
-cpu->pmceid0 = 0x000;
-cpu->pmceid1 = 0x;
 cpu->id_afr0 = 0x;
 cpu->id_mmfr0 = 0x10201105;
 cpu->id_mmfr1 = 0x2000;
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 132e08d..f058f5c 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -935,6 +935,16 @@ void pmu_op_finish(CPUARMState *env);
 void pmu_pre_el_change(ARMCPU *cpu, void *ignored);
 void pmu_post_el_change(ARMCPU *cpu, void *ignored);
 
+/*
+ * get_pmceid
+ * @env: CPUARMState
+ *
+ * Return the PMCEID[01] register values corresponding to the counters which
+ * are supported given the current configuration (0 is low 32, 1 is high 32
+ * bits)
+ */
+uint64_t get_pmceid(CPUARMState *env);
+
 /* SCTLR bit meanings. Several bits have been reused in newer
  * versions of the architecture; in that case we define constants
  * for both old and new bit meanings. Code which tests against those
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 991d764..7da0ea4 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -141,8 +141,6 @@ static void aarch64_a57_initfn(Object *obj)
 cpu->id_isar5 = 0x00011121;
 cpu->id_aa64pfr0 = 0x;
 cpu->id_aa64dfr0 = 0x10305106;
-cpu->pmceid0 = 0x;
-cpu->pmceid1 = 0x;
 cpu->id_aa64isar0 = 0x00011120;
 cpu->id_aa64mmfr0 = 0x1124;
 cpu->dbgdidr = 0x3516d000;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 572709e..7a715a6 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -933,6 +933,43 @@ static inline uint64_t pmu_counter_mask(CPUARMState *env)
   return (1 << 31) | ((1 << pmu_num_counters(env)) - 1);
 }
 
+typedef struct pm_event {
+uint16_t number; /* PMEVTYPER.evtCount is 10 bits wide */
+/* If the event is supported on this CPU (used to generate PMCEID[01]) */
+bool (*supported)(CPUARMState *);
+/* Retrieve the current count of the underlying event. The programmed
+ * counters hold a difference from the return value from this function */
+uint64_t (*get_count)(CPUARMState *);
+} pm_event;
+
+#define SUPPORTED_EVENT_SENTINEL UINT16_MAX
+static const pm_event pm_events[] = {
+{ .number = SUPPORTED_EVENT_SENTINEL }
+};
+static uint16_t supported_event_map[0x3f];
+
+/*
+ * Called upon initialization to build PMCEID0 (low 32 bits) and PMCEID1 (high
+ * 32). We also use it to build a map of ARM event numbers to indices in
+ * our pm_events array.
+ */
+uint64_t get_pmceid(CPUARMState *env)
+{
+uint64_t pmceid = 0;
+unsigned int i = 0;
+while (pm_events[i].number != SUPPORTED_EVENT_SENTINEL) {
+const pm_event *cnt = _events[i];
+if (cnt->number < 0x3f && cnt->supported(env)) {
+ 

[Qemu-devel] [PATCH v4 06/21] target/arm: Support multiple EL change hooks

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/cpu.c   | 21 -
 target/arm/cpu.h   | 20 ++--
 target/arm/internals.h |  7 ---
 3 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 022d8c5..1f689f6 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -55,13 +55,15 @@ static bool arm_cpu_has_work(CPUState *cs)
  | CPU_INTERRUPT_EXITTB);
 }
 
-void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook,
+void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
  void *opaque)
 {
-/* We currently only support registering a single hook function */
-assert(!cpu->el_change_hook);
-cpu->el_change_hook = hook;
-cpu->el_change_hook_opaque = opaque;
+ARMELChangeHook *entry = g_new0(ARMELChangeHook, 1);
+
+entry->hook = hook;
+entry->opaque = opaque;
+
+QLIST_INSERT_HEAD(>el_change_hooks, entry, node);
 }
 
 static void cp_reg_reset(gpointer key, gpointer value, gpointer opaque)
@@ -552,6 +554,8 @@ static void arm_cpu_initfn(Object *obj)
 cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal,
  g_free, g_free);
 
+QLIST_INIT(>el_change_hooks);
+
 #ifndef CONFIG_USER_ONLY
 /* Our inbound IRQ and FIQ lines */
 if (kvm_enabled()) {
@@ -713,7 +717,14 @@ static void arm_cpu_post_init(Object *obj)
 static void arm_cpu_finalizefn(Object *obj)
 {
 ARMCPU *cpu = ARM_CPU(obj);
+ARMELChangeHook *hook, *next;
+
 g_hash_table_destroy(cpu->cp_regs);
+
+QLIST_FOREACH_SAFE(hook, >el_change_hooks, node, next) {
+QLIST_REMOVE(hook, node);
+g_free(hook);
+}
 }
 
 static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index ff349f5..50d129b 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -642,12 +642,17 @@ typedef struct CPUARMState {
 } CPUARMState;
 
 /**
- * ARMELChangeHook:
+ * ARMELChangeHookFn:
  * type of a function which can be registered via arm_register_el_change_hook()
  * to get callbacks when the CPU changes its exception level or mode.
  */
-typedef void ARMELChangeHook(ARMCPU *cpu, void *opaque);
-
+typedef void ARMELChangeHookFn(ARMCPU *cpu, void *opaque);
+typedef struct ARMELChangeHook ARMELChangeHook;
+struct ARMELChangeHook {
+ARMELChangeHookFn *hook;
+void *opaque;
+QLIST_ENTRY(ARMELChangeHook) node;
+};
 
 /* These values map onto the return values for
  * QEMU_PSCI_0_2_FN_AFFINITY_INFO */
@@ -836,8 +841,7 @@ struct ARMCPU {
  */
 bool cfgend;
 
-ARMELChangeHook *el_change_hook;
-void *el_change_hook_opaque;
+QLIST_HEAD(, ARMELChangeHook) el_change_hooks;
 
 int32_t node_id; /* NUMA node this CPU belongs to */
 
@@ -2906,12 +2910,8 @@ static inline AddressSpace *arm_addressspace(CPUState 
*cs, MemTxAttrs attrs)
  * CPU changes exception level or mode. The hook function will be
  * passed a pointer to the ARMCPU and the opaque data pointer passed
  * to this function when the hook was registered.
- *
- * Note that we currently only support registering a single hook function,
- * and will assert if this function is called twice.
- * This facility is intended for the use of the GICv3 emulation.
  */
-void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook,
+void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
  void *opaque);
 
 /**
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 8ce944b..6358c2a 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -727,11 +727,12 @@ void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr 
physaddr,
int mmu_idx, MemTxAttrs attrs,
MemTxResult response, uintptr_t retaddr);
 
-/* Call the EL change hook if one has been registered */
+/* Call any registered EL change hooks */
 static inline void arm_call_el_change_hook(ARMCPU *cpu)
 {
-if (cpu->el_change_hook) {
-cpu->el_change_hook(cpu, cpu->el_change_hook_opaque);
+ARMELChangeHook *hook, *next;
+QLIST_FOREACH_SAFE(hook, >el_change_hooks, node, next) {
+hook->hook(cpu, hook->opaque);
 }
 }
 
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v4 09/21] target/arm: Fix bitmask for PMCCFILTR writes

[Aaron Lindsay]

It was shifted to the left one bit too few.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index de3be11..8158d33 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1125,7 +1125,7 @@ static void pmccfiltr_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 uint64_t value)
 {
 pmccntr_op_start(env);
-env->cp15.pmccfiltr_el0 = value & 0x7E00;
+env->cp15.pmccfiltr_el0 = value & 0xfc00;
 pmccntr_op_finish(env);
 }
 
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v4 03/21] target/arm: Reorganize PMCCNTR accesses

[Aaron Lindsay]

pmccntr_read and pmccntr_write contained duplicate code that was already
being handled by pmccntr_sync. Consolidate the duplicated code into two
functions: pmccntr_op_start and pmccntr_op_finish. Add a companion to
c15_ccnt in CPUARMState so that we can simultaneously save both the
architectural register value and the last underlying cycle count - this
ensure time isn't lost and will also allow us to access the 'old'
architectural register value in order to detect overflows in later
patches.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/cpu.h|  28 ++-
 target/arm/helper.c | 100 
 2 files changed, 73 insertions(+), 55 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 19a0c03..04041db 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -454,10 +454,20 @@ typedef struct CPUARMState {
 uint64_t oslsr_el1; /* OS Lock Status */
 uint64_t mdcr_el2;
 uint64_t mdcr_el3;
-/* If the counter is enabled, this stores the last time the counter
- * was reset. Otherwise it stores the counter value
+/* Stores the architectural value of the counter *the last time it was
+ * updated* by pmccntr_op_start. Accesses should always be surrounded
+ * by pmccntr_op_start/pmccntr_op_finish to guarantee the latest
+ * architecturally-corect value is being read/set.
  */
 uint64_t c15_ccnt;
+/* Stores the delta between the architectural value and the underlying
+ * cycle count during normal operation. It is used to update c15_ccnt
+ * to be the correct architectural value before accesses. During
+ * accesses, c15_ccnt_delta contains the underlying count being used
+ * for the access, after which it reverts to the delta value in
+ * pmccntr_op_finish.
+ */
+uint64_t c15_ccnt_delta;
 uint64_t pmccfiltr_el0; /* Performance Monitor Filter Register */
 uint64_t vpidr_el2; /* Virtualization Processor ID Register */
 uint64_t vmpidr_el2; /* Virtualization Multiprocessor ID Register */
@@ -890,15 +900,17 @@ int cpu_arm_signal_handler(int host_signum, void *pinfo,
void *puc);
 
 /**
- * pmccntr_sync
+ * pmccntr_op_start/finish
  * @env: CPUARMState
  *
- * Synchronises the counter in the PMCCNTR. This must always be called twice,
- * once before any action that might affect the timer and again afterwards.
- * The function is used to swap the state of the register if required.
- * This only happens when not in user mode (!CONFIG_USER_ONLY)
+ * Convert the counter in the PMCCNTR between its delta form (the typical mode
+ * when it's enabled) and the guest-visible value. These two calls must always
+ * surround any action which might affect the counter, and the return value
+ * from pmccntr_op_start must be supplied as the second argument to
+ * pmccntr_op_finish.
  */
-void pmccntr_sync(CPUARMState *env);
+void pmccntr_op_start(CPUARMState *env);
+void pmccntr_op_finish(CPUARMState *env);
 
 /* SCTLR bit meanings. Several bits have been reused in newer
  * versions of the architecture; in that case we define constants
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 83ea8f4..f6269a2 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1000,28 +1000,53 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
 
 return true;
 }
-
-void pmccntr_sync(CPUARMState *env)
+/*
+ * Ensure c15_ccnt is the guest-visible count so that operations such as
+ * enabling/disabling the counter or filtering, modifying the count itself,
+ * etc. can be done logically. This is essentially a no-op if the counter is
+ * not enabled at the time of the call.
+ */
+void pmccntr_op_start(CPUARMState *env)
 {
-uint64_t temp_ticks;
-
-temp_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+uint64_t cycles = 0;
+cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
   ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
 
-if (env->cp15.c9_pmcr & PMCRD) {
-/* Increment once every 64 processor clock cycles */
-temp_ticks /= 64;
-}
-
 if (arm_ccnt_enabled(env)) {
-env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
+uint64_t eff_cycles = cycles;
+if (env->cp15.c9_pmcr & PMCRD) {
+/* Increment once every 64 processor clock cycles */
+eff_cycles /= 64;
+}
+
+env->cp15.c15_ccnt = eff_cycles - env->cp15.c15_ccnt_delta;
+}
+env->cp15.c15_ccnt_delta = cycles;
+}
+
+/*
+ * If PMCCNTR is enabled, recalculate the delta between the clock and the
+ * guest-visible count. A call to pmccntr_op_finish should follow every call to
+ * pmccntr_op_start.
+ */
+void pmccntr_op_finish(CPUARMState *env)
+{
+if (arm_ccnt_enabled(env)) {
+uint64_t prev_cycles = env->cp15.c1

[Qemu-devel] [PATCH v4 17/21] target/arm: PMU: Add instruction and cycle events

[Aaron Lindsay]

The instruction event is only enabled when icount is used, cycles are
always supported. Always defining get_cycle_count (but altering its
behavior depending on CONFIG_USER_ONLY) allows us to remove some
CONFIG_USER_ONLY #defines throughout the rest of the code.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/helper.c | 88 ++---
 1 file changed, 43 insertions(+), 45 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index b36630f..b91f022 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -15,6 +15,7 @@
 #include "arm_ldst.h"
 #include  /* For crc32 */
 #include "exec/semihost.h"
+#include "sysemu/cpus.h"
 #include "sysemu/kvm.h"
 #include "fpu/softfloat.h"
 
@@ -943,8 +944,49 @@ typedef struct pm_event {
 uint64_t (*get_count)(CPUARMState *);
 } pm_event;
 
+static bool event_always_supported(CPUARMState *env)
+{
+return true;
+}
+
+/*
+ * Return the underlying cycle count for the PMU cycle counters. If we're in
+ * usermode, simply return 0.
+ */
+static uint64_t cycles_get_count(CPUARMState *env)
+{
+#ifndef CONFIG_USER_ONLY
+return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+   ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
+#else
+return 0;
+#endif
+}
+
+#ifndef CONFIG_USER_ONLY
+static bool instructions_supported(CPUARMState *env)
+{
+return use_icount == 1 /* Precise instruction counting */;
+}
+
+static uint64_t instructions_get_count(CPUARMState *env)
+{
+return (uint64_t)cpu_get_icount_raw();
+}
+#endif
+
 #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
 static const pm_event pm_events[] = {
+#ifndef CONFIG_USER_ONLY
+{ .number = 0x008, /* INST_RETIRED, Instruction architecturally executed */
+  .supported = instructions_supported,
+  .get_count = instructions_get_count
+},
+{ .number = 0x011, /* CPU_CYCLES, Cycle */
+  .supported = event_always_supported,
+  .get_count = cycles_get_count
+},
+#endif
 { .number = SUPPORTED_EVENT_SENTINEL }
 };
 static uint16_t supported_event_map[0x3f];
@@ -1024,8 +1066,6 @@ static CPAccessResult pmreg_access_swinc(CPUARMState *env,
 return pmreg_access(env, ri, isread);
 }
 
-#ifndef CONFIG_USER_ONLY
-
 static CPAccessResult pmreg_access_selr(CPUARMState *env,
 const ARMCPRegInfo *ri,
 bool isread)
@@ -1140,9 +1180,7 @@ static inline bool pmu_counter_enabled(CPUARMState *env, 
uint8_t counter)
  */
 void pmccntr_op_start(CPUARMState *env)
 {
-uint64_t cycles = 0;
-cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-  ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
+uint64_t cycles = cycles_get_count(env);
 
 if (pmu_counter_enabled(env, 31)) {
 uint64_t eff_cycles = cycles;
@@ -1285,42 +1323,6 @@ static void pmccntr_write32(CPUARMState *env, const 
ARMCPRegInfo *ri,
 pmccntr_write(env, ri, deposit64(cur_val, 0, 32, value));
 }
 
-#else /* CONFIG_USER_ONLY */
-
-void pmccntr_op_start(CPUARMState *env)
-{
-}
-
-void pmccntr_op_finish(CPUARMState *env)
-{
-}
-
-void pmevcntr_op_start(CPUARMState *env, uint8_t i)
-{
-}
-
-void pmevcntr_op_finish(CPUARMState *env, uint8_t i)
-{
-}
-
-void pmu_op_start(CPUARMState *env)
-{
-}
-
-void pmu_op_finish(CPUARMState *env)
-{
-}
-
-void pmu_pre_el_change(ARMCPU *cpu, void *ignored)
-{
-}
-
-void pmu_post_el_change(ARMCPU *cpu, void *ignored)
-{
-}
-
-#endif
-
 static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 uint64_t value)
 {
@@ -1633,7 +1635,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
 /* Unimplemented so WI. */
 { .name = "PMSWINC", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 4,
   .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NOP },
-#ifndef CONFIG_USER_ONLY
 { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
   .access = PL0_RW, .type = ARM_CP_ALIAS,
   .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmselr),
@@ -1653,7 +1654,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
   .access = PL0_RW, .accessfn = pmreg_access_ccntr,
   .type = ARM_CP_IO,
   .readfn = pmccntr_read, .writefn = pmccntr_write, },
-#endif
 { .name = "PMCCFILTR", .cp = 15, .opc1 = 0, .crn = 14, .crm = 15, .opc2 = 
7,
   .writefn = pmccfiltr_write_a32, .readfn = pmccfiltr_read_a32,
   .access = PL0_RW, .accessfn = pmreg_access,
@@ -5191,7 +5191,6 @@ void register_cp_regs_for_features(ARMCPU *cpu)
  * field as main ID register, and we implement only the cycle
  * count register.
  */
-#ifndef CONFIG_USER_ONLY
 ARMCPRegInfo pmcr = {
 .name = "PMCR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 
0,
 .access = PL0_RW,
@@ -5245,7 +5244,6 @@ void reg

[Qemu-devel] [PATCH v4 04/21] target/arm: Mask PMU register writes based on PMCR_EL0.N

[Aaron Lindsay]

This is in preparation for enabling counters other than PMCCNTR

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
---
 target/arm/helper.c | 31 ++-
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index f6269a2..8bec07e 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -52,11 +52,6 @@ typedef struct V8M_SAttributes {
 static void v8m_security_lookup(CPUARMState *env, uint32_t address,
 MMUAccessType access_type, ARMMMUIdx mmu_idx,
 V8M_SAttributes *sattrs);
-
-/* Definitions for the PMCCNTR and PMCR registers */
-#define PMCRD   0x8
-#define PMCRC   0x4
-#define PMCRE   0x1
 #endif
 
 static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
@@ -906,6 +901,24 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 REGINFO_SENTINEL
 };
 
+/* Definitions for the PMU registers */
+#define PMCRN_MASK  0xf800
+#define PMCRN_SHIFT 11
+#define PMCRD   0x8
+#define PMCRC   0x4
+#define PMCRE   0x1
+
+static inline uint32_t pmu_num_counters(CPUARMState *env)
+{
+  return (env->cp15.c9_pmcr & PMCRN_MASK) >> PMCRN_SHIFT;
+}
+
+/* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */
+static inline uint64_t pmu_counter_mask(CPUARMState *env)
+{
+  return (1 << 31) | ((1 << pmu_num_counters(env)) - 1);
+}
+
 static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
bool isread)
 {
@@ -1119,14 +1132,14 @@ static void pmccfiltr_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
 uint64_t value)
 {
-value &= (1 << 31);
+value &= pmu_counter_mask(env);
 env->cp15.c9_pmcnten |= value;
 }
 
 static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
  uint64_t value)
 {
-value &= (1 << 31);
+value &= pmu_counter_mask(env);
 env->cp15.c9_pmcnten &= ~value;
 }
 
@@ -1174,14 +1187,14 @@ static void pmintenset_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
  uint64_t value)
 {
 /* We have no event counters so only the C bit can be changed */
-value &= (1 << 31);
+value &= pmu_counter_mask(env);
 env->cp15.c9_pminten |= value;
 }
 
 static void pmintenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
  uint64_t value)
 {
-value &= (1 << 31);
+value &= pmu_counter_mask(env);
 env->cp15.c9_pminten &= ~value;
 }
 
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v4 01/21] target/arm: Check PMCNTEN for whether PMCCNTR is enabled

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index b14fdab..485004e 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -994,7 +994,7 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
 {
 /* This does not support checking PMCCFILTR_EL0 register */
 
-if (!(env->cp15.c9_pmcr & PMCRE)) {
+if (!(env->cp15.c9_pmcr & PMCRE) || !(env->cp15.c9_pmcnten & (1 << 31))) {
 return false;
 }
 
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v4 12/21] target/arm: Make PMOVSCLR and PMUSERENR 64 bits wide

[Aaron Lindsay]

This is a bug fix to ensure 64-bit reads of these registers don't read
adjacent data.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/cpu.h| 4 ++--
 target/arm/helper.c | 5 +++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index a56e9a0..9f769ae 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -367,8 +367,8 @@ typedef struct CPUARMState {
 uint32_t c9_data;
 uint64_t c9_pmcr; /* performance monitor control register */
 uint64_t c9_pmcnten; /* perf monitor counter enables */
-uint32_t c9_pmovsr; /* perf monitor overflow status */
-uint32_t c9_pmuserenr; /* perf monitor user enable */
+uint64_t c9_pmovsr; /* perf monitor overflow status */
+uint64_t c9_pmuserenr; /* perf monitor user enable */
 uint64_t c9_pmselr; /* perf monitor counter selection register */
 uint64_t c9_pminten; /* perf monitor interrupt enables */
 union { /* Memory attribute redirection */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 62cace7..20b42b4 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1427,7 +1427,8 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
   .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
   .writefn = pmcntenclr_write },
 { .name = "PMOVSR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 3,
-  .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
+  .access = PL0_RW,
+  .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
   .accessfn = pmreg_access,
   .writefn = pmovsr_write,
   .raw_writefn = raw_write },
@@ -1487,7 +1488,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
   .accessfn = pmreg_access_xevcntr },
 { .name = "PMUSERENR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 0,
   .access = PL0_R | PL1_RW, .accessfn = access_tpm,
-  .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
+  .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmuserenr),
   .resetvalue = 0,
   .writefn = pmuserenr_write, .raw_writefn = raw_write },
 { .name = "PMUSERENR_EL0", .state = ARM_CP_STATE_AA64,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v4 02/21] target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0

[Aaron Lindsay]

They share the same underlying state

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 485004e..83ea8f4 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1318,7 +1318,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
   .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr),
   .writefn = pmselr_write, .raw_writefn = raw_write, },
 { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
-  .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
+  .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_ALIAS | ARM_CP_IO,
   .readfn = pmccntr_read, .writefn = pmccntr_write32,
   .accessfn = pmreg_access_ccntr },
 { .name = "PMCCNTR_EL0", .state = ARM_CP_STATE_AA64,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v4 05/21] target/arm: Fetch GICv3 state directly from CPUARMState

[Aaron Lindsay]

This eliminates the need for fetching it from el_change_hook_opaque, and
allows for supporting multiple el_change_hooks without having to hack
something together to find the registered opaque belonging to GICv3.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
---
 hw/intc/arm_gicv3_cpuif.c | 10 ++
 target/arm/cpu.h  | 10 --
 2 files changed, 2 insertions(+), 18 deletions(-)

diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 26f5eed..cb9a3a5 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -29,11 +29,7 @@ void gicv3_set_gicv3state(CPUState *cpu, GICv3CPUState *s)
 
 static GICv3CPUState *icc_cs_from_env(CPUARMState *env)
 {
-/* Given the CPU, find the right GICv3CPUState struct.
- * Since we registered the CPU interface with the EL change hook as
- * the opaque pointer, we can just directly get from the CPU to it.
- */
-return arm_get_el_change_hook_opaque(arm_env_get_cpu(env));
+return env->gicv3state;
 }
 
 static bool gicv3_use_ns_bank(CPUARMState *env)
@@ -2615,9 +2611,7 @@ void gicv3_init_cpuif(GICv3State *s)
  * it might be with code translated by CPU 0 but run by CPU 1, in
  * which case we'd get the wrong value.
  * So instead we define the regs with no ri->opaque info, and
- * get back to the GICv3CPUState from the ARMCPU by reading back
- * the opaque pointer from the el_change_hook, which we're going
- * to need to register anyway.
+ * get back to the GICv3CPUState from the CPUARMState.
  */
 define_arm_cp_regs(cpu, gicv3_cpuif_reginfo);
 if (arm_feature(>env, ARM_FEATURE_EL2)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 04041db..ff349f5 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2915,16 +2915,6 @@ void arm_register_el_change_hook(ARMCPU *cpu, 
ARMELChangeHook *hook,
  void *opaque);
 
 /**
- * arm_get_el_change_hook_opaque:
- * Return the opaque data that will be used by the el_change_hook
- * for this CPU.
- */
-static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
-{
-return cpu->el_change_hook_opaque;
-}
-
-/**
  * aa32_vfp_dreg:
  * Return a pointer to the Dn register within env in 32-bit mode.
  */
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v4 00/21] More fully implement ARM PMUv3

[Aaron Lindsay]

The ARM PMU implementation currently contains a basic cycle counter, but it is
often useful to gather counts of other events and filter them based on
execution mode. These patches flesh out the implementations of various PMU
registers including PM[X]EVCNTR and PM[X]EVTYPER, add a struct definition to
represent arbitrary counter types, implement mode filtering, send interrupts on
counter overflow, and add instruction, cycle, and software increment events.

Notable changes since v3:

* Detect counter overflow and send interrupts accordingly (adds a 'shadow' copy
  of both PMCCNTR and general-purpose counters, possibly/probably Doing It
  Wrong)
* Update counter filtering code to more closely resemble the ARM documentation
  in form and functionality 
* Don't mix EL change hooks and KVM
* Don't call gen_io_start/end if not actually using icount
* Reorganized a few of the patches to more logically group changes
* Clarify and otherwise improve a few comments
* There are also a number of less significant changes scattered around

Thanks,
Aaron

Aaron Lindsay (21):
  target/arm: Check PMCNTEN for whether PMCCNTR is enabled
  target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0
  target/arm: Reorganize PMCCNTR accesses
  target/arm: Mask PMU register writes based on PMCR_EL0.N
  target/arm: Fetch GICv3 state directly from CPUARMState
  target/arm: Support multiple EL change hooks
  target/arm: Add pre-EL change hooks
  target/arm: Allow EL change hooks to do IO
  target/arm: Fix bitmask for PMCCFILTR writes
  target/arm: Filter cycle counter based on PMCCFILTR_EL0
  target/arm: Allow AArch32 access for PMCCFILTR
  target/arm: Make PMOVSCLR and PMUSERENR 64 bits wide
  target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions
  target/arm: Implement PMOVSSET
  target/arm: Add array for supported PMU events, generate PMCEID[01]
  target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER
  target/arm: PMU: Add instruction and cycle events
  target/arm: PMU: Set PMCR.N to 4
  target/arm: Implement PMSWINC
  target/arm: Mark PMINTENSET accesses as possibly doing IO
  target/arm: Send interrupts on PMU counter overflow

 hw/intc/arm_gicv3_cpuif.c  |  10 +-
 target/arm/cpu.c   |  68 +++-
 target/arm/cpu.h   | 119 +--
 target/arm/cpu64.c |   2 -
 target/arm/helper.c| 752 ++---
 target/arm/internals.h |  14 +-
 target/arm/op_helper.c |   8 +
 target/arm/translate-a64.c |   6 +
 target/arm/translate.c |  12 +
 9 files changed, 834 insertions(+), 157 deletions(-)

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 12/22] target/arm: Filter cycle counter based on PMCCFILTR_EL0

[Aaron Lindsay]

On Apr 17 16:37, Peter Maydell wrote:
> On 17 April 2018 at 16:21, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > On Apr 12 13:36, Aaron Lindsay wrote:
> >> On Apr 12 18:15, Peter Maydell wrote:
> >> > On 16 March 2018 at 20:31, Aaron Lindsay <alind...@codeaurora.org> wrote:
> >> > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> >> > > index b0ef727..9c3b5ef 100644
> >> > > --- a/target/arm/cpu.h
> >> > > +++ b/target/arm/cpu.h
> >> > > @@ -458,6 +458,11 @@ typedef struct CPUARMState {
> >> > >   * was reset. Otherwise it stores the counter value
> >> > >   */
> >> > >  uint64_t c15_ccnt;
> >> > > +/* ccnt_cached_cycles is used to hold the last cycle count 
> >> > > when
> >> > > + * c15_ccnt holds the guest-visible count instead of the 
> >> > > delta during
> >> > > + * PMU operations which require this.
> >> > > + */
> >> > > +uint64_t ccnt_cached_cycles;
> >> >
> >> > Can this ever hold valid state at a point when we need to do VM
> >> > migration, or is it purely temporary ?
> >>
> >> I believe that as of this version of the patch it is temporary and will
> >> not need to be migrated. However, I believe it's going to be necessary
> >> to have two variables to represent the state of each counter in order to
> >> implement interrupt on overflow.
> >
> > Coming back around to this, I don't see a way around using two variables
> > to hold PMCCNTR's full state to make interrupt on overflow work. I
> > haven't been able to find other examples or documentation covering state
> > needing to be updated in more than one location for a given CP register
> > - do you know of any I've missed or have recommendations about how to
> > approach this?
> 
> Can you explain the problem in more detail? In general it's a bit of
> a red flag if you think you need more state storage space than the
> hardware has, and I don't think there's any "hidden" state in the h/w here.

The critical difference between hardware and QEMU's PMU implementation
is that hardware detects overflow when the overflow actually happens,
which would be inefficient to do in software. Because QEMU stores a
delta from the 'real' count (i.e. the clock/icount) and only updates the
architectural counter values when necessary (when they're read/written),
checking for overflow is less straightforward than checking if
incrementing an individual counter by one flips the high-order bit from
1 to 0 as it happens. If the only information you have is the current
counter value, and don't know how many events have occurred since you
last checked or what the counter value was at that time, you can't tell
whether or not it overflowed.

I haven't come up with a way to correctly and reliably detect overflow
without storing additional information. I'll go ahead and post v4 with
my first-pass implementation of the overflow code and see if you see
something I'm missing or can think of a trick we can play to keep this
inside of one register value.

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 12/22] target/arm: Filter cycle counter based on PMCCFILTR_EL0

[Aaron Lindsay]

On Apr 12 13:36, Aaron Lindsay wrote:
> On Apr 12 18:15, Peter Maydell wrote:
> > On 16 March 2018 at 20:31, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > > index b0ef727..9c3b5ef 100644
> > > --- a/target/arm/cpu.h
> > > +++ b/target/arm/cpu.h
> > > @@ -458,6 +458,11 @@ typedef struct CPUARMState {
> > >   * was reset. Otherwise it stores the counter value
> > >   */
> > >  uint64_t c15_ccnt;
> > > +/* ccnt_cached_cycles is used to hold the last cycle count when
> > > + * c15_ccnt holds the guest-visible count instead of the delta 
> > > during
> > > + * PMU operations which require this.
> > > + */
> > > +uint64_t ccnt_cached_cycles;
> > 
> > Can this ever hold valid state at a point when we need to do VM
> > migration, or is it purely temporary ?
> 
> I believe that as of this version of the patch it is temporary and will
> not need to be migrated. However, I believe it's going to be necessary
> to have two variables to represent the state of each counter in order to
> implement interrupt on overflow. 

Coming back around to this, I don't see a way around using two variables
to hold PMCCNTR's full state to make interrupt on overflow work. I
haven't been able to find other examples or documentation covering state
needing to be updated in more than one location for a given CP register
- do you know of any I've missed or have recommendations about how to
approach this?

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Aaron Lindsay]

On Apr 12 18:17, Peter Maydell wrote:
> On 16 March 2018 at 20:31, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
> > ---
> >  target/arm/cpu.c | 3 +++
> >  target/arm/cpu.h | 1 +
> >  2 files changed, 4 insertions(+)
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index b0d032c..e544f1d 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -765,6 +765,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> > **errp)
> >  /* Some features automatically imply others: */
> >  if (arm_feature(env, ARM_FEATURE_V8)) {
> >  set_feature(env, ARM_FEATURE_V7);
> > +set_feature(env, ARM_FEATURE_V7VE);
> >  set_feature(env, ARM_FEATURE_ARM_DIV);
> >  set_feature(env, ARM_FEATURE_LPAE);
> >  }
> > @@ -1481,6 +1482,7 @@ static void cortex_a7_initfn(Object *obj)
> >
> >  cpu->dtb_compatible = "arm,cortex-a7";
> >  set_feature(>env, ARM_FEATURE_V7);
> > +set_feature(>env, ARM_FEATURE_V7VE);
> >  set_feature(>env, ARM_FEATURE_VFP4);
> >  set_feature(>env, ARM_FEATURE_NEON);
> >  set_feature(>env, ARM_FEATURE_THUMB2EE);
> > @@ -1526,6 +1528,7 @@ static void cortex_a15_initfn(Object *obj)
> >
> >  cpu->dtb_compatible = "arm,cortex-a15";
> >  set_feature(>env, ARM_FEATURE_V7);
> > +set_feature(>env, ARM_FEATURE_V7VE);
> >  set_feature(>env, ARM_FEATURE_VFP4);
> >  set_feature(>env, ARM_FEATURE_NEON);
> >  set_feature(>env, ARM_FEATURE_THUMB2EE);
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index fb2f983..cc1e2fb 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -1439,6 +1439,7 @@ enum arm_features {
> >  ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling.  */
> >  ARM_FEATURE_THUMB2EE,
> >  ARM_FEATURE_V7MP,/* v7 Multiprocessing Extensions */
> > +ARM_FEATURE_V7VE,/* v7 with Virtualization Extensions */
> >  ARM_FEATURE_V4T,
> >  ARM_FEATURE_V5,
> >  ARM_FEATURE_STRONGARM,
> 
> What's the difference between this and ARM_FEATURE_EL2 ?

I use ARM_FEATURE_V7VE in a later patch to guard against implementing
PMOVSSET on v7 machines which don't implement the virtualization
extensions
(http://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg04917.html).
I could use ARM_FEATURE_EL2, but declaring that v7 machines supported
EL2 didn't feel right. I don't feel strongly one way or the other - how
do you prefer to handle this?

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 11/22] target/arm: Fix bitmask for PMCCFILTR writes

[Aaron Lindsay]

On Apr 12 17:41, Peter Maydell wrote:
> On 16 March 2018 at 20:31, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > It was shifted to the left one bit too few.
> >
> > Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
> > ---
> >  target/arm/helper.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index 50eaed7..0102357 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -1123,7 +1123,7 @@ static void pmccfiltr_write(CPUARMState *env, const 
> > ARMCPRegInfo *ri,
> >  uint64_t value)
> >  {
> >  uint64_t saved_cycles = pmccntr_op_start(env);
> > -env->cp15.pmccfiltr_el0 = value & 0x7E00;
> > +env->cp15.pmccfiltr_el0 = value & 0xfc00;
> >  pmccntr_op_finish(env, saved_cycles);
> >  }
> >
> 
> I wonder why we got that one wrong.
> 
> Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
> 
> Strictly speaking, bit 26 (M) should be visible only in
> the AArch64 view of the register, not the AArch32 one,
> but that's a separate issue.

Right. I addressed this when I added AArch32 access for PMCCFILTR:
[PATCH v3 13/22] target/arm: Allow AArch32 access for PMCCFILTR
https://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg04910.html

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 05/22] target/arm: Reorganize PMCCNTR read, write, sync

[Aaron Lindsay]

On Apr 12 17:18, Peter Maydell wrote:
> On 16 March 2018 at 20:31, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > pmccntr_read and pmccntr_write contained duplicate code that was already
> > being handled by pmccntr_sync. Split pmccntr_sync into pmccntr_op_start
> > and pmccntr_op_finish, passing the clock value between the two, to avoid
> > losing time between the two calls.
> >
> > Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
> > ---
> >  target/arm/helper.c | 101 
> > +---
> >  1 file changed, 56 insertions(+), 45 deletions(-)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index 5634561..6480b80 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -1000,28 +1000,58 @@ static inline bool arm_ccnt_enabled(CPUARMState 
> > *env)
> >
> >  return true;
> >  }
> > -
> > -void pmccntr_sync(CPUARMState *env)
> 
> If you configure your git to use the 'histogram' diff algorithm
> ("git config --global diff.algorithm histogram", or edit ~/.gitconfig
> equivalently), does it make git format-patch make less of a mess
> of this commit ?

No, it doesn't seem to make much of a difference.

> > +/*
> > + * Ensure c15_ccnt is the guest-visible count so that operations such as
> > + * enabling/disabling the counter or filtering, modifying the count itself,
> > + * etc. can be done logically. This is essentially a no-op if the counter 
> > is
> > + * not enabled at the time of the call.
> > + *
> > + * The current cycle count is returned so that it can be passed into the 
> > paired
> > + * pmccntr_op_finish() call which must follow each call to 
> > pmccntr_op_start().
> > + */
> > +uint64_t pmccntr_op_start(CPUARMState *env)
> >  {
> > -uint64_t temp_ticks;
> > -
> > -temp_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> > +uint64_t cycles = 0;
> > +#ifndef CONFIG_USER_ONLY
> > +cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> >ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
> > +#endif
> 
> Is this ifdef necessary? You have a do-nothing version of
> pmccntr_op_start() for CONFIG_USER_ONLY later on, so presumably
> this one is already inside a suitable ifndef.

You're right that it is unnecessary as of this patch. A later patch
removes the surrounding `#ifndef CONFIG_USER_ONLY` when it is no longer
necessary at that level. It would be cleaner to add the #ifndef with
smaller scope at the same time - I'll make that change.  

> Otherwise
> 
> Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>

Because I've modified how pmccntr_op_start/finish keep track of the
cycles so that they store the counter values differently for v4, I don't
feel comfortable adding your Reviewed-by. I apologize for the churn.

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 00/22] More fully implement ARM PMUv3

[Aaron Lindsay]

On Apr 12 18:32, Peter Maydell wrote:
> On 16 March 2018 at 20:30, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > The ARM PMU implementation currently contains a basic cycle counter, but it 
> > is
> > often useful to gather counts of other events and filter them based on
> > execution mode. These patches flesh out the implementations of various PMU
> > registers including PM[X]EVCNTR and PM[X]EVTYPER, add a struct definition to
> > represent arbitrary counter types, implement mode filtering, and add
> > instruction, cycle, and software increment events.
> 
> Hi; sorry it's taken me a while to get to this (I've been focusing
> on for-2.12 work). I've now reviewed most of the patches in this
> set. My suggestion is that you fix the stuff I've commented on
> and send out a v4, and then I can take some of the patches from
> the start of that into target-arm.next, and I'll review the tail
> end of the set then.

Thanks for the review! I'll see if I can get a v4 out late this week or
early next.

I'll make a note of this when I send it out, but one thing to look out
for in the next version is that I had to reorganize a few things to make
the interrupt-on-overflow functionality work better. I think the changes
are fairly minor and limited to the later patches, with the possible
exception of using two variables per counter (because you have to know
what the previous counter value was in addition to the current value to
know if you've overflowed since your last check).

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 12/22] target/arm: Filter cycle counter based on PMCCFILTR_EL0

[Aaron Lindsay]

On Apr 12 18:15, Peter Maydell wrote:
> On 16 March 2018 at 20:31, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > The pmu_counter_filtered and pmu_op_start/finish functions are generic
> > (as opposed to PMCCNTR-specific) to allow for the implementation of
> > other events.
> >
> > Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
> > ---
> >  target/arm/cpu.c|  3 ++
> >  target/arm/cpu.h| 37 +++
> >  target/arm/helper.c | 87 
> > ++---
> >  3 files changed, 116 insertions(+), 11 deletions(-)
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index a2cb21e..b0d032c 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -887,6 +887,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> > **errp)
> >  if (!cpu->has_pmu) {
> >  unset_feature(env, ARM_FEATURE_PMU);
> >  cpu->id_aa64dfr0 &= ~0xf00;
> > +} else {
> > +arm_register_pre_el_change_hook(cpu, _pre_el_change, 0);
> > +arm_register_el_change_hook(cpu, _post_el_change, 0);
> 
> You probably don't want to do this if we're using KVM, as there
> are some code paths where we call do_interrupt() when using KVM
> which will trigger the hooks and likely do unexpected things.
> (For instance kvm_arm_handle_debug() does this to set the guest
> up to take debug exceptions.)

Okay, I'll surround this with `if (!kvm_enabled())` for the next pass.
> 
> >  }
> >
> >  if (!arm_feature(env, ARM_FEATURE_EL2)) {
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index b0ef727..9c3b5ef 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -458,6 +458,11 @@ typedef struct CPUARMState {
> >   * was reset. Otherwise it stores the counter value
> >   */
> >  uint64_t c15_ccnt;
> > +/* ccnt_cached_cycles is used to hold the last cycle count when
> > + * c15_ccnt holds the guest-visible count instead of the delta 
> > during
> > + * PMU operations which require this.
> > + */
> > +uint64_t ccnt_cached_cycles;
> 
> Can this ever hold valid state at a point when we need to do VM
> migration, or is it purely temporary ?

I believe that as of this version of the patch it is temporary and will
not need to be migrated. However, I believe it's going to be necessary
to have two variables to represent the state of each counter in order to
implement interrupt on overflow. 

> 
> >  uint64_t pmccfiltr_el0; /* Performance Monitor Filter Register */
> >  uint64_t vpidr_el2; /* Virtualization Processor ID Register */
> >  uint64_t vmpidr_el2; /* Virtualization Multiprocessor ID Register 
> > */
> > @@ -896,15 +901,35 @@ int cpu_arm_signal_handler(int host_signum, void 
> > *pinfo,
> > void *puc);
> >
> >  /**
> > - * pmccntr_sync
> > + * pmccntr_op_start/finish
> 
> Shouldn't this doc change and prototype change have gone with the earlier
> patch that changed the implementation?

Hmmm, yes. Seems like I got pmccntr_op_start and pmu_op_start confused
when staging this.

> >   * @env: CPUARMState
> >   *
> > - * Synchronises the counter in the PMCCNTR. This must always be called 
> > twice,
> > - * once before any action that might affect the timer and again afterwards.
> > - * The function is used to swap the state of the register if required.
> > - * This only happens when not in user mode (!CONFIG_USER_ONLY)
> > + * Convert the counter in the PMCCNTR between its delta form (the typical 
> > mode
> > + * when it's enabled) and the guest-visible value. These two calls must 
> > always
> > + * surround any action which might affect the counter, and the return value
> > + * from pmccntr_op_start must be supplied as the second argument to
> > + * pmccntr_op_finish.
> > + */
> > +uint64_t pmccntr_op_start(CPUARMState *env);
> > +void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles);
> > +
> > +/**
> > + * pmu_op_start/finish
> > + * @env: CPUARMState
> > + *
> > + * Convert all PMU counters between their delta form (the typical mode when
> > + * they are enabled) and the guest-visible values. These two calls must
> > + * surround any action which might affect the counters, and the return 
> > value
> > + * from pmu_op_start must be supplied as the second argument to 
> > pmu_op_finish.
> > + */
> > +uint64_t pmu_op_start(CPUARMState 

[Qemu-devel] [PATCH v3] RFC: target/arm: Send interrupts on PMU counter overflow

[Aaron Lindsay]

On Mar 16 16:30, Aaron Lindsay wrote:
> I aim to eventually add raising interrupts on counter overflow, but that is 
> not
> covered by this patchset. I think I have a reasonable grasp of the mechanics 
> of
> *how* to raise them, but am curious if anyone has thoughts on how to determine
> *when* to raise them - we don't want to call into PMU code every time an
> instruction is executed to check if any instruction counters have overflowed,
> etc. The main candidate I've seen for doing this so far would be to set up a
> QEMUTimer, but I haven't fully explored it. Does that seem plausible? Any
> other/better ideas?

I'm planning to post a full v4 of this patchset soon, pending a few
review fixes, but I figured I'd throw out an early version of a patch to
add interrupts on overflow in case it obviously has major issues that
will need to be addressed.

This patch sets up a QEMUTimer to get a callback when we expect counters
to next overflow and triggers an interrupt at that time.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/cpu.c|  11 +
 target/arm/cpu.h|   7 +++
 target/arm/helper.c | 129 
 3 files changed, 138 insertions(+), 9 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index df27188..9108c6b 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -740,6 +740,12 @@ static void arm_cpu_finalizefn(Object *obj)
 QLIST_REMOVE(hook, node);
 g_free(hook);
 }
+#ifndef CONFIG_USER_ONLY
+if (arm_feature(>env, ARM_FEATURE_PMU)) {
+timer_deinit(cpu->pmu_timer);
+timer_free(cpu->pmu_timer);
+}
+#endif
 }
 
 static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
@@ -907,6 +913,11 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
**errp)
 
 arm_register_pre_el_change_hook(cpu, _pre_el_change, 0);
 arm_register_el_change_hook(cpu, _post_el_change, 0);
+
+#ifndef CONFIG_USER_ONLY
+cpu->pmu_timer = timer_new(QEMU_CLOCK_VIRTUAL, 1, arm_pmu_timer_cb,
+cpu);
+#endif
 } else {
 cpu->pmceid0 = 0x;
 cpu->pmceid1 = 0x;
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 5e6bbd3..bc0867f 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -703,6 +703,8 @@ struct ARMCPU {
 
 /* Timers used by the generic (architected) timer */
 QEMUTimer *gt_timer[NUM_GTIMERS];
+/* Timer used by the PMU */
+QEMUTimer *pmu_timer;
 /* GPIO outputs for generic timer */
 qemu_irq gt_timer_outputs[NUM_GTIMERS];
 /* GPIO output for GICv3 maintenance interrupt signal */
@@ -934,6 +936,11 @@ void pmu_op_start(CPUARMState *env);
 void pmu_op_finish(CPUARMState *env);
 
 /**
+ * Called when a PMU counter is due to overflow
+ */
+void arm_pmu_timer_cb(void *opaque);
+
+/**
  * Functions to register as EL change hooks for PMU mode filtering
  */
 void pmu_pre_el_change(ARMCPU *cpu, void *ignored);
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 2147678..abe24dc 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -905,6 +905,7 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 /* Definitions for the PMU registers */
 #define PMCRN_MASK  0xf800
 #define PMCRN_SHIFT 11
+#define PMCRLC  0x40
 #define PMCRD   0x8
 #define PMCRC   0x4
 #define PMCRP   0x2
@@ -919,6 +920,8 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 #define PMXEVTYPER_MT 0x0200
 #define PMXEVTYPER_EVTCOUNT   0x03ff
 
+#define PMEVCNTR_OVERFLOW_MASK ((uint64_t)1 << 31)
+
 #define PMCCFILTR 0xf800
 #define PMCCFILTR_M   PMXEVTYPER_M
 #define PMCCFILTR_EL0 (PMCCFILTR | PMCCFILTR_M)
@@ -934,6 +937,11 @@ typedef struct pm_event {
 /* Retrieve the current count of the underlying event. The programmed
  * counters hold a difference from the return value from this function */
 uint64_t (*get_count)(CPUARMState *);
+/* Return how many nanoseconds it will take (at a minimum) for count events
+ * to occur. A negative value indicates the counter will never overflow, or
+ * that the counter has otherwise arranged for the overflow bit to be set
+ * and the PMU interrupt to be raised on overflow. */
+int64_t (*ns_per_count)(uint64_t);
 } pm_event;
 
 static bool event_always_supported(CPUARMState *env)
@@ -950,6 +958,11 @@ static uint64_t swinc_get_count(CPUARMState *env)
 return 0;
 }
 
+static int64_t swinc_ns_per(uint64_t ignored)
+{
+return -1;
+}
+
 /*
  * Return the underlying cycle count for the PMU cycle counters. If we're in
  * usermode, simply return 0.
@@ -965,6 +978,11 @@ static uint64_t cycles_get_count(CPUARMState *env)
 }
 
 #ifndef CONFIG_USER_ONLY
+static int64_t cycles_ns_per(uint64_t cycles)
+{
+return ARM_CPU_FREQ/NANOSECONDS_PER_SECOND;
+}
+
 static bool instructions_supported(CPUARMState *env)
 {
 return use_icount == 1 /* Precise in

Re: [Qemu-devel] [PATCH v3 10/22] target/arm: Allow EL change hooks to do IO

[Aaron Lindsay]

On Apr 12 17:53, Peter Maydell wrote:
> On 16 March 2018 at 20:31, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > During code generation, surround CPSR writes and exception returns which
> > call the EL change hooks with gen_io_start/end. The immediate need is
> > for the PMU to access the clock and icount during EL change to support
> > mode filtering.
> >
> > Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
> > ---
> >  target/arm/translate-a64.c | 2 ++
> >  target/arm/translate.c | 4 
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> > index 31ff047..e1ae676 100644
> > --- a/target/arm/translate-a64.c
> > +++ b/target/arm/translate-a64.c
> > @@ -1919,7 +1919,9 @@ static void disas_uncond_b_reg(DisasContext *s, 
> > uint32_t insn)
> >  unallocated_encoding(s);
> >  return;
> >  }
> > +gen_io_start();
> >  gen_helper_exception_return(cpu_env);
> > +gen_io_end();
> 
> You don't want to call gen_io_start() or gen_io_end() unless
> tb_cflags(s->base.tb) & CF_USE_ICOUNT) is true.
> 
> (Ditto in the other cases below.)

I assume there's nothing tricky about this and updating this as follows
is sufficient?

> > +if (tb_cflags(s->base.tb) & CF_USE_ICOUNT) {
> > +gen_io_start();
> > +}
> >  gen_helper_exception_return(cpu_env);
> > +if (tb_cflags(s->base.tb) & CF_USE_ICOUNT) {
> > +gen_io_end();
> > +}

-Aaron

> 
> >  /* Must exit loop to check un-masked IRQs */
> >  s->base.is_jmp = DISAS_EXIT;
> >  return;
> > diff --git a/target/arm/translate.c b/target/arm/translate.c
> > index ba6ab7d..fd5871e 100644
> > --- a/target/arm/translate.c
> > +++ b/target/arm/translate.c
> > @@ -4536,7 +4536,9 @@ static void gen_rfe(DisasContext *s, TCGv_i32 pc, 
> > TCGv_i32 cpsr)
> >   * appropriately depending on the new Thumb bit, so it must
> >   * be called after storing the new PC.
> >   */
> > +gen_io_start();
> >  gen_helper_cpsr_write_eret(cpu_env, cpsr);
> > +gen_io_end();
> >  tcg_temp_free_i32(cpsr);
> >  /* Must exit loop to check un-masked IRQs */
> >  s->base.is_jmp = DISAS_EXIT;
> > @@ -9828,7 +9830,9 @@ static void disas_arm_insn(DisasContext *s, unsigned 
> > int insn)
> >  if (exc_return) {
> >  /* Restore CPSR from SPSR.  */
> >  tmp = load_cpu_field(spsr);
> > +gen_io_start();
> >  gen_helper_cpsr_write_eret(cpu_env, tmp);
> > +gen_io_end();
> >  tcg_temp_free_i32(tmp);
> >  /* Must exit loop to check un-masked IRQs */
> >  s->base.is_jmp = DISAS_EXIT;
> > --
> > Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, 
> > Inc.
> > Qualcomm Technologies, Inc. is a member of the
> > Code Aurora Forum, a Linux Foundation Collaborative Project.
> >
> 
> thanks
> -- PMM

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 09/22] target/arm: Add pre-EL change hooks

[Aaron Lindsay]

On Apr 12 17:49, Peter Maydell wrote:
> On 16 March 2018 at 20:31, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > Because the design of the PMU requires that the counter values be
> > converted between their delta and guest-visible forms for mode
> > filtering, an additional hook which occurs before the EL is changed is
> > necessary.
> >
> > Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
> > ---
> >  target/arm/cpu.c   | 13 +
> >  target/arm/cpu.h   | 12 
> >  target/arm/helper.c| 14 --
> >  target/arm/internals.h |  7 +++
> >  target/arm/op_helper.c |  8 
> >  5 files changed, 44 insertions(+), 10 deletions(-)
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 5f782bf..a2cb21e 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -55,6 +55,18 @@ static bool arm_cpu_has_work(CPUState *cs)
> >   | CPU_INTERRUPT_EXITTB);
> >  }
> >
> > +void arm_register_pre_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
> > + void *opaque)
> > +{
> > +ARMELChangeHook *entry;
> > +entry = g_malloc0(sizeof (*entry));
> 
> g_new0().
> 
> > +
> > +entry->hook = hook;
> > +entry->opaque = opaque;
> > +
> > +QLIST_INSERT_HEAD(>pre_el_change_hooks, entry, node);
> > +}
> > +
> >  void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
> >   void *opaque)
> >  {
> > @@ -747,6 +759,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> > **errp)
> >  return;
> >  }
> >
> > +QLIST_INIT(>pre_el_change_hooks);
> >  QLIST_INIT(>el_change_hooks);
> >
> >  /* Some features automatically imply others: */
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 3b45d3d..b0ef727 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -832,6 +832,7 @@ struct ARMCPU {
> >   */
> >  bool cfgend;
> >
> > +QLIST_HEAD(, ARMELChangeHook) pre_el_change_hooks;
> >  QLIST_HEAD(, ARMELChangeHook) el_change_hooks;
> >
> >  int32_t node_id; /* NUMA node this CPU belongs to */
> > @@ -2895,12 +2896,15 @@ static inline AddressSpace 
> > *arm_addressspace(CPUState *cs, MemTxAttrs attrs)
> >  #endif
> >
> >  /**
> > + * arm_register_pre_el_change_hook:
> >   * arm_register_el_change_hook:
> > - * Register a hook function which will be called back whenever this
> > - * CPU changes exception level or mode. The hook function will be
> > - * passed a pointer to the ARMCPU and the opaque data pointer passed
> > - * to this function when the hook was registered.
> > + * Register a hook function which will be called back before or after this 
> > CPU
> > + * changes exception level or mode. The hook function will be passed a 
> > pointer
> > + * to the ARMCPU and the opaque data pointer passed to this function when 
> > the
> > + * hook was registered.
> >   */
> 
> I would just have one doc comment for each function, rather than
> trying to share. (Some day we may actually autogenerate HTML docs
> from these comments...)
> 
> Do we make the guarantee that if we call the pre-change hook
> then we will definitely subsequently call the post-change hook?
> (ie does the PMU hook rely on that?)

Yes, the PMU relies on the pre- and post- hooks being called in pairs
since they drive the state machine of sorts that exists in the variables
holding the counter values/deltas. And unless I've really screwed up the
implementation, I believe the change hooks in my patchset make that
guarantee.

-Aaron

> 
> Otherwise looks OK.
> 
> thanks
> -- PMM

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 04/22] target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0

[Aaron Lindsay]

On Apr 12 17:10, Peter Maydell wrote:
> On 16 March 2018 at 20:31, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > They share the same underlying state
> >
> > Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
> > ---
> >  target/arm/helper.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index 5e48982..5634561 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -1318,7 +1318,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
> >.fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr),
> >.writefn = pmselr_write, .raw_writefn = raw_write, },
> >  { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 
> > 0,
> > -  .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
> > +  .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_ALIAS | ARM_CP_IO,
> >.readfn = pmccntr_read, .writefn = pmccntr_write32,
> >.accessfn = pmreg_access_ccntr },
> >  { .name = "PMCCNTR_EL0", .state = ARM_CP_STATE_AA64,
> > --
> 
> Does this fix an observed bug (presumably migration related),
> or is it just something you saw in code inspection ?
> Worth noting in the commit message if the former.

Purely code inspection. I forget now, but I think I found it after
chasing down some other register issue in the vicinity and noticed this
while digging around.

-Aaron

> Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
> 
> thanks
> -- PMM

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 01/22] target/arm: A53: Initialize PMCEID[01]

[Aaron Lindsay]

On Mar 20 02:03, Philippe Mathieu-Daudé wrote:
> On 03/19/2018 09:35 PM, Aaron Lindsay wrote:
> > On Mar 18 23:35, Philippe Mathieu-Daudé wrote:
> >> Hi Aaron,
> >>
> >> On 03/16/2018 09:30 PM, Aaron Lindsay wrote:
> >>> A53 advertises ARM_FEATURE_PMU, but wasn't initializing pmceid[01].
> >>> pmceid[01] are already being initialized to zero for both A15 and A57.
> >>>
> >>> Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
> >>> ---
> >>>  target/arm/cpu64.c | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> >>> index 991d764..8c4db31 100644
> >>> --- a/target/arm/cpu64.c
> >>> +++ b/target/arm/cpu64.c
> >>> @@ -201,6 +201,8 @@ static void aarch64_a53_initfn(Object *obj)
> >>>  cpu->id_isar5 = 0x00011121;
> >>>  cpu->id_aa64pfr0 = 0x;
> >>>  cpu->id_aa64dfr0 = 0x10305106;
> >>> +cpu->pmceid0 = 0x;
> >>> +cpu->pmceid1 = 0x;
> >>>  cpu->id_aa64isar0 = 0x00011120;
> >>>  cpu->id_aa64mmfr0 = 0x1122; /* 40 bit physical addr */
> >>>  cpu->dbgdidr = 0x3516d000;
> >>>
> >>
> >> Maybe we can move this at a single place in arm_cpu_post_init():
> >>
> >> if (arm_feature(>env, ARM_FEATURE_PMU)) {
> >> cpu->pmceid0 = 0x;
> >> cpu->pmceid1 = 0x;
> >> }
> > 
> > I like consolidating the initialization - though I think it can go in
> > arm_cpu_realizefn() with the preexisting PMU-related id_aa64dfr0
> > initialization since it is constant once you've chosen a type of
> > processor. One of the other patches in this set actually already adds
> > some PMCEID initialization there based on PMCR.N.
> 
> Indeed, arm_cpu_realizefn() is a good place.

I've consolidated the pmceid[01] initialization into arm_cpu_realizefn()
for v4.

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 08/22] target/arm: Support multiple EL change hooks

[Aaron Lindsay]

On Mar 18 23:41, Philippe Mathieu-Daudé wrote:
> On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> > Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
> > ---
> >  target/arm/cpu.c   | 15 ++-
> >  target/arm/cpu.h   | 23 ---
> >  target/arm/internals.h |  7 ---
> >  3 files changed, 26 insertions(+), 19 deletions(-)
> > 
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 072cbbf..5f782bf 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -55,13 +55,16 @@ static bool arm_cpu_has_work(CPUState *cs)
> >   | CPU_INTERRUPT_EXITTB);
> >  }
> >  
> > -void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook,
> > +void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
> >   void *opaque)
> >  {
> > -/* We currently only support registering a single hook function */
> > -assert(!cpu->el_change_hook);
> > -cpu->el_change_hook = hook;
> > -cpu->el_change_hook_opaque = opaque;
> > +ARMELChangeHook *entry;
> > +entry = g_malloc0(sizeof (*entry));
> 
> imho g_malloc() is enough.

It seems like the only difference is between initializing it to zero
(g_malloc0) and making it as uninitialized (g_malloc) for coverity. Are
there coding standards for when we should choose which?

> 
> > +
> > +entry->hook = hook;
> > +entry->opaque = opaque;
> > +
> > +QLIST_INSERT_HEAD(>el_change_hooks, entry, node);
> >  }
> >  
> >  static void cp_reg_reset(gpointer key, gpointer value, gpointer opaque)
> > @@ -744,6 +747,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> > **errp)
> >  return;
> >  }
> >  
> > +QLIST_INIT(>el_change_hooks);
> > +
> 
> You missed to fill arm_cpu_unrealizefn() with:
> 
> QLIST_FOREACH(...) {
> QLIST_REMOVE(...);
> g_free(...);
> }

Do you mean arm_cpu_finalizefn()?

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 01/22] target/arm: A53: Initialize PMCEID[01]

[Aaron Lindsay]

On Mar 18 23:35, Philippe Mathieu-Daudé wrote:
> Hi Aaron,
> 
> On 03/16/2018 09:30 PM, Aaron Lindsay wrote:
> > A53 advertises ARM_FEATURE_PMU, but wasn't initializing pmceid[01].
> > pmceid[01] are already being initialized to zero for both A15 and A57.
> > 
> > Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
> > ---
> >  target/arm/cpu64.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> > index 991d764..8c4db31 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -201,6 +201,8 @@ static void aarch64_a53_initfn(Object *obj)
> >  cpu->id_isar5 = 0x00011121;
> >  cpu->id_aa64pfr0 = 0x;
> >  cpu->id_aa64dfr0 = 0x10305106;
> > +cpu->pmceid0 = 0x;
> > +cpu->pmceid1 = 0x;
> >  cpu->id_aa64isar0 = 0x00011120;
> >  cpu->id_aa64mmfr0 = 0x1122; /* 40 bit physical addr */
> >  cpu->dbgdidr = 0x3516d000;
> > 
> 
> Maybe we can move this at a single place in arm_cpu_post_init():
> 
> if (arm_feature(>env, ARM_FEATURE_PMU)) {
> cpu->pmceid0 = 0x;
> cpu->pmceid1 = 0x;
> }

I like consolidating the initialization - though I think it can go in
arm_cpu_realizefn() with the preexisting PMU-related id_aa64dfr0
initialization since it is constant once you've chosen a type of
processor. One of the other patches in this set actually already adds
some PMCEID initialization there based on PMCR.N.

-Aaron

> 
> Regards,
> 
> Phil.

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 20/22] target/arm: PMU: Add instruction and cycle events

[Aaron Lindsay]

On Mar 18 23:48, Philippe Mathieu-Daudé wrote:
> On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> > The instruction event is only enabled when icount is used, cycles are
> > always supported. Always defining get_cycle_count (but altering its
> > behavior depending on CONFIG_USER_ONLY) allows us to remove some
> > CONFIG_USER_ONLY #defines throughout the rest of the code.
> > 
> > Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
> > ---
> [...]>  #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
> >  static const pm_event pm_events[] = {
> > +#ifndef CONFIG_USER_ONLY
> > +{ .number = 0x008, /* INST_RETIRED */
> 
> "Instruction architecturally executed" seems more explicit to me.

I've updated v4 to include this wording as well.

> > +  .supported = instructions_supported,
> > +  .get_count = instructions_get_count
> > +},
> > +{ .number = 0x011, /* CPU_CYCLES */
> > +  .supported = event_always_supported,
> > +  .get_count = cycles_get_count
> > +},
> > +#endif
> [...]

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 14/22] target/arm: Make PMOVSCLR 64 bits wide

[Aaron Lindsay]

Phil,

On Mar 19 00:14, Philippe Mathieu-Daudé wrote:
> Hi Aaron,
> 
> On 03/16/2018 09:31 PM, Aaron Lindsay wrote:
> > This is a bug fix to ensure 64-bit reads of this register don't read
> > adjacent data.
> > 
> > Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
> > ---
> >  target/arm/cpu.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 9c3b5ef..fb2f983 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -367,7 +367,7 @@ typedef struct CPUARMState {
> >  uint32_t c9_data;
> >  uint64_t c9_pmcr; /* performance monitor control register */
> >  uint64_t c9_pmcnten; /* perf monitor counter enables */
> > -uint32_t c9_pmovsr; /* perf monitor overflow status */
> > +uint64_t c9_pmovsr; /* perf monitor overflow status */
> 
> This doesn't look correct, since this reg is 32b.
> 
> I *think* the correct fix is in ARMCPRegInfo v7_cp_reginfo[]:
> 
> { .name = "PMOVSR", ...
> - ..., .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
> + ..., .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
>   .accessfn = pmreg_access,
>   .writefn = pmovsr_write,
>   .raw_writefn = raw_write },

Nearly all of these PMU registers are 32 bits wide, but most of them are
implemented as 64-bit registers (PMCR, PMCNTEN*, PMSELR, PMINTEN* are a
few examples I see in this patch's context). My understanding is that
AArch64 register accesses are handled as 64 bits, even if the register
itself isn't that wide (though I haven't personally verified this). See
an earlier email from Peter from v2 of this patchset:

https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03983.html

Does this still look wrong to you? If so, I'll take a more thorough look
into how these accesses work.

> >  uint32_t c9_pmuserenr; /* perf monitor user enable */

Whatever we decide should likely be done to PMUSERENR too - I think I
overlooked this one before.

> >  uint64_t c9_pmselr; /* perf monitor counter selection register */
> >  uint64_t c9_pminten; /* perf monitor interrupt enables */
> > 
> 
> Regards,
> 
> Phil.

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 00/22] More fully implement ARM PMUv3

[Aaron Lindsay]

My apologies for the below style issues - I've already fixed them up for
v4...

-Aaron

On Mar 16 13:58, no-re...@patchew.org wrote:
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Type: series
> Message-id: 1521232280-13089-1-git-send-email-alind...@codeaurora.org
> Subject: [Qemu-devel] [PATCH v3 00/22] More fully implement ARM PMUv3
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
> echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
> if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; 
> then
> failed=1
> echo
> fi
> n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>  * [new tag]   
> patchew/1521232280-13089-1-git-send-email-alind...@codeaurora.org -> 
> patchew/1521232280-13089-1-git-send-email-alind...@codeaurora.org
> Switched to a new branch 'test'
> d81791b184 target/arm: Implement PMSWINC
> 512124d018 target/arm: PMU: Set PMCR.N to 4
> b748e97306 target/arm: PMU: Add instruction and cycle events
> 7e46a77f89 target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER
> 82cffef855 target/arm: Add array for supported PMU events, generate PMCEID[01]
> d635021d0a target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled
> 3e1b438deb target/arm: Implement PMOVSSET
> c13c832988 target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions
> 9c80af4d82 target/arm: Make PMOVSCLR 64 bits wide
> 8ffbcd3dd8 target/arm: Allow AArch32 access for PMCCFILTR
> 7dc4ec9715 target/arm: Filter cycle counter based on PMCCFILTR_EL0
> f1e40f9fff target/arm: Fix bitmask for PMCCFILTR writes
> eb142b42b4 target/arm: Allow EL change hooks to do IO
> d37186abdd target/arm: Add pre-EL change hooks
> ae12e161da target/arm: Support multiple EL change hooks
> 9bfa99805d target/arm: Fetch GICv3 state directly from CPUARMState
> 881bee5e96 target/arm: Mask PMU register writes based on PMCR_EL0.N
> 890ad6472b target/arm: Reorganize PMCCNTR read, write, sync
> 922eec023b target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0
> 5b1ce33c0b target/arm: Check PMCNTEN for whether PMCCNTR is enabled
> 2ecb09ae04 target/arm: A15 PMCEID0 initialization style nit
> 0f26a1568a target/arm: A53: Initialize PMCEID[01]
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/22: target/arm: A53: Initialize PMCEID[01]...
> Checking PATCH 2/22: target/arm: A15 PMCEID0 initialization style nit...
> Checking PATCH 3/22: target/arm: Check PMCNTEN for whether PMCCNTR is 
> enabled...
> Checking PATCH 4/22: target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0...
> Checking PATCH 5/22: target/arm: Reorganize PMCCNTR read, write, sync...
> Checking PATCH 6/22: target/arm: Mask PMU register writes based on 
> PMCR_EL0.N...
> Checking PATCH 7/22: target/arm: Fetch GICv3 state directly from 
> CPUARMState...
> Checking PATCH 8/22: target/arm: Support multiple EL change hooks...
> ERROR: space prohibited between function name and open parenthesis '('
> #26: FILE: target/arm/cpu.c:62:
> +entry = g_malloc0(sizeof (*entry));
> 
> total: 1 errors, 0 warnings, 87 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> Checking PATCH 9/22: target/arm: Add pre-EL change hooks...
> ERROR: space prohibited between function name and open parenthesis '('
> #26: FILE: target/arm/cpu.c:62:
> +entry = g_malloc0(sizeof (*entry));
> 
> total: 1 errors, 0 warnings, 110 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> Checking PATCH 10/22: target/arm: Allow EL change hooks to do IO...
> Checking PATCH 11/22: target/arm: Fix bitmask for PMCCFILTR writes...
> Checking PATCH 12/22: target/arm: Filter cycle counter based on 
> PMCCFILTR_EL0...
> Checking PATCH 13/22: target/arm: Allow AArch32 access for PMCCFILTR...
> Checking PATCH 14/22: target/arm: Make PMOVSCLR 64 bits wide...
> Checking PATCH 15/22: target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization 
> Extensions...
> Checking PATCH 16/22: target/arm: Implement PMOVSSET...
> WARNING: line over 80 characters
> #39: FILE: target/arm/helper.c:1417:
> +  .access = PL0_RW, .fieldoffset = offsetoflow32(CPUARMState, 
> cp15.c9_pmovsr),
> 
> total: 0 errors, 1 warnings, 59 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in 

[Qemu-devel] [PATCH v3 20/22] target/arm: PMU: Add instruction and cycle events

[Aaron Lindsay]

The instruction event is only enabled when icount is used, cycles are
always supported. Always defining get_cycle_count (but altering its
behavior depending on CONFIG_USER_ONLY) allows us to remove some
CONFIG_USER_ONLY #defines throughout the rest of the code.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/helper.c | 99 -
 1 file changed, 52 insertions(+), 47 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 2fa8308..679897a 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -15,6 +15,7 @@
 #include "arm_ldst.h"
 #include  /* For crc32 */
 #include "exec/semihost.h"
+#include "sysemu/cpus.h"
 #include "sysemu/kvm.h"
 #include "fpu/softfloat.h"
 
@@ -935,8 +936,54 @@ typedef struct pm_event {
 uint64_t (*get_count)(CPUARMState *, uint64_t cycles);
 } pm_event;
 
+/*
+ * Return the underlying cycle count for the PMU cycle counters. If we're in
+ * usermode, simply return 0.
+ */
+static uint64_t get_cycle_count(CPUARMState *env)
+{
+#ifndef CONFIG_USER_ONLY
+return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+   ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
+#else
+return 0;
+#endif
+}
+
+static bool event_always_supported(CPUARMState *env)
+{
+return true;
+}
+
+#ifndef CONFIG_USER_ONLY
+static uint64_t cycles_get_count(CPUARMState *env, uint64_t cycles)
+{
+return cycles;
+}
+
+static bool instructions_supported(CPUARMState *env)
+{
+return use_icount == 1 /* Precise instruction counting */;
+}
+
+static uint64_t instructions_get_count(CPUARMState *env, uint64_t cycles)
+{
+return (uint64_t)cpu_get_icount_raw();
+}
+#endif
+
 #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
 static const pm_event pm_events[] = {
+#ifndef CONFIG_USER_ONLY
+{ .number = 0x008, /* INST_RETIRED */
+  .supported = instructions_supported,
+  .get_count = instructions_get_count
+},
+{ .number = 0x011, /* CPU_CYCLES */
+  .supported = event_always_supported,
+  .get_count = cycles_get_count
+},
+#endif
 { .number = SUPPORTED_EVENT_SENTINEL }
 };
 static uint16_t supported_event_map[0x3f];
@@ -1016,8 +1063,6 @@ static CPAccessResult pmreg_access_swinc(CPUARMState *env,
 return pmreg_access(env, ri, isread);
 }
 
-#ifndef CONFIG_USER_ONLY
-
 static CPAccessResult pmreg_access_selr(CPUARMState *env,
 const ARMCPRegInfo *ri,
 bool isread)
@@ -1126,11 +1171,7 @@ static inline bool pmu_counter_filtered(CPUARMState 
*env, uint64_t pmxevtyper)
  */
 uint64_t pmccntr_op_start(CPUARMState *env)
 {
-uint64_t cycles = 0;
-#ifndef CONFIG_USER_ONLY
-cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-  ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
-#endif
+uint64_t cycles = get_cycle_count(env);
 
 if (arm_ccnt_enabled(env) &&
   !pmu_counter_filtered(env, env->cp15.pmccfiltr_el0)) {
@@ -1268,26 +1309,6 @@ static void pmccntr_write32(CPUARMState *env, const 
ARMCPRegInfo *ri,
 pmccntr_write(env, ri, deposit64(cur_val, 0, 32, value));
 }
 
-#else /* CONFIG_USER_ONLY */
-
-uint64_t pmccntr_op_start(CPUARMState *env)
-{
-}
-
-void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
-{
-}
-
-uint64_t pmu_op_start(CPUARMState *env)
-{
-}
-
-void pmu_op_finish(CPUARMState *env, uint64_t prev_cycles)
-{
-}
-
-#endif
-
 static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 uint64_t value)
 {
@@ -1346,11 +1367,7 @@ static void pmevtyper_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 if (counter == 0x1f) {
 pmccfiltr_write(env, ri, value);
 } else if (counter < PMU_NUM_COUNTERS(env)) {
-uint64_t cycles = 0;
-#ifndef CONFIG_USER_ONLY
-cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-  ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
-#endif
+uint64_t cycles = get_cycle_count(env);
 pmu_sync_counter(env, counter, cycles);
 env->cp15.c14_pmevtyper[counter] = value & 0xfe0003ff;
 pmu_sync_counter(env, counter, cycles);
@@ -1404,11 +1421,7 @@ static void pmevcntr_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
  uint64_t value, uint8_t counter)
 {
 if (counter < PMU_NUM_COUNTERS(env)) {
-uint64_t cycles = 0;
-#ifndef CONFIG_USER_ONLY
-cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-  ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
-#endif
+uint64_t cycles = get_cycle_count(env);
 env->cp15.c14_pmevcntr[counter] = value;
 pmu_sync_counter(env, counter, cycles);
 }
@@ -1420,12 +1433,8 @@ static uint64_t pmevcntr_read(CPUARMState *env, const 
ARMCPRegInfo *ri,
   uint8_t counter)
 {
 if (cou

[Qemu-devel] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/cpu.c | 3 +++
 target/arm/cpu.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index b0d032c..e544f1d 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -765,6 +765,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
**errp)
 /* Some features automatically imply others: */
 if (arm_feature(env, ARM_FEATURE_V8)) {
 set_feature(env, ARM_FEATURE_V7);
+set_feature(env, ARM_FEATURE_V7VE);
 set_feature(env, ARM_FEATURE_ARM_DIV);
 set_feature(env, ARM_FEATURE_LPAE);
 }
@@ -1481,6 +1482,7 @@ static void cortex_a7_initfn(Object *obj)
 
 cpu->dtb_compatible = "arm,cortex-a7";
 set_feature(>env, ARM_FEATURE_V7);
+set_feature(>env, ARM_FEATURE_V7VE);
 set_feature(>env, ARM_FEATURE_VFP4);
 set_feature(>env, ARM_FEATURE_NEON);
 set_feature(>env, ARM_FEATURE_THUMB2EE);
@@ -1526,6 +1528,7 @@ static void cortex_a15_initfn(Object *obj)
 
 cpu->dtb_compatible = "arm,cortex-a15";
 set_feature(>env, ARM_FEATURE_V7);
+set_feature(>env, ARM_FEATURE_V7VE);
 set_feature(>env, ARM_FEATURE_VFP4);
 set_feature(>env, ARM_FEATURE_NEON);
 set_feature(>env, ARM_FEATURE_THUMB2EE);
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index fb2f983..cc1e2fb 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1439,6 +1439,7 @@ enum arm_features {
 ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling.  */
 ARM_FEATURE_THUMB2EE,
 ARM_FEATURE_V7MP,/* v7 Multiprocessing Extensions */
+ARM_FEATURE_V7VE,/* v7 with Virtualization Extensions */
 ARM_FEATURE_V4T,
 ARM_FEATURE_V5,
 ARM_FEATURE_STRONGARM,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 12/22] target/arm: Filter cycle counter based on PMCCFILTR_EL0

[Aaron Lindsay]

The pmu_counter_filtered and pmu_op_start/finish functions are generic
(as opposed to PMCCNTR-specific) to allow for the implementation of
other events.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/cpu.c|  3 ++
 target/arm/cpu.h| 37 +++
 target/arm/helper.c | 87 ++---
 3 files changed, 116 insertions(+), 11 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index a2cb21e..b0d032c 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -887,6 +887,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
**errp)
 if (!cpu->has_pmu) {
 unset_feature(env, ARM_FEATURE_PMU);
 cpu->id_aa64dfr0 &= ~0xf00;
+} else {
+arm_register_pre_el_change_hook(cpu, _pre_el_change, 0);
+arm_register_el_change_hook(cpu, _post_el_change, 0);
 }
 
 if (!arm_feature(env, ARM_FEATURE_EL2)) {
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index b0ef727..9c3b5ef 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -458,6 +458,11 @@ typedef struct CPUARMState {
  * was reset. Otherwise it stores the counter value
  */
 uint64_t c15_ccnt;
+/* ccnt_cached_cycles is used to hold the last cycle count when
+ * c15_ccnt holds the guest-visible count instead of the delta during
+ * PMU operations which require this.
+ */
+uint64_t ccnt_cached_cycles;
 uint64_t pmccfiltr_el0; /* Performance Monitor Filter Register */
 uint64_t vpidr_el2; /* Virtualization Processor ID Register */
 uint64_t vmpidr_el2; /* Virtualization Multiprocessor ID Register */
@@ -896,15 +901,35 @@ int cpu_arm_signal_handler(int host_signum, void *pinfo,
void *puc);
 
 /**
- * pmccntr_sync
+ * pmccntr_op_start/finish
  * @env: CPUARMState
  *
- * Synchronises the counter in the PMCCNTR. This must always be called twice,
- * once before any action that might affect the timer and again afterwards.
- * The function is used to swap the state of the register if required.
- * This only happens when not in user mode (!CONFIG_USER_ONLY)
+ * Convert the counter in the PMCCNTR between its delta form (the typical mode
+ * when it's enabled) and the guest-visible value. These two calls must always
+ * surround any action which might affect the counter, and the return value
+ * from pmccntr_op_start must be supplied as the second argument to
+ * pmccntr_op_finish.
+ */
+uint64_t pmccntr_op_start(CPUARMState *env);
+void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles);
+
+/**
+ * pmu_op_start/finish
+ * @env: CPUARMState
+ *
+ * Convert all PMU counters between their delta form (the typical mode when
+ * they are enabled) and the guest-visible values. These two calls must
+ * surround any action which might affect the counters, and the return value
+ * from pmu_op_start must be supplied as the second argument to pmu_op_finish.
+ */
+uint64_t pmu_op_start(CPUARMState *env);
+void pmu_op_finish(CPUARMState *env, uint64_t prev_cycles);
+
+/**
+ * Functions to register as EL change hooks for PMU mode filtering
  */
-void pmccntr_sync(CPUARMState *env);
+void pmu_pre_el_change(ARMCPU *cpu, void *ignored);
+void pmu_post_el_change(ARMCPU *cpu, void *ignored);
 
 /* SCTLR bit meanings. Several bits have been reused in newer
  * versions of the architecture; in that case we define constants
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 0102357..95b09d6 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -908,6 +908,15 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 #define PMCRC   0x4
 #define PMCRE   0x1
 
+#define PMXEVTYPER_P  0x8000
+#define PMXEVTYPER_U  0x4000
+#define PMXEVTYPER_NSK0x2000
+#define PMXEVTYPER_NSU0x1000
+#define PMXEVTYPER_NSH0x0800
+#define PMXEVTYPER_M  0x0400
+#define PMXEVTYPER_MT 0x0200
+#define PMXEVTYPER_EVTCOUNT   0x03ff
+
 #define PMU_NUM_COUNTERS(env) ((env->cp15.c9_pmcr & PMCRN_MASK) >> PMCRN_SHIFT)
 /* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */
 #define PMU_COUNTER_MASK(env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 1))
@@ -998,7 +1007,7 @@ static CPAccessResult pmreg_access_ccntr(CPUARMState *env,
 
 static inline bool arm_ccnt_enabled(CPUARMState *env)
 {
-/* This does not support checking PMCCFILTR_EL0 register */
+/* Does not check PMCCFILTR_EL0, which is handled by pmu_counter_filtered 
*/
 
 if (!(env->cp15.c9_pmcr & PMCRE) || !(env->cp15.c9_pmcnten & (1 << 31))) {
 return false;
@@ -1006,6 +1015,44 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
 
 return true;
 }
+
+/* Returns true if the counter corresponding to the passed-in pmevtyper or
+ * pmccfiltr value is filtered using the current state */
+static inline bool pmu_counter_filter

[Qemu-devel] [PATCH v3 11/22] target/arm: Fix bitmask for PMCCFILTR writes

[Aaron Lindsay]

It was shifted to the left one bit too few.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 50eaed7..0102357 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1123,7 +1123,7 @@ static void pmccfiltr_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 uint64_t value)
 {
 uint64_t saved_cycles = pmccntr_op_start(env);
-env->cp15.pmccfiltr_el0 = value & 0x7E00;
+env->cp15.pmccfiltr_el0 = value & 0xfc00;
 pmccntr_op_finish(env, saved_cycles);
 }
 
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 19/22] target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER

[Aaron Lindsay]

Add arrays to hold the registers, the definitions themselves, access
functions, and add logic to reset counters when PMCR.P is set.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/cpu.h|   7 +-
 target/arm/helper.c | 219 
 2 files changed, 207 insertions(+), 19 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 19f005d..7a74966 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -454,8 +454,9 @@ typedef struct CPUARMState {
 uint64_t oslsr_el1; /* OS Lock Status */
 uint64_t mdcr_el2;
 uint64_t mdcr_el3;
-/* If the counter is enabled, this stores the last time the counter
- * was reset. Otherwise it stores the counter value
+/* If the pmccntr and pmevcntr counters are enabled, they store the
+ * offset the last time the counter was reset. Otherwise they store the
+ * counter value.
  */
 uint64_t c15_ccnt;
 /* ccnt_cached_cycles is used to hold the last cycle count when
@@ -463,6 +464,8 @@ typedef struct CPUARMState {
  * PMU operations which require this.
  */
 uint64_t ccnt_cached_cycles;
+uint64_t c14_pmevcntr[31];
+uint64_t c14_pmevtyper[31];
 uint64_t pmccfiltr_el0; /* Performance Monitor Filter Register */
 uint64_t vpidr_el2; /* Virtualization Processor ID Register */
 uint64_t vmpidr_el2; /* Virtualization Multiprocessor ID Register */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 6a4f900..2fa8308 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -906,6 +906,7 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 #define PMCRN_SHIFT 11
 #define PMCRD   0x8
 #define PMCRC   0x4
+#define PMCRP   0x2
 #define PMCRE   0x1
 
 #define PMXEVTYPER_P  0x8000
@@ -931,7 +932,7 @@ typedef struct pm_event {
 bool (*supported)(CPUARMState *);
 /* Retrieve the current count of the underlying event. The programmed
  * counters hold a difference from the return value from this function */
-uint64_t (*get_count)(CPUARMState *);
+uint64_t (*get_count)(CPUARMState *, uint64_t cycles);
 } pm_event;
 
 #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
@@ -1054,6 +1055,21 @@ static inline bool pmu_counter_enabled(CPUARMState *env, 
uint8_t counter)
 return false;
 }
 
+if (counter != 31) {
+/* If not checking PMCCNTR, ensure the counter is setup to an event we
+ * support */
+uint16_t event = env->cp15.c14_pmevtyper[counter] & 
PMXEVTYPER_EVTCOUNT;
+if (event > 0x3f) {
+return false; /* We only support common architectural and
+ microarchitectural events */
+}
+
+uint16_t event_idx = supported_event_map[event];
+if (event_idx == SUPPORTED_EVENT_SENTINEL) {
+return false;
+}
+}
+
 return true;
 }
 
@@ -1149,14 +1165,37 @@ void pmccntr_op_finish(CPUARMState *env, uint64_t 
prev_cycles)
 }
 }
 
+static void pmu_sync_counter(CPUARMState *env, uint8_t counter, uint64_t 
cycles)
+{
+if (pmu_counter_enabled(env, counter) &&
+!pmu_counter_filtered(env, env->cp15.c14_pmevtyper[counter])) {
+
+uint16_t event = env->cp15.c14_pmevtyper[counter] & 
PMXEVTYPER_EVTCOUNT;
+uint16_t event_idx = supported_event_map[event];
+
+uint64_t count = pm_events[event_idx].get_count(env, cycles);
+env->cp15.c14_pmevcntr[counter] =
+count - env->cp15.c14_pmevcntr[counter];
+}
+}
+
 uint64_t pmu_op_start(CPUARMState *env)
 {
-return pmccntr_op_start(env);
+uint64_t saved_cycles = pmccntr_op_start(env);
+unsigned int i;
+for (i = 0; i < PMU_NUM_COUNTERS(env); i++) {
+pmu_sync_counter(env, i, saved_cycles);
+}
+return saved_cycles;
 }
 
 void pmu_op_finish(CPUARMState *env, uint64_t prev_cycles)
 {
 pmccntr_op_finish(env, prev_cycles);
+unsigned int i;
+for (i = 0; i < PMU_NUM_COUNTERS(env); i++) {
+pmu_sync_counter(env, i, prev_cycles);
+}
 }
 
 void pmu_pre_el_change(ARMCPU *cpu, void *ignored)
@@ -1179,6 +1218,13 @@ static void pmcr_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 env->cp15.c15_ccnt = 0;
 }
 
+if (value & PMCRP) {
+unsigned int i;
+for (i = 0; i < PMU_NUM_COUNTERS(env); i++) {
+env->cp15.c14_pmevcntr[i] = 0;
+}
+}
+
 /* only the DP, X, D and E bits are writable */
 env->cp15.c9_pmcr &= ~0x39;
 env->cp15.c9_pmcr |= (value & 0x39);
@@ -1294,30 +1340,127 @@ static void pmovsset_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 env->cp15.c9_pmovsr |= value;
 }
 
-static void pmxevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
- uint64_t value)
+static void pmevtyper_write(CPUARMState *env, const A

[Qemu-devel] [PATCH v3 10/22] target/arm: Allow EL change hooks to do IO

[Aaron Lindsay]

During code generation, surround CPSR writes and exception returns which
call the EL change hooks with gen_io_start/end. The immediate need is
for the PMU to access the clock and icount during EL change to support
mode filtering.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/translate-a64.c | 2 ++
 target/arm/translate.c | 4 
 2 files changed, 6 insertions(+)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 31ff047..e1ae676 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1919,7 +1919,9 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t 
insn)
 unallocated_encoding(s);
 return;
 }
+gen_io_start();
 gen_helper_exception_return(cpu_env);
+gen_io_end();
 /* Must exit loop to check un-masked IRQs */
 s->base.is_jmp = DISAS_EXIT;
 return;
diff --git a/target/arm/translate.c b/target/arm/translate.c
index ba6ab7d..fd5871e 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -4536,7 +4536,9 @@ static void gen_rfe(DisasContext *s, TCGv_i32 pc, 
TCGv_i32 cpsr)
  * appropriately depending on the new Thumb bit, so it must
  * be called after storing the new PC.
  */
+gen_io_start();
 gen_helper_cpsr_write_eret(cpu_env, cpsr);
+gen_io_end();
 tcg_temp_free_i32(cpsr);
 /* Must exit loop to check un-masked IRQs */
 s->base.is_jmp = DISAS_EXIT;
@@ -9828,7 +9830,9 @@ static void disas_arm_insn(DisasContext *s, unsigned int 
insn)
 if (exc_return) {
 /* Restore CPSR from SPSR.  */
 tmp = load_cpu_field(spsr);
+gen_io_start();
 gen_helper_cpsr_write_eret(cpu_env, tmp);
+gen_io_end();
 tcg_temp_free_i32(tmp);
 /* Must exit loop to check un-masked IRQs */
 s->base.is_jmp = DISAS_EXIT;
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 21/22] target/arm: PMU: Set PMCR.N to 4

[Aaron Lindsay]

This both advertises that we support four counters and adds them to the
implementation because the PMU_NUM_COUNTERS macro reads this value from
the PMCR.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/helper.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 679897a..06e2e2c 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1575,7 +1575,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
   .access = PL1_W, .type = ARM_CP_NOP },
 /* Performance monitors are implementation defined in v7,
  * but with an ARM recommended set of registers, which we
- * follow (although we don't actually implement any counters)
+ * follow.
  *
  * Performance registers fall into three categories:
  *  (a) always UNDEF in PL0, RW in PL1 (PMINTENSET, PMINTENCLR)
@@ -5192,7 +5192,8 @@ void register_cp_regs_for_features(ARMCPU *cpu)
 .access = PL0_RW, .accessfn = pmreg_access,
 .type = ARM_CP_IO,
 .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr),
-.resetvalue = cpu->midr & 0xff00,
+/* 4 counters enabled */
+.resetvalue = (cpu->midr & 0xff00) | (0x4 << PMCRN_SHIFT),
 .writefn = pmcr_write, .raw_writefn = raw_write,
 };
 define_one_arm_cp_reg(cpu, );
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 22/22] target/arm: Implement PMSWINC

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/helper.c | 44 ++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 06e2e2c..4f8d11c 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -955,6 +955,15 @@ static bool event_always_supported(CPUARMState *env)
 return true;
 }
 
+static uint64_t swinc_get_count(CPUARMState *env, uint64_t cycles)
+{
+/*
+ * SW_INCR events are written directly to the pmevcntr's by writes to
+ * PMSWINC, so there is no underlying count maintained by the PMU itself
+ */
+return 0;
+}
+
 #ifndef CONFIG_USER_ONLY
 static uint64_t cycles_get_count(CPUARMState *env, uint64_t cycles)
 {
@@ -974,6 +983,10 @@ static uint64_t instructions_get_count(CPUARMState *env, 
uint64_t cycles)
 
 #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
 static const pm_event pm_events[] = {
+{ .number = 0x000, /* SW_INCR */
+  .supported = event_always_supported,
+  .get_count = swinc_get_count
+},
 #ifndef CONFIG_USER_ONLY
 { .number = 0x008, /* INST_RETIRED */
   .supported = instructions_supported,
@@ -1273,6 +1286,29 @@ static void pmcr_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 pmu_op_finish(env, saved_cycles);
 }
 
+static void pmswinc_write(CPUARMState *env, const ARMCPRegInfo *ri,
+  uint64_t value)
+{
+unsigned int i;
+for (i = 0; i < PMU_NUM_COUNTERS(env); i++) {
+/* Increment a counter's count iff: */
+if ((value & (1 << i)) && /* counter's bit is set */
+/* counter is enabled and not filtered */
+pmu_counter_enabled(env, i) &&
+!pmu_counter_filtered(env, env->cp15.c14_pmevtyper[i]) &&
+/* counter is SW_INCR */
+(env->cp15.c14_pmevtyper[i] & PMXEVTYPER_EVTCOUNT) == 0x0) {
+uint64_t cycles = 0;
+#ifndef CONFIG_USER_ONLY
+cycles = get_cycle_count(env);
+#endif
+pmu_sync_counter(env, i, cycles);
+env->cp15.c14_pmevcntr[i]++;
+pmu_sync_counter(env, i, cycles);
+}
+}
+}
+
 static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
 uint64_t ret;
@@ -1619,9 +1655,13 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
   .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
   .writefn = pmovsr_write,
   .raw_writefn = raw_write },
-/* Unimplemented so WI. */
 { .name = "PMSWINC", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 4,
-  .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NOP },
+  .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NO_RAW,
+  .writefn = pmswinc_write },
+{ .name = "PMSWINC_EL0", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 4,
+  .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NO_RAW,
+  .writefn = pmswinc_write },
 { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
   .access = PL0_RW, .type = ARM_CP_ALIAS,
   .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmselr),
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 16/22] target/arm: Implement PMOVSSET

[Aaron Lindsay]

Adding an array for v7VE+ CP registers was necessary so that PMOVSSET
wasn't defined for all v7 processors.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/helper.c | 32 +++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index d4f06e6..f5e800e 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1241,9 +1241,17 @@ static void pmcntenclr_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 static void pmovsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
  uint64_t value)
 {
+value &= PMU_COUNTER_MASK(env);
 env->cp15.c9_pmovsr &= ~value;
 }
 
+static void pmovsset_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value)
+{
+value &= PMU_COUNTER_MASK(env);
+env->cp15.c9_pmovsr |= value;
+}
+
 static void pmxevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
  uint64_t value)
 {
@@ -1406,7 +1414,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
   .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
   .writefn = pmcntenclr_write },
 { .name = "PMOVSR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 3,
-  .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
+  .access = PL0_RW, .fieldoffset = offsetoflow32(CPUARMState, 
cp15.c9_pmovsr),
   .accessfn = pmreg_access,
   .writefn = pmovsr_write,
   .raw_writefn = raw_write },
@@ -1592,6 +1600,25 @@ static const ARMCPRegInfo v7mp_cp_reginfo[] = {
 REGINFO_SENTINEL
 };
 
+static const ARMCPRegInfo v7ve_cp_reginfo[] = {
+/* Performance monitor registers which are not implemented in v7 before
+ * v7ve:
+ */
+{ .name = "PMOVSSET", .cp = 15, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 3,
+  .access = PL0_RW, .accessfn = pmreg_access,
+  .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
+  .writefn = pmovsset_write,
+  .raw_writefn = raw_write },
+{ .name = "PMOVSSET_EL0", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 14, .opc2 = 3,
+  .access = PL0_RW, .accessfn = pmreg_access,
+  .type = ARM_CP_ALIAS,
+  .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
+  .writefn = pmovsset_write,
+  .raw_writefn = raw_write },
+REGINFO_SENTINEL
+};
+
 static void teecr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 uint64_t value)
 {
@@ -4943,6 +4970,9 @@ void register_cp_regs_for_features(ARMCPU *cpu)
 !arm_feature(env, ARM_FEATURE_PMSA)) {
 define_arm_cp_regs(cpu, v7mp_cp_reginfo);
 }
+if (arm_feature(env, ARM_FEATURE_V7VE)) {
+define_arm_cp_regs(cpu, v7ve_cp_reginfo);
+}
 if (arm_feature(env, ARM_FEATURE_V7)) {
 /* v7 performance monitor control register: same implementor
  * field as main ID register, and we implement only the cycle
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 09/22] target/arm: Add pre-EL change hooks

[Aaron Lindsay]

Because the design of the PMU requires that the counter values be
converted between their delta and guest-visible forms for mode
filtering, an additional hook which occurs before the EL is changed is
necessary.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/cpu.c   | 13 +
 target/arm/cpu.h   | 12 
 target/arm/helper.c| 14 --
 target/arm/internals.h |  7 +++
 target/arm/op_helper.c |  8 
 5 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 5f782bf..a2cb21e 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -55,6 +55,18 @@ static bool arm_cpu_has_work(CPUState *cs)
  | CPU_INTERRUPT_EXITTB);
 }
 
+void arm_register_pre_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
+ void *opaque)
+{
+ARMELChangeHook *entry;
+entry = g_malloc0(sizeof (*entry));
+
+entry->hook = hook;
+entry->opaque = opaque;
+
+QLIST_INSERT_HEAD(>pre_el_change_hooks, entry, node);
+}
+
 void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
  void *opaque)
 {
@@ -747,6 +759,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
**errp)
 return;
 }
 
+QLIST_INIT(>pre_el_change_hooks);
 QLIST_INIT(>el_change_hooks);
 
 /* Some features automatically imply others: */
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 3b45d3d..b0ef727 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -832,6 +832,7 @@ struct ARMCPU {
  */
 bool cfgend;
 
+QLIST_HEAD(, ARMELChangeHook) pre_el_change_hooks;
 QLIST_HEAD(, ARMELChangeHook) el_change_hooks;
 
 int32_t node_id; /* NUMA node this CPU belongs to */
@@ -2895,12 +2896,15 @@ static inline AddressSpace *arm_addressspace(CPUState 
*cs, MemTxAttrs attrs)
 #endif
 
 /**
+ * arm_register_pre_el_change_hook:
  * arm_register_el_change_hook:
- * Register a hook function which will be called back whenever this
- * CPU changes exception level or mode. The hook function will be
- * passed a pointer to the ARMCPU and the opaque data pointer passed
- * to this function when the hook was registered.
+ * Register a hook function which will be called back before or after this CPU
+ * changes exception level or mode. The hook function will be passed a pointer
+ * to the ARMCPU and the opaque data pointer passed to this function when the
+ * hook was registered.
  */
+void arm_register_pre_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
+ void *opaque);
 void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
  void *opaque);
 
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 5d5c738..50eaed7 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8253,6 +8253,14 @@ void arm_cpu_do_interrupt(CPUState *cs)
 return;
 }
 
+/* Hooks may change global state so BQL should be held, also the
+ * BQL needs to be held for any modification of
+ * cs->interrupt_request.
+ */
+g_assert(qemu_mutex_iothread_locked());
+
+arm_call_pre_el_change_hook(cpu);
+
 assert(!excp_is_internal(cs->exception_index));
 if (arm_el_is_aa64(env, new_el)) {
 arm_cpu_do_interrupt_aarch64(cs);
@@ -8260,12 +8268,6 @@ void arm_cpu_do_interrupt(CPUState *cs)
 arm_cpu_do_interrupt_aarch32(cs);
 }
 
-/* Hooks may change global state so BQL should be held, also the
- * BQL needs to be held for any modification of
- * cs->interrupt_request.
- */
-g_assert(qemu_mutex_iothread_locked());
-
 arm_call_el_change_hook(cpu);
 
 if (!kvm_enabled()) {
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 7df3eda..6ea6766 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -728,6 +728,13 @@ void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr 
physaddr,
MemTxResult response, uintptr_t retaddr);
 
 /* Call any registered EL change hooks */
+static inline void arm_call_pre_el_change_hook(ARMCPU *cpu)
+{
+ARMELChangeHook *hook, *next;
+QLIST_FOREACH_SAFE(hook, >pre_el_change_hooks, node, next) {
+hook->hook(cpu, hook->opaque);
+}
+}
 static inline void arm_call_el_change_hook(ARMCPU *cpu)
 {
 ARMELChangeHook *hook, *next;
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 7a88fd2..be417ce 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -496,6 +496,10 @@ void HELPER(cpsr_write)(CPUARMState *env, uint32_t val, 
uint32_t mask)
 /* Write the CPSR for a 32-bit exception return */
 void HELPER(cpsr_write_eret)(CPUARMState *env, uint32_t val)
 {
+qemu_mutex_lock_iothread();
+arm_call_pre_el_change_hook(arm_env_get_cpu(env));
+qemu_mutex_unlock_iothread();
+
 cpsr_write(env, val, C

[Qemu-devel] [PATCH v3 18/22] target/arm: Add array for supported PMU events, generate PMCEID[01]

[Aaron Lindsay]

This commit doesn't add any supported events, but provides the framework
for adding them. We store the pm_event structs in a simple array, and
provide the mapping from the event numbers to array indexes in
the supported_event_map array.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/cpu.c|  4 
 target/arm/cpu.h| 10 ++
 target/arm/helper.c | 37 +
 3 files changed, 51 insertions(+)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index e544f1d..69d6a80 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -889,6 +889,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
**errp)
 unset_feature(env, ARM_FEATURE_PMU);
 cpu->id_aa64dfr0 &= ~0xf00;
 } else {
+uint64_t pmceid = get_pmceid(>env);
+cpu->pmceid0 = pmceid & 0x;
+cpu->pmceid1 = (pmceid >> 32) & 0x;
+
 arm_register_pre_el_change_hook(cpu, _pre_el_change, 0);
 arm_register_el_change_hook(cpu, _post_el_change, 0);
 }
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index cc1e2fb..19f005d 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -931,6 +931,16 @@ void pmu_op_finish(CPUARMState *env, uint64_t prev_cycles);
 void pmu_pre_el_change(ARMCPU *cpu, void *ignored);
 void pmu_post_el_change(ARMCPU *cpu, void *ignored);
 
+/*
+ * get_pmceid
+ * @env: CPUARMState
+ *
+ * Return the PMCEID[01] register values corresponding to the counters which
+ * are supported given the current configuration (0 is low 32, 1 is high 32
+ * bits)
+ */
+uint64_t get_pmceid(CPUARMState *env);
+
 /* SCTLR bit meanings. Several bits have been reused in newer
  * versions of the architecture; in that case we define constants
  * for both old and new bit meanings. Code which tests against those
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 2073d56..6a4f900 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -925,6 +925,43 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 /* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */
 #define PMU_COUNTER_MASK(env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 1))
 
+typedef struct pm_event {
+uint16_t number; /* PMEVTYPER.evtCount is 10 bits wide */
+/* If the event is supported on this CPU (used to generate PMCEID[01]) */
+bool (*supported)(CPUARMState *);
+/* Retrieve the current count of the underlying event. The programmed
+ * counters hold a difference from the return value from this function */
+uint64_t (*get_count)(CPUARMState *);
+} pm_event;
+
+#define SUPPORTED_EVENT_SENTINEL UINT16_MAX
+static const pm_event pm_events[] = {
+{ .number = SUPPORTED_EVENT_SENTINEL }
+};
+static uint16_t supported_event_map[0x3f];
+
+/*
+ * Called upon initialization to build PMCEID0 (low 32 bits) and PMCEID1 (high
+ * 32). We also use it to build a map of ARM event numbers to indices in
+ * our pm_events array.
+ */
+uint64_t get_pmceid(CPUARMState *env)
+{
+uint64_t pmceid = 0;
+unsigned int i = 0;
+while (pm_events[i].number != SUPPORTED_EVENT_SENTINEL) {
+const pm_event *cnt = _events[i];
+if (cnt->number < 0x3f && cnt->supported(env)) {
+pmceid |= (1 << cnt->number);
+supported_event_map[cnt->number] = i;
+} else {
+supported_event_map[cnt->number] = SUPPORTED_EVENT_SENTINEL;
+}
+i++;
+}
+return pmceid;
+}
+
 static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
bool isread)
 {
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 17/22] target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
---
 target/arm/helper.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index f5e800e..2073d56 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1009,17 +1009,22 @@ static CPAccessResult pmreg_access_ccntr(CPUARMState 
*env,
 return pmreg_access(env, ri, isread);
 }
 
-static inline bool arm_ccnt_enabled(CPUARMState *env)
+static inline bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)
 {
 /* Does not check PMCCFILTR_EL0, which is handled by pmu_counter_filtered 
*/
-
-if (!(env->cp15.c9_pmcr & PMCRE) || !(env->cp15.c9_pmcnten & (1 << 31))) {
+if (!(env->cp15.c9_pmcr & PMCRE) ||
+!(env->cp15.c9_pmcnten & (1 << counter))) {
 return false;
 }
 
 return true;
 }
 
+static inline bool arm_ccnt_enabled(CPUARMState *env)
+{
+return pmu_counter_enabled(env, 31);
+}
+
 /* Returns true if the counter corresponding to the passed-in pmevtyper or
  * pmccfiltr value is filtered using the current state */
 static inline bool pmu_counter_filtered(CPUARMState *env, uint64_t pmxevtyper)
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 06/22] target/arm: Mask PMU register writes based on PMCR_EL0.N

[Aaron Lindsay]

This is in preparation for enabling counters other than PMCCNTR

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/helper.c | 24 +++-
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 6480b80..5d5c738 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -52,11 +52,6 @@ typedef struct V8M_SAttributes {
 static void v8m_security_lookup(CPUARMState *env, uint32_t address,
 MMUAccessType access_type, ARMMMUIdx mmu_idx,
 V8M_SAttributes *sattrs);
-
-/* Definitions for the PMCCNTR and PMCR registers */
-#define PMCRD   0x8
-#define PMCRC   0x4
-#define PMCRE   0x1
 #endif
 
 static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
@@ -906,6 +901,17 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 REGINFO_SENTINEL
 };
 
+/* Definitions for the PMU registers */
+#define PMCRN_MASK  0xf800
+#define PMCRN_SHIFT 11
+#define PMCRD   0x8
+#define PMCRC   0x4
+#define PMCRE   0x1
+
+#define PMU_NUM_COUNTERS(env) ((env->cp15.c9_pmcr & PMCRN_MASK) >> PMCRN_SHIFT)
+/* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */
+#define PMU_COUNTER_MASK(env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 1))
+
 static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
bool isread)
 {
@@ -1124,14 +1130,14 @@ static void pmccfiltr_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
 uint64_t value)
 {
-value &= (1 << 31);
+value &= PMU_COUNTER_MASK(env);
 env->cp15.c9_pmcnten |= value;
 }
 
 static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
  uint64_t value)
 {
-value &= (1 << 31);
+value &= PMU_COUNTER_MASK(env);
 env->cp15.c9_pmcnten &= ~value;
 }
 
@@ -1179,14 +1185,14 @@ static void pmintenset_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
  uint64_t value)
 {
 /* We have no event counters so only the C bit can be changed */
-value &= (1 << 31);
+value &= PMU_COUNTER_MASK(env);
 env->cp15.c9_pminten |= value;
 }
 
 static void pmintenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
  uint64_t value)
 {
-value &= (1 << 31);
+value &= PMU_COUNTER_MASK(env);
 env->cp15.c9_pminten &= ~value;
 }
 
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 08/22] target/arm: Support multiple EL change hooks

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/cpu.c   | 15 ++-
 target/arm/cpu.h   | 23 ---
 target/arm/internals.h |  7 ---
 3 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 072cbbf..5f782bf 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -55,13 +55,16 @@ static bool arm_cpu_has_work(CPUState *cs)
  | CPU_INTERRUPT_EXITTB);
 }
 
-void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook,
+void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
  void *opaque)
 {
-/* We currently only support registering a single hook function */
-assert(!cpu->el_change_hook);
-cpu->el_change_hook = hook;
-cpu->el_change_hook_opaque = opaque;
+ARMELChangeHook *entry;
+entry = g_malloc0(sizeof (*entry));
+
+entry->hook = hook;
+entry->opaque = opaque;
+
+QLIST_INSERT_HEAD(>el_change_hooks, entry, node);
 }
 
 static void cp_reg_reset(gpointer key, gpointer value, gpointer opaque)
@@ -744,6 +747,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
**errp)
 return;
 }
 
+QLIST_INIT(>el_change_hooks);
+
 /* Some features automatically imply others: */
 if (arm_feature(env, ARM_FEATURE_V8)) {
 set_feature(env, ARM_FEATURE_V7);
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index f17592b..3b45d3d 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -633,11 +633,17 @@ typedef struct CPUARMState {
 
 /**
  * ARMELChangeHook:
- * type of a function which can be registered via arm_register_el_change_hook()
- * to get callbacks when the CPU changes its exception level or mode.
+ * Support registering functions with ARMELChangeHookFn's signature via
+ * arm_register_el_change_hook() to get callbacks when the CPU changes its
+ * exception level or mode.
  */
-typedef void ARMELChangeHook(ARMCPU *cpu, void *opaque);
-
+typedef void ARMELChangeHookFn(ARMCPU *cpu, void *opaque);
+typedef struct ARMELChangeHook ARMELChangeHook;
+struct ARMELChangeHook {
+ARMELChangeHookFn *hook;
+void *opaque;
+QLIST_ENTRY(ARMELChangeHook) node;
+};
 
 /* These values map onto the return values for
  * QEMU_PSCI_0_2_FN_AFFINITY_INFO */
@@ -826,8 +832,7 @@ struct ARMCPU {
  */
 bool cfgend;
 
-ARMELChangeHook *el_change_hook;
-void *el_change_hook_opaque;
+QLIST_HEAD(, ARMELChangeHook) el_change_hooks;
 
 int32_t node_id; /* NUMA node this CPU belongs to */
 
@@ -2895,12 +2900,8 @@ static inline AddressSpace *arm_addressspace(CPUState 
*cs, MemTxAttrs attrs)
  * CPU changes exception level or mode. The hook function will be
  * passed a pointer to the ARMCPU and the opaque data pointer passed
  * to this function when the hook was registered.
- *
- * Note that we currently only support registering a single hook function,
- * and will assert if this function is called twice.
- * This facility is intended for the use of the GICv3 emulation.
  */
-void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHook *hook,
+void arm_register_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
  void *opaque);
 
 /**
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 47cc224..7df3eda 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -727,11 +727,12 @@ void arm_cpu_do_transaction_failed(CPUState *cs, hwaddr 
physaddr,
int mmu_idx, MemTxAttrs attrs,
MemTxResult response, uintptr_t retaddr);
 
-/* Call the EL change hook if one has been registered */
+/* Call any registered EL change hooks */
 static inline void arm_call_el_change_hook(ARMCPU *cpu)
 {
-if (cpu->el_change_hook) {
-cpu->el_change_hook(cpu, cpu->el_change_hook_opaque);
+ARMELChangeHook *hook, *next;
+QLIST_FOREACH_SAFE(hook, >el_change_hooks, node, next) {
+hook->hook(cpu, hook->opaque);
 }
 }
 
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 07/22] target/arm: Fetch GICv3 state directly from CPUARMState

[Aaron Lindsay]

This eliminates the need for fetching it from el_change_hook_opaque, and
allows for supporting multiple el_change_hooks without having to hack
something together to find the registered opaque belonging to GICv3.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 hw/intc/arm_gicv3_cpuif.c | 10 ++
 target/arm/cpu.h  | 10 --
 2 files changed, 2 insertions(+), 18 deletions(-)

diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 5cbafaf..801f91b 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -29,11 +29,7 @@ void gicv3_set_gicv3state(CPUState *cpu, GICv3CPUState *s)
 
 static GICv3CPUState *icc_cs_from_env(CPUARMState *env)
 {
-/* Given the CPU, find the right GICv3CPUState struct.
- * Since we registered the CPU interface with the EL change hook as
- * the opaque pointer, we can just directly get from the CPU to it.
- */
-return arm_get_el_change_hook_opaque(arm_env_get_cpu(env));
+return env->gicv3state;
 }
 
 static bool gicv3_use_ns_bank(CPUARMState *env)
@@ -2615,9 +2611,7 @@ void gicv3_init_cpuif(GICv3State *s)
  * it might be with code translated by CPU 0 but run by CPU 1, in
  * which case we'd get the wrong value.
  * So instead we define the regs with no ri->opaque info, and
- * get back to the GICv3CPUState from the ARMCPU by reading back
- * the opaque pointer from the el_change_hook, which we're going
- * to need to register anyway.
+ * get back to the GICv3CPUState from the CPUARMState.
  */
 define_arm_cp_regs(cpu, gicv3_cpuif_reginfo);
 if (arm_feature(>env, ARM_FEATURE_EL2)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 1e7e1f8..f17592b 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2904,16 +2904,6 @@ void arm_register_el_change_hook(ARMCPU *cpu, 
ARMELChangeHook *hook,
  void *opaque);
 
 /**
- * arm_get_el_change_hook_opaque:
- * Return the opaque data that will be used by the el_change_hook
- * for this CPU.
- */
-static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
-{
-return cpu->el_change_hook_opaque;
-}
-
-/**
  * aa32_vfp_dreg:
  * Return a pointer to the Dn register within env in 32-bit mode.
  */
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 13/22] target/arm: Allow AArch32 access for PMCCFILTR

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/helper.c | 27 ++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 95b09d6..d4f06e6 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -917,6 +917,10 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 #define PMXEVTYPER_MT 0x0200
 #define PMXEVTYPER_EVTCOUNT   0x03ff
 
+#define PMCCFILTR 0xf800
+#define PMCCFILTR_M   PMXEVTYPER_M
+#define PMCCFILTR_EL0 (PMCCFILTR | PMCCFILTR_M)
+
 #define PMU_NUM_COUNTERS(env) ((env->cp15.c9_pmcr & PMCRN_MASK) >> PMCRN_SHIFT)
 /* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */
 #define PMU_COUNTER_MASK(env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 1))
@@ -1200,10 +1204,26 @@ static void pmccfiltr_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 uint64_t value)
 {
 uint64_t saved_cycles = pmccntr_op_start(env);
-env->cp15.pmccfiltr_el0 = value & 0xfc00;
+env->cp15.pmccfiltr_el0 = value & PMCCFILTR_EL0;
+pmccntr_op_finish(env, saved_cycles);
+}
+
+static void pmccfiltr_write_a32(CPUARMState *env, const ARMCPRegInfo *ri,
+uint64_t value)
+{
+uint64_t saved_cycles = pmccntr_op_start(env);
+/* M is not accessible from AArch32 */
+env->cp15.pmccfiltr_el0 = (env->cp15.pmccfiltr_el0 & PMCCFILTR_M) |
+(value & PMCCFILTR);
 pmccntr_op_finish(env, saved_cycles);
 }
 
+static uint64_t pmccfiltr_read_a32(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+/* M is not visible in AArch32 */
+return env->cp15.pmccfiltr_el0 & PMCCFILTR;
+}
+
 static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
 uint64_t value)
 {
@@ -1421,6 +1441,11 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
   .type = ARM_CP_IO,
   .readfn = pmccntr_read, .writefn = pmccntr_write, },
 #endif
+{ .name = "PMCCFILTR", .cp = 15, .opc1 = 0, .crn = 14, .crm = 15, .opc2 = 
7,
+  .writefn = pmccfiltr_write_a32, .readfn = pmccfiltr_read_a32,
+  .access = PL0_RW, .accessfn = pmreg_access,
+  .type = ARM_CP_ALIAS | ARM_CP_IO,
+  .resetvalue = 0, },
 { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64,
   .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7,
   .writefn = pmccfiltr_write,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 14/22] target/arm: Make PMOVSCLR 64 bits wide

[Aaron Lindsay]

This is a bug fix to ensure 64-bit reads of this register don't read
adjacent data.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/cpu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9c3b5ef..fb2f983 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -367,7 +367,7 @@ typedef struct CPUARMState {
 uint32_t c9_data;
 uint64_t c9_pmcr; /* performance monitor control register */
 uint64_t c9_pmcnten; /* perf monitor counter enables */
-uint32_t c9_pmovsr; /* perf monitor overflow status */
+uint64_t c9_pmovsr; /* perf monitor overflow status */
 uint32_t c9_pmuserenr; /* perf monitor user enable */
 uint64_t c9_pmselr; /* perf monitor counter selection register */
 uint64_t c9_pminten; /* perf monitor interrupt enables */
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 05/22] target/arm: Reorganize PMCCNTR read, write, sync

[Aaron Lindsay]

pmccntr_read and pmccntr_write contained duplicate code that was already
being handled by pmccntr_sync. Split pmccntr_sync into pmccntr_op_start
and pmccntr_op_finish, passing the clock value between the two, to avoid
losing time between the two calls.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/helper.c | 101 +---
 1 file changed, 56 insertions(+), 45 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 5634561..6480b80 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1000,28 +1000,58 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
 
 return true;
 }
-
-void pmccntr_sync(CPUARMState *env)
+/*
+ * Ensure c15_ccnt is the guest-visible count so that operations such as
+ * enabling/disabling the counter or filtering, modifying the count itself,
+ * etc. can be done logically. This is essentially a no-op if the counter is
+ * not enabled at the time of the call.
+ *
+ * The current cycle count is returned so that it can be passed into the paired
+ * pmccntr_op_finish() call which must follow each call to pmccntr_op_start().
+ */
+uint64_t pmccntr_op_start(CPUARMState *env)
 {
-uint64_t temp_ticks;
-
-temp_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+uint64_t cycles = 0;
+#ifndef CONFIG_USER_ONLY
+cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
   ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
+#endif
+
+if (arm_ccnt_enabled(env)) {
 
-if (env->cp15.c9_pmcr & PMCRD) {
-/* Increment once every 64 processor clock cycles */
-temp_ticks /= 64;
+uint64_t eff_cycles = cycles;
+if (env->cp15.c9_pmcr & PMCRD) {
+/* Increment once every 64 processor clock cycles */
+eff_cycles /= 64;
+}
+
+env->cp15.c15_ccnt = eff_cycles - env->cp15.c15_ccnt;
 }
+return cycles;
+}
 
+/*
+ * If enabled, convert c15_ccnt back into the delta between the clock and the
+ * guest-visible count. A call to pmccntr_op_finish should follow every call to
+ * pmccntr_op_start.
+ */
+void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
+{
 if (arm_ccnt_enabled(env)) {
-env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
+
+if (env->cp15.c9_pmcr & PMCRD) {
+/* Increment once every 64 processor clock cycles */
+prev_cycles /= 64;
+}
+
+env->cp15.c15_ccnt = prev_cycles - env->cp15.c15_ccnt;
 }
 }
 
 static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
uint64_t value)
 {
-pmccntr_sync(env);
+uint64_t saved_cycles = pmccntr_op_start(env);
 
 if (value & PMCRC) {
 /* The counter has been reset */
@@ -1032,26 +1062,16 @@ static void pmcr_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 env->cp15.c9_pmcr &= ~0x39;
 env->cp15.c9_pmcr |= (value & 0x39);
 
-pmccntr_sync(env);
+pmccntr_op_finish(env, saved_cycles);
 }
 
 static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
-uint64_t total_ticks;
-
-if (!arm_ccnt_enabled(env)) {
-/* Counter is disabled, do not change value */
-return env->cp15.c15_ccnt;
-}
-
-total_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-   ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
-
-if (env->cp15.c9_pmcr & PMCRD) {
-/* Increment once every 64 processor clock cycles */
-total_ticks /= 64;
-}
-return total_ticks - env->cp15.c15_ccnt;
+uint64_t ret;
+uint64_t saved_cycles = pmccntr_op_start(env);
+ret = env->cp15.c15_ccnt;
+pmccntr_op_finish(env, saved_cycles);
+return ret;
 }
 
 static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1068,22 +1088,9 @@ static void pmselr_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 uint64_t value)
 {
-uint64_t total_ticks;
-
-if (!arm_ccnt_enabled(env)) {
-/* Counter is disabled, set the absolute value */
-env->cp15.c15_ccnt = value;
-return;
-}
-
-total_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-   ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
-
-if (env->cp15.c9_pmcr & PMCRD) {
-/* Increment once every 64 processor clock cycles */
-total_ticks /= 64;
-}
-env->cp15.c15_ccnt = total_ticks - value;
+uint64_t saved_cycles = pmccntr_op_start(env);
+env->cp15.c15_ccnt = value;
+pmccntr_op_finish(env, saved_cycles);
 }
 
 static void pmccntr_write32(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1096,7 +1103,11 @@ static void pmccntr_write32(CPUARMState *env, const 
ARMCPRegInfo *ri,
 
 #else /* CONFIG_USER_ONLY */
 
-void pmccntr_sync(CPUARMState *env

[Qemu-devel] [PATCH v3 04/22] target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0

[Aaron Lindsay]

They share the same underlying state

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 5e48982..5634561 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1318,7 +1318,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
   .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr),
   .writefn = pmselr_write, .raw_writefn = raw_write, },
 { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
-  .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
+  .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_ALIAS | ARM_CP_IO,
   .readfn = pmccntr_read, .writefn = pmccntr_write32,
   .accessfn = pmreg_access_ccntr },
 { .name = "PMCCNTR_EL0", .state = ARM_CP_STATE_AA64,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 02/22] target/arm: A15 PMCEID0 initialization style nit

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 022d8c5..072cbbf 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1524,7 +1524,7 @@ static void cortex_a15_initfn(Object *obj)
 cpu->id_pfr0 = 0x1131;
 cpu->id_pfr1 = 0x00011011;
 cpu->id_dfr0 = 0x02010555;
-cpu->pmceid0 = 0x000;
+cpu->pmceid0 = 0x;
 cpu->pmceid1 = 0x;
 cpu->id_afr0 = 0x;
 cpu->id_mmfr0 = 0x10201105;
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 01/22] target/arm: A53: Initialize PMCEID[01]

[Aaron Lindsay]

A53 advertises ARM_FEATURE_PMU, but wasn't initializing pmceid[01].
pmceid[01] are already being initialized to zero for both A15 and A57.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/cpu64.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 991d764..8c4db31 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -201,6 +201,8 @@ static void aarch64_a53_initfn(Object *obj)
 cpu->id_isar5 = 0x00011121;
 cpu->id_aa64pfr0 = 0x;
 cpu->id_aa64dfr0 = 0x10305106;
+cpu->pmceid0 = 0x;
+cpu->pmceid1 = 0x;
 cpu->id_aa64isar0 = 0x00011120;
 cpu->id_aa64mmfr0 = 0x1122; /* 40 bit physical addr */
 cpu->dbgdidr = 0x3516d000;
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 03/22] target/arm: Check PMCNTEN for whether PMCCNTR is enabled

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 09893e3..5e48982 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -994,7 +994,7 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
 {
 /* This does not support checking PMCCFILTR_EL0 register */
 
-if (!(env->cp15.c9_pmcr & PMCRE)) {
+if (!(env->cp15.c9_pmcr & PMCRE) || !(env->cp15.c9_pmcnten & (1 << 31))) {
 return false;
 }
 
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v3 00/22] More fully implement ARM PMUv3

[Aaron Lindsay]

The ARM PMU implementation currently contains a basic cycle counter, but it is
often useful to gather counts of other events and filter them based on
execution mode. These patches flesh out the implementations of various PMU
registers including PM[X]EVCNTR and PM[X]EVTYPER, add a struct definition to
represent arbitrary counter types, implement mode filtering, and add
instruction, cycle, and software increment events.

I aim to eventually add raising interrupts on counter overflow, but that is not
covered by this patchset. I think I have a reasonable grasp of the mechanics of
*how* to raise them, but am curious if anyone has thoughts on how to determine
*when* to raise them - we don't want to call into PMU code every time an
instruction is executed to check if any instruction counters have overflowed,
etc. The main candidate I've seen for doing this so far would be to set up a
QEMUTimer, but I haven't fully explored it. Does that seem plausible? Any
other/better ideas?


Changes from v2:

* Many minor fixups (splitting patches, style/comment fixes, etc.)
* Save off current cycle count during operations which may change whether a
  counter is enabled, ensuring time isn't lost (update to patch 5)
* Added the ability to have more than one el_change_hook, added hooks before EL
  changes (patches 7-9)
* Added proper handling of can_do_io during code-gen before and after the
  el_change_hooks (patch 8)
* Split off PMOVSSET register definitions so they are only enabled for v7ve+
  (added patch 15, update to 16)

Thanks for any feedback,
Aaron
 

Aaron Lindsay (22):
  target/arm: A53: Initialize PMCEID[01]
  target/arm: A15 PMCEID0 initialization style nit
  target/arm: Check PMCNTEN for whether PMCCNTR is enabled
  target/arm: Treat PMCCNTR as alias of PMCCNTR_EL0
  target/arm: Reorganize PMCCNTR read, write, sync
  target/arm: Mask PMU register writes based on PMCR_EL0.N
  target/arm: Fetch GICv3 state directly from CPUARMState
  target/arm: Support multiple EL change hooks
  target/arm: Add pre-EL change hooks
  target/arm: Allow EL change hooks to do IO
  target/arm: Fix bitmask for PMCCFILTR writes
  target/arm: Filter cycle counter based on PMCCFILTR_EL0
  target/arm: Allow AArch32 access for PMCCFILTR
  target/arm: Make PMOVSCLR 64 bits wide
  target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions
  target/arm: Implement PMOVSSET
  target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled
  target/arm: Add array for supported PMU events, generate PMCEID[01]
  target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER
  target/arm: PMU: Add instruction and cycle events
  target/arm: PMU: Set PMCR.N to 4
  target/arm: Implement PMSWINC

 hw/intc/arm_gicv3_cpuif.c  |  10 +-
 target/arm/cpu.c   |  40 ++-
 target/arm/cpu.h   | 102 +---
 target/arm/cpu64.c |   2 +
 target/arm/helper.c| 616 ++---
 target/arm/internals.h |  14 +-
 target/arm/op_helper.c |   8 +
 target/arm/translate-a64.c |   2 +
 target/arm/translate.c |   4 +
 9 files changed, 651 insertions(+), 147 deletions(-)

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH] build: Don't force preserving permissions on config-devices.mak.old

[Aaron Lindsay]

I get the following error when building on an NFSv3 filesystem:

% make -j8
  GEN aarch64-softmmu/config-devices.mak.tmp
  GEN config-host.h
[snip]
  GEN qmp-marshal.c
  GEN aarch64-softmmu/config-devices.mak
cp: preserving permissions for ‘aarch64-softmmu/config-devices.mak.old’: 
Operation not supported
make: *** Deleting file `aarch64-softmmu/config-devices.mak'
  GEN qapi-types.c
[snip]
  CC  scsi/qemu-pr-helper.o
make: *** No rule to make target `config-all-devices.mak', needed by 
`subdir-aarch64-softmmu'.  Stop.
make: *** Waiting for unfinished jobs

Ideally you would only build on a filesystem with proper support, but I haven't
been able to find a reason why preserving exact permissions is important in
this case.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 9372742..952b6df 100644
--- a/Makefile
+++ b/Makefile
@@ -287,7 +287,7 @@ endif
$(call quiet-command, if test -f $@; then \
  if cmp -s $@.old $@; then \
mv $@.tmp $@; \
-   cp -p $@ $@.old; \
+   cp $@ $@.old; \
  else \
if test -f $@.old; then \
  echo "WARNING: $@ (user modified) out of date.";\
@@ -299,7 +299,7 @@ endif
  fi; \
 else \
  mv $@.tmp $@; \
- cp -p $@ $@.old; \
+ cp $@ $@.old; \
 fi,"GEN","$@");
 
 defconfig:
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH] build: Fix dtc-checkout race condition in Makefile

[Aaron Lindsay]

This was introduced by:
commit aef45d51d1204f3335fb99de6658e0c5612c2b67
Author: Daniel P. Berrange <berra...@redhat.com>
Date:   Fri Sep 29 11:11:56 2017 +0100

build: automatically handle GIT submodule checkout for dtc

On my system, I see the following with a fresh clone:

% ./configure --disable-gtk --target-list=aarch64-softmmu
% make -j8
  GEN aarch64-softmmu/config-devices.mak.tmp
  GEN config-host.h
mkdir -p dtc/libfdt
  GIT ui/keycodemapdb dtc
mkdir -p dtc/tests
  GEN qemu-options.def
[snip]
  GEN migration/trace.h
make: *** [git-submodule-update] Error 1
make: *** Waiting for unfinished jobs

Upon closer inspection, the root cause of the error is:

% git submodule update --init ui/keycodemapdb dtc
fatal: destination path 'dtc' already exists and is not an empty directory.
Clone of 'git://git.qemu-project.org/dtc.git' into submodule path 'dtc' failed

This patch fixes this race condition by forcing the 'dtc/%' rule which caused
'dtc' to be non-empty to wait on '.git-submodule-status'.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 90f91e5..ffa82e8 100644
--- a/Makefile
+++ b/Makefile
@@ -380,7 +380,7 @@ DTC_CPPFLAGS=-I$(BUILD_DIR)/dtc -I$(SRC_PATH)/dtc 
-I$(SRC_PATH)/dtc/libfdt
 subdir-dtc: .git-submodule-status dtc/libfdt dtc/tests
$(call quiet-command,$(MAKE) $(DTC_MAKE_ARGS) 
CPPFLAGS="$(DTC_CPPFLAGS)" CFLAGS="$(DTC_CFLAGS)" LDFLAGS="$(LDFLAGS)" 
ARFLAGS="$(ARFLAGS)" CC="$(CC)" AR="$(AR)" LD="$(LD)" $(SUBDIR_MAKEFLAGS) 
libfdt/libfdt.a,)
 
-dtc/%:
+dtc/%: .git-submodule-status
mkdir -p $@
 
 $(SUBDIR_RULES): libqemuutil.a $(common-obj-y) $(chardev-obj-y) \
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v2 00/13] More fully implement ARM PMUv3

[Aaron Lindsay]

On Oct 17 16:09, Peter Maydell wrote:
> On 30 September 2017 at 03:08, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > The ARM PMU implementation currently contains a basic cycle counter, but it 
> > is
> > often useful to gather counts of other events and filter them based on
> > execution mode. These patches flesh out the implementations of various PMU
> > registers including PM[X]EVCNTR and PM[X]EVTYPER, add a struct definition to
> > represent arbitrary counter types, implement mode filtering, and add
> > instruction, cycle, and software increment events.
> >
> > I am particularly interested in feedback on the following two patches 
> > because I
> > think I'm likely Doing It Wrong:
> >  [1] target/arm: Filter cycle counter based on PMCCFILTR_EL0
> >  [2] target/arm: PMU: Add instruction and cycle events
> >
> > In order to implement mode filtering in an event-driven way, [1] adds a 
> > pair of
> > calls to pmu_sync() surrounding every update to a register/variable which 
> > may
> > affect whether any counter is currently filtered. These pmu_sync() calls
> > ultimately call cpu_get_icount_raw() for enabled instruction and cycle 
> > counters
> > when using icount. Unfortunately, cpu->can_do_io may otherwise be zero for
> > these calls so the current implementation in [2] temporarily sets can_do_io 
> > to
> > 1. I haven't see any ill side effects from this in my testing, but it 
> > doesn't
> > seem like the right way to handle this.
> 
> I've now reviewed the early stuff and provided what I hope is
> a useful direction for the mode-filtering, so I'm not going to
> look at the patches at the tail end on this version of the series.

Thank you, your review has been very helpful - the next pass of the
mode-filtering patch will be more maintainable.

> > I would like to eventually add sending interrupts on counter overflow.
> > Suggestions for the best direction to handle this are most welcome.
> 
> Check out how the helper.c timer interrupts are wired up
> (the CPU object exposes outbound irq lines, which then get
> wired up by the board to the GIC.)

Thanks for the pointer - I'll take a look.

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH 06/13] target/arm: Filter cycle counter based on PMCCFILTR_EL0

[Aaron Lindsay]

On Oct 17 15:57, Peter Maydell wrote:
> On 30 September 2017 at 03:08, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > The pmu_counter_filtered and pmu_sync functions are generic (as opposed
> > to PMCCNTR-specific) to allow for the implementation of other events.
> >
> > RFC: I know that many of the locations of the calls to pmu_sync are
> > problematic when icount is enabled because can_do_io will not be set.
> > The documentation says that for deterministic execution, IO must only be
> > performed by the last instruction of a thread block.
> 
> Yes. You need to arrange that gen_io_start() and gen_io_end()
> are called around the generation of code for operations that might
> do IO or care about the state of the clock, and that we end the TB
> after gen_io_end().
> 
> > Because
> > cpu_handle_interrupt() and cpu_handle_exception() are actually made
> > outside of a thread block, is it safe to set can_do_io=1 for them to
> > allow this to succeed? Is there a better mechanism for handling this?
> 
> From my reading of the code, can_do_io should already be 1
> when these functions are called. It's only set to 0 just
> before we call tcg_qemu_tb_exec() and then set back to 1
> immediately after (see cpu_tb_exec()).
> 
> In general, the approach you have here looks like it's going to
> be pretty invasive and also hard to keep working right. I think
> we can come up with something a bit better.

Yes, I am hoping so.

> Specifically, the filtering criteria effectively only change
> when we change exception level, right? (since you can only
> change security state as part of an exception level change).
> We already have a mechanism for getting a callback when the EL
> changes -- arm_register_el_change_hook(). (We would need to
> upgrade it to support more than one hook function.)
> 
> You still need to get the io-count handling right, but there
> are many fewer places that need to change (just the codegen
> for calls to exception-return helpers, I think) and they already
> end the TB, so you don't have the complexity of trying to end the
> TB in places you didn't before, you just need the gen_io_start/end.

Using hooks/callbacks is clearly a better solution. I shied away from
using *el_change_hook in this patchset because A) it seemed to be marked
explicitly for GICv3 emulation, B) the one-hook limitation you
mentioned, and C) it doesn't currently provide you with which EL you're
coming from.

If everyone is amenable to changing how the hook is structured, I don't
think any of those reasons stand. My initial thought is that the best
way to fix C) is to add a pre_el_change_hook to be called at the top of
the exception_return and cpsr_write_eret helpers in
target/arm/op_helper.c to drive the first call to pmu_sync() (or
whatever I rename the first half of that pair to based on your
suggestions on that patch).

If I don't receive any additional feedback before then, I'll do my best
to adapt the existing hooks to allow for a cleaner approach to filtering
for v3.

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH 07/13] target/arm: Implement PMOVSSET

[Aaron Lindsay]

On Oct 17 15:19, Peter Maydell wrote:
> On 30 September 2017 at 03:08, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > Also modify it to be stored as a uint64_t
> >
> > Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
> > ---
> >  target/arm/cpu.h|  2 +-
> >  target/arm/helper.c | 27 ---
> >  2 files changed, 25 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 811b1fe..365a809 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -325,7 +325,7 @@ typedef struct CPUARMState {
> >  uint32_t c9_data;
> >  uint64_t c9_pmcr; /* performance monitor control register */
> >  uint64_t c9_pmcnten; /* perf monitor counter enables */
> > -uint32_t c9_pmovsr; /* perf monitor overflow status */
> > +uint64_t c9_pmovsr; /* perf monitor overflow status */
> 
> This is a bug fix, so it should go in its own patch. Specifically,
> we already have an AArch64 sysreg PMOVSCLR_EL0 which has
>   .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
> so we should have made the CPUARMState field 64 bits when we
> added it. (I think without this bugfix reads of the AArch64
> reg will return data from the adjoining field in the struct.)

Okay, I'll split it off in v3.

> 
> >  uint32_t c9_pmuserenr; /* perf monitor user enable */
> >  uint64_t c9_pmselr; /* perf monitor counter selection register */
> >  uint64_t c9_pminten; /* perf monitor interrupt enables */
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index 74e90c5..3932ac0 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -1150,9 +1150,17 @@ static void pmcntenclr_write(CPUARMState *env, const 
> > ARMCPRegInfo *ri,
> >  static void pmovsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> >   uint64_t value)
> >  {
> > +value &= PMU_COUNTER_MASK(env);
> >  env->cp15.c9_pmovsr &= ~value;
> >  }
> >
> > +static void pmovsset_write(CPUARMState *env, const ARMCPRegInfo *ri,
> > + uint64_t value)
> > +{
> > +value &= PMU_COUNTER_MASK(env);
> > +env->cp15.c9_pmovsr |= value;
> > +}
> > +
> >  static void pmxevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
> >   uint64_t value)
> >  {
> > @@ -1317,10 +1325,10 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
> >.fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
> >.writefn = pmcntenclr_write },
> >  { .name = "PMOVSR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 
> > 3,
> > -  .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, 
> > cp15.c9_pmovsr),
> > -  .accessfn = pmreg_access,
> > +  .access = PL0_RW, .accessfn = pmreg_access,
> > +  .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
> 
> Is this just reshuffling the order of .field initializers?

Also updated offsetof -> offsetoflow32. IIRC, I shuffled the order to
match that of the surrounding registers and to appease my sense of
order. Would you prefer that I don't shuffle them, or split that off as
a separate patch?

> >.writefn = pmovsr_write,
> > -  .raw_writefn = raw_write },
> > +  .raw_writefn = raw_write, .resetvalue = 0 },
> 
> .resetvalue 0 is the default, but it doesn't hurt to specify it
> explicitly I guess.
> 
> >  { .name = "PMOVSCLR_EL0", .state = ARM_CP_STATE_AA64,
> >.opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 3,
> >.access = PL0_RW, .accessfn = pmreg_access,
> > @@ -1328,6 +1336,19 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
> >.fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
> >.writefn = pmovsr_write,
> >.raw_writefn = raw_write },
> > +{ .name = "PMOVSSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 
> > = 3,
> > +  .access = PL0_RW, .accessfn = pmreg_access,
> > +  .type = ARM_CP_ALIAS,
> > +  .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
> > +  .writefn = pmovsset_write,
> > +  .raw_writefn = raw_write },
> > +{ .name = "PMOVSSET_EL0", .state = ARM_CP_STATE_AA64,
> > +  .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 14, .opc2 = 3,
> > +  .access = PL0_RW, .accessfn = pmreg_access,
> > +  .type = ARM_CP_ALIAS,
> > +  .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
> > +  .writefn = pmovsset

Re: [Qemu-devel] [PATCH 04/13] target/arm: Mask PMU register writes based on PMCR_EL0.N

[Aaron Lindsay]

On Oct 17 14:41, Peter Maydell wrote:
> On 30 September 2017 at 03:08, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > This is in preparation for enabling counters other than PMCCNTR
> >
> > Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
> > ---
> >  target/arm/helper.c | 24 +++-
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index ecf8c55..54070a3 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -30,11 +30,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> > target_ulong address,
> > hwaddr *phys_ptr, MemTxAttrs *txattrs, int 
> > *prot,
> > target_ulong *page_size_ptr, uint32_t *fsr,
> > ARMMMUFaultInfo *fi);
> > -
> > -/* Definitions for the PMCCNTR and PMCR registers */
> > -#define PMCRD   0x8
> > -#define PMCRC   0x4
> > -#define PMCRE   0x1
> >  #endif
> >
> >  static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
> > @@ -876,6 +871,17 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
> >  REGINFO_SENTINEL
> >  };
> >
> > +/* Definitions for the PMU registers */
> > +#define PMCRN   0xf800
> 
> We usually name this kind of whole-field mask value something like PMCRN_MASK.
> 
> (We also have a FIELD() macro in hw/registerfields.h for defining
> mask/length/shift constants; though it can be a bit verbose in
> the length of the names it uses, so I don't insist on its use
> if you don't like the results. I'm on the fence myself about it.)

Will do.

> > +#define PMCRN_SHIFT 11
> > +#define PMCRD   0x8
> > +#define PMCRC   0x4
> > +#define PMCRE   0x1
> > +
> > +#define PMU_NUM_COUNTERS(env) ((env->cp15.c9_pmcr & PMCRN) >> PMCRN_SHIFT)
> > +/* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */
> > +#define PMU_COUNTER_MASK(env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 
> > 1))
> > +
> >  static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo 
> > *ri,
> > bool isread)
> >  {
> > @@ -1060,14 +1066,14 @@ static void pmccfiltr_write(CPUARMState *env, const 
> > ARMCPRegInfo *ri,
> >  static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
> >  uint64_t value)
> >  {
> > -value &= (1 << 31);
> > +value &= (PMU_COUNTER_MASK(env) | (1 << 31));
> 
> PMU_COUNTER_MASK always contains bit 31, so why do we manually OR it in
> again here?

I forgot I'd also included the cycle counter in PMU_COUNTER_MASK... I'll
remove the duplication here and in pmcntenclr_write below.

> >  env->cp15.c9_pmcnten |= value;
> >  }
> >
> >  static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> >   uint64_t value)
> >  {
> > -value &= (1 << 31);
> > +value &= (PMU_COUNTER_MASK(env) | (1 << 31));
> >  env->cp15.c9_pmcnten &= ~value;
> >  }
> >
> > @@ -1115,14 +1121,14 @@ static void pmintenset_write(CPUARMState *env, 
> > const ARMCPRegInfo *ri,
> >   uint64_t value)
> >  {
> >  /* We have no event counters so only the C bit can be changed */
> > -value &= (1 << 31);
> > +value &= PMU_COUNTER_MASK(env);
> >  env->cp15.c9_pminten |= value;
> >  }
> >
> >  static void pmintenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> >   uint64_t value)
> >  {
> > -value &= (1 << 31);
> > +value &= PMU_COUNTER_MASK(env);
> >  env->cp15.c9_pminten &= ~value;
> >  }
> >
> > --
> > Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, 
> > Inc.\nQualcomm Technologies, Inc. is a member of the\nCode Aurora Forum, a 
> > Linux Foundation Collaborative Project.
> 
> Shouldn't these '\n' in your sig be literal line breaks? :-)

Yes :( ...apparently `git config` doesn't interpret them as I expected.

> thanks
> -- PMM

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH 03/13] target/arm: Reorganize PMCCNTR read, write, sync

[Aaron Lindsay]

On Oct 17 14:25, Peter Maydell wrote:
> On 30 September 2017 at 03:08, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > pmccntr_read and pmccntr_write contained duplicate code that was already
> > being handled by pmccntr_sync. This also moves the calls to get the
> > clock inside the 'if' statement so they are not executed if not needed.
> 
> It is duplicate code, yes, but I also find it a bit confusing,
> because it's the same code doing two different operations:
> 
> (1) if (as is usual when the counter is running) c15_ccnt
> contains a delta between the clock ticks and the register
> value, pmccntr_sync() sets c15_ccnt to the current
> guest-visible register value
> 
> (2) if c15_ccnt contains a guest-visible register value
> and the clock is running, pmccntr_sync() sets c15_ccnt
> to the ticks-to-value delta
> 
> and we use this in a couple of places like:
>pmccntr_sync();
>do stuff that operates on the guest register values,
>  or maybe turns off the counter, etc;
>pmccntr_sync();
> 
> But that's wrong really -- we'll slightly lose time because
> the nanosecond clock advances between the two calls
> to qemu_clock_get_ns(). (It only works at all because
> it happens that f(x) = C - x  is a self-inverse function.)
> It's also a confusing API because it looks like something
> you only need to call once but actually in most cases
> you need to call it twice, before and after whatever it
> is you're doing with the counter.

Interesting, I hadn't thought about the loss of time issue.

> So I think we should refactor this so that we have a
> pair of functions, something like:
> uint64_t clocknow = pmccntr_op_start(env);
> /* Now c15_ccnt is the guest visible value, and
>  * we can do things like change PMCRD, enable bits, etc
>  */
> [...]
> /* convert c15_ccnt back to the clock-to-value delta,
>  * passing it the tick count we used when we did the
>  * delta-to-value conversion earlier.
>  */
> pmccntr_op_finish(env, clocknow);
> 
> (clocknow here should be the output of the muldiv64(),
> not the divided-by-64 version.)
> 
> Some more specific review comments below, though the
> suggested refactoring above might render some of them moot.

I agree that what you describe would be cleaner. I'll work towards that
in v3.

> > Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
> > ---
> >  target/arm/helper.c | 55 
> > -
> >  1 file changed, 16 insertions(+), 39 deletions(-)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index 40c9273..ecf8c55 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -973,17 +973,18 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
> >
> >  void pmccntr_sync(CPUARMState *env)
> >  {
> > -uint64_t temp_ticks;
> > +if (arm_ccnt_enabled(env) &&
> > +  !pmu_counter_filtered(env, env->cp15.pmccfiltr_el0)) {
> 
> This seems to be adding an extra condition that the commit
> message doesn't mention.

Eek. This slipped the cracks through during a rebase and belongs in
'target/arm: Filter cycle counter based on PMCCFILTR_EL0'.

> > +uint64_t temp_ticks;
> >
> > -temp_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> > -  ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
> > +temp_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> > +  ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
> >
> > -if (env->cp15.c9_pmcr & PMCRD) {
> > -/* Increment once every 64 processor clock cycles */
> > -temp_ticks /= 64;
> > -}
> > +if (env->cp15.c9_pmcr & PMCRD) {
> > +/* Increment once every 64 processor clock cycles */
> > +temp_ticks /= 64;
> > +}
> >
> > -if (arm_ccnt_enabled(env)) {
> >  env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
> >  }
> >  }
> > @@ -1007,21 +1008,11 @@ static void pmcr_write(CPUARMState *env, const 
> > ARMCPRegInfo *ri,
> >
> >  static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> >  {
> > -uint64_t total_ticks;
> > -
> > -if (!arm_ccnt_enabled(env)) {
> > -/* Counter is disabled, do not change value */
> > -return env->cp15.c15_ccnt;
> > -}
> > -
> > -total_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> > -   ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
> > -
&

Re: [Qemu-devel] [PATCH 02/13] target/arm: Check PMCNTEN for whether PMCCNTR is enabled

[Aaron Lindsay]

On Oct 17 13:49, Peter Maydell wrote:
> On 30 September 2017 at 03:08, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
> > ---
> >  target/arm/helper.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index 8be78ea..40c9273 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -964,7 +964,7 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
> >  {
> >  /* This does not support checking PMCCFILTR_EL0 register */
> >
> > -if (!(env->cp15.c9_pmcr & PMCRE)) {
> > +if (!(env->cp15.c9_pmcr & PMCRE) || !(env->cp15.c9_pmcnten & (1 << 
> > 31))) {
> >  return false;
> >  }
> >
> 
> Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
> 
> I keep replying to your v1 patchset by mistake -- can you use
> the git format-patch options to put make the subject prefix
> be "[PATCH v3]" for the next round in all the patchmails to
> help keep the versions distinct, please?

Yes, and I apologize for the nuisance. Also, thanks - I didn't know
about the `-v, --reroll-count` option and must have gotten distracted
between updating the subject to be 'v2' for the cover letter and the patches.

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH 01/13] target/arm: A53: Initialize PMCEID[0]

[Aaron Lindsay]

On Oct 09 19:19, Peter Maydell wrote:
> On 19 April 2017 at 18:41, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > A53 advertises ARM_FEATURE_PMU, but wasn't initializing pmceid[01]
> >
> > Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
> > ---
> >  target/arm/cpu.c   | 2 +-
> >  target/arm/cpu64.c | 2 ++
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 04b062c..921b028 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -1342,7 +1342,7 @@ static void cortex_a15_initfn(Object *obj)
> >  cpu->id_pfr0 = 0x1131;
> >  cpu->id_pfr1 = 0x00011011;
> >  cpu->id_dfr0 = 0x02010555;
> > -cpu->pmceid0 = 0x000;
> > +cpu->pmceid0 = 0x;
> >  cpu->pmceid1 = 0x;
> >  cpu->id_afr0 = 0x;
> >  cpu->id_mmfr0 = 0x10201105;
> 
> This is A15 code, which the commit message doesn't say anything about.
> Fixing this code style nit should probably be a separate patch.

I'll split this off for the next version.

> > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> > index 670c07a..7b1642e 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -198,6 +198,8 @@ static void aarch64_a53_initfn(Object *obj)
> >  cpu->id_isar5 = 0x00011121;
> >  cpu->id_aa64pfr0 = 0x;
> >  cpu->id_aa64dfr0 = 0x10305106;
> > +cpu->pmceid0 = 0x;
> > +cpu->pmceid1 = 0x;
> >  cpu->id_aa64isar0 = 0x00011120;
> >  cpu->id_aa64mmfr0 = 0x1122; /* 40 bit physical addr */
> >  cpu->dbgdidr = 0x3516d000;
> 
> Does this actually make a difference? The field values should be 0
> anyway if the CPU-specific initfn doesn't set them to anything.

Perhaps not. I thought the omission was accidental since A15 and A57
both initialize them to zero (added in
4054bfa9e7986c9b7d2bf70f9e10af9647e376fc: "target-arm: Add the pmceid0
and pmceid1 registers")

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v2 00/13] More fully implement ARM PMUv3

[Aaron Lindsay]

On Oct 09 19:27, Peter Maydell wrote:
> On 9 October 2017 at 15:46, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > Unfortunately I'm not sure who to add other than the current recipients,
> > but I'm eager for feedback and would love to work this into something
> > that will allow for using the full ARM PMU.
> 
> Hi -- I do have this on my review queue, but unfortunately it's
> sitting behind some other fairly chunky hard-to-review patchsets.

No problem - I'll wait my turn.

> As a first quick "is this going in the right direction" review based
> pretty much only on the cover letter:
> 
> What extra events do you want to try to support in the emulated PMU?
> Part of the reason we only support the cycle counter is because
>  (1) a lot of the events in a real PMU would be hard to support
>  (2) it's not clear to me that exposing events to the guest would be
>  very useful to it anyway -- performance profiling of guest code
>  running under emulation is fraught with difficulty
> 
> Giving more of an idea of what your use case is would help in
> evaluating these patches.

My goal isn't to expose events concerned with microarchitectural
performance, but rather those that can help characterize architectural
behavior (initially instructions and maybe branches, but perhaps
anything in ARM ARM D5.10.4: "Common architectural event numbers"). We
use a number of platforms at different points along the accuracy/speed
trade-off continuum, and it is convenient to use self-hosted tools that
we can expect to work on all of them.

For instance, implementing the instruction counter allows us to stop
processes after executing prescribed numbers of instructions and resume
them on other platforms for further study (using CRIU). It can also be
useful to get a `perf` profile based on instruction count to get a rough
idea of where an application is spending its time.

> Some of what you're doing looks like it's fixing bugs in our current
> implementation, which is definitely fine in principle.

Yes, I believe patches 1-5 are fairly straightforward fixes, with the
later patches getting into the more invasive changes.

> I haven't looked at the icount related stuff (and I can never remember
> how it works either) but fiddling with can_do_io does sound like it's not
> the right approach...

Agreed. Perhaps part of the same offense as the pmu_sync() calls
scattered around - I was unable to find a better way to drive the mode
filtering, and am more than glad to pursue a different approach if
pointed in the right direction.

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v2 00/13] More fully implement ARM PMUv3

[Aaron Lindsay]

Ping!

Unfortunately I'm not sure who to add other than the current recipients,
but I'm eager for feedback and would love to work this into something
that will allow for using the full ARM PMU. 

I've also updated Peter Crosthwaite's email since the xilinx one appears
to be stale.

-Aaron

On Sep 29 22:08, Aaron Lindsay wrote:
> The ARM PMU implementation currently contains a basic cycle counter, but it is
> often useful to gather counts of other events and filter them based on
> execution mode. These patches flesh out the implementations of various PMU
> registers including PM[X]EVCNTR and PM[X]EVTYPER, add a struct definition to
> represent arbitrary counter types, implement mode filtering, and add
> instruction, cycle, and software increment events.
> 
> I am particularly interested in feedback on the following two patches because 
> I
> think I'm likely Doing It Wrong:
>  [1] target/arm: Filter cycle counter based on PMCCFILTR_EL0
>  [2] target/arm: PMU: Add instruction and cycle events
> 
> In order to implement mode filtering in an event-driven way, [1] adds a pair 
> of
> calls to pmu_sync() surrounding every update to a register/variable which may
> affect whether any counter is currently filtered. These pmu_sync() calls
> ultimately call cpu_get_icount_raw() for enabled instruction and cycle 
> counters
> when using icount. Unfortunately, cpu->can_do_io may otherwise be zero for
> these calls so the current implementation in [2] temporarily sets can_do_io to
> 1. I haven't see any ill side effects from this in my testing, but it doesn't
> seem like the right way to handle this.
> 
> I would like to eventually add sending interrupts on counter overflow.
> Suggestions for the best direction to handle this are most welcome.  
> 
> Thanks for any feedback,
> Aaron
> 
> Aaron Lindsay (13):
>   target/arm: A53: Initialize PMCEID[0]
>   target/arm: Check PMCNTEN for whether PMCCNTR is enabled
>   target/arm: Reorganize PMCCNTR read, write, sync
>   target/arm: Mask PMU register writes based on PMCR_EL0.N
>   target/arm: Allow AArch32 access for PMCCFILTR
>   target/arm: Filter cycle counter based on PMCCFILTR_EL0
>   target/arm: Implement PMOVSSET
>   target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled
>   target/arm: Add array for supported PMU events, generate PMCEID[01]
>   target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER
>   target/arm: PMU: Add instruction and cycle events
>   target/arm: PMU: Set PMCR.N to 4
>   target/arm: Implement PMSWINC
> 
>  target/arm/cpu.c   |  10 +-
>  target/arm/cpu.h   |  34 +++-
>  target/arm/cpu64.c |   2 +
>  target/arm/helper.c| 535 
> +
>  target/arm/kvm64.c |   2 +
>  target/arm/machine.c   |   2 +
>  target/arm/op_helper.c |   4 +
>  7 files changed, 500 insertions(+), 89 deletions(-)
> 
> -- 
> Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, 
> Inc.
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.
> 

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH 12/13] target/arm: PMU: Set PMCR.N to 4

[Aaron Lindsay]

This both advertises that we support four counters and adds them to the
implementation because the PMU_NUM_COUNTERS macro reads this value from
the PMCR.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 5972984..a15b932 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -4936,7 +4936,8 @@ void register_cp_regs_for_features(ARMCPU *cpu)
 .access = PL0_RW, .accessfn = pmreg_access,
 .type = ARM_CP_IO,
 .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr),
-.resetvalue = cpu->midr & 0xff00,
+/* 4 counters enabled */
+.resetvalue = (cpu->midr & 0xff00) | (0x4 << PMCRN_SHIFT),
 .writefn = pmcr_write, .raw_writefn = raw_write,
 };
 define_one_arm_cp_reg(cpu, );
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH 11/13] target/arm: PMU: Add instruction and cycle events

[Aaron Lindsay]

The instruction event is only enabled when icount is used, cycles are
always supported.

Note: Setting can_do_io=1 should not be done here. It is ugly and wrong,
but I am not sure of the proper way to handle this (See 'target/arm:
Filter cycle counter based on PMCCFILTR_EL0')

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/helper.c | 49 +++--
 1 file changed, 47 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 66e576a..5972984 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -14,6 +14,7 @@
 #include "arm_ldst.h"
 #include  /* For crc32 */
 #include "exec/semihost.h"
+#include "sysemu/cpus.h"
 #include "sysemu/kvm.h"
 
 #define ARM_CPU_FREQ 10 /* FIXME: 1 GHz, should be configurable */
@@ -901,8 +902,53 @@ typedef struct pm_event {
 uint64_t (*get_count)(CPUARMState *);
 } pm_event;
 
+static bool event_always_supported(CPUARMState *env)
+{
+return true;
+}
+
+static uint64_t cycles_get_count(CPUARMState *env)
+{
+uint64_t ret;
+CPUState *cpu = ENV_GET_CPU(env);
+uint32_t saved_can_do_io = cpu->can_do_io;
+cpu->can_do_io = 1;
+
+ret = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+   ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
+
+cpu->can_do_io = saved_can_do_io;
+return ret;
+}
+
+static bool instructions_supported(CPUARMState *env)
+{
+return use_icount == 1 /* Precise instruction counting */;
+}
+
+static uint64_t instructions_get_count(CPUARMState *env)
+{
+uint64_t ret;
+CPUState *cpu = ENV_GET_CPU(env);
+uint32_t saved_can_do_io = cpu->can_do_io;
+cpu->can_do_io = 1;
+
+ret = (uint64_t)cpu_get_icount_raw();
+
+cpu->can_do_io = saved_can_do_io;
+return ret;
+}
+
 #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
 static const pm_event pm_events[] = {
+{ .number = 0x008, /* INST_RETIRED */
+  .supported = instructions_supported,
+  .get_count = instructions_get_count
+},
+{ .number = 0x011, /* CPU_CYCLES */
+  .supported = event_always_supported,
+  .get_count = cycles_get_count
+},
 { .number = SUPPORTED_EVENT_SENTINEL }
 };
 static uint16_t supported_event_map[0x3f];
@@ -1087,8 +1133,7 @@ void pmccntr_sync(CPUARMState *env)
   !pmu_counter_filtered(env, env->cp15.pmccfiltr_el0)) {
 uint64_t temp_ticks;
 
-temp_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-  ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
+temp_ticks = cycles_get_count(env);
 
 if (env->cp15.c9_pmcr & PMCRD) {
 /* Increment once every 64 processor clock cycles */
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH 10/13] target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER

[Aaron Lindsay]

Add arrays to hold the registers, the definitions themselves, access
functions, and add logic to reset counters when PMCR.P is set.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/cpu.h|   7 +-
 target/arm/helper.c | 187 
 2 files changed, 179 insertions(+), 15 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 57ca684..26df432 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -397,10 +397,13 @@ typedef struct CPUARMState {
 uint64_t oslsr_el1; /* OS Lock Status */
 uint64_t mdcr_el2;
 uint64_t mdcr_el3;
-/* If the counter is enabled, this stores the last time the counter
- * was reset. Otherwise it stores the counter value
+/* If the pmccntr and pmevcntr counters are enabled, they store the
+ * offset the last time the counter was reset. Otherwise they store the
+ * counter value.
  */
 uint64_t c15_ccnt;
+uint64_t c14_pmevcntr[31];
+uint64_t c14_pmevtyper[31];
 uint64_t pmccfiltr_el0; /* Performance Monitor Filter Register */
 uint64_t vpidr_el2; /* Virtualization Processor ID Register */
 uint64_t vmpidr_el2; /* Virtualization Multiprocessor ID Register */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index a0ae201..66e576a 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -876,6 +876,7 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 #define PMCRN_SHIFT 11
 #define PMCRD   0x8
 #define PMCRC   0x4
+#define PMCRP   0x2
 #define PMCRE   0x1
 
 #define PMXEVTYPER_P  0x8000
@@ -1020,6 +1021,21 @@ static inline bool pmu_counter_enabled(CPUARMState *env, 
uint8_t counter)
 return false;
 }
 
+if (counter != 31) {
+/* If not checking PMCCNTR, ensure the counter is setup to an event we
+ * support */
+uint16_t event = env->cp15.c14_pmevtyper[counter] & 
PMXEVTYPER_EVTCOUNT;
+if (event > 0x3f) {
+return false; /* We only support common architectural and
+ microarchitectural events */
+}
+
+uint16_t event_idx = supported_event_map[event];
+if (event_idx == SUPPORTED_EVENT_SENTINEL) {
+return false;
+}
+}
+
 return true;
 }
 
@@ -1083,8 +1099,26 @@ void pmccntr_sync(CPUARMState *env)
 }
 }
 
+static void pmu_sync_counter(CPUARMState *env, uint8_t counter)
+{
+if (pmu_counter_enabled(env, counter) &&
+!pmu_counter_filtered(env, env->cp15.c14_pmevtyper[counter])) {
+
+uint16_t event = env->cp15.c14_pmevtyper[counter] & 
PMXEVTYPER_EVTCOUNT;
+uint16_t event_idx = supported_event_map[event];
+
+uint64_t count = pm_events[event_idx].get_count(env);
+env->cp15.c14_pmevcntr[counter] =
+count - env->cp15.c14_pmevcntr[counter];
+}
+}
+
 void pmu_sync(CPUARMState *env)
 {
+unsigned int i;
+for (i = 0; i < PMU_NUM_COUNTERS(env); i++) {
+pmu_sync_counter(env, i);
+}
 pmccntr_sync(env);
 }
 
@@ -1098,6 +1132,13 @@ static void pmcr_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 env->cp15.c15_ccnt = 0;
 }
 
+if (value & PMCRP) {
+unsigned int i;
+for (i = 0; i < PMU_NUM_COUNTERS(env); i++) {
+env->cp15.c14_pmevcntr[i] = 0;
+}
+}
+
 /* only the DP, X, D and E bits are writable */
 env->cp15.c9_pmcr &= ~0x39;
 env->cp15.c9_pmcr |= (value & 0x39);
@@ -1203,30 +1244,112 @@ static void pmovsset_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 env->cp15.c9_pmovsr |= value;
 }
 
-static void pmxevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
- uint64_t value)
+static void pmevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value, const uint8_t counter)
 {
+if (counter == 0x1f) {
+pmccfiltr_write(env, ri, value);
+} else if (counter < PMU_NUM_COUNTERS(env)) {
+pmu_sync_counter(env, counter);
+env->cp15.c14_pmevtyper[counter] = value & 0xfe0003ff;
+pmu_sync_counter(env, counter);
+}
 /* Attempts to access PMXEVTYPER are CONSTRAINED UNPREDICTABLE when
  * PMSELR value is equal to or greater than the number of implemented
  * counters, but not equal to 0x1f. We opt to behave as a RAZ/WI.
  */
-if (env->cp15.c9_pmselr == 0x1f) {
-pmccfiltr_write(env, ri, value);
+}
+
+static uint64_t pmevtyper_read(CPUARMState *env, const ARMCPRegInfo *ri,
+   const uint8_t counter)
+{
+if (counter == 0x1f) {
+return env->cp15.pmccfiltr_el0;
+} else if (counter < PMU_NUM_COUNTERS(env)) {
+return env->cp15.c14_pmevtyper[counter];
+} else {
+  /* We opt to behave as a RAZ/WI when attempt

[Qemu-devel] [PATCH 09/13] target/arm: Add array for supported PMU events, generate PMCEID[01]

[Aaron Lindsay]

This commit doesn't add any supported events, but provides the framework
for adding them. We store the pm_event structs in a simple array, and
provide the mapping from the event numbers to array indexes in
the supported_event_map array.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/cpu.c|  4 
 target/arm/cpu.h| 10 ++
 target/arm/helper.c | 37 +
 3 files changed, 51 insertions(+)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 44c965c..d61ea12 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -788,6 +788,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
**errp)
 if (!cpu->has_pmu) {
 cpu->has_pmu = false;
 unset_feature(env, ARM_FEATURE_PMU);
+} else {
+uint64_t pmceid = get_pmceid(>env);
+cpu->pmceid0 = pmceid & 0x;
+cpu->pmceid1 = (pmceid >> 32) & 0x;
 }
 
 if (!arm_feature(env, ARM_FEATURE_EL2)) {
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index f3524f6..57ca684 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -780,6 +780,16 @@ void pmccntr_sync(CPUARMState *env);
  */
 void pmu_sync(CPUARMState *env);
 
+/*
+ * get_pmceid
+ * @env: CPUARMState
+ *
+ * Return the PMCEID[01] register values corresponding to the counters which
+ * are supported given the current configuration (0 is low 32, 1 is high 32
+ * bits)
+ */
+uint64_t get_pmceid(CPUARMState *env);
+
 /* SCTLR bit meanings. Several bits have been reused in newer
  * versions of the architecture; in that case we define constants
  * for both old and new bit meanings. Code which tests against those
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 5d07f72..a0ae201 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -891,6 +891,43 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 /* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */
 #define PMU_COUNTER_MASK(env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 1))
 
+typedef struct pm_event {
+uint16_t number; /* PMEVTYPER.evtCount is 10 bits wide */
+/* If the event is supported on this CPU (used to generate PMCEID[01]) */
+bool (*supported)(CPUARMState *);
+/* Retrieve the current count of the underlying event. The programmed
+ * counters hold a difference from the return value from this function */
+uint64_t (*get_count)(CPUARMState *);
+} pm_event;
+
+#define SUPPORTED_EVENT_SENTINEL UINT16_MAX
+static const pm_event pm_events[] = {
+{ .number = SUPPORTED_EVENT_SENTINEL }
+};
+static uint16_t supported_event_map[0x3f];
+
+/*
+ * Called upon initialization to build PMCEID0 (low 32 bits) and PMCEID1 (high
+ * 32). We also use it to build a map of ARM event numbers to indices in
+ * our pm_events array.
+ */
+uint64_t get_pmceid(CPUARMState *env)
+{
+uint64_t pmceid = 0;
+unsigned int i = 0;
+while (pm_events[i].number != SUPPORTED_EVENT_SENTINEL) {
+const pm_event *cnt = _events[i];
+if (cnt->number < 0x3f && cnt->supported(env)) {
+pmceid |= (1 << cnt->number);
+supported_event_map[cnt->number] = i;
+} else {
+supported_event_map[cnt->number] = SUPPORTED_EVENT_SENTINEL;
+}
+i++;
+}
+return pmceid;
+}
+
 static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
bool isread)
 {
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH 13/13] target/arm: Implement PMSWINC

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/helper.c | 40 ++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index a15b932..2c51f92 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -907,6 +907,15 @@ static bool event_always_supported(CPUARMState *env)
 return true;
 }
 
+static uint64_t swinc_get_count(CPUARMState *env)
+{
+/*
+ * SW_INCR events are written directly to the pmevcntr's by writes to
+ * PMSWINC, so don't do anything here...
+ */
+return 0;
+}
+
 static uint64_t cycles_get_count(CPUARMState *env)
 {
 uint64_t ret;
@@ -941,6 +950,10 @@ static uint64_t instructions_get_count(CPUARMState *env)
 
 #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
 static const pm_event pm_events[] = {
+{ .number = 0x000, /* SW_INCR */
+  .supported = event_always_supported,
+  .get_count = swinc_get_count
+},
 { .number = 0x008, /* INST_RETIRED */
   .supported = instructions_supported,
   .get_count = instructions_get_count
@@ -1191,6 +1204,25 @@ static void pmcr_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 pmu_sync(env);
 }
 
+static void pmswinc_write(CPUARMState *env, const ARMCPRegInfo *ri,
+  uint64_t value)
+{
+unsigned int i;
+for (i = 0; i < PMU_NUM_COUNTERS(env); i++) {
+/* Increment a counter's count iff: */
+if ((value & (1 << i)) && /* counter's bit is set */
+/* counter is enabled and not filtered */
+pmu_counter_enabled(env, i) &&
+!pmu_counter_filtered(env, env->cp15.c14_pmevtyper[i]) &&
+/* counter is SW_INCR */
+(env->cp15.c14_pmevtyper[i] & PMXEVTYPER_EVTCOUNT) == 0x0) {
+pmu_sync_counter(env, i);
+env->cp15.c14_pmevcntr[i]++;
+pmu_sync_counter(env, i);
+}
+}
+}
+
 static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
 uint64_t ret;
@@ -1559,9 +1591,13 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
   .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
   .writefn = pmovsset_write,
   .raw_writefn = raw_write },
-/* Unimplemented so WI. */
 { .name = "PMSWINC", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 4,
-  .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NOP },
+  .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NO_RAW,
+  .writefn = pmswinc_write },
+{ .name = "PMSWINC_EL0", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 4,
+  .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NO_RAW,
+  .writefn = pmswinc_write },
 #ifndef CONFIG_USER_ONLY
 { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
   .access = PL0_RW, .type = ARM_CP_ALIAS,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH 06/13] target/arm: Filter cycle counter based on PMCCFILTR_EL0

[Aaron Lindsay]

The pmu_counter_filtered and pmu_sync functions are generic (as opposed
to PMCCNTR-specific) to allow for the implementation of other events.

RFC: I know that many of the locations of the calls to pmu_sync are
problematic when icount is enabled because can_do_io will not be set.
The documentation says that for deterministic execution, IO must only be
performed by the last instruction of a thread block. Because
cpu_handle_interrupt() and cpu_handle_exception() are actually made
outside of a thread block, is it safe to set can_do_io=1 for them to
allow this to succeed? Is there a better mechanism for handling this?

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/cpu.c   |  4 +++
 target/arm/cpu.h   | 15 +++
 target/arm/helper.c| 73 +++---
 target/arm/kvm64.c |  2 ++
 target/arm/machine.c   |  2 ++
 target/arm/op_helper.c |  4 +++
 6 files changed, 97 insertions(+), 3 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 921b028..44c965c 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -139,6 +139,8 @@ static void arm_cpu_reset(CPUState *s)
 env->iwmmxt.cregs[ARM_IWMMXT_wCID] = 0x69051000 | 'Q';
 }
 
+pmu_sync(env); /* Surround writes to uncached_cpsr, pstate, and aarch64 */
+
 if (arm_feature(env, ARM_FEATURE_AARCH64)) {
 /* 64 bit CPUs always start in 64 bit mode */
 env->aarch64 = 1;
@@ -180,6 +182,8 @@ static void arm_cpu_reset(CPUState *s)
 env->uncached_cpsr = ARM_CPU_MODE_SVC;
 env->daif = PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F;
 
+pmu_sync(env); /* Surround writes to uncached_cpsr, pstate, and aarch64 */
+
 if (arm_feature(env, ARM_FEATURE_M)) {
 uint32_t initial_msp; /* Loaded from 0x0 */
 uint32_t initial_pc; /* Loaded from 0x4 */
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index a8aabce..ae2a294 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -767,6 +767,19 @@ int cpu_arm_signal_handler(int host_signum, void *pinfo,
  */
 void pmccntr_sync(CPUARMState *env);
 
+/**
+ * pmu_sync
+ * @env: CPUARMState
+ *
+ * Synchronises all PMU counters. This must always be called twice, once before
+ * any action that might affect the filtering of all counters and again
+ * afterwards. The function is used to swap the state of the registers if
+ * required. This only happens when not in user mode (!CONFIG_USER_ONLY). Any
+ * writes to env's aarch64, pstate, uncached_cpsr, cp15.scr_el3, or
+ * cp15.hcr_el2 must be protected by calls to this function.
+ */
+void pmu_sync(CPUARMState *env);
+
 /* SCTLR bit meanings. Several bits have been reused in newer
  * versions of the architecture; in that case we define constants
  * for both old and new bit meanings. Code which tests against those
@@ -947,7 +960,9 @@ static inline void pstate_write(CPUARMState *env, uint32_t 
val)
 env->CF = (val >> 29) & 1;
 env->VF = (val << 3) & 0x8000;
 env->daif = val & PSTATE_DAIF;
+pmu_sync(env);
 env->pstate = val & ~CACHED_PSTATE_BITS;
+pmu_sync(env);
 }
 
 /* Return the current CPSR value.  */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 530fc7c..bf9f164 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -878,6 +878,15 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 #define PMCRC   0x4
 #define PMCRE   0x1
 
+#define PMXEVTYPER_P  0x8000
+#define PMXEVTYPER_U  0x4000
+#define PMXEVTYPER_NSK0x2000
+#define PMXEVTYPER_NSU0x1000
+#define PMXEVTYPER_NSH0x0800
+#define PMXEVTYPER_M  0x0400
+#define PMXEVTYPER_MT 0x0200
+#define PMXEVTYPER_EVTCOUNT   0x03ff
+
 #define PMU_NUM_COUNTERS(env) ((env->cp15.c9_pmcr & PMCRN) >> PMCRN_SHIFT)
 /* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */
 #define PMU_COUNTER_MASK(env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 1))
@@ -968,7 +977,7 @@ static CPAccessResult pmreg_access_ccntr(CPUARMState *env,
 
 static inline bool arm_ccnt_enabled(CPUARMState *env)
 {
-/* This does not support checking PMCCFILTR_EL0 register */
+/* Does not check PMCCFILTR_EL0, which is handled by pmu_counter_filtered 
*/
 
 if (!(env->cp15.c9_pmcr & PMCRE) || !(env->cp15.c9_pmcnten & (1 << 31))) {
 return false;
@@ -977,6 +986,43 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
 return true;
 }
 
+/* Returns true if the counter corresponding to the passed-in pmevtyper or
+ * pmccfiltr value is filtered using the current state */
+static inline bool pmu_counter_filtered(CPUARMState *env, uint64_t pmxevtyper)
+{
+bool secure = arm_is_secure(env);
+int el = arm_current_el(env);
+
+bool P   = pmxevtyper & PMXEVTYPER_P;
+bool U   = pmxevtyper & PMXEVTYPER_U;
+bool NSK = pmxevtyper & PMXEVTYPER_NSK;
+

[Qemu-devel] [PATCH 08/13] target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/helper.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 9c01269..5d07f72 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -975,17 +975,22 @@ static CPAccessResult pmreg_access_ccntr(CPUARMState *env,
 return pmreg_access(env, ri, isread);
 }
 
-static inline bool arm_ccnt_enabled(CPUARMState *env)
+static inline bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)
 {
 /* Does not check PMCCFILTR_EL0, which is handled by pmu_counter_filtered 
*/
-
-if (!(env->cp15.c9_pmcr & PMCRE) || !(env->cp15.c9_pmcnten & (1 << 31))) {
+if (!(env->cp15.c9_pmcr & PMCRE) ||
+!(env->cp15.c9_pmcnten & (1 << counter))) {
 return false;
 }
 
 return true;
 }
 
+static inline bool arm_ccnt_enabled(CPUARMState *env)
+{
+return pmu_counter_enabled(env, 31);
+}
+
 /* Returns true if the counter corresponding to the passed-in pmevtyper or
  * pmccfiltr value is filtered using the current state */
 static inline bool pmu_counter_filtered(CPUARMState *env, uint64_t pmxevtyper)
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH 05/13] target/arm: Allow AArch32 access for PMCCFILTR

[Aaron Lindsay]

Also fix the existing bitmask for writes.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/helper.c | 23 ++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index e8189b8..530fc7c 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1059,10 +1059,25 @@ static void pmccfiltr_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 uint64_t value)
 {
 pmccntr_sync(env);
-env->cp15.pmccfiltr_el0 = value & 0x7E00;
+env->cp15.pmccfiltr_el0 = value & 0xfc00;
 pmccntr_sync(env);
 }
 
+static void pmccfiltr_write_a32(CPUARMState *env, const ARMCPRegInfo *ri,
+uint64_t value)
+{
+pmccntr_sync(env);
+env->cp15.pmccfiltr_el0 = (env->cp15.pmccfiltr_el0 & 0x0400) |
+(value & 0xf800); /* M is not visible in AArch32 */
+pmccntr_sync(env);
+}
+
+static uint64_t pmccfiltr_read_a32(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+/* M is not visible in AArch32 */
+return env->cp15.pmccfiltr_el0 & 0xf800;
+}
+
 static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
 uint64_t value)
 {
@@ -1280,6 +1295,12 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
   .type = ARM_CP_IO,
   .readfn = pmccntr_read, .writefn = pmccntr_write, },
 #endif
+{ .name = "PMCCFILTR", .cp = 15, .opc1 = 0, .crn = 14, .crm = 15, .opc2 = 
7,
+  .writefn = pmccfiltr_write_a32, .readfn = pmccfiltr_read_a32,
+  .access = PL0_RW, .accessfn = pmreg_access,
+  .type = ARM_CP_ALIAS,
+  .fieldoffset = offsetoflow32(CPUARMState, cp15.pmccfiltr_el0),
+  .resetvalue = 0, },
 { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64,
   .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7,
   .writefn = pmccfiltr_write,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH 07/13] target/arm: Implement PMOVSSET

[Aaron Lindsay]

Also modify it to be stored as a uint64_t

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/cpu.h|  2 +-
 target/arm/helper.c | 27 ---
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index ae2a294..f3524f6 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -310,7 +310,7 @@ typedef struct CPUARMState {
 uint32_t c9_data;
 uint64_t c9_pmcr; /* performance monitor control register */
 uint64_t c9_pmcnten; /* perf monitor counter enables */
-uint32_t c9_pmovsr; /* perf monitor overflow status */
+uint64_t c9_pmovsr; /* perf monitor overflow status */
 uint32_t c9_pmuserenr; /* perf monitor user enable */
 uint64_t c9_pmselr; /* perf monitor counter selection register */
 uint64_t c9_pminten; /* perf monitor interrupt enables */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index bf9f164..9c01269 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1150,9 +1150,17 @@ static void pmcntenclr_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 static void pmovsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
  uint64_t value)
 {
+value &= PMU_COUNTER_MASK(env);
 env->cp15.c9_pmovsr &= ~value;
 }
 
+static void pmovsset_write(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t value)
+{
+value &= PMU_COUNTER_MASK(env);
+env->cp15.c9_pmovsr |= value;
+}
+
 static void pmxevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
  uint64_t value)
 {
@@ -1317,10 +1325,10 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
   .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
   .writefn = pmcntenclr_write },
 { .name = "PMOVSR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 3,
-  .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
-  .accessfn = pmreg_access,
+  .access = PL0_RW, .accessfn = pmreg_access,
+  .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
   .writefn = pmovsr_write,
-  .raw_writefn = raw_write },
+  .raw_writefn = raw_write, .resetvalue = 0 },
 { .name = "PMOVSCLR_EL0", .state = ARM_CP_STATE_AA64,
   .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 3,
   .access = PL0_RW, .accessfn = pmreg_access,
@@ -1328,6 +1336,19 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
   .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
   .writefn = pmovsr_write,
   .raw_writefn = raw_write },
+{ .name = "PMOVSSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 3,
+  .access = PL0_RW, .accessfn = pmreg_access,
+  .type = ARM_CP_ALIAS,
+  .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
+  .writefn = pmovsset_write,
+  .raw_writefn = raw_write },
+{ .name = "PMOVSSET_EL0", .state = ARM_CP_STATE_AA64,
+  .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 14, .opc2 = 3,
+  .access = PL0_RW, .accessfn = pmreg_access,
+  .type = ARM_CP_ALIAS,
+  .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
+  .writefn = pmovsset_write,
+  .raw_writefn = raw_write },
 /* Unimplemented so WI. */
 { .name = "PMSWINC", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 4,
   .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NOP },
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH 04/13] target/arm: Mask PMU register writes based on PMCR_EL0.N

[Aaron Lindsay]

This is in preparation for enabling counters other than PMCCNTR

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/helper.c | 24 +++-
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 390256b..e8189b8 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -30,11 +30,6 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
target_ulong address,
hwaddr *phys_ptr, MemTxAttrs *txattrs, int 
*prot,
target_ulong *page_size_ptr, uint32_t *fsr,
ARMMMUFaultInfo *fi);
-
-/* Definitions for the PMCCNTR and PMCR registers */
-#define PMCRD   0x8
-#define PMCRC   0x4
-#define PMCRE   0x1
 #endif
 
 static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
@@ -876,6 +871,17 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 REGINFO_SENTINEL
 };
 
+/* Definitions for the PMU registers */
+#define PMCRN   0xf800
+#define PMCRN_SHIFT 11
+#define PMCRD   0x8
+#define PMCRC   0x4
+#define PMCRE   0x1
+
+#define PMU_NUM_COUNTERS(env) ((env->cp15.c9_pmcr & PMCRN) >> PMCRN_SHIFT)
+/* Bits allowed to be set/cleared for PMCNTEN* and PMINTEN* */
+#define PMU_COUNTER_MASK(env) ((1 << 31) | ((1 << PMU_NUM_COUNTERS(env)) - 1))
+
 static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
bool isread)
 {
@@ -1060,14 +1066,14 @@ static void pmccfiltr_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
 uint64_t value)
 {
-value &= (1 << 31);
+value &= (PMU_COUNTER_MASK(env) | (1 << 31));
 env->cp15.c9_pmcnten |= value;
 }
 
 static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
  uint64_t value)
 {
-value &= (1 << 31);
+value &= (PMU_COUNTER_MASK(env) | (1 << 31));
 env->cp15.c9_pmcnten &= ~value;
 }
 
@@ -1115,14 +1121,14 @@ static void pmintenset_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
  uint64_t value)
 {
 /* We have no event counters so only the C bit can be changed */
-value &= (1 << 31);
+value &= PMU_COUNTER_MASK(env);
 env->cp15.c9_pminten |= value;
 }
 
 static void pmintenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
  uint64_t value)
 {
-value &= (1 << 31);
+value &= PMU_COUNTER_MASK(env);
 env->cp15.c9_pminten &= ~value;
 }
 
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH 03/13] target/arm: Reorganize PMCCNTR read, write, sync

[Aaron Lindsay]

pmccntr_read and pmccntr_write contained duplicate code that was already
being handled by pmccntr_sync. This also moves the calls to get the
clock inside the 'if' statement so they are not executed if not needed.

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/helper.c | 55 -
 1 file changed, 16 insertions(+), 39 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 391..390256b 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -973,17 +973,18 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
 
 void pmccntr_sync(CPUARMState *env)
 {
-uint64_t temp_ticks;
+if (arm_ccnt_enabled(env) &&
+  !pmu_counter_filtered(env, env->cp15.pmccfiltr_el0)) {
+uint64_t temp_ticks;
 
-temp_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-  ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
+temp_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+  ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
 
-if (env->cp15.c9_pmcr & PMCRD) {
-/* Increment once every 64 processor clock cycles */
-temp_ticks /= 64;
-}
+if (env->cp15.c9_pmcr & PMCRD) {
+/* Increment once every 64 processor clock cycles */
+temp_ticks /= 64;
+}
 
-if (arm_ccnt_enabled(env)) {
 env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
 }
 }
@@ -1007,21 +1008,11 @@ static void pmcr_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 
 static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
-uint64_t total_ticks;
-
-if (!arm_ccnt_enabled(env)) {
-/* Counter is disabled, do not change value */
-return env->cp15.c15_ccnt;
-}
-
-total_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-   ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
-
-if (env->cp15.c9_pmcr & PMCRD) {
-/* Increment once every 64 processor clock cycles */
-total_ticks /= 64;
-}
-return total_ticks - env->cp15.c15_ccnt;
+uint64_t ret;
+pmccntr_sync(env);
+ret = env->cp15.c15_ccnt;
+pmccntr_sync(env);
+return ret;
 }
 
 static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1038,22 +1029,8 @@ static void pmselr_write(CPUARMState *env, const 
ARMCPRegInfo *ri,
 static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 uint64_t value)
 {
-uint64_t total_ticks;
-
-if (!arm_ccnt_enabled(env)) {
-/* Counter is disabled, set the absolute value */
-env->cp15.c15_ccnt = value;
-return;
-}
-
-total_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-   ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
-
-if (env->cp15.c9_pmcr & PMCRD) {
-/* Increment once every 64 processor clock cycles */
-total_ticks /= 64;
-}
-env->cp15.c15_ccnt = total_ticks - value;
+env->cp15.c15_ccnt = value;
+pmccntr_sync(env);
 }
 
 static void pmccntr_write32(CPUARMState *env, const ARMCPRegInfo *ri,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH 00/13] More fully implement ARM PMUv3

[Aaron Lindsay]

The ARM PMU implementation currently contains a basic cycle counter, but it is
often useful to gather counts of other events and filter them based on
execution mode. These patches flesh out the implementations of various PMU
registers including PM[X]EVCNTR and PM[X]EVTYPER, add a struct definition to
represent arbitrary counter types, implement mode filtering, and add
instruction, cycle, and software increment events.

I am particularly interested in feedback on the following two patches because I
think I'm likely Doing It Wrong:
 [1] target/arm: Filter cycle counter based on PMCCFILTR_EL0
 [2] target/arm: PMU: Add instruction and cycle events

In order to implement mode filtering in an event-driven way, [1] adds a pair of
calls to pmu_sync() surrounding every update to a register/variable which may
affect whether any counter is currently filtered. These pmu_sync() calls
ultimately call cpu_get_icount_raw() for enabled instruction and cycle counters
when using icount. Unfortunately, cpu->can_do_io may otherwise be zero for
these calls so the current implementation in [2] temporarily sets can_do_io to
1. I haven't see any ill side effects from this in my testing, but it doesn't
seem like the right way to handle this.

I would like to eventually add sending interrupts on counter overflow.
Suggestions for the best direction to handle this are most welcome.  

Thanks for any feedback,
Aaron


Aaron Lindsay (13):
  target/arm: A53: Initialize PMCEID[0]
  target/arm: Check PMCNTEN for whether PMCCNTR is enabled
  target/arm: Reorganize PMCCNTR read, write, sync
  target/arm: Mask PMU register writes based on PMCR_EL0.N
  target/arm: Allow AArch32 access for PMCCFILTR
  target/arm: Filter cycle counter based on PMCCFILTR_EL0
  target/arm: Implement PMOVSSET
  target/arm: Split arm_ccnt_enabled into generic pmu_counter_enabled
  target/arm: Add array for supported PMU events, generate PMCEID[01]
  target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER
  target/arm: PMU: Add instruction and cycle events
  target/arm: PMU: Set PMCR.N to 4
  target/arm: Implement PMSWINC

 target/arm/cpu.c   |  10 +-
 target/arm/cpu.h   |  34 +++-
 target/arm/cpu64.c |   2 +
 target/arm/helper.c| 523 ++---
 target/arm/kvm64.c |   2 +
 target/arm/machine.c   |   2 +
 target/arm/op_helper.c |   4 +
 7 files changed, 500 insertions(+), 77 deletions(-)

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH 02/13] target/arm: Check PMCNTEN for whether PMCCNTR is enabled

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 8cb7a94..391 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -964,7 +964,7 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
 {
 /* This does not support checking PMCCFILTR_EL0 register */
 
-if (!(env->cp15.c9_pmcr & PMCRE)) {
+if (!(env->cp15.c9_pmcr & PMCRE) || !(env->cp15.c9_pmcnten & (1 << 31))) {
 return false;
 }
 
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH 01/13] target/arm: A53: Initialize PMCEID[0]

[Aaron Lindsay]

A53 advertises ARM_FEATURE_PMU, but wasn't initializing pmceid[01]

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 target/arm/cpu.c   | 2 +-
 target/arm/cpu64.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 04b062c..921b028 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1342,7 +1342,7 @@ static void cortex_a15_initfn(Object *obj)
 cpu->id_pfr0 = 0x1131;
 cpu->id_pfr1 = 0x00011011;
 cpu->id_dfr0 = 0x02010555;
-cpu->pmceid0 = 0x000;
+cpu->pmceid0 = 0x;
 cpu->pmceid1 = 0x;
 cpu->id_afr0 = 0x;
 cpu->id_mmfr0 = 0x10201105;
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 670c07a..7b1642e 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -198,6 +198,8 @@ static void aarch64_a53_initfn(Object *obj)
 cpu->id_isar5 = 0x00011121;
 cpu->id_aa64pfr0 = 0x;
 cpu->id_aa64dfr0 = 0x10305106;
+cpu->pmceid0 = 0x;
+cpu->pmceid1 = 0x;
 cpu->id_aa64isar0 = 0x00011120;
 cpu->id_aa64mmfr0 = 0x1122; /* 40 bit physical addr */
 cpu->dbgdidr = 0x3516d000;
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PULL 08/24] tcg: drop global lock during TCG code execution

[Aaron Lindsay]

On Feb 27 14:39, Alex Bennée wrote:
> 
> Laurent Desnogues  writes:
> 
> > Hello,
> >
> > On Fri, Feb 24, 2017 at 12:20 PM, Alex Bennée  
> > wrote:
> >> From: Jan Kiszka 
> >>
> >> This finally allows TCG to benefit from the iothread introduction: Drop
> >> the global mutex while running pure TCG CPU code. Reacquire the lock
> >> when entering MMIO or PIO emulation, or when leaving the TCG loop.
> >>
> >> We have to revert a few optimization for the current TCG threading
> >> model, namely kicking the TCG thread in qemu_mutex_lock_iothread and not
> >> kicking it in qemu_cpu_kick. We also need to disable RAM block
> >> reordering until we have a more efficient locking mechanism at hand.
> >>
> >> Still, a Linux x86 UP guest and my Musicpal ARM model boot fine here.
> >> These numbers demonstrate where we gain something:
> >>
> >> 20338 jan   20   0  331m  75m 6904 R   99  0.9   0:50.95 
> >> qemu-system-arm
> >> 20337 jan   20   0  331m  75m 6904 S   20  0.9   0:26.50 
> >> qemu-system-arm
> >>
> >> The guest CPU was fully loaded, but the iothread could still run mostly
> >> independent on a second core. Without the patch we don't get beyond
> >>
> >> 32206 jan   20   0  330m  73m 7036 R   82  0.9   1:06.00 
> >> qemu-system-arm
> >> 32204 jan   20   0  330m  73m 7036 S   21  0.9   0:17.03 
> >> qemu-system-arm
> >>
> >> We don't benefit significantly, though, when the guest is not fully
> >> loading a host CPU.
> >
> > I tried this patch (8d04fb55 in the repository) with the following image:
> >
> >http://wiki.qemu.org/download/arm-test-0.2.tar.gz
> >
> > Running the image with no option works fine.  But specifying '-icount
> > 1' results in a (guest) deadlock. Enabling some heavy logging (-d
> > in_asm,exec) sometimes results in a 'Bad ram offset'.
> >
> > Is it expected that this patch breaks -icount?
> 
> Not really. Using icount will disable MTTCG and run single threaded as
> before. Paolo reported another icount failure so they may be related. I
> shall have a look at it.
> 
> Thanks for the report.

I have not experienced a guest deadlock, but for me this patch makes
booting a simple Linux distribution take about an order of magnitude
longer when using '-icount 0' (from about 1.6 seconds to 17.9). It was
slow enough to get to the printk the first time after recompiling that I
killed it thinking it *had* deadlocked.

`perf report` from before this patch (snipped to >1%):
 23.81%  qemu-system-aar  perf-9267.map[.] 0x41a5cc9e
  7.15%  qemu-system-aar  [kernel.kallsyms][k] 0x8172bc82
  6.29%  qemu-system-aar  qemu-system-aarch64  [.] cpu_exec
  4.99%  qemu-system-aar  qemu-system-aarch64  [.] tcg_gen_code
  4.71%  qemu-system-aar  qemu-system-aarch64  [.] cpu_get_tb_cpu_state
  4.39%  qemu-system-aar  qemu-system-aarch64  [.] tcg_optimize
  3.28%  qemu-system-aar  qemu-system-aarch64  [.] helper_dc_zva
  2.66%  qemu-system-aar  qemu-system-aarch64  [.] liveness_pass_1
  1.98%  qemu-system-aar  qemu-system-aarch64  [.] qht_lookup
  1.93%  qemu-system-aar  qemu-system-aarch64  [.] tcg_out_opc
  1.81%  qemu-system-aar  qemu-system-aarch64  [.] get_phys_addr_lpae
  1.71%  qemu-system-aar  qemu-system-aarch64  [.] 
object_class_dynamic_cast_assert
  1.38%  qemu-system-aar  qemu-system-aarch64  [.] arm_regime_tbi1
  1.10%  qemu-system-aar  qemu-system-aarch64  [.] arm_regime_tbi0

and after this patch:
 20.10%  qemu-system-aar  perf-3285.map[.] 0x40a3b690
*18.08%  qemu-system-aar  [kernel.kallsyms][k] 0x81371865
  7.87%  qemu-system-aar  qemu-system-aarch64  [.] cpu_exec
  4.70%  qemu-system-aar  qemu-system-aarch64  [.] cpu_get_tb_cpu_state
* 2.64%  qemu-system-aar  qemu-system-aarch64  [.] g_mutex_get_impl
  2.39%  qemu-system-aar  qemu-system-aarch64  [.] gic_update
* 1.89%  qemu-system-aar  qemu-system-aarch64  [.] pthread_mutex_unlock
  1.61%  qemu-system-aar  qemu-system-aarch64  [.] 
object_class_dynamic_cast_assert
* 1.55%  qemu-system-aar  qemu-system-aarch64  [.] pthread_mutex_lock
  1.31%  qemu-system-aar  qemu-system-aarch64  [.] get_phys_addr_lpae
  1.21%  qemu-system-aar  qemu-system-aarch64  [.] arm_regime_tbi0
  1.13%  qemu-system-aar  qemu-system-aarch64  [.] arm_regime_tbi1

I've put asterisks by a few suspicious mutex-related functions, though I wonder
if the slowdowns are also partially inlined into some of the other functions.
The kernel also jumps up, presumably from handling more mutexes?

I confess I'm not familiar enough with this code to suggest optimizations, but
I'll be glad to test any.

-Aaron

> 
> >
> > Thanks,
> >
> > Laurent
> >
> > PS - To clarify 791158d9 works.
> >
> >> Signed-off-by: Jan Kiszka 
> >> Message-Id: <1439220437-23957-10-git-send-email-fred.kon...@greensocs.com>
> >> [FK: Rebase, fix qemu_devices_reset deadlock, rm address_space_* mutex]
> >> Signed-off-by: KONRAD Frederic 

Re: [Qemu-devel] [PULL 03/12] target-arm: Add support for PMU register PMINTENSET_EL1

[Aaron Lindsay]

Wei, Peter,

On Feb 10 18:07, Peter Maydell wrote:
> From: Wei Huang 
> 
> This patch adds access support for PMINTENSET_EL1.
> 
> Signed-off-by: Wei Huang 
> Reviewed-by: Peter Maydell 
> Message-id: 1486504171-26807-4-git-send-email-...@redhat.com
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/cpu.h|  2 +-
>  target/arm/helper.c | 10 +-
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index edc1f76..0956a54 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -309,7 +309,7 @@ typedef struct CPUARMState {
>  uint32_t c9_pmovsr; /* perf monitor overflow status */
>  uint32_t c9_pmuserenr; /* perf monitor user enable */
>  uint64_t c9_pmselr; /* perf monitor counter selection register */
> -uint32_t c9_pminten; /* perf monitor interrupt enables */
> +uint64_t c9_pminten; /* perf monitor interrupt enables */

PMINTENSET_EL1 and PMINTENCLR_EL1 are both 32-bit registers, just like
their AArch32 counterparts. Is there a reason I'm missing for why this
has been changed to a uint64_t? There are a number of other 32-bit PMU
registers also currently being represented by uint64_t.

-Aaron

>  union { /* Memory attribute redirection */
>  struct {
>  #ifdef HOST_WORDS_BIGENDIAN
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index b837d36..5358ac6 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1275,9 +1275,17 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>.writefn = pmuserenr_write, .raw_writefn = raw_write },
>  { .name = "PMINTENSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 
> = 1,
>.access = PL1_RW, .accessfn = access_tpm,
> -  .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
> +  .type = ARM_CP_ALIAS,
> +  .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pminten),
>.resetvalue = 0,
>.writefn = pmintenset_write, .raw_writefn = raw_write },
> +{ .name = "PMINTENSET_EL1", .state = ARM_CP_STATE_AA64,
> +  .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 1,
> +  .access = PL1_RW, .accessfn = access_tpm,
> +  .type = ARM_CP_IO,
> +  .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
> +  .writefn = pmintenset_write, .raw_writefn = raw_write,
> +  .resetvalue = 0x0 },
>  { .name = "PMINTENCLR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 
> = 2,
>.access = PL1_RW, .accessfn = access_tpm, .type = ARM_CP_ALIAS,
>.fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
> -- 
> 2.7.4
> 
> 

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH] avx2 configure: Disable if static build

[Aaron Lindsay]

This avoids a segfault like the following for at least some 4.8 versions
of gcc when configured with --static if avx2 instructions are also
enabled:

Program received signal SIGSEGV, Segmentation fault.
buffer_find_nonzero_offset_ifunc () at ./util/cutils.c:333
333 {
(gdb) bt
#0  buffer_find_nonzero_offset_ifunc () at ./util/cutils.c:333
#1  0x00939c58 in __libc_start_main ()
#2  0x00419337 in _start ()

Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
---
 configure | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/configure b/configure
index 5ada56d..169aa88 100755
--- a/configure
+++ b/configure
@@ -1788,7 +1788,9 @@ fi
 ##
 # avx2 optimization requirement check
 
-cat > $TMPC << EOF
+
+if test "$static" = "no" ; then
+  cat > $TMPC << EOF
 #pragma GCC push_options
 #pragma GCC target("avx2")
 #include 
@@ -1801,12 +1803,13 @@ static void *bar_ifunc(void) {return (void*) bar;}
 int foo(void *a) __attribute__((ifunc("bar_ifunc")));
 int main(int argc, char *argv[]) { return foo(argv[0]);}
 EOF
-if compile_object "" ; then
-if has readelf; then
-if readelf --syms $TMPO 2>/dev/null |grep -q "IFUNC.*foo"; then
-avx2_opt="yes"
-fi
-fi
+  if compile_object "" ; then
+  if has readelf; then
+  if readelf --syms $TMPO 2>/dev/null |grep -q "IFUNC.*foo"; then
+  avx2_opt="yes"
+  fi
+  fi
+  fi
 fi
 
 #
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 2/2] avx2 configure: Use primitives in test

[Aaron Lindsay]

On Jul 14 16:05, Dr. David Alan Gilbert wrote:
> * Aaron Lindsay (alind...@codeaurora.org) wrote:
> > On Jul 14 14:33, Dr. David Alan Gilbert wrote:
> > > * Aaron Lindsay (alind...@codeaurora.org) wrote:
> > > > I'm configuring with:
> > > > # ./configure \
> > > > --static \
> > > > --disable-gtk \
> > > > --target-list=aarch64-softmmu
> > > 
> > > Does it work if you configure without the --static?
> > 
> > Yes, it works if I configure without --static.
> 
> Hmm; I wonder what the best fix is here; we could just disable
> it with static; it seems an easy fix assuming very few people
> care about the combination of avx2 performance and static,
> or we could try and detect the problem/old version.
> (it seems fine on a modern Fedora; have you tried using a modern
> Ubuntu?)

That seems like a reasonable fix to me. Assuming your intention was to
do this in 'configure', I'll send out a patch shortly.

Unfortunately I haven't been able to try on a 'modern' Ubuntu yet.

-Aaron

--
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 2/2] avx2 configure: Use primitives in test

[Aaron Lindsay]

On Jul 14 15:35, Peter Maydell wrote:
> On 14 July 2016 at 15:27, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > On Jul 14 14:23, Peter Maydell wrote:
> >> On 14 July 2016 at 14:15, Paolo Bonzini <pbonz...@redhat.com> wrote:
> >> > On 14/07/2016 15:13, Aaron Lindsay wrote:
> >> >> I'm configuring with:
> >> >> # ./configure \
> >> >> --static \
> >> >>   --disable-gtk \
> >> >>   --target-list=aarch64-softmmu
> >>
> >> > Hmm, it's possible that we have to disable ifunc together with --static.
> >>
> >> I'm still tempted to say we should just forbid building the softmmu
> >> binaries with --static, unless somebody has a serious use case for it...
> >
> > FWIW, we do find it convenient to be able to compile one binary capable
> > of running on multiple systems with different (and incompatible) library
> > versions.
> 
> The difficulty here is that the system emulators may call
> functions in glibc which do dynamic resolution anyway, and so
> "require the shared libraries from the glibc version used for
> linking", to quote the linker warning. For the user-only binaries
> we can claim we don't make those function calls, but for softmmu
> we do.

We haven't observed any problems with this approach so far, but your
point is well taken. Perhaps we get what we deserve if we insist on
statically compiling ;-)

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v3 2/2] avx2 configure: Use primitives in test

[Aaron Lindsay]

On Jul 14 14:23, Peter Maydell wrote:
> On 14 July 2016 at 14:15, Paolo Bonzini <pbonz...@redhat.com> wrote:
> > On 14/07/2016 15:13, Aaron Lindsay wrote:
> >> I'm configuring with:
> >> # ./configure \
> >> --static \
> >>   --disable-gtk \
> >>   --target-list=aarch64-softmmu
> 
> > Hmm, it's possible that we have to disable ifunc together with --static.
> 
> I'm still tempted to say we should just forbid building the softmmu
> binaries with --static, unless somebody has a serious use case for it...

FWIW, we do find it convenient to be able to compile one binary capable
of running on multiple systems with different (and incompatible) library
versions.

-Aaron

Re: [Qemu-devel] [PATCH v3 2/2] avx2 configure: Use primitives in test

[Aaron Lindsay]

On Jul 14 14:33, Dr. David Alan Gilbert wrote:
> * Aaron Lindsay (alind...@codeaurora.org) wrote:
> > I'm configuring with:
> > # ./configure \
> > --static \
> > --disable-gtk \
> > --target-list=aarch64-softmmu
> 
> Does it work if you configure without the --static?

Yes, it works if I configure without --static.

-Aaron

Re: [Qemu-devel] [PATCH v3 2/2] avx2 configure: Use primitives in test

[Aaron Lindsay]

On Jun 10 12:16, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> Use the avx2 primitives during the test, thus making sure that the
> compiler and assembler could actually use avx2.
> 
> This also detects the failure case on gcc 4.8.x with -save-temps
> and avoids the need for the gcc version check in cutils.

I'm getting a segfault when running the latest tip compiled with gcc
4.8.4 on Ubuntu 14.04 and I've bisected it to this commit.

# gcc --version
gcc (Ubuntu 4.8.4-2ubuntu1~14.04) 4.8.4

I'm configuring with:
# ./configure \
--static \
--disable-gtk \
--target-list=aarch64-softmmu

When run under gdb, I get:
Program received signal SIGSEGV, Segmentation fault.
buffer_find_nonzero_offset_ifunc () at ./util/cutils.c:333
333 {
(gdb) bt
#0  buffer_find_nonzero_offset_ifunc () at ./util/cutils.c:333
#1  0x00939c58 in __libc_start_main ()
#2  0x00419337 in _start ()

I confess I don't understand the intricacies here, but I'm willing to
test fixes if you have any ideas for how to make this also work for my
compiler without blindly excluding all gcc < 4.9.

-Aaron

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

[Aaron Lindsay]

On Feb 09 15:11, Alistair Francis wrote:
> On Tue, Feb 9, 2016 at 9:19 AM, Peter Maydell <peter.mayd...@linaro.org> 
> wrote:
> > On 6 February 2016 at 00:55, Alistair Francis
> > <alistair.fran...@xilinx.com> wrote:
> >> Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
> >> Signed-off-by: Alistair Francis <alistair.fran...@xilinx.com>
> >> Tested-by: Nathan Rossi <nat...@nathanrossi.com>
> >> ---
> >>
> >>  target-arm/cpu-qom.h | 2 ++
> >>  target-arm/cpu.c | 2 ++
> >>  target-arm/cpu64.c   | 2 ++
> >>  target-arm/helper.c  | 8 
> >>  4 files changed, 14 insertions(+)
> >>
> >> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> >> index 07c0a71..1cc4502 100644
> >> --- a/target-arm/cpu-qom.h
> >> +++ b/target-arm/cpu-qom.h
> >> @@ -148,6 +148,8 @@ typedef struct ARMCPU {
> >>  uint32_t id_pfr0;
> >>  uint32_t id_pfr1;
> >>  uint32_t id_dfr0;
> >> +uint32_t pmceid0;
> >> +uint32_t pmceid1;
> >>  uint32_t id_afr0;
> >>  uint32_t id_mmfr0;
> >>  uint32_t id_mmfr1;
> >> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> >> index 7ddbf3d..937f845 100644
> >> --- a/target-arm/cpu.c
> >> +++ b/target-arm/cpu.c
> >> @@ -1156,6 +1156,8 @@ static void cortex_a15_initfn(Object *obj)
> >>  cpu->id_pfr0 = 0x1131;
> >>  cpu->id_pfr1 = 0x00011011;
> >>  cpu->id_dfr0 = 0x02010555;
> >> +cpu->pmceid0 = 0x0481; /* PMUv3 events 0x0, 0x8, and 0x11 */
> >
> > These are:
> >  SW_INCR   # insn architecturally executed, cc pass, software increment
> >  INST_RETIRED # insn architecturally executed
> >  CPU_CYCLES # cycle
> >
> > However we don't actually implement any of these, so should
> > we be advertising them?
> 
> So this part I took directly from Chris's RFC. I'm happy to take it
> out if you would like.

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

-Aaron

> 
> >
> >> +cpu->pmceid1 = 0x;
> >>  cpu->id_afr0 = 0x;
> >>  cpu->id_mmfr0 = 0x10201105;
> >>  cpu->id_mmfr1 = 0x2000;
> >> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
> >> index c847513..8c4b6fd 100644
> >> --- a/target-arm/cpu64.c
> >> +++ b/target-arm/cpu64.c
> >> @@ -134,6 +134,8 @@ static void aarch64_a57_initfn(Object *obj)
> >>  cpu->id_isar5 = 0x00011121;
> >>  cpu->id_aa64pfr0 = 0x;
> >>  cpu->id_aa64dfr0 = 0x10305106;
> >> +cpu->pmceid0 = 0x0481; /* PMUv3 events 0x0, 0x8, and 0x11 */
> >> +cpu->pmceid1 = 0x;
> >>  cpu->id_aa64isar0 = 0x00011120;
> >>  cpu->id_aa64mmfr0 = 0x1124;
> >>  cpu->dbgdidr = 0x3516d000;
> >> diff --git a/target-arm/helper.c b/target-arm/helper.c
> >> index 5ea507f..66aa406 100644
> >> --- a/target-arm/helper.c
> >> +++ b/target-arm/helper.c
> >> @@ -4192,6 +4192,14 @@ void register_cp_regs_for_features(ARMCPU *cpu)
> >>.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 1,
> >>.access = PL1_R, .type = ARM_CP_CONST,
> >>.resetvalue = cpu->id_aa64dfr1 },
> >> +{ .name = "PMCEID0_EL0", .state = ARM_CP_STATE_AA64,
> >> +  .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 6,
> >> +  .access = PL1_R, .type = ARM_CP_CONST,
> >> +  .resetvalue = cpu->pmceid0},
> >
> > These have 32-bit versions from v8 and up (sadly not with the
> > right opc values to use STATE_BOTH, so second stanza needed).
> 
> Ok, I have added PMCEID0 and PMCEID1.
> 
> >
> > These are configurably RO from EL0, controlled by PMUSERENR_EL0.EN,
> > so you want
> >.access = PL0_R, .accessfn = pmreg_access
> >
> > Space before final "}", please.
> >
> > Can we move these down so they're not placed right in the
> > middle of the ID_AA64* registers ?
> 
> Fixed the rest.
> 
> Thanks,
> 
> Alistair
> 
> >
> >> +{ .name = "PMCEID1_EL0", .state = ARM_CP_STATE_AA64,
> >> +  .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 7,
> >> +  .access = PL1_R, .type = ARM_CP_CONST,
> >> +  .resetvalue = cpu->pmceid1},
> >
> > Ditto.
> >
> >>  { .name = "ID_AA64AFR0_EL1", .state = ARM_CP_STATE_AA64,
> >>.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 4,
> >>.access = PL1_R, .type = ARM_CP_CONST,
> >> --
> >> 2.5.0
> >
> > thanks
> > -- PMM
> >

Re: [Qemu-devel] [PATCH v1 0/3] Extend the performance monitoring registers

[Aaron Lindsay]

On Feb 04 10:52, Alistair Francis wrote:
> On Thu, Feb 4, 2016 at 5:39 AM, Aaron Lindsay <alind...@codeaurora.org> wrote:
> > Please add my
> > Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
> > to all three.
> 
> Ok, I wasn't sure what you wanted to do there. I'll add them all and
> they will be there in the next version. I won't re-send this one until
> I have some comments. Is that ok with you?

Absolutely.

-Aaron

Re: [Qemu-devel] [PATCH v1 0/3] Extend the performance monitoring registers

[Aaron Lindsay]

Alistair,

On Feb 03 16:34, Alistair Francis wrote:
> This patch set is based on the patch sent by Christopher Covington and
> written by Aaron Lindsay which was sent as an RFC (Implement remaining
> PMU functionality).

These patches look like a good start to improving the PMU support,
thanks for splitting them out for review.

Please add my
Signed-off-by: Aaron Lindsay <alind...@codeaurora.org>
to all three.

If you're so inclined, you can also add me as Author (Chris originally
omitted it for reasons that no longer apply), but that's up to you.

-Aaron