hw/phb3: Fix potential race in EOI

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-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
Tested-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
Signed-off-by: Stewart Smith <stewart@linux.vnet.ibm.com>

1 files changed, 29 insertions, 5 deletions

diff --git a/hw/phb3.c b/hw/phb3.c
index f20f5f84..11fcf393 100644
--- a/hw/phb3.c
+++ b/hw/phb3.c
@@ -1100,7 +1100,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)
@@ -1112,16 +1112,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 */