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

[Andre Przywara <andre.przywara@arm.com>]

Hi Marc,

On 13/07/16 13:15, Marc Zyngier wrote:
> On 13/07/16 02:58, 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 <andre.przywara@arm.com>
>> ---
>>  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         | 53 ++++++++++++++++++++++++++++++++--------
>>  virt/kvm/arm/vgic/vgic.h         |  1 +
>>  9 files changed, 94 insertions(+), 18 deletions(-)
> 
> [...]
> 
>> --- a/virt/kvm/arm/vgic/vgic.c
>> +++ b/virt/kvm/arm/vgic/vgic.c
>> @@ -48,13 +48,20 @@ struct vgic_global __section(.hyp.text) kvm_vgic_global_state;
>>  struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
>>  			      u32 intid)
>>  {
>> -	/* SGIs and PPIs */
>> -	if (intid <= VGIC_MAX_PRIVATE)
>> -		return &vcpu->arch.vgic_cpu.private_irqs[intid];
>> +	struct vgic_dist *dist = &kvm->arch.vgic;
>> +	struct vgic_irq *irq;
>>  
>> -	/* SPIs */
>> -	if (intid <= VGIC_MAX_SPI)
>> -		return &kvm->arch.vgic.spis[intid - VGIC_NR_PRIVATE_IRQS];
>> +	if (intid <= VGIC_MAX_PRIVATE) {        /* SGIs and PPIs */
>> +		irq = &vcpu->arch.vgic_cpu.private_irqs[intid];
>> +		kref_get(&irq->refcount);
>> +		return irq;
>> +	}
>> +
>> +	if (intid <= VGIC_MAX_SPI) {            /* SPIs */
>> +		irq = &dist->spis[intid - VGIC_NR_PRIVATE_IRQS];
>> +		kref_get(&irq->refcount);
>> +		return irq;
>> +	}
> 
> I'm a bit concerned by the fact that we perform the refcounting on 
> objects that shouldn't be concerned by it. None of the static interrupts
> should have to suffer from the overhead, as they cannot be freed. 
> So I came up with the following patch:
> 
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index c7cd1a3..6bbff9a 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -91,19 +91,12 @@ struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
>  			      u32 intid)
>  {
>  	struct vgic_dist *dist = &kvm->arch.vgic;
> -	struct vgic_irq *irq;
>  
> -	if (intid <= VGIC_MAX_PRIVATE) {        /* SGIs and PPIs */
> -		irq = &vcpu->arch.vgic_cpu.private_irqs[intid];
> -		kref_get(&irq->refcount);
> -		return irq;
> -	}
> +	if (intid <= VGIC_MAX_PRIVATE)		/* SGIs and PPIs */
> +		return &vcpu->arch.vgic_cpu.private_irqs[intid];
>  
> -	if (intid <= VGIC_MAX_SPI) {            /* SPIs */
> -		irq = &dist->spis[intid - VGIC_NR_PRIVATE_IRQS];
> -		kref_get(&irq->refcount);
> -		return irq;
> -	}
> +	if (intid <= VGIC_MAX_SPI)		/* SPIs */
> +		return &dist->spis[intid - VGIC_NR_PRIVATE_IRQS];
>  
>  	if (intid >= VGIC_MIN_LPI)		/* LPIs */
>  		return vgic_get_lpi(kvm, intid);
> @@ -112,6 +105,14 @@ struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
>  	return NULL;
>  }
>  
> +static void vgic_get_irq_kref(struct vgic_irq *irq)
> +{
> +	if (irq->intid < VGIC_MIN_LPI)
> +		return;
> +
> +	kref_get(&irq->refcount);
> +}
> +
>  /*
>   * We can't do anything in here, because we lack the kvm pointer to
>   * lock and remove the item from the lpi_list. So we keep this function
> @@ -125,10 +126,10 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
>  {
>  	struct vgic_dist *dist;
>  
> -	if (!kref_put(&irq->refcount, vgic_irq_release))
> +	if (irq->intid < VGIC_MIN_LPI)
>  		return;
>  
> -	if (irq->intid < VGIC_MIN_LPI)
> +	if (!kref_put(&irq->refcount, vgic_irq_release))
>  		return;
>  
>  	dist = &kvm->arch.vgic;
> @@ -313,7 +314,11 @@ retry:
>  		goto retry;
>  	}
>  
> -	kref_get(&irq->refcount);
> +	/*
> +	 * Grab a reference to the irq to reflect the fact that it is
> +	 * now in the ap_list.
> +	 */
> +	vgic_get_irq_kref(irq);
>  	list_add_tail(&irq->ap_list, &vcpu->arch.vgic_cpu.ap_list_head);
>  	irq->vcpu = vcpu;
>  
> @@ -473,8 +478,11 @@ retry:
>  			spin_unlock(&irq->irq_lock);
>  
>  			/*
> -			 * This put matches the get when we added the LPI to
> -			 * the ap_list. We now drop the reference from the list.
> +			 * This vgic_put_irq call matches the
> +			 * vgic_get_irq_kref in vgic_queue_irq_unlock,
> +			 * where we added the LPI to the ap_list. As
> +			 * we remove the irq from the list, we drop
> +			 * also drop the refcount.
>  			 */
>  			vgic_put_irq(vcpu->kvm, irq);
>  			continue;
> 
> Thoughts?

Looks good to me, I checked the code and quickly tested it as well,
without any issues.
I'd integrate this in next respin.

Thanks!
Andre.