Re: [PATCH v3 04/13] xen: arm: add a quirk to handle platforms with unusual GIC layout

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

On 11/25/2013 12:38 PM, Ian Campbell wrote:
> On Mon, 2013-11-25 at 12:36 +0000, Ian Campbell wrote:
>> On Mon, 2013-11-25 at 12:34 +0000, Julien Grall wrote:
>>>> +    if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
>>>> +        ret = map_mmio_regions(d, d->arch.vgic.cbase + PAGE_SIZE,
>>>> +                               d->arch.vgic.cbase + (2 * PAGE_SIZE) - 1,
>>>> +                               gic.vbase + PAGE_SIZE);
>>>> +    else
>>>> +        ret = map_mmio_regions(d, d->arch.vgic.cbase + PAGE_SIZE,
>>>> +                               d->arch.vgic.cbase + (2 * PAGE_SIZE) - 1,
>>>> +                               gic.vbase + 16*PAGE_SIZE);
>>>
>>> The condition needs to be inverted here ... the CPU second page is at
>>> 64K only on platform where the quirk is enabled.
>>
>> Doh! Shows how much use Linux makes of the secnd page of registers (i.e.
>> none!)
>
> Here is what would be in v4:
>
> 8>------------
>
>  From 5221157d3b9986cdfede8b1299f35569e6e29670 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Thu, 21 Nov 2013 15:55:37 +0000
> Subject: [PATCH] xen: arm: add a quirk to handle platforms with unusual GIC
>   layout
>
> On some platforms the pages are placed at a 64K stride instead of as
> contiguous 4K pages.
>
> This is because the ARM64 architecture allows for page sizes of 4/16/64K in
> the MMU so a larger stride allow more granular control of mappings. We only
> currently support 4K.
>
> Use this quirk on the xgene platform.
>
> This should ideally be fixed by an extension to the device tree bindings as
> described in http://www.spinics.net/lists/devicetree/msg10473.html especially
> http://www.spinics.net/lists/devicetree/msg10478.html. However for the time
> being a platform specific quirk will do.
>
> Note that we always map the GICV to the guest (including dom0) at a 4K stride
> length and this is reflected in the DTB passed to the guest.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

For V4:
Acked-by: Julien Grall <julien.grall@linaro.org>

> ---
> v4: FIx the condition when mapping the GIC to the guest.
> v3: Always map the GICV to the guest as contiguous 4k pages.
> v2: This replaces "xen: arm: GICC_DIR register at offset 0x10000 instead of 0x1000"
> ---
>   xen/arch/arm/gic.c                   |   37 ++++++++++++++++++++++++++++------
>   xen/arch/arm/platforms/xgene-storm.c |    2 +-
>   xen/include/asm-arm/platform.h       |    5 +++++
>   3 files changed, 37 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index ab49106..0084f50 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -30,6 +30,7 @@
>   #include <xen/device_tree.h>
>   #include <asm/p2m.h>
>   #include <asm/domain.h>
> +#include <asm/platform.h>
>
>   #include <asm/gic.h>
>
> @@ -444,7 +445,10 @@ void __init gic_init(void)
>       BUILD_BUG_ON(FIXMAP_ADDR(FIXMAP_GICC1) !=
>                    FIXMAP_ADDR(FIXMAP_GICC2)-PAGE_SIZE);
>       set_fixmap(FIXMAP_GICC1, gic.cbase >> PAGE_SHIFT, DEV_SHARED);
> -    set_fixmap(FIXMAP_GICC2, (gic.cbase >> PAGE_SHIFT) + 1, DEV_SHARED);
> +    if ( platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
> +        set_fixmap(FIXMAP_GICC2, (gic.cbase >> PAGE_SHIFT) + 0x10, DEV_SHARED);
> +    else
> +        set_fixmap(FIXMAP_GICC2, (gic.cbase >> PAGE_SHIFT) + 0x1, DEV_SHARED);
>       set_fixmap(FIXMAP_GICH, gic.hbase >> PAGE_SHIFT, DEV_SHARED);
>
>       /* Global settings: interrupt distributor */
> @@ -823,6 +827,8 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq)
>
>   int gicv_setup(struct domain *d)
>   {
> +    int ret;
> +
>       /*
>        * Domain 0 gets the hardware address.
>        * Guests get the virtual platform layout.
> @@ -840,11 +846,30 @@ int gicv_setup(struct domain *d)
>
>       d->arch.vgic.nr_lines = 0;
>
> -    /* map the gic virtual cpu interface in the gic cpu interface region of
> -     * the guest */
> -    return map_mmio_regions(d, d->arch.vgic.cbase,
> -                            d->arch.vgic.cbase + (2 * PAGE_SIZE) - 1,
> -                            gic.vbase);
> +    /*
> +     * 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, d->arch.vgic.cbase,
> +                           d->arch.vgic.cbase + PAGE_SIZE - 1,
> +                           gic.vbase);
> +    if (ret)
> +        return ret;
> +
> +    if ( !platform_has_quirk(PLATFORM_QUIRK_GIC_64K_STRIDE) )
> +        ret = map_mmio_regions(d, d->arch.vgic.cbase + PAGE_SIZE,
> +                               d->arch.vgic.cbase + (2 * PAGE_SIZE) - 1,
> +                               gic.vbase + PAGE_SIZE);
> +    else
> +        ret = map_mmio_regions(d, d->arch.vgic.cbase + PAGE_SIZE,
> +                               d->arch.vgic.cbase + (2 * PAGE_SIZE) - 1,
> +                               gic.vbase + 16*PAGE_SIZE);
> +
> +    return ret;
> +
>   }
>
>   static void gic_irq_eoi(void *info)
> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
> index 0198cec..23ec46d 100644
> --- a/xen/arch/arm/platforms/xgene-storm.c
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -23,7 +23,7 @@
>
>   static uint32_t xgene_storm_quirks(void)
>   {
> -    return PLATFORM_QUIRK_DOM0_MAPPING_11;
> +    return PLATFORM_QUIRK_DOM0_MAPPING_11|PLATFORM_QUIRK_GIC_64K_STRIDE;
>   }
>
>
> diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
> index c282b30..c9314e5 100644
> --- a/xen/include/asm-arm/platform.h
> +++ b/xen/include/asm-arm/platform.h
> @@ -44,6 +44,11 @@ struct platform_desc {
>    * Useful on platform where System MMU is not yet implemented
>    */
>   #define PLATFORM_QUIRK_DOM0_MAPPING_11 (1 << 0)
> +/*
> + * Quirk for platforms where the 4K GIC register ranges are placed at
> + * 64K stride.
> + */
> +#define PLATFORM_QUIRK_GIC_64K_STRIDE (1 << 1)
>
>   void __init platform_init(void);
>   int __init platform_init_time(void);
>

-- 
Julien Grall