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

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 3/16/21 11:26 AM, Philippe Mathieu-Daudé wrote:
>> On 3/16/21 10:02 AM, Philippe Mathieu-Daudé wrote:
>>> 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.
>> 
>> This is actually quite interesting :) Accelerator definitions are
>> declared in config-target.h, but acceleration is host specific...
>
> Thomas, I guess I hit Claudio's reported bug again...
>
> 1/ generic libqemuutil.a is built without any CONFIG_accel definition.
>
> So this qapi-generated enum ... :
>
> typedef enum Accelerator {
>     ACCELERATOR_QTEST,
> #if defined(CONFIG_TCG)
>     ACCELERATOR_TCG,
> #endif /* defined(CONFIG_TCG) */
> #if defined(CONFIG_KVM)
>     ACCELERATOR_KVM,
> #endif /* defined(CONFIG_KVM) */
> #if defined(CONFIG_HAX)
>     ACCELERATOR_HAX,
> #endif /* defined(CONFIG_HAX) */
> #if defined(CONFIG_HVF)
>     ACCELERATOR_HVF,
> #endif /* defined(CONFIG_HVF) */
> #if defined(CONFIG_WHPX)
>     ACCELERATOR_WHPX,
> #endif /* defined(CONFIG_WHPX) */
> #if defined(CONFIG_XEN_BACKEND)
>     ACCELERATOR_XEN,
> #endif /* defined(CONFIG_XEN_BACKEND) */
>     ACCELERATOR__MAX,
> } Accelerator;
>
> ... is expanded to:
>
> typedef enum Accelerator {
>     ACCELERATOR_QTEST,
>     ACCELERATOR__MAX,
> } Accelerator;

CONFIG_KVM, CONFIG_TCG, ...  are defined in ${target}-config-target.h,
and may only be used in target-specific code.

If the enum ends up in libqemuutil.a, there are uses outside
target-specific code.

exec/poison.h lacks CONFIG_KVM, CONFIG_TCG, ...  Should they be added?

[...]