[PATCH 0/3] arm/arm64: KVM: arch timer boot fixes

[PATCH 0/3] arm/arm64: KVM: arch timer boot fixes

[Christoffer Dall]

This small series fixes a number of issues relating to the arch timer
switching to using the active state instead of the masking hack in v4.3.

The first patch fixes an issue where the guest doesn't proceed after
PSCI reboots, because the arch timer is screaming at the physical GIC.
The second patch fixes an issue where the guest gets stuck after calling
WFI during boot, because the timer never fires.  The third patch fixes
an issue that occasionally causes warnings when queueing interrupts
because the distributor and CPU interface state gets inconsistent.

I stole some of the code for patch 2 from Marc's initial patch looking
to solve the problem solved in patch 1 here.

This is going to conflict horribly with some of the stuff we have in
next at the moment, so assumning there are no objections to these
patches, I'll rework the stuff in next after adding these to our current
list of fixes.

Christoffer Dall (3):
  arm/arm64: KVM: Fix arch timer behavior for disabled interrupts
  arm/arm64: KVM: Clear map->active on pend/active clear
  arm/arm64: KVM: Fix disabled distributor operation

 virt/kvm/arm/arch_timer.c | 19 +++++++++++
 virt/kvm/arm/vgic.c       | 86 ++++++++++++++++++++++++++---------------------
 2 files changed, 67 insertions(+), 38 deletions(-)

-- 
2.1.2.330.g565301e.dirty

[PATCH 1/3] arm/arm64: KVM: Fix arch timer behavior for disabled interrupts

[Christoffer Dall]

We have an interesting issue when the guest disables the timer interrupt
on the VGIC, which happens when turning VCPUs off using PSCI, for
example.

The problem is that because the guest disables the virtual interrupt at
the VGIC level, we never inject interrupts to the guest and therefore
never mark the interrupt as active on the physical distributor.  The
host also never takes the timer interrupt (we only use the timer device
to trigger a guest exit and everything else is done in software), so the
interrupt does not become active through normal means.

The result is that we keep entering the guest with a programmed timer
that will always fire as soon as we context switch the hardware timer
state and run the guest, preventing forward progress for the VCPU.

Since the active state on the physical distributor is really part of the
timer logic, it is the job of our virtual arch timer driver to manage
this state.

The timer->map->active boolean field indicates whether we have signalled
this interrupt to the vgic and if that interrupt is still pending or
active.  As long as that is the case, the hardware doesn't have to
generate physical interrupts and therefore we mark the interrupt as
active on the physical distributor.

Cc: Marc Zyngier <marc.zyngier@arm.com>
Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 virt/kvm/arm/arch_timer.c | 19 +++++++++++++++++++
 virt/kvm/arm/vgic.c       | 43 +++++++++++--------------------------------
 2 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 48c6e1a..b9d3a32 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -137,6 +137,8 @@ bool kvm_timer_should_fire(struct kvm_vcpu *vcpu)
 void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	bool phys_active;
+	int ret;
 
 	/*
 	 * We're about to run this vcpu again, so there is no need to
@@ -151,6 +153,23 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
 	 */
 	if (kvm_timer_should_fire(vcpu))
 		kvm_timer_inject_irq(vcpu);
+
+	/*
+	 * We keep track of whether the edge-triggered interrupt has been
+	 * signalled to the vgic/guest, and if so, we mask the interrupt and
+	 * the physical distributor to prevent the timer from raising a
+	 * physical interrupt whenever we run a guest, preventing forward
+	 * VCPU progress.
+	 */
+	if (kvm_vgic_get_phys_irq_active(timer->map))
+		phys_active = true;
+	else
+		phys_active = false;
+
+	ret = irq_set_irqchip_state(timer->map->irq,
+				    IRQCHIP_STATE_ACTIVE,
+				    phys_active);
+	WARN_ON(ret);
 }
 
 /**
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 596455a..ea21bc2 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1092,6 +1092,15 @@ static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu)
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_lr vlr = vgic_get_lr(vcpu, lr_nr);
 
+	/*
+	 * We must transfer the pending state back to the distributor before
+	 * retiring the LR, otherwise we may loose edge-triggered interrupts.
+	 */
+	if (vlr.state & LR_STATE_PENDING) {
+		vgic_dist_irq_set_pending(vcpu, irq);
+		vlr.hwirq = 0;
+	}
+
 	vlr.state = 0;
 	vgic_set_lr(vcpu, lr_nr, vlr);
 	clear_bit(lr_nr, vgic_cpu->lr_used);
@@ -1241,7 +1250,7 @@ static void __kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 	unsigned long *pa_percpu, *pa_shared;
-	int i, vcpu_id, lr, ret;
+	int i, vcpu_id;
 	int overflow = 0;
 	int nr_shared = vgic_nr_shared_irqs(dist);
 
@@ -1296,31 +1305,6 @@ epilog:
 		 */
 		clear_bit(vcpu_id, dist->irq_pending_on_cpu);
 	}
-
-	for (lr = 0; lr < vgic->nr_lr; lr++) {
-		struct vgic_lr vlr;
-
-		if (!test_bit(lr, vgic_cpu->lr_used))
-			continue;
-
-		vlr = vgic_get_lr(vcpu, lr);
-
-		/*
-		 * If we have a mapping, and the virtual interrupt is
-		 * presented to the guest (as pending or active), then we must
-		 * set the state to active in the physical world. See
-		 * Documentation/virtual/kvm/arm/vgic-mapped-irqs.txt.
-		 */
-		if (vlr.state & LR_HW) {
-			struct irq_phys_map *map;
-			map = vgic_irq_map_search(vcpu, vlr.irq);
-
-			ret = irq_set_irqchip_state(map->irq,
-						    IRQCHIP_STATE_ACTIVE,
-						    true);
-			WARN_ON(ret);
-		}
-	}
 }
 
 static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
@@ -1430,13 +1414,8 @@ static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr)
 
 	WARN_ON(ret);
 
-	if (map->active) {
-		ret = irq_set_irqchip_state(map->irq,
-					    IRQCHIP_STATE_ACTIVE,
-					    false);
-		WARN_ON(ret);
+	if (map->active)
 		return 0;
-	}
 
 	return 1;
 }
-- 
2.1.2.330.g565301e.dirty

[PATCH 2/3] arm/arm64: KVM: Clear map->active on pend/active clear

[Christoffer Dall]

When a guest reboots or offlines/onlines CPUs, it is not uncommon for it
to clear the pending and active states of an interrupt through the
emulated VGIC distributor.  However, since we emulate an edge-triggered
based on a level-triggered device, the guest expects the timer interrupt
to hit even after clearing the pending state.

We currently do not signal the VGIC when the map->active field is true,
because it indicates that the guest has already been signalled of the
interrupt as required.  Normally this field is set to false when the
guest deactivates the virtual interrupt through the sync path.

We also need to catch the case where the guest deactivates the interrupt
through the emulated distributor, again allowing guests to boot even if
the original virtual timer signal hit before the guest's GIC
initialization sequence is run.

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

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index ea21bc2..58b1256 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -531,6 +531,34 @@ bool vgic_handle_set_pending_reg(struct kvm *kvm,
 	return false;
 }
 
+/*
+ * If a mapped interrupt's state has been modified by the guest such that it
+ * is no longer active or pending, without it have gone through the sync path,
+ * then the map->active field must be cleared so the interrupt can be taken
+ * again.
+ */
+static void vgic_handle_clear_mapped_irq(struct kvm_vcpu *vcpu)
+{
+	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+	struct list_head *root;
+	struct irq_phys_map_entry *entry;
+	struct irq_phys_map *map;
+
+	rcu_read_lock();
+
+	/* Check for PPIs */
+	root = &vgic_cpu->irq_phys_map_list;
+	list_for_each_entry_rcu(entry, root, entry) {
+		map = &entry->map;
+
+		if (!vgic_dist_irq_is_pending(vcpu, map->virt_irq) &&
+		    !vgic_irq_is_active(vcpu, map->virt_irq))
+			map->active = false;
+	}
+
+	rcu_read_unlock();
+}
+
 bool vgic_handle_clear_pending_reg(struct kvm *kvm,
 				   struct kvm_exit_mmio *mmio,
 				   phys_addr_t offset, int vcpu_id)
@@ -561,6 +589,7 @@ bool vgic_handle_clear_pending_reg(struct kvm *kvm,
 					  vcpu_id, offset);
 		vgic_reg_access(mmio, reg, offset, mode);
 
+		vgic_handle_clear_mapped_irq(kvm_get_vcpu(kvm, vcpu_id));
 		vgic_update_state(kvm);
 		return true;
 	}
@@ -598,6 +627,7 @@ bool vgic_handle_clear_active_reg(struct kvm *kvm,
 			ACCESS_READ_VALUE | ACCESS_WRITE_CLEARBIT);
 
 	if (mmio->is_write) {
+		vgic_handle_clear_mapped_irq(kvm_get_vcpu(kvm, vcpu_id));
 		vgic_update_state(kvm);
 		return true;
 	}
@@ -1406,7 +1436,7 @@ static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr)
 		return 0;
 
 	map = vgic_irq_map_search(vcpu, vlr.irq);
-	BUG_ON(!map || !map->active);
+	BUG_ON(!map);
 
 	ret = irq_get_irqchip_state(map->irq,
 				    IRQCHIP_STATE_ACTIVE,
-- 
2.1.2.330.g565301e.dirty

[PATCH 3/3] arm/arm64: KVM: Fix disabled distributor operation

[Christoffer Dall]

We currently do a single update of the vgic state when the distrbutor
enable/disable control register is accessed and then bypass updating the
state for as long as the distributor remains disabled.

This is incorrect, because updating the state does not consider the
distributor enable bit, and this you can end up in a situation where an
interrupt is marked as pending on the CPU interface, but not pending on
the distributor, which is an impossible state to be in, and triggers a
warning.  Consider for example the following sequence of events:

1. An interrupt is marked as pending on the distributor
   - the interrupt is also forwarded to the CPU interface
2. The guest turns off the distributor (it's about to do a reboot)
   - we stop updating the CPU interface state from now on
3. The guest disables the pending interrupt
   - we remove the pending state from the distributor, but don't touch
     the CPU interface, see point 2.

Since the distributor disable bit really means that no interrupts should
be forwarded to the CPU interface, we modify the code to keep updating
the internal VGIC state, but always set the CPU interface pending bits
to zero when the distributor is disabled.

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

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 58b1256..66c6616 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1012,6 +1012,12 @@ static int compute_pending_for_cpu(struct kvm_vcpu *vcpu)
 	pend_percpu = vcpu->arch.vgic_cpu.pending_percpu;
 	pend_shared = vcpu->arch.vgic_cpu.pending_shared;
 
+	if (!dist->enabled) {
+		bitmap_zero(pend_percpu, VGIC_NR_PRIVATE_IRQS);
+		bitmap_zero(pend_shared, nr_shared);
+		return 0;
+	}
+
 	pending = vgic_bitmap_get_cpu_map(&dist->irq_pending, vcpu_id);
 	enabled = vgic_bitmap_get_cpu_map(&dist->irq_enabled, vcpu_id);
 	bitmap_and(pend_percpu, pending, enabled, VGIC_NR_PRIVATE_IRQS);
@@ -1039,11 +1045,6 @@ void vgic_update_state(struct kvm *kvm)
 	struct kvm_vcpu *vcpu;
 	int c;
 
-	if (!dist->enabled) {
-		set_bit(0, dist->irq_pending_on_cpu);
-		return;
-	}
-
 	kvm_for_each_vcpu(c, vcpu, kvm) {
 		if (compute_pending_for_cpu(vcpu))
 			set_bit(c, dist->irq_pending_on_cpu);
-- 
2.1.2.330.g565301e.dirty

[PATCH 1/3] arm/arm64: KVM: Fix arch timer behavior for disabled interrupts

[Christoffer Dall]

On Sat, Oct 17, 2015 at 10:30:20PM +0200, Christoffer Dall wrote:
> We have an interesting issue when the guest disables the timer interrupt
> on the VGIC, which happens when turning VCPUs off using PSCI, for
> example.
> 
> The problem is that because the guest disables the virtual interrupt at
> the VGIC level, we never inject interrupts to the guest and therefore
> never mark the interrupt as active on the physical distributor.  The
> host also never takes the timer interrupt (we only use the timer device
> to trigger a guest exit and everything else is done in software), so the
> interrupt does not become active through normal means.
> 
> The result is that we keep entering the guest with a programmed timer
> that will always fire as soon as we context switch the hardware timer
> state and run the guest, preventing forward progress for the VCPU.
> 
> Since the active state on the physical distributor is really part of the
> timer logic, it is the job of our virtual arch timer driver to manage
> this state.
> 
> The timer->map->active boolean field indicates whether we have signalled
> this interrupt to the vgic and if that interrupt is still pending or
> active.  As long as that is the case, the hardware doesn't have to
> generate physical interrupts and therefore we mark the interrupt as
> active on the physical distributor.
> 
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
Marc was worried about the performance implications of this fix on
Mustang given the potentially slow MMIO path to the GIC on that system,
so I ran some before and after applying this series:

BM	Hackbench	Kernbench	PbZip C		PbZip D
--	---------	---------	-------		-------
Before	17.94		51.66		17.69		10.59
After	18.14		51.62		17.82		10.62

The slight increase on hackbench is well within the variability (1.409
for the 8 runs behind these numbers) so I don't think this will be
noticable.  That said, there's room for optimizations here by only
touching the GIC on vcpu load/put and when the value changes, but I
think this is premature.

-Christoffer

[PATCH 1/3] arm/arm64: KVM: Fix arch timer behavior for disabled interrupts

[Eric Auger]

Hi Christoffer,
On 10/17/2015 10:30 PM, Christoffer Dall wrote:
> We have an interesting issue when the guest disables the timer interrupt
> on the VGIC, which happens when turning VCPUs off using PSCI, for
> example.
> 
> The problem is that because the guest disables the virtual interrupt at
> the VGIC level, we never inject interrupts to the guest and therefore
> never mark the interrupt as active on the physical distributor.  The
> host also never takes the timer interrupt (we only use the timer device
> to trigger a guest exit and everything else is done in software), so the
> interrupt does not become active through normal means.
> 
> The result is that we keep entering the guest with a programmed timer
> that will always fire as soon as we context switch the hardware timer
> state and run the guest, preventing forward progress for the VCPU.
> 
> Since the active state on the physical distributor is really part of the
> timer logic, it is the job of our virtual arch timer driver to manage
> this state.
> 
> The timer->map->active boolean field indicates whether we have signalled
> this interrupt to the vgic and if that interrupt is still pending or
> active.  As long as that is the case, the hardware doesn't have to
> generate physical interrupts and therefore we mark the interrupt as
> active on the physical distributor.
> 
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  virt/kvm/arm/arch_timer.c | 19 +++++++++++++++++++
>  virt/kvm/arm/vgic.c       | 43 +++++++++++--------------------------------
>  2 files changed, 30 insertions(+), 32 deletions(-)
> 
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 48c6e1a..b9d3a32 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -137,6 +137,8 @@ bool kvm_timer_should_fire(struct kvm_vcpu *vcpu)
>  void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
>  {
>  	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> +	bool phys_active;
> +	int ret;
>  
>  	/*
>  	 * We're about to run this vcpu again, so there is no need to
> @@ -151,6 +153,23 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
>  	 */
>  	if (kvm_timer_should_fire(vcpu))
>  		kvm_timer_inject_irq(vcpu);
> +
> +	/*
> +	 * We keep track of whether the edge-triggered interrupt has been
> +	 * signalled to the vgic/guest, and if so, we mask the interrupt and
> +	 * the physical distributor to prevent the timer from raising a
> +	 * physical interrupt whenever we run a guest, preventing forward
> +	 * VCPU progress.
In practice don't you simply mark the IRQ as active at GIC physical
distributor level, hence preventing the same IRQ from hitting again
> +	 */
> +	if (kvm_vgic_get_phys_irq_active(timer->map))
> +		phys_active = true;
> +	else
> +		phys_active = false;
> +
> +	ret = irq_set_irqchip_state(timer->map->irq,
> +				    IRQCHIP_STATE_ACTIVE,
> +				    phys_active);

physical distributor state is set in arch timer flush. It relates to a
shared device behavior so I find it natural to do it there.

However the map->active is set in arch_timer IRQ injection and unset in
vgic sync. Why not doing the set in kvm_vgic_inject_mapped_irq?

> +	WARN_ON(ret);
>  }
>  
>  /**
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 596455a..ea21bc2 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1092,6 +1092,15 @@ static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu)
>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>  	struct vgic_lr vlr = vgic_get_lr(vcpu, lr_nr);
>  
> +	/*
> +	 * We must transfer the pending state back to the distributor before
> +	 * retiring the LR, otherwise we may loose edge-triggered interrupts.
> +	 */
> +	if (vlr.state & LR_STATE_PENDING) {
> +		vgic_dist_irq_set_pending(vcpu, irq);
> +		vlr.hwirq = 0;
> +	}
That fix applies to any edge-sensitive IRQ, ie. not especially the
timer's one? In the positive shouldn't you precise this in the commit
msg too?

Best Regards

Eric

> +
>  	vlr.state = 0;
>  	vgic_set_lr(vcpu, lr_nr, vlr);
>  	clear_bit(lr_nr, vgic_cpu->lr_used);
> @@ -1241,7 +1250,7 @@ static void __kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>  	unsigned long *pa_percpu, *pa_shared;
> -	int i, vcpu_id, lr, ret;
> +	int i, vcpu_id;
>  	int overflow = 0;
>  	int nr_shared = vgic_nr_shared_irqs(dist);
>  
> @@ -1296,31 +1305,6 @@ epilog:
>  		 */
>  		clear_bit(vcpu_id, dist->irq_pending_on_cpu);
>  	}
> -
> -	for (lr = 0; lr < vgic->nr_lr; lr++) {
> -		struct vgic_lr vlr;
> -
> -		if (!test_bit(lr, vgic_cpu->lr_used))
> -			continue;
> -
> -		vlr = vgic_get_lr(vcpu, lr);
> -
> -		/*
> -		 * If we have a mapping, and the virtual interrupt is
> -		 * presented to the guest (as pending or active), then we must
> -		 * set the state to active in the physical world. See
> -		 * Documentation/virtual/kvm/arm/vgic-mapped-irqs.txt.
> -		 */
> -		if (vlr.state & LR_HW) {
> -			struct irq_phys_map *map;
> -			map = vgic_irq_map_search(vcpu, vlr.irq);
> -
> -			ret = irq_set_irqchip_state(map->irq,
> -						    IRQCHIP_STATE_ACTIVE,
> -						    true);
> -			WARN_ON(ret);
> -		}
> -	}
>  }
>  
>  static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
> @@ -1430,13 +1414,8 @@ static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr)
>  
>  	WARN_ON(ret);
>  
> -	if (map->active) {
> -		ret = irq_set_irqchip_state(map->irq,
> -					    IRQCHIP_STATE_ACTIVE,
> -					    false);
> -		WARN_ON(ret);
> +	if (map->active)
>  		return 0;
> -	}
>  
>  	return 1;
>  }
> 

[PATCH 1/3] arm/arm64: KVM: Fix arch timer behavior for disabled interrupts

[Christoffer Dall]

On Mon, Oct 19, 2015 at 03:07:16PM +0200, Eric Auger wrote:
> Hi Christoffer,
> On 10/17/2015 10:30 PM, Christoffer Dall wrote:
> > We have an interesting issue when the guest disables the timer interrupt
> > on the VGIC, which happens when turning VCPUs off using PSCI, for
> > example.
> > 
> > The problem is that because the guest disables the virtual interrupt at
> > the VGIC level, we never inject interrupts to the guest and therefore
> > never mark the interrupt as active on the physical distributor.  The
> > host also never takes the timer interrupt (we only use the timer device
> > to trigger a guest exit and everything else is done in software), so the
> > interrupt does not become active through normal means.
> > 
> > The result is that we keep entering the guest with a programmed timer
> > that will always fire as soon as we context switch the hardware timer
> > state and run the guest, preventing forward progress for the VCPU.
> > 
> > Since the active state on the physical distributor is really part of the
> > timer logic, it is the job of our virtual arch timer driver to manage
> > this state.
> > 
> > The timer->map->active boolean field indicates whether we have signalled
> > this interrupt to the vgic and if that interrupt is still pending or
> > active.  As long as that is the case, the hardware doesn't have to
> > generate physical interrupts and therefore we mark the interrupt as
> > active on the physical distributor.
> > 
> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  virt/kvm/arm/arch_timer.c | 19 +++++++++++++++++++
> >  virt/kvm/arm/vgic.c       | 43 +++++++++++--------------------------------
> >  2 files changed, 30 insertions(+), 32 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > index 48c6e1a..b9d3a32 100644
> > --- a/virt/kvm/arm/arch_timer.c
> > +++ b/virt/kvm/arm/arch_timer.c
> > @@ -137,6 +137,8 @@ bool kvm_timer_should_fire(struct kvm_vcpu *vcpu)
> >  void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
> >  {
> >  	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> > +	bool phys_active;
> > +	int ret;
> >  
> >  	/*
> >  	 * We're about to run this vcpu again, so there is no need to
> > @@ -151,6 +153,23 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
> >  	 */
> >  	if (kvm_timer_should_fire(vcpu))
> >  		kvm_timer_inject_irq(vcpu);
> > +
> > +	/*
> > +	 * We keep track of whether the edge-triggered interrupt has been
> > +	 * signalled to the vgic/guest, and if so, we mask the interrupt and
> > +	 * the physical distributor to prevent the timer from raising a
> > +	 * physical interrupt whenever we run a guest, preventing forward
> > +	 * VCPU progress.
> In practice don't you simply mark the IRQ as active at GIC physical
> distributor level, hence preventing the same IRQ from hitting again

yes, that's what I meant with my comment, I should reword to "...we mark
the interrupt as active on the physical distributor..."

> > +	 */
> > +	if (kvm_vgic_get_phys_irq_active(timer->map))
> > +		phys_active = true;
> > +	else
> > +		phys_active = false;
> > +
> > +	ret = irq_set_irqchip_state(timer->map->irq,
> > +				    IRQCHIP_STATE_ACTIVE,
> > +				    phys_active);
> 
> physical distributor state is set in arch timer flush. It relates to a
> shared device behavior so I find it natural to do it there.
> 
> However the map->active is set in arch_timer IRQ injection and unset in
> vgic sync. Why not doing the set in kvm_vgic_inject_mapped_irq?

Because you have to set it at every entry to the guest if you run
multiple VCPUs/VMs on this CPU or migrate this VCPU to a different CPU.

> 
> > +	WARN_ON(ret);
> >  }
> >  
> >  /**
> > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> > index 596455a..ea21bc2 100644
> > --- a/virt/kvm/arm/vgic.c
> > +++ b/virt/kvm/arm/vgic.c
> > @@ -1092,6 +1092,15 @@ static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu)
> >  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> >  	struct vgic_lr vlr = vgic_get_lr(vcpu, lr_nr);
> >  
> > +	/*
> > +	 * We must transfer the pending state back to the distributor before
> > +	 * retiring the LR, otherwise we may loose edge-triggered interrupts.
> > +	 */
> > +	if (vlr.state & LR_STATE_PENDING) {
> > +		vgic_dist_irq_set_pending(vcpu, irq);
> > +		vlr.hwirq = 0;
> > +	}
> That fix applies to any edge-sensitive IRQ, ie. not especially the
> timer's one? In the positive shouldn't you precise this in the commit
> msg too?
> 

Probably, it could also be a separate patch.  I'll rework this.

Thanks,
-Christoffer

[PATCH 1/3] arm/arm64: KVM: Fix arch timer behavior for disabled interrupts

[Eric Auger]

On 10/19/2015 03:14 PM, Christoffer Dall wrote:
> On Mon, Oct 19, 2015 at 03:07:16PM +0200, Eric Auger wrote:
>> Hi Christoffer,
>> On 10/17/2015 10:30 PM, Christoffer Dall wrote:
>>> We have an interesting issue when the guest disables the timer interrupt
>>> on the VGIC, which happens when turning VCPUs off using PSCI, for
>>> example.
>>>
>>> The problem is that because the guest disables the virtual interrupt at
>>> the VGIC level, we never inject interrupts to the guest and therefore
>>> never mark the interrupt as active on the physical distributor.  The
>>> host also never takes the timer interrupt (we only use the timer device
>>> to trigger a guest exit and everything else is done in software), so the
>>> interrupt does not become active through normal means.
>>>
>>> The result is that we keep entering the guest with a programmed timer
>>> that will always fire as soon as we context switch the hardware timer
>>> state and run the guest, preventing forward progress for the VCPU.
>>>
>>> Since the active state on the physical distributor is really part of the
>>> timer logic, it is the job of our virtual arch timer driver to manage
>>> this state.
>>>
>>> The timer->map->active boolean field indicates whether we have signalled
>>> this interrupt to the vgic and if that interrupt is still pending or
>>> active.  As long as that is the case, the hardware doesn't have to
>>> generate physical interrupts and therefore we mark the interrupt as
>>> active on the physical distributor.
>>>
>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>> Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>>> ---
>>>  virt/kvm/arm/arch_timer.c | 19 +++++++++++++++++++
>>>  virt/kvm/arm/vgic.c       | 43 +++++++++++--------------------------------
>>>  2 files changed, 30 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>>> index 48c6e1a..b9d3a32 100644
>>> --- a/virt/kvm/arm/arch_timer.c
>>> +++ b/virt/kvm/arm/arch_timer.c
>>> @@ -137,6 +137,8 @@ bool kvm_timer_should_fire(struct kvm_vcpu *vcpu)
>>>  void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
>>>  {
>>>  	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>>> +	bool phys_active;
>>> +	int ret;
>>>  
>>>  	/*
>>>  	 * We're about to run this vcpu again, so there is no need to
>>> @@ -151,6 +153,23 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
>>>  	 */
>>>  	if (kvm_timer_should_fire(vcpu))
>>>  		kvm_timer_inject_irq(vcpu);
>>> +
>>> +	/*
>>> +	 * We keep track of whether the edge-triggered interrupt has been
>>> +	 * signalled to the vgic/guest, and if so, we mask the interrupt and
>>> +	 * the physical distributor to prevent the timer from raising a
>>> +	 * physical interrupt whenever we run a guest, preventing forward
>>> +	 * VCPU progress.
>> In practice don't you simply mark the IRQ as active at GIC physical
>> distributor level, hence preventing the same IRQ from hitting again
> 
> yes, that's what I meant with my comment, I should reword to "...we mark
> the interrupt as active on the physical distributor..."
> 
>>> +	 */
>>> +	if (kvm_vgic_get_phys_irq_active(timer->map))
>>> +		phys_active = true;
>>> +	else
>>> +		phys_active = false;
>>> +
>>> +	ret = irq_set_irqchip_state(timer->map->irq,
>>> +				    IRQCHIP_STATE_ACTIVE,
>>> +				    phys_active);
>>
>> physical distributor state is set in arch timer flush. It relates to a
>> shared device behavior so I find it natural to do it there.
>>
>> However the map->active is set in arch_timer IRQ injection and unset in
>> vgic sync. Why not doing the set in kvm_vgic_inject_mapped_irq?
> 
> Because you have to set it at every entry to the guest if you run
> multiple VCPUs/VMs on this CPU or migrate this VCPU to a different CPU.
I meant 	kvm_vgic_set_phys_irq_active(timer->map, true) call in
kvm_timer_inject_irq? Couldn' that been done in
kvm_vgic_inject_mapped_irq instead. Doesn't this apply to all mapped IRQs?

Eric
> 
>>
>>> +	WARN_ON(ret);
>>>  }
>>>  
>>>  /**
>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>> index 596455a..ea21bc2 100644
>>> --- a/virt/kvm/arm/vgic.c
>>> +++ b/virt/kvm/arm/vgic.c
>>> @@ -1092,6 +1092,15 @@ static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu)
>>>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>>  	struct vgic_lr vlr = vgic_get_lr(vcpu, lr_nr);
>>>  
>>> +	/*
>>> +	 * We must transfer the pending state back to the distributor before
>>> +	 * retiring the LR, otherwise we may loose edge-triggered interrupts.
>>> +	 */
>>> +	if (vlr.state & LR_STATE_PENDING) {
>>> +		vgic_dist_irq_set_pending(vcpu, irq);
>>> +		vlr.hwirq = 0;
>>> +	}
>> That fix applies to any edge-sensitive IRQ, ie. not especially the
>> timer's one? In the positive shouldn't you precise this in the commit
>> msg too?
>>
> 
> Probably, it could also be a separate patch.  I'll rework this.
> 
> Thanks,
> -Christoffer
> 

[PATCH 1/3] arm/arm64: KVM: Fix arch timer behavior for disabled interrupts

[Christoffer Dall]

On Mon, Oct 19, 2015 at 03:27:42PM +0200, Eric Auger wrote:
> On 10/19/2015 03:14 PM, Christoffer Dall wrote:
> > On Mon, Oct 19, 2015 at 03:07:16PM +0200, Eric Auger wrote:
> >> Hi Christoffer,
> >> On 10/17/2015 10:30 PM, Christoffer Dall wrote:
> >>> We have an interesting issue when the guest disables the timer interrupt
> >>> on the VGIC, which happens when turning VCPUs off using PSCI, for
> >>> example.
> >>>
> >>> The problem is that because the guest disables the virtual interrupt at
> >>> the VGIC level, we never inject interrupts to the guest and therefore
> >>> never mark the interrupt as active on the physical distributor.  The
> >>> host also never takes the timer interrupt (we only use the timer device
> >>> to trigger a guest exit and everything else is done in software), so the
> >>> interrupt does not become active through normal means.
> >>>
> >>> The result is that we keep entering the guest with a programmed timer
> >>> that will always fire as soon as we context switch the hardware timer
> >>> state and run the guest, preventing forward progress for the VCPU.
> >>>
> >>> Since the active state on the physical distributor is really part of the
> >>> timer logic, it is the job of our virtual arch timer driver to manage
> >>> this state.
> >>>
> >>> The timer->map->active boolean field indicates whether we have signalled
> >>> this interrupt to the vgic and if that interrupt is still pending or
> >>> active.  As long as that is the case, the hardware doesn't have to
> >>> generate physical interrupts and therefore we mark the interrupt as
> >>> active on the physical distributor.
> >>>
> >>> Cc: Marc Zyngier <marc.zyngier@arm.com>
> >>> Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> >>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> >>> ---
> >>>  virt/kvm/arm/arch_timer.c | 19 +++++++++++++++++++
> >>>  virt/kvm/arm/vgic.c       | 43 +++++++++++--------------------------------
> >>>  2 files changed, 30 insertions(+), 32 deletions(-)
> >>>
> >>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> >>> index 48c6e1a..b9d3a32 100644
> >>> --- a/virt/kvm/arm/arch_timer.c
> >>> +++ b/virt/kvm/arm/arch_timer.c
> >>> @@ -137,6 +137,8 @@ bool kvm_timer_should_fire(struct kvm_vcpu *vcpu)
> >>>  void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
> >>>  {
> >>>  	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> >>> +	bool phys_active;
> >>> +	int ret;
> >>>  
> >>>  	/*
> >>>  	 * We're about to run this vcpu again, so there is no need to
> >>> @@ -151,6 +153,23 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
> >>>  	 */
> >>>  	if (kvm_timer_should_fire(vcpu))
> >>>  		kvm_timer_inject_irq(vcpu);
> >>> +
> >>> +	/*
> >>> +	 * We keep track of whether the edge-triggered interrupt has been
> >>> +	 * signalled to the vgic/guest, and if so, we mask the interrupt and
> >>> +	 * the physical distributor to prevent the timer from raising a
> >>> +	 * physical interrupt whenever we run a guest, preventing forward
> >>> +	 * VCPU progress.
> >> In practice don't you simply mark the IRQ as active at GIC physical
> >> distributor level, hence preventing the same IRQ from hitting again
> > 
> > yes, that's what I meant with my comment, I should reword to "...we mark
> > the interrupt as active on the physical distributor..."
> > 
> >>> +	 */
> >>> +	if (kvm_vgic_get_phys_irq_active(timer->map))
> >>> +		phys_active = true;
> >>> +	else
> >>> +		phys_active = false;
> >>> +
> >>> +	ret = irq_set_irqchip_state(timer->map->irq,
> >>> +				    IRQCHIP_STATE_ACTIVE,
> >>> +				    phys_active);
> >>
> >> physical distributor state is set in arch timer flush. It relates to a
> >> shared device behavior so I find it natural to do it there.
> >>
> >> However the map->active is set in arch_timer IRQ injection and unset in
> >> vgic sync. Why not doing the set in kvm_vgic_inject_mapped_irq?
> > 
> > Because you have to set it at every entry to the guest if you run
> > multiple VCPUs/VMs on this CPU or migrate this VCPU to a different CPU.
> I meant 	kvm_vgic_set_phys_irq_active(timer->map, true) call in
> kvm_timer_inject_irq? Couldn' that been done in
> kvm_vgic_inject_mapped_irq instead. Doesn't this apply to all mapped IRQs?
> 
I could, but that would be outside the scope of this patch, which aims
to fix the behavior in 4.3 simply.  Unless I'm missing some sort of
change in functionality there?

The map->active stuff all goes away in 4.4 when we change to
level-triggered semantics anyway.

-Christoffer

[PATCH 2/3] arm/arm64: KVM: Clear map->active on pend/active clear

[Eric Auger]

Hi,
On 10/17/2015 10:30 PM, Christoffer Dall wrote:
> When a guest reboots or offlines/onlines CPUs, it is not uncommon for it
> to clear the pending and active states of an interrupt through the
> emulated VGIC distributor.  However, since we emulate an edge-triggered
> based on a level-triggered device,
I do not get this sentence.

Besides that
Reviewed-by: Eric Auger <eric.auger@linaro.org>

Best Regards

Eric
 the guest expects the timer interrupt
> to hit even after clearing the pending state.
> 
> We currently do not signal the VGIC when the map->active field is true,
> because it indicates that the guest has already been signalled of the
> interrupt as required.  Normally this field is set to false when the
> guest deactivates the virtual interrupt through the sync path.
> 
> We also need to catch the case where the guest deactivates the interrupt
> through the emulated distributor, again allowing guests to boot even if
> the original virtual timer signal hit before the guest's GIC
> initialization sequence is run.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  virt/kvm/arm/vgic.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index ea21bc2..58b1256 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -531,6 +531,34 @@ bool vgic_handle_set_pending_reg(struct kvm *kvm,
>  	return false;
>  }
>  
> +/*
> + * If a mapped interrupt's state has been modified by the guest such that it
> + * is no longer active or pending, without it have gone through the sync path,
> + * then the map->active field must be cleared so the interrupt can be taken
> + * again.
> + */
> +static void vgic_handle_clear_mapped_irq(struct kvm_vcpu *vcpu)
> +{
> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> +	struct list_head *root;
> +	struct irq_phys_map_entry *entry;
> +	struct irq_phys_map *map;
> +
> +	rcu_read_lock();
> +
> +	/* Check for PPIs */
> +	root = &vgic_cpu->irq_phys_map_list;
> +	list_for_each_entry_rcu(entry, root, entry) {
> +		map = &entry->map;
> +
> +		if (!vgic_dist_irq_is_pending(vcpu, map->virt_irq) &&
> +		    !vgic_irq_is_active(vcpu, map->virt_irq))
> +			map->active = false;
> +	}
> +
> +	rcu_read_unlock();
> +}
> +
>  bool vgic_handle_clear_pending_reg(struct kvm *kvm,
>  				   struct kvm_exit_mmio *mmio,
>  				   phys_addr_t offset, int vcpu_id)
> @@ -561,6 +589,7 @@ bool vgic_handle_clear_pending_reg(struct kvm *kvm,
>  					  vcpu_id, offset);
>  		vgic_reg_access(mmio, reg, offset, mode);
>  
> +		vgic_handle_clear_mapped_irq(kvm_get_vcpu(kvm, vcpu_id));
>  		vgic_update_state(kvm);
>  		return true;
>  	}
> @@ -598,6 +627,7 @@ bool vgic_handle_clear_active_reg(struct kvm *kvm,
>  			ACCESS_READ_VALUE | ACCESS_WRITE_CLEARBIT);
>  
>  	if (mmio->is_write) {
> +		vgic_handle_clear_mapped_irq(kvm_get_vcpu(kvm, vcpu_id));
>  		vgic_update_state(kvm);
>  		return true;
>  	}
> @@ -1406,7 +1436,7 @@ static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr)
>  		return 0;
>  
>  	map = vgic_irq_map_search(vcpu, vlr.irq);
> -	BUG_ON(!map || !map->active);
> +	BUG_ON(!map);
>  
>  	ret = irq_get_irqchip_state(map->irq,
>  				    IRQCHIP_STATE_ACTIVE,
> 

[PATCH 2/3] arm/arm64: KVM: Clear map->active on pend/active clear

[Christoffer Dall]

On Mon, Oct 19, 2015 at 05:32:42PM +0200, Eric Auger wrote:
> Hi,
> On 10/17/2015 10:30 PM, Christoffer Dall wrote:
> > When a guest reboots or offlines/onlines CPUs, it is not uncommon for it
> > to clear the pending and active states of an interrupt through the
> > emulated VGIC distributor.  However, since we emulate an edge-triggered
> > based on a level-triggered device,
> I do not get this sentence.

Fair enough.  The guest expects a level-triggered timer, so it expects
to be able to do:

  disable interrupt, set timer(*), clear pending state, enable interrupt

and this only works because the device is level-triggered, so it should
hit again after clearing the pending state and the guest should see it
after enabling interrupts.

but because we have virtual edge-triggered behavior, we loose the
interrupt generated at (*) unless we inject it again, which we will
after this patch, because the actual device is level triggered, so we'll
sample its state in the KVM timer code contiuously and now  (after this
patch) notice that we have to inject the edge-triggered IRQ again.

Yes, I know, we're lucky we can make it work like this.

> 
> Besides that
> Reviewed-by: Eric Auger <eric.auger@linaro.org>

Thanks!

-Christoffer

>  the guest expects the timer interrupt
> > to hit even after clearing the pending state.
> > 
> > We currently do not signal the VGIC when the map->active field is true,
> > because it indicates that the guest has already been signalled of the
> > interrupt as required.  Normally this field is set to false when the
> > guest deactivates the virtual interrupt through the sync path.
> > 
> > We also need to catch the case where the guest deactivates the interrupt
> > through the emulated distributor, again allowing guests to boot even if
> > the original virtual timer signal hit before the guest's GIC
> > initialization sequence is run.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  virt/kvm/arm/vgic.c | 32 +++++++++++++++++++++++++++++++-
> >  1 file changed, 31 insertions(+), 1 deletion(-)
> > 
> > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> > index ea21bc2..58b1256 100644
> > --- a/virt/kvm/arm/vgic.c
> > +++ b/virt/kvm/arm/vgic.c
> > @@ -531,6 +531,34 @@ bool vgic_handle_set_pending_reg(struct kvm *kvm,
> >  	return false;
> >  }
> >  
> > +/*
> > + * If a mapped interrupt's state has been modified by the guest such that it
> > + * is no longer active or pending, without it have gone through the sync path,
> > + * then the map->active field must be cleared so the interrupt can be taken
> > + * again.
> > + */
> > +static void vgic_handle_clear_mapped_irq(struct kvm_vcpu *vcpu)
> > +{
> > +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> > +	struct list_head *root;
> > +	struct irq_phys_map_entry *entry;
> > +	struct irq_phys_map *map;
> > +
> > +	rcu_read_lock();
> > +
> > +	/* Check for PPIs */
> > +	root = &vgic_cpu->irq_phys_map_list;
> > +	list_for_each_entry_rcu(entry, root, entry) {
> > +		map = &entry->map;
> > +
> > +		if (!vgic_dist_irq_is_pending(vcpu, map->virt_irq) &&
> > +		    !vgic_irq_is_active(vcpu, map->virt_irq))
> > +			map->active = false;
> > +	}
> > +
> > +	rcu_read_unlock();
> > +}
> > +
> >  bool vgic_handle_clear_pending_reg(struct kvm *kvm,
> >  				   struct kvm_exit_mmio *mmio,
> >  				   phys_addr_t offset, int vcpu_id)
> > @@ -561,6 +589,7 @@ bool vgic_handle_clear_pending_reg(struct kvm *kvm,
> >  					  vcpu_id, offset);
> >  		vgic_reg_access(mmio, reg, offset, mode);
> >  
> > +		vgic_handle_clear_mapped_irq(kvm_get_vcpu(kvm, vcpu_id));
> >  		vgic_update_state(kvm);
> >  		return true;
> >  	}
> > @@ -598,6 +627,7 @@ bool vgic_handle_clear_active_reg(struct kvm *kvm,
> >  			ACCESS_READ_VALUE | ACCESS_WRITE_CLEARBIT);
> >  
> >  	if (mmio->is_write) {
> > +		vgic_handle_clear_mapped_irq(kvm_get_vcpu(kvm, vcpu_id));
> >  		vgic_update_state(kvm);
> >  		return true;
> >  	}
> > @@ -1406,7 +1436,7 @@ static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr)
> >  		return 0;
> >  
> >  	map = vgic_irq_map_search(vcpu, vlr.irq);
> > -	BUG_ON(!map || !map->active);
> > +	BUG_ON(!map);
> >  
> >  	ret = irq_get_irqchip_state(map->irq,
> >  				    IRQCHIP_STATE_ACTIVE,
> > 
> 

[PATCH 2/3] arm/arm64: KVM: Clear map->active on pend/active clear

[Eric Auger]

On 10/19/2015 05:39 PM, Christoffer Dall wrote:
> On Mon, Oct 19, 2015 at 05:32:42PM +0200, Eric Auger wrote:
>> Hi,
>> On 10/17/2015 10:30 PM, Christoffer Dall wrote:
>>> When a guest reboots or offlines/onlines CPUs, it is not uncommon for it
>>> to clear the pending and active states of an interrupt through the
>>> emulated VGIC distributor.  However, since we emulate an edge-triggered
>>> based on a level-triggered device,
>> I do not get this sentence.
> 
> Fair enough.  The guest expects a level-triggered timer, so it expects
> to be able to do:
> 
>   disable interrupt, set timer(*), clear pending state, enable interrupt
> 
> and this only works because the device is level-triggered, so it should
> hit again after clearing the pending state and the guest should see it
> after enabling interrupts.
> 
> but because we have virtual edge-triggered behavior, we loose the
> interrupt generated at (*) unless we inject it again, which we will
> after this patch, because the actual device is level triggered, so we'll
> sample its state in the KVM timer code contiuously and now  (after this
> patch) notice that we have to inject the edge-triggered IRQ again.

That helps ;-)

Eric
> 
> Yes, I know, we're lucky we can make it work like this.
> 
>>
>> Besides that
>> Reviewed-by: Eric Auger <eric.auger@linaro.org>
> 
> Thanks!
> 
> -Christoffer
> 
>>  the guest expects the timer interrupt
>>> to hit even after clearing the pending state.
>>>
>>> We currently do not signal the VGIC when the map->active field is true,
>>> because it indicates that the guest has already been signalled of the
>>> interrupt as required.  Normally this field is set to false when the
>>> guest deactivates the virtual interrupt through the sync path.
>>>
>>> We also need to catch the case where the guest deactivates the interrupt
>>> through the emulated distributor, again allowing guests to boot even if
>>> the original virtual timer signal hit before the guest's GIC
>>> initialization sequence is run.
>>>
>>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>>> ---
>>>  virt/kvm/arm/vgic.c | 32 +++++++++++++++++++++++++++++++-
>>>  1 file changed, 31 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>> index ea21bc2..58b1256 100644
>>> --- a/virt/kvm/arm/vgic.c
>>> +++ b/virt/kvm/arm/vgic.c
>>> @@ -531,6 +531,34 @@ bool vgic_handle_set_pending_reg(struct kvm *kvm,
>>>  	return false;
>>>  }
>>>  
>>> +/*
>>> + * If a mapped interrupt's state has been modified by the guest such that it
>>> + * is no longer active or pending, without it have gone through the sync path,
>>> + * then the map->active field must be cleared so the interrupt can be taken
>>> + * again.
>>> + */
>>> +static void vgic_handle_clear_mapped_irq(struct kvm_vcpu *vcpu)
>>> +{
>>> +	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>> +	struct list_head *root;
>>> +	struct irq_phys_map_entry *entry;
>>> +	struct irq_phys_map *map;
>>> +
>>> +	rcu_read_lock();
>>> +
>>> +	/* Check for PPIs */
>>> +	root = &vgic_cpu->irq_phys_map_list;
>>> +	list_for_each_entry_rcu(entry, root, entry) {
>>> +		map = &entry->map;
>>> +
>>> +		if (!vgic_dist_irq_is_pending(vcpu, map->virt_irq) &&
>>> +		    !vgic_irq_is_active(vcpu, map->virt_irq))
>>> +			map->active = false;
>>> +	}
>>> +
>>> +	rcu_read_unlock();
>>> +}
>>> +
>>>  bool vgic_handle_clear_pending_reg(struct kvm *kvm,
>>>  				   struct kvm_exit_mmio *mmio,
>>>  				   phys_addr_t offset, int vcpu_id)
>>> @@ -561,6 +589,7 @@ bool vgic_handle_clear_pending_reg(struct kvm *kvm,
>>>  					  vcpu_id, offset);
>>>  		vgic_reg_access(mmio, reg, offset, mode);
>>>  
>>> +		vgic_handle_clear_mapped_irq(kvm_get_vcpu(kvm, vcpu_id));
>>>  		vgic_update_state(kvm);
>>>  		return true;
>>>  	}
>>> @@ -598,6 +627,7 @@ bool vgic_handle_clear_active_reg(struct kvm *kvm,
>>>  			ACCESS_READ_VALUE | ACCESS_WRITE_CLEARBIT);
>>>  
>>>  	if (mmio->is_write) {
>>> +		vgic_handle_clear_mapped_irq(kvm_get_vcpu(kvm, vcpu_id));
>>>  		vgic_update_state(kvm);
>>>  		return true;
>>>  	}
>>> @@ -1406,7 +1436,7 @@ static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr)
>>>  		return 0;
>>>  
>>>  	map = vgic_irq_map_search(vcpu, vlr.irq);
>>> -	BUG_ON(!map || !map->active);
>>> +	BUG_ON(!map);
>>>  
>>>  	ret = irq_get_irqchip_state(map->irq,
>>>  				    IRQCHIP_STATE_ACTIVE,
>>>
>>

[PATCH 3/3] arm/arm64: KVM: Fix disabled distributor operation

[Eric Auger]

Hi Christoffer,
On 10/17/2015 10:30 PM, Christoffer Dall wrote:
> We currently do a single update of the vgic state when the distrbutor
distributor
> enable/disable control register is accessed and then bypass updating the
> state for as long as the distributor remains disabled.
> 
> This is incorrect, because updating the state does not consider the
> distributor enable bit, and this you can end up in a situation where an
> interrupt is marked as pending on the CPU interface, but not pending on
> the distributor, which is an impossible state to be in, and triggers a
> warning.  Consider for example the following sequence of events:
> 
> 1. An interrupt is marked as pending on the distributor
>    - the interrupt is also forwarded to the CPU interface
> 2. The guest turns off the distributor (it's about to do a reboot)
>    - we stop updating the CPU interface state from now on
> 3. The guest disables the pending interrupt
>    - we remove the pending state from the distributor, but don't touch
>      the CPU interface, see point 2.
> 
> Since the distributor disable bit really means that no interrupts should
> be forwarded to the CPU interface, we modify the code to keep updating
> the internal VGIC state, but always set the CPU interface pending bits
> to zero when the distributor is disabled.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  virt/kvm/arm/vgic.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 58b1256..66c6616 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1012,6 +1012,12 @@ static int compute_pending_for_cpu(struct kvm_vcpu *vcpu)
>  	pend_percpu = vcpu->arch.vgic_cpu.pending_percpu;
>  	pend_shared = vcpu->arch.vgic_cpu.pending_shared;
>  
> +	if (!dist->enabled) {
> +		bitmap_zero(pend_percpu, VGIC_NR_PRIVATE_IRQS);
> +		bitmap_zero(pend_shared, nr_shared);
> +		return 0;
> +	}
> +
>  	pending = vgic_bitmap_get_cpu_map(&dist->irq_pending, vcpu_id);
>  	enabled = vgic_bitmap_get_cpu_map(&dist->irq_enabled, vcpu_id);
>  	bitmap_and(pend_percpu, pending, enabled, VGIC_NR_PRIVATE_IRQS);
> @@ -1039,11 +1045,6 @@ void vgic_update_state(struct kvm *kvm)
>  	struct kvm_vcpu *vcpu;
>  	int c;
>  
> -	if (!dist->enabled) {
> -		set_bit(0, dist->irq_pending_on_cpu);
> -		return;
I am confused. Don't you want to clear the whole bitmap?

Shouldn't we also handle interrupts programmed in the LR. Spec says any
ack should return a spurious ID. Is it what is going to happen with the
current implementation?

Eric
> -	}
> -
>  	kvm_for_each_vcpu(c, vcpu, kvm) {
>  		if (compute_pending_for_cpu(vcpu))
>  			set_bit(c, dist->irq_pending_on_cpu);
> 

[PATCH 3/3] arm/arm64: KVM: Fix disabled distributor operation

[Christoffer Dall]

On Tue, Oct 20, 2015 at 11:08:44AM +0200, Eric Auger wrote:
> Hi Christoffer,
> On 10/17/2015 10:30 PM, Christoffer Dall wrote:
> > We currently do a single update of the vgic state when the distrbutor
> distributor
> > enable/disable control register is accessed and then bypass updating the
> > state for as long as the distributor remains disabled.
> > 
> > This is incorrect, because updating the state does not consider the
> > distributor enable bit, and this you can end up in a situation where an
> > interrupt is marked as pending on the CPU interface, but not pending on
> > the distributor, which is an impossible state to be in, and triggers a
> > warning.  Consider for example the following sequence of events:
> > 
> > 1. An interrupt is marked as pending on the distributor
> >    - the interrupt is also forwarded to the CPU interface
> > 2. The guest turns off the distributor (it's about to do a reboot)
> >    - we stop updating the CPU interface state from now on
> > 3. The guest disables the pending interrupt
> >    - we remove the pending state from the distributor, but don't touch
> >      the CPU interface, see point 2.
> > 
> > Since the distributor disable bit really means that no interrupts should
> > be forwarded to the CPU interface, we modify the code to keep updating
> > the internal VGIC state, but always set the CPU interface pending bits
> > to zero when the distributor is disabled.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  virt/kvm/arm/vgic.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> > index 58b1256..66c6616 100644
> > --- a/virt/kvm/arm/vgic.c
> > +++ b/virt/kvm/arm/vgic.c
> > @@ -1012,6 +1012,12 @@ static int compute_pending_for_cpu(struct kvm_vcpu *vcpu)
> >  	pend_percpu = vcpu->arch.vgic_cpu.pending_percpu;
> >  	pend_shared = vcpu->arch.vgic_cpu.pending_shared;
> >  
> > +	if (!dist->enabled) {
> > +		bitmap_zero(pend_percpu, VGIC_NR_PRIVATE_IRQS);
> > +		bitmap_zero(pend_shared, nr_shared);
> > +		return 0;
> > +	}
> > +
> >  	pending = vgic_bitmap_get_cpu_map(&dist->irq_pending, vcpu_id);
> >  	enabled = vgic_bitmap_get_cpu_map(&dist->irq_enabled, vcpu_id);
> >  	bitmap_and(pend_percpu, pending, enabled, VGIC_NR_PRIVATE_IRQS);
> > @@ -1039,11 +1045,6 @@ void vgic_update_state(struct kvm *kvm)
> >  	struct kvm_vcpu *vcpu;
> >  	int c;
> >  
> > -	if (!dist->enabled) {
> > -		set_bit(0, dist->irq_pending_on_cpu);
> > -		return;
> I am confused. Don't you want to clear the whole bitmap?

So the first line used to set irq_pending_on_cpu for VCPU0 when the
distributor was disabled, which I think basically worked around the
guest kernel expecting to see a timer interrupt during boot when doing a
WFI.  Now when that's fixed, we don't need this (gigantuous hack) anymore.

The return statement was also weird and buggy, because it never
prevented anything from going to the CPU interface, it just stopped
updating things.


> 
> Shouldn't we also handle interrupts programmed in the LR. Spec says any
> ack should return a spurious ID. Is it what is going to happen with the
> current implementation?
> 

yes, we really should.  We should unqueue them, but I haven't seen any
bugs from this, and I feel like we're already changing a lot with short
notice, so I'd rather not distrupt anything more right now.

Besides, when we get around to redesigning this whole thing, the concept
of unqueueing goes away.

I know it sucks reviewing fixes that only fix a subset of a bad
implementation, but I'm aiming for 'slightly better than current state'
right now :)

-Christoffer


> > -	}
> > -
> >  	kvm_for_each_vcpu(c, vcpu, kvm) {
> >  		if (compute_pending_for_cpu(vcpu))
> >  			set_bit(c, dist->irq_pending_on_cpu);
> > 
> 

[PATCH 3/3] arm/arm64: KVM: Fix disabled distributor operation

[Eric Auger]

On 10/20/2015 11:44 AM, Christoffer Dall wrote:
> On Tue, Oct 20, 2015 at 11:08:44AM +0200, Eric Auger wrote:
>> Hi Christoffer,
>> On 10/17/2015 10:30 PM, Christoffer Dall wrote:
>>> We currently do a single update of the vgic state when the distrbutor
>> distributor
>>> enable/disable control register is accessed and then bypass updating the
>>> state for as long as the distributor remains disabled.
>>>
>>> This is incorrect, because updating the state does not consider the
>>> distributor enable bit, and this you can end up in a situation where an
>>> interrupt is marked as pending on the CPU interface, but not pending on
>>> the distributor, which is an impossible state to be in, and triggers a
>>> warning.  Consider for example the following sequence of events:
>>>
>>> 1. An interrupt is marked as pending on the distributor
>>>    - the interrupt is also forwarded to the CPU interface
>>> 2. The guest turns off the distributor (it's about to do a reboot)
>>>    - we stop updating the CPU interface state from now on
>>> 3. The guest disables the pending interrupt
>>>    - we remove the pending state from the distributor, but don't touch
>>>      the CPU interface, see point 2.
>>>
>>> Since the distributor disable bit really means that no interrupts should
>>> be forwarded to the CPU interface, we modify the code to keep updating
>>> the internal VGIC state, but always set the CPU interface pending bits
>>> to zero when the distributor is disabled.
>>>
>>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>>> ---
>>>  virt/kvm/arm/vgic.c | 11 ++++++-----
>>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>> index 58b1256..66c6616 100644
>>> --- a/virt/kvm/arm/vgic.c
>>> +++ b/virt/kvm/arm/vgic.c
>>> @@ -1012,6 +1012,12 @@ static int compute_pending_for_cpu(struct kvm_vcpu *vcpu)
>>>  	pend_percpu = vcpu->arch.vgic_cpu.pending_percpu;
>>>  	pend_shared = vcpu->arch.vgic_cpu.pending_shared;
>>>  
>>> +	if (!dist->enabled) {
>>> +		bitmap_zero(pend_percpu, VGIC_NR_PRIVATE_IRQS);
>>> +		bitmap_zero(pend_shared, nr_shared);
>>> +		return 0;
>>> +	}
>>> +
>>>  	pending = vgic_bitmap_get_cpu_map(&dist->irq_pending, vcpu_id);
>>>  	enabled = vgic_bitmap_get_cpu_map(&dist->irq_enabled, vcpu_id);
>>>  	bitmap_and(pend_percpu, pending, enabled, VGIC_NR_PRIVATE_IRQS);
>>> @@ -1039,11 +1045,6 @@ void vgic_update_state(struct kvm *kvm)
>>>  	struct kvm_vcpu *vcpu;
>>>  	int c;
>>>  
>>> -	if (!dist->enabled) {
>>> -		set_bit(0, dist->irq_pending_on_cpu);
>>> -		return;
>> I am confused. Don't you want to clear the whole bitmap?
> 
> So the first line used to set irq_pending_on_cpu for VCPU0 when the
> distributor was disabled, which I think basically worked around the
> guest kernel expecting to see a timer interrupt during boot when doing a
> WFI.  Now when that's fixed, we don't need this (gigantuous hack) anymore.
> 
> The return statement was also weird and buggy, because it never
> prevented anything from going to the CPU interface, it just stopped
> updating things.

Yeah sorry I read it as an addition so I didn't understand that code but
I definitively understand you remove it. Sorry - tired.

Best Regards

Eric
> 
> 
>>
>> Shouldn't we also handle interrupts programmed in the LR. Spec says any
>> ack should return a spurious ID. Is it what is going to happen with the
>> current implementation?
>>
> 
> yes, we really should.  We should unqueue them, but I haven't seen any
> bugs from this, and I feel like we're already changing a lot with short
> notice, so I'd rather not distrupt anything more right now.
> 
> Besides, when we get around to redesigning this whole thing, the concept
> of unqueueing goes away.
> 
> I know it sucks reviewing fixes that only fix a subset of a bad
> implementation, but I'm aiming for 'slightly better than current state'
> right now :)
> 
> -Christoffer
> 
> 
>>> -	}
>>> -
>>>  	kvm_for_each_vcpu(c, vcpu, kvm) {
>>>  		if (compute_pending_for_cpu(vcpu))
>>>  			set_bit(c, dist->irq_pending_on_cpu);
>>>
>>