Re: [PATCH v3 10/10] hmp: Deprecate 'singlestep' member of StatusInfo

[Peter Maydell]

Markus -- could I trouble you for a quick review of this patch?
It's the only one in the series that touches QMP. Everything
else has been reviewed so otherwise this series is ready to go in.

thanks
-- PMM

On Mon, 17 Apr 2023 at 17:40, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> 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>
> ---
> For v3: because we're only deprecating the existing member,
> not trying to provide a replacement with a new name, we don't
> need to update the iotests that use the command. (We will when
> we eventually drop the deprecated member.)
> ---
>  docs/about/deprecated.rst | 14 ++++++++++++++
>  qapi/run-state.json       | 14 +++++++++++---
>  2 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 6f5e689aa45..d5eda0f566c 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.
> +
>  Human Monitor Protocol (HMP) commands
>  -------------------------------------
>
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index 9d34afa39e0..daf03a6fe9c 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
> @@ -104,16 +104,24 @@
>  #
>  # @running: true if all VCPUs are runnable, false if not runnable
>  #
> -# @singlestep: true if VCPUs are in single-step mode
> +# @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 through the GDB stub
> +# 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': 'bool', 'status': 'RunState'} }
> +  'data': {'running': 'bool',
> +           'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]},
> +           'status': 'RunState'} }
>
>  ##
>  # @query-status:
> --
> 2.34.1