On 3/21/25 11:26, Philippe Mathieu-Daudé wrote:
On 21/3/25 18:36, Pierrick Bouvier wrote:
On 3/21/25 08:59, Philippe Mathieu-Daudé wrote:
Use the OnOffAuto type as 3-state.

Since the TCGState instance is zero-initialized, the
mttcg_enabled is initialzed as AUTO (ON_OFF_AUTO_AUTO).

In tcg_init_machine(), if mttcg_enabled is still AUTO,
set a default value (effectively inlining the
default_mttcg_enabled() method content).

Instead of emiting a warning when the 'thread' property
is set in tcg_set_thread(), emit it in tcg_init_machine()
where it is consumed.

In the tcg_get_thread() getter, consider AUTO / OFF states
as "single", otherwise ON is "multi".

Signed-off-by: Philippe Mathieu-Daudé <phi...@linaro.org>
---
   accel/tcg/tcg-all.c | 68 ++++++++++++++++++++++-----------------------
   1 file changed, 33 insertions(+), 35 deletions(-)

diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
index d75ecf531b6..2b7f89eaa20 100644
--- a/accel/tcg/tcg-all.c
+++ b/accel/tcg/tcg-all.c
@@ -32,6 +32,7 @@
   #include "qemu/error-report.h"
   #include "qemu/accel.h"
   #include "qemu/atomic.h"
+#include "qapi/qapi-types-common.h"
   #include "qapi/qapi-builtin-visit.h"
   #include "qemu/units.h"
   #if defined(CONFIG_USER_ONLY)
@@ -47,7 +48,7 @@
   struct TCGState {
       AccelState parent_obj;
-    bool mttcg_enabled;
+    OnOffAuto mttcg_enabled;
       bool one_insn_per_tb;
       int splitwx_enabled;
       unsigned long tb_size;
@@ -68,37 +69,10 @@ bool qemu_tcg_mttcg_enabled(void)
   }
   #endif
-/*
- * We default to false if we know other options have been enabled
- * which are currently incompatible with MTTCG. Otherwise when each
- * guest (target) has been updated to support:
- *   - atomic instructions
- *   - memory ordering primitives (barriers)
- * they can set the appropriate CONFIG flags in ${target}-softmmu.mak
- *
- * Once a guest architecture has been converted to the new primitives
- * there is one remaining limitation to check:
- *   - The guest can't be oversized (e.g. 64 bit guest on 32 bit host)
- */
-
-static bool default_mttcg_enabled(void)
-{
-    if (icount_enabled()) {
-        return false;
-    }
-#ifdef TARGET_SUPPORTS_MTTCG
-    return true;
-#else
-    return false;
-#endif
-}
-
   static void tcg_accel_instance_init(Object *obj)
   {
       TCGState *s = TCG_STATE(obj);
-    s->mttcg_enabled = default_mttcg_enabled();
-
       /* If debugging enabled, default "auto on", otherwise off. */
   #if defined(CONFIG_DEBUG_TCG) && !defined(CONFIG_USER_ONLY)
       s->splitwx_enabled = -1;
@@ -117,9 +91,37 @@ static int tcg_init_machine(MachineState *ms)
   #else
       unsigned max_cpus = ms->smp.max_cpus;
   #endif
+#ifdef TARGET_SUPPORTS_MTTCG
+    bool mttcg_supported = true;
+#else
+    bool mttcg_supported = false;
+#endif
       tcg_allowed = true;
       mttcg_enabled = s->mttcg_enabled;
+    if (mttcg_enabled == ON_OFF_AUTO_AUTO) {
+        /*
+         * We default to false if we know other options have been
enabled
+         * which are currently incompatible with MTTCG. Otherwise
when each
+         * guest (target) has been updated to support:
+         *   - atomic instructions
+         *   - memory ordering primitives (barriers)
+         * they can set the appropriate CONFIG flags in ${target}-
softmmu.mak
+         *
+         * Once a guest architecture has been converted to the new
primitives
+         * there is one remaining limitation to check:
+         *   - The guest can't be oversized (e.g. 64 bit guest on 32
bit host)
+         */
+        if (icount_enabled()) {
+            mttcg_enabled = ON_OFF_AUTO_OFF;
+        } else {
+            mttcg_enabled = mttcg_supported;
+        }
+    }
+    if (mttcg_enabled == ON_OFF_AUTO_ON && !mttcg_supported) {
+        warn_report("Guest not yet converted to MTTCG - "
+                    "you may get unexpected results");
+    }
       page_init();
       tb_htable_init();
@@ -144,7 +146,7 @@ static char *tcg_get_thread(Object *obj, Error
**errp)
   {
       TCGState *s = TCG_STATE(obj);
-    return g_strdup(s->mttcg_enabled ? "multi" : "single");
+    return g_strdup(s->mttcg_enabled == ON_OFF_AUTO_ON ? "multi" :
"single");
   }
   static void tcg_set_thread(Object *obj, const char *value, Error
**errp)
@@ -155,14 +157,10 @@ static void tcg_set_thread(Object *obj, const
char *value, Error **errp)
           if (icount_enabled()) {
               error_setg(errp, "No MTTCG when icount is enabled");
           } else {
-#ifndef TARGET_SUPPORTS_MTTCG
-            warn_report("Guest not yet converted to MTTCG - "
-                        "you may get unexpected results");
-#endif
-            s->mttcg_enabled = true;
+            s->mttcg_enabled = ON_OFF_AUTO_ON;
           }
       } else if (strcmp(value, "single") == 0) {
-        s->mttcg_enabled = false;
+        s->mttcg_enabled = ON_OFF_AUTO_OFF;
       } else {
           error_setg(errp, "Invalid 'thread' setting %s", value);
       }

What are we gaining by moving this warning?

Before: default_mttcg_enabled() was directly using TARGET_SUPPORTS_MTTCG
definition, so we could call it to initialize a default value in
tcg_accel_instance_init().

After: considering the next patch where we need a CPUState initialized
to get TCGCPUOps::mttcg_supported, we can not use instance_init(), so
we defer to tcg_init_machine() where we access &first_cpu.


Ok, makes sense.
It might be worth to mention in the commit message, as it seems initially this change is more a "personal" preference, than something needed.

Are there cases where it was not reported before?

No idea, I'll let Alex answer that.

Reply via email to