[PATCH v2 5/6] KVM: arm/arm64: Rearrange kvm_vgic_[un]map_phys code in vgic.c

[cdall@linaro.org (Christoffer Dall)]

On Tue, Sep 05, 2017 at 12:26:14PM +0200, Auger Eric wrote:
> Hi Christoffer,
> On 04/09/2017 12:24, Christoffer Dall wrote:
> > The small indirection of a static function made the locking very obvious
> > but becomes pretty ugly as we start passing function pointer around.
> > Let's inline these two functions first to make the following patch more
> > readable.
> > 
> > Signed-off-by: Christoffer Dall <cdall@linaro.org>
> > Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> >  virt/kvm/arm/vgic/vgic.c | 38 +++++++++++++-------------------------
> >  1 file changed, 13 insertions(+), 25 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> > index 7aec730..b704ff5 100644
> > --- a/virt/kvm/arm/vgic/vgic.c
> > +++ b/virt/kvm/arm/vgic/vgic.c
> > @@ -435,12 +435,17 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
> >  	return 0;
> >  }
> >  
> > -/* @irq->irq_lock must be held */
> I chose to hold the lock outside of kvm_vgic_map/unmap_irq because in
> kvm_vgic_set_forwarding(see https://lkml.org/lkml/2017/6/15/278) I was
> also testing hw and target_vcpu fields. As you pointed out maybe I am
> not obliged to check them but that was the rationale.
> 

Ah ok, I see, you want to reuse this bit of code and the caller will
already be holding the spin-lock?

I can rework it then to pass the callback in kvm_vgic_map_irq.  Would
that fit better with your subsequent patches?

Thanks,
-Christoffer

> > -static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> > -			    unsigned int host_irq)
> > +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
> > +			  u32 vintid)
> >  {
> > +	struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, vintid);
> >  	struct irq_desc *desc;
> >  	struct irq_data *data;
> > +	int ret = 0;
> > +
> > +	BUG_ON(!irq);
> > +
> > +	spin_lock(&irq->irq_lock);
> >  
> >  	/*
> >  	 * Find the physical IRQ number corresponding to @host_irq
> > @@ -448,7 +453,8 @@ static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> >  	desc = irq_to_desc(host_irq);
> >  	if (!desc) {
> >  		kvm_err("%s: no interrupt descriptor\n", __func__);
> > -		return -EINVAL;
> > +		ret = -EINVAL;
> > +		goto out;
> >  	}
> >  	data = irq_desc_get_irq_data(desc);
> >  	while (data->parent_data)
> > @@ -457,29 +463,10 @@ static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
> >  	irq->hw = true;
> >  	irq->host_irq = host_irq;
> >  	irq->hwintid = data->hwirq;
> > -	return 0;
> > -}
> > -
> > -/* @irq->irq_lock must be held */
> > -static inline void kvm_vgic_unmap_irq(struct vgic_irq *irq)
> > -{
> > -	irq->hw = false;
> > -	irq->hwintid = 0;
> > -}
> > -
> > -int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
> > -			  u32 vintid)
> > -{
> > -	struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, vintid);
> > -	int ret;
> >  
> > -	BUG_ON(!irq);
> > -
> > -	spin_lock(&irq->irq_lock);
> > -	ret = kvm_vgic_map_irq(vcpu, irq, host_irq);
> > +out:
> >  	spin_unlock(&irq->irq_lock);
> >  	vgic_put_irq(vcpu->kvm, irq);
> > -
> >  	return ret;
> >  }
> >  
> > @@ -494,7 +481,8 @@ int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int vintid)
> >  	BUG_ON(!irq);
> >  
> >  	spin_lock(&irq->irq_lock);
> > -	kvm_vgic_unmap_irq(irq);
> > +	irq->hw = false;
> > +	irq->hwintid = 0;
> >  	spin_unlock(&irq->irq_lock);
> >  	vgic_put_irq(vcpu->kvm, irq);
> >  
> >