1. Patchew
  2. Xen
  3. View series

[PATCH] xen/arm: gic: Introduce GIC_PRI_{IRQ/IPI}_ALL

Michal Orzel posted 1 patch 2 years, 9 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20220302090212.115922-1-michal.orzel@arm.com
There is a newer version of this series
xen/arch/arm/gic-v2.c          | 12 +++---------
xen/arch/arm/gic-v3.c          | 16 +++-------------
xen/arch/arm/include/asm/gic.h | 12 ++++++++----
3 files changed, 14 insertions(+), 26 deletions(-)
[PATCH] xen/arm: gic: Introduce GIC_PRI_{IRQ/IPI}_ALL
Posted by Michal Orzel 2 years, 9 months ago 
Introduce macros GIC_PRI_IRQ_ALL and GIC_PRI_IPI_ALL to be used in all
the places where we want to set default priority for all the offsets
in interrupt priority register. This will improve readability and
allow to get rid of introducing variables just to store this value.

Take the opportunity to mark GIC_PRI_{IRQ/IPI} as unsigned values
to suppress static analyzer warnings as they are used in expressions
exceeding integer range (shifting into signed bit). Modify also other
priority related macros to be coherent.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 xen/arch/arm/gic-v2.c          | 12 +++---------
 xen/arch/arm/gic-v3.c          | 16 +++-------------
 xen/arch/arm/include/asm/gic.h | 12 ++++++++----
 3 files changed, 14 insertions(+), 26 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index b2adc8ec9a..2cc2f6bc18 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -373,9 +373,7 @@ static void __init gicv2_dist_init(void)
 
     /* Default priority for global interrupts */
     for ( i = 32; i < nr_lines; i += 4 )
-        writel_gicd(GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 |
-                    GIC_PRI_IRQ << 8 | GIC_PRI_IRQ,
-                    GICD_IPRIORITYR + (i / 4) * 4);
+        writel_gicd(GIC_PRI_IRQ_ALL, GICD_IPRIORITYR + (i / 4) * 4);
 
     /* Disable all global interrupts */
     for ( i = 32; i < nr_lines; i += 32 )
@@ -403,15 +401,11 @@ static void gicv2_cpu_init(void)
 
     /* Set SGI priorities */
     for ( i = 0; i < 16; i += 4 )
-        writel_gicd(GIC_PRI_IPI << 24 | GIC_PRI_IPI << 16 |
-                    GIC_PRI_IPI << 8 | GIC_PRI_IPI,
-                    GICD_IPRIORITYR + (i / 4) * 4);
+        writel_gicd(GIC_PRI_IPI_ALL, GICD_IPRIORITYR + (i / 4) * 4);
 
     /* Set PPI priorities */
     for ( i = 16; i < 32; i += 4 )
-        writel_gicd(GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 |
-                    GIC_PRI_IRQ << 8 | GIC_PRI_IRQ,
-                    GICD_IPRIORITYR + (i / 4) * 4);
+        writel_gicd(GIC_PRI_IRQ_ALL, GICD_IPRIORITYR + (i / 4) * 4);
 
     /* Local settings: interface controller */
     /* Don't mask by priority */
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 9a3a175ad7..3c472ed768 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -594,7 +594,6 @@ static void gicv3_set_irq_priority(struct irq_desc *desc,
 static void __init gicv3_dist_init(void)
 {
     uint32_t type;
-    uint32_t priority;
     uint64_t affinity;
     unsigned int nr_lines;
     int i;
@@ -621,11 +620,7 @@ static void __init gicv3_dist_init(void)
 
     /* Default priority for global interrupts */
     for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i += 4 )
-    {
-        priority = (GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 |
-                    GIC_PRI_IRQ << 8 | GIC_PRI_IRQ);
-        writel_relaxed(priority, GICD + GICD_IPRIORITYR + (i / 4) * 4);
-    }
+        writel_relaxed(GIC_PRI_IRQ_ALL, GICD + GICD_IPRIORITYR + (i / 4) * 4);
 
     /* Disable/deactivate all global interrupts */
     for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i += 32 )
@@ -806,7 +801,6 @@ static int __init gicv3_populate_rdist(void)
 static int gicv3_cpu_init(void)
 {
     int i, ret;
-    uint32_t priority;
 
     /* Register ourselves with the rest of the world */
     if ( gicv3_populate_rdist() )
@@ -826,16 +820,12 @@ static int gicv3_cpu_init(void)
     }
 
     /* Set priority on PPI and SGI interrupts */
-    priority = (GIC_PRI_IPI << 24 | GIC_PRI_IPI << 16 | GIC_PRI_IPI << 8 |
-                GIC_PRI_IPI);
     for (i = 0; i < NR_GIC_SGI; i += 4)
-        writel_relaxed(priority,
+        writel_relaxed(GIC_PRI_IPI_ALL,
                 GICD_RDIST_SGI_BASE + GICR_IPRIORITYR0 + (i / 4) * 4);
 
-    priority = (GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 | GIC_PRI_IRQ << 8 |
-                GIC_PRI_IRQ);
     for (i = NR_GIC_SGI; i < NR_GIC_LOCAL_IRQS; i += 4)
-        writel_relaxed(priority,
+        writel_relaxed(GIC_PRI_IRQ_ALL,
                 GICD_RDIST_SGI_BASE + GICR_IPRIORITYR0 + (i / 4) * 4);
 
     /*
diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
index c7f0c343d1..69ceac36b1 100644
--- a/xen/arch/arm/include/asm/gic.h
+++ b/xen/arch/arm/include/asm/gic.h
@@ -143,10 +143,14 @@
  *
  * A GIC must support a mimimum of 16 priority levels.
  */
-#define GIC_PRI_LOWEST     0xf0
-#define GIC_PRI_IRQ        0xa0
-#define GIC_PRI_IPI        0x90 /* IPIs must preempt normal interrupts */
-#define GIC_PRI_HIGHEST    0x80 /* Higher priorities belong to Secure-World */
+#define GIC_PRI_LOWEST     0xf0U
+#define GIC_PRI_IRQ        0xa0U
+#define GIC_PRI_IRQ_ALL    ((GIC_PRI_IRQ << 24) | (GIC_PRI_IRQ << 16) |\
+                            (GIC_PRI_IRQ << 8) | GIC_PRI_IRQ)
+#define GIC_PRI_IPI        0x90U /* IPIs must preempt normal interrupts */
+#define GIC_PRI_IPI_ALL    ((GIC_PRI_IPI << 24) | (GIC_PRI_IPI << 16) |\
+                            (GIC_PRI_IPI << 8) | GIC_PRI_IPI)
+#define GIC_PRI_HIGHEST    0x80U /* Higher priorities belong to Secure-World */
 #define GIC_PRI_TO_GUEST(pri) (pri >> 3) /* GICH_LR and GICH_VMCR only support
                                             5 bits for guest irq priority */
 
-- 
2.25.1
Re: [PATCH] xen/arm: gic: Introduce GIC_PRI_{IRQ/IPI}_ALL
Posted by Julien Grall 2 years, 9 months ago 
Hi Michal,

On 02/03/2022 09:02, Michal Orzel wrote:
> Introduce macros GIC_PRI_IRQ_ALL and GIC_PRI_IPI_ALL to be used in all
> the places where we want to set default priority for all the offsets
> in interrupt priority register. This will improve readability and
> allow to get rid of introducing variables just to store this value.
> 
> Take the opportunity to mark GIC_PRI_{IRQ/IPI} as unsigned values
> to suppress static analyzer warnings as they are used in expressions
> exceeding integer range (shifting into signed bit). Modify also other
> priority related macros to be coherent.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> ---
>   xen/arch/arm/gic-v2.c          | 12 +++---------
>   xen/arch/arm/gic-v3.c          | 16 +++-------------
>   xen/arch/arm/include/asm/gic.h | 12 ++++++++----
>   3 files changed, 14 insertions(+), 26 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index b2adc8ec9a..2cc2f6bc18 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -373,9 +373,7 @@ static void __init gicv2_dist_init(void)
>   
>       /* Default priority for global interrupts */
>       for ( i = 32; i < nr_lines; i += 4 )
> -        writel_gicd(GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 |
> -                    GIC_PRI_IRQ << 8 | GIC_PRI_IRQ,
> -                    GICD_IPRIORITYR + (i / 4) * 4);
> +        writel_gicd(GIC_PRI_IRQ_ALL, GICD_IPRIORITYR + (i / 4) * 4);
>   
>       /* Disable all global interrupts */
>       for ( i = 32; i < nr_lines; i += 32 )
> @@ -403,15 +401,11 @@ static void gicv2_cpu_init(void)
>   
>       /* Set SGI priorities */
>       for ( i = 0; i < 16; i += 4 )
> -        writel_gicd(GIC_PRI_IPI << 24 | GIC_PRI_IPI << 16 |
> -                    GIC_PRI_IPI << 8 | GIC_PRI_IPI,
> -                    GICD_IPRIORITYR + (i / 4) * 4);
> +        writel_gicd(GIC_PRI_IPI_ALL, GICD_IPRIORITYR + (i / 4) * 4);
>   
>       /* Set PPI priorities */
>       for ( i = 16; i < 32; i += 4 )
> -        writel_gicd(GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 |
> -                    GIC_PRI_IRQ << 8 | GIC_PRI_IRQ,
> -                    GICD_IPRIORITYR + (i / 4) * 4);
> +        writel_gicd(GIC_PRI_IRQ_ALL, GICD_IPRIORITYR + (i / 4) * 4);
>   
>       /* Local settings: interface controller */
>       /* Don't mask by priority */
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 9a3a175ad7..3c472ed768 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -594,7 +594,6 @@ static void gicv3_set_irq_priority(struct irq_desc *desc,
>   static void __init gicv3_dist_init(void)
>   {
>       uint32_t type;
> -    uint32_t priority;
>       uint64_t affinity;
>       unsigned int nr_lines;
>       int i;
> @@ -621,11 +620,7 @@ static void __init gicv3_dist_init(void)
>   
>       /* Default priority for global interrupts */
>       for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i += 4 )
> -    {
> -        priority = (GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 |
> -                    GIC_PRI_IRQ << 8 | GIC_PRI_IRQ);
> -        writel_relaxed(priority, GICD + GICD_IPRIORITYR + (i / 4) * 4);
> -    }
> +        writel_relaxed(GIC_PRI_IRQ_ALL, GICD + GICD_IPRIORITYR + (i / 4) * 4);
>   
>       /* Disable/deactivate all global interrupts */
>       for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i += 32 )
> @@ -806,7 +801,6 @@ static int __init gicv3_populate_rdist(void)
>   static int gicv3_cpu_init(void)
>   {
>       int i, ret;
> -    uint32_t priority;
>   
>       /* Register ourselves with the rest of the world */
>       if ( gicv3_populate_rdist() )
> @@ -826,16 +820,12 @@ static int gicv3_cpu_init(void)
>       }
>   
>       /* Set priority on PPI and SGI interrupts */
> -    priority = (GIC_PRI_IPI << 24 | GIC_PRI_IPI << 16 | GIC_PRI_IPI << 8 |
> -                GIC_PRI_IPI);
>       for (i = 0; i < NR_GIC_SGI; i += 4)
> -        writel_relaxed(priority,
> +        writel_relaxed(GIC_PRI_IPI_ALL,
>                   GICD_RDIST_SGI_BASE + GICR_IPRIORITYR0 + (i / 4) * 4);
>   
> -    priority = (GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 | GIC_PRI_IRQ << 8 |
> -                GIC_PRI_IRQ);
>       for (i = NR_GIC_SGI; i < NR_GIC_LOCAL_IRQS; i += 4)
> -        writel_relaxed(priority,
> +        writel_relaxed(GIC_PRI_IRQ_ALL,
>                   GICD_RDIST_SGI_BASE + GICR_IPRIORITYR0 + (i / 4) * 4);
>   
>       /*
> diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
> index c7f0c343d1..69ceac36b1 100644
> --- a/xen/arch/arm/include/asm/gic.h
> +++ b/xen/arch/arm/include/asm/gic.h
> @@ -143,10 +143,14 @@
>    *
>    * A GIC must support a mimimum of 16 priority levels.
>    */
> -#define GIC_PRI_LOWEST     0xf0
> -#define GIC_PRI_IRQ        0xa0
> -#define GIC_PRI_IPI        0x90 /* IPIs must preempt normal interrupts */
> -#define GIC_PRI_HIGHEST    0x80 /* Higher priorities belong to Secure-World */
> +#define GIC_PRI_LOWEST     0xf0U
> +#define GIC_PRI_IRQ        0xa0U
> +#define GIC_PRI_IRQ_ALL    ((GIC_PRI_IRQ << 24) | (GIC_PRI_IRQ << 16) |\
> +                            (GIC_PRI_IRQ << 8) | GIC_PRI_IRQ)
> +#define GIC_PRI_IPI        0x90U /* IPIs must preempt normal interrupts */
> +#define GIC_PRI_IPI_ALL    ((GIC_PRI_IPI << 24) | (GIC_PRI_IPI << 16) |\
> +                            (GIC_PRI_IPI << 8) | GIC_PRI_IPI)
This is matter of taste. I think it would read better to keep 
GIC_PRI_{LOWEST, IRQ, IPI, HIGHEST} defined together and then define 
*_ALL separately.

> +#define GIC_PRI_HIGHEST    0x80U /* Higher priorities belong to Secure-World */

NIT: While you are there, I would suggest to add a newline here. So we 
separate priority definitions from handy helpers.

>   #define GIC_PRI_TO_GUEST(pri) (pri >> 3) /* GICH_LR and GICH_VMCR only support
>                                               5 bits for guest irq priority */

Other than that. The patch itself looks good to me. So:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall
Re: [PATCH] xen/arm: gic: Introduce GIC_PRI_{IRQ/IPI}_ALL
Posted by Michal Orzel 2 years, 9 months ago 
Hi Julien,

On 3/2/22 10:40, Julien Grall wrote:
> Hi Michal,
> 
> On 02/03/2022 09:02, Michal Orzel wrote:
>> Introduce macros GIC_PRI_IRQ_ALL and GIC_PRI_IPI_ALL to be used in all
>> the places where we want to set default priority for all the offsets
>> in interrupt priority register. This will improve readability and
>> allow to get rid of introducing variables just to store this value.
>>
>> Take the opportunity to mark GIC_PRI_{IRQ/IPI} as unsigned values
>> to suppress static analyzer warnings as they are used in expressions
>> exceeding integer range (shifting into signed bit). Modify also other
>> priority related macros to be coherent.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>> ---
>>   xen/arch/arm/gic-v2.c          | 12 +++---------
>>   xen/arch/arm/gic-v3.c          | 16 +++-------------
>>   xen/arch/arm/include/asm/gic.h | 12 ++++++++----
>>   3 files changed, 14 insertions(+), 26 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
>> index b2adc8ec9a..2cc2f6bc18 100644
>> --- a/xen/arch/arm/gic-v2.c
>> +++ b/xen/arch/arm/gic-v2.c
>> @@ -373,9 +373,7 @@ static void __init gicv2_dist_init(void)
>>         /* Default priority for global interrupts */
>>       for ( i = 32; i < nr_lines; i += 4 )
>> -        writel_gicd(GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 |
>> -                    GIC_PRI_IRQ << 8 | GIC_PRI_IRQ,
>> -                    GICD_IPRIORITYR + (i / 4) * 4);
>> +        writel_gicd(GIC_PRI_IRQ_ALL, GICD_IPRIORITYR + (i / 4) * 4);
>>         /* Disable all global interrupts */
>>       for ( i = 32; i < nr_lines; i += 32 )
>> @@ -403,15 +401,11 @@ static void gicv2_cpu_init(void)
>>         /* Set SGI priorities */
>>       for ( i = 0; i < 16; i += 4 )
>> -        writel_gicd(GIC_PRI_IPI << 24 | GIC_PRI_IPI << 16 |
>> -                    GIC_PRI_IPI << 8 | GIC_PRI_IPI,
>> -                    GICD_IPRIORITYR + (i / 4) * 4);
>> +        writel_gicd(GIC_PRI_IPI_ALL, GICD_IPRIORITYR + (i / 4) * 4);
>>         /* Set PPI priorities */
>>       for ( i = 16; i < 32; i += 4 )
>> -        writel_gicd(GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 |
>> -                    GIC_PRI_IRQ << 8 | GIC_PRI_IRQ,
>> -                    GICD_IPRIORITYR + (i / 4) * 4);
>> +        writel_gicd(GIC_PRI_IRQ_ALL, GICD_IPRIORITYR + (i / 4) * 4);
>>         /* Local settings: interface controller */
>>       /* Don't mask by priority */
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 9a3a175ad7..3c472ed768 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -594,7 +594,6 @@ static void gicv3_set_irq_priority(struct irq_desc *desc,
>>   static void __init gicv3_dist_init(void)
>>   {
>>       uint32_t type;
>> -    uint32_t priority;
>>       uint64_t affinity;
>>       unsigned int nr_lines;
>>       int i;
>> @@ -621,11 +620,7 @@ static void __init gicv3_dist_init(void)
>>         /* Default priority for global interrupts */
>>       for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i += 4 )
>> -    {
>> -        priority = (GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 |
>> -                    GIC_PRI_IRQ << 8 | GIC_PRI_IRQ);
>> -        writel_relaxed(priority, GICD + GICD_IPRIORITYR + (i / 4) * 4);
>> -    }
>> +        writel_relaxed(GIC_PRI_IRQ_ALL, GICD + GICD_IPRIORITYR + (i / 4) * 4);
>>         /* Disable/deactivate all global interrupts */
>>       for ( i = NR_GIC_LOCAL_IRQS; i < nr_lines; i += 32 )
>> @@ -806,7 +801,6 @@ static int __init gicv3_populate_rdist(void)
>>   static int gicv3_cpu_init(void)
>>   {
>>       int i, ret;
>> -    uint32_t priority;
>>         /* Register ourselves with the rest of the world */
>>       if ( gicv3_populate_rdist() )
>> @@ -826,16 +820,12 @@ static int gicv3_cpu_init(void)
>>       }
>>         /* Set priority on PPI and SGI interrupts */
>> -    priority = (GIC_PRI_IPI << 24 | GIC_PRI_IPI << 16 | GIC_PRI_IPI << 8 |
>> -                GIC_PRI_IPI);
>>       for (i = 0; i < NR_GIC_SGI; i += 4)
>> -        writel_relaxed(priority,
>> +        writel_relaxed(GIC_PRI_IPI_ALL,
>>                   GICD_RDIST_SGI_BASE + GICR_IPRIORITYR0 + (i / 4) * 4);
>>   -    priority = (GIC_PRI_IRQ << 24 | GIC_PRI_IRQ << 16 | GIC_PRI_IRQ << 8 |
>> -                GIC_PRI_IRQ);
>>       for (i = NR_GIC_SGI; i < NR_GIC_LOCAL_IRQS; i += 4)
>> -        writel_relaxed(priority,
>> +        writel_relaxed(GIC_PRI_IRQ_ALL,
>>                   GICD_RDIST_SGI_BASE + GICR_IPRIORITYR0 + (i / 4) * 4);
>>         /*
>> diff --git a/xen/arch/arm/include/asm/gic.h b/xen/arch/arm/include/asm/gic.h
>> index c7f0c343d1..69ceac36b1 100644
>> --- a/xen/arch/arm/include/asm/gic.h
>> +++ b/xen/arch/arm/include/asm/gic.h
>> @@ -143,10 +143,14 @@
>>    *
>>    * A GIC must support a mimimum of 16 priority levels.
>>    */
>> -#define GIC_PRI_LOWEST     0xf0
>> -#define GIC_PRI_IRQ        0xa0
>> -#define GIC_PRI_IPI        0x90 /* IPIs must preempt normal interrupts */
>> -#define GIC_PRI_HIGHEST    0x80 /* Higher priorities belong to Secure-World */
>> +#define GIC_PRI_LOWEST     0xf0U
>> +#define GIC_PRI_IRQ        0xa0U
>> +#define GIC_PRI_IRQ_ALL    ((GIC_PRI_IRQ << 24) | (GIC_PRI_IRQ << 16) |\
>> +                            (GIC_PRI_IRQ << 8) | GIC_PRI_IRQ)
>> +#define GIC_PRI_IPI        0x90U /* IPIs must preempt normal interrupts */
>> +#define GIC_PRI_IPI_ALL    ((GIC_PRI_IPI << 24) | (GIC_PRI_IPI << 16) |\
>> +                            (GIC_PRI_IPI << 8) | GIC_PRI_IPI)
> This is matter of taste. I think it would read better to keep GIC_PRI_{LOWEST, IRQ, IPI, HIGHEST} defined together and then define *_ALL separately.
> 
Ok, I will do this in v2.
>> +#define GIC_PRI_HIGHEST    0x80U /* Higher priorities belong to Secure-World */
> 
> NIT: While you are there, I would suggest to add a newline here. So we separate priority definitions from handy helpers.
> 
Ok.
>>   #define GIC_PRI_TO_GUEST(pri) (pri >> 3) /* GICH_LR and GICH_VMCR only support
>>                                               5 bits for guest irq priority */
> 
> Other than that. The patch itself looks good to me. So:
> 
> Acked-by: Julien Grall <jgrall@amazon.com>
> 
> Cheers,
> 

Cheers,
Michal

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.