[PATCH 0/3] Additional fixes for kvmarm/next

[PATCH 0/3] Additional fixes for kvmarm/next

[Christoffer Dall]

A bit last minute, we have three fixes for patches already queued in
kvmarm/next related to the level-triggered mapped interrupts support.

These patches apply on kvmarm/next and are pushed to kvmarm/queue.

Thanks,
-Christoffer

Christoffer Dall (3):
  KVM: arm/arm64: Fix incorrect timer_is_pending logic
  KVM: arm/arm64: Fix userspace_irqchip_in_use counting
  KVM: arm/arm64: Fixup userspace irqchip static key optimization

 virt/kvm/arm/arch_timer.c | 38 ++++++++++++++++++--------------------
 virt/kvm/arm/arm.c        |  5 +++--
 2 files changed, 21 insertions(+), 22 deletions(-)

-- 
2.14.2

[PATCH 1/3] KVM: arm/arm64: Fix incorrect timer_is_pending logic

[Christoffer Dall]

After the recently introduced support for level-triggered mapped
interrupt, I accidentally left the VCPU thread busily going back and
forward between the guest and the hypervisor whenever the guest was
blocking, because I would always incorrectly report that a timer
interrupt was pending.

This is because the timer->irq.level field is not valid for mapped
interrupts, where we offload the level state to the hardware, and as a
result this field is always true.

Luckily the problem can be relatively easily solved by not checking the
cached signal state of either timer in kvm_timer_should_fire() but
instead compute the timer state on the fly, which we do already if the
cached signal state wasn't high.  In fact, the only reason for checking
the cached signal state was a tiny optimization which would only be
potentially faster when the polling loop detects a pending timer
interrupt, which is quite unlikely.

Instead of duplicating the logic from kvm_arch_timer_handler(), we
enlighten kvm_timer_should_fire() to report something valid when the
timer state is loaded onto the hardware.  We can then call this from
kvm_arch_timer_handler() as well and avoid the call to
__timer_snapshot_state() in kvm_arch_timer_get_input_level().

Reported-by: Tomasz Nowicki <tn@semihalf.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 virt/kvm/arm/arch_timer.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index cfcd0323deab..63cf828f3c4f 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -97,10 +97,9 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
 		pr_warn_once("Spurious arch timer IRQ on non-VCPU thread\n");
 		return IRQ_NONE;
 	}
-	vtimer = vcpu_vtimer(vcpu);
 
-	vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
-	if (kvm_timer_irq_can_fire(vtimer))
+	vtimer = vcpu_vtimer(vcpu);
+	if (kvm_timer_should_fire(vtimer))
 		kvm_timer_update_irq(vcpu, true, vtimer);
 
 	if (static_branch_unlikely(&userspace_irqchip_in_use) &&
@@ -230,6 +229,16 @@ static bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx)
 {
 	u64 cval, now;
 
+	if (timer_ctx->loaded) {
+		u32 cnt_ctl;
+
+		/* Only the virtual timer can be loaded so far */
+		cnt_ctl = read_sysreg_el0(cntv_ctl);
+		return  (cnt_ctl & ARCH_TIMER_CTRL_ENABLE) &&
+		        (cnt_ctl & ARCH_TIMER_CTRL_IT_STAT) &&
+		       !(cnt_ctl & ARCH_TIMER_CTRL_IT_MASK);
+	}
+
 	if (!kvm_timer_irq_can_fire(timer_ctx))
 		return false;
 
@@ -244,15 +253,7 @@ bool kvm_timer_is_pending(struct kvm_vcpu *vcpu)
 	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
 
-	if (vtimer->irq.level || ptimer->irq.level)
-		return true;
-
-	/*
-	 * When this is called from withing the wait loop of kvm_vcpu_block(),
-	 * the software view of the timer state is up to date (timer->loaded
-	 * is false), and so we can simply check if the timer should fire now.
-	 */
-	if (!vtimer->loaded && kvm_timer_should_fire(vtimer))
+	if (kvm_timer_should_fire(vtimer))
 		return true;
 
 	return kvm_timer_should_fire(ptimer);
@@ -270,9 +271,9 @@ void kvm_timer_update_run(struct kvm_vcpu *vcpu)
 	/* Populate the device bitmap with the timer states */
 	regs->device_irq_level &= ~(KVM_ARM_DEV_EL1_VTIMER |
 				    KVM_ARM_DEV_EL1_PTIMER);
-	if (vtimer->irq.level)
+	if (kvm_timer_should_fire(vtimer))
 		regs->device_irq_level |= KVM_ARM_DEV_EL1_VTIMER;
-	if (ptimer->irq.level)
+	if (kvm_timer_should_fire(ptimer))
 		regs->device_irq_level |= KVM_ARM_DEV_EL1_PTIMER;
 }
 
@@ -507,8 +508,8 @@ bool kvm_timer_should_notify_user(struct kvm_vcpu *vcpu)
 	vlevel = sregs->device_irq_level & KVM_ARM_DEV_EL1_VTIMER;
 	plevel = sregs->device_irq_level & KVM_ARM_DEV_EL1_PTIMER;
 
-	return vtimer->irq.level != vlevel ||
-	       ptimer->irq.level != plevel;
+	return kvm_timer_should_fire(vtimer) != vlevel ||
+	       kvm_timer_should_fire(ptimer) != plevel;
 }
 
 void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
@@ -801,9 +802,6 @@ bool kvm_arch_timer_get_input_level(int vintid)
 	else
 		BUG(); /* We only map the vtimer so far */
 
-	if (timer->loaded)
-		__timer_snapshot_state(timer);
-
 	return kvm_timer_should_fire(timer);
 }
 
-- 
2.14.2

[PATCH 1/3] KVM: arm/arm64: Fix incorrect timer_is_pending logic

[Tomasz Nowicki]

Hi Christoffer,

I confirm the patch fixes the issue I saw before. I do not see 
unnecessary WFI trap storm any more, hence:

Tested-by: Tomasz Nowicki <tn@semihalf.com>

Thank you for fixing this.

Tomasz

On 30.01.2018 13:42, Christoffer Dall wrote:
> After the recently introduced support for level-triggered mapped
> interrupt, I accidentally left the VCPU thread busily going back and
> forward between the guest and the hypervisor whenever the guest was
> blocking, because I would always incorrectly report that a timer
> interrupt was pending.
> 
> This is because the timer->irq.level field is not valid for mapped
> interrupts, where we offload the level state to the hardware, and as a
> result this field is always true.
> 
> Luckily the problem can be relatively easily solved by not checking the
> cached signal state of either timer in kvm_timer_should_fire() but
> instead compute the timer state on the fly, which we do already if the
> cached signal state wasn't high.  In fact, the only reason for checking
> the cached signal state was a tiny optimization which would only be
> potentially faster when the polling loop detects a pending timer
> interrupt, which is quite unlikely.
> 
> Instead of duplicating the logic from kvm_arch_timer_handler(), we
> enlighten kvm_timer_should_fire() to report something valid when the
> timer state is loaded onto the hardware.  We can then call this from
> kvm_arch_timer_handler() as well and avoid the call to
> __timer_snapshot_state() in kvm_arch_timer_get_input_level().
> 
> Reported-by: Tomasz Nowicki <tn@semihalf.com>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>   virt/kvm/arm/arch_timer.c | 36 +++++++++++++++++-------------------
>   1 file changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index cfcd0323deab..63cf828f3c4f 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -97,10 +97,9 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
>   		pr_warn_once("Spurious arch timer IRQ on non-VCPU thread\n");
>   		return IRQ_NONE;
>   	}
> -	vtimer = vcpu_vtimer(vcpu);
>   
> -	vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl);
> -	if (kvm_timer_irq_can_fire(vtimer))
> +	vtimer = vcpu_vtimer(vcpu);
> +	if (kvm_timer_should_fire(vtimer))
>   		kvm_timer_update_irq(vcpu, true, vtimer);
>   
>   	if (static_branch_unlikely(&userspace_irqchip_in_use) &&
> @@ -230,6 +229,16 @@ static bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx)
>   {
>   	u64 cval, now;
>   
> +	if (timer_ctx->loaded) {
> +		u32 cnt_ctl;
> +
> +		/* Only the virtual timer can be loaded so far */
> +		cnt_ctl = read_sysreg_el0(cntv_ctl);
> +		return  (cnt_ctl & ARCH_TIMER_CTRL_ENABLE) &&
> +		        (cnt_ctl & ARCH_TIMER_CTRL_IT_STAT) &&
> +		       !(cnt_ctl & ARCH_TIMER_CTRL_IT_MASK);
> +	}
> +
>   	if (!kvm_timer_irq_can_fire(timer_ctx))
>   		return false;
>   
> @@ -244,15 +253,7 @@ bool kvm_timer_is_pending(struct kvm_vcpu *vcpu)
>   	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
>   	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
>   
> -	if (vtimer->irq.level || ptimer->irq.level)
> -		return true;
> -
> -	/*
> -	 * When this is called from withing the wait loop of kvm_vcpu_block(),
> -	 * the software view of the timer state is up to date (timer->loaded
> -	 * is false), and so we can simply check if the timer should fire now.
> -	 */
> -	if (!vtimer->loaded && kvm_timer_should_fire(vtimer))
> +	if (kvm_timer_should_fire(vtimer))
>   		return true;
>   
>   	return kvm_timer_should_fire(ptimer);
> @@ -270,9 +271,9 @@ void kvm_timer_update_run(struct kvm_vcpu *vcpu)
>   	/* Populate the device bitmap with the timer states */
>   	regs->device_irq_level &= ~(KVM_ARM_DEV_EL1_VTIMER |
>   				    KVM_ARM_DEV_EL1_PTIMER);
> -	if (vtimer->irq.level)
> +	if (kvm_timer_should_fire(vtimer))
>   		regs->device_irq_level |= KVM_ARM_DEV_EL1_VTIMER;
> -	if (ptimer->irq.level)
> +	if (kvm_timer_should_fire(ptimer))
>   		regs->device_irq_level |= KVM_ARM_DEV_EL1_PTIMER;
>   }
>   
> @@ -507,8 +508,8 @@ bool kvm_timer_should_notify_user(struct kvm_vcpu *vcpu)
>   	vlevel = sregs->device_irq_level & KVM_ARM_DEV_EL1_VTIMER;
>   	plevel = sregs->device_irq_level & KVM_ARM_DEV_EL1_PTIMER;
>   
> -	return vtimer->irq.level != vlevel ||
> -	       ptimer->irq.level != plevel;
> +	return kvm_timer_should_fire(vtimer) != vlevel ||
> +	       kvm_timer_should_fire(ptimer) != plevel;
>   }
>   
>   void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
> @@ -801,9 +802,6 @@ bool kvm_arch_timer_get_input_level(int vintid)
>   	else
>   		BUG(); /* We only map the vtimer so far */
>   
> -	if (timer->loaded)
> -		__timer_snapshot_state(timer);
> -
>   	return kvm_timer_should_fire(timer);
>   }
>   
> 

[PATCH 1/3] KVM: arm/arm64: Fix incorrect timer_is_pending logic

[Marc Zyngier]

On 30/01/18 12:42, Christoffer Dall wrote:
> After the recently introduced support for level-triggered mapped
> interrupt, I accidentally left the VCPU thread busily going back and
> forward between the guest and the hypervisor whenever the guest was
> blocking, because I would always incorrectly report that a timer
> interrupt was pending.
> 
> This is because the timer->irq.level field is not valid for mapped
> interrupts, where we offload the level state to the hardware, and as a
> result this field is always true.
> 
> Luckily the problem can be relatively easily solved by not checking the
> cached signal state of either timer in kvm_timer_should_fire() but
> instead compute the timer state on the fly, which we do already if the
> cached signal state wasn't high.  In fact, the only reason for checking
> the cached signal state was a tiny optimization which would only be
> potentially faster when the polling loop detects a pending timer
> interrupt, which is quite unlikely.
> 
> Instead of duplicating the logic from kvm_arch_timer_handler(), we
> enlighten kvm_timer_should_fire() to report something valid when the
> timer state is loaded onto the hardware.  We can then call this from
> kvm_arch_timer_handler() as well and avoid the call to
> __timer_snapshot_state() in kvm_arch_timer_get_input_level().
> 
> Reported-by: Tomasz Nowicki <tn@semihalf.com>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>

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

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

[PATCH 2/3] KVM: arm/arm64: Fix userspace_irqchip_in_use counting

[Christoffer Dall]

We were not decrementing the static key count in the right location.
kvm_arch_vcpu_destroy() is only called to clean up after a failed
VCPU create attempt, whereas kvm_arch_vcpu_free() is called on teardown
of the VM as well.  Move the static key decrement call to
kvm_arch_vcpu_free().

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 virt/kvm/arm/arm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 639dca0c0560..04ee7a327870 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -295,6 +295,9 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
 
 void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu)
 {
+	if (vcpu->arch.has_run_once && unlikely(!irqchip_in_kernel(vcpu->kvm)))
+		static_branch_dec(&userspace_irqchip_in_use);
+
 	kvm_mmu_free_memory_caches(vcpu);
 	kvm_timer_vcpu_terminate(vcpu);
 	kvm_pmu_vcpu_destroy(vcpu);
@@ -304,8 +307,6 @@ void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu)
 
 void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
 {
-	if (vcpu->arch.has_run_once && unlikely(!irqchip_in_kernel(vcpu->kvm)))
-		static_branch_dec(&userspace_irqchip_in_use);
 	kvm_arch_vcpu_free(vcpu);
 }
 
-- 
2.14.2

[PATCH 2/3] KVM: arm/arm64: Fix userspace_irqchip_in_use counting

[Marc Zyngier]

On 30/01/18 12:42, Christoffer Dall wrote:
> We were not decrementing the static key count in the right location.
> kvm_arch_vcpu_destroy() is only called to clean up after a failed
> VCPU create attempt, whereas kvm_arch_vcpu_free() is called on teardown
> of the VM as well.  Move the static key decrement call to
> kvm_arch_vcpu_free().
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>

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

	N,
-- 
Jazz is not dead. It just smells funny...

[PATCH 3/3] KVM: arm/arm64: Fixup userspace irqchip static key optimization

[Christoffer Dall]

When I introduced a static key to avoid work in the critical path for
userspace irqchips which is very rarely used, I accidentally messed up
my logic and used && where I should have used ||, because the point was
to short-circuit the evaluation in case userspace irqchips weren't even
in use.

This fixes an issue when running in-kernel irqchip VMs alongside
userspace irqchip VMs.

Fixes: c44c232ee2d3 ("KVM: arm/arm64: Avoid work when userspace iqchips are not used")
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 virt/kvm/arm/arch_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 63cf828f3c4f..fb6bd9b9845e 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -286,7 +286,7 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
 	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
 				   timer_ctx->irq.level);
 
-	if (!static_branch_unlikely(&userspace_irqchip_in_use) &&
+	if (!static_branch_unlikely(&userspace_irqchip_in_use) ||
 	    likely(irqchip_in_kernel(vcpu->kvm))) {
 		ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
 					  timer_ctx->irq.irq,
-- 
2.14.2

[PATCH 3/3] KVM: arm/arm64: Fixup userspace irqchip static key optimization

[Marc Zyngier]

On 30/01/18 12:42, Christoffer Dall wrote:
> When I introduced a static key to avoid work in the critical path for
> userspace irqchips which is very rarely used, I accidentally messed up
> my logic and used && where I should have used ||, because the point was
> to short-circuit the evaluation in case userspace irqchips weren't even
> in use.
> 
> This fixes an issue when running in-kernel irqchip VMs alongside
> userspace irqchip VMs.
> 
> Fixes: c44c232ee2d3 ("KVM: arm/arm64: Avoid work when userspace iqchips are not used")
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>

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

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