| Message ID | 20250321155925.96626-4-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tcg: Convert TARGET_SUPPORTS_MTTCG to TCGCPUOps::mttcg_supported field | expand |
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é <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"); > + } [...] > > 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 <anjo@rev.ng>
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?
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é <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; >> -/* >> - * 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 <anjo@rev.ng> Thanks!
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. > Are there cases where it was not reported before? No idea, I'll let Alex answer that.
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.
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); }
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(-)