Re: [PATCH v2 15/16] KVM: selftests: Add VGICv5 ITS translation cache tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: selftests: IDR1 narrowing clears cache before ITS disable in vgic_v5
--

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

KVM: selftests: Add VGICv5 ITS translation cache tests

This commit adds a KVM selftest for VGICv5 ITS translation caching, guest
invalidation commands, register width accesses, and negative KVM_SIGNAL_MSI
paths.

>     KVM: selftests: Add VGICv5 ITS translation cache tests
>     
>     Add end-to-end coverage for VGICv5 ITS translation and cache
>     invalidation. The ITS attribute test only checks the userspace API
>     surface; this test verifies that KVM can parse guest-provided linear
>     DT and ITT state and use it to deliver LPIs through guest GEN_EVENTR
>     and KVM_SIGNAL_MSI.
>     
>     Build a two-VPE VM with IRS, ITS and an LPI IST. VPE0 programs a
>     linear device table and interrupt translation table, enables the
>     virtual ITS, enables two LPIs targeting VPE1, and sends GEN_EVENTR to
>     populate the translation cache with EventID to LPI A. VPE1 reports the
>     LPI identity it consumed.
>     
>     VPE0 then rewrites the ITT to point at LPI B without invalidation, and
>     a host KVM_SIGNAL_MSI injection verifies that the stale cached LPI A
>     translation is reused. VPE0 invalidates the event with ITS_INV_EVENTR
>     and the host verifies that LPI B is delivered.
>     
>     VPE0 then rewrites the ITT back to LPI A, invalidates the device with
>     ITS_INV_DEVICER, and the host verifies that LPI A is delivered again.
>     This exercises the guest-side event and device invalidation paths
>     rather than invalidating translations directly from the test host.
>     
>     Also cover negative MSI paths by checking that KVM_SIGNAL_MSI rejects a
>     message without KVM_MSI_VALID_DEVID and rejects a message whose address
>     does not target the GICv5 ITS. Finally, inject once before guest-side
>     ITS disable to prove delivery still works, then have VPE0 disable the
>     ITS and verify that a cached KVM_SIGNAL_MSI delivery is rejected with
>     ENXIO.
>     
>     Check 32-bit accesses to both halves of DT_BASER and preservation of
>     the untouched half. Verify that guest writes to the ITS ID registers
>     and READ_EVENT_DATAR are ignored, and that READ_EVENT_DATAR can be read
>     at both 32-bit and 64-bit widths.
>     
>     Signed-off-by: Sascha Bischoff <sascha.bischoff@arm.com>
> 
> diff --git a/tools/testing/selftests/kvm/arm64/vgic_v5.c b/tools/testing/selftests/kvm/arm64/vgic_v5.c
> index 92da9753ce8e6..a9b0bca24ec08 100644
> --- a/tools/testing/selftests/kvm/arm64/vgic_v5.c
> +++ b/tools/testing/selftests/kvm/arm64/vgic_v5.c

[ ... ]

> @@ -2312,6 +2644,183 @@ static void test_vgic_v5_ist_save_restore(void)

[ ... ]

> +static void test_vgic_v5_its_translation_cache(void)
> +{

[ ... ]

> +	/*
> +	 * Device invalidation must cover all cached EventIDs for the
> +	 * DeviceID. The guest has again re-written the event to translate to
> +	 * LPI A.
> +	 */
> +	ret = run_vcpu(vcpus[0]);
> +	TEST_ASSERT(!ret, "Failed to run GICv5 ITS device invalidation");
> +	vgic_v5_expect_sync(vcpus[0], GUEST_CMD_ITS_DEVICE_INVALIDATED,
> +			    "GICv5 ITS guest did not invalidate device");
> +	vgic_v5_expect_its_lpi(&v, vcpus[1], ITS_TEST_LPI_A);
> +
> +	/* A narrower userspace IDR1 must invalidate the cached DeviceID. */
> +	ret = __kvm_device_attr_get(v.its_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +				    GICV5_ITS_IDR1, &idr1);
> +	TEST_ASSERT(!ret, "GICv5 ITS_REGS failed to read IDR1");
> +	idr1 = (idr1 & ~GICV5_ITS_IDR1_DEVICEID_BITS) |
> +		FIELD_PREP(GICV5_ITS_IDR1_DEVICEID_BITS, 2);
> +	ret = __kvm_device_attr_set(v.its_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +				    GICV5_ITS_IDR1, &idr1);
> +	TEST_ASSERT(!ret, "GICv5 ITS_REGS failed to narrow DeviceID width");
> +	ret = __vgic_v5_signal_msi(v.vm, GICV5_ITS_BASE_GPA +
> +				   GICV5_ITS_TRANSLATER,
> +				   ITS_TEST_DEVICE_ID, ITS_TEST_EVENT_ID,
> +				   KVM_MSI_VALID_DEVID);
> +	TEST_ASSERT(ret == 0,
> +		    "GICv5 ITS did not block MSI after DeviceID width changed");
> +
> +	/* Disabling the ITS must also prevent delivery from cached entries. */
> +	ret = run_vcpu(vcpus[0]);
> +	TEST_ASSERT(!ret, "Failed to run GICv5 ITS disable");
> +	vgic_v5_expect_sync(vcpus[0], GUEST_CMD_ITS_DISABLED,
> +			    "GICv5 ITS guest did not disable ITS");
> +
> +	/*
> +	 * We should fail to inject this MSI. Use the raw call to ensure we
> +	 * catch the return value.
> +	 */
> +	ret = __vgic_v5_signal_msi(v.vm, GICV5_ITS_BASE_GPA +
> +				   GICV5_ITS_TRANSLATER,
> +				   ITS_TEST_DEVICE_ID, ITS_TEST_EVENT_ID,
> +				   KVM_MSI_VALID_DEVID);
> +	TEST_ASSERT(ret < 0 && errno == ENXIO,
> +		    "GICv5 ITS accepted cached MSI while disabled");

[Severity: Medium]
Does this final check in test_vgic_v5_its_translation_cache() actually test
disabling the ITS while a valid entry is present in the translation cache?

In the preceding block, writing GICV5_ITS_IDR1 via
KVM_DEV_ARM_VGIC_GRP_ITS_REGS to narrow GICV5_ITS_IDR1_DEVICEID_BITS to 2
causes vgic_v5_uaccess_write_its_idr() -> its_cache_clear_translations() to
flush the translation cache, and leaves ITS_TEST_DEVICE_ID (5) out of range
so the subsequent __vgic_v5_signal_msi() call fails in get_itt() without
repopulating the cache.

Because GICV5_ITS_IDR1 is not restored and vgic_v5_expect_its_lpi() is not
called again before VCPU0 runs to GUEST_CMD_ITS_DISABLED, the cache is
already empty when the ITS is disabled, which also differs from the sequence
described in the commit message ("Finally, inject once before guest-side ITS
disable to prove delivery still works, then have VPE0 disable the ITS and
verify that a cached KVM_SIGNAL_MSI delivery is rejected with ENXIO").

Should GICV5_ITS_IDR1 be restored and the translation cache repopulated via
vgic_v5_expect_its_lpi() before disabling the ITS (and should the commit
message also mention the IDR1 narrowing, read-only DT register, and DeviceID
cap checks)?

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