[PATCH] KVM: arm/arm64: vgic-new: Synchronize changes to active state

[PATCH] KVM: arm/arm64: vgic-new: Synchronize changes to active state

[Christoffer Dall]

When modifying the active state of an interrupt via the MMIO interface,
we should ensure that the write has the intended effect.

If a guest sets an interrupt to active, but that interrupt is already
flushed into a list register on a running VCPU, then that VCPU will
write the active state back into the struct vgic_irq upon returning from
the guest and syncing its state.  This is a non-benign race, because the
guest can observe that an interrupt is not active, and it can have a
reasonable expectations that other VCPUs will not ack any IRQs, and then
set the state to active, and expect it to stay that way.  Currently we
are not honoring this case.

Thefore, change both the SACTIVE and CACTIVE mmio handlers to stop the
world, change the irq state, potentially queue the irq if we're setting
it to active, and then continue.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 virt/kvm/arm/vgic/vgic-mmio.c | 69 ++++++++++++++++++++-----------------------
 1 file changed, 32 insertions(+), 37 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index 4ef3571..90b0dc4 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -173,6 +173,34 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
 	return value;
 }
 
+static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
+				    bool new_active_state)
+{
+	spin_lock(&irq->irq_lock);
+	/*
+	 * If this virtual IRQ was written into a list register, we
+	 * have to make sure the CPU that runs the VCPU thread has
+	 * synced back LR state to the struct vgic_irq.  We can only
+	 * know this for sure, when either this irq is not assigned to
+	 * anyone's AP list anymore, or the VCPU thread is not
+	 * running on any CPUs.
+	 *
+	 * In the opposite case, we know the VCPU thread may be on its
+	 * way back from the guest and still has to sync back this
+	 * IRQ, so we release and re-acquire the spin_lock to let the
+	 * other thread sync back the IRQ.
+	 */
+	while (irq->vcpu && /* IRQ may have state in an LR somewhere */
+	       irq->vcpu->cpu != -1) /* VCPU thread is running */
+		cond_resched_lock(&irq->irq_lock);
+
+	irq->active = new_active_state;
+	if (new_active_state)
+		vgic_queue_irq_unlock(vcpu->kvm, irq);
+	else
+		spin_unlock(&irq->irq_lock);
+}
+
 void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
 			     gpa_t addr, unsigned int len,
 			     unsigned long val)
@@ -183,27 +211,7 @@ void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
 	kvm_arm_halt_guest(vcpu->kvm);
 	for_each_set_bit(i, &val, len * 8) {
 		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
-
-		spin_lock(&irq->irq_lock);
-		/*
-		 * If this virtual IRQ was written into a list register, we
-		 * have to make sure the CPU that runs the VCPU thread has
-		 * synced back LR state to the struct vgic_irq.  We can only
-		 * know this for sure, when either this irq is not assigned to
-		 * anyone's AP list anymore, or the VCPU thread is not
-		 * running on any CPUs.
-		 *
-		 * In the opposite case, we know the VCPU thread may be on its
-		 * way back from the guest and still has to sync back this
-		 * IRQ, so we release and re-acquire the spin_lock to let the
-		 * other thread sync back the IRQ.
-		 */
-		while (irq->vcpu && /* IRQ may have state in an LR somewhere */
-		       irq->vcpu->cpu != -1) /* VCPU thread is running */
-			cond_resched_lock(&irq->irq_lock);
-
-		irq->active = false;
-		spin_unlock(&irq->irq_lock);
+		vgic_mmio_change_active(vcpu, irq, false);
 	}
 	kvm_arm_resume_guest(vcpu->kvm);
 }
@@ -215,25 +223,12 @@ void vgic_mmio_write_sactive(struct kvm_vcpu *vcpu,
 	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
 	int i;
 
+	kvm_arm_halt_guest(vcpu->kvm);
 	for_each_set_bit(i, &val, len * 8) {
 		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
-
-		spin_lock(&irq->irq_lock);
-
-		/*
-		 * If the IRQ was already active or there is no target VCPU
-		 * assigned at the moment, then just proceed.
-		 */
-		if (irq->active || !irq->target_vcpu) {
-			irq->active = true;
-
-			spin_unlock(&irq->irq_lock);
-			continue;
-		}
-
-		irq->active = true;
-		vgic_queue_irq_unlock(vcpu->kvm, irq);
+		vgic_mmio_change_active(vcpu, irq, true);
 	}
+	kvm_arm_resume_guest(vcpu->kvm);
 }
 
 unsigned long vgic_mmio_read_priority(struct kvm_vcpu *vcpu,
-- 
2.1.2.330.g565301e.dirty

[PATCH v2] KVM: arm/arm64: vgic-new: Synchronize changes to active state

[Christoffer Dall]

When modifying the active state of an interrupt via the MMIO interface,
we should ensure that the write has the intended effect.

If a guest sets an interrupt to active, but that interrupt is already
flushed into a list register on a running VCPU, then that VCPU will
write the active state back into the struct vgic_irq upon returning from
the guest and syncing its state.  This is a non-benign race, because the
guest can observe that an interrupt is not active, and it can have a
reasonable expectations that other VCPUs will not ack any IRQs, and then
set the state to active, and expect it to stay that way.  Currently we
are not honoring this case.

Thefore, change both the SACTIVE and CACTIVE mmio handlers to stop the
world, change the irq state, potentially queue the irq if we're setting
it to active, and then continue.

We take this chance to slightly optimize these functions by not stopping
the world when touching private interrupts where there is inherently no
possible race.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
Changes since v1:
 - Dont' stop the world for private IRQs

 virt/kvm/arm/vgic/vgic-mmio.c | 87 ++++++++++++++++++++++++-------------------
 1 file changed, 48 insertions(+), 39 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index 4ef3571..b014c8c 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -173,6 +173,36 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
 	return value;
 }
 
+static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
+				    bool new_active_state)
+{
+	spin_lock(&irq->irq_lock);
+	/*
+	 * If this virtual IRQ was written into a list register, we
+	 * have to make sure the CPU that runs the VCPU thread has
+	 * synced back LR state to the struct vgic_irq.  We can only
+	 * know this for sure, when either this irq is not assigned to
+	 * anyone's AP list anymore, or the VCPU thread is not
+	 * running on any CPUs.
+	 *
+	 * In the opposite case, we know the VCPU thread may be on its
+	 * way back from the guest and still has to sync back this
+	 * IRQ, so we release and re-acquire the spin_lock to let the
+	 * other thread sync back the IRQ.
+	 */
+	while (irq->vcpu && /* IRQ may have state in an LR somewhere */
+	       irq->vcpu->cpu != -1) { /* VCPU thread is running */
+		BUG_ON(irq->intid < VGIC_NR_PRIVATE_IRQS);
+		cond_resched_lock(&irq->irq_lock);
+	}
+
+	irq->active = new_active_state;
+	if (new_active_state)
+		vgic_queue_irq_unlock(vcpu->kvm, irq);
+	else
+		spin_unlock(&irq->irq_lock);
+}
+
 void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
 			     gpa_t addr, unsigned int len,
 			     unsigned long val)
@@ -180,32 +210,18 @@ void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
 	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
 	int i;
 
-	kvm_arm_halt_guest(vcpu->kvm);
+	/* Only the VCPU itself can access its active state regs */
+	if (intid >= VGIC_NR_PRIVATE_IRQS)
+		kvm_arm_halt_guest(vcpu->kvm);
+
 	for_each_set_bit(i, &val, len * 8) {
 		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
-
-		spin_lock(&irq->irq_lock);
-		/*
-		 * If this virtual IRQ was written into a list register, we
-		 * have to make sure the CPU that runs the VCPU thread has
-		 * synced back LR state to the struct vgic_irq.  We can only
-		 * know this for sure, when either this irq is not assigned to
-		 * anyone's AP list anymore, or the VCPU thread is not
-		 * running on any CPUs.
-		 *
-		 * In the opposite case, we know the VCPU thread may be on its
-		 * way back from the guest and still has to sync back this
-		 * IRQ, so we release and re-acquire the spin_lock to let the
-		 * other thread sync back the IRQ.
-		 */
-		while (irq->vcpu && /* IRQ may have state in an LR somewhere */
-		       irq->vcpu->cpu != -1) /* VCPU thread is running */
-			cond_resched_lock(&irq->irq_lock);
-
-		irq->active = false;
-		spin_unlock(&irq->irq_lock);
+		vgic_mmio_change_active(vcpu, irq, false);
 	}
-	kvm_arm_resume_guest(vcpu->kvm);
+
+	/* Only the VCPU itself can access its active state regs */
+	if (intid >= VGIC_NR_PRIVATE_IRQS)
+		kvm_arm_resume_guest(vcpu->kvm);
 }
 
 void vgic_mmio_write_sactive(struct kvm_vcpu *vcpu,
@@ -215,25 +231,18 @@ void vgic_mmio_write_sactive(struct kvm_vcpu *vcpu,
 	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
 	int i;
 
+	/* Only the VCPU itself can access its active state regs */
+	if (intid >= VGIC_NR_PRIVATE_IRQS)
+		kvm_arm_halt_guest(vcpu->kvm);
+
 	for_each_set_bit(i, &val, len * 8) {
 		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
-
-		spin_lock(&irq->irq_lock);
-
-		/*
-		 * If the IRQ was already active or there is no target VCPU
-		 * assigned at the moment, then just proceed.
-		 */
-		if (irq->active || !irq->target_vcpu) {
-			irq->active = true;
-
-			spin_unlock(&irq->irq_lock);
-			continue;
-		}
-
-		irq->active = true;
-		vgic_queue_irq_unlock(vcpu->kvm, irq);
+		vgic_mmio_change_active(vcpu, irq, true);
 	}
+
+	/* Only the VCPU itself can access its active state regs */
+	if (intid >= VGIC_NR_PRIVATE_IRQS)
+		kvm_arm_resume_guest(vcpu->kvm);
 }
 
 unsigned long vgic_mmio_read_priority(struct kvm_vcpu *vcpu,
-- 
2.1.2.330.g565301e.dirty

[PATCH v2] KVM: arm/arm64: vgic-new: Synchronize changes to active state

[Marc Zyngier]

On 20/05/16 14:53, Christoffer Dall wrote:
> When modifying the active state of an interrupt via the MMIO interface,
> we should ensure that the write has the intended effect.
> 
> If a guest sets an interrupt to active, but that interrupt is already
> flushed into a list register on a running VCPU, then that VCPU will
> write the active state back into the struct vgic_irq upon returning from
> the guest and syncing its state.  This is a non-benign race, because the
> guest can observe that an interrupt is not active, and it can have a
> reasonable expectations that other VCPUs will not ack any IRQs, and then
> set the state to active, and expect it to stay that way.  Currently we
> are not honoring this case.
> 
> Thefore, change both the SACTIVE and CACTIVE mmio handlers to stop the
> world, change the irq state, potentially queue the irq if we're setting
> it to active, and then continue.
> 
> We take this chance to slightly optimize these functions by not stopping
> the world when touching private interrupts where there is inherently no
> possible race.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> Changes since v1:
>  - Dont' stop the world for private IRQs
> 
>  virt/kvm/arm/vgic/vgic-mmio.c | 87 ++++++++++++++++++++++++-------------------
>  1 file changed, 48 insertions(+), 39 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index 4ef3571..b014c8c 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -173,6 +173,36 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
>  	return value;
>  }
>  
> +static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> +				    bool new_active_state)
> +{
> +	spin_lock(&irq->irq_lock);
> +	/*
> +	 * If this virtual IRQ was written into a list register, we
> +	 * have to make sure the CPU that runs the VCPU thread has
> +	 * synced back LR state to the struct vgic_irq.  We can only
> +	 * know this for sure, when either this irq is not assigned to
> +	 * anyone's AP list anymore, or the VCPU thread is not
> +	 * running on any CPUs.
> +	 *
> +	 * In the opposite case, we know the VCPU thread may be on its
> +	 * way back from the guest and still has to sync back this
> +	 * IRQ, so we release and re-acquire the spin_lock to let the
> +	 * other thread sync back the IRQ.
> +	 */
> +	while (irq->vcpu && /* IRQ may have state in an LR somewhere */
> +	       irq->vcpu->cpu != -1) { /* VCPU thread is running */
> +		BUG_ON(irq->intid < VGIC_NR_PRIVATE_IRQS);
> +		cond_resched_lock(&irq->irq_lock);
> +	}
> +
> +	irq->active = new_active_state;
> +	if (new_active_state)
> +		vgic_queue_irq_unlock(vcpu->kvm, irq);
> +	else
> +		spin_unlock(&irq->irq_lock);
> +}
> +
>  void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
>  			     gpa_t addr, unsigned int len,
>  			     unsigned long val)
> @@ -180,32 +210,18 @@ void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
>  	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
>  	int i;
>  
> -	kvm_arm_halt_guest(vcpu->kvm);
> +	/* Only the VCPU itself can access its active state regs */

I'm afraid this is not true for GICv3 (the private interrupts are
handled by the redistributors, which are not banked).

> +	if (intid >= VGIC_NR_PRIVATE_IRQS)
> +		kvm_arm_halt_guest(vcpu->kvm);
	else {
		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid);
		irq->target_vcpu.arch.pause = true;
		kvm_make_request(irq->target_vcpu, KVM_REQ_VCPU_EXIT);
		/* and then it is a bit complicated... */
	}
> +
>  	for_each_set_bit(i, &val, len * 8) {
>  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> -
> -		spin_lock(&irq->irq_lock);
> -		/*
> -		 * If this virtual IRQ was written into a list register, we
> -		 * have to make sure the CPU that runs the VCPU thread has
> -		 * synced back LR state to the struct vgic_irq.  We can only
> -		 * know this for sure, when either this irq is not assigned to
> -		 * anyone's AP list anymore, or the VCPU thread is not
> -		 * running on any CPUs.
> -		 *
> -		 * In the opposite case, we know the VCPU thread may be on its
> -		 * way back from the guest and still has to sync back this
> -		 * IRQ, so we release and re-acquire the spin_lock to let the
> -		 * other thread sync back the IRQ.
> -		 */
> -		while (irq->vcpu && /* IRQ may have state in an LR somewhere */
> -		       irq->vcpu->cpu != -1) /* VCPU thread is running */
> -			cond_resched_lock(&irq->irq_lock);
> -
> -		irq->active = false;
> -		spin_unlock(&irq->irq_lock);
> +		vgic_mmio_change_active(vcpu, irq, false);
>  	}
> -	kvm_arm_resume_guest(vcpu->kvm);
> +
> +	/* Only the VCPU itself can access its active state regs */
> +	if (intid >= VGIC_NR_PRIVATE_IRQS)
> +		kvm_arm_resume_guest(vcpu->kvm);
>  }

I though we had a way to stop a single vcpu without too much hassle,
but I'm not seeing any standard way to do that. Grmbl...

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v2] KVM: arm/arm64: vgic-new: Synchronize changes to active state

[Christoffer Dall]

On Fri, May 20, 2016 at 03:07:57PM +0100, Marc Zyngier wrote:
> On 20/05/16 14:53, Christoffer Dall wrote:
> > When modifying the active state of an interrupt via the MMIO interface,
> > we should ensure that the write has the intended effect.
> > 
> > If a guest sets an interrupt to active, but that interrupt is already
> > flushed into a list register on a running VCPU, then that VCPU will
> > write the active state back into the struct vgic_irq upon returning from
> > the guest and syncing its state.  This is a non-benign race, because the
> > guest can observe that an interrupt is not active, and it can have a
> > reasonable expectations that other VCPUs will not ack any IRQs, and then
> > set the state to active, and expect it to stay that way.  Currently we
> > are not honoring this case.
> > 
> > Thefore, change both the SACTIVE and CACTIVE mmio handlers to stop the
> > world, change the irq state, potentially queue the irq if we're setting
> > it to active, and then continue.
> > 
> > We take this chance to slightly optimize these functions by not stopping
> > the world when touching private interrupts where there is inherently no
> > possible race.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> > Changes since v1:
> >  - Dont' stop the world for private IRQs
> > 
> >  virt/kvm/arm/vgic/vgic-mmio.c | 87 ++++++++++++++++++++++++-------------------
> >  1 file changed, 48 insertions(+), 39 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> > index 4ef3571..b014c8c 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> > @@ -173,6 +173,36 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> >  	return value;
> >  }
> >  
> > +static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> > +				    bool new_active_state)
> > +{
> > +	spin_lock(&irq->irq_lock);
> > +	/*
> > +	 * If this virtual IRQ was written into a list register, we
> > +	 * have to make sure the CPU that runs the VCPU thread has
> > +	 * synced back LR state to the struct vgic_irq.  We can only
> > +	 * know this for sure, when either this irq is not assigned to
> > +	 * anyone's AP list anymore, or the VCPU thread is not
> > +	 * running on any CPUs.
> > +	 *
> > +	 * In the opposite case, we know the VCPU thread may be on its
> > +	 * way back from the guest and still has to sync back this
> > +	 * IRQ, so we release and re-acquire the spin_lock to let the
> > +	 * other thread sync back the IRQ.
> > +	 */
> > +	while (irq->vcpu && /* IRQ may have state in an LR somewhere */
> > +	       irq->vcpu->cpu != -1) { /* VCPU thread is running */
> > +		BUG_ON(irq->intid < VGIC_NR_PRIVATE_IRQS);
> > +		cond_resched_lock(&irq->irq_lock);
> > +	}
> > +
> > +	irq->active = new_active_state;
> > +	if (new_active_state)
> > +		vgic_queue_irq_unlock(vcpu->kvm, irq);
> > +	else
> > +		spin_unlock(&irq->irq_lock);
> > +}
> > +
> >  void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
> >  			     gpa_t addr, unsigned int len,
> >  			     unsigned long val)
> > @@ -180,32 +210,18 @@ void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
> >  	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> >  	int i;
> >  
> > -	kvm_arm_halt_guest(vcpu->kvm);
> > +	/* Only the VCPU itself can access its active state regs */
> 
> I'm afraid this is not true for GICv3 (the private interrupts are
> handled by the redistributors, which are not banked).
> 
> > +	if (intid >= VGIC_NR_PRIVATE_IRQS)
> > +		kvm_arm_halt_guest(vcpu->kvm);
> 	else {
> 		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid);
> 		irq->target_vcpu.arch.pause = true;
> 		kvm_make_request(irq->target_vcpu, KVM_REQ_VCPU_EXIT);
> 		/* and then it is a bit complicated... */
> 	}
> > +
> >  	for_each_set_bit(i, &val, len * 8) {
> >  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> > -
> > -		spin_lock(&irq->irq_lock);
> > -		/*
> > -		 * If this virtual IRQ was written into a list register, we
> > -		 * have to make sure the CPU that runs the VCPU thread has
> > -		 * synced back LR state to the struct vgic_irq.  We can only
> > -		 * know this for sure, when either this irq is not assigned to
> > -		 * anyone's AP list anymore, or the VCPU thread is not
> > -		 * running on any CPUs.
> > -		 *
> > -		 * In the opposite case, we know the VCPU thread may be on its
> > -		 * way back from the guest and still has to sync back this
> > -		 * IRQ, so we release and re-acquire the spin_lock to let the
> > -		 * other thread sync back the IRQ.
> > -		 */
> > -		while (irq->vcpu && /* IRQ may have state in an LR somewhere */
> > -		       irq->vcpu->cpu != -1) /* VCPU thread is running */
> > -			cond_resched_lock(&irq->irq_lock);
> > -
> > -		irq->active = false;
> > -		spin_unlock(&irq->irq_lock);
> > +		vgic_mmio_change_active(vcpu, irq, false);
> >  	}
> > -	kvm_arm_resume_guest(vcpu->kvm);
> > +
> > +	/* Only the VCPU itself can access its active state regs */
> > +	if (intid >= VGIC_NR_PRIVATE_IRQS)
> > +		kvm_arm_resume_guest(vcpu->kvm);
> >  }
> 
> I though we had a way to stop a single vcpu without too much hassle,
> but I'm not seeing any standard way to do that. Grmbl...
> 
You can pause it and kick it, I think that should work... Let me have a
look.  Otherwise we'll fall back to v1 and optimize this later.

-Christoffer

[PATCH v3] KVM: arm/arm64: vgic-new: Synchronize changes to active state

[Christoffer Dall]

When modifying the active state of an interrupt via the MMIO interface,
we should ensure that the write has the intended effect.

If a guest sets an interrupt to active, but that interrupt is already
flushed into a list register on a running VCPU, then that VCPU will
write the active state back into the struct vgic_irq upon returning from
the guest and syncing its state.  This is a non-benign race, because the
guest can observe that an interrupt is not active, and it can have a
reasonable expectations that other VCPUs will not ack any IRQs, and then
set the state to active, and expect it to stay that way.  Currently we
are not honoring this case.

Thefore, change both the SACTIVE and CACTIVE mmio handlers to stop the
world, change the irq state, potentially queue the irq if we're setting
it to active, and then continue.

We take this chance to slightly optimize these functions by not stopping
the world when touching private interrupts where there is inherently no
possible race.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
Changes since v1:
 - Dont' stop the world for private IRQs
Changes since v2:
 - Explicitly stop the VCPU that private IRQs target because GICv3
   redistributors allow any VCPU to tamper with the active state of
   interrupts private to another VCPU.

 arch/arm/include/asm/kvm_host.h   |   2 +
 arch/arm/kvm/arm.c                |   8 ++-
 arch/arm64/include/asm/kvm_host.h |   2 +
 virt/kvm/arm/vgic/vgic-mmio.c     | 105 ++++++++++++++++++++++++--------------
 4 files changed, 77 insertions(+), 40 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 832be03..987da9f 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -229,6 +229,8 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);
 struct kvm_vcpu __percpu **kvm_get_running_vcpus(void);
 void kvm_arm_halt_guest(struct kvm *kvm);
 void kvm_arm_resume_guest(struct kvm *kvm);
+void kvm_arm_halt_vcpu(struct kvm_vcpu *vcpu);
+void kvm_arm_resume_vcpu(struct kvm_vcpu *vcpu);
 
 int kvm_arm_copy_coproc_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
 unsigned long kvm_arm_num_coproc_regs(struct kvm_vcpu *vcpu);
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index e89329d..5a599c2 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -502,7 +502,13 @@ void kvm_arm_halt_guest(struct kvm *kvm)
 	kvm_make_all_cpus_request(kvm, KVM_REQ_VCPU_EXIT);
 }
 
-static void kvm_arm_resume_vcpu(struct kvm_vcpu *vcpu)
+void kvm_arm_halt_vcpu(struct kvm_vcpu *vcpu)
+{
+	vcpu->arch.pause = true;
+	kvm_vcpu_kick(vcpu);
+}
+
+void kvm_arm_resume_vcpu(struct kvm_vcpu *vcpu)
 {
 	struct swait_queue_head *wq = kvm_arch_vcpu_wq(vcpu);
 
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index fa94f91..38746d2 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -329,6 +329,8 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);
 struct kvm_vcpu * __percpu *kvm_get_running_vcpus(void);
 void kvm_arm_halt_guest(struct kvm *kvm);
 void kvm_arm_resume_guest(struct kvm *kvm);
+void kvm_arm_halt_vcpu(struct kvm_vcpu *vcpu);
+void kvm_arm_resume_vcpu(struct kvm_vcpu *vcpu);
 
 u64 __kvm_call_hyp(void *hypfn, ...);
 #define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index 4ef3571..059595e 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -173,6 +173,66 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
 	return value;
 }
 
+static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
+				    bool new_active_state)
+{
+	spin_lock(&irq->irq_lock);
+	/*
+	 * If this virtual IRQ was written into a list register, we
+	 * have to make sure the CPU that runs the VCPU thread has
+	 * synced back LR state to the struct vgic_irq.  We can only
+	 * know this for sure, when either this irq is not assigned to
+	 * anyone's AP list anymore, or the VCPU thread is not
+	 * running on any CPUs.
+	 *
+	 * In the opposite case, we know the VCPU thread may be on its
+	 * way back from the guest and still has to sync back this
+	 * IRQ, so we release and re-acquire the spin_lock to let the
+	 * other thread sync back the IRQ.
+	 */
+	while (irq->vcpu && /* IRQ may have state in an LR somewhere */
+	       irq->vcpu->cpu != -1) { /* VCPU thread is running */
+		BUG_ON(irq->intid < VGIC_NR_PRIVATE_IRQS);
+		cond_resched_lock(&irq->irq_lock);
+	}
+
+	irq->active = new_active_state;
+	if (new_active_state)
+		vgic_queue_irq_unlock(vcpu->kvm, irq);
+	else
+		spin_unlock(&irq->irq_lock);
+}
+
+/*
+ * If we are fiddling with an IRQ's active state, we have to make sure the IRQ
+ * is not queued on some running VCPU's LRs, because then the change to the
+ * active state can be overwritten when the VCPU's state is synced coming back
+ * from the guest.
+ *
+ * For shared interrupts, we have to stop all the VCPUs because interrupts can
+ * be migrated while we don't hold the IRQ locks and we don't want to be
+ * chasing moving targets.
+ *
+ * For private interrupts, we only have to make sure the single and only VCPU
+ * that can potentially queue the IRQ is stopped.
+ */
+static void vgic_change_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
+{
+	if (intid < VGIC_NR_PRIVATE_IRQS)
+		kvm_arm_halt_vcpu(vcpu);
+	else
+		kvm_arm_halt_guest(vcpu->kvm);
+}
+
+/* See vgic_change_active_prepare */
+static void vgic_change_active_finish(struct kvm_vcpu *vcpu, u32 intid)
+{
+	if (intid < VGIC_NR_PRIVATE_IRQS)
+		kvm_arm_resume_vcpu(vcpu);
+	else
+		kvm_arm_resume_guest(vcpu->kvm);
+}
+
 void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
 			     gpa_t addr, unsigned int len,
 			     unsigned long val)
@@ -180,32 +240,12 @@ void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
 	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
 	int i;
 
-	kvm_arm_halt_guest(vcpu->kvm);
+	vgic_change_active_prepare(vcpu, intid);
 	for_each_set_bit(i, &val, len * 8) {
 		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
-
-		spin_lock(&irq->irq_lock);
-		/*
-		 * If this virtual IRQ was written into a list register, we
-		 * have to make sure the CPU that runs the VCPU thread has
-		 * synced back LR state to the struct vgic_irq.  We can only
-		 * know this for sure, when either this irq is not assigned to
-		 * anyone's AP list anymore, or the VCPU thread is not
-		 * running on any CPUs.
-		 *
-		 * In the opposite case, we know the VCPU thread may be on its
-		 * way back from the guest and still has to sync back this
-		 * IRQ, so we release and re-acquire the spin_lock to let the
-		 * other thread sync back the IRQ.
-		 */
-		while (irq->vcpu && /* IRQ may have state in an LR somewhere */
-		       irq->vcpu->cpu != -1) /* VCPU thread is running */
-			cond_resched_lock(&irq->irq_lock);
-
-		irq->active = false;
-		spin_unlock(&irq->irq_lock);
+		vgic_mmio_change_active(vcpu, irq, false);
 	}
-	kvm_arm_resume_guest(vcpu->kvm);
+	vgic_change_active_finish(vcpu, intid);
 }
 
 void vgic_mmio_write_sactive(struct kvm_vcpu *vcpu,
@@ -215,25 +255,12 @@ void vgic_mmio_write_sactive(struct kvm_vcpu *vcpu,
 	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
 	int i;
 
+	vgic_change_active_prepare(vcpu, intid);
 	for_each_set_bit(i, &val, len * 8) {
 		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
-
-		spin_lock(&irq->irq_lock);
-
-		/*
-		 * If the IRQ was already active or there is no target VCPU
-		 * assigned at the moment, then just proceed.
-		 */
-		if (irq->active || !irq->target_vcpu) {
-			irq->active = true;
-
-			spin_unlock(&irq->irq_lock);
-			continue;
-		}
-
-		irq->active = true;
-		vgic_queue_irq_unlock(vcpu->kvm, irq);
+		vgic_mmio_change_active(vcpu, irq, true);
 	}
+	vgic_change_active_finish(vcpu, intid);
 }
 
 unsigned long vgic_mmio_read_priority(struct kvm_vcpu *vcpu,
-- 
2.1.2.330.g565301e.dirty

[PATCH v3] KVM: arm/arm64: vgic-new: Synchronize changes to active state

[Marc Zyngier]

On 20/05/16 15:31, Christoffer Dall wrote:
> When modifying the active state of an interrupt via the MMIO interface,
> we should ensure that the write has the intended effect.
> 
> If a guest sets an interrupt to active, but that interrupt is already
> flushed into a list register on a running VCPU, then that VCPU will
> write the active state back into the struct vgic_irq upon returning from
> the guest and syncing its state.  This is a non-benign race, because the
> guest can observe that an interrupt is not active, and it can have a
> reasonable expectations that other VCPUs will not ack any IRQs, and then
> set the state to active, and expect it to stay that way.  Currently we
> are not honoring this case.
> 
> Thefore, change both the SACTIVE and CACTIVE mmio handlers to stop the
> world, change the irq state, potentially queue the irq if we're setting
> it to active, and then continue.
> 
> We take this chance to slightly optimize these functions by not stopping
> the world when touching private interrupts where there is inherently no
> possible race.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> Changes since v1:
>  - Dont' stop the world for private IRQs
> Changes since v2:
>  - Explicitly stop the VCPU that private IRQs target because GICv3
>    redistributors allow any VCPU to tamper with the active state of
>    interrupts private to another VCPU.
> 
>  arch/arm/include/asm/kvm_host.h   |   2 +
>  arch/arm/kvm/arm.c                |   8 ++-
>  arch/arm64/include/asm/kvm_host.h |   2 +
>  virt/kvm/arm/vgic/vgic-mmio.c     | 105 ++++++++++++++++++++++++--------------
>  4 files changed, 77 insertions(+), 40 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 832be03..987da9f 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -229,6 +229,8 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);
>  struct kvm_vcpu __percpu **kvm_get_running_vcpus(void);
>  void kvm_arm_halt_guest(struct kvm *kvm);
>  void kvm_arm_resume_guest(struct kvm *kvm);
> +void kvm_arm_halt_vcpu(struct kvm_vcpu *vcpu);
> +void kvm_arm_resume_vcpu(struct kvm_vcpu *vcpu);
>  
>  int kvm_arm_copy_coproc_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
>  unsigned long kvm_arm_num_coproc_regs(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index e89329d..5a599c2 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -502,7 +502,13 @@ void kvm_arm_halt_guest(struct kvm *kvm)
>  	kvm_make_all_cpus_request(kvm, KVM_REQ_VCPU_EXIT);
>  }
>  
> -static void kvm_arm_resume_vcpu(struct kvm_vcpu *vcpu)
> +void kvm_arm_halt_vcpu(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->arch.pause = true;
> +	kvm_vcpu_kick(vcpu);
> +}
> +
> +void kvm_arm_resume_vcpu(struct kvm_vcpu *vcpu)
>  {
>  	struct swait_queue_head *wq = kvm_arch_vcpu_wq(vcpu);
>  
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index fa94f91..38746d2 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -329,6 +329,8 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);
>  struct kvm_vcpu * __percpu *kvm_get_running_vcpus(void);
>  void kvm_arm_halt_guest(struct kvm *kvm);
>  void kvm_arm_resume_guest(struct kvm *kvm);
> +void kvm_arm_halt_vcpu(struct kvm_vcpu *vcpu);
> +void kvm_arm_resume_vcpu(struct kvm_vcpu *vcpu);
>  
>  u64 __kvm_call_hyp(void *hypfn, ...);
>  #define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index 4ef3571..059595e 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -173,6 +173,66 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
>  	return value;
>  }
>  
> +static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> +				    bool new_active_state)
> +{
> +	spin_lock(&irq->irq_lock);
> +	/*
> +	 * If this virtual IRQ was written into a list register, we
> +	 * have to make sure the CPU that runs the VCPU thread has
> +	 * synced back LR state to the struct vgic_irq.  We can only
> +	 * know this for sure, when either this irq is not assigned to
> +	 * anyone's AP list anymore, or the VCPU thread is not
> +	 * running on any CPUs.
> +	 *
> +	 * In the opposite case, we know the VCPU thread may be on its
> +	 * way back from the guest and still has to sync back this
> +	 * IRQ, so we release and re-acquire the spin_lock to let the
> +	 * other thread sync back the IRQ.
> +	 */
> +	while (irq->vcpu && /* IRQ may have state in an LR somewhere */
> +	       irq->vcpu->cpu != -1) { /* VCPU thread is running */
> +		BUG_ON(irq->intid < VGIC_NR_PRIVATE_IRQS);
> +		cond_resched_lock(&irq->irq_lock);
> +	}
> +
> +	irq->active = new_active_state;
> +	if (new_active_state)
> +		vgic_queue_irq_unlock(vcpu->kvm, irq);
> +	else
> +		spin_unlock(&irq->irq_lock);
> +}
> +
> +/*
> + * If we are fiddling with an IRQ's active state, we have to make sure the IRQ
> + * is not queued on some running VCPU's LRs, because then the change to the
> + * active state can be overwritten when the VCPU's state is synced coming back
> + * from the guest.
> + *
> + * For shared interrupts, we have to stop all the VCPUs because interrupts can
> + * be migrated while we don't hold the IRQ locks and we don't want to be
> + * chasing moving targets.
> + *
> + * For private interrupts, we only have to make sure the single and only VCPU
> + * that can potentially queue the IRQ is stopped.
> + */
> +static void vgic_change_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
> +{
> +	if (intid < VGIC_NR_PRIVATE_IRQS)
> +		kvm_arm_halt_vcpu(vcpu);
> +	else
> +		kvm_arm_halt_guest(vcpu->kvm);
> +}
> +
> +/* See vgic_change_active_prepare */
> +static void vgic_change_active_finish(struct kvm_vcpu *vcpu, u32 intid)
> +{
> +	if (intid < VGIC_NR_PRIVATE_IRQS)
> +		kvm_arm_resume_vcpu(vcpu);
> +	else
> +		kvm_arm_resume_guest(vcpu->kvm);
> +}
> +
>  void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
>  			     gpa_t addr, unsigned int len,
>  			     unsigned long val)
> @@ -180,32 +240,12 @@ void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
>  	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
>  	int i;
>  
> -	kvm_arm_halt_guest(vcpu->kvm);
> +	vgic_change_active_prepare(vcpu, intid);

You're still only halting the current vcpu, not the one targeted by the
interrupt.

Let's not bother with that for the time being. I'm happy to merge v1,
and optimize this when this starts bugging us (and this is going to take
a while).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v3] KVM: arm/arm64: vgic-new: Synchronize changes to active state

[Marc Zyngier]

On 20/05/16 15:43, Marc Zyngier wrote:
> On 20/05/16 15:31, Christoffer Dall wrote:
>> When modifying the active state of an interrupt via the MMIO interface,
>> we should ensure that the write has the intended effect.
>>
>> If a guest sets an interrupt to active, but that interrupt is already
>> flushed into a list register on a running VCPU, then that VCPU will
>> write the active state back into the struct vgic_irq upon returning from
>> the guest and syncing its state.  This is a non-benign race, because the
>> guest can observe that an interrupt is not active, and it can have a
>> reasonable expectations that other VCPUs will not ack any IRQs, and then
>> set the state to active, and expect it to stay that way.  Currently we
>> are not honoring this case.
>>
>> Thefore, change both the SACTIVE and CACTIVE mmio handlers to stop the
>> world, change the irq state, potentially queue the irq if we're setting
>> it to active, and then continue.
>>
>> We take this chance to slightly optimize these functions by not stopping
>> the world when touching private interrupts where there is inherently no
>> possible race.
>>
>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>> ---
>> Changes since v1:
>>  - Dont' stop the world for private IRQs
>> Changes since v2:
>>  - Explicitly stop the VCPU that private IRQs target because GICv3
>>    redistributors allow any VCPU to tamper with the active state of
>>    interrupts private to another VCPU.
>>
>>  arch/arm/include/asm/kvm_host.h   |   2 +
>>  arch/arm/kvm/arm.c                |   8 ++-
>>  arch/arm64/include/asm/kvm_host.h |   2 +
>>  virt/kvm/arm/vgic/vgic-mmio.c     | 105 ++++++++++++++++++++++++--------------
>>  4 files changed, 77 insertions(+), 40 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index 832be03..987da9f 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -229,6 +229,8 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);
>>  struct kvm_vcpu __percpu **kvm_get_running_vcpus(void);
>>  void kvm_arm_halt_guest(struct kvm *kvm);
>>  void kvm_arm_resume_guest(struct kvm *kvm);
>> +void kvm_arm_halt_vcpu(struct kvm_vcpu *vcpu);
>> +void kvm_arm_resume_vcpu(struct kvm_vcpu *vcpu);
>>  
>>  int kvm_arm_copy_coproc_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
>>  unsigned long kvm_arm_num_coproc_regs(struct kvm_vcpu *vcpu);
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index e89329d..5a599c2 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -502,7 +502,13 @@ void kvm_arm_halt_guest(struct kvm *kvm)
>>  	kvm_make_all_cpus_request(kvm, KVM_REQ_VCPU_EXIT);
>>  }
>>  
>> -static void kvm_arm_resume_vcpu(struct kvm_vcpu *vcpu)
>> +void kvm_arm_halt_vcpu(struct kvm_vcpu *vcpu)
>> +{
>> +	vcpu->arch.pause = true;
>> +	kvm_vcpu_kick(vcpu);
>> +}
>> +
>> +void kvm_arm_resume_vcpu(struct kvm_vcpu *vcpu)
>>  {
>>  	struct swait_queue_head *wq = kvm_arch_vcpu_wq(vcpu);
>>  
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index fa94f91..38746d2 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -329,6 +329,8 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);
>>  struct kvm_vcpu * __percpu *kvm_get_running_vcpus(void);
>>  void kvm_arm_halt_guest(struct kvm *kvm);
>>  void kvm_arm_resume_guest(struct kvm *kvm);
>> +void kvm_arm_halt_vcpu(struct kvm_vcpu *vcpu);
>> +void kvm_arm_resume_vcpu(struct kvm_vcpu *vcpu);
>>  
>>  u64 __kvm_call_hyp(void *hypfn, ...);
>>  #define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
>> index 4ef3571..059595e 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>> @@ -173,6 +173,66 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
>>  	return value;
>>  }
>>  
>> +static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
>> +				    bool new_active_state)
>> +{
>> +	spin_lock(&irq->irq_lock);
>> +	/*
>> +	 * If this virtual IRQ was written into a list register, we
>> +	 * have to make sure the CPU that runs the VCPU thread has
>> +	 * synced back LR state to the struct vgic_irq.  We can only
>> +	 * know this for sure, when either this irq is not assigned to
>> +	 * anyone's AP list anymore, or the VCPU thread is not
>> +	 * running on any CPUs.
>> +	 *
>> +	 * In the opposite case, we know the VCPU thread may be on its
>> +	 * way back from the guest and still has to sync back this
>> +	 * IRQ, so we release and re-acquire the spin_lock to let the
>> +	 * other thread sync back the IRQ.
>> +	 */
>> +	while (irq->vcpu && /* IRQ may have state in an LR somewhere */
>> +	       irq->vcpu->cpu != -1) { /* VCPU thread is running */
>> +		BUG_ON(irq->intid < VGIC_NR_PRIVATE_IRQS);
>> +		cond_resched_lock(&irq->irq_lock);
>> +	}
>> +
>> +	irq->active = new_active_state;
>> +	if (new_active_state)
>> +		vgic_queue_irq_unlock(vcpu->kvm, irq);
>> +	else
>> +		spin_unlock(&irq->irq_lock);
>> +}
>> +
>> +/*
>> + * If we are fiddling with an IRQ's active state, we have to make sure the IRQ
>> + * is not queued on some running VCPU's LRs, because then the change to the
>> + * active state can be overwritten when the VCPU's state is synced coming back
>> + * from the guest.
>> + *
>> + * For shared interrupts, we have to stop all the VCPUs because interrupts can
>> + * be migrated while we don't hold the IRQ locks and we don't want to be
>> + * chasing moving targets.
>> + *
>> + * For private interrupts, we only have to make sure the single and only VCPU
>> + * that can potentially queue the IRQ is stopped.
>> + */
>> +static void vgic_change_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
>> +{
>> +	if (intid < VGIC_NR_PRIVATE_IRQS)
>> +		kvm_arm_halt_vcpu(vcpu);
>> +	else
>> +		kvm_arm_halt_guest(vcpu->kvm);
>> +}
>> +
>> +/* See vgic_change_active_prepare */
>> +static void vgic_change_active_finish(struct kvm_vcpu *vcpu, u32 intid)
>> +{
>> +	if (intid < VGIC_NR_PRIVATE_IRQS)
>> +		kvm_arm_resume_vcpu(vcpu);
>> +	else
>> +		kvm_arm_resume_guest(vcpu->kvm);
>> +}
>> +
>>  void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
>>  			     gpa_t addr, unsigned int len,
>>  			     unsigned long val)
>> @@ -180,32 +240,12 @@ void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
>>  	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
>>  	int i;
>>  
>> -	kvm_arm_halt_guest(vcpu->kvm);
>> +	vgic_change_active_prepare(vcpu, intid);
> 
> You're still only halting the current vcpu, not the one targeted by the
> interrupt.
> 
> Let's not bother with that for the time being. I'm happy to merge v1,
> and optimize this when this starts bugging us (and this is going to take
> a while).

Actually, I now realize that you are right. The MMIO handler is passing
us the target vcpu, not the the one causing the exit. I'm an idiot.

So this looks fine to me.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v3] KVM: arm/arm64: vgic-new: Synchronize changes to active state

[Christoffer Dall]

On Fri, May 20, 2016 at 03:47:01PM +0100, Marc Zyngier wrote:
> On 20/05/16 15:43, Marc Zyngier wrote:
> > On 20/05/16 15:31, Christoffer Dall wrote:
> >> When modifying the active state of an interrupt via the MMIO interface,
> >> we should ensure that the write has the intended effect.
> >>
> >> If a guest sets an interrupt to active, but that interrupt is already
> >> flushed into a list register on a running VCPU, then that VCPU will
> >> write the active state back into the struct vgic_irq upon returning from
> >> the guest and syncing its state.  This is a non-benign race, because the
> >> guest can observe that an interrupt is not active, and it can have a
> >> reasonable expectations that other VCPUs will not ack any IRQs, and then
> >> set the state to active, and expect it to stay that way.  Currently we
> >> are not honoring this case.
> >>
> >> Thefore, change both the SACTIVE and CACTIVE mmio handlers to stop the
> >> world, change the irq state, potentially queue the irq if we're setting
> >> it to active, and then continue.
> >>
> >> We take this chance to slightly optimize these functions by not stopping
> >> the world when touching private interrupts where there is inherently no
> >> possible race.
> >>
> >> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> >> ---
> >> Changes since v1:
> >>  - Dont' stop the world for private IRQs
> >> Changes since v2:
> >>  - Explicitly stop the VCPU that private IRQs target because GICv3
> >>    redistributors allow any VCPU to tamper with the active state of
> >>    interrupts private to another VCPU.
> >>
> >>  arch/arm/include/asm/kvm_host.h   |   2 +
> >>  arch/arm/kvm/arm.c                |   8 ++-
> >>  arch/arm64/include/asm/kvm_host.h |   2 +
> >>  virt/kvm/arm/vgic/vgic-mmio.c     | 105 ++++++++++++++++++++++++--------------
> >>  4 files changed, 77 insertions(+), 40 deletions(-)
> >>
> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> >> index 832be03..987da9f 100644
> >> --- a/arch/arm/include/asm/kvm_host.h
> >> +++ b/arch/arm/include/asm/kvm_host.h
> >> @@ -229,6 +229,8 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);
> >>  struct kvm_vcpu __percpu **kvm_get_running_vcpus(void);
> >>  void kvm_arm_halt_guest(struct kvm *kvm);
> >>  void kvm_arm_resume_guest(struct kvm *kvm);
> >> +void kvm_arm_halt_vcpu(struct kvm_vcpu *vcpu);
> >> +void kvm_arm_resume_vcpu(struct kvm_vcpu *vcpu);
> >>  
> >>  int kvm_arm_copy_coproc_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
> >>  unsigned long kvm_arm_num_coproc_regs(struct kvm_vcpu *vcpu);
> >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> >> index e89329d..5a599c2 100644
> >> --- a/arch/arm/kvm/arm.c
> >> +++ b/arch/arm/kvm/arm.c
> >> @@ -502,7 +502,13 @@ void kvm_arm_halt_guest(struct kvm *kvm)
> >>  	kvm_make_all_cpus_request(kvm, KVM_REQ_VCPU_EXIT);
> >>  }
> >>  
> >> -static void kvm_arm_resume_vcpu(struct kvm_vcpu *vcpu)
> >> +void kvm_arm_halt_vcpu(struct kvm_vcpu *vcpu)
> >> +{
> >> +	vcpu->arch.pause = true;
> >> +	kvm_vcpu_kick(vcpu);
> >> +}
> >> +
> >> +void kvm_arm_resume_vcpu(struct kvm_vcpu *vcpu)
> >>  {
> >>  	struct swait_queue_head *wq = kvm_arch_vcpu_wq(vcpu);
> >>  
> >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> >> index fa94f91..38746d2 100644
> >> --- a/arch/arm64/include/asm/kvm_host.h
> >> +++ b/arch/arm64/include/asm/kvm_host.h
> >> @@ -329,6 +329,8 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void);
> >>  struct kvm_vcpu * __percpu *kvm_get_running_vcpus(void);
> >>  void kvm_arm_halt_guest(struct kvm *kvm);
> >>  void kvm_arm_resume_guest(struct kvm *kvm);
> >> +void kvm_arm_halt_vcpu(struct kvm_vcpu *vcpu);
> >> +void kvm_arm_resume_vcpu(struct kvm_vcpu *vcpu);
> >>  
> >>  u64 __kvm_call_hyp(void *hypfn, ...);
> >>  #define kvm_call_hyp(f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
> >> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> >> index 4ef3571..059595e 100644
> >> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> >> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> >> @@ -173,6 +173,66 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> >>  	return value;
> >>  }
> >>  
> >> +static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> >> +				    bool new_active_state)
> >> +{
> >> +	spin_lock(&irq->irq_lock);
> >> +	/*
> >> +	 * If this virtual IRQ was written into a list register, we
> >> +	 * have to make sure the CPU that runs the VCPU thread has
> >> +	 * synced back LR state to the struct vgic_irq.  We can only
> >> +	 * know this for sure, when either this irq is not assigned to
> >> +	 * anyone's AP list anymore, or the VCPU thread is not
> >> +	 * running on any CPUs.
> >> +	 *
> >> +	 * In the opposite case, we know the VCPU thread may be on its
> >> +	 * way back from the guest and still has to sync back this
> >> +	 * IRQ, so we release and re-acquire the spin_lock to let the
> >> +	 * other thread sync back the IRQ.
> >> +	 */
> >> +	while (irq->vcpu && /* IRQ may have state in an LR somewhere */
> >> +	       irq->vcpu->cpu != -1) { /* VCPU thread is running */
> >> +		BUG_ON(irq->intid < VGIC_NR_PRIVATE_IRQS);
> >> +		cond_resched_lock(&irq->irq_lock);
> >> +	}
> >> +
> >> +	irq->active = new_active_state;
> >> +	if (new_active_state)
> >> +		vgic_queue_irq_unlock(vcpu->kvm, irq);
> >> +	else
> >> +		spin_unlock(&irq->irq_lock);
> >> +}
> >> +
> >> +/*
> >> + * If we are fiddling with an IRQ's active state, we have to make sure the IRQ
> >> + * is not queued on some running VCPU's LRs, because then the change to the
> >> + * active state can be overwritten when the VCPU's state is synced coming back
> >> + * from the guest.
> >> + *
> >> + * For shared interrupts, we have to stop all the VCPUs because interrupts can
> >> + * be migrated while we don't hold the IRQ locks and we don't want to be
> >> + * chasing moving targets.
> >> + *
> >> + * For private interrupts, we only have to make sure the single and only VCPU
> >> + * that can potentially queue the IRQ is stopped.
> >> + */
> >> +static void vgic_change_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
> >> +{
> >> +	if (intid < VGIC_NR_PRIVATE_IRQS)
> >> +		kvm_arm_halt_vcpu(vcpu);
> >> +	else
> >> +		kvm_arm_halt_guest(vcpu->kvm);
> >> +}
> >> +
> >> +/* See vgic_change_active_prepare */
> >> +static void vgic_change_active_finish(struct kvm_vcpu *vcpu, u32 intid)
> >> +{
> >> +	if (intid < VGIC_NR_PRIVATE_IRQS)
> >> +		kvm_arm_resume_vcpu(vcpu);
> >> +	else
> >> +		kvm_arm_resume_guest(vcpu->kvm);
> >> +}
> >> +
> >>  void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
> >>  			     gpa_t addr, unsigned int len,
> >>  			     unsigned long val)
> >> @@ -180,32 +240,12 @@ void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
> >>  	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> >>  	int i;
> >>  
> >> -	kvm_arm_halt_guest(vcpu->kvm);
> >> +	vgic_change_active_prepare(vcpu, intid);
> > 
> > You're still only halting the current vcpu, not the one targeted by the
> > interrupt.
> > 
> > Let's not bother with that for the time being. I'm happy to merge v1,
> > and optimize this when this starts bugging us (and this is going to take
> > a while).
> 
> Actually, I now realize that you are right. The MMIO handler is passing
> us the target vcpu, not the the one causing the exit. I'm an idiot.

You are most certainly not.

> 
> So this looks fine to me.
> 

Cool, thanks.

-Christoffer