Peter Maydell <peter.mayd...@linaro.org> writes:
> 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.mayd...@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(-)
>
> diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
> index dc1f16a9777..f3ac76e6f6d 100644
> --- a/include/hw/core/tcg-cpu-ops.h
> +++ b/include/hw/core/tcg-cpu-ops.h
> @@ -111,8 +111,15 @@ struct TCGCPUOps {
> void (*do_interrupt)(CPUState *cpu);
> /** @cpu_exec_interrupt: Callback for processing interrupts in cpu_exec
> */
> bool (*cpu_exec_interrupt)(CPUState *cpu, int interrupt_request);
> - /** @cpu_exec_halt: Callback for handling halt in cpu_exec */
> - void (*cpu_exec_halt)(CPUState *cpu);
> + /**
> + * @cpu_exec_halt: Callback for handling halt in cpu_exec.
> + *
> + * Return true to indicate that the CPU should now leave halt, false
> + * if it should remain in the halted state.
> + * If this method is not provided, the default is to leave halt
> + * if cpu_has_work() returns true.
> + */
> + bool (*cpu_exec_halt)(CPUState *cpu);
Would it be too much to rename the method to cpu_exec_leave_halt() to
make it clearer on use the sense of the return value?
> /**
> * @tlb_fill: Handle a softmmu tlb miss
> *
> diff --git a/target/i386/tcg/helper-tcg.h b/target/i386/tcg/helper-tcg.h
> index effc2c1c984..85957943bf3 100644
> --- a/target/i386/tcg/helper-tcg.h
> +++ b/target/i386/tcg/helper-tcg.h
> @@ -39,7 +39,7 @@ QEMU_BUILD_BUG_ON(TCG_PHYS_ADDR_BITS >
> TARGET_PHYS_ADDR_SPACE_BITS);
> */
> void x86_cpu_do_interrupt(CPUState *cpu);
> #ifndef CONFIG_USER_ONLY
> -void x86_cpu_exec_halt(CPUState *cpu);
> +bool x86_cpu_exec_halt(CPUState *cpu);
> bool x86_need_replay_interrupt(int interrupt_request);
> bool x86_cpu_exec_interrupt(CPUState *cpu, int int_req);
> #endif
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 5c70748060a..550f93b19ce 100644
> --- 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;
> }
>
> diff --git a/target/i386/tcg/sysemu/seg_helper.c
> b/target/i386/tcg/sysemu/seg_helper.c
> index 2db8083748e..9ba94deb3aa 100644
> --- a/target/i386/tcg/sysemu/seg_helper.c
> +++ b/target/i386/tcg/sysemu/seg_helper.c
> @@ -128,7 +128,7 @@ void x86_cpu_do_interrupt(CPUState *cs)
> }
> }
>
> -void x86_cpu_exec_halt(CPUState *cpu)
> +bool x86_cpu_exec_halt(CPUState *cpu)
> {
> if (cpu->interrupt_request & CPU_INTERRUPT_POLL) {
> X86CPU *x86_cpu = X86_CPU(cpu);
> @@ -138,6 +138,7 @@ void x86_cpu_exec_halt(CPUState *cpu)
> cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
> bql_unlock();
> }
> + return cpu_has_work(cpu);
The x86 version is essentially being called for side effects. Do we want
to document this usage in the method?
> }
>
> bool x86_need_replay_interrupt(int interrupt_request)
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
