Re: [PATCH-4.5 v2 02/10] xen/arm: support HW interrupts in gic_set_lr

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

On 02/14/2014 03:51 PM, Stefano Stabellini wrote:
> If the irq to be injected is an hardware irq (p->desc != NULL), set
> GICH_LR_HW.
> 
> Remove the code to EOI a physical interrupt on behalf of the guest
> because it has become unnecessary.
> 
> Also add a struct vcpu* parameter to gic_set_lr.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

IRL you told me that this patch as dependency on another. It would be
nice to mention this dependency in the commit message for bisection.

> ---
> 
> Changes in v2:
> - remove the EOI code, now unnecessary;
> - do not assume physical IRQ == virtual IRQ;
> - refactor gic_set_lr.
> ---
>  xen/arch/arm/gic.c |   52 +++++++++++++++++-----------------------------------
>  1 file changed, 17 insertions(+), 35 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index acf7195..64c8aa7 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -618,20 +618,24 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>      return rc;
>  }
>  
> -static inline void gic_set_lr(int lr, unsigned int virtual_irq,
> +static inline void gic_set_lr(struct vcpu *v, int lr, unsigned int irq,
>          unsigned int state, unsigned int priority)
>  {
> -    int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
> -    struct pending_irq *p = irq_to_pending(current, virtual_irq);
> +    struct pending_irq *p = irq_to_pending(v, 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 |
> +    lr_reg = state | GICH_LR_MAINTENANCE_IRQ |
>          ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> -        ((virtual_irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> +        ((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);
> @@ -666,7 +670,7 @@ void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
>      spin_unlock(&gic.lock);
>  }
>  
> -void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
> +void gic_set_guest_irq(struct vcpu *v, unsigned int irq,
>          unsigned int state, unsigned int priority)
>  {
>      int i;
> @@ -679,12 +683,12 @@ void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
>          i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
>          if (i < nr_lrs) {
>              set_bit(i, &this_cpu(lr_mask));
> -            gic_set_lr(i, virtual_irq, state, priority);
> +            gic_set_lr(v, i, irq, state, priority);
>              goto out;
>          }
>      }
>  
> -    gic_add_to_lr_pending(v, virtual_irq, priority);
> +    gic_add_to_lr_pending(v, irq, priority);
>  
>  out:
>      spin_unlock_irqrestore(&gic.lock, flags);
> @@ -703,7 +707,7 @@ static void gic_restore_pending_irqs(struct vcpu *v)
>          if ( i >= nr_lrs ) return;
>  
>          spin_lock_irqsave(&gic.lock, flags);
> -        gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
> +        gic_set_lr(v, i, p->irq, GICH_LR_PENDING, p->priority);
>          list_del_init(&p->lr_queue);
>          set_bit(i, &this_cpu(lr_mask));
>          spin_unlock_irqrestore(&gic.lock, flags);
> @@ -904,15 +908,9 @@ int gicv_setup(struct domain *d)
>  
>  }
>  
> -static void gic_irq_eoi(void *info)
> -{
> -    int virq = (uintptr_t) info;
> -    GICC[GICC_DIR] = virq;
> -}
> -
>  static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
>  {
> -    int i = 0, virq, pirq = -1;
> +    int i = 0, virq;
>      uint32_t lr;
>      struct vcpu *v = current;
>      uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t) GICH[GICH_EISR1]) << 32);
> @@ -920,10 +918,8 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
>      while ((i = find_next_bit((const long unsigned int *) &eisr,
>                                64, i)) < 64) {
>          struct pending_irq *p, *p2;
> -        int cpu;
>          bool_t inflight;
>  
> -        cpu = -1;
>          inflight = 0;
>  
>          spin_lock_irq(&gic.lock);
> @@ -933,12 +929,8 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
>          clear_bit(i, &this_cpu(lr_mask));
>  
>          p = irq_to_pending(v, virq);
> -        if ( p->desc != NULL ) {
> +        if ( p->desc != NULL )
>              p->desc->status &= ~IRQ_INPROGRESS;
> -            /* Assume only one pcpu needs to EOI the irq */
> -            cpu = p->desc->arch.eoi_cpu;
> -            pirq = p->desc->irq;
> -        }
>          if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
>               test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
>          {
> @@ -950,7 +942,7 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
>  
>          if ( !list_empty(&v->arch.vgic.lr_pending) ) {
>              p2 = list_entry(v->arch.vgic.lr_pending.next, typeof(*p2), lr_queue);
> -            gic_set_lr(i, p2->irq, GICH_LR_PENDING, p2->priority);
> +            gic_set_lr(v, i, p2->irq, GICH_LR_PENDING, p2->priority);
>              list_del_init(&p2->lr_queue);
>              set_bit(i, &this_cpu(lr_mask));
>          }
> @@ -963,16 +955,6 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
>              spin_unlock_irq(&v->arch.vgic.lock);
>          }
>  
> -        if ( p->desc != NULL ) {
> -            /* this is not racy because we can't receive another irq of the
> -             * same type until we EOI it.  */
> -            if ( cpu == smp_processor_id() )
> -                gic_irq_eoi((void*)(uintptr_t)pirq);
> -            else
> -                on_selected_cpus(cpumask_of(cpu),
> -                                 gic_irq_eoi, (void*)(uintptr_t)pirq, 0);
> -        }
> -
>          i++;
>      }
>  }
> 


-- 
Julien Grall