From: Haibo Xu <haibo.xu@linaro.org>
Allow virt arm machine to set the interrupt ID for the KVM
GIC maintenance interrupt.
This setting must be done before the KVM_DEV_ARM_VGIC_CTRL_INIT
hence the choice to perform the setting in the GICv3 realize
instead of proceeding the same way as kvm_arm_pmu_set_irq().
Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
v3 -> v4:
- only set maint_irq if vms->virt
v2 -> v3:
- tweak the commit message and explain why we do not proceed
the same way as kvm_arm_pmu_set_irq (Peter)
v1 -> v2:
- [Miguel] replaced the has_virt_extensions by the maintenance irq
intid property. [Eric] restored kvm_device_check_attr and
kvm_device_access standard usage and conditionally call those
if the prop is set.
---
include/hw/intc/arm_gicv3_common.h | 1 +
hw/arm/virt.c | 3 +++
hw/intc/arm_gicv3_common.c | 1 +
hw/intc/arm_gicv3_kvm.c | 21 +++++++++++++++++++++
4 files changed, 26 insertions(+)
diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
index a3d6a0e507..c18503869f 100644
--- a/include/hw/intc/arm_gicv3_common.h
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -231,6 +231,7 @@ struct GICv3State {
uint32_t num_cpu;
uint32_t num_irq;
uint32_t revision;
+ uint32_t maint_irq;
bool lpi_enable;
bool nmi_support;
bool security_extn;
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 3bcdf92e2f..550a272fbb 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -828,6 +828,9 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
OBJECT(mem), &error_fatal);
qdev_prop_set_bit(vms->gic, "has-lpi", true);
}
+ } else if (vms->virt) {
+ qdev_prop_set_uint32(vms->gic, "maintenance-interrupt-id",
+ ARCH_GIC_MAINT_IRQ);
}
} else {
if (!kvm_irqchip_in_kernel()) {
diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index 1cee68193c..e438d8c042 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -612,6 +612,7 @@ static const Property arm_gicv3_common_properties[] = {
DEFINE_PROP_BOOL("has-lpi", GICv3State, lpi_enable, 0),
DEFINE_PROP_BOOL("has-nmi", GICv3State, nmi_support, 0),
DEFINE_PROP_BOOL("has-security-extensions", GICv3State, security_extn, 0),
+ DEFINE_PROP_UINT32("maintenance-interrupt-id", GICv3State, maint_irq, 0),
/*
* Compatibility property: force 8 bits of physical priority, even
* if the CPU being emulated should have fewer.
diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 3be3bf6c28..b30aac7aee 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -22,6 +22,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "hw/intc/arm_gicv3_common.h"
+#include "hw/arm/virt.h"
#include "qemu/error-report.h"
#include "qemu/module.h"
#include "system/kvm.h"
@@ -825,6 +826,26 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
return;
}
+ if (s->maint_irq) {
+ int ret;
+
+ ret = kvm_device_check_attr(s->dev_fd,
+ KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ, 0);
+ if (!ret) {
+ error_setg_errno(errp, errno,
+ "VGICv3 setting maintenance IRQ is not "
+ "supported by this host kernel");
+ return;
+ }
+
+ ret = kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ, 0,
+ &s->maint_irq, true, errp);
+ if (ret) {
+ error_setg_errno(errp, errno, "Failed to set VGIC maintenance IRQ");
+ return;
+ }
+ }
+
multiple_redist_region_allowed =
kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION);
--
2.49.0On Wed, 2 Jul 2025 at 17:31, Eric Auger <eric.auger@redhat.com> wrote:
>
> From: Haibo Xu <haibo.xu@linaro.org>
>
> Allow virt arm machine to set the interrupt ID for the KVM
> GIC maintenance interrupt.
>
> This setting must be done before the KVM_DEV_ARM_VGIC_CTRL_INIT
> hence the choice to perform the setting in the GICv3 realize
> instead of proceeding the same way as kvm_arm_pmu_set_irq().
>
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> @@ -231,6 +231,7 @@ struct GICv3State {
> uint32_t num_cpu;
> uint32_t num_irq;
> uint32_t revision;
> + uint32_t maint_irq;
> bool lpi_enable;
> bool nmi_support;
> bool security_extn;
> + if (s->maint_irq) {
> + int ret;
> +
> + ret = kvm_device_check_attr(s->dev_fd,
> + KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ, 0);
> + if (!ret) {
> + error_setg_errno(errp, errno,
> + "VGICv3 setting maintenance IRQ is not "
> + "supported by this host kernel");
> + return;
> + }
> +
> + ret = kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ, 0,
> + &s->maint_irq, true, errp);
This doesn't seem to line up with what the kernel documents
for KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ:
https://www.kernel.org/doc/Documentation/virt/kvm/devices/arm-vgic-v3.rst
says
# KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ
# Attributes:
#
# The attr field of kvm_device_attr encodes the following values:
#
# bits: | 31 .... 5 | 4 .... 0 |
# values: | RES0 | vINTID |
#
# The vINTID specifies which interrupt is generated when the vGIC
# must generate a maintenance interrupt. This must be a PPI.
but this code sets kvmattr.addr to 0 and kvmaddr.addr to
the address of a uint32_t with the vINTID in it.
Looking at the kernel code in vgic_v3_attr_regs_access() it
looks like maybe the kernel docs are wrong, but I'm not sure.
thanks
-- PMM
On Fri, 04 Jul 2025 13:01:05 +0100,
Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 2 Jul 2025 at 17:31, Eric Auger <eric.auger@redhat.com> wrote:
> >
> > From: Haibo Xu <haibo.xu@linaro.org>
> >
> > Allow virt arm machine to set the interrupt ID for the KVM
> > GIC maintenance interrupt.
> >
> > This setting must be done before the KVM_DEV_ARM_VGIC_CTRL_INIT
> > hence the choice to perform the setting in the GICv3 realize
> > instead of proceeding the same way as kvm_arm_pmu_set_irq().
> >
> > Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> > Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
> > Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> > @@ -231,6 +231,7 @@ struct GICv3State {
> > uint32_t num_cpu;
> > uint32_t num_irq;
> > uint32_t revision;
> > + uint32_t maint_irq;
> > bool lpi_enable;
> > bool nmi_support;
> > bool security_extn;
>
> > + if (s->maint_irq) {
> > + int ret;
> > +
> > + ret = kvm_device_check_attr(s->dev_fd,
> > + KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ, 0);
> > + if (!ret) {
> > + error_setg_errno(errp, errno,
> > + "VGICv3 setting maintenance IRQ is not "
> > + "supported by this host kernel");
> > + return;
> > + }
> > +
> > + ret = kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ, 0,
> > + &s->maint_irq, true, errp);
>
> This doesn't seem to line up with what the kernel documents
> for KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ:
>
> https://www.kernel.org/doc/Documentation/virt/kvm/devices/arm-vgic-v3.rst
> says
>
> # KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ
> # Attributes:
> #
> # The attr field of kvm_device_attr encodes the following values:
> #
> # bits: | 31 .... 5 | 4 .... 0 |
> # values: | RES0 | vINTID |
> #
> # The vINTID specifies which interrupt is generated when the vGIC
> # must generate a maintenance interrupt. This must be a PPI.
>
> but this code sets kvmattr.addr to 0 and kvmaddr.addr to
> the address of a uint32_t with the vINTID in it.
>
> Looking at the kernel code in vgic_v3_attr_regs_access() it
> looks like maybe the kernel docs are wrong, but I'm not sure.
I think this is a pretty bad case of one person implementing
something, and another (me) hoping (but not checking) it would be
implemented in a particular way. Oh well.
Would the following match your expectations (and the code)?
diff --git a/Documentation/virt/kvm/devices/arm-vgic-v3.rst b/Documentation/virt/kvm/devices/arm-vgic-v3.rst
index e860498b1e359..4843c91052786 100644
--- a/Documentation/virt/kvm/devices/arm-vgic-v3.rst
+++ b/Documentation/virt/kvm/devices/arm-vgic-v3.rst
@@ -299,7 +299,7 @@ Groups:
KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ
Attributes:
- The attr field of kvm_device_attr encodes the following values:
+ The attribute data pointed to by kvm_device_attr.addr is a __u32 value::
bits: | 31 .... 5 | 4 .... 0 |
values: | RES0 | vINTID |
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On Fri, 4 Jul 2025 at 14:51, Marc Zyngier <maz@kernel.org> wrote: > > On Fri, 04 Jul 2025 13:01:05 +0100, > Peter Maydell <peter.maydell@linaro.org> wrote: > > https://www.kernel.org/doc/Documentation/virt/kvm/devices/arm-vgic-v3.rst > > says > > > > # KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ > > # Attributes: > > # > > # The attr field of kvm_device_attr encodes the following values: > > # > > # bits: | 31 .... 5 | 4 .... 0 | > > # values: | RES0 | vINTID | > > # > > # The vINTID specifies which interrupt is generated when the vGIC > > # must generate a maintenance interrupt. This must be a PPI. > > > > but this code sets kvmattr.addr to 0 and kvmaddr.addr to > > the address of a uint32_t with the vINTID in it. > > > > Looking at the kernel code in vgic_v3_attr_regs_access() it > > looks like maybe the kernel docs are wrong, but I'm not sure. > > I think this is a pretty bad case of one person implementing > something, and another (me) hoping (but not checking) it would be > implemented in a particular way. Oh well. > > Would the following match your expectations (and the code)? > > diff --git a/Documentation/virt/kvm/devices/arm-vgic-v3.rst b/Documentation/virt/kvm/devices/arm-vgic-v3.rst > index e860498b1e359..4843c91052786 100644 > --- a/Documentation/virt/kvm/devices/arm-vgic-v3.rst > +++ b/Documentation/virt/kvm/devices/arm-vgic-v3.rst > @@ -299,7 +299,7 @@ Groups: > KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ > Attributes: > > - The attr field of kvm_device_attr encodes the following values: > + The attribute data pointed to by kvm_device_attr.addr is a __u32 value:: I think it's maybe better to explicitly say "Attributes: ignored" or "Attributes: none" or similar. Otherwise it looks like the text is describing attr, because it follows the "Attributes:" subheader. (Though we don't do this for KVM_DEV_ARM_VGIC_GRP_NR_IRQS either, which also ignores attrs and reads/writes a value pointed to by addr.) > > bits: | 31 .... 5 | 4 .... 0 | > values: | RES0 | vINTID | Is the kvmattr.attr field mandatorily 0, or does the kernel ignore it? (I think the answer is "ignored".) Side note, we seem to allow this attribute to be read via get_attr for gicv2, even though has_attr denies it exists there, because the get code is in vgic_get_common_attr(). set_attr is in the gicv3-only vgic_v3_attr_regs_access(). -- PMM
© 2016 - 2025 Red Hat, Inc.