Re: [Qemu-devel] [RFC v3 11/19] tcg: add options for enabling MTTCG

[Sergey Fedorov]

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
> address@hidden -accel @var{name}[,address@hidden,...]]
> address@hidden -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.
> address@hidden @option
> address@hidden 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);
        }
 

> address@hidden 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