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

[Julien Grall <julien.grall@citrix.com>]

Hi Ian,

On 25/09/15 17:19, Ian Campbell wrote:
> 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>)

I think so. I've CCed Shameerali who seems to be more responsive and
tested my previous changes about GIC-hip04.

> 
>> @@ -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?

If so, we should have to be picky for any value other than SZ_4K, SZ_8K,
SZ_128K.

Although I didn't do it because I wasn't not sure if there is DT out
with other possible value.

I've added this check because any size below 8K would result to a
non-obvious crash when Xen is trying to access GIC_DIR.

> 
>> +    {
>> +        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.

Good point. I will put the message one a single line.

>> +               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.

The GICv2 CPU interface is always 8KB. Having an higher value may mean
that the GIC is aliased.

GICv2 on GICv3 is only used for guest. I prefer to restrict the usage to
known and safe value until we have someone using different size.

This will avoid to expose unwanted data/value to a guest.

> 
>> +     */
>> +    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"

Will fix it.

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

Yes.

Regards,

-- 
Julien Grall