Re: [Xen-devel] [PATCH v2 08/45] ARM: GIC: extend LR read/write functions to cover EOI and source

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 15 Mar 2018, Andre Przywara wrote:
> From: Julien Grall <julien.grall@xxxxxxx>
> 
> So far our LR read/write functions do not handle the EOI bit and the
> source CPUID bits in an LR, because the current VGIC implementation does
> not use them.
> Extend the gic_lr data structure to hold these bits of information by
> using a union to differentiate field used depending on whether the vIRQ
> has a corresponding pIRQ.
> 
> This allows the new VGIC to use this information.
> 
> This is based on the original patch sent by Andre Przywara [1].
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2018-03/msg00435.html
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>

Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

> ---
> Changes:
> - Reset authorship
> - Rework source
> - Add source support on GICv3
> 
>  xen/arch/arm/gic-v2.c             | 26 +++++++++++++++++++++++---
>  xen/arch/arm/gic-v3.c             | 34 +++++++++++++++++++++++++++++++---
>  xen/include/asm-arm/gic.h         | 16 ++++++++++++++--
>  xen/include/asm-arm/gic_v3_defs.h |  2 ++
>  4 files changed, 70 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 2f012692e0..7dfe6fc68d 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -474,8 +474,17 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
>  
>      if ( lr_reg->hw_status )
>      {
> -        lr_reg->pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
> -        lr_reg->pirq &= GICH_V2_LR_PHYSICAL_MASK;
> +        lr_reg->hw.pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
> +        lr_reg->hw.pirq &= GICH_V2_LR_PHYSICAL_MASK;
> +    }
> +    else
> +    {
> +        lr_reg->virt.eoi = (lrv & GICH_V2_LR_MAINTENANCE_IRQ);
> +        if ( lr_reg->virq < NR_GIC_SGI )
> +        {
> +            lr_reg->virt.source = (lrv >> GICH_V2_LR_CPUID_SHIFT)
> +                & GICH_V2_LR_CPUID_MASK;
> +        }
>      }
>  }
>  
> @@ -496,7 +505,18 @@ static void gicv2_write_lr(int lr, const struct gic_lr 
> *lr_reg)
>      if ( lr_reg->hw_status )
>      {
>          lrv |= GICH_V2_LR_HW;
> -        lrv |= lr_reg->pirq << GICH_V2_LR_PHYSICAL_SHIFT;
> +        lrv |= lr_reg->hw.pirq << GICH_V2_LR_PHYSICAL_SHIFT;
> +    }
> +    else
> +    {
> +        if ( lr_reg->virt.eoi )
> +            lrv |= GICH_V2_LR_MAINTENANCE_IRQ;
> +        /*
> +         * This is only valid for SGI, but it does not matter to always
> +         * read it as it should be 0 by default.
> +         */
> +        ASSERT(!lr_reg->virt.source || lr_reg->virq < NR_GIC_SGI);
> +        lrv |= (uint32_t)lr_reg->virt.source << GICH_V2_LR_CPUID_SHIFT;
>      }
>  
>      writel_gich(lrv, GICH_LR + lr * 4);
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index e901210b78..392cf91b58 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1014,12 +1014,25 @@ static void gicv3_read_lr(int lr, struct gic_lr 
> *lr_reg)
>      lr_reg->hw_status = lrv & ICH_LR_HW;
>  
>      if ( lr_reg->hw_status )
> -        lr_reg->pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
> +        lr_reg->hw.pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & 
> ICH_LR_PHYSICAL_MASK;
> +    else
> +    {
> +        lr_reg->virt.eoi = (lrv & ICH_LR_MAINTENANCE_IRQ);
> +        /* Source only exists for SGI and in GICv2 compatible mode */
> +        if ( lr_reg->virq < NR_GIC_SGI &&
> +             current->domain->arch.vgic.version == GIC_V2 )
> +        {
> +            lr_reg->virt.source = (lrv >> ICH_LR_CPUID_SHIFT)
> +                & ICH_LR_CPUID_MASK;
> +        }
> +    }
>  }
>  
>  static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
>  {
>      uint64_t lrv = 0;
> +    const enum gic_version vgic_version = current->domain->arch.vgic.version;
> +
>  
>      lrv = ( ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK)  << ICH_LR_VIRTUAL_SHIFT) 
> |
>          ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << 
> ICH_LR_PRIORITY_SHIFT) );
> @@ -1033,14 +1046,29 @@ static void gicv3_write_lr(int lr_reg, const struct 
> gic_lr *lr)
>      if ( lr->hw_status )
>      {
>          lrv |= ICH_LR_HW;
> -        lrv |= (uint64_t)lr->pirq << ICH_LR_PHYSICAL_SHIFT;
> +        lrv |= (uint64_t)lr->hw.pirq << ICH_LR_PHYSICAL_SHIFT;
> +    }
> +    else
> +    {
> +        if ( lr->virt.eoi )
> +            lrv |= ICH_LR_MAINTENANCE_IRQ;
> +        /* Source is only set in GICv2 compatible mode */
> +        if ( vgic_version == GIC_V2 )
> +        {
> +            /*
> +             * This is only valid for SGI, but it does not matter to always
> +             * read it as it should be 0 by default.
> +             */
> +            ASSERT(!lr->virt.source || lr->virq < NR_GIC_SGI);
> +            lrv |= (uint64_t)lr->virt.source << ICH_LR_CPUID_SHIFT;
> +        }
>      }
>  
>      /*
>       * When the guest is using vGICv3, all the IRQs are Group 1. Group 0
>       * would result in a FIQ, which will not be expected by the guest OS.
>       */
> -    if ( current->domain->arch.vgic.version == GIC_V3 )
> +    if ( vgic_version == GIC_V3 )
>          lrv |= ICH_LR_GRP1;
>  
>      gicv3_ich_write_lr(lr_reg, lrv);
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 545901b120..565b0875ca 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -204,14 +204,26 @@ union gic_state_data {
>   * The LR register format is different for GIC HW version
>   */
>  struct gic_lr {
> -   /* Physical IRQ -> Only set when hw_status is set. */
> -   uint32_t pirq;
>     /* Virtual IRQ */
>     uint32_t virq;
>     uint8_t priority;
>     bool active;
>     bool pending;
>     bool hw_status;
> +   union
> +   {
> +       /* Only filled when there are a corresponding pIRQ (hw_state = true) 
> */
> +       struct
> +       {
> +           uint32_t pirq;
> +       } hw;
> +       /* Only filled when there are no corresponding pIRQ (hw_state = 
> false) */
> +       struct
> +       {
> +           bool eoi;
> +           uint8_t source;      /* GICv2 only */
> +       } virt;
> +   };
>  };
>  
>  enum gic_version {
> diff --git a/xen/include/asm-arm/gic_v3_defs.h 
> b/xen/include/asm-arm/gic_v3_defs.h
> index d9827bd84c..10a2aeea93 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -167,6 +167,8 @@
>  
>  #define ICH_LR_VIRTUAL_MASK          0xffff
>  #define ICH_LR_VIRTUAL_SHIFT         0
> +#define ICH_LR_CPUID_MASK            0x7
> +#define ICH_LR_CPUID_SHIFT           10
>  #define ICH_LR_PHYSICAL_MASK         0x3ff
>  #define ICH_LR_PHYSICAL_SHIFT        32
>  #define ICH_LR_STATE_MASK            0x3
> -- 
> 2.14.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel