Re: [PATCH] qemu-options.hx: Fix minor issues in icount documentation

[Laurent Vivier]

Le 21/11/2020 à 22:35, Peter Maydell a écrit :
> The documentation for the icount documentation has some minor issues:
>  * in a couple of places it says "sleep=on|off" when in the context of the
>    sentence it means specifically "sleep=on"
>  * the synopsis line for the documentation has drifted out of sync
>    with the synopsis line in the DEF() macro (used for "-help" output)
>  * the synopsis line in the DEF() macro is missing a "][" between
>    the sleep= part and the rr= part
>  * the synopsis line doesn't indicate that rrsnapshot is an optional
>    part of the rr=mode,rrfile=filename subgrouping
>  * we don't document that sleep=on can't be used with shift=auto
>    or align=on
>  * the rr option description had some minor grammar and formatting
>    errors and was a bit terse
>  * in commit f1f4b57e88ff in 2015 the documentation of the sleep=
>    suboption got added between the two paragraphs defining general
>    behaviour of the icount option. This meant that the second
>    paragraph talking about the behaviour of "this option" reads as
>    if it's talking about sleep=on, when it's really describing -icount
>    as a whole. The paragraph is better moved back up to above the
>    sleep= section.
>  * the summary text displayed in "-help" output didn't mention
>    the record-and-replay part
> 
> Fix these errors.
> 
> Fixes: https://bugs.launchpad.net/qemu/+bug/1774412
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  qemu-options.hx | 42 ++++++++++++++++++++++++------------------
>  1 file changed, 24 insertions(+), 18 deletions(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 2c83390504d..41cb42e7a5d 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3956,30 +3956,34 @@ SRST
>  ERST
>  
>  DEF("icount", HAS_ARG, QEMU_OPTION_icount, \
> -    "-icount 
> [shift=N|auto][,align=on|off][,sleep=on|off,rr=record|replay,rrfile=<filename>,rrsnapshot=<snapshot>]\n"
>  \
> +    "-icount 
> [shift=N|auto][,align=on|off][,sleep=on|off][,rr=record|replay,rrfile=<filename>[,rrsnapshot=<snapshot>]]\n"
>  \
>      "                enable virtual instruction counter with 2^N clock ticks 
> per\n" \
>      "                instruction, enable aligning the host and virtual 
> clocks\n" \
> -    "                or disable real time cpu sleeping\n", QEMU_ARCH_ALL)
> +    "                or disable real time cpu sleeping, and optionally 
> enable\n" \
> +    "                record-and-replay mode\n", QEMU_ARCH_ALL)
>  SRST
> -``-icount 
> [shift=N|auto][,rr=record|replay,rrfile=filename,rrsnapshot=snapshot]``
> +``-icount 
> [shift=N|auto][,align=on|off][,sleep=on|off][,rr=record|replay,rrfile=filename[,rrsnapshot=snapshot]]``
>      Enable virtual instruction counter. The virtual cpu will execute one
>      instruction every 2^N ns of virtual time. If ``auto`` is specified
>      then the virtual cpu speed will be automatically adjusted to keep
>      virtual time within a few seconds of real time.
>  
> -    When the virtual cpu is sleeping, the virtual time will advance at
> -    default speed unless ``sleep=on|off`` is specified. With
> -    ``sleep=on|off``, the virtual time will jump to the next timer
> -    deadline instantly whenever the virtual cpu goes to sleep mode and
> -    will not advance if no timer is enabled. This behavior give
> -    deterministic execution times from the guest point of view.
> -
>      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.
>  
> +    When the virtual cpu is sleeping, the virtual time will advance at
> +    default speed unless ``sleep=on`` is specified. With
> +    ``sleep=on``, the virtual time will jump to the next timer
> +    deadline instantly whenever the virtual cpu goes to sleep mode and
> +    will not advance if no timer is enabled. This behavior gives
> +    deterministic execution times from the guest point of view.
> +    The default if icount is enabled is ``sleep=off``.
> +    ``sleep=on`` cannot be used together with either ``shift=auto``
> +    or ``align=on``.
> +
>      ``align=on`` will activate the delay algorithm which will try to
>      synchronise the host clock and the virtual clock. The goal is to
>      have a guest running at the real frequency imposed by the shift
> @@ -3989,15 +3993,17 @@ SRST
>      ``shift`` is ``auto``. Note: The sync algorithm will work for those
>      shift values for which the guest clock runs ahead of the host clock.
>      Typically this happens when the shift value is high (how high
> -    depends on the host machine).
> +    depends on the host machine). The default if icount is enabled
> +    is ``align=off``.
>  
> -    When ``rr`` option is specified deterministic record/replay is
> -    enabled. Replay log is written into filename file in record mode and
> -    read from this file in replay mode.
> -
> -    Option rrsnapshot is used to create new vm snapshot named snapshot
> -    at the start of execution recording. In replay mode this option is
> -    used to load the initial VM state.
> +    When the ``rr`` option is specified deterministic record/replay is
> +    enabled. The ``rrfile=`` option must also be provided to
> +    specify the path to the replay log. In record mode data is written
> +    to this file, and in replay mode it is read back.
> +    If the ``rrsnapshot`` option is given then it specifies a VM snapshot
> +    name. In record mode, a new VM snapshot with the given name is created
> +    at the start of execution recording. In replay mode this option
> +    specifies the snapshot name used to load the initial VM state.
>  ERST
>  
>  DEF("watchdog", HAS_ARG, QEMU_OPTION_watchdog, \
> 

Applied to my linux-user-for-6.0 branch.

Thanks,
Laurent