Re: [PATCH v5 43/49] KVM: selftests: Add VGICv5 IST attribute tests

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Incomplete test coverage for VGICv5 IST attribute validation
--

--- Patch [43]: [PATCH v5 43/49] KVM: selftests: Add VGICv5 IST attribute tests ---
commit 9ba6d67381650763d50c218233de98711d15304f
Author: Sascha Bischoff <[email protected]>

KVM: selftests: Add VGICv5 IST attribute tests

This commit adds selftests for the VGICv5 IST attribute group. It verifies
that KVM reports the attribute group correctly and rejects invalid operations,
such as out-of-order save/restore attempts and badly sized IST buffers.

> diff --git a/tools/testing/selftests/kvm/arm64/vgic_v5.c b/tools/testing/selftests/kvm/arm64/vgic_v5.c
> index 54fa13906990b..e053b01bb5be3 100644
> --- a/tools/testing/selftests/kvm/arm64/vgic_v5.c
> +++ b/tools/testing/selftests/kvm/arm64/vgic_v5.c

[ ... ]

> @@ -523,6 +534,114 @@ static void test_vgic_v5_irs_regs_attrs(void)
>  	vm_gic_destroy(&v);
>  }
>  
> +static void test_vgic_v5_ist_attrs(void)
> +{

[ ... ]

> +	/* Check missing and incorrectly sized SPI IST buffers. */
> +	ist_attr = vgic_v5_ist_attr(NULL, 0, NULL, 0);
> +	ret = __kvm_device_attr_get(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_IST,
> +				    0, &ist_attr);
> +	TEST_ASSERT(ret && errno == EINVAL, "GICv5 IST get accepted missing SPI buffer");
> +
> +	ist_attr = vgic_v5_ist_attr(spi_ist, sizeof(spi_ist) - sizeof(__u32),
> +					 NULL, 0);
> +	ret = __kvm_device_attr_set(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_IST,
> +				    0, &ist_attr);
> +	TEST_ASSERT(ret && errno == EINVAL, "GICv5 IST set accepted bad SPI size");

[Severity: Medium]
Does this test leave coverage gaps for the set/get paths? For the missing
SPI buffer, the test only checks __kvm_device_attr_get(), but neglects
__kvm_device_attr_set(). Similarly, for the incorrectly sized SPI buffer,
it checks __kvm_device_attr_set() but not __kvm_device_attr_get(). 

Would it make sense to test both the get and set paths for these validation
checks, similar to how the NULL descriptor checks were handled earlier?

> +
> +	/* LPI storage must be absent when no LPI IST is configured. */
> +	ist_attr = vgic_v5_ist_attr(spi_ist, sizeof(spi_ist), spi_ist,
> +					 sizeof(__u32));
> +	ret = __kvm_device_attr_get(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_IST,
> +				    0, &ist_attr);
> +	TEST_ASSERT(ret && errno == EINVAL, "GICv5 IST get accepted unexpected LPI buffer");

[Severity: Medium]
Is this test missing the corresponding check for the set path? As with the
SPI buffer tests above, it only validates that __kvm_device_attr_get()
rejects the unexpected LPI buffer, but does not check if
__kvm_device_attr_set() correctly rejects it as well.

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