[PATCH 0/3] cpu_exec_halt: make method mandatory

[PATCH 0/3] cpu_exec_halt: make method mandatory

[Peter Maydell]

This patchset makes the TCGCPUOps::cpu_exec_halt method
mandatory, which is a cleanup that RTH asked for when we
were discussing my FEAT_WFxT series.

I'm not 100% convinced about this, because if we ever find we
need to change the cpu_exec_halt method so it's no longer the
exact same function signature as the has_work method then
we'll have to undo this change. But I don't feel very strongly
about it, and it does mean we can skip a runtime check for
whether the method exists.

We probably want patch 1 anyway; I didn't notice at the
time that M-profile has its own TCGCPUOps, and it's less
confusing if A and M both use the same arm_cpu_exec_halt().
(This isn't a bug in the FEAT_WFxT commit, though -- the
behaviour is the same.)

thanks
-- PMM

Peter Maydell (3):
  target/arm: Set arm_v7m_tcg_ops cpu_exec_halt to arm_cpu_exec_halt()
  target: Set TCGCPUOps::cpu_exec_halt to target's has_work
    implementation
  accel/tcg: Make TCGCPUOps::cpu_exec_halt mandatory

 include/hw/core/tcg-cpu-ops.h | 9 ++++++---
 target/arm/internals.h        | 3 +++
 target/riscv/internals.h      | 3 +++
 accel/tcg/cpu-exec.c          | 7 +------
 target/alpha/cpu.c            | 1 +
 target/arm/cpu.c              | 2 +-
 target/arm/tcg/cpu-v7m.c      | 1 +
 target/avr/cpu.c              | 1 +
 target/cris/cpu.c             | 2 ++
 target/hppa/cpu.c             | 1 +
 target/loongarch/cpu.c        | 1 +
 target/m68k/cpu.c             | 1 +
 target/microblaze/cpu.c       | 1 +
 target/mips/cpu.c             | 1 +
 target/openrisc/cpu.c         | 1 +
 target/ppc/cpu_init.c         | 2 ++
 target/riscv/cpu.c            | 2 +-
 target/riscv/tcg/tcg-cpu.c    | 2 ++
 target/rx/cpu.c               | 1 +
 target/s390x/cpu.c            | 1 +
 target/sh4/cpu.c              | 1 +
 target/sparc/cpu.c            | 1 +
 target/xtensa/cpu.c           | 1 +
 23 files changed, 35 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH 1/3] target/arm: Set arm_v7m_tcg_ops cpu_exec_halt to arm_cpu_exec_halt()

[Peter Maydell]

In commit a96edb687e76 we set the cpu_exec_halt field of the
TCGCPUOps arm_tcg_ops to arm_cpu_exec_halt(), but we left the
arm_v7m_tcg_ops struct unchanged.  That isn't wrong, because for
M-profile FEAT_WFxT doesn't exist and the default handling for "no
cpu_exec_halt method" is correct, but it's perhaps a little
confusing.  We would also like to make setting the cpu_exec_halt
method mandatory.

Initialize arm_v7m_tcg_ops cpu_exec_halt to the same function we use
for A-profile.  (On M-profile we never set up the wfxt timer so there
is no change in behaviour here.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/internals.h   | 3 +++
 target/arm/cpu.c         | 2 +-
 target/arm/tcg/cpu-v7m.c | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 11b5da2562f..e45ccd983e1 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -364,6 +364,9 @@ void arm_restore_state_to_opc(CPUState *cs,
 
 #ifdef CONFIG_TCG
 void arm_cpu_synchronize_from_tb(CPUState *cs, const TranslationBlock *tb);
+
+/* Our implementation of TCGCPUOps::cpu_exec_halt */
+bool arm_cpu_exec_halt(CPUState *cs);
 #endif /* CONFIG_TCG */
 
 typedef enum ARMFPRounding {
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 35fa281f1b9..948e904bd8a 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1133,7 +1133,7 @@ static bool arm_cpu_virtio_is_big_endian(CPUState *cs)
 }
 
 #ifdef CONFIG_TCG
-static bool arm_cpu_exec_halt(CPUState *cs)
+bool arm_cpu_exec_halt(CPUState *cs)
 {
     bool leave_halt = cpu_has_work(cs);
 
diff --git a/target/arm/tcg/cpu-v7m.c b/target/arm/tcg/cpu-v7m.c
index c059c681e94..5496f14dc16 100644
--- a/target/arm/tcg/cpu-v7m.c
+++ b/target/arm/tcg/cpu-v7m.c
@@ -244,6 +244,7 @@ static const TCGCPUOps arm_v7m_tcg_ops = {
 #else
     .tlb_fill = arm_cpu_tlb_fill,
     .cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt,
+    .cpu_exec_halt = arm_cpu_exec_halt,
     .do_interrupt = arm_v7m_cpu_do_interrupt,
     .do_transaction_failed = arm_cpu_do_transaction_failed,
     .do_unaligned_access = arm_cpu_do_unaligned_access,
-- 
2.34.1

[PATCH 2/3] target: Set TCGCPUOps::cpu_exec_halt to target's has_work implementation

[Peter Maydell]

Currently the TCGCPUOps::cpu_exec_halt method is optional, and if it
is not set then the default is to call the CPUClass::has_work
method (which has an identical function signature).

We would like to make the cpu_exec_halt method mandatory so we can
remove the runtime check and fallback handling.  In preparation for
that, make all the targets which don't need special handling in their
cpu_exec_halt set it to their cpu_has_work implementation instead of
leaving it unset.  (This is every target except for arm and i386.)

In the riscv case this requires us to make the function not
be local to the source file it's defined in.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/riscv/internals.h   | 3 +++
 target/alpha/cpu.c         | 1 +
 target/avr/cpu.c           | 1 +
 target/cris/cpu.c          | 2 ++
 target/hppa/cpu.c          | 1 +
 target/loongarch/cpu.c     | 1 +
 target/m68k/cpu.c          | 1 +
 target/microblaze/cpu.c    | 1 +
 target/mips/cpu.c          | 1 +
 target/openrisc/cpu.c      | 1 +
 target/ppc/cpu_init.c      | 2 ++
 target/riscv/cpu.c         | 2 +-
 target/riscv/tcg/tcg-cpu.c | 2 ++
 target/rx/cpu.c            | 1 +
 target/s390x/cpu.c         | 1 +
 target/sh4/cpu.c           | 1 +
 target/sparc/cpu.c         | 1 +
 target/xtensa/cpu.c        | 1 +
 18 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/target/riscv/internals.h b/target/riscv/internals.h
index 8239ae83ccc..0ac17bc5adb 100644
--- a/target/riscv/internals.h
+++ b/target/riscv/internals.h
@@ -136,4 +136,7 @@ static inline float16 check_nanbox_h(CPURISCVState *env, uint64_t f)
     }
 }
 
+/* Our implementation of CPUClass::has_work */
+bool riscv_cpu_has_work(CPUState *cs);
+
 #endif
diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
index 0e2fbcb397f..9db1dffc03e 100644
--- a/target/alpha/cpu.c
+++ b/target/alpha/cpu.c
@@ -219,6 +219,7 @@ static const TCGCPUOps alpha_tcg_ops = {
 #else
     .tlb_fill = alpha_cpu_tlb_fill,
     .cpu_exec_interrupt = alpha_cpu_exec_interrupt,
+    .cpu_exec_halt = alpha_cpu_has_work,
     .do_interrupt = alpha_cpu_do_interrupt,
     .do_transaction_failed = alpha_cpu_do_transaction_failed,
     .do_unaligned_access = alpha_cpu_do_unaligned_access,
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index f53e1192b15..3132842d565 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -210,6 +210,7 @@ static const TCGCPUOps avr_tcg_ops = {
     .synchronize_from_tb = avr_cpu_synchronize_from_tb,
     .restore_state_to_opc = avr_restore_state_to_opc,
     .cpu_exec_interrupt = avr_cpu_exec_interrupt,
+    .cpu_exec_halt = avr_cpu_has_work,
     .tlb_fill = avr_cpu_tlb_fill,
     .do_interrupt = avr_cpu_do_interrupt,
 };
diff --git a/target/cris/cpu.c b/target/cris/cpu.c
index 535ec39c730..ff31ca7fbc1 100644
--- a/target/cris/cpu.c
+++ b/target/cris/cpu.c
@@ -186,6 +186,7 @@ static const TCGCPUOps crisv10_tcg_ops = {
 #ifndef CONFIG_USER_ONLY
     .tlb_fill = cris_cpu_tlb_fill,
     .cpu_exec_interrupt = cris_cpu_exec_interrupt,
+    .cpu_exec_halt = cris_cpu_has_work,
     .do_interrupt = crisv10_cpu_do_interrupt,
 #endif /* !CONFIG_USER_ONLY */
 };
@@ -197,6 +198,7 @@ static const TCGCPUOps crisv32_tcg_ops = {
 #ifndef CONFIG_USER_ONLY
     .tlb_fill = cris_cpu_tlb_fill,
     .cpu_exec_interrupt = cris_cpu_exec_interrupt,
+    .cpu_exec_halt = cris_cpu_has_work,
     .do_interrupt = cris_cpu_do_interrupt,
 #endif /* !CONFIG_USER_ONLY */
 };
diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
index f0507874ce6..7cf2e2f266d 100644
--- a/target/hppa/cpu.c
+++ b/target/hppa/cpu.c
@@ -228,6 +228,7 @@ static const TCGCPUOps hppa_tcg_ops = {
 #ifndef CONFIG_USER_ONLY
     .tlb_fill = hppa_cpu_tlb_fill,
     .cpu_exec_interrupt = hppa_cpu_exec_interrupt,
+    .cpu_exec_halt = hppa_cpu_has_work,
     .do_interrupt = hppa_cpu_do_interrupt,
     .do_unaligned_access = hppa_cpu_do_unaligned_access,
     .do_transaction_failed = hppa_cpu_do_transaction_failed,
diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index b5c1ec94af5..70b41807c8d 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -736,6 +736,7 @@ static const TCGCPUOps loongarch_tcg_ops = {
 #ifndef CONFIG_USER_ONLY
     .tlb_fill = loongarch_cpu_tlb_fill,
     .cpu_exec_interrupt = loongarch_cpu_exec_interrupt,
+    .cpu_exec_halt = loongarch_cpu_has_work,
     .do_interrupt = loongarch_cpu_do_interrupt,
     .do_transaction_failed = loongarch_cpu_do_transaction_failed,
 #endif
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index efd6bbded86..1d49f4cb238 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -536,6 +536,7 @@ static const TCGCPUOps m68k_tcg_ops = {
 #ifndef CONFIG_USER_ONLY
     .tlb_fill = m68k_cpu_tlb_fill,
     .cpu_exec_interrupt = m68k_cpu_exec_interrupt,
+    .cpu_exec_halt = m68k_cpu_has_work,
     .do_interrupt = m68k_cpu_do_interrupt,
     .do_transaction_failed = m68k_cpu_transaction_failed,
 #endif /* !CONFIG_USER_ONLY */
diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
index 41ad47d04cb..135947ee800 100644
--- a/target/microblaze/cpu.c
+++ b/target/microblaze/cpu.c
@@ -413,6 +413,7 @@ static const TCGCPUOps mb_tcg_ops = {
 #ifndef CONFIG_USER_ONLY
     .tlb_fill = mb_cpu_tlb_fill,
     .cpu_exec_interrupt = mb_cpu_exec_interrupt,
+    .cpu_exec_halt = mb_cpu_has_work,
     .do_interrupt = mb_cpu_do_interrupt,
     .do_transaction_failed = mb_cpu_transaction_failed,
     .do_unaligned_access = mb_cpu_do_unaligned_access,
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index bbe01d07dd8..89655b1900f 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -555,6 +555,7 @@ static const TCGCPUOps mips_tcg_ops = {
 #if !defined(CONFIG_USER_ONLY)
     .tlb_fill = mips_cpu_tlb_fill,
     .cpu_exec_interrupt = mips_cpu_exec_interrupt,
+    .cpu_exec_halt = mips_cpu_has_work,
     .do_interrupt = mips_cpu_do_interrupt,
     .do_transaction_failed = mips_cpu_do_transaction_failed,
     .do_unaligned_access = mips_cpu_do_unaligned_access,
diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
index fdaaa09fc87..6ec54ad7a6c 100644
--- a/target/openrisc/cpu.c
+++ b/target/openrisc/cpu.c
@@ -233,6 +233,7 @@ static const TCGCPUOps openrisc_tcg_ops = {
 #ifndef CONFIG_USER_ONLY
     .tlb_fill = openrisc_cpu_tlb_fill,
     .cpu_exec_interrupt = openrisc_cpu_exec_interrupt,
+    .cpu_exec_halt = openrisc_cpu_has_work,
     .do_interrupt = openrisc_cpu_do_interrupt,
 #endif /* !CONFIG_USER_ONLY */
 };
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 01e358a4a5a..cdada7987d8 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -1,3 +1,4 @@
+
 /*
  *  PowerPC CPU initialization for qemu.
  *
@@ -7481,6 +7482,7 @@ static const TCGCPUOps ppc_tcg_ops = {
 #else
   .tlb_fill = ppc_cpu_tlb_fill,
   .cpu_exec_interrupt = ppc_cpu_exec_interrupt,
+  .cpu_exec_halt = ppc_cpu_has_work,
   .do_interrupt = ppc_cpu_do_interrupt,
   .cpu_exec_enter = ppc_cpu_exec_enter,
   .cpu_exec_exit = ppc_cpu_exec_exit,
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index eb1a2e7d6d9..54e12eed466 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -896,7 +896,7 @@ static vaddr riscv_cpu_get_pc(CPUState *cs)
     return env->pc;
 }
 
-static bool riscv_cpu_has_work(CPUState *cs)
+bool riscv_cpu_has_work(CPUState *cs)
 {
 #ifndef CONFIG_USER_ONLY
     RISCVCPU *cpu = RISCV_CPU(cs);
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 40054a391a6..643f9a224b8 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -21,6 +21,7 @@
 #include "exec/exec-all.h"
 #include "tcg-cpu.h"
 #include "cpu.h"
+#include "internals.h"
 #include "pmu.h"
 #include "time_helper.h"
 #include "qapi/error.h"
@@ -137,6 +138,7 @@ static const TCGCPUOps riscv_tcg_ops = {
 #ifndef CONFIG_USER_ONLY
     .tlb_fill = riscv_cpu_tlb_fill,
     .cpu_exec_interrupt = riscv_cpu_exec_interrupt,
+    .cpu_exec_halt = riscv_cpu_has_work,
     .do_interrupt = riscv_cpu_do_interrupt,
     .do_transaction_failed = riscv_cpu_do_transaction_failed,
     .do_unaligned_access = riscv_cpu_do_unaligned_access,
diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index 8a584f0a111..36d2a6f1890 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -192,6 +192,7 @@ static const TCGCPUOps rx_tcg_ops = {
 
 #ifndef CONFIG_USER_ONLY
     .cpu_exec_interrupt = rx_cpu_exec_interrupt,
+    .cpu_exec_halt = rx_cpu_has_work,
     .do_interrupt = rx_cpu_do_interrupt,
 #endif /* !CONFIG_USER_ONLY */
 };
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 2bbeaca36e4..0fbfcd35d83 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -370,6 +370,7 @@ static const TCGCPUOps s390_tcg_ops = {
 #else
     .tlb_fill = s390_cpu_tlb_fill,
     .cpu_exec_interrupt = s390_cpu_exec_interrupt,
+    .cpu_exec_halt = s390_cpu_has_work,
     .do_interrupt = s390_cpu_do_interrupt,
     .debug_excp_handler = s390x_cpu_debug_excp_handler,
     .do_unaligned_access = s390x_cpu_do_unaligned_access,
diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
index 618aa7154ed..8f07261dcfd 100644
--- a/target/sh4/cpu.c
+++ b/target/sh4/cpu.c
@@ -254,6 +254,7 @@ static const TCGCPUOps superh_tcg_ops = {
 #ifndef CONFIG_USER_ONLY
     .tlb_fill = superh_cpu_tlb_fill,
     .cpu_exec_interrupt = superh_cpu_exec_interrupt,
+    .cpu_exec_halt = superh_cpu_has_work,
     .do_interrupt = superh_cpu_do_interrupt,
     .do_unaligned_access = superh_cpu_do_unaligned_access,
     .io_recompile_replay_branch = superh_io_recompile_replay_branch,
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 5be1592e66a..87f0b4a6c05 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -914,6 +914,7 @@ static const TCGCPUOps sparc_tcg_ops = {
 #ifndef CONFIG_USER_ONLY
     .tlb_fill = sparc_cpu_tlb_fill,
     .cpu_exec_interrupt = sparc_cpu_exec_interrupt,
+    .cpu_exec_halt = sparc_cpu_has_work,
     .do_interrupt = sparc_cpu_do_interrupt,
     .do_transaction_failed = sparc_cpu_do_transaction_failed,
     .do_unaligned_access = sparc_cpu_do_unaligned_access,
diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index de907cfeb1b..a08c7a0b1f2 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -234,6 +234,7 @@ static const TCGCPUOps xtensa_tcg_ops = {
 #ifndef CONFIG_USER_ONLY
     .tlb_fill = xtensa_cpu_tlb_fill,
     .cpu_exec_interrupt = xtensa_cpu_exec_interrupt,
+    .cpu_exec_halt = xtensa_cpu_has_work,
     .do_interrupt = xtensa_cpu_do_interrupt,
     .do_transaction_failed = xtensa_cpu_do_transaction_failed,
     .do_unaligned_access = xtensa_cpu_do_unaligned_access,
-- 
2.34.1

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

[Peter Maydell]

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 0/3] cpu_exec_halt: make method mandatory

[Richard Henderson]

On 6/3/24 11:09, Peter Maydell wrote:
> Peter Maydell (3):
>    target/arm: Set arm_v7m_tcg_ops cpu_exec_halt to arm_cpu_exec_halt()
>    target: Set TCGCPUOps::cpu_exec_halt to target's has_work
>      implementation
>    accel/tcg: Make TCGCPUOps::cpu_exec_halt mandatory

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/3] target: Set TCGCPUOps::cpu_exec_halt to target's has_work implementation

[Philippe Mathieu-Daudé]

On 3/6/24 18:09, Peter Maydell wrote:
> Currently the TCGCPUOps::cpu_exec_halt method is optional, and if it
> is not set then the default is to call the CPUClass::has_work
> method (which has an identical function signature).
> 
> We would like to make the cpu_exec_halt method mandatory so we can
> remove the runtime check and fallback handling.  In preparation for
> that, make all the targets which don't need special handling in their
> cpu_exec_halt set it to their cpu_has_work implementation instead of
> leaving it unset.  (This is every target except for arm and i386.)
> 
> In the riscv case this requires us to make the function not
> be local to the source file it's defined in.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/riscv/internals.h   | 3 +++
>   target/alpha/cpu.c         | 1 +
>   target/avr/cpu.c           | 1 +
>   target/cris/cpu.c          | 2 ++
>   target/hppa/cpu.c          | 1 +
>   target/loongarch/cpu.c     | 1 +
>   target/m68k/cpu.c          | 1 +
>   target/microblaze/cpu.c    | 1 +
>   target/mips/cpu.c          | 1 +
>   target/openrisc/cpu.c      | 1 +
>   target/ppc/cpu_init.c      | 2 ++
>   target/riscv/cpu.c         | 2 +-
>   target/riscv/tcg/tcg-cpu.c | 2 ++
>   target/rx/cpu.c            | 1 +
>   target/s390x/cpu.c         | 1 +
>   target/sh4/cpu.c           | 1 +
>   target/sparc/cpu.c         | 1 +
>   target/xtensa/cpu.c        | 1 +
>   18 files changed, 23 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Philippe Mathieu-Daudé]

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 1/3] target/arm: Set arm_v7m_tcg_ops cpu_exec_halt to arm_cpu_exec_halt()

[Philippe Mathieu-Daudé]

On 3/6/24 18:09, Peter Maydell wrote:
> In commit a96edb687e76 we set the cpu_exec_halt field of the
> TCGCPUOps arm_tcg_ops to arm_cpu_exec_halt(), but we left the
> arm_v7m_tcg_ops struct unchanged.  That isn't wrong, because for
> M-profile FEAT_WFxT doesn't exist and the default handling for "no
> cpu_exec_halt method" is correct, but it's perhaps a little
> confusing.  We would also like to make setting the cpu_exec_halt
> method mandatory.
> 
> Initialize arm_v7m_tcg_ops cpu_exec_halt to the same function we use
> for A-profile.  (On M-profile we never set up the wfxt timer so there
> is no change in behaviour here.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/internals.h   | 3 +++
>   target/arm/cpu.c         | 2 +-
>   target/arm/tcg/cpu-v7m.c | 1 +
>   3 files changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Peter Maydell]

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

[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