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

[PATCH 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.


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        | 24 +++++++++++++++++++++---
 hw/intc/xive2.c       | 17 ++++++++++-------
 include/hw/ppc/xive.h |  1 +
 3 files changed, 32 insertions(+), 10 deletions(-)

-- 
2.35.1

[PATCH 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  | 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, 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);
 }
 
 /*
-- 
2.35.1

[PATCH 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.

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 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
-- 
2.35.1

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

[Cédric Le Goater]

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);
>   }
>   
>   /*

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

[Cédric Le Goater]

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