Re: [Xen-devel] [PATCH] xen/arm: gic: Read unconditionally the source from the LRs

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 21 Mar 2018, Julien Grall wrote:
> Commit 5cb00d1 "ARM: GIC: extend LR read/write functions to cover EOI
> and source" extended gic_lr to cover the source. The new field was only
> set for SGIs interrupt in the read function. However, the write function
> is writing the field unconditionally for virtual interrupt.
> 
> This means that if the caller was combining the 2 functions (e.g to
> update the LR), the source need to be set to 0 by the caller.
> Unfortunately, gic_update_one_lr is not zeroing the structure before
> reading the LRs. This will lead to trigger the assert randomly.
> 
> Instead of zeroing the structure in gic_update_one_lr, make sure that
> the source is written unconditionally on read. This is also simplifying
> the code to avoid an if statement in the read path.
> 
> Lastly, properly update the comments in write_lr that was mistakenly
> speaking about the read lr path.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

and committed

> ---
>  xen/arch/arm/gic-v2.c | 15 ++++++++-------
>  xen/arch/arm/gic-v3.c | 13 ++++++++-----
>  2 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 7dfe6fc68d..aa0fc6c1a1 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -480,11 +480,12 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
>      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;
> -        }
> +        /*
> +         * This is only valid for SGI, but it does not matter to always
> +         * read it as it should be 0 by default.
> +         */
> +        lr_reg->virt.source = (lrv >> GICH_V2_LR_CPUID_SHIFT)
> +            & GICH_V2_LR_CPUID_MASK;
>      }
>  }
>  
> @@ -512,8 +513,8 @@ static void gicv2_write_lr(int lr, const struct gic_lr 
> *lr_reg)
>          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.
> +         * Source is only valid for SGIs, the caller should make sure
> +         * the field virt.source is always 0 for non-SGI.
>           */
>          ASSERT(!lr_reg->virt.source || lr_reg->virq < NR_GIC_SGI);
>          lrv |= (uint32_t)lr_reg->virt.source << GICH_V2_LR_CPUID_SHIFT;
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 392cf91b58..cb41844af2 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1018,10 +1018,13 @@ static void gicv3_read_lr(int lr, struct gic_lr 
> *lr_reg)
>      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 )
> +        /* Source only exists in GICv2 compatible mode */
> +        if ( current->domain->arch.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.
> +             */
>              lr_reg->virt.source = (lrv >> ICH_LR_CPUID_SHIFT)
>                  & ICH_LR_CPUID_MASK;
>          }
> @@ -1056,8 +1059,8 @@ static void gicv3_write_lr(int lr_reg, const struct 
> gic_lr *lr)
>          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.
> +             * Source is only valid for SGIs, the caller should make
> +             * sure the field virt.source is always 0 for non-SGI.
>               */
>              ASSERT(!lr->virt.source || lr->virq < NR_GIC_SGI);
>              lrv |= (uint64_t)lr->virt.source << ICH_LR_CPUID_SHIFT;
> -- 
> 2.11.0
> 

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