| Message ID | 20240806141940.22095-3-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/intc/arm_gic: Only provide query-gic-capabilities when GIC built-in | expand |
On 8/7/24 00:19, Philippe Mathieu-Daudé wrote: > 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(-) Ah, nevermind my final question for patch 1. Entire series: Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
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? > #include "qapi/qapi-commands-misc-target.h" > #include "kvm_arm.h" > > diff --git a/hw/intc/meson.build b/hw/intc/meson.build > index 45d3503d49..b9550967e2 100644 > --- a/hw/intc/meson.build > +++ b/hw/intc/meson.build > @@ -39,7 +39,7 @@ if config_all_devices.has_key('CONFIG_APIC') or \ > endif > > specific_ss.add(when: 'CONFIG_APIC', if_true: files('apic.c', 'apic_common.c')) > -specific_ss.add(when: 'CONFIG_ARM', if_true: files('arm_gic_qmp.c')) > +specific_ss.add(when: 'CONFIG_ARM_GIC', if_true: files('arm_gic_qmp.c')) > specific_ss.add(when: 'CONFIG_ARM_GIC', if_true: files('arm_gicv3_cpuif_common.c')) > specific_ss.add(when: 'CONFIG_ARM_GICV3_TCG', if_true: files('arm_gicv3_cpuif.c')) > specific_ss.add(when: 'CONFIG_ARM_GIC_KVM', if_true: files('arm_gic_kvm.c'))
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. > >> #include "qapi/qapi-commands-misc-target.h" >> #include "kvm_arm.h" >> >> diff --git a/hw/intc/meson.build b/hw/intc/meson.build >> index 45d3503d49..b9550967e2 100644 >> --- a/hw/intc/meson.build >> +++ b/hw/intc/meson.build >> @@ -39,7 +39,7 @@ if config_all_devices.has_key('CONFIG_APIC') or \ >> endif >> >> specific_ss.add(when: 'CONFIG_APIC', if_true: files('apic.c', 'apic_common.c')) >> -specific_ss.add(when: 'CONFIG_ARM', if_true: files('arm_gic_qmp.c')) >> +specific_ss.add(when: 'CONFIG_ARM_GIC', if_true: files('arm_gic_qmp.c')) >> specific_ss.add(when: 'CONFIG_ARM_GIC', if_true: files('arm_gicv3_cpuif_common.c')) >> specific_ss.add(when: 'CONFIG_ARM_GICV3_TCG', if_true: files('arm_gicv3_cpuif.c')) >> specific_ss.add(when: 'CONFIG_ARM_GIC_KVM', if_true: files('arm_gic_kvm.c')) >
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? [...]
On Wed, 7 Aug 2024 at 12:10, Markus Armbruster <armbru@redhat.com> wrote: > 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? The stuff in CONFIG_DEVICES is target-specific, so wanting to include it should be rare (currently we include it in only about 25 files). Any file that includes it has to be a compile-per-target file, and generally we'd rather avoid that. Plus it's a bit odd to need to change code based on whether some other device was configured into the system, so I think that's something worth restricting to only files that effectively opt in to it. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Wed, 7 Aug 2024 at 12:10, Markus Armbruster <armbru@redhat.com> wrote: >> 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? > > The stuff in CONFIG_DEVICES is target-specific, so wanting > to include it should be rare (currently we include it in > only about 25 files). Any file that includes it has to be > a compile-per-target file, and generally we'd rather avoid that. Since all the macros defined in CONFIG_DEVICES are poisoned by exec/poison.h, which *is* included by qemu/osdep.h, target-independent files never need to include CONFIG_DEVICES. My question was strictly about target-dependent files, i.e. exactly the ones that include CONFIG_TARGET. > Plus it's a bit odd to need to change code based on whether > some other device was configured into the system, I agree it's a bit odd for device code to check whether some other device is also selected for the current target. But is it odd for the QAPI schema to define a device-specific command only when the device is selected? > so I think > that's something worth restricting to only files that effectively > opt in to it. Is accidental use of a macro from CONFIG_DEVICES a risk? Is the risk mitigated to some useful degree by having to include CONFIG_DEVICES? I consider the combination of testing configuration symbols with #ifdef and requiring a manual #include to actually get the symbols (and make the #ifdef work) bad practice. Options: 1. Approximate symbols from CONFIG_DEVICES with symbols from CONFIG_TARGET. This is what we do now. 2. Use symbols from CONFIG_DEVICES. Generated headers are no longer self-contained. Strong dislike. 3. Define device-specific stuff unconditionally. We get to fool around with stubs, binaries carry more useless code, and introspection becomes less useful. Meh. Thoughts?
On Thu, 8 Aug 2024 at 06:35, Markus Armbruster <armbru@redhat.com> wrote: > Options: > > 1. Approximate symbols from CONFIG_DEVICES with symbols from > CONFIG_TARGET. This is what we do now. > > 2. Use symbols from CONFIG_DEVICES. Generated headers are no longer > self-contained. Strong dislike. > > 3. Define device-specific stuff unconditionally. We get to fool around > with stubs, binaries carry more useless code, and introspection > becomes less useful. Meh. I think that 3 is the way to go. With a single-QEMU-executable there are going to be lots and lots of cases where a QAPI command needs to be present in the binary but won't be applicable for the particular machine type / target architecture currently being emulated. So "the commands always exist but they might give you a suitable error if they're not relevant for the config you're currently running" is more consistent. Regarding introspection, doing it based on "we didn't put this in at compile time" is going to give you inaccurate results: even if a command was compiled in doesn't mean it's going to be relevant to what you want to run. -- PMM
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 #include "qapi/qapi-commands-misc-target.h" #include "kvm_arm.h" diff --git a/hw/intc/meson.build b/hw/intc/meson.build index 45d3503d49..b9550967e2 100644 --- a/hw/intc/meson.build +++ b/hw/intc/meson.build @@ -39,7 +39,7 @@ if config_all_devices.has_key('CONFIG_APIC') or \ endif specific_ss.add(when: 'CONFIG_APIC', if_true: files('apic.c', 'apic_common.c')) -specific_ss.add(when: 'CONFIG_ARM', if_true: files('arm_gic_qmp.c')) +specific_ss.add(when: 'CONFIG_ARM_GIC', if_true: files('arm_gic_qmp.c')) specific_ss.add(when: 'CONFIG_ARM_GIC', if_true: files('arm_gicv3_cpuif_common.c')) specific_ss.add(when: 'CONFIG_ARM_GICV3_TCG', if_true: files('arm_gicv3_cpuif.c')) specific_ss.add(when: 'CONFIG_ARM_GIC_KVM', if_true: files('arm_gic_kvm.c'))
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(-)