Re: [PATCH 12/16] tcg: add "-accel tcg,tb-size" and deprecate "-tb-size"

[Thomas Huth]

On 13/11/2019 15.39, Paolo Bonzini wrote:
> -tb-size fits nicely in the new framework for accelerator-specific options.  
> It
> is a very niche option, so insta-deprecate the old version.

Good idea!

> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  accel/tcg/tcg-all.c    | 40 +++++++++++++++++++++++++++++++++++++---
>  include/sysemu/accel.h |  2 --
>  qemu-deprecated.texi   |  6 ++++++
>  qemu-options.hx        |  6 +++++-
>  vl.c                   |  8 ++++----
>  5 files changed, 52 insertions(+), 10 deletions(-)
> 
> diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
> index 7829f02..1dc384c 100644
> --- a/accel/tcg/tcg-all.c
> +++ b/accel/tcg/tcg-all.c
> @@ -34,11 +34,13 @@
>  #include "include/qapi/error.h"
>  #include "include/qemu/error-report.h"
>  #include "include/hw/boards.h"
> +#include "qapi/qapi-builtin-visit.h"
>  
>  typedef struct TCGState {
>      AccelState parent_obj;
>  
>      bool mttcg_enabled;
> +    unsigned long tb_size;

So this tb_size is "long" ...

>  } TCGState;
>  
>  #define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg")
> @@ -46,8 +48,6 @@ typedef struct TCGState {
>  #define TCG_STATE(obj) \
>          OBJECT_CHECK(TCGState, (obj), TYPE_TCG_ACCEL)
>  
> -unsigned long tcg_tb_size;
> -
>  /* mask must never be zero, except for A20 change call */
>  static void tcg_handle_interrupt(CPUState *cpu, int mask)
>  {
> @@ -126,7 +126,7 @@ static int tcg_init(MachineState *ms)
>  {
>      TCGState *s = TCG_STATE(current_machine->accelerator);
>  
> -    tcg_exec_init(tcg_tb_size * 1024 * 1024);
> +    tcg_exec_init(s->tb_size * 1024 * 1024);

... which is likely ok for this calculation here ...

>      cpu_interrupt_handler = tcg_handle_interrupt;
>      mttcg_enabled = s->mttcg_enabled;
>      return 0;
> @@ -167,6 +167,33 @@ static void tcg_set_thread(Object *obj, const char 
> *value, Error **errp)
>      }
>  }
>  
> +static void tcg_get_tb_size(Object *obj, Visitor *v,
> +                            const char *name, void *opaque,
> +                            Error **errp)
> +{
> +    TCGState *s = TCG_STATE(obj);
> +    uint32_t value = s->tb_size;
> +
> +    visit_type_uint32(v, name, &value, errp);

... but this is only using 32-bit values.

Should be fine, I guess, but it's a little bit confusing when looking at
the code with a quick glance only.

Maybe turn tb_size into a "uint32_t", too, and then use "* MiB" in above
calculation instead of "* 1024 * 1024" ?

> +}
[...]
> diff --git a/qemu-options.hx b/qemu-options.hx
> index b95bf9f..3931f90 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -120,6 +120,7 @@ ETEXI
>  DEF("accel", HAS_ARG, QEMU_OPTION_accel,
>      "-accel [accel=]accelerator[,thread=single|multi]\n"

Maybe add the "[,prop[=value]" change from the next patch here already?

Or should we even split it up into separate lines, like we do it for
-netdev for example already? i.e.:

        -accel tcg,[,thread=single|multi][,tb-size=n]\n
           ...
        -accel xen,[igd-passthrough=...]\n

?

>      "                select accelerator (kvm, xen, hax, hvf, whpx or tcg; 
> use 'help' for a list)\n"
> +    "                tb-size=n (TCG translation block cache size)\n"
>      "                thread=single|multi (enable multi-threaded TCG)\n", 
> QEMU_ARCH_ALL)
>  STEXI
>  @item -accel @var{name}[,prop=@var{value}[,...]]
> @@ -129,6 +130,8 @@ kvm, xen, hax, hvf, whpx or tcg can be available. By 
> default, tcg is used. If th
>  more than one accelerator specified, the next one is used if the previous one
>  fails to initialize.
>  @table @option
> +@item tb-size=@var{n}
> +Controls the size (in MiB) of the TCG translation block cache.
>  @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
> @@ -3995,7 +3998,8 @@ DEF("tb-size", HAS_ARG, QEMU_OPTION_tb_size, \
>  STEXI
>  @item -tb-size @var{n}
>  @findex -tb-size
> -Set TB size.
> +Set TCG translation block cache size.  Deprecated, use @samp{-accel 
> tcg,tb-size=@var{n}}
> +instead.
>  ETEXI
>  
>  DEF("incoming", HAS_ARG, QEMU_OPTION_incoming, \
> diff --git a/vl.c b/vl.c
> index 2ea94c7..06c6ad9 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2857,6 +2857,7 @@ static int do_configure_accelerator(void *opaque, 
> QemuOpts *opts, Error **errp)
>          return 0;
>      }
>      accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
> +    object_apply_compat_props(OBJECT(accel));
>      qemu_opt_foreach(opts, accelerator_set_property,
>                       accel,
>                       &error_fatal);
> @@ -2868,6 +2869,7 @@ static int do_configure_accelerator(void *opaque, 
> QemuOpts *opts, Error **errp)
>                       acc, strerror(-ret));
>          return 0;
>      }
> +

Unnecessary white space change.

>      return 1;
>  }
>  
> @@ -3747,10 +3749,8 @@ int main(int argc, char **argv, char **envp)
>                  error_report("TCG is disabled");
>                  exit(1);
>  #endif
> -                if (qemu_strtoul(optarg, NULL, 0, &tcg_tb_size) < 0) {
> -                    error_report("Invalid argument to -tb-size");
> -                    exit(1);
> -                }
> +                warn_report("The -tb-size option is deprecated, use -accel 
> tcg,tb-size instead");
> +                object_register_sugar_prop(ACCEL_CLASS_NAME("tcg"), 
> "tb-size", optarg);
>                  break;
>              case QEMU_OPTION_icount:
>                  icount_opts = 
> qemu_opts_parse_noisily(qemu_find_opts("icount"),
> 

 Thomas