|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 06/45] xen/arm: gic: Split the field state in gic_lr in 2 fields active and pending
On Thu, 15 Mar 2018, Andre Przywara wrote:
> From: Julien Grall <julien.grall@xxxxxxx>
>
> Mostly making the code nicer to read.
>
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
> 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
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |