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

Message ID 20180315203050.19791-7-andre.przywara@linaro.org
State New
Headers show
Series
  • New VGIC(-v2) implementation
Related show

Commit Message

Andre Przywara March 15, 2018, 8:30 p.m.
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(-)

Comments

Stefano Stabellini March 16, 2018, 9:34 p.m. | #1
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
>
Julien Grall March 16, 2018, 10:14 p.m. | #2
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,
Stefano Stabellini March 16, 2018, 10:52 p.m. | #3
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>
Andre Przywara March 19, 2018, 9:10 a.m. | #4
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.

Patch

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