Re: [PATCH 28/57] ARM: new VGIC: Add acccessor to new struct vgic_irq instance

[Julien Grall <julien.grall@arm.com>]

Hi Andre,

On 05/03/18 16:03, Andre Przywara wrote:
> The new VGIC implementation centers around a struct vgic_irq instance
> per virtual IRQ.
> Provide a function to retrieve the right instance for a given IRQ
> number and (in case of private interrupts) the right VCPU.
> This also includes the corresponding put function, which does nothing
> for private interrupts and SPIs, but handles the ref-counting for LPIs.
> 
> This is based on Linux commit 64a959d66e47, written by Christoffer Dall.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
> Changelog RFC ... v1:
> - add kernel-doc comments to exported functions
> - adapt to previous changes (new_vgic.h, arch_vcpu member name)
> - use ASSERT_UNREACHABLE
> 
>   xen/arch/arm/vgic/vgic.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/vgic/vgic.h |  41 ++++++++++++++++
>   2 files changed, 165 insertions(+)
>   create mode 100644 xen/arch/arm/vgic/vgic.c
>   create mode 100644 xen/arch/arm/vgic/vgic.h
> 
> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> new file mode 100644
> index 0000000000..ace30f78d0
> --- /dev/null
> +++ b/xen/arch/arm/vgic/vgic.c
> @@ -0,0 +1,124 @@
> +/*
> + * Copyright (C) 2015, 2016 ARM Ltd.
> + * Imported from Linux ("new" KVM VGIC) and heavily adapted to Xen.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <asm/bug.h>
> +#include <asm/new_vgic.h>
> +#include <xen/sched.h>
> +
> +#include "vgic.h"
> +
> +/*
> + * Iterate over the VM's list of mapped LPIs to find the one with a
> + * matching interrupt ID and return a reference to the IRQ structure.
> + */
> +static struct vgic_irq *vgic_get_lpi(struct domain *d, u32 intid)
> +{
> +    struct vgic_dist *dist = &d->arch.vgic;
> +    struct vgic_irq *irq = NULL;
> +
> +    spin_lock(&dist->lpi_list_lock);
> +
> +    list_for_each_entry( irq, &dist->lpi_list_head, lpi_list )

I am still not a big fan of the list solution. Strictly speaking nobody 
is populating that list and likely going to be too slow in Xen case (I 
am thinking for the hardware domain). So I think I would prefer to see 
the LPI related code disappear for this cut. This could easily be added 
back as they are standalone.

> +    {
> +        if ( irq->intid != intid )
> +            continue;
> +
> +        /*
> +         * This increases the refcount, the caller is expected to
> +         * call vgic_put_irq() later once it's finished with the IRQ.
> +         */
> +        vgic_get_irq_kref(irq);
> +        goto out_unlock;
> +    }
> +    irq = NULL;
> +
> +out_unlock:
> +    spin_unlock(&dist->lpi_list_lock);
> +
> +    return irq;
> +}
> +
> +/**
> + * vgic_get_irq() - obtain a reference to a virtual IRQ
> + * @d:        The domain the virtual IRQ belongs to.
> + * @vcpu:     For private IRQs (SGIs, PPIs) the virtual CPU this IRQ
> + *            is associated with. Will be ignored for SPIs and LPIs.
> + * @intid:    The virtual IRQ number.
> + *
> + * This looks up the virtual interrupt ID to get the corresponding
> + * struct vgic_irq. It also increases the refcount, so any caller is expected
> + * to call vgic_put_irq() once it's finished with this IRQ.
> + *
> + * Return: The pointer to the requested struct vgic_irq.
> + */
> +struct vgic_irq *vgic_get_irq(struct domain *d, struct vcpu *vcpu,
> +                              u32 intid)
> +{
> +    /* SGIs and PPIs */
> +    if ( intid <= VGIC_MAX_PRIVATE )
> +        return &vcpu->arch.vgic.private_irqs[intid];
> +
> +    /* SPIs */
> +    if ( intid <= VGIC_MAX_SPI )
> +        return &d->arch.vgic.spis[intid - VGIC_NR_PRIVATE_IRQS];
> +
> +    /* LPIs */
> +    if ( intid >= VGIC_MIN_LPI )
> +        return vgic_get_lpi(d, intid);
> +
> +    ASSERT_UNREACHABLE();

You need to keep the return NULL here as ASSERT_UNREACHABLE() is turned 
to a nop on non-debug build.

My point with ASSERT is to avoid potential flaw in the code and a way to 
flood Xen console with the WARN in non-debug build.

> +}
> +
> +/**
> + * vgic_put_irq() - drop the reference to a virtual IRQ
> + * @d:        The domain the virtual IRQ belongs to.
> + * @irq:      The pointer to struct vgic_irq, as obtained from vgic_get_irq().
> + *
> + * This drops the reference to a virtual IRQ. It decreases the refcount
> + * of the pointer, so dynamic IRQs can be freed when no longer needed.
> + * This should always be called after a vgic_get_irq(), though the reference
> + * can be deliberately held for longer periods, if needed.
> + */
> +void vgic_put_irq(struct domain *d, struct vgic_irq *irq)
> +{
> +    struct vgic_dist *dist = &d->arch.vgic;
> +
> +    if ( irq->intid < VGIC_MIN_LPI )
> +        return;
> +
> +    spin_lock(&dist->lpi_list_lock);
> +    if ( !atomic_dec_and_test(&irq->refcount) )
> +    {
> +        spin_unlock(&dist->lpi_list_lock);
> +        return;
> +    };
> +
> +    list_del(&irq->lpi_list);
> +    dist->lpi_list_count--;
> +    spin_unlock(&dist->lpi_list_lock);
> +
> +    xfree(irq);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h
> new file mode 100644
> index 0000000000..a3befd386b
> --- /dev/null
> +++ b/xen/arch/arm/vgic/vgic.h
> @@ -0,0 +1,41 @@
> +/*
> + * Copyright (C) 2015, 2016 ARM Ltd.
> + * Imported from Linux ("new" KVM VGIC) and heavily adapted to Xen.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#ifndef __XEN_ARM_VGIC_VGIC_H__
> +#define __XEN_ARM_VGIC_VGIC_H__
> +
> +struct vgic_irq *vgic_get_irq(struct domain *d, struct vcpu *vcpu,
> +                              u32 intid);
> +void vgic_put_irq(struct domain *d, struct vgic_irq *irq);
> +
> +static inline void vgic_get_irq_kref(struct vgic_irq *irq)
> +{
> +    if ( irq->intid < VGIC_MIN_LPI )
> +        return;
> +
> +    atomic_inc(&irq->refcount);
> +}
> +
> +#endif
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel