Re: [RFC PATCH-for-9.1? 2/2] hw/intc/arm_gic: Only provide query-gic-capabilities when GIC built-in

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 7/8/24 10:18, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>> 
>>> When configuring QEMU with --without-default-devices and
>>> not including machines using a GIC, the GIC model is not
>>> built in but the 'query-gic-capabilities' command still
>>> returns false hopes about GIC:
>>>
>>>    {"execute": "query-gic-capabilities"}
>>>    {"return": [{"emulated": true, "version": 3, "kernel": false}, 
>>> {"emulated": true, "version": 2, "kernel": false}]}
>>>
>>> Restrict the command to when the GIC is available. If it
>>> isn't we'll get:
>>>
>>>    { "execute": "query-gic-capabilities" }
>>>    {"error": {"class": "CommandNotFound", "desc": "The command 
>>> query-gic-capabilities has not been found"}}
>>>
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2484
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   qapi/misc-target.json | 4 ++--
>>>   hw/intc/arm_gic_qmp.c | 2 ++
>>>   hw/intc/meson.build   | 2 +-
>>>   3 files changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
>>> index 8d70bd24d8..b857e44c2e 100644
>>> --- a/qapi/misc-target.json
>>> +++ b/qapi/misc-target.json
>>> @@ -316,7 +316,7 @@
>>>     'data': { 'version': 'int',
>>>               'emulated': 'bool',
>>>               'kernel': 'bool' },
>>> -  'if': 'TARGET_ARM' }
>>> +  'if': 'CONFIG_ARM_GIC' }
>>>   
>>>   ##
>>>   # @query-gic-capabilities:
>>> @@ -335,7 +335,7 @@
>>>   #                     { "version": 3, "emulated": false, "kernel": true } 
>>> ] }
>>>   ##
>>>   { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'],
>>> -  'if': 'TARGET_ARM' }
>>> +  'if': 'CONFIG_ARM_GIC' }
>>>   
>>>   ##
>>>   # @SGXEPCSection:
>>> diff --git a/hw/intc/arm_gic_qmp.c b/hw/intc/arm_gic_qmp.c
>>> index 71056a0c10..1fc79c775b 100644
>>> --- a/hw/intc/arm_gic_qmp.c
>>> +++ b/hw/intc/arm_gic_qmp.c
>>> @@ -6,6 +6,8 @@
>>>   
>>>   #include "qemu/osdep.h"
>>>   #include "qapi/util.h"
>>> +
>>> +#include CONFIG_DEVICES
>> 
>> Uh, why do we need this now?
>
> Now qapi-commands-misc-target.h is generated guarded with
> '#ifdef CONFIG_ARM_GIC', and CONFIG_ARM_GIC is defined per
> target in CONFIG_DEVICES.
>
> I'll update the patch description, but does this makes
> sense to you? QAPI headers don't include headers defining
> guards, we have to include them manually where we use QAPI
> headers.

Hmm.  Then the generated headers aren't self-contained anymore.

Having to manually include a configuration header like CONFIG_DEVICES
wherever you use configuration symbols strikes me as unadvisable when
uses include checking for definedness, such as #ifdef: silent miscompile
when you forget to include.

This is why Autoconf wants you to include config.h first in any .c: it
makes #ifdef & friends safe.

qemu/osdep.h does include some configuration headers:

    #include "config-host.h"
    #ifdef COMPILING_PER_TARGET
    #include CONFIG_TARGET
    #else
    #include "exec/poison.h"
    #endif

Why not CONFIG_DEVICES?

[...]