From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55191) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvOYx-0006Pk-9s for qemu-devel@nongnu.org; Fri, 13 Jun 2014 06:18:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WvOYs-0004ZM-3q for qemu-devel@nongnu.org; Fri, 13 Jun 2014 06:18:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:21591) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvOYr-0004Yj-P7 for qemu-devel@nongnu.org; Fri, 13 Jun 2014 06:18:18 -0400 Message-ID: <539ACFD3.1090409@redhat.com> Date: Fri, 13 Jun 2014 12:17:55 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1402652437-13194-1-git-send-email-sebastian.tanase@openwide.fr> <1402652437-13194-2-git-send-email-sebastian.tanase@openwide.fr> In-Reply-To: <1402652437-13194-2-git-send-email-sebastian.tanase@openwide.fr> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC PATCH V2 1/5] icount: Add 'align' and 'icount' options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sebastian Tanase , qemu-devel@nongnu.org Cc: kwolf@redhat.com, peter.maydell@linaro.org, jeremy.rosen@openwide.fr, aliguori@amazon.com, wenchaoqemu@gmail.com, quintela@redhat.com, mjt@tls.msk.ru, mst@redhat.com, stefanha@redhat.com, armbru@redhat.com, lcapitulino@redhat.com, michael@walle.cc, camille.begue@openwide.fr, alex@alex.org.uk, crobinso@redhat.com, pierre.lemagourou@openwide.fr, afaerber@suse.de, rth@twiddle.net 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=3D][N|auto][,align]' > > Signed-off-by: Sebastian Tanase > Tested-by: Camille B=C3=A9gu=C3=A9 > --- > 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 =3D= { > } > }; > > -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 =3D qemu_opt_get(opts, "icount"); > + icount_align_option =3D qemu_opt_get_bool(opts, "align", false); > + /* When using -icount, it is mandatory to specify an icount value: > + -icount icount=3DN|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") =3D=3D 0 || strcmp(option, "off") =3D=3D = 0) { > + error_set(errp, QERR_INVALID_ICOUNT); > return; > } > > @@ -452,6 +465,10 @@ void configure_icount(const char *option) > icount_time_shift =3D strtol(option, NULL, 0); > use_icount =3D 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=20 does that stil). You can use error_setg and include the error string her= e. > + return; > } > > use_icount =3D 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 incompatibl= e" > + > +#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 > #include > #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=3DN|auto][,align=3Don|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 virt= ual clocks\n", QEMU_ARCH_ALL) > STEXI > -@item -icount [@var{N}|auto] > +@item -icount [icount=3D@var{N}|auto][,align=3Don|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 spe= cified > @@ -2913,6 +2913,19 @@ Note that while this option can give determinist= ic behavior, it does not > provide cycle accurate emulation. Modern CPUs contain superscalar out= of > order cores with complex cache hierarchies. The number of instruction= s > executed often has little or no correlation with actual performance. > + > +@option{align=3Don} will activate the delay algorithm which will try t= o > +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=3Don} 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 =3D qemu_opts_parse(qemu_find_opts("icount"), opti= ons, 0); > + Error *errp =3D NULL; > + configure_icount(opts, &errp); You can just pass &error_abort here. > + qemu_opts_del(opts); > + if (errp !=3D NULL) { > + error_report("%s", error_get_pretty(errp)); > + error_free(errp); > + exit(1); > + } > +} > > +int qtest_init_accel(MachineClass *mc) > +{ > + configure_qtest_icount("icount=3D0"); No need to add "icount=3D" 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 =3D { > }, > }; > > +static QemuOptsList qemu_icount_opts =3D { > + .name =3D "icount", ... you add this here: .implied_opt_name =3D "icount", You will also need: .merge_lists =3D true, so that "-icount 0 -icount align=3Don" also works. In fact, what could be a better name for the implied suboption? I=20 thought of speed and shift. "speed" is more understandable while=20 "shift" is more accurate and matches the code. Otherwise the idea is okay. Thanks! Paolo > + .head =3D QTAILQ_HEAD_INITIALIZER(qemu_icount_opts.head), > + .desc =3D { > + { > + .name =3D "icount", > + .type =3D QEMU_OPT_STRING, > + }, { > + .name =3D "align", > + .type =3D 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 =3D 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 =3D NULL, *opts, *machine_opts; > + QemuOpts *hda_opts =3D NULL, *opts, *machine_opts, *icount_opts =3D= NULL; > QemuOptsList *olist; > int optind; > const char *optarg; > @@ -2991,6 +3006,7 @@ int main(int argc, char **argv, char **envp) > const char *trace_file =3D NULL; > const ram_addr_t default_ram_size =3D (ram_addr_t)DEFAULT_RAM_SIZE= * > 1024 * 1024; > + Error *icount_err =3D 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 =3D optarg; > + icount_opts =3D qemu_opts_parse(qemu_find_opts("icount= "), > + optarg, 0); > + if (!icount_opts) { > + exit(1); > + } > break; > case QEMU_OPTION_incoming: > incoming =3D 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 !=3D 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); >