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

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?

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.

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.

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~

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

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.

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

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?

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

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 :|

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

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 :|

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

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 :|

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".

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.