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

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2015-09-22 at 18:47 +0100, Julien Grall wrote:
> 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 the GICv1 size (i.e 4KB) in

                                        ^incorrectly

(to avoid implying they actually have a GICv1)

> their DT. Only warn and update the size to avoid any breakage on these
> platforms.
> 
> Furthermore, Xen is relying on the Virtual CPU interface been at least

"relying on the fact that the..."

> 8KB. As in reality the Virtual CPU interface match the CPU interface,

"matches"

> check that the 2 interfaces have the same size. Also only warn, to avoid
> any breakage with buggy DT.
> 
> For GICv3, only allow GICv2 compatibility when the Virtual CPU interface
> and CPU interface are 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.

Speaking of which, wasn't their going to be a new comaintainer at one point
(mid.gname.org/<554B5C71.3020007@linaro.org>)

> @@ -641,7 +643,31 @@ 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 )

What do you think of checking for exactly SZ_4K (the only actually used
incorrect value) and still being picky about other sizes <8K?

> +    {
> +        printk(XENLOG_WARNING "GICv2: WARNING: "
> +               "The CPU interface size is wrong: %#"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 != vbase )
> +        printk(XENLOG_WARNING "GICv2: WARNING: "
> +               "The size of the CPU interface (%#"PRIpaddr") and the
> vCPU"
> +               " interface (%#"PRIpaddr") doesn't match\n",

"don't match"  or "do not match".

In general we should prefer not to split string literally in the middle of
sentences, so grep still works. I think we can tolerate the long lines in
such cases.

> +               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 957e491..4c58baf 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1143,22 +1143,38 @@ static void __init gicv3_init_v2(const struct
> dt_device_node *node,
>                                   paddr_t dbase)
>  {
>      int res;
> -    paddr_t cbase, vbase;
> +    paddr_t cbase, csize;
> +    paddr_t vbase, vsize;
>  
>      /*
>       * For GICv3 supporting GICv2, GICC and GICV base address will be
>       * provided.
>       */
>      res = dt_device_get_address(node, 1 + gicv3.rdist_count,
> -                                &cbase, NULL);
> +                                &cbase, &csize);
>      if ( res )
>          return;
>  
>      res = dt_device_get_address(node, 1 + gicv3.rdist_count + 2,
> -                                &vbase, NULL);
> +                                &vbase, &vsize);
>      if ( res )
>          return;
>  
> +    /*
> +     * Only allow support of GICv2 compatible when the CPU interface
> +     * and virtual CPU interface are 8KB
> +     * XXX: Handle other size?

Any size >= 8KB ought to be ok, no?

We could clamp to 8KB if we were worried about what is in the rest.

> +     */
> +    if ( csize != SZ_8K && vsize != SZ_8K )
> +    {
> +        printk(XENLOG_WARNING
> +               "GICv3: WARNING: Don't enable support of GICv2.\n"

"Not enabling support for GICv2 compat mode"

> +               "The size of the CPU interface (%#"PRIpaddr") and the
> vCPU"
> +               " interface (%#"PRIpaddr") should be 8KB.\n",

"should both be" ?

> +               csize, vsize);
> +        return;
> +    }
> +
>      printk("GICv3 compatible with GICv2 cbase %#"PRIpaddr" vbase
> %#"PRIpaddr"\n",
>             cbase, vbase);
>