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

[Auger Eric]

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 
>>> Acked-by: Marc Zyngier 
>>> ---
>>>  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_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_lock);
>>> -   ret = kvm_vgic_map_irq(vcpu, irq, host_irq);
>>> +out:
>>> spin_unlock(>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_lock);
>>> -   kvm_vgic_unmap_irq(irq);
>>> +   irq->hw = false;
>>> +   irq->hwintid = 0;
>>> spin_unlock(>irq_lock);
>>> vgic_put_irq(vcpu->kvm, irq);
>>>  
>>>
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[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 
> > Acked-by: Marc Zyngier 
> > ---
> >  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_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_lock);
> > -   ret = kvm_vgic_map_irq(vcpu, irq, host_irq);
> > +out:
> > spin_unlock(>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_lock);
> > -   kvm_vgic_unmap_irq(irq);
> > +   irq->hw = false;
> > +   irq->hwintid = 0;
> > spin_unlock(>irq_lock);
> > vgic_put_irq(vcpu->kvm, irq);
> >  
> > 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Auger Eric]

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 
> Acked-by: Marc Zyngier 
> ---
>  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_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_lock);
> - ret = kvm_vgic_map_irq(vcpu, irq, host_irq);
> +out:
>   spin_unlock(>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_lock);
> - kvm_vgic_unmap_irq(irq);
> + irq->hw = false;
> + irq->hwintid = 0;
>   spin_unlock(>irq_lock);
>   vgic_put_irq(vcpu->kvm, irq);
>  
> 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Christoffer Dall]

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 
Acked-by: Marc Zyngier 
---
 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 */
-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_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_lock);
-   ret = kvm_vgic_map_irq(vcpu, irq, host_irq);
+out:
spin_unlock(>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_lock);
-   kvm_vgic_unmap_irq(irq);
+   irq->hw = false;
+   irq->hwintid = 0;
spin_unlock(>irq_lock);
vgic_put_irq(vcpu->kvm, irq);
 
-- 
2.9.0

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm