Skip to contentSkip to conversation

KVM: arm/arm64: Clean up some obsolete code

PATCH Superseded Pavel Fedin 4 participants 15 messages Oct 2, 2015 – Oct 23, 2015

Subsystems (2)
KVM KVM arm64

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Pavel Fedin

author
#0 lore raw

cover KVM: arm/arm64: Clean up some obsolete code

Current KVM code has lots of old redundancies, which can be cleaned up. This patchset is actually a better alternative to
http://www.spinics.net/lists/arm-kernel/msg430726.html, which allows to keep piggy-backed LRs. The idea is based on the fact that our code also maintains LR state in elrsr, and this information is enough to track LR usage.

This patchset is made against linux-next of 02.10.2015. Thanks to Andre for pointing out some 4.3 specifics.

Pavel Fedin (2):
Optimize away redundant LR tracking
Merge vgic_set_lr() and vgic_sync_lr_elrsr()

 include/kvm/arm_vgic.h |   7 ----
 virt/kvm/arm/vgic-v2.c |   5 ---
 virt/kvm/arm/vgic-v3.c |   5 ---
 virt/kvm/arm/vgic.c    | 104 +++++++++++++++----------------------------------
 4 files changed, 32 insertions(+), 89 deletions(-)
Signature · 1 line
2.4.4
author
#1 lore raw

PATCH 1/2 KVM: arm/arm64: Optimize away redundant LR tracking

Currently we use vgic_irq_lr_map in order to track which LRs hold which IRQs, and lr_used bitmap in order to track which LRs are used or free.

vgic_irq_lr_map is actually used only for piggy-back optimization, and can be easily replaced by iteration over lr_used. This is good because in future, when LPI support is introduced, number of IRQs will grow up to at least 16384, while numbers from 1024 to 8192 are never going to be used. This would be a huge memory waste.

In its turn, lr_used is also completely redundant since ae705930fca6322600690df9dc1c7d0516145a93 ("arm/arm64: KVM: Keep elrsr/aisr in sync with software model"), because together with lr_used we also update elrsr. This allows to easily replace lr_used with elrsr, inverting all conditions (because in elrsr '1' means 'free').

Signed-off-by Pavel Fedin <[email protected]>
---
 include/kvm/arm_vgic.h |  6 ----
 virt/kvm/arm/vgic.c    | 74 +++++++++++++++++++-------------------------------
 2 files changed, 28 insertions(+), 52 deletions(-)
include/kvm/arm_vgic.h +0 -6
index 4e14dac..d908028 100644
@@ -296,9 +296,6 @@ struct vgic_v3_cpu_if {
 };
 
 struct vgic_cpu {
-	/* per IRQ to LR mapping */
-	u8		*vgic_irq_lr_map;
-
 	/* Pending/active/both interrupts on this VCPU */
 	DECLARE_BITMAP(	pending_percpu, VGIC_NR_PRIVATE_IRQS);
 	DECLARE_BITMAP(	active_percpu, VGIC_NR_PRIVATE_IRQS);

@@ -309,9 +306,6 @@ struct vgic_cpu {
 	unsigned long   *active_shared;
 	unsigned long   *pend_act_shared;
 
-	/* Bitmap of used/free list registers */
-	DECLARE_BITMAP(	lr_used, VGIC_V2_MAX_LRS);
-
 	/* Number of list registers on this CPU */
 	int		nr_lr;
virt/kvm/arm/vgic.c +28 -46

203 lines not on this page · raw message

Signature · 1 line
2.4.4
#2 lore raw

Re: [PATCH 1/2] KVM: arm/arm64: Optimize away redundant LR tracking

Hi,

Quoted 9 lines · Pavel Fedin

Currently we use vgic_irq_lr_map in order to track which LRs hold which IRQs, and lr_used bitmap in order to track which LRs are used or free.

vgic_irq_lr_map is actually used only for piggy-back optimization, and can be easily replaced by iteration over lr_used. This is good because in future, when LPI support is introduced, number of IRQs will grow up to at least 16384, while numbers from 1024 to 8192 are never going to be used. This would be a huge memory waste.

Quoted 5 lines

In its turn, lr_used is also completely redundant since ae705930fca6322600690df9dc1c7d0516145a93 ("arm/arm64: KVM: Keep elrsr/aisr in sync with software model"), because together with lr_used we also update elrsr. This allows to easily replace lr_used with elrsr, inverting all conditions (because in elrsr '1' means 'free').

So this looks pretty good to me, probably a better (because less intrusive) solution than my first two patches of the ITS emulation, which have a very similar scope.
I will give this some testing on my boxes here to spot any regressions, but I guess I will use these two patches as the base for my next version of the ITS emulation series.

Christoffer, Marc, do you consider these for 4.4 (since they are an independent cleanup) or do you want them to be part of the ITS emulation series since they make more sense in there?

Cheers,
Andre.

Quoted 252 lines
Signed-off-by Pavel Fedin <[email protected]>
---
 include/kvm/arm_vgic.h |  6 ----
 virt/kvm/arm/vgic.c    | 74 +++++++++++++++++++-------------------------------
 2 files changed, 28 insertions(+), 52 deletions(-)
include/kvm/arm_vgic.h +0 -6
index 4e14dac..d908028 100644
@@ -296,9 +296,6 @@ struct vgic_v3_cpu_if {
 };
 
 struct vgic_cpu {
-	/* per IRQ to LR mapping */
-	u8		*vgic_irq_lr_map;
-
 	/* Pending/active/both interrupts on this VCPU */
 	DECLARE_BITMAP(	pending_percpu, VGIC_NR_PRIVATE_IRQS);
 	DECLARE_BITMAP(	active_percpu, VGIC_NR_PRIVATE_IRQS);

@@ -309,9 +306,6 @@ struct vgic_cpu {
 	unsigned long   *active_shared;
 	unsigned long   *pend_act_shared;
 
-	/* Bitmap of used/free list registers */
-	DECLARE_BITMAP(	lr_used, VGIC_V2_MAX_LRS);
-
 	/* Number of list registers on this CPU */
 	int		nr_lr;
virt/kvm/arm/vgic.c +28 -46
index 6bd1c9b..2f4d25a 100644
@@ -102,9 +102,10 @@
 #include "vgic.h"
 
 static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
-static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu);
+static void vgic_retire_lr(int lr_nr, struct kvm_vcpu *vcpu);
 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 u64 vgic_get_elrsr(struct kvm_vcpu *vcpu);
 static struct irq_phys_map *vgic_irq_map_search(struct kvm_vcpu *vcpu,
 						int virt_irq);
 

@@ -683,9 +684,11 @@ bool vgic_handle_cfg_reg(u32 *reg, struct kvm_exit_mmio *mmio,
 void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
 {
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+	u64 elrsr = vgic_get_elrsr(vcpu);
+	unsigned long *elrsr_ptr = u64_to_bitmask(&elrsr);
 	int i;
 
-	for_each_set_bit(i, vgic_cpu->lr_used, vgic_cpu->nr_lr) {
+	for_each_clear_bit(i, elrsr_ptr, vgic_cpu->nr_lr) {
 		struct vgic_lr lr = vgic_get_lr(vcpu, i);
 
 		/*

@@ -728,7 +731,7 @@ void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
 		 * Mark the LR as free for other use.
 		 */
 		BUG_ON(lr.state & LR_STATE_MASK);
-		vgic_retire_lr(i, lr.irq, vcpu);
+		vgic_retire_lr(i, vcpu);
 		vgic_irq_clear_queued(vcpu, lr.irq);
 
 		/* Finally update the VGIC state. */

@@ -1087,15 +1090,12 @@ static inline void vgic_enable(struct kvm_vcpu *vcpu)
 	vgic_ops->enable(vcpu);
 }
 
-static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu)
+static void vgic_retire_lr(int lr_nr, struct kvm_vcpu *vcpu)
 {
-	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_lr vlr = vgic_get_lr(vcpu, lr_nr);
 
 	vlr.state = 0;
 	vgic_set_lr(vcpu, lr_nr, vlr);
-	clear_bit(lr_nr, vgic_cpu->lr_used);
-	vgic_cpu->vgic_irq_lr_map[irq] = LR_EMPTY;
 	vgic_sync_lr_elrsr(vcpu, lr_nr, vlr);
 }
 

@@ -1110,14 +1110,15 @@ static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu)
  */
 static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu)
 {
-	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+	u64 elrsr = vgic_get_elrsr(vcpu);
+	unsigned long *elrsr_ptr = u64_to_bitmask(&elrsr);
 	int lr;
 
-	for_each_set_bit(lr, vgic_cpu->lr_used, vgic->nr_lr) {
+	for_each_clear_bit(lr, elrsr_ptr, vgic->nr_lr) {
 		struct vgic_lr vlr = vgic_get_lr(vcpu, lr);
 
 		if (!vgic_irq_is_enabled(vcpu, vlr.irq)) {
-			vgic_retire_lr(lr, vlr.irq, vcpu);
+			vgic_retire_lr(lr, vcpu);
 			if (vgic_irq_is_queued(vcpu, vlr.irq))
 				vgic_irq_clear_queued(vcpu, vlr.irq);
 		}

@@ -1169,8 +1170,9 @@ static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq,
  */
 bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
 {
-	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+	u64 elrsr = vgic_get_elrsr(vcpu);
+	unsigned long *elrsr_ptr = u64_to_bitmask(&elrsr);
 	struct vgic_lr vlr;
 	int lr;
 

@@ -1181,28 +1183,22 @@ bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
 
 	kvm_debug("Queue IRQ%d\n", irq);
 
-	lr = vgic_cpu->vgic_irq_lr_map[irq];
-
 	/* Do we have an active interrupt for the same CPUID? */
-	if (lr != LR_EMPTY) {
+	for_each_clear_bit(lr, elrsr_ptr, vgic->nr_lr) {
 		vlr = vgic_get_lr(vcpu, lr);
-		if (vlr.source == sgi_source_id) {
+		if (vlr.irq == irq && vlr.source == sgi_source_id) {
 			kvm_debug("LR%d piggyback for IRQ%d\n", lr, vlr.irq);
-			BUG_ON(!test_bit(lr, vgic_cpu->lr_used));
 			vgic_queue_irq_to_lr(vcpu, irq, lr, vlr);
 			return true;
 		}
 	}
 
 	/* Try to use another LR for this interrupt */
-	lr = find_first_zero_bit((unsigned long *)vgic_cpu->lr_used,
-			       vgic->nr_lr);
+	lr = find_first_bit(elrsr_ptr, vgic->nr_lr);
 	if (lr >= vgic->nr_lr)
 		return false;
 
 	kvm_debug("LR%d allocated for IRQ%d %x\n", lr, irq, sgi_source_id);
-	vgic_cpu->vgic_irq_lr_map[irq] = lr;
-	set_bit(lr, vgic_cpu->lr_used);
 
 	vlr.irq = irq;
 	vlr.source = sgi_source_id;

@@ -1243,6 +1239,8 @@ static void __kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
 	int i, vcpu_id, lr, ret;
 	int overflow = 0;
 	int nr_shared = vgic_nr_shared_irqs(dist);
+	u64 elrsr;
+	unsigned long *elrsr_ptr;
 
 	vcpu_id = vcpu->vcpu_id;
 

@@ -1296,13 +1294,11 @@ epilog:
 		clear_bit(vcpu_id, dist->irq_pending_on_cpu);
 	}
 
-	for (lr = 0; lr < vgic->nr_lr; lr++) {
-		struct vgic_lr vlr;
-
-		if (!test_bit(lr, vgic_cpu->lr_used))
-			continue;
+	elrsr = vgic_get_elrsr(vcpu);
+	elrsr_ptr = u64_to_bitmask(&elrsr);
 
-		vlr = vgic_get_lr(vcpu, lr);
+	for_each_clear_bit(lr, elrsr_ptr, vgic->nr_lr) {
+		struct vgic_lr vlr = vgic_get_lr(vcpu, lr);
 
 		/*
 		 * If we have a mapping, and the virtual interrupt is

@@ -1443,7 +1439,6 @@ static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr)
 /* Sync back the VGIC state after a guest run */
 static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
 {
-	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 	u64 elrsr;
 	unsigned long *elrsr_ptr;

@@ -1456,12 +1451,8 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
 
 	/* Deal with HW interrupts, and clear mappings for empty LRs */
 	for (lr = 0; lr < vgic->nr_lr; lr++) {
-		struct vgic_lr vlr;
-
-		if (!test_bit(lr, vgic_cpu->lr_used))
-			continue;
+		struct vgic_lr vlr = vgic_get_lr(vcpu, lr);
 
-		vlr = vgic_get_lr(vcpu, lr);
 		if (vgic_sync_hwirq(vcpu, vlr)) {
 			/*
 			 * So this is a HW interrupt that the guest

@@ -1474,14 +1465,6 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
 			vgic_irq_clear_queued(vcpu, vlr.irq);
 			set_bit(lr, elrsr_ptr);
 		}
-
-		if (!test_bit(lr, elrsr_ptr))
-			continue;
-
-		clear_bit(lr, vgic_cpu->lr_used);
-
-		BUG_ON(vlr.irq >= dist->nr_irqs);
-		vgic_cpu->vgic_irq_lr_map[vlr.irq] = LR_EMPTY;
 	}
 
 	/* Check if we still have something up our sleeve... */

@@ -1927,33 +1910,32 @@ void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
 	kfree(vgic_cpu->pending_shared);
 	kfree(vgic_cpu->active_shared);
 	kfree(vgic_cpu->pend_act_shared);
-	kfree(vgic_cpu->vgic_irq_lr_map);
 	vgic_destroy_irq_phys_map(vcpu->kvm, &vgic_cpu->irq_phys_map_list);
 	vgic_cpu->pending_shared = NULL;
 	vgic_cpu->active_shared = NULL;
 	vgic_cpu->pend_act_shared = NULL;
-	vgic_cpu->vgic_irq_lr_map = NULL;
 }
 
 static int vgic_vcpu_init_maps(struct kvm_vcpu *vcpu, int nr_irqs)
 {
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+	int lr;
 
 	int sz = (nr_irqs - VGIC_NR_PRIVATE_IRQS) / 8;
 	vgic_cpu->pending_shared = kzalloc(sz, GFP_KERNEL);
 	vgic_cpu->active_shared = kzalloc(sz, GFP_KERNEL);
 	vgic_cpu->pend_act_shared = kzalloc(sz, GFP_KERNEL);
-	vgic_cpu->vgic_irq_lr_map = kmalloc(nr_irqs, GFP_KERNEL);
 
 	if (!vgic_cpu->pending_shared
 		|| !vgic_cpu->active_shared
-		|| !vgic_cpu->pend_act_shared
-		|| !vgic_cpu->vgic_irq_lr_map) {
+		|| !vgic_cpu->pend_act_shared) {
 		kvm_vgic_vcpu_destroy(vcpu);
 		return -ENOMEM;
 	}
 
-	memset(vgic_cpu->vgic_irq_lr_map, LR_EMPTY, nr_irqs);
+	/* This initializes elrsr to "all empty" */
+	for (lr = 0; lr < vgic->nr_lr; lr++)
+		vgic_retire_lr(lr, vcpu);
 
 	/*
 	 * Store the number of LRs per vcpu, so we don't have to go
#3 lore raw

Re: [PATCH 1/2] KVM: arm/arm64: Optimize away redundant LR tracking

Quoted 25 lines · Andre Przywara

Hi,

Quoted 9 lines · Pavel Fedin

Currently we use vgic_irq_lr_map in order to track which LRs hold which IRQs, and lr_used bitmap in order to track which LRs are used or free.

vgic_irq_lr_map is actually used only for piggy-back optimization, and can be easily replaced by iteration over lr_used. This is good because in future, when LPI support is introduced, number of IRQs will grow up to at least 16384, while numbers from 1024 to 8192 are never going to be used. This would be a huge memory waste.

Quoted 5 lines

In its turn, lr_used is also completely redundant since ae705930fca6322600690df9dc1c7d0516145a93 ("arm/arm64: KVM: Keep elrsr/aisr in sync with software model"), because together with lr_used we also update elrsr. This allows to easily replace lr_used with elrsr, inverting all conditions (because in elrsr '1' means 'free').

So this looks pretty good to me, probably a better (because less intrusive) solution than my first two patches of the ITS emulation, which have a very similar scope.
I will give this some testing on my boxes here to spot any regressions, but I guess I will use these two patches as the base for my next version of the ITS emulation series.

Quoted 4 lines

Christoffer, Marc, do you consider these for 4.4 (since they are an independent cleanup) or do you want them to be part of the ITS emulation series since they make more sense in there?

I'll try to have a look at these tomorrow and I'll think about how to merge this after I've looked at them.

Did you review these to the point where you can give your r-b tag here?

Thanks,
-Christoffer

#4 lore raw

Re: [PATCH 1/2] KVM: arm/arm64: Optimize away redundant LR tracking

In reply to Pavel Fedin
Quoted 198 lines · Pavel Fedin

Currently we use vgic_irq_lr_map in order to track which LRs hold which IRQs, and lr_used bitmap in order to track which LRs are used or free.

vgic_irq_lr_map is actually used only for piggy-back optimization, and can be easily replaced by iteration over lr_used. This is good because in future, when LPI support is introduced, number of IRQs will grow up to at least 16384, while numbers from 1024 to 8192 are never going to be used. This would be a huge memory waste.

In its turn, lr_used is also completely redundant since ae705930fca6322600690df9dc1c7d0516145a93 ("arm/arm64: KVM: Keep elrsr/aisr in sync with software model"), because together with lr_used we also update elrsr. This allows to easily replace lr_used with elrsr, inverting all conditions (because in elrsr '1' means 'free').

Signed-off-by Pavel Fedin <[email protected]>
---
 include/kvm/arm_vgic.h |  6 ----
 virt/kvm/arm/vgic.c    | 74 +++++++++++++++++++-------------------------------
 2 files changed, 28 insertions(+), 52 deletions(-)
include/kvm/arm_vgic.h +0 -6
index 4e14dac..d908028 100644
@@ -296,9 +296,6 @@ struct vgic_v3_cpu_if {
 };
 
 struct vgic_cpu {
-	/* per IRQ to LR mapping */
-	u8		*vgic_irq_lr_map;
-
 	/* Pending/active/both interrupts on this VCPU */
 	DECLARE_BITMAP(	pending_percpu, VGIC_NR_PRIVATE_IRQS);
 	DECLARE_BITMAP(	active_percpu, VGIC_NR_PRIVATE_IRQS);

@@ -309,9 +306,6 @@ struct vgic_cpu {
 	unsigned long   *active_shared;
 	unsigned long   *pend_act_shared;
 
-	/* Bitmap of used/free list registers */
-	DECLARE_BITMAP(	lr_used, VGIC_V2_MAX_LRS);
-
 	/* Number of list registers on this CPU */
 	int		nr_lr;
virt/kvm/arm/vgic.c +22 -27
index 6bd1c9b..2f4d25a 100644
@@ -102,9 +102,10 @@
 #include "vgic.h"
 
 static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
-static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu);
+static void vgic_retire_lr(int lr_nr, struct kvm_vcpu *vcpu);
 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 u64 vgic_get_elrsr(struct kvm_vcpu *vcpu);
 static struct irq_phys_map *vgic_irq_map_search(struct kvm_vcpu *vcpu,
 						int virt_irq);
 

@@ -683,9 +684,11 @@ bool vgic_handle_cfg_reg(u32 *reg, struct kvm_exit_mmio *mmio,
 void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
 {
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+	u64 elrsr = vgic_get_elrsr(vcpu);
+	unsigned long *elrsr_ptr = u64_to_bitmask(&elrsr);
 	int i;
 
-	for_each_set_bit(i, vgic_cpu->lr_used, vgic_cpu->nr_lr) {
+	for_each_clear_bit(i, elrsr_ptr, vgic_cpu->nr_lr) {
 		struct vgic_lr lr = vgic_get_lr(vcpu, i);
 
 		/*

@@ -728,7 +731,7 @@ void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
 		 * Mark the LR as free for other use.
 		 */
 		BUG_ON(lr.state & LR_STATE_MASK);
-		vgic_retire_lr(i, lr.irq, vcpu);
+		vgic_retire_lr(i, vcpu);
 		vgic_irq_clear_queued(vcpu, lr.irq);
 
 		/* Finally update the VGIC state. */

@@ -1087,15 +1090,12 @@ static inline void vgic_enable(struct kvm_vcpu *vcpu)
 	vgic_ops->enable(vcpu);
 }
 
-static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu)
+static void vgic_retire_lr(int lr_nr, struct kvm_vcpu *vcpu)
 {
-	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_lr vlr = vgic_get_lr(vcpu, lr_nr);
 
 	vlr.state = 0;
 	vgic_set_lr(vcpu, lr_nr, vlr);
-	clear_bit(lr_nr, vgic_cpu->lr_used);
-	vgic_cpu->vgic_irq_lr_map[irq] = LR_EMPTY;
 	vgic_sync_lr_elrsr(vcpu, lr_nr, vlr);
 }
 

@@ -1110,14 +1110,15 @@ static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu)
  */
 static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu)
 {
-	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+	u64 elrsr = vgic_get_elrsr(vcpu);
+	unsigned long *elrsr_ptr = u64_to_bitmask(&elrsr);
 	int lr;
 
-	for_each_set_bit(lr, vgic_cpu->lr_used, vgic->nr_lr) {
+	for_each_clear_bit(lr, elrsr_ptr, vgic->nr_lr) {
 		struct vgic_lr vlr = vgic_get_lr(vcpu, lr);
 
 		if (!vgic_irq_is_enabled(vcpu, vlr.irq)) {
-			vgic_retire_lr(lr, vlr.irq, vcpu);
+			vgic_retire_lr(lr, vcpu);
 			if (vgic_irq_is_queued(vcpu, vlr.irq))
 				vgic_irq_clear_queued(vcpu, vlr.irq);
 		}

@@ -1169,8 +1170,9 @@ static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq,
  */
 bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
 {
-	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+	u64 elrsr = vgic_get_elrsr(vcpu);
+	unsigned long *elrsr_ptr = u64_to_bitmask(&elrsr);
 	struct vgic_lr vlr;
 	int lr;
 

@@ -1181,28 +1183,22 @@ bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
 
 	kvm_debug("Queue IRQ%d\n", irq);
 
-	lr = vgic_cpu->vgic_irq_lr_map[irq];
-
 	/* Do we have an active interrupt for the same CPUID? */
-	if (lr != LR_EMPTY) {
+	for_each_clear_bit(lr, elrsr_ptr, vgic->nr_lr) {
 		vlr = vgic_get_lr(vcpu, lr);
-		if (vlr.source == sgi_source_id) {
+		if (vlr.irq == irq && vlr.source == sgi_source_id) {
 			kvm_debug("LR%d piggyback for IRQ%d\n", lr, vlr.irq);
-			BUG_ON(!test_bit(lr, vgic_cpu->lr_used));
 			vgic_queue_irq_to_lr(vcpu, irq, lr, vlr);
 			return true;
 		}
 	}
 
 	/* Try to use another LR for this interrupt */
-	lr = find_first_zero_bit((unsigned long *)vgic_cpu->lr_used,
-			       vgic->nr_lr);
+	lr = find_first_bit(elrsr_ptr, vgic->nr_lr);
 	if (lr >= vgic->nr_lr)
 		return false;
 
 	kvm_debug("LR%d allocated for IRQ%d %x\n", lr, irq, sgi_source_id);
-	vgic_cpu->vgic_irq_lr_map[irq] = lr;
-	set_bit(lr, vgic_cpu->lr_used);
 
 	vlr.irq = irq;
 	vlr.source = sgi_source_id;

@@ -1243,6 +1239,8 @@ static void __kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
 	int i, vcpu_id, lr, ret;
 	int overflow = 0;
 	int nr_shared = vgic_nr_shared_irqs(dist);
+	u64 elrsr;
+	unsigned long *elrsr_ptr;
 
 	vcpu_id = vcpu->vcpu_id;
 

@@ -1296,13 +1294,11 @@ epilog:
 		clear_bit(vcpu_id, dist->irq_pending_on_cpu);
 	}
 
-	for (lr = 0; lr < vgic->nr_lr; lr++) {
-		struct vgic_lr vlr;
-
-		if (!test_bit(lr, vgic_cpu->lr_used))
-			continue;
+	elrsr = vgic_get_elrsr(vcpu);
+	elrsr_ptr = u64_to_bitmask(&elrsr);
 
-		vlr = vgic_get_lr(vcpu, lr);
+	for_each_clear_bit(lr, elrsr_ptr, vgic->nr_lr) {
+		struct vgic_lr vlr = vgic_get_lr(vcpu, lr);
 
 		/*
 		 * If we have a mapping, and the virtual interrupt is

@@ -1443,7 +1439,6 @@ static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr)
 /* Sync back the VGIC state after a guest run */
 static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
 {
-	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 	u64 elrsr;
 	unsigned long *elrsr_ptr;

virt/kvm/arm/vgic.c static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu) · line 1462

Quoted 7 lines
virt/kvm/arm/vgic.c +0 -4
@@ -1456,12 +1451,8 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
 
 	/* Deal with HW interrupts, and clear mappings for empty LRs */
 	for (lr = 0; lr < vgic->nr_lr; lr++) {
-		struct vgic_lr vlr;
-
-		if (!test_bit(lr, vgic_cpu->lr_used))
-			continue;

Is there not at least a theoretical change in functionality here?

After this patch, we would consider all LRs, not just those we knew were set when we entered the VM.

Do we have a guarantee that anything we consider in vgic_sync_hwirq at this point have had lr_used set?

Thanks,
-Christoffer

author
#5 lore raw

RE: [PATCH 1/2] KVM: arm/arm64: Optimize away redundant LR tracking

Hello!

Quoted 216 lines

-----Original Message-----
From: Christoffer Dall [mailto:[email protected]]
Sent: Friday, October 23, 2015 12:43 AM
To: Pavel Fedin
Cc: [email protected]; [email protected]; Marc Zyngier; Andre Przywara
Subject: Re: [PATCH 1/2] KVM: arm/arm64: Optimize away redundant LR tracking

Quoted 198 lines · Pavel Fedin

Currently we use vgic_irq_lr_map in order to track which LRs hold which IRQs, and lr_used bitmap in order to track which LRs are used or free.

vgic_irq_lr_map is actually used only for piggy-back optimization, and can be easily replaced by iteration over lr_used. This is good because in future, when LPI support is introduced, number of IRQs will grow up to at least 16384, while numbers from 1024 to 8192 are never going to be used. This would be a huge memory waste.

In its turn, lr_used is also completely redundant since ae705930fca6322600690df9dc1c7d0516145a93 ("arm/arm64: KVM: Keep elrsr/aisr in sync with software model"), because together with lr_used we also update elrsr. This allows to easily replace lr_used with elrsr, inverting all conditions (because in elrsr '1' means 'free').

Signed-off-by Pavel Fedin <[email protected]>
---
 include/kvm/arm_vgic.h |  6 ----
 virt/kvm/arm/vgic.c    | 74 +++++++++++++++++++-------------------------------
 2 files changed, 28 insertions(+), 52 deletions(-)
include/kvm/arm_vgic.h +0 -6
index 4e14dac..d908028 100644
@@ -296,9 +296,6 @@ struct vgic_v3_cpu_if {
 };
 
 struct vgic_cpu {
-	/* per IRQ to LR mapping */
-	u8		*vgic_irq_lr_map;
-
 	/* Pending/active/both interrupts on this VCPU */
 	DECLARE_BITMAP(	pending_percpu, VGIC_NR_PRIVATE_IRQS);
 	DECLARE_BITMAP(	active_percpu, VGIC_NR_PRIVATE_IRQS);

@@ -309,9 +306,6 @@ struct vgic_cpu {
 	unsigned long   *active_shared;
 	unsigned long   *pend_act_shared;
 
-	/* Bitmap of used/free list registers */
-	DECLARE_BITMAP(	lr_used, VGIC_V2_MAX_LRS);
-
 	/* Number of list registers on this CPU */
 	int		nr_lr;
virt/kvm/arm/vgic.c +22 -27
index 6bd1c9b..2f4d25a 100644
@@ -102,9 +102,10 @@
 #include "vgic.h"
 
 static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
-static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu);
+static void vgic_retire_lr(int lr_nr, struct kvm_vcpu *vcpu);
 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 u64 vgic_get_elrsr(struct kvm_vcpu *vcpu);
 static struct irq_phys_map *vgic_irq_map_search(struct kvm_vcpu *vcpu,
 						int virt_irq);
 

@@ -683,9 +684,11 @@ bool vgic_handle_cfg_reg(u32 *reg, struct kvm_exit_mmio *mmio,
 void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
 {
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+	u64 elrsr = vgic_get_elrsr(vcpu);
+	unsigned long *elrsr_ptr = u64_to_bitmask(&elrsr);
 	int i;
 
-	for_each_set_bit(i, vgic_cpu->lr_used, vgic_cpu->nr_lr) {
+	for_each_clear_bit(i, elrsr_ptr, vgic_cpu->nr_lr) {
 		struct vgic_lr lr = vgic_get_lr(vcpu, i);
 
 		/*

@@ -728,7 +731,7 @@ void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
 		 * Mark the LR as free for other use.
 		 */
 		BUG_ON(lr.state & LR_STATE_MASK);
-		vgic_retire_lr(i, lr.irq, vcpu);
+		vgic_retire_lr(i, vcpu);
 		vgic_irq_clear_queued(vcpu, lr.irq);
 
 		/* Finally update the VGIC state. */

@@ -1087,15 +1090,12 @@ static inline void vgic_enable(struct kvm_vcpu *vcpu)
 	vgic_ops->enable(vcpu);
 }
 
-static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu)
+static void vgic_retire_lr(int lr_nr, struct kvm_vcpu *vcpu)
 {
-	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_lr vlr = vgic_get_lr(vcpu, lr_nr);
 
 	vlr.state = 0;
 	vgic_set_lr(vcpu, lr_nr, vlr);
-	clear_bit(lr_nr, vgic_cpu->lr_used);
-	vgic_cpu->vgic_irq_lr_map[irq] = LR_EMPTY;
 	vgic_sync_lr_elrsr(vcpu, lr_nr, vlr);
 }
 

@@ -1110,14 +1110,15 @@ static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu)
  */
 static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu)
 {
-	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+	u64 elrsr = vgic_get_elrsr(vcpu);
+	unsigned long *elrsr_ptr = u64_to_bitmask(&elrsr);
 	int lr;
 
-	for_each_set_bit(lr, vgic_cpu->lr_used, vgic->nr_lr) {
+	for_each_clear_bit(lr, elrsr_ptr, vgic->nr_lr) {
 		struct vgic_lr vlr = vgic_get_lr(vcpu, lr);
 
 		if (!vgic_irq_is_enabled(vcpu, vlr.irq)) {
-			vgic_retire_lr(lr, vlr.irq, vcpu);
+			vgic_retire_lr(lr, vcpu);
 			if (vgic_irq_is_queued(vcpu, vlr.irq))
 				vgic_irq_clear_queued(vcpu, vlr.irq);
 		}

@@ -1169,8 +1170,9 @@ static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq,
  */
 bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
 {
-	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+	u64 elrsr = vgic_get_elrsr(vcpu);
+	unsigned long *elrsr_ptr = u64_to_bitmask(&elrsr);
 	struct vgic_lr vlr;
 	int lr;
 

@@ -1181,28 +1183,22 @@ bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
 
 	kvm_debug("Queue IRQ%d\n", irq);
 
-	lr = vgic_cpu->vgic_irq_lr_map[irq];
-
 	/* Do we have an active interrupt for the same CPUID? */
-	if (lr != LR_EMPTY) {
+	for_each_clear_bit(lr, elrsr_ptr, vgic->nr_lr) {
 		vlr = vgic_get_lr(vcpu, lr);
-		if (vlr.source == sgi_source_id) {
+		if (vlr.irq == irq && vlr.source == sgi_source_id) {
 			kvm_debug("LR%d piggyback for IRQ%d\n", lr, vlr.irq);
-			BUG_ON(!test_bit(lr, vgic_cpu->lr_used));
 			vgic_queue_irq_to_lr(vcpu, irq, lr, vlr);
 			return true;
 		}
 	}
 
 	/* Try to use another LR for this interrupt */
-	lr = find_first_zero_bit((unsigned long *)vgic_cpu->lr_used,
-			       vgic->nr_lr);
+	lr = find_first_bit(elrsr_ptr, vgic->nr_lr);
 	if (lr >= vgic->nr_lr)
 		return false;
 
 	kvm_debug("LR%d allocated for IRQ%d %x\n", lr, irq, sgi_source_id);
-	vgic_cpu->vgic_irq_lr_map[irq] = lr;
-	set_bit(lr, vgic_cpu->lr_used);
 
 	vlr.irq = irq;
 	vlr.source = sgi_source_id;

@@ -1243,6 +1239,8 @@ static void __kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
 	int i, vcpu_id, lr, ret;
 	int overflow = 0;
 	int nr_shared = vgic_nr_shared_irqs(dist);
+	u64 elrsr;
+	unsigned long *elrsr_ptr;
 
 	vcpu_id = vcpu->vcpu_id;
 

@@ -1296,13 +1294,11 @@ epilog:
 		clear_bit(vcpu_id, dist->irq_pending_on_cpu);
 	}
 
-	for (lr = 0; lr < vgic->nr_lr; lr++) {
-		struct vgic_lr vlr;
-
-		if (!test_bit(lr, vgic_cpu->lr_used))
-			continue;
+	elrsr = vgic_get_elrsr(vcpu);
+	elrsr_ptr = u64_to_bitmask(&elrsr);
 
-		vlr = vgic_get_lr(vcpu, lr);
+	for_each_clear_bit(lr, elrsr_ptr, vgic->nr_lr) {
+		struct vgic_lr vlr = vgic_get_lr(vcpu, lr);
 
 		/*
 		 * If we have a mapping, and the virtual interrupt is

@@ -1443,7 +1439,6 @@ static int vgic_sync_hwirq(struct kvm_vcpu *vcpu, struct vgic_lr vlr)
 /* Sync back the VGIC state after a guest run */
 static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
 {
-	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 	u64 elrsr;
 	unsigned long *elrsr_ptr;

virt/kvm/arm/vgic.c static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu) · line 1462

Quoted 7 lines
virt/kvm/arm/vgic.c +0 -4
@@ -1456,12 +1451,8 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
 
 	/* Deal with HW interrupts, and clear mappings for empty LRs */
 	for (lr = 0; lr < vgic->nr_lr; lr++) {
-		struct vgic_lr vlr;
-
-		if (!test_bit(lr, vgic_cpu->lr_used))
-			continue;

Is there not at least a theoretical change in functionality here?

Quoted 2 lines

After this patch, we would consider all LRs, not just those we knew were set when we entered the VM.

My first thing to say: actually, we enter ALL LRs into the VM every time. Unused LRs do not contain any state (vlr.state == 0).
Otherwise we would get strange spurious interrupts.

Quoted 2 lines

Do we have a guarantee that anything we consider in vgic_sync_hwirq at this point have had lr_used set?

Of course, this is how the current code works. Take a look at vgic_queue_irq(). Every time when it allocates a new LR, it sets
corresponding bit in lr_used. Then, the whole thing goes into VM. After the VM exits, we have ELRSR set by the hardware according to states in LR. And, right after that we sync up our lr_used with ELRSR.
Now, let's just have a look at how the new code would behave with empty LR, which has neither PENDING nor ACTIVE state. So far,
after the patch we have only:
--- cut ---

	/* Deal with HW interrupts, and clear mappings for empty LRs */
	for (lr = 0; lr < vgic->nr_lr; lr++) {
		struct vgic_lr vlr = vgic_get_lr(vcpu, lr);

		if (vgic_sync_hwirq(vcpu, lr, vlr))
			level_pending = true;
	}

--- cut ---
... and the question is: what happens if vgic_sync_hwirq() gets empty vlr? The answer is "nothing", because it will check for
LR_HW, and immediately return, because it's zero. If it is set, then it was not empty LR, it contained a forwarded hardware IRQ. LR_HW bit can be set only by vgic_queue_irq_to_lr(), and it is called only by vgic_queue_irq(), which in turn currently sets a bit in lr_used (or, in piggyback case, it has already been set).

A short resume: lr_used != 0 always goes together with vlr.state != 0.
Actually, Andre Przywara has tested the same approach in: http://www.spinics.net/lists/kvm/msg121879.html

Quoted 2 lines

If you can send out a new set of these rebased on kvmarm/next, and if Andre has time to test them on GICv3, then I may queue them for v4.4.

Of course, i'll rebase today.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

author
#6 lore raw

PATCH 2/2 KVM: arm/arm64: Merge vgic_set_lr() and vgic_sync_lr_elrsr()

In reply to Pavel Fedin

Now we see that vgic_set_lr() and vgic_sync_lr_elrsr() are always used together. Merge them into one function, saving from second vgic_ops dereferencing every time.

Additionally, remove unnecessary vgic_set_lr() in vgic_unqueue_irqs(), because the following vgic_retire_lr() will reset lr.state to zero anyway.

Signed-off-by Pavel Fedin <[email protected]>
---
 include/kvm/arm_vgic.h |  1 -
 virt/kvm/arm/vgic-v2.c |  5 -----
 virt/kvm/arm/vgic-v3.c |  5 -----
 virt/kvm/arm/vgic.c    | 30 ++++--------------------------
 4 files changed, 4 insertions(+), 37 deletions(-)
include/kvm/arm_vgic.h +0 -1
index d908028..ab5d242 100644
@@ -112,7 +112,6 @@ struct vgic_vmcr {
 struct vgic_ops {
 	struct vgic_lr	(*get_lr)(const struct kvm_vcpu *, int);
 	void	(*set_lr)(struct kvm_vcpu *, int, struct vgic_lr);
-	void	(*sync_lr_elrsr)(struct kvm_vcpu *, int, struct vgic_lr);
 	u64	(*get_elrsr)(const struct kvm_vcpu *vcpu);
 	u64	(*get_eisr)(const struct kvm_vcpu *vcpu);
 	void	(*clear_eisr)(struct kvm_vcpu *vcpu);
virt/kvm/arm/vgic-v2.c +0 -5
index 8d7b04d..f9d8da5 100644
@@ -79,11 +79,7 @@ static void vgic_v2_set_lr(struct kvm_vcpu *vcpu, int lr,
 		lr_val |= (lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT);
 
 	vcpu->arch.vgic_cpu.vgic_v2.vgic_lr[lr] = lr_val;
-}
 
-static void vgic_v2_sync_lr_elrsr(struct kvm_vcpu *vcpu, int lr,
-				  struct vgic_lr lr_desc)
-{
 	if (!(lr_desc.state & LR_STATE_MASK))
 		vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr |= (1ULL << lr);
 	else

@@ -166,7 +162,6 @@ static void vgic_v2_enable(struct kvm_vcpu *vcpu)
 static const struct vgic_ops vgic_v2_ops = {
 	.get_lr			= vgic_v2_get_lr,
 	.set_lr			= vgic_v2_set_lr,
-	.sync_lr_elrsr		= vgic_v2_sync_lr_elrsr,
 	.get_elrsr		= vgic_v2_get_elrsr,
 	.get_eisr		= vgic_v2_get_eisr,
 	.clear_eisr		= vgic_v2_clear_eisr,
virt/kvm/arm/vgic-v3.c +0 -5
index 7dd5d62..75f6d91 100644
@@ -112,11 +112,7 @@ static void vgic_v3_set_lr(struct kvm_vcpu *vcpu, int lr,
 	}
 
 	vcpu->arch.vgic_cpu.vgic_v3.vgic_lr[LR_INDEX(lr)] = lr_val;
-}
 
-static void vgic_v3_sync_lr_elrsr(struct kvm_vcpu *vcpu, int lr,
-				  struct vgic_lr lr_desc)
-{
 	if (!(lr_desc.state & LR_STATE_MASK))
 		vcpu->arch.vgic_cpu.vgic_v3.vgic_elrsr |= (1U << lr);
 	else

@@ -211,7 +207,6 @@ static void vgic_v3_enable(struct kvm_vcpu *vcpu)
 static const struct vgic_ops vgic_v3_ops = {
 	.get_lr			= vgic_v3_get_lr,
 	.set_lr			= vgic_v3_set_lr,
-	.sync_lr_elrsr		= vgic_v3_sync_lr_elrsr,
 	.get_elrsr		= vgic_v3_get_elrsr,
 	.get_eisr		= vgic_v3_get_eisr,
 	.clear_eisr		= vgic_v3_clear_eisr,
virt/kvm/arm/vgic.c +4 -26
index 2f4d25a..7e164eb 100644
@@ -709,10 +709,8 @@ void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
 		 * interrupt then move the active state to the
 		 * distributor tracking bit.
 		 */
-		if (lr.state & LR_STATE_ACTIVE) {
+		if (lr.state & LR_STATE_ACTIVE)
 			vgic_irq_set_active(vcpu, lr.irq);
-			lr.state &= ~LR_STATE_ACTIVE;
-		}
 
 		/*
 		 * Reestablish the pending state on the distributor and the

@@ -720,17 +718,12 @@ void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
 		 * is fine, then we are only setting a few bits that were
 		 * already set.
 		 */
-		if (lr.state & LR_STATE_PENDING) {
+		if (lr.state & LR_STATE_PENDING)
 			vgic_dist_irq_set_pending(vcpu, lr.irq);
-			lr.state &= ~LR_STATE_PENDING;
-		}
-
-		vgic_set_lr(vcpu, i, lr);
 
 		/*
 		 * Mark the LR as free for other use.
 		 */
-		BUG_ON(lr.state & LR_STATE_MASK);
 		vgic_retire_lr(i, vcpu);
 		vgic_irq_clear_queued(vcpu, lr.irq);
 

@@ -1039,12 +1032,6 @@ static void vgic_set_lr(struct kvm_vcpu *vcpu, int lr,
 	vgic_ops->set_lr(vcpu, lr, vlr);
 }
 
-static void vgic_sync_lr_elrsr(struct kvm_vcpu *vcpu, int lr,
-			       struct vgic_lr vlr)
-{
-	vgic_ops->sync_lr_elrsr(vcpu, lr, vlr);
-}
-
 static inline u64 vgic_get_elrsr(struct kvm_vcpu *vcpu)
 {
 	return vgic_ops->get_elrsr(vcpu);

@@ -1096,7 +1083,6 @@ static void vgic_retire_lr(int lr_nr, struct kvm_vcpu *vcpu)
 
 	vlr.state = 0;
 	vgic_set_lr(vcpu, lr_nr, vlr);
-	vgic_sync_lr_elrsr(vcpu, lr_nr, vlr);
 }
 
 /*

@@ -1160,7 +1146,6 @@ static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq,
 	}
 
 	vgic_set_lr(vcpu, lr_nr, vlr);
-	vgic_sync_lr_elrsr(vcpu, lr_nr, vlr);
 }
 
 /*

@@ -1380,12 +1365,6 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
 			}
 
 			spin_unlock(&dist->lock);
-
-			/*
-			 * Despite being EOIed, the LR may not have
-			 * been marked as empty.
-			 */
-			vgic_sync_lr_elrsr(vcpu, lr, vlr);
 		}
 	}
 

@@ -1446,8 +1425,6 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
 	bool level_pending;
 
 	level_pending = vgic_process_maintenance(vcpu);
-	elrsr = vgic_get_elrsr(vcpu);
-	elrsr_ptr = u64_to_bitmask(&elrsr);
 
 	/* Deal with HW interrupts, and clear mappings for empty LRs */
 	for (lr = 0; lr < vgic->nr_lr; lr++) {

@@ -1463,11 +1440,12 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
 			vlr.hwirq = 0;
 			vgic_set_lr(vcpu, lr, vlr);
 			vgic_irq_clear_queued(vcpu, vlr.irq);
-			set_bit(lr, elrsr_ptr);
 		}
 	}
 
 	/* Check if we still have something up our sleeve... */
+	elrsr = vgic_get_elrsr(vcpu);
+	elrsr_ptr = u64_to_bitmask(&elrsr);
 	pending = find_first_zero_bit(elrsr_ptr, vgic->nr_lr);
 	if (level_pending || pending < vgic->nr_lr)
 		set_bit(vcpu->vcpu_id, dist->irq_pending_on_cpu);
Signature · 1 line
2.4.4
#7 lore raw

Re: [PATCH 2/2] KVM: arm/arm64: Merge vgic_set_lr() and vgic_sync_lr_elrsr()

Quoted 167 lines · Pavel Fedin

Now we see that vgic_set_lr() and vgic_sync_lr_elrsr() are always used together. Merge them into one function, saving from second vgic_ops dereferencing every time.

Additionally, remove unnecessary vgic_set_lr() in vgic_unqueue_irqs(), because the following vgic_retire_lr() will reset lr.state to zero anyway.

Signed-off-by Pavel Fedin <[email protected]>
---
 include/kvm/arm_vgic.h |  1 -
 virt/kvm/arm/vgic-v2.c |  5 -----
 virt/kvm/arm/vgic-v3.c |  5 -----
 virt/kvm/arm/vgic.c    | 30 ++++--------------------------
 4 files changed, 4 insertions(+), 37 deletions(-)
include/kvm/arm_vgic.h +0 -1
index d908028..ab5d242 100644
@@ -112,7 +112,6 @@ struct vgic_vmcr {
 struct vgic_ops {
 	struct vgic_lr	(*get_lr)(const struct kvm_vcpu *, int);
 	void	(*set_lr)(struct kvm_vcpu *, int, struct vgic_lr);
-	void	(*sync_lr_elrsr)(struct kvm_vcpu *, int, struct vgic_lr);
 	u64	(*get_elrsr)(const struct kvm_vcpu *vcpu);
 	u64	(*get_eisr)(const struct kvm_vcpu *vcpu);
 	void	(*clear_eisr)(struct kvm_vcpu *vcpu);
virt/kvm/arm/vgic-v2.c +0 -5
index 8d7b04d..f9d8da5 100644
@@ -79,11 +79,7 @@ static void vgic_v2_set_lr(struct kvm_vcpu *vcpu, int lr,
 		lr_val |= (lr_desc.source << GICH_LR_PHYSID_CPUID_SHIFT);
 
 	vcpu->arch.vgic_cpu.vgic_v2.vgic_lr[lr] = lr_val;
-}
 
-static void vgic_v2_sync_lr_elrsr(struct kvm_vcpu *vcpu, int lr,
-				  struct vgic_lr lr_desc)
-{
 	if (!(lr_desc.state & LR_STATE_MASK))
 		vcpu->arch.vgic_cpu.vgic_v2.vgic_elrsr |= (1ULL << lr);
 	else

@@ -166,7 +162,6 @@ static void vgic_v2_enable(struct kvm_vcpu *vcpu)
 static const struct vgic_ops vgic_v2_ops = {
 	.get_lr			= vgic_v2_get_lr,
 	.set_lr			= vgic_v2_set_lr,
-	.sync_lr_elrsr		= vgic_v2_sync_lr_elrsr,
 	.get_elrsr		= vgic_v2_get_elrsr,
 	.get_eisr		= vgic_v2_get_eisr,
 	.clear_eisr		= vgic_v2_clear_eisr,
virt/kvm/arm/vgic-v3.c +0 -5
index 7dd5d62..75f6d91 100644
@@ -112,11 +112,7 @@ static void vgic_v3_set_lr(struct kvm_vcpu *vcpu, int lr,
 	}
 
 	vcpu->arch.vgic_cpu.vgic_v3.vgic_lr[LR_INDEX(lr)] = lr_val;
-}
 
-static void vgic_v3_sync_lr_elrsr(struct kvm_vcpu *vcpu, int lr,
-				  struct vgic_lr lr_desc)
-{
 	if (!(lr_desc.state & LR_STATE_MASK))
 		vcpu->arch.vgic_cpu.vgic_v3.vgic_elrsr |= (1U << lr);
 	else

@@ -211,7 +207,6 @@ static void vgic_v3_enable(struct kvm_vcpu *vcpu)
 static const struct vgic_ops vgic_v3_ops = {
 	.get_lr			= vgic_v3_get_lr,
 	.set_lr			= vgic_v3_set_lr,
-	.sync_lr_elrsr		= vgic_v3_sync_lr_elrsr,
 	.get_elrsr		= vgic_v3_get_elrsr,
 	.get_eisr		= vgic_v3_get_eisr,
 	.clear_eisr		= vgic_v3_clear_eisr,
virt/kvm/arm/vgic.c +2 -25
index 2f4d25a..7e164eb 100644
@@ -709,10 +709,8 @@ void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
 		 * interrupt then move the active state to the
 		 * distributor tracking bit.
 		 */
-		if (lr.state & LR_STATE_ACTIVE) {
+		if (lr.state & LR_STATE_ACTIVE)
 			vgic_irq_set_active(vcpu, lr.irq);
-			lr.state &= ~LR_STATE_ACTIVE;
-		}
 
 		/*
 		 * Reestablish the pending state on the distributor and the

@@ -720,17 +718,12 @@ void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
 		 * is fine, then we are only setting a few bits that were
 		 * already set.
 		 */
-		if (lr.state & LR_STATE_PENDING) {
+		if (lr.state & LR_STATE_PENDING)
 			vgic_dist_irq_set_pending(vcpu, lr.irq);
-			lr.state &= ~LR_STATE_PENDING;
-		}
-
-		vgic_set_lr(vcpu, i, lr);
 
 		/*
 		 * Mark the LR as free for other use.
 		 */
-		BUG_ON(lr.state & LR_STATE_MASK);
 		vgic_retire_lr(i, vcpu);
 		vgic_irq_clear_queued(vcpu, lr.irq);
 

@@ -1039,12 +1032,6 @@ static void vgic_set_lr(struct kvm_vcpu *vcpu, int lr,
 	vgic_ops->set_lr(vcpu, lr, vlr);
 }
 
-static void vgic_sync_lr_elrsr(struct kvm_vcpu *vcpu, int lr,
-			       struct vgic_lr vlr)
-{
-	vgic_ops->sync_lr_elrsr(vcpu, lr, vlr);
-}
-
 static inline u64 vgic_get_elrsr(struct kvm_vcpu *vcpu)
 {
 	return vgic_ops->get_elrsr(vcpu);

@@ -1096,7 +1083,6 @@ static void vgic_retire_lr(int lr_nr, struct kvm_vcpu *vcpu)
 
 	vlr.state = 0;
 	vgic_set_lr(vcpu, lr_nr, vlr);
-	vgic_sync_lr_elrsr(vcpu, lr_nr, vlr);
 }
 
 /*

@@ -1160,7 +1146,6 @@ static void vgic_queue_irq_to_lr(struct kvm_vcpu *vcpu, int irq,
 	}
 
 	vgic_set_lr(vcpu, lr_nr, vlr);
-	vgic_sync_lr_elrsr(vcpu, lr_nr, vlr);
 }
 
 /*

@@ -1380,12 +1365,6 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
 			}
 
 			spin_unlock(&dist->lock);
-
-			/*
-			 * Despite being EOIed, the LR may not have
-			 * been marked as empty.
-			 */
-			vgic_sync_lr_elrsr(vcpu, lr, vlr);
 		}
 	}
 

@@ -1446,8 +1425,6 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
 	bool level_pending;
 
 	level_pending = vgic_process_maintenance(vcpu);
-	elrsr = vgic_get_elrsr(vcpu);
-	elrsr_ptr = u64_to_bitmask(&elrsr);
 
 	/* Deal with HW interrupts, and clear mappings for empty LRs */
 	for (lr = 0; lr < vgic->nr_lr; lr++) {

@@ -1463,11 +1440,12 @@ static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
 			vlr.hwirq = 0;
 			vgic_set_lr(vcpu, lr, vlr);
 			vgic_irq_clear_queued(vcpu, vlr.irq);

virt/kvm/arm/vgic.c static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu) · line 1451

Quoted 10 lines
virt/kvm/arm/vgic.c +2 -1
-			set_bit(lr, elrsr_ptr);
 		}
 	}
 
 	/* Check if we still have something up our sleeve... */
+	elrsr = vgic_get_elrsr(vcpu);
+	elrsr_ptr = u64_to_bitmask(&elrsr);
 	pending = find_first_zero_bit(elrsr_ptr, vgic->nr_lr);
 	if (level_pending || pending < vgic->nr_lr)
 		set_bit(vcpu->vcpu_id, dist->irq_pending_on_cpu);

This patch looks reasonable assuming I manage to convince myself about the correctness of the first one.

If you can send out a new set of these rebased on kvmarm/next, and if Andre has time to test them on GICv3, then I may queue them for v4.4.

Thanks,
-Christoffer

#8 lore raw
In reply to Pavel Fedin

Hi Pavel,

Quoted 10 lines · Pavel Fedin

Current KVM code has lots of old redundancies, which can be cleaned up. This patchset is actually a better alternative to
http://www.spinics.net/lists/arm-kernel/msg430726.html, which allows to keep piggy-backed LRs. The idea is based on the fact that our code also maintains LR state in elrsr, and this information is enough to track LR usage.

This patchset is made against linux-next of 02.10.2015. Thanks to Andre for pointing out some 4.3 specifics.

I'm not opposed to these changes, they clean up the data structures which is definitely a good thing.

I am a bit worries about how/if this is going to conflict with the ITS series and other patches in flight touchignt he vgic.

Marc/Andre, any thoughts on this?

-Christoffer

#9 lore raw
Quoted 13 lines · Christoffer Dall

Hi Pavel,

Quoted 10 lines · Pavel Fedin

Current KVM code has lots of old redundancies, which can be cleaned up. This patchset is actually a better alternative to
http://www.spinics.net/lists/arm-kernel/msg430726.html, which allows to keep piggy-backed LRs. The idea is based on the fact that our code also maintains LR state in elrsr, and this information is enough to track LR usage.

This patchset is made against linux-next of 02.10.2015. Thanks to Andre for pointing out some 4.3 specifics.

Quoted 7 lines

I'm not opposed to these changes, they clean up the data structures which is definitely a good thing.

I am a bit worries about how/if this is going to conflict with the ITS series and other patches in flight touchignt he vgic.

Marc/Andre, any thoughts on this?

I don't mind the simplification (Andre was already removing the piggybacking stuff as part of his ITS series). I'm a bit more cautious about the sync_elrsr stuff, but that's mostly because I've only read the patch in a superficial way.

But yes, this is probably going to clash, unless we make this part of an existing series (/me looks at André... ;-)

	M.
Signature · 1 line
Jazz is not dead. It just smells funny...
#10 lore raw

Hi,

Quoted 22 lines · Marc Zyngier
Quoted 13 lines · Christoffer Dall

Hi Pavel,

Quoted 10 lines · Pavel Fedin

Current KVM code has lots of old redundancies, which can be cleaned up. This patchset is actually a better alternative to
http://www.spinics.net/lists/arm-kernel/msg430726.html, which allows to keep piggy-backed LRs. The idea is based on the fact that our code also maintains LR state in elrsr, and this information is enough to track LR usage.

This patchset is made against linux-next of 02.10.2015. Thanks to Andre for pointing out some 4.3 specifics.

Quoted 7 lines

I'm not opposed to these changes, they clean up the data structures which is definitely a good thing.

I am a bit worries about how/if this is going to conflict with the ITS series and other patches in flight touchignt he vgic.

Marc/Andre, any thoughts on this?

Quoted 7 lines

I don't mind the simplification (Andre was already removing the piggybacking stuff as part of his ITS series). I'm a bit more cautious about the sync_elrsr stuff, but that's mostly because I've only read the patch in a superficial way.

But yes, this is probably going to clash, unless we make this part of an existing series (/me looks at André... ;-)

Yes, I am looking at merging this. From the discussion with Pavel I remember some things that I disagreed with, so I may propose a follow-up patch. I will give this a try tomorrow.

Cheers,
Andre.

author Tested-by
#11 lore raw

Hello!

Quoted 3 lines

Yes, I am looking at merging this. From the discussion with Pavel I remember some things that I disagreed with, so I may propose a follow-up patch. I will give this a try tomorrow.

I reply to this thread, because this relates to the whole changeset. After the merge, some pieces
are missing, considering my live migration W.I.P (see patch below). Together with this, vITS v3 backported to v4.2.2 and...

Tested-by Pavel Fedin <[email protected]>

---

Quoted 1 line

From b08e9ba1da69f9cf5731c89a4ff39561cd16e6ea Mon Sep 17 00:00:00 2001

From: Pavel Fedin <[email protected]>
Date: Thu, 8 Oct 2015 14:43:23 +0300
Subject: [PATCH] Missing vITS pieces for live migration and safety

1. Split up vits_init() and perform allocations on PENDBASER access. Fixes crash when setting LPI registers from userspace before any vCPU has been run. The bug is triggered by reset routine in qemu.

2. Properly handle LPIs in vgic_unqueue_irqs(). Does not corrupt memory any more if LPI happens to get in there.

Signed-off-by Pavel Fedin <[email protected]>
---
 virt/kvm/arm/its-emul.c     | 26 +++++++++++++++++++-------
 virt/kvm/arm/its-emul.h     |  1 +
 virt/kvm/arm/vgic-v3-emul.c | 11 ++++++++++-
 virt/kvm/arm/vgic.c         | 15 +++++++++++----
 4 files changed, 41 insertions(+), 12 deletions(-)
virt/kvm/arm/its-emul.c +19 -7
index b40a7fc..b1d61df 100644
@@ -1117,7 +1117,9 @@ int vits_init(struct kvm *kvm)
 {
 	struct vgic_dist *dist = &kvm->arch.vgic;
 	struct vgic_its *its = &dist->its;
-	int ret;
+
+	if (dist->pendbaser)
+		return 0;
 
 	dist->pendbaser = kcalloc(dist->nr_cpus, sizeof(u64), GFP_KERNEL);
 	if (!dist->pendbaser)

@@ -1132,18 +1134,27 @@ int vits_init(struct kvm *kvm)
 	INIT_LIST_HEAD(&its->device_list);
 	INIT_LIST_HEAD(&its->collection_list);
 
-	ret = vgic_register_kvm_io_dev(kvm, dist->vgic_its_base,
-				       KVM_VGIC_V3_ITS_SIZE, vgicv3_its_ranges,
-				       -1, &its->iodev);
-	if (ret)
-		return ret;
-
 	its->enabled = false;
 	dist->msis_require_devid = true;
 
 	return 0;
 }
 
+int vits_map_resources(struct kvm *kvm)
+{
+	struct vgic_dist *dist = &kvm->arch.vgic;
+	struct vgic_its *its = &dist->its;
+	int ret;
+
+	ret = vits_init(kvm);
+	if (ret)
+		return ret;
+
+	return vgic_register_kvm_io_dev(kvm, dist->vgic_its_base,
+				       KVM_VGIC_V3_ITS_SIZE, vgicv3_its_ranges,
+				       -1, &its->iodev);
+}
+
 void vits_destroy(struct kvm *kvm)
 {
 	struct vgic_dist *dist = &kvm->arch.vgic;

@@ -1182,6 +1193,7 @@ void vits_destroy(struct kvm *kvm)
 	kfree(its->buffer_page);
 	kfree(dist->pendbaser);
 
+	dist->pendbaser = NULL;
 	its->enabled = false;
 	spin_unlock(&its->lock);
 }
virt/kvm/arm/its-emul.h +1 -0
index 95e56a7..236f153 100644
@@ -34,6 +34,7 @@
 
 void vgic_enable_lpis(struct kvm_vcpu *vcpu);
 int vits_init(struct kvm *kvm);
+int vits_map_resources(struct kvm *kvm);
 void vits_destroy(struct kvm *kvm);
 
 int vits_inject_msi(struct kvm *kvm, struct kvm_msi *msi);
virt/kvm/arm/vgic-v3-emul.c +10 -1
index 3e262f3..b340202 100644
@@ -693,6 +693,15 @@ static bool handle_mmio_pendbaser_redist(struct kvm_vcpu *vcpu,
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 	int mode = ACCESS_READ_VALUE;
 
+	/*
+	 * This makes sure that dist->pendbaser is allocated.
+	 * We don't use any locks here because the actual initialization will
+	 * happen either during register access from userspace, or upon first
+	 * run. Both situations are already single-threaded.
+	 */
+	if (vits_init(vcpu->kvm))
+		return false;
+
 	/* Storing a value with LPIs already enabled is undefined */
 	mode |= dist->lpis_enabled ? ACCESS_WRITE_IGNORED : ACCESS_WRITE_VALUE;
 	vgic_reg64_access(mmio, offset,

@@ -880,7 +889,7 @@ static int vgic_v3_map_resources(struct kvm *kvm,
 	}
 
 	if (vgic_has_its(kvm)) {
-		ret = vits_init(kvm);
+		ret = vits_map_resources(kvm);
 		if (ret)
 			goto out_unregister;
 	}
virt/kvm/arm/vgic.c +11 -4
index b32baa0..8dbbb1a 100644
@@ -711,6 +711,7 @@ bool vgic_handle_cfg_reg(u32 *reg, struct kvm_exit_mmio *mmio,
  */
 void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
 {
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	u64 elrsr = vgic_get_elrsr(vcpu);
 	unsigned long *elrsr_ptr = u64_to_bitmask(&elrsr);

@@ -737,8 +738,10 @@ void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
 		 * interrupt then move the active state to the
 		 * distributor tracking bit.
 		 */
-		if (lr.state & LR_STATE_ACTIVE)
-			vgic_irq_set_active(vcpu, lr.irq);
+		if (lr.state & LR_STATE_ACTIVE) {
+			if (lr.irq < dist->nr_irqs)
+				vgic_irq_set_active(vcpu, lr.irq);
+		}
 
 		/*
 		 * Reestablish the pending state on the distributor and the

@@ -746,8 +749,12 @@ void vgic_unqueue_irqs(struct kvm_vcpu *vcpu)
 		 * is fine, then we are only setting a few bits that were
 		 * already set.
 		 */
-		if (lr.state & LR_STATE_PENDING)
-			vgic_dist_irq_set_pending(vcpu, lr.irq);
+		if (lr.state & LR_STATE_PENDING) {
+			if (lr.irq < dist->nr_irqs)
+				vgic_dist_irq_set_pending(vcpu, lr.irq);
+			else
+				vgic_unqueue_lpi(vcpu, lr.irq);
+		}
 
 		/*
 		 * Mark the LR as free for other use.
Signature · 7 lines
2.4.4


Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
#12 lore raw
In reply to Andre Przywara
Quoted 30 lines · Andre Przywara

Hi,

Quoted 27 lines · Marc Zyngier
Quoted 13 lines · Christoffer Dall

Hi Pavel,

Quoted 10 lines · Pavel Fedin

Current KVM code has lots of old redundancies, which can be cleaned up. This patchset is actually a better alternative to
http://www.spinics.net/lists/arm-kernel/msg430726.html, which allows to keep piggy-backed LRs. The idea is based on the fact that our code also maintains LR state in elrsr, and this information is enough to track LR usage.

This patchset is made against linux-next of 02.10.2015. Thanks to Andre for pointing out some 4.3 specifics.

Quoted 7 lines

I'm not opposed to these changes, they clean up the data structures which is definitely a good thing.

I am a bit worries about how/if this is going to conflict with the ITS series and other patches in flight touchignt he vgic.

Marc/Andre, any thoughts on this?

I don't mind the simplification (Andre was already removing the piggybacking stuff as part of his ITS series). I'm a bit more cautious about the sync_elrsr stuff, but that's mostly because I've only read the patch in a superficial way.

Quoted 7 lines
Quoted 2 lines

But yes, this is probably going to clash, unless we make this part of an existing series (/me looks at André... ;-)

Yes, I am looking at merging this. From the discussion with Pavel I remember some things that I disagreed with, so I may propose a follow-up patch. I will give this a try tomorrow.

From a maintainer perspective I'd much prefer Andre take these patches as part of his series and send everything together in one go.

Thanks,
-Christoffer

author
#13 lore raw
In reply to Marc Zyngier

Hello!

Quoted 4 lines

I don't mind the simplification (Andre was already removing the piggybacking stuff as part of his ITS series). I'm a bit more cautious about the sync_elrsr stuff, but that's mostly because I've only read the patch in a superficial way.

If you are really afraid of it, you can omit 2/2. The reason why i've done it is exactly what i
said in commit message - LR setting and ELRSR sync *always* go together.
The main part of the cleanup is 1/2.

Quoted 2 lines

But yes, this is probably going to clash, unless we make this part of an existing series (/me looks at André... ;-)

It a little bit clashes, patch No 0012 needs small modifications, but i'd say they are trivial. If
you want, i could rebase the whole thing on top of current kvmarm.git by myself.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

author
#14 lore raw
In reply to Christoffer Dall

Hello!

Quoted 2 lines

I am a bit worries about how/if this is going to conflict with the ITS series and other patches in flight touchignt he vgic.

Just to note: of course i test them together. This works fine at least with vITS v2, where it
replaces patch 0001 from the original series. I guess it should also work fine with v3, replacing patches 0001 and 0002. Merging this at the moment.
Yes, vgic_unqueue_lpi() falls out of use, but this is temporary, because it will be very useful for
live migration.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia