Re: [PATCH 5/5] qemu-options: Remove the deprecated -singlestep option

[Thomas Huth <thuth@redhat.com>]

On 12/01/2024 16.39, Philippe Mathieu-Daudé wrote:
> Hi Thomas
> 
> +Laurent & Peter
> 
> On 12/1/24 11:00, Thomas Huth wrote:
>> It's been marked as deprecated since QEMU 8.1, so it should be fine
>> to remove this now.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   docs/about/deprecated.rst       |  6 ------
>>   docs/about/removed-features.rst |  7 +++++++
>>   system/vl.c                     | 18 +-----------------
>>   qemu-options.hx                 |  8 --------
>>   4 files changed, 8 insertions(+), 31 deletions(-)
>>
>> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
>> index b50cfe596b..81a5149f1e 100644
>> --- a/docs/about/deprecated.rst
>> +++ b/docs/about/deprecated.rst
>> @@ -63,12 +63,6 @@ as short-form boolean values, and passed to plugins as 
>> ``arg_name=on``.
>>   However, short-form booleans are deprecated and full explicit 
>> ``arg_name=on``
>>   form is preferred.
>> -``-singlestep`` (since 8.1)
>> -'''''''''''''''''''''''''''
>> -
>> -The ``-singlestep`` option has been turned into an accelerator property,
>> -and given a name that better reflects what it actually does.
>> -Use ``-accel tcg,one-insn-per-tb=on`` instead.
>>   User-mode emulator command line arguments
>>   -----------------------------------------
>> diff --git a/docs/about/removed-features.rst 
>> b/docs/about/removed-features.rst
>> index a8546f4787..d5c60ff47f 100644
>> --- a/docs/about/removed-features.rst
>> +++ b/docs/about/removed-features.rst
>> @@ -482,6 +482,13 @@ Use ``-run-with async-teardown=on`` instead.
>>   Use ``-run-with chroot=dir`` instead.
>> +``-singlestep`` (removed in 9.0)
>> +''''''''''''''''''''''''''''''''
>> +
>> +The ``-singlestep`` option has been turned into an accelerator property,
>> +and given a name that better reflects what it actually does.
>> +Use ``-accel tcg,one-insn-per-tb=on`` instead.
>> +
>>   QEMU Machine Protocol (QMP) commands
>>   ------------------------------------
>> diff --git a/system/vl.c b/system/vl.c
>> index c125fb9079..809f867bcc 100644
>> --- a/system/vl.c
>> +++ b/system/vl.c
>> @@ -181,7 +181,6 @@ static const char *log_file;
>>   static bool list_data_dirs;
>>   static const char *qtest_chrdev;
>>   static const char *qtest_log;
>> -static bool opt_one_insn_per_tb;
>>   static int has_defaults = 1;
>>   static int default_audio = 1;
>> @@ -2308,19 +2307,7 @@ static int do_configure_accelerator(void *opaque, 
>> QemuOpts *opts, Error **errp)
>>       qemu_opt_foreach(opts, accelerator_set_property,
>>                        accel,
>>                        &error_fatal);
>> -    /*
>> -     * If legacy -singlestep option is set, honour it for TCG and
>> -     * silently ignore for any other accelerator (which is how this
>> -     * option has always behaved).
>> -     */
>> -    if (opt_one_insn_per_tb) {
>> -        /*
>> -         * This will always succeed for TCG, and we want to ignore
>> -         * the error from trying to set a nonexistent property
>> -         * on any other accelerator.
>> -         */
>> -        object_property_set_bool(OBJECT(accel), "one-insn-per-tb", true, 
>> NULL);
>> -    }
>> +
>>       ret = accel_init_machine(accel, current_machine);
>>       if (ret < 0) {
>>           if (!qtest_with_kvm || ret != -ENOENT) {
>> @@ -3057,9 +3044,6 @@ void qemu_init(int argc, char **argv)
>>               case QEMU_OPTION_bios:
>>                   qdict_put_str(machine_opts_dict, "firmware", optarg);
>>                   break;
>> -            case QEMU_OPTION_singlestep:
>> -                opt_one_insn_per_tb = true;
>> -                break;
>>               case QEMU_OPTION_S:
>>                   autostart = 0;
>>                   break;
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 9be6beb5a0..033fa873e4 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -4357,14 +4357,6 @@ SRST
>>       from a script.
>>   ERST
>> -DEF("singlestep", 0, QEMU_OPTION_singlestep, \
>> -    "-singlestep     deprecated synonym for -accel 
>> tcg,one-insn-per-tb=on\n", QEMU_ARCH_ALL)
>> -SRST
>> -``-singlestep``
>> -    This is a deprecated synonym for the TCG accelerator property
>> -    ``one-insn-per-tb``.
>> -ERST
>> -
>>   DEF("preconfig", 0, QEMU_OPTION_preconfig, \
>>       "--preconfig     pause QEMU before machine is initialized 
>> (experimental)\n",
>>       QEMU_ARCH_ALL)
> 
> This is the system part, what about the user part?
> 
> StatusInfo::singlestep was deprecated at the same time,
> can we remove it?
> 
> IOW could we complete your patch with this?
> 
> -- >8 --
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index b47763330c..a1ae93664a 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -63,17 +63,6 @@ as short-form boolean values, and passed to plugins as 
> ``arg_name=on``.
>   However, short-form booleans are deprecated and full explicit ``arg_name=on``
>   form is preferred.
> 
> -
> -User-mode emulator command line arguments
> ------------------------------------------
> -
> -``-singlestep`` (since 8.1)
> -'''''''''''''''''''''''''''
> -
> -The ``-singlestep`` option has been given a name that better reflects
> -what it actually does. For both linux-user and bsd-user, use the
> -new ``-one-insn-per-tb`` option instead.
> -
>   QEMU Machine Protocol (QMP) commands
>   ------------------------------------
> 
> @@ -145,20 +134,6 @@ accepted incorrect commands will return an error. Users 
> should make sure that
>   all arguments passed to ``device_add`` are consistent with the documented
>   property types.
> 
> -``StatusInfo`` member ``singlestep`` (since 8.1)
> -''''''''''''''''''''''''''''''''''''''''''''''''
> -
> -The ``singlestep`` member of the ``StatusInfo`` returned from the
> -``query-status`` command is deprecated. This member has a confusing
> -name and it never did what the documentation claimed or what its name
> -suggests. We do not believe that anybody is actually using the
> -information provided in this member.
> -
> -The information it reports is whether the TCG JIT is in "one
> -instruction per translated block" mode (which can be set on the
> -command line or via the HMP, but not via QMP). The information remains
> -available via the HMP 'info jit' command.
> -
>   QEMU Machine Protocol (QMP) events
>   ----------------------------------
> 
> diff --git a/docs/user/main.rst b/docs/user/main.rst
> index f478635396..7e7ad07409 100644
> --- a/docs/user/main.rst
> +++ b/docs/user/main.rst
> @@ -98,9 +98,6 @@ Debug options:
>      This slows down emulation a lot, but can be useful in some situations,
>      such as when trying to analyse the logs produced by the ``-d`` option.
> 
> -``-singlestep``
> -   This is a deprecated synonym for the ``-one-insn-per-tb`` option.
> -
>   Environment variables:
> 
>   QEMU_STRACE
> @@ -251,6 +248,3 @@ Debug options:
>      Run the emulation with one guest instruction per translation block.
>      This slows down emulation a lot, but can be useful in some situations,
>      such as when trying to analyse the logs produced by the ``-d`` option.
> -
> -``-singlestep``
> -   This is a deprecated synonym for the ``-one-insn-per-tb`` option.
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index ca05502e0a..08bc99cb85 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
> @@ -106,25 +106,15 @@
>   #
>   # @running: true if all VCPUs are runnable, false if not runnable
>   #
> -# @singlestep: true if using TCG with one guest instruction per
> -#     translation block
> -#
>   # @status: the virtual machine @RunState
>   #
>   # Features:
>   #
> -# @deprecated: Member 'singlestep' is deprecated (with no
> -#     replacement).
> -#
>   # Since: 0.14
>   #
> -# Notes: @singlestep is enabled on the command line with '-accel
> -#     tcg,one-insn-per-tb=on', or with the HMP 'one-insn-per-tb'
> -#     command.
>   ##
>   { 'struct': 'StatusInfo',
>     'data': {'running': 'bool',
> -           'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]},
>              'status': 'RunState'} }

Uh, oh, that's a bigger change already ... can we safely remove the field 
here without upsetting 3rd party apps that rely on this interface?
Anyway, I withdraw my patch for -singlestep ... this should be handled by 
someone who's more familiar with these pieces of the code.

  Thomas