Re: [Qemu-devel] [RFC PATCH V2 1/5] icount: Add 'align' and 'icount' options

[Paolo Bonzini <pbonzini@redhat.com>]

Il 13/06/2014 11:40, Sebastian Tanase ha scritto:
> The align option is used for activating the align algorithm
> in order to synchronise the host clock and the guest clock.
> Therefore we slightly modified the existing icount option.
> Thus, the new way to specify an icount is
> '-icount [icount=][N|auto][,align]'
>
> Signed-off-by: Sebastian Tanase <sebastian.tanase@openwide.fr>
> Tested-by: Camille Bégué <camille.begue@openwide.fr>
> ---
>  cpus.c                    | 19 ++++++++++++++++++-
>  include/qapi/qmp/qerror.h |  9 +++++++++
>  include/qemu-common.h     |  4 +++-
>  qemu-options.hx           | 19 ++++++++++++++++---
>  qtest.c                   | 19 +++++++++++++++++--
>  vl.c                      | 43 ++++++++++++++++++++++++++++++++++++-------
>  6 files changed, 99 insertions(+), 14 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index dd7ac13..b78a418 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -438,11 +438,24 @@ static const VMStateDescription vmstate_timers = {
>      }
>  };
>
> -void configure_icount(const char *option)
> +void configure_icount(QemuOpts *opts, Error **errp)
>  {
> +    const char *option;
> +
>      seqlock_init(&timers_state.vm_clock_seqlock, NULL);
>      vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
> +    option = qemu_opt_get(opts, "icount");
> +    icount_align_option = qemu_opt_get_bool(opts, "align", false);
> +    /* When using -icount, it is mandatory to specify an icount value:
> +       -icount icount=N|auto */
>      if (!option) {
> +        error_set(errp, QERR_ICOUNT_MISSING);
> +        return;
> +    }
> +    /* When using -icount icount, the icount option will be
> +       misinterpreted as a boolean */
> +    if (strcmp(option, "on") == 0 || strcmp(option, "off") == 0) {
> +        error_set(errp, QERR_INVALID_ICOUNT);
>          return;
>      }
>
> @@ -452,6 +465,10 @@ void configure_icount(const char *option)
>          icount_time_shift = strtol(option, NULL, 0);
>          use_icount = 1;
>          return;
> +    } else if (icount_align_option) {
> +        /* Do not allow icount auto and align on */
> +        error_set(errp, QERR_INVALID_ALIGN, option);

There's no need to define new QERRs anymore (unfortunately a lot of code 
does that stil).  You can use error_setg and include the error string here.

> +        return;
>      }
>
>      use_icount = 2;
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 902d1a7..ac307d0 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -91,6 +91,15 @@ void qerror_report_err(Error *err);
>  #define QERR_FEATURE_DISABLED \
>      ERROR_CLASS_GENERIC_ERROR, "The feature '%s' is not enabled"
>
> +#define QERR_ICOUNT_MISSING \
> +    ERROR_CLASS_GENERIC_ERROR, "Icount option missing"
> +
> +#define QERR_INVALID_ALIGN \
> +    ERROR_CLASS_GENERIC_ERROR, "Icount %s and align on are incompatible"
> +
> +#define QERR_INVALID_ICOUNT \
> +    ERROR_CLASS_GENERIC_ERROR, "Icount must be a number or auto"
> +
>  #define QERR_INVALID_BLOCK_FORMAT \
>      ERROR_CLASS_GENERIC_ERROR, "Invalid block format '%s'"
>
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 66ceceb..288b460 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -41,6 +41,7 @@
>  #include <assert.h>
>  #include <signal.h>
>  #include "glib-compat.h"
> +#include "qemu/option.h"
>
>  #ifdef _WIN32
>  #include "sysemu/os-win32.h"
> @@ -105,8 +106,9 @@ static inline char *realpath(const char *path, char *resolved_path)
>  #endif
>
>  /* icount */
> -void configure_icount(const char *option);
> +void configure_icount(QemuOpts *opts, Error **errp);
>  extern int use_icount;
> +extern int icount_align_option;
>
>  #include "qemu/osdep.h"
>  #include "qemu/bswap.h"
> diff --git a/qemu-options.hx b/qemu-options.hx
> index d0714c4..5e7c956 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2898,11 +2898,11 @@ re-inject them.
>  ETEXI
>
>  DEF("icount", HAS_ARG, QEMU_OPTION_icount, \
> -    "-icount [N|auto]\n" \
> +    "-icount [icount=N|auto][,align=on|off]\n" \
>      "                enable virtual instruction counter with 2^N clock ticks per\n" \
> -    "                instruction\n", QEMU_ARCH_ALL)
> +    "                instruction and enable aligning the host and virtual clocks\n", QEMU_ARCH_ALL)
>  STEXI
> -@item -icount [@var{N}|auto]
> +@item -icount [icount=@var{N}|auto][,align=on|off]
>  @findex -icount
>  Enable virtual instruction counter.  The virtual cpu will execute one
>  instruction every 2^@var{N} ns of virtual time.  If @code{auto} is specified
> @@ -2913,6 +2913,19 @@ Note that while this option can give deterministic behavior, it does not
>  provide cycle accurate emulation.  Modern CPUs contain superscalar out of
>  order cores with complex cache hierarchies.  The number of instructions
>  executed often has little or no correlation with actual performance.
> +
> +@option{align=on} will activate the delay algorithm which will try to
> +to synchronise the host clock and the virtual clock. The goal is to
> +have a guest running at the real frequency imposed by
> +icount. Whenever the guest clock is behind the host clock and if
> +@option{align=on} is specified then we print a messsage to the user
> +to inform about the delay.
> +You must provide a value for @option{icount} other than @code{auto}
> +for the align option to be taken into account.
> +Currently this option does not work with @code{auto}.
> +Note: The sync algorithm will work on those icounts on which
> +the guest clock runs ahead of the host clock. Typically this happens
> +when the icount value is high (how high depends on the host machine).
>  ETEXI
>
>  DEF("watchdog", HAS_ARG, QEMU_OPTION_watchdog, \
> diff --git a/qtest.c b/qtest.c
> index 04a6dc1..9b6c29b 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -19,6 +19,9 @@
>  #include "hw/irq.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/cpus.h"
> +#include "qemu/config-file.h"
> +#include "qemu/option.h"
> +#include "qemu/error-report.h"
>
>  #define MAX_IRQ 256
>
> @@ -509,10 +512,22 @@ static void qtest_event(void *opaque, int event)
>      }
>  }
>
> -int qtest_init_accel(MachineClass *mc)
> +static void configure_qtest_icount(const char *options)
>  {
> -    configure_icount("0");
> +    QemuOpts *opts  = qemu_opts_parse(qemu_find_opts("icount"), options, 0);
> +    Error *errp = NULL;
> +    configure_icount(opts, &errp);

You can just pass &error_abort here.

> +    qemu_opts_del(opts);
> +    if (errp != NULL) {
> +        error_report("%s", error_get_pretty(errp));
> +        error_free(errp);
> +        exit(1);
> +    }
> +}
>
> +int qtest_init_accel(MachineClass *mc)
> +{
> +    configure_qtest_icount("icount=0");

No need to add "icount=" here if...

>      return 0;
>  }
>
> diff --git a/vl.c b/vl.c
> index ac0e3d7..1bd467a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -182,6 +182,7 @@ uint8_t *boot_splash_filedata;
>  size_t boot_splash_filedata_size;
>  uint8_t qemu_extra_params_fw[2];
>
> +int icount_align_option;
>  typedef struct FWBootEntry FWBootEntry;
>
>  struct FWBootEntry {
> @@ -524,6 +525,21 @@ static QemuOptsList qemu_mem_opts = {
>      },
>  };
>
> +static QemuOptsList qemu_icount_opts = {
> +    .name = "icount",

... you add this here:

     .implied_opt_name = "icount",

You will also need:

     .merge_lists = true,

so that "-icount 0 -icount align=on" also works.

In fact, what could be a better name for the implied suboption?  I 
thought of speed and shift.  "speed" is more understandable while 
"shift" is more accurate and matches the code.

Otherwise the idea is okay.

Thanks!

Paolo

> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_icount_opts.head),
> +    .desc = {
> +        {
> +            .name = "icount",
> +            .type = QEMU_OPT_STRING,
> +        }, {
> +            .name = "align",
> +            .type = QEMU_OPT_BOOL,
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>  /**
>   * Get machine options
>   *
> @@ -2957,13 +2973,12 @@ int main(int argc, char **argv, char **envp)
>  {
>      int i;
>      int snapshot, linux_boot;
> -    const char *icount_option = NULL;
>      const char *initrd_filename;
>      const char *kernel_filename, *kernel_cmdline;
>      const char *boot_order;
>      DisplayState *ds;
>      int cyls, heads, secs, translation;
> -    QemuOpts *hda_opts = NULL, *opts, *machine_opts;
> +    QemuOpts *hda_opts = NULL, *opts, *machine_opts, *icount_opts = NULL;
>      QemuOptsList *olist;
>      int optind;
>      const char *optarg;
> @@ -2991,6 +3006,7 @@ int main(int argc, char **argv, char **envp)
>      const char *trace_file = NULL;
>      const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
>                                          1024 * 1024;
> +    Error *icount_err = NULL;
>
>      atexit(qemu_run_exit_notifiers);
>      error_set_progname(argv[0]);
> @@ -3024,6 +3040,7 @@ int main(int argc, char **argv, char **envp)
>      qemu_add_opts(&qemu_realtime_opts);
>      qemu_add_opts(&qemu_msg_opts);
>      qemu_add_opts(&qemu_name_opts);
> +    qemu_add_opts(&qemu_icount_opts);
>
>      runstate_init();
>
> @@ -3833,7 +3850,11 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  break;
>              case QEMU_OPTION_icount:
> -                icount_option = optarg;
> +                icount_opts = qemu_opts_parse(qemu_find_opts("icount"),
> +                                              optarg, 0);
> +                if (!icount_opts) {
> +                    exit(1);
> +                }
>                  break;
>              case QEMU_OPTION_incoming:
>                  incoming = optarg;
> @@ -4301,11 +4322,19 @@ int main(int argc, char **argv, char **envp)
>      qemu_spice_init();
>  #endif
>
> -    if (icount_option && (kvm_enabled() || xen_enabled())) {
> -        fprintf(stderr, "-icount is not allowed with kvm or xen\n");
> -        exit(1);
> +    if (icount_opts) {
> +        if (kvm_enabled() || xen_enabled()) {
> +            fprintf(stderr, "-icount is not allowed with kvm or xen\n");
> +            exit(1);
> +        }
> +        configure_icount(icount_opts, &icount_err);
> +        qemu_opts_del(icount_opts);
> +        if (icount_err != NULL) {
> +            error_report("%s", error_get_pretty(icount_err));
> +            error_free(icount_err);
> +            exit(1);
> +        }
>      }
> -    configure_icount(icount_option);
>
>      /* clean up network at qemu process termination */
>      atexit(&net_cleanup);
>