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

[PATCH v2 0/5] 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.

v2
  - split machine enabling into patches
  - rename IRQ
  - use CP_ACCESS_TRAP_UNCATEGORIZED for UNDEF cases

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

The following still need review:

  hw/arm: enable secure EL2 timers for sbsa machine
  hw/arm: enable secure EL2 timers for virt machine
  target/arm: implement SEL2 physical and virtual timers
  target/arm: ensure cntvoff_el2 also used for EL2 virt timer

Alex.

Alex Bennée (5):
  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
  hw/arm: enable secure EL2 timers for virt machine
  hw/arm: enable secure EL2 timers for sbsa machine

 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 v2 1/5] 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>
Reviewed-by: Peter Maydell <peter.maydell@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

[PATCH v2 2/5] 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 910ae62c47..5a1b416e18 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2465,16 +2465,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 v2 3/5] 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]
v2
  - rename IRQ to ARCH_TIMER_S_EL2_VIRT_IRQ
  - split machine enablement into separate patches
  - return CP_ACCESS_TRAP_UNCATEGORIZED for UNDEF cases
---
 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  | 158 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 169 insertions(+), 1 deletion(-)

diff --git a/include/hw/arm/bsa.h b/include/hw/arm/bsa.h
index 8eaab603c0..13ed2d2ac1 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 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/target/arm/cpu.c b/target/arm/cpu.c
index 1afa07511e..631cc2728d 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2088,6 +2088,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 5a1b416e18..79894b4802 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2401,6 +2401,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_UNCATEGORIZED;
+        } 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_UNCATEGORIZED;
+        }
+        return CP_ACCESS_OK;
+    case 3:
+        if (env->cp15.scr_el3 & SCR_EEL2) {
+            return CP_ACCESS_OK;
+        } else {
+            return CP_ACCESS_TRAP_UNCATEGORIZED;
+        }
+    default:
+        g_assert_not_reached();
+    }
+}
+
 uint64_t gt_get_countervalue(CPUARMState *env)
 {
     ARMCPU *cpu = env_archcpu(env);
@@ -2477,6 +2512,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:
@@ -2591,6 +2627,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:
@@ -2611,6 +2648,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:
@@ -2919,6 +2957,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);
@@ -2975,6 +3069,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;
@@ -5696,6 +5804,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 v2 3/5] target/arm: implement SEL2 physical and virtual timers

[Philippe Mathieu-Daudé]

On 18/12/24 19:15, Alex Bennée 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]
> v2
>    - rename IRQ to ARCH_TIMER_S_EL2_VIRT_IRQ
>    - split machine enablement into separate patches
>    - return CP_ACCESS_TRAP_UNCATEGORIZED for UNDEF cases
> ---
>   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  | 158 +++++++++++++++++++++++++++++++++++++++++++
>   5 files changed, 169 insertions(+), 1 deletion(-)


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

Should we rename as GTIMER_SEC_PEL3 for consistency?

>       GTIMER_HYPVIRT  = 4, /* EL2 virtual timer */

Also GTIMER_HYP     -> GTIMER_PEL2,
      GTIMER_HYPVIRT -> GTIMER_VEL2?

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

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

[Daniel Wielandt]

[-- Attachment #1: Type: text/plain, Size: 2610 bytes --]

It's not the context that lacks its the protocol.. lol. I wish I was good
with words and a genius... oh no wait. It's easier being simple..you
think.u could find the s
B listers. There's gotta b a mpatch

On Thu, Jan 9, 2025, 5:19 AM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> On 18/12/24 19:15, Alex Bennée 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]
> > v2
> >    - rename IRQ to ARCH_TIMER_S_EL2_VIRT_IRQ
> >    - split machine enablement into separate patches
> >    - return CP_ACCESS_TRAP_UNCATEGORIZED for UNDEF cases
> > ---
> >   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  | 158 +++++++++++++++++++++++++++++++++++++++++++
> >   5 files changed, 169 insertions(+), 1 deletion(-)
>
>
> > 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 */
>
> Should we rename as GTIMER_SEC_PEL3 for consistency?
>
> >       GTIMER_HYPVIRT  = 4, /* EL2 virtual timer */
>
> Also GTIMER_HYP     -> GTIMER_PEL2,
>       GTIMER_HYPVIRT -> GTIMER_VEL2?
>
> > -#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|
>
>

[-- Attachment #2: Type: text/html, Size: 3678 bytes --]

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

[Peter Maydell]

On Wed, 18 Dec 2024 at 18:15, 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]
> v2
>   - rename IRQ to ARCH_TIMER_S_EL2_VIRT_IRQ
>   - split machine enablement into separate patches
>   - return CP_ACCESS_TRAP_UNCATEGORIZED for UNDEF cases
> ---

> +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_UNCATEGORIZED;
> +        } 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_UNCATEGORIZED;
> +        }
> +        return CP_ACCESS_OK;
> +    case 3:
> +        if (env->cp15.scr_el3 & SCR_EEL2) {
> +            return CP_ACCESS_OK;
> +        } else {
> +            return CP_ACCESS_TRAP_UNCATEGORIZED;
> +        }
> +    default:
> +        g_assert_not_reached();
> +    }
> +}

This code is still using CP_ACCESS_TRAP in some codepaths, which
isn't correct. Either:
 * you want an UNDEF: that's CP_ACCESS_TRAP_UNCATEGORIZED
 * you want to trap to some specific EL: that's CP_ACCESS_TRAP_EL2
   or CP_ACCESS_TRAP_EL3 depending on where you need to trap to.

thanks
-- PMM

[PATCH v2 4/5] hw/arm: enable secure EL2 timers for virt machine

[Alex Bennée]

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

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 333eaf67ea..5e3589dc6a 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_EL2_VIRT_IRQ,
         };
 
         for (unsigned irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
-- 
2.39.5

Re: [PATCH v2 4/5] hw/arm: enable secure EL2 timers for virt machine

[Peter Maydell]

On Wed, 18 Dec 2024 at 18:15, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: qemu-stable@nongnu.org
> ---
>  hw/arm/virt.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 333eaf67ea..5e3589dc6a 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_EL2_VIRT_IRQ,
>          };
>
>          for (unsigned irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {

Do these timer interrupts have a defined devicetree binding that
we need to set up in fdt_add_timer_nodes()? How about ACPI?

thanks
-- PMM

Re: [PATCH v2 4/5] hw/arm: enable secure EL2 timers for virt machine

[Alex Bennée]

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

> On Wed, 18 Dec 2024 at 18:15, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: qemu-stable@nongnu.org
>> ---
>>  hw/arm/virt.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 333eaf67ea..5e3589dc6a 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_EL2_VIRT_IRQ,
>>          };
>>
>>          for (unsigned irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
>
> Do these timer interrupts have a defined devicetree binding that
> we need to set up in fdt_add_timer_nodes()? How about ACPI?

The DT in the kernel doesn't care (why would it, it never sees the SEL2
timers). The hafnium test case works without it.

I'm not sure where there is a DT specification that describes what
should be there for SEL2.

>
> thanks
> -- PMM

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH v2 5/5] hw/arm: enable secure EL2 timers for sbsa machine

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: qemu-stable@nongnu.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 e3195d5449..c02344004e 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_EL2_VIRT_IRQ,
         };
 
         for (irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
-- 
2.39.5

Re: [PATCH v2 5/5] hw/arm: enable secure EL2 timers for sbsa machine

[Peter Maydell]

On Wed, 18 Dec 2024 at 18:15, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: qemu-stable@nongnu.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 e3195d5449..c02344004e 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_EL2_VIRT_IRQ,
>          };
>
>          for (irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
> --
> 2.39.5

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

thanks
-- PMM

Re: [PATCH v2 0/5] target/arm: implement SEL2 physical and virtual timers

[Alex Bennée]

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

> 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.
>
> v2
>   - split machine enabling into patches
>   - rename IRQ
>   - use CP_ACCESS_TRAP_UNCATEGORIZED for UNDEF cases
>
> v1
>   - improve GTIMER docs
>   - fix gt_recalc bug
>   - address review comments for the main patch
>   - cc qemu-stable (no rush for 9.2.0)
>
> The following still need review:
>
>   hw/arm: enable secure EL2 timers for sbsa machine
>   hw/arm: enable secure EL2 timers for virt machine
>   target/arm: implement SEL2 physical and virtual timers
>   target/arm: ensure cntvoff_el2 also used for EL2 virt timer

Gentle ping - I think everything is ready for merging.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 0/5] target/arm: implement SEL2 physical and virtual timers

[Peter Maydell]

On Thu, 9 Jan 2025 at 10:40, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Alex Bennée <alex.bennee@linaro.org> writes:
>
> > 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.
> >
> > v2
> >   - split machine enabling into patches
> >   - rename IRQ
> >   - use CP_ACCESS_TRAP_UNCATEGORIZED for UNDEF cases
> >
> > v1
> >   - improve GTIMER docs
> >   - fix gt_recalc bug
> >   - address review comments for the main patch
> >   - cc qemu-stable (no rush for 9.2.0)
> >
> > The following still need review:
> >
> >   hw/arm: enable secure EL2 timers for sbsa machine
> >   hw/arm: enable secure EL2 timers for virt machine
> >   target/arm: implement SEL2 physical and virtual timers
> >   target/arm: ensure cntvoff_el2 also used for EL2 virt timer
>
> Gentle ping - I think everything is ready for merging.

It's on my queue to test and review, but I'm likely to be lagging
behind on code review til the end of the month :-)

-- PMM