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

[Thomas Huth <thuth@redhat.com>]

On 16/03/2021 13.41, Markus Armbruster wrote:
> 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?

CONFIG_KVM is already in poison.h, and CONFIG_TCG cannot be added there 
since it is also defined in config-host.h. But the other accelerator 
switches should be marked as poisoned, too. I've got a patch for this in the 
works already - I'll send it out in a minute...

  Thomas