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