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

[Markus Armbruster]

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.

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.
+
[...]