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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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)

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

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

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

Despite all the comments I don't think there are any blockers:

Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>



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