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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 3/16/21 7:51 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
> [...]
>>> 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)' },
>> ...

These accelerator definitions are supposed to be poisoned in generic
code... But I like the simplicity of your suggestion, so I'll give it
a try and see what happens with removing the poisoned definitions.

> Makes introspection more useful.  Management applications can get the
> information the list of compiled-in accelerators from query-qmp-schema.
> They don't have to be taught to use query-accels.
> 
> In fact, query-accels becomes useless except as a tool to force
> visibility of Accelerator in query-qmp-schema.  We wouldn't have to
> force if we had CLI introspection that shows the type of -accel's
> parameter @accel.  Adding a query command is a common work-around for
> our anemic CLI introspection capabilities.
> 
> The query command could be made more useful than introspection if it
> reflected run time state, i.e. it showed an accelerator only when the
> host system actually supports it.  Can't say how practical that would
> be.
> 
>>>
>>> +AcceleratorInfoList *qmp_query_accels(Error **errp)
>>> +{
>>> +    AcceleratorInfoList *list = NULL, **tail = &list;
>>> +
>>> +    for (unsigned i = 0; i < ARRAY_SIZE(accel_list); i++) {
>>> +        AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);
>>> +
>>> +        info->name = accel_list[i];
>>> +
>>> +        QAPI_LIST_APPEND(tail, info);
>>> +    }
>>> +
>>> +    return list;
>>> +}
> 
> You could then use something like
> 
>         for (accel = 0; accel < ACCELERATOR__MAX; accel++) {
>             AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);
> 
>             info->name = Accelerator_str(accel);
> 
>             QAPI_LIST_APPEND(tail, info);
>         }
>