[RFC PATCH 0/6] arm/arm64: KVM: Various VGIC cleanups and improvements

[RFC PATCH 0/6] arm/arm64: KVM: Various VGIC cleanups and improvements

[Christoffer Dall]

The VGIC distributor struct has a number of fields which are named
subopimally when needing to add more refined handling of level-triggered
interrupts.

We also never handled writes to the GICD_ISPENDRn and GICD_ICPENDRn
properly, which was never a real concern until we started playing with
IRQ injection for assigned devices, IRQFDs, and other fun stuff.

This RFC series tries to address some of these issues.

*** WARNING ***
This series is untested!!

I am only sending it out now, untested, as is, giving people
ever-lasting right to ridicule me, because Eric Auger is blocked on this
work and I wanted to communicate my thoughts on how to handle this.

It also has the added benefit of receiving early comments (yes, please!)
and let other people who are messing around with this file a chance to
scream at me.

Christoffer Dall (6):
  arm/arm64: KVM: Rename irq_state to irq_pending
  arm/arm64: KVM: Rename irq_active to irq_queued
  arm/arm64: KVM: vgic: Clear queued flags on unqueue
  arm/arm64: KVM: vgic: Improve handling of GICD_I{CS}PENDRn
  arm/arm64: KVM: vgic: Fix SGI writes to GICD_I{CS}PENDR0
  arm/arm64: KVM: vgic: Clarify and correct vgic documentation

 include/kvm/arm_vgic.h |  24 ++++--
 virt/kvm/arm/vgic.c    | 217 +++++++++++++++++++++++++++++++++++++------------
 2 files changed, 184 insertions(+), 57 deletions(-)

-- 
1.8.5.2

[RFC PATCH 1/6] arm/arm64: KVM: Rename irq_state to irq_pending

[Christoffer Dall]

The irq_state field on the distributor struct is ambiguous in its
meaning; the comment says it's the level of the input put, but that
doesn't make much sense for edge-triggered interrupts.  The code
actually uses this state variable to check if the interrupt is in the
pending state on the distributor so clarify the comment and rename the
actual variable and accessor methods.

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

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index f27000f..a5d55d5 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -86,8 +86,8 @@ struct vgic_dist {
 	/* Interrupt enabled (one bit per IRQ) */
 	struct vgic_bitmap	irq_enabled;
 
-	/* Interrupt 'pin' level */
-	struct vgic_bitmap	irq_state;
+	/* Interrupt state is pending on the distributor */
+	struct vgic_bitmap	irq_pending;
 
 	/* Level-triggered interrupt in progress */
 	struct vgic_bitmap	irq_active;
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 47b2983..aba960a 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -37,7 +37,7 @@
  *
  * - At any time, the dist->irq_pending_on_cpu is the oracle that knows if
  *   something is pending
- * - VGIC pending interrupts are stored on the vgic.irq_state vgic
+ * - VGIC pending interrupts are stored on the vgic.irq_pending vgic
  *   bitmap (this bitmap is updated by both user land ioctls and guest
  *   mmio ops, and other in-kernel peripherals such as the
  *   arch. timers) and indicate the 'wire' state.
@@ -45,8 +45,8 @@
  *   recalculated
  * - To calculate the oracle, we need info for each cpu from
  *   compute_pending_for_cpu, which considers:
- *   - PPI: dist->irq_state & dist->irq_enable
- *   - SPI: dist->irq_state & dist->irq_enable & dist->irq_spi_target
+ *   - PPI: dist->irq_pending & dist->irq_enable
+ *   - SPI: dist->irq_pending & dist->irq_enable & dist->irq_spi_target
  *   - irq_spi_target is a 'formatted' version of the GICD_ICFGR
  *     registers, stored on each vcpu. We only keep one bit of
  *     information per interrupt, making sure that only one vcpu can
@@ -204,21 +204,21 @@ static int vgic_dist_irq_is_pending(struct kvm_vcpu *vcpu, int irq)
 {
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 
-	return vgic_bitmap_get_irq_val(&dist->irq_state, vcpu->vcpu_id, irq);
+	return vgic_bitmap_get_irq_val(&dist->irq_pending, vcpu->vcpu_id, irq);
 }
 
-static void vgic_dist_irq_set(struct kvm_vcpu *vcpu, int irq)
+static void vgic_dist_irq_set_pending(struct kvm_vcpu *vcpu, int irq)
 {
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 
-	vgic_bitmap_set_irq_val(&dist->irq_state, vcpu->vcpu_id, irq, 1);
+	vgic_bitmap_set_irq_val(&dist->irq_pending, vcpu->vcpu_id, irq, 1);
 }
 
-static void vgic_dist_irq_clear(struct kvm_vcpu *vcpu, int irq)
+static void vgic_dist_irq_clear_pending(struct kvm_vcpu *vcpu, int irq)
 {
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 
-	vgic_bitmap_set_irq_val(&dist->irq_state, vcpu->vcpu_id, irq, 0);
+	vgic_bitmap_set_irq_val(&dist->irq_pending, vcpu->vcpu_id, irq, 0);
 }
 
 static void vgic_cpu_irq_set(struct kvm_vcpu *vcpu, int irq)
@@ -392,7 +392,7 @@ static bool handle_mmio_set_pending_reg(struct kvm_vcpu *vcpu,
 					struct kvm_exit_mmio *mmio,
 					phys_addr_t offset)
 {
-	u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_state,
+	u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_pending,
 				       vcpu->vcpu_id, offset);
 	vgic_reg_access(mmio, reg, offset,
 			ACCESS_READ_VALUE | ACCESS_WRITE_SETBIT);
@@ -408,7 +408,7 @@ static bool handle_mmio_clear_pending_reg(struct kvm_vcpu *vcpu,
 					  struct kvm_exit_mmio *mmio,
 					  phys_addr_t offset)
 {
-	u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_state,
+	u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_pending,
 				       vcpu->vcpu_id, offset);
 	vgic_reg_access(mmio, reg, offset,
 			ACCESS_READ_VALUE | ACCESS_WRITE_CLEARBIT);
@@ -650,7 +650,7 @@ static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
 		 * is fine, then we are only setting a few bits that were
 		 * already set.
 		 */
-		vgic_dist_irq_set(vcpu, irq);
+		vgic_dist_irq_set_pending(vcpu, irq);
 		if (irq < VGIC_NR_SGIS)
 			dist->irq_sgi_sources[vcpu_id][irq] |= 1 << source_cpu;
 		*lr &= ~GICH_LR_PENDING_BIT;
@@ -929,7 +929,7 @@ static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg)
 	kvm_for_each_vcpu(c, vcpu, kvm) {
 		if (target_cpus & 1) {
 			/* Flag the SGI as pending */
-			vgic_dist_irq_set(vcpu, sgi);
+			vgic_dist_irq_set_pending(vcpu, sgi);
 			dist->irq_sgi_sources[c][sgi] |= 1 << vcpu_id;
 			kvm_debug("SGI%d from CPU%d to CPU%d\n", sgi, vcpu_id, c);
 		}
@@ -949,11 +949,11 @@ 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;
 
-	pending = vgic_bitmap_get_cpu_map(&dist->irq_state, vcpu_id);
+	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);
 
-	pending = vgic_bitmap_get_shared_map(&dist->irq_state);
+	pending = vgic_bitmap_get_shared_map(&dist->irq_pending);
 	enabled = vgic_bitmap_get_shared_map(&dist->irq_enabled);
 	bitmap_and(pend_shared, pending, enabled, VGIC_NR_SHARED_IRQS);
 	bitmap_and(pend_shared, pend_shared,
@@ -1085,7 +1085,7 @@ static bool vgic_queue_sgi(struct kvm_vcpu *vcpu, int irq)
 	 * our emulated gic and can get rid of them.
 	 */
 	if (!sources) {
-		vgic_dist_irq_clear(vcpu, irq);
+		vgic_dist_irq_clear_pending(vcpu, irq);
 		vgic_cpu_irq_clear(vcpu, irq);
 		return true;
 	}
@@ -1100,7 +1100,7 @@ static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, int irq)
 
 	if (vgic_queue_irq(vcpu, 0, irq)) {
 		if (vgic_irq_is_edge(vcpu, irq)) {
-			vgic_dist_irq_clear(vcpu, irq);
+			vgic_dist_irq_clear_pending(vcpu, irq);
 			vgic_cpu_irq_clear(vcpu, irq);
 		} else {
 			vgic_irq_set_active(vcpu, irq);
@@ -1297,7 +1297,7 @@ static void vgic_kick_vcpus(struct kvm *kvm)
 
 static int vgic_validate_injection(struct kvm_vcpu *vcpu, int irq, int level)
 {
-	int is_edge = vgic_irq_is_edge(vcpu, irq);
+	int edge_triggered = vgic_irq_is_edge(vcpu, irq);
 	int state = vgic_dist_irq_is_pending(vcpu, irq);
 
 	/*
@@ -1305,26 +1305,26 @@ static int vgic_validate_injection(struct kvm_vcpu *vcpu, int irq, int level)
 	 * - edge triggered and we have a rising edge
 	 * - level triggered and we change level
 	 */
-	if (is_edge)
+	if (edge_triggered)
 		return level > state;
 	else
 		return level != state;
 }
 
-static bool vgic_update_irq_state(struct kvm *kvm, int cpuid,
+static bool vgic_update_irq_pending(struct kvm *kvm, int cpuid,
 				  unsigned int irq_num, bool level)
 {
 	struct vgic_dist *dist = &kvm->arch.vgic;
 	struct kvm_vcpu *vcpu;
-	int is_edge, is_level;
+	int edge_triggered, level_triggered;
 	int enabled;
 	bool ret = true;
 
 	spin_lock(&dist->lock);
 
 	vcpu = kvm_get_vcpu(kvm, cpuid);
-	is_edge = vgic_irq_is_edge(vcpu, irq_num);
-	is_level = !is_edge;
+	edge_triggered = vgic_irq_is_edge(vcpu, irq_num);
+	level_triggered = !edge_triggered;
 
 	if (!vgic_validate_injection(vcpu, irq_num, level)) {
 		ret = false;
@@ -1339,9 +1339,9 @@ static bool vgic_update_irq_state(struct kvm *kvm, int cpuid,
 	kvm_debug("Inject IRQ%d level %d CPU%d\n", irq_num, level, cpuid);
 
 	if (level)
-		vgic_dist_irq_set(vcpu, irq_num);
+		vgic_dist_irq_set_pending(vcpu, irq_num);
 	else
-		vgic_dist_irq_clear(vcpu, irq_num);
+		vgic_dist_irq_clear_pending(vcpu, irq_num);
 
 	enabled = vgic_irq_is_enabled(vcpu, irq_num);
 
@@ -1350,7 +1350,7 @@ static bool vgic_update_irq_state(struct kvm *kvm, int cpuid,
 		goto out;
 	}
 
-	if (is_level && vgic_irq_is_active(vcpu, irq_num)) {
+	if (level_triggered && vgic_irq_is_active(vcpu, irq_num)) {
 		/*
 		 * Level interrupt in progress, will be picked up
 		 * when EOId.
@@ -1387,7 +1387,7 @@ out:
 int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
 			bool level)
 {
-	if (vgic_update_irq_state(kvm, cpuid, irq_num, level))
+	if (vgic_update_irq_pending(kvm, cpuid, irq_num, level))
 		vgic_kick_vcpus(kvm);
 
 	return 0;
-- 
1.8.5.2

[RFC PATCH 1/6] arm/arm64: KVM: Rename irq_state to irq_pending

[Eric Auger]

On 06/14/2014 10:51 PM, Christoffer Dall wrote:
> The irq_state field on the distributor struct is ambiguous in its
> meaning; the comment says it's the level of the input put, but that
> doesn't make much sense for edge-triggered interrupts.  The code
> actually uses this state variable to check if the interrupt is in the
> pending state on the distributor so clarify the comment and rename the
> actual variable and accessor methods.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  include/kvm/arm_vgic.h |  4 ++--
>  virt/kvm/arm/vgic.c    | 52 +++++++++++++++++++++++++-------------------------
>  2 files changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index f27000f..a5d55d5 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -86,8 +86,8 @@ struct vgic_dist {
>  	/* Interrupt enabled (one bit per IRQ) */
>  	struct vgic_bitmap	irq_enabled;
>  
> -	/* Interrupt 'pin' level */
> -	struct vgic_bitmap	irq_state;
> +	/* Interrupt state is pending on the distributor */
> +	struct vgic_bitmap	irq_pending;
>  
>  	/* Level-triggered interrupt in progress */
>  	struct vgic_bitmap	irq_active;
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 47b2983..aba960a 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -37,7 +37,7 @@
>   *
>   * - At any time, the dist->irq_pending_on_cpu is the oracle that knows if
>   *   something is pending
> - * - VGIC pending interrupts are stored on the vgic.irq_state vgic
> + * - VGIC pending interrupts are stored on the vgic.irq_pending vgic
>   *   bitmap (this bitmap is updated by both user land ioctls and guest
>   *   mmio ops, and other in-kernel peripherals such as the
>   *   arch. timers) and indicate the 'wire' state.
Hi Christoffer,

Shouldn't we say that vgic.irq_pending is a combination of wire state
and soft pending set action(ISPENDR), corresponding to GIC archi spec
"status_includes_pending" - at least for level sensitive IRQS - ?

Best Regards

Eric

> @@ -45,8 +45,8 @@
>   *   recalculated
>   * - To calculate the oracle, we need info for each cpu from
>   *   compute_pending_for_cpu, which considers:
> - *   - PPI: dist->irq_state & dist->irq_enable
> - *   - SPI: dist->irq_state & dist->irq_enable & dist->irq_spi_target
> + *   - PPI: dist->irq_pending & dist->irq_enable
> + *   - SPI: dist->irq_pending & dist->irq_enable & dist->irq_spi_target
>   *   - irq_spi_target is a 'formatted' version of the GICD_ICFGR
>   *     registers, stored on each vcpu. We only keep one bit of
>   *     information per interrupt, making sure that only one vcpu can
> @@ -204,21 +204,21 @@ static int vgic_dist_irq_is_pending(struct kvm_vcpu *vcpu, int irq)
>  {
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>  
> -	return vgic_bitmap_get_irq_val(&dist->irq_state, vcpu->vcpu_id, irq);
> +	return vgic_bitmap_get_irq_val(&dist->irq_pending, vcpu->vcpu_id, irq);
>  }
>  
> -static void vgic_dist_irq_set(struct kvm_vcpu *vcpu, int irq)
> +static void vgic_dist_irq_set_pending(struct kvm_vcpu *vcpu, int irq)
>  {
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>  
> -	vgic_bitmap_set_irq_val(&dist->irq_state, vcpu->vcpu_id, irq, 1);
> +	vgic_bitmap_set_irq_val(&dist->irq_pending, vcpu->vcpu_id, irq, 1);
>  }
>  
> -static void vgic_dist_irq_clear(struct kvm_vcpu *vcpu, int irq)
> +static void vgic_dist_irq_clear_pending(struct kvm_vcpu *vcpu, int irq)
>  {
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>  
> -	vgic_bitmap_set_irq_val(&dist->irq_state, vcpu->vcpu_id, irq, 0);
> +	vgic_bitmap_set_irq_val(&dist->irq_pending, vcpu->vcpu_id, irq, 0);
>  }
>  
>  static void vgic_cpu_irq_set(struct kvm_vcpu *vcpu, int irq)
> @@ -392,7 +392,7 @@ static bool handle_mmio_set_pending_reg(struct kvm_vcpu *vcpu,
>  					struct kvm_exit_mmio *mmio,
>  					phys_addr_t offset)
>  {
> -	u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_state,
> +	u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_pending,
>  				       vcpu->vcpu_id, offset);
>  	vgic_reg_access(mmio, reg, offset,
>  			ACCESS_READ_VALUE | ACCESS_WRITE_SETBIT);
> @@ -408,7 +408,7 @@ static bool handle_mmio_clear_pending_reg(struct kvm_vcpu *vcpu,
>  					  struct kvm_exit_mmio *mmio,
>  					  phys_addr_t offset)
>  {
> -	u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_state,
> +	u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_pending,
>  				       vcpu->vcpu_id, offset);
>  	vgic_reg_access(mmio, reg, offset,
>  			ACCESS_READ_VALUE | ACCESS_WRITE_CLEARBIT);
> @@ -650,7 +650,7 @@ static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
>  		 * is fine, then we are only setting a few bits that were
>  		 * already set.
>  		 */
> -		vgic_dist_irq_set(vcpu, irq);
> +		vgic_dist_irq_set_pending(vcpu, irq);
>  		if (irq < VGIC_NR_SGIS)
>  			dist->irq_sgi_sources[vcpu_id][irq] |= 1 << source_cpu;
>  		*lr &= ~GICH_LR_PENDING_BIT;
> @@ -929,7 +929,7 @@ static void vgic_dispatch_sgi(struct kvm_vcpu *vcpu, u32 reg)
>  	kvm_for_each_vcpu(c, vcpu, kvm) {
>  		if (target_cpus & 1) {
>  			/* Flag the SGI as pending */
> -			vgic_dist_irq_set(vcpu, sgi);
> +			vgic_dist_irq_set_pending(vcpu, sgi);
>  			dist->irq_sgi_sources[c][sgi] |= 1 << vcpu_id;
>  			kvm_debug("SGI%d from CPU%d to CPU%d\n", sgi, vcpu_id, c);
>  		}
> @@ -949,11 +949,11 @@ 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;
>  
> -	pending = vgic_bitmap_get_cpu_map(&dist->irq_state, vcpu_id);
> +	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);
>  
> -	pending = vgic_bitmap_get_shared_map(&dist->irq_state);
> +	pending = vgic_bitmap_get_shared_map(&dist->irq_pending);
>  	enabled = vgic_bitmap_get_shared_map(&dist->irq_enabled);
>  	bitmap_and(pend_shared, pending, enabled, VGIC_NR_SHARED_IRQS);
>  	bitmap_and(pend_shared, pend_shared,
> @@ -1085,7 +1085,7 @@ static bool vgic_queue_sgi(struct kvm_vcpu *vcpu, int irq)
>  	 * our emulated gic and can get rid of them.
>  	 */
>  	if (!sources) {
> -		vgic_dist_irq_clear(vcpu, irq);
> +		vgic_dist_irq_clear_pending(vcpu, irq);
>  		vgic_cpu_irq_clear(vcpu, irq);
>  		return true;
>  	}
> @@ -1100,7 +1100,7 @@ static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, int irq)
>  
>  	if (vgic_queue_irq(vcpu, 0, irq)) {
>  		if (vgic_irq_is_edge(vcpu, irq)) {
> -			vgic_dist_irq_clear(vcpu, irq);
> +			vgic_dist_irq_clear_pending(vcpu, irq);
>  			vgic_cpu_irq_clear(vcpu, irq);
>  		} else {
>  			vgic_irq_set_active(vcpu, irq);
> @@ -1297,7 +1297,7 @@ static void vgic_kick_vcpus(struct kvm *kvm)
>  
>  static int vgic_validate_injection(struct kvm_vcpu *vcpu, int irq, int level)
>  {
> -	int is_edge = vgic_irq_is_edge(vcpu, irq);
> +	int edge_triggered = vgic_irq_is_edge(vcpu, irq);
>  	int state = vgic_dist_irq_is_pending(vcpu, irq);
>  
>  	/*
> @@ -1305,26 +1305,26 @@ static int vgic_validate_injection(struct kvm_vcpu *vcpu, int irq, int level)
>  	 * - edge triggered and we have a rising edge
>  	 * - level triggered and we change level
>  	 */
> -	if (is_edge)
> +	if (edge_triggered)
>  		return level > state;
>  	else
>  		return level != state;
>  }
>  
> -static bool vgic_update_irq_state(struct kvm *kvm, int cpuid,
> +static bool vgic_update_irq_pending(struct kvm *kvm, int cpuid,
>  				  unsigned int irq_num, bool level)
>  {
>  	struct vgic_dist *dist = &kvm->arch.vgic;
>  	struct kvm_vcpu *vcpu;
> -	int is_edge, is_level;
> +	int edge_triggered, level_triggered;
>  	int enabled;
>  	bool ret = true;
>  
>  	spin_lock(&dist->lock);
>  
>  	vcpu = kvm_get_vcpu(kvm, cpuid);
> -	is_edge = vgic_irq_is_edge(vcpu, irq_num);
> -	is_level = !is_edge;
> +	edge_triggered = vgic_irq_is_edge(vcpu, irq_num);
> +	level_triggered = !edge_triggered;
>  
>  	if (!vgic_validate_injection(vcpu, irq_num, level)) {
>  		ret = false;
> @@ -1339,9 +1339,9 @@ static bool vgic_update_irq_state(struct kvm *kvm, int cpuid,
>  	kvm_debug("Inject IRQ%d level %d CPU%d\n", irq_num, level, cpuid);
>  
>  	if (level)
> -		vgic_dist_irq_set(vcpu, irq_num);
> +		vgic_dist_irq_set_pending(vcpu, irq_num);
>  	else
> -		vgic_dist_irq_clear(vcpu, irq_num);
> +		vgic_dist_irq_clear_pending(vcpu, irq_num);
>  
>  	enabled = vgic_irq_is_enabled(vcpu, irq_num);
>  
> @@ -1350,7 +1350,7 @@ static bool vgic_update_irq_state(struct kvm *kvm, int cpuid,
>  		goto out;
>  	}
>  
> -	if (is_level && vgic_irq_is_active(vcpu, irq_num)) {
> +	if (level_triggered && vgic_irq_is_active(vcpu, irq_num)) {
>  		/*
>  		 * Level interrupt in progress, will be picked up
>  		 * when EOId.
> @@ -1387,7 +1387,7 @@ out:
>  int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>  			bool level)
>  {
> -	if (vgic_update_irq_state(kvm, cpuid, irq_num, level))
> +	if (vgic_update_irq_pending(kvm, cpuid, irq_num, level))
>  		vgic_kick_vcpus(kvm);
>  
>  	return 0;
> 

[RFC PATCH 1/6] arm/arm64: KVM: Rename irq_state to irq_pending

[Marc Zyngier]

On 2014-06-14 21:51, Christoffer Dall wrote:
> The irq_state field on the distributor struct is ambiguous in its
> meaning; the comment says it's the level of the input put, but that
> doesn't make much sense for edge-triggered interrupts.  The code
> actually uses this state variable to check if the interrupt is in the
> pending state on the distributor so clarify the comment and rename 
> the
> actual variable and accessor methods.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>

Looks good to me.

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

         M.
-- 
Who you jivin' with that Cosmik Debris?

[RFC PATCH 2/6] arm/arm64: KVM: Rename irq_active to irq_queued

[Christoffer Dall]

We have a special bitmap on the distributor struct to keep track of when
level-triggered interrupts are queued on the list registers.  This was
named irq_active, which is confusing, because the active state of an
interrupt as per the GIC spec is a different thing, not specifically
related to edge-triggered/level-triggered configurations but rather
indicates an interrupt which has been ack'ed but not yet eoi'ed.

Rename the bitmap and the corresponding accessor functions to irq_queued
to clarify what this is actually used for.

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

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index a5d55d5..10fa64b 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -89,8 +89,8 @@ struct vgic_dist {
 	/* Interrupt state is pending on the distributor */
 	struct vgic_bitmap	irq_pending;
 
-	/* Level-triggered interrupt in progress */
-	struct vgic_bitmap	irq_active;
+	/* Level-triggered interrupt queued on VCPU interface */
+	struct vgic_bitmap	irq_queued;
 
 	/* Interrupt priority. Not used yet. */
 	struct vgic_bytemap	irq_priority;
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index aba960a..00e6bdd 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -60,12 +60,12 @@
  * the 'line' again. This is achieved as such:
  *
  * - When a level interrupt is moved onto a vcpu, the corresponding
- *   bit in irq_active is set. As long as this bit is set, the line
+ *   bit in irq_queued is set. As long as this bit is set, the line
  *   will be ignored for further interrupts. The interrupt is injected
  *   into the vcpu with the GICH_LR_EOI bit set (generate a
  *   maintenance interrupt on EOI).
  * - When the interrupt is EOIed, the maintenance interrupt fires,
- *   and clears the corresponding bit in irq_active. This allow the
+ *   and clears the corresponding bit in irq_queued. This allow the
  *   interrupt line to be sampled again.
  */
 
@@ -179,25 +179,25 @@ static int vgic_irq_is_enabled(struct kvm_vcpu *vcpu, int irq)
 	return vgic_bitmap_get_irq_val(&dist->irq_enabled, vcpu->vcpu_id, irq);
 }
 
-static int vgic_irq_is_active(struct kvm_vcpu *vcpu, int irq)
+static int vgic_irq_is_queued(struct kvm_vcpu *vcpu, int irq)
 {
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 
-	return vgic_bitmap_get_irq_val(&dist->irq_active, vcpu->vcpu_id, irq);
+	return vgic_bitmap_get_irq_val(&dist->irq_queued, vcpu->vcpu_id, irq);
 }
 
-static void vgic_irq_set_active(struct kvm_vcpu *vcpu, int irq)
+static void vgic_irq_set_queued(struct kvm_vcpu *vcpu, int irq)
 {
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 
-	vgic_bitmap_set_irq_val(&dist->irq_active, vcpu->vcpu_id, irq, 1);
+	vgic_bitmap_set_irq_val(&dist->irq_queued, vcpu->vcpu_id, irq, 1);
 }
 
-static void vgic_irq_clear_active(struct kvm_vcpu *vcpu, int irq)
+static void vgic_irq_clear_queued(struct kvm_vcpu *vcpu, int irq)
 {
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 
-	vgic_bitmap_set_irq_val(&dist->irq_active, vcpu->vcpu_id, irq, 0);
+	vgic_bitmap_set_irq_val(&dist->irq_queued, vcpu->vcpu_id, irq, 0);
 }
 
 static int vgic_dist_irq_is_pending(struct kvm_vcpu *vcpu, int irq)
@@ -1011,8 +1011,8 @@ static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu)
 
 		if (!vgic_irq_is_enabled(vcpu, irq)) {
 			vgic_retire_lr(lr, irq, vgic_cpu);
-			if (vgic_irq_is_active(vcpu, irq))
-				vgic_irq_clear_active(vcpu, irq);
+			if (vgic_irq_is_queued(vcpu, irq))
+				vgic_irq_clear_queued(vcpu, irq);
 		}
 	}
 }
@@ -1095,7 +1095,7 @@ static bool vgic_queue_sgi(struct kvm_vcpu *vcpu, int irq)
 
 static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, int irq)
 {
-	if (vgic_irq_is_active(vcpu, irq))
+	if (vgic_irq_is_queued(vcpu, irq))
 		return true; /* level interrupt, already queued */
 
 	if (vgic_queue_irq(vcpu, 0, irq)) {
@@ -1103,7 +1103,7 @@ static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, int irq)
 			vgic_dist_irq_clear_pending(vcpu, irq);
 			vgic_cpu_irq_clear(vcpu, irq);
 		} else {
-			vgic_irq_set_active(vcpu, irq);
+			vgic_irq_set_queued(vcpu, irq);
 		}
 
 		return true;
@@ -1186,7 +1186,7 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
 				 vgic_cpu->nr_lr) {
 			irq = vgic_cpu->vgic_lr[lr] & GICH_LR_VIRTUALID;
 
-			vgic_irq_clear_active(vcpu, irq);
+			vgic_irq_clear_queued(vcpu, irq);
 			vgic_cpu->vgic_lr[lr] &= ~GICH_LR_EOI;
 
 			/* Any additional pending interrupt? */
@@ -1350,7 +1350,7 @@ static bool vgic_update_irq_pending(struct kvm *kvm, int cpuid,
 		goto out;
 	}
 
-	if (level_triggered && vgic_irq_is_active(vcpu, irq_num)) {
+	if (level_triggered && vgic_irq_is_queued(vcpu, irq_num)) {
 		/*
 		 * Level interrupt in progress, will be picked up
 		 * when EOId.
-- 
1.8.5.2

[RFC PATCH 2/6] arm/arm64: KVM: Rename irq_active to irq_queued

[Marc Zyngier]

On 2014-06-14 21:51, Christoffer Dall wrote:
> We have a special bitmap on the distributor struct to keep track of 
> when
> level-triggered interrupts are queued on the list registers.  This 
> was
> named irq_active, which is confusing, because the active state of an
> interrupt as per the GIC spec is a different thing, not specifically
> related to edge-triggered/level-triggered configurations but rather
> indicates an interrupt which has been ack'ed but not yet eoi'ed.
>
> Rename the bitmap and the corresponding accessor functions to 
> irq_queued
> to clarify what this is actually used for.

While I agree that irq_active is confusing, I would tend to object to 
irq_queued for similar reasons. Edge interrupts get queued as well.

What this bit does is to allow or forbid resampling of a level 
interrupt.

How about irq_resample instead? That would mandate a small refactor of 
the code (a bit set to one would allow resampling, which is the opposite 
of the current logic), but would look better, I believe.

What do you think?

         M.
-- 
Who you jivin' with that Cosmik Debris?

[RFC PATCH 2/6] arm/arm64: KVM: Rename irq_active to irq_queued

[Christoffer Dall]

On Sun, Jun 22, 2014 at 12:25:02PM +0100, Marc Zyngier wrote:
> On 2014-06-14 21:51, Christoffer Dall wrote:
> >We have a special bitmap on the distributor struct to keep track
> >of when
> >level-triggered interrupts are queued on the list registers.  This
> >was
> >named irq_active, which is confusing, because the active state of an
> >interrupt as per the GIC spec is a different thing, not specifically
> >related to edge-triggered/level-triggered configurations but rather
> >indicates an interrupt which has been ack'ed but not yet eoi'ed.
> >
> >Rename the bitmap and the corresponding accessor functions to
> >irq_queued
> >to clarify what this is actually used for.
> 
> While I agree that irq_active is confusing, I would tend to object
> to irq_queued for similar reasons. Edge interrupts get queued as
> well.

yeah, but this is never checked for edge-triggered IRQs so I don't find
that part confusing.  I find the queued word suitable, because we set in
in the _queue function and unset it in the unqueue function.

> 
> What this bit does is to allow or forbid resampling of a level
> interrupt.
> 
> How about irq_resample instead? That would mandate a small refactor
> of the code (a bit set to one would allow resampling, which is the
> opposite of the current logic), but would look better, I believe.
> 
> What do you think?
> 
hmm, maybe.  Feel like illustrating what you mean exactly in form of a
patch?

-Christoffer

[RFC PATCH 3/6] arm/arm64: KVM: vgic: Clear queued flags on unqueue

[Christoffer Dall]

If we unqueue a level-triggered interrupt completely, and the LR does
not stick around in the active state (and will therefore no longer
generate a maintenance interrupt), then we should clear the queued flag
so that the vgic can actually queue this level-triggered interrupt at a
later time and deal with its pending state then.

Note: This should actually be properly fixed to handle the active state
on the distributor.

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

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 00e6bdd..87c977c 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -660,8 +660,10 @@ static void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
 		 * active), then the LR does not hold any useful info and can
 		 * be marked as free for other use.
 		 */
-		if (!(*lr & GICH_LR_STATE))
+		if (!(*lr & GICH_LR_STATE)) {
 			vgic_retire_lr(i, irq, vgic_cpu);
+			vgic_irq_clear_queued(vcpu, irq);
+		}
 
 		/* Finally update the VGIC state. */
 		vgic_update_state(vcpu->kvm);
-- 
1.8.5.2

[RFC PATCH 3/6] arm/arm64: KVM: vgic: Clear queued flags on unqueue

[Marc Zyngier]

On 2014-06-14 21:51, Christoffer Dall wrote:
> If we unqueue a level-triggered interrupt completely, and the LR does
> not stick around in the active state (and will therefore no longer
> generate a maintenance interrupt), then we should clear the queued 
> flag
> so that the vgic can actually queue this level-triggered interrupt at 
> a
> later time and deal with its pending state then.
>
> Note: This should actually be properly fixed to handle the active 
> state
> on the distributor.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>

With the previous remark about queued/resample in mind,

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

         M.
-- 
Fast, cheap, reliable. Pick two.

[RFC PATCH 4/6] arm/arm64: KVM: vgic: Improve handling of GICD_I{CS}PENDRn

[Christoffer Dall]

The handling of writes to the GICD_ISPENDRn and GICD_ICPENDRn is
currently not handled correctly for level-triggered interrupts.  The
spec states that for level-triggered interrupts, writes to the
GICD_ISPENDRn activates the output of a flip-flop which is in turn or'ed
with the actual input interrupt signal.  Correspondingly, writes to
GICD_ICPENDRn simply deactives the output of that flip-flop, but does
not (of course) affect the external input signal.  Reads from GICC_IAR
will also deactivate the flip-flop output.

This requires us to track the state of the level-input separately from
the state in the flip-flop.  Introduce two new variables on the
distributor struct to track these two exact states.  Astute readers
may notice that this is introducing more state than required (because an
OR of the two states give you the pending state), but the remainding
vgic code uses the pending bitmap for optimized operations to figure
out, at the end of the day, if an interrupt is pending or not on the
distributor side.  Changing all the to consider the two state variables
did not look pretty.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 include/kvm/arm_vgic.h |  16 ++++++-
 virt/kvm/arm/vgic.c    | 118 ++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 122 insertions(+), 12 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 10fa64b..f678d5c 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -86,9 +86,23 @@ struct vgic_dist {
 	/* Interrupt enabled (one bit per IRQ) */
 	struct vgic_bitmap	irq_enabled;
 
-	/* Interrupt state is pending on the distributor */
+	/* Level-triggered interrupt external input is asserted */
+	struct vgic_bitmap	irq_level;
+
+	/*
+	 * Interrupt state is pending on the distributor
+	 */
 	struct vgic_bitmap	irq_pending;
 
+	/*
+	 * Tracks writes to GICD_ISPENDRn and GICD_ICPENDRn for level-triggered
+	 * interrupts.  Essentially holds the state of the flip-flop in
+	 * Figure 4-10 on page 4-101 in ARM IHI 0048B.b.
+	 * Once set, it is only cleared for level-triggered interrupts on
+	 * guest ACKs (when we queue it) or writes to GICD_ICPENDRn.
+	 */
+	struct vgic_bitmap	irq_soft_pend;
+
 	/* Level-triggered interrupt queued on VCPU interface */
 	struct vgic_bitmap	irq_queued;
 
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 87c977c..0b41875 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -67,6 +67,11 @@
  * - When the interrupt is EOIed, the maintenance interrupt fires,
  *   and clears the corresponding bit in irq_queued. This allow the
  *   interrupt line to be sampled again.
+ * - Note that level-triggered interrupts can also be set to pending from
+ *   writes to GICD_ISPENDRn and lowering the external input line does not
+ *   cause the interrupt to become inactive in such a situation.
+ *   Conversely, writes to GICD_ICPENDRn do not cause the interrupt to become
+ *   inactive as long as the external input line is held high.
  */
 
 #define VGIC_ADDR_UNDEF		(-1)
@@ -200,6 +205,41 @@ static void vgic_irq_clear_queued(struct kvm_vcpu *vcpu, int irq)
 	vgic_bitmap_set_irq_val(&dist->irq_queued, vcpu->vcpu_id, irq, 0);
 }
 
+static int vgic_dist_irq_get_level(struct kvm_vcpu *vcpu, int irq)
+{
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+	return vgic_bitmap_get_irq_val(&dist->irq_level, vcpu->vcpu_id, irq);
+}
+
+static void vgic_dist_irq_set_level(struct kvm_vcpu *vcpu, int irq)
+{
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+	vgic_bitmap_set_irq_val(&dist->irq_level, vcpu->vcpu_id, irq, 1);
+}
+
+static void vgic_dist_irq_clear_level(struct kvm_vcpu *vcpu, int irq)
+{
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+	vgic_bitmap_set_irq_val(&dist->irq_level, vcpu->vcpu_id, irq, 0);
+}
+
+static int vgic_dist_irq_soft_pend(struct kvm_vcpu *vcpu, int irq)
+{
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+	return vgic_bitmap_get_irq_val(&dist->irq_soft_pend, vcpu->vcpu_id, irq);
+}
+
+static void vgic_dist_irq_clear_soft_pend(struct kvm_vcpu *vcpu, int irq)
+{
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+	vgic_bitmap_set_irq_val(&dist->irq_soft_pend, vcpu->vcpu_id, irq, 0);
+}
+
 static int vgic_dist_irq_is_pending(struct kvm_vcpu *vcpu, int irq)
 {
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
@@ -392,11 +432,26 @@ static bool handle_mmio_set_pending_reg(struct kvm_vcpu *vcpu,
 					struct kvm_exit_mmio *mmio,
 					phys_addr_t offset)
 {
-	u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_pending,
-				       vcpu->vcpu_id, offset);
+	u32 *reg;
+	u32 level_mask;
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+	reg = vgic_bitmap_get_reg(&dist->irq_cfg, vcpu->vcpu_id, offset);
+	level_mask = (~(*reg));
+
+	/* Mark both level and edge triggered irqs as pending */
+	reg = vgic_bitmap_get_reg(&dist->irq_pending, vcpu->vcpu_id, offset);
 	vgic_reg_access(mmio, reg, offset,
 			ACCESS_READ_VALUE | ACCESS_WRITE_SETBIT);
+
 	if (mmio->is_write) {
+		/* Set the soft-pending flag only for level-triggered irqs */
+		reg = vgic_bitmap_get_reg(&dist->irq_soft_pend,
+					  vcpu->vcpu_id, offset);
+		vgic_reg_access(mmio, reg, offset,
+				ACCESS_READ_VALUE | ACCESS_WRITE_SETBIT);
+		*reg &= level_mask;
+
 		vgic_update_state(vcpu->kvm);
 		return true;
 	}
@@ -408,11 +463,27 @@ static bool handle_mmio_clear_pending_reg(struct kvm_vcpu *vcpu,
 					  struct kvm_exit_mmio *mmio,
 					  phys_addr_t offset)
 {
-	u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_pending,
-				       vcpu->vcpu_id, offset);
+	u32 *level_active;
+	u32 *reg;
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+	reg = vgic_bitmap_get_reg(&dist->irq_pending, vcpu->vcpu_id, offset);
 	vgic_reg_access(mmio, reg, offset,
 			ACCESS_READ_VALUE | ACCESS_WRITE_CLEARBIT);
 	if (mmio->is_write) {
+		/* Re-set level triggered level-active interrupts */
+		level_active = vgic_bitmap_get_reg(&dist->irq_level,
+					  vcpu->vcpu_id, offset);
+		reg = vgic_bitmap_get_reg(&dist->irq_pending,
+					  vcpu->vcpu_id, offset);
+		*reg |= *level_active;
+
+		/* Clear soft-pending flags */
+		reg = vgic_bitmap_get_reg(&dist->irq_soft_pend,
+					  vcpu->vcpu_id, offset);
+		vgic_reg_access(mmio, reg, offset,
+				ACCESS_READ_VALUE | ACCESS_WRITE_CLEARBIT);
+
 		vgic_update_state(vcpu->kvm);
 		return true;
 	}
@@ -1187,15 +1258,29 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
 		for_each_set_bit(lr, (unsigned long *)vgic_cpu->vgic_eisr,
 				 vgic_cpu->nr_lr) {
 			irq = vgic_cpu->vgic_lr[lr] & GICH_LR_VIRTUALID;
+			BUG_ON(vgic_irq_is_edge(vcpu, irq));
 
 			vgic_irq_clear_queued(vcpu, irq);
 			vgic_cpu->vgic_lr[lr] &= ~GICH_LR_EOI;
 
+			/*
+			 * If the IRQ was EOIed it was most certainly also
+			 * ACKed and we can therefore always clear the soft
+			 * pending state (should it had been set) of this
+			 * interrupt.
+			 */
+			vgic_dist_irq_clear_soft_pend(vcpu, irq);
+
 			/* Any additional pending interrupt? */
-			if (vgic_dist_irq_is_pending(vcpu, irq)) {
+			if (vgic_dist_irq_get_level(vcpu, irq)) {
+				/*
+				 * XXX: vgic_cpu_irq_set not always be true in
+				 * this case?
+				 */
 				vgic_cpu_irq_set(vcpu, irq);
 				level_pending = true;
 			} else {
+				vgic_dist_irq_clear_pending(vcpu, irq);
 				vgic_cpu_irq_clear(vcpu, irq);
 			}
 
@@ -1300,17 +1385,19 @@ static void vgic_kick_vcpus(struct kvm *kvm)
 static int vgic_validate_injection(struct kvm_vcpu *vcpu, int irq, int level)
 {
 	int edge_triggered = vgic_irq_is_edge(vcpu, irq);
-	int state = vgic_dist_irq_is_pending(vcpu, irq);
 
 	/*
 	 * Only inject an interrupt if:
 	 * - edge triggered and we have a rising edge
 	 * - level triggered and we change level
 	 */
-	if (edge_triggered)
+	if (edge_triggered) {
+		int state = vgic_dist_irq_is_pending(vcpu, irq);
 		return level > state;
-	else
+	} else {
+		int state = vgic_dist_irq_get_level(vcpu, irq);
 		return level != state;
+	}
 }
 
 static bool vgic_update_irq_pending(struct kvm *kvm, int cpuid,
@@ -1340,10 +1427,19 @@ static bool vgic_update_irq_pending(struct kvm *kvm, int cpuid,
 
 	kvm_debug("Inject IRQ%d level %d CPU%d\n", irq_num, level, cpuid);
 
-	if (level)
+	if (level) {
+		if (level_triggered)
+			vgic_dist_irq_set_level(vcpu, irq_num);
 		vgic_dist_irq_set_pending(vcpu, irq_num);
-	else
-		vgic_dist_irq_clear_pending(vcpu, irq_num);
+	} else {
+		if (level_triggered) {
+			vgic_dist_irq_clear_level(vcpu, irq_num);
+			if (!vgic_dist_irq_soft_pend(vcpu, irq_num))
+				vgic_dist_irq_clear_pending(vcpu, irq_num);
+		} else {
+			vgic_dist_irq_clear_pending(vcpu, irq_num);
+		}
+	}
 
 	enabled = vgic_irq_is_enabled(vcpu, irq_num);
 
-- 
1.8.5.2

[RFC PATCH 4/6] arm/arm64: KVM: vgic: Improve handling of GICD_I{CS}PENDRn

[Eric Auger]

On 06/14/2014 10:51 PM, Christoffer Dall wrote:
> The handling of writes to the GICD_ISPENDRn and GICD_ICPENDRn is
> currently not handled correctly for level-triggered interrupts.
Hi Christoffer,

Thanks for this patch serie. I can confirm it fixes my QEMU/VFIO issue
where all IRQs were pending cleared at guest OS boot while IRQ wires
were set. Now those IRQs are left pending which is compliant with the
GIC spec. You will find few comments/questions below.

Best Regards

Eric
> spec states that for level-triggered interrupts, writes to the
> GICD_ISPENDRn activates the output of a flip-flop which is in turn or'ed
> with the actual input interrupt signal.  Correspondingly, writes to
> GICD_ICPENDRn simply deactives the output of that flip-flop, but does
deactivates
> not (of course) affect the external input signal.  Reads from GICC_IAR
> will also deactivate the flip-flop output.
> 
> This requires us to track the state of the level-input separately from
> the state in the flip-flop.  Introduce two new variables on the
> distributor struct to track these two exact states.  Astute readers
> may notice that this is introducing more state than required (because an
> OR of the two states give you the pending state), but the remainding
remaining
> vgic code uses the pending bitmap for optimized operations to figure
> out, at the end of the day, if an interrupt is pending or not on the
> distributor side.  Changing all the to consider the two state variables
sentence
> did not look pretty.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  include/kvm/arm_vgic.h |  16 ++++++-
>  virt/kvm/arm/vgic.c    | 118 ++++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 122 insertions(+), 12 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 10fa64b..f678d5c 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -86,9 +86,23 @@ struct vgic_dist {
>  	/* Interrupt enabled (one bit per IRQ) */
>  	struct vgic_bitmap	irq_enabled;
>  
> -	/* Interrupt state is pending on the distributor */
> +	/* Level-triggered interrupt external input is asserted */
> +	struct vgic_bitmap	irq_level;
was a bit confused by the name. I now understand this is the wire
(interrupt signal to GIC), relevant in case of level-sensitive IRQ.
~ wire_level.
> +
> +	/*
> +	 * Interrupt state is pending on the distributor
> +	 */
>  	struct vgic_bitmap	irq_pending;
~ status_includes_pending
>  
> +	/*
> +	 * Tracks writes to GICD_ISPENDRn and GICD_ICPENDRn for level-triggered
> +	 * interrupts.  Essentially holds the state of the flip-flop in
> +	 * Figure 4-10 on page 4-101 in ARM IHI 0048B.b.
> +	 * Once set, it is only cleared for level-triggered interrupts on
> +	 * guest ACKs (when we queue it) or writes to GICD_ICPENDRn.
> +	 */
> +	struct vgic_bitmap	irq_soft_pend;
> +
>  	/* Level-triggered interrupt queued on VCPU interface */
>  	struct vgic_bitmap	irq_queued;
>  
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 87c977c..0b41875 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -67,6 +67,11 @@
>   * - When the interrupt is EOIed, the maintenance interrupt fires,
>   *   and clears the corresponding bit in irq_queued. This allow the
>   *   interrupt line to be sampled again.
> + * - Note that level-triggered interrupts can also be set to pending from
> + *   writes to GICD_ISPENDRn and lowering the external input line does not
> + *   cause the interrupt to become inactive in such a situation.
> + *   Conversely, writes to GICD_ICPENDRn do not cause the interrupt to become
> + *   inactive as long as the external input line is held high.
>   */
>  
>  #define VGIC_ADDR_UNDEF		(-1)
> @@ -200,6 +205,41 @@ static void vgic_irq_clear_queued(struct kvm_vcpu *vcpu, int irq)
>  	vgic_bitmap_set_irq_val(&dist->irq_queued, vcpu->vcpu_id, irq, 0);
>  }
>  
> +static int vgic_dist_irq_get_level(struct kvm_vcpu *vcpu, int irq)
> +{
> +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +
> +	return vgic_bitmap_get_irq_val(&dist->irq_level, vcpu->vcpu_id, irq);
> +}
> +
> +static void vgic_dist_irq_set_level(struct kvm_vcpu *vcpu, int irq)
> +{
> +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +
> +	vgic_bitmap_set_irq_val(&dist->irq_level, vcpu->vcpu_id, irq, 1);
> +}
> +
> +static void vgic_dist_irq_clear_level(struct kvm_vcpu *vcpu, int irq)
> +{
> +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +
> +	vgic_bitmap_set_irq_val(&dist->irq_level, vcpu->vcpu_id, irq, 0);
> +}
> +
> +static int vgic_dist_irq_soft_pend(struct kvm_vcpu *vcpu, int irq)
> +{
> +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +
> +	return vgic_bitmap_get_irq_val(&dist->irq_soft_pend, vcpu->vcpu_id, irq);
> +}
> +
> +static void vgic_dist_irq_clear_soft_pend(struct kvm_vcpu *vcpu, int irq)
> +{
> +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +
> +	vgic_bitmap_set_irq_val(&dist->irq_soft_pend, vcpu->vcpu_id, irq, 0);
> +}
> +
>  static int vgic_dist_irq_is_pending(struct kvm_vcpu *vcpu, int irq)
>  {
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> @@ -392,11 +432,26 @@ static bool handle_mmio_set_pending_reg(struct kvm_vcpu *vcpu,
>  					struct kvm_exit_mmio *mmio,
>  					phys_addr_t offset)
>  {
> -	u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_pending,
> -				       vcpu->vcpu_id, offset);
> +	u32 *reg;
> +	u32 level_mask;
> +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +
> +	reg = vgic_bitmap_get_reg(&dist->irq_cfg, vcpu->vcpu_id, offset);
> +	level_mask = (~(*reg));
> +
> +	/* Mark both level and edge triggered irqs as pending */
OK, includes pending status latched on a write to the GICD_ISPENDRn
> +	reg = vgic_bitmap_get_reg(&dist->irq_pending, vcpu->vcpu_id, offset);
>  	vgic_reg_access(mmio, reg, offset,
>  			ACCESS_READ_VALUE | ACCESS_WRITE_SETBIT);
> +
>  	if (mmio->is_write) {
> +		/* Set the soft-pending flag only for level-triggered irqs */
> +		reg = vgic_bitmap_get_reg(&dist->irq_soft_pend,
> +					  vcpu->vcpu_id, offset);
> +		vgic_reg_access(mmio, reg, offset,
> +				ACCESS_READ_VALUE | ACCESS_WRITE_SETBIT);
> +		*reg &= level_mask;
OK, for level-sensitive IRQs, ISPENDR write updates the output of flip flop
> +
>  		vgic_update_state(vcpu->kvm);
>  		return true;
>  	}
> @@ -408,11 +463,27 @@ static bool handle_mmio_clear_pending_reg(struct kvm_vcpu *vcpu,
>  					  struct kvm_exit_mmio *mmio,
>  					  phys_addr_t offset)
>  {
> -	u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_pending,
> -				       vcpu->vcpu_id, offset);
> +	u32 *level_active;
> +	u32 *reg;
> +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> +
> +	reg = vgic_bitmap_get_reg(&dist->irq_pending, vcpu->vcpu_id, offset);
>  	vgic_reg_access(mmio, reg, offset,
>  			ACCESS_READ_VALUE | ACCESS_WRITE_CLEARBIT);
>  	if (mmio->is_write) {
> +		/* Re-set level triggered level-active interrupts */
I was confused by this comment ;-)
compute new status_includes_pending taking into account wire state and
GICD_ICPENDR?
> +		level_active = vgic_bitmap_get_reg(&dist->irq_level,
> +					  vcpu->vcpu_id, offset);
> +		reg = vgic_bitmap_get_reg(&dist->irq_pending,
> +					  vcpu->vcpu_id, offset);
> +		*reg |= *level_active;
OK, or between the wire and the GICD_ICPENDR
> +
> +		/* Clear soft-pending flags */
> +		reg = vgic_bitmap_get_reg(&dist->irq_soft_pend,
> +					  vcpu->vcpu_id, offset);
> +		vgic_reg_access(mmio, reg, offset,
> +				ACCESS_READ_VALUE | ACCESS_WRITE_CLEARBIT);
only relevant for level-triggered IRQ but OK
> +
>  		vgic_update_state(vcpu->kvm);
>  		return true;
>  	}
> @@ -1187,15 +1258,29 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
>  		for_each_set_bit(lr, (unsigned long *)vgic_cpu->vgic_eisr,
>  				 vgic_cpu->nr_lr) {
>  			irq = vgic_cpu->vgic_lr[lr] & GICH_LR_VIRTUALID;
> +			BUG_ON(vgic_irq_is_edge(vcpu, irq));
>  
>  			vgic_irq_clear_queued(vcpu, irq);
>  			vgic_cpu->vgic_lr[lr] &= ~GICH_LR_EOI;
>  
> +			/*
> +			 * If the IRQ was EOIed it was most certainly also
> +			 * ACKed and we can therefore always clear the soft
> +			 * pending state (should it had been set) of this
> +			 * interrupt.
> +			 */
> +			vgic_dist_irq_clear_soft_pend(vcpu, irq);
what if the virq was Acked and ISPENDR was set after? Can't it happen?
Anyway since we do not trap the ACK, I guess we can't do better?
> +
>  			/* Any additional pending interrupt? */
> -			if (vgic_dist_irq_is_pending(vcpu, irq)) {
> +			if (vgic_dist_irq_get_level(vcpu, irq)) {
> +				/*
> +				 * XXX: vgic_cpu_irq_set not always be true in
> +				 * this case?
> +				 */
>  				vgic_cpu_irq_set(vcpu, irq);
>  				level_pending = true;
>  			} else {
> +				vgic_dist_irq_clear_pending(vcpu, irq);
>  				vgic_cpu_irq_clear(vcpu, irq);
>  			}
>  
> @@ -1300,17 +1385,19 @@ static void vgic_kick_vcpus(struct kvm *kvm)
>  static int vgic_validate_injection(struct kvm_vcpu *vcpu, int irq, int level)
>  {
>  	int edge_triggered = vgic_irq_is_edge(vcpu, irq);
> -	int state = vgic_dist_irq_is_pending(vcpu, irq);
>  
>  	/*
>  	 * Only inject an interrupt if:
>  	 * - edge triggered and we have a rising edge
>  	 * - level triggered and we change level
>  	 */
> -	if (edge_triggered)
> +	if (edge_triggered) {
> +		int state = vgic_dist_irq_is_pending(vcpu, irq);
>  		return level > state;
> -	else
> +	} else {
> +		int state = vgic_dist_irq_get_level(vcpu, irq);
shouldn't we still compare against pending? What if soft pending happened?
>  		return level != state;
> +	}
>  }
>  
>  static bool vgic_update_irq_pending(struct kvm *kvm, int cpuid,
> @@ -1340,10 +1427,19 @@ static bool vgic_update_irq_pending(struct kvm *kvm, int cpuid,
>  
>  	kvm_debug("Inject IRQ%d level %d CPU%d\n", irq_num, level, cpuid);
>  
> -	if (level)
> +	if (level) {
> +		if (level_triggered)
> +			vgic_dist_irq_set_level(vcpu, irq_num);
>  		vgic_dist_irq_set_pending(vcpu, irq_num);
> -	else
> -		vgic_dist_irq_clear_pending(vcpu, irq_num);
> +	} else {
> +		if (level_triggered) {
> +			vgic_dist_irq_clear_level(vcpu, irq_num);
> +			if (!vgic_dist_irq_soft_pend(vcpu, irq_num))
> +				vgic_dist_irq_clear_pending(vcpu, irq_num);
OK wire toggled down cannot clear a soft pending IRQ
> +		} else {
> +			vgic_dist_irq_clear_pending(vcpu, irq_num);
> +		}
> +	}
>  
>  	enabled = vgic_irq_is_enabled(vcpu, irq_num);
>  
> 

[RFC PATCH 4/6] arm/arm64: KVM: vgic: Improve handling of GICD_I{CS}PENDRn

[Christoffer Dall]

On Wed, Jun 18, 2014 at 04:25:01PM +0200, Eric Auger wrote:
> On 06/14/2014 10:51 PM, Christoffer Dall wrote:
> > The handling of writes to the GICD_ISPENDRn and GICD_ICPENDRn is
> > currently not handled correctly for level-triggered interrupts.
> Hi Christoffer,
> 
> Thanks for this patch serie. I can confirm it fixes my QEMU/VFIO issue
> where all IRQs were pending cleared at guest OS boot while IRQ wires
> were set. Now those IRQs are left pending which is compliant with the
> GIC spec. You will find few comments/questions below.
> 
> Best Regards
> 
> Eric
> > spec states that for level-triggered interrupts, writes to the
> > GICD_ISPENDRn activates the output of a flip-flop which is in turn or'ed
> > with the actual input interrupt signal.  Correspondingly, writes to
> > GICD_ICPENDRn simply deactives the output of that flip-flop, but does
> deactivates
> > not (of course) affect the external input signal.  Reads from GICC_IAR
> > will also deactivate the flip-flop output.
> > 
> > This requires us to track the state of the level-input separately from
> > the state in the flip-flop.  Introduce two new variables on the
> > distributor struct to track these two exact states.  Astute readers
> > may notice that this is introducing more state than required (because an
> > OR of the two states give you the pending state), but the remainding
> remaining
> > vgic code uses the pending bitmap for optimized operations to figure
> > out, at the end of the day, if an interrupt is pending or not on the
> > distributor side.  Changing all the to consider the two state variables
> sentence
> > did not look pretty.

all fixed.

> > 
> > ---

[...]

> >  	}
> > @@ -408,11 +463,27 @@ static bool handle_mmio_clear_pending_reg(struct kvm_vcpu *vcpu,
> >  					  struct kvm_exit_mmio *mmio,
> >  					  phys_addr_t offset)
> >  {
> > -	u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_pending,
> > -				       vcpu->vcpu_id, offset);
> > +	u32 *level_active;
> > +	u32 *reg;
> > +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> > +
> > +	reg = vgic_bitmap_get_reg(&dist->irq_pending, vcpu->vcpu_id, offset);
> >  	vgic_reg_access(mmio, reg, offset,
> >  			ACCESS_READ_VALUE | ACCESS_WRITE_CLEARBIT);
> >  	if (mmio->is_write) {
> > +		/* Re-set level triggered level-active interrupts */
> I was confused by this comment ;-)
> compute new status_includes_pending taking into account wire state and
> GICD_ICPENDR?

so instead of modifying the register value that the guest writes
(because we'd have to consider byte-stores, halfword-stores, and
word-stores and such that vgic_bitmap_get_reg already handles for us),
we just clear the pending state regardless, but if it's a
level-triggered interrupt with the external input active, then the
interrupt needs to stay pending, so we just set those.  All this is
under a lock and happening atomically, so it should have the same effect
as an actual or-gate in hw.

> > +		level_active = vgic_bitmap_get_reg(&dist->irq_level,
> > +					  vcpu->vcpu_id, offset);
> > +		reg = vgic_bitmap_get_reg(&dist->irq_pending,
> > +					  vcpu->vcpu_id, offset);
> > +		*reg |= *level_active;
> OK, or between the wire and the GICD_ICPENDR
> > +
> > +		/* Clear soft-pending flags */
> > +		reg = vgic_bitmap_get_reg(&dist->irq_soft_pend,
> > +					  vcpu->vcpu_id, offset);
> > +		vgic_reg_access(mmio, reg, offset,
> > +				ACCESS_READ_VALUE | ACCESS_WRITE_CLEARBIT);
> only relevant for level-triggered IRQ but OK

in that case we're clearing already cleared bits, I thought the extra
logic would simply be confusing.

> > +
> >  		vgic_update_state(vcpu->kvm);
> >  		return true;
> >  	}
> > @@ -1187,15 +1258,29 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
> >  		for_each_set_bit(lr, (unsigned long *)vgic_cpu->vgic_eisr,
> >  				 vgic_cpu->nr_lr) {
> >  			irq = vgic_cpu->vgic_lr[lr] & GICH_LR_VIRTUALID;
> > +			BUG_ON(vgic_irq_is_edge(vcpu, irq));
> >  
> >  			vgic_irq_clear_queued(vcpu, irq);
> >  			vgic_cpu->vgic_lr[lr] &= ~GICH_LR_EOI;
> >  
> > +			/*
> > +			 * If the IRQ was EOIed it was most certainly also
> > +			 * ACKed and we can therefore always clear the soft
> > +			 * pending state (should it had been set) of this
> > +			 * interrupt.
> > +			 */
> > +			vgic_dist_irq_clear_soft_pend(vcpu, irq);
> what if the virq was Acked and ISPENDR was set after? Can't it happen?

hmm, yeah, I guess.

> Anyway since we do not trap the ACK, I guess we can't do better?

Right, basically the soft pending state would be set before or after the
ack, there is no way for us to know unless we start trapping ack's when
the soft pending flag is set (which would be the most architecturally
correct thing to do I suppose), but in practice I don't expect this to
be a problem.

I've added a note in the comments for v2.

> > +
> >  			/* Any additional pending interrupt? */
> > -			if (vgic_dist_irq_is_pending(vcpu, irq)) {
> > +			if (vgic_dist_irq_get_level(vcpu, irq)) {
> > +				/*
> > +				 * XXX: vgic_cpu_irq_set not always be true in
> > +				 * this case?
> > +				 */
> >  				vgic_cpu_irq_set(vcpu, irq);
> >  				level_pending = true;
> >  			} else {
> > +				vgic_dist_irq_clear_pending(vcpu, irq);
> >  				vgic_cpu_irq_clear(vcpu, irq);
> >  			}
> >  
> > @@ -1300,17 +1385,19 @@ static void vgic_kick_vcpus(struct kvm *kvm)
> >  static int vgic_validate_injection(struct kvm_vcpu *vcpu, int irq, int level)
> >  {
> >  	int edge_triggered = vgic_irq_is_edge(vcpu, irq);
> > -	int state = vgic_dist_irq_is_pending(vcpu, irq);
> >  
> >  	/*
> >  	 * Only inject an interrupt if:
> >  	 * - edge triggered and we have a rising edge
> >  	 * - level triggered and we change level
> >  	 */
> > -	if (edge_triggered)
> > +	if (edge_triggered) {
> > +		int state = vgic_dist_irq_is_pending(vcpu, irq);
> >  		return level > state;
> > -	else
> > +	} else {
> > +		int state = vgic_dist_irq_get_level(vcpu, irq);
> shouldn't we still compare against pending? What if soft pending happened?

we have to track all *updates* to the level state for
level-triggered interrupts, so this is basically just a shortcut-out if
nothing changed, which is why we only check against the existing level.

For example, consider state=0, soft_pend=1, pend=1, level=0, you don't
have to do anything, despite pend != level.

Do you have any counterexamples?

> >  		return level != state;
> > +	}
> >  }
> >  

Thanks,
-Christoffer

[RFC PATCH 5/6] arm/arm64: KVM: vgic: Fix SGI writes to GICD_I{CS}PENDR0

[Christoffer Dall]

Writes to GICD_ISPENDR0 and GICD_ICPENDR0 ignore all settings of the
pending state for SGIs.  Make sure the implementation handles this
correctly.

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

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 0b41875..1f91b3b 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -432,7 +432,7 @@ static bool handle_mmio_set_pending_reg(struct kvm_vcpu *vcpu,
 					struct kvm_exit_mmio *mmio,
 					phys_addr_t offset)
 {
-	u32 *reg;
+	u32 *reg, orig;
 	u32 level_mask;
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 
@@ -441,6 +441,7 @@ static bool handle_mmio_set_pending_reg(struct kvm_vcpu *vcpu,
 
 	/* Mark both level and edge triggered irqs as pending */
 	reg = vgic_bitmap_get_reg(&dist->irq_pending, vcpu->vcpu_id, offset);
+	orig = *reg;
 	vgic_reg_access(mmio, reg, offset,
 			ACCESS_READ_VALUE | ACCESS_WRITE_SETBIT);
 
@@ -452,6 +453,12 @@ static bool handle_mmio_set_pending_reg(struct kvm_vcpu *vcpu,
 				ACCESS_READ_VALUE | ACCESS_WRITE_SETBIT);
 		*reg &= level_mask;
 
+		/* Ignore writes to SGIs */
+		if (offset < 2) {
+			*reg &= ~0xffff;
+			*reg |= orig & 0xffff;
+		}
+
 		vgic_update_state(vcpu->kvm);
 		return true;
 	}
@@ -464,10 +471,11 @@ static bool handle_mmio_clear_pending_reg(struct kvm_vcpu *vcpu,
 					  phys_addr_t offset)
 {
 	u32 *level_active;
-	u32 *reg;
+	u32 *reg, orig;
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 
 	reg = vgic_bitmap_get_reg(&dist->irq_pending, vcpu->vcpu_id, offset);
+	orig = *reg;
 	vgic_reg_access(mmio, reg, offset,
 			ACCESS_READ_VALUE | ACCESS_WRITE_CLEARBIT);
 	if (mmio->is_write) {
@@ -478,6 +486,12 @@ static bool handle_mmio_clear_pending_reg(struct kvm_vcpu *vcpu,
 					  vcpu->vcpu_id, offset);
 		*reg |= *level_active;
 
+		/* Ignore writes to SGIs */
+		if (offset < 2) {
+			*reg &= ~0xffff;
+			*reg |= orig & 0xffff;
+		}
+
 		/* Clear soft-pending flags */
 		reg = vgic_bitmap_get_reg(&dist->irq_soft_pend,
 					  vcpu->vcpu_id, offset);
-- 
1.8.5.2

[RFC PATCH 6/6] arm/arm64: KVM: vgic: Clarify and correct vgic documentation

[Christoffer Dall]

The VGIC virtual distributor implementation documentation was written a
very long time ago, before the true nature of the beast had been
partially absorbed into my bloodstream.  I think this amalgamates the
two evil beings (myself and the code) a little more.

Plus, it fixes an actual bug.  ICFRn, pfff.

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

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 1f91b3b..cc776af 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -36,21 +36,22 @@
  * How the whole thing works (courtesy of Christoffer Dall):
  *
  * - At any time, the dist->irq_pending_on_cpu is the oracle that knows if
- *   something is pending
- * - VGIC pending interrupts are stored on the vgic.irq_pending vgic
- *   bitmap (this bitmap is updated by both user land ioctls and guest
- *   mmio ops, and other in-kernel peripherals such as the
- *   arch. timers) and indicate the 'wire' state.
+ *   something is pending on the CPU interface.
+ * - Interrupts that are pending on the distributor are stored on the
+ *   vgic.irq_pending vgic bitmap (this bitmap is updated by both user land
+ *   ioctls and guest mmio ops, and other in-kernel peripherals such as the
+ *   arch. timers).
  * - Every time the bitmap changes, the irq_pending_on_cpu oracle is
  *   recalculated
  * - To calculate the oracle, we need info for each cpu from
  *   compute_pending_for_cpu, which considers:
  *   - PPI: dist->irq_pending & dist->irq_enable
  *   - SPI: dist->irq_pending & dist->irq_enable & dist->irq_spi_target
- *   - irq_spi_target is a 'formatted' version of the GICD_ICFGR
+ *   - irq_spi_target is a 'formatted' version of the GICD_ITARGETSRn
  *     registers, stored on each vcpu. We only keep one bit of
  *     information per interrupt, making sure that only one vcpu can
  *     accept the interrupt.
+ * - If any of the above state changes, we must recalculate the oracle.
  * - The same is true when injecting an interrupt, except that we only
  *   consider a single interrupt at a time. The irq_spi_cpu array
  *   contains the target CPU for each SPI.
-- 
1.8.5.2

[RFC PATCH 6/6] arm/arm64: KVM: vgic: Clarify and correct vgic documentation

[Eric Auger]

On 06/14/2014 10:51 PM, Christoffer Dall wrote:
> The VGIC virtual distributor implementation documentation was written a
> very long time ago, before the true nature of the beast had been
> partially absorbed into my bloodstream.  I think this amalgamates the
> two evil beings (myself and the code) a little more.
> 
> Plus, it fixes an actual bug.  ICFRn, pfff.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  virt/kvm/arm/vgic.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 1f91b3b..cc776af 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -36,21 +36,22 @@
>   * How the whole thing works (courtesy of Christoffer Dall):
>   *
>   * - At any time, the dist->irq_pending_on_cpu is the oracle that knows if
> - *   something is pending
> - * - VGIC pending interrupts are stored on the vgic.irq_pending vgic
> - *   bitmap (this bitmap is updated by both user land ioctls and guest
> - *   mmio ops, and other in-kernel peripherals such as the
> - *   arch. timers) and indicate the 'wire' state.
> + *   something is pending on the CPU interface.
> + * - Interrupts that are pending on the distributor are stored on the
> + *   vgic.irq_pending vgic bitmap (this bitmap is updated by both user land
> + *   ioctls and guest mmio ops, and other in-kernel peripherals such as the
> + *   arch. timers).
ok forget my previous comment related to wire state;-)
>   * - Every time the bitmap changes, the irq_pending_on_cpu oracle is
>   *   recalculated
>   * - To calculate the oracle, we need info for each cpu from
>   *   compute_pending_for_cpu, which considers:
>   *   - PPI: dist->irq_pending & dist->irq_enable
>   *   - SPI: dist->irq_pending & dist->irq_enable & dist->irq_spi_target
> - *   - irq_spi_target is a 'formatted' version of the GICD_ICFGR
> + *   - irq_spi_target is a 'formatted' version of the GICD_ITARGETSRn
>   *     registers, stored on each vcpu. We only keep one bit of
>   *     information per interrupt, making sure that only one vcpu can
>   *     accept the interrupt.
> + * - If any of the above state changes, we must recalculate the oracle.
>   * - The same is true when injecting an interrupt, except that we only
>   *   consider a single interrupt at a time. The irq_spi_cpu array
>   *   contains the target CPU for each SPI.
>