[Xen-devel,PATCH-4.5,2/4] xen/arm: support HW interrupts in gic_set_lr

Message ID 1391799378-31664-2-git-send-email-stefano.stabellini@eu.citrix.com
State New
Headers show

Commit Message

Stefano Stabellini Feb. 7, 2014, 6:56 p.m.
If the irq to be injected is an hardware irq (p->desc != NULL), set
GICH_LR_HW.

Also add a struct vcpu* parameter to gic_set_lr.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/gic.c |   28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

Comments

Julien Grall Feb. 7, 2014, 10:31 p.m. | #1
Hi Stefano,

On 07/02/14 18:56, Stefano Stabellini wrote:
> If the irq to be injected is an hardware irq (p->desc != NULL), set
> GICH_LR_HW.

If you set the GICH_LR_HW, I think you should remove the EOI of physical 
interrupt in maintenance IRQ in this patch. Otherwise we will EOI twice 
and from the documentation the behavior is unpredicatable.

> Also add a struct vcpu* parameter to gic_set_lr.
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>   xen/arch/arm/gic.c |   28 ++++++++++++++++------------
>   1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index acf7195..215b679 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);
>
>       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 |
> -        ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> -        ((virtual_irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> +    if ( p->desc != NULL )
> +        GICH[GICH_LR + lr] = GICH_LR_HW | state | GICH_LR_MAINTENANCE_IRQ |
> +            ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> +            ((irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT) |

We should not assume that the physical IRQ == virtual IRQ. You should 
use p->desc->irq

> +            ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> +    else
> +        GICH[GICH_LR + lr] = state | GICH_LR_MAINTENANCE_IRQ |
> +            ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> +            ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);

The final result of virtual IRQ is a subset of the physical IRQ. Can you 
factor the code?
Stefano Stabellini Feb. 10, 2014, 4:50 p.m. | #2
On Fri, 7 Feb 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 07/02/14 18:56, Stefano Stabellini wrote:
> > If the irq to be injected is an hardware irq (p->desc != NULL), set
> > GICH_LR_HW.
> 
> If you set the GICH_LR_HW, I think you should remove the EOI of physical
> interrupt in maintenance IRQ in this patch. Otherwise we will EOI twice and
> from the documentation the behavior is unpredicatable.

Yes, you are right.


> > Also add a struct vcpu* parameter to gic_set_lr.
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >   xen/arch/arm/gic.c |   28 ++++++++++++++++------------
> >   1 file changed, 16 insertions(+), 12 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index acf7195..215b679 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);
> > 
> >       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 |
> > -        ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> > -        ((virtual_irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> > +    if ( p->desc != NULL )
> > +        GICH[GICH_LR + lr] = GICH_LR_HW | state | GICH_LR_MAINTENANCE_IRQ |
> > +            ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> > +            ((irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT) |
> 
> We should not assume that the physical IRQ == virtual IRQ. You should use
> p->desc->irq

Right, I'll make the change.


> > +            ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> > +    else
> > +        GICH[GICH_LR + lr] = state | GICH_LR_MAINTENANCE_IRQ |
> > +            ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> > +            ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> 
> The final result of virtual IRQ is a subset of the physical IRQ. Can you
> factor the code?

Yeah

Patch

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index acf7195..215b679 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);
 
     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 |
-        ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
-        ((virtual_irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
+    if ( p->desc != NULL )
+        GICH[GICH_LR + lr] = GICH_LR_HW | state | GICH_LR_MAINTENANCE_IRQ |
+            ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
+            ((irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT) |
+            ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
+    else
+        GICH[GICH_LR + lr] = state | GICH_LR_MAINTENANCE_IRQ |
+            ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
+            ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
 
     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);
@@ -950,7 +954,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));
         }