Re: [PATCH 3/3] accel/tcg: Make TCGCPUOps::cpu_exec_halt mandatory

[Philippe Mathieu-Daudé]

On 11/6/24 10:36, Peter Maydell wrote:
> On Tue, 11 Jun 2024 at 09:25, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > Hi Peter,
> >
> > On 3/6/24 18:09, Peter Maydell wrote:
> > > Now that all targets set TCGCPUOps::cpu_exec_halt, we can make it
> > > mandatory and remove the fallback handling that calls cpu_has_work.
> > >
> > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > > ---
> > >    include/hw/core/tcg-cpu-ops.h | 9 ++++++---
> > >    accel/tcg/cpu-exec.c          | 7 +------
> > >    2 files changed, 7 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
> > > index 099de3375e3..34318cf0e60 100644
> > > --- a/include/hw/core/tcg-cpu-ops.h
> > > +++ b/include/hw/core/tcg-cpu-ops.h
> > > @@ -122,10 +122,13 @@ struct TCGCPUOps {
> > >         * to do when the CPU is in the halted state.
> > >         *
> > >         * Return true to indicate that the CPU should now leave halt, false
> > > -     * if it should remain in the halted state.
> > > +     * if it should remain in the halted state. (This should generally
> > > +     * be the same value that cpu_has_work() would return.)
> > >         *
> > > -     * If this method is not provided, the default is to do nothing, and
> > > -     * to leave halt if cpu_has_work() returns true.
> > > +     * This method must be provided. If the target does not need to
> > > +     * do anything special for halt, the same function used for its
> > > +     * CPUClass::has_work method can be used here, as they have the
> > > +     * same function signature.
> > >         */
> > >        bool (*cpu_exec_halt)(CPUState *cpu);
> > >        /**
> > > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> > > index 6711b58e0b2..8be4d2a1330 100644
> > > --- a/accel/tcg/cpu-exec.c
> > > +++ b/accel/tcg/cpu-exec.c
> > > @@ -682,13 +682,8 @@ 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;
> > > +        bool leave_halt = tcg_ops->cpu_exec_halt(cpu);
> > >
> > > -        if (tcg_ops->cpu_exec_halt) {
> > > -            leave_halt = tcg_ops->cpu_exec_halt(cpu);
> > > -        } else {
> > > -            leave_halt = cpu_has_work(cpu);
> > > -        }
> > >            if (!leave_halt) {
> > >                return true;
> > >            }
> >
> > Could we assert the handler is assigned in tcg_exec_realizefn()?
>
> Yeah, we could. I thought about an assert that it was set up,
> but couldn't identify a place to do that.
>
> > If you agree I could squash these 3 lines:
> >
> > -- >8 --
> > --- a/accel/tcg/cpu-exec.c
> > +++ b/accel/tcg/cpu-exec.c
> > @@ -1077,6 +1077,10 @@ bool tcg_exec_realizefn(CPUState *cpu, Error **errp)
> >        static bool tcg_target_initialized;
> >
> >        if (!tcg_target_initialized) {
> > +        /* Check mandatory TCGCPUOps handlers */
> > +        assert(cpu->cc->tcg_ops->initialize);
> > +        assert(cpu->cc->tcg_ops->cpu_exec_halt);
> > +
> >            cpu->cc->tcg_ops->initialize();
>
> I don't think we need to assert initialize if we're about to call
> it anyway -- the call will crash if it's NULL in an easy to diagnose way.

Pro of assert: obvious error message on stderr.

Con of crash: we need to use a debugger to figure out the NULL deref.

Anyway, series queued without the "assert(initialize)" squashed,

Thanks!

> >            tcg_target_initialized = true;
> >        }
> > ---
> >
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> thanks
> -- PMM