diff mbox series

[v2] xive: disable store EOI support

Message ID 20180326150820.27288-1-clg@kaod.org
State Superseded
Headers show
Series [v2] xive: disable store EOI support | expand

Commit Message

Cédric Le Goater March 26, 2018, 3:08 p.m. UTC 
Hardware has limitations which would require to put a sync after each
store EOI to make sure the MMIO operations that change the ESB state
are ordered. This is a killer for performance and the PHBs do not
support the sync. So remove the store EOI for the moment, until
hardware is improved.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---

 Changes since v2 :

 - also removed support from PHB4 ...
 
 hw/phb4.c      |  8 ++++++--
 hw/xive.c      | 13 ++++++++++---
 include/xive.h |  3 +++
 3 files changed, 19 insertions(+), 5 deletions(-)

Comments

Benjamin Herrenschmidt March 26, 2018, 9:50 p.m. UTC | #1 
On Mon, 2018-03-26 at 17:08 +0200, Cédric Le Goater wrote:
> @@ -5027,7 +5031,7 @@ static void phb4_create(struct dt_node *np)
>  
>         /* Register all interrupt sources with XIVE */
>         irq_flags = XIVE_SRC_SHIFT_BUG | XIVE_SRC_TRIGGER_PAGE;
> -       if (p->rev >= PHB4_REV_NIMBUS_DD20)
> +       if (PHB4_CAN_STORE_EOI(p))
>                 irq_flags |= XIVE_SRC_STORE_EOI;
>         xive_register_hw_source(p->base_msi, p->num_irqs - 8, 16,
>                                 p->int_mmio, irq_flags, NULL, NULL);

Looking at this, it looks like the old code was a bit broken already...

If STORE_EOI is enabled, there is no trigger page.

Also the shift bug only happens on DD1.0 afaik.

Cheers,
Ben.
Cédric Le Goater April 3, 2018, 6:22 a.m. UTC | #2 
On 03/26/2018 11:50 PM, Benjamin Herrenschmidt wrote:
> On Mon, 2018-03-26 at 17:08 +0200, Cédric Le Goater wrote:
>> @@ -5027,7 +5031,7 @@ static void phb4_create(struct dt_node *np)
>>  
>>         /* Register all interrupt sources with XIVE */
>>         irq_flags = XIVE_SRC_SHIFT_BUG | XIVE_SRC_TRIGGER_PAGE;
>> -       if (p->rev >= PHB4_REV_NIMBUS_DD20)
>> +       if (PHB4_CAN_STORE_EOI(p))
>>                 irq_flags |= XIVE_SRC_STORE_EOI;
>>         xive_register_hw_source(p->base_msi, p->num_irqs - 8, 16,
>>                                 p->int_mmio, irq_flags, NULL, NULL);
> 
> Looking at this, it looks like the old code was a bit broken already...
> 
> If STORE_EOI is enabled, there is no trigger page.

Is that true for the IPIs also or only for the PHB4s interrupts ? 
I am a bit confused.

Thanks,

C.

 
> Also the shift bug only happens on DD1.0 afaik.
> 
> Cheers,
> Ben.
>
diff mbox series

Patch

diff --git a/hw/phb4.c b/hw/phb4.c
index 6c59462bd610..28794e1b2716 100644
--- a/hw/phb4.c
+++ b/hw/phb4.c
@@ -145,6 +145,9 @@  static void phb4_init_hw(struct phb4 *p, bool first_init);
 #define PHBLOGCFG(p, fmt, a...) do {} while (0)
 #endif
 
+#define PHB4_CAN_STORE_EOI(p) \
+	(XIVE_STORE_EOI_ENABLED && ((p)->rev >= PHB4_REV_NIMBUS_DD20))
+
 static bool verbose_eeh;
 static bool pci_tracing;
 static bool pci_eeh_mmio;
@@ -4439,7 +4442,8 @@  static void phb4_init_hw(struct phb4 *p, bool first_init)
 		val |= SETFIELD(PHB_CTRLR_TVT_ADDR_SEL, 0ull, TVT_DD1_2_PER_PE);
 	} else {
 		val |= SETFIELD(PHB_CTRLR_TVT_ADDR_SEL, 0ull, TVT_2_PER_PE);
-		val |= PHB_CTRLR_IRQ_STORE_EOI;
+		if (PHB4_CAN_STORE_EOI(p))
+			val |= PHB_CTRLR_IRQ_STORE_EOI;
 	}
 
 	if (!pci_eeh_mmio)
@@ -5027,7 +5031,7 @@  static void phb4_create(struct dt_node *np)
 
 	/* Register all interrupt sources with XIVE */
 	irq_flags = XIVE_SRC_SHIFT_BUG | XIVE_SRC_TRIGGER_PAGE;
-	if (p->rev >= PHB4_REV_NIMBUS_DD20)
+	if (PHB4_CAN_STORE_EOI(p))
 		irq_flags |= XIVE_SRC_STORE_EOI;
 	xive_register_hw_source(p->base_msi, p->num_irqs - 8, 16,
 				p->int_mmio, irq_flags, NULL, NULL);
diff --git a/hw/xive.c b/hw/xive.c
index 468e5b0446e9..a9e578381424 100644
--- a/hw/xive.c
+++ b/hw/xive.c
@@ -486,6 +486,9 @@  struct xive {
 	void		*q_ovf;
 };
 
+#define XIVE_CAN_STORE_EOI(x) \
+	(XIVE_STORE_EOI_ENABLED && ((x)->rev >= XIVE_REV_2))
+
 /* Global DT node */
 static struct dt_node *xive_dt_node;
 
@@ -1750,7 +1753,11 @@  static bool xive_config_init(struct xive *x)
 
 	/* Enable StoreEOI */
 	val = xive_regr(x, VC_SBC_CONFIG);
-	val |= VC_SBC_CONF_CPLX_CIST | VC_SBC_CONF_CIST_BOTH;
+	if (XIVE_CAN_STORE_EOI(x))
+		val |= VC_SBC_CONF_CPLX_CIST | VC_SBC_CONF_CIST_BOTH;
+	else
+		xive_dbg(x, "store EOI is disabled\n");
+
 	val |= VC_SBC_CONF_NO_UPD_PRF;
 	xive_regw(x, VC_SBC_CONFIG, val);
 
@@ -2790,7 +2797,7 @@  void xive_register_ipi_source(uint32_t base, uint32_t count, void *data,
 	assert(s);
 
 	/* Store EOI supported on DD2.0 */
-	if (x->rev >= XIVE_REV_2)
+	if (XIVE_CAN_STORE_EOI(x))
 		flags |= XIVE_SRC_STORE_EOI;
 
 	/* Callbacks assume the MMIO base corresponds to the first
@@ -2898,7 +2905,7 @@  static struct xive *init_one_xive(struct dt_node *np)
 
 	/* Register built-in source controllers (aka IPIs) */
 	flags = XIVE_SRC_EOI_PAGE1 | XIVE_SRC_TRIGGER_PAGE;
-	if (x->rev >= XIVE_REV_2)
+	if (XIVE_CAN_STORE_EOI(x))
 		flags |= XIVE_SRC_STORE_EOI;
 	__xive_register_source(x, &x->ipis, x->int_base,
 			       x->int_hw_bot - x->int_base, IPI_ESB_SHIFT,
diff --git a/include/xive.h b/include/xive.h
index 5262cb4817f3..383e8e5644eb 100644
--- a/include/xive.h
+++ b/include/xive.h
@@ -490,6 +490,9 @@  uint32_t xive_alloc_ipi_irqs(uint32_t chip_id, uint32_t count, uint32_t align);
 uint64_t xive_get_notify_port(uint32_t chip_id, uint32_t ent);
 uint32_t xive_get_notify_base(uint32_t girq);
 
+/* XIVE feature flag to de/activate store EOI */
+#define XIVE_STORE_EOI_ENABLED 0
+
 /* Internal IRQ flags */
 #define XIVE_SRC_TRIGGER_PAGE	0x01 /* Trigger page exist (either separate
 				      * or not, so different from the OPAL