Re: [PATCH v4 41/48] KVM: selftests: Add VGICv5 NR_IRQS attribute tests

On Fri, 2026-07-24 at 12:02 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
> - [Medium] The return value of `kvm_has_device_attr()` is silently
> ignored, rendering the attribute check ineffective.
> - [Medium] The test for exceeding the maximum supported SPI count
> uses an unaligned value, failing to correctly verify the bounds
> check.
> - [Medium] The test meant to verify that `NR_IRQS` is immutable after
> the VM has run is defeated by an explicit `CTRL_INIT` call.
> --
> 
> --- Patch [41]: [PATCH v4 41/48] KVM: selftests: Add VGICv5 NR_IRQS
> attribute tests ---
> commit 4c2364c5e2833c1962977d2e2805cafe763b427d
> Author: Sascha Bischoff <[email protected]>
> 
> KVM: selftests: Add VGICv5 NR_IRQS attribute tests
> 
> Add coverage for the VGICv5 NR_IRQS attribute. VGICv5 exposes a
> userspace-selectable SPI count, and KVM must reject values that
> cannot
> be represented by the virtual IRS or that would change the interrupt
> layout after it has been fixed.
> 
> > diff --git a/tools/testing/selftests/kvm/arm64/vgic_v5.c
> > b/tools/testing/selftests/kvm/arm64/vgic_v5.c
> > index 51ea2fe7141ad..95d74f501ef09 100644
> > --- a/tools/testing/selftests/kvm/arm64/vgic_v5.c
> > +++ b/tools/testing/selftests/kvm/arm64/vgic_v5.c
> 
> [ ... ]
> 
> > @@ -125,60 +183,106 @@ static void test_vgic_v5_create(void)
> >  	vm_gic_destroy(&v);
> >  }
> >  
> > -static void guest_irq_handler(struct ex_regs *regs)
> > +static void test_vgic_v5_nr_irqs_attrs(void)
> >  {
> > -	bool valid;
> > -	u32 hwirq;
> > -	u64 ia;
> > -	static int count;
> > -
> > -	/*
> > -	 * We have pending interrupts. Should never actually enter
> > WFI
> > -	 * here!
> > -	 */
> > -	wfi();
> > -	GUEST_SYNC(GUEST_CMD_IS_AWAKE);
> > -
> > -	ia = gicr_insn(CDIA);
> > -	valid = GICV5_GICR_CDIA_VALID(ia);
> > -
> > -	GUEST_SYNC(GUEST_CMD_IRQ_CDIA);
> > +	struct kvm_vcpu *vcpu;
> > +	struct vm_gic v;
> > +	uint64_t attr;
> > +	u32 nr_irqs;
> > +	int ret;
> >  
> > -	if (!valid)
> > -		return;
> > +	v.gic_dev_type = KVM_DEV_TYPE_ARM_VGIC_V5;
> > +	v.vm = __vm_create(VM_SHAPE_DEFAULT, NR_VCPUS, 0);
> > +	v.gic_fd = kvm_create_device(v.vm, v.gic_dev_type);
> >  
> > -	gsb_ack();
> > -	isb();
> > +	/* Check existing group/attribute */
> > +	kvm_has_device_attr(v.gic_fd,
> > KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0);
> 
> [Severity: Medium]
> Is the return value of kvm_has_device_attr() intentionally ignored
> here?

There isn't one. This returns void. There's a TEST_ASSERT() in that
function that catches the issue.

> 
> If the attribute is missing, the test will proceed and fail on
> subsequent
> operations rather than cleanly skipping or reporting the missing
> attribute.
> Does this check need to be wrapped in a TEST_REQUIRE() or
> TEST_ASSERT()?

As I stated above, it is!

> 
> > +
> > +	/* Before userspace sets NR_IRQS, no SPI count has been
> > selected. */
> > +	nr_irqs = 0xbad;
> 
> [ ... ]
> 
> > +	/* Not a multiple of 32 */
> > +	nr_irqs = VGIC_V5_DEFAULT_NR_SPIS + 1;
> > +	ret = __kvm_device_attr_set(v.gic_fd,
> > KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
> > +				    0, &nr_irqs);
> > +	TEST_ASSERT(ret && errno == EINVAL, "GICv5 NR_IRQS not 32-
> > aligned");
> > +
> > +	/* Larger than KVM's supported VGICv5 SPI count */
> > +	nr_irqs = VGIC_V5_MAX_NR_SPIS + 1;
> > +	ret = __kvm_device_attr_set(v.gic_fd,
> > KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
> > +				    0, &nr_irqs);
> > +	TEST_ASSERT(ret && errno == EINVAL, "GICv5 NR_IRQS above
> > maximum");
> 
> [Severity: Medium]
> Will this check actually verify the maximum limit?
> 
> Since VGIC_V5_MAX_NR_SPIS (65536) + 1 is 65537, which is not a
> multiple of
> 32, the kernel will reject it with EINVAL due to the alignment
> constraint
> before it reaches the maximum SPI limit check.
> 
> Should this test use VGIC_V5_MAX_NR_SPIS + 32 to ensure we test the
> upper
> bound check rather than triggering the alignment check again?

I've addressed this as part of reducing the GICv5 SPI limit to 1024.

> 
> > +
> > +	/* Valid custom SPI count */
> > +	nr_irqs = VGIC_V5_DEFAULT_NR_SPIS * 2;
> 
> [ ... ]
> 
> > +	/* NR_IRQS must remain immutable after the VM has run. */
> > +	v.vm = __vm_create(VM_SHAPE_DEFAULT, NR_VCPUS, 0);
> > +	v.gic_fd = kvm_create_device(v.vm, v.gic_dev_type);
> > +	vcpu = vm_vcpu_add(v.vm, 0, guest_code);
> >  
> > -	gic_insn(hwirq, CDDI);
> > -	gic_insn(0, CDEOI);
> > +	attr = GICV5_IRS_CONFIG_BASE_GPA;
> > +	kvm_device_attr_set(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > +			    KVM_VGIC_V5_ADDR_TYPE_IRS, &attr);
> > +	kvm_device_attr_set(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> > +			    KVM_DEV_ARM_VGIC_CTRL_INIT, NULL);
> >  
> > -	GUEST_SYNC(GUEST_CMD_IRQ_DIEOI);
> > +	ret = run_vcpu(vcpu);
> > +	TEST_ASSERT(!ret, "Failed to run GICv5 VM before NR_IRQS
> > test");
> 
> [Severity: Medium]
> Does this explicit KVM_DEV_ARM_VGIC_CTRL_INIT call defeat the purpose
> of
> testing if the attribute is immutable after the VM has run?
> 
> The previous test block already verifies that an explicit INIT locks
> the
> NR_IRQS attribute. By calling it here before run_vcpu(), the
> subsequent
> __kvm_device_attr_set() will return EBUSY solely because of this
> explicit
> INIT, masking whether KVM_RUN actually implicitly locks the attribute
> as
> intended.

This test is a duplicate of what has already been tested. Therefore,
I've dropped it. It didn't add anything useful.

> 
> >  
> > -	if (++count >= 2)
> > -		GUEST_DONE();
> > +	nr_irqs = VGIC_V5_DEFAULT_NR_SPIS * 2;
> > +	ret = __kvm_device_attr_set(v.gic_fd,
> > KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
> > +				    0, &nr_irqs);
> > +	TEST_ASSERT(ret && errno == EBUSY, "GICv5 NR_IRQS set
> > after run");
> >  
> > -	/* Ask for the next interrupt to be injected */
> > -	GUEST_SYNC(GUEST_CMD_IS_READY);
> > -}
> > +	vm_gic_destroy(&v);
> >  }
> 

Thanks,
Sascha