Re: [PATCH 3/5] KVM: arm64: Resolve vLPI by host IRQ in vgic_v4_unset_forwarding()

[Marc Zyngier <maz@kernel.org>]

On Fri, 23 May 2025 17:08:08 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> The virtual mapping and "GSI" routing of a particular vLPI is subject to
> change in response to the guest / userspace. This can be pretty annoying
> to deal with when KVM needs to track the physical state that's managed
> for vLPI direct injection.
> 
> Make vgic_v4_unset_forwarding() resilient by using the host IRQ to
> resolve the vgic IRQ.
> 
> Tested-by: Sweet Tea Dorminy <sweettea@google.com>
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/arm.c          |  3 +--
>  arch/arm64/kvm/vgic/vgic-v4.c | 45 +++++++++++++++++++----------------
>  include/kvm/arm_vgic.h        |  3 +--
>  3 files changed, 27 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 36cfcffb40d8..1de49b48e35e 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -2800,8 +2800,7 @@ void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
>  	if (irq_entry->type != KVM_IRQ_ROUTING_MSI)
>  		return;
>  
> -	kvm_vgic_v4_unset_forwarding(irqfd->kvm, prod->irq,
> -				     &irqfd->irq_entry);
> +	kvm_vgic_v4_unset_forwarding(irqfd->kvm, prod->irq);
>  }
>  
>  void kvm_arch_irq_bypass_stop(struct irq_bypass_consumer *cons)
> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
> index 01a5de8e9e94..193946108192 100644
> --- a/arch/arm64/kvm/vgic/vgic-v4.c
> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
> @@ -508,10 +508,27 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
>  	return ret;
>  }
>  
> -int kvm_vgic_v4_unset_forwarding(struct kvm *kvm, int virq,
> -				 struct kvm_kernel_irq_routing_entry *irq_entry)
> +static struct vgic_irq *__vgic_host_irq_get_vlpi(struct kvm *kvm, int host_irq)
> +{
> +	struct vgic_irq *irq;
> +	unsigned long idx;
> +
> +	guard(rcu)();
> +	xa_for_each(&kvm->arch.vgic.lpi_xa, idx, irq) {
> +		if (!irq->hw || irq->host_irq != host_irq)
> +			continue;
> +
> +		if (!vgic_try_get_irq_kref(irq))
> +			return NULL;
> +
> +		return irq;
> +	}
> +
> +	return NULL;
> +}
> +
> +int kvm_vgic_v4_unset_forwarding(struct kvm *kvm, int host_irq)
>  {
> -	struct vgic_its *its;
>  	struct vgic_irq *irq;
>  	unsigned long flags;
>  	int ret = 0;
> @@ -519,31 +536,19 @@ int kvm_vgic_v4_unset_forwarding(struct kvm *kvm, int virq,
>  	if (!vgic_supports_direct_msis(kvm))
>  		return 0;
>  
> -	/*
> -	 * Get the ITS, and escape early on error (not a valid
> -	 * doorbell for any of our vITSs).
> -	 */
> -	its = vgic_get_its(kvm, irq_entry);
> -	if (IS_ERR(its))
> +	irq = __vgic_host_irq_get_vlpi(kvm, host_irq);
> +	if (!irq)
>  		return 0;
>  
> -	mutex_lock(&its->its_lock);
> -
> -	ret = vgic_its_resolve_lpi(kvm, its, irq_entry->msi.devid,
> -				   irq_entry->msi.data, &irq);
> -	if (ret)
> -		goto out;
> -

Removing the reliance on the ITS locking is another thing that could
be mentioned in the commit message, as it is slightly surprising to
see it here, given that the previous patch is all about that.

My other gripe is that we lose the doorbell validation. I'm not sure
it is a big deal, but I'd rather we keep verifying we're not being fed
rubbish data, in case we need to rely on that in the future.

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.