cpu_exec_halt: make method mandatory

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

Posted by Peter Maydell 1 year, 8 months ago

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;
         }
-- 
2.34.1

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

Posted by Philippe Mathieu-Daudé 1 year, 8 months ago

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>

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

Posted by Peter Maydell 1 year, 8 months ago

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

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

Posted by Philippe Mathieu-Daudé 1 year, 8 months ago

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