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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 16/1/24 07:27, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
>> On Mon, Jan 15, 2024 at 05:39:19PM +0000, Peter Maydell wrote:
>>> On Mon, 15 Jan 2024 at 13:54, Thomas Huth <thuth@redhat.com> wrote:
>>>>
>>>> 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>
>>>
>>>>> StatusInfo::singlestep was deprecated at the same time,
>>>>> can we remove it?
>>>>>
>>>>> IOW could we complete your patch with this?
>>>
>>>>> 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?
>>>
>>> That was the whole point of marking it 'deprecated' in the JSON,
>>> I thought? We don't think anybody's using it, we've given fair
>>> warning, isn't the next step "remove it"? Markus, you're the
>>> expert on QAPI deprecations...
>>
>> Yes, it is fine to delete it without thinking further about possible usage,
>> unless someone steps forward quickly with new information that wasn't known
>> when the deprecation was added....
> 
> Concur.

Thanks all for the feedback.

Thomas, are you OK to post a v2 with the changes I suggested
or do you want me to do it?

> Supporting data:
> 
> commit 34c18203d472c5bf969ebd87dc06c7c3a957efc4
> Author: Peter Maydell <peter.maydell@linaro.org>
> Date:   Mon Apr 17 17:40:41 2023 +0100
> 
>      qmp: Deprecate 'singlestep' member of StatusInfo
>      
>      The 'singlestep' member of StatusInfo has never done what the QMP
>      documentation claims it does.  What it actually reports is whether
>      TCG is working in "one guest instruction per translation block" mode.
>      
>      We no longer need this field for the HMP 'info status' command, as
>      we've moved that information to 'info jit'.  It seems unlikely that
>      anybody is monitoring the state of this obscure TCG setting via QMP,
>      especially since QMP provides no means for changing the setting.  So
>      simply deprecate the field, without providing any replacement.
>      
>      Until we do eventually delete the member, correct the misstatements
>      in the QAPI documentation about it.
>      
>      If we do find that there are users for this, then the most likely way
>      we would provide replacement access to the information would be to
>      put the accelerator QOM object at a well-known path such as
>      /machine/accel, which could then be used with the existing qom-set
>      and qom-get commands.
>      
>      Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>      Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>      Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>      Reviewed-by: Markus Armbruster <armbru@redhat.com>
>      Message-id: 20230417164041.684562-11-peter.maydell@linaro.org
> 
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 6f5e689aa4..d5eda0f566 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -199,6 +199,20 @@ 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.
> +
> [...]
>