On 21/3/25 18:01, Anton Johansson wrote:
On 21/03/25, 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;
-/*
- * 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");
+ }
[...]
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
Patch itself looks good! My only concern is moving the warning, would
it be worthwhile to have a mttcg_supported field in TCGState as well?
I.e in a heterogeneous setup it would correspond to wether or not all
targets support mttcg.
OK, I'll update s->mttcg_enabled then.
Otherwise:
Reviewed-by: Anton Johansson <a...@rev.ng>
Thanks!
