Re: [PATCH v8 06/17] KVM: arm/arm64: VGIC: add refcounting for IRQs

2016-07-08 Thread André Przywara 
On 08/07/16 14:09, Marc Zyngier wrote:
> On 08/07/16 13:54, André Przywara wrote:
>> On 08/07/16 11:50, Marc Zyngier wrote:
>>> On 08/07/16 11:28, Andre Przywara wrote:
 Hi,

 On 07/07/16 16:00, Marc Zyngier wrote:
> On 05/07/16 12:22, Andre Przywara wrote:



>> @@ -236,6 +254,7 @@ retry:
>>  goto retry;
>>  }
>>  
>> +kref_get(>refcount);
>
> Could you use vgic_get_irq() instead? I know it is slightly overkill,
> but I can already tell that we'll need to add some tracing in both the
> put and get helpers in order to do some debugging. Having straight
> kref_get/put is going to make this tracing difficult, so let's not go 
> there.

 I'd rather not.
 1) Putting the IRQ on the ap_list is the "other user" of the
 refcounting, I don't want to mix that unnecessarily with the
 vgic_get_irq() (as in: get the struct by the number) use case. That may
 actually help tracing, since we can have separate tracepoints to
 distinguish them.
>>>
>>> And yet you end-up doing a vgic_put_irq() in the fold operation. Which
>>> is wrong, by the way, as the interrupt is still in in ap_list. This
>>> should be moved to the prune operation.
>>>
 2) This would violate the locking order, since we hold the irq_lock here
 and possibly take the lpi_list_lock in vgic_get_irq().
 I don't think we can or should drop the irq_lock and re-take it just for
 this.
>>>
>>> That's a much more convincing argument. And when you take the above into
>>> account, you realize that your locking is not correct. You shouldn't be
>>> dropping the refcount in fold, but in prune, meaning that you're holding
>>> the ap_lock and the irq_lock, same as when you inserted the interrupt in
>>> the list.
>>>
>>> This is outlining an essential principle: if your locking/refcounting is
>>> not symmetric, it is likely that you're doing something wrong, and that
>>> should bother you really badly.
>>
>> Can you point me to the exact location where it's not symmetric?
>> I just looked at it again and can't find the issue.
>> I "put" it in v[23]_fold because we did a vgic_get_irq a few lines
>> before to translate the LR's intid into our struct vgic_irq pointer.
> 
> Right. I misread that one, apologies.
> 
>> The vgic_get_irq() call isn't in this patch, because we used it already
>> before and this patch is just adding the respective puts.
>> The only asymmetry I could find is the expected one when it comes to and
>> goes from the ap_list.
> 
> So I assume this is the pendent of the kref_get call?

Yes.

> 
> @@ -386,6 +412,7 @@ retry:
>   list_del(>ap_list);
>   irq->vcpu = NULL;
>   spin_unlock(>irq_lock);
> + vgic_put_irq(vcpu->kvm, irq);
>   continue;
> 
> If that's the case, please add a comment, because this is really hard to
> find out which vgic_put_irq() balances with a kref_get() and not a
> vgic_get_irq().

Yes, that's what I thought as well just _after_ having hit the Send
button ...

I think much of the confusion stems from the fact that we used
vgic_get_irq() before, only that it wasn't a "get" in the refcounting
get-put sense.

Cheers,
Andre.

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

Re: [PATCH v8 06/17] KVM: arm/arm64: VGIC: add refcounting for IRQs

2016-07-08 Thread Marc Zyngier 
On 08/07/16 13:54, André Przywara wrote:
> On 08/07/16 11:50, Marc Zyngier wrote:
>> On 08/07/16 11:28, Andre Przywara wrote:
>>> Hi,
>>>
>>> On 07/07/16 16:00, Marc Zyngier wrote:
 On 05/07/16 12:22, Andre Przywara wrote:
> In the moment our struct vgic_irq's are statically allocated at guest
> creation time. So getting a pointer to an IRQ structure is trivial and
> safe. LPIs are more dynamic, they can be mapped and unmapped at any time
> during the guest's _runtime_.
> In preparation for supporting LPIs we introduce reference counting for
> those structures using the kernel's kref infrastructure.
> Since private IRQs and SPIs are statically allocated, the refcount never
> drops to 0 at the moment, but we increase it when an IRQ gets onto a VCPU
> list and decrease it when it gets removed.
> This introduces vgic_put_irq(), which wraps kref_put and hides the
> release function from the callers.
>
> Signed-off-by: Andre Przywara 
> ---
>  include/kvm/arm_vgic.h   |  1 +
>  virt/kvm/arm/vgic/vgic-init.c|  2 ++
>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  8 +++
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 20 +++--
>  virt/kvm/arm/vgic/vgic-mmio.c| 25 -
>  virt/kvm/arm/vgic/vgic-v2.c  |  1 +
>  virt/kvm/arm/vgic/vgic-v3.c  |  1 +
>  virt/kvm/arm/vgic/vgic.c | 48 
> +++-
>  virt/kvm/arm/vgic/vgic.h |  1 +
>  9 files changed, 89 insertions(+), 18 deletions(-)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 5142e2a..450b4da 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -96,6 +96,7 @@ struct vgic_irq {
>   bool active;/* not used for LPIs */
>   bool enabled;
>   bool hw;/* Tied to HW IRQ */
> + struct kref refcount;   /* Used for LPIs */
>   u32 hwintid;/* HW INTID number */
>   union {
>   u8 targets; /* GICv2 target VCPUs mask */
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 90cae48..ac3c1a5 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -177,6 +177,7 @@ static int kvm_vgic_dist_init(struct kvm *kvm, 
> unsigned int nr_spis)
>   spin_lock_init(>irq_lock);
>   irq->vcpu = NULL;
>   irq->target_vcpu = vcpu0;
> + kref_init(>refcount);
>   if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
>   irq->targets = 0;
>   else
> @@ -211,6 +212,7 @@ static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>   irq->vcpu = NULL;
>   irq->target_vcpu = vcpu;
>   irq->targets = 1U << vcpu->vcpu_id;
> + kref_init(>refcount);
>   if (vgic_irq_is_sgi(i)) {
>   /* SGIs */
>   irq->enabled = 1;
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c 
> b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index a213936..4152348 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -102,6 +102,7 @@ static void vgic_mmio_write_sgir(struct kvm_vcpu 
> *source_vcpu,
>   irq->source |= 1U << source_vcpu->vcpu_id;
>  
>   vgic_queue_irq_unlock(source_vcpu->kvm, irq);
> + vgic_put_irq(source_vcpu->kvm, irq);
>   }
>  }
>  
> @@ -116,6 +117,8 @@ static unsigned long vgic_mmio_read_target(struct 
> kvm_vcpu *vcpu,
>   struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>  
>   val |= (u64)irq->targets << (i * 8);
> +
> + vgic_put_irq(vcpu->kvm, irq);
>   }
>  
>   return val;
> @@ -143,6 +146,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu 
> *vcpu,
>   irq->target_vcpu = kvm_get_vcpu(vcpu->kvm, target);
>  
>   spin_unlock(>irq_lock);
> + vgic_put_irq(vcpu->kvm, irq);
>   }
>  }
>  
> @@ -157,6 +161,8 @@ static unsigned long vgic_mmio_read_sgipend(struct 
> kvm_vcpu *vcpu,
>   struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>  
>   val |= (u64)irq->source << (i * 8);
> +
> + vgic_put_irq(vcpu->kvm, irq);
>   }
>   return val;
>  }
> @@ -178,6 +184,7 @@ static void vgic_mmio_write_sgipendc(struct kvm_vcpu 
> *vcpu,
>   irq->pending = false;
>  
>   spin_unlock(>irq_lock);
> + vgic_put_irq(vcpu->kvm, irq);
>   }
>  }
>  
> @@ -201,6 +208,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu 
> *vcpu,
>   } else {
>

Re: [PATCH v8 06/17] KVM: arm/arm64: VGIC: add refcounting for IRQs

2016-07-08 Thread André Przywara 
On 08/07/16 11:50, Marc Zyngier wrote:
> On 08/07/16 11:28, Andre Przywara wrote:
>> Hi,
>>
>> On 07/07/16 16:00, Marc Zyngier wrote:
>>> On 05/07/16 12:22, Andre Przywara wrote:
 In the moment our struct vgic_irq's are statically allocated at guest
 creation time. So getting a pointer to an IRQ structure is trivial and
 safe. LPIs are more dynamic, they can be mapped and unmapped at any time
 during the guest's _runtime_.
 In preparation for supporting LPIs we introduce reference counting for
 those structures using the kernel's kref infrastructure.
 Since private IRQs and SPIs are statically allocated, the refcount never
 drops to 0 at the moment, but we increase it when an IRQ gets onto a VCPU
 list and decrease it when it gets removed.
 This introduces vgic_put_irq(), which wraps kref_put and hides the
 release function from the callers.

 Signed-off-by: Andre Przywara 
 ---
  include/kvm/arm_vgic.h   |  1 +
  virt/kvm/arm/vgic/vgic-init.c|  2 ++
  virt/kvm/arm/vgic/vgic-mmio-v2.c |  8 +++
  virt/kvm/arm/vgic/vgic-mmio-v3.c | 20 +++--
  virt/kvm/arm/vgic/vgic-mmio.c| 25 -
  virt/kvm/arm/vgic/vgic-v2.c  |  1 +
  virt/kvm/arm/vgic/vgic-v3.c  |  1 +
  virt/kvm/arm/vgic/vgic.c | 48 
 +++-
  virt/kvm/arm/vgic/vgic.h |  1 +
  9 files changed, 89 insertions(+), 18 deletions(-)

 diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
 index 5142e2a..450b4da 100644
 --- a/include/kvm/arm_vgic.h
 +++ b/include/kvm/arm_vgic.h
 @@ -96,6 +96,7 @@ struct vgic_irq {
bool active;/* not used for LPIs */
bool enabled;
bool hw;/* Tied to HW IRQ */
 +  struct kref refcount;   /* Used for LPIs */
u32 hwintid;/* HW INTID number */
union {
u8 targets; /* GICv2 target VCPUs mask */
 diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
 index 90cae48..ac3c1a5 100644
 --- a/virt/kvm/arm/vgic/vgic-init.c
 +++ b/virt/kvm/arm/vgic/vgic-init.c
 @@ -177,6 +177,7 @@ static int kvm_vgic_dist_init(struct kvm *kvm, 
 unsigned int nr_spis)
spin_lock_init(>irq_lock);
irq->vcpu = NULL;
irq->target_vcpu = vcpu0;
 +  kref_init(>refcount);
if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
irq->targets = 0;
else
 @@ -211,6 +212,7 @@ static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
irq->vcpu = NULL;
irq->target_vcpu = vcpu;
irq->targets = 1U << vcpu->vcpu_id;
 +  kref_init(>refcount);
if (vgic_irq_is_sgi(i)) {
/* SGIs */
irq->enabled = 1;
 diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c 
 b/virt/kvm/arm/vgic/vgic-mmio-v2.c
 index a213936..4152348 100644
 --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
 +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
 @@ -102,6 +102,7 @@ static void vgic_mmio_write_sgir(struct kvm_vcpu 
 *source_vcpu,
irq->source |= 1U << source_vcpu->vcpu_id;
  
vgic_queue_irq_unlock(source_vcpu->kvm, irq);
 +  vgic_put_irq(source_vcpu->kvm, irq);
}
  }
  
 @@ -116,6 +117,8 @@ static unsigned long vgic_mmio_read_target(struct 
 kvm_vcpu *vcpu,
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
  
val |= (u64)irq->targets << (i * 8);
 +
 +  vgic_put_irq(vcpu->kvm, irq);
}
  
return val;
 @@ -143,6 +146,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu 
 *vcpu,
irq->target_vcpu = kvm_get_vcpu(vcpu->kvm, target);
  
spin_unlock(>irq_lock);
 +  vgic_put_irq(vcpu->kvm, irq);
}
  }
  
 @@ -157,6 +161,8 @@ static unsigned long vgic_mmio_read_sgipend(struct 
 kvm_vcpu *vcpu,
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
  
val |= (u64)irq->source << (i * 8);
 +
 +  vgic_put_irq(vcpu->kvm, irq);
}
return val;
  }
 @@ -178,6 +184,7 @@ static void vgic_mmio_write_sgipendc(struct kvm_vcpu 
 *vcpu,
irq->pending = false;
  
spin_unlock(>irq_lock);
 +  vgic_put_irq(vcpu->kvm, irq);
}
  }
  
 @@ -201,6 +208,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu 
 *vcpu,
} else {
spin_unlock(>irq_lock);
}
 +  vgic_put_irq(vcpu->kvm, irq);
}

Re: [PATCH v8 06/17] KVM: arm/arm64: VGIC: add refcounting for IRQs

2016-07-08 Thread Marc Zyngier 
On 08/07/16 11:28, Andre Przywara wrote:
> Hi,
> 
> On 07/07/16 16:00, Marc Zyngier wrote:
>> On 05/07/16 12:22, Andre Przywara wrote:
>>> In the moment our struct vgic_irq's are statically allocated at guest
>>> creation time. So getting a pointer to an IRQ structure is trivial and
>>> safe. LPIs are more dynamic, they can be mapped and unmapped at any time
>>> during the guest's _runtime_.
>>> In preparation for supporting LPIs we introduce reference counting for
>>> those structures using the kernel's kref infrastructure.
>>> Since private IRQs and SPIs are statically allocated, the refcount never
>>> drops to 0 at the moment, but we increase it when an IRQ gets onto a VCPU
>>> list and decrease it when it gets removed.
>>> This introduces vgic_put_irq(), which wraps kref_put and hides the
>>> release function from the callers.
>>>
>>> Signed-off-by: Andre Przywara 
>>> ---
>>>  include/kvm/arm_vgic.h   |  1 +
>>>  virt/kvm/arm/vgic/vgic-init.c|  2 ++
>>>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  8 +++
>>>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 20 +++--
>>>  virt/kvm/arm/vgic/vgic-mmio.c| 25 -
>>>  virt/kvm/arm/vgic/vgic-v2.c  |  1 +
>>>  virt/kvm/arm/vgic/vgic-v3.c  |  1 +
>>>  virt/kvm/arm/vgic/vgic.c | 48 
>>> +++-
>>>  virt/kvm/arm/vgic/vgic.h |  1 +
>>>  9 files changed, 89 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>> index 5142e2a..450b4da 100644
>>> --- a/include/kvm/arm_vgic.h
>>> +++ b/include/kvm/arm_vgic.h
>>> @@ -96,6 +96,7 @@ struct vgic_irq {
>>> bool active;/* not used for LPIs */
>>> bool enabled;
>>> bool hw;/* Tied to HW IRQ */
>>> +   struct kref refcount;   /* Used for LPIs */
>>> u32 hwintid;/* HW INTID number */
>>> union {
>>> u8 targets; /* GICv2 target VCPUs mask */
>>> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
>>> index 90cae48..ac3c1a5 100644
>>> --- a/virt/kvm/arm/vgic/vgic-init.c
>>> +++ b/virt/kvm/arm/vgic/vgic-init.c
>>> @@ -177,6 +177,7 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned 
>>> int nr_spis)
>>> spin_lock_init(>irq_lock);
>>> irq->vcpu = NULL;
>>> irq->target_vcpu = vcpu0;
>>> +   kref_init(>refcount);
>>> if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
>>> irq->targets = 0;
>>> else
>>> @@ -211,6 +212,7 @@ static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>>> irq->vcpu = NULL;
>>> irq->target_vcpu = vcpu;
>>> irq->targets = 1U << vcpu->vcpu_id;
>>> +   kref_init(>refcount);
>>> if (vgic_irq_is_sgi(i)) {
>>> /* SGIs */
>>> irq->enabled = 1;
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c 
>>> b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>>> index a213936..4152348 100644
>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>>> @@ -102,6 +102,7 @@ static void vgic_mmio_write_sgir(struct kvm_vcpu 
>>> *source_vcpu,
>>> irq->source |= 1U << source_vcpu->vcpu_id;
>>>  
>>> vgic_queue_irq_unlock(source_vcpu->kvm, irq);
>>> +   vgic_put_irq(source_vcpu->kvm, irq);
>>> }
>>>  }
>>>  
>>> @@ -116,6 +117,8 @@ static unsigned long vgic_mmio_read_target(struct 
>>> kvm_vcpu *vcpu,
>>> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>>  
>>> val |= (u64)irq->targets << (i * 8);
>>> +
>>> +   vgic_put_irq(vcpu->kvm, irq);
>>> }
>>>  
>>> return val;
>>> @@ -143,6 +146,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu 
>>> *vcpu,
>>> irq->target_vcpu = kvm_get_vcpu(vcpu->kvm, target);
>>>  
>>> spin_unlock(>irq_lock);
>>> +   vgic_put_irq(vcpu->kvm, irq);
>>> }
>>>  }
>>>  
>>> @@ -157,6 +161,8 @@ static unsigned long vgic_mmio_read_sgipend(struct 
>>> kvm_vcpu *vcpu,
>>> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>>  
>>> val |= (u64)irq->source << (i * 8);
>>> +
>>> +   vgic_put_irq(vcpu->kvm, irq);
>>> }
>>> return val;
>>>  }
>>> @@ -178,6 +184,7 @@ static void vgic_mmio_write_sgipendc(struct kvm_vcpu 
>>> *vcpu,
>>> irq->pending = false;
>>>  
>>> spin_unlock(>irq_lock);
>>> +   vgic_put_irq(vcpu->kvm, irq);
>>> }
>>>  }
>>>  
>>> @@ -201,6 +208,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu 
>>> *vcpu,
>>> } else {
>>> spin_unlock(>irq_lock);
>>> }
>>> +   vgic_put_irq(vcpu->kvm, irq);
>>> }
>>>  }
>>>  
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c 
>>> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>>> index

Re: [PATCH v8 06/17] KVM: arm/arm64: VGIC: add refcounting for IRQs

2016-07-08 Thread Andre Przywara 
Hi,

On 07/07/16 16:00, Marc Zyngier wrote:
> On 05/07/16 12:22, Andre Przywara wrote:
>> In the moment our struct vgic_irq's are statically allocated at guest
>> creation time. So getting a pointer to an IRQ structure is trivial and
>> safe. LPIs are more dynamic, they can be mapped and unmapped at any time
>> during the guest's _runtime_.
>> In preparation for supporting LPIs we introduce reference counting for
>> those structures using the kernel's kref infrastructure.
>> Since private IRQs and SPIs are statically allocated, the refcount never
>> drops to 0 at the moment, but we increase it when an IRQ gets onto a VCPU
>> list and decrease it when it gets removed.
>> This introduces vgic_put_irq(), which wraps kref_put and hides the
>> release function from the callers.
>>
>> Signed-off-by: Andre Przywara 
>> ---
>>  include/kvm/arm_vgic.h   |  1 +
>>  virt/kvm/arm/vgic/vgic-init.c|  2 ++
>>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  8 +++
>>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 20 +++--
>>  virt/kvm/arm/vgic/vgic-mmio.c| 25 -
>>  virt/kvm/arm/vgic/vgic-v2.c  |  1 +
>>  virt/kvm/arm/vgic/vgic-v3.c  |  1 +
>>  virt/kvm/arm/vgic/vgic.c | 48 
>> +++-
>>  virt/kvm/arm/vgic/vgic.h |  1 +
>>  9 files changed, 89 insertions(+), 18 deletions(-)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index 5142e2a..450b4da 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -96,6 +96,7 @@ struct vgic_irq {
>>  bool active;/* not used for LPIs */
>>  bool enabled;
>>  bool hw;/* Tied to HW IRQ */
>> +struct kref refcount;   /* Used for LPIs */
>>  u32 hwintid;/* HW INTID number */
>>  union {
>>  u8 targets; /* GICv2 target VCPUs mask */
>> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
>> index 90cae48..ac3c1a5 100644
>> --- a/virt/kvm/arm/vgic/vgic-init.c
>> +++ b/virt/kvm/arm/vgic/vgic-init.c
>> @@ -177,6 +177,7 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned 
>> int nr_spis)
>>  spin_lock_init(>irq_lock);
>>  irq->vcpu = NULL;
>>  irq->target_vcpu = vcpu0;
>> +kref_init(>refcount);
>>  if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
>>  irq->targets = 0;
>>  else
>> @@ -211,6 +212,7 @@ static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>>  irq->vcpu = NULL;
>>  irq->target_vcpu = vcpu;
>>  irq->targets = 1U << vcpu->vcpu_id;
>> +kref_init(>refcount);
>>  if (vgic_irq_is_sgi(i)) {
>>  /* SGIs */
>>  irq->enabled = 1;
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c 
>> b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> index a213936..4152348 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> @@ -102,6 +102,7 @@ static void vgic_mmio_write_sgir(struct kvm_vcpu 
>> *source_vcpu,
>>  irq->source |= 1U << source_vcpu->vcpu_id;
>>  
>>  vgic_queue_irq_unlock(source_vcpu->kvm, irq);
>> +vgic_put_irq(source_vcpu->kvm, irq);
>>  }
>>  }
>>  
>> @@ -116,6 +117,8 @@ static unsigned long vgic_mmio_read_target(struct 
>> kvm_vcpu *vcpu,
>>  struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>  
>>  val |= (u64)irq->targets << (i * 8);
>> +
>> +vgic_put_irq(vcpu->kvm, irq);
>>  }
>>  
>>  return val;
>> @@ -143,6 +146,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu *vcpu,
>>  irq->target_vcpu = kvm_get_vcpu(vcpu->kvm, target);
>>  
>>  spin_unlock(>irq_lock);
>> +vgic_put_irq(vcpu->kvm, irq);
>>  }
>>  }
>>  
>> @@ -157,6 +161,8 @@ static unsigned long vgic_mmio_read_sgipend(struct 
>> kvm_vcpu *vcpu,
>>  struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>  
>>  val |= (u64)irq->source << (i * 8);
>> +
>> +vgic_put_irq(vcpu->kvm, irq);
>>  }
>>  return val;
>>  }
>> @@ -178,6 +184,7 @@ static void vgic_mmio_write_sgipendc(struct kvm_vcpu 
>> *vcpu,
>>  irq->pending = false;
>>  
>>  spin_unlock(>irq_lock);
>> +vgic_put_irq(vcpu->kvm, irq);
>>  }
>>  }
>>  
>> @@ -201,6 +208,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu 
>> *vcpu,
>>  } else {
>>  spin_unlock(>irq_lock);
>>  }
>> +vgic_put_irq(vcpu->kvm, irq);
>>  }
>>  }
>>  
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c 
>> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> index fc7b6c9..bfcafbd 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> @@ -80,15 +80,17 @@

Re: [PATCH v8 06/17] KVM: arm/arm64: VGIC: add refcounting for IRQs

2016-07-07 Thread Marc Zyngier 
On 05/07/16 12:22, Andre Przywara wrote:
> In the moment our struct vgic_irq's are statically allocated at guest
> creation time. So getting a pointer to an IRQ structure is trivial and
> safe. LPIs are more dynamic, they can be mapped and unmapped at any time
> during the guest's _runtime_.
> In preparation for supporting LPIs we introduce reference counting for
> those structures using the kernel's kref infrastructure.
> Since private IRQs and SPIs are statically allocated, the refcount never
> drops to 0 at the moment, but we increase it when an IRQ gets onto a VCPU
> list and decrease it when it gets removed.
> This introduces vgic_put_irq(), which wraps kref_put and hides the
> release function from the callers.
> 
> Signed-off-by: Andre Przywara 
> ---
>  include/kvm/arm_vgic.h   |  1 +
>  virt/kvm/arm/vgic/vgic-init.c|  2 ++
>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  8 +++
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 20 +++--
>  virt/kvm/arm/vgic/vgic-mmio.c| 25 -
>  virt/kvm/arm/vgic/vgic-v2.c  |  1 +
>  virt/kvm/arm/vgic/vgic-v3.c  |  1 +
>  virt/kvm/arm/vgic/vgic.c | 48 
> +++-
>  virt/kvm/arm/vgic/vgic.h |  1 +
>  9 files changed, 89 insertions(+), 18 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 5142e2a..450b4da 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -96,6 +96,7 @@ struct vgic_irq {
>   bool active;/* not used for LPIs */
>   bool enabled;
>   bool hw;/* Tied to HW IRQ */
> + struct kref refcount;   /* Used for LPIs */
>   u32 hwintid;/* HW INTID number */
>   union {
>   u8 targets; /* GICv2 target VCPUs mask */
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 90cae48..ac3c1a5 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -177,6 +177,7 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned 
> int nr_spis)
>   spin_lock_init(>irq_lock);
>   irq->vcpu = NULL;
>   irq->target_vcpu = vcpu0;
> + kref_init(>refcount);
>   if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
>   irq->targets = 0;
>   else
> @@ -211,6 +212,7 @@ static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>   irq->vcpu = NULL;
>   irq->target_vcpu = vcpu;
>   irq->targets = 1U << vcpu->vcpu_id;
> + kref_init(>refcount);
>   if (vgic_irq_is_sgi(i)) {
>   /* SGIs */
>   irq->enabled = 1;
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c 
> b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index a213936..4152348 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -102,6 +102,7 @@ static void vgic_mmio_write_sgir(struct kvm_vcpu 
> *source_vcpu,
>   irq->source |= 1U << source_vcpu->vcpu_id;
>  
>   vgic_queue_irq_unlock(source_vcpu->kvm, irq);
> + vgic_put_irq(source_vcpu->kvm, irq);
>   }
>  }
>  
> @@ -116,6 +117,8 @@ static unsigned long vgic_mmio_read_target(struct 
> kvm_vcpu *vcpu,
>   struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>  
>   val |= (u64)irq->targets << (i * 8);
> +
> + vgic_put_irq(vcpu->kvm, irq);
>   }
>  
>   return val;
> @@ -143,6 +146,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu *vcpu,
>   irq->target_vcpu = kvm_get_vcpu(vcpu->kvm, target);
>  
>   spin_unlock(>irq_lock);
> + vgic_put_irq(vcpu->kvm, irq);
>   }
>  }
>  
> @@ -157,6 +161,8 @@ static unsigned long vgic_mmio_read_sgipend(struct 
> kvm_vcpu *vcpu,
>   struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>  
>   val |= (u64)irq->source << (i * 8);
> +
> + vgic_put_irq(vcpu->kvm, irq);
>   }
>   return val;
>  }
> @@ -178,6 +184,7 @@ static void vgic_mmio_write_sgipendc(struct kvm_vcpu 
> *vcpu,
>   irq->pending = false;
>  
>   spin_unlock(>irq_lock);
> + vgic_put_irq(vcpu->kvm, irq);
>   }
>  }
>  
> @@ -201,6 +208,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu 
> *vcpu,
>   } else {
>   spin_unlock(>irq_lock);
>   }
> + vgic_put_irq(vcpu->kvm, irq);
>   }
>  }
>  
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c 
> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index fc7b6c9..bfcafbd 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -80,15 +80,17 @@ static unsigned long vgic_mmio_read_irouter(struct 
> kvm_vcpu *vcpu,
>  {
>   int intid = VGIC_ADDR_TO_INTID(addr,

Re: [PATCH v8 06/17] KVM: arm/arm64: VGIC: add refcounting for IRQs

2016-07-07 Thread Christoffer Dall 
On Tue, Jul 05, 2016 at 12:22:58PM +0100, Andre Przywara wrote:
> In the moment our struct vgic_irq's are statically allocated at guest
> creation time. So getting a pointer to an IRQ structure is trivial and
> safe. LPIs are more dynamic, they can be mapped and unmapped at any time
> during the guest's _runtime_.
> In preparation for supporting LPIs we introduce reference counting for
> those structures using the kernel's kref infrastructure.
> Since private IRQs and SPIs are statically allocated, the refcount never
> drops to 0 at the moment, but we increase it when an IRQ gets onto a VCPU
> list and decrease it when it gets removed.
> This introduces vgic_put_irq(), which wraps kref_put and hides the
> release function from the callers.
> 
> Signed-off-by: Andre Przywara 
> ---
>  include/kvm/arm_vgic.h   |  1 +
>  virt/kvm/arm/vgic/vgic-init.c|  2 ++
>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  8 +++
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 20 +++--
>  virt/kvm/arm/vgic/vgic-mmio.c| 25 -
>  virt/kvm/arm/vgic/vgic-v2.c  |  1 +
>  virt/kvm/arm/vgic/vgic-v3.c  |  1 +
>  virt/kvm/arm/vgic/vgic.c | 48 
> +++-
>  virt/kvm/arm/vgic/vgic.h |  1 +
>  9 files changed, 89 insertions(+), 18 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 5142e2a..450b4da 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -96,6 +96,7 @@ struct vgic_irq {
>   bool active;/* not used for LPIs */
>   bool enabled;
>   bool hw;/* Tied to HW IRQ */
> + struct kref refcount;   /* Used for LPIs */
>   u32 hwintid;/* HW INTID number */
>   union {
>   u8 targets; /* GICv2 target VCPUs mask */
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 90cae48..ac3c1a5 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -177,6 +177,7 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned 
> int nr_spis)
>   spin_lock_init(>irq_lock);
>   irq->vcpu = NULL;
>   irq->target_vcpu = vcpu0;
> + kref_init(>refcount);
>   if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
>   irq->targets = 0;
>   else
> @@ -211,6 +212,7 @@ static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>   irq->vcpu = NULL;
>   irq->target_vcpu = vcpu;
>   irq->targets = 1U << vcpu->vcpu_id;
> + kref_init(>refcount);
>   if (vgic_irq_is_sgi(i)) {
>   /* SGIs */
>   irq->enabled = 1;
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c 
> b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index a213936..4152348 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -102,6 +102,7 @@ static void vgic_mmio_write_sgir(struct kvm_vcpu 
> *source_vcpu,
>   irq->source |= 1U << source_vcpu->vcpu_id;
>  
>   vgic_queue_irq_unlock(source_vcpu->kvm, irq);
> + vgic_put_irq(source_vcpu->kvm, irq);
>   }
>  }
>  
> @@ -116,6 +117,8 @@ static unsigned long vgic_mmio_read_target(struct 
> kvm_vcpu *vcpu,
>   struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>  
>   val |= (u64)irq->targets << (i * 8);
> +
> + vgic_put_irq(vcpu->kvm, irq);
>   }
>  
>   return val;
> @@ -143,6 +146,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu *vcpu,
>   irq->target_vcpu = kvm_get_vcpu(vcpu->kvm, target);
>  
>   spin_unlock(>irq_lock);
> + vgic_put_irq(vcpu->kvm, irq);
>   }
>  }
>  
> @@ -157,6 +161,8 @@ static unsigned long vgic_mmio_read_sgipend(struct 
> kvm_vcpu *vcpu,
>   struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>  
>   val |= (u64)irq->source << (i * 8);
> +
> + vgic_put_irq(vcpu->kvm, irq);
>   }
>   return val;
>  }
> @@ -178,6 +184,7 @@ static void vgic_mmio_write_sgipendc(struct kvm_vcpu 
> *vcpu,
>   irq->pending = false;
>  
>   spin_unlock(>irq_lock);
> + vgic_put_irq(vcpu->kvm, irq);
>   }
>  }
>  
> @@ -201,6 +208,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu 
> *vcpu,
>   } else {
>   spin_unlock(>irq_lock);
>   }
> + vgic_put_irq(vcpu->kvm, irq);
>   }
>  }
>  
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c 
> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index fc7b6c9..bfcafbd 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -80,15 +80,17 @@ static unsigned long vgic_mmio_read_irouter(struct 
> kvm_vcpu *vcpu,
>  {
>   int intid =

[PATCH v8 06/17] KVM: arm/arm64: VGIC: add refcounting for IRQs

2016-07-05 Thread Andre Przywara 
In the moment our struct vgic_irq's are statically allocated at guest
creation time. So getting a pointer to an IRQ structure is trivial and
safe. LPIs are more dynamic, they can be mapped and unmapped at any time
during the guest's _runtime_.
In preparation for supporting LPIs we introduce reference counting for
those structures using the kernel's kref infrastructure.
Since private IRQs and SPIs are statically allocated, the refcount never
drops to 0 at the moment, but we increase it when an IRQ gets onto a VCPU
list and decrease it when it gets removed.
This introduces vgic_put_irq(), which wraps kref_put and hides the
release function from the callers.

Signed-off-by: Andre Przywara 
---
 include/kvm/arm_vgic.h   |  1 +
 virt/kvm/arm/vgic/vgic-init.c|  2 ++
 virt/kvm/arm/vgic/vgic-mmio-v2.c |  8 +++
 virt/kvm/arm/vgic/vgic-mmio-v3.c | 20 +++--
 virt/kvm/arm/vgic/vgic-mmio.c| 25 -
 virt/kvm/arm/vgic/vgic-v2.c  |  1 +
 virt/kvm/arm/vgic/vgic-v3.c  |  1 +
 virt/kvm/arm/vgic/vgic.c | 48 +++-
 virt/kvm/arm/vgic/vgic.h |  1 +
 9 files changed, 89 insertions(+), 18 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 5142e2a..450b4da 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -96,6 +96,7 @@ struct vgic_irq {
bool active;/* not used for LPIs */
bool enabled;
bool hw;/* Tied to HW IRQ */
+   struct kref refcount;   /* Used for LPIs */
u32 hwintid;/* HW INTID number */
union {
u8 targets; /* GICv2 target VCPUs mask */
diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
index 90cae48..ac3c1a5 100644
--- a/virt/kvm/arm/vgic/vgic-init.c
+++ b/virt/kvm/arm/vgic/vgic-init.c
@@ -177,6 +177,7 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int 
nr_spis)
spin_lock_init(>irq_lock);
irq->vcpu = NULL;
irq->target_vcpu = vcpu0;
+   kref_init(>refcount);
if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
irq->targets = 0;
else
@@ -211,6 +212,7 @@ static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
irq->vcpu = NULL;
irq->target_vcpu = vcpu;
irq->targets = 1U << vcpu->vcpu_id;
+   kref_init(>refcount);
if (vgic_irq_is_sgi(i)) {
/* SGIs */
irq->enabled = 1;
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index a213936..4152348 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -102,6 +102,7 @@ static void vgic_mmio_write_sgir(struct kvm_vcpu 
*source_vcpu,
irq->source |= 1U << source_vcpu->vcpu_id;
 
vgic_queue_irq_unlock(source_vcpu->kvm, irq);
+   vgic_put_irq(source_vcpu->kvm, irq);
}
 }
 
@@ -116,6 +117,8 @@ static unsigned long vgic_mmio_read_target(struct kvm_vcpu 
*vcpu,
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
 
val |= (u64)irq->targets << (i * 8);
+
+   vgic_put_irq(vcpu->kvm, irq);
}
 
return val;
@@ -143,6 +146,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu *vcpu,
irq->target_vcpu = kvm_get_vcpu(vcpu->kvm, target);
 
spin_unlock(>irq_lock);
+   vgic_put_irq(vcpu->kvm, irq);
}
 }
 
@@ -157,6 +161,8 @@ static unsigned long vgic_mmio_read_sgipend(struct kvm_vcpu 
*vcpu,
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
 
val |= (u64)irq->source << (i * 8);
+
+   vgic_put_irq(vcpu->kvm, irq);
}
return val;
 }
@@ -178,6 +184,7 @@ static void vgic_mmio_write_sgipendc(struct kvm_vcpu *vcpu,
irq->pending = false;
 
spin_unlock(>irq_lock);
+   vgic_put_irq(vcpu->kvm, irq);
}
 }
 
@@ -201,6 +208,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu,
} else {
spin_unlock(>irq_lock);
}
+   vgic_put_irq(vcpu->kvm, irq);
}
 }
 
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index fc7b6c9..bfcafbd 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -80,15 +80,17 @@ static unsigned long vgic_mmio_read_irouter(struct kvm_vcpu 
*vcpu,
 {
int intid = VGIC_ADDR_TO_INTID(addr, 64);
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, NULL, intid);
+   unsigned long ret = 0;
 
if (!irq)
return 0;
 
/* The upper word is RAZ for us. */
-