Re: [PATCH-for-10.1 3/4] tcg: Convert TCGState::mttcg_enabled to TriState

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

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é <philmd@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?
Are there cases where it was not reported before?