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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] irqchip/gic-v5: Racy ITS MMIO invalidation sequence
- [High] irqchip/gic-v5: missing event drain in set_vcpu_affinity causes cross-VM interrupt
--

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

irqchip/gic-v5: Enable direct injection in ITS

This commit introduces an irq_chip callback to allow KVM to install and remove
virtual translations for direct interrupt injection. It replaces a host ITTE
with a virtual translation and disables lazy masking while the interrupt is
forwarded.

> diff --git a/drivers/irqchip/irq-gic-v5-its.c b/drivers/irqchip/irq-gic-v5-its.c
> index 28e39b065de0e..7de40a7cff403 100644
> --- a/drivers/irqchip/irq-gic-v5-its.c
> +++ b/drivers/irqchip/irq-gic-v5-its.c
[ ... ]
> @@ -723,6 +765,95 @@ 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)
> +{
[ ... ]
> +invalidate:
> +	/* Make the change ITS-visible */
> +	dsb(st);
> +	ret = gicv5_its_itt_cache_inv(its, dev_id, event_id);

[Severity: High]
Does this new KVM callback expose the gicv5_its_itt_cache_inv() sequence
to concurrent execution?

This callback appears to only be serialized by the per-IRQ lock, but
gicv5_its_itt_cache_inv() writes to the global shared ITS MMIO registers:

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

Without an ITS-wide spinlock, concurrent configuration of different IRQs/vCPUs
from the same or different guests could freely interleave their MMIO writes on
the shared physical ITS. 

Could this result in the ITS hardware receiving corrupted invalidation commands
(e.g. invalidating the wrong device/event while leaving another stale), leading
to dropped or misrouted interrupts?


[Severity: High]
Are in-flight events drained here when direct injection is disabled?

When disabling direct injection (where data is NULL), the ITTE is updated to
the host LPI and the ITS cache is invalidated. However, there is no call to
gicv5_its_syncr() or gicv5_irs_syncr() to drain accepted events before
returning.

If KVM subsequently tears down the VM and reuses the VM ID for a new guest,
could an in-flight event reach the IRS and be injected into the new VM's IST
using the old translation? 

Disabling forwarding would likely need to drain events before returning to
safely reuse the resources.

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

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