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

[Peter Maydell <peter.maydell@linaro.org>]

On Tue, 30 Apr 2024 at 18:15, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@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.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(-)
> >
> > 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?

We could, but that makes it sound like it's a method to say
"should we leave halt?", which ...

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

...is not how the x86 target is using it, as you note.

thanks
-- PMM