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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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

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

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



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