Re: [PATCH kvmtool v3 3/6] arm64: nested: add support for setting maintenance IRQ

Marc Zyngier
2025-08-04T17:51:58Z

On Mon, 04 Aug 2025 15:43:09 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Hi Andre,
>
> 'add' should be capitalized.
>
> On Tue, Jul 29, 2025 at 10:57:42AM +0100, Andre Przywara wrote:
> > Uses the new VGIC KVM device attribute to set the maintenance IRQ.
> > This is fixed to use PPI 9, as a platform decision made by kvmtool,
> > matching the SBSA recommendation.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> > arm64/arm-cpu.c | 3 ++-
> > arm64/gic.c | 21 ++++++++++++++++++++-
> > arm64/include/kvm/gic.h | 2 +-
> > 3 files changed, 23 insertions(+), 3 deletions(-)
> >
> > diff --git a/arm64/arm-cpu.c b/arm64/arm-cpu.c
> > index 69bb2cb2c..1e456f2c6 100644
> > --- a/arm64/arm-cpu.c
> > +++ b/arm64/arm-cpu.c
> > @@ -14,7 +14,8 @@ static void generate_fdt_nodes(void *fdt, struct kvm *kvm)
> > {
> > int timer_interrupts[4] = {13, 14, 11, 10};
> >
> > - gic__generate_fdt_nodes(fdt, kvm->cfg.arch.irqchip);
> > + gic__generate_fdt_nodes(fdt, kvm->cfg.arch.irqchip,
> > + kvm->cfg.arch.nested_virt);
> > timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
> > pmu__generate_fdt_nodes(fdt, kvm);
> > }
> > diff --git a/arm64/gic.c b/arm64/gic.c
> > index b0d3a1abb..7461b0f3f 100644
> > --- a/arm64/gic.c
> > +++ b/arm64/gic.c
> > @@ -11,6 +11,8 @@
> >
> > #define IRQCHIP_GIC 0
> >
> > +#define GIC_MAINT_IRQ 9
> > +
> > static int gic_fd = -1;
> > static u64 gic_redists_base;
> > static u64 gic_redists_size;
> > @@ -302,10 +304,15 @@ static int gic__init_gic(struct kvm *kvm)
> >
> > int lines = irq__get_nr_allocated_lines();
> > u32 nr_irqs = ALIGN(lines, 32) + GIC_SPI_IRQ_BASE;
> > + u32 maint_irq = GIC_MAINT_IRQ + 16; /* PPI */
>
> There's already a define for PPIs:
>
> u32 maint_irq = GIC_PPI_IRQ_BASE + GIC_MAINT_IRQ;
>
> > struct kvm_device_attr nr_irqs_attr = {
> > .group = KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
> > .addr = (u64)(unsigned long)&nr_irqs,
> > };
> > + struct kvm_device_attr maint_irq_attr = {
> > + .group = KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ,
> > + .addr = (u64)(unsigned long)&maint_irq,
> > + };
> > struct kvm_device_attr vgic_init_attr = {
> > .group = KVM_DEV_ARM_VGIC_GRP_CTRL,
> > .attr = KVM_DEV_ARM_VGIC_CTRL_INIT,
> > @@ -325,6 +332,13 @@ static int gic__init_gic(struct kvm *kvm)
> > return ret;
> > }
> >
> > + if (kvm->cfg.arch.nested_virt &&
> > + !ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &maint_irq_attr)) {
>
> I'm not sure how useful the HAS_DEVICE_ATTR call is here: kvm_cpu__arch_init(),
> which checks for KVM_CAP_ARM_EL2 capability, is called before gic__init_gic()
> (base_init() vs late_init()). So at this point we know that KVM supports nested
> virtualization.

I disagree. All optional features should be guarded by a check for
that particular feature. If anything, this serves as validation for
the kernel side (which is, let's face it, the *only* use-case for
kvmtool).

> Was it that KVM at some point supported nested virtualization but didn't have
> the KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ device attribute implemented? And if that was
> the case, do we want to support that version of KVM in kvmtool?

Then make kvmtool die in this case. But the check stays.

M.

--
Jazz isn't dead. It just smells funny.