Re: [PATCH v4 42/48] KVM: selftests: Add VGICv5 IRS_REGS attribute tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Using a `u64` variable for 32-bit KVM device attributes causes test logic failures on Big-Endian architectures.
- [Medium] `IST_BASER` is configured to point to the VGIC MMIO region instead of guest RAM.
- [Medium] Spurious test failure on hardware supporting 8 priority bits.
--

--- Patch [42]: [PATCH v4 42/48] KVM: selftests: Add VGICv5 IRS_REGS attribute tests ---
commit 7457ae5ec0d03e64cfdb7419437558dc7acaf9df
Author: Sascha Bischoff <Sascha.Bischoff@arm.com>
Subject: KVM: selftests: Add VGICv5 IRS_REGS attribute tests

Add coverage for the VGICv5 IRS_REGS attribute group. Migration needs a
userspace-visible view of the virtual IRS register state, but KVM must
only expose valid registers and must reject state that cannot be restored
safely.

> diff --git a/tools/testing/selftests/kvm/arm64/vgic_v5.c b/tools/testing/selftests/kvm/arm64/vgic_v5.c
> --- a/tools/testing/selftests/kvm/arm64/vgic_v5.c
> +++ b/tools/testing/selftests/kvm/arm64/vgic_v5.c

[ ... ]

> +static void test_vgic_v5_irs_regs_attrs(void)
> +{

[ ... ]

> +	ret = __kvm_device_attr_get(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_IRS_REGS,
> +				    GICV5_IRS_IDR1, &val);
> +	TEST_ASSERT(!ret, "GICv5 IRS_REGS get IDR1 failed");
> +	val |= FIELD_PREP(GICV5_IRS_IDR1_PRIORITY_BITS, 0x7);
> +	ret = __kvm_device_attr_set(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_IRS_REGS,
> +				    GICV5_IRS_IDR1, &val);
> +	TEST_ASSERT(ret && errno == EINVAL, "GICv5 IRS_REGS accepted bad IDR1");

[Severity: Medium]
Will this assertion falsely fail on hardware that supports 8 priority bits?

The architecture allows up to 8 priority bits, which encodes as 0x7. If the
test runs on a host whose hardware legitimately supports this configuration,
KVM will likely accept the write instead of returning -EINVAL, causing a
spurious test failure.

[ ... ]

> +	ret = __kvm_device_attr_get(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_IRS_REGS,
> +				    GICV5_IRS_IDR5, &val);
> +	TEST_ASSERT(!ret, "GICv5 IRS_REGS get IDR5 failed");
> +	val++;
> +	ret = __kvm_device_attr_set(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_IRS_REGS,
> +				    GICV5_IRS_IDR5, &val);
> +	TEST_ASSERT(ret && errno == EINVAL, "GICv5 IRS_REGS accepted bad IDR5");

[Severity: Medium]
Could this cause spurious test failures on Big-Endian architectures?

KVM reads and writes 32-bit attributes to the upper 32 bits of a u64 buffer
on BE systems. The val++ operation increments the lower 32 bits, leaving
the actual 32-bit attribute value in the upper 32 bits untouched. When
this is written back, KVM might legitimately accept the unchanged value,
causing this assertion to fail.

[ ... ]

> +	/* Status registers read as idle through userspace accessors. */
> +	ret = __kvm_device_attr_get(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_IRS_REGS,
> +				    GICV5_IRS_SYNC_STATUSR, &val);
> +	TEST_ASSERT(!ret && val == GICV5_IRS_SYNC_STATUSR_IDLE,
> +		    "GICv5 IRS_REGS SYNC_STATUSR is not idle");

[ ... ]

> +	/* Userspace restore of IST_BASER updates state without allocating ISTs. */
> +	val = FIELD_PREP(GICV5_IRS_IST_BASER_ADDR_MASK,
> +			 GICV5_IRS_CONFIG_BASE_GPA >> GICV5_IRS_IST_BASER_ADDR_SHIFT);
> +	val |= GICV5_IRS_IST_BASER_VALID;
> +	ret = __kvm_device_attr_set(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_IRS_REGS,
> +				    GICV5_IRS_IST_BASER, &val);
> +	TEST_ASSERT(!ret, "GICv5 IRS_REGS failed to restore IST_BASER");

[Severity: Medium]
Is it intentional to configure the Interrupt Status Table to overlap with the
VGIC's own MMIO region here?

GICV5_IRS_CONFIG_BASE_GPA is the base address of the IRS MMIO region, but
the IST should typically reside in standard guest RAM (such as
VGIC_V5_LPI_IST_BASE_GPA). If KVM adds validation in the future to ensure
IST_BASER points to a valid memory slot instead of an MMIO region, could
this configuration be rejected and break the test?

[ ... ]

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