[PATCH 0/5] KVM: arm64: Some VGIC-related fixes

[PATCH 0/5] KVM: arm64: Some VGIC-related fixes

[Oliver Upton]

Flushing out some patches I had sitting around before vacation:

 - The irqbypass hooks are not resilient to changes in the GSI<->MSI
   routing and can sometimes leave behind stale vLPI mappings for the
   VM. The fix is to resolve the VGIC IRQ using the host IRQ (which is
   stable) and nuking the vLPI mapping upon a routing change.

 - Closing another VGIC race where vCPU creation races with VGIC
   creation, leading to in-flight vCPUs entering the kernel w/o private
   IRQs allocated.

Applies to kvmarm-6.16.

Oliver Upton (5):
  KVM: arm64: Use lock guard in vgic_v4_set_forwarding()
  KVM: arm64: Protect vLPI translation with vgic_irq::irq_lock
  KVM: arm64: Resolve vLPI by host IRQ in vgic_v4_unset_forwarding()
  KVM: arm64: Unmap vLPIs affected by changes to GSI routing information
  KVM: arm64: vgic-init: Plug vCPU vs. VGIC creation race

 arch/arm64/kvm/arm.c            | 26 +++++++++-
 arch/arm64/kvm/vgic/vgic-init.c | 27 +++++++++-
 arch/arm64/kvm/vgic/vgic-its.c  | 48 ++++++++---------
 arch/arm64/kvm/vgic/vgic-v4.c   | 92 ++++++++++++++++++---------------
 include/kvm/arm_vgic.h          |  3 +-
 5 files changed, 125 insertions(+), 71 deletions(-)


base-commit: 1b85d923ba8c9e6afaf19e26708411adde94fba8
-- 
2.39.5

[PATCH 1/5] KVM: arm64: Use lock guard in vgic_v4_set_forwarding()

[Oliver Upton]

The locking dance is about to get more interesting, switch the its_lock
over to a lock guard to make it a bit easier to handle.

Tested-by: Sweet Tea Dorminy <sweettea@google.com>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/vgic/vgic-v4.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
index c7de6154627c..8b25e7650998 100644
--- a/arch/arm64/kvm/vgic/vgic-v4.c
+++ b/arch/arm64/kvm/vgic/vgic-v4.c
@@ -444,7 +444,7 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
 	if (IS_ERR(its))
 		return 0;
 
-	mutex_lock(&its->its_lock);
+	guard(mutex)(&its->its_lock);
 
 	/*
 	 * Perform the actual DevID/EventID -> LPI translation.
@@ -455,11 +455,11 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
 	 */
 	if (vgic_its_resolve_lpi(kvm, its, irq_entry->msi.devid,
 				 irq_entry->msi.data, &irq))
-		goto out;
+		return 0;
 
 	/* Silently exit if the vLPI is already mapped */
 	if (irq->hw)
-		goto out;
+		return 0;
 
 	/*
 	 * Emit the mapping request. If it fails, the ITS probably
@@ -479,7 +479,7 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
 
 	ret = its_map_vlpi(virq, &map);
 	if (ret)
-		goto out;
+		return ret;
 
 	irq->hw		= true;
 	irq->host_irq	= virq;
@@ -503,8 +503,6 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
 		raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
 	}
 
-out:
-	mutex_unlock(&its->its_lock);
 	return ret;
 }
 
-- 
2.39.5

[PATCH 2/5] KVM: arm64: Protect vLPI translation with vgic_irq::irq_lock

[Oliver Upton]

Though undocumented, KVM generally protects the translation of a vLPI
with the its_lock. While this makes perfectly good sense, as the ITS
itself contains the guest translation, an upcoming change will require
twiddling the vLPI mapping in an atomic context.

Switch to using the vIRQ's irq_lock to protect the translation.

Tested-by: Sweet Tea Dorminy <sweettea@google.com>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/vgic/vgic-its.c | 48 +++++++++++++++++-----------------
 arch/arm64/kvm/vgic/vgic-v4.c  | 41 ++++++++++++++++-------------
 2 files changed, 47 insertions(+), 42 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 569f9da9049f..beca12dae779 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -306,39 +306,34 @@ static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq,
 		}
 	}
 
-	raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
-
 	if (irq->hw)
-		return its_prop_update_vlpi(irq->host_irq, prop, needs_inv);
+		ret = its_prop_update_vlpi(irq->host_irq, prop, needs_inv);
 
-	return 0;
+	raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
+	return ret;
 }
 
 static int update_affinity(struct vgic_irq *irq, struct kvm_vcpu *vcpu)
 {
-	int ret = 0;
-	unsigned long flags;
+	struct its_vlpi_map map;
+	int ret;
 
-	raw_spin_lock_irqsave(&irq->irq_lock, flags);
+	guard(raw_spinlock_irqsave)(&irq->irq_lock);
 	irq->target_vcpu = vcpu;
-	raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
 
-	if (irq->hw) {
-		struct its_vlpi_map map;
-
-		ret = its_get_vlpi(irq->host_irq, &map);
-		if (ret)
-			return ret;
+	if (!irq->hw)
+		return 0;
 
-		if (map.vpe)
-			atomic_dec(&map.vpe->vlpi_count);
-		map.vpe = &vcpu->arch.vgic_cpu.vgic_v3.its_vpe;
-		atomic_inc(&map.vpe->vlpi_count);
+	ret = its_get_vlpi(irq->host_irq, &map);
+	if (ret)
+		return ret;
 
-		ret = its_map_vlpi(irq->host_irq, &map);
-	}
+	if (map.vpe)
+		atomic_dec(&map.vpe->vlpi_count);
 
-	return ret;
+	map.vpe = &vcpu->arch.vgic_cpu.vgic_v3.its_vpe;
+	atomic_inc(&map.vpe->vlpi_count);
+	return its_map_vlpi(irq->host_irq, &map);
 }
 
 static struct kvm_vcpu *collection_to_vcpu(struct kvm *kvm,
@@ -756,12 +751,17 @@ int vgic_its_inject_msi(struct kvm *kvm, struct kvm_msi *msi)
 /* Requires the its_lock to be held. */
 static void its_free_ite(struct kvm *kvm, struct its_ite *ite)
 {
+	struct vgic_irq *irq = ite->irq;
 	list_del(&ite->ite_list);
 
 	/* This put matches the get in vgic_add_lpi. */
-	if (ite->irq) {
-		if (ite->irq->hw)
-			WARN_ON(its_unmap_vlpi(ite->irq->host_irq));
+	if (irq) {
+		scoped_guard(raw_spinlock_irqsave, &irq->irq_lock) {
+			if (irq->hw)
+				WARN_ON(its_unmap_vlpi(ite->irq->host_irq));
+
+			irq->hw = false;
+		}
 
 		vgic_put_irq(kvm, ite->irq);
 	}
diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
index 8b25e7650998..01a5de8e9e94 100644
--- a/arch/arm64/kvm/vgic/vgic-v4.c
+++ b/arch/arm64/kvm/vgic/vgic-v4.c
@@ -457,9 +457,11 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
 				 irq_entry->msi.data, &irq))
 		return 0;
 
+	raw_spin_lock_irqsave(&irq->irq_lock, flags);
+
 	/* Silently exit if the vLPI is already mapped */
 	if (irq->hw)
-		return 0;
+		goto out_unlock_irq;
 
 	/*
 	 * Emit the mapping request. If it fails, the ITS probably
@@ -479,30 +481,30 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
 
 	ret = its_map_vlpi(virq, &map);
 	if (ret)
-		return ret;
+		goto out_unlock_irq;
 
 	irq->hw		= true;
 	irq->host_irq	= virq;
 	atomic_inc(&map.vpe->vlpi_count);
 
 	/* Transfer pending state */
-	raw_spin_lock_irqsave(&irq->irq_lock, flags);
-	if (irq->pending_latch) {
-		ret = irq_set_irqchip_state(irq->host_irq,
-					    IRQCHIP_STATE_PENDING,
-					    irq->pending_latch);
-		WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq);
+	if (!irq->pending_latch)
+		goto out_unlock_irq;
 
-		/*
-		 * Clear pending_latch and communicate this state
-		 * change via vgic_queue_irq_unlock.
-		 */
-		irq->pending_latch = false;
-		vgic_queue_irq_unlock(kvm, irq, flags);
-	} else {
-		raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
-	}
+	ret = irq_set_irqchip_state(irq->host_irq, IRQCHIP_STATE_PENDING,
+				    irq->pending_latch);
+	WARN_RATELIMIT(ret, "IRQ %d", irq->host_irq);
+
+	/*
+	 * Clear pending_latch and communicate this state
+	 * change via vgic_queue_irq_unlock.
+	 */
+	irq->pending_latch = false;
+	vgic_queue_irq_unlock(kvm, irq, flags);
+	return ret;
 
+out_unlock_irq:
+	raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
 	return ret;
 }
 
@@ -511,7 +513,8 @@ int kvm_vgic_v4_unset_forwarding(struct kvm *kvm, int virq,
 {
 	struct vgic_its *its;
 	struct vgic_irq *irq;
-	int ret;
+	unsigned long flags;
+	int ret = 0;
 
 	if (!vgic_supports_direct_msis(kvm))
 		return 0;
@@ -531,6 +534,7 @@ int kvm_vgic_v4_unset_forwarding(struct kvm *kvm, int virq,
 	if (ret)
 		goto out;
 
+	raw_spin_lock_irqsave(&irq->irq_lock, flags);
 	WARN_ON(irq->hw && irq->host_irq != virq);
 	if (irq->hw) {
 		atomic_dec(&irq->target_vcpu->arch.vgic_cpu.vgic_v3.its_vpe.vlpi_count);
@@ -538,6 +542,7 @@ int kvm_vgic_v4_unset_forwarding(struct kvm *kvm, int virq,
 		ret = its_unmap_vlpi(virq);
 	}
 
+	raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
 out:
 	mutex_unlock(&its->its_lock);
 	return ret;
-- 
2.39.5

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

[Oliver Upton]

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;
-
 	raw_spin_lock_irqsave(&irq->irq_lock, flags);
-	WARN_ON(irq->hw && irq->host_irq != virq);
+	WARN_ON(irq->hw && irq->host_irq != host_irq);
 	if (irq->hw) {
 		atomic_dec(&irq->target_vcpu->arch.vgic_cpu.vgic_v3.its_vpe.vlpi_count);
 		irq->hw = false;
-		ret = its_unmap_vlpi(virq);
+		ret = its_unmap_vlpi(host_irq);
 	}
 
 	raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
-out:
-	mutex_unlock(&its->its_lock);
+	vgic_put_irq(kvm, irq);
 	return ret;
 }
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 714cef854c1c..4a34f7f0a864 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -434,8 +434,7 @@ struct kvm_kernel_irq_routing_entry;
 int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int irq,
 			       struct kvm_kernel_irq_routing_entry *irq_entry);
 
-int kvm_vgic_v4_unset_forwarding(struct kvm *kvm, int irq,
-				 struct kvm_kernel_irq_routing_entry *irq_entry);
+int kvm_vgic_v4_unset_forwarding(struct kvm *kvm, int host_irq);
 
 int vgic_v4_load(struct kvm_vcpu *vcpu);
 void vgic_v4_commit(struct kvm_vcpu *vcpu);
-- 
2.39.5

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

[Marc Zyngier]

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.

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

[Oliver Upton]

Hey,

On Fri, May 23, 2025 at 06:25:50PM +0100, Marc Zyngier wrote:
> > +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.

Sure, I can add a bit of color. The prior patch left the its_lock in
place since it is still needed to walk the ITS device / ITE lists.

> 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.

We still get doorbell validation on the set() side of things, which IMO
is where it belongs. Whether or not the MSI routing is garbage at time
of unset() isn't relevant, if we handed out a vLPI mapping we need to
kill it.

Thanks,
Oliver

[PATCH 4/5] KVM: arm64: Unmap vLPIs affected by changes to GSI routing information

[Oliver Upton]

KVM's interrupt infrastructure is dodgy at best, allowing for some ugly
'off label' usage of the various UAPIs. In one example, userspace can
change the routing entry of a particular "GSI" after configuring
irqbypass with KVM_IRQFD. KVM/arm64 is oblivious to this, and winds up
preserving the stale translation in cases where vLPIs are configured.

Honor userspace's intentions and tear down the vLPI mapping if affected
by a "GSI" routing change. Make no attempt to reconstruct vLPIs if the
new target is an MSI and just fall back to software injection.

Tested-by: Sweet Tea Dorminy <sweettea@google.com>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/arm.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 1de49b48e35e..505d504b52b5 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -2790,6 +2790,7 @@ int kvm_arch_irq_bypass_add_producer(struct irq_bypass_consumer *cons,
 	return kvm_vgic_v4_set_forwarding(irqfd->kvm, prod->irq,
 					  &irqfd->irq_entry);
 }
+
 void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
 				      struct irq_bypass_producer *prod)
 {
@@ -2803,6 +2804,28 @@ void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
 	kvm_vgic_v4_unset_forwarding(irqfd->kvm, prod->irq);
 }
 
+bool kvm_arch_irqfd_route_changed(struct kvm_kernel_irq_routing_entry *old,
+				  struct kvm_kernel_irq_routing_entry *new)
+{
+	if (new->type != KVM_IRQ_ROUTING_MSI)
+		return true;
+
+	return memcmp(&old->msi, &new->msi, sizeof(new->msi));
+}
+
+int kvm_arch_update_irqfd_routing(struct kvm *kvm, unsigned int host_irq,
+				  uint32_t guest_irq, bool set)
+{
+	/*
+	 * Remapping the vLPI requires taking the its_lock mutex to resolve
+	 * the new translation. We're in spinlock land at this point, so no
+	 * chance of resolving the translation.
+	 *
+	 * Unmap the vLPI and fall back to software LPI injection.
+	 */
+	return kvm_vgic_v4_unset_forwarding(kvm, host_irq);
+}
+
 void kvm_arch_irq_bypass_stop(struct irq_bypass_consumer *cons)
 {
 	struct kvm_kernel_irqfd *irqfd =
-- 
2.39.5

Re: [PATCH 4/5] KVM: arm64: Unmap vLPIs affected by changes to GSI routing information

[Marc Zyngier]

On Fri, 23 May 2025 17:08:09 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> KVM's interrupt infrastructure is dodgy at best, allowing for some ugly
> 'off label' usage of the various UAPIs. In one example, userspace can
> change the routing entry of a particular "GSI" after configuring
> irqbypass with KVM_IRQFD. KVM/arm64 is oblivious to this, and winds up
> preserving the stale translation in cases where vLPIs are configured.
> 
> Honor userspace's intentions and tear down the vLPI mapping if affected
> by a "GSI" routing change. Make no attempt to reconstruct vLPIs if the
> new target is an MSI and just fall back to software injection.
> 
> Tested-by: Sweet Tea Dorminy <sweettea@google.com>
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/arm.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 1de49b48e35e..505d504b52b5 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -2790,6 +2790,7 @@ int kvm_arch_irq_bypass_add_producer(struct irq_bypass_consumer *cons,
>  	return kvm_vgic_v4_set_forwarding(irqfd->kvm, prod->irq,
>  					  &irqfd->irq_entry);
>  }
> +
>  void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
>  				      struct irq_bypass_producer *prod)
>  {
> @@ -2803,6 +2804,28 @@ void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
>  	kvm_vgic_v4_unset_forwarding(irqfd->kvm, prod->irq);
>  }
>  
> +bool kvm_arch_irqfd_route_changed(struct kvm_kernel_irq_routing_entry *old,
> +				  struct kvm_kernel_irq_routing_entry *new)
> +{
> +	if (new->type != KVM_IRQ_ROUTING_MSI)
> +		return true;
> +
> +	return memcmp(&old->msi, &new->msi, sizeof(new->msi));
> +}
> +
> +int kvm_arch_update_irqfd_routing(struct kvm *kvm, unsigned int host_irq,
> +				  uint32_t guest_irq, bool set)

If we're adding this, can we take out the trash and get rid of this
'set' parameter? Its only purpose is to be set to '1' and fed to the
x86-specific stuff. How about this:

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index df5b99ea1f181..2d69609c1ec00 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -13613,9 +13613,9 @@ void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
 }
 
 int kvm_arch_update_irqfd_routing(struct kvm *kvm, unsigned int host_irq,
-				   uint32_t guest_irq, bool set)
+				   uint32_t guest_irq)
 {
-	return kvm_x86_call(pi_update_irte)(kvm, host_irq, guest_irq, set);
+	return kvm_x86_call(pi_update_irte)(kvm, host_irq, guest_irq, true);
 }
 
 bool kvm_arch_irqfd_route_changed(struct kvm_kernel_irq_routing_entry *old,
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 11e5d1e3f12ea..2e60d0bf02e2f 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -287,7 +287,7 @@ void __attribute__((weak)) kvm_arch_irq_bypass_start(
 
 int  __attribute__((weak)) kvm_arch_update_irqfd_routing(
 				struct kvm *kvm, unsigned int host_irq,
-				uint32_t guest_irq, bool set)
+				uint32_t guest_irq)
 {
 	return 0;
 }
@@ -621,7 +621,7 @@ void kvm_irq_routing_update(struct kvm *kvm)
 		    kvm_arch_irqfd_route_changed(&old, &irqfd->irq_entry)) {
 			int ret = kvm_arch_update_irqfd_routing(
 					irqfd->kvm, irqfd->producer->irq,
-					irqfd->gsi, 1);
+					irqfd->gsi);
 			WARN_ON(ret);
 		}
 #endif


Thanks,

	M.

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

Re: [PATCH 4/5] KVM: arm64: Unmap vLPIs affected by changes to GSI routing information

[Sean Christopherson]

On Fri, May 23, 2025, Marc Zyngier wrote:
> On Fri, 23 May 2025 17:08:09 +0100,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > KVM's interrupt infrastructure is dodgy at best, allowing for some ugly
> > 'off label' usage of the various UAPIs. In one example, userspace can
> > change the routing entry of a particular "GSI" after configuring
> > irqbypass with KVM_IRQFD. KVM/arm64 is oblivious to this, and winds up
> > preserving the stale translation in cases where vLPIs are configured.
> > 
> > Honor userspace's intentions and tear down the vLPI mapping if affected
> > by a "GSI" routing change. Make no attempt to reconstruct vLPIs if the
> > new target is an MSI and just fall back to software injection.
> > 
> > Tested-by: Sweet Tea Dorminy <sweettea@google.com>
> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > ---
> >  arch/arm64/kvm/arm.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index 1de49b48e35e..505d504b52b5 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -2790,6 +2790,7 @@ int kvm_arch_irq_bypass_add_producer(struct irq_bypass_consumer *cons,
> >  	return kvm_vgic_v4_set_forwarding(irqfd->kvm, prod->irq,
> >  					  &irqfd->irq_entry);
> >  }
> > +
> >  void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
> >  				      struct irq_bypass_producer *prod)
> >  {
> > @@ -2803,6 +2804,28 @@ void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
> >  	kvm_vgic_v4_unset_forwarding(irqfd->kvm, prod->irq);
> >  }
> >  
> > +bool kvm_arch_irqfd_route_changed(struct kvm_kernel_irq_routing_entry *old,
> > +				  struct kvm_kernel_irq_routing_entry *new)
> > +{
> > +	if (new->type != KVM_IRQ_ROUTING_MSI)
> > +		return true;
> > +
> > +	return memcmp(&old->msi, &new->msi, sizeof(new->msi));
> > +}
> > +
> > +int kvm_arch_update_irqfd_routing(struct kvm *kvm, unsigned int host_irq,
> > +				  uint32_t guest_irq, bool set)
> 
> If we're adding this, can we take out the trash and get rid of this
> 'set' parameter? Its only purpose is to be set to '1' and fed to the
> x86-specific stuff. How about this:

Can we hold off on any changes to the common APIs?  Unless they're urgent and
need to land in 6.16, I've got a massive series that I've been slowing working
on for ~7 months that fixes this wart, and several more, e.g. gets rid of
kvm_arch_irqfd_route_changed() entirely, and make the above ugliness into:

void kvm_arch_update_irqfd_routing(struct kvm_kernel_irqfd *irqfd,
				   struct kvm_kernel_irq_routing_entry *old,
				   struct kvm_kernel_irq_routing_entry *new)


I'm ~90% confident it'll land in 6.17:
https://lore.kernel.org/all/20250523010004.3240643-1-seanjc@google.com

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index df5b99ea1f181..2d69609c1ec00 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -13613,9 +13613,9 @@ void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
>  }
>  
>  int kvm_arch_update_irqfd_routing(struct kvm *kvm, unsigned int host_irq,
> -				   uint32_t guest_irq, bool set)
> +				   uint32_t guest_irq)
>  {
> -	return kvm_x86_call(pi_update_irte)(kvm, host_irq, guest_irq, set);
> +	return kvm_x86_call(pi_update_irte)(kvm, host_irq, guest_irq, true);
>  }
>  
>  bool kvm_arch_irqfd_route_changed(struct kvm_kernel_irq_routing_entry *old,
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index 11e5d1e3f12ea..2e60d0bf02e2f 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -287,7 +287,7 @@ void __attribute__((weak)) kvm_arch_irq_bypass_start(
>  
>  int  __attribute__((weak)) kvm_arch_update_irqfd_routing(
>  				struct kvm *kvm, unsigned int host_irq,
> -				uint32_t guest_irq, bool set)
> +				uint32_t guest_irq)
>  {
>  	return 0;
>  }
> @@ -621,7 +621,7 @@ void kvm_irq_routing_update(struct kvm *kvm)
>  		    kvm_arch_irqfd_route_changed(&old, &irqfd->irq_entry)) {
>  			int ret = kvm_arch_update_irqfd_routing(
>  					irqfd->kvm, irqfd->producer->irq,
> -					irqfd->gsi, 1);
> +					irqfd->gsi);
>  			WARN_ON(ret);
>  		}
>  #endif
> 
> 
> Thanks,
> 
> 	M.
> 
> -- 
> Jazz isn't dead. It just smells funny.

Re: [PATCH 4/5] KVM: arm64: Unmap vLPIs affected by changes to GSI routing information

[Marc Zyngier]

On Fri, 23 May 2025 18:48:02 +0100,
Sean Christopherson <seanjc@google.com> wrote:
> 
> On Fri, May 23, 2025, Marc Zyngier wrote:
> > On Fri, 23 May 2025 17:08:09 +0100,
> > Oliver Upton <oliver.upton@linux.dev> wrote:
> > > 
> > > KVM's interrupt infrastructure is dodgy at best, allowing for some ugly
> > > 'off label' usage of the various UAPIs. In one example, userspace can
> > > change the routing entry of a particular "GSI" after configuring
> > > irqbypass with KVM_IRQFD. KVM/arm64 is oblivious to this, and winds up
> > > preserving the stale translation in cases where vLPIs are configured.
> > > 
> > > Honor userspace's intentions and tear down the vLPI mapping if affected
> > > by a "GSI" routing change. Make no attempt to reconstruct vLPIs if the
> > > new target is an MSI and just fall back to software injection.
> > > 
> > > Tested-by: Sweet Tea Dorminy <sweettea@google.com>
> > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > > ---
> > >  arch/arm64/kvm/arm.c | 23 +++++++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > > 
> > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > > index 1de49b48e35e..505d504b52b5 100644
> > > --- a/arch/arm64/kvm/arm.c
> > > +++ b/arch/arm64/kvm/arm.c
> > > @@ -2790,6 +2790,7 @@ int kvm_arch_irq_bypass_add_producer(struct irq_bypass_consumer *cons,
> > >  	return kvm_vgic_v4_set_forwarding(irqfd->kvm, prod->irq,
> > >  					  &irqfd->irq_entry);
> > >  }
> > > +
> > >  void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
> > >  				      struct irq_bypass_producer *prod)
> > >  {
> > > @@ -2803,6 +2804,28 @@ void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
> > >  	kvm_vgic_v4_unset_forwarding(irqfd->kvm, prod->irq);
> > >  }
> > >  
> > > +bool kvm_arch_irqfd_route_changed(struct kvm_kernel_irq_routing_entry *old,
> > > +				  struct kvm_kernel_irq_routing_entry *new)
> > > +{
> > > +	if (new->type != KVM_IRQ_ROUTING_MSI)
> > > +		return true;
> > > +
> > > +	return memcmp(&old->msi, &new->msi, sizeof(new->msi));
> > > +}
> > > +
> > > +int kvm_arch_update_irqfd_routing(struct kvm *kvm, unsigned int host_irq,
> > > +				  uint32_t guest_irq, bool set)
> > 
> > If we're adding this, can we take out the trash and get rid of this
> > 'set' parameter? Its only purpose is to be set to '1' and fed to the
> > x86-specific stuff. How about this:
> 
> Can we hold off on any changes to the common APIs?  Unless they're urgent and
> need to land in 6.16, I've got a massive series that I've been slowing working
> on for ~7 months that fixes this wart, and several more, e.g. gets rid of
> kvm_arch_irqfd_route_changed() entirely, and make the above ugliness into:
> 
> void kvm_arch_update_irqfd_routing(struct kvm_kernel_irqfd *irqfd,
> 				   struct kvm_kernel_irq_routing_entry *old,
> 				   struct kvm_kernel_irq_routing_entry *new)
> 
> 
> I'm ~90% confident it'll land in 6.17:
> https://lore.kernel.org/all/20250523010004.3240643-1-seanjc@google.com

Sure, if you want to preserve this fossil for another round, I don't
really care. But if you are planning your grand plan for 6.17, you
could as well rebase on top of theses patches which are aimed at -rc1
(yes, they are bug fixes), and it would then be perfectly acceptable
to have this cleanup early.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 4/5] KVM: arm64: Unmap vLPIs affected by changes to GSI routing information

[Sean Christopherson]

On Fri, May 23, 2025, Marc Zyngier wrote:
> On Fri, 23 May 2025 18:48:02 +0100,
> Sean Christopherson <seanjc@google.com> wrote:
> > 
> > On Fri, May 23, 2025, Marc Zyngier wrote:
> > > > @@ -2803,6 +2804,28 @@ void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
> > > >  	kvm_vgic_v4_unset_forwarding(irqfd->kvm, prod->irq);
> > > >  }
> > > >  
> > > > +bool kvm_arch_irqfd_route_changed(struct kvm_kernel_irq_routing_entry *old,
> > > > +				  struct kvm_kernel_irq_routing_entry *new)
> > > > +{
> > > > +	if (new->type != KVM_IRQ_ROUTING_MSI)
> > > > +		return true;
> > > > +
> > > > +	return memcmp(&old->msi, &new->msi, sizeof(new->msi));
> > > > +}
> > > > +
> > > > +int kvm_arch_update_irqfd_routing(struct kvm *kvm, unsigned int host_irq,
> > > > +				  uint32_t guest_irq, bool set)
> > > 
> > > If we're adding this, can we take out the trash and get rid of this
> > > 'set' parameter? Its only purpose is to be set to '1' and fed to the
> > > x86-specific stuff. How about this:
> > 
> > Can we hold off on any changes to the common APIs?  Unless they're urgent and
> > need to land in 6.16, I've got a massive series that I've been slowing working
> > on for ~7 months that fixes this wart, and several more, e.g. gets rid of
> > kvm_arch_irqfd_route_changed() entirely, and make the above ugliness into:
> > 
> > void kvm_arch_update_irqfd_routing(struct kvm_kernel_irqfd *irqfd,
> > 				   struct kvm_kernel_irq_routing_entry *old,
> > 				   struct kvm_kernel_irq_routing_entry *new)
> > 
> > 
> > I'm ~90% confident it'll land in 6.17:
> > https://lore.kernel.org/all/20250523010004.3240643-1-seanjc@google.com
> 
> Sure, if you want to preserve this fossil for another round, I don't
> really care. But if you are planning your grand plan for 6.17, you
> could as well rebase on top of theses patches which are aimed at -rc1
> (yes, they are bug fixes), and it would then be perfectly acceptable
> to have this cleanup early.

I'll be rebasing either way, so I'm not comletely opposed to it, I just don't
see much value in rushing this particular change into 6.16 given that there's a
bigger cleanup looming.

Re: [PATCH 4/5] KVM: arm64: Unmap vLPIs affected by changes to GSI routing information

[Oliver Upton]

On Fri, May 23, 2025 at 01:54:01PM -0700, Sean Christopherson wrote:
> On Fri, May 23, 2025, Marc Zyngier wrote:
> > On Fri, 23 May 2025 18:48:02 +0100,
> > Sean Christopherson <seanjc@google.com> wrote:
> > > 
> > > On Fri, May 23, 2025, Marc Zyngier wrote:
> > > > > @@ -2803,6 +2804,28 @@ void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
> > > > >  	kvm_vgic_v4_unset_forwarding(irqfd->kvm, prod->irq);
> > > > >  }
> > > > >  
> > > > > +bool kvm_arch_irqfd_route_changed(struct kvm_kernel_irq_routing_entry *old,
> > > > > +				  struct kvm_kernel_irq_routing_entry *new)
> > > > > +{
> > > > > +	if (new->type != KVM_IRQ_ROUTING_MSI)
> > > > > +		return true;
> > > > > +
> > > > > +	return memcmp(&old->msi, &new->msi, sizeof(new->msi));
> > > > > +}
> > > > > +
> > > > > +int kvm_arch_update_irqfd_routing(struct kvm *kvm, unsigned int host_irq,
> > > > > +				  uint32_t guest_irq, bool set)
> > > > 
> > > > If we're adding this, can we take out the trash and get rid of this
> > > > 'set' parameter? Its only purpose is to be set to '1' and fed to the
> > > > x86-specific stuff. How about this:
> > > 
> > > Can we hold off on any changes to the common APIs?  Unless they're urgent and
> > > need to land in 6.16, I've got a massive series that I've been slowing working
> > > on for ~7 months that fixes this wart, and several more, e.g. gets rid of
> > > kvm_arch_irqfd_route_changed() entirely, and make the above ugliness into:
> > > 
> > > void kvm_arch_update_irqfd_routing(struct kvm_kernel_irqfd *irqfd,
> > > 				   struct kvm_kernel_irq_routing_entry *old,
> > > 				   struct kvm_kernel_irq_routing_entry *new)
> > > 
> > > 
> > > I'm ~90% confident it'll land in 6.17:
> > > https://lore.kernel.org/all/20250523010004.3240643-1-seanjc@google.com
> > 
> > Sure, if you want to preserve this fossil for another round, I don't
> > really care. But if you are planning your grand plan for 6.17, you
> > could as well rebase on top of theses patches which are aimed at -rc1
> > (yes, they are bug fixes), and it would then be perfectly acceptable
> > to have this cleanup early.
> 
> I'll be rebasing either way, so I'm not comletely opposed to it, I just don't
> see much value in rushing this particular change into 6.16 given that there's a
> bigger cleanup looming.

+1. Having a minor refactor that survives for a single kernel release is
just unnecessary churn. I imagine some folks will want to backport this
and basing the fixes series on the existing pile of crap will make that
a bit easier.

Thanks,
Oliver

[PATCH 5/5] KVM: arm64: vgic-init: Plug vCPU vs. VGIC creation race

[Oliver Upton]

syzkaller has found another ugly race in the VGIC, this time dealing
with VGIC creation. Since kvm_vgic_create() doesn't sufficiently protect
against in-flight vCPU creations, it is possible to get a vCPU into the
kernel w/ an in-kernel VGIC but no allocation of private IRQs:

  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000d20
  Mem abort info:
    ESR = 0x0000000096000046
    EC = 0x25: DABT (current EL), IL = 32 bits
    SET = 0, FnV = 0
    EA = 0, S1PTW = 0
    FSC = 0x06: level 2 translation fault
  Data abort info:
    ISV = 0, ISS = 0x00000046, ISS2 = 0x00000000
    CM = 0, WnR = 1, TnD = 0, TagAccess = 0
    GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
  user pgtable: 4k pages, 48-bit VAs, pgdp=0000000103e4f000
  [0000000000000d20] pgd=0800000102e1c403, p4d=0800000102e1c403, pud=0800000101146403, pmd=0000000000000000
  Internal error: Oops: 0000000096000046 [#1] PREEMPT SMP
  CPU: 9 UID: 0 PID: 246 Comm: test Not tainted 6.14.0-rc6-00097-g0c90821f5db8 #16
  Hardware name: linux,dummy-virt (DT)
  pstate: 814020c5 (Nzcv daIF +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
  pc : _raw_spin_lock_irqsave+0x34/0x8c
  lr : kvm_vgic_set_owner+0x54/0xa4
  sp : ffff80008086ba20
  x29: ffff80008086ba20 x28: ffff0000c19b5640 x27: 0000000000000000
  x26: 0000000000000000 x25: ffff0000c4879bd0 x24: 000000000000001e
  x23: 0000000000000000 x22: 0000000000000000 x21: ffff0000c487af80
  x20: ffff0000c487af18 x19: 0000000000000000 x18: 0000001afadd5a8b
  x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000001
  x14: ffff0000c19b56c0 x13: 0030c9adf9d9889e x12: ffffc263710e1908
  x11: 0000001afb0d74f2 x10: e0966b840b373664 x9 : ec806bf7d6a57cd5
  x8 : ffff80008086b980 x7 : 0000000000000001 x6 : 0000000000000001
  x5 : 0000000080800054 x4 : 4ec4ec4ec4ec4ec5 x3 : 0000000000000000
  x2 : 0000000000000001 x1 : 0000000000000000 x0 : 0000000000000d20
  Call trace:
   _raw_spin_lock_irqsave+0x34/0x8c (P)
   kvm_vgic_set_owner+0x54/0xa4
   kvm_timer_enable+0xf4/0x274
   kvm_arch_vcpu_run_pid_change+0xe0/0x380
   kvm_vcpu_ioctl+0x93c/0x9e0
   __arm64_sys_ioctl+0xb4/0xec
   invoke_syscall+0x48/0x110
   el0_svc_common.constprop.0+0x40/0xe0
   do_el0_svc+0x1c/0x28
   el0_svc+0x30/0xd0
   el0t_64_sync_handler+0x10c/0x138
   el0t_64_sync+0x198/0x19c
   Code: b9000841 d503201f 52800001 52800022 (88e17c02)
   ---[ end trace 0000000000000000 ]---

Plug the race by explicitly checking for an in-progress vCPU creation
and failing kvm_vgic_create() when that's the case. Add some comments to
document all the things kvm_vgic_create() is trying to guard against
too.

Reported-by: Alexander Potapenko <glider@google.com>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/vgic/vgic-init.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index 1f33e71c2a73..0611a1c2ce8e 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -84,15 +84,40 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
 		!kvm_vgic_global_state.can_emulate_gicv2)
 		return -ENODEV;
 
-	/* Must be held to avoid race with vCPU creation */
+	/*
+	 * Ensure mutual exclusion with vCPU creation and any vCPU ioctls by:
+	 *
+	 *  - Holding kvm->lock to prevent KVM_CREATE_VCPU from reaching
+	 *    kvm_arch_vcpu_precreate() and ensuring created_vcpus is stable.
+	 *    This alone is insufficient, as kvm_vm_ioctl_create_vcpu() drops
+	 *    the kvm->lock before completing the vCPU creation.
+	 */
 	lockdep_assert_held(&kvm->lock);
 
+	/*
+	 *  - Acquiring the vCPU mutex for every *online* vCPU to prevent
+	 *    concurrent vCPU ioctls for vCPUs already visible to userspace.
+	 */
 	ret = -EBUSY;
 	if (!lock_all_vcpus(kvm))
 		return ret;
 
+	/*
+	 *  - Taking the config_lock which protects VGIC data structures such
+	 *    as the per-vCPU arrays of private IRQs (SGIs, PPIs).
+	 */
 	mutex_lock(&kvm->arch.config_lock);
 
+	/*
+	 * - Bailing on the entire thing if a vCPU is in the middle of creation,
+	 *   dropped the kvm->lock, but hasn't reached kvm_arch_vcpu_create().
+	 *
+	 * The whole combination of this guarantees that no vCPU can get into
+	 * KVM with a VGIC configuration inconsistent with the VM's VGIC.
+	 */
+	if (kvm->created_vcpus != atomic_read(&kvm->online_vcpus))
+		goto out_unlock;
+
 	if (irqchip_in_kernel(kvm)) {
 		ret = -EEXIST;
 		goto out_unlock;
-- 
2.39.5

Re: [PATCH 0/5] KVM: arm64: Some VGIC-related fixes

[Marc Zyngier]

On Fri, 23 May 2025 17:08:05 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Flushing out some patches I had sitting around before vacation:
> 
>  - The irqbypass hooks are not resilient to changes in the GSI<->MSI
>    routing and can sometimes leave behind stale vLPI mappings for the
>    VM. The fix is to resolve the VGIC IRQ using the host IRQ (which is
>    stable) and nuking the vLPI mapping upon a routing change.
> 
>  - Closing another VGIC race where vCPU creation races with VGIC
>    creation, leading to in-flight vCPUs entering the kernel w/o private
>    IRQs allocated.
> 
> Applies to kvmarm-6.16.

Apart from the couple of nits I mentioned in my reply to specific
patches, this looks good to me. If you can tidy it up some time before
-rc1, I'll happily queue it.

Thanks,

	M.

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