[PATCH v2] KVM: arm/arm64: Let vcpu thread modify its own active state

[PATCH v2] KVM: arm/arm64: Let vcpu thread modify its own active state

[Christoffer Dall]

From: Jintack Lim <jintack@cs.columbia.edu>

Currently, if a vcpu thread tries to change the active state of an
interrupt which is already on the same vcpu's AP list, Since the VGIC
mmio handler is called after a vcpu has already synced back the LR state
to the struct vgic_irq, we can just let it proceed safely.

Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
---
Changes since v1:
 - Reworked comment
 - Consider userspace accesses
 - Get the right requester VCPU for GICv3 private IRQ accesses
 - Tested using kvm-unit-tests and verified that it deadlocked without
   this patch and passed the test with this patch :)

 virt/kvm/arm/vgic/vgic-mmio.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index 3654b4c..2a5db13 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -180,21 +180,37 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
 static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
 				    bool new_active_state)
 {
+	struct kvm_vcpu *requester_vcpu;
 	spin_lock(&irq->irq_lock);
+
+	/*
+	 * The vcpu parameter here can mean multiple things depending on how
+	 * this function is called; when handling a trap from the kernel it
+	 * depends on the GIC version, and these functions are also called as
+	 * part of save/restore from userspace.
+	 *
+	 * Therefore, we have to figure out the requester in a reliable way.
+	 *
+	 * When accessing VGIC state from user space, the requester_vcpu is
+	 * NULL, which is fine, because we guarantee that no VCPUs are running
+	 * when accessing VGIC state from user space so irq->vcpu->cpu is
+	 * always -1.
+	 */
+	requester_vcpu = kvm_arm_get_running_vcpu();
+
 	/*
 	 * 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.
+	 * synced back the LR state to the struct vgic_irq.
 	 *
-	 * 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.
+	 * As long as the conditions below are true, we know the VCPU thread
+	 * may be on its way back from the guest (we kicked the VCPU thread in
+	 * vgic_change_active_prepare)  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 != requester_vcpu && /* Current thread is not the VCPU thread */
 	       irq->vcpu->cpu != -1) /* VCPU thread is running */
 		cond_resched_lock(&irq->irq_lock);
 
-- 
2.5.0

[PATCH v2] KVM: arm/arm64: Let vcpu thread modify its own active state

[Marc Zyngier]

On Mon, Mar 06 2017 at  1:42:37 pm GMT, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> From: Jintack Lim <jintack@cs.columbia.edu>
>
> Currently, if a vcpu thread tries to change the active state of an
> interrupt which is already on the same vcpu's AP list, Since the VGIC
> mmio handler is called after a vcpu has already synced back the LR state
> to the struct vgic_irq, we can just let it proceed safely.
>
> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead, it just smell funny.

[PATCH v2] KVM: arm/arm64: Let vcpu thread modify its own active state

[Jintack Lim]

Hi Christoffer,

thanks for submitting this patch.

On Mon, Mar 6, 2017 at 8:42 AM, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> From: Jintack Lim <jintack@cs.columbia.edu>
>
> Currently, if a vcpu thread tries to change the active state of an
> interrupt which is already on the same vcpu's AP list,

"it'll loop forever" is remove accidentally in the commit message in v2?

> Since the VGIC
> mmio handler is called after a vcpu has already synced back the LR state
> to the struct vgic_irq, we can just let it proceed safely.
>
> Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
> ---
> Changes since v1:
>  - Reworked comment
>  - Consider userspace accesses
>  - Get the right requester VCPU for GICv3 private IRQ accesses
>  - Tested using kvm-unit-tests and verified that it deadlocked without
>    this patch and passed the test with this patch :)

nice!

>
>  virt/kvm/arm/vgic/vgic-mmio.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index 3654b4c..2a5db13 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -180,21 +180,37 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
>  static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
>                                     bool new_active_state)
>  {
> +       struct kvm_vcpu *requester_vcpu;
>         spin_lock(&irq->irq_lock);
> +
> +       /*
> +        * The vcpu parameter here can mean multiple things depending on how
> +        * this function is called; when handling a trap from the kernel it
> +        * depends on the GIC version, and these functions are also called as
> +        * part of save/restore from userspace.
> +        *
> +        * Therefore, we have to figure out the requester in a reliable way.
> +        *
> +        * When accessing VGIC state from user space, the requester_vcpu is
> +        * NULL, which is fine, because we guarantee that no VCPUs are running
> +        * when accessing VGIC state from user space so irq->vcpu->cpu is
> +        * always -1.
> +        */
> +       requester_vcpu = kvm_arm_get_running_vcpu();
> +
>         /*
>          * 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.
> +        * synced back the LR state to the struct vgic_irq.
>          *
> -        * 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.
> +        * As long as the conditions below are true, we know the VCPU thread
> +        * may be on its way back from the guest (we kicked the VCPU thread in
> +        * vgic_change_active_prepare)  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 != requester_vcpu && /* Current thread is not the VCPU thread */
>                irq->vcpu->cpu != -1) /* VCPU thread is running */
>                 cond_resched_lock(&irq->irq_lock);
>
> --
> 2.5.0
>
>

[PATCH v2] KVM: arm/arm64: Let vcpu thread modify its own active state

[Christoffer Dall]

On Mon, Mar 06, 2017 at 02:20:44PM -0500, Jintack Lim wrote:
> Hi Christoffer,
> 
> thanks for submitting this patch.
> 
> On Mon, Mar 6, 2017 at 8:42 AM, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > From: Jintack Lim <jintack@cs.columbia.edu>
> >
> > Currently, if a vcpu thread tries to change the active state of an
> > interrupt which is already on the same vcpu's AP list,
> 
> "it'll loop forever" is remove accidentally in the commit message in v2?

yes, we can fix this up when applying the patch.  Thanks.

> 
> > Since the VGIC
> > mmio handler is called after a vcpu has already synced back the LR state
> > to the struct vgic_irq, we can just let it proceed safely.
> >
> > Signed-off-by: Jintack Lim <jintack@cs.columbia.edu>
> > ---
> > Changes since v1:
> >  - Reworked comment
> >  - Consider userspace accesses
> >  - Get the right requester VCPU for GICv3 private IRQ accesses
> >  - Tested using kvm-unit-tests and verified that it deadlocked without
> >    this patch and passed the test with this patch :)
> 
> nice!
> 
> >
> >  virt/kvm/arm/vgic/vgic-mmio.c | 32 ++++++++++++++++++++++++--------
> >  1 file changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> > index 3654b4c..2a5db13 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> > @@ -180,21 +180,37 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> >  static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> >                                     bool new_active_state)
> >  {
> > +       struct kvm_vcpu *requester_vcpu;
> >         spin_lock(&irq->irq_lock);
> > +
> > +       /*
> > +        * The vcpu parameter here can mean multiple things depending on how
> > +        * this function is called; when handling a trap from the kernel it
> > +        * depends on the GIC version, and these functions are also called as
> > +        * part of save/restore from userspace.
> > +        *
> > +        * Therefore, we have to figure out the requester in a reliable way.
> > +        *
> > +        * When accessing VGIC state from user space, the requester_vcpu is
> > +        * NULL, which is fine, because we guarantee that no VCPUs are running
> > +        * when accessing VGIC state from user space so irq->vcpu->cpu is
> > +        * always -1.
> > +        */
> > +       requester_vcpu = kvm_arm_get_running_vcpu();
> > +
> >         /*
> >          * 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.
> > +        * synced back the LR state to the struct vgic_irq.
> >          *
> > -        * 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.
> > +        * As long as the conditions below are true, we know the VCPU thread
> > +        * may be on its way back from the guest (we kicked the VCPU thread in
> > +        * vgic_change_active_prepare)  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 != requester_vcpu && /* Current thread is not the VCPU thread */
> >                irq->vcpu->cpu != -1) /* VCPU thread is running */
> >                 cond_resched_lock(&irq->irq_lock);
> >
> > --
> > 2.5.0
> >
> >
>