[PATCH 0/3] KVM: arm/arm64: Fix some more timer related issues

[PATCH 0/3] KVM: arm/arm64: Fix some more timer related issues

[Christoffer Dall]

This little series addresses some problems we've been observing with the
arch timer.  First, we were fiddling with a PPI timer interrupt outside
of a preemptible section, which is bad for obvious reasons.  Second, we
were clearing the physical active state when we shouldn't.  Third, we
can simplify the vgic code by just considering the LR state instead of
the GIC physical state on guest return.

Christoffer Dall (3):
  KVM: arm/arm64: Fix preemptible timer active state crazyness
  KVM: arm/arm64: arch_timer: Preserve physical dist. active state on
    LR.active
  KVM: arm/arm64: vgic: Trust the LR state for HW IRQs

 arch/arm/kvm/arm.c        |  7 +------
 include/kvm/arm_vgic.h    |  2 +-
 virt/kvm/arm/arch_timer.c | 28 +++++++++++++++----------
 virt/kvm/arm/vgic.c       | 53 ++++++++++++++++++++++++-----------------------
 4 files changed, 46 insertions(+), 44 deletions(-)

-- 
2.1.2.330.g565301e.dirty

[PATCH 1/3] KVM: arm/arm64: Fix preemptible timer active state crazyness

[Christoffer Dall]

We were setting the physical active state on the GIC distributor in a
preemptible section, which could cause us to set the active state on
different physical CPU from the one we were actually going to run on,
hacoc ensues.

Since we are no longer descheduling/scheduling soft timers in the
flush/sync timer functions, simply moving the timer flush into a
non-preemptible section.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 arch/arm/kvm/arm.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index eab83b2..e06fd29 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -564,17 +564,12 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			vcpu_sleep(vcpu);
 
 		/*
-		 * Disarming the background timer must be done in a
-		 * preemptible context, as this call may sleep.
-		 */
-		kvm_timer_flush_hwstate(vcpu);
-
-		/*
 		 * Preparing the interrupts to be injected also
 		 * involves poking the GIC, which must be done in a
 		 * non-preemptible context.
 		 */
 		preempt_disable();
+		kvm_timer_flush_hwstate(vcpu);
 		kvm_vgic_flush_hwstate(vcpu);
 
 		local_irq_disable();
-- 
2.1.2.330.g565301e.dirty

[PATCH 1/3] KVM: arm/arm64: Fix preemptible timer active state crazyness

[Marc Zyngier]

On Tue, 24 Nov 2015 16:43:58 +0100
Christoffer Dall <christoffer.dall@linaro.org> wrote:

> We were setting the physical active state on the GIC distributor in a
> preemptible section, which could cause us to set the active state on
> different physical CPU from the one we were actually going to run on,
> hacoc ensues.
> 
> Since we are no longer descheduling/scheduling soft timers in the
> flush/sync timer functions, simply moving the timer flush into a
> non-preemptible section.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  arch/arm/kvm/arm.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index eab83b2..e06fd29 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -564,17 +564,12 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  			vcpu_sleep(vcpu);
>  
>  		/*
> -		 * Disarming the background timer must be done in a
> -		 * preemptible context, as this call may sleep.
> -		 */
> -		kvm_timer_flush_hwstate(vcpu);
> -
> -		/*
>  		 * Preparing the interrupts to be injected also
>  		 * involves poking the GIC, which must be done in a
>  		 * non-preemptible context.
>  		 */
>  		preempt_disable();
> +		kvm_timer_flush_hwstate(vcpu);
>  		kvm_vgic_flush_hwstate(vcpu);
>  
>  		local_irq_disable();

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

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

[PATCH 2/3] KVM: arm/arm64: arch_timer: Preserve physical dist. active state on LR.active

[Christoffer Dall]

We were incorrectly removing the active state from the physical
distributor on the timer interrupt when the timer output level was
deasserted.  We shouldn't be doing this without considering the virtual
interrupt's active state, because the architecture requires that when an
LR has the HW bit set and the pending or active bits set, then the
physical interrupt must also have the corresponding bits set.

This addresses an issue where we have been observing an inconsistency
between the LR state and the physical distributor state where the LR
state was active and the physical distributor was not active, which
shouldn't happen.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 include/kvm/arm_vgic.h    |  2 +-
 virt/kvm/arm/arch_timer.c | 28 +++++++++++++++++-----------
 virt/kvm/arm/vgic.c       | 37 +++++++++++++++++++++++++------------
 3 files changed, 43 insertions(+), 24 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 9c747cb..d2f4147 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -342,10 +342,10 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
 			       struct irq_phys_map *map, bool level);
 void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
 int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
-int kvm_vgic_vcpu_active_irq(struct kvm_vcpu *vcpu);
 struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
 					   int virt_irq, int irq);
 int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
+bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
 
 #define irqchip_in_kernel(k)	(!!((k)->arch.vgic.in_kernel))
 #define vgic_initialized(k)	(!!((k)->arch.vgic.nr_cpus))
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 21a0ab2..69bca18 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -221,17 +221,23 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
 	kvm_timer_update_state(vcpu);
 
 	/*
-	 * If we enter the guest with the virtual input level to the VGIC
-	 * asserted, then we have already told the VGIC what we need to, and
-	 * we don't need to exit from the guest until the guest deactivates
-	 * the already injected interrupt, so therefore we should set the
-	 * hardware active state to prevent unnecessary exits from the guest.
-	 *
-	 * Conversely, if the virtual input level is deasserted, then always
-	 * clear the hardware active state to ensure that hardware interrupts
-	 * from the timer triggers a guest exit.
-	 */
-	if (timer->irq.level)
+	* If we enter the guest with the virtual input level to the VGIC
+	* asserted, then we have already told the VGIC what we need to, and
+	* we don't need to exit from the guest until the guest deactivates
+	* the already injected interrupt, so therefore we should set the
+	* hardware active state to prevent unnecessary exits from the guest.
+	*
+	* Also, if we enter the guest with the virtual timer interrupt active,
+	* then it must be active on the physical distributor, because we set
+	* the HW bit and the guest must be able to deactivate the virtual and
+	* physical interrupt at the same time.
+	*
+	* Conversely, if the virtual input level is deasserted and the virtual
+	* interrupt is not active, then always clear the hardware active state
+	* to ensure that hardware interrupts from the timer triggers a guest
+	* exit.
+	*/
+	if (timer->irq.level || kvm_vgic_map_is_active(vcpu, timer->map))
 		phys_active = true;
 	else
 		phys_active = false;
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 5335383..9002f0d 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1096,6 +1096,30 @@ static void vgic_retire_lr(int lr_nr, struct kvm_vcpu *vcpu)
 	vgic_set_lr(vcpu, lr_nr, vlr);
 }
 
+static int dist_active_irq(struct kvm_vcpu *vcpu)
+{
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+	if (!irqchip_in_kernel(vcpu->kvm))
+		return 0;
+
+	return test_bit(vcpu->vcpu_id, dist->irq_active_on_cpu);
+}
+
+bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, struct irq_phys_map *map)
+{
+	int i;
+
+	for (i = 0; i < vcpu->arch.vgic_cpu.nr_lr; i++) {
+		struct vgic_lr vlr = vgic_get_lr(vcpu, i);
+
+		if (vlr.irq == map->virt_irq && vlr.state & LR_STATE_ACTIVE)
+			return true;
+	}
+
+	return dist_active_irq(vcpu);
+}
+
 /*
  * An interrupt may have been disabled after being made pending on the
  * CPU interface (the classic case is a timer running while we're
@@ -1248,7 +1272,7 @@ static void __kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
 	 * may have been serviced from another vcpu. In all cases,
 	 * move along.
 	 */
-	if (!kvm_vgic_vcpu_pending_irq(vcpu) && !kvm_vgic_vcpu_active_irq(vcpu))
+	if (!kvm_vgic_vcpu_pending_irq(vcpu) && !dist_active_irq(vcpu))
 		goto epilog;
 
 	/* SGIs */
@@ -1479,17 +1503,6 @@ int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu)
 	return test_bit(vcpu->vcpu_id, dist->irq_pending_on_cpu);
 }
 
-int kvm_vgic_vcpu_active_irq(struct kvm_vcpu *vcpu)
-{
-	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
-
-	if (!irqchip_in_kernel(vcpu->kvm))
-		return 0;
-
-	return test_bit(vcpu->vcpu_id, dist->irq_active_on_cpu);
-}
-
-
 void vgic_kick_vcpus(struct kvm *kvm)
 {
 	struct kvm_vcpu *vcpu;
-- 
2.1.2.330.g565301e.dirty

[PATCH 2/3] KVM: arm/arm64: arch_timer: Preserve physical dist. active state on LR.active

[Marc Zyngier]

On Tue, 24 Nov 2015 16:43:59 +0100
Christoffer Dall <christoffer.dall@linaro.org> wrote:

> We were incorrectly removing the active state from the physical
> distributor on the timer interrupt when the timer output level was
> deasserted.  We shouldn't be doing this without considering the virtual
> interrupt's active state, because the architecture requires that when an
> LR has the HW bit set and the pending or active bits set, then the
> physical interrupt must also have the corresponding bits set.
> 
> This addresses an issue where we have been observing an inconsistency
> between the LR state and the physical distributor state where the LR
> state was active and the physical distributor was not active, which
> shouldn't happen.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  include/kvm/arm_vgic.h    |  2 +-
>  virt/kvm/arm/arch_timer.c | 28 +++++++++++++++++-----------
>  virt/kvm/arm/vgic.c       | 37 +++++++++++++++++++++++++------------
>  3 files changed, 43 insertions(+), 24 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 9c747cb..d2f4147 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -342,10 +342,10 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
>  			       struct irq_phys_map *map, bool level);
>  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
> -int kvm_vgic_vcpu_active_irq(struct kvm_vcpu *vcpu);
>  struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
>  					   int virt_irq, int irq);
>  int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
> +bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
>  
>  #define irqchip_in_kernel(k)	(!!((k)->arch.vgic.in_kernel))
>  #define vgic_initialized(k)	(!!((k)->arch.vgic.nr_cpus))
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 21a0ab2..69bca18 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -221,17 +221,23 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
>  	kvm_timer_update_state(vcpu);
>  
>  	/*
> -	 * If we enter the guest with the virtual input level to the VGIC
> -	 * asserted, then we have already told the VGIC what we need to, and
> -	 * we don't need to exit from the guest until the guest deactivates
> -	 * the already injected interrupt, so therefore we should set the
> -	 * hardware active state to prevent unnecessary exits from the guest.
> -	 *
> -	 * Conversely, if the virtual input level is deasserted, then always
> -	 * clear the hardware active state to ensure that hardware interrupts
> -	 * from the timer triggers a guest exit.
> -	 */
> -	if (timer->irq.level)
> +	* If we enter the guest with the virtual input level to the VGIC
> +	* asserted, then we have already told the VGIC what we need to, and
> +	* we don't need to exit from the guest until the guest deactivates
> +	* the already injected interrupt, so therefore we should set the
> +	* hardware active state to prevent unnecessary exits from the guest.
> +	*
> +	* Also, if we enter the guest with the virtual timer interrupt active,
> +	* then it must be active on the physical distributor, because we set
> +	* the HW bit and the guest must be able to deactivate the virtual and
> +	* physical interrupt at the same time.
> +	*
> +	* Conversely, if the virtual input level is deasserted and the virtual
> +	* interrupt is not active, then always clear the hardware active state
> +	* to ensure that hardware interrupts from the timer triggers a guest
> +	* exit.
> +	*/
> +	if (timer->irq.level || kvm_vgic_map_is_active(vcpu, timer->map))
>  		phys_active = true;
>  	else
>  		phys_active = false;
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 5335383..9002f0d 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1096,6 +1096,30 @@ static void vgic_retire_lr(int lr_nr, struct kvm_vcpu *vcpu)
>  	vgic_set_lr(vcpu, lr_nr, vlr);
>  }
>  
> +static int dist_active_irq(struct kvm_vcpu *vcpu)

bool? That'd be consistent with kvm_vgic_map_is_active.

> +{
> +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +
> +	if (!irqchip_in_kernel(vcpu->kvm))
> +		return 0;
> +

I believe you can drop this test, as the only other use case for this
function is on the flush path, which obviously mandates an in-kernel
irqchip.

> +	return test_bit(vcpu->vcpu_id, dist->irq_active_on_cpu);
> +}
> +
> +bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, struct irq_phys_map *map)
> +{
> +	int i;
> +
> +	for (i = 0; i < vcpu->arch.vgic_cpu.nr_lr; i++) {
> +		struct vgic_lr vlr = vgic_get_lr(vcpu, i);
> +
> +		if (vlr.irq == map->virt_irq && vlr.state & LR_STATE_ACTIVE)
> +			return true;
> +	}
> +
> +	return dist_active_irq(vcpu);
> +}
> +
>  /*
>   * An interrupt may have been disabled after being made pending on the
>   * CPU interface (the classic case is a timer running while we're
> @@ -1248,7 +1272,7 @@ static void __kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
>  	 * may have been serviced from another vcpu. In all cases,
>  	 * move along.
>  	 */
> -	if (!kvm_vgic_vcpu_pending_irq(vcpu) && !kvm_vgic_vcpu_active_irq(vcpu))
> +	if (!kvm_vgic_vcpu_pending_irq(vcpu) && !dist_active_irq(vcpu))
>  		goto epilog;
>  
>  	/* SGIs */
> @@ -1479,17 +1503,6 @@ int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu)
>  	return test_bit(vcpu->vcpu_id, dist->irq_pending_on_cpu);
>  }
>  
> -int kvm_vgic_vcpu_active_irq(struct kvm_vcpu *vcpu)
> -{
> -	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> -
> -	if (!irqchip_in_kernel(vcpu->kvm))
> -		return 0;
> -
> -	return test_bit(vcpu->vcpu_id, dist->irq_active_on_cpu);
> -}
> -
> -
>  void vgic_kick_vcpus(struct kvm *kvm)
>  {
>  	struct kvm_vcpu *vcpu;


Other than the above nits:

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

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

[PATCH 3/3] KVM: arm/arm64: vgic: Trust the LR state for HW IRQs

[Christoffer Dall]

We were probing the physial distributor state for the active state of a
HW virtual IRQ, because we had seen evidence that the LR state was not
cleared when the guest deactivated a virtual interrupted.

However, this issue turned out to be a software bug in the GIC, which
was solved by: 84aab5e68c2a5e1e18d81ae8308c3ce25d501b29
(KVM: arm/arm64: arch_timer: Preserve physical dist. active
state on LR.active, 2015-11-24)

Therefore, get rid of the complexities and just look at the LR.

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

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 9002f0d..55cd7e3 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1420,25 +1420,13 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
 static bool vgic_sync_hwirq(struct kvm_vcpu *vcpu, int lr, struct vgic_lr vlr)
 {
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
-	struct irq_phys_map *map;
-	bool phys_active;
 	bool level_pending;
-	int ret;
 
 	if (!(vlr.state & LR_HW))
 		return false;
 
-	map = vgic_irq_map_search(vcpu, vlr.irq);
-	BUG_ON(!map);
-
-	ret = irq_get_irqchip_state(map->irq,
-				    IRQCHIP_STATE_ACTIVE,
-				    &phys_active);
-
-	WARN_ON(ret);
-
-	if (phys_active)
-		return 0;
+	if (vlr.state & LR_STATE_ACTIVE)
+		return false;
 
 	spin_lock(&dist->lock);
 	level_pending = process_queued_irq(vcpu, lr, vlr);
-- 
2.1.2.330.g565301e.dirty

[PATCH 3/3] KVM: arm/arm64: vgic: Trust the LR state for HW IRQs

[Marc Zyngier]

On Tue, 24 Nov 2015 16:44:00 +0100
Christoffer Dall <christoffer.dall@linaro.org> wrote:

> We were probing the physial distributor state for the active state of a
> HW virtual IRQ, because we had seen evidence that the LR state was not
> cleared when the guest deactivated a virtual interrupted.
> 
> However, this issue turned out to be a software bug in the GIC, which
> was solved by: 84aab5e68c2a5e1e18d81ae8308c3ce25d501b29
> (KVM: arm/arm64: arch_timer: Preserve physical dist. active
> state on LR.active, 2015-11-24)
> 
> Therefore, get rid of the complexities and just look at the LR.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  virt/kvm/arm/vgic.c | 16 ++--------------
>  1 file changed, 2 insertions(+), 14 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 9002f0d..55cd7e3 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1420,25 +1420,13 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
>  static bool vgic_sync_hwirq(struct kvm_vcpu *vcpu, int lr, struct vgic_lr vlr)
>  {
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> -	struct irq_phys_map *map;
> -	bool phys_active;
>  	bool level_pending;
> -	int ret;
>  
>  	if (!(vlr.state & LR_HW))
>  		return false;
>  
> -	map = vgic_irq_map_search(vcpu, vlr.irq);
> -	BUG_ON(!map);
> -
> -	ret = irq_get_irqchip_state(map->irq,
> -				    IRQCHIP_STATE_ACTIVE,
> -				    &phys_active);
> -
> -	WARN_ON(ret);
> -
> -	if (phys_active)
> -		return 0;
> +	if (vlr.state & LR_STATE_ACTIVE)
> +		return false;
>  
>  	spin_lock(&dist->lock);
>  	level_pending = process_queued_irq(vcpu, lr, vlr);

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

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