Re: [Qemu-devel] [RFC PATCH V3 1/6] icount: Add QemuOpts for icount

[Frederic Konrad <fred.konrad@greensocs.com>]

On 30/06/2014 15:59, Sebastian Tanase wrote:
> Make icount parameter use QemuOpts style options in order
> to easily add other suboptions.
>
> Signed-off-by: Sebastian Tanase <sebastian.tanase@openwide.fr>
> Tested-by: Camille Bégué <camille.begue@openwide.fr>
> ---
>   cpus.c                | 10 +++++++++-
>   include/qemu-common.h |  3 ++-
>   qemu-options.hx       |  4 ++--
>   qtest.c               | 13 +++++++++++--
>   vl.c                  | 35 ++++++++++++++++++++++++++++-------
>   5 files changed, 52 insertions(+), 13 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index 5e7f2cf..dcca96a 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -440,13 +440,21 @@ 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, "shift");
>       if (!option) {
>           return;
>       }
> +    /* When using -icount shift, the shift option will be
> +       misinterpreted as a boolean */
> +    if (strcmp(option, "on") == 0 || strcmp(option, "off") == 0) {
> +        error_setg(errp, "The shift option must be a number or auto");
> +    }
>   
>       icount_warp_timer = timer_new_ns(QEMU_CLOCK_REALTIME,
>                                             icount_warp_rt, NULL);
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index ae76197..cc346ec 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,7 +106,7 @@ 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;
>   
>   #include "qemu/osdep.h"
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9e54686..143def4 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3011,11 +3011,11 @@ re-inject them.
>   ETEXI
>   
>   DEF("icount", HAS_ARG, QEMU_OPTION_icount, \
> -    "-icount [N|auto]\n" \
> +    "-icount [shift=N|auto]\n" \
>       "                enable virtual instruction counter with 2^N clock ticks per\n" \
>       "                instruction\n", QEMU_ARCH_ALL)
>   STEXI
> -@item -icount [@var{N}|auto]
> +@item -icount [shift=@var{N}|auto]
>   @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
> diff --git a/qtest.c b/qtest.c
> index 04a6dc1..ef0d991 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,16 @@ 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, 1);
> +    configure_icount(opts, &error_abort);
> +    qemu_opts_del(opts);
> +}
>   
> +int qtest_init_accel(MachineClass *mc)
> +{
> +    configure_qtest_icount("0");
>       return 0;
>   }
>   
> diff --git a/vl.c b/vl.c
> index 41ddcd2..103027f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -537,6 +537,20 @@ static QemuOptsList qemu_mem_opts = {
>       },
>   };
>   
> +static QemuOptsList qemu_icount_opts = {
> +    .name = "icount",
> +    .implied_opt_name = "shift",
> +    .merge_lists = true,
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_icount_opts.head),
> +    .desc = {
> +        {
> +            .name = "shift",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>   /**
>    * Get machine options
>    *
> @@ -2896,13 +2910,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;
> @@ -2967,6 +2980,7 @@ int main(int argc, char **argv, char **envp)
>       qemu_add_opts(&qemu_msg_opts);
>       qemu_add_opts(&qemu_name_opts);
>       qemu_add_opts(&qemu_numa_opts);
> +    qemu_add_opts(&qemu_icount_opts);
>   
>       runstate_init();
>   
> @@ -3817,7 +3831,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, 1);
> +                if (!icount_opts) {
> +                    exit(1);
> +                }
>                   break;
>               case QEMU_OPTION_incoming:
>                   incoming = optarg;
> @@ -4294,11 +4312,14 @@ 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, &error_abort);
> +        qemu_opts_del(icount_opts);
>       }
> -    configure_icount(icount_option);
>   
>       /* clean up network at qemu process termination */
>       atexit(&net_cleanup);
Looks ok to me ;).

Fred