[PATCH v2 0/2] ppc/xive: Update for guest interrupt handling

[PATCH v2 0/2] ppc/xive: Update for guest interrupt handling

[Frederic Barrat]

This short series fixes a couple of issues about interrupt handling
found when running a KVM guest on the powernv9 and powernv10 models. I
split a patch I previously sent.

Changlog:
v2:
 - update comment on patch 1
 - picked up Cedric's reviewed-by on patch 2

Frederic Barrat (2):
  ppc/xive: Always recompute the PIPR when pushing an OS context
  ppc/xive: Update the state of the External interrupt signal

 hw/intc/xive.c        | 25 ++++++++++++++++++++++---
 hw/intc/xive2.c       | 18 +++++++++++-------
 include/hw/ppc/xive.h |  1 +
 3 files changed, 34 insertions(+), 10 deletions(-)

-- 
2.35.1

[PATCH v2 1/2] ppc/xive: Always recompute the PIPR when pushing an OS context

[Frederic Barrat]

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  | 11 ++++++++---
 hw/intc/xive2.c | 16 +++++++++-------
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index b8e4c7294d..c729f6a478 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -413,10 +413,15 @@ 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 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).
+     */
+    xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb);
 }
 
 /*
diff --git a/hw/intc/xive2.c b/hw/intc/xive2.c
index 3aff42a69e..400fd70aa8 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,14 @@ 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 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).
+     */
+    xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb);
 }
 
 /*
-- 
2.35.1

[PATCH v2 2/2] ppc/xive: Update the state of the External interrupt signal

[Frederic Barrat]

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.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
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(+)

diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index c729f6a478..ae221fed73 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;
 }
 
@@ -420,6 +433,7 @@ static void xive_tctx_need_resend(XiveRouter *xrtr, XiveTCTX *tctx,
      * was saved when the context was pulled and that we need to take
      * into account by recalculating 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 400fd70aa8..4d9ff41956 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;
 }
 
@@ -350,6 +351,7 @@ static void xive2_tctx_need_resend(Xive2Router *xrtr, XiveTCTX *tctx,
      * was saved when the context was pulled and that we need to take
      * into account by recalculating 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
-- 
2.35.1

Re: [PATCH v2 1/2] ppc/xive: Always recompute the PIPR when pushing an OS context

[Cédric Le Goater]

On 4/29/22 09:16, 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.
> 

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks for the update,

C.

> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> ---
>   hw/intc/xive.c  | 11 ++++++++---
>   hw/intc/xive2.c | 16 +++++++++-------
>   2 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index b8e4c7294d..c729f6a478 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -413,10 +413,15 @@ 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 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).
> +     */
> +    xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb);
>   }
>   
>   /*
> diff --git a/hw/intc/xive2.c b/hw/intc/xive2.c
> index 3aff42a69e..400fd70aa8 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,14 @@ 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 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).
> +     */
> +    xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb);
>   }
>   
>   /*

Re: [PATCH v2 0/2] ppc/xive: Update for guest interrupt handling

[Daniel Henrique Barboza]

On 4/29/22 04:16, Frederic Barrat wrote:
> This short series fixes a couple of issues about interrupt handling
> found when running a KVM guest on the powernv9 and powernv10 models. I
> split a patch I previously sent.
> 
> Changlog:
> v2:
>   - update comment on patch 1
>   - picked up Cedric's reviewed-by on patch 2
> 

Queued in gitlab.com/danielhb/qemu/tree/ppc-next. Thanks,


Daniel

> Frederic Barrat (2):
>    ppc/xive: Always recompute the PIPR when pushing an OS context
>    ppc/xive: Update the state of the External interrupt signal
> 
>   hw/intc/xive.c        | 25 ++++++++++++++++++++++---
>   hw/intc/xive2.c       | 18 +++++++++++-------
>   include/hw/ppc/xive.h |  1 +
>   3 files changed, 34 insertions(+), 10 deletions(-)
>