On Tue, 30 Apr 2024 at 18:15, Alex Bennée <alex.ben...@linaro.org> wrote: > > 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?
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