diff mbox

[1/2] hw/phb3: Fix potential race in EOI

Message ID 1455164733-6560-1-git-send-email-mikey@neuling.org
State Accepted
Headers show

Commit Message

Michael Neuling Feb. 11, 2016, 4:25 a.m. UTC
When we EOI we need to clear the present (P) bit in the Interrupt
Vector Cache (IVC).  We must clear P ensuring that any additional
interrupts that come in aren't lost while also maintaining coherency
with the Interrupt Vector Table (IVT).

To do this, the hardware provides a conditional update bit in the
IVC. This bit ensures that generation counts between the IVT and the
IVC updates are synchronised.

Unfortunately we never set this the bit to conditionally update the P
bit in the IVC based on the generation count.  Also, we didn't set
what we wanted the new generation count to be if the update was
successful.

This patch fixes sets both of these.  It also reworks and documents
the code so that mortals may eventually be able to understand this
process.

Signed-off-by: Michael Neuling <mikey@neuling.org>
---
 hw/phb3.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

Comments

Vaibhav Jain April 22, 2016, 7:25 a.m. UTC | #1
Tested with genwqe framework to simulate heavy interrupt traffic and cxl
driver patches at:

https://github.com/ibm-genwqe/genwqe-user
http://patchwork.ozlabs.org/patch/613380/
http://patchwork.ozlabs.org/patch/613381/


Michael Neuling <mikey@neuling.org> writes:
> Signed-off-by: Michael Neuling <mikey@neuling.org>
Tested-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
Andrew Donnellan April 25, 2016, 11:55 p.m. UTC | #2
On 11/02/16 15:25, Michael Neuling wrote:
> When we EOI we need to clear the present (P) bit in the Interrupt
> Vector Cache (IVC).  We must clear P ensuring that any additional
> interrupts that come in aren't lost while also maintaining coherency
> with the Interrupt Vector Table (IVT).
>
> To do this, the hardware provides a conditional update bit in the
> IVC. This bit ensures that generation counts between the IVT and the
> IVC updates are synchronised.
>
> Unfortunately we never set this the bit to conditionally update the P
> bit in the IVC based on the generation count.  Also, we didn't set
> what we wanted the new generation count to be if the update was
> successful.
>
> This patch fixes sets both of these.  It also reworks and documents
> the code so that mortals may eventually be able to understand this
> process.
>
> Signed-off-by: Michael Neuling <mikey@neuling.org>

Tested with an IBM internal CAPI accelerator that hit this issue.

Tested-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
Stewart Smith April 26, 2016, 9:59 p.m. UTC | #3
Michael Neuling <mikey@neuling.org> writes:
> When we EOI we need to clear the present (P) bit in the Interrupt
> Vector Cache (IVC).  We must clear P ensuring that any additional
> interrupts that come in aren't lost while also maintaining coherency
> with the Interrupt Vector Table (IVT).
>
> To do this, the hardware provides a conditional update bit in the
> IVC. This bit ensures that generation counts between the IVT and the
> IVC updates are synchronised.
>
> Unfortunately we never set this the bit to conditionally update the P
> bit in the IVC based on the generation count.  Also, we didn't set
> what we wanted the new generation count to be if the update was
> successful.
>
> This patch fixes sets both of these.  It also reworks and documents
> the code so that mortals may eventually be able to understand this
> process.
>
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> ---
>  hw/phb3.c | 34 +++++++++++++++++++++++++++++-----
>  1 file changed, 29 insertions(+), 5 deletions(-)

Thanks to everybody who solidly tested this patch series.

Now merged to 5.1.x as of c8bea6e01608f77550ef686bb7359094311de810
merged to 5.2.x as of d729ddbfd8cb7b5dc60f336bf7208214c96a3233
and master as of 2fd158b58a8a38a07cde5f0ac354bd127823a0c9
diff mbox

Patch

diff --git a/hw/phb3.c b/hw/phb3.c
index adff5bc..a11f84e 100644
--- a/hw/phb3.c
+++ b/hw/phb3.c
@@ -1185,7 +1185,7 @@  static int64_t phb3_pci_msi_eoi(struct phb *phb,
 	struct phb3 *p = phb_to_phb3(phb);
 	uint32_t ive_num = PHB3_IRQ_NUM(hwirq);
 	uint64_t ive, ivc;
-	uint8_t *p_byte, gp, gen;
+	uint8_t *p_byte, gp, gen, newgen;
 
 	/* OS might not configure IVT yet */
 	if (!p->tbl_ivt)
@@ -1197,16 +1197,40 @@  static int64_t phb3_pci_msi_eoi(struct phb *phb,
 
 	/* Read generation and P */
 	gp = *p_byte;
-	gen = gp >> 1;
+	gen = (gp >> 1) & 3;
+	newgen = (gen + 1) & 3;
 
 	/* Increment generation count and clear P */
-	*p_byte = ((gen + 1) << 1) & 0x7;
+	*p_byte = newgen << 1;
+
+	/* If at this point:
+	 *   - the IVC is invalid (due to high IRQ load) and
+	 *   - we get a new interrupt on this hwirq.
+	 * Due to the new interrupt, the IVC will fetch from the IVT.
+	 * This IVC reload will result in P set and gen=n+1.  This
+	 * interrupt may not actually be delievered at this point
+	 * though.
+	 *
+	 * Software will then try to clear P in the IVC (out_be64
+	 * below).  This could cause an interrupt to be lost because P
+	 * is cleared in the IVC without the new interrupt being
+	 * delivered.
+	 *
+	 * To avoid this race, we increment the generation count in
+	 * the IVT when we clear P. When software writes the IVC with
+	 * P cleared but with gen=n, the IVC won't actually clear P
+	 * becuase gen doesn't match what it just cached from the IVT.
+	 * Hence we don't lose P being set.
+	 */
 
-	/* Update the IVC with a match against the old gen count */
+	/* Update the P bit in the IVC is gen count matches */
 	ivc = SETFIELD(PHB_IVC_UPDATE_SID, 0ul, ive_num) |
 		PHB_IVC_UPDATE_ENABLE_P |
 		PHB_IVC_UPDATE_ENABLE_GEN |
-		SETFIELD(PHB_IVC_UPDATE_GEN_MATCH, 0ul, gen);
+		PHB_IVC_UPDATE_ENABLE_CON |
+		SETFIELD(PHB_IVC_UPDATE_GEN_MATCH, 0ul, gen) |
+		SETFIELD(PHB_IVC_UPDATE_GEN, 0ul, newgen);
+	/* out_be64 has a sync to order with the IVT update above */
 	out_be64(p->regs + PHB_IVC_UPDATE, ivc);
 
 	/* Handle Q bit */