Re: [PATCH 14/16] KVM: selftests: Add VGICv5 ITS attribute tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] kvm: selftests: tautological test for ITS DT_CFGR/DT_BASER updates
- [Low] kvm: selftests: missing idle status register checks for ITS
--

commit c7601fac392a61f968d6064e81cd764b666cb504
Author: Sascha Bischoff <sascha.bischoff@arm.com>

KVM: selftests: Add VGICv5 ITS attribute tests

This commit adds coverage for the VGICv5 ITS device address and ITS_REGS
attribute groups. Userspace needs to be able to discover and save the virtual
ITS register state, while KVM must reject invalid address placement and
malformed register accesses.

> diff --git a/tools/testing/selftests/kvm/arm64/vgic_v5.c b/tools/testing/selftests/kvm/arm64/vgic_v5.c
> index d5df742b23001..b7fb002a64f6c 100644
> --- a/tools/testing/selftests/kvm/arm64/vgic_v5.c
> +++ b/tools/testing/selftests/kvm/arm64/vgic_v5.c
[ ... ]
> @@ -1287,6 +1339,177 @@ static void test_vgic_v5_irs_regs_attrs(void)
>  	vm_gic_destroy(&v);
>  }
>  
> +static void test_vgic_v5_its_attrs(void)
> +{
[ ... ]
> +	/* Check MMIO by checking if present, reading, then writing back */
> +	for (i = 0; i < ARRAY_SIZE(gic_v5_its_regs); i++) {
> +		attr = gic_v5_its_regs[i].attr;
> +		ret = __kvm_has_device_attr(v.its_fd,
> +					    KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +					    attr);

[Severity: Low]
The commit message mentions exercising "idle status register values", but this
loop only reads and writes back the registers without checking their values.
Should there be an explicit assertion that the status registers (like STATUSR,
SYNC_STATUSR, GEN_EVENT_STATUSR) actually read as idle? The corresponding
test_vgic_v5_irs_regs_attrs() test includes these checks for the IRS.

[ ... ]
> +	/* Make sure we can restore the vITS's enable state */
> +	val = GICV5_ITS_CR0_ITSEN;
> +	ret = __kvm_device_attr_set(v.its_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +				    GICV5_ITS_CR0, &val);
> +	TEST_ASSERT(!ret, "GICv5 ITS_REGS failed to enable ITS");
> +
> +	/* And DT CFGR/BASER */
> +	val = 0;
> +	ret = __kvm_device_attr_set(v.its_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +				    GICV5_ITS_DT_CFGR, &val);
> +	TEST_ASSERT(!ret, "GICv5 ITS_REGS rejected enabled DT_CFGR update");
> +	ret = __kvm_device_attr_get(v.its_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +				    GICV5_ITS_DT_CFGR, &val);
> +	TEST_ASSERT(!ret && !val, "GICv5 ITS_REGS DT_CFGR restore mismatch");

[Severity: Low]
Is this test effectively verifying that the updates to GICV5_ITS_DT_CFGR work
while the ITS is enabled? 

Because val is set to 0 (the default/reset value of the register) before the
write, if the kernel incorrectly discards the write while ITSEN=1, the
register will remain 0 and the read-back will return 0, allowing the test to
pass spuriously. Writing a non-zero valid value would avoid this tautology.

Also, the test writes GICV5_ITS_CR0_ITSEN to CR0 but never reads it back.
Could the test be trying to update the configuration registers on a disabled
ITS if the enable bit didn't actually stick?

> diff --git a/tools/testing/selftests/kvm/include/arm64/gic_v5.h b/tools/testing/selftests/kvm/include/arm64/gic_v5.h
> index 2d455515563e0..22ab78626af5a 100644
> --- a/tools/testing/selftests/kvm/include/arm64/gic_v5.h
> +++ b/tools/testing/selftests/kvm/include/arm64/gic_v5.h
[ ... ]
> +#define GICV5_ITS_CR0_IDLE		BIT(1)

[Severity: Low]
Is this macro correct for the ITS? It defines an IDLE bit for CR0, but it
appears to be unused in the test. Looking at the GICv5 architecture, it seems
only the IRS has an IDLE bit in CR0, not the ITS.

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