[PATCH 0/3] target/arm: implement SEL2 physical and virtual timers

[PATCH 0/3] target/arm: implement SEL2 physical and virtual timers

[Alex Bennée]

Follow Peter's review I've split this into a several patches as there
are some other fixes that should be made to other EL2 times that
shouldn't be rolled together.

v1
  - improve GTIMER docs
  - fix gt_recalc bug
  - address review comments for the main patch
  - cc qemu-stable (no rush for 9.2.0)

Please review.

Alex.

Alex Bennée (3):
  target/arm: document the architectural names of our GTIMERs
  target/arm: ensure cntvoff_el2 also used for EL2 virt timer
  target/arm: implement SEL2 physical and virtual timers

 include/hw/arm/bsa.h |   2 +
 target/arm/cpu.h     |   2 +
 target/arm/gtimer.h  |  14 ++--
 hw/arm/sbsa-ref.c    |   2 +
 hw/arm/virt.c        |   2 +
 target/arm/cpu.c     |   4 +
 target/arm/helper.c  | 179 +++++++++++++++++++++++++++++++++++++++++--
 7 files changed, 194 insertions(+), 11 deletions(-)

-- 
2.39.5

[PATCH 1/3] target/arm: document the architectural names of our GTIMERs

[Alex Bennée]

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>
Cc: qemu-stable@nongnu.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 b992941bef..de016e6da3 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, /* EL1 physical timer */
+    GTIMER_VIRT     = 1, /* EL1 virtual timer */
+    GTIMER_HYP      = 2, /* EL2 physical timer */
+    GTIMER_SEC      = 3, /* EL3 physical timer */
+    GTIMER_HYPVIRT  = 4, /* EL2 virtual timer */
 #define NUM_GTIMERS   5
 };
 
-- 
2.39.5

Re: [PATCH 1/3] target/arm: document the architectural names of our GTIMERs

[Peter Maydell]

On Fri, 6 Dec 2024 at 16:02, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> 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>
> Cc: qemu-stable@nongnu.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 b992941bef..de016e6da3 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, /* EL1 physical timer */
> +    GTIMER_VIRT     = 1, /* EL1 virtual timer */
> +    GTIMER_HYP      = 2, /* EL2 physical timer */
> +    GTIMER_SEC      = 3, /* EL3 physical timer */
> +    GTIMER_HYPVIRT  = 4, /* EL2 virtual timer */
>  #define NUM_GTIMERS   5
>  };

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

thanks
-- PMM

[PATCH 2/3] target/arm: ensure cntvoff_el2 also used for EL2 virt timer

[Alex Bennée]

We were missing this case and will shortly be adding another.
Re-arrange the code and use a switch statement to group the virtual
timers.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: qemu-stable@nongnu.org
---
 target/arm/helper.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index f38eb054c0..cd147b717a 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2732,16 +2732,27 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
     ARMGenericTimer *gt = &cpu->env.cp15.c14_timer[timeridx];
 
     if (gt->ctl & 1) {
+        uint64_t count = gt_get_countervalue(&cpu->env);
+        uint64_t offset;
+        uint64_t nexttick;
+        int istatus;
+
         /*
          * 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 count = gt_get_countervalue(&cpu->env);
+        switch (timeridx) {
+        case GTIMER_VIRT:
+        case GTIMER_HYPVIRT:
+            offset = cpu->env.cp15.cntvoff_el2;
+            break;
+        default:
+            offset =gt_phys_raw_cnt_offset(&cpu->env);
+            break;
+        }
+
         /* Note that this must be unsigned 64 bit arithmetic: */
-        int istatus = count - offset >= gt->cval;
-        uint64_t nexttick;
+        istatus = count - offset >= gt->cval;
 
         gt->ctl = deposit32(gt->ctl, 2, 1, istatus);
 
-- 
2.39.5

[PATCH 3/3] target/arm: implement SEL2 physical and virtual timers

[Alex Bennée]

When FEAT_SEL2 was implemented the SEL2 timers where 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>

---
v1
  - add better comments to GTIMER descriptions
  - also define new timers for sbsa-ref
  - don't conditionally gate qemu_timer creation on the feature
  - take cntvoff_el2 int account for SEC_VEL2 in gt_recalc/g_tval_[read|write]
---
 include/hw/arm/bsa.h |   2 +
 target/arm/cpu.h     |   2 +
 target/arm/gtimer.h  |   4 +-
 hw/arm/sbsa-ref.c    |   2 +
 hw/arm/virt.c        |   2 +
 target/arm/cpu.c     |   4 ++
 target/arm/helper.c  | 158 +++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 173 insertions(+), 1 deletion(-)

diff --git a/include/hw/arm/bsa.h b/include/hw/arm/bsa.h
index 8eaab603c0..b4ecca1b1c 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_VIRT_EL2_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 d86e641280..10b5354d6f 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1139,6 +1139,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 de016e6da3..f8f7425a5f 100644
--- a/target/arm/gtimer.h
+++ b/target/arm/gtimer.h
@@ -15,7 +15,9 @@ enum {
     GTIMER_HYP      = 2, /* EL2 physical timer */
     GTIMER_SEC      = 3, /* EL3 physical timer */
     GTIMER_HYPVIRT  = 4, /* EL2 virtual timer */
-#define NUM_GTIMERS   5
+    GTIMER_SEC_PEL2 = 5, /* Secure EL2 physical timer */
+    GTIMER_SEC_VEL2 = 6, /* Secure EL2 virtual timer */
+#define NUM_GTIMERS   7
 };
 
 #endif
diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index e3195d5449..5dc36b3cee 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_SEC_PEL2] = ARCH_TIMER_S_EL2_IRQ,
+            [GTIMER_SEC_VEL2] = ARCH_TIMER_S_VIRT_EL2_IRQ,
         };
 
         for (irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1a381e9a2b..451d154459 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_SEC_PEL2] = ARCH_TIMER_S_EL2_IRQ,
+            [GTIMER_SEC_VEL2] = ARCH_TIMER_S_VIRT_EL2_IRQ,
         };
 
         for (unsigned irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 6938161b95..d15916c436 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2078,6 +2078,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_SEC_PEL2] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
+                                                   arm_gt_sel2timer_cb, cpu);
+        cpu->gt_timer[GTIMER_SEC_VEL2] = 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 cd147b717a..f82503304e 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2668,6 +2668,41 @@ 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:
+        return CP_ACCESS_TRAP;
+    case 1:
+        if (!arm_is_secure(env)) {
+            return CP_ACCESS_TRAP;
+        } else if (arm_hcr_el2_eff(env) & HCR_NV) {
+            return CP_ACCESS_TRAP_EL2;
+        }
+        return CP_ACCESS_TRAP;
+    case 2:
+        if (!arm_is_secure(env)) {
+            return CP_ACCESS_TRAP;
+        }
+        return CP_ACCESS_OK;
+    case 3:
+        if (env->cp15.scr_el3 & SCR_EEL2) {
+            return CP_ACCESS_OK;
+        } else {
+            return CP_ACCESS_TRAP;
+        }
+    default:
+        g_assert_not_reached();
+    }
+}
+
 uint64_t gt_get_countervalue(CPUARMState *env)
 {
     ARMCPU *cpu = env_archcpu(env);
@@ -2744,6 +2779,7 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
         switch (timeridx) {
         case GTIMER_VIRT:
         case GTIMER_HYPVIRT:
+        case GTIMER_SEC_VEL2:
             offset = cpu->env.cp15.cntvoff_el2;
             break;
         default:
@@ -2858,6 +2894,7 @@ static uint64_t gt_tval_read(CPUARMState *env, const ARMCPRegInfo *ri,
     switch (timeridx) {
     case GTIMER_VIRT:
     case GTIMER_HYPVIRT:
+    case GTIMER_SEC_VEL2:
         offset = gt_virt_cnt_offset(env);
         break;
     case GTIMER_PHYS:
@@ -2878,6 +2915,7 @@ static void gt_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
     switch (timeridx) {
     case GTIMER_VIRT:
     case GTIMER_HYPVIRT:
+    case GTIMER_SEC_VEL2:
         offset = gt_virt_cnt_offset(env);
         break;
     case GTIMER_PHYS:
@@ -3186,6 +3224,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_SEC_PEL2);
+}
+
+static void gt_sec_pel2_cval_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                                   uint64_t value)
+{
+    gt_cval_write(env, ri, GTIMER_SEC_PEL2, value);
+}
+
+static uint64_t gt_sec_pel2_tval_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    return gt_tval_read(env, ri, GTIMER_SEC_PEL2);
+}
+
+static void gt_sec_pel2_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    gt_tval_write(env, ri, GTIMER_SEC_PEL2, value);
+}
+
+static void gt_sec_pel2_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    gt_ctl_write(env, ri, GTIMER_SEC_PEL2, value);
+}
+
+static void gt_sec_vel2_timer_reset(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    gt_timer_reset(env, ri, GTIMER_SEC_VEL2);
+}
+
+static void gt_sec_vel2_cval_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    gt_cval_write(env, ri, GTIMER_SEC_VEL2, value);
+}
+
+static uint64_t gt_sec_vel2_tval_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    return gt_tval_read(env, ri, GTIMER_SEC_VEL2);
+}
+
+static void gt_sec_vel2_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                                   uint64_t value)
+{
+    gt_tval_write(env, ri, GTIMER_SEC_VEL2, value);
+}
+
+static void gt_sec_vel2_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    gt_ctl_write(env, ri, GTIMER_SEC_VEL2, value);
+}
+
 static void gt_hv_timer_reset(CPUARMState *env, const ARMCPRegInfo *ri)
 {
     gt_timer_reset(env, ri, GTIMER_HYPVIRT);
@@ -3242,6 +3336,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_SEC_PEL2);
+}
+
+void arm_gt_sel2vtimer_cb(void *opaque)
+{
+    ARMCPU *cpu = opaque;
+
+    gt_recalc_timer(cpu, GTIMER_SEC_VEL2);
+}
+
 void arm_gt_hvtimer_cb(void *opaque)
 {
     ARMCPU *cpu = opaque;
@@ -6624,6 +6732,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_SEC_PEL2].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_SEC_PEL2].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_SEC_VEL2].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_SEC_VEL2].cval),
+      .writefn = gt_sec_vel2_cval_write, .raw_writefn = raw_write,
+    },
+#endif
 };
 
 static CPAccessResult nsacr_access(CPUARMState *env, const ARMCPRegInfo *ri,
-- 
2.39.5

Re: [PATCH 3/3] target/arm: implement SEL2 physical and virtual timers

[Peter Maydell]

On Fri, 6 Dec 2024 at 16:02, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> When FEAT_SEL2 was implemented the SEL2 timers where 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>
>
> ---
> v1
>   - add better comments to GTIMER descriptions
>   - also define new timers for sbsa-ref
>   - don't conditionally gate qemu_timer creation on the feature
>   - take cntvoff_el2 int account for SEC_VEL2 in gt_recalc/g_tval_[read|write]
> ---
>  include/hw/arm/bsa.h |   2 +
>  target/arm/cpu.h     |   2 +
>  target/arm/gtimer.h  |   4 +-
>  hw/arm/sbsa-ref.c    |   2 +
>  hw/arm/virt.c        |   2 +

I would put the board changes in their own patch(es).


> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index cd147b717a..f82503304e 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2668,6 +2668,41 @@ 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:
> +        return CP_ACCESS_TRAP;
> +    case 1:
> +        if (!arm_is_secure(env)) {
> +            return CP_ACCESS_TRAP;
> +        } else if (arm_hcr_el2_eff(env) & HCR_NV) {
> +            return CP_ACCESS_TRAP_EL2;
> +        }
> +        return CP_ACCESS_TRAP;
> +    case 2:
> +        if (!arm_is_secure(env)) {
> +            return CP_ACCESS_TRAP;
> +        }
> +        return CP_ACCESS_OK;
> +    case 3:
> +        if (env->cp15.scr_el3 & SCR_EEL2) {
> +            return CP_ACCESS_OK;
> +        } else {
> +            return CP_ACCESS_TRAP;
> +        }

These should generally be using CP_ACCESS_TRAP_UNCATEGORIZED,
not CP_ACCESS_TRAP. The pseudocode uses "UNDEF", which means
it wants ESR to be reported as an uncategorized-exception
(classic UNDEF), not as a "trapped system register access".

Almost always a trapped-sysreg-access is directed to a
specific EL; an UNDEF is never directed to a specific EL
but always to the usual destination for exceptions.
I should probably check whether the other uses of
CP_ACCESS_TRAP are correct or just bugs we haven't noticed
yet...

> +    default:
> +        g_assert_not_reached();
> +    }
> +}

thanks
-- PMM

Re: [PATCH 3/3] target/arm: implement SEL2 physical and virtual timers

[Alex Bennée]

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

> On Fri, 6 Dec 2024 at 16:02, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> When FEAT_SEL2 was implemented the SEL2 timers where 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>
>>
>> ---
>> v1
>>   - add better comments to GTIMER descriptions
>>   - also define new timers for sbsa-ref
>>   - don't conditionally gate qemu_timer creation on the feature
>>   - take cntvoff_el2 int account for SEC_VEL2 in gt_recalc/g_tval_[read|write]
>> ---
>>  include/hw/arm/bsa.h |   2 +
>>  target/arm/cpu.h     |   2 +
>>  target/arm/gtimer.h  |   4 +-
>>  hw/arm/sbsa-ref.c    |   2 +
>>  hw/arm/virt.c        |   2 +
>
> I would put the board changes in their own patch(es).

Won't that break bisection?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 3/3] target/arm: implement SEL2 physical and virtual timers

[Peter Maydell]

On Mon, 16 Dec 2024 at 19:32, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Fri, 6 Dec 2024 at 16:02, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >> When FEAT_SEL2 was implemented the SEL2 timers where 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>
> >>
> >> ---
> >> v1
> >>   - add better comments to GTIMER descriptions
> >>   - also define new timers for sbsa-ref
> >>   - don't conditionally gate qemu_timer creation on the feature
> >>   - take cntvoff_el2 int account for SEC_VEL2 in gt_recalc/g_tval_[read|write]
> >> ---
> >>  include/hw/arm/bsa.h |   2 +
> >>  target/arm/cpu.h     |   2 +
> >>  target/arm/gtimer.h  |   4 +-
> >>  hw/arm/sbsa-ref.c    |   2 +
> >>  hw/arm/virt.c        |   2 +
> >
> > I would put the board changes in their own patch(es).
>
> Won't that break bisection?

Any guest code attempting to use this timer currently is
not going to work because the registers don't even exist.
So there's no previous working state that would be broken.

thanks
-- PMM

Re: [PATCH 3/3] target/arm: implement SEL2 physical and virtual timers

[Peter Maydell]

On Fri, 6 Dec 2024 at 16:02, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> When FEAT_SEL2 was implemented the SEL2 timers where 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

> diff --git a/include/hw/arm/bsa.h b/include/hw/arm/bsa.h
> index 8eaab603c0..b4ecca1b1c 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_VIRT_EL2_IRQ  19

Can we call this ARM_TIMER_S_EL2_VIRT_IRQ please?
We currently have ARCH_TIMER_NS_EL2_VIRT_IRQ
so we should be consistent about where in
the name we put the "VIRT" bit.

> +#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

-- PMM

Re: [PATCH 3/3] target/arm: implement SEL2 physical and virtual timers

[Alex Bennée]

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

> On Fri, 6 Dec 2024 at 16:02, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> When FEAT_SEL2 was implemented the SEL2 timers where 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
>
>> diff --git a/include/hw/arm/bsa.h b/include/hw/arm/bsa.h
>> index 8eaab603c0..b4ecca1b1c 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_VIRT_EL2_IRQ  19
>
> Can we call this ARM_TIMER_S_EL2_VIRT_IRQ please?

I'm going to assume you mean ARCH_TIMER_S_EL2_VIRT_IRQ ;-)

> We currently have ARCH_TIMER_NS_EL2_VIRT_IRQ
> so we should be consistent about where in
> the name we put the "VIRT" bit.
>
>> +#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
>
> -- PMM

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro