| Message ID | 20180315203050.19791-7-andre.przywara@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | New VGIC(-v2) implementation | expand |
On Thu, 15 Mar 2018, Andre Przywara wrote: > From: Julien Grall <julien.grall@arm.com> > > Mostly making the code nicer to read. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > Changes: > - Use 1ULL > - Remove pointless == *_STATE_* > > xen/arch/arm/gic-v2.c | 15 +++++++++++---- > xen/arch/arm/gic-v3.c | 12 +++++++++--- > xen/arch/arm/gic-vgic.c | 6 +++--- > xen/include/asm-arm/gic.h | 3 ++- > xen/include/asm-arm/gic_v3_defs.h | 2 ++ > 5 files changed, 27 insertions(+), 11 deletions(-) > > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c > index 9d589115bd..6dae5c1e55 100644 > --- a/xen/arch/arm/gic-v2.c > +++ b/xen/arch/arm/gic-v2.c > @@ -51,6 +51,8 @@ > #define GICH_V2_LR_PHYSICAL_SHIFT 10 > #define GICH_V2_LR_STATE_MASK 0x3 > #define GICH_V2_LR_STATE_SHIFT 28 > +#define GICH_V2_LR_PENDING (1U << 28) > +#define GICH_V2_LR_ACTIVE (1U << 29) > #define GICH_V2_LR_PRIORITY_SHIFT 23 > #define GICH_V2_LR_PRIORITY_MASK 0x1f > #define GICH_V2_LR_HW_SHIFT 31 > @@ -467,7 +469,8 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg) > lr_reg->pirq = (lrv >> GICH_V2_LR_PHYSICAL_SHIFT) & GICH_V2_LR_PHYSICAL_MASK; > lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & GICH_V2_LR_VIRTUAL_MASK; > lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK; > - lr_reg->state = (lrv >> GICH_V2_LR_STATE_SHIFT) & GICH_V2_LR_STATE_MASK; > + lr_reg->pending = lrv & GICH_V2_LR_PENDING; > + lr_reg->active = lrv & GICH_V2_LR_ACTIVE; > lr_reg->hw_status = lrv & GICH_V2_LR_HW; > } > > @@ -478,9 +481,13 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg) > lrv = ( ((lr_reg->pirq & GICH_V2_LR_PHYSICAL_MASK) << GICH_V2_LR_PHYSICAL_SHIFT) | > ((lr_reg->virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT) | > ((uint32_t)(lr_reg->priority & GICH_V2_LR_PRIORITY_MASK) > - << GICH_V2_LR_PRIORITY_SHIFT) | > - ((uint32_t)(lr_reg->state & GICH_V2_LR_STATE_MASK) > - << GICH_V2_LR_STATE_SHIFT) ); > + << GICH_V2_LR_PRIORITY_SHIFT) ); > + > + if ( lr_reg->active ) > + lrv |= GICH_V2_LR_ACTIVE; > + > + if ( lr_reg->pending ) > + lrv |= GICH_V2_LR_PENDING; > > if ( lr_reg->hw_status ) > lrv |= GICH_V2_LR_HW; > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c > index f761ae60d6..6547b5eb0d 100644 > --- a/xen/arch/arm/gic-v3.c > +++ b/xen/arch/arm/gic-v3.c > @@ -1010,7 +1010,8 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg) > lr_reg->virq = (lrv >> ICH_LR_VIRTUAL_SHIFT) & ICH_LR_VIRTUAL_MASK; > > lr_reg->priority = (lrv >> ICH_LR_PRIORITY_SHIFT) & ICH_LR_PRIORITY_MASK; > - lr_reg->state = (lrv >> ICH_LR_STATE_SHIFT) & ICH_LR_STATE_MASK; > + lr_reg->pending = lrv & ICH_LR_STATE_PENDING; > + lr_reg->active = lrv & ICH_LR_STATE_ACTIVE; > lr_reg->hw_status = lrv & ICH_LR_HW; > } > > @@ -1020,8 +1021,13 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr) > > lrv = ( ((u64)(lr->pirq & ICH_LR_PHYSICAL_MASK) << ICH_LR_PHYSICAL_SHIFT)| > ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK) << ICH_LR_VIRTUAL_SHIFT) | > - ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT)| > - ((u64)(lr->state & ICH_LR_STATE_MASK) << ICH_LR_STATE_SHIFT) ); > + ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT) ); > + > + if ( lr->active ) > + lrv |= ICH_LR_STATE_ACTIVE; > + > + if ( lr->pending ) > + lrv |= ICH_LR_STATE_PENDING; > > if ( lr->hw_status ) > lrv |= ICH_LR_HW; > diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c > index e3cb47e80e..d831b35525 100644 > --- a/xen/arch/arm/gic-vgic.c > +++ b/xen/arch/arm/gic-vgic.c > @@ -189,7 +189,7 @@ static void gic_update_one_lr(struct vcpu *v, int i) > return; > } > > - if ( lr_val.state & GICH_LR_ACTIVE ) > + if ( lr_val.active ) > { > set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status); > if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) && > @@ -197,7 +197,7 @@ static void gic_update_one_lr(struct vcpu *v, int i) > { > if ( p->desc == NULL ) > { > - lr_val.state |= GICH_LR_PENDING; > + lr_val.pending = true; > gic_hw_ops->write_lr(i, &lr_val); > } > else > @@ -205,7 +205,7 @@ static void gic_update_one_lr(struct vcpu *v, int i) > irq, v->domain->domain_id, v->vcpu_id, i); > } > } > - else if ( lr_val.state & GICH_LR_PENDING ) > + else if ( lr_val.pending ) > { > int q __attribute__ ((unused)) = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status); > #ifdef GIC_DEBUG > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index daec51499c..c32861d4fa 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -209,7 +209,8 @@ struct gic_lr { > /* Virtual IRQ */ > uint32_t virq; > uint8_t priority; > - uint8_t state; > + bool active; > + bool pending; > bool hw_status; > }; I like the readability but dislike the increase memory usage. I would have kept a single uint8_t and I would have used status flags as an approach, maybe I would have improved on those flags. That said, it doesn't bother me enough to nack the patch :-) > diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h > index ccb72cf0f1..d9827bd84c 100644 > --- a/xen/include/asm-arm/gic_v3_defs.h > +++ b/xen/include/asm-arm/gic_v3_defs.h > @@ -171,6 +171,8 @@ > #define ICH_LR_PHYSICAL_SHIFT 32 > #define ICH_LR_STATE_MASK 0x3 > #define ICH_LR_STATE_SHIFT 62 > +#define ICH_LR_STATE_PENDING (1ULL << 62) > +#define ICH_LR_STATE_ACTIVE (1ULL << 63) > #define ICH_LR_PRIORITY_MASK 0xff > #define ICH_LR_PRIORITY_SHIFT 48 > #define ICH_LR_HW_MASK 0x1 > -- > 2.14.1 >
On 16/03/2018 21:34, Stefano Stabellini wrote: > On Thu, 15 Mar 2018, Andre Przywara wrote: >> From: Julien Grall <julien.grall@arm.com> >> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h >> index daec51499c..c32861d4fa 100644 >> --- a/xen/include/asm-arm/gic.h >> +++ b/xen/include/asm-arm/gic.h >> @@ -209,7 +209,8 @@ struct gic_lr { >> /* Virtual IRQ */ >> uint32_t virq; >> uint8_t priority; >> - uint8_t state; >> + bool active; >> + bool pending; >> bool hw_status; >> }; > > I like the readability but dislike the increase memory usage. I would > have kept a single uint8_t and I would have used status flags as an > approach, maybe I would have improved on those flags. Why is that important? gic_lr will only be allocated on the stack... Cheers,
On Fri, 16 Mar 2018, Julien Grall wrote: > On 16/03/2018 21:34, Stefano Stabellini wrote: > > On Thu, 15 Mar 2018, Andre Przywara wrote: > > > From: Julien Grall <julien.grall@arm.com> > > > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > > > index daec51499c..c32861d4fa 100644 > > > --- a/xen/include/asm-arm/gic.h > > > +++ b/xen/include/asm-arm/gic.h > > > @@ -209,7 +209,8 @@ struct gic_lr { > > > /* Virtual IRQ */ > > > uint32_t virq; > > > uint8_t priority; > > > - uint8_t state; > > > + bool active; > > > + bool pending; > > > bool hw_status; > > > }; > > > > I like the readability but dislike the increase memory usage. I would > > have kept a single uint8_t and I would have used status flags as an > > approach, maybe I would have improved on those flags. > > Why is that important? gic_lr will only be allocated on the stack... You are right, so it is even less important than I thought. Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Hi, On 16/03/18 22:52, Stefano Stabellini wrote: > On Fri, 16 Mar 2018, Julien Grall wrote: >> On 16/03/2018 21:34, Stefano Stabellini wrote: >>> On Thu, 15 Mar 2018, Andre Przywara wrote: >>>> From: Julien Grall <julien.grall@arm.com> >>>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h >>>> index daec51499c..c32861d4fa 100644 >>>> --- a/xen/include/asm-arm/gic.h >>>> +++ b/xen/include/asm-arm/gic.h >>>> @@ -209,7 +209,8 @@ struct gic_lr { >>>> /* Virtual IRQ */ >>>> uint32_t virq; >>>> uint8_t priority; >>>> - uint8_t state; >>>> + bool active; >>>> + bool pending; >>>> bool hw_status; >>>> }; >>> >>> I like the readability but dislike the increase memory usage. I would >>> have kept a single uint8_t and I would have used status flags as an >>> approach, maybe I would have improved on those flags. >> >> Why is that important? gic_lr will only be allocated on the stack... > > You are right, so it is even less important than I thought. ... especially given that this patch increases it from 11 to 12 bytes, just to fill up the padding. And actually patch 04 decreased the size of the structure by that one byte. So it was 12 bytes before this series, is 12 bytes after this patch and will be 12 bytes after the whole series. So actually no change at all. > Acked-by: Stefano Stabellini <sstabellini@kernel.org> Thanks! Andre.
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index 9d589115bd..6dae5c1e55 100644 --- a/xen/arch/arm/gic-v2.c +++ b/xen/arch/arm/gic-v2.c @@ -51,6 +51,8 @@ #define GICH_V2_LR_PHYSICAL_SHIFT 10 #define GICH_V2_LR_STATE_MASK 0x3 #define GICH_V2_LR_STATE_SHIFT 28 +#define GICH_V2_LR_PENDING (1U << 28) +#define GICH_V2_LR_ACTIVE (1U << 29) #define GICH_V2_LR_PRIORITY_SHIFT 23 #define GICH_V2_LR_PRIORITY_MASK 0x1f #define GICH_V2_LR_HW_SHIFT 31 @@ -467,7 +469,8 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg) lr_reg->pirq = (lrv >> GICH_V2_LR_PHYSICAL_SHIFT) & GICH_V2_LR_PHYSICAL_MASK; lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & GICH_V2_LR_VIRTUAL_MASK; lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK; - lr_reg->state = (lrv >> GICH_V2_LR_STATE_SHIFT) & GICH_V2_LR_STATE_MASK; + lr_reg->pending = lrv & GICH_V2_LR_PENDING; + lr_reg->active = lrv & GICH_V2_LR_ACTIVE; lr_reg->hw_status = lrv & GICH_V2_LR_HW; } @@ -478,9 +481,13 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg) lrv = ( ((lr_reg->pirq & GICH_V2_LR_PHYSICAL_MASK) << GICH_V2_LR_PHYSICAL_SHIFT) | ((lr_reg->virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT) | ((uint32_t)(lr_reg->priority & GICH_V2_LR_PRIORITY_MASK) - << GICH_V2_LR_PRIORITY_SHIFT) | - ((uint32_t)(lr_reg->state & GICH_V2_LR_STATE_MASK) - << GICH_V2_LR_STATE_SHIFT) ); + << GICH_V2_LR_PRIORITY_SHIFT) ); + + if ( lr_reg->active ) + lrv |= GICH_V2_LR_ACTIVE; + + if ( lr_reg->pending ) + lrv |= GICH_V2_LR_PENDING; if ( lr_reg->hw_status ) lrv |= GICH_V2_LR_HW; diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index f761ae60d6..6547b5eb0d 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -1010,7 +1010,8 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg) lr_reg->virq = (lrv >> ICH_LR_VIRTUAL_SHIFT) & ICH_LR_VIRTUAL_MASK; lr_reg->priority = (lrv >> ICH_LR_PRIORITY_SHIFT) & ICH_LR_PRIORITY_MASK; - lr_reg->state = (lrv >> ICH_LR_STATE_SHIFT) & ICH_LR_STATE_MASK; + lr_reg->pending = lrv & ICH_LR_STATE_PENDING; + lr_reg->active = lrv & ICH_LR_STATE_ACTIVE; lr_reg->hw_status = lrv & ICH_LR_HW; } @@ -1020,8 +1021,13 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr) lrv = ( ((u64)(lr->pirq & ICH_LR_PHYSICAL_MASK) << ICH_LR_PHYSICAL_SHIFT)| ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK) << ICH_LR_VIRTUAL_SHIFT) | - ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT)| - ((u64)(lr->state & ICH_LR_STATE_MASK) << ICH_LR_STATE_SHIFT) ); + ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT) ); + + if ( lr->active ) + lrv |= ICH_LR_STATE_ACTIVE; + + if ( lr->pending ) + lrv |= ICH_LR_STATE_PENDING; if ( lr->hw_status ) lrv |= ICH_LR_HW; diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c index e3cb47e80e..d831b35525 100644 --- a/xen/arch/arm/gic-vgic.c +++ b/xen/arch/arm/gic-vgic.c @@ -189,7 +189,7 @@ static void gic_update_one_lr(struct vcpu *v, int i) return; } - if ( lr_val.state & GICH_LR_ACTIVE ) + if ( lr_val.active ) { set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status); if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) && @@ -197,7 +197,7 @@ static void gic_update_one_lr(struct vcpu *v, int i) { if ( p->desc == NULL ) { - lr_val.state |= GICH_LR_PENDING; + lr_val.pending = true; gic_hw_ops->write_lr(i, &lr_val); } else @@ -205,7 +205,7 @@ static void gic_update_one_lr(struct vcpu *v, int i) irq, v->domain->domain_id, v->vcpu_id, i); } } - else if ( lr_val.state & GICH_LR_PENDING ) + else if ( lr_val.pending ) { int q __attribute__ ((unused)) = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status); #ifdef GIC_DEBUG diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index daec51499c..c32861d4fa 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -209,7 +209,8 @@ struct gic_lr { /* Virtual IRQ */ uint32_t virq; uint8_t priority; - uint8_t state; + bool active; + bool pending; bool hw_status; }; diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h index ccb72cf0f1..d9827bd84c 100644 --- a/xen/include/asm-arm/gic_v3_defs.h +++ b/xen/include/asm-arm/gic_v3_defs.h @@ -171,6 +171,8 @@ #define ICH_LR_PHYSICAL_SHIFT 32 #define ICH_LR_STATE_MASK 0x3 #define ICH_LR_STATE_SHIFT 62 +#define ICH_LR_STATE_PENDING (1ULL << 62) +#define ICH_LR_STATE_ACTIVE (1ULL << 63) #define ICH_LR_PRIORITY_MASK 0xff #define ICH_LR_PRIORITY_SHIFT 48 #define ICH_LR_HW_MASK 0x1