Re: [PATCH v4 03/10] xen/arm: support HW interrupts, do not request maintenance_interrupts

[Julien Grall <julien.grall@linaro.org>]

Hi Stefano,

On 03/19/2014 12:31 PM, Stefano Stabellini wrote:

[..]

> @@ -625,16 +628,19 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>  static inline void gic_set_lr(int lr, struct pending_irq *p,
>          unsigned int state)
>  {
> -    int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
> +    uint32_t lr_reg;
>  
>      BUG_ON(lr >= nr_lrs);
>      BUG_ON(lr < 0);
>      BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT));
>  
> -    GICH[GICH_LR + lr] = state |
> -        maintenance_int |
> -        ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> +    lr_reg = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
>          ((p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> +    if ( p->desc != NULL )
> +        lr_reg |= GICH_LR_HW |
> +            ((p->desc->irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT);
> +
> +    GICH[GICH_LR + lr] = lr_reg;
>  
>      set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
>      clear_bit(GIC_IRQ_GUEST_PENDING, &p->status);
> @@ -669,7 +675,7 @@ void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
>      spin_unlock_irqrestore(&gic.lock, flags);
>  }
>  
> -void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
> +void gic_set_guest_irq(struct vcpu *v, unsigned int irq,

Any reason to rename virtual_irq into irq?

[..]

> +static void gic_clear_lrs(struct vcpu *v)
> +{
> +    struct pending_irq *p;
> +    int i = 0, irq;
> +    uint32_t lr;
> +    bool_t inflight;
> +
> +    ASSERT(!local_irq_is_enabled());
> +
> +    while ((i = find_next_bit((const long unsigned int *) &this_cpu(lr_mask),
> +                              nr_lrs, i)) < nr_lrs) {
> +        lr = GICH[GICH_LR + i];
> +        if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
> +        {
> +            inflight = 0;
> +            GICH[GICH_LR + i] = 0;
> +            clear_bit(i, &this_cpu(lr_mask));
> +
> +            irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
> +            spin_lock(&gic.lock);

Not completely related to this patch ... taking gic.lock seems a bit too
strong here. The critical section only update data for the current
domain. It seems a bit stupid to block the other interrupt to handle
their interrupts at the same time.

Maybe introducing a dist lock would be a better solution?

[..]

>  void gic_dump_info(struct vcpu *v)
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index aab490c..566f0ff 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -701,8 +701,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
>          if ( (irq != current->domain->arch.evtchn_irq) ||
>               (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) )
>              set_bit(GIC_IRQ_GUEST_PENDING, &n->status);
> -        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> -        return;
> +        goto out;

We don't want to kick the other VCPU every time. I think it's enough
when the interrupt is updated.

Regards,

-- 
Julien Grall