Re: [PATCH v2 13/16] KVM: arm64: gic-v5: Implement irq forwarding

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] KVM: arm64: gic-v5: sleep in atomic context when disabling direct injection
- [High] KVM: arm64: gic-v5: stale host_irq and leaked LPI on unforward failure
- [High] KVM: arm64: gic-v5: unserialized physical ITS MMIO in gicv5_its_itt_cache_inv()
- [Low] KVM: arm64: gic-v5: unused VGIC_V5 check in vgic_supports_direct_msis()
--

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

KVM: arm64: gic-v5: Implement irq forwarding

This commit implements direct interrupt injection (IRQ forwarding) for GICv5
via the ITS, placing the GICv4 and GICv5 forwarding hooks behind common
kvm_vgic_set_forwarding() and kvm_vgic_unset_forwarding() shims. It also
tracks the translation cache entry that owns forwarding on each LPI and uses
access_remote_vm() when resolving guest DT/ITT entries from another
process's address space.

> diff --git a/arch/arm64/kvm/vgic/vgic-its-v5.c b/arch/arm64/kvm/vgic/vgic-its-v5.c
> index 638ee47f6e27d..3648d3f7c1b29 100644
> --- a/arch/arm64/kvm/vgic/vgic-its-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-its-v5.c
> @@ -38,7 +39,35 @@ static void its_cache_drop_translation(struct kvm *kvm,
>  				       struct its_cache_translation_entry *entry)
>  {
>  	struct vgic_irq *irq = entry->irq;
> +	unsigned long flags;
> +	bool drop_forwarding_ref = false;
>  
> +	/*
> +	 * Multiple translations can share an LPI as we have no way of stopping
> +	 * the guest from doing so. Only the entry associated with direct injection
> +	 * may tear down the IRQ's forwarding state, as otherwise we can end up
> +	 * accidentally tearing something down. Realistically, this only would
> +	 * result in a performance degradation, but we might as well avoid that
> +	 * in the first place if we can.
> +	 *
> +	 * Keep the lock until the host IRQ is restored, so the producer cannot
> +	 * free it while we are using its IRQ number.
> +	 */
> +	raw_spin_lock_irqsave(&irq->irq_lock, flags);
> +	if (irq->forwarding_owner == entry) {
> +		/* The entry is about to be freed, even if unforwarding fails. */
> +		irq->forwarding_owner = NULL;
> +		if (irq->hw &&
> +		    !WARN_ON_ONCE(gicv5_its_disable_direct_injection(irq->host_irq))) {
> +			irq->hw = false;
> +			irq->host_irq = 0;
> +			drop_forwarding_ref = true;
> +		}
> +	}
> +	raw_spin_unlock_irqrestore(&irq->irq_lock, flags);

[Severity: High]
Can calling gicv5_its_disable_direct_injection() here and in
kvm_vgic_v5_unset_forwarding() sleep in atomic context?

Both its_cache_drop_translation() and kvm_vgic_v5_unset_forwarding() hold
raw_spin_lock_irqsave(&irq->irq_lock, flags) (and
kvm_arch_update_irqfd_routing() -> kvm_vgic_unset_forwarding() is also
invoked under spin_lock_irq(&kvm->irqfds.lock)). The call chain is:

  gicv5_its_disable_direct_injection()
    -> irq_set_vcpu_affinity(virq, NULL) [acquires desc->lock raw_spinlock]
      -> gicv5_its_set_vcpu_affinity(d, NULL)
        -> gicv5_irs_syncr()
          -> gicv5_wait_for_op(..., GICV5_IRS_SYNC_STATUSR,
                               GICV5_IRS_SYNC_STATUSR_IDLE)

In drivers/irqchip/irq-gic-v5-irs.c, gicv5_irs_syncr() uses the sleeping
gicv5_wait_for_op() macro (which expands to readl_poll_timeout() with a 1 us
sleep and calls might_sleep() and usleep_range()) rather than
gicv5_wait_for_op_atomic().

Should gicv5_irs_syncr() use gicv5_wait_for_op_atomic() since it is called
under desc->lock and irq->irq_lock?

[ ... ]

> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c
> index 3c0c858608752..3268e3d3d5170 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio.c
> @@ -1144,6 +1144,8 @@ int vgic_register_dist_iodev(struct kvm *kvm, gpa_t dist_base_address,
>  
>  bool vgic_supports_direct_msis(struct kvm *kvm)
>  {
> +	const struct vgic_dist *dist = &kvm->arch.vgic;
> +
>  	/*
>  	 * Deliberately conflate vLPI and vSGI support on GICv4.1 hardware,
>  	 * indirectly allowing userspace to control whether or not vPEs are
> @@ -1152,7 +1154,13 @@ bool vgic_supports_direct_msis(struct kvm *kvm)
>  	if (system_supports_direct_sgis() && !vgic_supports_direct_sgis(kvm))
>  		return false;
>  
> -	return kvm_vgic_global_state.has_gicv4 && vgic_has_its(kvm);
> +	if (kvm_vgic_global_state.has_gicv4 && vgic_has_its(kvm))
> +		return true;
> +	if (kvm_vgic_global_state.type == VGIC_V5 && vgic_has_its(kvm) &&
> +	    dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V5)
> +		return true;
> +
> +	return false;
>  }

[Severity: Low]
This isn't a bug, but is the new VGIC_V5 check in vgic_supports_direct_msis()
ever reached?

In arch/arm64/kvm/vgic/vgic.h, vgic_supports_direct_irqs() already returns
true early when vgic_is_v5(kvm) is true before calling
vgic_supports_direct_msis(kvm). The only other callers of
vgic_supports_direct_msis() are kvm_vgic_v4_set_forwarding() and
kvm_vgic_v4_unset_forwarding(), whereas kvm_vgic_v5_set_forwarding() and
kvm_vgic_v5_unset_forwarding() check !vgic_has_its(kvm) directly.

Should kvm_vgic_v5_set_forwarding() and kvm_vgic_v5_unset_forwarding() (or
the common kvm_vgic_set_forwarding() and kvm_vgic_unset_forwarding() shims)
call vgic_supports_direct_msis(kvm) instead of vgic_has_its(kvm)?

[ ... ]

> diff --git a/arch/arm64/kvm/vgic/vgic-v5.c b/arch/arm64/kvm/vgic/vgic-v5.c
> index 18184020e188b..c2ae7b0fb3a7f 100644
> --- a/arch/arm64/kvm/vgic/vgic-v5.c
> +++ b/arch/arm64/kvm/vgic/vgic-v5.c
> @@ -1635,3 +1635,151 @@ unsigned long vgic_v5_handle_guest_coresight_idreg_read(u32 offset)
> +int kvm_vgic_v5_set_forwarding(struct kvm *kvm, int virq,
> +			       struct kvm_kernel_irq_routing_entry *irq_entry)
> +{

[ ... ]

> +	ret = gicv5_its_enable_direct_injection(virq, vgic_v5_vm_id(kvm),
> +						vgic_v5_get_hwirq_id(irq->intid));

[Severity: High]
What serializes concurrent physical ITS MMIO register accesses in
gicv5_its_itt_cache_inv() when forwarding or unforwarding interrupts?

Enabling and disabling direct injection calls:

  gicv5_its_enable_direct_injection() / gicv5_its_disable_direct_injection()
    -> irq_set_vcpu_affinity() [locks per-IRQ desc->lock]
      -> gicv5_its_set_vcpu_affinity()
        -> gicv5_its_itt_cache_inv()

In drivers/irqchip/irq-gic-v5-its.c, gicv5_its_itt_cache_inv() writes the
shared physical ITS registers and waits for completion without a per-ITS lock
in struct gicv5_its_chip_data:

	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 irq_set_vcpu_affinity() only holds the per-IRQ desc->lock and
kvm_vgic_v5_unset_forwarding() only holds the per-LPI irq->irq_lock (without
its->its_lock), two CPUs forwarding, unforwarding, or invalidating different
IRQs on the same physical ITS (within the same VM, across VMs, or racing with
host gicv5_its_map_event() / gicv5_its_unmap_event()) can interleave writes
to GICV5_ITS_DIDR and GICV5_ITS_EIDR before GICV5_ITS_INV_EVENTR is written.

Could one CPU overwrite GICV5_ITS_DIDR or GICV5_ITS_EIDR so that the physical
ITS invalidates the wrong DeviceID/EventID entry while gicv5_its_cache_sync()
still sees GICV5_ITS_STATUSR_IDLE and returns success, leaving a stale
virtual or host ITTE cached in the physical ITS?

> +	if (ret) {
> +		if (ret != -ETIMEDOUT) {
> +			vgic_put_irq(kvm, irq);
> +			return 0;
> +		}

[ ... ]

> +void kvm_vgic_v5_unset_forwarding(struct kvm *kvm, int virq)
> +{
> +	struct vgic_irq *irq;
> +	bool drop_forwarding_ref = false;
> +
> +	if (!vgic_has_its(kvm))
> +		return;
> +
> +	/*
> +	 * Note: We don't remove the entry from our translation cache here. It
> +	 * is up to the guest to invalidate the old translations, and hence the
> +	 * removal path is tied into ITS_INV_DEVICER and ITS_INV_EVENTR, rather
> +	 * than being explicitly called here.
> +	 */
> +	irq = vgic_v5_host_irq_get_lpi(kvm, virq);
> +	if (!irq)
> +		return;
> +
> +	scoped_guard(raw_spinlock_irqsave, &irq->irq_lock) {
> +		if (irq->hw && irq->host_irq == virq &&
> +		    !WARN_ON_ONCE(gicv5_its_disable_direct_injection(virq))) {
> +			irq->hw = false;
> +			irq->host_irq = 0;
> +			irq->forwarding_owner = NULL;
> +			drop_forwarding_ref = true;
> +		}
> +	}
> +
> +	if (drop_forwarding_ref)
> +		vgic_put_irq(kvm, irq);
> +
> +	vgic_put_irq(kvm, irq);
> +}

[Severity: High]
If gicv5_its_disable_direct_injection(virq) fails in
kvm_vgic_v5_unset_forwarding(), can a subsequent cache eviction operate on a
freed or reallocated host Linux IRQ number, or leak the LPI and reuse the VM
ID at teardown?

1. kvm_arch_irq_bypass_del_producer() returns void to __disconnect() in
   virt/lib/irqbypass.c, so producer unregistration cannot be aborted when
   gicv5_its_disable_direct_injection(virq) fails. Once
   irq_bypass_unregister_producer() returns, the producer (e.g. VFIO) tears
   down and frees virq, which can then be reallocated to another device.
   Because irq->forwarding_owner, irq->hw, and irq->host_irq remain set here,
   a later guest invalidation or vgic_v5_its_destroy() ->
   its_cache_clear_translations() -> its_cache_drop_translation() will call
   gicv5_its_disable_direct_injection(irq->host_irq) on that freed or
   reallocated virq.

2. Similarly, if kvm_arch_update_irqfd_routing() calls
   kvm_vgic_unset_forwarding() when an irqfd routing entry changes from
   KVM_IRQ_ROUTING_MSI to a non-MSI type and
   gicv5_its_disable_direct_injection() fails, a subsequent
   kvm_arch_irq_bypass_del_producer() sees irq_entry->type !=
   KVM_IRQ_ROUTING_MSI and returns early without retrying
   kvm_vgic_unset_forwarding().

3. If gicv5_its_disable_direct_injection() fails in both
   its_cache_drop_translation() and kvm_vgic_v5_unset_forwarding(),
   drop_forwarding_ref is never set. When the VM is destroyed,
   kvm_vgic_dist_destroy() runs xa_destroy(&dist->lpi_xa) and leaks the
   struct vgic_irq, while vgic_v5_teardown() unconditionally calls
   vgic_v5_release_vm_id(kvm) even though a physical ITS virtual ITTE may
   still reference that VM ID.

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