Re: [Xen-devel] [PATCH v2 15/15] xen/arm: gic-v3: Add support of vGICv2 when available

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Ian,

On 30/06/15 14:16, Ian Campbell wrote:
> On Fri, 2015-06-26 at 10:34 +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.
>>
>> * 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@xxxxxxxxxx>
>>
>> ---
>>     I haven't address the request from Ian to not use the R/M/W idiom.
>>     Most of the usage of the idiom are for EL2 registers (i.e registers
>>     used by the hypervisor). They can be modified at any time by Xen,
>>     and not specific to the running domain. We would have to progate the
>>     change to each domain if we don't use the R/W/M.
>>
>>     ICC_SRE_EL2 falls under the same umbrella. It would be preferable to
>>     stay with the same model as today i.e:
>>         - *_EL1: straight save/restore
>>         - *_EL2: R/M/W to necessary field
> 
> I suppose I can live with this.
> 
>>
>>     Changes in v2:
>>         - Use vgic_v2_hw_setup to notify that GICv3 supports GICv2
>>         - Revert changes in comment
>> ---
>>  xen/arch/arm/gic-v3.c | 55 
>> ++++++++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 52 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 337fbb9..876a56b 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -247,7 +247,7 @@ 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();
>> @@ -375,6 +375,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
> 
> I suppose we can assume that v4, v5 etc will want to expose these (i.e.
> the condition needn't be inverted to be don't give when not using v3)

GICv3 and onwards are always using System Registers (i.e SRE = 1). On
platform which don't require to be backward compatible with GICv2, this
bit may be RAO/WI.

This is true for v4, but I don't know for v5 as I don't have the spec.
It may even be a completely different driver.

> 
>> +     */
>> +    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);
>> +    isb();
> 
> Is the isb strictly needed? I suppose we are already using rather too
> many, perhaps a more complete audit is in order.

AFAICT no, the ENEL1 doesn't gate any access to EL1 systems register in EL2.

There is an isb in the caller (gic_restore_state) but I find it
confusing because we rely on the caller doing the right thing for us.
I'm thinking to push the isb within the callee for more clarify.

It can be part of a bigger audit.

> 
>> +
>>      WRITE_SYSREG32(v->arch.gic.v3.sre_el1, ICC_SRE_EL1);
>>      WRITE_SYSREG32(v->arch.gic.v3.vmcr, ICH_VMCR_EL2);
>>      restore_aprn_regs(&v->arch.gic);
>> @@ -866,13 +880,20 @@ static void gicv3_disable_interface(void)
>>  static void gicv3_update_lr(int lr, const struct pending_irq *p,
>>                              unsigned int state)
>>  {
>> -    uint64_t grp = GICH_LR_GRP1;
>>      uint64_t val = 0;
>>  
>>      BUG_ON(lr >= gicv3_info.nr_lrs);
>>      BUG_ON(lr < 0);
>>  
>> -    val =  (((uint64_t)state & 0x3) << GICH_LR_STATE_SHIFT) | grp;
>> +    val =  (((uint64_t)state & 0x3) << GICH_LR_STATE_SHIFT);
>> +
>> +    /*
>> +     * When the guest is GICv3, all guest IRQs are Group 1, as Group0
>> +     * would result in a FIQ in the guest, which it wouldn't expect
> 
> Unless the guest actually asked for a FIQ (not sure how that works).
> Anyway, I don't think you need to support that case here...

Well, I don't even think that FIQ has ever working with guest and Xen.

AFAICT, supporting FIQ would require to have 2 groups (GRP0 and GRP1).
Although both our vGICv2 and vGICv3 driver only support a single group
(GRP0 for GICv2 and GRP1 for GICv3).

So I would expect to see more work than this small change.

> 
>> +     */
>> +    if ( current->domain->arch.vgic.version == GIC_V3 )
>> +        val |= GICH_LR_GRP1;
>> +
>>      val |= ((uint64_t)p->priority & 0xff) << GICH_LR_PRIORITY_SHIFT;
>>      val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << 
>> GICH_LR_VIRTUAL_SHIFT;
>>  
>> @@ -1119,6 +1140,33 @@ 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,
>> +                                 paddr_t dbase)
>> +{
>> +    int res;
>> +    paddr_t cbase, vbase;
>> +
>> +    /*
>> +     * For GICv3 supporting GICv2, GICC and GICV base address will be
>> +     * provided.
> 
> 
> I wonder how one specifies without GICC, given that GICH comes after
> it... Oh well!

GICv3 doesn't use the GICH to control the GIC hypervisor interface but
the system register.

> Despite all the comments I don't think there are any blockers:
> 
> Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

Thank you.

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel