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;
> @@ -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");
> + }
[...]
>
> 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.
Otherwise:
Reviewed-by: Anton Johansson <a...@rev.ng>
