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

[sashiko-bot@kernel.org]

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 <Sascha.Bischoff@arm.com>

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?

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()?

> +
> +	/* 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?

> +
> +	/* 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.

>  
> -	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);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724104819.1296803-1-sascha.bischoff@arm.com?part=41