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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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...

> 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.

>      }
>  
>  }
> 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).

>      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