| Message ID | 20240806141940.22095-2-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: > 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~
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.
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.
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 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
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 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
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
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
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
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'))
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