| Message ID | 20220427160223.96758-3-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: > When pulling or pushing an OS context from/to a CPU, we should > re-evaluate the state of the External interrupt signal. Otherwise, we > can end up catching the External interrupt exception in hypervisor > mode, which is unexpected. > > The problem is best illustrated with the following scenario: > > 1. an External interrupt is raised while the guest is on the CPU. > > 2. before the guest can ack the External interrupt, an hypervisor > interrupt is raised, for example the Hypervisor Decrementer or > Hypervisor Virtualization interrupt. The hypervisor interrupt forces > the guest to exit while the External interrupt is still pending. > > 3. the hypervisor handles the hypervisor interrupt. At this point, the > External interrupt is still pending. So it's very likely to be > delivered while the hypervisor is running. That's unexpected and can > result in an infinite loop where the hypervisor catches the External > interrupt, looks for an interrupt in its hypervisor queue, doesn't > find any, exits the interrupt handler with the External interrupt > still raised, repeat... > > The fix is simply to always lower the External interrupt signal when > pulling an OS context. It means it needs to be raised again when > re-pushing the OS context. Fortunately, it's already the case, as we > now always call xive_tctx_ipb_update(), which will raise the signal if > needed. > > Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > --- > hw/intc/xive.c | 14 ++++++++++++++ > hw/intc/xive2.c | 2 ++ > include/hw/ppc/xive.h | 1 + > 3 files changed, 17 insertions(+) > > diff --git a/hw/intc/xive.c b/hw/intc/xive.c > index 6b62918ea7..e230b14e94 100644 > --- a/hw/intc/xive.c > +++ b/hw/intc/xive.c > @@ -114,6 +114,17 @@ static void xive_tctx_notify(XiveTCTX *tctx, uint8_t ring) > } > } > > +void xive_tctx_reset_os_signal(XiveTCTX *tctx) > +{ > + /* > + * Lower the External interrupt. Used when pulling an OS > + * context. It is necessary to avoid catching it in the hypervisor > + * context. It should be raised again when re-pushing the OS > + * context. > + */ > + qemu_irq_lower(xive_tctx_output(tctx, TM_QW1_OS)); > +} > + > static void xive_tctx_set_cppr(XiveTCTX *tctx, uint8_t ring, uint8_t cppr) > { > uint8_t *regs = &tctx->regs[ring]; > @@ -388,6 +399,8 @@ static uint64_t xive_tm_pull_os_ctx(XivePresenter *xptr, XiveTCTX *tctx, > /* Invalidate CAM line */ > qw1w2_new = xive_set_field32(TM_QW1W2_VO, qw1w2, 0); > xive_tctx_set_os_cam(tctx, qw1w2_new); > + > + xive_tctx_reset_os_signal(tctx); > return qw1w2; > } > > @@ -419,6 +432,7 @@ static void xive_tctx_need_resend(XiveRouter *xrtr, XiveTCTX *tctx, > * 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). > + * It will also raise the External interrupt signal if needed. > */ > xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb); > } > diff --git a/hw/intc/xive2.c b/hw/intc/xive2.c > index 2c62f68444..173e0120f8 100644 > --- a/hw/intc/xive2.c > +++ b/hw/intc/xive2.c > @@ -269,6 +269,7 @@ uint64_t xive2_tm_pull_os_ctx(XivePresenter *xptr, XiveTCTX *tctx, > xive2_tctx_save_os_ctx(xrtr, tctx, nvp_blk, nvp_idx); > } > > + xive_tctx_reset_os_signal(tctx); > return qw1w2; > } > > @@ -349,6 +350,7 @@ static void xive2_tctx_need_resend(Xive2Router *xrtr, XiveTCTX *tctx, > * 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). > + * It will also raise the External interrupt signal if needed. > */ > xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb); > } > diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h > index 126e4e2c3a..f7eea4ca81 100644 > --- a/include/hw/ppc/xive.h > +++ b/include/hw/ppc/xive.h > @@ -527,6 +527,7 @@ Object *xive_tctx_create(Object *cpu, XivePresenter *xptr, Error **errp); > void xive_tctx_reset(XiveTCTX *tctx); > void xive_tctx_destroy(XiveTCTX *tctx); > void xive_tctx_ipb_update(XiveTCTX *tctx, uint8_t ring, uint8_t ipb); > +void xive_tctx_reset_os_signal(XiveTCTX *tctx); > > /* > * KVM XIVE device helpers
diff --git a/hw/intc/xive.c b/hw/intc/xive.c index 6b62918ea7..e230b14e94 100644 --- a/hw/intc/xive.c +++ b/hw/intc/xive.c @@ -114,6 +114,17 @@ static void xive_tctx_notify(XiveTCTX *tctx, uint8_t ring) } } +void xive_tctx_reset_os_signal(XiveTCTX *tctx) +{ + /* + * Lower the External interrupt. Used when pulling an OS + * context. It is necessary to avoid catching it in the hypervisor + * context. It should be raised again when re-pushing the OS + * context. + */ + qemu_irq_lower(xive_tctx_output(tctx, TM_QW1_OS)); +} + static void xive_tctx_set_cppr(XiveTCTX *tctx, uint8_t ring, uint8_t cppr) { uint8_t *regs = &tctx->regs[ring]; @@ -388,6 +399,8 @@ static uint64_t xive_tm_pull_os_ctx(XivePresenter *xptr, XiveTCTX *tctx, /* Invalidate CAM line */ qw1w2_new = xive_set_field32(TM_QW1W2_VO, qw1w2, 0); xive_tctx_set_os_cam(tctx, qw1w2_new); + + xive_tctx_reset_os_signal(tctx); return qw1w2; } @@ -419,6 +432,7 @@ static void xive_tctx_need_resend(XiveRouter *xrtr, XiveTCTX *tctx, * 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). + * It will also raise the External interrupt signal if needed. */ xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb); } diff --git a/hw/intc/xive2.c b/hw/intc/xive2.c index 2c62f68444..173e0120f8 100644 --- a/hw/intc/xive2.c +++ b/hw/intc/xive2.c @@ -269,6 +269,7 @@ uint64_t xive2_tm_pull_os_ctx(XivePresenter *xptr, XiveTCTX *tctx, xive2_tctx_save_os_ctx(xrtr, tctx, nvp_blk, nvp_idx); } + xive_tctx_reset_os_signal(tctx); return qw1w2; } @@ -349,6 +350,7 @@ static void xive2_tctx_need_resend(Xive2Router *xrtr, XiveTCTX *tctx, * 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). + * It will also raise the External interrupt signal if needed. */ xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb); } diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h index 126e4e2c3a..f7eea4ca81 100644 --- a/include/hw/ppc/xive.h +++ b/include/hw/ppc/xive.h @@ -527,6 +527,7 @@ Object *xive_tctx_create(Object *cpu, XivePresenter *xptr, Error **errp); void xive_tctx_reset(XiveTCTX *tctx); void xive_tctx_destroy(XiveTCTX *tctx); void xive_tctx_ipb_update(XiveTCTX *tctx, uint8_t ring, uint8_t ipb); +void xive_tctx_reset_os_signal(XiveTCTX *tctx); /* * KVM XIVE device helpers
When pulling or pushing an OS context from/to a CPU, we should re-evaluate the state of the External interrupt signal. Otherwise, we can end up catching the External interrupt exception in hypervisor mode, which is unexpected. The problem is best illustrated with the following scenario: 1. an External interrupt is raised while the guest is on the CPU. 2. before the guest can ack the External interrupt, an hypervisor interrupt is raised, for example the Hypervisor Decrementer or Hypervisor Virtualization interrupt. The hypervisor interrupt forces the guest to exit while the External interrupt is still pending. 3. the hypervisor handles the hypervisor interrupt. At this point, the External interrupt is still pending. So it's very likely to be delivered while the hypervisor is running. That's unexpected and can result in an infinite loop where the hypervisor catches the External interrupt, looks for an interrupt in its hypervisor queue, doesn't find any, exits the interrupt handler with the External interrupt still raised, repeat... The fix is simply to always lower the External interrupt signal when pulling an OS context. It means it needs to be raised again when re-pushing the OS context. Fortunately, it's already the case, as we now always call xive_tctx_ipb_update(), which will raise the signal if needed. Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> --- hw/intc/xive.c | 14 ++++++++++++++ hw/intc/xive2.c | 2 ++ include/hw/ppc/xive.h | 1 + 3 files changed, 17 insertions(+)