Re: [PATCH 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] The guest code writes L1 device table entries using native endianness instead of little-endian, which will cause corrupted data on big-endian guests.
- [Medium] The guest code modifies GITS_BASER immediately after clearing GITS_CTLR.Enable without polling for the GITS_CTLR.Quiescent bit.
--

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

KVM: arm64: selftests: Add ITS table save tests

This commit adds selftests for KVM_DEV_ARM_ITS_SAVE_TABLES to handle two
cases where a guest leaves a table in a modified state. It covers a
GITS_BASER write that drops described collections, and an invalidated L2
block that causes a device to be skipped.

> 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
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/arm64/vgic_its_save.c

[ ... ]

> +static void guest_shrink_coll_table(void)
> +{
> +	unsigned long offset = its_baser_offset(GITS_BASER_TYPE_COLLECTION);
> +	u64 baser;
> +
> +	its_set_enable(false);
> +
> +	baser = readq_relaxed(GITS_BASE_GVA + offset);

[Severity: Medium]
Is it architecturally valid to read and modify GITS_BASER immediately after
clearing GITS_CTLR_ENABLE without first polling for the GITS_CTLR.Quiescent
bit?

While KVM's synchronous implementation might tolerate this sequence today,
could this missing poll cause unpredictability on asynchronous hardware or
future emulators?

> +	baser &= ~GITS_BASER_PAGES_MASK;
> +	writeq_relaxed(baser, GITS_BASE_GVA + offset);
> +}

[ ... ]

> +/* Turn the already installed device table into an indirect one. */
> +static void guest_make_device_table_indirect(void)
> +{
> +	unsigned long offset = its_baser_offset(GITS_BASER_TYPE_DEVICE);
> +	u64 baser;
> +
> +	its_set_enable(false);
> +
> +	baser = readq_relaxed(GITS_BASE_GVA + offset);

[Severity: Medium]
Similarly here in guest_make_device_table_indirect(), does this need a
poll for the Quiescent bit after disabling the ITS and before accessing
GITS_BASER?

> +	writeq_relaxed(baser | GITS_BASER_INDIRECT, GITS_BASE_GVA + offset);
> +
> +	its_set_enable(true);
> +}
> +
> +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]
Since the host code later reads this memory using le64toh(), will writing
these L1 device table entries in native endianness cause data corruption
when running the selftest on big-endian guests?

Should these entries be converted using cpu_to_le64() before assignment?

> +
> +	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);

[ ... ]

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