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

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

[Philippe Mathieu-Daudé]

Attempt to clarify confuse output from the
'query-gic-capabilities' QMP command when
no GIC devices are built in.

Philippe Mathieu-Daudé (2):
  target/arm: Move qmp_query_gic_capabilities() to hw/intc/
  hw/intc/arm_gic: Only provide query-gic-capabilities when GIC built-in

 qapi/misc-target.json     |  4 +--
 hw/intc/arm_gic_qmp.c     | 61 +++++++++++++++++++++++++++++++++++++++
 target/arm/arm-qmp-cmds.c | 52 +--------------------------------
 hw/intc/meson.build       |  1 +
 4 files changed, 65 insertions(+), 53 deletions(-)
 create mode 100644 hw/intc/arm_gic_qmp.c

-- 
2.45.2

[RFC PATCH-for-9.1? 1/2] target/arm: Move qmp_query_gic_capabilities() to hw/intc/

[Philippe Mathieu-Daudé]

qmp_query_gic_capabilities() is not specific to the ARM
architecture but to the GIC device which is modelled in
hw/intc/, so move the code there for clarity. No logical
change intended.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/intc/arm_gic_qmp.c     | 59 +++++++++++++++++++++++++++++++++++++++
 target/arm/arm-qmp-cmds.c | 52 +---------------------------------
 hw/intc/meson.build       |  1 +
 3 files changed, 61 insertions(+), 51 deletions(-)
 create mode 100644 hw/intc/arm_gic_qmp.c

diff --git a/hw/intc/arm_gic_qmp.c b/hw/intc/arm_gic_qmp.c
new file mode 100644
index 0000000000..71056a0c10
--- /dev/null
+++ b/hw/intc/arm_gic_qmp.c
@@ -0,0 +1,59 @@
+/*
+ * QEMU ARM GIC QMP command
+ *
+ * SPDX-License-Identifier: MIT
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/util.h"
+#include "qapi/qapi-commands-misc-target.h"
+#include "kvm_arm.h"
+
+static GICCapability *gic_cap_new(int version)
+{
+    GICCapability *cap = g_new0(GICCapability, 1);
+    cap->version = version;
+    /* by default, support none */
+    cap->emulated = false;
+    cap->kernel = false;
+    return cap;
+}
+
+static inline void gic_cap_kvm_probe(GICCapability *v2, GICCapability *v3)
+{
+#ifdef CONFIG_KVM
+    int fdarray[3];
+
+    if (!kvm_arm_create_scratch_host_vcpu(NULL, fdarray, NULL)) {
+        return;
+    }
+
+    /* Test KVM GICv2 */
+    if (kvm_device_supported(fdarray[1], KVM_DEV_TYPE_ARM_VGIC_V2)) {
+        v2->kernel = true;
+    }
+
+    /* Test KVM GICv3 */
+    if (kvm_device_supported(fdarray[1], KVM_DEV_TYPE_ARM_VGIC_V3)) {
+        v3->kernel = true;
+    }
+
+    kvm_arm_destroy_scratch_host_vcpu(fdarray);
+#endif
+}
+
+GICCapabilityList *qmp_query_gic_capabilities(Error **errp)
+{
+    GICCapabilityList *head = NULL;
+    GICCapability *v2 = gic_cap_new(2), *v3 = gic_cap_new(3);
+
+    v2->emulated = true;
+    v3->emulated = true;
+
+    gic_cap_kvm_probe(v2, v3);
+
+    QAPI_LIST_PREPEND(head, v2);
+    QAPI_LIST_PREPEND(head, v3);
+
+    return head;
+}
diff --git a/target/arm/arm-qmp-cmds.c b/target/arm/arm-qmp-cmds.c
index 3cc8cc738b..3303c71b21 100644
--- a/target/arm/arm-qmp-cmds.c
+++ b/target/arm/arm-qmp-cmds.c
@@ -22,64 +22,14 @@
 
 #include "qemu/osdep.h"
 #include "hw/boards.h"
-#include "kvm_arm.h"
+#include "sysemu/kvm.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/qapi-commands-machine-target.h"
-#include "qapi/qapi-commands-misc-target.h"
 #include "qapi/qmp/qdict.h"
 #include "qom/qom-qobject.h"
 
-static GICCapability *gic_cap_new(int version)
-{
-    GICCapability *cap = g_new0(GICCapability, 1);
-    cap->version = version;
-    /* by default, support none */
-    cap->emulated = false;
-    cap->kernel = false;
-    return cap;
-}
-
-static inline void gic_cap_kvm_probe(GICCapability *v2, GICCapability *v3)
-{
-#ifdef CONFIG_KVM
-    int fdarray[3];
-
-    if (!kvm_arm_create_scratch_host_vcpu(NULL, fdarray, NULL)) {
-        return;
-    }
-
-    /* Test KVM GICv2 */
-    if (kvm_device_supported(fdarray[1], KVM_DEV_TYPE_ARM_VGIC_V2)) {
-        v2->kernel = true;
-    }
-
-    /* Test KVM GICv3 */
-    if (kvm_device_supported(fdarray[1], KVM_DEV_TYPE_ARM_VGIC_V3)) {
-        v3->kernel = true;
-    }
-
-    kvm_arm_destroy_scratch_host_vcpu(fdarray);
-#endif
-}
-
-GICCapabilityList *qmp_query_gic_capabilities(Error **errp)
-{
-    GICCapabilityList *head = NULL;
-    GICCapability *v2 = gic_cap_new(2), *v3 = gic_cap_new(3);
-
-    v2->emulated = true;
-    v3->emulated = true;
-
-    gic_cap_kvm_probe(v2, v3);
-
-    QAPI_LIST_PREPEND(head, v2);
-    QAPI_LIST_PREPEND(head, v3);
-
-    return head;
-}
-
 QEMU_BUILD_BUG_ON(ARM_MAX_VQ > 16);
 
 /*
diff --git a/hw/intc/meson.build b/hw/intc/meson.build
index afd1aa51ee..45d3503d49 100644
--- a/hw/intc/meson.build
+++ b/hw/intc/meson.build
@@ -39,6 +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_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'))
-- 
2.45.2

Re: [RFC PATCH-for-9.1? 1/2] target/arm: Move qmp_query_gic_capabilities() to hw/intc/

[Richard Henderson]

On 8/7/24 00:19, Philippe Mathieu-Daudé wrote:
> qmp_query_gic_capabilities() is not specific to the ARM
> architecture but to the GIC device which is modelled in
> hw/intc/, so move the code there for clarity.

But the GIC is certainly arm architecture specific.
It's built into the CPU, and shares state.

The fact that it's modeled in hw/intc/ and not in target/arm/ has always been a needle in 
the side, though it seems there are no good options.

> @@ -39,6 +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_gicv3_cpuif_common.c'))

Is it more or less confusing that you're not using CONFIG_ARM_GIC, for something that is 
GIC related?


r~

Re: [RFC PATCH-for-9.1? 1/2] target/arm: Move qmp_query_gic_capabilities() to hw/intc/

[Philippe Mathieu-Daudé]

On 7/8/24 05:46, Richard Henderson wrote:
> On 8/7/24 00:19, Philippe Mathieu-Daudé wrote:
>> qmp_query_gic_capabilities() is not specific to the ARM
>> architecture but to the GIC device which is modelled in
>> hw/intc/, so move the code there for clarity.
> 
> But the GIC is certainly arm architecture specific.
> It's built into the CPU, and shares state.

Yes... but there are also SoC with ARM cores, GIC and non-ARM cores ;)
Example: ZynqMP with MicroBlaze cores.

> The fact that it's modeled in hw/intc/ and not in target/arm/ has always 
> been a needle in the side, though it seems there are no good options.
> 
>> @@ -39,6 +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_gicv3_cpuif_common.c'))
> 
> Is it more or less confusing that you're not using CONFIG_ARM_GIC, for 
> something that is GIC related?

(You figured in the next patch) this commit aims to be "no logical
change" to indeed use CONFIG_ARM_GIC in the next (simpler) patch.

Re: [RFC PATCH-for-9.1? 1/2] target/arm: Move qmp_query_gic_capabilities() to hw/intc/

[Peter Maydell]

On Wed, 7 Aug 2024 at 04:46, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/7/24 00:19, Philippe Mathieu-Daudé wrote:
> > qmp_query_gic_capabilities() is not specific to the ARM
> > architecture but to the GIC device which is modelled in
> > hw/intc/, so move the code there for clarity.
>
> But the GIC is certainly arm architecture specific.
> It's built into the CPU, and shares state.
>
> The fact that it's modeled in hw/intc/ and not in target/arm/ has always been a needle in
> the side, though it seems there are no good options.

In retrospect I wonder if we should have modelled the
"GIC stream protocol" described in the GIC architecture
spec, which formalises the communication between the
CPU (including the GIC CPU interface) and the rest of
the GIC (redistributors, distributor). It would probably
have been more work and perhaps less efficient but it
would have been a cleaner way to split the hw/intc
code from the target/arm code. But regardless I don't
think it would really be very feasible to move to that
design at this point.

WRT the commit message, the GIC is definitely
Arm architecture specific. There might be boards
with both Arm cores and non-Arm cores and a GIC,
but the GIC will be connected to the Arm cores, and
there won't be boards with a GIC but no Arm cores.

The QAPI command which this code is implementing is
also (a) target-specific and (b) unfortunately
designed so that it doesn't get passed a particular
CPU or particular device to query, it's just assumed
to be a part of the whole simulation.

-- PMM

Re: [RFC PATCH-for-9.1? 1/2] target/arm: Move qmp_query_gic_capabilities() to hw/intc/

[Markus Armbruster]

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

[...]

> The QAPI command which this code is implementing is
> also (a) target-specific and (b) unfortunately
> designed so that it doesn't get passed a particular
> CPU or particular device to query, it's just assumed
> to be a part of the whole simulation.

We can fix (b) if we care: add a suitable optional argument, default to
the sole GIC in the system, fail if there's more than one.  I assume we
have no machines with more than one now.

Re: [RFC PATCH-for-9.1? 1/2] target/arm: Move qmp_query_gic_capabilities() to hw/intc/

[Peter Maydell]

On Thu, 8 Aug 2024 at 05:32, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> [...]
>
> > The QAPI command which this code is implementing is
> > also (a) target-specific and (b) unfortunately
> > designed so that it doesn't get passed a particular
> > CPU or particular device to query, it's just assumed
> > to be a part of the whole simulation.
>
> We can fix (b) if we care: add a suitable optional argument, default to
> the sole GIC in the system, fail if there's more than one.  I assume we
> have no machines with more than one now.

The exynos4210 SoC (board types 'nuri', 'smdkc210') has
two GICs. (It's a rather odd design -- there's the
interrupt controller that's part of the main CPU
cluster, and then they used a second "external" GIC
that feeds into that one.)

-- PMM

Re: [RFC PATCH-for-9.1? 1/2] target/arm: Move qmp_query_gic_capabilities() to hw/intc/

[Markus Armbruster]

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

> On Thu, 8 Aug 2024 at 05:32, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>> [...]
>>
>> > The QAPI command which this code is implementing is
>> > also (a) target-specific and (b) unfortunately
>> > designed so that it doesn't get passed a particular
>> > CPU or particular device to query, it's just assumed
>> > to be a part of the whole simulation.
>>
>> We can fix (b) if we care: add a suitable optional argument, default to
>> the sole GIC in the system, fail if there's more than one.  I assume we
>> have no machines with more than one now.
>
> The exynos4210 SoC (board types 'nuri', 'smdkc210') has
> two GICs. (It's a rather odd design -- there's the
> interrupt controller that's part of the main CPU
> cluster, and then they used a second "external" GIC
> that feeds into that one.)

Then "fail if there's more than one" would be an incompatible change for
this machine.

If the two GICs have identical capabilities, it doesn't matter to which
of the two query-gic-capabilities technically applies.

Else, it matters, and we have an interface issue.  Do we?

Re: [RFC PATCH-for-9.1? 1/2] target/arm: Move qmp_query_gic_capabilities() to hw/intc/

[Peter Maydell]

On Thu, 8 Aug 2024 at 10:02, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Thu, 8 Aug 2024 at 05:32, Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Peter Maydell <peter.maydell@linaro.org> writes:
> >>
> >> [...]
> >>
> >> > The QAPI command which this code is implementing is
> >> > also (a) target-specific and (b) unfortunately
> >> > designed so that it doesn't get passed a particular
> >> > CPU or particular device to query, it's just assumed
> >> > to be a part of the whole simulation.
> >>
> >> We can fix (b) if we care: add a suitable optional argument, default to
> >> the sole GIC in the system, fail if there's more than one.  I assume we
> >> have no machines with more than one now.
> >
> > The exynos4210 SoC (board types 'nuri', 'smdkc210') has
> > two GICs. (It's a rather odd design -- there's the
> > interrupt controller that's part of the main CPU
> > cluster, and then they used a second "external" GIC
> > that feeds into that one.)
>
> Then "fail if there's more than one" would be an incompatible change for
> this machine.
>
> If the two GICs have identical capabilities, it doesn't matter to which
> of the two query-gic-capabilities technically applies.
>
> Else, it matters, and we have an interface issue.  Do we?

It's not possible to use KVM with that machine type, so the
question is a bit moot. (This also indicates that the
interface is not very helpful -- it purports to tell the
management layer whether it can use an accelerated in-kernel
GIC, but because it doesn't specifiy the board type there's
no way to provide an accurate answer. It would be useful
to know exactly what libvirt/etc actually use this for...)

thanks
-- PMM

Re: [RFC PATCH-for-9.1? 1/2] target/arm: Move qmp_query_gic_capabilities() to hw/intc/

[Markus Armbruster]

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

> On Thu, 8 Aug 2024 at 10:02, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>> > On Thu, 8 Aug 2024 at 05:32, Markus Armbruster <armbru@redhat.com> wrote:
>> >>
>> >> Peter Maydell <peter.maydell@linaro.org> writes:
>> >>
>> >> [...]
>> >>
>> >> > The QAPI command which this code is implementing is
>> >> > also (a) target-specific and (b) unfortunately
>> >> > designed so that it doesn't get passed a particular
>> >> > CPU or particular device to query, it's just assumed
>> >> > to be a part of the whole simulation.
>> >>
>> >> We can fix (b) if we care: add a suitable optional argument, default to
>> >> the sole GIC in the system, fail if there's more than one.  I assume we
>> >> have no machines with more than one now.
>> >
>> > The exynos4210 SoC (board types 'nuri', 'smdkc210') has
>> > two GICs. (It's a rather odd design -- there's the
>> > interrupt controller that's part of the main CPU
>> > cluster, and then they used a second "external" GIC
>> > that feeds into that one.)
>>
>> Then "fail if there's more than one" would be an incompatible change for
>> this machine.
>>
>> If the two GICs have identical capabilities, it doesn't matter to which
>> of the two query-gic-capabilities technically applies.
>>
>> Else, it matters, and we have an interface issue.  Do we?
>
> It's not possible to use KVM with that machine type, so the
> question is a bit moot. (This also indicates that the
> interface is not very helpful -- it purports to tell the
> management layer whether it can use an accelerated in-kernel
> GIC, but because it doesn't specifiy the board type there's
> no way to provide an accurate answer. It would be useful
> to know exactly what libvirt/etc actually use this for...)

I had a look at libvirt.

It executes query-gic-capabilities only for certain ARM architectures,
namely

    qemuCaps->arch == VIR_ARCH_AARCH64 ||
    qemuCaps->arch == VIR_ARCH_ARMV6L ||
    qemuCaps->arch == VIR_ARCH_ARMV7L

This is virQEMUCapsProbeQMPGICCapabilities().  It stores the result of
query-gic-capabilities in a virGICCapability array.

The only use of the array I can see is in
virQEMUCapsSupportsGICVersion().  It returns true when one of the array
entries matches its version argument and supports its virtType argument.
Two users:

* virQEMUCapsFillDomainFeatureGICCaps() uses it to find a supported GIC
  version.  I believe this is then stored in domain XML.

* qemuDomainDefEnableDefaultFeatures() uses it to pick a default GIC
  version.  I believe this is then used to set machine property
  "gic-version".

Re: [RFC PATCH-for-9.1? 1/2] target/arm: Move qmp_query_gic_capabilities() to hw/intc/

[Daniel P. Berrangé]

On Thu, Aug 08, 2024 at 11:15:17AM +0100, Peter Maydell wrote:
> On Thu, 8 Aug 2024 at 10:02, Markus Armbruster <armbru@redhat.com> wrote:
> >
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >
> > > On Thu, 8 Aug 2024 at 05:32, Markus Armbruster <armbru@redhat.com> wrote:
> > >>
> > >> Peter Maydell <peter.maydell@linaro.org> writes:
> > >>
> > >> [...]
> > >>
> > >> > The QAPI command which this code is implementing is
> > >> > also (a) target-specific and (b) unfortunately
> > >> > designed so that it doesn't get passed a particular
> > >> > CPU or particular device to query, it's just assumed
> > >> > to be a part of the whole simulation.
> > >>
> > >> We can fix (b) if we care: add a suitable optional argument, default to
> > >> the sole GIC in the system, fail if there's more than one.  I assume we
> > >> have no machines with more than one now.
> > >
> > > The exynos4210 SoC (board types 'nuri', 'smdkc210') has
> > > two GICs. (It's a rather odd design -- there's the
> > > interrupt controller that's part of the main CPU
> > > cluster, and then they used a second "external" GIC
> > > that feeds into that one.)
> >
> > Then "fail if there's more than one" would be an incompatible change for
> > this machine.
> >
> > If the two GICs have identical capabilities, it doesn't matter to which
> > of the two query-gic-capabilities technically applies.
> >
> > Else, it matters, and we have an interface issue.  Do we?
> 
> It's not possible to use KVM with that machine type, so the
> question is a bit moot. (This also indicates that the
> interface is not very helpful -- it purports to tell the
> management layer whether it can use an accelerated in-kernel
> GIC, but because it doesn't specifiy the board type there's
> no way to provide an accurate answer. It would be useful
> to know exactly what libvirt/etc actually use this for...)

Libvirt uses this exclusively with the arm 'virt' machine type.

If the user didn't express any GIC preference, then if KVM is in use,
we'll pick the highest GIC version QEMU reports as supported. If TGCG
is in use we'll always pick v2, even if QEMU reports v3 is emulatable
due to the v3 impl lacking MSI controller which we need for PCI-e

  https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_domain.c#L4456

We'll also report to mgmt apps what GIC versions are available.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH-for-9.1? 1/2] target/arm: Move qmp_query_gic_capabilities() to hw/intc/

[Peter Maydell]

On Thu, 8 Aug 2024 at 12:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Thu, Aug 08, 2024 at 11:15:17AM +0100, Peter Maydell wrote:
> > It's not possible to use KVM with that machine type, so the
> > question is a bit moot. (This also indicates that the
> > interface is not very helpful -- it purports to tell the
> > management layer whether it can use an accelerated in-kernel
> > GIC, but because it doesn't specifiy the board type there's
> > no way to provide an accurate answer. It would be useful
> > to know exactly what libvirt/etc actually use this for...)
>
> Libvirt uses this exclusively with the arm 'virt' machine type.
>
> If the user didn't express any GIC preference, then if KVM is in use,
> we'll pick the highest GIC version QEMU reports as supported.

You can get that without querying QEMU by asking for 'gic-version=max'
if you like.

> If TCG
> is in use we'll always pick v2, even if QEMU reports v3 is emulatable
> due to the v3 impl lacking MSI controller which we need for PCI-e

Our emulated GICv3 supports the ITS which has MSI support, so I'm not
sure what forcing GICv2 is getting you here. Looking at the linked
RHEL bugzilla bug, I suspect this is an out-of-date policy from before
we added the ITS emulation in 2021 (it's present by default
in virt-2.8 and later machine types). So that is something that
libvirt should update I think.

thanks
-- PMM

Re: [RFC PATCH-for-9.1? 1/2] target/arm: Move qmp_query_gic_capabilities() to hw/intc/

[Daniel P. Berrangé]

On Thu, Aug 08, 2024 at 12:32:35PM +0100, Peter Maydell wrote:
> On Thu, 8 Aug 2024 at 12:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > On Thu, Aug 08, 2024 at 11:15:17AM +0100, Peter Maydell wrote:
> > > It's not possible to use KVM with that machine type, so the
> > > question is a bit moot. (This also indicates that the
> > > interface is not very helpful -- it purports to tell the
> > > management layer whether it can use an accelerated in-kernel
> > > GIC, but because it doesn't specifiy the board type there's
> > > no way to provide an accurate answer. It would be useful
> > > to know exactly what libvirt/etc actually use this for...)
> >
> > Libvirt uses this exclusively with the arm 'virt' machine type.
> >
> > If the user didn't express any GIC preference, then if KVM is in use,
> > we'll pick the highest GIC version QEMU reports as supported.
> 
> You can get that without querying QEMU by asking for 'gic-version=max'
> if you like.

This isn't in the VM startup path. It is when we expand the user
provided XML config into an ABI stable XML config by filling in
the blanks left by the user. So we need to actually query the
values available.

> > If TCG
> > is in use we'll always pick v2, even if QEMU reports v3 is emulatable
> > due to the v3 impl lacking MSI controller which we need for PCI-e
> 
> Our emulated GICv3 supports the ITS which has MSI support, so I'm not
> sure what forcing GICv2 is getting you here. Looking at the linked
> RHEL bugzilla bug, I suspect this is an out-of-date policy from before
> we added the ITS emulation in 2021 (it's present by default
> in virt-2.8 and later machine types). So that is something that
> libvirt should update I think.

It looks like it is virt-6.2 or later, and libvirt can probe it
by looking for existence of the arm-gicv3-its QOM type IIUC.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH-for-9.1? 1/2] target/arm: Move qmp_query_gic_capabilities() to hw/intc/

[Daniel P. Berrangé]

On Thu, Aug 08, 2024 at 12:48:39PM +0100, Daniel P. Berrangé wrote:
> On Thu, Aug 08, 2024 at 12:32:35PM +0100, Peter Maydell wrote:
> > On Thu, 8 Aug 2024 at 12:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > On Thu, Aug 08, 2024 at 11:15:17AM +0100, Peter Maydell wrote:
> > > > It's not possible to use KVM with that machine type, so the
> > > > question is a bit moot. (This also indicates that the
> > > > interface is not very helpful -- it purports to tell the
> > > > management layer whether it can use an accelerated in-kernel
> > > > GIC, but because it doesn't specifiy the board type there's
> > > > no way to provide an accurate answer. It would be useful
> > > > to know exactly what libvirt/etc actually use this for...)
> > >
> > > Libvirt uses this exclusively with the arm 'virt' machine type.
> > >
> > > If the user didn't express any GIC preference, then if KVM is in use,
> > > we'll pick the highest GIC version QEMU reports as supported.
> > 
> > You can get that without querying QEMU by asking for 'gic-version=max'
> > if you like.
> 
> This isn't in the VM startup path. It is when we expand the user
> provided XML config into an ABI stable XML config by filling in
> the blanks left by the user. So we need to actually query the
> values available.
> 
> > > If TCG
> > > is in use we'll always pick v2, even if QEMU reports v3 is emulatable
> > > due to the v3 impl lacking MSI controller which we need for PCI-e
> > 
> > Our emulated GICv3 supports the ITS which has MSI support, so I'm not
> > sure what forcing GICv2 is getting you here. Looking at the linked
> > RHEL bugzilla bug, I suspect this is an out-of-date policy from before
> > we added the ITS emulation in 2021 (it's present by default
> > in virt-2.8 and later machine types). So that is something that
> > libvirt should update I think.
> 
> It looks like it is virt-6.2 or later, and libvirt can probe it
> by looking for existence of the arm-gicv3-its QOM type IIUC.

https://gitlab.com/libvirt/libvirt/-/issues/659

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH-for-9.1? 1/2] target/arm: Move qmp_query_gic_capabilities() to hw/intc/

[Peter Maydell]

On Thu, 8 Aug 2024 at 12:54, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Aug 08, 2024 at 12:32:35PM +0100, Peter Maydell wrote:
> > On Thu, 8 Aug 2024 at 12:23, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > On Thu, Aug 08, 2024 at 11:15:17AM +0100, Peter Maydell wrote:
> > > > It's not possible to use KVM with that machine type, so the
> > > > question is a bit moot. (This also indicates that the
> > > > interface is not very helpful -- it purports to tell the
> > > > management layer whether it can use an accelerated in-kernel
> > > > GIC, but because it doesn't specifiy the board type there's
> > > > no way to provide an accurate answer. It would be useful
> > > > to know exactly what libvirt/etc actually use this for...)
> > >
> > > Libvirt uses this exclusively with the arm 'virt' machine type.
> > >
> > > If the user didn't express any GIC preference, then if KVM is in use,
> > > we'll pick the highest GIC version QEMU reports as supported.
> >
> > You can get that without querying QEMU by asking for 'gic-version=max'
> > if you like.
>
> This isn't in the VM startup path. It is when we expand the user
> provided XML config into an ABI stable XML config by filling in
> the blanks left by the user. So we need to actually query the
> values available.
>
> > > If TCG
> > > is in use we'll always pick v2, even if QEMU reports v3 is emulatable
> > > due to the v3 impl lacking MSI controller which we need for PCI-e
> >
> > Our emulated GICv3 supports the ITS which has MSI support, so I'm not
> > sure what forcing GICv2 is getting you here. Looking at the linked
> > RHEL bugzilla bug, I suspect this is an out-of-date policy from before
> > we added the ITS emulation in 2021 (it's present by default
> > in virt-2.8 and later machine types). So that is something that
> > libvirt should update I think.
>
> It looks like it is virt-6.2 or later, and libvirt can probe it
> by looking for existence of the arm-gicv3-its QOM type IIUC.

Yes. (I was confused by the 'no_its' setting in virt-2.8, but
that is for "is there a KVM ITS", and no_tcg_its is separate
and as you say added with 6.2.)

-- PMM

Re: [RFC PATCH-for-9.1? 1/2] target/arm: Move qmp_query_gic_capabilities() to hw/intc/

[Markus Armbruster]

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

> qmp_query_gic_capabilities() is not specific to the ARM
> architecture but to the GIC device which is modelled in
> hw/intc/, so move the code there for clarity. No logical
> change intended.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/intc/arm_gic_qmp.c     | 59 +++++++++++++++++++++++++++++++++++++++
>  target/arm/arm-qmp-cmds.c | 52 +---------------------------------
>  hw/intc/meson.build       |  1 +
>  3 files changed, 61 insertions(+), 51 deletions(-)
>  create mode 100644 hw/intc/arm_gic_qmp.c

[...]

> diff --git a/hw/intc/meson.build b/hw/intc/meson.build
> index afd1aa51ee..45d3503d49 100644
> --- a/hw/intc/meson.build
> +++ b/hw/intc/meson.build
> @@ -39,6 +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_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'))

You move qmp_query_gic_capabilities() from target/arm/arm-qmp-cmds.c (in
arm_system_ss) to hw/intc/arm_gic_qmp.c (in specific_ss when
CONFIG_ARM).

Both _ss are target-dependent.  In my testing, both get only compiled
for target arm and aarch64.  Obvious for arm-qmp-cmds.c in
arm_system_ss.  Less so for arm_gic_qmp.c in specific_ss; I guess the
CONFIG_ARM does the trick there.  Correct?

Re: [RFC PATCH-for-9.1? 1/2] target/arm: Move qmp_query_gic_capabilities() to hw/intc/

[Philippe Mathieu-Daudé]

On 7/8/24 07:12, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> qmp_query_gic_capabilities() is not specific to the ARM
>> architecture but to the GIC device which is modelled in
>> hw/intc/, so move the code there for clarity. No logical
>> change intended.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/intc/arm_gic_qmp.c     | 59 +++++++++++++++++++++++++++++++++++++++
>>   target/arm/arm-qmp-cmds.c | 52 +---------------------------------
>>   hw/intc/meson.build       |  1 +
>>   3 files changed, 61 insertions(+), 51 deletions(-)
>>   create mode 100644 hw/intc/arm_gic_qmp.c
> 
> [...]
> 
>> diff --git a/hw/intc/meson.build b/hw/intc/meson.build
>> index afd1aa51ee..45d3503d49 100644
>> --- a/hw/intc/meson.build
>> +++ b/hw/intc/meson.build
>> @@ -39,6 +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_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'))
> 
> You move qmp_query_gic_capabilities() from target/arm/arm-qmp-cmds.c (in
> arm_system_ss) to hw/intc/arm_gic_qmp.c (in specific_ss when
> CONFIG_ARM).
> 
> Both _ss are target-dependent.  In my testing, both get only compiled
> for target arm and aarch64.  Obvious for arm-qmp-cmds.c in
> arm_system_ss.  Less so for arm_gic_qmp.c in specific_ss; I guess the
> CONFIG_ARM does the trick there.  Correct?

Correct, Kconfig CONFIG_ARM_GIC depends on Kconfig CONFIG_ARM,
itself defined for configure TARGET_ARM (see target/arm/Kconfig).
(Also CONFIG_AARCH64 selects CONFIG_ARM).

I can clarify that in the patch description if necessary.

Re: [RFC PATCH-for-9.1? 1/2] target/arm: Move qmp_query_gic_capabilities() to hw/intc/

[Markus Armbruster]

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

> On 7/8/24 07:12, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>> 
>>> qmp_query_gic_capabilities() is not specific to the ARM
>>> architecture but to the GIC device which is modelled in
>>> hw/intc/, so move the code there for clarity. No logical
>>> change intended.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/intc/arm_gic_qmp.c     | 59 +++++++++++++++++++++++++++++++++++++++
>>>   target/arm/arm-qmp-cmds.c | 52 +---------------------------------
>>>   hw/intc/meson.build       |  1 +
>>>   3 files changed, 61 insertions(+), 51 deletions(-)
>>>   create mode 100644 hw/intc/arm_gic_qmp.c
>> [...]
>> 
>>> diff --git a/hw/intc/meson.build b/hw/intc/meson.build
>>> index afd1aa51ee..45d3503d49 100644
>>> --- a/hw/intc/meson.build
>>> +++ b/hw/intc/meson.build
>>> @@ -39,6 +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_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'))
>> You move qmp_query_gic_capabilities() from target/arm/arm-qmp-cmds.c (in
>> arm_system_ss) to hw/intc/arm_gic_qmp.c (in specific_ss when
>> CONFIG_ARM).
>>
>> Both _ss are target-dependent.  In my testing, both get only compiled
>> for target arm and aarch64.  Obvious for arm-qmp-cmds.c in
>> arm_system_ss.  Less so for arm_gic_qmp.c in specific_ss; I guess the
>> CONFIG_ARM does the trick there.  Correct?
>
> Correct, Kconfig CONFIG_ARM_GIC depends on Kconfig CONFIG_ARM,
> itself defined for configure TARGET_ARM (see target/arm/Kconfig).
> (Also CONFIG_AARCH64 selects CONFIG_ARM).
>
> I can clarify that in the patch description if necessary.

Wouldn't hurt.  If figure this patch is meant to be just code motion,
but it's not obvious from the patch that the code being moved gets
compiled into the exact same binaries before and after the move, unless
you know how CONFIG_ARM works.

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

[Philippe Mathieu-Daudé]

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
 #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'))
-- 
2.45.2

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

[Richard Henderson]

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~

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:

> 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'))

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

[Philippe Mathieu-Daudé]

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'))
> 

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?

[...]

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

[Peter Maydell]

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

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

[Markus Armbruster]

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?

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

[Peter Maydell]

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

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

[Philippe Mathieu-Daudé]

+Pierrick for historical context

On 6/8/24 16:19, Philippe Mathieu-Daudé wrote:
> Attempt to clarify confuse output from the
> 'query-gic-capabilities' QMP command when
> no GIC devices are built in.
> 
> Philippe Mathieu-Daudé (2):
>    target/arm: Move qmp_query_gic_capabilities() to hw/intc/
>    hw/intc/arm_gic: Only provide query-gic-capabilities when GIC built-in
> 
>   qapi/misc-target.json     |  4 +--
>   hw/intc/arm_gic_qmp.c     | 61 +++++++++++++++++++++++++++++++++++++++
>   target/arm/arm-qmp-cmds.c | 52 +--------------------------------
>   hw/intc/meson.build       |  1 +
>   4 files changed, 65 insertions(+), 53 deletions(-)
>   create mode 100644 hw/intc/arm_gic_qmp.c
>