Re: [PATCH] ppc/xive: Save/restore state of the External interrupt

[Cédric Le Goater]

Hello Frederic,

On 4/26/22 12:11, Frederic Barrat wrote:
> When pulling an OS context from a vcpu, we should lower the External
> interrupt if it is pending. Otherwise, it may be delivered in the
> hypervisor context, which is unexpected. It can lead to an infinite
> loop where the hypervisor catches the External exception, looks for an
> interrupt, doesn't find any, exits the exception handler, repeat...

Nice ! I have been chasing this one for a while and couldn't corner it.

Setting an environment for this scenario is a bit painful. I had a
pseries disk image including a nested guest for it. Depending on the
need, I would run the image under KVM (for updates) or under QEMU
PowerNV for dev/tests.

I thne switched to a simpler buildroot env.

> It also means that when pushing a new OS context on a vcpu, we need to
> always check the restored Interrupt Pending Buffer (IPB), potentially
> merge it with any escalation interrupt, and re-apply the External
> interrupt signal if needed.

See below for a proposal to split this patch in 2 or 3 patches.

 
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> ---
>
> Cedric: I'm wondering the reason behind commit
> 8256870ada9379abfd1f5b2c209ad01092dd0904, which makes the PIPR field
> of the OS context read-only.

The comments is related to direct load/store on the TCTX registers.
When using the magic offsets, the logic is different.


> The comment says it is re-evaluated from
> the IPB when pushing a context, but I don't think it's true on P9
> if there's no escalation. It's not a problem on P10 because we always
> re-evaluate the PIPR if CPPR>0.
> In any case, it's no longer an issue with this patch, but I'm
> curious as to why restoring the PIPR was a problem in the first place.
>
>
>   hw/intc/xive.c        | 26 +++++++++++++++++++++++---
>   hw/intc/xive2.c       | 14 ++++++++------
>   include/hw/ppc/xive.h |  1 +
>   3 files changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index b8e4c7294d..8430db687f 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -114,6 +114,21 @@ static void xive_tctx_notify(XiveTCTX *tctx, uint8_t ring)
>       }
>   }
>   
> +void xive_tctx_eval_ext_signal(XiveTCTX *tctx)

I would call it xive_tctx_reset_os_signal()

> +{
> +    uint8_t *regs = &tctx->regs[TM_QW1_OS];
> +
> +    /*
> +     * Used when pulling an OS context.
> +     * Lower the External interrupt if it's pending. It is necessary
> +     * to avoid catching it in the hypervisor context. It should be
> +     * raised again when re-pushing the OS context.
> +     */
> +    if (regs[TM_PIPR] < regs[TM_CPPR]) {

This seems useless. Since we want the signal down always.

> +        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 +403,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_eval_ext_signal(tctx);


These change forcing the signal down when the OS context is pulled
should be in its own patch.

>       return qw1w2;
>   }
>   
> @@ -413,10 +430,13 @@ 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 merge in current context. Even if there's no escalation,
> +     * it will check with the IPB value restored before pushing the OS
> +     * context and set the External interrupt signal if needed.
> +     */
> +    xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb);

That's another patch and I am not sure it is needed.

An IPB value is recorded in the NVT when an interrupt is delivered
while a vCPU is not dispatched. With this change, you would force
the update in any case when the context is pushed.

Is that to close a potential race window on an interrupt being
delivered to a vCPU just before it is dispatched by the HV ?

>   }
>   
>   /*
> diff --git a/hw/intc/xive2.c b/hw/intc/xive2.c
> index 3aff42a69e..b7f1917cd2 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_eval_ext_signal(tctx);
>       return qw1w2;
>   }
>   
> @@ -316,7 +317,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 +337,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);
> @@ -346,10 +346,12 @@ static void xive2_tctx_need_resend(Xive2Router *xrtr, 
> XiveTCTX *tctx,
>           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 merge in current context. Even if there's no escalation,
> +     * it will check with the IPB value restored and set the External
> +     * interrupt signal if needed.
> +     */
> +    xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb);


I guess this fine. The test on the cppr is just an optimisation, if I am
correct. But it needs to be in another patch.

Same previous comment for ibp.

Thanks,

C.

>   }
>   
>   /*
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 126e4e2c3a..91512ed5e6 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_eval_ext_signal(XiveTCTX *tctx);
>   
>   /*
>    * KVM XIVE device helpers