[PATCH v3 0/5] Handle forwarded level-triggered interrupts

[PATCH v3 0/5] Handle forwarded level-triggered interrupts

[Christoffer Dall]

This series illustrates an alternative approach to Eric Auger's direct EOI
setup patches [1] in terms of the KVM VGIC support.

The idea is to maintain existing semantics for the VGIC for mapped
level-triggered IRQs and think support for the timer into it.

Patch 1 is necessary to align the timer and VFIO ways of signaling the
VGIC.  Patch 2 is stolen from Eric's series and is necessary for these
patches to compile as well.  Patch 3 includes the core support for
mapped level-triggered interrupts.  Patch 4 handles guest MMIO access to
the virtual distributor.  Patch 5 moves some code around for patch 6.
Patch 6 implements an optimization for the timer.  The last two patches
could be deferred until the timer optimization series.

Based on v4.13

Changes since v2:
 - Removed patch 5 from v2 and integrating the changes in what's now
   patch 5 to make it easier to reuse code when adding VFIO integration.
 - Changed the virtual distributor MMIO handling to use the
   pending_latch and more closely match the semantics of SPENDR and
   CPENDR for both level and edge mapped interrupts.

Changes since v1:
 - Added necessary changes to the timer (Patch 1)
 - Added handling of guest MMIO accesses to the virtual distributor
   (Patch 4)
 - Addressed Marc's comments from the initial RFC (mostly renames)

Thanks,
-Christoffer

---

Christoffer Dall (4):
  KVM: arm/arm64: Don't cache the timer IRQ level
  KVM: arm/arm64: vgic: Support level-triggered mapped interrupts
  KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs
  KVM: arm/arm64: Provide a vgic interrupt line level sample function

Eric Auger (1):
  KVM: arm/arm64: vgic: restructure kvm_vgic_(un)map_phys_irq

 include/kvm/arm_vgic.h        | 19 +++++++--
 virt/kvm/arm/arch_timer.c     | 52 +++++++++++------------
 virt/kvm/arm/vgic/vgic-mmio.c | 33 +++++++++++++++
 virt/kvm/arm/vgic/vgic-v2.c   | 29 +++++++++++++
 virt/kvm/arm/vgic/vgic-v3.c   | 29 +++++++++++++
 virt/kvm/arm/vgic/vgic.c      | 96 ++++++++++++++++++++++++++++++++++++-------
 virt/kvm/arm/vgic/vgic.h      |  8 ++++
 7 files changed, 219 insertions(+), 47 deletions(-)

-- 
2.9.0

[PATCH v3 1/5] KVM: arm/arm64: Don't cache the timer IRQ level

[Christoffer Dall]

The timer was modeled after a strict idea of modelling an interrupt line
level in software, meaning that only transitions in the level needed to
be reported to the VGIC.  This works well for the timer, because the
arch timer code is in complete control of the device and can track the
transitions of the line.

However, as we are about to support using the HW bit in the VGIC not
just for the timer, but also for VFIO which cannot track transitions of
the interrupt line, we have to decide on an interface for level
triggered mapped interrupts to the GIC, which both the timer and VFIO
can use.

VFIO only sees an asserting transition of the physical interrupt line,
and tells the VGIC when that happens.  That means that part of the
interrupt flow is offloaded to the hardware.

To use the same interface for VFIO devices and the timer, we therefore
have to change the timer (we cannot change VFIO because it doesn't know
the details of the device it is assigning to a VM).

Luckily, changing the timer is simple, we just need to stop 'caching'
the line level, but instead let the VGIC know the state of the timer on
every entry to the guest, and the VGIC can ignore notifications using
its validate mechanism.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 virt/kvm/arm/arch_timer.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 8e89d63..2a5f877 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -219,9 +219,10 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
 	int ret;
 
 	timer_ctx->active_cleared_last = false;
+	if (timer_ctx->irq.level != new_level)
+		trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
+					   new_level);
 	timer_ctx->irq.level = new_level;
-	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
-				   timer_ctx->irq.level);
 
 	if (likely(irqchip_in_kernel(vcpu->kvm))) {
 		ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
@@ -241,6 +242,7 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
 	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
+	bool level;
 
 	/*
 	 * If userspace modified the timer registers via SET_ONE_REG before
@@ -251,11 +253,11 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
 	if (unlikely(!timer->enabled))
 		return;
 
-	if (kvm_timer_should_fire(vtimer) != vtimer->irq.level)
-		kvm_timer_update_irq(vcpu, !vtimer->irq.level, vtimer);
+	level = kvm_timer_should_fire(vtimer);
+	kvm_timer_update_irq(vcpu, level, vtimer);
 
-	if (kvm_timer_should_fire(ptimer) != ptimer->irq.level)
-		kvm_timer_update_irq(vcpu, !ptimer->irq.level, ptimer);
+	level = kvm_timer_should_fire(ptimer);
+	kvm_timer_update_irq(vcpu, level, ptimer);
 }
 
 /* Schedule the background timer for the emulated timer. */
-- 
2.9.0

[PATCH v3 2/5] KVM: arm/arm64: vgic: restructure kvm_vgic_(un)map_phys_irq

[Christoffer Dall]

From: Eric Auger <eric.auger@redhat.com>

We want to reuse the core of the map/unmap functions for IRQ forwarding.
Let's move the computation of the hwirq in kvm_vgic_map_phys_irq and
pass the linux IRQ as parameter.

The host_irq is added to struct vgic_irq because it is needed in later
patches which manipulate the physical GIC state to support forwarded
IRQs.

We introduce kvm_vgic_map/unmap_irq which take a struct vgic_irq handle
as a parameter.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Christoffer Dall <cdall@linaro.org>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/kvm/arm_vgic.h    |  8 ++++---
 virt/kvm/arm/arch_timer.c | 24 +------------------
 virt/kvm/arm/vgic/vgic.c  | 60 +++++++++++++++++++++++++++++++++++------------
 3 files changed, 51 insertions(+), 41 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 34dba51..53f631b 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -116,6 +116,7 @@ struct vgic_irq {
 	bool hw;			/* Tied to HW IRQ */
 	struct kref refcount;		/* Used for LPIs */
 	u32 hwintid;			/* HW INTID number */
+	unsigned int host_irq;		/* linux irq corresponding to hwintid */
 	union {
 		u8 targets;			/* GICv2 target VCPUs mask */
 		u32 mpidr;			/* GICv3 target VCPU */
@@ -307,9 +308,10 @@ void kvm_vgic_init_cpu_hardware(void);
 
 int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
 			bool level, void *owner);
-int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq);
-int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq);
-bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
+int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
+			  u32 vintid);
+int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int vintid);
+bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int vintid);
 
 int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
 
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 2a5f877..c4fa675 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -649,9 +649,6 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
 	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
-	struct irq_desc *desc;
-	struct irq_data *data;
-	int phys_irq;
 	int ret;
 
 	if (timer->enabled)
@@ -669,26 +666,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
 		return -EINVAL;
 	}
 
-	/*
-	 * Find the physical IRQ number corresponding to the host_vtimer_irq
-	 */
-	desc = irq_to_desc(host_vtimer_irq);
-	if (!desc) {
-		kvm_err("%s: no interrupt descriptor\n", __func__);
-		return -EINVAL;
-	}
-
-	data = irq_desc_get_irq_data(desc);
-	while (data->parent_data)
-		data = data->parent_data;
-
-	phys_irq = data->hwirq;
-
-	/*
-	 * Tell the VGIC that the virtual interrupt is tied to a
-	 * physical interrupt. We do that once per VCPU.
-	 */
-	ret = kvm_vgic_map_phys_irq(vcpu, vtimer->irq.irq, phys_irq);
+	ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq, vtimer->irq.irq);
 	if (ret)
 		return ret;
 
diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index fed717e..9d557efd 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -17,6 +17,8 @@
 #include <linux/kvm.h>
 #include <linux/kvm_host.h>
 #include <linux/list_sort.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
 
 #include "vgic.h"
 
@@ -403,38 +405,66 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
 	return 0;
 }
 
-int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq)
+/* @irq->irq_lock must be held */
+static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
+			    unsigned int host_irq)
 {
-	struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, virt_irq);
+	struct irq_desc *desc;
+	struct irq_data *data;
 
-	BUG_ON(!irq);
-
-	spin_lock(&irq->irq_lock);
+	/*
+	 * Find the physical IRQ number corresponding to @host_irq
+	 */
+	desc = irq_to_desc(host_irq);
+	if (!desc) {
+		kvm_err("%s: no interrupt descriptor\n", __func__);
+		return -EINVAL;
+	}
+	data = irq_desc_get_irq_data(desc);
+	while (data->parent_data)
+		data = data->parent_data;
 
 	irq->hw = true;
-	irq->hwintid = phys_irq;
+	irq->host_irq = host_irq;
+	irq->hwintid = data->hwirq;
+	return 0;
+}
+
+/* @irq->irq_lock must be held */
+static inline void kvm_vgic_unmap_irq(struct vgic_irq *irq)
+{
+	irq->hw = false;
+	irq->hwintid = 0;
+}
+
+int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
+			  u32 vintid)
+{
+	struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, vintid);
+	int ret;
 
+	BUG_ON(!irq);
+
+	spin_lock(&irq->irq_lock);
+	ret = kvm_vgic_map_irq(vcpu, irq, host_irq);
 	spin_unlock(&irq->irq_lock);
 	vgic_put_irq(vcpu->kvm, irq);
 
-	return 0;
+	return ret;
 }
 
-int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq)
+int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int vintid)
 {
 	struct vgic_irq *irq;
 
 	if (!vgic_initialized(vcpu->kvm))
 		return -EAGAIN;
 
-	irq = vgic_get_irq(vcpu->kvm, vcpu, virt_irq);
+	irq = vgic_get_irq(vcpu->kvm, vcpu, vintid);
 	BUG_ON(!irq);
 
 	spin_lock(&irq->irq_lock);
-
-	irq->hw = false;
-	irq->hwintid = 0;
-
+	kvm_vgic_unmap_irq(irq);
 	spin_unlock(&irq->irq_lock);
 	vgic_put_irq(vcpu->kvm, irq);
 
@@ -772,9 +802,9 @@ void vgic_kick_vcpus(struct kvm *kvm)
 	}
 }
 
-bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq)
+bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int vintid)
 {
-	struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, virt_irq);
+	struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, vintid);
 	bool map_is_active;
 
 	spin_lock(&irq->irq_lock);
-- 
2.9.0

[PATCH v3 3/5] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

[Christoffer Dall]

Level-triggered mapped IRQs are special because we only observe rising
edges as input to the VGIC, and we don't set the EOI flag and therefore
are not told when the level goes down, so that we can re-queue a new
interrupt when the level goes up.

One way to solve this problem is to side-step the logic of the VGIC and
special case the validation in the injection path, but it has the
unfortunate drawback of having to peak into the physical GIC state
whenever we want to know if the interrupt is pending on the virtual
distributor.

Instead, we can maintain the current semantics of a level triggered
interrupt by sort of treating it as an edge-triggered interrupt,
following from the fact that we only observe an asserting edge.  This
requires us to be a bit careful when populating the LRs and when folding
the state back in though:

 * We lower the line level when populating the LR, so that when
   subsequently observing an asserting edge, the VGIC will do the right
   thing.

 * If the guest never acked the interrupt while running (for example if
   it had masked interrupts at the CPU level while running), we have
   to preserve the pending state of the LR and move it back to the
   line_level field of the struct irq when folding LR state.

   If the guest never acked the interrupt while running, but changed the
   device state and lowered the line (again with interrupts masked) then
   we need to observe this change in the line_level.

   Both of the above situations are solved by sampling the physical line
   and set the line level when folding the LR back.

 * Finally, if the guest never acked the interrupt while running and
   sampling the line reveals that the device state has changed and the
   line has been lowered, we must clear the physical active state, since
   we will otherwise never be told when the interrupt becomes asserted
   again.

This has the added benefit of making the timer optimization patches
(https://lists.cs.columbia.edu/pipermail/kvmarm/2017-July/026343.html) a
bit simpler, because the timer code doesn't have to clear the active
state on the sync anymore.  It also potentially improves the performance
of the timer implementation because the GIC knows the state or the LR
and only needs to clear the
active state when the pending bit in the LR is still set, where the
timer has to always clear it when returning from running the guest with
an injected timer interrupt.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-v2.c | 29 +++++++++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic-v3.c | 29 +++++++++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic.c    | 23 +++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic.h    |  7 +++++++
 4 files changed, 88 insertions(+)

diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index e4187e5..618ed3f 100644
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -104,6 +104,26 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
 				irq->pending_latch = false;
 		}
 
+		/*
+		 * Level-triggered mapped IRQs are special because we only
+		 * observe rising edges as input to the VGIC.
+		 *
+		 * If the guest never acked the interrupt we have to sample
+		 * the physical line and set the line level, because the
+		 * device state could have changed or we simply need to
+		 * process the still pending interrupt later.
+		 *
+		 * If this causes us to lower the level, we have to also clear
+		 * the physical active state, since we will otherwise never be
+		 * told when the interrupt becomes asserted again.
+		 */
+		if (vgic_irq_is_mapped_level(irq) && (val & GICH_LR_PENDING_BIT)) {
+			irq->line_level = vgic_get_phys_line_level(irq);
+
+			if (!irq->line_level)
+				vgic_irq_set_phys_active(irq, false);
+		}
+
 		spin_unlock(&irq->irq_lock);
 		vgic_put_irq(vcpu->kvm, irq);
 	}
@@ -161,6 +181,15 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
 			val |= GICH_LR_EOI;
 	}
 
+	/*
+	 * Level-triggered mapped IRQs are special because we only observe
+	 * rising edges as input to the VGIC.  We therefore lower the line
+	 * level here, so that we can take new virtual IRQs.  See
+	 * vgic_v2_fold_lr_state for more info.
+	 */
+	if (vgic_irq_is_mapped_level(irq) && (val & GICH_LR_PENDING_BIT))
+		irq->line_level = false;
+
 	/* The GICv2 LR only holds five bits of priority. */
 	val |= (irq->priority >> 3) << GICH_LR_PRIORITY_SHIFT;
 
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 96ea597..0f72bcb 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -94,6 +94,26 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
 				irq->pending_latch = false;
 		}
 
+		/*
+		 * Level-triggered mapped IRQs are special because we only
+		 * observe rising edges as input to the VGIC.
+		 *
+		 * If the guest never acked the interrupt we have to sample
+		 * the physical line and set the line level, because the
+		 * device state could have changed or we simply need to
+		 * process the still pending interrupt later.
+		 *
+		 * If this causes us to lower the level, we have to also clear
+		 * the physical active state, since we will otherwise never be
+		 * told when the interrupt becomes asserted again.
+		 */
+		if (vgic_irq_is_mapped_level(irq) && (val & ICH_LR_PENDING_BIT)) {
+			irq->line_level = vgic_get_phys_line_level(irq);
+
+			if (!irq->line_level)
+				vgic_irq_set_phys_active(irq, false);
+		}
+
 		spin_unlock(&irq->irq_lock);
 		vgic_put_irq(vcpu->kvm, irq);
 	}
@@ -144,6 +164,15 @@ void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
 	}
 
 	/*
+	 * Level-triggered mapped IRQs are special because we only observe
+	 * rising edges as input to the VGIC.  We therefore lower the line
+	 * level here, so that we can take new virtual IRQs.  See
+	 * vgic_v3_fold_lr_state for more info.
+	 */
+	if (vgic_irq_is_mapped_level(irq) && (val & ICH_LR_PENDING_BIT))
+		irq->line_level = false;
+
+	/*
 	 * We currently only support Group1 interrupts, which is a
 	 * known defect. This needs to be addressed at some point.
 	 */
diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index 9d557efd..8072969 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -140,6 +140,29 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
 	kfree(irq);
 }
 
+/* Get the input level of a mapped IRQ directly from the physical GIC */
+bool vgic_get_phys_line_level(struct vgic_irq *irq)
+{
+	bool line_level;
+
+	BUG_ON(!irq->hw);
+
+	WARN_ON(irq_get_irqchip_state(irq->host_irq,
+				      IRQCHIP_STATE_PENDING,
+				      &line_level));
+	return line_level;
+}
+
+/* Set/Clear the physical active state */
+void vgic_irq_set_phys_active(struct vgic_irq *irq, bool active)
+{
+
+	BUG_ON(!irq->hw);
+	WARN_ON(irq_set_irqchip_state(irq->host_irq,
+				      IRQCHIP_STATE_ACTIVE,
+				      active));
+}
+
 /**
  * kvm_vgic_target_oracle - compute the target vcpu for an irq
  *
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index bba7fa2..7bdcda2 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -104,6 +104,11 @@ static inline bool irq_is_pending(struct vgic_irq *irq)
 		return irq->pending_latch || irq->line_level;
 }
 
+static inline bool vgic_irq_is_mapped_level(struct vgic_irq *irq)
+{
+	return irq->config == VGIC_CONFIG_LEVEL && irq->hw;
+}
+
 /*
  * This struct provides an intermediate representation of the fields contained
  * in the GICH_VMCR and ICH_VMCR registers, such that code exporting the GIC
@@ -140,6 +145,8 @@ vgic_get_mmio_region(struct kvm_vcpu *vcpu, struct vgic_io_device *iodev,
 struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
 			      u32 intid);
 void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq);
+bool vgic_get_phys_line_level(struct vgic_irq *irq);
+void vgic_irq_set_phys_active(struct vgic_irq *irq, bool active);
 bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq);
 void vgic_kick_vcpus(struct kvm *kvm);
 
-- 
2.9.0

[PATCH v3 4/5] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs

[Christoffer Dall]

For mapped IRQs (with the HW bit set in the LR) we have to follow some
rules of the architecture.  One of these rules is that VM must not be
allowed to deactivate a virtual interrupt with the HW bit set unless the
physical interrupt is also active.

This works fine when injecting mapped interrupts, because we leave it up
to the injector to either set EOImode==1 or manually set the active
state of the physical interrupt.

However, the guest can set virtual interrupt to be pending or active by
writing to the virtual distributor, which could lead to deactivating a
virtual interrupt with the HW bit set without the physical interrupt
being active.

We could set the physical interrupt to active whenever we are about to
enter the VM with a HW interrupt either pending or active, but that
would be really slow, especially on GICv2.  So we take the long way
around and do the hard work when needed, which is expected to be
extremely rare.

When the VM sets the pending state for a HW interrupt on the virtual
distributor we set the active state on the physical distributor, because
the virtual interrupt can become active and then the guest can
deactivate it.

When the VM clears the pending state we also clear it on the physical
side, because the injector might otherwise raise the interrupt.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 virt/kvm/arm/vgic/vgic-mmio.c | 33 +++++++++++++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic.c      |  7 +++++++
 virt/kvm/arm/vgic/vgic.h      |  1 +
 3 files changed, 41 insertions(+)

diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index c1e4bdd..00003ae 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -131,6 +131,9 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
 		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
 
 		spin_lock(&irq->irq_lock);
+		if (irq->hw)
+			vgic_irq_set_phys_active(irq, true);
+
 		irq->pending_latch = true;
 
 		vgic_queue_irq_unlock(vcpu->kvm, irq);
@@ -149,6 +152,20 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
 		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
 
 		spin_lock(&irq->irq_lock);
+		/*
+		 * We don't want the guest to effectively mask the physical
+		 * interrupt by doing a write to SPENDR followed by a write to
+		 * CPENDR for HW interrupts, so we clear the active state on
+		 * the physical side here.  This may lead to taking an
+		 * additional interrupt on the host, but that should not be a
+		 * problem as the worst that can happen is an additional vgic
+		 * injection.  We also clear the pending state to maintain
+		 * proper semantics for edge HW interrupts.
+		 */
+		if (irq->hw) {
+			vgic_irq_set_phys_pending(irq, false);
+			vgic_irq_set_phys_active(irq, false);
+		}
 
 		irq->pending_latch = false;
 
@@ -214,6 +231,22 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
 	       irq->vcpu->cpu != -1) /* VCPU thread is running */
 		cond_resched_lock(&irq->irq_lock);
 
+	if (irq->hw) {
+		/*
+		 * We cannot support setting the physical active state for
+		 * private interrupts from another CPU than the one running
+		 * the VCPU which identifies which private interrupt it is
+		 * trying to modify.
+		 */
+		if (irq->intid < VGIC_NR_PRIVATE_IRQS &&
+		    irq->target_vcpu != requester_vcpu) {
+			spin_unlock(&irq->irq_lock);
+			return;
+		}
+
+		vgic_irq_set_phys_active(irq, new_active_state);
+	}
+
 	irq->active = new_active_state;
 	if (new_active_state)
 		vgic_queue_irq_unlock(vcpu->kvm, irq);
diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index 8072969..7aec730 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -140,6 +140,13 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
 	kfree(irq);
 }
 
+void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending)
+{
+	WARN_ON(irq_set_irqchip_state(irq->host_irq,
+				      IRQCHIP_STATE_PENDING,
+				      pending));
+}
+
 /* Get the input level of a mapped IRQ directly from the physical GIC */
 bool vgic_get_phys_line_level(struct vgic_irq *irq)
 {
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 7bdcda2..498ee05 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -146,6 +146,7 @@ struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
 			      u32 intid);
 void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq);
 bool vgic_get_phys_line_level(struct vgic_irq *irq);
+void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending);
 void vgic_irq_set_phys_active(struct vgic_irq *irq, bool active);
 bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq);
 void vgic_kick_vcpus(struct kvm *kvm);
-- 
2.9.0

[PATCH v3 5/5] KVM: arm/arm64: Provide a vgic interrupt line level sample function

[Christoffer Dall]

The GIC sometimes need to sample the physical line of a mapped
interrupt.  As we know this to be notoriously slow, provide a callback
function for devices (such as the timer) which can do this much faster
than talking to the distributor, for example by comparing a few
in-memory values.  Fall back to the good old method of poking the
physical GIC if no callback is provided.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/kvm/arm_vgic.h    | 13 ++++++++++++-
 virt/kvm/arm/arch_timer.c | 16 +++++++++++++++-
 virt/kvm/arm/vgic/vgic.c  | 12 +++++++++---
 3 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 53f631b..7f76c9e 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -125,6 +125,17 @@ struct vgic_irq {
 	u8 priority;
 	enum vgic_irq_config config;	/* Level or edge */
 
+	/*
+	 * Callback function pointer to in-kernel devices that can tell us the
+	 * state of the input level of mapped level-triggered IRQ faster than
+	 * peaking into the physical GIC.
+	 *
+	 * Always called in non-preemptible section and the functions can use
+	 * kvm_arm_get_running_vcpu() to get the vcpu pointer for private
+	 * IRQs.
+	 */
+	bool (*get_input_level)(int vintid);
+
 	void *owner;			/* Opaque pointer to reserve an interrupt
 					   for in-kernel devices. */
 };
@@ -309,7 +320,7 @@ void kvm_vgic_init_cpu_hardware(void);
 int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
 			bool level, void *owner);
 int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
-			  u32 vintid);
+			  u32 vintid, bool (*get_input_level)(int vindid));
 int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int vintid);
 bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int vintid);
 
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index c4fa675..5cb96ca 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -645,6 +645,19 @@ static bool timer_irqs_are_valid(struct kvm_vcpu *vcpu)
 	return true;
 }
 
+static bool timer_get_input_level(int vintid)
+{
+	struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
+	struct arch_timer_context *timer;
+
+	if (vintid == vcpu_vtimer(vcpu)->irq.irq)
+		timer = vcpu_vtimer(vcpu);
+	else
+		BUG(); /* We only map the vtimer so far */
+
+	return kvm_timer_should_fire(timer);
+}
+
 int kvm_timer_enable(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
@@ -666,7 +679,8 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
 		return -EINVAL;
 	}
 
-	ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq, vtimer->irq.irq);
+	ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq, vtimer->irq.irq,
+				    timer_get_input_level);
 	if (ret)
 		return ret;
 
diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index 7aec730..0ca2b3d 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -154,6 +154,9 @@ bool vgic_get_phys_line_level(struct vgic_irq *irq)
 
 	BUG_ON(!irq->hw);
 
+	if (irq->get_input_level)
+		return irq->get_input_level(irq->intid);
+
 	WARN_ON(irq_get_irqchip_state(irq->host_irq,
 				      IRQCHIP_STATE_PENDING,
 				      &line_level));
@@ -437,7 +440,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
 
 /* @irq->irq_lock must be held */
 static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
-			    unsigned int host_irq)
+			    unsigned int host_irq,
+			    bool (*get_input_level)(int vindid))
 {
 	struct irq_desc *desc;
 	struct irq_data *data;
@@ -457,6 +461,7 @@ static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
 	irq->hw = true;
 	irq->host_irq = host_irq;
 	irq->hwintid = data->hwirq;
+	irq->get_input_level = get_input_level;
 	return 0;
 }
 
@@ -465,10 +470,11 @@ static inline void kvm_vgic_unmap_irq(struct vgic_irq *irq)
 {
 	irq->hw = false;
 	irq->hwintid = 0;
+	irq->get_input_level = NULL;
 }
 
 int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
-			  u32 vintid)
+			  u32 vintid, bool (*get_input_level)(int vindid))
 {
 	struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, vintid);
 	int ret;
@@ -476,7 +482,7 @@ int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
 	BUG_ON(!irq);
 
 	spin_lock(&irq->irq_lock);
-	ret = kvm_vgic_map_irq(vcpu, irq, host_irq);
+	ret = kvm_vgic_map_irq(vcpu, irq, host_irq, get_input_level);
 	spin_unlock(&irq->irq_lock);
 	vgic_put_irq(vcpu->kvm, irq);
 
-- 
2.9.0

[PATCH v3 4/5] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs

[Auger Eric]

Hi Christoffer,

On 06/09/2017 14:26, Christoffer Dall wrote:
> For mapped IRQs (with the HW bit set in the LR) we have to follow some
> rules of the architecture.  One of these rules is that VM must not be
> allowed to deactivate a virtual interrupt with the HW bit set unless the
> physical interrupt is also active.
> 
> This works fine when injecting mapped interrupts, because we leave it up
> to the injector to either set EOImode==1 or manually set the active
> state of the physical interrupt.
> 
> However, the guest can set virtual interrupt to be pending or active by
> writing to the virtual distributor, which could lead to deactivating a
> virtual interrupt with the HW bit set without the physical interrupt
> being active.
> 
> We could set the physical interrupt to active whenever we are about to
> enter the VM with a HW interrupt either pending or active, but that
> would be really slow, especially on GICv2.  So we take the long way
> around and do the hard work when needed, which is expected to be
> extremely rare.
> 
> When the VM sets the pending state for a HW interrupt on the virtual
> distributor we set the active state on the physical distributor, because
> the virtual interrupt can become active and then the guest can
> deactivate it.
> 
> When the VM clears the pending state we also clear it on the physical
> side, because the injector might otherwise raise the interrupt.
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> ---
>  virt/kvm/arm/vgic/vgic-mmio.c | 33 +++++++++++++++++++++++++++++++++
>  virt/kvm/arm/vgic/vgic.c      |  7 +++++++
>  virt/kvm/arm/vgic/vgic.h      |  1 +
>  3 files changed, 41 insertions(+)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index c1e4bdd..00003ae 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -131,6 +131,9 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
>  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>  
>  		spin_lock(&irq->irq_lock);
> +		if (irq->hw)
> +			vgic_irq_set_phys_active(irq, true);
> +
>  		irq->pending_latch = true;
>  
>  		vgic_queue_irq_unlock(vcpu->kvm, irq);
> @@ -149,6 +152,20 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
>  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>  
>  		spin_lock(&irq->irq_lock);
> +		/*
> +		 * We don't want the guest to effectively mask the physical
> +		 * interrupt by doing a write to SPENDR followed by a write to
> +		 * CPENDR for HW interrupts, so we clear the active state on
> +		 * the physical side here.  This may lead to taking an
> +		 * additional interrupt on the host, but that should not be a
> +		 * problem as the worst that can happen is an additional vgic
> +		 * injection.  We also clear the pending state to maintain
> +		 * proper semantics for edge HW interrupts.
> +		 */
> +		if (irq->hw) {
> +			vgic_irq_set_phys_pending(irq, false);
> +			vgic_irq_set_phys_active(irq, false);
I fail in understanding why the active state is reset here. Can you
provide an example?

If the physical dist has an active state can't the virtual IRQ be in
active state, in which case you may later on deactivate a phys IRQ which
is not active?

Thanks

Eric
> +		}
>  
>  		irq->pending_latch = false;
>  
> @@ -214,6 +231,22 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
>  	       irq->vcpu->cpu != -1) /* VCPU thread is running */
>  		cond_resched_lock(&irq->irq_lock);
>  
> +	if (irq->hw) {
> +		/*
> +		 * We cannot support setting the physical active state for
> +		 * private interrupts from another CPU than the one running
> +		 * the VCPU which identifies which private interrupt it is
> +		 * trying to modify.
> +		 */
> +		if (irq->intid < VGIC_NR_PRIVATE_IRQS &&
> +		    irq->target_vcpu != requester_vcpu) {
> +			spin_unlock(&irq->irq_lock);
> +			return;
> +		}
> +
> +		vgic_irq_set_phys_active(irq, new_active_state);
> +	}
> +
>  	irq->active = new_active_state;
>  	if (new_active_state)
>  		vgic_queue_irq_unlock(vcpu->kvm, irq);
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index 8072969..7aec730 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -140,6 +140,13 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
>  	kfree(irq);
>  }
>  
> +void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending)
> +{
> +	WARN_ON(irq_set_irqchip_state(irq->host_irq,
> +				      IRQCHIP_STATE_PENDING,
> +				      pending));
> +}
> +
>  /* Get the input level of a mapped IRQ directly from the physical GIC */
>  bool vgic_get_phys_line_level(struct vgic_irq *irq)
>  {
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index 7bdcda2..498ee05 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -146,6 +146,7 @@ struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
>  			      u32 intid);
>  void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq);
>  bool vgic_get_phys_line_level(struct vgic_irq *irq);
> +void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending);
>  void vgic_irq_set_phys_active(struct vgic_irq *irq, bool active);
>  bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq);
>  void vgic_kick_vcpus(struct kvm *kvm);
> 

[PATCH v3 4/5] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs

[Marc Zyngier]

On 08/09/17 14:04, Auger Eric wrote:
> Hi Christoffer,
> 
> On 06/09/2017 14:26, Christoffer Dall wrote:
>> For mapped IRQs (with the HW bit set in the LR) we have to follow some
>> rules of the architecture.  One of these rules is that VM must not be
>> allowed to deactivate a virtual interrupt with the HW bit set unless the
>> physical interrupt is also active.
>>
>> This works fine when injecting mapped interrupts, because we leave it up
>> to the injector to either set EOImode==1 or manually set the active
>> state of the physical interrupt.
>>
>> However, the guest can set virtual interrupt to be pending or active by
>> writing to the virtual distributor, which could lead to deactivating a
>> virtual interrupt with the HW bit set without the physical interrupt
>> being active.
>>
>> We could set the physical interrupt to active whenever we are about to
>> enter the VM with a HW interrupt either pending or active, but that
>> would be really slow, especially on GICv2.  So we take the long way
>> around and do the hard work when needed, which is expected to be
>> extremely rare.
>>
>> When the VM sets the pending state for a HW interrupt on the virtual
>> distributor we set the active state on the physical distributor, because
>> the virtual interrupt can become active and then the guest can
>> deactivate it.
>>
>> When the VM clears the pending state we also clear it on the physical
>> side, because the injector might otherwise raise the interrupt.
>>
>> Signed-off-by: Christoffer Dall <cdall@linaro.org>
>> ---
>>  virt/kvm/arm/vgic/vgic-mmio.c | 33 +++++++++++++++++++++++++++++++++
>>  virt/kvm/arm/vgic/vgic.c      |  7 +++++++
>>  virt/kvm/arm/vgic/vgic.h      |  1 +
>>  3 files changed, 41 insertions(+)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
>> index c1e4bdd..00003ae 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>> @@ -131,6 +131,9 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
>>  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>  
>>  		spin_lock(&irq->irq_lock);
>> +		if (irq->hw)
>> +			vgic_irq_set_phys_active(irq, true);
>> +
>>  		irq->pending_latch = true;
>>  
>>  		vgic_queue_irq_unlock(vcpu->kvm, irq);
>> @@ -149,6 +152,20 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
>>  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>  
>>  		spin_lock(&irq->irq_lock);
>> +		/*
>> +		 * We don't want the guest to effectively mask the physical
>> +		 * interrupt by doing a write to SPENDR followed by a write to
>> +		 * CPENDR for HW interrupts, so we clear the active state on
>> +		 * the physical side here.  This may lead to taking an
>> +		 * additional interrupt on the host, but that should not be a
>> +		 * problem as the worst that can happen is an additional vgic
>> +		 * injection.  We also clear the pending state to maintain
>> +		 * proper semantics for edge HW interrupts.
>> +		 */
>> +		if (irq->hw) {
>> +			vgic_irq_set_phys_pending(irq, false);
>> +			vgic_irq_set_phys_active(irq, false);
> I fail in understanding why the active state is reset here. Can you
> provide an example?


If we're clearing the pending state on the virtual side, we need to be
able to let an incoming physical interrupt fire so that it can be made
pending again. We may have to check that the virtual active state is
clear though.

> If the physical dist has an active state can't the virtual IRQ be in
> active state, in which case you may later on deactivate a phys IRQ which
> is not active?


Well, I think we must be able to handle both cases:

- CPENDR write:
	clear physical pending
	if (virtual active clear)
		clear physical active

- CACTIVER write:
	clear physical active

Does this work for you?

Thanks,

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

[PATCH v3 4/5] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs

[Auger Eric]

Hi Marc,

On 08/09/2017 15:32, Marc Zyngier wrote:
> On 08/09/17 14:04, Auger Eric wrote:
>> Hi Christoffer,
>>
>> On 06/09/2017 14:26, Christoffer Dall wrote:
>>> For mapped IRQs (with the HW bit set in the LR) we have to follow some
>>> rules of the architecture.  One of these rules is that VM must not be
>>> allowed to deactivate a virtual interrupt with the HW bit set unless the
>>> physical interrupt is also active.
>>>
>>> This works fine when injecting mapped interrupts, because we leave it up
>>> to the injector to either set EOImode==1 or manually set the active
>>> state of the physical interrupt.
>>>
>>> However, the guest can set virtual interrupt to be pending or active by
>>> writing to the virtual distributor, which could lead to deactivating a
>>> virtual interrupt with the HW bit set without the physical interrupt
>>> being active.
>>>
>>> We could set the physical interrupt to active whenever we are about to
>>> enter the VM with a HW interrupt either pending or active, but that
>>> would be really slow, especially on GICv2.  So we take the long way
>>> around and do the hard work when needed, which is expected to be
>>> extremely rare.
>>>
>>> When the VM sets the pending state for a HW interrupt on the virtual
>>> distributor we set the active state on the physical distributor, because
>>> the virtual interrupt can become active and then the guest can
>>> deactivate it.
>>>
>>> When the VM clears the pending state we also clear it on the physical
>>> side, because the injector might otherwise raise the interrupt.
>>>
>>> Signed-off-by: Christoffer Dall <cdall@linaro.org>
>>> ---
>>>  virt/kvm/arm/vgic/vgic-mmio.c | 33 +++++++++++++++++++++++++++++++++
>>>  virt/kvm/arm/vgic/vgic.c      |  7 +++++++
>>>  virt/kvm/arm/vgic/vgic.h      |  1 +
>>>  3 files changed, 41 insertions(+)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
>>> index c1e4bdd..00003ae 100644
>>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>>> @@ -131,6 +131,9 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
>>>  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>>  
>>>  		spin_lock(&irq->irq_lock);
>>> +		if (irq->hw)
>>> +			vgic_irq_set_phys_active(irq, true);
>>> +
>>>  		irq->pending_latch = true;
>>>  
>>>  		vgic_queue_irq_unlock(vcpu->kvm, irq);
>>> @@ -149,6 +152,20 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
>>>  		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>>  
>>>  		spin_lock(&irq->irq_lock);
>>> +		/*
>>> +		 * We don't want the guest to effectively mask the physical
>>> +		 * interrupt by doing a write to SPENDR followed by a write to
>>> +		 * CPENDR for HW interrupts, so we clear the active state on
>>> +		 * the physical side here.  This may lead to taking an
>>> +		 * additional interrupt on the host, but that should not be a
>>> +		 * problem as the worst that can happen is an additional vgic
>>> +		 * injection.  We also clear the pending state to maintain
>>> +		 * proper semantics for edge HW interrupts.
>>> +		 */
>>> +		if (irq->hw) {
>>> +			vgic_irq_set_phys_pending(irq, false);
>>> +			vgic_irq_set_phys_active(irq, false);
>> I fail in understanding why the active state is reset here. Can you
>> provide an example?
> 
> 
> If we're clearing the pending state on the virtual side, we need to be
> able to let an incoming physical interrupt fire so that it can be made
> pending again. We may have to check that the virtual active state is
> clear though.
> 
>> If the physical dist has an active state can't the virtual IRQ be in
>> active state, in which case you may later on deactivate a phys IRQ which
>> is not active?
> 
> 
> Well, I think we must be able to handle both cases:
> 
> - CPENDR write:
> 	clear physical pending
> 	if (virtual active clear)
> 		clear physical active
> 
> - CACTIVER write:
> 	clear physical active
> 
> Does this work for you?

yes it does with that change!

Thanks

Eric

> 
> Thanks,
> 
> 	M.
> 

[PATCH v3 4/5] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs

[Christoffer Dall]

On Fri, Sep 8, 2017 at 4:27 PM, Auger Eric <eric.auger@redhat.com> wrote:
> Hi Marc,
>
> On 08/09/2017 15:32, Marc Zyngier wrote:
>> On 08/09/17 14:04, Auger Eric wrote:
>>> Hi Christoffer,
>>>
>>> On 06/09/2017 14:26, Christoffer Dall wrote:
>>>> For mapped IRQs (with the HW bit set in the LR) we have to follow some
>>>> rules of the architecture.  One of these rules is that VM must not be
>>>> allowed to deactivate a virtual interrupt with the HW bit set unless the
>>>> physical interrupt is also active.
>>>>
>>>> This works fine when injecting mapped interrupts, because we leave it up
>>>> to the injector to either set EOImode==1 or manually set the active
>>>> state of the physical interrupt.
>>>>
>>>> However, the guest can set virtual interrupt to be pending or active by
>>>> writing to the virtual distributor, which could lead to deactivating a
>>>> virtual interrupt with the HW bit set without the physical interrupt
>>>> being active.
>>>>
>>>> We could set the physical interrupt to active whenever we are about to
>>>> enter the VM with a HW interrupt either pending or active, but that
>>>> would be really slow, especially on GICv2.  So we take the long way
>>>> around and do the hard work when needed, which is expected to be
>>>> extremely rare.
>>>>
>>>> When the VM sets the pending state for a HW interrupt on the virtual
>>>> distributor we set the active state on the physical distributor, because
>>>> the virtual interrupt can become active and then the guest can
>>>> deactivate it.
>>>>
>>>> When the VM clears the pending state we also clear it on the physical
>>>> side, because the injector might otherwise raise the interrupt.
>>>>
>>>> Signed-off-by: Christoffer Dall <cdall@linaro.org>
>>>> ---
>>>>  virt/kvm/arm/vgic/vgic-mmio.c | 33 +++++++++++++++++++++++++++++++++
>>>>  virt/kvm/arm/vgic/vgic.c      |  7 +++++++
>>>>  virt/kvm/arm/vgic/vgic.h      |  1 +
>>>>  3 files changed, 41 insertions(+)
>>>>
>>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
>>>> index c1e4bdd..00003ae 100644
>>>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>>>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>>>> @@ -131,6 +131,9 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
>>>>             struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>>>
>>>>             spin_lock(&irq->irq_lock);
>>>> +           if (irq->hw)
>>>> +                   vgic_irq_set_phys_active(irq, true);
>>>> +
>>>>             irq->pending_latch = true;
>>>>
>>>>             vgic_queue_irq_unlock(vcpu->kvm, irq);
>>>> @@ -149,6 +152,20 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
>>>>             struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>>>
>>>>             spin_lock(&irq->irq_lock);
>>>> +           /*
>>>> +            * We don't want the guest to effectively mask the physical
>>>> +            * interrupt by doing a write to SPENDR followed by a write to
>>>> +            * CPENDR for HW interrupts, so we clear the active state on
>>>> +            * the physical side here.  This may lead to taking an
>>>> +            * additional interrupt on the host, but that should not be a
>>>> +            * problem as the worst that can happen is an additional vgic
>>>> +            * injection.  We also clear the pending state to maintain
>>>> +            * proper semantics for edge HW interrupts.
>>>> +            */
>>>> +           if (irq->hw) {
>>>> +                   vgic_irq_set_phys_pending(irq, false);
>>>> +                   vgic_irq_set_phys_active(irq, false);
>>> I fail in understanding why the active state is reset here. Can you
>>> provide an example?
>>
>>
>> If we're clearing the pending state on the virtual side, we need to be
>> able to let an incoming physical interrupt fire so that it can be made
>> pending again. We may have to check that the virtual active state is
>> clear though.
>>
>>> If the physical dist has an active state can't the virtual IRQ be in
>>> active state, in which case you may later on deactivate a phys IRQ which
>>> is not active?
>>
>>
>> Well, I think we must be able to handle both cases:
>>
>> - CPENDR write:
>>       clear physical pending
>>       if (virtual active clear)
>>               clear physical active
>>
>> - CACTIVER write:
>>       clear physical active
>>
>> Does this work for you?
>
> yes it does with that change!
>
I agree, I should fix that.

But then I wondered, are we properly handling PPIs for
setting/clearing the pending state?  Don't we actually need to do
something similar like the active cpu where we check the requester CPU
and make sure we're not fiddling with some random hardware state when
accessed by userspace?

Thanks,
-Christoffer

[PATCH v3 4/5] KVM: arm/arm64: Support VGIC dist pend/active changes for mapped IRQs

[Marc Zyngier]

On 08/09/17 17:05, Christoffer Dall wrote:
> On Fri, Sep 8, 2017 at 4:27 PM, Auger Eric <eric.auger@redhat.com> wrote:
>> Hi Marc,
>>
>> On 08/09/2017 15:32, Marc Zyngier wrote:
>>> On 08/09/17 14:04, Auger Eric wrote:
>>>> Hi Christoffer,
>>>>
>>>> On 06/09/2017 14:26, Christoffer Dall wrote:
>>>>> For mapped IRQs (with the HW bit set in the LR) we have to follow some
>>>>> rules of the architecture.  One of these rules is that VM must not be
>>>>> allowed to deactivate a virtual interrupt with the HW bit set unless the
>>>>> physical interrupt is also active.
>>>>>
>>>>> This works fine when injecting mapped interrupts, because we leave it up
>>>>> to the injector to either set EOImode==1 or manually set the active
>>>>> state of the physical interrupt.
>>>>>
>>>>> However, the guest can set virtual interrupt to be pending or active by
>>>>> writing to the virtual distributor, which could lead to deactivating a
>>>>> virtual interrupt with the HW bit set without the physical interrupt
>>>>> being active.
>>>>>
>>>>> We could set the physical interrupt to active whenever we are about to
>>>>> enter the VM with a HW interrupt either pending or active, but that
>>>>> would be really slow, especially on GICv2.  So we take the long way
>>>>> around and do the hard work when needed, which is expected to be
>>>>> extremely rare.
>>>>>
>>>>> When the VM sets the pending state for a HW interrupt on the virtual
>>>>> distributor we set the active state on the physical distributor, because
>>>>> the virtual interrupt can become active and then the guest can
>>>>> deactivate it.
>>>>>
>>>>> When the VM clears the pending state we also clear it on the physical
>>>>> side, because the injector might otherwise raise the interrupt.
>>>>>
>>>>> Signed-off-by: Christoffer Dall <cdall@linaro.org>
>>>>> ---
>>>>>  virt/kvm/arm/vgic/vgic-mmio.c | 33 +++++++++++++++++++++++++++++++++
>>>>>  virt/kvm/arm/vgic/vgic.c      |  7 +++++++
>>>>>  virt/kvm/arm/vgic/vgic.h      |  1 +
>>>>>  3 files changed, 41 insertions(+)
>>>>>
>>>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
>>>>> index c1e4bdd..00003ae 100644
>>>>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>>>>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>>>>> @@ -131,6 +131,9 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
>>>>>             struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>>>>
>>>>>             spin_lock(&irq->irq_lock);
>>>>> +           if (irq->hw)
>>>>> +                   vgic_irq_set_phys_active(irq, true);
>>>>> +
>>>>>             irq->pending_latch = true;
>>>>>
>>>>>             vgic_queue_irq_unlock(vcpu->kvm, irq);
>>>>> @@ -149,6 +152,20 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
>>>>>             struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>>>>
>>>>>             spin_lock(&irq->irq_lock);
>>>>> +           /*
>>>>> +            * We don't want the guest to effectively mask the physical
>>>>> +            * interrupt by doing a write to SPENDR followed by a write to
>>>>> +            * CPENDR for HW interrupts, so we clear the active state on
>>>>> +            * the physical side here.  This may lead to taking an
>>>>> +            * additional interrupt on the host, but that should not be a
>>>>> +            * problem as the worst that can happen is an additional vgic
>>>>> +            * injection.  We also clear the pending state to maintain
>>>>> +            * proper semantics for edge HW interrupts.
>>>>> +            */
>>>>> +           if (irq->hw) {
>>>>> +                   vgic_irq_set_phys_pending(irq, false);
>>>>> +                   vgic_irq_set_phys_active(irq, false);
>>>> I fail in understanding why the active state is reset here. Can you
>>>> provide an example?
>>>
>>>
>>> If we're clearing the pending state on the virtual side, we need to be
>>> able to let an incoming physical interrupt fire so that it can be made
>>> pending again. We may have to check that the virtual active state is
>>> clear though.
>>>
>>>> If the physical dist has an active state can't the virtual IRQ be in
>>>> active state, in which case you may later on deactivate a phys IRQ which
>>>> is not active?
>>>
>>>
>>> Well, I think we must be able to handle both cases:
>>>
>>> - CPENDR write:
>>>       clear physical pending
>>>       if (virtual active clear)
>>>               clear physical active
>>>
>>> - CACTIVER write:
>>>       clear physical active
>>>
>>> Does this work for you?
>>
>> yes it does with that change!
>>
> I agree, I should fix that.
> 
> But then I wondered, are we properly handling PPIs for
> setting/clearing the pending state?  Don't we actually need to do
> something similar like the active cpu where we check the requester CPU
> and make sure we're not fiddling with some random hardware state when
> accessed by userspace?
Hmmm. Yeah, that's another can of worm. We should certainly make sure
that whatever userspace does is not directly impacting the HW state (and
only the guest does).

I think. I'm not quite sure...

	M. (GIC-ed, again).
-- 
Jazz is not dead. It just smells funny...