[PATCH v4 0/4] xen/arm: gic-v2: Detect automatically aliased GIC 400

[PATCH v4 0/4] xen/arm: gic-v2: Detect automatically aliased GIC 400

[Julien Grall]

Hi all,

Only patch #3 is related to the subject of the cover letter. The rest is clean
up of code I looked while I was working on this series. Though, patch #1 is a
new bug fix I noticed between v3 and v4.

Sincerely yours,

Julien Grall (4):
  xen/arm: vgic-v2: Report the correct GICC size to the guest
  xen/arm: gic: Check the size of the CPU and vCPU interface retrieved
    from DT
  xen/arm: gic-v2: Automatically detect aliased GIC400
  xen/arm: platform: Drop the quirks callback

 xen/arch/arm/gic-hip04.c             | 12 ++---
 xen/arch/arm/gic-v2.c                | 90 ++++++++++++++++++++++++++++--------
 xen/arch/arm/gic-v3.c                | 23 +++++++--
 xen/arch/arm/platform.c              | 10 ----
 xen/arch/arm/platforms/xgene-storm.c |  6 ---
 xen/arch/arm/vgic-v2.c               | 49 ++++++++++++--------
 xen/include/asm-arm/gic.h            |  1 +
 xen/include/asm-arm/platform.h       | 14 ------
 xen/include/asm-arm/vgic.h           |  3 +-
 xen/include/public/arch-arm.h        |  2 +-
 10 files changed, 128 insertions(+), 82 deletions(-)

-- 
2.1.4

[PATCH v4 1/4] xen/arm: vgic-v2: Report the correct GICC size to the guest

[Julien Grall]

The GICv2 DT node is usually used by the guest to know the address/size
of the regions (GICD, GICC...) to map into their virtual memory.

While the GICv2 spec requires the size of the GICC to be 8KB, we
correctly do an 8KB stage-2 mapping but errornously report 256 in the
device tree (based on GUEST_GICC_SIZE).

I bet we didn't see any issue so far because all the registers except
GICC_DIR lives in the first 256 bytes of the GICC region and all the guest
I have seen so far are driving the GIC with GICC_CTLR.EIOmode =
0.

Signed-off-by: Julien Grall <julien.grall@citrix.com>

---
    This patch is a good candidate to backport for Xen 4.6 - 4.4.
    Without it a guest relying on the DT can't use GICC_DIR.

    Changes in v4:
        - Patch added
---
 xen/include/public/arch-arm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 9a96401..3c365f3 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -397,7 +397,7 @@ typedef uint64_t xen_callback_t;
 #define GUEST_GICD_BASE   0x03001000ULL
 #define GUEST_GICD_SIZE   0x00001000ULL
 #define GUEST_GICC_BASE   0x03002000ULL
-#define GUEST_GICC_SIZE   0x00000100ULL
+#define GUEST_GICC_SIZE   0x00002000ULL
 
 /* vGIC v3 mappings */
 #define GUEST_GICV3_GICD_BASE      0x03001000ULL
-- 
2.1.4

Re: [PATCH v4 1/4] xen/arm: vgic-v2: Report the correct GICC size to the guest

[Ian Campbell]

On Thu, 2015-10-08 at 19:23 +0100, Julien Grall wrote:
> The GICv2 DT node is usually used by the guest to know the address/size
> of the regions (GICD, GICC...) to map into their virtual memory.
> 
> While the GICv2 spec requires the size of the GICC to be 8KB, we
> correctly do an 8KB stage-2 mapping but errornously report 256 in the
> device tree (based on GUEST_GICC_SIZE).

"erroneously"

> 
> I bet we didn't see any issue so far because all the registers except
> GICC_DIR lives in the first 256 bytes of the GICC region and all the guest

"guests"

> I have seen so far are driving the GIC with GICC_CTLR.EIOmode =
> 0.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

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

(typo's fixable on commit).

> ---
>     This patch is a good candidate to backport for Xen 4.6 - 4.4.
>     Without it a guest relying on the DT can't use GICC_DIR.

Noted, but just to check: This patch (and none of the other fixes in this
series) are all which are required for a guest to be able to use GICC_DIR,
right?

Re: [PATCH v4 1/4] xen/arm: vgic-v2: Report the correct GICC size to the guest

[Julien Grall]

Hi Ian,

On 23/10/15 14:28, Ian Campbell wrote:
> On Thu, 2015-10-08 at 19:23 +0100, Julien Grall wrote:
>> The GICv2 DT node is usually used by the guest to know the address/size
>> of the regions (GICD, GICC...) to map into their virtual memory.
>>
>> While the GICv2 spec requires the size of the GICC to be 8KB, we
>> correctly do an 8KB stage-2 mapping but errornously report 256 in the
>> device tree (based on GUEST_GICC_SIZE).
> 
> "erroneously"
> 
>>
>> I bet we didn't see any issue so far because all the registers except
>> GICC_DIR lives in the first 256 bytes of the GICC region and all the guest
> 
> "guests"
> 
>> I have seen so far are driving the GIC with GICC_CTLR.EIOmode =
>> 0.
>>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> (typo's fixable on commit).

Thank you!

>> ---
>>     This patch is a good candidate to backport for Xen 4.6 - 4.4.
>>     Without it a guest relying on the DT can't use GICC_DIR.
> 
> Noted, but just to check: This patch (and none of the other fixes in this
> series) are all which are required for a guest to be able to use GICC_DIR,
> right?

Correct. BTW, I forgot to mention that this patch may not apply cleanly
on Xen 4.4 as rearranged the guest memory address space in Xen 4.5.

Regards,

-- 
Julien Grall

Re: [PATCH v4 1/4] xen/arm: vgic-v2: Report the correct GICC size to the guest

[Ian Campbell]

On Fri, 2015-10-23 at 14:34 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 23/10/15 14:28, Ian Campbell wrote:
> > On Thu, 2015-10-08 at 19:23 +0100, Julien Grall wrote:
> > > The GICv2 DT node is usually used by the guest to know the
> > > address/size
> > > of the regions (GICD, GICC...) to map into their virtual memory.
> > > 
> > > While the GICv2 spec requires the size of the GICC to be 8KB, we
> > > correctly do an 8KB stage-2 mapping but errornously report 256 in the
> > > device tree (based on GUEST_GICC_SIZE).
> > 
> > "erroneously"
> > 
> > > 
> > > I bet we didn't see any issue so far because all the registers except
> > > GICC_DIR lives in the first 256 bytes of the GICC region and all the
> > > guest
> > 
> > "guests"
> > 
> > > I have seen so far are driving the GIC with GICC_CTLR.EIOmode =
> > > 0.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@citrix.com>
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > (typo's fixable on commit).
> 
> Thank you!
> 
> > > ---
> > >     This patch is a good candidate to backport for Xen 4.6 - 4.4.
> > >     Without it a guest relying on the DT can't use GICC_DIR.
> > 
> > Noted, but just to check: This patch (and none of the other fixes in
> > this
> > series) are all which are required for a guest to be able to use
> > GICC_DIR,
> > right?
> 
> Correct. BTW, I forgot to mention that this patch may not apply cleanly
> on Xen 4.4 as rearranged the guest memory address space in Xen 4.5.

I'd be inclined not to bother with it for 4.4 at this juncture then.

Ian.

Re: [PATCH v4 1/4] xen/arm: vgic-v2: Report the correct GICC size to the guest

[Julien Grall]

On 23/10/15 14:39, Ian Campbell wrote:
> On Fri, 2015-10-23 at 14:34 +0100, Julien Grall wrote:
>> Hi Ian,
>>
>> On 23/10/15 14:28, Ian Campbell wrote:
>>> On Thu, 2015-10-08 at 19:23 +0100, Julien Grall wrote:
>>>> The GICv2 DT node is usually used by the guest to know the
>>>> address/size
>>>> of the regions (GICD, GICC...) to map into their virtual memory.
>>>>
>>>> While the GICv2 spec requires the size of the GICC to be 8KB, we
>>>> correctly do an 8KB stage-2 mapping but errornously report 256 in the
>>>> device tree (based on GUEST_GICC_SIZE).
>>>
>>> "erroneously"
>>>
>>>>
>>>> I bet we didn't see any issue so far because all the registers except
>>>> GICC_DIR lives in the first 256 bytes of the GICC region and all the
>>>> guest
>>>
>>> "guests"
>>>
>>>> I have seen so far are driving the GIC with GICC_CTLR.EIOmode =
>>>> 0.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>>>
>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>>
>>> (typo's fixable on commit).
>>
>> Thank you!
>>
>>>> ---
>>>>     This patch is a good candidate to backport for Xen 4.6 - 4.4.
>>>>     Without it a guest relying on the DT can't use GICC_DIR.
>>>
>>> Noted, but just to check: This patch (and none of the other fixes in
>>> this
>>> series) are all which are required for a guest to be able to use
>>> GICC_DIR,
>>> right?
>>
>> Correct. BTW, I forgot to mention that this patch may not apply cleanly
>> on Xen 4.4 as rearranged the guest memory address space in Xen 4.5.
> 
> I'd be inclined not to bother with it for 4.4 at this juncture then.

I'm fine with that.

Regards,

-- 
Julien Grall

[PATCH v4 2/4] xen/arm: gic: Check the size of the CPU and vCPU interface retrieved from DT

[Julien Grall]

The size of the CPU interface will used in a follow-up patch to map the
region in Xen memory.

Based on GICv2 spec, the CPU interface should at least be 8KB, although
most of the platform we are supporting use incorrectly the GICv1 size
(i.e 4KB) in their DT. Only warn and update the size to avoid any
breakage on these platforms.

Furthermore, Xen is relying on the fact that the Virtual CPU interface
been at least 8KB. As in reality the Virtual CPU interface matches the CPU
interface, check that the 2 interfaces have the same size.

For GICv3, vGICv2 is only available for guest. So we only need to check
that the GICV is at least 8KB.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
Cc: Zoltan Kiss <zoltan.kiss@huawei.com>

I haven't done any change in the gic-hip04 driver. I will let the
maintainers doing it if they feel it's necessary.

The check on the CPU interface size has not been modified because if we
really want to check the validity of it, we would have to check at least
SZ_4K, SZ_8K and SZ_128K. Although I'm not sure if there is other
possible values.

    Changes in v4:
        - Only check the GICV size is at least 8KB to support GICv2 on
        GICv3
        - Panic if the sizes of GICC and GICV are different when
        initializing the GICv2 driver
        - Update warning in gicv2_init
        - Update commit message

    Changes in v3:
        - Shorten the error messages
        - Typo

    Changes in v2:
        - Fix typoes in the commit message
        - Fix typoes in warning messages
        - Avoid to split string literally in the middle of the sentence, so
        grep still works.
        - csize should be checked against vsize and not vbase!
---
 xen/arch/arm/gic-v2.c | 31 +++++++++++++++++++++++++++----
 xen/arch/arm/gic-v3.c | 19 +++++++++++++++++--
 2 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 5841e59..48155cb 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -617,14 +617,16 @@ static hw_irq_controller gicv2_guest_irq_type = {
 static int __init gicv2_init(void)
 {
     int res;
-    paddr_t hbase, dbase, cbase, vbase;
+    paddr_t hbase, dbase;
+    paddr_t cbase, csize;
+    paddr_t vbase, vsize;
     const struct dt_device_node *node = gicv2_info.node;
 
     res = dt_device_get_address(node, 0, &dbase, NULL);
     if ( res )
         panic("GICv2: Cannot find a valid address for the distributor");
 
-    res = dt_device_get_address(node, 1, &cbase, NULL);
+    res = dt_device_get_address(node, 1, &cbase, &csize);
     if ( res )
         panic("GICv2: Cannot find a valid address for the CPU");
 
@@ -632,7 +634,7 @@ static int __init gicv2_init(void)
     if ( res )
         panic("GICv2: Cannot find a valid address for the hypervisor");
 
-    res = dt_device_get_address(node, 3, &vbase, NULL);
+    res = dt_device_get_address(node, 3, &vbase, &vsize);
     if ( res )
         panic("GICv2: Cannot find a valid address for the virtual CPU");
 
@@ -641,7 +643,28 @@ static int __init gicv2_init(void)
         panic("GICv2: Cannot find the maintenance IRQ");
     gicv2_info.maintenance_irq = res;
 
-    /* TODO: Add check on distributor, cpu size */
+    /* TODO: Add check on distributor */
+
+    /*
+     * The GICv2 CPU interface should at least be 8KB. Although, most of the DT
+     * doesn't correctly set it and use the GICv1 CPU interface size (i.e 4KB).
+     * Warn and then fixup.
+     */
+    if ( csize < SZ_8K )
+    {
+        printk(XENLOG_WARNING "GICv2: WARNING: "
+               "The GICC size is too small: %#"PRIx64" expected %#x\n",
+               csize, SZ_8K);
+        csize = SZ_8K;
+    }
+
+    /*
+     * Check if the CPU interface and virtual CPU interface have the
+     * same size.
+     */
+    if ( csize != vsize )
+        panic("GICv2: Sizes of GICC (%#"PRIpaddr") and GICV (%#"PRIpaddr") don't match\n",
+               csize, vsize);
 
     printk("GICv2 initialization:\n"
               "        gic_dist_addr=%"PRIpaddr"\n"
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 957c6e0..fe0654f 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1143,7 +1143,8 @@ static void __init gicv3_init_v2(const struct dt_device_node *node,
                                  paddr_t dbase)
 {
     int res;
-    paddr_t cbase, vbase;
+    paddr_t cbase;
+    paddr_t vbase, vsize;
 
     /*
      * For GICv3 supporting GICv2, GICC and GICV base address will be
@@ -1155,10 +1156,24 @@ static void __init gicv3_init_v2(const struct dt_device_node *node,
         return;
 
     res = dt_device_get_address(node, 1 + gicv3.rdist_count + 2,
-                                &vbase, NULL);
+                                &vbase, &vsize);
     if ( res )
         return;
 
+    /*
+     * We emulate a vGICv2 using a GIC CPU interface of GUEST_GICC_SIZE.
+     * So only support GICv2 on GICv3 when the virtuaL CPU interface is
+     * at least GUEST_GICC_SIZE.
+     */
+    if ( vsize < GUEST_GICC_SIZE )
+    {
+        printk(XENLOG_WARNING
+               "GICv3: WARNING: Not enabling support for GICv2 compat mode.\n"
+               "Size of GICV (%#"PRIpaddr") must at least be %#llx.\n",
+               vsize, GUEST_GICC_SIZE);
+        return;
+    }
+
     printk("GICv3 compatible with GICv2 cbase %#"PRIpaddr" vbase %#"PRIpaddr"\n",
            cbase, vbase);
 
-- 
2.1.4

Re: [PATCH v4 2/4] xen/arm: gic: Check the size of the CPU and vCPU interface retrieved from DT

[Ian Campbell]

On Thu, 2015-10-08 at 19:23 +0100, Julien Grall wrote:
> The size of the CPU interface will used in a follow-up patch to map the

                                    ^be

> region in Xen memory.
> 
> Based on GICv2 spec, the CPU interface should at least be 8KB, although
> most of the platform we are supporting use incorrectly the GICv1 size
> (i.e 4KB) in their DT. Only warn and update the size to avoid any
> breakage on these platforms.
> 
> Furthermore, Xen is relying on the fact that the Virtual CPU interface
> been at least 8KB. As in reality the Virtual CPU interface matches the


  s/been/is/

> +    /* TODO: Add check on distributor */
> +
> +    /*
> +     * The GICv2 CPU interface should at least be 8KB. Although, most of the DT
> +     * doesn't correctly set it and use the GICv1 CPU interface size (i.e 4KB).

           don't

> +                                &vbase, &vsize);
>      if ( res )
>          return;
>  
> +    /*
> +     * We emulate a vGICv2 using a GIC CPU interface of GUEST_GICC_SIZE.
> +     * So only support GICv2 on GICv3 when the virtuaL CPU interface is

s/L/l/

Apart from the typos:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH v4 3/4] xen/arm: gic-v2: Automatically detect aliased GIC400

[Julien Grall]

We are currently using a per-platform quirk to know if the 2 4KB region of
the GIC CPU interface are each aligned to 64KB. Although, it may be
possible to have different layout on a same platform (depending on the
firmware version).

Rather than having a quirk it's possible to detect by reading the GIC
memory. This patch is based from the Linux commit "irqchip/GIC: Add workaround
for aliased GIC400" [1].

Take the opportunity to clean up the GICv2 of code which was only
required because of the quirk.

Note that none of the platform using the gic-hip04 were actually using
the quirk, so the code has been dropped. I will let the maintainers
decide whether it's relevant or not to add proper detection for aliased
GIC for this hardware.

[1] commit 12e14066f4835f5ee1ca795f0309415b54c067a9
    Author: Marc Zyngier <marc.zyngier@arm.com>
    Date:   Sun Sep 13 12:14:31 2015 +0100

    irqchip/GIC: Add workaround for aliased GIC400

    The GICv2 architecture mandates that the two 4kB GIC regions are
    contiguous, and on two separate physical pages (so that access to
    the second page can be trapped by a hypervisor). This doesn't work
    very well when PAGE_SIZE is 64kB.

    A relatively common hack^Wway to work around this is to alias each
    4kB region over its own 64kB page. Of course in this case, the base
    address you want to use is not really the begining of the region,
    but base + 60kB (so that you get a contiguous 8kB region over two
    distinct pages).

    Normally, this would be described in DT with a new property, but
    some HW is already out there, and the firmware makes sure that
    it will override whatever you put in the GIC node. Duh. And of course,
    said firmware source code is not available, despite being based
    on u-boot.

    The workaround is to detect the case where the CPU interface size
    is set to 128kB, and verify the aliasing by checking that the ID
    register for GIC400 (which is the only GIC wired this way so far)
    is the same at base and base + 0xF000. In this case, we update
    the GIC base address and let it roll.

    And if you feel slightly sick by looking at this, rest assured that
    I do too...

    Reported-by: Julien Grall <julien.grall@citrix.com>
    Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
    Cc: linux-arm-kernel@lists.infradead.org
    Cc: Stuart Yoder <stuart.yoder@freescale.com>
    Cc: Pavel Fedin <p.fedin@samsung.com>
    Cc: Jason Cooper <jason@lakedaemon.net>
    Link: http://lkml.kernel.org/r/1442142873-20213-2-git-send-email-marc.zyngier@arm.com
    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Signed-off-by: Julien Grall <julien.grall@citrix.com>

---
Cc: Zoltan Kiss <zoltan.kiss@huawei.com>

    Changes in v4:
        - Initialize aliased_offset to 0 when GICv2 is not aliased. I'm
        not sure why GCC didn't spot this uninitiliazed value...
        - Use GUEST_GICC_SIZE rather than 8KB but ensure it's always
        8KB.
        - With those changes remove Ian's acked-by

    Changes in v3:
        - Add Ian's acked-by

    Changes in v2:
        - Drop XXX for gic-v3 and gic-hip04 as won't support aliased GIC
        - Fix typoes
---
 xen/arch/arm/gic-hip04.c             | 12 +++-----
 xen/arch/arm/gic-v2.c                | 59 ++++++++++++++++++++++++++----------
 xen/arch/arm/gic-v3.c                |  6 ++--
 xen/arch/arm/platforms/xgene-storm.c |  6 ----
 xen/arch/arm/vgic-v2.c               | 49 +++++++++++++++++++-----------
 xen/include/asm-arm/gic.h            |  1 +
 xen/include/asm-arm/platform.h       |  6 ----
 xen/include/asm-arm/vgic.h           |  3 +-
 8 files changed, 84 insertions(+), 58 deletions(-)

diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
index e8cdcd4..310f35a 100644
--- a/xen/arch/arm/gic-hip04.c
+++ b/xen/arch/arm/gic-hip04.c
@@ -631,14 +631,14 @@ static hw_irq_controller hip04gic_guest_irq_type = {
 static int __init hip04gic_init(void)
 {
     int res;
-    paddr_t hbase, dbase, cbase, vbase;
+    paddr_t hbase, dbase, cbase, csize, vbase;
     const struct dt_device_node *node = gicv2_info.node;
 
     res = dt_device_get_address(node, 0, &dbase, NULL);
     if ( res )
         panic("GIC-HIP04: Cannot find a valid address for the distributor");
 
-    res = dt_device_get_address(node, 1, &cbase, NULL);
+    res = dt_device_get_address(node, 1, &cbase, &csize);
     if ( res )
         panic("GIC-HIP04: Cannot find a valid address for the CPU");
 
@@ -675,11 +675,7 @@ static int __init hip04gic_init(void)
         panic("GIC-HIP04: Failed to ioremap for GIC distributor\n");
 
     gicv2.map_cbase[0] = ioremap_nocache(cbase, PAGE_SIZE);
-
-    if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
-        gicv2.map_cbase[1] = ioremap_nocache(cbase + SZ_64K, PAGE_SIZE);
-    else
-        gicv2.map_cbase[1] = ioremap_nocache(cbase + PAGE_SIZE, PAGE_SIZE);
+    gicv2.map_cbase[1] = ioremap_nocache(cbase + PAGE_SIZE, PAGE_SIZE);
 
     if ( !gicv2.map_cbase[0] || !gicv2.map_cbase[1] )
         panic("GIC-HIP04: Failed to ioremap for GIC CPU interface\n");
@@ -688,7 +684,7 @@ static int __init hip04gic_init(void)
     if ( !gicv2.map_hbase )
         panic("GIC-HIP04: Failed to ioremap for GIC Virtual interface\n");
 
-    vgic_v2_setup_hw(dbase, cbase, vbase);
+    vgic_v2_setup_hw(dbase, cbase, csize, vbase, 0);
 
     /* Global settings: interrupt distributor */
     spin_lock_init(&gicv2.lock);
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 48155cb..850d08e 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -65,7 +65,7 @@
 /* Global state */
 static struct {
     void __iomem * map_dbase; /* IO mapped Address of distributor registers */
-    void __iomem * map_cbase[2]; /* IO mapped Address of CPU interface registers */
+    void __iomem * map_cbase; /* IO mapped Address of CPU interface registers */
     void __iomem * map_hbase; /* IO Address of virtual interface registers */
     spinlock_t lock;
 } gicv2;
@@ -98,16 +98,12 @@ static inline uint32_t readl_gicd(unsigned int offset)
 
 static inline void writel_gicc(uint32_t val, unsigned int offset)
 {
-    unsigned int page = offset >> PAGE_SHIFT;
-    offset &= ~PAGE_MASK;
-    writel_relaxed(val, gicv2.map_cbase[page] + offset);
+    writel_relaxed(val, gicv2.map_cbase + offset);
 }
 
 static inline uint32_t readl_gicc(unsigned int offset)
 {
-    unsigned int page = offset >> PAGE_SHIFT;
-    offset &= ~PAGE_MASK;
-    return readl_relaxed(gicv2.map_cbase[page] + offset);
+    return readl_relaxed(gicv2.map_cbase + offset);
 }
 
 static inline void writel_gich(uint32_t val, unsigned int offset)
@@ -614,12 +610,31 @@ static hw_irq_controller gicv2_guest_irq_type = {
     .set_affinity = gicv2_irq_set_affinity,
 };
 
+static bool_t gicv2_is_aliased(paddr_t cbase, paddr_t csize)
+{
+    uint32_t val_low, val_high;
+
+    if ( csize != SZ_128K )
+        return false;
+
+    /*
+     * Verify that we have the first 4kB of a GIC400
+     * aliased over the first 64kB by checking the
+     * GICC_IIDR register on both ends.
+     */
+    val_low = readl_gicc(GICC_IIDR);
+    val_high = readl_gicc(GICC_IIDR + 0xf000);
+
+    return ((val_low & 0xfff0fff) == 0x0202043B && val_low == val_high);
+}
+
 static int __init gicv2_init(void)
 {
     int res;
     paddr_t hbase, dbase;
     paddr_t cbase, csize;
     paddr_t vbase, vsize;
+    uint32_t aliased_offset = 0;
     const struct dt_device_node *node = gicv2_info.node;
 
     res = dt_device_get_address(node, 0, &dbase, NULL);
@@ -683,21 +698,33 @@ static int __init gicv2_init(void)
     if ( !gicv2.map_dbase )
         panic("GICv2: Failed to ioremap for GIC distributor\n");
 
-    gicv2.map_cbase[0] = ioremap_nocache(cbase, PAGE_SIZE);
-
-    if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
-        gicv2.map_cbase[1] = ioremap_nocache(cbase + SZ_64K, PAGE_SIZE);
-    else
-        gicv2.map_cbase[1] = ioremap_nocache(cbase + PAGE_SIZE, PAGE_SIZE);
-
-    if ( !gicv2.map_cbase[0] || !gicv2.map_cbase[1] )
+    gicv2.map_cbase = ioremap_nocache(cbase, csize);
+    if ( !gicv2.map_cbase )
         panic("GICv2: Failed to ioremap for GIC CPU interface\n");
 
+    if ( gicv2_is_aliased(cbase, csize) )
+    {
+        /*
+         * Move the base up by 60kB, so that we have a 8kB contiguous
+         * region, which allows us to use GICC_DIR at its
+         * normal offset.
+         * Note the variable cbase is not updated as we need the original
+         * value for the vGICv2 emulation.
+         */
+        aliased_offset = 0xf000;
+
+        gicv2.map_cbase += aliased_offset;
+
+        printk(XENLOG_WARNING
+               "GICv2: Adjusting CPU interface base to %#"PRIx64"\n",
+               cbase + aliased_offset);
+    }
+
     gicv2.map_hbase = ioremap_nocache(hbase, PAGE_SIZE);
     if ( !gicv2.map_hbase )
         panic("GICv2: Failed to ioremap for GIC Virtual interface\n");
 
-    vgic_v2_setup_hw(dbase, cbase, vbase);
+    vgic_v2_setup_hw(dbase, cbase, csize, vbase, aliased_offset);
 
     /* Global settings: interrupt distributor */
     spin_lock_init(&gicv2.lock);
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index fe0654f..396d134 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1143,7 +1143,7 @@ static void __init gicv3_init_v2(const struct dt_device_node *node,
                                  paddr_t dbase)
 {
     int res;
-    paddr_t cbase;
+    paddr_t cbase, csize;
     paddr_t vbase, vsize;
 
     /*
@@ -1151,7 +1151,7 @@ static void __init gicv3_init_v2(const struct dt_device_node *node,
      * provided.
      */
     res = dt_device_get_address(node, 1 + gicv3.rdist_count,
-                                &cbase, NULL);
+                                &cbase, &csize);
     if ( res )
         return;
 
@@ -1177,7 +1177,7 @@ static void __init gicv3_init_v2(const struct dt_device_node *node,
     printk("GICv3 compatible with GICv2 cbase %#"PRIpaddr" vbase %#"PRIpaddr"\n",
            cbase, vbase);
 
-    vgic_v2_setup_hw(dbase, cbase, vbase);
+    vgic_v2_setup_hw(dbase, cbase, csize, vbase, 0);
 }
 
 /* Set up the GIC */
diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index 8b05ed5..70cb655 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -68,11 +68,6 @@ static void __init xgene_check_pirq_eoi(void)
               "Please upgrade your firmware to the latest version");
 }
 
-static uint32_t xgene_storm_quirks(void)
-{
-    return PLATFORM_QUIRK_GIC_64K_STRIDE;
-}
-
 static void xgene_storm_reset(void)
 {
     void __iomem *addr;
@@ -122,7 +117,6 @@ PLATFORM_START(xgene_storm, "APM X-GENE STORM")
     .compatible = xgene_storm_dt_compat,
     .init = xgene_storm_init,
     .reset = xgene_storm_reset,
-    .quirks = xgene_storm_quirks,
 PLATFORM_END
 
 /*
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 2d63e12..f7d784b 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -36,18 +36,25 @@ static struct {
     bool_t enabled;
     /* Distributor interface address */
     paddr_t dbase;
-    /* CPU interface address */
+    /* CPU interface address & size */
     paddr_t cbase;
+    paddr_t csize;
     /* Virtual CPU interface address */
     paddr_t vbase;
+
+    /* Offset to add to get an 8kB contiguous region if GIC is aliased */
+    uint32_t aliased_offset;
 } vgic_v2_hw;
 
-void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t vbase)
+void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
+                      paddr_t vbase, uint32_t aliased_offset)
 {
     vgic_v2_hw.enabled = 1;
     vgic_v2_hw.dbase = dbase;
     vgic_v2_hw.cbase = cbase;
+    vgic_v2_hw.csize = csize;
     vgic_v2_hw.vbase = vbase;
+    vgic_v2_hw.aliased_offset = aliased_offset;
 }
 
 static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
@@ -524,7 +531,8 @@ static int vgic_v2_vcpu_init(struct vcpu *v)
 static int vgic_v2_domain_init(struct domain *d)
 {
     int i, ret;
-    paddr_t cbase;
+    paddr_t cbase, csize;
+    paddr_t vbase;
 
     /*
      * The hardware domain gets the hardware address.
@@ -533,33 +541,38 @@ static int vgic_v2_domain_init(struct domain *d)
     if ( is_hardware_domain(d) )
     {
         d->arch.vgic.dbase = vgic_v2_hw.dbase;
+        /*
+         * For the hardware domain, we always map the whole HW CPU
+         * interface region in order to match the device tree (the "reg"
+         * properties is copied as it is).
+         * Note that we assume the size of the CPU interface is always
+         * aligned to PAGE_SIZE.
+         */
         cbase = vgic_v2_hw.cbase;
+        csize = vgic_v2_hw.csize;
+        vbase = vgic_v2_hw.vbase;
     }
     else
     {
         d->arch.vgic.dbase = GUEST_GICD_BASE;
+        /*
+         * The CPU interface exposed to the guest is always 8kB. We may
+         * need to add an offset to the virtual CPU interface base
+         * address when in the GIC is aliased to get a 8kB contiguous
+         * region.
+         */
+        BUILD_BUG_ON(GUEST_GICC_SIZE != SZ_8K);
         cbase = GUEST_GICC_BASE;
+        csize = GUEST_GICC_SIZE;
+        vbase = vgic_v2_hw.vbase + vgic_v2_hw.aliased_offset;
     }
 
     /*
      * Map the gic virtual cpu interface in the gic cpu interface
      * region of the guest.
-     *
-     * The second page is always mapped at +4K irrespective of the
-     * GIC_64K_STRIDE quirk. The DTB passed to the guest reflects this.
      */
-    ret = map_mmio_regions(d, paddr_to_pfn(cbase), 1,
-                           paddr_to_pfn(vgic_v2_hw.vbase));
-    if ( ret )
-        return ret;
-
-    if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
-        ret = map_mmio_regions(d, paddr_to_pfn(cbase + PAGE_SIZE),
-                               1, paddr_to_pfn(vgic_v2_hw.vbase + PAGE_SIZE));
-    else
-        ret = map_mmio_regions(d, paddr_to_pfn(cbase + PAGE_SIZE),
-                               1, paddr_to_pfn(vgic_v2_hw.vbase + SZ_64K));
-
+    ret = map_mmio_regions(d, paddr_to_pfn(cbase), csize / PAGE_SIZE,
+                           paddr_to_pfn(vbase));
     if ( ret )
         return ret;
 
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 6d53f97..0116481 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -76,6 +76,7 @@
 #define GICC_HPPIR      (0x0018)
 #define GICC_APR        (0x00D0)
 #define GICC_NSAPR      (0x00E0)
+#define GICC_IIDR       (0x00FC)
 #define GICC_DIR        (0x1000)
 
 #define GICH_HCR        (0x00)
diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
index b8fc5ac..5e462ac 100644
--- a/xen/include/asm-arm/platform.h
+++ b/xen/include/asm-arm/platform.h
@@ -39,12 +39,6 @@ struct platform_desc {
     const struct dt_device_match *blacklist_dev;
 };
 
-/*
- * Quirk for platforms where the 4K GIC register ranges are placed at
- * 64K stride.
- */
-#define PLATFORM_QUIRK_GIC_64K_STRIDE (1 << 0)
-
 void __init platform_init(void);
 int __init platform_init_time(void);
 int __init platform_specific_mapping(struct domain *d);
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index ba74d0f..cb51a9e 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -248,7 +248,8 @@ static inline int vgic_allocate_spi(struct domain *d)
 
 extern void vgic_free_virq(struct domain *d, unsigned int virq);
 
-void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t vbase);
+void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
+                      paddr_t vbase, uint32_t aliased_offset);
 
 #ifdef HAS_GICV3
 struct rdist_region;
-- 
2.1.4

Re: [PATCH v4 3/4] xen/arm: gic-v2: Automatically detect aliased GIC400

[Ian Campbell]

On Thu, 2015-10-08 at 19:23 +0100, Julien Grall wrote:
> We are currently using a per-platform quirk to know if the 2 4KB region
> of
> the GIC CPU interface are each aligned to 64KB. Although, it may be
> possible to have different layout on a same platform (depending on the
> firmware version).
> 
> Rather than having a quirk it's possible to detect by reading the GIC
> memory. This patch is based from the Linux commit "irqchip/GIC: Add
> workaround
> for aliased GIC400" [1].
> 
> Take the opportunity to clean up the GICv2 of code which was only
> required because of the quirk.
> 
> Note that none of the platform using the gic-hip04 were actually using
> the quirk, so the code has been dropped. I will let the maintainers
> decide whether it's relevant or not to add proper detection for aliased
> GIC for this hardware.
> 
> [1] commit 12e14066f4835f5ee1ca795f0309415b54c067a9
>     Author: Marc Zyngier <marc.zyngier@arm.com>
>     Date:   Sun Sep 13 12:14:31 2015 +0100
> 
>     irqchip/GIC: Add workaround for aliased GIC400
> 
>     The GICv2 architecture mandates that the two 4kB GIC regions are
>     contiguous, and on two separate physical pages (so that access to
>     the second page can be trapped by a hypervisor). This doesn't work
>     very well when PAGE_SIZE is 64kB.
> 
>     A relatively common hack^Wway to work around this is to alias each
>     4kB region over its own 64kB page. Of course in this case, the base
>     address you want to use is not really the begining of the region,
>     but base + 60kB (so that you get a contiguous 8kB region over two
>     distinct pages).
> 
>     Normally, this would be described in DT with a new property, but
>     some HW is already out there, and the firmware makes sure that
>     it will override whatever you put in the GIC node. Duh. And of
> course,
>     said firmware source code is not available, despite being based
>     on u-boot.
> 
>     The workaround is to detect the case where the CPU interface size
>     is set to 128kB, and verify the aliasing by checking that the ID
>     register for GIC400 (which is the only GIC wired this way so far)
>     is the same at base and base + 0xF000. In this case, we update
>     the GIC base address and let it roll.
> 
>     And if you feel slightly sick by looking at this, rest assured that
>     I do too...
> 
>     Reported-by: Julien Grall <julien.grall@citrix.com>
>     Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>     Cc: linux-arm-kernel@lists.infradead.org
>     Cc: Stuart Yoder <stuart.yoder@freescale.com>
>     Cc: Pavel Fedin <p.fedin@samsung.com>
>     Cc: Jason Cooper <jason@lakedaemon.net>
>     Link: http://lkml.kernel.org/r/1442142873-20213-2-git-send-email-marc
> .zyngier@arm.com
>     Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

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

[PATCH v4 4/4] xen/arm: platform: Drop the quirks callback

[Julien Grall]

All the quirks has been replaced by proper detection. Lets drop the
callback and hope that no one will need new quirks.

At the same time, remove the definition platform_dom0_evtchn_ppi with is
not used any more.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
    Changes in v2:
        - Add Ian's acked-by
---
 xen/arch/arm/platform.c        | 10 ----------
 xen/include/asm-arm/platform.h |  8 --------
 2 files changed, 18 deletions(-)

diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
index 0af6d57..b0bfaa9 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -127,16 +127,6 @@ void platform_poweroff(void)
         platform->poweroff();
 }
 
-bool_t platform_has_quirk(uint32_t quirk)
-{
-    uint32_t quirks = 0;
-
-    if ( platform && platform->quirks )
-        quirks = platform->quirks();
-
-    return !!(quirks & quirk);
-}
-
 bool_t platform_device_is_blacklisted(const struct dt_device_node *node)
 {
     const struct dt_device_match *blacklist = NULL;
diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
index 5e462ac..f97315d 100644
--- a/xen/include/asm-arm/platform.h
+++ b/xen/include/asm-arm/platform.h
@@ -27,12 +27,6 @@ struct platform_desc {
     /* Platform power-off */
     void (*poweroff)(void);
     /*
-     * Platform quirks
-     * Defined has a function because a platform can support multiple
-     * board with different quirk on each
-     */
-    uint32_t (*quirks)(void);
-    /*
      * Platform blacklist devices
      * List of devices which must not pass-through to a guest
      */
@@ -48,9 +42,7 @@ int platform_cpu_up(int cpu);
 #endif
 void platform_reset(void);
 void platform_poweroff(void);
-bool_t platform_has_quirk(uint32_t quirk);
 bool_t platform_device_is_blacklisted(const struct dt_device_node *node);
-unsigned int platform_dom0_evtchn_ppi(void);
 
 #define PLATFORM_START(_name, _namestr)                         \
 static const struct platform_desc  __plat_desc_##_name __used   \
-- 
2.1.4

Re: [PATCH v4 4/4] xen/arm: platform: Drop the quirks callback

[Ian Campbell]

On Thu, 2015-10-08 at 19:23 +0100, Julien Grall wrote:
> All the quirks has been replaced by proper detection. Lets drop the

                 have

> callback and hope that no one will need new quirks.
> 
> At the same time, remove the definition platform_dom0_evtchn_ppi with is

                                         ^of                       which

> not used any more.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> ---
>     Changes in v2:
>         - Add Ian's acked-by
> ---
>  xen/arch/arm/platform.c        | 10 ----------
>  xen/include/asm-arm/platform.h |  8 --------
>  2 files changed, 18 deletions(-)
> 
> diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
> index 0af6d57..b0bfaa9 100644
> --- a/xen/arch/arm/platform.c
> +++ b/xen/arch/arm/platform.c
> @@ -127,16 +127,6 @@ void platform_poweroff(void)
>          platform->poweroff();
>  }
>  
> -bool_t platform_has_quirk(uint32_t quirk)
> -{
> -    uint32_t quirks = 0;
> -
> -    if ( platform && platform->quirks )
> -        quirks = platform->quirks();
> -
> -    return !!(quirks & quirk);
> -}
> -
>  bool_t platform_device_is_blacklisted(const struct dt_device_node *node)
>  {
>      const struct dt_device_match *blacklist = NULL;
> diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm
> -arm/platform.h
> index 5e462ac..f97315d 100644
> --- a/xen/include/asm-arm/platform.h
> +++ b/xen/include/asm-arm/platform.h
> @@ -27,12 +27,6 @@ struct platform_desc {
>      /* Platform power-off */
>      void (*poweroff)(void);
>      /*
> -     * Platform quirks
> -     * Defined has a function because a platform can support multiple
> -     * board with different quirk on each
> -     */
> -    uint32_t (*quirks)(void);
> -    /*
>       * Platform blacklist devices
>       * List of devices which must not pass-through to a guest
>       */
> @@ -48,9 +42,7 @@ int platform_cpu_up(int cpu);
>  #endif
>  void platform_reset(void);
>  void platform_poweroff(void);
> -bool_t platform_has_quirk(uint32_t quirk);
>  bool_t platform_device_is_blacklisted(const struct dt_device_node
> *node);
> -unsigned int platform_dom0_evtchn_ppi(void);
>  
>  #define PLATFORM_START(_name, _namestr)                         \
>  static const struct platform_desc  __plat_desc_##_name __used   \

Re: [PATCH v4 0/4] xen/arm: gic-v2: Detect automatically aliased GIC 400

[Ian Campbell]

On Thu, 2015-10-08 at 19:23 +0100, Julien Grall wrote:
> Hi all,
> 
> Only patch #3 is related to the subject of the cover letter. The rest is
> clean
> up of code I looked while I was working on this series. Though, patch #1
> is a
> new bug fix I noticed between v3 and v4.

I fixed the typos I mentioned in my review and have now committed all four.