Re: [Xen-devel] [PATCH 1/4] xen/arm: gic: Ensure we have an ISB between ack and do_IRQ()

[Andrii Anisov <Andrii_Anisov@xxxxxxxx>]

Hello Julien


On 25.10.18 17:11, Andrii Anisov wrote:
> I guess I should make a dedicated patch applicable to mainline to reveal
> the issue. I hope I'll do this nearest days.

Please find below the diff applicable to the current xenbits/smoke which 
exposes the issue.

With that diff I see (on my board) outputs like:


     (XEN) Stored LR is stale: stored 0xAA025C97 read 0x8A025C97
     (XEN) Stored LR is stale: stored 0x9A025C97 read 0x8A025C97
     (XEN) Stored LR is stale: stored 0x9A025C97 read 0x8A025C97
     (XEN) Stored LR is stale: stored 0x9A025C97 read 0x8A025C97
     (XEN) Stored LR is stale: stored 0x1A00001B read 0xA00001B
     (XEN) Stored LR is stale: stored 0x9A025C97 read 0x8A025C97
     (XEN) Stored LR is stale: stored 0xAA025C97 read 0x8A025C97


Those prints I treat as LR content change (state PENDING->INVALID) 
without exiting from hyp to guest. Unfortunately, I did not find a kind 
of explanation to this effect in ARM IHI 0048B.b document.
I have GICv2 on the die.

I appreciate you find some time to look at this and express your opinion.


---

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 511c8d7..0b9aa2d 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -171,6 +171,14 @@ static unsigned int gicv2_cpu_mask(const cpumask_t 
*cpumask)
      return mask;
  }

+static void gicv2_store_lrs(void)
+{
+    int i;
+
+    for ( i = 0; i < gicv2_info.nr_lrs; i++ )
+        current->arch.gic.v2.lr[i] = readl_gich(GICH_LR + i * 4);
+}
+
  static void gicv2_save_state(struct vcpu *v)
  {
      int i;
@@ -446,11 +454,13 @@ static void gicv2_update_lr(int lr, const struct 
pending_irq *p,
                                     << GICH_V2_LR_PHYSICAL_SHIFT);

      writel_gich(lr_reg, GICH_LR + lr * 4);
+    current->arch.gic.v2.lr[lr] = lr_reg;
  }

  static void gicv2_clear_lr(int lr)
  {
      writel_gich(0, GICH_LR + lr * 4);
+    current->arch.gic.v2.lr[lr] = 0;
  }

  static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
@@ -458,6 +468,9 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
      uint32_t lrv;

      lrv          = readl_gich(GICH_LR + lr * 4);
+    if ( lrv != current->arch.gic.v2.lr[lr])
+        printk("Stored LR is stale: stored 0x%"PRIX32" read 0x%"PRIX32"\n",
+               current->arch.gic.v2.lr[lr], lrv);
      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;
@@ -481,6 +494,7 @@ static void gicv2_write_lr(int lr, const struct 
gic_lr *lr_reg)
            ((uint32_t)(lr_reg->grp & GICH_V2_LR_GRP_MASK) << 
GICH_V2_LR_GRP_SHIFT) );

      writel_gich(lrv, GICH_LR + lr * 4);
+    current->arch.gic.v2.lr[lr] = lrv;
  }

  static void gicv2_hcr_status(uint32_t flag, bool status)
@@ -1232,6 +1246,7 @@ const static struct gic_hw_operations gicv2_ops = {
      .info                = &gicv2_info,
      .init                = gicv2_init,
      .secondary_init      = gicv2_secondary_cpu_init,
+    .store_lrs           = gicv2_store_lrs,
      .save_state          = gicv2_save_state,
      .restore_state       = gicv2_restore_state,
      .dump_state          = gicv2_dump_state,
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index ed363f6..a05c518 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -594,6 +594,12 @@ static void gic_update_one_lr(struct vcpu *v, int i)
      }
  }

+void gic_store_lrs(void)
+{
+    if(gic_hw_ops->store_lrs)
+        gic_hw_ops->store_lrs();
+}
+
  void gic_clear_lrs(struct vcpu *v)
  {
      int i = 0;
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6f6de3..985192b 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2095,6 +2095,7 @@ static void enter_hypervisor_head(struct 
cpu_user_regs *regs)
          if ( current->arch.hcr_el2 & HCR_VA )
              current->arch.hcr_el2 = READ_SYSREG(HCR_EL2);

+        gic_store_lrs();
          gic_clear_lrs(current);
      }
  }
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index d3d7bda..6fe3fdb 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -293,6 +293,7 @@ extern unsigned int gic_number_lines(void);
  /* IRQ translation function for the device tree */
  int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
                    unsigned int *out_hwirq, unsigned int *out_type);
+void gic_store_lrs(void);
  void gic_clear_lrs(struct vcpu *v);

  struct gic_info {
@@ -314,6 +315,8 @@ struct gic_hw_operations {
      /* Initialize the GIC and the boot CPU */
      int (*init)(void);
      /* Save GIC registers */
+    void (*store_lrs)(void);
+    /* Save GIC registers */
      void (*save_state)(struct vcpu *);
      /* Restore GIC registers */
      void (*restore_state)(const struct vcpu *);



-- 

*Andrii Anisov*



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