[RFC PATCH 0/4] Handle forwarded level-triggered interrupts

[RFC PATCH 0/4] 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 stolen from Eric's series and is necessary for these patches
to compile as well.  Patch 2 includes the core support for mapped
level-triggered interrupts.  Patch 3 moves some code around for patch 4.
Patch 4 implements an optimization for the timer.  The last two patches
could be deferred until the timer optimization series.

This series is untested (hence the RFC tag) and I'm looking for
feedback from the VGIC guys (you know who you are) on the approach and
whether we know up-front that this breaks for some reason.

I'll be happy to test and debug further if we agree on the overall
approach.

Based on v4.13-rc7.

Thanks,
-Christoffer

[1]: https://lists.cs.columbia.edu/pipermail/kvmarm/2017-June/026072.html

Christoffer Dall (3):
  KVM: arm/arm64: vgic: Support level-triggered mapped interrupts
  KVM: arm/arm64: Rearrange kvm_vgic_[un]map_phys code in vgic.c
  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   | 38 ++++++++++---------------
 virt/kvm/arm/vgic/vgic-v2.c | 29 +++++++++++++++++++
 virt/kvm/arm/vgic/vgic-v3.c | 29 +++++++++++++++++++
 virt/kvm/arm/vgic/vgic.c    | 68 +++++++++++++++++++++++++++++++++++++--------
 virt/kvm/arm/vgic/vgic.h    |  7 +++++
 6 files changed, 153 insertions(+), 37 deletions(-)

-- 
2.9.0

[RFC PATCH 0/4] 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 stolen from Eric's series and is necessary for these patches
to compile as well.  Patch 2 includes the core support for mapped
level-triggered interrupts.  Patch 3 moves some code around for patch 4.
Patch 4 implements an optimization for the timer.  The last two patches
could be deferred until the timer optimization series.

This series is untested (hence the RFC tag) and I'm looking for
feedback from the VGIC guys (you know who you are) on the approach and
whether we know up-front that this breaks for some reason.

I'll be happy to test and debug further if we agree on the overall
approach.

Based on v4.13-rc7.

Thanks,
-Christoffer

[1]: https://lists.cs.columbia.edu/pipermail/kvmarm/2017-June/026072.html

Christoffer Dall (3):
  KVM: arm/arm64: vgic: Support level-triggered mapped interrupts
  KVM: arm/arm64: Rearrange kvm_vgic_[un]map_phys code in vgic.c
  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   | 38 ++++++++++---------------
 virt/kvm/arm/vgic/vgic-v2.c | 29 +++++++++++++++++++
 virt/kvm/arm/vgic/vgic-v3.c | 29 +++++++++++++++++++
 virt/kvm/arm/vgic/vgic.c    | 68 +++++++++++++++++++++++++++++++++++++--------
 virt/kvm/arm/vgic/vgic.h    |  7 +++++
 6 files changed, 153 insertions(+), 37 deletions(-)

-- 
2.9.0

[RFC PATCH 1/4] 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>
---
 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 8e89d63..b24e2f7 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -647,9 +647,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)
@@ -667,26 +664,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

[RFC PATCH 1/4] 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>
---
 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 8e89d63..b24e2f7 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -647,9 +647,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)
@@ -667,26 +664,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

Re: [RFC PATCH 1/4] KVM: arm/arm64: vgic: restructure kvm_vgic_(un)map_phys_irq

[Marc Zyngier]

On Tue, Aug 29 2017 at 11:38:59 am BST, Christoffer Dall <cdall@linaro.org> wrote:
> 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>

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

[RFC PATCH 1/4] KVM: arm/arm64: vgic: restructure kvm_vgic_(un)map_phys_irq

[Marc Zyngier]

On Tue, Aug 29 2017 at 11:38:59 am BST, Christoffer Dall <cdall@linaro.org> wrote:
> 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>

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

[RFC PATCH 2/4] 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>
---
 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..f7c5cb5 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_irq_line_level(irq);
+
+			if (!irq->line_level)
+				vgic_irq_clear_phys_active(irq);
+		}
+
 		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..e377036 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_irq_line_level(irq);
+
+			if (!irq->line_level)
+				vgic_irq_clear_phys_active(irq);
+		}
+
 		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..2691a0a 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_irq_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;
+}
+
+/* Clear the physical active state */
+void vgic_irq_clear_phys_active(struct vgic_irq *irq)
+{
+
+	BUG_ON(!irq->hw);
+	WARN_ON(irq_set_irqchip_state(irq->host_irq,
+				      IRQCHIP_STATE_ACTIVE,
+				      false));
+}
+
 /**
  * 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..22f106d 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_irq_line_level(struct vgic_irq *irq);
+void vgic_irq_clear_phys_active(struct vgic_irq *irq);
 bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq);
 void vgic_kick_vcpus(struct kvm *kvm);
 
-- 
2.9.0

[RFC PATCH 2/4] 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>
---
 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..f7c5cb5 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_irq_line_level(irq);
+
+			if (!irq->line_level)
+				vgic_irq_clear_phys_active(irq);
+		}
+
 		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..e377036 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_irq_line_level(irq);
+
+			if (!irq->line_level)
+				vgic_irq_clear_phys_active(irq);
+		}
+
 		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..2691a0a 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_irq_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;
+}
+
+/* Clear the physical active state */
+void vgic_irq_clear_phys_active(struct vgic_irq *irq)
+{
+
+	BUG_ON(!irq->hw);
+	WARN_ON(irq_set_irqchip_state(irq->host_irq,
+				      IRQCHIP_STATE_ACTIVE,
+				      false));
+}
+
 /**
  * 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..22f106d 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_irq_line_level(struct vgic_irq *irq);
+void vgic_irq_clear_phys_active(struct vgic_irq *irq);
 bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq);
 void vgic_kick_vcpus(struct kvm *kvm);
 
-- 
2.9.0

Re: [RFC PATCH 2/4] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

[Auger Eric]

Hi Christoffer,

On 29/08/2017 11:39, Christoffer Dall wrote:
> 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>
> ---
>  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..f7c5cb5 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_irq_line_level(irq);
> +
> +			if (!irq->line_level)
> +				vgic_irq_clear_phys_active(irq);
> +		}
> +
>  		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..e377036 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_irq_line_level(irq);

I don't see anything related to the GICD_ISPENDING/ICPENDING. In the
last thread, we said we were obliged to set the pending bit on the
physical distributor to avoid the guest deactivating a non active
physical IRQ. If we still intend to do so, with that addition, aren't we
likely to deactivate the physical IRQ before the guest?

Thanks

Eric
> +
> +			if (!irq->line_level)
> +				vgic_irq_clear_phys_active(irq);
> +		}
> +
>  		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..2691a0a 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_irq_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;
> +}
> +
> +/* Clear the physical active state */
> +void vgic_irq_clear_phys_active(struct vgic_irq *irq)
> +{
> +
> +	BUG_ON(!irq->hw);
> +	WARN_ON(irq_set_irqchip_state(irq->host_irq,
> +				      IRQCHIP_STATE_ACTIVE,
> +				      false));
> +}
> +
>  /**
>   * 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..22f106d 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_irq_line_level(struct vgic_irq *irq);
> +void vgic_irq_clear_phys_active(struct vgic_irq *irq);
>  bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq);
>  void vgic_kick_vcpus(struct kvm *kvm);
>  
> 

[RFC PATCH 2/4] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

[Auger Eric]

Hi Christoffer,

On 29/08/2017 11:39, Christoffer Dall wrote:
> 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>
> ---
>  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..f7c5cb5 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_irq_line_level(irq);
> +
> +			if (!irq->line_level)
> +				vgic_irq_clear_phys_active(irq);
> +		}
> +
>  		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..e377036 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_irq_line_level(irq);

I don't see anything related to the GICD_ISPENDING/ICPENDING. In the
last thread, we said we were obliged to set the pending bit on the
physical distributor to avoid the guest deactivating a non active
physical IRQ. If we still intend to do so, with that addition, aren't we
likely to deactivate the physical IRQ before the guest?

Thanks

Eric
> +
> +			if (!irq->line_level)
> +				vgic_irq_clear_phys_active(irq);
> +		}
> +
>  		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..2691a0a 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_irq_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;
> +}
> +
> +/* Clear the physical active state */
> +void vgic_irq_clear_phys_active(struct vgic_irq *irq)
> +{
> +
> +	BUG_ON(!irq->hw);
> +	WARN_ON(irq_set_irqchip_state(irq->host_irq,
> +				      IRQCHIP_STATE_ACTIVE,
> +				      false));
> +}
> +
>  /**
>   * 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..22f106d 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_irq_line_level(struct vgic_irq *irq);
> +void vgic_irq_clear_phys_active(struct vgic_irq *irq);
>  bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq);
>  void vgic_kick_vcpus(struct kvm *kvm);
>  
> 

Re: [RFC PATCH 2/4] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

[Christoffer Dall]

On Wed, Aug 30, 2017 at 10:19:38AM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 29/08/2017 11:39, Christoffer Dall wrote:
> > 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>
> > ---
> >  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..f7c5cb5 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_irq_line_level(irq);
> > +
> > +			if (!irq->line_level)
> > +				vgic_irq_clear_phys_active(irq);
> > +		}
> > +
> >  		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..e377036 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_irq_line_level(irq);
> 
> I don't see anything related to the GICD_ISPENDING/ICPENDING.

vgic_irq_line_level() reads GICD_ISPEND.  Not sure what you mean?

> In the
> last thread, we said we were obliged to set the pending bit on the
> physical distributor to avoid the guest deactivating a non active
> physical IRQ.

Did we?  I don't remember this, and this doesn't make sense to me.  The
only constraint I think we have is that when setting the HW bit in the
LR, the interrupt must be active on the physical side, so that a
deactivate by the guest is also a deactivate on the physical side.

> If we still intend to do so, with that addition, aren't we
> likely to deactivate the physical IRQ before the guest?

Do you mean if it's pending+active in the LR?  That can never happen for
a mapped (HW bit set) interrupt.

So this particular code sequence means that
    (val & ICH_LR_STATE) == ICH_LR_PENDING_BIT

and we never clear the physical active state while the virtual interrupt
is active.

There is an interesting point in terms of handling guest accesses to the
virtual distributor GICD_ISPENDR, GICD_ICPENDR, GICD_ISACTIVER, and
GICD_ICACTIVER where we must also propogate those changes to the
physical side.

Was this what you meant?

Thanks,
-Christoffer


> > +
> > +			if (!irq->line_level)
> > +				vgic_irq_clear_phys_active(irq);
> > +		}
> > +
> >  		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..2691a0a 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_irq_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;
> > +}
> > +
> > +/* Clear the physical active state */
> > +void vgic_irq_clear_phys_active(struct vgic_irq *irq)
> > +{
> > +
> > +	BUG_ON(!irq->hw);
> > +	WARN_ON(irq_set_irqchip_state(irq->host_irq,
> > +				      IRQCHIP_STATE_ACTIVE,
> > +				      false));
> > +}
> > +
> >  /**
> >   * 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..22f106d 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_irq_line_level(struct vgic_irq *irq);
> > +void vgic_irq_clear_phys_active(struct vgic_irq *irq);
> >  bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq);
> >  void vgic_kick_vcpus(struct kvm *kvm);
> >  
> > 

[RFC PATCH 2/4] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

[Christoffer Dall]

On Wed, Aug 30, 2017 at 10:19:38AM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 29/08/2017 11:39, Christoffer Dall wrote:
> > 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>
> > ---
> >  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..f7c5cb5 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_irq_line_level(irq);
> > +
> > +			if (!irq->line_level)
> > +				vgic_irq_clear_phys_active(irq);
> > +		}
> > +
> >  		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..e377036 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_irq_line_level(irq);
> 
> I don't see anything related to the GICD_ISPENDING/ICPENDING.

vgic_irq_line_level() reads GICD_ISPEND.  Not sure what you mean?

> In the
> last thread, we said we were obliged to set the pending bit on the
> physical distributor to avoid the guest deactivating a non active
> physical IRQ.

Did we?  I don't remember this, and this doesn't make sense to me.  The
only constraint I think we have is that when setting the HW bit in the
LR, the interrupt must be active on the physical side, so that a
deactivate by the guest is also a deactivate on the physical side.

> If we still intend to do so, with that addition, aren't we
> likely to deactivate the physical IRQ before the guest?

Do you mean if it's pending+active in the LR?  That can never happen for
a mapped (HW bit set) interrupt.

So this particular code sequence means that
    (val & ICH_LR_STATE) == ICH_LR_PENDING_BIT

and we never clear the physical active state while the virtual interrupt
is active.

There is an interesting point in terms of handling guest accesses to the
virtual distributor GICD_ISPENDR, GICD_ICPENDR, GICD_ISACTIVER, and
GICD_ICACTIVER where we must also propogate those changes to the
physical side.

Was this what you meant?

Thanks,
-Christoffer


> > +
> > +			if (!irq->line_level)
> > +				vgic_irq_clear_phys_active(irq);
> > +		}
> > +
> >  		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..2691a0a 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_irq_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;
> > +}
> > +
> > +/* Clear the physical active state */
> > +void vgic_irq_clear_phys_active(struct vgic_irq *irq)
> > +{
> > +
> > +	BUG_ON(!irq->hw);
> > +	WARN_ON(irq_set_irqchip_state(irq->host_irq,
> > +				      IRQCHIP_STATE_ACTIVE,
> > +				      false));
> > +}
> > +
> >  /**
> >   * 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..22f106d 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_irq_line_level(struct vgic_irq *irq);
> > +void vgic_irq_clear_phys_active(struct vgic_irq *irq);
> >  bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq);
> >  void vgic_kick_vcpus(struct kvm *kvm);
> >  
> > 

Re: [RFC PATCH 2/4] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

[Auger Eric]

Hi Christoffer,

On 30/08/2017 11:20, Christoffer Dall wrote:
> On Wed, Aug 30, 2017 at 10:19:38AM +0200, Auger Eric wrote:
>> Hi Christoffer,
>>
>> On 29/08/2017 11:39, Christoffer Dall wrote:
>>> 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>
>>> ---
>>>  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..f7c5cb5 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_irq_line_level(irq);
>>> +
>>> +			if (!irq->line_level)
>>> +				vgic_irq_clear_phys_active(irq);
>>> +		}
>>> +
>>>  		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..e377036 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_irq_line_level(irq);
>>
>> I don't see anything related to the GICD_ISPENDING/ICPENDING.
> 
> vgic_irq_line_level() reads GICD_ISPEND.  Not sure what you mean?
> 
>> In the
>> last thread, we said we were obliged to set the pending bit on the
>> physical distributor to avoid the guest deactivating a non active
>> physical IRQ.
> 
> Did we?  I don't remember this, and this doesn't make sense to me.  The
> only constraint I think we have is that when setting the HW bit in the
> LR, the interrupt must be active on the physical side, so that a
> deactivate by the guest is also a deactivate on the physical side.

That what I understood from this thread:
https://lkml.org/lkml/2017/7/25/534

At the moment the LR HW bit is set whatever the source of the IRQ I
think, HW driven or ISPENDRn driven (we only test irq->hw)


> 
>> If we still intend to do so, with that addition, aren't we
>> likely to deactivate the physical IRQ before the guest?
> 
> Do you mean if it's pending+active in the LR?  That can never happen for
> a mapped (HW bit set) interrupt.
> 
> So this particular code sequence means that
>     (val & ICH_LR_STATE) == ICH_LR_PENDING_BIT
> 
> and we never clear the physical active state while the virtual interrupt
> is active.

No my scenario was:
IRQ originates from a ISPENDRn. I Thought we set the physical
distributor state accordingly. pIRQ is active. vIRQ is pending. On fold
we detect the line is low and we deactivate the pIRQ. pending_latch is
still true. The vIRQ will be processed but its physical IRQ has been
deactivated.

> There is an interesting point in terms of handling guest accesses to the
> virtual distributor GICD_ISPENDR, GICD_ICPENDR, GICD_ISACTIVER, and
> GICD_ICACTIVER where we must also propogate those changes to the
> physical side.
> 
> Was this what you meant?
yes I think we also need to address this to get the whole picture.

Thanks

Eric
> 
> Thanks,
> -Christoffer
> 
> 
>>> +
>>> +			if (!irq->line_level)
>>> +				vgic_irq_clear_phys_active(irq);
>>> +		}
>>> +
>>>  		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..2691a0a 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_irq_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;
>>> +}
>>> +
>>> +/* Clear the physical active state */
>>> +void vgic_irq_clear_phys_active(struct vgic_irq *irq)
>>> +{
>>> +
>>> +	BUG_ON(!irq->hw);
>>> +	WARN_ON(irq_set_irqchip_state(irq->host_irq,
>>> +				      IRQCHIP_STATE_ACTIVE,
>>> +				      false));
>>> +}
>>> +
>>>  /**
>>>   * 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..22f106d 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_irq_line_level(struct vgic_irq *irq);
>>> +void vgic_irq_clear_phys_active(struct vgic_irq *irq);
>>>  bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq);
>>>  void vgic_kick_vcpus(struct kvm *kvm);
>>>  
>>>

[RFC PATCH 2/4] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

[Auger Eric]

Hi Christoffer,

On 30/08/2017 11:20, Christoffer Dall wrote:
> On Wed, Aug 30, 2017 at 10:19:38AM +0200, Auger Eric wrote:
>> Hi Christoffer,
>>
>> On 29/08/2017 11:39, Christoffer Dall wrote:
>>> 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>
>>> ---
>>>  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..f7c5cb5 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_irq_line_level(irq);
>>> +
>>> +			if (!irq->line_level)
>>> +				vgic_irq_clear_phys_active(irq);
>>> +		}
>>> +
>>>  		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..e377036 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_irq_line_level(irq);
>>
>> I don't see anything related to the GICD_ISPENDING/ICPENDING.
> 
> vgic_irq_line_level() reads GICD_ISPEND.  Not sure what you mean?
> 
>> In the
>> last thread, we said we were obliged to set the pending bit on the
>> physical distributor to avoid the guest deactivating a non active
>> physical IRQ.
> 
> Did we?  I don't remember this, and this doesn't make sense to me.  The
> only constraint I think we have is that when setting the HW bit in the
> LR, the interrupt must be active on the physical side, so that a
> deactivate by the guest is also a deactivate on the physical side.

That what I understood from this thread:
https://lkml.org/lkml/2017/7/25/534

At the moment the LR HW bit is set whatever the source of the IRQ I
think, HW driven or ISPENDRn driven (we only test irq->hw)


> 
>> If we still intend to do so, with that addition, aren't we
>> likely to deactivate the physical IRQ before the guest?
> 
> Do you mean if it's pending+active in the LR?  That can never happen for
> a mapped (HW bit set) interrupt.
> 
> So this particular code sequence means that
>     (val & ICH_LR_STATE) == ICH_LR_PENDING_BIT
> 
> and we never clear the physical active state while the virtual interrupt
> is active.

No my scenario was:
IRQ originates from a ISPENDRn. I Thought we set the physical
distributor state accordingly. pIRQ is active. vIRQ is pending. On fold
we detect the line is low and we deactivate the pIRQ. pending_latch is
still true. The vIRQ will be processed but its physical IRQ has been
deactivated.

> There is an interesting point in terms of handling guest accesses to the
> virtual distributor GICD_ISPENDR, GICD_ICPENDR, GICD_ISACTIVER, and
> GICD_ICACTIVER where we must also propogate those changes to the
> physical side.
> 
> Was this what you meant?
yes I think we also need to address this to get the whole picture.

Thanks

Eric
> 
> Thanks,
> -Christoffer
> 
> 
>>> +
>>> +			if (!irq->line_level)
>>> +				vgic_irq_clear_phys_active(irq);
>>> +		}
>>> +
>>>  		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..2691a0a 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_irq_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;
>>> +}
>>> +
>>> +/* Clear the physical active state */
>>> +void vgic_irq_clear_phys_active(struct vgic_irq *irq)
>>> +{
>>> +
>>> +	BUG_ON(!irq->hw);
>>> +	WARN_ON(irq_set_irqchip_state(irq->host_irq,
>>> +				      IRQCHIP_STATE_ACTIVE,
>>> +				      false));
>>> +}
>>> +
>>>  /**
>>>   * 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..22f106d 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_irq_line_level(struct vgic_irq *irq);
>>> +void vgic_irq_clear_phys_active(struct vgic_irq *irq);
>>>  bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq);
>>>  void vgic_kick_vcpus(struct kvm *kvm);
>>>  
>>>

Re: [RFC PATCH 2/4] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

[Christoffer Dall]

On Wed, Aug 30, 2017 at 12:13:32PM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 30/08/2017 11:20, Christoffer Dall wrote:
> > On Wed, Aug 30, 2017 at 10:19:38AM +0200, Auger Eric wrote:
> >> Hi Christoffer,
> >>
> >> On 29/08/2017 11:39, Christoffer Dall wrote:
> >>> 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>
> >>> ---
> >>>  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..f7c5cb5 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_irq_line_level(irq);
> >>> +
> >>> +			if (!irq->line_level)
> >>> +				vgic_irq_clear_phys_active(irq);
> >>> +		}
> >>> +
> >>>  		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..e377036 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_irq_line_level(irq);
> >>
> >> I don't see anything related to the GICD_ISPENDING/ICPENDING.
> > 
> > vgic_irq_line_level() reads GICD_ISPEND.  Not sure what you mean?
> > 
> >> In the
> >> last thread, we said we were obliged to set the pending bit on the
> >> physical distributor to avoid the guest deactivating a non active
> >> physical IRQ.
> > 
> > Did we?  I don't remember this, and this doesn't make sense to me.  The
> > only constraint I think we have is that when setting the HW bit in the
> > LR, the interrupt must be active on the physical side, so that a
> > deactivate by the guest is also a deactivate on the physical side.
> 
> That what I understood from this thread:
> https://lkml.org/lkml/2017/7/25/534

Right, but that was specific to how we handle the guest writing to
PEND/ACT on the virtual distributor.

> 
> At the moment the LR HW bit is set whatever the source of the IRQ I
> think, HW driven or ISPENDRn driven (we only test irq->hw)
> 

Ah, I didn't actually consider that we could special-case the
pending_latch for a HW interrupt.  I suppose we should actually consider
that if someone tries to map an edge-triggered SPI.  Let me have a look
at that.

> 
> > 
> >> If we still intend to do so, with that addition, aren't we
> >> likely to deactivate the physical IRQ before the guest?
> > 
> > Do you mean if it's pending+active in the LR?  That can never happen for
> > a mapped (HW bit set) interrupt.
> > 
> > So this particular code sequence means that
> >     (val & ICH_LR_STATE) == ICH_LR_PENDING_BIT
> > 
> > and we never clear the physical active state while the virtual interrupt
> > is active.
> 
> No my scenario was:
> IRQ originates from a ISPENDRn.

virtual IRQ from a write to the VGIC ISPENDRn, correct?

> I Thought we set the physical
> distributor state accordingly. pIRQ is active. vIRQ is pending. On fold
> we detect the line is low and we deactivate the pIRQ. pending_latch is
> still true. The vIRQ will be processed but its physical IRQ has been
> deactivated.
> 

As long as we properly handle pending_latch and guest writes to the VGIC
PEND/ACT registers, I don't see a problem with this code.

> > There is an interesting point in terms of handling guest accesses to the
> > virtual distributor GICD_ISPENDR, GICD_ICPENDR, GICD_ISACTIVER, and
> > GICD_ICACTIVER where we must also propogate those changes to the
> > physical side.
> > 
> > Was this what you meant?
> yes I think we also need to address this to get the whole picture.
> 

Yes, I'll respin.

Thanks,
-Christoffer

[RFC PATCH 2/4] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

[Christoffer Dall]

On Wed, Aug 30, 2017 at 12:13:32PM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 30/08/2017 11:20, Christoffer Dall wrote:
> > On Wed, Aug 30, 2017 at 10:19:38AM +0200, Auger Eric wrote:
> >> Hi Christoffer,
> >>
> >> On 29/08/2017 11:39, Christoffer Dall wrote:
> >>> 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>
> >>> ---
> >>>  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..f7c5cb5 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_irq_line_level(irq);
> >>> +
> >>> +			if (!irq->line_level)
> >>> +				vgic_irq_clear_phys_active(irq);
> >>> +		}
> >>> +
> >>>  		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..e377036 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_irq_line_level(irq);
> >>
> >> I don't see anything related to the GICD_ISPENDING/ICPENDING.
> > 
> > vgic_irq_line_level() reads GICD_ISPEND.  Not sure what you mean?
> > 
> >> In the
> >> last thread, we said we were obliged to set the pending bit on the
> >> physical distributor to avoid the guest deactivating a non active
> >> physical IRQ.
> > 
> > Did we?  I don't remember this, and this doesn't make sense to me.  The
> > only constraint I think we have is that when setting the HW bit in the
> > LR, the interrupt must be active on the physical side, so that a
> > deactivate by the guest is also a deactivate on the physical side.
> 
> That what I understood from this thread:
> https://lkml.org/lkml/2017/7/25/534

Right, but that was specific to how we handle the guest writing to
PEND/ACT on the virtual distributor.

> 
> At the moment the LR HW bit is set whatever the source of the IRQ I
> think, HW driven or ISPENDRn driven (we only test irq->hw)
> 

Ah, I didn't actually consider that we could special-case the
pending_latch for a HW interrupt.  I suppose we should actually consider
that if someone tries to map an edge-triggered SPI.  Let me have a look
at that.

> 
> > 
> >> If we still intend to do so, with that addition, aren't we
> >> likely to deactivate the physical IRQ before the guest?
> > 
> > Do you mean if it's pending+active in the LR?  That can never happen for
> > a mapped (HW bit set) interrupt.
> > 
> > So this particular code sequence means that
> >     (val & ICH_LR_STATE) == ICH_LR_PENDING_BIT
> > 
> > and we never clear the physical active state while the virtual interrupt
> > is active.
> 
> No my scenario was:
> IRQ originates from a ISPENDRn.

virtual IRQ from a write to the VGIC ISPENDRn, correct?

> I Thought we set the physical
> distributor state accordingly. pIRQ is active. vIRQ is pending. On fold
> we detect the line is low and we deactivate the pIRQ. pending_latch is
> still true. The vIRQ will be processed but its physical IRQ has been
> deactivated.
> 

As long as we properly handle pending_latch and guest writes to the VGIC
PEND/ACT registers, I don't see a problem with this code.

> > There is an interesting point in terms of handling guest accesses to the
> > virtual distributor GICD_ISPENDR, GICD_ICPENDR, GICD_ISACTIVER, and
> > GICD_ICACTIVER where we must also propogate those changes to the
> > physical side.
> > 
> > Was this what you meant?
> yes I think we also need to address this to get the whole picture.
> 

Yes, I'll respin.

Thanks,
-Christoffer

Re: [RFC PATCH 2/4] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

[Auger Eric]

Hi Christoffer,

On 30/08/2017 14:03, Christoffer Dall wrote:
> On Wed, Aug 30, 2017 at 12:13:32PM +0200, Auger Eric wrote:
>> Hi Christoffer,
>>
>> On 30/08/2017 11:20, Christoffer Dall wrote:
>>> On Wed, Aug 30, 2017 at 10:19:38AM +0200, Auger Eric wrote:
>>>> Hi Christoffer,
>>>>
>>>> On 29/08/2017 11:39, Christoffer Dall wrote:
>>>>> 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>
>>>>> ---
>>>>>  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..f7c5cb5 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_irq_line_level(irq);
>>>>> +
>>>>> +			if (!irq->line_level)
>>>>> +				vgic_irq_clear_phys_active(irq);
>>>>> +		}
>>>>> +
>>>>>  		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..e377036 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_irq_line_level(irq);
>>>>
>>>> I don't see anything related to the GICD_ISPENDING/ICPENDING.
>>>
>>> vgic_irq_line_level() reads GICD_ISPEND.  Not sure what you mean?
>>>
>>>> In the
>>>> last thread, we said we were obliged to set the pending bit on the
>>>> physical distributor to avoid the guest deactivating a non active
>>>> physical IRQ.
>>>
>>> Did we?  I don't remember this, and this doesn't make sense to me.  The
>>> only constraint I think we have is that when setting the HW bit in the
>>> LR, the interrupt must be active on the physical side, so that a
>>> deactivate by the guest is also a deactivate on the physical side.
>>
>> That what I understood from this thread:
>> https://lkml.org/lkml/2017/7/25/534
> 
> Right, but that was specific to how we handle the guest writing to
> PEND/ACT on the virtual distributor.
> 
>>
>> At the moment the LR HW bit is set whatever the source of the IRQ I
>> think, HW driven or ISPENDRn driven (we only test irq->hw)
>>
> 
> Ah, I didn't actually consider that we could special-case the
> pending_latch for a HW interrupt.  I suppose we should actually consider
> that if someone tries to map an edge-triggered SPI.  Let me have a look
> at that.
> 
>>
>>>
>>>> If we still intend to do so, with that addition, aren't we
>>>> likely to deactivate the physical IRQ before the guest?
>>>
>>> Do you mean if it's pending+active in the LR?  That can never happen for
>>> a mapped (HW bit set) interrupt.
>>>
>>> So this particular code sequence means that
>>>     (val & ICH_LR_STATE) == ICH_LR_PENDING_BIT
>>>
>>> and we never clear the physical active state while the virtual interrupt
>>> is active.
>>
>> No my scenario was:
>> IRQ originates from a ISPENDRn.
> 
> virtual IRQ from a write to the VGIC ISPENDRn, correct?
correct.
> 
>> I Thought we set the physical
>> distributor state accordingly. pIRQ is active. vIRQ is pending. On fold
>> we detect the line is low and we deactivate the pIRQ. pending_latch is
>> still true. The vIRQ will be processed but its physical IRQ has been
>> deactivated.
>>
> 
> As long as we properly handle pending_latch and guest writes to the VGIC
> PEND/ACT registers, I don't see a problem with this code.
to me this depends if the LR HW bit is set.
> 
>>> There is an interesting point in terms of handling guest accesses to the
>>> virtual distributor GICD_ISPENDR, GICD_ICPENDR, GICD_ISACTIVER, and
>>> GICD_ICACTIVER where we must also propogate those changes to the
>>> physical side.
>>>
>>> Was this what you meant?
>> yes I think we also need to address this to get the whole picture.
>>
> 
> Yes, I'll respin.
OK

Thanks

Eric
> 
> Thanks,
> -Christoffer
> 

[RFC PATCH 2/4] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

[Auger Eric]

Hi Christoffer,

On 30/08/2017 14:03, Christoffer Dall wrote:
> On Wed, Aug 30, 2017 at 12:13:32PM +0200, Auger Eric wrote:
>> Hi Christoffer,
>>
>> On 30/08/2017 11:20, Christoffer Dall wrote:
>>> On Wed, Aug 30, 2017 at 10:19:38AM +0200, Auger Eric wrote:
>>>> Hi Christoffer,
>>>>
>>>> On 29/08/2017 11:39, Christoffer Dall wrote:
>>>>> 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>
>>>>> ---
>>>>>  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..f7c5cb5 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_irq_line_level(irq);
>>>>> +
>>>>> +			if (!irq->line_level)
>>>>> +				vgic_irq_clear_phys_active(irq);
>>>>> +		}
>>>>> +
>>>>>  		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..e377036 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_irq_line_level(irq);
>>>>
>>>> I don't see anything related to the GICD_ISPENDING/ICPENDING.
>>>
>>> vgic_irq_line_level() reads GICD_ISPEND.  Not sure what you mean?
>>>
>>>> In the
>>>> last thread, we said we were obliged to set the pending bit on the
>>>> physical distributor to avoid the guest deactivating a non active
>>>> physical IRQ.
>>>
>>> Did we?  I don't remember this, and this doesn't make sense to me.  The
>>> only constraint I think we have is that when setting the HW bit in the
>>> LR, the interrupt must be active on the physical side, so that a
>>> deactivate by the guest is also a deactivate on the physical side.
>>
>> That what I understood from this thread:
>> https://lkml.org/lkml/2017/7/25/534
> 
> Right, but that was specific to how we handle the guest writing to
> PEND/ACT on the virtual distributor.
> 
>>
>> At the moment the LR HW bit is set whatever the source of the IRQ I
>> think, HW driven or ISPENDRn driven (we only test irq->hw)
>>
> 
> Ah, I didn't actually consider that we could special-case the
> pending_latch for a HW interrupt.  I suppose we should actually consider
> that if someone tries to map an edge-triggered SPI.  Let me have a look
> at that.
> 
>>
>>>
>>>> If we still intend to do so, with that addition, aren't we
>>>> likely to deactivate the physical IRQ before the guest?
>>>
>>> Do you mean if it's pending+active in the LR?  That can never happen for
>>> a mapped (HW bit set) interrupt.
>>>
>>> So this particular code sequence means that
>>>     (val & ICH_LR_STATE) == ICH_LR_PENDING_BIT
>>>
>>> and we never clear the physical active state while the virtual interrupt
>>> is active.
>>
>> No my scenario was:
>> IRQ originates from a ISPENDRn.
> 
> virtual IRQ from a write to the VGIC ISPENDRn, correct?
correct.
> 
>> I Thought we set the physical
>> distributor state accordingly. pIRQ is active. vIRQ is pending. On fold
>> we detect the line is low and we deactivate the pIRQ. pending_latch is
>> still true. The vIRQ will be processed but its physical IRQ has been
>> deactivated.
>>
> 
> As long as we properly handle pending_latch and guest writes to the VGIC
> PEND/ACT registers, I don't see a problem with this code.
to me this depends if the LR HW bit is set.
> 
>>> There is an interesting point in terms of handling guest accesses to the
>>> virtual distributor GICD_ISPENDR, GICD_ICPENDR, GICD_ISACTIVER, and
>>> GICD_ICACTIVER where we must also propogate those changes to the
>>> physical side.
>>>
>>> Was this what you meant?
>> yes I think we also need to address this to get the whole picture.
>>
> 
> Yes, I'll respin.
OK

Thanks

Eric
> 
> Thanks,
> -Christoffer
> 

Re: [RFC PATCH 2/4] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

[Marc Zyngier]

On Tue, Aug 29 2017 at 11:39:00 am BST, Christoffer Dall <cdall@linaro.org> wrote:
> 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

Would it be worth mentioning that this is a consequence of offloading
part of the flow handling to the HW?

> 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>
> ---
>  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..f7c5cb5 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_irq_line_level(irq);
> +
> +			if (!irq->line_level)
> +				vgic_irq_clear_phys_active(irq);
> +		}
> +
>  		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..e377036 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_irq_line_level(irq);
> +
> +			if (!irq->line_level)
> +				vgic_irq_clear_phys_active(irq);
> +		}
> +
>  		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..2691a0a 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_irq_line_level(struct vgic_irq *irq)

nit: it would make the above clearer if this was named something like
vgic_irq_get_phys_line_level(), as the current name is pretty ambiguous
about which side of the GIC we're looking at.

> +{
> +	bool line_level;
> +
> +	BUG_ON(!irq->hw);
> +
> +	WARN_ON(irq_get_irqchip_state(irq->host_irq,
> +				      IRQCHIP_STATE_PENDING,
> +				      &line_level));
> +	return line_level;
> +}
> +
> +/* Clear the physical active state */
> +void vgic_irq_clear_phys_active(struct vgic_irq *irq)
> +{
> +
> +	BUG_ON(!irq->hw);
> +	WARN_ON(irq_set_irqchip_state(irq->host_irq,
> +				      IRQCHIP_STATE_ACTIVE,
> +				      false));
> +}
> +
>  /**
>   * 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..22f106d 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_irq_line_level(struct vgic_irq *irq);
> +void vgic_irq_clear_phys_active(struct vgic_irq *irq);
>  bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq);
>  void vgic_kick_vcpus(struct kvm *kvm);

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

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

[RFC PATCH 2/4] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

[Marc Zyngier]

On Tue, Aug 29 2017 at 11:39:00 am BST, Christoffer Dall <cdall@linaro.org> wrote:
> 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

Would it be worth mentioning that this is a consequence of offloading
part of the flow handling to the HW?

> 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>
> ---
>  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..f7c5cb5 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_irq_line_level(irq);
> +
> +			if (!irq->line_level)
> +				vgic_irq_clear_phys_active(irq);
> +		}
> +
>  		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..e377036 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_irq_line_level(irq);
> +
> +			if (!irq->line_level)
> +				vgic_irq_clear_phys_active(irq);
> +		}
> +
>  		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..2691a0a 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_irq_line_level(struct vgic_irq *irq)

nit: it would make the above clearer if this was named something like
vgic_irq_get_phys_line_level(), as the current name is pretty ambiguous
about which side of the GIC we're looking at.

> +{
> +	bool line_level;
> +
> +	BUG_ON(!irq->hw);
> +
> +	WARN_ON(irq_get_irqchip_state(irq->host_irq,
> +				      IRQCHIP_STATE_PENDING,
> +				      &line_level));
> +	return line_level;
> +}
> +
> +/* Clear the physical active state */
> +void vgic_irq_clear_phys_active(struct vgic_irq *irq)
> +{
> +
> +	BUG_ON(!irq->hw);
> +	WARN_ON(irq_set_irqchip_state(irq->host_irq,
> +				      IRQCHIP_STATE_ACTIVE,
> +				      false));
> +}
> +
>  /**
>   * 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..22f106d 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_irq_line_level(struct vgic_irq *irq);
> +void vgic_irq_clear_phys_active(struct vgic_irq *irq);
>  bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq);
>  void vgic_kick_vcpus(struct kvm *kvm);

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

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

Re: [RFC PATCH 2/4] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

[Christoffer Dall]

On Sat, Sep 02, 2017 at 11:52:57AM +0100, Marc Zyngier wrote:
> On Tue, Aug 29 2017 at 11:39:00 am BST, Christoffer Dall <cdall@linaro.org> wrote:
> > 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
> 
> Would it be worth mentioning that this is a consequence of offloading
> part of the flow handling to the HW?
> 

Possibly, but I'm not really sure what that actually means.  Can you
explain a little more, and I'll be happy to add the text?

> > 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>
> > ---
> >  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..f7c5cb5 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_irq_line_level(irq);
> > +
> > +			if (!irq->line_level)
> > +				vgic_irq_clear_phys_active(irq);
> > +		}
> > +
> >  		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..e377036 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_irq_line_level(irq);
> > +
> > +			if (!irq->line_level)
> > +				vgic_irq_clear_phys_active(irq);
> > +		}
> > +
> >  		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..2691a0a 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_irq_line_level(struct vgic_irq *irq)
> 
> nit: it would make the above clearer if this was named something like
> vgic_irq_get_phys_line_level(), as the current name is pretty ambiguous
> about which side of the GIC we're looking at.
> 
Yes, absolutely.

> > +{
> > +	bool line_level;
> > +
> > +	BUG_ON(!irq->hw);
> > +
> > +	WARN_ON(irq_get_irqchip_state(irq->host_irq,
> > +				      IRQCHIP_STATE_PENDING,
> > +				      &line_level));
> > +	return line_level;
> > +}
> > +
> > +/* Clear the physical active state */
> > +void vgic_irq_clear_phys_active(struct vgic_irq *irq)
> > +{
> > +
> > +	BUG_ON(!irq->hw);
> > +	WARN_ON(irq_set_irqchip_state(irq->host_irq,
> > +				      IRQCHIP_STATE_ACTIVE,
> > +				      false));
> > +}
> > +
> >  /**
> >   * 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..22f106d 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_irq_line_level(struct vgic_irq *irq);
> > +void vgic_irq_clear_phys_active(struct vgic_irq *irq);
> >  bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq);
> >  void vgic_kick_vcpus(struct kvm *kvm);
> 
> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
> 

Thanks!
-Christoffer

[RFC PATCH 2/4] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

[Christoffer Dall]

On Sat, Sep 02, 2017 at 11:52:57AM +0100, Marc Zyngier wrote:
> On Tue, Aug 29 2017 at 11:39:00 am BST, Christoffer Dall <cdall@linaro.org> wrote:
> > 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
> 
> Would it be worth mentioning that this is a consequence of offloading
> part of the flow handling to the HW?
> 

Possibly, but I'm not really sure what that actually means.  Can you
explain a little more, and I'll be happy to add the text?

> > 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>
> > ---
> >  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..f7c5cb5 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_irq_line_level(irq);
> > +
> > +			if (!irq->line_level)
> > +				vgic_irq_clear_phys_active(irq);
> > +		}
> > +
> >  		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..e377036 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_irq_line_level(irq);
> > +
> > +			if (!irq->line_level)
> > +				vgic_irq_clear_phys_active(irq);
> > +		}
> > +
> >  		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..2691a0a 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_irq_line_level(struct vgic_irq *irq)
> 
> nit: it would make the above clearer if this was named something like
> vgic_irq_get_phys_line_level(), as the current name is pretty ambiguous
> about which side of the GIC we're looking at.
> 
Yes, absolutely.

> > +{
> > +	bool line_level;
> > +
> > +	BUG_ON(!irq->hw);
> > +
> > +	WARN_ON(irq_get_irqchip_state(irq->host_irq,
> > +				      IRQCHIP_STATE_PENDING,
> > +				      &line_level));
> > +	return line_level;
> > +}
> > +
> > +/* Clear the physical active state */
> > +void vgic_irq_clear_phys_active(struct vgic_irq *irq)
> > +{
> > +
> > +	BUG_ON(!irq->hw);
> > +	WARN_ON(irq_set_irqchip_state(irq->host_irq,
> > +				      IRQCHIP_STATE_ACTIVE,
> > +				      false));
> > +}
> > +
> >  /**
> >   * 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..22f106d 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_irq_line_level(struct vgic_irq *irq);
> > +void vgic_irq_clear_phys_active(struct vgic_irq *irq);
> >  bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq);
> >  void vgic_kick_vcpus(struct kvm *kvm);
> 
> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
> 

Thanks!
-Christoffer

Re: [RFC PATCH 2/4] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

[Marc Zyngier]

On Tue, Aug 29 2017 at 11:39:00 am BST, Christoffer Dall <cdall@linaro.org> wrote:
> 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>
> ---
>  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..f7c5cb5 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)) {

I've just had a worrying though. Here, we're looking at the guest's view
of the trigger. But shouldn't that be the *host's*? We're assuming that
the two should match, and while they certainly do for the timer, this is
not something that can be enforced for SPIs.

What do you think?

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

[RFC PATCH 2/4] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

[Marc Zyngier]

On Tue, Aug 29 2017 at 11:39:00 am BST, Christoffer Dall <cdall@linaro.org> wrote:
> 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>
> ---
>  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..f7c5cb5 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)) {

I've just had a worrying though. Here, we're looking at the guest's view
of the trigger. But shouldn't that be the *host's*? We're assuming that
the two should match, and while they certainly do for the timer, this is
not something that can be enforced for SPIs.

What do you think?

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

Re: [RFC PATCH 2/4] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

[Christoffer Dall]

On Sat, Sep 02, 2017 at 12:04:06PM +0100, Marc Zyngier wrote:
> On Tue, Aug 29 2017 at 11:39:00 am BST, Christoffer Dall <cdall@linaro.org> wrote:
> > 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>
> > ---
> >  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..f7c5cb5 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)) {
> 
> I've just had a worrying though. Here, we're looking at the guest's view
> of the trigger. But shouldn't that be the *host's*? We're assuming that
> the two should match, and while they certainly do for the timer, this is
> not something that can be enforced for SPIs.
> 
> What do you think?
> 
This is a tricky one.  I think that in general, if the guest
misconfigures the level/edge of a virtual interrupt, it is only setting
itself up for trouble, which doesn't interfere with the host
functionality, so we don't have to worry about that.  What we do have to
worry is about is that the guest can't interfere with the host in
uninteded ways by playing with the interrupts.  Let's consider the two
scenarios:

Phys = level, Virt = Edge
-------------------------
These patches don't change anything from today's behavior.  We may loose
interrupts for the VM, tough.

Phys = edge, Virt = Level
-------------------------
In this case we will read the physical input level on return from the
VM.  That changes the virtual line_level, no harm done.  If we see the
input line to the VGIC (or physial pending state) is asserted, we will
deactivate a physical interrup which means, if it's actually pending,
the driver (VFIO or arch timer, typically) will take another
interrupt, no harm done.

So I think we're fine.


Thanks,
-Christoffer

[RFC PATCH 2/4] KVM: arm/arm64: vgic: Support level-triggered mapped interrupts

[Christoffer Dall]

On Sat, Sep 02, 2017 at 12:04:06PM +0100, Marc Zyngier wrote:
> On Tue, Aug 29 2017 at 11:39:00 am BST, Christoffer Dall <cdall@linaro.org> wrote:
> > 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>
> > ---
> >  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..f7c5cb5 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)) {
> 
> I've just had a worrying though. Here, we're looking at the guest's view
> of the trigger. But shouldn't that be the *host's*? We're assuming that
> the two should match, and while they certainly do for the timer, this is
> not something that can be enforced for SPIs.
> 
> What do you think?
> 
This is a tricky one.  I think that in general, if the guest
misconfigures the level/edge of a virtual interrupt, it is only setting
itself up for trouble, which doesn't interfere with the host
functionality, so we don't have to worry about that.  What we do have to
worry is about is that the guest can't interfere with the host in
uninteded ways by playing with the interrupts.  Let's consider the two
scenarios:

Phys = level, Virt = Edge
-------------------------
These patches don't change anything from today's behavior.  We may loose
interrupts for the VM, tough.

Phys = edge, Virt = Level
-------------------------
In this case we will read the physical input level on return from the
VM.  That changes the virtual line_level, no harm done.  If we see the
input line to the VGIC (or physial pending state) is asserted, we will
deactivate a physical interrup which means, if it's actually pending,
the driver (VFIO or arch timer, typically) will take another
interrupt, no harm done.

So I think we're fine.


Thanks,
-Christoffer

[RFC PATCH 3/4] KVM: arm/arm64: Rearrange kvm_vgic_[un]map_phys code in vgic.c

[Christoffer Dall]

The small indirection of a static function made the locking very obvious
but becomes pretty ugly as we start passing function pointer around.
Let's inline these two functions first to make the following patch more
readable.

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

diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index 2691a0a..e3ce2fa 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -428,12 +428,17 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
 	return 0;
 }
 
-/* @irq->irq_lock must be held */
-static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
-			    unsigned int host_irq)
+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);
 	struct irq_desc *desc;
 	struct irq_data *data;
+	int ret = 0;
+
+	BUG_ON(!irq);
+
+	spin_lock(&irq->irq_lock);
 
 	/*
 	 * Find the physical IRQ number corresponding to @host_irq
@@ -441,7 +446,8 @@ static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
 	desc = irq_to_desc(host_irq);
 	if (!desc) {
 		kvm_err("%s: no interrupt descriptor\n", __func__);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out;
 	}
 	data = irq_desc_get_irq_data(desc);
 	while (data->parent_data)
@@ -450,29 +456,10 @@ 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;
-	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);
+out:
 	spin_unlock(&irq->irq_lock);
 	vgic_put_irq(vcpu->kvm, irq);
-
 	return ret;
 }
 
@@ -487,7 +474,8 @@ int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int vintid)
 	BUG_ON(!irq);
 
 	spin_lock(&irq->irq_lock);
-	kvm_vgic_unmap_irq(irq);
+	irq->hw = false;
+	irq->hwintid = 0;
 	spin_unlock(&irq->irq_lock);
 	vgic_put_irq(vcpu->kvm, irq);
 
-- 
2.9.0

[RFC PATCH 3/4] KVM: arm/arm64: Rearrange kvm_vgic_[un]map_phys code in vgic.c

[Christoffer Dall]

The small indirection of a static function made the locking very obvious
but becomes pretty ugly as we start passing function pointer around.
Let's inline these two functions first to make the following patch more
readable.

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

diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index 2691a0a..e3ce2fa 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -428,12 +428,17 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
 	return 0;
 }
 
-/* @irq->irq_lock must be held */
-static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
-			    unsigned int host_irq)
+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);
 	struct irq_desc *desc;
 	struct irq_data *data;
+	int ret = 0;
+
+	BUG_ON(!irq);
+
+	spin_lock(&irq->irq_lock);
 
 	/*
 	 * Find the physical IRQ number corresponding to @host_irq
@@ -441,7 +446,8 @@ static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
 	desc = irq_to_desc(host_irq);
 	if (!desc) {
 		kvm_err("%s: no interrupt descriptor\n", __func__);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out;
 	}
 	data = irq_desc_get_irq_data(desc);
 	while (data->parent_data)
@@ -450,29 +456,10 @@ 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;
-	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);
+out:
 	spin_unlock(&irq->irq_lock);
 	vgic_put_irq(vcpu->kvm, irq);
-
 	return ret;
 }
 
@@ -487,7 +474,8 @@ int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int vintid)
 	BUG_ON(!irq);
 
 	spin_lock(&irq->irq_lock);
-	kvm_vgic_unmap_irq(irq);
+	irq->hw = false;
+	irq->hwintid = 0;
 	spin_unlock(&irq->irq_lock);
 	vgic_put_irq(vcpu->kvm, irq);
 
-- 
2.9.0

Re: [RFC PATCH 3/4] KVM: arm/arm64: Rearrange kvm_vgic_[un]map_phys code in vgic.c

[Marc Zyngier]

On Tue, Aug 29 2017 at 11:39:01 am BST, Christoffer Dall <cdall@linaro.org> wrote:
> The small indirection of a static function made the locking very obvious
> but becomes pretty ugly as we start passing function pointer around.
> Let's inline these two functions first to make the following patch more
> readable.
>
> Signed-off-by: Christoffer Dall <cdall@linaro.org>

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

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

[RFC PATCH 3/4] KVM: arm/arm64: Rearrange kvm_vgic_[un]map_phys code in vgic.c

[Marc Zyngier]

On Tue, Aug 29 2017 at 11:39:01 am BST, Christoffer Dall <cdall@linaro.org> wrote:
> The small indirection of a static function made the locking very obvious
> but becomes pretty ugly as we start passing function pointer around.
> Let's inline these two functions first to make the following patch more
> readable.
>
> Signed-off-by: Christoffer Dall <cdall@linaro.org>

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

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

[RFC PATCH 4/4] 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>
---
 include/kvm/arm_vgic.h    | 13 ++++++++++++-
 virt/kvm/arm/arch_timer.c | 16 +++++++++++++++-
 virt/kvm/arm/vgic/vgic.c  |  7 ++++++-
 3 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 53f631b..a52990b 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 (*callback)(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 b24e2f7..82169ef 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -643,6 +643,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;
@@ -664,7 +677,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 e3ce2fa..0466c10 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -147,6 +147,9 @@ bool vgic_irq_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));
@@ -429,7 +432,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
 }
 
 int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
-			  u32 vintid)
+			  u32 vintid, bool (*callback)(int vindid))
 {
 	struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, vintid);
 	struct irq_desc *desc;
@@ -456,6 +459,7 @@ int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
 	irq->hw = true;
 	irq->host_irq = host_irq;
 	irq->hwintid = data->hwirq;
+	irq->get_input_level = callback;
 
 out:
 	spin_unlock(&irq->irq_lock);
@@ -476,6 +480,7 @@ int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int vintid)
 	spin_lock(&irq->irq_lock);
 	irq->hw = false;
 	irq->hwintid = 0;
+	irq->get_input_level = NULL;
 	spin_unlock(&irq->irq_lock);
 	vgic_put_irq(vcpu->kvm, irq);
 
-- 
2.9.0

[RFC PATCH 4/4] 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>
---
 include/kvm/arm_vgic.h    | 13 ++++++++++++-
 virt/kvm/arm/arch_timer.c | 16 +++++++++++++++-
 virt/kvm/arm/vgic/vgic.c  |  7 ++++++-
 3 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 53f631b..a52990b 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 (*callback)(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 b24e2f7..82169ef 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -643,6 +643,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;
@@ -664,7 +677,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 e3ce2fa..0466c10 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -147,6 +147,9 @@ bool vgic_irq_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));
@@ -429,7 +432,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
 }
 
 int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
-			  u32 vintid)
+			  u32 vintid, bool (*callback)(int vindid))
 {
 	struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, vintid);
 	struct irq_desc *desc;
@@ -456,6 +459,7 @@ int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
 	irq->hw = true;
 	irq->host_irq = host_irq;
 	irq->hwintid = data->hwirq;
+	irq->get_input_level = callback;
 
 out:
 	spin_unlock(&irq->irq_lock);
@@ -476,6 +480,7 @@ int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int vintid)
 	spin_lock(&irq->irq_lock);
 	irq->hw = false;
 	irq->hwintid = 0;
+	irq->get_input_level = NULL;
 	spin_unlock(&irq->irq_lock);
 	vgic_put_irq(vcpu->kvm, irq);
 
-- 
2.9.0

Re: [RFC PATCH 4/4] KVM: arm/arm64: Provide a vgic interrupt line level sample function

[Marc Zyngier]

On Tue, Aug 29 2017 at 11:39:02 am BST, Christoffer Dall <cdall@linaro.org> wrote:
> 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>
> ---
>  include/kvm/arm_vgic.h    | 13 ++++++++++++-
>  virt/kvm/arm/arch_timer.c | 16 +++++++++++++++-
>  virt/kvm/arm/vgic/vgic.c  |  7 ++++++-
>  3 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 53f631b..a52990b 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 (*callback)(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 b24e2f7..82169ef 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -643,6 +643,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;
> @@ -664,7 +677,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);

nit: no need for a & here.

>  	if (ret)
>  		return ret;
>  
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index e3ce2fa..0466c10 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -147,6 +147,9 @@ bool vgic_irq_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));
> @@ -429,7 +432,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>  }
>  
>  int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
> -			  u32 vintid)
> +			  u32 vintid, bool (*callback)(int vindid))

nit #2: "callback" is a very non-descriptive name for a callback... ;-)
How about calling it "get_input_level", matching the vgic_irq field?

>  {
>  	struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, vintid);
>  	struct irq_desc *desc;
> @@ -456,6 +459,7 @@ int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
>  	irq->hw = true;
>  	irq->host_irq = host_irq;
>  	irq->hwintid = data->hwirq;
> +	irq->get_input_level = callback;
>  
>  out:
>  	spin_unlock(&irq->irq_lock);
> @@ -476,6 +480,7 @@ int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int vintid)
>  	spin_lock(&irq->irq_lock);
>  	irq->hw = false;
>  	irq->hwintid = 0;
> +	irq->get_input_level = NULL;
>  	spin_unlock(&irq->irq_lock);
>  	vgic_put_irq(vcpu->kvm, irq);

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

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

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

[Marc Zyngier]

On Tue, Aug 29 2017 at 11:39:02 am BST, Christoffer Dall <cdall@linaro.org> wrote:
> 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>
> ---
>  include/kvm/arm_vgic.h    | 13 ++++++++++++-
>  virt/kvm/arm/arch_timer.c | 16 +++++++++++++++-
>  virt/kvm/arm/vgic/vgic.c  |  7 ++++++-
>  3 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 53f631b..a52990b 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 (*callback)(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 b24e2f7..82169ef 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -643,6 +643,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;
> @@ -664,7 +677,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);

nit: no need for a & here.

>  	if (ret)
>  		return ret;
>  
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index e3ce2fa..0466c10 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -147,6 +147,9 @@ bool vgic_irq_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));
> @@ -429,7 +432,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>  }
>  
>  int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
> -			  u32 vintid)
> +			  u32 vintid, bool (*callback)(int vindid))

nit #2: "callback" is a very non-descriptive name for a callback... ;-)
How about calling it "get_input_level", matching the vgic_irq field?

>  {
>  	struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, vintid);
>  	struct irq_desc *desc;
> @@ -456,6 +459,7 @@ int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
>  	irq->hw = true;
>  	irq->host_irq = host_irq;
>  	irq->hwintid = data->hwirq;
> +	irq->get_input_level = callback;
>  
>  out:
>  	spin_unlock(&irq->irq_lock);
> @@ -476,6 +480,7 @@ int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int vintid)
>  	spin_lock(&irq->irq_lock);
>  	irq->hw = false;
>  	irq->hwintid = 0;
> +	irq->get_input_level = NULL;
>  	spin_unlock(&irq->irq_lock);
>  	vgic_put_irq(vcpu->kvm, irq);

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

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

Re: [RFC PATCH 4/4] KVM: arm/arm64: Provide a vgic interrupt line level sample function

[Christoffer Dall]

On Sat, Sep 02, 2017 at 12:20:34PM +0100, Marc Zyngier wrote:
> On Tue, Aug 29 2017 at 11:39:02 am BST, Christoffer Dall <cdall@linaro.org> wrote:
> > 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>
> > ---
> >  include/kvm/arm_vgic.h    | 13 ++++++++++++-
> >  virt/kvm/arm/arch_timer.c | 16 +++++++++++++++-
> >  virt/kvm/arm/vgic/vgic.c  |  7 ++++++-
> >  3 files changed, 33 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> > index 53f631b..a52990b 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 (*callback)(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 b24e2f7..82169ef 100644
> > --- a/virt/kvm/arm/arch_timer.c
> > +++ b/virt/kvm/arm/arch_timer.c
> > @@ -643,6 +643,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;
> > @@ -664,7 +677,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);
> 
> nit: no need for a & here.
> 
> >  	if (ret)
> >  		return ret;
> >  
> > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> > index e3ce2fa..0466c10 100644
> > --- a/virt/kvm/arm/vgic/vgic.c
> > +++ b/virt/kvm/arm/vgic/vgic.c
> > @@ -147,6 +147,9 @@ bool vgic_irq_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));
> > @@ -429,7 +432,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
> >  }
> >  
> >  int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
> > -			  u32 vintid)
> > +			  u32 vintid, bool (*callback)(int vindid))
> 
> nit #2: "callback" is a very non-descriptive name for a callback... ;-)
> How about calling it "get_input_level", matching the vgic_irq field?

Yeah, totally.

> 
> >  {
> >  	struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, vintid);
> >  	struct irq_desc *desc;
> > @@ -456,6 +459,7 @@ int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
> >  	irq->hw = true;
> >  	irq->host_irq = host_irq;
> >  	irq->hwintid = data->hwirq;
> > +	irq->get_input_level = callback;
> >  
> >  out:
> >  	spin_unlock(&irq->irq_lock);
> > @@ -476,6 +480,7 @@ int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int vintid)
> >  	spin_lock(&irq->irq_lock);
> >  	irq->hw = false;
> >  	irq->hwintid = 0;
> > +	irq->get_input_level = NULL;
> >  	spin_unlock(&irq->irq_lock);
> >  	vgic_put_irq(vcpu->kvm, irq);
> 
> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
> 
Thanks!
-Christoffer

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

[Christoffer Dall]

On Sat, Sep 02, 2017 at 12:20:34PM +0100, Marc Zyngier wrote:
> On Tue, Aug 29 2017 at 11:39:02 am BST, Christoffer Dall <cdall@linaro.org> wrote:
> > 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>
> > ---
> >  include/kvm/arm_vgic.h    | 13 ++++++++++++-
> >  virt/kvm/arm/arch_timer.c | 16 +++++++++++++++-
> >  virt/kvm/arm/vgic/vgic.c  |  7 ++++++-
> >  3 files changed, 33 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> > index 53f631b..a52990b 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 (*callback)(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 b24e2f7..82169ef 100644
> > --- a/virt/kvm/arm/arch_timer.c
> > +++ b/virt/kvm/arm/arch_timer.c
> > @@ -643,6 +643,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;
> > @@ -664,7 +677,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);
> 
> nit: no need for a & here.
> 
> >  	if (ret)
> >  		return ret;
> >  
> > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> > index e3ce2fa..0466c10 100644
> > --- a/virt/kvm/arm/vgic/vgic.c
> > +++ b/virt/kvm/arm/vgic/vgic.c
> > @@ -147,6 +147,9 @@ bool vgic_irq_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));
> > @@ -429,7 +432,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
> >  }
> >  
> >  int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
> > -			  u32 vintid)
> > +			  u32 vintid, bool (*callback)(int vindid))
> 
> nit #2: "callback" is a very non-descriptive name for a callback... ;-)
> How about calling it "get_input_level", matching the vgic_irq field?

Yeah, totally.

> 
> >  {
> >  	struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, vintid);
> >  	struct irq_desc *desc;
> > @@ -456,6 +459,7 @@ int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
> >  	irq->hw = true;
> >  	irq->host_irq = host_irq;
> >  	irq->hwintid = data->hwirq;
> > +	irq->get_input_level = callback;
> >  
> >  out:
> >  	spin_unlock(&irq->irq_lock);
> > @@ -476,6 +480,7 @@ int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int vintid)
> >  	spin_lock(&irq->irq_lock);
> >  	irq->hw = false;
> >  	irq->hwintid = 0;
> > +	irq->get_input_level = NULL;
> >  	spin_unlock(&irq->irq_lock);
> >  	vgic_put_irq(vcpu->kvm, irq);
> 
> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
> 
Thanks!
-Christoffer