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

[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 series tries to address some of these issues.

The code is also available here:
git://git.linaro.org/people/christoffer.dall/linux-kvm-arm.git vgic-set-clear-pending-v2

Changelog[v2]:
 - Rebased onto Marc's series from hell (kvm-arm/debug-merge-3.17)
 - Clarify comment in vgic_process_maintenance()
 - Fix spelling errors and other commit text blurbs
 - Actually tested it this time around (on TC2 and APM Mustang)

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    | 222 +++++++++++++++++++++++++++++++++++++------------
 2 files changed, 189 insertions(+), 57 deletions(-)

-- 
2.0.0

[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.

Acked-by: Marc Zyngier <marc.zyngier@arm.com>
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 35b0c12..388d442 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -140,8 +140,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 ede8f64..661fa64 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
@@ -221,21 +221,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)
@@ -409,7 +409,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);
@@ -425,7 +425,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);
@@ -651,7 +651,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, lr.irq);
+		vgic_dist_irq_set_pending(vcpu, lr.irq);
 		if (lr.irq < VGIC_NR_SGIS)
 			dist->irq_sgi_sources[vcpu_id][lr.irq] |= 1 << lr.source;
 		lr.state &= ~LR_STATE_PENDING;
@@ -932,7 +932,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);
 		}
@@ -952,11 +952,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,
@@ -1160,7 +1160,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;
 	}
@@ -1175,7 +1175,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);
@@ -1376,7 +1376,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);
 
 	/*
@@ -1384,26 +1384,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;
@@ -1418,9 +1418,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);
 
@@ -1429,7 +1429,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.
@@ -1466,7 +1466,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;
-- 
2.0.0

[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 388d442..7d8e61f 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -143,8 +143,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 661fa64..c5b1ad7 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.
  */
 
@@ -196,25 +196,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)
@@ -1079,8 +1079,8 @@ static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu)
 
 		if (!vgic_irq_is_enabled(vcpu, vlr.irq)) {
 			vgic_retire_lr(lr, vlr.irq, vcpu);
-			if (vgic_irq_is_active(vcpu, vlr.irq))
-				vgic_irq_clear_active(vcpu, vlr.irq);
+			if (vgic_irq_is_queued(vcpu, vlr.irq))
+				vgic_irq_clear_queued(vcpu, vlr.irq);
 		}
 	}
 }
@@ -1170,7 +1170,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)) {
@@ -1178,7 +1178,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;
@@ -1262,7 +1262,7 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
 		for_each_set_bit(lr, eisr_ptr, vgic->nr_lr) {
 			struct vgic_lr vlr = vgic_get_lr(vcpu, lr);
 
-			vgic_irq_clear_active(vcpu, vlr.irq);
+			vgic_irq_clear_queued(vcpu, vlr.irq);
 			WARN_ON(vlr.state & LR_STATE_MASK);
 			vlr.state = 0;
 			vgic_set_lr(vcpu, lr, vlr);
@@ -1429,7 +1429,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.
-- 
2.0.0

[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.

Acked-by: Marc Zyngier <marc.zyngier@arm.com>
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 c5b1ad7..1b85f42 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -662,8 +662,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.state & LR_STATE_MASK))
+		if (!(lr.state & LR_STATE_MASK)) {
 			vgic_retire_lr(i, lr.irq, vcpu);
+			vgic_irq_clear_queued(vcpu, lr.irq);
+		}
 
 		/* Finally update the VGIC state. */
 		vgic_update_state(vcpu->kvm);
-- 
2.0.0

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

[Christoffer Dall]

Writes to GICD_ISPENDRn and GICD_ICPENDRn are currently not handled
correctly for level-triggered interrupts.  The spec states that for
level-triggered interrupts, writes to the GICD_ISPENDRn activate the
output of a flip-flop which is in turn or'ed with the actual input
interrupt signal.  Correspondingly, writes to GICD_ICPENDRn simply
deactivates 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.  We therefore introduce two new variables on
the distributor struct to track these two states.  Astute readers may
notice that this is introducing more state than required (because an OR
of the two states gives you the pending state), but the 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.  Refactoring the code to consider the two state variables all the
places where we currently access the precomputed pending value, did not
look pretty.

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

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 7d8e61f..f074539 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -140,9 +140,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 1b85f42..7b0ab7f 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)
@@ -217,6 +222,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;
@@ -409,11 +449,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;
 	}
@@ -425,11 +480,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;
 	}
@@ -1263,17 +1334,36 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
 
 		for_each_set_bit(lr, eisr_ptr, vgic->nr_lr) {
 			struct vgic_lr vlr = vgic_get_lr(vcpu, lr);
+			WARN_ON(vgic_irq_is_edge(vcpu, vlr.irq));
 
 			vgic_irq_clear_queued(vcpu, vlr.irq);
 			WARN_ON(vlr.state & LR_STATE_MASK);
 			vlr.state = 0;
 			vgic_set_lr(vcpu, lr, vlr);
 
+			/*
+			 * If the IRQ was EOIed it was also ACKed and we we
+			 * therefore assume we can clear the soft pending
+			 * state (should it had been set) for this interrupt.
+			 *
+			 * Note: if the IRQ soft pending state was set after
+			 * the IRQ was acked, it actually shouldn't be
+			 * cleared, but we have no way of knowing that unless
+			 * we start trapping ACKs when the soft-pending state
+			 * is set.
+			 */
+			vgic_dist_irq_clear_soft_pend(vcpu, vlr.irq);
+
 			/* Any additional pending interrupt? */
-			if (vgic_dist_irq_is_pending(vcpu, vlr.irq)) {
+			if (vgic_dist_irq_get_level(vcpu, vlr.irq)) {
+				/*
+				 * XXX: vgic_cpu_irq_set not always be true in
+				 * this case?
+				 */
 				vgic_cpu_irq_set(vcpu, vlr.irq);
 				level_pending = true;
 			} else {
+				vgic_dist_irq_clear_pending(vcpu, vlr.irq);
 				vgic_cpu_irq_clear(vcpu, vlr.irq);
 			}
 
@@ -1379,17 +1469,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,
@@ -1419,10 +1511,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);
 
-- 
2.0.0

[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 7b0ab7f..2c13384 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -449,7 +449,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;
 
@@ -458,6 +458,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);
 
@@ -469,6 +470,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;
 	}
@@ -481,10 +488,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) {
@@ -495,6 +503,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);
-- 
2.0.0

[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.  Clarify the docs.

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 2c13384..df0785c 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.
-- 
2.0.0

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

[Marc Zyngier]

Hi Christoffer,

On Thu, Jul 10 2014 at  3:39:52 pm BST, Christoffer Dall <christoffer.dall@linaro.org> 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.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>

So to illustrate what I was going on about the first time you summitted
this patch, have a look at my below take on this. It is basically yours,
but just with the bitmap named "irq_can_sample", which is exactly what
this bitmap is about.

What do you think?

Thanks,

	M.

>From b6f864841878a5509e7d03581a224e270b25dd02 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <marc.zyngier@arm.com>
Date: Thu, 14 Aug 2014 13:14:34 +0100
Subject: [PATCH] KVM: arm: vgic: rename irq_active to irq_can sample

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/kvm/arm_vgic.h |  4 ++--
 virt/kvm/arm/vgic.c    | 35 +++++++++++++++++++----------------
 2 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 35b0c12..385d771 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -143,8 +143,8 @@ struct vgic_dist {
 	/* Interrupt 'pin' level */
 	struct vgic_bitmap	irq_state;
 
-	/* Level-triggered interrupt in progress */
-	struct vgic_bitmap	irq_active;
+	/* Can we sample the pending bit to inject an interrupt? */
+	struct vgic_bitmap	irq_can_sample;
 
 	/* 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 73eba79..1dedf03 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_can_sample is cleared. As long as this bit is 0, 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 sets the corresponding bit in irq_can_sample. This allow the
  *   interrupt line to be sampled again.
  */
 
@@ -196,25 +196,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_can_sample(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_can_sample, vcpu->vcpu_id, irq);
 }
 
-static void vgic_irq_set_active(struct kvm_vcpu *vcpu, int irq)
+static void vgic_irq_allow_sample(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_can_sample, vcpu->vcpu_id, irq, 1);
 }
 
-static void vgic_irq_clear_active(struct kvm_vcpu *vcpu, int irq)
+static void vgic_irq_forbid_sample(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_can_sample, vcpu->vcpu_id, irq, 0);
 }
 
 static int vgic_dist_irq_is_pending(struct kvm_vcpu *vcpu, int irq)
@@ -1079,8 +1079,8 @@ static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu)
 
 		if (!vgic_irq_is_enabled(vcpu, vlr.irq)) {
 			vgic_retire_lr(lr, vlr.irq, vcpu);
-			if (vgic_irq_is_active(vcpu, vlr.irq))
-				vgic_irq_clear_active(vcpu, vlr.irq);
+			if (!vgic_irq_can_sample(vcpu, vlr.irq))
+				vgic_irq_allow_sample(vcpu, vlr.irq);
 		}
 	}
 }
@@ -1170,7 +1170,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_can_sample(vcpu, irq))
 		return true; /* level interrupt, already queued */
 
 	if (vgic_queue_irq(vcpu, 0, irq)) {
@@ -1178,7 +1178,7 @@ static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, int irq)
 			vgic_dist_irq_clear(vcpu, irq);
 			vgic_cpu_irq_clear(vcpu, irq);
 		} else {
-			vgic_irq_set_active(vcpu, irq);
+			vgic_irq_forbid_sample(vcpu, irq);
 		}
 
 		return true;
@@ -1252,8 +1252,8 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
 
 	if (status & INT_STATUS_EOI) {
 		/*
-		 * Some level interrupts have been EOIed. Clear their
-		 * active bit.
+		 * Some level interrupts have been EOIed. Allow them
+		 * to be resampled.
 		 */
 		u64 eisr = vgic_get_eisr(vcpu);
 		unsigned long *eisr_ptr = (unsigned long *)&eisr;
@@ -1262,7 +1262,7 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
 		for_each_set_bit(lr, eisr_ptr, vgic->nr_lr) {
 			struct vgic_lr vlr = vgic_get_lr(vcpu, lr);
 
-			vgic_irq_clear_active(vcpu, vlr.irq);
+			vgic_irq_allow_sample(vcpu, vlr.irq);
 			WARN_ON(vlr.state & LR_STATE_MASK);
 			vlr.state = 0;
 			vgic_set_lr(vcpu, lr, vlr);
@@ -1429,7 +1429,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 (is_level && !vgic_irq_can_sample(vcpu, irq_num)) {
 		/*
 		 * Level interrupt in progress, will be picked up
 		 * when EOId.
@@ -1506,6 +1506,9 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
 		if (i < VGIC_NR_PRIVATE_IRQS)
 			vgic_bitmap_set_irq_val(&dist->irq_cfg,
 						vcpu->vcpu_id, i, VGIC_CFG_EDGE);
+		/* Let vcpu0 also allow sampling of SPIs */
+		if (i < VGIC_NR_PRIVATE_IRQS || vcpu->vcpu_id == 0)
+			vgic_irq_allow_sample(vcpu, i);
 
 		vgic_cpu->vgic_irq_lr_map[i] = LR_EMPTY;
 	}
-- 
2.0.0


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

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

[Marc Zyngier]

On Thu, Aug 14 2014 at  1:18:44 pm BST, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Hi Christoffer,
>
> On Thu, Jul 10 2014 at 3:39:52 pm BST, Christoffer Dall
> <christoffer.dall@linaro.org> 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.
>>
>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>
> So to illustrate what I was going on about the first time you summitted
> this patch, have a look at my below take on this. It is basically yours,
> but just with the bitmap named "irq_can_sample", which is exactly what
> this bitmap is about.
>
> What do you think?
>
> Thanks,
>
> 	M.
>
> From b6f864841878a5509e7d03581a224e270b25dd02 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <marc.zyngier@arm.com>
> Date: Thu, 14 Aug 2014 13:14:34 +0100
> Subject: [PATCH] KVM: arm: vgic: rename irq_active to irq_can sample
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  include/kvm/arm_vgic.h |  4 ++--
>  virt/kvm/arm/vgic.c    | 35 +++++++++++++++++++----------------
>  2 files changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 35b0c12..385d771 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -143,8 +143,8 @@ struct vgic_dist {
>  	/* Interrupt 'pin' level */
>  	struct vgic_bitmap	irq_state;
>  
> -	/* Level-triggered interrupt in progress */
> -	struct vgic_bitmap	irq_active;
> +	/* Can we sample the pending bit to inject an interrupt? */
> +	struct vgic_bitmap	irq_can_sample;
>  
>  	/* 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 73eba79..1dedf03 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_can_sample is cleared. As long as this bit is 0, 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 sets the corresponding bit in irq_can_sample. This allow the
>   *   interrupt line to be sampled again.
>   */
>  
> @@ -196,25 +196,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_can_sample(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_can_sample, vcpu->vcpu_id, irq);
>  }
>  
> -static void vgic_irq_set_active(struct kvm_vcpu *vcpu, int irq)
> +static void vgic_irq_allow_sample(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_can_sample, vcpu->vcpu_id, irq, 1);
>  }
>  
> -static void vgic_irq_clear_active(struct kvm_vcpu *vcpu, int irq)
> +static void vgic_irq_forbid_sample(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_can_sample, vcpu->vcpu_id, irq, 0);
>  }
>  
>  static int vgic_dist_irq_is_pending(struct kvm_vcpu *vcpu, int irq)
> @@ -1079,8 +1079,8 @@ static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu)
>  
>  		if (!vgic_irq_is_enabled(vcpu, vlr.irq)) {
>  			vgic_retire_lr(lr, vlr.irq, vcpu);
> -			if (vgic_irq_is_active(vcpu, vlr.irq))
> -				vgic_irq_clear_active(vcpu, vlr.irq);
> +			if (!vgic_irq_can_sample(vcpu, vlr.irq))
> +				vgic_irq_allow_sample(vcpu, vlr.irq);
>  		}
>  	}
>  }
> @@ -1170,7 +1170,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_can_sample(vcpu, irq))
>  		return true; /* level interrupt, already queued */
>  
>  	if (vgic_queue_irq(vcpu, 0, irq)) {
> @@ -1178,7 +1178,7 @@ static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, int irq)
>  			vgic_dist_irq_clear(vcpu, irq);
>  			vgic_cpu_irq_clear(vcpu, irq);
>  		} else {
> -			vgic_irq_set_active(vcpu, irq);
> +			vgic_irq_forbid_sample(vcpu, irq);
>  		}
>  
>  		return true;
> @@ -1252,8 +1252,8 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
>  
>  	if (status & INT_STATUS_EOI) {
>  		/*
> -		 * Some level interrupts have been EOIed. Clear their
> -		 * active bit.
> +		 * Some level interrupts have been EOIed. Allow them
> +		 * to be resampled.
>  		 */
>  		u64 eisr = vgic_get_eisr(vcpu);
>  		unsigned long *eisr_ptr = (unsigned long *)&eisr;
> @@ -1262,7 +1262,7 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
>  		for_each_set_bit(lr, eisr_ptr, vgic->nr_lr) {
>  			struct vgic_lr vlr = vgic_get_lr(vcpu, lr);
>  
> -			vgic_irq_clear_active(vcpu, vlr.irq);
> +			vgic_irq_allow_sample(vcpu, vlr.irq);
>  			WARN_ON(vlr.state & LR_STATE_MASK);
>  			vlr.state = 0;
>  			vgic_set_lr(vcpu, lr, vlr);
> @@ -1429,7 +1429,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 (is_level && !vgic_irq_can_sample(vcpu, irq_num)) {
>  		/*
>  		 * Level interrupt in progress, will be picked up
>  		 * when EOId.
> @@ -1506,6 +1506,9 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>  		if (i < VGIC_NR_PRIVATE_IRQS)
>  			vgic_bitmap_set_irq_val(&dist->irq_cfg,
>  						vcpu->vcpu_id, i, VGIC_CFG_EDGE);
> +		/* Let vcpu0 also allow sampling of SPIs */
> +		if (i < VGIC_NR_PRIVATE_IRQS || vcpu->vcpu_id == 0)
> +			vgic_irq_allow_sample(vcpu, i);
>  
>  		vgic_cpu->vgic_irq_lr_map[i] = LR_EMPTY;
>  	}
> -- 
> 2.0.0

The astute reader will have noticed that this change allows for further
cleanups, such as this:

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 1dedf03..327588d 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1395,15 +1395,12 @@ static bool vgic_update_irq_state(struct kvm *kvm, int cpuid,
 {
        struct vgic_dist *dist = &kvm->arch.vgic;
        struct kvm_vcpu *vcpu;
-       int is_edge, is_level;
        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;
 
        if (!vgic_validate_injection(vcpu, irq_num, level)) {
                ret = false;
@@ -1429,7 +1426,7 @@ static bool vgic_update_irq_state(struct kvm *kvm, int cpuid,
                goto out;
        }
 
-       if (is_level && !vgic_irq_can_sample(vcpu, irq_num)) {
+       if (!vgic_irq_can_sample(vcpu, irq_num)) {
                /*
                 * Level interrupt in progress, will be picked up
                 * when EOId.


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

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

[Marc Zyngier]

On Thu, Jul 10 2014 at  3:39:54 pm BST, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> Writes to GICD_ISPENDRn and GICD_ICPENDRn are currently not handled
> correctly for level-triggered interrupts.  The spec states that for
> level-triggered interrupts, writes to the GICD_ISPENDRn activate the
> output of a flip-flop which is in turn or'ed with the actual input
> interrupt signal.  Correspondingly, writes to GICD_ICPENDRn simply
> deactivates 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.  We therefore introduce two new variables on
> the distributor struct to track these two states.  Astute readers may
> notice that this is introducing more state than required (because an OR
> of the two states gives you the pending state), but the 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.  Refactoring the code to consider the two state variables all the
> places where we currently access the precomputed pending value, did not
> look pretty.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  include/kvm/arm_vgic.h |  16 ++++++-
>  virt/kvm/arm/vgic.c    | 123 ++++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 127 insertions(+), 12 deletions(-)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 7d8e61f..f074539 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -140,9 +140,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 1b85f42..7b0ab7f 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)
> @@ -217,6 +222,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;
> @@ -409,11 +449,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;

I'm a bit confused here. You're doing the MMIO access, and then updating
the register again?

> +
>  		vgic_update_state(vcpu->kvm);
>  		return true;
>  	}
> @@ -425,11 +480,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;

So here we're updating the main pending bitmap with what comes from the
wires...

> +		/* 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);
> +

... and then clearing the soft bits only. But we're supposed to have
"pending = wire | soft" at any time, aren't we? Here, I read "pending =
wire", and the soft bits not having influence on pending.

I'm probably missing something obvious...

>  		vgic_update_state(vcpu->kvm);
>  		return true;
>  	}
> @@ -1263,17 +1334,36 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
>  
>  		for_each_set_bit(lr, eisr_ptr, vgic->nr_lr) {
>  			struct vgic_lr vlr = vgic_get_lr(vcpu, lr);
> +			WARN_ON(vgic_irq_is_edge(vcpu, vlr.irq));
>  
>  			vgic_irq_clear_queued(vcpu, vlr.irq);
>  			WARN_ON(vlr.state & LR_STATE_MASK);
>  			vlr.state = 0;
>  			vgic_set_lr(vcpu, lr, vlr);
>  
> +			/*
> +			 * If the IRQ was EOIed it was also ACKed and we we
> +			 * therefore assume we can clear the soft pending
> +			 * state (should it had been set) for this interrupt.
> +			 *
> +			 * Note: if the IRQ soft pending state was set after
> +			 * the IRQ was acked, it actually shouldn't be
> +			 * cleared, but we have no way of knowing that unless
> +			 * we start trapping ACKs when the soft-pending state
> +			 * is set.
> +			 */
> +			vgic_dist_irq_clear_soft_pend(vcpu, vlr.irq);
> +
>  			/* Any additional pending interrupt? */
> -			if (vgic_dist_irq_is_pending(vcpu, vlr.irq)) {
> +			if (vgic_dist_irq_get_level(vcpu, vlr.irq)) {
> +				/*
> +				 * XXX: vgic_cpu_irq_set not always be true in
> +				 * this case?
> +				 */
>  				vgic_cpu_irq_set(vcpu, vlr.irq);
>  				level_pending = true;
>  			} else {
> +				vgic_dist_irq_clear_pending(vcpu, vlr.irq);
>  				vgic_cpu_irq_clear(vcpu, vlr.irq);
>  			}
>  
> @@ -1379,17 +1469,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,
> @@ -1419,10 +1511,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);

Overall, I find the change a bit confusing. I understand the need to
maintain two different bitmaps for wire and soft, but I don't really get
the logic that you've introduced here... Maybe I should try more coffee
or something? ;-)

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

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

[Marc Zyngier]

On Thu, Jul 10 2014 at  3:39:56 pm BST, Christoffer Dall <christoffer.dall@linaro.org> 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.  Clarify the docs.
>
> Plus, it fixes an actual bug.  ICFRn, pfff.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>

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

> ---
>  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 2c13384..df0785c 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.

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

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

[Christoffer Dall]

On Thu, Aug 14, 2014 at 03:43:11PM +0100, Marc Zyngier wrote:
> On Thu, Jul 10 2014 at  3:39:54 pm BST, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> > Writes to GICD_ISPENDRn and GICD_ICPENDRn are currently not handled
> > correctly for level-triggered interrupts.  The spec states that for
> > level-triggered interrupts, writes to the GICD_ISPENDRn activate the
> > output of a flip-flop which is in turn or'ed with the actual input
> > interrupt signal.  Correspondingly, writes to GICD_ICPENDRn simply
> > deactivates 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.  We therefore introduce two new variables on
> > the distributor struct to track these two states.  Astute readers may
> > notice that this is introducing more state than required (because an OR
> > of the two states gives you the pending state), but the 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.  Refactoring the code to consider the two state variables all the
> > places where we currently access the precomputed pending value, did not
> > look pretty.
> >
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  include/kvm/arm_vgic.h |  16 ++++++-
> >  virt/kvm/arm/vgic.c    | 123 ++++++++++++++++++++++++++++++++++++++++++++-----
> >  2 files changed, 127 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> > index 7d8e61f..f074539 100644
> > --- a/include/kvm/arm_vgic.h
> > +++ b/include/kvm/arm_vgic.h
> > @@ -140,9 +140,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 1b85f42..7b0ab7f 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)
> > @@ -217,6 +222,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;
> > @@ -409,11 +449,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;
> 
> I'm a bit confused here. You're doing the MMIO access, and then updating
> the register again?
> 

not quite, notice that I change the reg pointer to the soft_pending flag
if we're doing a write.  I then reuse the functionality in
vgic_reg_access to figure out the right offsets, mmio len access etc.,
to write the soft bits for all the bits set in the register passed to
the store function by the guest, but then I use the level_mask variable
to clear the bits for the level-triggered interrupts.

This can be rewritten to be slightly more straight forward, but at the
cost of code duplication and an increased code length.

> > +
> >  		vgic_update_state(vcpu->kvm);
> >  		return true;
> >  	}
> > @@ -425,11 +480,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;
> 
> So here we're updating the main pending bitmap with what comes from the
> wires...
> 

correct

> > +		/* 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);
> > +
> 
> ... and then clearing the soft bits only. But we're supposed to have
> "pending = wire | soft" at any time, aren't we? Here, I read "pending =
> wire", and the soft bits not having influence on pending.
> 
> I'm probably missing something obvious...
> 

I'm not sure it's obvious, but:

1) we clear the irq_pending bit for the set bits in the guest source
register (on the first vgic_reg_access, before the if (mmio->is_write))

2) we may now have cleared bits that shouldn't actually be cleared,
because we need to or it with the wire state, so

3) fetch the wire state

4) set the irq_pending bit for those with an active wire state (*reg |=
*level_active)

5) now we deal with the soft pending flags, which should simply be
cleared according tot he source register

Your question on soft bits not having influence on pending can be
answered by saying, "this is a write to the clear register for the soft
state, so the soft state for all relevant irqs we are dealing with here
is always 0 (we are clearing it right now), so

  pending = wire | soft && soft == 0   =>
  pending = wire

  for those interrupts addressed with bits set in the source register.


does this help?

> >  		vgic_update_state(vcpu->kvm);
> >  		return true;
> >  	}
> > @@ -1263,17 +1334,36 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
> >  
> >  		for_each_set_bit(lr, eisr_ptr, vgic->nr_lr) {
> >  			struct vgic_lr vlr = vgic_get_lr(vcpu, lr);
> > +			WARN_ON(vgic_irq_is_edge(vcpu, vlr.irq));
> >  
> >  			vgic_irq_clear_queued(vcpu, vlr.irq);
> >  			WARN_ON(vlr.state & LR_STATE_MASK);
> >  			vlr.state = 0;
> >  			vgic_set_lr(vcpu, lr, vlr);
> >  
> > +			/*
> > +			 * If the IRQ was EOIed it was also ACKed and we we
> > +			 * therefore assume we can clear the soft pending
> > +			 * state (should it had been set) for this interrupt.
> > +			 *
> > +			 * Note: if the IRQ soft pending state was set after
> > +			 * the IRQ was acked, it actually shouldn't be
> > +			 * cleared, but we have no way of knowing that unless
> > +			 * we start trapping ACKs when the soft-pending state
> > +			 * is set.
> > +			 */
> > +			vgic_dist_irq_clear_soft_pend(vcpu, vlr.irq);
> > +
> >  			/* Any additional pending interrupt? */
> > -			if (vgic_dist_irq_is_pending(vcpu, vlr.irq)) {
> > +			if (vgic_dist_irq_get_level(vcpu, vlr.irq)) {
> > +				/*
> > +				 * XXX: vgic_cpu_irq_set not always be true in
> > +				 * this case?
> > +				 */
> >  				vgic_cpu_irq_set(vcpu, vlr.irq);
> >  				level_pending = true;
> >  			} else {
> > +				vgic_dist_irq_clear_pending(vcpu, vlr.irq);
> >  				vgic_cpu_irq_clear(vcpu, vlr.irq);
> >  			}
> >  
> > @@ -1379,17 +1469,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,
> > @@ -1419,10 +1511,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);
> 
> Overall, I find the change a bit confusing. I understand the need to
> maintain two different bitmaps for wire and soft, but I don't really get
> the logic that you've introduced here... Maybe I should try more coffee
> or something? ;-)

Is this comment targeted at the changes to vgic_update_irq_pending, or
the whole patch?

For the whole patch, and specifically the mmio accessors, I started out
trying to write the code more straight-forward, trying to factor out
some stuff from vgic_bitmap_get_reg(), but it became really ugly and
very long and really easier to folllow.  I can try to rewrite the whole
thing again, but I really thought the more verbose version of the mmio
accessors was more difficult to follow.

-Christoffer

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

[Christoffer Dall]

On Thu, Aug 14, 2014 at 01:18:44PM +0100, Marc Zyngier wrote:
> Hi Christoffer,
> 
> On Thu, Jul 10 2014 at  3:39:52 pm BST, Christoffer Dall <christoffer.dall@linaro.org> 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.
> >
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> 
> So to illustrate what I was going on about the first time you summitted
> this patch, have a look at my below take on this. It is basically yours,
> but just with the bitmap named "irq_can_sample", which is exactly what
> this bitmap is about.
> 
> What do you think?

I find this more difficult to understand, because it prompts me to ask
the question in my head "why is it that we forbid sampling of the irq
line in some situations, and when is it that we set that bit?".

What I tried to say with my "irq_queued" suggestion is simply "the irq
in now on a LR, so wait until we hear something else from that LR before
we consider the external input again, so "level_irq_on_lr" as another
suggestion would precisely indicate when this bit is set or not,
regardless of having a true understanding of how the hardware works and
how we emulate it.

Maybe I have two neurons that need help to conenct with each other to
understand the can_sample stuff.

Grumble grumble....

[...]

> @@ -1429,7 +1429,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 (is_level && !vgic_irq_can_sample(vcpu, irq_num)) {
>  		/*
>  		 * Level interrupt in progress, will be picked up
>  		 * when EOId.
> @@ -1506,6 +1506,9 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>  		if (i < VGIC_NR_PRIVATE_IRQS)
>  			vgic_bitmap_set_irq_val(&dist->irq_cfg,
>  						vcpu->vcpu_id, i, VGIC_CFG_EDGE);
> +		/* Let vcpu0 also allow sampling of SPIs */

huh, what does this comment mean?  Isn't the vcpu_id == 0 thingy above
to catch all SPIs, not related specifically to vcpu0 and just to avoid
setting the same bits in the shared bitmap for all vcpus?

> +		if (i < VGIC_NR_PRIVATE_IRQS || vcpu->vcpu_id == 0)
> +			vgic_irq_allow_sample(vcpu, i);
>  
>  		vgic_cpu->vgic_irq_lr_map[i] = LR_EMPTY;
>  	}
> -- 
> 2.0.0
> 
> 
> -- 
> Jazz is not dead. It just smells funny.

Thanks :)
-Christoffer