xive: Fix potential for lost IPIs when manipulating CPPR

Message ID 1492729197.25766.156.camel@kernel.crashing.org
State Accepted
Headers show

Commit Message

Benjamin Herrenschmidt April 20, 2017, 10:59 p.m.
We don't trigger the IPI in set_mfrr() if the CPPR of the
destination is more favored than the MFRR. However, we fail
to re-evaluate it when the CPPR later changes.

This fixes it. The way this is implemented can lead to
spurious IPIs but these are harmless.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 hw/xive.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

Comments

Michael Neuling April 21, 2017, 1:27 a.m. | #1
On Fri, 2017-04-21 at 08:59 +1000, Benjamin Herrenschmidt wrote:
> We don't trigger the IPI in set_mfrr() if the CPPR of the
> destination is more favored than the MFRR. However, we fail
> to re-evaluate it when the CPPR later changes.
> 
> This fixes it. The way this is implemented can lead to
> spurious IPIs but these are harmless.
>
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  hw/xive.c | 35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/xive.c b/hw/xive.c
> index 1e2648c..eb75f5f 100644
> --- a/hw/xive.c
> +++ b/hw/xive.c
> @@ -3066,6 +3066,24 @@ static inline uint8_t opal_xive_check_pending(struct
> xive_cpu_state *xs,
>  	return xs->pending & mask;
>  }
> 
> +static void opal_xive_update_cppr(struct xive_cpu_state *xs, u8 cppr)
> +{
> +	/* Peform the update */
> +	xs->cppr = cppr;
> +	out_8(xs->tm_ring1 + TM_QW3_HV_PHYS + TM_CPPR, cppr);
> +
> +	/* Trigger the IPI if it's still more favored than the CPPR
> +	 *
> +	 * This can lead to a bunch of spurrious retriggers if the
> +	 * IPI is queued up behind other interrupts but that's not
> +	 * a big deal and keeps the code simpler
> +	 */
> +	if (xs->mfrr < cppr) {
> +		xive_ipi_trigger(xs->xive, GIRQ_TO_IDX(xs->ipi_irq));
> +		xs->ipi_sent = true;


What tree is this against?  mainline gets this.

	[CC]  hw/xive.o
hw/xive.c: In function ‘opal_xive_update_cppr’:
hw/xive.c:3083:5: error: ‘struct xive_cpu_state’ has no member named ‘ipi_sent’;
did you mean ‘ipi_irq’?
   xs->ipi_sent = true;
     ^~




> +	}
> +}
> +
>  static int64_t opal_xive_eoi(uint32_t xirr)
>  {
>  	struct cpu_thread *c = this_cpu();
> @@ -3150,13 +3168,6 @@ static int64_t opal_xive_eoi(uint32_t xirr)
>  		/* Is it an IPI ? */
>  		if (special_ipi) {
>  			xive_ipi_eoi(src_x, idx);
> -
> -			/* Check mfrr and eventually re-trigger. We check
> -			 * against the new CPPR since we are about to update
> -			 * the HW.
> -			 */
> -			if (xs->mfrr < cppr)
> -				xive_ipi_trigger(src_x, idx);
>  		} else {
>  			/* Otherwise go through the source mechanism */
>  			xive_vdbg(src_x, "EOI of IDX %x in EXT range\n",
> idx);
> @@ -3167,8 +3178,7 @@ static int64_t opal_xive_eoi(uint32_t xirr)
>  	}
> 
>  	/* Finally restore CPPR */
> -	xs->cppr = cppr;
> -	out_8(xs->tm_ring1 + TM_QW3_HV_PHYS + TM_CPPR, cppr);
> +	opal_xive_update_cppr(xs, cppr);
> 
>  	xive_cpu_vdbg(c, "  pending=0x%x cppr=%d\n", xs->pending, cppr);
> 
> @@ -3297,8 +3307,7 @@ static int64_t opal_xive_get_xirr(uint32_t *out_xirr,
> bool just_poll)
> 
>  	} else {
>  		/* Nothing was active, this is a fluke, restore CPPR */
> -		xs->cppr = old_cppr;
> -		out_8(xs->tm_ring1 + TM_QW3_HV_PHYS + TM_CPPR, old_cppr);
> +		opal_xive_update_cppr(xs, old_cppr);
>  		xive_cpu_vdbg(c, "  nothing active, restored CPPR to %d\n",
>  			      old_cppr);
>  	}
> @@ -3328,9 +3337,7 @@ static int64_t opal_xive_set_cppr(uint8_t cppr)
>  	xive_cpu_vdbg(c, "CPPR setting to %d\n", cppr);
> 
>  	lock(&xs->lock);
> -	c->xstate->cppr = cppr;
> -	out_8(xs->tm_ring1 + TM_QW3_HV_PHYS + TM_CPPR, cppr);
> -
> +	opal_xive_update_cppr(xs, cppr);
>  	unlock(&xs->lock);
> 
>  	return OPAL_SUCCESS;
>
Michael Neuling April 21, 2017, 2 a.m. | #2
This is upstream as of 50e1921f98


On Fri, 2017-04-21 at 08:59 +1000, Benjamin Herrenschmidt wrote:
> We don't trigger the IPI in set_mfrr() if the CPPR of the
> destination is more favored than the MFRR. However, we fail
> to re-evaluate it when the CPPR later changes.
> 
> This fixes it. The way this is implemented can lead to
> spurious IPIs but these are harmless.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  hw/xive.c | 35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/xive.c b/hw/xive.c
> index 1e2648c..eb75f5f 100644
> --- a/hw/xive.c
> +++ b/hw/xive.c
> @@ -3066,6 +3066,24 @@ static inline uint8_t opal_xive_check_pending(struct
> xive_cpu_state *xs,
>  	return xs->pending & mask;
>  }
>  
> +static void opal_xive_update_cppr(struct xive_cpu_state *xs, u8 cppr)
> +{
> +	/* Peform the update */
> +	xs->cppr = cppr;
> +	out_8(xs->tm_ring1 + TM_QW3_HV_PHYS + TM_CPPR, cppr);
> +
> +	/* Trigger the IPI if it's still more favored than the CPPR
> +	 *
> +	 * This can lead to a bunch of spurrious retriggers if the
> +	 * IPI is queued up behind other interrupts but that's not
> +	 * a big deal and keeps the code simpler
> +	 */
> +	if (xs->mfrr < cppr) {
> +		xive_ipi_trigger(xs->xive, GIRQ_TO_IDX(xs->ipi_irq));
> +		xs->ipi_sent = true;
> +	}
> +}
> +
>  static int64_t opal_xive_eoi(uint32_t xirr)
>  {
>  	struct cpu_thread *c = this_cpu();
> @@ -3150,13 +3168,6 @@ static int64_t opal_xive_eoi(uint32_t xirr)
>  		/* Is it an IPI ? */
>  		if (special_ipi) {
>  			xive_ipi_eoi(src_x, idx);
> -
> -			/* Check mfrr and eventually re-trigger. We check
> -			 * against the new CPPR since we are about to update
> -			 * the HW.
> -			 */
> -			if (xs->mfrr < cppr)
> -				xive_ipi_trigger(src_x, idx);
>  		} else {
>  			/* Otherwise go through the source mechanism */
>  			xive_vdbg(src_x, "EOI of IDX %x in EXT range\n",
> idx);
> @@ -3167,8 +3178,7 @@ static int64_t opal_xive_eoi(uint32_t xirr)
>  	}
>  
>  	/* Finally restore CPPR */
> -	xs->cppr = cppr;
> -	out_8(xs->tm_ring1 + TM_QW3_HV_PHYS + TM_CPPR, cppr);
> +	opal_xive_update_cppr(xs, cppr);
>  
>  	xive_cpu_vdbg(c, "  pending=0x%x cppr=%d\n", xs->pending, cppr);
>  
> @@ -3297,8 +3307,7 @@ static int64_t opal_xive_get_xirr(uint32_t *out_xirr,
> bool just_poll)
>  
>  	} else {
>  		/* Nothing was active, this is a fluke, restore CPPR */
> -		xs->cppr = old_cppr;
> -		out_8(xs->tm_ring1 + TM_QW3_HV_PHYS + TM_CPPR, old_cppr);
> +		opal_xive_update_cppr(xs, old_cppr);
>  		xive_cpu_vdbg(c, "  nothing active, restored CPPR to %d\n",
>  			      old_cppr);
>  	}
> @@ -3328,9 +3337,7 @@ static int64_t opal_xive_set_cppr(uint8_t cppr)
>  	xive_cpu_vdbg(c, "CPPR setting to %d\n", cppr);
>  
>  	lock(&xs->lock);
> -	c->xstate->cppr = cppr;
> -	out_8(xs->tm_ring1 + TM_QW3_HV_PHYS + TM_CPPR, cppr);
> -
> +	opal_xive_update_cppr(xs, cppr);
>  	unlock(&xs->lock);
>  
>  	return OPAL_SUCCESS;
> 
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot

Patch

diff --git a/hw/xive.c b/hw/xive.c
index 1e2648c..eb75f5f 100644
--- a/hw/xive.c
+++ b/hw/xive.c
@@ -3066,6 +3066,24 @@  static inline uint8_t opal_xive_check_pending(struct xive_cpu_state *xs,
 	return xs->pending & mask;
 }
 
+static void opal_xive_update_cppr(struct xive_cpu_state *xs, u8 cppr)
+{
+	/* Peform the update */
+	xs->cppr = cppr;
+	out_8(xs->tm_ring1 + TM_QW3_HV_PHYS + TM_CPPR, cppr);
+
+	/* Trigger the IPI if it's still more favored than the CPPR
+	 *
+	 * This can lead to a bunch of spurrious retriggers if the
+	 * IPI is queued up behind other interrupts but that's not
+	 * a big deal and keeps the code simpler
+	 */
+	if (xs->mfrr < cppr) {
+		xive_ipi_trigger(xs->xive, GIRQ_TO_IDX(xs->ipi_irq));
+		xs->ipi_sent = true;
+	}
+}
+
 static int64_t opal_xive_eoi(uint32_t xirr)
 {
 	struct cpu_thread *c = this_cpu();
@@ -3150,13 +3168,6 @@  static int64_t opal_xive_eoi(uint32_t xirr)
 		/* Is it an IPI ? */
 		if (special_ipi) {
 			xive_ipi_eoi(src_x, idx);
-
-			/* Check mfrr and eventually re-trigger. We check
-			 * against the new CPPR since we are about to update
-			 * the HW.
-			 */
-			if (xs->mfrr < cppr)
-				xive_ipi_trigger(src_x, idx);
 		} else {
 			/* Otherwise go through the source mechanism */
 			xive_vdbg(src_x, "EOI of IDX %x in EXT range\n", idx);
@@ -3167,8 +3178,7 @@  static int64_t opal_xive_eoi(uint32_t xirr)
 	}
 
 	/* Finally restore CPPR */
-	xs->cppr = cppr;
-	out_8(xs->tm_ring1 + TM_QW3_HV_PHYS + TM_CPPR, cppr);
+	opal_xive_update_cppr(xs, cppr);
 
 	xive_cpu_vdbg(c, "  pending=0x%x cppr=%d\n", xs->pending, cppr);
 
@@ -3297,8 +3307,7 @@  static int64_t opal_xive_get_xirr(uint32_t *out_xirr, bool just_poll)
 
 	} else {
 		/* Nothing was active, this is a fluke, restore CPPR */
-		xs->cppr = old_cppr;
-		out_8(xs->tm_ring1 + TM_QW3_HV_PHYS + TM_CPPR, old_cppr);
+		opal_xive_update_cppr(xs, old_cppr);
 		xive_cpu_vdbg(c, "  nothing active, restored CPPR to %d\n",
 			      old_cppr);
 	}
@@ -3328,9 +3337,7 @@  static int64_t opal_xive_set_cppr(uint8_t cppr)
 	xive_cpu_vdbg(c, "CPPR setting to %d\n", cppr);
 
 	lock(&xs->lock);
-	c->xstate->cppr = cppr;
-	out_8(xs->tm_ring1 + TM_QW3_HV_PHYS + TM_CPPR, cppr);
-
+	opal_xive_update_cppr(xs, cppr);
 	unlock(&xs->lock);
 
 	return OPAL_SUCCESS;