Re: [PATCH 1/2] accel/tcg: Make TCGCPUOps::cpu_exec_halt return bool for whether to halt

[Richard Henderson]

On 4/30/24 07:00, Peter Maydell wrote:
> The TCGCPUOps::cpu_exec_halt method is called from cpu_handle_halt()
> when the CPU is halted, so that a target CPU emulation can do
> anything target-specific it needs to do.  (At the moment we only use
> this on i386.)
>
> The current specification of the method doesn't allow the target
> specific code to do something different if the CPU is about to come
> out of the halt state, because cpu_handle_halt() only determines this
> after the method has returned.  (If the method called cpu_has_work()
> itself this would introduce a potential race if an interrupt arrived
> between the target's method implementation checking and
> cpu_handle_halt() repeating the check.)
>
> Change the definition of the method so that it returns a bool to
> tell cpu_handle_halt() whether to stay in halt or not.
>
> We will want this for the Arm target, where FEAT_WFxT wants to do
> some work only for the case where the CPU is in halt but about to
> leave it.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/core/tcg-cpu-ops.h       | 11 +++++++++--
>   target/i386/tcg/helper-tcg.h        |  2 +-
>   accel/tcg/cpu-exec.c                |  7 +++++--
>   target/i386/tcg/sysemu/seg_helper.c |  3 ++-
>   4 files changed, 17 insertions(+), 6 deletions(-)

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

I like Alex's suggested rename.

> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -669,11 +669,14 @@ static inline bool cpu_handle_halt(CPUState *cpu)
>   #ifndef CONFIG_USER_ONLY
>       if (cpu->halted) {
>           const TCGCPUOps *tcg_ops = cpu->cc->tcg_ops;
> +        bool leave_halt;
>   
>           if (tcg_ops->cpu_exec_halt) {
> -            tcg_ops->cpu_exec_halt(cpu);
> +            leave_halt = tcg_ops->cpu_exec_halt(cpu);
> +        } else {
> +            leave_halt = cpu_has_work(cpu);
>           }
> -        if (!cpu_has_work(cpu)) {
> +        if (!leave_halt) {
>               return true;
>           }

As a followup, I would also suggest making implementation of the hook mandatory.
We already require the has_work hook to be set; it would simply be a matter of copying the 
function pointer to the second slot.

Also, the assert in cpu_has_work could be moved to startup, as Phil has started to do with 
some of the other hooks.


r~