Re: [PATCH v2 02/14] ppc/xive2: Add grouping level to notification

[Nicholas Piggin]

On Tue Dec 10, 2024 at 10:05 AM AEST, Michael Kowal wrote:
> From: Frederic Barrat <fbarrat@linux.ibm.com>
>
> The NSR has a (so far unused) grouping level field. When a interrupt
> is presented, that field tells the hypervisor or OS if the interrupt
> is for an individual VP or for a VP-group/crowd. This patch reworks
> the presentation API to allow to set/unset the level when
> raising/accepting an interrupt.
>
> It also renames xive_tctx_ipb_update() to xive_tctx_pipr_update() as
> the IPB is only used for VP-specific target, whereas the PIPR always
> needs to be updated.
>
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> Signed-off-by: Michael Kowal <kowal@linux.ibm.com>
> ---

[...]

> @@ -495,16 +502,20 @@ 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);
> -    }
> +
> +        uint8_t *regs = &tctx->regs[TM_QW1_OS];
> +        regs[TM_IPB] |= ipb;
> +}

Indentation bug here.

> +
>      /*
> -     * Always call xive_tctx_ipb_update(). Even if there were no
> +     * Always call xive_tctx_pipr_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).
>       * It will also raise the External interrupt signal if needed.
>       */
> -    xive_tctx_ipb_update(tctx, TM_QW1_OS, ipb);
> +    xive_tctx_pipr_update(tctx, TM_QW1_OS, 0xFF, 0); /* fxb */
>  }
>  
>  /*

[...]

> @@ -594,15 +596,15 @@ 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);
>      }
> +    regs[TM_IPB] = ipb;
> +    backlog_prio = xive_ipb_to_pipr(ipb);
> +    backlog_level = 0;

There is also a bug here, ipb should be OR'ed into the IPB
reg (as xive1 code above does).

We have this fixed up in our internal tree, I have have just
folded that fix in here (Mike is on vacation so I've been trying
to help wrangle the xive patches...).

Otherwise I think it looks good.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>