| Message ID | 1395232325-19226-5-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
Hi Stefano, On 03/19/2014 12:32 PM, Stefano Stabellini wrote: > 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; You are using uint8_t here but nr_lrs is defined as unsigned. Can you also change nr_lrs type to uint8_t? Regards,
On Wed, 19 Mar 2014, Julien Grall wrote: > Hi Stefano, > > On 03/19/2014 12:32 PM, Stefano Stabellini wrote: > > 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; > > You are using uint8_t here but nr_lrs is defined as unsigned. Can you > also change nr_lrs type to uint8_t? Yes, I'll add a patch for that.
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@eu.citrix.com> > --- > 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
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@eu.citrix.com> > > --- > > 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 > >
On Fri, 2014-03-21 at 16:19 +0000, Stefano Stabellini wrote: > 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. Ah, that is a bit non-obvious. Better would be #define INVALID_LR ~(type_t)0 and use that. > > > } > > > > > > } > > > 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 There was a tool which would tell you about the holes in your structs. Now what was it called...
On Fri, 21 Mar 2014, Ian Campbell wrote: > On Fri, 2014-03-21 at 16:19 +0000, Stefano Stabellini wrote: > > 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. > > Ah, that is a bit non-obvious. Better would be > #define INVALID_LR ~(type_t)0 > and use that. Yeah, this is a good idea, I'll make the change.
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); } } 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; 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
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- xen/arch/arm/gic.c | 6 ++++-- xen/include/asm-arm/domain.h | 1 + 2 files changed, 5 insertions(+), 2 deletions(-)