Re: [PATCH v4 22/65] qapi: Move definitions related to accelerators in their own file

[Markus Armbruster <armbru@redhat.com>]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Extract TCG and KVM definitions from machine.json to accelerator.json.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  MAINTAINERS                |  1 +
>  qapi/accelerator.json      | 75 ++++++++++++++++++++++++++++++++++++++
>  qapi/machine.json          | 65 ---------------------------------
>  qapi/qapi-schema.json      |  1 +
>  accel/tcg/monitor.c        |  2 +-
>  hw/core/machine-hmp-cmds.c |  1 +
>  hw/core/machine-qmp-cmds.c |  1 +
>  qapi/meson.build           |  1 +
>  8 files changed, 81 insertions(+), 66 deletions(-)
>  create mode 100644 qapi/accelerator.json
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7128e0bc98e..5d6b337cef6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -507,6 +507,7 @@ F: accel/Makefile.objs
>  F: accel/stubs/Makefile.objs
>  F: cpu-common.c
>  F: cpu-target.c
> +F: qapi/accelerator.json
>  F: system/cpus.c
>  
>  Apple Silicon HVF CPUs
> diff --git a/qapi/accelerator.json b/qapi/accelerator.json
> new file mode 100644
> index 00000000000..1d2a83f1b22
> --- /dev/null
> +++ b/qapi/accelerator.json
> @@ -0,0 +1,75 @@
> +# -*- Mode: Python -*-
> +# vim: filetype=python
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +##
> +# = Accelerators
> +##

Going to conflict with John Snow's "[PATCH v3 4/5] docs/sphinx: remove
special parsing for freeform sections".  Don't worry about it.

> +
> +{ 'include': 'common.json' }
> +
> +##
> +# @KvmInfo:
> +#
> +# Information about support for KVM acceleration
> +#
> +# @enabled: true if KVM acceleration is active
> +#
> +# @present: true if KVM acceleration is built into this executable
> +#
> +# Since: 0.14
> +##
> +{ 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool'} }
> +
> +##
> +# @query-kvm:
> +#
> +# Return information about KVM acceleration
> +#
> +# Returns: @KvmInfo
> +#
> +# Since: 0.14
> +#
> +# .. qmp-example::
> +#
> +#     -> { "execute": "query-kvm" }
> +#     <- { "return": { "enabled": true, "present": true } }
> +##
> +{ 'command': 'query-kvm', 'returns': 'KvmInfo' }
> +
> +##
> +# @x-query-jit:
> +#
> +# Query TCG compiler statistics
> +#
> +# Features:
> +#
> +# @unstable: This command is meant for debugging.
> +#
> +# Returns: TCG compiler statistics
> +#
> +# Since: 6.2
> +##
> +{ 'command': 'x-query-jit',
> +  'returns': 'HumanReadableText',
> +  'if': 'CONFIG_TCG',
> +  'features': [ 'unstable' ] }
> +
> +##
> +# @x-query-opcount:
> +#
> +# Query TCG opcode counters
> +#
> +# Features:
> +#
> +# @unstable: This command is meant for debugging.
> +#
> +# Returns: TCG opcode counters
> +#
> +# Since: 6.2
> +##
> +{ 'command': 'x-query-opcount',
> +  'returns': 'HumanReadableText',
> +  'if': 'CONFIG_TCG',
> +  'features': [ 'unstable' ] }
> diff --git a/qapi/machine.json b/qapi/machine.json
> index d5bbb5e367e..e4713c405e8 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -454,35 +454,6 @@
>  ##
>  { 'command': 'inject-nmi' }
>  
> -##
> -# @KvmInfo:
> -#
> -# Information about support for KVM acceleration
> -#
> -# @enabled: true if KVM acceleration is active
> -#
> -# @present: true if KVM acceleration is built into this executable
> -#
> -# Since: 0.14
> -##
> -{ 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool'} }
> -
> -##
> -# @query-kvm:
> -#
> -# Return information about KVM acceleration
> -#
> -# Returns: @KvmInfo
> -#
> -# Since: 0.14
> -#
> -# .. qmp-example::
> -#
> -#     -> { "execute": "query-kvm" }
> -#     <- { "return": { "enabled": true, "present": true } }
> -##
> -{ 'command': 'query-kvm', 'returns': 'KvmInfo' }
> -
>  ##
>  # @NumaOptionsType:
>  #
> @@ -1729,24 +1700,6 @@
>    'returns': 'HumanReadableText',
>    'features': [ 'unstable' ] }
>  
> -##
> -# @x-query-jit:
> -#
> -# Query TCG compiler statistics
> -#
> -# Features:
> -#
> -# @unstable: This command is meant for debugging.
> -#
> -# Returns: TCG compiler statistics
> -#
> -# Since: 6.2
> -##
> -{ 'command': 'x-query-jit',
> -  'returns': 'HumanReadableText',
> -  'if': 'CONFIG_TCG',
> -  'features': [ 'unstable' ] }
> -
>  ##
>  # @x-query-numa:
>  #
> @@ -1764,24 +1717,6 @@
>    'returns': 'HumanReadableText',
>    'features': [ 'unstable' ] }
>  
> -##
> -# @x-query-opcount:
> -#
> -# Query TCG opcode counters
> -#
> -# Features:
> -#
> -# @unstable: This command is meant for debugging.
> -#
> -# Returns: TCG opcode counters
> -#
> -# Since: 6.2
> -##
> -{ 'command': 'x-query-opcount',
> -  'returns': 'HumanReadableText',
> -  'if': 'CONFIG_TCG',
> -  'features': [ 'unstable' ] }
> -
>  ##
>  # @x-query-ramblock:
>  #
> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> index a8f66163cb7..0477696ff02 100644
> --- a/qapi/qapi-schema.json
> +++ b/qapi/qapi-schema.json
> @@ -37,6 +37,7 @@
>  { 'include': 'run-state.json' }
>  { 'include': 'crypto.json' }
>  { 'include': 'job.json' }
> +{ 'include': 'accelerator.json' }
>  { 'include': 'block.json' }
>  { 'include': 'block-export.json' }
>  { 'include': 'char.json' }

This puts the new section "Accelerators" between "Background jobs" and
"Block devices".  Feels arbitrary.  Much of the existing order does.
Would it fit better next to "Machines"?

> diff --git a/accel/tcg/monitor.c b/accel/tcg/monitor.c
> index 1c182b6bfb5..5bdd837006c 100644
> --- a/accel/tcg/monitor.c
> +++ b/accel/tcg/monitor.c
> @@ -11,7 +11,7 @@
>  #include "qemu/qht.h"
>  #include "qapi/error.h"
>  #include "qapi/type-helpers.h"
> -#include "qapi/qapi-commands-machine.h"
> +#include "qapi/qapi-commands-accelerator.h"
>  #include "monitor/monitor.h"
>  #include "system/cpu-timers.h"
>  #include "exec/icount.h"
> diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
> index 65eeb5e9cc2..15ae5864d16 100644
> --- a/hw/core/machine-hmp-cmds.c
> +++ b/hw/core/machine-hmp-cmds.c
> @@ -18,6 +18,7 @@
>  #include "monitor/monitor.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-builtin-visit.h"
> +#include "qapi/qapi-commands-accelerator.h"
>  #include "qapi/qapi-commands-machine.h"
>  #include "qobject/qdict.h"
>  #include "qapi/string-output-visitor.h"
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index ab4fd1ec08a..f37fd220c2d 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -14,6 +14,7 @@
>  #include "hw/mem/memory-device.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-builtin-visit.h"
> +#include "qapi/qapi-commands-accelerator.h"
>  #include "qapi/qapi-commands-machine.h"
>  #include "qobject/qobject.h"
>  #include "qapi/qobject-input-visitor.h"

Have you considered extracting the accelerator command code to
accel/accel-qmp-cmds.c and accel/accel-hmp-cmds.c?

> diff --git a/qapi/meson.build b/qapi/meson.build
> index 3b035aea339..ca6b61a608d 100644
> --- a/qapi/meson.build
> +++ b/qapi/meson.build
> @@ -57,6 +57,7 @@ qapi_all_modules = [
>  ]
>  if have_system
>    qapi_all_modules += [
> +    'accelerator',
>      'acpi',
>      'audio',
>      'cryptodev',