[RFC 0/4] vgic additions for forwarded irq

[RFC 0/4] vgic additions for forwarded irq

[Eric Auger]

This series applies on top of "ARM: Forwarding physical
interrupts to a guest VM" (http://lwn.net/Articles/603514/)
series.

It brings some extra functionalities that were requested to
be able to inject virtual level sensitive IRQs triggered from
VFIO/irqfd.

It adds:
- a specific handling of forwarded IRQ into the VGIC state machine.
- deactivation of physical IRQ and unforwarding on vgic destruction
- handling of forwarded IRQ injection before the vgic readiness:
  this was needed because in a sample qemu/vfio use case, qemu
  registers forwarded IRQ and set up VFIO signaling before the first
  vcpu run and hence before vgic readiness. At that time some
  physical IRQ may hit before the VGIC readiness. This is typically
  observed with Calxeda xgmac on second QEMU run.
- rbtree lock addition.

Integrated pieces can be found at
ssh://git.linaro.org/people/eric.auger/linux.git
on branch irqfd_integ_v8

The first 2 patch files were previously part of [RFC v2 0/9]
KVM-VFIO IRQ forward control (https://lkml.org/lkml/2014/9/1/347).


Eric Auger (4):
  KVM: arm: vgic: fix state machine for forwarded IRQ
  KVM: arm: vgic: add forwarded irq rbtree lock
  KVM: arm: vgic: cleanup forwarded IRQs on destroy
  KVM: arm: vgic: handle irqfd forwarded IRQ injection before vgic
    readiness

 include/kvm/arm_vgic.h |   1 +
 virt/kvm/arm/vgic.c    | 128 +++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 110 insertions(+), 19 deletions(-)

-- 
1.9.1

[RFC 1/4] KVM: arm: vgic: fix state machine for forwarded IRQ

[Eric Auger]

Fix multiple injection of level sensitive forwarded IRQs.
With current code, the second injection fails since the state bitmaps
are not reset (process_maintenance is not called anymore).

New implementation follows those principles:
- A forwarded IRQ only can be sampled when it is pending
- when queueing the IRQ (programming the LR), the pending state is removed
  as for edge sensitive IRQs
- an injection of a forwarded IRQ is considered always valid since
  coming from the HW and level always is 1.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v2 -> v3:
- integration in new vgic_can_sample_irq
- remove the pending state when programming the LR

v1 -> v2:
- fix vgic state bypass in vgic_queue_hwirq

Conflicts:
	virt/kvm/arm/vgic.c
---
 virt/kvm/arm/vgic.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index ccb3801..5484e3d 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -360,7 +360,10 @@ static void vgic_cpu_irq_clear(struct kvm_vcpu *vcpu, int irq)
 
 static bool vgic_can_sample_irq(struct kvm_vcpu *vcpu, int irq)
 {
-	return vgic_irq_is_edge(vcpu, irq) || !vgic_irq_is_queued(vcpu, irq);
+	bool is_forwarded =  (vgic_get_phys_irq(vcpu, irq) >= 0);
+
+	return vgic_irq_is_edge(vcpu, irq) || !vgic_irq_is_queued(vcpu, irq) ||
+		(is_forwarded && vgic_dist_irq_is_pending(vcpu, irq));
 }
 
 static u32 mmio_data_read(struct kvm_exit_mmio *mmio, u32 mask)
@@ -1295,6 +1298,7 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 	struct vgic_lr vlr;
 	int lr;
+	bool is_forwarded =  (vgic_get_phys_irq(vcpu, irq) >= 0);
 
 	/* Sanitize the input... */
 	BUG_ON(sgi_source_id & ~7);
@@ -1330,7 +1334,7 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
 	vlr.irq = irq;
 	vlr.source = sgi_source_id;
 	vlr.state = LR_STATE_PENDING;
-	if (!vgic_irq_is_edge(vcpu, irq))
+	if (!vgic_irq_is_edge(vcpu, irq) && !is_forwarded)
 		vlr.state |= LR_EOI_INT;
 
 	vgic_set_lr(vcpu, lr, vlr);
@@ -1371,11 +1375,12 @@ static bool vgic_queue_sgi(struct kvm_vcpu *vcpu, int irq)
 
 static bool vgic_queue_hwirq(struct kvm_vcpu *vcpu, int irq)
 {
+	bool is_forwarded = (vgic_get_phys_irq(vcpu, irq) >= 0);
 	if (!vgic_can_sample_irq(vcpu, irq))
 		return true; /* level interrupt, already queued */
 
 	if (vgic_queue_irq(vcpu, 0, irq)) {
-		if (vgic_irq_is_edge(vcpu, irq)) {
+		if (vgic_irq_is_edge(vcpu, irq) || is_forwarded) {
 			vgic_dist_irq_clear_pending(vcpu, irq);
 			vgic_cpu_irq_clear(vcpu, irq);
 		} else {
@@ -1636,14 +1641,17 @@ static bool vgic_update_irq_pending(struct kvm *kvm, int cpuid,
 	int edge_triggered, level_triggered;
 	int enabled;
 	bool ret = true;
+	bool is_forwarded;
 
 	spin_lock(&dist->lock);
 
 	vcpu = kvm_get_vcpu(kvm, cpuid);
+	is_forwarded = (vgic_get_phys_irq(vcpu, irq_num) >= 0);
+
 	edge_triggered = vgic_irq_is_edge(vcpu, irq_num);
 	level_triggered = !edge_triggered;
 
-	if (!vgic_validate_injection(vcpu, irq_num, level)) {
+	if (!vgic_validate_injection(vcpu, irq_num, level) && !is_forwarded) {
 		ret = false;
 		goto out;
 	}
-- 
1.9.1

[RFC 2/4] KVM: arm: vgic: add forwarded irq rbtree lock

[Eric Auger]

Add a lock related to the rb tree manipulation. The rb tree can be
searched in one thread (irqfd handler for instance) and map/unmap
may happen in another.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v2 -> v3:
re-arrange lock sequence in vgic_map_phys_irq
---
 include/kvm/arm_vgic.h |  1 +
 virt/kvm/arm/vgic.c    | 56 ++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index ea31ac6..9b3290b 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -220,6 +220,7 @@ struct vgic_dist {
 	unsigned long		*irq_pending_on_cpu;
 
 	struct rb_root		irq_phys_map;
+	spinlock_t			rb_tree_lock;
 #endif
 };
 
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 5484e3d..f592219 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1750,9 +1750,22 @@ static struct rb_root *vgic_get_irq_phys_map(struct kvm_vcpu *vcpu,
 
 int vgic_map_phys_irq(struct kvm_vcpu *vcpu, int virt_irq, int phys_irq)
 {
-	struct rb_root *root = vgic_get_irq_phys_map(vcpu, virt_irq);
-	struct rb_node **new = &root->rb_node, *parent = NULL;
+	struct rb_root *root;
+	struct rb_node **new, *parent = NULL;
 	struct irq_phys_map *new_map;
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+	root = vgic_get_irq_phys_map(vcpu, virt_irq);
+	new = &root->rb_node;
+
+	new_map = kzalloc(sizeof(*new_map), GFP_KERNEL);
+	if (!new_map)
+		return -ENOMEM;
+
+	new_map->virt_irq = virt_irq;
+	new_map->phys_irq = phys_irq;
+
+	spin_lock(&dist->rb_tree_lock);
 
 	/* Boilerplate rb_tree code */
 	while (*new) {
@@ -1764,19 +1777,16 @@ int vgic_map_phys_irq(struct kvm_vcpu *vcpu, int virt_irq, int phys_irq)
 			new = &(*new)->rb_left;
 		else if (this->virt_irq > virt_irq)
 			new = &(*new)->rb_right;
-		else
+		else {
+			kfree(new_map);
+			spin_unlock(&dist->rb_tree_lock);
 			return -EEXIST;
+		}
 	}
 
-	new_map = kzalloc(sizeof(*new_map), GFP_KERNEL);
-	if (!new_map)
-		return -ENOMEM;
-
-	new_map->virt_irq = virt_irq;
-	new_map->phys_irq = phys_irq;
-
 	rb_link_node(&new_map->node, parent, new);
 	rb_insert_color(&new_map->node, root);
+	spin_unlock(&dist->rb_tree_lock);
 
 	return 0;
 }
@@ -1805,24 +1815,39 @@ static struct irq_phys_map *vgic_irq_map_search(struct kvm_vcpu *vcpu,
 
 int vgic_get_phys_irq(struct kvm_vcpu *vcpu, int virt_irq)
 {
-	struct irq_phys_map *map = vgic_irq_map_search(vcpu, virt_irq);
+	struct irq_phys_map *map;
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+	int ret;
+
+	spin_lock(&dist->rb_tree_lock);
+	map = vgic_irq_map_search(vcpu, virt_irq);
 
 	if (map)
-		return map->phys_irq;
+		ret = map->phys_irq;
+	else
+		ret =  -ENOENT;
+
+	spin_unlock(&dist->rb_tree_lock);
+	return ret;
 
-	return -ENOENT;
 }
 
 int vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, int virt_irq, int phys_irq)
 {
-	struct irq_phys_map *map = vgic_irq_map_search(vcpu, virt_irq);
+	struct irq_phys_map *map;
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
+	spin_lock(&dist->rb_tree_lock);
+
+	map = vgic_irq_map_search(vcpu, virt_irq);
 
 	if (map && map->phys_irq == phys_irq) {
 		rb_erase(&map->node, vgic_get_irq_phys_map(vcpu, virt_irq));
 		kfree(map);
+		spin_unlock(&dist->rb_tree_lock);
 		return 0;
 	}
-
+	spin_unlock(&dist->rb_tree_lock);
 	return -ENOENT;
 }
 
@@ -2078,6 +2103,7 @@ int kvm_vgic_create(struct kvm *kvm)
 	}
 
 	spin_lock_init(&kvm->arch.vgic.lock);
+	spin_lock_init(&kvm->arch.vgic.rb_tree_lock);
 	kvm->arch.vgic.in_kernel = true;
 	kvm->arch.vgic.vctrl_base = vgic->vctrl_base;
 	kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
-- 
1.9.1

[RFC 3/4] KVM: arm: vgic: cleanup forwarded IRQs on destroy

[Eric Auger]

When the VGIC is destroyed it must take care of
- restoring the forwarded IRQs in non forwarded state,
- deactivating the IRQ in case the guest left without doing it
- cleaning nodes of the phys_map rbtree

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 virt/kvm/arm/vgic.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index f592219..21419ac 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -32,6 +32,7 @@
 #include <asm/kvm_emulate.h>
 #include <asm/kvm_arm.h>
 #include <asm/kvm_mmu.h>
+#include <linux/spinlock.h>
 
 /*
  * How the whole thing works (courtesy of Christoffer Dall):
@@ -102,6 +103,8 @@ static struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr);
 static void vgic_set_lr(struct kvm_vcpu *vcpu, int lr, struct vgic_lr lr_desc);
 static void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
 static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
+static void vgic_clean_irq_phys_map(struct kvm_vcpu *vcpu,
+				    struct rb_root *root);
 
 static const struct vgic_ops *vgic_ops;
 static const struct vgic_params *vgic;
@@ -1813,6 +1816,36 @@ static struct irq_phys_map *vgic_irq_map_search(struct kvm_vcpu *vcpu,
 	return NULL;
 }
 
+static void vgic_clean_irq_phys_map(struct kvm_vcpu *vcpu,
+				    struct rb_root *root)
+{
+	unsigned long flags;
+
+	while (1) {
+		struct rb_node *node = rb_first(root);
+		struct irq_phys_map *map;
+		struct irq_desc *desc;
+		struct irq_data *d;
+		struct irq_chip *chip;
+
+		if (!node)
+			break;
+
+		map = container_of(node, struct irq_phys_map, node);
+		desc = irq_to_desc(map->phys_irq);
+
+		raw_spin_lock_irqsave(&desc->lock, flags);
+		d = &desc->irq_data;
+		chip = desc->irq_data.chip;
+		irqd_clr_irq_forwarded(d);
+		chip->irq_eoi(d);
+		raw_spin_unlock_irqrestore(&desc->lock, flags);
+
+		rb_erase(node, root);
+		kfree(map);
+	}
+}
+
 int vgic_get_phys_irq(struct kvm_vcpu *vcpu, int virt_irq)
 {
 	struct irq_phys_map *map;
@@ -1855,6 +1888,7 @@ void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
 {
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 
+	vgic_clean_irq_phys_map(vcpu, &vgic_cpu->irq_phys_map);
 	kfree(vgic_cpu->pending_shared);
 	kfree(vgic_cpu->vgic_irq_lr_map);
 	vgic_cpu->pending_shared = NULL;
@@ -1920,6 +1954,8 @@ void kvm_vgic_destroy(struct kvm *kvm)
 	kvm_for_each_vcpu(i, vcpu, kvm)
 		kvm_vgic_vcpu_destroy(vcpu);
 
+	vgic_clean_irq_phys_map(vcpu, &dist->irq_phys_map);
+
 	vgic_free_bitmap(&dist->irq_enabled);
 	vgic_free_bitmap(&dist->irq_level);
 	vgic_free_bitmap(&dist->irq_pending);
-- 
1.9.1

[RFC 4/4] KVM: arm: vgic: handle irqfd forwarded IRQ injection before vgic readiness

[Eric Auger]

This patch handles the case where irqfd attempts to inject
a forwarded IRQ before the vgic is ready to accept it. We
cannot simply return pretending nothing happened since the IRQ
will never be deactivated by the guest. Indeed, the corresponding
virtual IRQ cannot be injected into the guest and the host
must deactivate it.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 virt/kvm/arm/vgic.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 21419ac..27394b0 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -2672,6 +2672,13 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id,
 		u32 irq, int level, bool line_status)
 {
 	unsigned int spi = irq + VGIC_NR_PRIVATE_IRQS;
+	struct kvm_vcpu *vcpu = kvm_get_vcpu(kvm, 0);
+	int phys_irq =  vgic_get_phys_irq(vcpu, spi);
+	bool is_forwarded = (phys_irq >= 0);
+	unsigned long flags;
+	struct irq_desc *desc;
+	struct irq_data *d;
+	struct irq_chip *chip;
 
 	kvm_debug("irqfd sets vIRQ %d to %d\n", irq, level);
 
@@ -2679,6 +2686,19 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id,
 		if (spi > kvm->arch.vgic.nr_irqs)
 			return -EINVAL;
 		return kvm_vgic_inject_irq(kvm, 0, spi, level);
+	}
+
+	if (level && is_forwarded) {
+		/*
+		 * the virtual IRQ never is injected into the guest
+		 * it must be manually completed on host side.
+		 */
+		desc = irq_to_desc(phys_irq);
+		raw_spin_lock_irqsave(&desc->lock, flags);
+		d = &desc->irq_data;
+		chip = desc->irq_data.chip;
+		chip->irq_eoi(d);
+		raw_spin_unlock_irqrestore(&desc->lock, flags);
 	} else if (level && kvm_irq_has_notifier(kvm, 0, irq)) {
 		/*
 		 * any IRQ injected before vgic readiness is
-- 
1.9.1

Re: [RFC 0/4] vgic additions for forwarded irq

[Christoffer Dall]

On Sun, Nov 23, 2014 at 07:12:49PM +0100, Eric Auger wrote:
> This series applies on top of "ARM: Forwarding physical
> interrupts to a guest VM" (http://lwn.net/Articles/603514/)
> series.

Marc and Eric,

Does it make sense to review and look at these patches given the current
state of the forwarding patches, or should we wait until Marc respins
that series?

> 
> It brings some extra functionalities that were requested to
> be able to inject virtual level sensitive IRQs triggered from
> VFIO/irqfd.
> 
> It adds:
> - a specific handling of forwarded IRQ into the VGIC state machine.
> - deactivation of physical IRQ and unforwarding on vgic destruction
> - handling of forwarded IRQ injection before the vgic readiness:
>   this was needed because in a sample qemu/vfio use case, qemu
>   registers forwarded IRQ and set up VFIO signaling before the first
>   vcpu run and hence before vgic readiness. At that time some
>   physical IRQ may hit before the VGIC readiness. This is typically
>   observed with Calxeda xgmac on second QEMU run.

this seems related to my note in the last patch?  Same or different
problem?

-Christoffer

> - rbtree lock addition.
> 
> Integrated pieces can be found at
> ssh://git.linaro.org/people/eric.auger/linux.git
> on branch irqfd_integ_v8
> 
> The first 2 patch files were previously part of [RFC v2 0/9]
> KVM-VFIO IRQ forward control (https://lkml.org/lkml/2014/9/1/347).
> 
> 
> Eric Auger (4):
>   KVM: arm: vgic: fix state machine for forwarded IRQ
>   KVM: arm: vgic: add forwarded irq rbtree lock
>   KVM: arm: vgic: cleanup forwarded IRQs on destroy
>   KVM: arm: vgic: handle irqfd forwarded IRQ injection before vgic
>     readiness
> 
>  include/kvm/arm_vgic.h |   1 +
>  virt/kvm/arm/vgic.c    | 128 +++++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 110 insertions(+), 19 deletions(-)
> 
> -- 
> 1.9.1
> 

Re: [RFC 0/4] vgic additions for forwarded irq

[Marc Zyngier]

On 24/11/14 10:50, Christoffer Dall wrote:
> On Sun, Nov 23, 2014 at 07:12:49PM +0100, Eric Auger wrote:
>> This series applies on top of "ARM: Forwarding physical
>> interrupts to a guest VM" (http://lwn.net/Articles/603514/)
>> series.
> 
> Marc and Eric,
> 
> Does it make sense to review and look at these patches given the current
> state of the forwarding patches, or should we wait until Marc respins
> that series?

I'm still in the process of respining all of this (we've had a few
iterations with tglx, but I got sidetracked with the ITS/v2m/MSI side of
things).

I'll get back onto this this week. Hopefully.

Thanks,

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

Re: [RFC 0/4] vgic additions for forwarded irq

[Eric Auger]

On 11/24/2014 11:54 AM, Marc Zyngier wrote:
> On 24/11/14 10:50, Christoffer Dall wrote:
>> On Sun, Nov 23, 2014 at 07:12:49PM +0100, Eric Auger wrote:
>>> This series applies on top of "ARM: Forwarding physical
>>> interrupts to a guest VM" (http://lwn.net/Articles/603514/)
>>> series.
>>
>> Marc and Eric,
>>
>> Does it make sense to review and look at these patches given the current
>> state of the forwarding patches, or should we wait until Marc respins
>> that series?
> 
> I'm still in the process of respining all of this (we've had a few
> iterations with tglx, but I got sidetracked with the ITS/v2m/MSI side of
> things).

Hi Marc, Christoffer,

for your info, I integrated with kvm-arm64/irq-forward branch prior to
ITS integration, not with the RFC.

Best Regards

Eric

> 
> I'll get back onto this this week. Hopefully.
> 
> Thanks,
> 
> 	M.
>