[PATCH v3 0/9] target/arm: Implement SEL2 physical and virtual timers

[PATCH v3 0/9] target/arm: Implement SEL2 physical and virtual timers

[Peter Maydell]

This patchset is a respin of Alex's patches, with some extra fixes
for bugs I discovered along the way in our existing code (and
a bit of refactoring to make the fixes straightforward). It is:

Based-on: 20250130182309.717346-1-peter.maydell@linaro.org
("target/arm: Clean up some corner cases of sysreg traps")

because it wants to use the renamed CP_ACCESS_* constants that
that patchset introduced.

The bugfixes are not super exciting as they mostly are oddball
corner cases, but I've cc'd them to stable anyway. The actual
implementation of the missing SEL2 timers also should go to stable.

Alex Bennée (4):
  target/arm: Implement SEL2 physical and virtual timers
  target/arm: document the architectural names of our GTIMERs
  hw/arm: enable secure EL2 timers for virt machine
  hw/arm: enable secure EL2 timers for sbsa machine

Peter Maydell (5):
  target/arm: Apply correct timer offset when calculating deadlines
  target/arm: Don't apply CNTVOFF_EL2 for EL2_VIRT timer
  target/arm: Make CNTPS_* UNDEF from Secure EL1 when Secure EL2 is
    enabled
  target/arm: Always apply CNTVOFF_EL2 for CNTV_TVAL_EL02 accesses
  target/arm: Refactor handling of timer offset for direct register
    accesses

 include/hw/arm/bsa.h       |   2 +
 target/arm/cpu.h           |   2 +
 target/arm/gtimer.h        |  14 +-
 target/arm/internals.h     |   5 +-
 hw/arm/sbsa-ref.c          |   2 +
 hw/arm/virt.c              |   2 +
 target/arm/cpu.c           |   4 +
 target/arm/helper.c        | 324 ++++++++++++++++++++++++++++++-------
 target/arm/tcg/op_helper.c |   8 +-
 9 files changed, 296 insertions(+), 67 deletions(-)

-- 
2.34.1

[PATCH v3 1/9] target/arm: Apply correct timer offset when calculating deadlines

[Peter Maydell]

When we are calculating timer deadlines, the correct definition of
whether or not to apply an offset to the physical count is described
in the Arm ARM DDI4087 rev L.a section D12.2.4.1.  This is different
from when the offset should be applied for a direct read of the
counter sysreg.

We got this right for the EL1 physical timer and for the EL1 virtual
timer, but got all the rest wrong: they should be using a zero offset
always.

Factor the offset calculation out into a function that has a comment
documenting exactly which offset it is calculating and which gets the
HYP, SEC, and HYPVIRT cases right.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 1fee8fae127..049362a5500 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2460,6 +2460,32 @@ static uint64_t gt_phys_cnt_offset(CPUARMState *env)
     return gt_phys_raw_cnt_offset(env);
 }
 
+static uint64_t gt_indirect_access_timer_offset(CPUARMState *env, int timeridx)
+{
+    /*
+     * Return the timer offset to use for indirect accesses to the timer.
+     * This is the Offset value as defined in D12.2.4.1 "Operation of the
+     * CompareValue views of the timers".
+     *
+     * The condition here is not always the same as the condition for
+     * whether to apply an offset register when doing a direct read of
+     * the counter sysreg; those conditions are described in the
+     * access pseudocode for each counter register.
+     */
+    switch (timeridx) {
+    case GTIMER_PHYS:
+        return gt_phys_raw_cnt_offset(env);
+    case GTIMER_VIRT:
+        return env->cp15.cntvoff_el2;
+    case GTIMER_HYP:
+    case GTIMER_SEC:
+    case GTIMER_HYPVIRT:
+        return 0;
+    default:
+        g_assert_not_reached();
+    }
+}
+
 static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
 {
     ARMGenericTimer *gt = &cpu->env.cp15.c14_timer[timeridx];
@@ -2469,8 +2495,7 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
          * Timer enabled: calculate and set current ISTATUS, irq, and
          * reset timer to when ISTATUS next has to change
          */
-        uint64_t offset = timeridx == GTIMER_VIRT ?
-            cpu->env.cp15.cntvoff_el2 : gt_phys_raw_cnt_offset(&cpu->env);
+        uint64_t offset = gt_indirect_access_timer_offset(&cpu->env, timeridx);
         uint64_t count = gt_get_countervalue(&cpu->env);
         /* Note that this must be unsigned 64 bit arithmetic: */
         int istatus = count - offset >= gt->cval;
-- 
2.34.1

[PATCH v3 2/9] target/arm: Don't apply CNTVOFF_EL2 for EL2_VIRT timer

[Peter Maydell]

The CNTVOFF_EL2 offset register should only be applied for accessses
to CNTVCT_EL0 and for the EL1 virtual timer (CNTV_*).  We were
incorrectly applying it for the EL2 virtual timer (CNTHV_*).

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 049362a5500..ac8cb428925 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2604,7 +2604,6 @@ static uint64_t gt_tval_read(CPUARMState *env, const ARMCPRegInfo *ri,
 
     switch (timeridx) {
     case GTIMER_VIRT:
-    case GTIMER_HYPVIRT:
         offset = gt_virt_cnt_offset(env);
         break;
     case GTIMER_PHYS:
@@ -2624,7 +2623,6 @@ static void gt_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
 
     switch (timeridx) {
     case GTIMER_VIRT:
-    case GTIMER_HYPVIRT:
         offset = gt_virt_cnt_offset(env);
         break;
     case GTIMER_PHYS:
-- 
2.34.1

[PATCH v3 3/9] target/arm: Make CNTPS_* UNDEF from Secure EL1 when Secure EL2 is enabled

[Peter Maydell]

When we added Secure EL2 support, we missed that this needs an update
to the access code for the EL3 physical timer registers.  These are
supposed to UNDEF from Secure EL1 when Secure EL2 is enabled.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index ac8cb428925..7ec1e6cfaab 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2387,6 +2387,9 @@ static CPAccessResult gt_stimer_access(CPUARMState *env,
         if (!arm_is_secure(env)) {
             return CP_ACCESS_UNDEFINED;
         }
+        if (arm_is_el2_enabled(env)) {
+            return CP_ACCESS_UNDEFINED;
+        }
         if (!(env->cp15.scr_el3 & SCR_ST)) {
             return CP_ACCESS_TRAP_EL3;
         }
-- 
2.34.1

[PATCH v3 4/9] target/arm: Always apply CNTVOFF_EL2 for CNTV_TVAL_EL02 accesses

[Peter Maydell]

Currently we handle CNTV_TVAL_EL02 by calling gt_tval_read() for the
EL1 virt timer.  This is almost correct, but the underlying
CNTV_TVAL_EL0 register behaves slightly differently.  CNTV_TVAL_EL02
always applies the CNTVOFF_EL2 offset; CNTV_TVAL_EL0 doesn't do so if
we're at EL2 and HCR_EL2.E2H is 1.

We were getting this wrong, because we ended up in
gt_virt_cnt_offset() and did the E2H check.

Factor out the tval read/write calculation from the selection of the
offset, so that we can special case gt_virt_tval_read() and
gt_virt_tval_write() to unconditionally pass CNTVOFF_EL2.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 7ec1e6cfaab..01ca222903d 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2600,6 +2600,12 @@ static void gt_cval_write(CPUARMState *env, const ARMCPRegInfo *ri,
     gt_recalc_timer(env_archcpu(env), timeridx);
 }
 
+static uint64_t do_tval_read(CPUARMState *env, int timeridx, uint64_t offset)
+{
+    return (uint32_t)(env->cp15.c14_timer[timeridx].cval -
+                      (gt_get_countervalue(env) - offset));
+}
+
 static uint64_t gt_tval_read(CPUARMState *env, const ARMCPRegInfo *ri,
                              int timeridx)
 {
@@ -2614,8 +2620,16 @@ static uint64_t gt_tval_read(CPUARMState *env, const ARMCPRegInfo *ri,
         break;
     }
 
-    return (uint32_t)(env->cp15.c14_timer[timeridx].cval -
-                      (gt_get_countervalue(env) - offset));
+    return do_tval_read(env, timeridx, offset);
+}
+
+static void do_tval_write(CPUARMState *env, int timeridx, uint64_t value,
+                          uint64_t offset)
+{
+    trace_arm_gt_tval_write(timeridx, value);
+    env->cp15.c14_timer[timeridx].cval = gt_get_countervalue(env) - offset +
+                                         sextract64(value, 0, 32);
+    gt_recalc_timer(env_archcpu(env), timeridx);
 }
 
 static void gt_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -2632,11 +2646,7 @@ static void gt_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
         offset = gt_phys_cnt_offset(env);
         break;
     }
-
-    trace_arm_gt_tval_write(timeridx, value);
-    env->cp15.c14_timer[timeridx].cval = gt_get_countervalue(env) - offset +
-                                         sextract64(value, 0, 32);
-    gt_recalc_timer(env_archcpu(env), timeridx);
+    do_tval_write(env, timeridx, value, offset);
 }
 
 static void gt_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -2768,13 +2778,21 @@ static void gt_virt_cval_write(CPUARMState *env, const ARMCPRegInfo *ri,
 
 static uint64_t gt_virt_tval_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
-    return gt_tval_read(env, ri, GTIMER_VIRT);
+    /*
+     * This is CNTV_TVAL_EL02; unlike the underlying CNTV_TVAL_EL0
+     * we always apply CNTVOFF_EL2. Special case that here rather
+     * than going into the generic gt_tval_read() and then having
+     * to re-detect that it's this register.
+     * Note that the accessfn/perms mean we know we're at EL2 or EL3 here.
+     */
+    return do_tval_read(env, GTIMER_VIRT, env->cp15.cntvoff_el2);
 }
 
 static void gt_virt_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
                                uint64_t value)
 {
-    gt_tval_write(env, ri, GTIMER_VIRT, value);
+    /* Similarly for writes to CNTV_TVAL_EL02 */
+    do_tval_write(env, GTIMER_VIRT, value, env->cp15.cntvoff_el2);
 }
 
 static void gt_virt_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
-- 
2.34.1

[PATCH v3 5/9] target/arm: Refactor handling of timer offset for direct register accesses

[Peter Maydell]

When reading or writing the timer registers, sometimes we need to
apply one of the timer offsets.  Specifically, this happens for
direct reads of the counter registers CNTPCT_EL0 and CNTVCT_EL0 (and
their self-synchronized variants CNTVCTSS_EL0 and CNTPCTSS_EL0).  It
also applies for direct reads and writes of the CNT*_TVAL_EL*
registers that provide the 32-bit downcounting view of each timer.

We currently do this with duplicated code in gt_tval_read() and
gt_tval_write() and a special-case in gt_virt_cnt_read() and
gt_cnt_read().  Refactor this so that we handle it all in a single
function gt_direct_access_timer_offset(), to parallel how we handle
the offset for indirect accesses.

The call in the WFIT helper previously to gt_virt_cnt_offset() is
now to gt_direct_access_timer_offset(); this is the correct
behaviour, but it's not immediately obvious that it shouldn't be
considered an indirect access, so we add an explanatory comment.

This commit should make no behavioural changes.

(Cc to stable because the following bugfix commit will
depend on this one.)

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/internals.h     |   5 +-
 target/arm/helper.c        | 103 +++++++++++++++++++------------------
 target/arm/tcg/op_helper.c |   8 ++-
 3 files changed, 62 insertions(+), 54 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 863a84edf81..b4b3d196191 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1819,9 +1819,10 @@ int delete_hw_watchpoint(target_ulong addr, target_ulong len, int type);
 uint64_t gt_get_countervalue(CPUARMState *env);
 /*
  * Return the currently applicable offset between the system counter
- * and CNTVCT_EL0 (this will be either 0 or the value of CNTVOFF_EL2).
+ * and the counter for the specified timer, as used for direct register
+ * accesses.
  */
-uint64_t gt_virt_cnt_offset(CPUARMState *env);
+uint64_t gt_direct_access_timer_offset(CPUARMState *env, int timeridx);
 
 /*
  * Return mask of ARMMMUIdxBit values corresponding to an "invalidate
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 01ca222903d..c021c237b9b 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2455,14 +2455,6 @@ static uint64_t gt_phys_raw_cnt_offset(CPUARMState *env)
     return 0;
 }
 
-static uint64_t gt_phys_cnt_offset(CPUARMState *env)
-{
-    if (arm_current_el(env) >= 2) {
-        return 0;
-    }
-    return gt_phys_raw_cnt_offset(env);
-}
-
 static uint64_t gt_indirect_access_timer_offset(CPUARMState *env, int timeridx)
 {
     /*
@@ -2489,6 +2481,52 @@ static uint64_t gt_indirect_access_timer_offset(CPUARMState *env, int timeridx)
     }
 }
 
+uint64_t gt_direct_access_timer_offset(CPUARMState *env, int timeridx)
+{
+    /*
+     * Return the timer offset to use for direct accesses to the
+     * counter registers CNTPCT and CNTVCT, and for direct accesses
+     * to the CNT*_TVAL registers.
+     *
+     * This isn't exactly the same as the indirect-access offset,
+     * because here we also care about what EL the register access
+     * is being made from.
+     *
+     * This corresponds to the access pseudocode for the registers.
+     */
+    uint64_t hcr;
+
+    switch (timeridx) {
+    case GTIMER_PHYS:
+        if (arm_current_el(env) >= 2) {
+            return 0;
+        }
+        return gt_phys_raw_cnt_offset(env);
+    case GTIMER_VIRT:
+        switch (arm_current_el(env)) {
+        case 2:
+            hcr = arm_hcr_el2_eff(env);
+            if (hcr & HCR_E2H) {
+                return 0;
+            }
+            break;
+        case 0:
+            hcr = arm_hcr_el2_eff(env);
+            if ((hcr & (HCR_E2H | HCR_TGE)) == (HCR_E2H | HCR_TGE)) {
+                return 0;
+            }
+            break;
+        }
+        return env->cp15.cntvoff_el2;
+    case GTIMER_HYP:
+    case GTIMER_SEC:
+    case GTIMER_HYPVIRT:
+        return 0;
+    default:
+        g_assert_not_reached();
+    }
+}
+
 static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
 {
     ARMGenericTimer *gt = &cpu->env.cp15.c14_timer[timeridx];
@@ -2561,34 +2599,14 @@ static void gt_timer_reset(CPUARMState *env, const ARMCPRegInfo *ri,
 
 static uint64_t gt_cnt_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
-    return gt_get_countervalue(env) - gt_phys_cnt_offset(env);
-}
-
-uint64_t gt_virt_cnt_offset(CPUARMState *env)
-{
-    uint64_t hcr;
-
-    switch (arm_current_el(env)) {
-    case 2:
-        hcr = arm_hcr_el2_eff(env);
-        if (hcr & HCR_E2H) {
-            return 0;
-        }
-        break;
-    case 0:
-        hcr = arm_hcr_el2_eff(env);
-        if ((hcr & (HCR_E2H | HCR_TGE)) == (HCR_E2H | HCR_TGE)) {
-            return 0;
-        }
-        break;
-    }
-
-    return env->cp15.cntvoff_el2;
+    uint64_t offset = gt_direct_access_timer_offset(env, GTIMER_PHYS);
+    return gt_get_countervalue(env) - offset;
 }
 
 static uint64_t gt_virt_cnt_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
-    return gt_get_countervalue(env) - gt_virt_cnt_offset(env);
+    uint64_t offset = gt_direct_access_timer_offset(env, GTIMER_VIRT);
+    return gt_get_countervalue(env) - offset;
 }
 
 static void gt_cval_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -2609,16 +2627,7 @@ static uint64_t do_tval_read(CPUARMState *env, int timeridx, uint64_t offset)
 static uint64_t gt_tval_read(CPUARMState *env, const ARMCPRegInfo *ri,
                              int timeridx)
 {
-    uint64_t offset = 0;
-
-    switch (timeridx) {
-    case GTIMER_VIRT:
-        offset = gt_virt_cnt_offset(env);
-        break;
-    case GTIMER_PHYS:
-        offset = gt_phys_cnt_offset(env);
-        break;
-    }
+    uint64_t offset = gt_direct_access_timer_offset(env, timeridx);
 
     return do_tval_read(env, timeridx, offset);
 }
@@ -2636,16 +2645,8 @@ static void gt_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
                           int timeridx,
                           uint64_t value)
 {
-    uint64_t offset = 0;
+    uint64_t offset = gt_direct_access_timer_offset(env, timeridx);
 
-    switch (timeridx) {
-    case GTIMER_VIRT:
-        offset = gt_virt_cnt_offset(env);
-        break;
-    case GTIMER_PHYS:
-        offset = gt_phys_cnt_offset(env);
-        break;
-    }
     do_tval_write(env, timeridx, value, offset);
 }
 
diff --git a/target/arm/tcg/op_helper.c b/target/arm/tcg/op_helper.c
index 02c375d196d..30786fd1ff4 100644
--- a/target/arm/tcg/op_helper.c
+++ b/target/arm/tcg/op_helper.c
@@ -427,7 +427,13 @@ void HELPER(wfit)(CPUARMState *env, uint64_t timeout)
     int target_el = check_wfx_trap(env, false, &excp);
     /* The WFIT should time out when CNTVCT_EL0 >= the specified value. */
     uint64_t cntval = gt_get_countervalue(env);
-    uint64_t offset = gt_virt_cnt_offset(env);
+    /*
+     * We want the value that we would get if we read CNTVCT_EL0 from
+     * the current exception level, so the direct_access offset, not
+     * the indirect_access one. Compare the pseudocode LocalTimeoutEvent(),
+     * which calls VirtualCounterTimer().
+     */
+    uint64_t offset = gt_direct_access_timer_offset(env, GTIMER_VIRT);
     uint64_t cntvct = cntval - offset;
     uint64_t nexttick;
 
-- 
2.34.1

[PATCH v3 6/9] target/arm: Implement SEL2 physical and virtual timers

[Peter Maydell]

From: Alex Bennée <alex.bennee@linaro.org>

When FEAT_SEL2 was implemented the SEL2 timers were missed. This
shows up when building the latest Hafnium with SPMC_AT_EL=2. The
actual implementation utilises the same logic as the rest of the
timers so all we need to do is:

  - define the timers and their access functions
  - conditionally add the correct system registers
  - create a new accessfn as the rules are subtly different to the
    existing secure timer

Fixes: e9152ee91c (target/arm: add ARMv8.4-SEL2 system registers)
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: qemu-stable@nongnu.org
Cc: Andrei Homescu <ahomescu@google.com>
Cc: Arve Hjønnevåg <arve@google.com>
Cc: Rémi Denis-Courmont <remi.denis.courmont@huawei.com>
[PMM: CP_ACCESS_TRAP_UNCATEGORIZED -> CP_ACCESS_UNDEFINED;
 offset logic now in gt_{indirect,direct}_access_timer_offset() ]
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/bsa.h |   2 +
 target/arm/cpu.h     |   2 +
 target/arm/gtimer.h  |   4 +-
 target/arm/cpu.c     |   4 ++
 target/arm/helper.c  | 163 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 174 insertions(+), 1 deletion(-)

diff --git a/include/hw/arm/bsa.h b/include/hw/arm/bsa.h
index 8eaab603c03..13ed2d2ac19 100644
--- a/include/hw/arm/bsa.h
+++ b/include/hw/arm/bsa.h
@@ -22,6 +22,8 @@
 #define QEMU_ARM_BSA_H
 
 /* These are architectural INTID values */
+#define ARCH_TIMER_S_EL2_VIRT_IRQ  19
+#define ARCH_TIMER_S_EL2_IRQ       20
 #define VIRTUAL_PMU_IRQ            23
 #define ARCH_GIC_MAINT_IRQ         25
 #define ARCH_TIMER_NS_EL2_IRQ      26
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index ae1e8b1c779..3011595b3d0 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1148,6 +1148,8 @@ void arm_gt_vtimer_cb(void *opaque);
 void arm_gt_htimer_cb(void *opaque);
 void arm_gt_stimer_cb(void *opaque);
 void arm_gt_hvtimer_cb(void *opaque);
+void arm_gt_sel2timer_cb(void *opaque);
+void arm_gt_sel2vtimer_cb(void *opaque);
 
 unsigned int gt_cntfrq_period_ns(ARMCPU *cpu);
 void gt_rme_post_el_change(ARMCPU *cpu, void *opaque);
diff --git a/target/arm/gtimer.h b/target/arm/gtimer.h
index b992941bef1..0e89b8e58d0 100644
--- a/target/arm/gtimer.h
+++ b/target/arm/gtimer.h
@@ -15,7 +15,9 @@ enum {
     GTIMER_HYP      = 2,
     GTIMER_SEC      = 3,
     GTIMER_HYPVIRT  = 4,
-#define NUM_GTIMERS   5
+    GTIMER_S_EL2_PHYS = 5, /* CNTHPS_* ; only if FEAT_SEL2 */
+    GTIMER_S_EL2_VIRT = 6, /* CNTHVS_* ; only if FEAT_SEL2 */
+#define NUM_GTIMERS   7
 };
 
 #endif
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 7a83b9ee34f..97acd230298 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2091,6 +2091,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
                                               arm_gt_stimer_cb, cpu);
         cpu->gt_timer[GTIMER_HYPVIRT] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
                                                   arm_gt_hvtimer_cb, cpu);
+        cpu->gt_timer[GTIMER_S_EL2_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
+                                                     arm_gt_sel2timer_cb, cpu);
+        cpu->gt_timer[GTIMER_S_EL2_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
+                                                     arm_gt_sel2vtimer_cb, cpu);
     }
 #endif
 
diff --git a/target/arm/helper.c b/target/arm/helper.c
index c021c237b9b..bc820b0de76 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2404,6 +2404,45 @@ static CPAccessResult gt_stimer_access(CPUARMState *env,
     }
 }
 
+static CPAccessResult gt_sel2timer_access(CPUARMState *env,
+                                          const ARMCPRegInfo *ri,
+                                          bool isread)
+{
+    /*
+     * The AArch64 register view of the secure EL2 timers are mostly
+     * accessible from EL3 and EL2 although can also be trapped to EL2
+     * from EL1 depending on nested virt config.
+     */
+    switch (arm_current_el(env)) {
+    case 0: /* UNDEFINED */
+        return CP_ACCESS_UNDEFINED;
+    case 1:
+        if (!arm_is_secure(env)) {
+            /* UNDEFINED */
+            return CP_ACCESS_UNDEFINED;
+        } else if (arm_hcr_el2_eff(env) & HCR_NV) {
+            /* Aarch64.SystemAccessTrap(EL2, 0x18) */
+            return CP_ACCESS_TRAP_EL2;
+        }
+        /* UNDEFINED */
+        return CP_ACCESS_UNDEFINED;
+    case 2:
+        if (!arm_is_secure(env)) {
+            /* UNDEFINED */
+            return CP_ACCESS_UNDEFINED;
+        }
+        return CP_ACCESS_OK;
+    case 3:
+        if (env->cp15.scr_el3 & SCR_EEL2) {
+            return CP_ACCESS_OK;
+        } else {
+            return CP_ACCESS_UNDEFINED;
+        }
+    default:
+        g_assert_not_reached();
+    }
+}
+
 uint64_t gt_get_countervalue(CPUARMState *env)
 {
     ARMCPU *cpu = env_archcpu(env);
@@ -2475,6 +2514,8 @@ static uint64_t gt_indirect_access_timer_offset(CPUARMState *env, int timeridx)
     case GTIMER_HYP:
     case GTIMER_SEC:
     case GTIMER_HYPVIRT:
+    case GTIMER_S_EL2_PHYS:
+    case GTIMER_S_EL2_VIRT:
         return 0;
     default:
         g_assert_not_reached();
@@ -2521,6 +2562,8 @@ uint64_t gt_direct_access_timer_offset(CPUARMState *env, int timeridx)
     case GTIMER_HYP:
     case GTIMER_SEC:
     case GTIMER_HYPVIRT:
+    case GTIMER_S_EL2_PHYS:
+    case GTIMER_S_EL2_VIRT:
         return 0;
     default:
         g_assert_not_reached();
@@ -2953,6 +2996,62 @@ static void gt_sec_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
     gt_ctl_write(env, ri, GTIMER_SEC, value);
 }
 
+static void gt_sec_pel2_timer_reset(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    gt_timer_reset(env, ri, GTIMER_S_EL2_PHYS);
+}
+
+static void gt_sec_pel2_cval_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                                   uint64_t value)
+{
+    gt_cval_write(env, ri, GTIMER_S_EL2_PHYS, value);
+}
+
+static uint64_t gt_sec_pel2_tval_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    return gt_tval_read(env, ri, GTIMER_S_EL2_PHYS);
+}
+
+static void gt_sec_pel2_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    gt_tval_write(env, ri, GTIMER_S_EL2_PHYS, value);
+}
+
+static void gt_sec_pel2_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    gt_ctl_write(env, ri, GTIMER_S_EL2_PHYS, value);
+}
+
+static void gt_sec_vel2_timer_reset(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    gt_timer_reset(env, ri, GTIMER_S_EL2_VIRT);
+}
+
+static void gt_sec_vel2_cval_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    gt_cval_write(env, ri, GTIMER_S_EL2_VIRT, value);
+}
+
+static uint64_t gt_sec_vel2_tval_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    return gt_tval_read(env, ri, GTIMER_S_EL2_VIRT);
+}
+
+static void gt_sec_vel2_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                                   uint64_t value)
+{
+    gt_tval_write(env, ri, GTIMER_S_EL2_VIRT, value);
+}
+
+static void gt_sec_vel2_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    gt_ctl_write(env, ri, GTIMER_S_EL2_VIRT, value);
+}
+
 static void gt_hv_timer_reset(CPUARMState *env, const ARMCPRegInfo *ri)
 {
     gt_timer_reset(env, ri, GTIMER_HYPVIRT);
@@ -3009,6 +3108,20 @@ void arm_gt_stimer_cb(void *opaque)
     gt_recalc_timer(cpu, GTIMER_SEC);
 }
 
+void arm_gt_sel2timer_cb(void *opaque)
+{
+    ARMCPU *cpu = opaque;
+
+    gt_recalc_timer(cpu, GTIMER_S_EL2_PHYS);
+}
+
+void arm_gt_sel2vtimer_cb(void *opaque)
+{
+    ARMCPU *cpu = opaque;
+
+    gt_recalc_timer(cpu, GTIMER_S_EL2_VIRT);
+}
+
 void arm_gt_hvtimer_cb(void *opaque)
 {
     ARMCPU *cpu = opaque;
@@ -5733,6 +5846,56 @@ static const ARMCPRegInfo el2_sec_cp_reginfo[] = {
       .access = PL2_RW, .accessfn = sel2_access,
       .nv2_redirect_offset = 0x48,
       .fieldoffset = offsetof(CPUARMState, cp15.vstcr_el2) },
+#ifndef CONFIG_USER_ONLY
+    /* Secure EL2 Physical Timer */
+    { .name = "CNTHPS_TVAL_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 5, .opc2 = 0,
+      .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL2_RW,
+      .accessfn = gt_sel2timer_access,
+      .readfn = gt_sec_pel2_tval_read,
+      .writefn = gt_sec_pel2_tval_write,
+      .resetfn = gt_sec_pel2_timer_reset,
+    },
+    { .name = "CNTHPS_CTL_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 5, .opc2 = 1,
+      .type = ARM_CP_IO, .access = PL2_RW,
+      .accessfn = gt_sel2timer_access,
+      .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_S_EL2_PHYS].ctl),
+      .resetvalue = 0,
+      .writefn = gt_sec_pel2_ctl_write, .raw_writefn = raw_write,
+    },
+    { .name = "CNTHPS_CVAL_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 5, .opc2 = 2,
+      .type = ARM_CP_IO, .access = PL2_RW,
+      .accessfn = gt_sel2timer_access,
+      .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_S_EL2_PHYS].cval),
+      .writefn = gt_sec_pel2_cval_write, .raw_writefn = raw_write,
+    },
+    /* Secure EL2 Virtual Timer */
+    { .name = "CNTHVS_TVAL_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 4, .opc2 = 0,
+      .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL2_RW,
+      .accessfn = gt_sel2timer_access,
+      .readfn = gt_sec_vel2_tval_read,
+      .writefn = gt_sec_vel2_tval_write,
+      .resetfn = gt_sec_vel2_timer_reset,
+    },
+    { .name = "CNTHVS_CTL_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 4, .opc2 = 1,
+      .type = ARM_CP_IO, .access = PL2_RW,
+      .accessfn = gt_sel2timer_access,
+      .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_S_EL2_VIRT].ctl),
+      .resetvalue = 0,
+      .writefn = gt_sec_vel2_ctl_write, .raw_writefn = raw_write,
+    },
+    { .name = "CNTHVS_CVAL_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 4, .opc2 = 2,
+      .type = ARM_CP_IO, .access = PL2_RW,
+      .accessfn = gt_sel2timer_access,
+      .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_S_EL2_VIRT].cval),
+      .writefn = gt_sec_vel2_cval_write, .raw_writefn = raw_write,
+    },
+#endif
 };
 
 static CPAccessResult nsacr_access(CPUARMState *env, const ARMCPRegInfo *ri,
-- 
2.34.1

[PATCH v3 7/9] target/arm: document the architectural names of our GTIMERs

[Peter Maydell]

From: Alex Bennée <alex.bennee@linaro.org>

As we are about to add more physical and virtual timers lets make it
clear what each timer does.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
[PMM: Add timer register name prefix to each comment]
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/gtimer.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/target/arm/gtimer.h b/target/arm/gtimer.h
index 0e89b8e58d0..d49c63cbf87 100644
--- a/target/arm/gtimer.h
+++ b/target/arm/gtimer.h
@@ -10,11 +10,11 @@
 #define TARGET_ARM_GTIMER_H
 
 enum {
-    GTIMER_PHYS     = 0,
-    GTIMER_VIRT     = 1,
-    GTIMER_HYP      = 2,
-    GTIMER_SEC      = 3,
-    GTIMER_HYPVIRT  = 4,
+    GTIMER_PHYS     = 0, /* CNTP_* ; EL1 physical timer */
+    GTIMER_VIRT     = 1, /* CNTV_* ; EL1 virtual timer */
+    GTIMER_HYP      = 2, /* CNTHP_* ; EL2 physical timer */
+    GTIMER_SEC      = 3, /* CNTPS_* ; EL3 physical timer */
+    GTIMER_HYPVIRT  = 4, /* CNTHV_* ; EL2 virtual timer ; only if FEAT_VHE */
     GTIMER_S_EL2_PHYS = 5, /* CNTHPS_* ; only if FEAT_SEL2 */
     GTIMER_S_EL2_VIRT = 6, /* CNTHVS_* ; only if FEAT_SEL2 */
 #define NUM_GTIMERS   7
-- 
2.34.1

[PATCH v3 8/9] hw/arm: enable secure EL2 timers for virt machine

[Peter Maydell]

From: Alex Bennée <alex.bennee@linaro.org>

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: qemu-stable@nongnu.org
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 99e0a68b6c5..c00a6e410cd 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -873,6 +873,8 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
             [GTIMER_HYP]  = ARCH_TIMER_NS_EL2_IRQ,
             [GTIMER_SEC]  = ARCH_TIMER_S_EL1_IRQ,
             [GTIMER_HYPVIRT] = ARCH_TIMER_NS_EL2_VIRT_IRQ,
+            [GTIMER_S_EL2_PHYS] = ARCH_TIMER_S_EL2_IRQ,
+            [GTIMER_S_EL2_VIRT] = ARCH_TIMER_S_EL2_VIRT_IRQ,
         };
 
         for (unsigned irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
-- 
2.34.1

[PATCH v3 9/9] hw/arm: enable secure EL2 timers for sbsa machine

[Peter Maydell]

From: Alex Bennée <alex.bennee@linaro.org>

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/sbsa-ref.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 6183111f2de..d69e7aaa95e 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -484,6 +484,8 @@ static void create_gic(SBSAMachineState *sms, MemoryRegion *mem)
             [GTIMER_HYP]  = ARCH_TIMER_NS_EL2_IRQ,
             [GTIMER_SEC]  = ARCH_TIMER_S_EL1_IRQ,
             [GTIMER_HYPVIRT] = ARCH_TIMER_NS_EL2_VIRT_IRQ,
+            [GTIMER_S_EL2_PHYS] = ARCH_TIMER_S_EL2_IRQ,
+            [GTIMER_S_EL2_VIRT] = ARCH_TIMER_S_EL2_VIRT_IRQ,
         };
 
         for (irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
-- 
2.34.1

Re: [PATCH v3 0/9] target/arm: Implement SEL2 physical and virtual timers

[Peter Maydell]

Ping? Patches 1-5 need review here.

thanks
-- PMM

On Tue, 4 Feb 2025 at 12:50, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchset is a respin of Alex's patches, with some extra fixes
> for bugs I discovered along the way in our existing code (and
> a bit of refactoring to make the fixes straightforward). It is:
>
> Based-on: 20250130182309.717346-1-peter.maydell@linaro.org
> ("target/arm: Clean up some corner cases of sysreg traps")
>
> because it wants to use the renamed CP_ACCESS_* constants that
> that patchset introduced.
>
> The bugfixes are not super exciting as they mostly are oddball
> corner cases, but I've cc'd them to stable anyway. The actual
> implementation of the missing SEL2 timers also should go to stable.
>
> Alex Bennée (4):
>   target/arm: Implement SEL2 physical and virtual timers
>   target/arm: document the architectural names of our GTIMERs
>   hw/arm: enable secure EL2 timers for virt machine
>   hw/arm: enable secure EL2 timers for sbsa machine
>
> Peter Maydell (5):
>   target/arm: Apply correct timer offset when calculating deadlines
>   target/arm: Don't apply CNTVOFF_EL2 for EL2_VIRT timer
>   target/arm: Make CNTPS_* UNDEF from Secure EL1 when Secure EL2 is
>     enabled
>   target/arm: Always apply CNTVOFF_EL2 for CNTV_TVAL_EL02 accesses
>   target/arm: Refactor handling of timer offset for direct register
>     accesses
>
>  include/hw/arm/bsa.h       |   2 +
>  target/arm/cpu.h           |   2 +
>  target/arm/gtimer.h        |  14 +-
>  target/arm/internals.h     |   5 +-
>  hw/arm/sbsa-ref.c          |   2 +
>  hw/arm/virt.c              |   2 +
>  target/arm/cpu.c           |   4 +
>  target/arm/helper.c        | 324 ++++++++++++++++++++++++++++++-------
>  target/arm/tcg/op_helper.c |   8 +-
>  9 files changed, 296 insertions(+), 67 deletions(-)
>

Re: [PATCH v3 3/9] target/arm: Make CNTPS_* UNDEF from Secure EL1 when Secure EL2 is enabled

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> When we added Secure EL2 support, we missed that this needs an update
> to the access code for the EL3 physical timer registers.  These are
> supposed to UNDEF from Secure EL1 when Secure EL2 is enabled.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index ac8cb428925..7ec1e6cfaab 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2387,6 +2387,9 @@ static CPAccessResult gt_stimer_access(CPUARMState *env,
>          if (!arm_is_secure(env)) {
>              return CP_ACCESS_UNDEFINED;
>          }

Hmm this failed to apply as b4d3978c2f (target-arm: Add the AArch64 view
of the Secure physical timer) has the above as CP_ACCESS_TRAP. I guess
because I didn't apply 20250130182309.717346-1-peter.maydell@linaro.org.
I guess this needs fixing up for stable.


> +        if (arm_is_el2_enabled(env)) {
> +            return CP_ACCESS_UNDEFINED;
> +        }
>          if (!(env->cp15.scr_el3 & SCR_ST)) {
>              return CP_ACCESS_TRAP_EL3;
>          }


Anyway:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 3/9] target/arm: Make CNTPS_* UNDEF from Secure EL1 when Secure EL2 is enabled

[Peter Maydell]

On Fri, 21 Feb 2025 at 18:02, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > When we added Secure EL2 support, we missed that this needs an update
> > to the access code for the EL3 physical timer registers.  These are
> > supposed to UNDEF from Secure EL1 when Secure EL2 is enabled.
> >
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  target/arm/helper.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index ac8cb428925..7ec1e6cfaab 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -2387,6 +2387,9 @@ static CPAccessResult gt_stimer_access(CPUARMState *env,
> >          if (!arm_is_secure(env)) {
> >              return CP_ACCESS_UNDEFINED;
> >          }
>
> Hmm this failed to apply as b4d3978c2f (target-arm: Add the AArch64 view
> of the Secure physical timer) has the above as CP_ACCESS_TRAP. I guess
> because I didn't apply 20250130182309.717346-1-peter.maydell@linaro.org.
> I guess this needs fixing up for stable.

There is a Based-on: tag in the cover letter which will tell you
what this series should be based on if you want to apply it.

Yes, we'll need to either tweak this commit for stable
(i.e. use CP_ACCESS_TRAP_UNCATEGORIZED instead of UNDEFINED)
or else pull in the refactoring patches it depends on.

thanks
-- PMM

Re: [PATCH v3 1/9] target/arm: Apply correct timer offset when calculating deadlines

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> When we are calculating timer deadlines, the correct definition of
> whether or not to apply an offset to the physical count is described
> in the Arm ARM DDI4087 rev L.a section D12.2.4.1.  This is different
> from when the offset should be applied for a direct read of the
> counter sysreg.
>
> We got this right for the EL1 physical timer and for the EL1 virtual
> timer, but got all the rest wrong: they should be using a zero offset
> always.
>
> Factor the offset calculation out into a function that has a comment
> documenting exactly which offset it is calculating and which gets the
> HYP, SEC, and HYPVIRT cases right.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 2/9] target/arm: Don't apply CNTVOFF_EL2 for EL2_VIRT timer

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> The CNTVOFF_EL2 offset register should only be applied for accessses
> to CNTVCT_EL0 and for the EL1 virtual timer (CNTV_*).  We were
> incorrectly applying it for the EL2 virtual timer (CNTHV_*).
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 4/9] target/arm: Always apply CNTVOFF_EL2 for CNTV_TVAL_EL02 accesses

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Currently we handle CNTV_TVAL_EL02 by calling gt_tval_read() for the
> EL1 virt timer.  This is almost correct, but the underlying
> CNTV_TVAL_EL0 register behaves slightly differently.  CNTV_TVAL_EL02
> always applies the CNTVOFF_EL2 offset; CNTV_TVAL_EL0 doesn't do so if
> we're at EL2 and HCR_EL2.E2H is 1.
>
> We were getting this wrong, because we ended up in
> gt_virt_cnt_offset() and did the E2H check.
>
> Factor out the tval read/write calculation from the selection of the
> offset, so that we can special case gt_virt_tval_read() and
> gt_virt_tval_write() to unconditionally pass CNTVOFF_EL2.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 5/9] target/arm: Refactor handling of timer offset for direct register accesses

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> When reading or writing the timer registers, sometimes we need to
> apply one of the timer offsets.  Specifically, this happens for
> direct reads of the counter registers CNTPCT_EL0 and CNTVCT_EL0 (and
> their self-synchronized variants CNTVCTSS_EL0 and CNTPCTSS_EL0).  It
> also applies for direct reads and writes of the CNT*_TVAL_EL*
> registers that provide the 32-bit downcounting view of each timer.
>
> We currently do this with duplicated code in gt_tval_read() and
> gt_tval_write() and a special-case in gt_virt_cnt_read() and
> gt_cnt_read().  Refactor this so that we handle it all in a single
> function gt_direct_access_timer_offset(), to parallel how we handle
> the offset for indirect accesses.
>
> The call in the WFIT helper previously to gt_virt_cnt_offset() is
> now to gt_direct_access_timer_offset(); this is the correct
> behaviour, but it's not immediately obvious that it shouldn't be
> considered an indirect access, so we add an explanatory comment.
>
> This commit should make no behavioural changes.
>
> (Cc to stable because the following bugfix commit will
> depend on this one.)
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 0/9] target/arm: Implement SEL2 physical and virtual timers

[Michael Tokarev]

04.02.2025 15:50, Peter Maydell wrote:
> This patchset is a respin of Alex's patches, with some extra fixes
> for bugs I discovered along the way in our existing code (and
> a bit of refactoring to make the fixes straightforward). It is:
> 
> Based-on: 20250130182309.717346-1-peter.maydell@linaro.org
> ("target/arm: Clean up some corner cases of sysreg traps")
> 
> because it wants to use the renamed CP_ACCESS_* constants that
> that patchset introduced.
> 
> The bugfixes are not super exciting as they mostly are oddball
> corner cases, but I've cc'd them to stable anyway. The actual
> implementation of the missing SEL2 timers also should go to stable.
> 
> Alex Bennée (4):
>    target/arm: Implement SEL2 physical and virtual timers
>    target/arm: document the architectural names of our GTIMERs
>    hw/arm: enable secure EL2 timers for virt machine
>    hw/arm: enable secure EL2 timers for sbsa machine
> 
> Peter Maydell (5):
>    target/arm: Apply correct timer offset when calculating deadlines
>    target/arm: Don't apply CNTVOFF_EL2 for EL2_VIRT timer
>    target/arm: Make CNTPS_* UNDEF from Secure EL1 when Secure EL2 is
>      enabled
>    target/arm: Always apply CNTVOFF_EL2 for CNTV_TVAL_EL02 accesses
>    target/arm: Refactor handling of timer offset for direct register
>      accesses

Hi!

Which stable series this patchset is supposed to be applied to?
(Current active stable series are 7.2, 8.2 and 9.2)

Or put it in other words, is it supposed to go earlier than the
most recent stable series, 9.2?

For example, the very first patch, "Apply correct timer offset when calculating
deadlines", does not apply to 8.2 because it lacks v8.2.0-2122-g2808d3b38a
"target/arm: Implement FEAT_ECV CNTPOFF_EL2 handling" which touches the same
line in target/arm/helper.c:gt_recalc_timer().

Thanks,

/mjt

Re: [PATCH v3 0/9] target/arm: Implement SEL2 physical and virtual timers

[Alex Bennée]

Michael Tokarev <mjt@tls.msk.ru> writes:

> 04.02.2025 15:50, Peter Maydell wrote:
>> This patchset is a respin of Alex's patches, with some extra fixes
>> for bugs I discovered along the way in our existing code (and
>> a bit of refactoring to make the fixes straightforward). It is:
>> Based-on: 20250130182309.717346-1-peter.maydell@linaro.org
>> ("target/arm: Clean up some corner cases of sysreg traps")
>> because it wants to use the renamed CP_ACCESS_* constants that
>> that patchset introduced.
>> The bugfixes are not super exciting as they mostly are oddball
>> corner cases, but I've cc'd them to stable anyway. The actual
>> implementation of the missing SEL2 timers also should go to stable.
>> Alex Bennée (4):
>>    target/arm: Implement SEL2 physical and virtual timers
>>    target/arm: document the architectural names of our GTIMERs
>>    hw/arm: enable secure EL2 timers for virt machine
>>    hw/arm: enable secure EL2 timers for sbsa machine
>> Peter Maydell (5):
>>    target/arm: Apply correct timer offset when calculating deadlines
>>    target/arm: Don't apply CNTVOFF_EL2 for EL2_VIRT timer
>>    target/arm: Make CNTPS_* UNDEF from Secure EL1 when Secure EL2 is
>>      enabled
>>    target/arm: Always apply CNTVOFF_EL2 for CNTV_TVAL_EL02 accesses
>>    target/arm: Refactor handling of timer offset for direct register
>>      accesses
>
> Hi!
>
> Which stable series this patchset is supposed to be applied to?
> (Current active stable series are 7.2, 8.2 and 9.2)
>
> Or put it in other words, is it supposed to go earlier than the
> most recent stable series, 9.2?

I'd just do 9.2 because I think as you've found too much has changed.
That should become available in backports while we wait for trixie to
stabilise this year.

>
> For example, the very first patch, "Apply correct timer offset when calculating
> deadlines", does not apply to 8.2 because it lacks v8.2.0-2122-g2808d3b38a
> "target/arm: Implement FEAT_ECV CNTPOFF_EL2 handling" which touches the same
> line in target/arm/helper.c:gt_recalc_timer().
>
> Thanks,
>
> /mjt

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro