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

[Thomas Huth]

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