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. Thanks Eric > -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); > >
