[RFC,v3,11/19] tcg: add options for enabling MTTCG

From: KONRAD Frederic <fred.konrad@greensocs.com>

We know there will be cases where MTTCG won't work until additional work
is done in the front/back ends to support. It will however be useful to
be able to turn it on.

As a result MTTCG will default to off unless the combination is
supported. However the user can turn it on for the sake of testing.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
[AJB: move to -accel tcg,thread=multi|single, defaults]
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v1:
  - merge with add mttcg option.
  - update commit message
v2:
  - machine_init->opts_init
v3:
  - moved from -tcg to -accel tcg,thread=single|multi
  - fix checkpatch warnings
---
 cpus.c                | 28 ++++++++++++++++++++++++++++
 include/qom/cpu.h     |  8 ++++++++
 include/sysemu/cpus.h |  2 ++
 qemu-options.hx       | 20 ++++++++++++++++++++
 vl.c                  | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 102 insertions(+), 1 deletion(-)

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