| Message ID | 1459870344-16773-9-git-send-email-alex.bennee@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/04/16 18:32, Alex Bennée wrote: > diff --git a/qemu-options.hx b/qemu-options.hx > index a770086..4eca704 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -3224,6 +3224,20 @@ Attach to existing xen domain. > xend will use this when starting QEMU (XEN only). > ETEXI > > +DEF("tcg", HAS_ARG, QEMU_OPTION_tcg, \ > + "-tcg [mttcg=on|off] control TCG options\n", QEMU_ARCH_ALL) > +STEXI > +@item -tcg > +@findex -tcg > +@table @option > +@item mttcg=[on|off] > +Control multi-threaded TCG. By default QEMU will enable multi-threaded > +emulation for front/back-end combinations that are known to work. The > +user may enable it against the defaults however odd guest behaviour > +may occur. > +@end table > +ETEXI Maybe we'd better use existing qemu accelerator framework and introduce "-machine accel=mttcg" option? Kind regards, Sergey
Sergey Fedorov <serge.fdrv@gmail.com> writes: > On 05/04/16 18:32, Alex Bennée wrote: >> diff --git a/qemu-options.hx b/qemu-options.hx >> index a770086..4eca704 100644 >> --- a/qemu-options.hx >> +++ b/qemu-options.hx >> @@ -3224,6 +3224,20 @@ Attach to existing xen domain. >> xend will use this when starting QEMU (XEN only). >> ETEXI >> >> +DEF("tcg", HAS_ARG, QEMU_OPTION_tcg, \ >> + "-tcg [mttcg=on|off] control TCG options\n", QEMU_ARCH_ALL) >> +STEXI >> +@item -tcg >> +@findex -tcg >> +@table @option >> +@item mttcg=[on|off] >> +Control multi-threaded TCG. By default QEMU will enable multi-threaded >> +emulation for front/back-end combinations that are known to work. The >> +user may enable it against the defaults however odd guest behaviour >> +may occur. >> +@end table >> +ETEXI > > Maybe we'd better use existing qemu accelerator framework and introduce > "-machine accel=mttcg" option? My worry would be breaking existing code which assumes kvm | tcg. We will want to enable mttcg by default for combos that work without having to tweak tooling that already uses the -machine options. -- Alex Bennée
On 12 April 2016 at 12:48, Alex Bennée <alex.bennee@linaro.org> wrote: > Sergey Fedorov <serge.fdrv@gmail.com> writes: >> Maybe we'd better use existing qemu accelerator framework and introduce >> "-machine accel=mttcg" option? > > My worry would be breaking existing code which assumes kvm | tcg. We > will want to enable mttcg by default for combos that work without having > to tweak tooling that already uses the -machine options. Comments from the peanut gallery: * We definitely want to just default to MTTCG where it works: users shouldn't have to add new command line switches to get the better performance etc, because in practice that won't happen * Anything that adds a new command line option at all has to be supported practically forever, so options that are only useful for a transitional period are worth thinking twice about thanks -- PMM
On 12/04/16 14:59, Peter Maydell wrote: > On 12 April 2016 at 12:48, Alex Bennée <alex.bennee@linaro.org> wrote: >> Sergey Fedorov <serge.fdrv@gmail.com> writes: >>> Maybe we'd better use existing qemu accelerator framework and introduce >>> "-machine accel=mttcg" option? >> My worry would be breaking existing code which assumes kvm | tcg. We >> will want to enable mttcg by default for combos that work without having >> to tweak tooling that already uses the -machine options. > Comments from the peanut gallery: > * We definitely want to just default to MTTCG where it works: > users shouldn't have to add new command line switches to get the > better performance etc, because in practice that won't happen > * Anything that adds a new command line option at all has to > be supported practically forever, so options that are only > useful for a transitional period are worth thinking twice about Yes, but users may like to have an option to disable MTTCG for some reason. I'm also concerned about icount mode: not sure how to account virtual time when all vCPUs run in parallel. Kind regards, Sergey
On 12/04/16 14:48, Alex Bennée wrote: > Sergey Fedorov <serge.fdrv@gmail.com> writes: > >> On 05/04/16 18:32, Alex Bennée wrote: >>> diff --git a/qemu-options.hx b/qemu-options.hx >>> index a770086..4eca704 100644 >>> --- a/qemu-options.hx >>> +++ b/qemu-options.hx >>> @@ -3224,6 +3224,20 @@ Attach to existing xen domain. >>> xend will use this when starting QEMU (XEN only). >>> ETEXI >>> >>> +DEF("tcg", HAS_ARG, QEMU_OPTION_tcg, \ >>> + "-tcg [mttcg=on|off] control TCG options\n", QEMU_ARCH_ALL) >>> +STEXI >>> +@item -tcg >>> +@findex -tcg >>> +@table @option >>> +@item mttcg=[on|off] >>> +Control multi-threaded TCG. By default QEMU will enable multi-threaded >>> +emulation for front/back-end combinations that are known to work. The >>> +user may enable it against the defaults however odd guest behaviour >>> +may occur. >>> +@end table >>> +ETEXI >> Maybe we'd better use existing qemu accelerator framework and introduce >> "-machine accel=mttcg" option? > My worry would be breaking existing code which assumes kvm | tcg. We > will want to enable mttcg by default for combos that work without having > to tweak tooling that already uses the -machine options. You are right, we'd better make MTTCG as an option for TCG, not as a separate accelerator. Kind regards, Sergey
Le 12/04/2016 14:42, Sergey Fedorov a écrit : > On 12/04/16 14:59, Peter Maydell wrote: >> On 12 April 2016 at 12:48, Alex Bennée <alex.bennee@linaro.org> wrote: >>> Sergey Fedorov <serge.fdrv@gmail.com> writes: >>>> Maybe we'd better use existing qemu accelerator framework and introduce >>>> "-machine accel=mttcg" option? >>> My worry would be breaking existing code which assumes kvm | tcg. We >>> will want to enable mttcg by default for combos that work without having >>> to tweak tooling that already uses the -machine options. >> Comments from the peanut gallery: >> * We definitely want to just default to MTTCG where it works: >> users shouldn't have to add new command line switches to get the >> better performance etc, because in practice that won't happen >> * Anything that adds a new command line option at all has to >> be supported practically forever, so options that are only >> useful for a transitional period are worth thinking twice about > > Yes, but users may like to have an option to disable MTTCG for some > reason. I'm also concerned about icount mode: not sure how to account > virtual time when all vCPUs run in parallel. > Hi, I'm thinking the same, we don't have a solution for icount yet. The reverse execution support is probably badly broken as well. Fred > Kind regards, > Sergey >
On 12/04/16 15:50, KONRAD Frederic wrote: > > > Le 12/04/2016 14:42, Sergey Fedorov a écrit : >> On 12/04/16 14:59, Peter Maydell wrote: >>> On 12 April 2016 at 12:48, Alex Bennée <alex.bennee@linaro.org> wrote: >>>> Sergey Fedorov <serge.fdrv@gmail.com> writes: >>>>> Maybe we'd better use existing qemu accelerator framework and >>>>> introduce >>>>> "-machine accel=mttcg" option? >>>> My worry would be breaking existing code which assumes kvm | tcg. We >>>> will want to enable mttcg by default for combos that work without >>>> having >>>> to tweak tooling that already uses the -machine options. >>> Comments from the peanut gallery: >>> * We definitely want to just default to MTTCG where it works: >>> users shouldn't have to add new command line switches to get the >>> better performance etc, because in practice that won't happen >>> * Anything that adds a new command line option at all has to >>> be supported practically forever, so options that are only >>> useful for a transitional period are worth thinking twice about >> >> Yes, but users may like to have an option to disable MTTCG for some >> reason. I'm also concerned about icount mode: not sure how to account >> virtual time when all vCPUs run in parallel. >> > > > I'm thinking the same, we don't have a solution for icount yet. > The reverse execution support is probably badly broken as well. Reverse execution doesn't even seem to support more than a single core. As of icount, it looks like to be incompatible with MTTCG. Kind regards, Sergey
> From: mttcg-request@listserver.greensocs.com [mailto:mttcg-request@listserver.greensocs.com] > On 12/04/16 15:50, KONRAD Frederic wrote: > >> Yes, but users may like to have an option to disable MTTCG for some > >> reason. I'm also concerned about icount mode: not sure how to account > >> virtual time when all vCPUs run in parallel. > > > > I'm thinking the same, we don't have a solution for icount yet. > > The reverse execution support is probably badly broken as well. > > Reverse execution doesn't even seem to support more than a single core. > As of icount, it looks like to be incompatible with MTTCG. It doesn't, because there is no multicore support for tcg yet. I think, that we have to find some solution for icount (even if it will be slow). Pavel Dovgalyuk
On 12/04/16 16:03, Pavel Dovgalyuk wrote: >> From: mttcg-request@listserver.greensocs.com [mailto:mttcg-request@listserver.greensocs.com] >> On 12/04/16 15:50, KONRAD Frederic wrote: >>>> Yes, but users may like to have an option to disable MTTCG for some >>>> reason. I'm also concerned about icount mode: not sure how to account >>>> virtual time when all vCPUs run in parallel. >>> I'm thinking the same, we don't have a solution for icount yet. >>> The reverse execution support is probably badly broken as well. >> Reverse execution doesn't even seem to support more than a single core. >> As of icount, it looks like to be incompatible with MTTCG. > It doesn't, because there is no multicore support for tcg yet. > I think, that we have to find some solution for icount (even if it will be slow). Synchronize all vCPUs against their instruction counters? I wonder would it be of much use in MTTCG mode if it turns out to be quite slow? Anyhow, valid point, we should think about it. Kind regards, Sergey
On 05/04/16 18:32, Alex Bennée wrote: > diff --git a/cpus.c b/cpus.c > index 46732a5..8d27fb0 100644 > --- a/cpus.c > +++ b/cpus.c (snip) > @@ -146,6 +147,48 @@ typedef struct TimersState { > } TimersState; > > static TimersState timers_state; > +static bool mttcg_enabled; > + > +static QemuOptsList qemu_tcg_opts = { > + .name = "tcg", > + .head = QTAILQ_HEAD_INITIALIZER(qemu_tcg_opts.head), > + .desc = { > + { > + .name = "mttcg", > + .type = QEMU_OPT_BOOL, > + .help = "Enable/disable multi-threaded TCG", > + }, > + { /* end of list */ } > + }, > +}; > + > +static void tcg_register_config(void) > +{ > + qemu_add_opts(&qemu_tcg_opts); > +} > + > +opts_init(tcg_register_config); > + > +static bool default_mttcg_enabled(void) > +{ > + /* > + * TODO: Check if we have a chance to have MTTCG working on this guest/host. > + * Basically is the atomic instruction implemented? Is there any > + * memory ordering issue? > + */ I think this could be decided in configure/makefiles. > + return false; > +} > + > +void qemu_tcg_configure(QemuOpts *opts) > +{ > + mttcg_enabled = qemu_opt_get_bool(opts, "mttcg", default_mttcg_enabled()); > +} > + > +bool qemu_tcg_mttcg_enabled(void) > +{ > + return mttcg_enabled; > +} > + > > int64_t cpu_get_icount_raw(void) > { > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 13eeaae..5e3826c 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -369,6 +369,20 @@ extern struct CPUTailQ cpus; > extern __thread CPUState *current_cpu; > > /** > + * qemu_tcg_enable_mttcg: > + * Enable the MultiThread TCG support. > + */ > +void qemu_tcg_enable_mttcg(void); Seems to be an orphaned declaration. Kind regards, Sergey
Pavel Dovgalyuk <dovgaluk@ispras.ru> writes: >> From: mttcg-request@listserver.greensocs.com [mailto:mttcg-request@listserver.greensocs.com] >> On 12/04/16 15:50, KONRAD Frederic wrote: >> >> Yes, but users may like to have an option to disable MTTCG for some >> >> reason. I'm also concerned about icount mode: not sure how to account >> >> virtual time when all vCPUs run in parallel. >> > >> > I'm thinking the same, we don't have a solution for icount yet. >> > The reverse execution support is probably badly broken as well. >> >> Reverse execution doesn't even seem to support more than a single core. >> As of icount, it looks like to be incompatible with MTTCG. > > It doesn't, because there is no multicore support for tcg yet. > I think, that we have to find some solution for icount (even if it > will be slow). Well one of the reasons I'm keeping single thread behaviour is so things like icount/replay support can continue to work. The two options will be incompatible to start with. I'm also expecting people will want to rule out mttcg as a problem at least in the early days. -- Alex Bennée
Sergey Fedorov <serge.fdrv@gmail.com> writes: > On 05/04/16 18:32, Alex Bennée wrote: >> diff --git a/cpus.c b/cpus.c >> index 46732a5..8d27fb0 100644 >> --- a/cpus.c >> +++ b/cpus.c > > (snip) > >> @@ -146,6 +147,48 @@ typedef struct TimersState { >> } TimersState; >> >> static TimersState timers_state; >> +static bool mttcg_enabled; >> + >> +static QemuOptsList qemu_tcg_opts = { >> + .name = "tcg", >> + .head = QTAILQ_HEAD_INITIALIZER(qemu_tcg_opts.head), >> + .desc = { >> + { >> + .name = "mttcg", >> + .type = QEMU_OPT_BOOL, >> + .help = "Enable/disable multi-threaded TCG", >> + }, >> + { /* end of list */ } >> + }, >> +}; >> + >> +static void tcg_register_config(void) >> +{ >> + qemu_add_opts(&qemu_tcg_opts); >> +} >> + >> +opts_init(tcg_register_config); >> + >> +static bool default_mttcg_enabled(void) >> +{ >> + /* >> + * TODO: Check if we have a chance to have MTTCG working on this guest/host. >> + * Basically is the atomic instruction implemented? Is there any >> + * memory ordering issue? >> + */ > > I think this could be decided in configure/makefiles. I was think we might have other interactions, like if the user enabled replay/playback mode. There is also an argument that by having the logic in the code it's easier for developers to see the logic as people don't generally grok Makefiles. > >> + return false; >> +} >> + >> +void qemu_tcg_configure(QemuOpts *opts) >> +{ >> + mttcg_enabled = qemu_opt_get_bool(opts, "mttcg", default_mttcg_enabled()); >> +} >> + >> +bool qemu_tcg_mttcg_enabled(void) >> +{ >> + return mttcg_enabled; >> +} >> + >> >> int64_t cpu_get_icount_raw(void) >> { >> diff --git a/include/qom/cpu.h b/include/qom/cpu.h >> index 13eeaae..5e3826c 100644 >> --- a/include/qom/cpu.h >> +++ b/include/qom/cpu.h >> @@ -369,6 +369,20 @@ extern struct CPUTailQ cpus; >> extern __thread CPUState *current_cpu; >> >> /** >> + * qemu_tcg_enable_mttcg: >> + * Enable the MultiThread TCG support. >> + */ >> +void qemu_tcg_enable_mttcg(void); > > Seems to be an orphaned declaration. > > Kind regards, > Sergey -- Alex Bennée
On 11/04/2016 22:50, Sergey Fedorov wrote: >> > +Control multi-threaded TCG. By default QEMU will enable multi-threaded >> > +emulation for front/back-end combinations that are known to work. The >> > +user may enable it against the defaults however odd guest behaviour >> > +may occur. >> > +@end table >> > +ETEXI > Maybe we'd better use existing qemu accelerator framework and introduce > "-machine accel=mttcg" option? Another possibility is "-accel tcg,thread=single|multi". Paolo
On 12/04/2016 16:23, Alex Bennée wrote: > Well one of the reasons I'm keeping single thread behaviour is so things > like icount/replay support can continue to work. The two options will be > incompatible to start with. As long as the single-threaded TCG gets rid of iothread's special behavior of kicking VCPUs, I don't think it's going to be painful to keep it around. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 11/04/2016 22:50, Sergey Fedorov wrote: >>> > +Control multi-threaded TCG. By default QEMU will enable multi-threaded >>> > +emulation for front/back-end combinations that are known to work. The >>> > +user may enable it against the defaults however odd guest behaviour >>> > +may occur. >>> > +@end table >>> > +ETEXI >> Maybe we'd better use existing qemu accelerator framework and introduce >> "-machine accel=mttcg" option? > > Another possibility is "-accel tcg,thread=single|multi". That does seem a better choice. > > Paolo -- Alex Bennée
diff --git a/cpus.c b/cpus.c index 46732a5..8d27fb0 100644 --- a/cpus.c +++ b/cpus.c @@ -25,6 +25,7 @@ /* Needed early for CONFIG_BSD etc. */ #include "qemu/osdep.h" +#include "qemu/config-file.h" #include "monitor/monitor.h" #include "qapi/qmp/qerror.h" #include "qemu/error-report.h" @@ -146,6 +147,48 @@ typedef struct TimersState { } TimersState; static TimersState timers_state; +static bool mttcg_enabled; + +static QemuOptsList qemu_tcg_opts = { + .name = "tcg", + .head = QTAILQ_HEAD_INITIALIZER(qemu_tcg_opts.head), + .desc = { + { + .name = "mttcg", + .type = QEMU_OPT_BOOL, + .help = "Enable/disable multi-threaded TCG", + }, + { /* end of list */ } + }, +}; + +static void tcg_register_config(void) +{ + qemu_add_opts(&qemu_tcg_opts); +} + +opts_init(tcg_register_config); + +static bool default_mttcg_enabled(void) +{ + /* + * TODO: Check if we have a chance to have MTTCG working on this guest/host. + * Basically is the atomic instruction implemented? Is there any + * memory ordering issue? + */ + return false; +} + +void qemu_tcg_configure(QemuOpts *opts) +{ + mttcg_enabled = qemu_opt_get_bool(opts, "mttcg", default_mttcg_enabled()); +} + +bool qemu_tcg_mttcg_enabled(void) +{ + return mttcg_enabled; +} + int64_t cpu_get_icount_raw(void) { diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 13eeaae..5e3826c 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -369,6 +369,20 @@ extern struct CPUTailQ cpus; extern __thread CPUState *current_cpu; /** + * qemu_tcg_enable_mttcg: + * Enable the MultiThread TCG support. + */ +void qemu_tcg_enable_mttcg(void); + +/** + * qemu_tcg_mttcg_enabled: + * Check whether we are running MultiThread TCG or not. + * + * Returns: %true if we are in MTTCG mode %false otherwise. + */ +bool qemu_tcg_mttcg_enabled(void); + +/** * cpu_paging_enabled: * @cpu: The CPU whose state is to be inspected. * diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h index 3d1e5ba..606426f 100644 --- a/include/sysemu/cpus.h +++ b/include/sysemu/cpus.h @@ -26,4 +26,6 @@ extern int smp_threads; void list_cpus(FILE *f, fprintf_function cpu_fprintf, const char *optarg); +void qemu_tcg_configure(QemuOpts *opts); + #endif diff --git a/qemu-options.hx b/qemu-options.hx index a770086..4eca704 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -3224,6 +3224,20 @@ Attach to existing xen domain. xend will use this when starting QEMU (XEN only). ETEXI +DEF("tcg", HAS_ARG, QEMU_OPTION_tcg, \ + "-tcg [mttcg=on|off] control TCG options\n", QEMU_ARCH_ALL) +STEXI +@item -tcg +@findex -tcg +@table @option +@item mttcg=[on|off] +Control multi-threaded TCG. By default QEMU will enable multi-threaded +emulation for front/back-end combinations that are known to work. The +user may enable it against the defaults however odd guest behaviour +may occur. +@end table +ETEXI + DEF("no-reboot", 0, QEMU_OPTION_no_reboot, \ "-no-reboot exit instead of rebooting\n", QEMU_ARCH_ALL) STEXI diff --git a/vl.c b/vl.c index bd81ea9..51bbdbc 100644 --- a/vl.c +++ b/vl.c @@ -2961,7 +2961,8 @@ int main(int argc, char **argv, char **envp) const char *boot_once = NULL; DisplayState *ds; int cyls, heads, secs, translation; - QemuOpts *hda_opts = NULL, *opts, *machine_opts, *icount_opts = NULL; + QemuOpts *opts, *machine_opts; + QemuOpts *hda_opts = NULL, *icount_opts = NULL, *tcg_opts = NULL; QemuOptsList *olist; int optind; const char *optarg; @@ -3750,6 +3751,13 @@ int main(int argc, char **argv, char **envp) case QEMU_OPTION_no_reboot: no_reboot = 1; break; + case QEMU_OPTION_tcg: + tcg_opts = qemu_opts_parse_noisily(qemu_find_opts("tcg"), + optarg, false); + if (!tcg_opts) { + exit(1); + } + break; case QEMU_OPTION_no_shutdown: no_shutdown = 1; break; @@ -4028,6 +4036,8 @@ int main(int argc, char **argv, char **envp) */ loc_set_none(); + qemu_tcg_configure(tcg_opts); + replay_configure(icount_opts); machine_class = select_machine();