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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> > ---
> >   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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel