| Message ID | 20240603160933.1141717-4-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | cpu_exec_halt: make method mandatory | expand |
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()? 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(); tcg_target_initialized = true; } --- Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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. > tcg_target_initialized = true; > } > --- > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> thanks -- PMM
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
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; }
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(-)