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

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





On 10/26/18 1:49 PM, Andrii Anisov wrote:
Hello Julien

Hi,

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.

The functions below does not exist in latest Xen. So are you sure this based on mainline?


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

How many domain do you have run at that time? Under which condition this is hapenning?

Also, what is your Xen command line? I assume you didn't disable serrors, right? So we should have plenty of dsb(sy) and isb() in the path to there.

So I would rule out a missing barrier here.



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(...) may rewrite the LRs. To confirm this stall LRs are not due by this function, can you move that after gic_clear_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 *);

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