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

[Christoffer Dall <christoffer.dall@linaro.org>]

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