[PATCH 0/2] KVM: arm/arm64: Clean up some obsolete code

[PATCH 0/2] KVM: arm/arm64: Clean up some obsolete code

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

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(-)

-- 
2.4.4

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

[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 <p.fedin@samsung.com>
---
 include/kvm/arm_vgic.h |  6 ----
 virt/kvm/arm/vgic.c    | 74 +++++++++++++++++++-------------------------------
 2 files changed, 28 insertions(+), 52 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 4e14dac..d908028 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -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;
 
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 6bd1c9b..2f4d25a 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -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
-- 
2.4.4

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

[Andre Przywara]

Hi,

On 02/10/15 15:44, Pavel Fedin wrote:
> 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').

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.

> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  include/kvm/arm_vgic.h |  6 ----
>  virt/kvm/arm/vgic.c    | 74 +++++++++++++++++++-------------------------------
>  2 files changed, 28 insertions(+), 52 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 4e14dac..d908028 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -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;
>  
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 6bd1c9b..2f4d25a 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -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
> 

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

[Christoffer Dall]

On Mon, Oct 12, 2015 at 05:56:14PM +0100, Andre Przywara wrote:
> Hi,
> 
> On 02/10/15 15:44, Pavel Fedin wrote:
> > 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').
> 
> 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?
> 
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

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

[Christoffer Dall]

On Fri, Oct 02, 2015 at 05:44:28PM +0300, Pavel Fedin wrote:
> 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 <p.fedin@samsung.com>
> ---
>  include/kvm/arm_vgic.h |  6 ----
>  virt/kvm/arm/vgic.c    | 74 +++++++++++++++++++-------------------------------
>  2 files changed, 28 insertions(+), 52 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 4e14dac..d908028 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -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;
>  
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 6bd1c9b..2f4d25a 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -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;

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

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

[Pavel Fedin]

 Hello!

> -----Original Message-----
> From: Christoffer Dall [mailto:christoffer.dall@linaro.org]
> Sent: Friday, October 23, 2015 12:43 AM
> To: Pavel Fedin
> Cc: kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; Marc Zyngier; Andre Przywara
> Subject: Re: [PATCH 1/2] KVM: arm/arm64: Optimize away redundant LR tracking
> 
> On Fri, Oct 02, 2015 at 05:44:28PM +0300, Pavel Fedin wrote:
> > 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 <p.fedin@samsung.com>
> > ---
> >  include/kvm/arm_vgic.h |  6 ----
> >  virt/kvm/arm/vgic.c    | 74 +++++++++++++++++++-------------------------------
> >  2 files changed, 28 insertions(+), 52 deletions(-)
> >
> > diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> > index 4e14dac..d908028 100644
> > --- a/include/kvm/arm_vgic.h
> > +++ b/include/kvm/arm_vgic.h
> > @@ -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;
> >
> > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> > index 6bd1c9b..2f4d25a 100644
> > --- a/virt/kvm/arm/vgic.c
> > +++ b/virt/kvm/arm/vgic.c
> > @@ -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;
> 
> 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.

 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.

> 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

> 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

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

[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 <p.fedin@samsung.com>
---
 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(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index d908028..ab5d242 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -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);
diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
index 8d7b04d..f9d8da5 100644
--- a/virt/kvm/arm/vgic-v2.c
+++ b/virt/kvm/arm/vgic-v2.c
@@ -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,
diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
index 7dd5d62..75f6d91 100644
--- a/virt/kvm/arm/vgic-v3.c
+++ b/virt/kvm/arm/vgic-v3.c
@@ -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,
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 2f4d25a..7e164eb 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -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);
-- 
2.4.4

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

[Christoffer Dall]

On Fri, Oct 02, 2015 at 05:44:29PM +0300, Pavel Fedin wrote:
> 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 <p.fedin@samsung.com>
> ---
>  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(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index d908028..ab5d242 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -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);
> diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
> index 8d7b04d..f9d8da5 100644
> --- a/virt/kvm/arm/vgic-v2.c
> +++ b/virt/kvm/arm/vgic-v2.c
> @@ -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,
> diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
> index 7dd5d62..75f6d91 100644
> --- a/virt/kvm/arm/vgic-v3.c
> +++ b/virt/kvm/arm/vgic-v3.c
> @@ -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,
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 2f4d25a..7e164eb 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -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);
> -- 

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

Re: [PATCH 0/2] KVM: arm/arm64: Clean up some obsolete code

[Christoffer Dall]

Hi Pavel,

On Fri, Oct 02, 2015 at 05:44:27PM +0300, Pavel Fedin wrote:
> 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

RE: [PATCH 0/2] KVM: arm/arm64: Clean up some obsolete code

[Pavel Fedin]

 Hello!

> 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

Re: [PATCH 0/2] KVM: arm/arm64: Clean up some obsolete code

[Marc Zyngier]

On 08/10/15 11:14, Christoffer Dall wrote:
> Hi Pavel,
> 
> On Fri, Oct 02, 2015 at 05:44:27PM +0300, Pavel Fedin wrote:
>> 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?

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.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 0/2] KVM: arm/arm64: Clean up some obsolete code

[Andre Przywara]

Hi,

On 08/10/15 11:56, Marc Zyngier wrote:
> On 08/10/15 11:14, Christoffer Dall wrote:
>> Hi Pavel,
>>
>> On Fri, Oct 02, 2015 at 05:44:27PM +0300, Pavel Fedin wrote:
>>> 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?
> 
> 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.

RE: [PATCH 0/2] KVM: arm/arm64: Clean up some obsolete code

[Pavel Fedin]

 Hello!

> 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 <p.fedin@samsung.com>

---
>From b08e9ba1da69f9cf5731c89a4ff39561cd16e6ea Mon Sep 17 00:00:00 2001
From: Pavel Fedin <p.fedin@samsung.com>
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 <p.fedin@samsung.com>
---
 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(-)

diff --git a/virt/kvm/arm/its-emul.c b/virt/kvm/arm/its-emul.c
index b40a7fc..b1d61df 100644
--- a/virt/kvm/arm/its-emul.c
+++ b/virt/kvm/arm/its-emul.c
@@ -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);
 }
diff --git a/virt/kvm/arm/its-emul.h b/virt/kvm/arm/its-emul.h
index 95e56a7..236f153 100644
--- a/virt/kvm/arm/its-emul.h
+++ b/virt/kvm/arm/its-emul.h
@@ -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);
diff --git a/virt/kvm/arm/vgic-v3-emul.c b/virt/kvm/arm/vgic-v3-emul.c
index 3e262f3..b340202 100644
--- a/virt/kvm/arm/vgic-v3-emul.c
+++ b/virt/kvm/arm/vgic-v3-emul.c
@@ -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;
 	}
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index b32baa0..8dbbb1a 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -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.
-- 
2.4.4


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

Re: [PATCH 0/2] KVM: arm/arm64: Clean up some obsolete code

[Christoffer Dall]

On Thu, Oct 08, 2015 at 12:15:06PM +0100, Andre Przywara wrote:
> Hi,
> 
> On 08/10/15 11:56, Marc Zyngier wrote:
> > On 08/10/15 11:14, Christoffer Dall wrote:
> >> Hi Pavel,
> >>
> >> On Fri, Oct 02, 2015 at 05:44:27PM +0300, Pavel Fedin wrote:
> >>> 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?
> > 
> > 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.
> 
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

RE: [PATCH 0/2] KVM: arm/arm64: Clean up some obsolete code

[Pavel Fedin]

 Hello!

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

> 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