[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxx>
>> ---
>>   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@xxxxxxxxxx>
> 
> Cheers,
> 

Cheers,
Michal



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.