[PATCH 3/4] target/arm: pass rd directly to wfit helper

Alex Bennée posted 4 patches 1 month, 2 weeks ago
Maintainers: Peter Maydell <peter.maydell@linaro.org>
[PATCH 3/4] target/arm: pass rd directly to wfit helper
Posted by Alex Bennée 1 month, 2 weeks ago
If we generate an exception for WFIT we could also fill out the rv/rn
fields of the ISS. To facilitate this pass the register number
directly and read the timeout value in the helper itself.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/tcg/helper-defs.h   | 2 +-
 target/arm/tcg/op_helper.c     | 4 +++-
 target/arm/tcg/translate-a64.c | 2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/target/arm/tcg/helper-defs.h b/target/arm/tcg/helper-defs.h
index 5a10a9fba3b..a9a062cf777 100644
--- a/target/arm/tcg/helper-defs.h
+++ b/target/arm/tcg/helper-defs.h
@@ -55,7 +55,7 @@ DEF_HELPER_2(exception_pc_alignment, noreturn, env, vaddr)
 DEF_HELPER_1(setend, void, env)
 DEF_HELPER_2(wfi, void, env, i32)
 DEF_HELPER_1(wfe, void, env)
-DEF_HELPER_2(wfit, void, env, i64)
+DEF_HELPER_FLAGS_2(wfit, TCG_CALL_NO_WG, void, env, i32)
 DEF_HELPER_1(yield, void, env)
 DEF_HELPER_1(pre_hvc, void, env)
 DEF_HELPER_2(pre_smc, void, env, i32)
diff --git a/target/arm/tcg/op_helper.c b/target/arm/tcg/op_helper.c
index aa14f15eb62..28a1c7243ad 100644
--- a/target/arm/tcg/op_helper.c
+++ b/target/arm/tcg/op_helper.c
@@ -409,7 +409,7 @@ void HELPER(wfi)(CPUARMState *env, uint32_t insn_len)
 #endif
 }
 
-void HELPER(wfit)(CPUARMState *env, uint64_t timeout)
+void HELPER(wfit)(CPUARMState *env, uint32_t rd)
 {
 #ifdef CONFIG_USER_ONLY
     /*
@@ -428,6 +428,8 @@ 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 timeout = env->xregs[rd];
+
     /*
      * We want the value that we would get if we read CNTVCT_EL0 from
      * the current exception level, so the direct_access offset, not
diff --git a/target/arm/tcg/translate-a64.c b/target/arm/tcg/translate-a64.c
index 5d261a5e32b..073454b9195 100644
--- a/target/arm/tcg/translate-a64.c
+++ b/target/arm/tcg/translate-a64.c
@@ -2064,7 +2064,7 @@ static bool trans_WFIT(DisasContext *s, arg_WFIT *a)
     }
 
     gen_a64_update_pc(s, 4);
-    gen_helper_wfit(tcg_env, cpu_reg(s, a->rd));
+    gen_helper_wfit(tcg_env, tcg_constant_i32(a->rd));
     /* Go back to the main loop to check for interrupts */
     s->base.is_jmp = DISAS_EXIT;
     return true;
-- 
2.47.3


Re: [PATCH 3/4] target/arm: pass rd directly to wfit helper
Posted by Peter Maydell 1 month, 2 weeks ago
On Tue, 24 Feb 2026 at 15:43, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> If we generate an exception for WFIT we could also fill out the rv/rn
> fields of the ISS. To facilitate this pass the register number
> directly and read the timeout value in the helper itself.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/arm/tcg/helper-defs.h   | 2 +-
>  target/arm/tcg/op_helper.c     | 4 +++-
>  target/arm/tcg/translate-a64.c | 2 +-
>  3 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/target/arm/tcg/helper-defs.h b/target/arm/tcg/helper-defs.h
> index 5a10a9fba3b..a9a062cf777 100644
> --- a/target/arm/tcg/helper-defs.h
> +++ b/target/arm/tcg/helper-defs.h
> @@ -55,7 +55,7 @@ DEF_HELPER_2(exception_pc_alignment, noreturn, env, vaddr)
>  DEF_HELPER_1(setend, void, env)
>  DEF_HELPER_2(wfi, void, env, i32)
>  DEF_HELPER_1(wfe, void, env)
> -DEF_HELPER_2(wfit, void, env, i64)
> +DEF_HELPER_FLAGS_2(wfit, TCG_CALL_NO_WG, void, env, i32)

Why the change to NO_WG ? The code change doesn't remove any
writes to globals...

>  DEF_HELPER_1(yield, void, env)
>  DEF_HELPER_1(pre_hvc, void, env)
>  DEF_HELPER_2(pre_smc, void, env, i32)
> diff --git a/target/arm/tcg/op_helper.c b/target/arm/tcg/op_helper.c
> index aa14f15eb62..28a1c7243ad 100644
> --- a/target/arm/tcg/op_helper.c
> +++ b/target/arm/tcg/op_helper.c
> @@ -409,7 +409,7 @@ void HELPER(wfi)(CPUARMState *env, uint32_t insn_len)
>  #endif
>  }
>
> -void HELPER(wfit)(CPUARMState *env, uint64_t timeout)
> +void HELPER(wfit)(CPUARMState *env, uint32_t rd)
>  {
>  #ifdef CONFIG_USER_ONLY
>      /*
> @@ -428,6 +428,8 @@ 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 timeout = env->xregs[rd];
> +
>      /*
>       * We want the value that we would get if we read CNTVCT_EL0 from
>       * the current exception level, so the direct_access offset, not
> diff --git a/target/arm/tcg/translate-a64.c b/target/arm/tcg/translate-a64.c
> index 5d261a5e32b..073454b9195 100644
> --- a/target/arm/tcg/translate-a64.c
> +++ b/target/arm/tcg/translate-a64.c
> @@ -2064,7 +2064,7 @@ static bool trans_WFIT(DisasContext *s, arg_WFIT *a)
>      }
>
>      gen_a64_update_pc(s, 4);
> -    gen_helper_wfit(tcg_env, cpu_reg(s, a->rd));
> +    gen_helper_wfit(tcg_env, tcg_constant_i32(a->rd));
>      /* Go back to the main loop to check for interrupts */
>      s->base.is_jmp = DISAS_EXIT;
>      return true;

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

You might squash this in to patch 4 if you like.

thanks
-- PMM
Re: [PATCH 3/4] target/arm: pass rd directly to wfit helper
Posted by Alex Bennée 1 month, 2 weeks ago
Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 24 Feb 2026 at 15:43, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> If we generate an exception for WFIT we could also fill out the rv/rn
>> fields of the ISS. To facilitate this pass the register number
>> directly and read the timeout value in the helper itself.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  target/arm/tcg/helper-defs.h   | 2 +-
>>  target/arm/tcg/op_helper.c     | 4 +++-
>>  target/arm/tcg/translate-a64.c | 2 +-
>>  3 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/arm/tcg/helper-defs.h b/target/arm/tcg/helper-defs.h
>> index 5a10a9fba3b..a9a062cf777 100644
>> --- a/target/arm/tcg/helper-defs.h
>> +++ b/target/arm/tcg/helper-defs.h
>> @@ -55,7 +55,7 @@ DEF_HELPER_2(exception_pc_alignment, noreturn, env, vaddr)
>>  DEF_HELPER_1(setend, void, env)
>>  DEF_HELPER_2(wfi, void, env, i32)
>>  DEF_HELPER_1(wfe, void, env)
>> -DEF_HELPER_2(wfit, void, env, i64)
>> +DEF_HELPER_FLAGS_2(wfit, TCG_CALL_NO_WG, void, env, i32)
>
> Why the change to NO_WG ? The code change doesn't remove any
> writes to globals...

I thought that was the right way round - it doesn't write to any
globals, but it does read so the helper needs to ensure globals are
synced before the callback.

>
>>  DEF_HELPER_1(yield, void, env)
>>  DEF_HELPER_1(pre_hvc, void, env)
>>  DEF_HELPER_2(pre_smc, void, env, i32)
>> diff --git a/target/arm/tcg/op_helper.c b/target/arm/tcg/op_helper.c
>> index aa14f15eb62..28a1c7243ad 100644
>> --- a/target/arm/tcg/op_helper.c
>> +++ b/target/arm/tcg/op_helper.c
>> @@ -409,7 +409,7 @@ void HELPER(wfi)(CPUARMState *env, uint32_t insn_len)
>>  #endif
>>  }
>>
>> -void HELPER(wfit)(CPUARMState *env, uint64_t timeout)
>> +void HELPER(wfit)(CPUARMState *env, uint32_t rd)
>>  {
>>  #ifdef CONFIG_USER_ONLY
>>      /*
>> @@ -428,6 +428,8 @@ 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 timeout = env->xregs[rd];
>> +
>>      /*
>>       * We want the value that we would get if we read CNTVCT_EL0 from
>>       * the current exception level, so the direct_access offset, not
>> diff --git a/target/arm/tcg/translate-a64.c b/target/arm/tcg/translate-a64.c
>> index 5d261a5e32b..073454b9195 100644
>> --- a/target/arm/tcg/translate-a64.c
>> +++ b/target/arm/tcg/translate-a64.c
>> @@ -2064,7 +2064,7 @@ static bool trans_WFIT(DisasContext *s, arg_WFIT *a)
>>      }
>>
>>      gen_a64_update_pc(s, 4);
>> -    gen_helper_wfit(tcg_env, cpu_reg(s, a->rd));
>> +    gen_helper_wfit(tcg_env, tcg_constant_i32(a->rd));
>>      /* Go back to the main loop to check for interrupts */
>>      s->base.is_jmp = DISAS_EXIT;
>>      return true;
>
> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> You might squash this in to patch 4 if you like.
>
> thanks
> -- PMM

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro
Re: [PATCH 3/4] target/arm: pass rd directly to wfit helper
Posted by Peter Maydell 1 month, 2 weeks ago
On Thu, 26 Feb 2026 at 10:33, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Tue, 24 Feb 2026 at 15:43, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >> If we generate an exception for WFIT we could also fill out the rv/rn
> >> fields of the ISS. To facilitate this pass the register number
> >> directly and read the timeout value in the helper itself.
> >>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> ---
> >>  target/arm/tcg/helper-defs.h   | 2 +-
> >>  target/arm/tcg/op_helper.c     | 4 +++-
> >>  target/arm/tcg/translate-a64.c | 2 +-
> >>  3 files changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/target/arm/tcg/helper-defs.h b/target/arm/tcg/helper-defs.h
> >> index 5a10a9fba3b..a9a062cf777 100644
> >> --- a/target/arm/tcg/helper-defs.h
> >> +++ b/target/arm/tcg/helper-defs.h
> >> @@ -55,7 +55,7 @@ DEF_HELPER_2(exception_pc_alignment, noreturn, env, vaddr)
> >>  DEF_HELPER_1(setend, void, env)
> >>  DEF_HELPER_2(wfi, void, env, i32)
> >>  DEF_HELPER_1(wfe, void, env)
> >> -DEF_HELPER_2(wfit, void, env, i64)
> >> +DEF_HELPER_FLAGS_2(wfit, TCG_CALL_NO_WG, void, env, i32)
> >
> > Why the change to NO_WG ? The code change doesn't remove any
> > writes to globals...
>
> I thought that was the right way round - it doesn't write to any
> globals, but it does read so the helper needs to ensure globals are
> synced before the callback.

The default if you use a DEF_HELPER macro that doesn't specify
flags is "reading and writing to globals is OK" (it's a zero
flags argument). So this change goes from "flags 0" to "flags NO_WG"
and doesn't make any difference to how TCG thinks of its behaviour
w.r.t. reading globals.

I think you're right that the helper doesn't need to be able to write
globals, but making a stricter promise about what the helper does is
something that's a distinct change from adding the rn info to the syndrome.

-- PMM