[PATCH 0/6] xen/arm: Rework the way to store the LR

[PATCH 0/6] xen/arm: Rework the way to store the LR

[julien.grall]

From: Julien Grall <julien.grall@arm.com>

Hi all,

This series is meant to replace patch #21 "ARM: GICv2: extend LR read/write
functions to cover EOI and source" from Andre's vGIC series (see [1]).

It has some more clean-up to address potential shortcomings with the interface.

The series is based on "ARM: vGIC: prepare for splitting the vGIC code" [2].

Cheers,

[1] https://lists.xenproject.org/archives/html/xen-devel/2018-03/msg00435.html
[2] https://lists.xenproject.org/archives/html/xen-devel/2018-03/msg00950.html

Andre Przywara (1):
  ARM: GIC: extend LR read/write functions to cover EOI and source

Julien Grall (5):
  xen/arm: gic: Fix indentation in gic_update_one_lr
  xen/arm: vgic: Override the group in lr everytime
  xen/arm: gic: Use bool instead of uint8_t for the hw_status in gic_lr
  xen/arm: gic: Split the field state in gic_lr in 2 fields active and
    pending
  xen/arm: GIC: Only set pirq in the LR when hw_status is set

 xen/arch/arm/gic-v2.c             | 53 ++++++++++++++++++++++++++++++---------
 xen/arch/arm/gic-v3.c             | 44 ++++++++++++++++++++++++--------
 xen/arch/arm/gic-vgic.c           |  8 +++---
 xen/include/asm-arm/gic.h         | 22 ++++++++++++----
 xen/include/asm-arm/gic_v3_defs.h |  2 ++
 5 files changed, 98 insertions(+), 31 deletions(-)

-- 
2.11.0


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

[PATCH 1/6] xen/arm: gic: Fix indentation in gic_update_one_lr

[julien.grall]

From: Julien Grall <julien.grall@arm.com>

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/gic-vgic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 61f093db50..e3cb47e80e 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -197,8 +197,8 @@ static void gic_update_one_lr(struct vcpu *v, int i)
         {
             if ( p->desc == NULL )
             {
-                 lr_val.state |= GICH_LR_PENDING;
-                 gic_hw_ops->write_lr(i, &lr_val);
+                lr_val.state |= GICH_LR_PENDING;
+                gic_hw_ops->write_lr(i, &lr_val);
             }
             else
                 gdprintk(XENLOG_WARNING, "unable to inject hw irq=%d into d%dv%d: already active in LR%d\n",
-- 
2.11.0


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

[PATCH 2/6] xen/arm: vgic: Override the group in lr everytime

[julien.grall]

From: Julien Grall <julien.grall@arm.com>

At the moment, write_lr is assuming the caller will set correctly the
group. However the group should always be 0 when the guest is using
vGICv2 and 1 for vGICv3. As the caller should not care about the group,
override it directly.

With that change, write_lr is now behaving like update_lr for the group.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/gic-v2.c     |  4 +---
 xen/arch/arm/gic-v3.c     | 11 ++++++++---
 xen/include/asm-arm/gic.h |  1 -
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index f16e17c1a3..fc105c08b8 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -469,7 +469,6 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
     lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
     lr_reg->state     = (lrv >> GICH_V2_LR_STATE_SHIFT) & GICH_V2_LR_STATE_MASK;
     lr_reg->hw_status = (lrv >> GICH_V2_LR_HW_SHIFT) & GICH_V2_LR_HW_MASK;
-    lr_reg->grp       = (lrv >> GICH_V2_LR_GRP_SHIFT) & GICH_V2_LR_GRP_MASK;
 }
 
 static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
@@ -483,8 +482,7 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
           ((uint32_t)(lr_reg->state & GICH_V2_LR_STATE_MASK)
                                    << GICH_V2_LR_STATE_SHIFT) |
           ((uint32_t)(lr_reg->hw_status & GICH_V2_LR_HW_MASK)
-                                       << GICH_V2_LR_HW_SHIFT)  |
-          ((uint32_t)(lr_reg->grp & GICH_V2_LR_GRP_MASK) << GICH_V2_LR_GRP_SHIFT) );
+                                       << GICH_V2_LR_HW_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 09b49a07d5..0dfa1a1e08 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1012,7 +1012,6 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
     lr_reg->priority  = (lrv >> ICH_LR_PRIORITY_SHIFT) & ICH_LR_PRIORITY_MASK;
     lr_reg->state     = (lrv >> ICH_LR_STATE_SHIFT) & ICH_LR_STATE_MASK;
     lr_reg->hw_status = (lrv >> ICH_LR_HW_SHIFT) & ICH_LR_HW_MASK;
-    lr_reg->grp       = (lrv >> ICH_LR_GRP_SHIFT) & ICH_LR_GRP_MASK;
 }
 
 static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
@@ -1023,8 +1022,14 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
         ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK)  << ICH_LR_VIRTUAL_SHIFT) |
         ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT)|
         ((u64)(lr->state & ICH_LR_STATE_MASK) << ICH_LR_STATE_SHIFT) |
-        ((u64)(lr->hw_status & ICH_LR_HW_MASK) << ICH_LR_HW_SHIFT)  |
-        ((u64)(lr->grp & ICH_LR_GRP_MASK) << ICH_LR_GRP_SHIFT) );
+        ((u64)(lr->hw_status & ICH_LR_HW_MASK) << ICH_LR_HW_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 )
+        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 49cb94f792..1eb08b856e 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -211,7 +211,6 @@ struct gic_lr {
    uint8_t priority;
    uint8_t state;
    uint8_t hw_status;
-   uint8_t grp;
 };
 
 enum gic_version {
-- 
2.11.0


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

[PATCH 3/6] xen/arm: gic: Use bool instead of uint8_t for the hw_status in gic_lr

[julien.grall]

From: Julien Grall <julien.grall@arm.com>

hw_status can only be 1 or 0. So convert to a bool.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/gic-v2.c     | 9 +++++----
 xen/arch/arm/gic-v3.c     | 8 +++++---
 xen/include/asm-arm/gic.h | 2 +-
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index fc105c08b8..23223575a2 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -468,7 +468,7 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
     lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & GICH_V2_LR_VIRTUAL_MASK;
     lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
     lr_reg->state     = (lrv >> GICH_V2_LR_STATE_SHIFT) & GICH_V2_LR_STATE_MASK;
-    lr_reg->hw_status = (lrv >> GICH_V2_LR_HW_SHIFT) & GICH_V2_LR_HW_MASK;
+    lr_reg->hw_status = (lrv & GICH_V2_LR_HW) == GICH_V2_LR_HW;
 }
 
 static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
@@ -480,9 +480,10 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
           ((uint32_t)(lr_reg->priority & GICH_V2_LR_PRIORITY_MASK)
                                       << GICH_V2_LR_PRIORITY_SHIFT) |
           ((uint32_t)(lr_reg->state & GICH_V2_LR_STATE_MASK)
-                                   << GICH_V2_LR_STATE_SHIFT) |
-          ((uint32_t)(lr_reg->hw_status & GICH_V2_LR_HW_MASK)
-                                       << GICH_V2_LR_HW_SHIFT));
+                                   << GICH_V2_LR_STATE_SHIFT) );
+
+    if ( lr_reg->hw_status )
+        lrv |= GICH_V2_LR_HW;
 
     writel_gich(lrv, GICH_LR + lr * 4);
 }
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 0dfa1a1e08..0711e509a6 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1011,7 +1011,7 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
 
     lr_reg->priority  = (lrv >> ICH_LR_PRIORITY_SHIFT) & ICH_LR_PRIORITY_MASK;
     lr_reg->state     = (lrv >> ICH_LR_STATE_SHIFT) & ICH_LR_STATE_MASK;
-    lr_reg->hw_status = (lrv >> ICH_LR_HW_SHIFT) & ICH_LR_HW_MASK;
+    lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
 }
 
 static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
@@ -1021,8 +1021,10 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
     lrv = ( ((u64)(lr->pirq & ICH_LR_PHYSICAL_MASK) << ICH_LR_PHYSICAL_SHIFT)|
         ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK)  << ICH_LR_VIRTUAL_SHIFT) |
         ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT)|
-        ((u64)(lr->state & ICH_LR_STATE_MASK) << ICH_LR_STATE_SHIFT) |
-        ((u64)(lr->hw_status & ICH_LR_HW_MASK) << ICH_LR_HW_SHIFT) );
+        ((u64)(lr->state & ICH_LR_STATE_MASK) << ICH_LR_STATE_SHIFT) );
+
+    if ( lr->hw_status )
+        lrv |= ICH_LR_HW;
 
     /*
      * When the guest is using vGICv3, all the IRQs are Group 1. Group 0
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 1eb08b856e..daec51499c 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -210,7 +210,7 @@ struct gic_lr {
    uint32_t virq;
    uint8_t priority;
    uint8_t state;
-   uint8_t hw_status;
+   bool hw_status;
 };
 
 enum gic_version {
-- 
2.11.0


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

[PATCH 4/6] xen/arm: gic: Split the field state in gic_lr in 2 fields active and pending

[julien.grall]

From: Julien Grall <julien.grall@arm.com>

Mostly making the code nicer to read.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/gic-v2.c             | 15 +++++++++++----
 xen/arch/arm/gic-v3.c             | 12 +++++++++---
 xen/arch/arm/gic-vgic.c           |  6 +++---
 xen/include/asm-arm/gic.h         |  3 ++-
 xen/include/asm-arm/gic_v3_defs.h |  2 ++
 5 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 23223575a2..90d8f652d3 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -51,6 +51,8 @@
 #define GICH_V2_LR_PHYSICAL_SHIFT  10
 #define GICH_V2_LR_STATE_MASK      0x3
 #define GICH_V2_LR_STATE_SHIFT     28
+#define GICH_V2_LR_PENDING         (1U << 28)
+#define GICH_V2_LR_ACTIVE          (1U << 29)
 #define GICH_V2_LR_PRIORITY_SHIFT  23
 #define GICH_V2_LR_PRIORITY_MASK   0x1f
 #define GICH_V2_LR_HW_SHIFT        31
@@ -467,7 +469,8 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
     lr_reg->pirq = (lrv >> GICH_V2_LR_PHYSICAL_SHIFT) & GICH_V2_LR_PHYSICAL_MASK;
     lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & GICH_V2_LR_VIRTUAL_MASK;
     lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
-    lr_reg->state     = (lrv >> GICH_V2_LR_STATE_SHIFT) & GICH_V2_LR_STATE_MASK;
+    lr_reg->pending = (lrv & GICH_V2_LR_PENDING) == GICH_V2_LR_PENDING;
+    lr_reg->active = (lrv & GICH_V2_LR_ACTIVE) == GICH_V2_LR_ACTIVE;
     lr_reg->hw_status = (lrv & GICH_V2_LR_HW) == GICH_V2_LR_HW;
 }
 
@@ -478,9 +481,13 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
     lrv = ( ((lr_reg->pirq & GICH_V2_LR_PHYSICAL_MASK) << GICH_V2_LR_PHYSICAL_SHIFT) |
           ((lr_reg->virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT)   |
           ((uint32_t)(lr_reg->priority & GICH_V2_LR_PRIORITY_MASK)
-                                      << GICH_V2_LR_PRIORITY_SHIFT) |
-          ((uint32_t)(lr_reg->state & GICH_V2_LR_STATE_MASK)
-                                   << GICH_V2_LR_STATE_SHIFT) );
+                                      << GICH_V2_LR_PRIORITY_SHIFT) );
+
+    if ( lr_reg->active )
+        lrv |= GICH_V2_LR_ACTIVE;
+
+    if ( lr_reg->pending )
+        lrv |= GICH_V2_LR_PENDING;
 
     if ( lr_reg->hw_status )
         lrv |= GICH_V2_LR_HW;
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 0711e509a6..4dbbf0afd2 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1010,7 +1010,8 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
     lr_reg->virq = (lrv >> ICH_LR_VIRTUAL_SHIFT) & ICH_LR_VIRTUAL_MASK;
 
     lr_reg->priority  = (lrv >> ICH_LR_PRIORITY_SHIFT) & ICH_LR_PRIORITY_MASK;
-    lr_reg->state     = (lrv >> ICH_LR_STATE_SHIFT) & ICH_LR_STATE_MASK;
+    lr_reg->pending   = (lrv & ICH_LR_STATE_PENDING) == ICH_LR_STATE_PENDING;
+    lr_reg->active    = (lrv & ICH_LR_STATE_ACTIVE) == ICH_LR_STATE_ACTIVE;
     lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
 }
 
@@ -1020,8 +1021,13 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
 
     lrv = ( ((u64)(lr->pirq & ICH_LR_PHYSICAL_MASK) << ICH_LR_PHYSICAL_SHIFT)|
         ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK)  << ICH_LR_VIRTUAL_SHIFT) |
-        ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT)|
-        ((u64)(lr->state & ICH_LR_STATE_MASK) << ICH_LR_STATE_SHIFT) );
+        ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT) );
+
+    if ( lr->active )
+        lrv |= ICH_LR_STATE_ACTIVE;
+
+    if ( lr->pending )
+        lrv |= ICH_LR_STATE_PENDING;
 
     if ( lr->hw_status )
         lrv |= ICH_LR_HW;
diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index e3cb47e80e..d831b35525 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -189,7 +189,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
         return;
     }
 
-    if ( lr_val.state & GICH_LR_ACTIVE )
+    if ( lr_val.active )
     {
         set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
         if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
@@ -197,7 +197,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
         {
             if ( p->desc == NULL )
             {
-                lr_val.state |= GICH_LR_PENDING;
+                lr_val.pending = true;
                 gic_hw_ops->write_lr(i, &lr_val);
             }
             else
@@ -205,7 +205,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
                          irq, v->domain->domain_id, v->vcpu_id, i);
         }
     }
-    else if ( lr_val.state & GICH_LR_PENDING )
+    else if ( lr_val.pending )
     {
         int q __attribute__ ((unused)) = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
 #ifdef GIC_DEBUG
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index daec51499c..c32861d4fa 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -209,7 +209,8 @@ struct gic_lr {
    /* Virtual IRQ */
    uint32_t virq;
    uint8_t priority;
-   uint8_t state;
+   bool active;
+   bool pending;
    bool hw_status;
 };
 
diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
index ccb72cf0f1..817bb0d5c7 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -171,6 +171,8 @@
 #define ICH_LR_PHYSICAL_SHIFT        32
 #define ICH_LR_STATE_MASK            0x3
 #define ICH_LR_STATE_SHIFT           62
+#define ICH_LR_STATE_PENDING         (1UL << 62)
+#define ICH_LR_STATE_ACTIVE          (1UL << 63)
 #define ICH_LR_PRIORITY_MASK         0xff
 #define ICH_LR_PRIORITY_SHIFT        48
 #define ICH_LR_HW_MASK               0x1
-- 
2.11.0


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

[PATCH 5/6] xen/arm: GIC: Only set pirq in the LR when hw_status is set

[julien.grall]

From: Julien Grall <julien.grall@arm.com>

The field pirq should only be valid when the virtual interrupt
is associated to a physical interrupt.

This change will help to extend gic_lr for supporting specific virtual
interrupt field (e.g eoi, source) that clashes with the PIRQ field.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/gic-v2.c     | 13 ++++++++++---
 xen/arch/arm/gic-v3.c     | 10 +++++++---
 xen/include/asm-arm/gic.h |  2 +-
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 90d8f652d3..daf8c61258 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -466,20 +466,24 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
     uint32_t lrv;
 
     lrv          = readl_gich(GICH_LR + lr * 4);
-    lr_reg->pirq = (lrv >> GICH_V2_LR_PHYSICAL_SHIFT) & GICH_V2_LR_PHYSICAL_MASK;
     lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & GICH_V2_LR_VIRTUAL_MASK;
     lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
     lr_reg->pending = (lrv & GICH_V2_LR_PENDING) == GICH_V2_LR_PENDING;
     lr_reg->active = (lrv & GICH_V2_LR_ACTIVE) == GICH_V2_LR_ACTIVE;
     lr_reg->hw_status = (lrv & GICH_V2_LR_HW) == GICH_V2_LR_HW;
+
+    if ( lr_reg->hw_status )
+    {
+        lr_reg->pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
+        lr_reg->pirq &= GICH_V2_LR_PHYSICAL_MASK;
+    }
 }
 
 static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
 {
     uint32_t lrv = 0;
 
-    lrv = ( ((lr_reg->pirq & GICH_V2_LR_PHYSICAL_MASK) << GICH_V2_LR_PHYSICAL_SHIFT) |
-          ((lr_reg->virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT)   |
+    lrv = (((lr_reg->virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT)   |
           ((uint32_t)(lr_reg->priority & GICH_V2_LR_PRIORITY_MASK)
                                       << GICH_V2_LR_PRIORITY_SHIFT) );
 
@@ -490,7 +494,10 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
         lrv |= GICH_V2_LR_PENDING;
 
     if ( lr_reg->hw_status )
+    {
         lrv |= GICH_V2_LR_HW;
+        lrv |= lr_reg->pirq << GICH_V2_LR_PHYSICAL_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 4dbbf0afd2..f73d386df1 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1006,21 +1006,22 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
 
     lrv = gicv3_ich_read_lr(lr);
 
-    lr_reg->pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
     lr_reg->virq = (lrv >> ICH_LR_VIRTUAL_SHIFT) & ICH_LR_VIRTUAL_MASK;
 
     lr_reg->priority  = (lrv >> ICH_LR_PRIORITY_SHIFT) & ICH_LR_PRIORITY_MASK;
     lr_reg->pending   = (lrv & ICH_LR_STATE_PENDING) == ICH_LR_STATE_PENDING;
     lr_reg->active    = (lrv & ICH_LR_STATE_ACTIVE) == ICH_LR_STATE_ACTIVE;
     lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
+
+    if ( lr_reg->hw_status )
+        lr_reg->pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
 }
 
 static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
 {
     uint64_t lrv = 0;
 
-    lrv = ( ((u64)(lr->pirq & ICH_LR_PHYSICAL_MASK) << ICH_LR_PHYSICAL_SHIFT)|
-        ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK)  << ICH_LR_VIRTUAL_SHIFT) |
+    lrv = ( ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK)  << ICH_LR_VIRTUAL_SHIFT) |
         ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT) );
 
     if ( lr->active )
@@ -1030,7 +1031,10 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
         lrv |= ICH_LR_STATE_PENDING;
 
     if ( lr->hw_status )
+    {
         lrv |= ICH_LR_HW;
+        lrv |= (uint64_t)lr->pirq << ICH_LR_PHYSICAL_SHIFT;
+    }
 
     /*
      * When the guest is using vGICv3, all the IRQs are Group 1. Group 0
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index c32861d4fa..545901b120 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -204,7 +204,7 @@ union gic_state_data {
  * The LR register format is different for GIC HW version
  */
 struct gic_lr {
-   /* Physical IRQ */
+   /* Physical IRQ -> Only set when hw_status is set. */
    uint32_t pirq;
    /* Virtual IRQ */
    uint32_t virq;
-- 
2.11.0


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

[PATCH 6/6] ARM: GIC: extend LR read/write functions to cover EOI and source

[julien.grall]

From: Andre Przywara <andre.przywara@linaro.org>

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.

Note that source is not covered by GICv3 LR.

This allows the new VGIC to use this information.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/gic-v2.c     | 22 +++++++++++++++++++---
 xen/arch/arm/gic-v3.c     | 11 +++++++++--
 xen/include/asm-arm/gic.h | 16 ++++++++++++++--
 3 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index daf8c61258..69f8d6044a 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->h.pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
+        lr_reg->h.pirq &= GICH_V2_LR_PHYSICAL_MASK;
+    }
+    else
+    {
+        lr_reg->v.eoi = (lrv & GICH_V2_LR_MAINTENANCE_IRQ) == 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.
+         */
+        lr_reg->v.source = (lrv >> GICH_V2_LR_CPUID_SHIFT) & GICH_V2_LR_CPUID_MASK;
     }
 }
 
@@ -496,7 +505,14 @@ 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->h.pirq << GICH_V2_LR_PHYSICAL_SHIFT;
+    }
+    else
+    {
+        if ( lr_reg->v.eoi )
+            lrv |= GICH_V2_LR_MAINTENANCE_IRQ;
+        if ( lr_reg->virq < NR_GIC_SGI )
+            lrv |= (uint32_t)lr_reg->v.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 f73d386df1..a855069111 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1014,7 +1014,9 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
     lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
 
     if ( lr_reg->hw_status )
-        lr_reg->pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
+        lr_reg->h.pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
+    else
+        lr_reg->v.eoi = (lrv & ICH_LR_MAINTENANCE_IRQ) == ICH_LR_MAINTENANCE_IRQ;
 }
 
 static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
@@ -1033,7 +1035,12 @@ 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->h.pirq << ICH_LR_PHYSICAL_SHIFT;
+    }
+    else
+    {
+        if ( lr->v.eoi )
+            lrv |= ICH_LR_MAINTENANCE_IRQ;
     }
 
     /*
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 545901b120..4cf5bb385d 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;
+       } h;
+       /* Only filled when there are no corresponding pIRQ (hw_state = false) */
+       struct
+       {
+           bool eoi;
+           uint8_t source;      /* GICv2 only */
+       } v;
+   };
 };
 
 enum gic_version {
-- 
2.11.0


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

Re: [PATCH 1/6] xen/arm: gic: Fix indentation in gic_update_one_lr

[André Przywara]

On 09/03/18 17:35, julien.grall@arm.com wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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

Cheers,
Andre.

> ---
>  xen/arch/arm/gic-vgic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index 61f093db50..e3cb47e80e 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -197,8 +197,8 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>          {
>              if ( p->desc == NULL )
>              {
> -                 lr_val.state |= GICH_LR_PENDING;
> -                 gic_hw_ops->write_lr(i, &lr_val);
> +                lr_val.state |= GICH_LR_PENDING;
> +                gic_hw_ops->write_lr(i, &lr_val);
>              }
>              else
>                  gdprintk(XENLOG_WARNING, "unable to inject hw irq=%d into d%dv%d: already active in LR%d\n",
> 


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

Re: [PATCH 2/6] xen/arm: vgic: Override the group in lr everytime

[Andre Przywara]

On 09/03/18 17:35, julien.grall@arm.com wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> At the moment, write_lr is assuming the caller will set correctly the
> group. However the group should always be 0 when the guest is using
> vGICv2 and 1 for vGICv3. As the caller should not care about the group,
> override it directly.

I think that makes sense, mostly because this is what KVM does as well.
And it's a good idea to do this in write_lr().

I understand that this is effectively what I did in the new VGIC, but it
would be good to double check that this is the right thing to do
for the old VGIC as well. Did you test this on some GICv3 h/w or the
model? Or do we always call update_lr() anyway?

Cheers,
Andre.

> With that change, write_lr is now behaving like update_lr for the group.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/gic-v2.c     |  4 +---
>  xen/arch/arm/gic-v3.c     | 11 ++++++++---
>  xen/include/asm-arm/gic.h |  1 -
>  3 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index f16e17c1a3..fc105c08b8 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -469,7 +469,6 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
>      lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
>      lr_reg->state     = (lrv >> GICH_V2_LR_STATE_SHIFT) & GICH_V2_LR_STATE_MASK;
>      lr_reg->hw_status = (lrv >> GICH_V2_LR_HW_SHIFT) & GICH_V2_LR_HW_MASK;
> -    lr_reg->grp       = (lrv >> GICH_V2_LR_GRP_SHIFT) & GICH_V2_LR_GRP_MASK;
>  }
>  
>  static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
> @@ -483,8 +482,7 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
>            ((uint32_t)(lr_reg->state & GICH_V2_LR_STATE_MASK)
>                                     << GICH_V2_LR_STATE_SHIFT) |
>            ((uint32_t)(lr_reg->hw_status & GICH_V2_LR_HW_MASK)
> -                                       << GICH_V2_LR_HW_SHIFT)  |
> -          ((uint32_t)(lr_reg->grp & GICH_V2_LR_GRP_MASK) << GICH_V2_LR_GRP_SHIFT) );
> +                                       << GICH_V2_LR_HW_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 09b49a07d5..0dfa1a1e08 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1012,7 +1012,6 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
>      lr_reg->priority  = (lrv >> ICH_LR_PRIORITY_SHIFT) & ICH_LR_PRIORITY_MASK;
>      lr_reg->state     = (lrv >> ICH_LR_STATE_SHIFT) & ICH_LR_STATE_MASK;
>      lr_reg->hw_status = (lrv >> ICH_LR_HW_SHIFT) & ICH_LR_HW_MASK;
> -    lr_reg->grp       = (lrv >> ICH_LR_GRP_SHIFT) & ICH_LR_GRP_MASK;
>  }
>  
>  static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
> @@ -1023,8 +1022,14 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
>          ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK)  << ICH_LR_VIRTUAL_SHIFT) |
>          ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT)|
>          ((u64)(lr->state & ICH_LR_STATE_MASK) << ICH_LR_STATE_SHIFT) |
> -        ((u64)(lr->hw_status & ICH_LR_HW_MASK) << ICH_LR_HW_SHIFT)  |
> -        ((u64)(lr->grp & ICH_LR_GRP_MASK) << ICH_LR_GRP_SHIFT) );
> +        ((u64)(lr->hw_status & ICH_LR_HW_MASK) << ICH_LR_HW_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 )
> +        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 49cb94f792..1eb08b856e 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -211,7 +211,6 @@ struct gic_lr {
>     uint8_t priority;
>     uint8_t state;
>     uint8_t hw_status;
> -   uint8_t grp;
>  };
>  
>  enum gic_version {
> 


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

Re: [PATCH 3/6] xen/arm: gic: Use bool instead of uint8_t for the hw_status in gic_lr

[Andre Przywara]

Hi,

On 09/03/18 16:35, julien.grall@arm.com wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> hw_status can only be 1 or 0. So convert to a bool.
> 
> 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     | 9 +++++----
>  xen/arch/arm/gic-v3.c     | 8 +++++---
>  xen/include/asm-arm/gic.h | 2 +-
>  3 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index fc105c08b8..23223575a2 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -468,7 +468,7 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
>      lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & GICH_V2_LR_VIRTUAL_MASK;
>      lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
>      lr_reg->state     = (lrv >> GICH_V2_LR_STATE_SHIFT) & GICH_V2_LR_STATE_MASK;
> -    lr_reg->hw_status = (lrv >> GICH_V2_LR_HW_SHIFT) & GICH_V2_LR_HW_MASK;
> +    lr_reg->hw_status = (lrv & GICH_V2_LR_HW) == GICH_V2_LR_HW;
>  }
>  
>  static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
> @@ -480,9 +480,10 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
>            ((uint32_t)(lr_reg->priority & GICH_V2_LR_PRIORITY_MASK)
>                                        << GICH_V2_LR_PRIORITY_SHIFT) |
>            ((uint32_t)(lr_reg->state & GICH_V2_LR_STATE_MASK)
> -                                   << GICH_V2_LR_STATE_SHIFT) |
> -          ((uint32_t)(lr_reg->hw_status & GICH_V2_LR_HW_MASK)
> -                                       << GICH_V2_LR_HW_SHIFT));
> +                                   << GICH_V2_LR_STATE_SHIFT) );
> +
> +    if ( lr_reg->hw_status )
> +        lrv |= GICH_V2_LR_HW;
>  
>      writel_gich(lrv, GICH_LR + lr * 4);
>  }
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 0dfa1a1e08..0711e509a6 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1011,7 +1011,7 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
>  
>      lr_reg->priority  = (lrv >> ICH_LR_PRIORITY_SHIFT) & ICH_LR_PRIORITY_MASK;
>      lr_reg->state     = (lrv >> ICH_LR_STATE_SHIFT) & ICH_LR_STATE_MASK;
> -    lr_reg->hw_status = (lrv >> ICH_LR_HW_SHIFT) & ICH_LR_HW_MASK;
> +    lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
>  }
>  
>  static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
> @@ -1021,8 +1021,10 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
>      lrv = ( ((u64)(lr->pirq & ICH_LR_PHYSICAL_MASK) << ICH_LR_PHYSICAL_SHIFT)|
>          ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK)  << ICH_LR_VIRTUAL_SHIFT) |
>          ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT)|
> -        ((u64)(lr->state & ICH_LR_STATE_MASK) << ICH_LR_STATE_SHIFT) |
> -        ((u64)(lr->hw_status & ICH_LR_HW_MASK) << ICH_LR_HW_SHIFT) );
> +        ((u64)(lr->state & ICH_LR_STATE_MASK) << ICH_LR_STATE_SHIFT) );
> +
> +    if ( lr->hw_status )
> +        lrv |= ICH_LR_HW;
>  
>      /*
>       * When the guest is using vGICv3, all the IRQs are Group 1. Group 0
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 1eb08b856e..daec51499c 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -210,7 +210,7 @@ struct gic_lr {
>     uint32_t virq;
>     uint8_t priority;
>     uint8_t state;
> -   uint8_t hw_status;
> +   bool hw_status;
>  };
>  
>  enum gic_version {
> 

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

Re: [PATCH 2/6] xen/arm: vgic: Override the group in lr everytime

[Julien Grall]

Hi Andre,

On 03/14/2018 06:02 PM, Andre Przywara wrote:
> On 09/03/18 17:35, julien.grall@arm.com wrote:
>> From: Julien Grall <julien.grall@arm.com>
>>
>> At the moment, write_lr is assuming the caller will set correctly the
>> group. However the group should always be 0 when the guest is using
>> vGICv2 and 1 for vGICv3. As the caller should not care about the group,
>> override it directly.
> 
> I think that makes sense, mostly because this is what KVM does as well.
> And it's a good idea to do this in write_lr().
> 
> I understand that this is effectively what I did in the new VGIC, but it
> would be good to double check that this is the right thing to do
> for the old VGIC as well. Did you test this on some GICv3 h/w or the
> model? Or do we always call update_lr() anyway?

I had a patch to remove update_lr and use write_lr. I exercised the code 
with it but didn't send it because of some nasty bug with the priority 
in write_lr and the old vGIC.

On the old vGIC write_lr is only used when in gic_update_one_lr(...) 
combine with read_lr before. In that context we don't care about the 
group, so overwriting it is fine and the right thing to do.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 4/6] xen/arm: gic: Split the field state in gic_lr in 2 fields active and pending

[Andre Przywara]

Hi,

On 09/03/18 16:35, julien.grall@arm.com wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> Mostly making the code nicer to read.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/gic-v2.c             | 15 +++++++++++----
>  xen/arch/arm/gic-v3.c             | 12 +++++++++---
>  xen/arch/arm/gic-vgic.c           |  6 +++---
>  xen/include/asm-arm/gic.h         |  3 ++-
>  xen/include/asm-arm/gic_v3_defs.h |  2 ++
>  5 files changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 23223575a2..90d8f652d3 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -51,6 +51,8 @@
>  #define GICH_V2_LR_PHYSICAL_SHIFT  10
>  #define GICH_V2_LR_STATE_MASK      0x3
>  #define GICH_V2_LR_STATE_SHIFT     28
> +#define GICH_V2_LR_PENDING         (1U << 28)
> +#define GICH_V2_LR_ACTIVE          (1U << 29)
>  #define GICH_V2_LR_PRIORITY_SHIFT  23
>  #define GICH_V2_LR_PRIORITY_MASK   0x1f
>  #define GICH_V2_LR_HW_SHIFT        31
> @@ -467,7 +469,8 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
>      lr_reg->pirq = (lrv >> GICH_V2_LR_PHYSICAL_SHIFT) & GICH_V2_LR_PHYSICAL_MASK;
>      lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & GICH_V2_LR_VIRTUAL_MASK;
>      lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
> -    lr_reg->state     = (lrv >> GICH_V2_LR_STATE_SHIFT) & GICH_V2_LR_STATE_MASK;
> +    lr_reg->pending = (lrv & GICH_V2_LR_PENDING) == GICH_V2_LR_PENDING;
> +    lr_reg->active = (lrv & GICH_V2_LR_ACTIVE) == GICH_V2_LR_ACTIVE;
>      lr_reg->hw_status = (lrv & GICH_V2_LR_HW) == GICH_V2_LR_HW;
>  }
>  
> @@ -478,9 +481,13 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
>      lrv = ( ((lr_reg->pirq & GICH_V2_LR_PHYSICAL_MASK) << GICH_V2_LR_PHYSICAL_SHIFT) |
>            ((lr_reg->virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT)   |
>            ((uint32_t)(lr_reg->priority & GICH_V2_LR_PRIORITY_MASK)
> -                                      << GICH_V2_LR_PRIORITY_SHIFT) |
> -          ((uint32_t)(lr_reg->state & GICH_V2_LR_STATE_MASK)
> -                                   << GICH_V2_LR_STATE_SHIFT) );
> +                                      << GICH_V2_LR_PRIORITY_SHIFT) );
> +
> +    if ( lr_reg->active )
> +        lrv |= GICH_V2_LR_ACTIVE;
> +
> +    if ( lr_reg->pending )
> +        lrv |= GICH_V2_LR_PENDING;
>  
>      if ( lr_reg->hw_status )
>          lrv |= GICH_V2_LR_HW;
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 0711e509a6..4dbbf0afd2 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1010,7 +1010,8 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
>      lr_reg->virq = (lrv >> ICH_LR_VIRTUAL_SHIFT) & ICH_LR_VIRTUAL_MASK;
>  
>      lr_reg->priority  = (lrv >> ICH_LR_PRIORITY_SHIFT) & ICH_LR_PRIORITY_MASK;
> -    lr_reg->state     = (lrv >> ICH_LR_STATE_SHIFT) & ICH_LR_STATE_MASK;
> +    lr_reg->pending   = (lrv & ICH_LR_STATE_PENDING) == ICH_LR_STATE_PENDING;
> +    lr_reg->active    = (lrv & ICH_LR_STATE_ACTIVE) == ICH_LR_STATE_ACTIVE;
>      lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
>  }
>  
> @@ -1020,8 +1021,13 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
>  
>      lrv = ( ((u64)(lr->pirq & ICH_LR_PHYSICAL_MASK) << ICH_LR_PHYSICAL_SHIFT)|
>          ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK)  << ICH_LR_VIRTUAL_SHIFT) |
> -        ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT)|
> -        ((u64)(lr->state & ICH_LR_STATE_MASK) << ICH_LR_STATE_SHIFT) );
> +        ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT) );
> +
> +    if ( lr->active )
> +        lrv |= ICH_LR_STATE_ACTIVE;
> +
> +    if ( lr->pending )
> +        lrv |= ICH_LR_STATE_PENDING;
>  
>      if ( lr->hw_status )
>          lrv |= ICH_LR_HW;
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index e3cb47e80e..d831b35525 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -189,7 +189,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>          return;
>      }
>  
> -    if ( lr_val.state & GICH_LR_ACTIVE )
> +    if ( lr_val.active )
>      {
>          set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
>          if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
> @@ -197,7 +197,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>          {
>              if ( p->desc == NULL )
>              {
> -                lr_val.state |= GICH_LR_PENDING;
> +                lr_val.pending = true;
>                  gic_hw_ops->write_lr(i, &lr_val);
>              }
>              else
> @@ -205,7 +205,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>                           irq, v->domain->domain_id, v->vcpu_id, i);
>          }
>      }
> -    else if ( lr_val.state & GICH_LR_PENDING )
> +    else if ( lr_val.pending )
>      {
>          int q __attribute__ ((unused)) = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
>  #ifdef GIC_DEBUG
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index daec51499c..c32861d4fa 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -209,7 +209,8 @@ struct gic_lr {
>     /* Virtual IRQ */
>     uint32_t virq;
>     uint8_t priority;
> -   uint8_t state;
> +   bool active;
> +   bool pending;
>     bool hw_status;
>  };
>  
> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
> index ccb72cf0f1..817bb0d5c7 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -171,6 +171,8 @@
>  #define ICH_LR_PHYSICAL_SHIFT        32
>  #define ICH_LR_STATE_MASK            0x3
>  #define ICH_LR_STATE_SHIFT           62
> +#define ICH_LR_STATE_PENDING         (1UL << 62)
> +#define ICH_LR_STATE_ACTIVE          (1UL << 63)

Should that be 1ULL, just in case we ever get 32-bit support for GICv3?

Regardless of that:

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

Cheers,
Andre.

>  #define ICH_LR_PRIORITY_MASK         0xff
>  #define ICH_LR_PRIORITY_SHIFT        48
>  #define ICH_LR_HW_MASK               0x1
> 

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

Re: [PATCH 5/6] xen/arm: GIC: Only set pirq in the LR when hw_status is set

[Andre Przywara]

Hi,

On 09/03/18 16:35, julien.grall@arm.com wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> The field pirq should only be valid when the virtual interrupt
> is associated to a physical interrupt.
> 
> This change will help to extend gic_lr for supporting specific virtual
> interrupt field (e.g eoi, source) that clashes with the PIRQ field.

Makes some sense, yes.

> 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     | 13 ++++++++++---
>  xen/arch/arm/gic-v3.c     | 10 +++++++---
>  xen/include/asm-arm/gic.h |  2 +-
>  3 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 90d8f652d3..daf8c61258 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -466,20 +466,24 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
>      uint32_t lrv;
>  
>      lrv          = readl_gich(GICH_LR + lr * 4);
> -    lr_reg->pirq = (lrv >> GICH_V2_LR_PHYSICAL_SHIFT) & GICH_V2_LR_PHYSICAL_MASK;
>      lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & GICH_V2_LR_VIRTUAL_MASK;
>      lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & GICH_V2_LR_PRIORITY_MASK;
>      lr_reg->pending = (lrv & GICH_V2_LR_PENDING) == GICH_V2_LR_PENDING;
>      lr_reg->active = (lrv & GICH_V2_LR_ACTIVE) == GICH_V2_LR_ACTIVE;
>      lr_reg->hw_status = (lrv & GICH_V2_LR_HW) == GICH_V2_LR_HW;
> +
> +    if ( lr_reg->hw_status )
> +    {
> +        lr_reg->pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
> +        lr_reg->pirq &= GICH_V2_LR_PHYSICAL_MASK;
> +    }
>  }
>  
>  static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
>  {
>      uint32_t lrv = 0;
>  
> -    lrv = ( ((lr_reg->pirq & GICH_V2_LR_PHYSICAL_MASK) << GICH_V2_LR_PHYSICAL_SHIFT) |
> -          ((lr_reg->virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT)   |
> +    lrv = (((lr_reg->virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT)   |
>            ((uint32_t)(lr_reg->priority & GICH_V2_LR_PRIORITY_MASK)
>                                        << GICH_V2_LR_PRIORITY_SHIFT) );
>  
> @@ -490,7 +494,10 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
>          lrv |= GICH_V2_LR_PENDING;
>  
>      if ( lr_reg->hw_status )
> +    {
>          lrv |= GICH_V2_LR_HW;
> +        lrv |= lr_reg->pirq << GICH_V2_LR_PHYSICAL_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 4dbbf0afd2..f73d386df1 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1006,21 +1006,22 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
>  
>      lrv = gicv3_ich_read_lr(lr);
>  
> -    lr_reg->pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
>      lr_reg->virq = (lrv >> ICH_LR_VIRTUAL_SHIFT) & ICH_LR_VIRTUAL_MASK;
>  
>      lr_reg->priority  = (lrv >> ICH_LR_PRIORITY_SHIFT) & ICH_LR_PRIORITY_MASK;
>      lr_reg->pending   = (lrv & ICH_LR_STATE_PENDING) == ICH_LR_STATE_PENDING;
>      lr_reg->active    = (lrv & ICH_LR_STATE_ACTIVE) == ICH_LR_STATE_ACTIVE;
>      lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
> +
> +    if ( lr_reg->hw_status )
> +        lr_reg->pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
>  }
>  
>  static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
>  {
>      uint64_t lrv = 0;
>  
> -    lrv = ( ((u64)(lr->pirq & ICH_LR_PHYSICAL_MASK) << ICH_LR_PHYSICAL_SHIFT)|
> -        ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK)  << ICH_LR_VIRTUAL_SHIFT) |
> +    lrv = ( ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK)  << ICH_LR_VIRTUAL_SHIFT) |
>          ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << ICH_LR_PRIORITY_SHIFT) );
>  
>      if ( lr->active )
> @@ -1030,7 +1031,10 @@ static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
>          lrv |= ICH_LR_STATE_PENDING;
>  
>      if ( lr->hw_status )
> +    {
>          lrv |= ICH_LR_HW;
> +        lrv |= (uint64_t)lr->pirq << ICH_LR_PHYSICAL_SHIFT;
> +    }
>  
>      /*
>       * When the guest is using vGICv3, all the IRQs are Group 1. Group 0
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index c32861d4fa..545901b120 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -204,7 +204,7 @@ union gic_state_data {
>   * The LR register format is different for GIC HW version
>   */
>  struct gic_lr {
> -   /* Physical IRQ */
> +   /* Physical IRQ -> Only set when hw_status is set. */
>     uint32_t pirq;
>     /* Virtual IRQ */
>     uint32_t virq;
> 

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

Re: [PATCH 6/6] ARM: GIC: extend LR read/write functions to cover EOI and source

[Andre Przywara]

Hi,

On 09/03/18 16:35, julien.grall@arm.com wrote:
> From: Andre Przywara <andre.przywara@linaro.org>

I think this is quite different from what I ever wrote, so please drop
my authorship here.

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

As mentioned before, I am not sure this is really necessary. The idea of
struct gic_lr is to provide a hardware agnostic view of an LR. So the
actual read_lr/write_lr function take care of reading/populating pINTID,
iff the HW bit is set (as done in your patch 5/6).
Given that, I don't think we need to change the current code in this
respect, as this is just a small internal interface.

But then again I don't care enough, so if that makes you happy ....

> Note that source is not covered by GICv3 LR.

I was thinking the same, but Marc pointed me to that inconspicuous note
on page 8-299 in ARM IHI 0069D (at the end of "vINTID", chapter 8.4.6).

> This allows the new VGIC to use this information.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/gic-v2.c     | 22 +++++++++++++++++++---
>  xen/arch/arm/gic-v3.c     | 11 +++++++++--
>  xen/include/asm-arm/gic.h | 16 ++++++++++++++--
>  3 files changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index daf8c61258..69f8d6044a 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->h.pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
> +        lr_reg->h.pirq &= GICH_V2_LR_PHYSICAL_MASK;
> +    }
> +    else
> +    {
> +        lr_reg->v.eoi = (lrv & GICH_V2_LR_MAINTENANCE_IRQ) == GICH_V2_LR_MAINTENANCE_IRQ;

I think you can drop the " == GICH_V2_LR_MAINTENANCE_IRQ", as .eoi is a
bool. Avoids the linebreak.

> +        /*
> +         * This is only valid for SGI, but it does not matter to always
> +         * read it as it should be 0 by default.
> +         */
> +        lr_reg->v.source = (lrv >> GICH_V2_LR_CPUID_SHIFT) & GICH_V2_LR_CPUID_MASK;
>      }
>  }
>  
> @@ -496,7 +505,14 @@ 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->h.pirq << GICH_V2_LR_PHYSICAL_SHIFT;
> +    }
> +    else
> +    {
> +        if ( lr_reg->v.eoi )
> +            lrv |= GICH_V2_LR_MAINTENANCE_IRQ;
> +        if ( lr_reg->virq < NR_GIC_SGI )
> +            lrv |= (uint32_t)lr_reg->v.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 f73d386df1..a855069111 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1014,7 +1014,9 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
>      lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
>  
>      if ( lr_reg->hw_status )
> -        lr_reg->pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
> +        lr_reg->h.pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
> +    else
> +        lr_reg->v.eoi = (lrv & ICH_LR_MAINTENANCE_IRQ) == ICH_LR_MAINTENANCE_IRQ;

Same here.

>  }
>  
>  static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
> @@ -1033,7 +1035,12 @@ 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->h.pirq << ICH_LR_PHYSICAL_SHIFT;
> +    }
> +    else
> +    {
> +        if ( lr->v.eoi )
> +            lrv |= ICH_LR_MAINTENANCE_IRQ;
>      }
>  
>      /*
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 545901b120..4cf5bb385d 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;
> +       } h;
> +       /* Only filled when there are no corresponding pIRQ (hw_state = false) */
> +       struct
> +       {
> +           bool eoi;
> +           uint8_t source;      /* GICv2 only */
> +       } v;

That looks somewhat confusing to me. So either you use "hw" and "virt"
for the struct identifiers, or, preferably, just drop them altogether:

union {
	uint32_t pirq;	/* Contains the associated hardware IRQ. */
	struct		/* Only used for virtual interrupts. */
	{
		bool eoi;
		uint8_t source;
	};
};

Cheers,
Andre.

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

Re: [PATCH 4/6] xen/arm: gic: Split the field state in gic_lr in 2 fields active and pending

[Julien Grall]

On 03/14/2018 06:09 PM, Andre Przywara wrote:
> On 09/03/18 16:35, julien.grall@arm.com wrote:
>> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
>> index ccb72cf0f1..817bb0d5c7 100644
>> --- a/xen/include/asm-arm/gic_v3_defs.h
>> +++ b/xen/include/asm-arm/gic_v3_defs.h
>> @@ -171,6 +171,8 @@
>>   #define ICH_LR_PHYSICAL_SHIFT        32
>>   #define ICH_LR_STATE_MASK            0x3
>>   #define ICH_LR_STATE_SHIFT           62
>> +#define ICH_LR_STATE_PENDING         (1UL << 62)
>> +#define ICH_LR_STATE_ACTIVE          (1UL << 63)
> 
> Should that be 1ULL, just in case we ever get 32-bit support for GICv3?

Yes, good point. I will fix that.

> 
> Regardless of that:
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,

-- 
Julien Grall

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

Re: [PATCH 6/6] ARM: GIC: extend LR read/write functions to cover EOI and source

[Julien Grall]

On 03/14/2018 06:19 PM, Andre Przywara wrote:
> Hi,

Hi Andre,

Thank you for the review.

> On 09/03/18 16:35, julien.grall@arm.com wrote:
>> From: Andre Przywara <andre.przywara@linaro.org>
> 
> I think this is quite different from what I ever wrote, so please drop
> my authorship here.

Fine, I wasn't sure given that you were the original author or extending 
the LR. I can pointing that in the commit message :).

>> 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.
> 
> As mentioned before, I am not sure this is really necessary. The idea of
> struct gic_lr is to provide a hardware agnostic view of an LR. So the
> actual read_lr/write_lr function take care of reading/populating pINTID,
> iff the HW bit is set (as done in your patch 5/6).
> Given that, I don't think we need to change the current code in this
> respect, as this is just a small internal interface.

Even if I know the vGIC, I find easier with this solution to 
differentiate what is for the HW IRQ or not.

The size of Xen Arm is becoming quite significant for me to remember 
every single line/structure. So the main goal here is to help the 
reviewer to spend less time on patch review as it helps to spot directly 
misusage.

> 
> But then again I don't care enough, so if that makes you happy ....
> 
>> Note that source is not covered by GICv3 LR.
> 
> I was thinking the same, but Marc pointed me to that inconspicuous note
> on page 8-299 in ARM IHI 0069D (at the end of "vINTID", chapter 8.4.6).

Doh :/. I will drop the comment and update the GICv3 code then.

> 
>> This allows the new VGIC to use this information.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/gic-v2.c     | 22 +++++++++++++++++++---
>>   xen/arch/arm/gic-v3.c     | 11 +++++++++--
>>   xen/include/asm-arm/gic.h | 16 ++++++++++++++--
>>   3 files changed, 42 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
>> index daf8c61258..69f8d6044a 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->h.pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
>> +        lr_reg->h.pirq &= GICH_V2_LR_PHYSICAL_MASK;
>> +    }
>> +    else
>> +    {
>> +        lr_reg->v.eoi = (lrv & GICH_V2_LR_MAINTENANCE_IRQ) == GICH_V2_LR_MAINTENANCE_IRQ;
> 
> I think you can drop the " == GICH_V2_LR_MAINTENANCE_IRQ", as .eoi is a
> bool. Avoids the linebreak.

The '==' was more to avoid assuming GIC_V2_LR_MAINTENANCE_IRQ is a 
single bit. But I was probably too cautious, I will drop it.

>>       writel_gich(lrv, GICH_LR + lr * 4);
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index f73d386df1..a855069111 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -1014,7 +1014,9 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
>>       lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
>>   
>>       if ( lr_reg->hw_status )
>> -        lr_reg->pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
>> +        lr_reg->h.pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
>> +    else
>> +        lr_reg->v.eoi = (lrv & ICH_LR_MAINTENANCE_IRQ) == ICH_LR_MAINTENANCE_IRQ;
> 
> Same here.

Ditto.

> 
>>   }
>>   
>>   static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
>> @@ -1033,7 +1035,12 @@ 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->h.pirq << ICH_LR_PHYSICAL_SHIFT;
>> +    }
>> +    else
>> +    {
>> +        if ( lr->v.eoi )
>> +            lrv |= ICH_LR_MAINTENANCE_IRQ;
>>       }
>>   
>>       /*
>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>> index 545901b120..4cf5bb385d 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;
>> +       } h;
>> +       /* Only filled when there are no corresponding pIRQ (hw_state = false) */
>> +       struct
>> +       {
>> +           bool eoi;
>> +           uint8_t source;      /* GICv2 only */
>> +       } v;
> 
> That looks somewhat confusing to me. So either you use "hw" and "virt"
> for the struct identifiers, or, preferably, just drop them altogether:
> 
> union {
> 	uint32_t pirq;	/* Contains the associated hardware IRQ. */
> 	struct		/* Only used for virtual interrupts. */
> 	{
> 		bool eoi;
> 		uint8_t source;
> 	};
> };

I would prefer to keep a name for each structure. It is not obvious for 
me that eoi is only for virtual IRQ. I mostly want to help code review 
in the future.

Cheers,

-- 
Julien Grall

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