Re: [RFC 22/22] xen/arm: gic-v3: Add support of vGICv2 when available

[Julien Grall <julien.grall.oss@gmail.com>]

On 05/06/15 13:48, Ian Campbell wrote:
> On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
>> * Modify the GICv3 driver to recognize a such device. I wasn't able
>>   to find a register which tell if GICv2 is supported on GICv3. The only
>>   way to find it seems to check if the DT node provides GICC and GICV.
> 
> I think that's the way...
> 
>> * Disable access to ICC_SRE_EL1 to guest using vGICv2
>>
>> * The LR is slightly different for vGICv2. The interrupt is always
>> injected with group0.
>>
>> * Add a comment explaining why Group1 is used for vGICv3.
>>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>> ---
>>  xen/arch/arm/gic-v3.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 53 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 329d6ca..8533ae5 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -237,15 +237,14 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
>>  }
>>  
>>  /*
>> - * System Register Enable (SRE). Enable to access CPU & Virtual
>> - * interface registers as system registers in EL2
>> + * System Register Enable (SRE).
> 
> What was wrong with the comment (apart from the grammar). It was
> incomplete but I think you are removing the code which wasn't described,
> while removing the comment which describes the remaining behaviour.

I read EL1 instead of EL2 :/.

>>   */
>>  static void gicv3_enable_sre(void)
>>  {
>>      uint32_t val;
>>  
>>      val = READ_SYSREG32(ICC_SRE_EL2);
>> -    val |= GICC_SRE_EL2_SRE | GICC_SRE_EL2_ENEL1;
>> +    val |= GICC_SRE_EL2_SRE;
>>  
>>      WRITE_SYSREG32(val, ICC_SRE_EL2);
>>      isb();
>> @@ -373,6 +372,20 @@ static void gicv3_save_state(struct vcpu *v)
>>  
>>  static void gicv3_restore_state(const struct vcpu *v)
>>  {
>> +    uint32_t val;
>> +
>> +    val = READ_SYSREG32(ICC_SRE_EL2);
>> +    /*
>> +     * Don't give access to system registers when the guest is using
>> +     * GICv2
>> +     */
>> +    if ( v->domain->arch.vgic.version == GIC_V2 )
>> +        val &= ~GICC_SRE_EL2_ENEL1;
>> +    else
>> +        val |= GICC_SRE_EL2_ENEL1;
>> +    WRITE_SYSREG32(val, ICC_SRE_EL2);
> 
> Perhaps save/restore v->arch.gic.v3.sre_el2 rather than reading and
> recalculating each time? Then you just need to set sre_el2 appropriately
> during domain init.

Hmmm that would mean to reintroduce gicv_setup for setting sre_el2.

What is your concern here?

> 
>> @@ -1107,6 +1127,32 @@ static int __init cmp_rdist(const void *a, const void *b)
>>      return ( l->base < r->base) ? -1 : 0;
>>  }
>>  
>> +/* If the GICv3 supports GICv2, initialize it */
>> +static void __init gicv3_init_v2(const struct dt_device_node *node)
>> +{
>> +    int res;
>> +
>> +    /*
>> +     * For GICv3 supporting GICv2, GICC and GICV base address will be
>> +     * provided.
>> +     */
>> +
>> +    res = dt_device_get_address(node, 1 + gicv3_info.nr_rdist_regions,
>> +                                &gicv3_info.cbase, NULL);
>> +    if ( res )
>> +        return;
>> +
>> +    res = dt_device_get_address(node, 1 + gicv3_info.nr_rdist_regions + 2,
>> +                                &gicv3_info.vbase, NULL);
>> +    if ( res )
>> +        return;
>> +
>> +    printk("GICv3 compatible with GICv2 cbase %#"PRIpaddr" vbase %#"PRIpaddr"\n",
>> +           gicv3_info.cbase, gicv3_info.vbase);
>> +
>> +    gicv3_info.vgic_versions |= GIC_V2;
> 
> So I think this is a second type of compat, right? After this we
> support:
> 
> Guests using GICv2, via vgic-v2.c
> Guests using GICv3, via vgic-v3.c
> 
> But also:
> 
> Guests using GICv2, via vgic-v3.c, i.e. vgicv3 in compat mode.
> 
> Is that right? Is it intended?

No, we don't support the last one. This variable is used to know which
callback set we have to use.

It may be clearer with the suggested solution in patch #16.

Regards,

-- 
Julien Grall