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.przyw...@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.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
