diff mbox series

[Xen-devel,04/57] ARM: GICv3: simplify GICv3 redistributor stride handling

Message ID 20180305160415.16760-5-andre.przywara@linaro.org
State New
Headers show
Series New VGIC(-v2) implementation | expand

Commit Message

Andre Przywara March 5, 2018, 4:03 p.m. UTC 
Instead of hard coding the architected redistributor stride into the
code, lets use a clear #define to the two values for GICv3 and GICv4 and
clarify the algorithm to determine the needed stride value.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
Changelog RFC ... v1:
- no changes

 xen/arch/arm/gic-v3.c             | 18 ++++++++++--------
 xen/include/asm-arm/gic_v3_defs.h |  5 +++++
 2 files changed, 15 insertions(+), 8 deletions(-)

Comments

Julien Grall March 5, 2018, 5:08 p.m. UTC | #1 
Hi Andre,

On 05/03/18 16:03, Andre Przywara wrote:
> Instead of hard coding the architected redistributor stride into the
> code, lets use a clear #define to the two values for GICv3 and GICv4 and
> clarify the algorithm to determine the needed stride value.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
> Changelog RFC ... v1:
> - no changes
> 
>   xen/arch/arm/gic-v3.c             | 18 ++++++++++--------
>   xen/include/asm-arm/gic_v3_defs.h |  5 +++++
>   2 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index b1f8a86409..be1787b39a 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -690,6 +690,15 @@ static int __init gicv3_populate_rdist(void)
>           do {
>               typer = readq_relaxed(ptr + GICR_TYPER);
>   
> +            /* Set the architectural redist size if not overridden by DT. */
> +            if ( !gicv3.rdist_stride )
> +            {
> +                if ( typer & GICR_TYPER_VLPIS )

Is there anything in the spec promising you that *all* the 
redistributors will support vLPIs?

But I am not fully convinced of having this patch in Xen. I feel it 
makes the code more confusing to read. The current code shows that the 
next re-distributor start is based on the current TYPER values. So I 
think it would make sense to just rework the current code:

if ( gicv3.rdist_stride )
{
    ptr += ...
}
else
{
   if ( typer & GICR_TYPER_VLPIS )
     ptr += GICV4_GICR_SIZE;
   else
     ptr += GICV3_GICR_SIZE;
}

> +                    gicv3.rdist_stride = GICV4_GICR_SIZE;
> +                else
> +                    gicv3.rdist_stride = GICV3_GICR_SIZE;
> +            } > +
>               if ( (typer >> 32) == aff )
>               {
>                   this_cpu(rbase) = ptr;
> @@ -732,14 +741,7 @@ static int __init gicv3_populate_rdist(void)
>               if ( gicv3.rdist_regions[i].single_rdist )
>                   break;
>   
> -            if ( gicv3.rdist_stride )
> -                ptr += gicv3.rdist_stride;
> -            else
> -            {
> -                ptr += SZ_64K * 2; /* Skip RD_base + SGI_base */
> -                if ( typer & GICR_TYPER_VLPIS )
> -                    ptr += SZ_64K * 2; /* Skip VLPI_base + reserved page */
> -            }
> +            ptr += gicv3.rdist_stride;
>   
>           } while ( !(typer & GICR_TYPER_LAST) );
>       }
> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
> index 65c9dc47cf..412e41afed 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -18,6 +18,8 @@
>   #ifndef __ASM_ARM_GIC_V3_DEFS_H__
>   #define __ASM_ARM_GIC_V3_DEFS_H__
>   
> +#include <xen/sizes.h>
> +
>   /*
>    * Additional registers defined in GIC v3.
>    * Common GICD registers are defined in gic.h
> @@ -68,6 +70,9 @@
>   #define GICV3_GICD_IIDR_VAL          0x34c
>   #define GICV3_GICR_IIDR_VAL          GICV3_GICD_IIDR_VAL
>   
> +#define GICV3_GICR_SIZE              (2 * SZ_64K)
> +#define GICV4_GICR_SIZE              (4 * SZ_64K)

Do you mind adding a comment either linking to the spec (Section 8.10 
ARM IHI 0069D) or explain the 2/4?

> +
>   #define GICR_CTLR                    (0x0000)
>   #define GICR_IIDR                    (0x0004)
>   #define GICR_TYPER                   (0x0008)
> 

Cheers,
Julien Grall March 6, 2018, 1:49 p.m. UTC | #2 
On 05/03/18 17:08, Julien Grall wrote:
> On 05/03/18 16:03, Andre Przywara wrote:
>> Instead of hard coding the architected redistributor stride into the
>> code, lets use a clear #define to the two values for GICv3 and GICv4 and
>> clarify the algorithm to determine the needed stride value.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>> Changelog RFC ... v1:
>> - no changes
>>
>>   xen/arch/arm/gic-v3.c             | 18 ++++++++++--------
>>   xen/include/asm-arm/gic_v3_defs.h |  5 +++++
>>   2 files changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index b1f8a86409..be1787b39a 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -690,6 +690,15 @@ static int __init gicv3_populate_rdist(void)
>>           do {
>>               typer = readq_relaxed(ptr + GICR_TYPER);
>> +            /* Set the architectural redist size if not overridden by 
>> DT. */
>> +            if ( !gicv3.rdist_stride )
>> +            {
>> +                if ( typer & GICR_TYPER_VLPIS )
> 
> Is there anything in the spec promising you that *all* the 
> redistributors will support vLPIs?

Answering to myself, Marc pointed out that nothing was promising all 
redistributors will support vLPIs. This is confirmed by the section 9.7 
"Mixing GICv3 and GICv4" in "GICv3 and GICv4 Software Overview" DAI 0492B.

So I would prefer to drop that patch and move GICV*_GICR_SIZE definition 
in patch #5. I am assuming we will never expose a mix of GICv3/GICv4 on Xen.

Cheers,
Andre Przywara March 8, 2018, 12:40 p.m. UTC | #3 
Hi,

On 06/03/18 13:49, Julien Grall wrote:
> 
> 
> On 05/03/18 17:08, Julien Grall wrote:
>> On 05/03/18 16:03, Andre Przywara wrote:
>>> Instead of hard coding the architected redistributor stride into the
>>> code, lets use a clear #define to the two values for GICv3 and GICv4 and
>>> clarify the algorithm to determine the needed stride value.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>>> ---
>>> Changelog RFC ... v1:
>>> - no changes
>>>
>>>   xen/arch/arm/gic-v3.c             | 18 ++++++++++--------
>>>   xen/include/asm-arm/gic_v3_defs.h |  5 +++++
>>>   2 files changed, 15 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>>> index b1f8a86409..be1787b39a 100644
>>> --- a/xen/arch/arm/gic-v3.c
>>> +++ b/xen/arch/arm/gic-v3.c
>>> @@ -690,6 +690,15 @@ static int __init gicv3_populate_rdist(void)
>>>           do {
>>>               typer = readq_relaxed(ptr + GICR_TYPER);
>>> +            /* Set the architectural redist size if not overridden
>>> by DT. */
>>> +            if ( !gicv3.rdist_stride )
>>> +            {
>>> +                if ( typer & GICR_TYPER_VLPIS )
>>
>> Is there anything in the spec promising you that *all* the
>> redistributors will support vLPIs?
> 
> Answering to myself, Marc pointed out that nothing was promising all
> redistributors will support vLPIs. This is confirmed by the section 9.7
> "Mixing GICv3 and GICv4" in "GICv3 and GICv4 Software Overview" DAI 0492B.
> 
> So I would prefer to drop that patch and move GICV*_GICR_SIZE definition
> in patch #5.

So I can surely do that, but then we have a definition of
GICV*_GICR_SIZE, but still the hardcoded values in the code.
So what about I keep this as a patch, but remove the actual code change
and just replace it with using the symbol names instead of the "SZ_64K *
2" plus comment?

Cheers,
Andre.


 I am assuming we will never expose a mix of GICv3/GICv4 on
> Xen.
> 
> Cheers,
>
Julien Grall March 8, 2018, 3:29 p.m. UTC | #4 
On 08/03/18 12:40, Andre Przywara wrote:
> Hi,

Hi,

> 
> On 06/03/18 13:49, Julien Grall wrote:
>>
>>
>> On 05/03/18 17:08, Julien Grall wrote:
>>> On 05/03/18 16:03, Andre Przywara wrote:
>>>> Instead of hard coding the architected redistributor stride into the
>>>> code, lets use a clear #define to the two values for GICv3 and GICv4 and
>>>> clarify the algorithm to determine the needed stride value.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>>>> ---
>>>> Changelog RFC ... v1:
>>>> - no changes
>>>>
>>>>    xen/arch/arm/gic-v3.c             | 18 ++++++++++--------
>>>>    xen/include/asm-arm/gic_v3_defs.h |  5 +++++
>>>>    2 files changed, 15 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>>>> index b1f8a86409..be1787b39a 100644
>>>> --- a/xen/arch/arm/gic-v3.c
>>>> +++ b/xen/arch/arm/gic-v3.c
>>>> @@ -690,6 +690,15 @@ static int __init gicv3_populate_rdist(void)
>>>>            do {
>>>>                typer = readq_relaxed(ptr + GICR_TYPER);
>>>> +            /* Set the architectural redist size if not overridden
>>>> by DT. */
>>>> +            if ( !gicv3.rdist_stride )
>>>> +            {
>>>> +                if ( typer & GICR_TYPER_VLPIS )
>>>
>>> Is there anything in the spec promising you that *all* the
>>> redistributors will support vLPIs?
>>
>> Answering to myself, Marc pointed out that nothing was promising all
>> redistributors will support vLPIs. This is confirmed by the section 9.7
>> "Mixing GICv3 and GICv4" in "GICv3 and GICv4 Software Overview" DAI 0492B.
>>
>> So I would prefer to drop that patch and move GICV*_GICR_SIZE definition
>> in patch #5.
> 
> So I can surely do that, but then we have a definition of
> GICV*_GICR_SIZE, but still the hardcoded values in the code.
> So what about I keep this as a patch, but remove the actual code change
> and just replace it with using the symbol names instead of the "SZ_64K *
> 2" plus comment?

GICV*_GICR_SIZE feels a bit weird to read. More that reading the spec 
gives the impression that it is possible to have GICv4 without vLPIS 
(see the description of GICR_TYPER.vLPIS).

So I think I would prefer to keep the hardcoding values here.

Cheers,
diff mbox series

Patch

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index b1f8a86409..be1787b39a 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -690,6 +690,15 @@  static int __init gicv3_populate_rdist(void)
         do {
             typer = readq_relaxed(ptr + GICR_TYPER);
 
+            /* Set the architectural redist size if not overridden by DT. */
+            if ( !gicv3.rdist_stride )
+            {
+                if ( typer & GICR_TYPER_VLPIS )
+                    gicv3.rdist_stride = GICV4_GICR_SIZE;
+                else
+                    gicv3.rdist_stride = GICV3_GICR_SIZE;
+            }
+
             if ( (typer >> 32) == aff )
             {
                 this_cpu(rbase) = ptr;
@@ -732,14 +741,7 @@  static int __init gicv3_populate_rdist(void)
             if ( gicv3.rdist_regions[i].single_rdist )
                 break;
 
-            if ( gicv3.rdist_stride )
-                ptr += gicv3.rdist_stride;
-            else
-            {
-                ptr += SZ_64K * 2; /* Skip RD_base + SGI_base */
-                if ( typer & GICR_TYPER_VLPIS )
-                    ptr += SZ_64K * 2; /* Skip VLPI_base + reserved page */
-            }
+            ptr += gicv3.rdist_stride;
 
         } while ( !(typer & GICR_TYPER_LAST) );
     }
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index 65c9dc47cf..412e41afed 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -18,6 +18,8 @@ 
 #ifndef __ASM_ARM_GIC_V3_DEFS_H__
 #define __ASM_ARM_GIC_V3_DEFS_H__
 
+#include <xen/sizes.h>
+
 /*
  * Additional registers defined in GIC v3.
  * Common GICD registers are defined in gic.h
@@ -68,6 +70,9 @@ 
 #define GICV3_GICD_IIDR_VAL          0x34c
 #define GICV3_GICR_IIDR_VAL          GICV3_GICD_IIDR_VAL
 
+#define GICV3_GICR_SIZE              (2 * SZ_64K)
+#define GICV4_GICR_SIZE              (4 * SZ_64K)
+
 #define GICR_CTLR                    (0x0000)
 #define GICR_IIDR                    (0x0004)
 #define GICR_TYPER                   (0x0008)