[Xen-changelog] [xen master] xen/arm: gic: Read unconditionally the source from the LRs

[patchbot@xxxxxxx]

commit bde2870e7da1896b36d5117d307a8ac2f07ae276
Author:     Julien Grall <julien.grall@xxxxxxx>
AuthorDate: Wed Mar 21 03:34:35 2018 +0000
Commit:     Stefano Stabellini <sstabellini@xxxxxxxxxx>
CommitDate: Wed Mar 21 09:33:10 2018 -0700

    xen/arm: gic: Read unconditionally the source from the LRs
    
    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>
    Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>
---
 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;
--
generated by git-patchbot for /home/xen/git/xen.git#master

_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/xen-changelog