| Message ID | 1464986428-6739-12-git-send-email-alex.bennee@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/06/16 23:40, Alex Bennée wrote: > diff --git a/cpus.c b/cpus.c > index 4cc2ce6..1694ce9 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -25,6 +25,7 @@ > /* Needed early for CONFIG_BSD etc. */ > #include "qemu/osdep.h" > #include "qemu-common.h" > +#include "qemu/config-file.h" > #include "cpu.h" > #include "monitor/monitor.h" > #include "qapi/qmp/qerror.h" @@ -148,6 +149,33 @@ typedef struct TimersState { } TimersState; > static TimersState timers_state; +static bool mttcg_enabled; + +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) +{ + const char *t = qemu_opt_get(opts, "thread"); > + if (t) { > + mttcg_enabled = (strcmp(t, "multi") == 0); > + } else { > + mttcg_enabled = default_mttcg_enabled(); Wouldn't we like to check for user errors here? Suppose a user have passed "multy" or "mult" and think they've enabled MTTCG whereas such values are silently ignored. > + } > +} > + > +bool qemu_tcg_mttcg_enabled(void) > +{ > + return mttcg_enabled; > +} > + > > int64_t cpu_get_icount_raw(void) > { (snip) > diff --git a/qemu-options.hx b/qemu-options.hx > index 6106520..dc865ec 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -99,6 +99,26 @@ STEXI > Select CPU model (@code{-cpu help} for list and additional feature selection) > ETEXI > > +DEF("accel", HAS_ARG, QEMU_OPTION_accel, > + "-accel [accel=]accelerator[,thread=single|multi]\n" > + " select accelerator ('-accel help for list')\n" > + " thread=single|multi (enable multi-threaded TCG)", QEMU_ARCH_ALL) "mttcg=on|off" or "multithread=on|off", instead of "thread=single|multi", are another possible variants. The former has less letters :) > +STEXI > +@item -accel @var{name}[,prop=@var{value}[,...]] > +@findex -accel > +This is used to enable an accelerator. Depending on the target architecture, > +kvm, xen, or tcg can be available. By default, tcg is used. If there is more > +than one accelerator specified, the next one is used if the previous one fails > +to initialize. > +@table @option > +@item thread=single|multi > +Controls number of TCG threads. When the TCG is multi-threaded there will be one > +thread per vCPU therefor taking advantage of additional host cores. The default > +is to enable multi-threading where both the back-end and front-ends support it and > +no incompatible TCG features have been enabled (e.g. icount/replay). We could raise and error on attempts to use icount with MTTCG enabled modifying this piece of code in vl.c: if (icount_opts) { if (kvm_enabled() || xen_enabled()) { error_report("-icount is not allowed with kvm or xen"); exit(1); } > +@end table > +ETEXI > + > DEF("smp", HAS_ARG, QEMU_OPTION_smp, > "-smp [cpus=]n[,maxcpus=cpus][,cores=cores][,threads=threads][,sockets=sockets]\n" > " set the number of CPUs to 'n' [default=1]\n" > diff --git a/vl.c b/vl.c > index 18d1423..b1224f9 100644 > --- a/vl.c > +++ b/vl.c (snip) > @@ -3682,6 +3704,27 @@ int main(int argc, char **argv, char **envp) > qdev_prop_register_global(&kvm_pit_lost_tick_policy); > break; > } > + case QEMU_OPTION_accel: > + opts = qemu_opts_parse_noisily(qemu_find_opts("accel"), > + optarg, true); > + optarg = qemu_opt_get(opts, "accel"); > + > + olist = qemu_find_opts("machine"); > + if (strcmp("kvm", optarg) == 0) { > + qemu_opts_parse_noisily(olist, "accel=kvm", false); > + } else if (strcmp("xen", optarg) == 0) { > + qemu_opts_parse_noisily(olist, "accel=xen", false); > + } else if (strcmp("tcg", optarg) == 0) { > + qemu_opts_parse_noisily(olist, "accel=tcg", false); > + qemu_tcg_configure(opts); > + } else { > + if (!is_help_option(optarg)) { > + error_printf("Unknown accelerator: %s", optarg); > + } > + error_printf("Supported accelerators: kvm, xen, tcg\n"); > + exit(1); > + } I am wondering if we should use accel_find() here like in configure_accelerator() and probably also make checks with AccelClass::available(). Please consider wrapping this code in a separate function. > + break; > case QEMU_OPTION_usb: > olist = qemu_find_opts("machine"); > qemu_opts_parse_noisily(olist, "usb=on", false); Kind regards, Sergey
On 06/03/2016 01:40 PM, Alex Bennée wrote: > +bool qemu_tcg_mttcg_enabled(void) > +{ > + return mttcg_enabled; > +} Is there a good reason to expose this via function call, rather than just test the variable? r~
Richard Henderson <rth@twiddle.net> writes: > On 06/03/2016 01:40 PM, Alex Bennée wrote: >> +bool qemu_tcg_mttcg_enabled(void) >> +{ >> + return mttcg_enabled; >> +} > > Is there a good reason to expose this via function call, rather than just test > the variable? It was in Fred's original patch. I guess there could be an argument versus a global variable but as mttcg_enabled is set during option parsing we don't need to do anything dynamic. > > > r~ -- Alex Bennée
On 02/07/16 10:11, Alex Bennée wrote: > Richard Henderson <rth@twiddle.net> writes: > >> On 06/03/2016 01:40 PM, Alex Bennée wrote: >>> +bool qemu_tcg_mttcg_enabled(void) >>> +{ >>> + return mttcg_enabled; >>> +} >> Is there a good reason to expose this via function call, rather than just test >> the variable? > It was in Fred's original patch. I guess there could be an argument > versus a global variable but as mttcg_enabled is set during option > parsing we don't need to do anything dynamic. I think it just resembles tcg_enabled(), kvm_enabled() etc. Regards, Sergey
On 02/07/2016 09:38, Sergey Fedorov wrote: >>>> >>> +bool qemu_tcg_mttcg_enabled(void) >>>> >>> +{ >>>> >>> + return mttcg_enabled; >>>> >>> +} >>> >> Is there a good reason to expose this via function call, rather than just test >>> >> the variable? >> > It was in Fred's original patch. I guess there could be an argument >> > versus a global variable but as mttcg_enabled is set during option >> > parsing we don't need to do anything dynamic. > I think it just resembles tcg_enabled(), kvm_enabled() etc. Those are macros however, not functions. They are macros in order to cull KVM code for files that are compiled per-target. Paolo
I've taken all the suggestions apart from the bellow (comment inline): Sergey Fedorov <serge.fdrv@gmail.com> writes: <snip> >> --- a/qemu-options.hx >> +++ b/qemu-options.hx >> @@ -99,6 +99,26 @@ STEXI >> Select CPU model (@code{-cpu help} for list and additional feature selection) >> ETEXI >> >> +DEF("accel", HAS_ARG, QEMU_OPTION_accel, >> + "-accel [accel=]accelerator[,thread=single|multi]\n" >> + " select accelerator ('-accel help for list')\n" >> + " thread=single|multi (enable multi-threaded TCG)", QEMU_ARCH_ALL) > > "mttcg=on|off" or "multithread=on|off", instead of > "thread=single|multi", are another possible variants. The former has > less letters :) I went with the previous suggestion during the last round. mttcg could be a little cryptic, multithread=on|off is much of a muchness. <snip> >> } >> + case QEMU_OPTION_accel: >> + opts = qemu_opts_parse_noisily(qemu_find_opts("accel"), >> + optarg, true); >> + optarg = qemu_opt_get(opts, "accel"); >> + >> + olist = qemu_find_opts("machine"); >> + if (strcmp("kvm", optarg) == 0) { >> + qemu_opts_parse_noisily(olist, "accel=kvm", false); >> + } else if (strcmp("xen", optarg) == 0) { >> + qemu_opts_parse_noisily(olist, "accel=xen", false); >> + } else if (strcmp("tcg", optarg) == 0) { >> + qemu_opts_parse_noisily(olist, "accel=tcg", false); >> + qemu_tcg_configure(opts); >> + } else { >> + if (!is_help_option(optarg)) { >> + error_printf("Unknown accelerator: %s", optarg); >> + } >> + error_printf("Supported accelerators: kvm, xen, tcg\n"); >> + exit(1); >> + } > > I am wondering if we should use accel_find() here like in > configure_accelerator() and probably also make checks with > AccelClass::available(). I'm not sure exactly what magic is going on here in the pseudo-class stuff. > > Please consider wrapping this code in a separate function. It seems broadly in line with other snippets in vl.c so I've left it for now. > > >> + break; >> case QEMU_OPTION_usb: >> olist = qemu_find_opts("machine"); >> qemu_opts_parse_noisily(olist, "usb=on", false); > > Kind regards, > Sergey -- Alex Bennée
diff --git a/cpus.c b/cpus.c index 4cc2ce6..1694ce9 100644 --- a/cpus.c +++ b/cpus.c @@ -25,6 +25,7 @@ /* Needed early for CONFIG_BSD etc. */ #include "qemu/osdep.h" #include "qemu-common.h" +#include "qemu/config-file.h" #include "cpu.h" #include "monitor/monitor.h" #include "qapi/qmp/qerror.h" @@ -148,6 +149,33 @@ typedef struct TimersState { } TimersState; static TimersState timers_state; +static bool mttcg_enabled; + +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) +{ + const char *t = qemu_opt_get(opts, "thread"); + if (t) { + mttcg_enabled = (strcmp(t, "multi") == 0); + } else { + mttcg_enabled = 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 b82625f..40823bf 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -376,6 +376,14 @@ extern struct CPUTailQ cpus; extern __thread CPUState *current_cpu; /** + * 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 fe992a8..4948c40 100644 --- a/include/sysemu/cpus.h +++ b/include/sysemu/cpus.h @@ -39,4 +39,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 6106520..dc865ec 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -99,6 +99,26 @@ STEXI Select CPU model (@code{-cpu help} for list and additional feature selection) ETEXI +DEF("accel", HAS_ARG, QEMU_OPTION_accel, + "-accel [accel=]accelerator[,thread=single|multi]\n" + " select accelerator ('-accel help for list')\n" + " thread=single|multi (enable multi-threaded TCG)", QEMU_ARCH_ALL) +STEXI +@item -accel @var{name}[,prop=@var{value}[,...]] +@findex -accel +This is used to enable an accelerator. Depending on the target architecture, +kvm, xen, or tcg can be available. By default, tcg is used. If there is more +than one accelerator specified, the next one is used if the previous one fails +to initialize. +@table @option +@item thread=single|multi +Controls number of TCG threads. When the TCG is multi-threaded there will be one +thread per vCPU therefor taking advantage of additional host cores. The default +is to enable multi-threading where both the back-end and front-ends support it and +no incompatible TCG features have been enabled (e.g. icount/replay). +@end table +ETEXI + DEF("smp", HAS_ARG, QEMU_OPTION_smp, "-smp [cpus=]n[,maxcpus=cpus][,cores=cores][,threads=threads][,sockets=sockets]\n" " set the number of CPUs to 'n' [default=1]\n" diff --git a/vl.c b/vl.c index 18d1423..b1224f9 100644 --- a/vl.c +++ b/vl.c @@ -312,6 +312,26 @@ static QemuOptsList qemu_machine_opts = { }, }; +static QemuOptsList qemu_accel_opts = { + .name = "accel", + .implied_opt_name = "accel", + .head = QTAILQ_HEAD_INITIALIZER(qemu_accel_opts.head), + .merge_lists = true, + .desc = { + { + .name = "accel", + .type = QEMU_OPT_STRING, + .help = "Select the type of accelerator", + }, + { + .name = "thread", + .type = QEMU_OPT_STRING, + .help = "Enable/disable multi-threaded TCG", + }, + { /* end of list */ } + }, +}; + static QemuOptsList qemu_boot_opts = { .name = "boot-opts", .implied_opt_name = "order", @@ -2946,7 +2966,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; QemuOptsList *olist; int optind; const char *optarg; @@ -2996,6 +3017,7 @@ int main(int argc, char **argv, char **envp) qemu_add_opts(&qemu_trace_opts); qemu_add_opts(&qemu_option_rom_opts); qemu_add_opts(&qemu_machine_opts); + qemu_add_opts(&qemu_accel_opts); qemu_add_opts(&qemu_mem_opts); qemu_add_opts(&qemu_smp_opts); qemu_add_opts(&qemu_boot_opts); @@ -3682,6 +3704,27 @@ int main(int argc, char **argv, char **envp) qdev_prop_register_global(&kvm_pit_lost_tick_policy); break; } + case QEMU_OPTION_accel: + opts = qemu_opts_parse_noisily(qemu_find_opts("accel"), + optarg, true); + optarg = qemu_opt_get(opts, "accel"); + + olist = qemu_find_opts("machine"); + if (strcmp("kvm", optarg) == 0) { + qemu_opts_parse_noisily(olist, "accel=kvm", false); + } else if (strcmp("xen", optarg) == 0) { + qemu_opts_parse_noisily(olist, "accel=xen", false); + } else if (strcmp("tcg", optarg) == 0) { + qemu_opts_parse_noisily(olist, "accel=tcg", false); + qemu_tcg_configure(opts); + } else { + if (!is_help_option(optarg)) { + error_printf("Unknown accelerator: %s", optarg); + } + error_printf("Supported accelerators: kvm, xen, tcg\n"); + exit(1); + } + break; case QEMU_OPTION_usb: olist = qemu_find_opts("machine"); qemu_opts_parse_noisily(olist, "usb=on", false);