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

[Philippe Mathieu-Daudé]

On 3/16/21 1:48 PM, Thomas Huth wrote:
> 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.

Maybe we hit "exec/poison.h" limit. Maybe it is too wide / generic,
and need split, or multiple ones.

Should we redefine on which code area we want these definitions
poisoned?

AFAIK accel/ is not target specific but host specific.

My list of area / useful to poison:

- user-mode
  . non-tcg accel
  . hardware emulation

- generic hardware emulation
  . target specific
  . accel specific

For this one it is already too late:

- target acceleration
  . hardware emulation

Thoughts?