All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] xen/arm: gic: Read unconditionally the source from the LRs
@ 2018-03-21  3:34 Julien Grall
  2018-03-21 16:28 ` Andre Przywara
  2018-03-21 16:36 ` Stefano Stabellini
  0 siblings, 2 replies; 3+ messages in thread
From: Julien Grall @ 2018-03-21  3:34 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, sstabellini, andre.przywara

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@arm.com>
---
 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] xen/arm: gic: Read unconditionally the source from the LRs
  2018-03-21  3:34 [PATCH] xen/arm: gic: Read unconditionally the source from the LRs Julien Grall
@ 2018-03-21 16:28 ` Andre Przywara
  2018-03-21 16:36 ` Stefano Stabellini
  1 sibling, 0 replies; 3+ messages in thread
From: Andre Przywara @ 2018-03-21 16:28 UTC (permalink / raw)
  To: Julien Grall, xen-devel; +Cc: sstabellini

Hi,

On 21/03/18 03:34, 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.

I could indeed reproduce this today, though it interestingly didn't fire
in my testing when sending out v2. Weird.

The patch looks fine to me, I actually included it in my new series.

> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> ---
>  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;
> 

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

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] xen/arm: gic: Read unconditionally the source from the LRs
  2018-03-21  3:34 [PATCH] xen/arm: gic: Read unconditionally the source from the LRs Julien Grall
  2018-03-21 16:28 ` Andre Przywara
@ 2018-03-21 16:36 ` Stefano Stabellini
  1 sibling, 0 replies; 3+ messages in thread
From: Stefano Stabellini @ 2018-03-21 16:36 UTC (permalink / raw)
  To: Julien Grall; +Cc: sstabellini, andre.przywara, xen-devel

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

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-03-21 16:36 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-21  3:34 [PATCH] xen/arm: gic: Read unconditionally the source from the LRs Julien Grall
2018-03-21 16:28 ` Andre Przywara
2018-03-21 16:36 ` Stefano Stabellini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.