Can we unpoison CONFIG_FOO macros?

Can we unpoison CONFIG_FOO macros?

[Markus Armbruster]

We have a boatload of CONFIG_FOO macros that may only be used in
target-dependent code.  We use generated config-poison.h to enforce.

This is a bit annoying in the QAPI schema.  Let me demonstrate with an
example: QMP commands query-rocker, query-rocker-ports, and so forth.
These commands are useful only with "rocker" devices.  They are
compile-time optional.  hw/net/Kconfig:

    config ROCKER
        bool
        default y if PCI_DEVICES
        depends on PCI && MSI_NONBROKEN

The rocker device and QMP code is actually target-independent:
hw/net/meson.build puts it into softmmu_ss.

Disabling the "rocker" device type ideally disables the rocker QMP
commands, too.  Should be easy enough: 'if': 'CONFIG_FOO' in the QAPI
schema.

Except that makes the entire code QAPI generates for rocker.json
device-dependent: it now contains #if defined(CONFIG_ROCKER), and
CONFIG_ROCKER is poisoned.  The rocker code implementing monitor
commands also becomes device-dependent, because it includes generated
headers.  We compile all that per target for no sane reason at all.
That's why we don't actually disable the commands.

Not disabling them creates another problem: we have the commands always,
but their implementation depends on CONFIG_ROCKER.  So we provide stubs
that always fail for use when CONFIG_ROCKER is off.  Drawbacks: we
generate, compile and link useless code, and QAPI/QMP introspection is
less useful than it could be.

This isn't terrible.  It still annoys me.  I wonder whether Philippe's
work on having a single qemu-system binary could improve things here.

Comments?

Re: Can we unpoison CONFIG_FOO macros?

[Peter Maydell]

On Tue, 7 Feb 2023 at 15:41, Markus Armbruster <armbru@redhat.com> wrote:
> We have a boatload of CONFIG_FOO macros that may only be used in
> target-dependent code.  We use generated config-poison.h to enforce.
>
> This is a bit annoying in the QAPI schema.  Let me demonstrate with an
> example: QMP commands query-rocker, query-rocker-ports, and so forth.
> These commands are useful only with "rocker" devices.  They are
> compile-time optional.  hw/net/Kconfig:
>
>     config ROCKER
>         bool
>         default y if PCI_DEVICES
>         depends on PCI && MSI_NONBROKEN
>
> The rocker device and QMP code is actually target-independent:
> hw/net/meson.build puts it into softmmu_ss.
>
> Disabling the "rocker" device type ideally disables the rocker QMP
> commands, too.  Should be easy enough: 'if': 'CONFIG_FOO' in the QAPI
> schema.
>
> Except that makes the entire code QAPI generates for rocker.json
> device-dependent: it now contains #if defined(CONFIG_ROCKER), and
> CONFIG_ROCKER is poisoned.  The rocker code implementing monitor
> commands also becomes device-dependent, because it includes generated
> headers.  We compile all that per target for no sane reason at all.
> That's why we don't actually disable the commands.
>
> Not disabling them creates another problem: we have the commands always,
> but their implementation depends on CONFIG_ROCKER.  So we provide stubs
> that always fail for use when CONFIG_ROCKER is off.  Drawbacks: we
> generate, compile and link useless code, and QAPI/QMP introspection is
> less useful than it could be.

If you want the introspection to be useful, then you need to
make the appearance of the commands depend on what machine
type and devices are created on the command line. There are
lots of machine types where the rocker commands are irrelevant
because they don't apply to that machine even though it happens
that PCI_DEVICES got built into that QEMU executable.

I think the underlying question is "what does it mean to be
only building in a QMP command when a Kconfig value is set?".
It doesn't mean "this command only appears when it's useful",
so anybody introspecting with QMP has to handle the "command
exists but doesn't do anything helpful" case anyway. My guess
is that the check you're trying to do at compile time ought
to be done at runtime somehow instead (which is a general
theme for 'single system emulation executable' work).

thanks
-- PMM

Re: Can we unpoison CONFIG_FOO macros?

[Thomas Huth]

On 07/02/2023 16.39, Markus Armbruster wrote:
> We have a boatload of CONFIG_FOO macros that may only be used in
> target-dependent code.  We use generated config-poison.h to enforce.
> 
> This is a bit annoying in the QAPI schema.  Let me demonstrate with an
> example: QMP commands query-rocker, query-rocker-ports, and so forth.
> These commands are useful only with "rocker" devices.  They are
> compile-time optional.  hw/net/Kconfig:
> 
>      config ROCKER
>          bool
>          default y if PCI_DEVICES
>          depends on PCI && MSI_NONBROKEN
> 
> The rocker device and QMP code is actually target-independent:
> hw/net/meson.build puts it into softmmu_ss.
> 
> Disabling the "rocker" device type ideally disables the rocker QMP
> commands, too.  Should be easy enough: 'if': 'CONFIG_FOO' in the QAPI
> schema.
> 
> Except that makes the entire code QAPI generates for rocker.json
> device-dependent: it now contains #if defined(CONFIG_ROCKER), and
> CONFIG_ROCKER is poisoned.  The rocker code implementing monitor
> commands also becomes device-dependent, because it includes generated
> headers.  We compile all that per target for no sane reason at all.

Well, CONFIG_ROCKER depends on the target config, so that is a sane reason 
in my eyes. Depending on which target you currently compile, it's either set 
or not set - there is no reasonable way to compile generic code and still 
test the CONFIG_ROCKER macro.

So adding such a switch to rocker.json itself just cannot work.

But at the "make" level, we distinguish between config-all-devices.mak and 
*-softmmu-config-devices.mak , so if you want to disable the rocker QMP code 
depending on whether CONFIG_ROCKER is set in *any* target binary or not, you 
would need a similar mechanism to config-all-devices.mak here, e.g. 
something that would switch the inclusion of rocker.json on or off depending 
on the switch in config-all-devices.mak ...
Would it be possible to add such a switch to qapi/meson.build ?
... and to qapi/qapi-schema.json, I guess, since the rocker.json gets 
included from there? Something like that:

diff --git a/qapi/meson.build b/qapi/meson.build
--- a/qapi/meson.build
+++ b/qapi/meson.build
@@ -59,9 +59,11 @@ if have_system
      'qdev',
      'pci',
      'rdma',
-    'rocker',
      'tpm',
    ]
+  if config_all_devices.has_key('CONFIG_ROCKER')
+    qapi_all_modules += [ 'rocker', ]
+  endif
  endif
  if have_system or have_tools
    qapi_all_modules += [
diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -72,7 +72,7 @@
  { 'include': 'job.json' }
  { 'include': 'net.json' }
  { 'include': 'rdma.json' }
-{ 'include': 'rocker.json' }
+{ 'include': 'rocker.json', 'if': 'CONFIG_ROCKER' }
  { 'include': 'tpm.json' }
  { 'include': 'ui.json' }
  { 'include': 'authz.json' }

No clue whether the qapi parser could be extended like that, though...

  Thomas

Re: Can we unpoison CONFIG_FOO macros?

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 7 Feb 2023 at 15:41, Markus Armbruster <armbru@redhat.com> wrote:
>> We have a boatload of CONFIG_FOO macros that may only be used in
>> target-dependent code.  We use generated config-poison.h to enforce.
>>
>> This is a bit annoying in the QAPI schema.  Let me demonstrate with an
>> example: QMP commands query-rocker, query-rocker-ports, and so forth.
>> These commands are useful only with "rocker" devices.  They are
>> compile-time optional.  hw/net/Kconfig:
>>
>>     config ROCKER
>>         bool
>>         default y if PCI_DEVICES
>>         depends on PCI && MSI_NONBROKEN
>>
>> The rocker device and QMP code is actually target-independent:
>> hw/net/meson.build puts it into softmmu_ss.
>>
>> Disabling the "rocker" device type ideally disables the rocker QMP
>> commands, too.  Should be easy enough: 'if': 'CONFIG_FOO' in the QAPI
>> schema.
>>
>> Except that makes the entire code QAPI generates for rocker.json
>> device-dependent: it now contains #if defined(CONFIG_ROCKER), and
>> CONFIG_ROCKER is poisoned.  The rocker code implementing monitor
>> commands also becomes device-dependent, because it includes generated
>> headers.  We compile all that per target for no sane reason at all.
>> That's why we don't actually disable the commands.
>>
>> Not disabling them creates another problem: we have the commands always,
>> but their implementation depends on CONFIG_ROCKER.  So we provide stubs
>> that always fail for use when CONFIG_ROCKER is off.  Drawbacks: we
>> generate, compile and link useless code, and QAPI/QMP introspection is
>> less useful than it could be.
>
> If you want the introspection to be useful, then you need to
> make the appearance of the commands depend on what machine
> type and devices are created on the command line. There are
> lots of machine types where the rocker commands are irrelevant
> because they don't apply to that machine even though it happens
> that PCI_DEVICES got built into that QEMU executable.
>
> I think the underlying question is "what does it mean to be
> only building in a QMP command when a Kconfig value is set?".
> It doesn't mean "this command only appears when it's useful",
> so anybody introspecting with QMP has to handle the "command
> exists but doesn't do anything helpful" case anyway. My guess
> is that the check you're trying to do at compile time ought
> to be done at runtime somehow instead (which is a general
> theme for 'single system emulation executable' work).

For better or worse, QAPI/QMP introspection is compile-time static.
It's oblivious of run-time configuration and state.

You point out that the question "is rocker built into this binary" isn't
particularly interesting.  You're right.

Still, it's a kind of question static introspection *should* be able to
answer.  Due to the way our compile time configuration machinery works,
QAPI/QMP introspection can't actually answer it, and that irks me.

For the same reason, we can't fully disable things like rocker: the
generated QAPI code remains along with command stubs.  Irks me, too.

Neither is a serious problem for us as far as I can tell.  As I wrote:

> This isn't terrible.  It still annoys me.  I wonder whether Philippe's
> work on having a single qemu-system binary could improve things here.

Re: Can we unpoison CONFIG_FOO macros?

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 07/02/2023 16.39, Markus Armbruster wrote:
>> We have a boatload of CONFIG_FOO macros that may only be used in
>> target-dependent code.  We use generated config-poison.h to enforce.
>> This is a bit annoying in the QAPI schema.  Let me demonstrate with an
>> example: QMP commands query-rocker, query-rocker-ports, and so forth.
>> These commands are useful only with "rocker" devices.  They are
>> compile-time optional.  hw/net/Kconfig:
>>      config ROCKER
>>          bool
>>          default y if PCI_DEVICES
>>          depends on PCI && MSI_NONBROKEN
>> The rocker device and QMP code is actually target-independent:
>> hw/net/meson.build puts it into softmmu_ss.
>> Disabling the "rocker" device type ideally disables the rocker QMP
>> commands, too.  Should be easy enough: 'if': 'CONFIG_FOO' in the QAPI
>> schema.
>> Except that makes the entire code QAPI generates for rocker.json
>> device-dependent: it now contains #if defined(CONFIG_ROCKER), and
>> CONFIG_ROCKER is poisoned.  The rocker code implementing monitor
>> commands also becomes device-dependent, because it includes generated
>> headers.  We compile all that per target for no sane reason at all.
>
> Well, CONFIG_ROCKER depends on the target config, so that is a sane reason in my eyes. Depending on which target you currently compile, it's either set or not set - there is no reasonable way to compile generic code and still test the CONFIG_ROCKER macro.
>
> So adding such a switch to rocker.json itself just cannot work.
>
> But at the "make" level, we distinguish between config-all-devices.mak and *-softmmu-config-devices.mak , so if you want to disable the rocker QMP code depending on whether CONFIG_ROCKER is set in *any* target binary or not, you would need a similar mechanism to config-all-devices.mak here, e.g. something that would switch the inclusion of rocker.json on or off depending on the switch in config-all-devices.mak ...
> Would it be possible to add such a switch to qapi/meson.build ?
> ... and to qapi/qapi-schema.json, I guess, since the rocker.json gets included from there? Something like that:
>
> diff --git a/qapi/meson.build b/qapi/meson.build
> --- a/qapi/meson.build
> +++ b/qapi/meson.build
> @@ -59,9 +59,11 @@ if have_system
>      'qdev',
>      'pci',
>      'rdma',
> -    'rocker',
>      'tpm',
>    ]
> +  if config_all_devices.has_key('CONFIG_ROCKER')
> +    qapi_all_modules += [ 'rocker', ]
> +  endif
>  endif
>  if have_system or have_tools
>    qapi_all_modules += [
> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> --- a/qapi/qapi-schema.json
> +++ b/qapi/qapi-schema.json
> @@ -72,7 +72,7 @@
>  { 'include': 'job.json' }
>  { 'include': 'net.json' }
>  { 'include': 'rdma.json' }
> -{ 'include': 'rocker.json' }
> +{ 'include': 'rocker.json', 'if': 'CONFIG_ROCKER' }
>  { 'include': 'tpm.json' }
>  { 'include': 'ui.json' }
>  { 'include': 'authz.json' }
>
> No clue whether the qapi parser could be extended like that, though...

Feels feasible.

I had vague ideas going this direction, too.  Thanks for fleshing them
out for me!

Hmm, we have somewhat hairy code in qapi/meson.build to enumerate the
QAPI modules and generated source files, and to add them to the right
source sets.  Perhaps we could have qapi-gen generate some of that.