Re: [Xen-devel] [PATCH v4 5/8] ARM: VGIC: factor out vgic_connect_hw_irq()

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 02/05/2018 04:19 PM, Andre Przywara wrote:
> At the moment we happily access VGIC internal data structures like
> the rank and struct pending_irq in gic.c, which should be VGIC agnostic.
>
> Factor out a new function vgic_connect_hw_irq(), which allows a virtual
> IRQ to be connected to a hardware IRQ (using the hw bit in the LR).
>
> This removes said accesses to VGIC data structures and improves abstraction.
I was expecting some explanation regarding the new locking order in the 
commit message.
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
>   xen/arch/arm/gic-vgic.c    | 36 ++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/gic.c         | 44 ++++++++++----------------------------------
>   xen/include/asm-arm/vgic.h |  2 ++
>   3 files changed, 48 insertions(+), 34 deletions(-)
>
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index 1d5744ecc8..fff7c01ee8 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -397,6 +397,42 @@ void gic_dump_vgic_info(struct vcpu *v)
>           printk("Pending irq=%d\n", p->irq);
>   }
>   
> +int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
> +                        struct irq_desc *desc, bool connect)
> +{
> +    unsigned long flags;
> +    /* Use vcpu0 to retrieve the pending_irq struct. Given that we only
> +     * route SPIs to guests, it doesn't make any difference. */
Please fix the coding style as requested in v3.

> +    struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
> +    struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
> +    struct pending_irq *p = irq_to_pending(v_target, virq);
> +    int ret = 0;
> +
> +    ASSERT(connect && desc);
I am not sure why you allow desc to be non-NULL when disconnecting it 
(see more below).
> +
> +    /* We are taking to rank lock to prevent parallel connections. */
> +    vgic_lock_rank(v_target, rank, flags);
> +
> +    if ( connect )
> +    {
> +        /* The VIRQ should not be already enabled by the guest */
> +        if ( !p->desc &&
> +             !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
> +            p->desc = desc;
> +        else
> +            ret = -EBUSY;
> +    }
> +    else
> +    {
> +        if ( !desc || p->desc == desc )
From a quick glance, no caller will have desc is NULL. Even if it was, 
it will not harm because p->desc will be set to NULL.
> +            p->desc = NULL;
> +    }
But likely you want to return an error if p->desc != desc as this is a 
user input error. Ignoring it is a pretty bad.
> +
> +    vgic_unlock_rank(v_target, rank, flags);
> +
> +    return ret;
> +}
> +
>   /*
>    * Local variables:
>    * mode: C
Cheers,

--
Julien Grall

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