[PATCH 0/5] target/arm: a few timer fixes while checking FEAT_ECV_POFF

[PATCH 0/5] target/arm: a few timer fixes while checking FEAT_ECV_POFF

[Alex Bennée]

While I was checking what was needed for FEAT_ECV_POFF I discovered a
few potential correctness issues. We had actually already done the
work for the feature so the final patch is just documentation.

Alex.

Alex Bennée (5):
  target/arm: split evaluation of CNTHCTL timer IRQ masks
  target/arm: trigger timer recalculation when toggling CNTHCTL:ECV
  target/arm: trigger timer recalc on SCR:ECVEN change
  target/arm: trigger timer recalc on HCR:(E2H|TGE) changes
  docs/system: add FEAT_ECV_POFF to the emulation list

 docs/system/arm/emulation.rst |  1 +
 target/arm/helper.c           | 20 +++++++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

-- 
2.47.3

[PATCH 1/5] target/arm: split evaluation of CNTHCTL timer IRQ masks

[Alex Bennée]

Whether the physical and virtual timer IRQs are masked are independent
of each other so the checking of CNTHCTL:CNTPMASK shouldn't depend of
not changing CNTVMASK.

While unlikely to be seen in real life we should still make sure we
behave correctly.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 9dd8fdfa41b..f4aae0a4072 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1802,7 +1802,8 @@ static void gt_cnthctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
 
     if ((oldval ^ value) & R_CNTHCTL_CNTVMASK_MASK) {
         gt_update_irq(cpu, GTIMER_VIRT);
-    } else if ((oldval ^ value) & R_CNTHCTL_CNTPMASK_MASK) {
+    }
+    if ((oldval ^ value) & R_CNTHCTL_CNTPMASK_MASK) {
         gt_update_irq(cpu, GTIMER_PHYS);
     }
 }
-- 
2.47.3

[PATCH 2/5] target/arm: trigger timer recalculation when toggling CNTHCTL:ECV

[Alex Bennée]

When toggling the state of ECV we affect the offset applied to timers.
As a result we should trigger a recalculation of the timer value to
take into account the new offset.

Signed-off-by: Alex Bennée <alex.bennee@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 f4aae0a4072..5c640853060 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1806,6 +1806,9 @@ static void gt_cnthctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
     if ((oldval ^ value) & R_CNTHCTL_CNTPMASK_MASK) {
         gt_update_irq(cpu, GTIMER_PHYS);
     }
+    if ((oldval ^ value) & R_CNTHCTL_ECV_MASK) {
+        gt_recalc_timer(cpu, GTIMER_PHYS);
+    }
 }
 
 static void gt_cntvoff_write(CPUARMState *env, const ARMCPRegInfo *ri,
-- 
2.47.3

[PATCH 3/5] target/arm: trigger timer recalc on SCR:ECVEN change

[Alex Bennée]

Toggling the ECVEN state affects the offset calculated in
gt_phys_raw_cnt_offset so we should trigger a re-calculation on its
change.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/helper.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 5c640853060..22f5117ee54 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -37,6 +37,9 @@
 #include "qemu/plugin.h"
 
 static void switch_mode(CPUARMState *env, int mode);
+#ifndef CONFIG_USER_ONLY
+static void gt_recalc_timer(ARMCPU *cpu, int timeridx);
+#endif
 
 int compare_u64(const void *a, const void *b)
 {
@@ -821,6 +824,12 @@ static void scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
     changed = env->cp15.scr_el3 ^ value;
     env->cp15.scr_el3 = value;
 
+#ifndef CONFIG_USER_ONLY
+    if (changed & SCR_ECVEN) {
+        gt_recalc_timer(cpu, GTIMER_PHYS);
+    }
+#endif
+
     /*
      * If SCR_EL3.{NS,NSE} changes, i.e. change of security state,
      * we must invalidate all TLBs below EL3.
-- 
2.47.3

[PATCH 4/5] target/arm: trigger timer recalc on HCR:(E2H|TGE) changes

[Alex Bennée]

Toggling the HCR state affects the offset calculated in
gt_phys_raw_cnt_offset so we should trigger a re-calculation on its
change.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 22f5117ee54..4e7d1178fb8 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3799,6 +3799,11 @@ static void do_hcr_write(CPUARMState *env, uint64_t value, uint64_t valid_mask)
         (HCR_VM | HCR_PTW | HCR_DC | HCR_DCT | HCR_FWB | HCR_NV | HCR_NV1)) {
         tlb_flush(CPU(cpu));
     }
+#ifndef CONFIG_USER_ONLY
+    if ((env->cp15.hcr_el2 ^ value) & (HCR_E2H | HCR_TGE)) {
+        gt_recalc_timer(cpu, GTIMER_PHYS);
+    }
+#endif
     env->cp15.hcr_el2 = value;
 
     /*
-- 
2.47.3

[PATCH 5/5] docs/system: add FEAT_ECV_POFF to the emulation list

[Alex Bennée]

We already had this implemented since 2808d3b38a5 (target/arm:
Implement FEAT_ECV CNTPOFF_EL2 handling) but it has its own feature
name now. Add it to the list.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/system/arm/emulation.rst | 1 +
 1 file changed, 1 insertion(+)

diff --git a/docs/system/arm/emulation.rst b/docs/system/arm/emulation.rst
index 18c63559679..3622e3889c7 100644
--- a/docs/system/arm/emulation.rst
+++ b/docs/system/arm/emulation.rst
@@ -60,6 +60,7 @@ the following architecture extensions:
 - FEAT_E2H0 (Programming of HCR_EL2.E2H)
 - FEAT_EBF16 (AArch64 Extended BFloat16 instructions)
 - FEAT_ECV (Enhanced Counter Virtualization)
+- FEAT_ECV_POFF (Enhanced Counter Virtualization Physical Offset)
 - FEAT_EL0 (Support for execution at EL0)
 - FEAT_EL1 (Support for execution at EL1)
 - FEAT_EL2 (Support for execution at EL2)
-- 
2.47.3

Re: [PATCH 4/5] target/arm: trigger timer recalc on HCR:(E2H|TGE) changes

[Richard Henderson]

On 6/9/26 07:00, Alex Bennée wrote:
> Toggling the HCR state affects the offset calculated in
> gt_phys_raw_cnt_offset so we should trigger a re-calculation on its
> change.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/arm/helper.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 22f5117ee54..4e7d1178fb8 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3799,6 +3799,11 @@ static void do_hcr_write(CPUARMState *env, uint64_t value, uint64_t valid_mask)
>           (HCR_VM | HCR_PTW | HCR_DC | HCR_DCT | HCR_FWB | HCR_NV | HCR_NV1)) {
>           tlb_flush(CPU(cpu));
>       }
> +#ifndef CONFIG_USER_ONLY
> +    if ((env->cp15.hcr_el2 ^ value) & (HCR_E2H | HCR_TGE)) {
> +        gt_recalc_timer(cpu, GTIMER_PHYS);
> +    }
> +#endif
>       env->cp15.hcr_el2 = value;
>   
>       /*

The recalc must be after assigning to hcr_el2.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 1/5] target/arm: split evaluation of CNTHCTL timer IRQ masks

[Richard Henderson]

On 6/9/26 07:00, Alex Bennée wrote:
> Whether the physical and virtual timer IRQs are masked are independent
> of each other so the checking of CNTHCTL:CNTPMASK shouldn't depend of
> not changing CNTVMASK.
> 
> While unlikely to be seen in real life we should still make sure we
> behave correctly.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/arm/helper.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 9dd8fdfa41b..f4aae0a4072 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1802,7 +1802,8 @@ static void gt_cnthctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
>   
>       if ((oldval ^ value) & R_CNTHCTL_CNTVMASK_MASK) {
>           gt_update_irq(cpu, GTIMER_VIRT);
> -    } else if ((oldval ^ value) & R_CNTHCTL_CNTPMASK_MASK) {
> +    }
> +    if ((oldval ^ value) & R_CNTHCTL_CNTPMASK_MASK) {
>           gt_update_irq(cpu, GTIMER_PHYS);
>       }
>   }

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/5] target/arm: trigger timer recalculation when toggling CNTHCTL:ECV

[Richard Henderson]

On 6/9/26 07:00, Alex Bennée wrote:
> When toggling the state of ECV we affect the offset applied to timers.
> As a result we should trigger a recalculation of the timer value to
> take into account the new offset.
> 
> Signed-off-by: Alex Bennée <alex.bennee@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 f4aae0a4072..5c640853060 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1806,6 +1806,9 @@ static void gt_cnthctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
>       if ((oldval ^ value) & R_CNTHCTL_CNTPMASK_MASK) {
>           gt_update_irq(cpu, GTIMER_PHYS);
>       }
> +    if ((oldval ^ value) & R_CNTHCTL_ECV_MASK) {
> +        gt_recalc_timer(cpu, GTIMER_PHYS);
> +    }
>   }
>   
>   static void gt_cntvoff_write(CPUARMState *env, const ARMCPRegInfo *ri,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 3/5] target/arm: trigger timer recalc on SCR:ECVEN change

[Richard Henderson]

On 6/9/26 07:00, Alex Bennée wrote:
> Toggling the ECVEN state affects the offset calculated in
> gt_phys_raw_cnt_offset so we should trigger a re-calculation on its
> change.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/arm/helper.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 5c640853060..22f5117ee54 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -37,6 +37,9 @@
>   #include "qemu/plugin.h"
>   
>   static void switch_mode(CPUARMState *env, int mode);
> +#ifndef CONFIG_USER_ONLY
> +static void gt_recalc_timer(ARMCPU *cpu, int timeridx);
> +#endif
>   
>   int compare_u64(const void *a, const void *b)
>   {
> @@ -821,6 +824,12 @@ static void scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>       changed = env->cp15.scr_el3 ^ value;
>       env->cp15.scr_el3 = value;
>   
> +#ifndef CONFIG_USER_ONLY
> +    if (changed & SCR_ECVEN) {
> +        gt_recalc_timer(cpu, GTIMER_PHYS);
> +    }
> +#endif
> +
>       /*
>        * If SCR_EL3.{NS,NSE} changes, i.e. change of security state,
>        * we must invalidate all TLBs below EL3.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

As an aside, gt_phys_raw_cnt_offset is missing a check on EL3 existing, before the check 
for ECVEn set.


r~

Re: [PATCH 5/5] docs/system: add FEAT_ECV_POFF to the emulation list

[Richard Henderson]

On 6/9/26 07:00, Alex Bennée wrote:
> We already had this implemented since 2808d3b38a5 (target/arm:
> Implement FEAT_ECV CNTPOFF_EL2 handling) but it has its own feature
> name now. Add it to the list.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   docs/system/arm/emulation.rst | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/docs/system/arm/emulation.rst b/docs/system/arm/emulation.rst
> index 18c63559679..3622e3889c7 100644
> --- a/docs/system/arm/emulation.rst
> +++ b/docs/system/arm/emulation.rst
> @@ -60,6 +60,7 @@ the following architecture extensions:
>   - FEAT_E2H0 (Programming of HCR_EL2.E2H)
>   - FEAT_EBF16 (AArch64 Extended BFloat16 instructions)
>   - FEAT_ECV (Enhanced Counter Virtualization)
> +- FEAT_ECV_POFF (Enhanced Counter Virtualization Physical Offset)
>   - FEAT_EL0 (Support for execution at EL0)
>   - FEAT_EL1 (Support for execution at EL1)
>   - FEAT_EL2 (Support for execution at EL2)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 3/5] target/arm: trigger timer recalc on SCR:ECVEN change

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 6/9/26 07:00, Alex Bennée wrote:
>> Toggling the ECVEN state affects the offset calculated in
>> gt_phys_raw_cnt_offset so we should trigger a re-calculation on its
>> change.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   target/arm/helper.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index 5c640853060..22f5117ee54 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -37,6 +37,9 @@
>>   #include "qemu/plugin.h"
>>     static void switch_mode(CPUARMState *env, int mode);
>> +#ifndef CONFIG_USER_ONLY
>> +static void gt_recalc_timer(ARMCPU *cpu, int timeridx);
>> +#endif
>>     int compare_u64(const void *a, const void *b)
>>   {
>> @@ -821,6 +824,12 @@ static void scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>>       changed = env->cp15.scr_el3 ^ value;
>>       env->cp15.scr_el3 = value;
>>   +#ifndef CONFIG_USER_ONLY
>> +    if (changed & SCR_ECVEN) {
>> +        gt_recalc_timer(cpu, GTIMER_PHYS);
>> +    }
>> +#endif
>> +
>>       /*
>>        * If SCR_EL3.{NS,NSE} changes, i.e. change of security state,
>>        * we must invalidate all TLBs below EL3.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> As an aside, gt_phys_raw_cnt_offset is missing a check on EL3
> existing, before the check for ECVEn set.

Not many of the references to cp15.scr_el3 seem to have those checks
(although I guess that may be inferred from higher up the call chain).

I can add a arm_feature(env, ARM_FEATURE_EL3) in front or maybe we want
something similar to arm_is_el2_enabled?

>
>
> r~

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 3/5] target/arm: trigger timer recalc on SCR:ECVEN change

[Richard Henderson]

On 6/10/26 05:56, Alex Bennée wrote:
> Not many of the references to cp15.scr_el3 seem to have those checks
> (although I guess that may be inferred from higher up the call chain).

It depends on the sense of the test.  Some of the bits are 1 to disable, others are 1 to 
enable.  The disable sense bits don't need the feature test.

> I can add a arm_feature(env, ARM_FEATURE_EL3) in front...

Yes please.


r~