Re: [PATCH v5 49/49] KVM: selftests: Add VGICv5 sparse vCPU IDs test

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The VGICv5 sparse vCPU IDs test uses an out-of-bounds vCPU ID of 512 to verify GICv5 VPE ID validation, but KVM core globally rejects IDs >= 512, making the test a false positive.
- [Medium] The VGICv5 sparse vCPU IDs test hardcodes a vCPU ID of 17, which will cause spurious test failures on hardware that supports 16 or fewer VPEs per VM.
--

--- Patch [49]: [PATCH v5 49/49] KVM: selftests: Add VGICv5 sparse vCPU IDs test ---
commit 253db5fcbd1548e3eb0e84e65ece4ca5747e50c2
Author: Sascha Bischoff <[email protected]>

KVM: selftests: Add VGICv5 sparse vCPU IDs test

GICv5 uses the userspace-provided vCPU ID as the VPE ID. This must
work when vCPU IDs are sparse and differ from the dense internal vCPU
index.

Add a test that creates vCPUs with IDs 17 and 3, verifies that a vCPU
with ID 512 is rejected after a GICv5 has been created, and runs both
vCPUs to check that ICC_IAFFIDR_EL1 reports the correct VPE IDs on
each.

> diff --git a/tools/testing/selftests/kvm/arm64/vgic_v5.c b/tools/testing/selftests/kvm/arm64/vgic_v5.c
> index 47e147bc14b87..3eef2d2da595f 100644
> --- a/tools/testing/selftests/kvm/arm64/vgic_v5.c
> +++ b/tools/testing/selftests/kvm/arm64/vgic_v5.c
> @@ -14,6 +14,10 @@

[ ... ]

> +static void test_vgic_v5_sparse_vcpu_ids(void)
> +{
> +	struct kvm_vcpu *vcpus[VGIC_V5_SPARSE_NR_VCPUS];
> +	const u32 vcpu_ids[VGIC_V5_SPARSE_NR_VCPUS] = {
> +		VGIC_V5_SPARSE_VCPU0_ID,
> +		VGIC_V5_SPARSE_VCPU1_ID,
> +	};
> +	int ret, i;
> +	struct ucall uc;
> +	struct vm_gic v;
> +	u64 attr;
> +
> +	v.gic_dev_type = KVM_DEV_TYPE_ARM_VGIC_V5;
> +	v.vm = __vm_create(VM_SHAPE_DEFAULT, VGIC_V5_SPARSE_NR_VCPUS, 0);
> +	v.gic_fd = kvm_create_device(v.vm, v.gic_dev_type);
> +
> +	/*
> +	 * A GICv5 VM cannot create a vCPU outside the VPE ID range. We've
> +	 * already created a GICv5, so it should reject VCPU creation.
> +	 */
> +	ret = __vm_ioctl(v.vm, KVM_CREATE_VCPU,
> +			 (void *)(unsigned long)VGIC_V5_SPARSE_TOO_BIG_ID);
> +	TEST_ASSERT(ret < 0 && errno == EINVAL,
> +		    "GICv5 accepted vCPU ID %u", VGIC_V5_SPARSE_TOO_BIG_ID);

[Severity: Medium]
Will this test actually verify the GICv5 validation logic as intended?

The test unconditionally calls KVM_CREATE_VCPU with VGIC_V5_SPARSE_TOO_BIG_ID
(which is 512). The KVM core's kvm_vm_ioctl_create_vcpu() will immediately
return -EINVAL because the ID is >= KVM_MAX_VCPU_IDS (which defaults to 512
on ARM64).

This happens before any architecture-specific or GICv5 validation is reached,
potentially making this test a false positive.

> +
> +	/* Now, create two vCPUs with legal, albeit sparse, IDs */
> +	for (i = 0; i < ARRAY_SIZE(vcpus); i++) {
> +		vcpus[i] = vm_vcpu_add(v.vm, vcpu_ids[i], guest_iaffid_code);
> +		TEST_ASSERT(vcpus[i], "Failed to create vCPU %u", vcpu_ids[i]);

[Severity: Medium]
Could hardcoding the vCPU ID to 17 cause spurious test failures on some
hardware?

The test unconditionally adds a vCPU with ID 17 (VGIC_V5_SPARSE_VCPU0_ID).
In KVM ARM64, a VM's max_vcpus is bounded by hardware capabilities (for
example, GICV5_IRS_IDR4_VPE_ID_BITS).

If the hardware supports 16 or fewer VPEs per VM, the KVM_CREATE_VCPU ioctl
for ID 17 will be legitimately rejected by KVM with -EINVAL, which triggers
this TEST_ASSERT and fails the test.

> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=49