Hi Christoffer, On 05/09/2017 16:00, Christoffer Dall wrote: > 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? Yes it would. Thanks Eric > > 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); >>> >>>
