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

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

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é <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?
> 
> 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.