More fully implement ARM PMUv3

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 <[email protected]>

---
 target/arm/cpu64.c | 2 ++
 1 file changed, 2 insertions(+)

target/arm/cpu64.c +0 -0

index 991d764..8c4db31 100644

Hi Aaron,

Quoted 8 lines · 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 <[email protected]>

---
 target/arm/cpu64.c | 2 ++
 1 file changed, 2 insertions(+)

Quoted 6 lines

target/arm/cpu64.c +0 -0

index 991d764..8c4db31 100644

target/arm/cpu64.c static void aarch64_a53_initfn(Object *obj) · line 208

Quoted 8 lines

target/arm/cpu64.c +2 -0

@@ -201,6 +201,8 @@ static void aarch64_a53_initfn(Object *obj)
     cpu->id_isar5 = 0x00011121;
     cpu->id_aa64pfr0 = 0x00002222;
     cpu->id_aa64dfr0 = 0x10305106;
+    cpu->pmceid0 = 0x00000000;
+    cpu->pmceid1 = 0x00000000;
     cpu->id_aa64isar0 = 0x00011120;
     cpu->id_aa64mmfr0 = 0x00001122; /* 40 bit physical addr */
     cpu->dbgdidr = 0x3516d000;

Maybe we can move this at a single place in arm_cpu_post_init():

    if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
        cpu->pmceid0 = 0x00000000;
        cpu->pmceid1 = 0x00000000;
    }

Regards,

Phil.

Hi Aaron,

Quoted 14 lines · 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 <[email protected]>

---
 target/arm/cpu64.c | 2 ++
 1 file changed, 2 insertions(+)

target/arm/cpu64.c +0 -0

index 991d764..8c4db31 100644

target/arm/cpu64.c static void aarch64_a53_initfn(Object *obj) · line 208

Quoted 8 lines

target/arm/cpu64.c +2 -0

@@ -201,6 +201,8 @@ static void aarch64_a53_initfn(Object *obj)
     cpu->id_isar5 = 0x00011121;
     cpu->id_aa64pfr0 = 0x00002222;
     cpu->id_aa64dfr0 = 0x10305106;
+    cpu->pmceid0 = 0x00000000;
+    cpu->pmceid1 = 0x00000000;
     cpu->id_aa64isar0 = 0x00011120;
     cpu->id_aa64mmfr0 = 0x00001122; /* 40 bit physical addr */
     cpu->dbgdidr = 0x3516d000;

Maybe we can move this at a single place in arm_cpu_post_init():

    if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
        cpu->pmceid0 = 0x00000000;
        cpu->pmceid1 = 0x00000000;
    }

Regards,

Phil.

Quoted 28 lines · Philippe Mathieu-Daudé

Hi Aaron,

Quoted 14 lines · 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 <[email protected]>

---
 target/arm/cpu64.c | 2 ++
 1 file changed, 2 insertions(+)

target/arm/cpu64.c +0 -0

index 991d764..8c4db31 100644

target/arm/cpu64.c static void aarch64_a53_initfn(Object *obj) · line 208

Quoted 8 lines

target/arm/cpu64.c +2 -0

@@ -201,6 +201,8 @@ static void aarch64_a53_initfn(Object *obj)
     cpu->id_isar5 = 0x00011121;
     cpu->id_aa64pfr0 = 0x00002222;
     cpu->id_aa64dfr0 = 0x10305106;
+    cpu->pmceid0 = 0x00000000;
+    cpu->pmceid1 = 0x00000000;
     cpu->id_aa64isar0 = 0x00011120;
     cpu->id_aa64mmfr0 = 0x00001122; /* 40 bit physical addr */
     cpu->dbgdidr = 0x3516d000;

Maybe we can move this at a single place in arm_cpu_post_init():

Quoted 4 lines

    if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
        cpu->pmceid0 = 0x00000000;
        cpu->pmceid1 = 0x00000000;
    }

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.

Quoted 34 lines · Aaron Lindsay

Quoted 32 lines · Philippe Mathieu-Daudé

Hi Aaron,

Quoted 14 lines · 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 <[email protected]>

---
 target/arm/cpu64.c | 2 ++
 1 file changed, 2 insertions(+)

target/arm/cpu64.c +0 -0

index 991d764..8c4db31 100644

target/arm/cpu64.c static void aarch64_a53_initfn(Object *obj) · line 208

Quoted 8 lines

target/arm/cpu64.c +2 -0

@@ -201,6 +201,8 @@ static void aarch64_a53_initfn(Object *obj)
     cpu->id_isar5 = 0x00011121;
     cpu->id_aa64pfr0 = 0x00002222;
     cpu->id_aa64dfr0 = 0x10305106;
+    cpu->pmceid0 = 0x00000000;
+    cpu->pmceid1 = 0x00000000;
     cpu->id_aa64isar0 = 0x00011120;
     cpu->id_aa64mmfr0 = 0x00001122; /* 40 bit physical addr */
     cpu->dbgdidr = 0x3516d000;

Maybe we can move this at a single place in arm_cpu_post_init():

    if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
        cpu->pmceid0 = 0x00000000;
        cpu->pmceid1 = 0x00000000;
    }

Quoted 5 lines

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.

Signed-off-by Aaron Lindsay <[email protected]>

---
 target/arm/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

target/arm/cpu.c +0 -0

index 022d8c5..072cbbf 100644

They share the same underlying state

Signed-off-by Aaron Lindsay <[email protected]>

---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

target/arm/helper.c +0 -0

index 5e48982..5634561 100644

Quoted 17 lines · Aaron Lindsay

They share the same underlying state

Signed-off-by Aaron Lindsay <[email protected]>

---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

target/arm/helper.c +1 -1

index 5e48982..5634561 100644
@@ -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,

target/arm/helper.c static const ARMCPRegInfo v7_cp_reginfo[] = {

Quoted 4 lines

target/arm/helper.c +0 -1

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

Reviewed-by Peter Maydell <[email protected]>

thanks
-- PMM

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 <[email protected]>

---
 target/arm/helper.c | 101 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 56 insertions(+), 45 deletions(-)

target/arm/helper.c +0 -0

index 5634561..6480b80 100644

target/arm/helper.c +9 -0

+/*
+ * 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().
+ */

Quoted 16 lines · 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 <[email protected]>

---
 target/arm/helper.c | 101 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 56 insertions(+), 45 deletions(-)

target/arm/helper.c +0 -0

index 5634561..6480b80 100644
@@ -1000,28 +1000,58 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
 

target/arm/helper.c static inline bool arm_ccnt_enabled(CPUARMState *env) · line 1004

Quoted 4 lines

target/arm/helper.c +0 -2

     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 ?

Quoted 15 lines

target/arm/helper.c +11 -3

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

target/arm/helper.c static inline bool arm_ccnt_enabled(CPUARMState *env) · line 1018

Quoted 4 lines

target/arm/helper.c +3 -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.

Otherwise

Reviewed-by Peter Maydell <[email protected]>

This is in preparation for enabling counters other than PMCCNTR

Signed-off-by Aaron Lindsay <[email protected]>

---
 target/arm/helper.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

target/arm/helper.c +0 -5

index 6480b80..5d5c738 100644
@@ -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
 };
 

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 <[email protected]>

---
 hw/intc/arm_gicv3_cpuif.c | 10 ++--------
 target/arm/cpu.h          | 10 ----------
 2 files changed, 2 insertions(+), 18 deletions(-)

Signed-off-by Aaron Lindsay <[email protected]>

---
 target/arm/cpu.c       | 15 ++++++++++-----
 target/arm/cpu.h       | 23 ++++++++++++-----------
 target/arm/internals.h |  7 ++++---
 3 files changed, 26 insertions(+), 19 deletions(-)

target/arm/cpu.c +0 -0

index 072cbbf..5f782bf 100644
@@ -55,13 +55,16 @@ static bool arm_cpu_has_work(CPUState *cs)
          | CPU_INTERRUPT_EXITTB);
 }
 

target/arm/cpu.c +5 -0

+
+    entry->hook = hook;
+    entry->opaque = opaque;
+
+    QLIST_INSERT_HEAD(&cpu->el_change_hooks, entry, node);
 }
 
 static void cp_reg_reset(gpointer key, gpointer value, gpointer opaque)

target/arm/cpu.c +0 -0

     /* Some features automatically imply others: */
     if (arm_feature(env, ARM_FEATURE_V8)) {
         set_feature(env, ARM_FEATURE_V7);

target/arm/cpu.h +12 -11

index f17592b..3b45d3d 100644
@@ -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);
 
 /**

target/arm/internals.h +4 -3

index 47cc224..7df3eda 100644
@@ -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, &cpu->el_change_hooks, node, next) {
+        hook->hook(cpu, hook->opaque);
     }
 }
 

Quoted 15 lines · Aaron Lindsay

Signed-off-by Aaron Lindsay <[email protected]>

---
 target/arm/cpu.c       | 15 ++++++++++-----
 target/arm/cpu.h       | 23 ++++++++++++-----------
 target/arm/internals.h |  7 ++++---
 3 files changed, 26 insertions(+), 19 deletions(-)

target/arm/cpu.c +0 -0

index 072cbbf..5f782bf 100644
@@ -55,13 +55,16 @@ static bool arm_cpu_has_work(CPUState *cs)
          | CPU_INTERRUPT_EXITTB);
 }
 

target/arm/cpu.c static bool arm_cpu_has_work(CPUState *cs) · line 62

Quoted 10 lines

target/arm/cpu.c +3 -5

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

Quoted 10 lines

target/arm/cpu.c +5 -0

+
+    entry->hook = hook;
+    entry->opaque = opaque;
+
+    QLIST_INSERT_HEAD(&cpu->el_change_hooks, entry, node);
 }
 
 static void cp_reg_reset(gpointer key, gpointer value, gpointer opaque)

target/arm/cpu.c static void arm_cpu_realizefn(DeviceState *dev, Error **errp) · line 751

Quoted 5 lines

target/arm/cpu.c +2 -0

@@ -744,6 +747,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
+    QLIST_INIT(&cpu->el_change_hooks);
+

You missed to fill arm_cpu_unrealizefn() with:

    QLIST_FOREACH(...) {
        QLIST_REMOVE(...);
        g_free(...);
    }

Quoted 15 lines · Aaron Lindsay

Signed-off-by Aaron Lindsay <[email protected]>

---
 target/arm/cpu.c       | 15 ++++++++++-----
 target/arm/cpu.h       | 23 ++++++++++++-----------
 target/arm/internals.h |  7 ++++---
 3 files changed, 26 insertions(+), 19 deletions(-)

target/arm/cpu.c +0 -0

index 072cbbf..5f782bf 100644
@@ -55,13 +55,16 @@ static bool arm_cpu_has_work(CPUState *cs)
          | CPU_INTERRUPT_EXITTB);
 }
 

target/arm/cpu.c static bool arm_cpu_has_work(CPUState *cs) · line 62

Quoted 10 lines

target/arm/cpu.c +3 -5

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

target/arm/cpu.c static void arm_cpu_realizefn(DeviceState *dev,

Quoted 10 lines

target/arm/cpu.c +5 -0

+
+    entry->hook = hook;
+    entry->opaque = opaque;
+
+    QLIST_INSERT_HEAD(&cpu->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,

target/arm/cpu.c static void arm_cpu_realizefn(DeviceState *dev, Error **errp) · line 751

Quoted 5 lines

target/arm/cpu.c +2 -0

         return;
     }
 
+    QLIST_INIT(&cpu->el_change_hooks);
+

You missed to fill arm_cpu_unrealizefn() with:

    QLIST_FOREACH(...) {
        QLIST_REMOVE(...);
        g_free(...);
    }

Signed-off-by Aaron Lindsay <[email protected]>

---
 target/arm/cpu.c       | 15 ++++++++++-----
 target/arm/cpu.h       | 23 ++++++++++++-----------
 target/arm/internals.h |  7 ++++---
 3 files changed, 26 insertions(+), 19 deletions(-)

target/arm/cpu.c +0 -0

index 072cbbf..5f782bf 100644
@@ -55,13 +55,16 @@ static bool arm_cpu_has_work(CPUState *cs)
          | CPU_INTERRUPT_EXITTB);
 }
 

target/arm/cpu.c +5 -0

+
+    entry->hook = hook;
+    entry->opaque = opaque;
+
+    QLIST_INSERT_HEAD(&cpu->el_change_hooks, entry, node);
 }
 
 static void cp_reg_reset(gpointer key, gpointer value, gpointer opaque)

target/arm/cpu.c +0 -0

     /* Some features automatically imply others: */
     if (arm_feature(env, ARM_FEATURE_V8)) {
         set_feature(env, ARM_FEATURE_V7);

target/arm/cpu.h +0 -0

index f17592b..3b45d3d 100644

target/arm/cpu.h +9 -9

-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);
 
 /**

target/arm/internals.h +1 -1

index 47cc224..7df3eda 100644
@@ -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 */

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 <[email protected]>

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

target/arm/cpu.c +0 -0

index 5f782bf..a2cb21e 100644

target/arm/cpu.c +8 -0

+
+    entry->hook = hook;
+    entry->opaque = opaque;
+
+    QLIST_INSERT_HEAD(&cpu->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(&cpu->pre_el_change_hooks);
     QLIST_INIT(&cpu->el_change_hooks);
 
     /* Some features automatically imply others: */

target/arm/cpu.h +2 -0

index 3b45d3d..b0ef727 100644
@@ -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:

Quoted 19 lines · 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 <[email protected]>

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

target/arm/cpu.c +0 -0

index 5f782bf..a2cb21e 100644

target/arm/cpu.c static bool arm_cpu_has_work(CPUState *cs) · line 62

Quoted 8 lines

target/arm/cpu.c +5 -0

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

Quoted 35 lines

target/arm/cpu.c +8 -0

+
+    entry->hook = hook;
+    entry->opaque = opaque;
+
+    QLIST_INSERT_HEAD(&cpu->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(&cpu->pre_el_change_hooks);
     QLIST_INIT(&cpu->el_change_hooks);
 
     /* Some features automatically imply others: */

target/arm/cpu.h +2 -0

index 3b45d3d..b0ef727 100644
@@ -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:

target/arm/cpu.h static inline AddressSpace *arm_addressspace(CPUState *cs, MemTxAttrs attrs) · line 2905

Quoted 10 lines

target/arm/cpu.h +4 -4

  * 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?)

Otherwise looks OK.

thanks
-- PMM

Quoted 3 lines · Peter Maydell

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.

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 <[email protected]>

---
 target/arm/translate-a64.c | 2 ++
 target/arm/translate.c     | 4 ++++
 2 files changed, 6 insertions(+)

target/arm/translate-a64.c +0 -0

index 31ff047..e1ae676 100644

target/arm/translate-a64.c +0 -0

         /* Must exit loop to check un-masked IRQs */
         s->base.is_jmp = DISAS_EXIT;
         return;

target/arm/translate.c +4 -1

index ba6ab7d..fd5871e 100644
@@ -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.

Quoted 16 lines · 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 <[email protected]>

---
 target/arm/translate-a64.c | 2 ++
 target/arm/translate.c     | 4 ++++
 2 files changed, 6 insertions(+)

target/arm/translate-a64.c +0 -0

index 31ff047..e1ae676 100644

target/arm/translate-a64.c static void disas_uncond_b_reg(DisasContext *s, uint32_t insn) · line 1924

Quoted 6 lines

target/arm/translate-a64.c +2 -0

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

target/arm/translate-a64.c static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)

Quoted 7 lines

target/arm/translate-a64.c +6 -0

+        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();
+        }

Quoted 29 lines

         /* Must exit loop to check un-masked IRQs */
         s->base.is_jmp = DISAS_EXIT;
         return;

target/arm/translate.c +4 -1

index ba6ab7d..fd5871e 100644
@@ -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;
--

Quoted 4 lines

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

Quoted 27 lines · Peter Maydell

Quoted 16 lines · 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 <[email protected]>

---
 target/arm/translate-a64.c | 2 ++
 target/arm/translate.c     | 4 ++++
 2 files changed, 6 insertions(+)

target/arm/translate-a64.c +0 -0

index 31ff047..e1ae676 100644

target/arm/translate-a64.c static void disas_uncond_b_reg(DisasContext *s, uint32_t insn) · line 1924

Quoted 6 lines

target/arm/translate-a64.c +2 -0

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

Quoted 1 line

(Ditto in the other cases below.)

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

It was shifted to the left one bit too few.

Signed-off-by Aaron Lindsay <[email protected]>

---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

target/arm/helper.c +0 -0

index 50eaed7..0102357 100644

Quoted 12 lines · Aaron Lindsay

It was shifted to the left one bit too few.

Signed-off-by Aaron Lindsay <[email protected]>

---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

target/arm/helper.c +0 -0

index 50eaed7..0102357 100644

target/arm/helper.c static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri, · line 1129

Quoted 8 lines

target/arm/helper.c +1 -1

@@ -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 & 0x7E000000;
+    env->cp15.pmccfiltr_el0 = value & 0xfc000000;
     pmccntr_op_finish(env, saved_cycles);
 }
 

I wonder why we got that one wrong.

Reviewed-by Peter Maydell <[email protected]>

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.

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 <[email protected]>

---
 target/arm/cpu.c    |  3 ++
 target/arm/cpu.h    | 37 +++++++++++++++++++----
 target/arm/helper.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 116 insertions(+), 11 deletions(-)

target/arm/cpu.c +0 -0

index a2cb21e..b0d032c 100644

target/arm/cpu.c +0 -0

     }
 
     if (!arm_feature(env, ARM_FEATURE_EL2)) {

target/arm/cpu.h +0 -0

index b0ef727..9c3b5ef 100644

target/arm/cpu.h +25 -5

  * @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

target/arm/helper.c +10 -1

index 0102357..95b09d6 100644
@@ -908,6 +908,15 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 #define PMCRC   0x4
 #define PMCRE   0x1
 
+#define PMXEVTYPER_P          0x80000000
+#define PMXEVTYPER_U          0x40000000
+#define PMXEVTYPER_NSK        0x20000000
+#define PMXEVTYPER_NSU        0x10000000
+#define PMXEVTYPER_NSH        0x08000000
+#define PMXEVTYPER_M          0x04000000
+#define PMXEVTYPER_MT         0x02000000
+#define PMXEVTYPER_EVTCOUNT   0x000003ff
+
 #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;

target/arm/helper.c +10 -0

+
+    if (el == 1 && P) {
+        return true;
+    } else if (el == 0 && U) {
+        return true;
+    }
+
+    if (arm_feature(env, ARM_FEATURE_EL3)) {
+        if (el == 1 && !secure && NSK != P) {
+            return true;

target/arm/helper.c +7 -2

+    return false;
+}
+
 /*
  * Ensure c15_ccnt is the guest-visible count so that operations such as
  * enabling/disabling the counter or filtering, modifying the count itself,

@@ -1023,7 +1070,8 @@ uint64_t pmccntr_op_start(CPUARMState *env)
                           ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
 #endif
 
-    if (arm_ccnt_enabled(env)) {
+    if (arm_ccnt_enabled(env) &&
+          !pmu_counter_filtered(env, env->cp15.pmccfiltr_el0)) {
 
         uint64_t eff_cycles = cycles;
         if (env->cp15.c9_pmcr & PMCRD) {

@@ -1043,7 +1091,8 @@ uint64_t pmccntr_op_start(CPUARMState *env)
  */
 void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
 {
-    if (arm_ccnt_enabled(env)) {
+    if (arm_ccnt_enabled(env) &&
+          !pmu_counter_filtered(env, env->cp15.pmccfiltr_el0)) {
         if (env->cp15.c9_pmcr & PMCRD) {
             /* Increment once every 64 processor clock cycles */

target/arm/helper.c +20 -2

+
+void pmu_op_finish(CPUARMState *env, uint64_t prev_cycles)
+{
+    pmccntr_op_finish(env, prev_cycles);
+}
+
+void pmu_pre_el_change(ARMCPU *cpu, void *ignored)
+{
+    cpu->env.cp15.ccnt_cached_cycles = pmu_op_start(&cpu->env);
+}
+
+void pmu_post_el_change(ARMCPU *cpu, void *ignored)
+{
+    pmu_op_finish(&cpu->env, cpu->env.cp15.ccnt_cached_cycles);
+}
+
 static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                        uint64_t value)
 {
-    uint64_t saved_cycles = pmccntr_op_start(env);
+    uint64_t saved_cycles = pmu_op_start(env);
 
     if (value & PMCRC) {
         /* The counter has been reset */

@@ -1068,7 +1137,7 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     env->cp15.c9_pmcr &= ~0x39;
     env->cp15.c9_pmcr |= (value & 0x39);
 
-    pmccntr_op_finish(env, saved_cycles);
+    pmu_op_finish(env, saved_cycles);
 }
 
 static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)

@@ -1117,6 +1186,14 @@ void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
 {
 }
 
+uint64_t pmu_op_start(CPUARMState *env)
+{

Quoted 20 lines · 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 <[email protected]>

---
 target/arm/cpu.c    |  3 ++
 target/arm/cpu.h    | 37 +++++++++++++++++++----
 target/arm/helper.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 116 insertions(+), 11 deletions(-)

target/arm/cpu.c +1 -0

index a2cb21e..b0d032c 100644
@@ -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 {

target/arm/cpu.c static void arm_cpu_realizefn(DeviceState *dev, Error **errp) · line 892

Quoted 2 lines

target/arm/cpu.c +2 -0

+        arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0);
+        arm_register_el_change_hook(cpu, &pmu_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.)

Quoted 12 lines

target/arm/cpu.c +0 -0

     }
 
     if (!arm_feature(env, ARM_FEATURE_EL2)) {

target/arm/cpu.h +0 -0

index b0ef727..9c3b5ef 100644
@@ -458,6 +458,11 @@ typedef struct CPUARMState {
          * was reset. Otherwise it stores the counter value
          */
         uint64_t c15_ccnt;

target/arm/cpu.h typedef struct CPUARMState { · line 465

Quoted 5 lines

target/arm/cpu.h +5 -0

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

Quoted 7 lines

target/arm/cpu.h +0 -0

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

target/arm/cpu.h int cpu_arm_signal_handler(int host_signum, void *pinfo, · line 904

Quoted 3 lines

target/arm/cpu.h +1 -1

 /**
- * pmccntr_sync
+ * pmccntr_op_start/finish

Shouldn't this doc change and prototype change have gone with the earlier patch that changed the implementation?

Quoted 66 lines

target/arm/cpu.h +25 -5

  * @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

target/arm/helper.c +10 -1

index 0102357..95b09d6 100644
@@ -908,6 +908,15 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 #define PMCRC   0x4
 #define PMCRE   0x1
 
+#define PMXEVTYPER_P          0x80000000
+#define PMXEVTYPER_U          0x40000000
+#define PMXEVTYPER_NSK        0x20000000
+#define PMXEVTYPER_NSU        0x10000000
+#define PMXEVTYPER_NSH        0x08000000
+#define PMXEVTYPER_M          0x04000000
+#define PMXEVTYPER_MT         0x02000000
+#define PMXEVTYPER_EVTCOUNT   0x000003ff
+
 #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;

target/arm/helper.c static inline bool arm_ccnt_enabled(CPUARMState *env) · line 1020

Quoted 6 lines

target/arm/helper.c +3 -0

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

"is filtered (ie does not count events)" would make it clearer.

Nit: */ should be on a line of its own.

target/arm/helper.c static inline bool arm_ccnt_enabled(CPUARMState *env) · line 1031

Quoted 12 lines

target/arm/helper.c +11 -0

+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;
+    bool NSU = pmxevtyper & PMXEVTYPER_NSU;
+    bool NSH = pmxevtyper & PMXEVTYPER_NSH;
+    bool M   = pmxevtyper & PMXEVTYPER_M;

Lowercase for variable names, please.

Quoted 18 lines

target/arm/helper.c +18 -0

+
+    if (el == 1 && P) {
+        return true;
+    } else if (el == 0 && U) {
+        return true;
+    }
+
+    if (arm_feature(env, ARM_FEATURE_EL3)) {
+        if (el == 1 && !secure && NSK != P) {
+            return true;
+        } else if (el == 0 && !secure && NSU != U) {
+            return true;
+        } else if (el == 3 && secure && M != P) {
+            return true;
+        }
+    }
+
+    if (arm_feature(env, ARM_FEATURE_EL2) && el == 2 && !secure && !NSH) {

target/arm/helper.c static inline bool arm_ccnt_enabled(CPUARMState *env) · line 1051

Quoted 2 lines

target/arm/helper.c +2 -0

+        return true;
+    }

This doesn't follow the structure that the Arm ARM pseudocode uses, which makes it a bit tricky to review. (see AArch64.CountEvents()). It also doesn't implement the same logic -- for instance this code will return true if we're in NS EL1 and the U bit is set, whereas the pseudocode returns true for NS EL1 only if U != NSU.

Quoted 29 lines

target/arm/helper.c +7 -2

+    return false;
+}
+
 /*
  * Ensure c15_ccnt is the guest-visible count so that operations such as
  * enabling/disabling the counter or filtering, modifying the count itself,

@@ -1023,7 +1070,8 @@ uint64_t pmccntr_op_start(CPUARMState *env)
                           ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
 #endif
 
-    if (arm_ccnt_enabled(env)) {
+    if (arm_ccnt_enabled(env) &&
+          !pmu_counter_filtered(env, env->cp15.pmccfiltr_el0)) {
 
         uint64_t eff_cycles = cycles;
         if (env->cp15.c9_pmcr & PMCRD) {

@@ -1043,7 +1091,8 @@ uint64_t pmccntr_op_start(CPUARMState *env)
  */
 void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
 {
-    if (arm_ccnt_enabled(env)) {
+    if (arm_ccnt_enabled(env) &&
+          !pmu_counter_filtered(env, env->cp15.pmccfiltr_el0)) {
         if (env->cp15.c9_pmcr & PMCRD) {
             /* Increment once every 64 processor clock cycles */

@@ -1054,10 +1103,30 @@ void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles)
     }
 }
 

target/arm/helper.c void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles) · line 1109

Quoted 4 lines

target/arm/helper.c +4 -0

+uint64_t pmu_op_start(CPUARMState *env)
+{
+    return pmccntr_op_start(env);
+}

Do these two functions end up being different at the end of the patchset?

Quoted 17 lines · Peter Maydell

target/arm/cpu.h typedef struct CPUARMState { · line 465

Quoted 13 lines · Aaron Lindsay

target/arm/cpu.h +5 -0

index b0ef727..9c3b5ef 100644
@@ -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.

Quoted 19 lines · Aaron Lindsay

Quoted 17 lines · Peter Maydell

target/arm/cpu.h typedef struct CPUARMState { · line 465

Quoted 13 lines · Aaron Lindsay

target/arm/cpu.h +5 -0

index b0ef727..9c3b5ef 100644
@@ -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 ?

Quoted 4 lines

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?

Quoted 25 lines · Aaron Lindsay

Quoted 19 lines · Aaron Lindsay

Quoted 17 lines · Peter Maydell

target/arm/cpu.h typedef struct CPUARMState { · line 465

Quoted 13 lines · Aaron Lindsay

target/arm/cpu.h +5 -0

index b0ef727..9c3b5ef 100644
@@ -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 ?

Quoted 4 lines

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.

Quoted 6 lines

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.

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

Signed-off-by Aaron Lindsay <[email protected]>

---
 target/arm/cpu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

target/arm/cpu.h +0 -0

index 9c3b5ef..fb2f983 100644

Hi Aaron,

Quoted 13 lines · 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 <[email protected]>

---
 target/arm/cpu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

target/arm/cpu.h +0 -0

index 9c3b5ef..fb2f983 100644

target/arm/cpu.h typedef struct CPUARMState { · line 370

Quoted 5 lines

target/arm/cpu.h +1 -1

@@ -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", ...

target/arm/cpu.h typedef struct CPUARMState { · line 371

Quoted 1 line

target/arm/cpu.h +0 -0

         uint32_t c9_pmuserenr; /* perf monitor user enable */

target/arm/cpu.h typedef struct CPUARMState { · line 373

Quoted 3 lines

target/arm/cpu.h +0 -0

         uint64_t c9_pmselr; /* perf monitor counter selection register */
         uint64_t c9_pminten; /* perf monitor interrupt enables */

Regards,

Phil.

Phil,

Quoted 27 lines · Philippe Mathieu-Daudé

Hi Aaron,

Quoted 13 lines · 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 <[email protected]>

---
 target/arm/cpu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

target/arm/cpu.h +0 -0

index 9c3b5ef..fb2f983 100644

target/arm/cpu.h typedef struct CPUARMState { · line 370

Quoted 5 lines

target/arm/cpu.h +1 -1

@@ -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", ...

Review

Quoted 5 lines

(quoted diff) +1 -1

-     ..., .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).

Quoted 36 lines · Aaron Lindsay

Phil,

Quoted 27 lines · Philippe Mathieu-Daudé

Hi Aaron,

Quoted 13 lines · 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 <[email protected]>

---
 target/arm/cpu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

target/arm/cpu.h +0 -0

index 9c3b5ef..fb2f983 100644

target/arm/cpu.h typedef struct CPUARMState { · line 370

Quoted 5 lines

target/arm/cpu.h +1 -1

@@ -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", ...

Review

Quoted 5 lines

(quoted diff) +1 -1

-     ..., .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
+     ..., .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
      .accessfn = pmreg_access,
      .writefn = pmovsr_write,
      .raw_writefn = raw_write },

Quoted 5 lines

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

Correct. Technically there's no such thing as a 32-bit wide AArch64 system register -- that is just a shorthand in the Arm ARM for "64-bit wide with the top 32-bits being RES0".

Signed-off-by Aaron Lindsay <[email protected]>

---
 target/arm/cpu.c | 3 +++
 target/arm/cpu.h | 1 +
 2 files changed, 4 insertions(+)

target/arm/cpu.c +3 -0

index b0d032c..e544f1d 100644
@@ -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(&cpu->env, ARM_FEATURE_V7);
+    set_feature(&cpu->env, ARM_FEATURE_V7VE);
     set_feature(&cpu->env, ARM_FEATURE_VFP4);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);

@@ -1526,6 +1528,7 @@ static void cortex_a15_initfn(Object *obj)
 
     cpu->dtb_compatible = "arm,cortex-a15";
     set_feature(&cpu->env, ARM_FEATURE_V7);
+    set_feature(&cpu->env, ARM_FEATURE_V7VE);
     set_feature(&cpu->env, ARM_FEATURE_VFP4);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);

target/arm/cpu.h +1 -0

index fb2f983..cc1e2fb 100644
@@ -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,

Signed-off-by Aaron Lindsay <[email protected]>

---
 target/arm/cpu.c | 3 +++
 target/arm/cpu.h | 1 +
 2 files changed, 4 insertions(+)

target/arm/cpu.c +3 -0

index b0d032c..e544f1d 100644
@@ -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(&cpu->env, ARM_FEATURE_V7);
+    set_feature(&cpu->env, ARM_FEATURE_V7VE);
     set_feature(&cpu->env, ARM_FEATURE_VFP4);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);

@@ -1526,6 +1528,7 @@ static void cortex_a15_initfn(Object *obj)
 
     cpu->dtb_compatible = "arm,cortex-a15";
     set_feature(&cpu->env, ARM_FEATURE_V7);
+    set_feature(&cpu->env, ARM_FEATURE_V7VE);
     set_feature(&cpu->env, ARM_FEATURE_VFP4);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);

target/arm/cpu.h +0 -0

index fb2f983..cc1e2fb 100644

Quoted 40 lines · Aaron Lindsay

Signed-off-by Aaron Lindsay <[email protected]>

---
 target/arm/cpu.c | 3 +++
 target/arm/cpu.h | 1 +
 2 files changed, 4 insertions(+)

target/arm/cpu.c +3 -0

index b0d032c..e544f1d 100644
@@ -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(&cpu->env, ARM_FEATURE_V7);
+    set_feature(&cpu->env, ARM_FEATURE_V7VE);
     set_feature(&cpu->env, ARM_FEATURE_VFP4);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);

@@ -1526,6 +1528,7 @@ static void cortex_a15_initfn(Object *obj)
 
     cpu->dtb_compatible = "arm,cortex-a15";
     set_feature(&cpu->env, ARM_FEATURE_V7);
+    set_feature(&cpu->env, ARM_FEATURE_V7VE);
     set_feature(&cpu->env, ARM_FEATURE_VFP4);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);

target/arm/cpu.h +0 -0

index fb2f983..cc1e2fb 100644
@@ -1439,6 +1439,7 @@ enum arm_features {
     ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling.  */

target/arm/cpu.h enum arm_features { · line 1445

Quoted 6 lines

target/arm/cpu.h +1 -0

     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 ?

Quoted 1 line · Peter Maydell

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?

Quoted 10 lines · Aaron Lindsay

Quoted 1 line · Peter Maydell

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?

So, the underlying issue here is that there's a QEMU specific fudge going on. Architecturally, if the CPU implements the Virtualization Extensions, then:
* it has Hyp mode
* it must also implement the Security Extensions
* on reset it starts in the Secure world
* it has LPAE
* it has some stuff that is not inherently tied to having EL2, like the SDIV and UDIV instructions, and the presence of PMOVSSET

In an ideal world, we'd just have a feature flag that turned all that on. Unfortunately, a combination of backwards compatibility issues, the order in which various features were implemented in QEMU, and the fact that KVM can't emulate a guest CPU with the Security Extensions means that we want to be able to model variants of some CPUs that don't really exist in real hardware:
Cortex-A15 and -A7 which only implement EL0/EL1 but still have all the v7VE features that you can see from those ELs. But we didn't really properly lay out guidelines for how the feature bits should work in this case, with the result that we have a bunch of local hacks (for instance get_S1prot() has a check on the LPAE feature bit, since in practice that bit is set in exactly the CPUs that have v7VE; and the UDIV/SDIV insns have their own feature bits.)

So we should probably sort out this mess first, either by:

(a) state that we use ARM_FEATURE_LPAE for all checks for features that are architecturally v7VE but which we want to exist even on our v7VE-no-Hyp-no-Secure oddballs

(b) define an ARM_FEATURE_V7VE for them

(c) define separate feature bits for them individually

In any case we'd retain ARM_FEATURE_EL2 for "and really has EL2/Hyp mode", and we'd want to do an audit of current uses of various feature bits to see whether they followed the new rules.

(For AArch64 things are a bit less awkward because the architecture allows the idea of an implementation that has EL2 but not EL3.)

Quoted 10 lines · Aaron Lindsay

Quoted 1 line · Peter Maydell

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?

So, the underlying issue here is that there's a QEMU specific fudge going on. Architecturally, if the CPU implements the Virtualization Extensions, then:
* it has Hyp mode
* it must also implement the Security Extensions
* on reset it starts in the Secure world
* it has LPAE
* it has some stuff that is not inherently tied to having EL2, like the SDIV and UDIV instructions, and the presence of PMOVSSET

In an ideal world, we'd just have a feature flag that turned all that on. Unfortunately, a combination of backwards compatibility issues, the order in which various features were implemented in QEMU, and the fact that KVM can't emulate a guest CPU with the Security Extensions means that we want to be able to model variants of some CPUs that don't really exist in real hardware:
Cortex-A15 and -A7 which only implement EL0/EL1 but still have all the v7VE features that you can see from those ELs. But we didn't really properly lay out guidelines for how the feature bits should work in this case, with the result that we have a bunch of local hacks (for instance get_S1prot() has a check on the LPAE feature bit, since in practice that bit is set in exactly the CPUs that have v7VE; and the UDIV/SDIV insns have their own feature bits.)

So we should probably sort out this mess first, either by:

(a) state that we use ARM_FEATURE_LPAE for all checks for features that are architecturally v7VE but which we want to exist even on our v7VE-no-Hyp-no-Secure oddballs
(b) define an ARM_FEATURE_V7VE for them
(c) define separate feature bits for them individually

From what I can tell, using ARM_FEATURE_LPAE to represent all the

Quoted 26 lines · Peter Maydell

So, the underlying issue here is that there's a QEMU specific fudge going on. Architecturally, if the CPU implements the Virtualization Extensions, then:
* it has Hyp mode
* it must also implement the Security Extensions
* on reset it starts in the Secure world
* it has LPAE
* it has some stuff that is not inherently tied to having EL2, like the SDIV and UDIV instructions, and the presence of PMOVSSET

In an ideal world, we'd just have a feature flag that turned all that on. Unfortunately, a combination of backwards compatibility issues, the order in which various features were implemented in QEMU, and the fact that KVM can't emulate a guest CPU with the Security Extensions means that we want to be able to model variants of some CPUs that don't really exist in real hardware:
Cortex-A15 and -A7 which only implement EL0/EL1 but still have all the v7VE features that you can see from those ELs. But we didn't really properly lay out guidelines for how the feature bits should work in this case, with the result that we have a bunch of local hacks (for instance get_S1prot() has a check on the LPAE feature bit, since in practice that bit is set in exactly the CPUs that have v7VE; and the UDIV/SDIV insns have their own feature bits.)

Quoted 7 lines

So we should probably sort out this mess first, either by:

(a) state that we use ARM_FEATURE_LPAE for all checks for features that are architecturally v7VE but which we want to exist even on our v7VE-no-Hyp-no-Secure oddballs
(b) define an ARM_FEATURE_V7VE for them
(c) define separate feature bits for them individually

From what I can tell, using ARM_FEATURE_LPAE to represent all the almost-v7ve misfits won't work well because ARM_FEATURE_ARM_DIV may be supported on some platforms for which ARM_FEATURE_LPAE is not (Cortex R5), and ARM_FEATURE_ARM_DIV is read from ID_ISAR0 in kvm_arm_get_host_cpu_features() (and may be set/not set independently of ARM_FEATURE_LPAE). It appears there is a need to independently distinguish between them.

The same reasoning also seems to rule out
option (b) "one ARM_FEATURE_V7VE to rule them all", leaving me with option (c).

It almost seems silly to create ARM_FEATURE_PMOVSSET, but I'm not sure what else makes sense to do here. Am I missing something (I'm almost hoping I am)?