Re: [Xen-devel] [PATCH v4 05/10] xen/arm: keep track of the GICH_LR used for the irq in struct pending_irq

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 21 Mar 2014, Ian Campbell wrote:
> On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> 
> Strictly you are tracking the last LR which this interrupt was in, since
> you don't clear p->lr AFAICT. Maybe this is OK and things never get
> confused by it, but it might have surprising results...

Actually the patch is clearing p->lr by setting it to nr_lrs, that of
course is invalid.


> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > ---
> >  xen/arch/arm/gic.c           |    6 ++++--
> >  xen/include/asm-arm/domain.h |    1 +
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index d445e8b..78e043c 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -644,6 +644,7 @@ static inline void gic_set_lr(int lr, struct 
> > pending_irq *p,
> >  
> >      set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> >      clear_bit(GIC_IRQ_GUEST_PENDING, &p->status);
> > +    p->lr = lr;
> >  }
> >  
> >  static inline void gic_add_to_lr_pending(struct vcpu *v, struct 
> > pending_irq *n)
> > @@ -724,6 +725,7 @@ static void gic_clear_lrs(struct vcpu *v)
> >              if ( p->desc != NULL )
> >                  p->desc->status &= ~IRQ_INPROGRESS;
> >              clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> > +            p->lr = nr_lrs;
> >              if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
> >                      test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> >              {
> > @@ -966,12 +968,12 @@ void gic_dump_info(struct vcpu *v)
> >  
> >      list_for_each_entry ( p, &v->arch.vgic.inflight_irqs, inflight )
> >      {
> > -        printk("Inflight irq=%d\n", p->irq);
> > +        printk("Inflight irq=%d lr=%u\n", p->irq, p->lr);
> >      }
> >  
> >      list_for_each_entry( p, &v->arch.vgic.lr_pending, lr_queue )
> >      {
> > -        printk("Pending irq=%d\n", p->irq);
> > +        printk("Pending irq=%d lr=%u\n", p->irq, p->lr);
> 
> Are lr_pending interrupts in an LR? I thought they were waiting for an
> LR to become available.

You are right, this change is wrong (and confusing).


> >      }
> >  
> >  }
> > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> > index bc20a15..ea89057 100644
> > --- a/xen/include/asm-arm/domain.h
> > +++ b/xen/include/asm-arm/domain.h
> > @@ -59,6 +59,7 @@ struct pending_irq
> >  #define GIC_IRQ_GUEST_VISIBLE  1
> >  #define GIC_IRQ_GUEST_ENABLED  2
> >      unsigned long status;
> > +    uint8_t lr;
> 
> Put this next to priority to improve the packing, you've just added
> another 7 byte hole to the struct on arm64 (and 3 on arm32).
>
> (pulling int irq from just out of scope down into the same area might
> also improve packing on arm64, since irq is just 4 bytes).

Good idea

> >      struct irq_desc *desc; /* only set it the irq corresponds to a 
> > physical irq */
> >      uint8_t priority;
> >      /* inflight is used to append instances of pending_irq to
> 
> 

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