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@xxxxxxxxxx> > > Acked-by: Marc Zyngier <marc.zyngier@xxxxxxx> > > --- > > 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); > > > >
