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

On Wed, 19 Aug 2026 at 11:39, <[email protected]> wrote:
>
> 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.

arm64 big-endian has depended on BROKEN since 6.18, and is being
removed [1], so this is fine for a selftest.

> - [Medium] The guest code modifies GITS_BASER immediately after clearing GITS_CTLR.Enable without polling for the GITS_CTLR.Quiescent bit.

KVM's emulated ITS permits the GITS_BASER write whenever
GITS_CTLR.Enable is clear, so the poll is not needed here.

/fuad

[1] https://lore.kernel.org/all/[email protected]/


> --
>
> --- 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
>