Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Eric Blake <eblake@redhat.com>]

On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
> Introduce the 'query-accels' QMP command which returns a list
> of built-in accelerators names.

accelerator names

> 
> - Accelerator is an QAPI enum of all existing accelerators,

is a QAPI enum

> 
> - AcceleratorInfo is a QAPI structure providing accelerator
>   specific information. Currently the common structure base
>   provides the name of the accelerator, while the specific
>   part is empty, but each accelerator can expand it.

Do we expand it later in this series?  If not,...

> 
> - 'query-accels' QMP command returns a list of @AcceleratorInfo
> 
> For example on a KVM-only build we get:
> 
>     { "execute": "query-accels" }
>     {
>         "return": [
>             {
>                 "type": "qtest"
>             },
>             {
>                 "type": "kvm"
>             }

Inconsistent with the code, already pointed out in other reviews.

>         ]
>     }
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---

> +##
> +# @AcceleratorInfo:
> +#
> +# Accelerator information.
> +#
> +# @name: The accelerator name.
> +#
> +# Since: 6.0
> +##
> +{ 'union': 'AcceleratorInfo',
> +  'base': {'name': 'Accelerator'},
> +  'discriminator': 'name',
> +  'data': { } }

...it feels a bit over-engineered (we can turn it into a union later
while still preserving back-compat).  On the other hand, since you are
using conditional compilation...

> +
> +##
> +# @query-accels:
> +#
> +# Get a list of AcceleratorInfo for all built-in accelerators.
> +#
> +# Returns: a list of @AcceleratorInfo describing each accelerator.
> +#
> +# Since: 6.0
> +#
> +# Example:
> +#
> +# -> { "execute": "query-accels" }
> +# <- { "return": [
> +#        {
> +#            "type": "qtest"
> +#        },
> +#        {
> +#            "type": "kvm"

Another inconsistent example.

> +#        }
> +#    ] }
> +#
> +##
> +{ 'command': 'query-accels',
> +  'returns': ['AcceleratorInfo'] }
> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
> new file mode 100644
> index 00000000000..f16e49b8956
> --- /dev/null
> +++ b/accel/accel-qmp.c
> @@ -0,0 +1,47 @@
> +/*
> + * QEMU accelerators, QMP commands
> + *
> + * Copyright (c) 2021 Red Hat Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/qapi-commands-machine.h"
> +
> +static const Accelerator accel_list[] = {
> +    ACCELERATOR_QTEST,
> +#ifdef CONFIG_TCG
> +    ACCELERATOR_TCG,
> +#endif
> +#ifdef CONFIG_KVM
> +    ACCELERATOR_KVM,
> +#endif

...would it be worth compiling the enum to only list enum values that
were actually compiled in?  That would change it to:

{ 'enum': 'Accelerator',
  'data': [ 'qtest',
            { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
...


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org