Re: [PATCH v2 11/16] irqchip/gic-v5: Enable direct injection in ITS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] irqchip/gic-v5: sleep in atomic context via gicv5_irs_syncr() under desc->lock
- [High] irqchip/gic-v5: unlocked concurrent MMIO access in gicv5_its_itt_cache_inv()
- [Medium] irqchip/gic-v5: ignored timeout in gicv5_irs_syncr() during ITTE restoration
--

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

irqchip/gic-v5: Enable direct injection in ITS

This commit adds the irq_set_vcpu_affinity callback and helper functions to
install and remove virtual ITTE translations in the GICv5 ITS for KVM direct
interrupt injection, while disabling lazy masking for forwarded interrupts.

> diff --git a/drivers/irqchip/irq-gic-v5-its.c b/drivers/irqchip/irq-gic-v5-its.c
> index 28e39b065de0e..d91f4ea1ef440 100644
> --- a/drivers/irqchip/irq-gic-v5-its.c
> +++ b/drivers/irqchip/irq-gic-v5-its.c

[ ... ]

> @@ -723,6 +765,102 @@ static void gicv5_its_compose_msi_msg(struct irq_data *d, struct msi_msg *msg)
>  	msi_msg_set_addr(irq_data_get_msi_desc(d), msg, addr);
>  }
>  
> +#ifdef CONFIG_KVM
> +static int gicv5_its_set_vcpu_affinity(struct irq_data *d, void *int_data)
> +{
> +	struct gicv5_its_dev *its_dev = irq_data_get_irq_chip_data(d);
> +	u16 event_id = FIELD_GET(GICV5_ITS_HWIRQ_EVENT_ID, d->hwirq);
> +	u32 dev_id = FIELD_GET(GICV5_ITS_HWIRQ_DEVICE_ID, d->hwirq);
> +	struct virt_intr_data *data = int_data;
> +	struct gicv5_its_chip_data *its;
> +	__le64 *itte;
> +	u64 itte_val, tmp;
> +	int ret = 0;

[ ... ]

> +invalidate:
> +	/* Make the change ITS-visible */
> +	dsb(st);
> +	ret = gicv5_its_itt_cache_inv(its, dev_id, event_id);

[Severity: High]
As reported by Sashiko on v1 [1], can concurrent calls to
gicv5_its_itt_cache_inv() on the same physical ITS race with each other?

Both gicv5_its_enable_direct_injection() and
gicv5_its_disable_direct_injection() invoke irq_set_vcpu_affinity(), which
only holds the per-IRQ desc->lock before calling
gicv5_its_set_vcpu_affinity().

Inside gicv5_its_itt_cache_inv():

drivers/irqchip/irq-gic-v5-its.c:gicv5_its_itt_cache_inv() {
	...
	its_writeq_relaxed(its, didr, GICV5_ITS_DIDR);
	its_writel_relaxed(its, eidr, GICV5_ITS_EIDR);
	its_writel_relaxed(its, eventr, GICV5_ITS_INV_EVENTR);

	return gicv5_its_cache_sync(its);
}

Because struct gicv5_its_chip_data has no spinlock serializing access to the
shared ITS MMIO registers, two CPUs concurrently configuring different IRQs
on the same ITS (or racing with gicv5_its_irq_domain_activate(),
gicv5_its_irq_domain_deactivate(), or gicv5_its_device_cache_inv()) can
overwrite each other's GICV5_ITS_DIDR and GICV5_ITS_EIDR values before
GICV5_ITS_INV_EVENTR is written, invalidating the wrong (DeviceID, EventID)
entry and leaving stale translations cached in the ITS.

> +	if (!ret && !data) {
> +		/*
> +		 * Invalidation sends events using the old translation to the
> +		 * IRS, but does not wait for them to be processed.
> +		 */
> +		gicv5_irs_syncr();
> +	}

[Severity: High]
Will calling gicv5_irs_syncr() here sleep in atomic context?

gicv5_its_disable_direct_injection() calls irq_set_vcpu_affinity(virq, NULL),
which acquires raw_spin_lock_irqsave(&desc->lock, flags) via
scoped_irqdesc_get_and_lock() before invoking gicv5_its_set_vcpu_affinity().

When gicv5_its_set_vcpu_affinity() calls gicv5_irs_syncr():

drivers/irqchip/irq-gic-v5-irs.c:gicv5_irs_syncr() {
	...
	syncr = FIELD_PREP(GICV5_IRS_SYNCR_SYNC, 1);
	irs_writel_relaxed(irs_data, syncr, GICV5_IRS_SYNCR);

	gicv5_wait_for_op(irs_data->irs_base, GICV5_IRS_SYNC_STATUSR,
			  GICV5_IRS_SYNC_STATUSR_IDLE);
}

Unlike gicv5_wait_for_op_atomic(), gicv5_wait_for_op() expands to
gicv5_wait_for_op_s(), which calls readl_poll_timeout() with a sleep_us of 1,
executing might_sleep_if(1) and usleep_range() while holding desc->lock with
interrupts disabled.

> +	if (ret) {
> +		/* Keep a failed restoration distinguishable from a clean host ITTE. */
> +		if (!data)
> +			irqd_set_forwarded_to_vcpu(d);
> +		return ret;
> +	}

[Severity: Medium]
What happens if polling GICV5_IRS_SYNC_STATUSR times out inside
gicv5_irs_syncr() while restoring a host ITTE?

gicv5_irs_syncr() returns void and discards the -ETIMEDOUT return value from
gicv5_wait_for_op(). If the IRS synchronization times out after
gicv5_its_itt_cache_inv() succeeds, ret remains 0.

As a result, gicv5_its_set_vcpu_affinity() will proceed to clear
IRQD_FORWARDED_TO_VCPU instead of keeping the retryable forwarded state, and
gicv5_its_disable_direct_injection() will clear IRQ_DISABLE_UNLAZY and return
0 to the caller while events using the old virtual translation may still be
in flight at the IRS. Should gicv5_irs_syncr() propagate its error so ret can
be checked here?

[1] https://lore.kernel.org/r/sashiko-outbox-174629@kernel.org

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