[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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@xxxxxxx wrote:
From: Andre Przywara <andre.przywara@xxxxxxxxxx>

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@xxxxxxxxxx>
Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
---
  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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.