Re: [PATCH v2 4/4] KVM: arm64: selftests: Add ITS table save tests

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unsafe DMA Buffer Configuration (ITT Size Miscalculation)
- [Medium] Incorrect Endianness Conversion for L1 Device Table
--

--- Patch [4]: [PATCH v2 4/4] KVM: arm64: selftests: Add ITS table save tests ---
commit 00c3ff8ee303e906c6d0f346f17949dbb0abf5fc
Author: Fuad Tabba <[email protected]>
Subject: KVM: arm64: selftests: Add ITS table save tests

This commit adds selftests for KVM_DEV_ARM_ITS_SAVE_TABLES to cover cases
where a guest alters tables. It tests handling of a GITS_BASER<coll> write
that changes the table, and a device whose L2 block the guest invalidated.
Each case is then reset and restored to verify the save functionality.

> diff --git a/tools/testing/selftests/kvm/arm64/vgic_its_save.c b/tools/testing/selftests/kvm/arm64/vgic_its_save.c
> new file mode 100644
> index 0000000000000..864da01539f3f
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/arm64/vgic_its_save.c
> @@ -0,0 +1,441 @@
[ ... ]
> +/*
> + * its_send_mapd_cmd() encodes ilog2(itt_size) - 1 as num_eventid_bits, and
> + * vgic_its_restore_itt() scans BIT_ULL(num_eventid_bits) * ESZ, so the size
> + * handed to MAPD has to match the ITT allocated for it.
> + */
> +#define ITT_EVENTID_BITS	13
> +#define ITT_MAPD_SIZE		BIT_ULL(ITT_EVENTID_BITS + 1)

[Severity: Medium]
Does this calculation configure the hardware with an ITT size of 128KB instead
of the intended 64KB?

The test calculates ITT_MAPD_SIZE as BIT_ULL(ITT_EVENTID_BITS + 1) (evaluating
to 16,384) to compensate for its_send_mapd_cmd() subtracting 1. However, KVM's
its_cmd_get_size() adds 1 back, resulting in 14 bits of event IDs.

During restore, KVM computes the scan size as BIT_ULL(14) * 8 = 128KB, causing
it to read 64KB past the end of the allocated ITT memory.

> +#define ITT_SZ			(BIT_ULL(ITT_EVENTID_BITS) * ESZ)
[ ... ]
> +static void guest_unreachable_device(void)
> +{
> +	u64 *l1;
[ ... ]
> +	guest_make_device_table_indirect();
> +
> +	/* Both L2 blocks present, so both MAPDs are in range. */
> +	l1 = test_data.device_l1_va;
> +	l1[0] = L1E_VALID_MASK | (test_data.device_l2[0] & L1E_ADDR_MASK);
> +	l1[1] = L1E_VALID_MASK | (test_data.device_l2[1] & L1E_ADDR_MASK);

[Severity: Medium]
Will this fail on big-endian hosts? The L1 entries are being populated using
native CPU endianness.

Since the ARM GICv3 ITS architecture and KVM emulator require these tables to
be little-endian (using le64_to_cpu on read), shouldn't these use htole64()
when writing them?

> +
> +	its_send_mapd_cmd(test_data.cmdq_base_va, DEVICE_A_ID,
> +			  test_data.itt_tables, ITT_MAPD_SIZE, true);
> +	its_send_mapd_cmd(test_data.cmdq_base_va, DEVICE_B_ID,
> +			  test_data.itt_tables + ITT_SZ, ITT_MAPD_SIZE, true);

[Severity: Medium]
This isn't a separate bug, but as a result of the ITT_MAPD_SIZE miscalculation
above, doesn't this send MAPD commands configuring overlapping, out-of-bounds
ITT tables for the two devices?

> +
> +	/*
> +	 * Drop the block holding device B. No ITS command and no GITS_BASER
> +	 * write is involved, so nothing tells KVM the device is now
> +	 * unreachable.
> +	 */
> +	l1[1] = 0;
> +
> +	GUEST_DONE();
> +}

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