| Message ID | 20220427160223.96758-2-fbarrat@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ppc/xive: Update for guest interrupt handling | expand |
On 4/27/22 18:02, Frederic Barrat wrote: > The Post Interrupt Priority Register (PIPR) is not restored like the > other OS-context related fields of the TIMA when pushing an OS context > on the CPU. It's not needed because it can be calculated from the > Interrupt Pending Buffer (IPB), which is saved and restored. The PIPR > must therefore always be recomputed when pushing an OS context. > > This patch fixes a path on P9 and P10 where it was not done. If there > was a pending interrupt when the OS context was pulled, the IPB was > saved correctly. When pushing back the context, the code in > xive_tctx_need_resend() was checking for a interrupt raised while the > context was not on the CPU, saved in the NVT. If one was found, then > it was merged with the saved IPB and the PIPR updated and everything > was fine. However, if there was no interrupt found in the NVT, then > xive_tctx_ipb_update() was not being called and the PIPR was not > updated. This patch fixes it by always calling xive_tctx_ipb_update(). > > Note that on P10 (xive2.c) and because of the above, there's no longer > any need to check the CPPR value so it can go away. > > Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> Reviewed-by: Cédric Le Goater <clg@kaod.org> One small comment below, > --- > hw/intc/xive.c | 10 +++++++--- > hw/intc/xive2.c | 15 ++++++++------- > 2 files changed, 15 insertions(+), 10 deletions(-) > > diff --git a/hw/intc/xive.c b/hw/intc/xive.c > index b8e4c7294d..6b62918ea7 100644 > --- a/hw/intc/xive.c > +++ b/hw/intc/xive.c > @@ -413,10 +413,14 @@ static void xive_tctx_need_resend(XiveRouter *xrtr, XiveTCTX *tctx, > /* Reset the NVT value */ > nvt.w4 = xive_set_field32(NVT_W4_IPB, nvt.w4, 0); > xive_router_write_nvt(xrtr, nvt_blk, nvt_idx, &nvt, 4); > - > - /* Merge in current context */ > - xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb); > } > + /* > + * Always call xive_tctx_ipb_update(). Even if there's no > + * escalation found in the NVT above, The NVT does not contain an escalation. It contains saved state of the registers, among which the IPB representing the pending interrupt priorities. I would say something like: Even if there were no escalation triggered, there could be a pending interrupt which was saved when the context was pulled and that we need to take into account by recalculating the PIPR (which is not saved/restored). Same below. Thanks, C. > there could be a pending > + * interrupt which was saved when the context was pulled and we > + * need the recalculate the PIPR (which is not saved/restored). > + */ > + xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb); > } > > /* > diff --git a/hw/intc/xive2.c b/hw/intc/xive2.c > index 3aff42a69e..2c62f68444 100644 > --- a/hw/intc/xive2.c > +++ b/hw/intc/xive2.c > @@ -316,7 +316,6 @@ static void xive2_tctx_need_resend(Xive2Router *xrtr, XiveTCTX *tctx, > { > Xive2Nvp nvp; > uint8_t ipb; > - uint8_t cppr = 0; > > /* > * Grab the associated thread interrupt context registers in the > @@ -337,7 +336,7 @@ static void xive2_tctx_need_resend(Xive2Router *xrtr, XiveTCTX *tctx, > /* Automatically restore thread context registers */ > if (xive2_router_get_config(xrtr) & XIVE2_VP_SAVE_RESTORE && > do_restore) { > - cppr = xive2_tctx_restore_os_ctx(xrtr, tctx, nvp_blk, nvp_idx, &nvp); > + xive2_tctx_restore_os_ctx(xrtr, tctx, nvp_blk, nvp_idx, &nvp); > } > > ipb = xive_get_field32(NVP2_W2_IPB, nvp.w2); > @@ -345,11 +344,13 @@ static void xive2_tctx_need_resend(Xive2Router *xrtr, XiveTCTX *tctx, > nvp.w2 = xive_set_field32(NVP2_W2_IPB, nvp.w2, 0); > xive2_router_write_nvp(xrtr, nvp_blk, nvp_idx, &nvp, 2); > } > - > - /* An IPB or CPPR change can trigger a resend */ > - if (ipb || cppr) { > - xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb); > - } > + /* > + * Always call xive_tctx_ipb_update(). Even if there's no > + * escalation found in the NVT above, there could be a pending > + * interrupt which was saved when the context was pulled and we > + * need the recalculate the PIPR (which is not saved/restored). > + */ > + xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb); > } > > /*
diff --git a/hw/intc/xive.c b/hw/intc/xive.c index b8e4c7294d..6b62918ea7 100644 --- a/hw/intc/xive.c +++ b/hw/intc/xive.c @@ -413,10 +413,14 @@ static void xive_tctx_need_resend(XiveRouter *xrtr, XiveTCTX *tctx, /* Reset the NVT value */ nvt.w4 = xive_set_field32(NVT_W4_IPB, nvt.w4, 0); xive_router_write_nvt(xrtr, nvt_blk, nvt_idx, &nvt, 4); - - /* Merge in current context */ - xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb); } + /* + * Always call xive_tctx_ipb_update(). Even if there's no + * escalation found in the NVT above, there could be a pending + * interrupt which was saved when the context was pulled and we + * need the recalculate the PIPR (which is not saved/restored). + */ + xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb); } /* diff --git a/hw/intc/xive2.c b/hw/intc/xive2.c index 3aff42a69e..2c62f68444 100644 --- a/hw/intc/xive2.c +++ b/hw/intc/xive2.c @@ -316,7 +316,6 @@ static void xive2_tctx_need_resend(Xive2Router *xrtr, XiveTCTX *tctx, { Xive2Nvp nvp; uint8_t ipb; - uint8_t cppr = 0; /* * Grab the associated thread interrupt context registers in the @@ -337,7 +336,7 @@ static void xive2_tctx_need_resend(Xive2Router *xrtr, XiveTCTX *tctx, /* Automatically restore thread context registers */ if (xive2_router_get_config(xrtr) & XIVE2_VP_SAVE_RESTORE && do_restore) { - cppr = xive2_tctx_restore_os_ctx(xrtr, tctx, nvp_blk, nvp_idx, &nvp); + xive2_tctx_restore_os_ctx(xrtr, tctx, nvp_blk, nvp_idx, &nvp); } ipb = xive_get_field32(NVP2_W2_IPB, nvp.w2); @@ -345,11 +344,13 @@ static void xive2_tctx_need_resend(Xive2Router *xrtr, XiveTCTX *tctx, nvp.w2 = xive_set_field32(NVP2_W2_IPB, nvp.w2, 0); xive2_router_write_nvp(xrtr, nvp_blk, nvp_idx, &nvp, 2); } - - /* An IPB or CPPR change can trigger a resend */ - if (ipb || cppr) { - xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb); - } + /* + * Always call xive_tctx_ipb_update(). Even if there's no + * escalation found in the NVT above, there could be a pending + * interrupt which was saved when the context was pulled and we + * need the recalculate the PIPR (which is not saved/restored). + */ + xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb); } /*
The Post Interrupt Priority Register (PIPR) is not restored like the other OS-context related fields of the TIMA when pushing an OS context on the CPU. It's not needed because it can be calculated from the Interrupt Pending Buffer (IPB), which is saved and restored. The PIPR must therefore always be recomputed when pushing an OS context. This patch fixes a path on P9 and P10 where it was not done. If there was a pending interrupt when the OS context was pulled, the IPB was saved correctly. When pushing back the context, the code in xive_tctx_need_resend() was checking for a interrupt raised while the context was not on the CPU, saved in the NVT. If one was found, then it was merged with the saved IPB and the PIPR updated and everything was fine. However, if there was no interrupt found in the NVT, then xive_tctx_ipb_update() was not being called and the PIPR was not updated. This patch fixes it by always calling xive_tctx_ipb_update(). Note that on P10 (xive2.c) and because of the above, there's no longer any need to check the CPPR value so it can go away. Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> --- hw/intc/xive.c | 10 +++++++--- hw/intc/xive2.c | 15 ++++++++------- 2 files changed, 15 insertions(+), 10 deletions(-)