Re: [PATCH 2/2] xen/arm: Fix PLATFORM_QUIRK_GIC_64K_STRIDE

[Julien Grall <julien.grall@linaro.org>]

Hi Ian,

On 07/09/2014 12:54 PM, Ian Campbell wrote:
> The patch "xen/arm: use ioremap to map gic-v2 registers" handled this
> quirk by mapping a 64K+1 page region, but the code continued to
> assume that GICC_DIR was at offset 0x1000 from the start of the
> mapping.

I should have reviewed closer this patch, good catch!

> Fix this by mapping both pages of the GICC space independently and
> adjusting the accessor function to use the correct page.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> Cc: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> Cc: Anup Patel <anup.patel@linaro.org>
> ---
>  xen/arch/arm/gic-v2.c |   17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 3820be5..6cee883 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -65,7 +65,7 @@ static struct {
>      paddr_t dbase;            /* Address of distributor registers */
>      void __iomem * map_dbase; /* IO mapped Address of distributor registers */
>      paddr_t cbase;            /* Address of CPU interface registers */
> -    void __iomem * map_cbase; /* IO mapped Address of CPU interface registers */
> +    void __iomem * map_cbase[2]; /* IO mapped Address of CPU interface registers */
>      paddr_t hbase;            /* Address of virtual interface registers */
>      void __iomem * map_hbase; /* IO Address of virtual interface registers */
>      paddr_t vbase;            /* Address of virtual cpu interface registers */
> @@ -100,12 +100,16 @@ static inline uint32_t readl_gicd(int offset)
>  
>  static inline void writel_gicc(uint32_t val, int offset)
>  {
> -    writel_relaxed(val, gicv2.map_cbase + offset);
> +    int page = offset >> PAGE_SHIFT;

unsigned int?

> +    offset &= ~PAGE_MASK;
> +    writel_relaxed(val, gicv2.map_cbase[page] + offset);
>  }
>  
>  static inline uint32_t readl_gicc(int offset)
>  {
> -    return readl_relaxed(gicv2.map_cbase + offset);
> +    int page = offset >> PAGE_SHIFT;

same here.

> +    offset &= ~PAGE_MASK;
> +    return readl_relaxed(gicv2.map_cbase[page] + offset);
>  }
>  
>  static inline void writel_gich(uint32_t val, int offset)
> @@ -665,10 +669,13 @@ static int __init gicv2_init(struct dt_device_node *node, const void *data)
>      if ( !gicv2.map_dbase )
>          panic("GICv2: Failed to ioremap for GIC distributor\n");
>  
> +    gicv2.map_cbase[0] = ioremap_nocache(gicv2.cbase, PAGE_SIZE);
> +
>      if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
> -        gicv2.map_cbase = ioremap_nocache(gicv2.cbase, PAGE_SIZE * 0x10);
> +        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + PAGE_SIZE * 0x10,
> +                                           PAGE_SIZE);
>      else
> -        gicv2.map_cbase = ioremap_nocache(gicv2.cbase, PAGE_SIZE * 2);
> +        gicv2.map_cbase[1] = ioremap_nocache(gicv2.cbase + PAGE_SIZE, PAGE_SIZE);
>  
>      if ( !gicv2.map_cbase )

This test is now wrong. You have to check both map_cbase[0] and
map_cbase[1].

>          panic("GICv2: Failed to ioremap for GIC CPU interface\n");

Regards,

-- 
Julien Grall