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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] vgic_v5: missing ITS_STATUSR/ITS_GEN_EVENT_STATUSR completion poll
- [High] vgic_v5: priority drop coupled with deactivation in ITS IRQ handler
--

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

KVM: selftests: Add VGICv5 ITS translation cache tests

This commit adds end-to-end coverage for VGICv5 ITS translation and cache
invalidation. It 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. The test builds a two-VPE VM with IRS, ITS, and an LPI IST.

> diff --git a/tools/testing/selftests/kvm/arm64/vgic_v5.c b/tools/testing/selftests/kvm/arm64/vgic_v5.c
> index b7fb002a64f6c..6ea6aef4f0f46 100644
> --- a/tools/testing/selftests/kvm/arm64/vgic_v5.c
> +++ b/tools/testing/selftests/kvm/arm64/vgic_v5.c

[ ... ]

> +/* Inject a manually triggered translation via the ITS_GEN_EVENTR */
> +static void gicv5_send_its_gen_event(u32 device_id, u32 event_id)
> +{
> +	u64 val;
> +
> +	val = FIELD_PREP(GICV5_ITS_GEN_EVENT_DIDR_DEVICEID, device_id);
> +	writeq(val, GICV5_ITS_BASE_GVA + GICV5_ITS_GEN_EVENT_DIDR);
> +
> +	val = FIELD_PREP(GICV5_ITS_GEN_EVENT_EIDR_EVENTID, event_id);
> +	writel(val, GICV5_ITS_BASE_GVA + GICV5_ITS_GEN_EVENT_EIDR);
> +
> +	writel(GICV5_ITS_GEN_EVENTR_R |
> +	       FIELD_PREP(GICV5_ITS_GEN_EVENTR_TARGET_DOMAIN, 0),
> +	       GICV5_ITS_BASE_GVA + GICV5_ITS_GEN_EVENTR);

[Severity: High]
Does this operation require polling for completion? In gicv5_send_its_gen_event,
the event generation command is issued asynchronously by writing to
GICV5_ITS_GEN_EVENTR without a subsequent poll of the ITS_GEN_EVENT_STATUSR
IDLE bit.

Since the test subsequently yields to the host via GUEST_SYNC and expects the
event to be delivered, failing to wait for the virtual hardware to complete
introduces a race condition. The host might act before the virtual ITS finishes
processing, leading to flaky test execution.

> +}
> +
> +static void gicv5_invalidate_its_event(u32 device_id, u32 event_id)
> +{
> +	u64 val;
> +
> +	val = FIELD_PREP(GICV5_ITS_DIDR_DEVICEID, device_id);
> +	writeq(val, GICV5_ITS_BASE_GVA + GICV5_ITS_DIDR);
> +
> +	val = FIELD_PREP(GICV5_ITS_EIDR_EVENTID, event_id);
> +	writel(val, GICV5_ITS_BASE_GVA + GICV5_ITS_EIDR);
> +
> +	writel(GICV5_ITS_INV_EVENTR_I,
> +	       GICV5_ITS_BASE_GVA + GICV5_ITS_INV_EVENTR);

[Severity: High]
Should this function poll ITS_STATUSR IDLE after invalidating the event?

In gicv5_invalidate_its_event, the event invalidation command is issued
without a subsequent completion poll. The GICv5 guidelines state that an
ITS table entry invalidated without polling ITS_STATUSR IDLE is a bug, as
the host might act before the virtual ITS finishes processing the
invalidation.

> +}
> +
> +static void gicv5_invalidate_its_device(u32 device_id, u32 event_id_bits)
> +{
> +	u64 val;
> +
> +	val = FIELD_PREP(GICV5_ITS_DIDR_DEVICEID, device_id);
> +	writeq(val, GICV5_ITS_BASE_GVA + GICV5_ITS_DIDR);
> +
> +	val = GICV5_ITS_INV_DEVICER_I |
> +	      FIELD_PREP(GICV5_ITS_INV_DEVICER_EVENTID_BITS, event_id_bits);
> +	writel(val, GICV5_ITS_BASE_GVA + GICV5_ITS_INV_DEVICER);

[Severity: High]
Similar to the event invalidation above, should gicv5_invalidate_its_device
wait for the device invalidation to complete by polling ITS_STATUSR IDLE
before returning? Failing to do so can race with the host side.

> +}

[ ... ]

> +static void guest_its_irq_handler(struct ex_regs *regs)
> +{
> +	u64 expected_hwirq = gicv5_lpi_hwirq(READ_ONCE(its_expected_lpi));
> +	u32 hwirq;
> +	u64 ia;
> +
> +	ia = gicr_insn(CDIA);
> +	if (!GICV5_GICR_CDIA_VALID(ia))
> +		return;
> +
> +	gsb_ack();
> +	isb();
> +
> +	hwirq = FIELD_GET(GICV5_GICR_CDIA_INTID, ia);
> +	GUEST_ASSERT_EQ(hwirq, expected_hwirq);
> +
> +	gic_insn(hwirq, CDDI);
> +	gic_insn(0, CDEOI);

[Severity: High]
Can we separate the priority drop from the deactivation in this handler?

In guest_its_irq_handler, the priority drop instruction (CDEOI) is
executed back-to-back with deactivation (CDDI) at the end of the handler.
The GICv5 architecture strictly enforces a split-EOI model where the
priority drop must be performed immediately after the acknowledge and
synchronization barriers, before dispatching to the main handler logic.

Deferring the priority drop until the end blocks all interrupts of equal
or lower priority for the handler's duration.

> +
> +	GUEST_SYNC(GUEST_CMD_ITS_LPI_DELIVERED);
> +}

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