Re: [Qemu-devel] [PATCH v5 2/5] arm: qmp: add query-gic-capabilities interface

[Markus Armbruster]

Copying Eric in case further review is needed in my absence.

Peter Xu <address@hidden> writes:

> This patch adds the command "query-gic-capabilities" but not implemnet
> it. The command is ARM-only. Return of the command is a list of
> GICCapability struct that describes all GIC versions that current QEMU
> and system support.
>
> Signed-off-by: Peter Xu <address@hidden>
> ---
>  monitor.c                |  8 ++++++++
>  qapi-schema.json         | 11 +++++++++++
>  qmp-commands.hx          | 26 ++++++++++++++++++++++++++
>  scripts/qapi.py          |  1 +
>  target-arm/Makefile.objs |  2 +-
>  target-arm/monitor.c     | 31 +++++++++++++++++++++++++++++++
>  6 files changed, 78 insertions(+), 1 deletion(-)
>  create mode 100644 target-arm/monitor.c
>
> diff --git a/monitor.c b/monitor.c
> index 894f862..d463dc4 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4257,3 +4257,11 @@ void qmp_dump_skeys(const char *filename, Error **errp)
>      error_setg(errp, QERR_FEATURE_DISABLED, "dump-skeys");
>  }
>  #endif
> +
> +#ifndef TARGET_ARM
> +GICCapabilityList *qmp_query_gic_capabilities(Error **errp)
> +{
> +    error_setg(errp, QERR_FEATURE_DISABLED, "query-gic-capabilities");
> +    return NULL;
> +}
> +#endif
> diff --git a/qapi-schema.json b/qapi-schema.json
> index da9671a..b2ef149 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4156,3 +4156,14 @@
>    'data': { 'version': 'int',
>              'emulated': 'bool',
>              'kernel': 'bool' } }
> +
> +##
> +# @query-gic-capabilities:
> +#
> +# Return a list of supported GIC version capabilities.
> +#
> +# Returns: a list of GICCapability.
> +#
> +# Since: 2.6
> +##
> +{ 'command': 'query-gic-capabilities', 'returns': ['GICCapability'] }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 9e05365..a124ea8 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -4853,3 +4853,29 @@ Example:
>                   {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
>                    "pop-vlan": 1, "id": 251658240}
>     ]}
> +
> +EQMP
> +
> +#if defined TARGET_ARM
> +    {
> +        .name       = "query-gic-capabilities",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_query_gic_capabilities,
> +    },
> +#endif
> +
> +SQMP
> +query-gic-capabilities
> +---------------
> +
> +Return a list of supported ARM GIC versions and their capabilities.
> +
> +Arguments: None
> +
> +Example:
> +
> +-> { "execute": "query-gic-capabilities" }
> +<- { "return": [{ "version": 2, "emulated": true, "kernel": false },
> +                { "version": 3, "emulated": false, "kernel": true } ] }
> +
> +EQMP
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 6b2aa6e..716474e 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -46,6 +46,7 @@ returns_whitelist = [
   # Whitelist of commands allowed to return a non-dictionary
   returns_whitelist = [
       'human-monitor-command',
       'qom-get',
       'query-migrate-cache-size',
>      'query-tpm-models',
>      'query-tpm-types',
>      'ringbuf-read',
> +    'query-gic-capability',
>  
>      # From QGA:
>      'guest-file-open',

The whitelist exists to except existing commands from design rules on
return types.  New commands don't get to violate the rules without a
really, really compelling reason.

Do you actually need this?

If yes, why should your command be permitted to violate the design
rules?

> diff --git a/target-arm/Makefile.objs b/target-arm/Makefile.objs
> index a80eb39..334074c 100644
> --- a/target-arm/Makefile.objs
> +++ b/target-arm/Makefile.objs
> @@ -8,4 +8,4 @@ obj-y += translate.o op_helper.o helper.o cpu.o
>  obj-y += neon_helper.o iwmmxt_helper.o
>  obj-y += gdbstub.o
>  obj-$(TARGET_AARCH64) += cpu64.o translate-a64.o helper-a64.o gdbstub64.o
> -obj-y += crypto_helper.o
> +obj-y += crypto_helper.o monitor.o
> diff --git a/target-arm/monitor.c b/target-arm/monitor.c
> new file mode 100644
> index 0000000..5678eb8
> --- /dev/null
> +++ b/target-arm/monitor.c
> @@ -0,0 +1,31 @@
> +/*
> + * QEMU monitor.c for ARM.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +#include "qemu/osdep.h"
> +#include "hw/boards.h"
> +#include "qemu/error-report.h"
> +#include "sysemu/kvm.h"
> +#include "qmp-commands.h"

I very much doubt you need all these includes.  Try dropping all but the
first and the last one.

> +
> +GICCapabilityList *qmp_query_gic_capabilities(Error **errp)
> +{
> +    return NULL;
> +}