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

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

On Fri, 2015-06-05 at 17:35 +0100, Julien Grall wrote:

> >>   */
> >>  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?

I don't really like read/modify/write idiom in the restore path, I'd
prefer a straight save/restore model (I know we have some R/M/W
already).

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

Lets see ;-)