[PATCH 0/2] GICv3: Clean up + enable support on GICv4

[PATCH 0/2] GICv3: Clean up + enable support on GICv4

[Julien Grall]

Hi all,

This small patch series allow Xen to run on platform reporting GICv4
in the GIC*_PIDR2.

Sincerely yours,

Julien Grall (2):
  xen/arm: gic-v3: Clean-up the GIC*_PIDR2_* definitions
  xen/arm: gic-v3: Allow Xen to run on hardware reporting GICv4

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

-- 
2.1.4

[PATCH 1/2] xen/arm: gic-v3: Clean-up the GIC*_PIDR2_* definitions

[Julien Grall]

GICR_PIDR2 and GICD_PIDR2 use the same register layout. Rather than
define twice, one of which is an alias to the other, introduce GIC_PIDR2_*
defines.

Also:
    * Use the same prefix for the mask and the value
    * Integrate the shift in the value to avoid shifting in the code
    * Use GICv* to match the value name in the spec
    * Move them in a proper place

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/gic-v3.c             |  8 ++++----
 xen/include/asm-arm/gic_v3_defs.h | 12 ++++--------
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index d1db1ce..4d623bf 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -639,8 +639,8 @@ static int __init gicv3_populate_rdist(void)
     {
         void __iomem *ptr = gicv3.rdist_regions[i].map_base;
 
-        reg = readl_relaxed(ptr + GICR_PIDR2) & GICR_PIDR2_ARCH_REV_MASK;
-        if ( (reg >> GICR_PIDR2_ARCH_REV_SHIFT) != GICR_PIDR2_ARCH_GICV3 )
+        reg = readl_relaxed(ptr + GICR_PIDR2) & GIC_PIDR2_ARCH_MASK;
+        if ( reg != GIC_PIDR2_ARCH_GICv3 )
         {
             dprintk(XENLOG_ERR,
                     "GICv3: No redistributor present @%"PRIpaddr"\n",
@@ -1193,8 +1193,8 @@ static int __init gicv3_init(void)
     if ( !gicv3.map_dbase )
         panic("GICv3: Failed to ioremap for GIC distributor\n");
 
-    reg = readl_relaxed(GICD + GICD_PIDR2) & GICD_PIDR2_ARCH_REV_MASK;
-    if ( ((reg >> GICD_PIDR2_ARCH_REV_SHIFT) != GICD_PIDR2_ARCH_GICV3) )
+    reg = readl_relaxed(GICD + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
+    if ( reg != GIC_PIDR2_ARCH_GICv3 )
          panic("GICv3: no distributor detected\n");
 
     if ( !dt_property_read_u32(node, "#redistributor-regions",
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index bf7b239..b0ac6ff 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -40,6 +40,10 @@
 #define GICD_PIDR5                   (0xFFD4)
 #define GICD_PIDR7                   (0xFFDC)
 
+/* Common between GICD_PIDR2 and GICR_PIDR2 */
+#define GIC_PIDR2_ARCH_MASK         (0xf0)
+#define GIC_PIDR2_ARCH_GICv3        (0x30)
+
 #define GICC_SRE_EL2_SRE             (1UL << 0)
 #define GICC_SRE_EL2_DFB             (1UL << 1)
 #define GICC_SRE_EL2_DIB             (1UL << 2)
@@ -59,14 +63,6 @@
 #define GICR_WAKER_ProcessorSleep    (1U << 1)
 #define GICR_WAKER_ChildrenAsleep    (1U << 2)
 
-#define GICD_PIDR2_ARCH_REV_MASK     (0xf0)
-#define GICD_PIDR2_ARCH_REV_SHIFT    (0x4)
-#define GICD_PIDR2_ARCH_GICV3        (0x3)
-
-#define GICR_PIDR2_ARCH_REV_MASK     GICD_PIDR2_ARCH_REV_MASK
-#define GICR_PIDR2_ARCH_REV_SHIFT    GICD_PIDR2_ARCH_REV_SHIFT
-#define GICR_PIDR2_ARCH_GICV3        GICD_PIDR2_ARCH_GICV3
-
 #define GICR_SYNCR_NOT_BUSY          1
 /*
  * Implementation defined value JEP106?
-- 
2.1.4

Re: [PATCH 1/2] xen/arm: gic-v3: Clean-up the GIC*_PIDR2_* definitions

[Ian Campbell]

On Mon, 2015-09-14 at 16:32 +0100, Julien Grall wrote:
> GICR_PIDR2 and GICD_PIDR2 use the same register layout. Rather than
> define twice, one of which is an alias to the other, introduce
> GIC_PIDR2_*
> defines.
> 
> Also:
>     * Use the same prefix for the mask and the value
>     * Integrate the shift in the value to avoid shifting in the code
>     * Use GICv* to match the value name in the spec
>     * Move them in a proper place
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH 2/2] xen/arm: gic-v3: Allow Xen to run on hardware reporting GICv4

[Julien Grall]

It seems that there is some hardware which report start to report GICv4
in the GIC*_PIDR2 register.

As GICv4 is a superset of GICv3, it should just work on Xen.

Reported-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/gic-v3.c             | 4 ++--
 xen/include/asm-arm/gic_v3_defs.h | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 4d623bf..1e3c19b 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -640,7 +640,7 @@ static int __init gicv3_populate_rdist(void)
         void __iomem *ptr = gicv3.rdist_regions[i].map_base;
 
         reg = readl_relaxed(ptr + GICR_PIDR2) & GIC_PIDR2_ARCH_MASK;
-        if ( reg != GIC_PIDR2_ARCH_GICv3 )
+        if ( reg != GIC_PIDR2_ARCH_GICv3 && reg != GIC_PIDR2_ARCH_GICv4 )
         {
             dprintk(XENLOG_ERR,
                     "GICv3: No redistributor present @%"PRIpaddr"\n",
@@ -1194,7 +1194,7 @@ static int __init gicv3_init(void)
         panic("GICv3: Failed to ioremap for GIC distributor\n");
 
     reg = readl_relaxed(GICD + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
-    if ( reg != GIC_PIDR2_ARCH_GICv3 )
+    if ( reg != GIC_PIDR2_ARCH_GICv3 && reg != GIC_PIDR2_ARCH_GICv4 )
          panic("GICv3: no distributor detected\n");
 
     if ( !dt_property_read_u32(node, "#redistributor-regions",
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index b0ac6ff..c6d73df 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -43,6 +43,7 @@
 /* Common between GICD_PIDR2 and GICR_PIDR2 */
 #define GIC_PIDR2_ARCH_MASK         (0xf0)
 #define GIC_PIDR2_ARCH_GICv3        (0x30)
+#define GIC_PIDR2_ARCH_GICv4        (0x40)
 
 #define GICC_SRE_EL2_SRE             (1UL << 0)
 #define GICC_SRE_EL2_DFB             (1UL << 1)
-- 
2.1.4

Re: [PATCH 2/2] xen/arm: gic-v3: Allow Xen to run on hardware reporting GICv4

[Ian Campbell]

On Mon, 2015-09-14 at 16:32 +0100, Julien Grall wrote:
> It seems that there is some hardware which report start to report GICv4

s/report start to reports/reports/ ?

Also, this is an odd way to express it, what you mean is that some hardware
is now shipping with GICv4. Unless you are trying to imply that they are
claiming to be GICv4 without actually being so?

(If we agree on some wording I can modify this text on commit, subject to
the discussion below).

> in the GIC*_PIDR2 register.
> 
> As GICv4 is a superset of GICv3, it should just work on Xen.
> 
> Reported-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> ---
>  xen/arch/arm/gic-v3.c             | 4 ++--
>  xen/include/asm-arm/gic_v3_defs.h | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 4d623bf..1e3c19b 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -640,7 +640,7 @@ static int __init gicv3_populate_rdist(void)
>          void __iomem *ptr = gicv3.rdist_regions[i].map_base;
>  
>          reg = readl_relaxed(ptr + GICR_PIDR2) & GIC_PIDR2_ARCH_MASK;
> -        if ( reg != GIC_PIDR2_ARCH_GICv3 )
> +        if ( reg != GIC_PIDR2_ARCH_GICv3 && reg != GIC_PIDR2_ARCH_GICv4 )

Once we have GICv5, 6, etc this is going to get unwieldy, shall we switch
to a switch now?

>          {
>              dprintk(XENLOG_ERR,
>                      "GICv3: No redistributor present @%"PRIpaddr"\n",

I wonder if GICv3 ought to become GICv%d, on the other hand this is really
the GICv3 driver driving a v4 in v3 "mode", so maybe v3 is the best
logging.

Re: [PATCH 2/2] xen/arm: gic-v3: Allow Xen to run on hardware reporting GICv4

[Andre Przywara]

Hi,

On 15/09/15 10:49, Ian Campbell wrote:
> On Mon, 2015-09-14 at 16:32 +0100, Julien Grall wrote:
>> It seems that there is some hardware which report start to report GICv4
> 
> s/report start to reports/reports/ ?
> 
> Also, this is an odd way to express it, what you mean is that some hardware
> is now shipping with GICv4. Unless you are trying to imply that they are
> claiming to be GICv4 without actually being so?
> 
> (If we agree on some wording I can modify this text on commit, subject to
> the discussion below).
> 
>> in the GIC*_PIDR2 register.
>>
>> As GICv4 is a superset of GICv3, it should just work on Xen.
>>
>> Reported-by: Andre Przywara <andre.przywara@arm.com>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>> ---
>>  xen/arch/arm/gic-v3.c             | 4 ++--
>>  xen/include/asm-arm/gic_v3_defs.h | 1 +
>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 4d623bf..1e3c19b 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -640,7 +640,7 @@ static int __init gicv3_populate_rdist(void)
>>          void __iomem *ptr = gicv3.rdist_regions[i].map_base;
>>  
>>          reg = readl_relaxed(ptr + GICR_PIDR2) & GIC_PIDR2_ARCH_MASK;
>> -        if ( reg != GIC_PIDR2_ARCH_GICv3 )
>> +        if ( reg != GIC_PIDR2_ARCH_GICv3 && reg != GIC_PIDR2_ARCH_GICv4 )
> 
> Once we have GICv5, 6, etc this is going to get unwieldy, shall we switch
> to a switch now?

The current GICv3 documentation is actually subtitled:
"GIC architecture version 3.0 and version 4.0", so I'd view them both
together. We can care about GICv >4 once this appears, but for now I'd
just stick with that simple if extension.
I just stumbled upon this because the Linux driver compares against v3
and v4 with a very similar statement (and-ed compares in if).

> 
>>          {
>>              dprintk(XENLOG_ERR,
>>                      "GICv3: No redistributor present @%"PRIpaddr"\n",
> 
> I wonder if GICv3 ought to become GICv%d, on the other hand this is really
> the GICv3 driver driving a v4 in v3 "mode", so maybe v3 is the best
> logging.

I'd keep v3 as well, as this part is really a GICv3 property.
Maybe we should quote the spec in the commit message:
"Because GICv4 is an extension of GICv3, all references to GICv3 in this
manual apply equally to GICv4."

Cheers,
Andre.

Re: [PATCH 2/2] xen/arm: gic-v3: Allow Xen to run on hardware reporting GICv4

[Julien Grall]

On 15/09/15 10:49, Ian Campbell wrote:
> On Mon, 2015-09-14 at 16:32 +0100, Julien Grall wrote:
>> It seems that there is some hardware which report start to report GICv4
> 
> s/report start to reports/reports/ ?

Yes, reports.

> Also, this is an odd way to express it, what you mean is that some hardware
> is now shipping with GICv4. Unless you are trying to imply that they are
> claiming to be GICv4 without actually being so?

GICv4 is an extension of GICv3 and should support any GICv3 feature.

It's not like GICv2 vs GICv3 where the GICv2 part is an optional feature
in the GICv3.

> (If we agree on some wording I can modify this text on commit, subject to
> the discussion below).


What about:

"The GICv4 is an extension of GICv3 (see 1.1 in ARM IHI 0069A) which
means that the GICv3 driver can run normally on a GICv4 hardware.

Though, the GICv4-only feature won't be used."

>> in the GIC*_PIDR2 register.
>>
>> As GICv4 is a superset of GICv3, it should just work on Xen.
>>
>> Reported-by: Andre Przywara <andre.przywara@arm.com>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>> ---
>>  xen/arch/arm/gic-v3.c             | 4 ++--
>>  xen/include/asm-arm/gic_v3_defs.h | 1 +
>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 4d623bf..1e3c19b 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -640,7 +640,7 @@ static int __init gicv3_populate_rdist(void)
>>          void __iomem *ptr = gicv3.rdist_regions[i].map_base;
>>  
>>          reg = readl_relaxed(ptr + GICR_PIDR2) & GIC_PIDR2_ARCH_MASK;
>> -        if ( reg != GIC_PIDR2_ARCH_GICv3 )
>> +        if ( reg != GIC_PIDR2_ARCH_GICv3 && reg != GIC_PIDR2_ARCH_GICv4 )
> 
> Once we have GICv5, 6, etc this is going to get unwieldy, shall we switch
> to a switch now?

We don't have a spec for GICv5 and v6 right now. I would wait to see if
the driver can be re-used before doing a such change.

>>          {
>>              dprintk(XENLOG_ERR,
>>                      "GICv3: No redistributor present @%"PRIpaddr"\n",
> 
> I wonder if GICv3 ought to become GICv%d, on the other hand this is really
> the GICv3 driver driving a v4 in v3 "mode", so maybe v3 is the best
> logging.

We are logging the driver name and not the version. I'd like to keep
like that because it's simple and easy to grep.

Regards,

-- 
Julien Grall

Re: [PATCH 2/2] xen/arm: gic-v3: Allow Xen to run on hardware reporting GICv4

[Ian Campbell]

On Tue, 2015-09-15 at 12:02 +0100, Julien Grall wrote:
> What about:
> 
> "The GICv4 is an extension of GICv3 (see 1.1 in ARM IHI 0069A) which
> means that the GICv3 driver can run normally on a GICv4 hardware.
> 
> Though, the GICv4-only feature won't be used."

I've a few grammatical quibbles, plus I think s/reporting/supporting/ in
the title.

So how about:

Subject: xen/arm: gic-v3: Allow Xen to run on hardware supporting GICv4

GICv4 is an extension of GICv3 (see 1.1 in ARM IHI 0069A) which means that
the GICv3 driver can run normally on GICv4 hardware.

The GICv4-only features currently won't be used.

S-o-b: etc

Re: [PATCH 2/2] xen/arm: gic-v3: Allow Xen to run on hardware reporting GICv4

[Julien Grall]

On 15/09/15 12:15, Ian Campbell wrote:
> On Tue, 2015-09-15 at 12:02 +0100, Julien Grall wrote:
>> What about:
>>
>> "The GICv4 is an extension of GICv3 (see 1.1 in ARM IHI 0069A) which
>> means that the GICv3 driver can run normally on a GICv4 hardware.
>>
>> Though, the GICv4-only feature won't be used."
> 
> I've a few grammatical quibbles, plus I think s/reporting/supporting/ in
> the title.
> 
> So how about:
> 
> Subject: xen/arm: gic-v3: Allow Xen to run on hardware supporting GICv4
> 
> GICv4 is an extension of GICv3 (see 1.1 in ARM IHI 0069A) which means that
> the GICv3 driver can run normally on GICv4 hardware.
> 
> The GICv4-only features currently won't be used.
> 
> S-o-b: etc

I'm fine with this commit message.

Regards,

-- 
Julien Grall

Re: [PATCH 2/2] xen/arm: gic-v3: Allow Xen to run on hardware reporting GICv4

[Ian Campbell]

On Tue, 2015-09-15 at 12:19 +0100, Julien Grall wrote:
> On 15/09/15 12:15, Ian Campbell wrote:
> > On Tue, 2015-09-15 at 12:02 +0100, Julien Grall wrote:
> > > What about:
> > > 
> > > "The GICv4 is an extension of GICv3 (see 1.1 in ARM IHI 0069A) which
> > > means that the GICv3 driver can run normally on a GICv4 hardware.
> > > 
> > > Though, the GICv4-only feature won't be used."
> > 
> > I've a few grammatical quibbles, plus I think s/reporting/supporting/
> > in
> > the title.
> > 
> > So how about:
> > 
> > Subject: xen/arm: gic-v3: Allow Xen to run on hardware supporting GICv4
> > 
> > GICv4 is an extension of GICv3 (see 1.1 in ARM IHI 0069A) which means
> > that
> > the GICv3 driver can run normally on GICv4 hardware.
> > 
> > The GICv4-only features currently won't be used.
> > 
> > S-o-b: etc
> 
> I'm fine with this commit message.

Applied both to staging, thanks.

Re: [PATCH 0/2] GICv3: Clean up + enable support on GICv4

[Julien Grall]

Hi Ian,

I think this series is all acked. Would it be possible to merge it in
unstable?

Regards,

On 14/09/15 16:32, Julien Grall wrote:
> Hi all,
> 
> This small patch series allow Xen to run on platform reporting GICv4
> in the GIC*_PIDR2.
> 
> Sincerely yours,
> 
> Julien Grall (2):
>   xen/arm: gic-v3: Clean-up the GIC*_PIDR2_* definitions
>   xen/arm: gic-v3: Allow Xen to run on hardware reporting GICv4
> 
>  xen/arch/arm/gic-v3.c             |  8 ++++----
>  xen/include/asm-arm/gic_v3_defs.h | 13 +++++--------
>  2 files changed, 9 insertions(+), 12 deletions(-)
> 


-- 
Julien Grall