diff mbox series

[Xen-devel,4/6] xen/arm: gic: Split the field state in gic_lr in 2 fields active and pending

Message ID 20180309163511.18808-5-julien.grall@arm.com
State New
Headers show
Series xen/arm: Rework the way to store the LR | expand

Commit Message

Julien Grall March 9, 2018, 4:35 p.m. UTC 
From: Julien Grall <julien.grall@arm.com>

Mostly making the code nicer to read.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 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(-)

Comments

Andre Przywara March 14, 2018, 6:09 p.m. UTC | #1 
Hi,

On 09/03/18 16:35, julien.grall@arm.com wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> Mostly making the code nicer to read.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  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 23223575a2..90d8f652d3 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) == GICH_V2_LR_PENDING;
> +    lr_reg->active = (lrv & GICH_V2_LR_ACTIVE) == GICH_V2_LR_ACTIVE;
>      lr_reg->hw_status = (lrv & GICH_V2_LR_HW) == 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 0711e509a6..4dbbf0afd2 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) == ICH_LR_STATE_PENDING;
> +    lr_reg->active    = (lrv & ICH_LR_STATE_ACTIVE) == ICH_LR_STATE_ACTIVE;
>      lr_reg->hw_status = (lrv & ICH_LR_HW) == 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..817bb0d5c7 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         (1UL << 62)
> +#define ICH_LR_STATE_ACTIVE          (1UL << 63)

Should that be 1ULL, just in case we ever get 32-bit support for GICv3?

Regardless of that:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

>  #define ICH_LR_PRIORITY_MASK         0xff
>  #define ICH_LR_PRIORITY_SHIFT        48
>  #define ICH_LR_HW_MASK               0x1
>
Julien Grall March 14, 2018, 6:20 p.m. UTC | #2 
On 03/14/2018 06:09 PM, Andre Przywara wrote:
> On 09/03/18 16:35, julien.grall@arm.com wrote:
>> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
>> index ccb72cf0f1..817bb0d5c7 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         (1UL << 62)
>> +#define ICH_LR_STATE_ACTIVE          (1UL << 63)
> 
> Should that be 1ULL, just in case we ever get 32-bit support for GICv3?

Yes, good point. I will fix that.

> 
> Regardless of that:
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
diff mbox series

Patch

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 23223575a2..90d8f652d3 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) == GICH_V2_LR_PENDING;
+    lr_reg->active = (lrv & GICH_V2_LR_ACTIVE) == GICH_V2_LR_ACTIVE;
     lr_reg->hw_status = (lrv & GICH_V2_LR_HW) == 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 0711e509a6..4dbbf0afd2 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) == ICH_LR_STATE_PENDING;
+    lr_reg->active    = (lrv & ICH_LR_STATE_ACTIVE) == ICH_LR_STATE_ACTIVE;
     lr_reg->hw_status = (lrv & ICH_LR_HW) == 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..817bb0d5c7 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         (1UL << 62)
+#define ICH_LR_STATE_ACTIVE          (1UL << 63)
 #define ICH_LR_PRIORITY_MASK         0xff
 #define ICH_LR_PRIORITY_SHIFT        48
 #define ICH_LR_HW_MASK               0x1