Re: [PATCH 06/14] ppc/xive2: Process group backlog when updating the CPPR

[Mike Kowal <kowal@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 12453 bytes --]


On 11/18/2024 10:34 PM, Nicholas Piggin wrote:
> On Wed Oct 16, 2024 at 7:13 AM AEST, Michael Kowal wrote:
>> From: Frederic Barrat<fbarrat@linux.ibm.com>
>>
>> When the hypervisor or OS pushes a new value to the CPPR, if the LSMFB
>> value is lower than the new CPPR value, there could be a pending group
>> interrupt in the backlog, so it needs to be scanned.
>>
>> Signed-off-by: Frederic Barrat<fbarrat@linux.ibm.com>
>> Signed-off-by: Michael Kowal<kowal@linux.ibm.com>
>> ---
>>   include/hw/ppc/xive2.h |   4 +
>>   hw/intc/xive.c         |   4 +-
>>   hw/intc/xive2.c        | 173 ++++++++++++++++++++++++++++++++++++++++-
>>   3 files changed, 177 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/hw/ppc/xive2.h b/include/hw/ppc/xive2.h
>> index d88db05687..e61b978f37 100644
>> --- a/include/hw/ppc/xive2.h
>> +++ b/include/hw/ppc/xive2.h
>> @@ -115,6 +115,10 @@ typedef struct Xive2EndSource {
>>    * XIVE2 Thread Interrupt Management Area (POWER10)
>>    */
>>   
>> +void xive2_tm_set_hv_cppr(XivePresenter *xptr, XiveTCTX *tctx,
>> +                          hwaddr offset, uint64_t value, unsigned size);
>> +void xive2_tm_set_os_cppr(XivePresenter *xptr, XiveTCTX *tctx,
>> +                          hwaddr offset, uint64_t value, unsigned size);
>>   void xive2_tm_push_os_ctx(XivePresenter *xptr, XiveTCTX *tctx, hwaddr offset,
>>                              uint64_t value, unsigned size);
>>   uint64_t xive2_tm_pull_os_ctx(XivePresenter *xptr, XiveTCTX *tctx,
>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>> index 8ffcac4f65..2aa6e1fecc 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -603,7 +603,7 @@ static const XiveTmOp xive2_tm_operations[] = {
>>        * MMIOs below 2K : raw values and special operations without side
>>        * effects
>>        */
>> -    { XIVE_TM_OS_PAGE, TM_QW1_OS + TM_CPPR,       1, xive_tm_set_os_cppr,
>> +    { XIVE_TM_OS_PAGE, TM_QW1_OS + TM_CPPR,       1, xive2_tm_set_os_cppr,
>>                                                        NULL },
>>       { XIVE_TM_HV_PAGE, TM_QW1_OS + TM_WORD2,      4, xive2_tm_push_os_ctx,
>>                                                        NULL },
>> @@ -611,7 +611,7 @@ static const XiveTmOp xive2_tm_operations[] = {
>>                                                        NULL },
>>       { XIVE_TM_OS_PAGE, TM_QW1_OS + TM_LGS,        1, xive_tm_set_os_lgs,
>>                                                        NULL },
>> -    { XIVE_TM_HV_PAGE, TM_QW3_HV_PHYS + TM_CPPR,  1, xive_tm_set_hv_cppr,
>> +    { XIVE_TM_HV_PAGE, TM_QW3_HV_PHYS + TM_CPPR,  1, xive2_tm_set_hv_cppr,
>>                                                        NULL },
>>       { XIVE_TM_HV_PAGE, TM_QW3_HV_PHYS + TM_WORD2, 1, xive_tm_vt_push,
>>                                                        NULL },
>> diff --git a/hw/intc/xive2.c b/hw/intc/xive2.c
>> index 7130892482..0c53f71879 100644
>> --- a/hw/intc/xive2.c
>> +++ b/hw/intc/xive2.c
>> @@ -18,6 +18,7 @@
>>   #include "hw/ppc/xive.h"
>>   #include "hw/ppc/xive2.h"
>>   #include "hw/ppc/xive2_regs.h"
>> +#include "trace.h"
>>   
>>   uint32_t xive2_router_get_config(Xive2Router *xrtr)
>>   {
>> @@ -764,6 +765,172 @@ void xive2_tm_push_os_ctx(XivePresenter *xptr, XiveTCTX *tctx,
>>       }
>>   }
>>   
>> +static int xive2_tctx_get_nvp_indexes(XiveTCTX *tctx, uint8_t ring,
>> +                                      uint32_t *nvp_blk, uint32_t *nvp_idx)
>> +{
>> +    uint32_t w2, cam;
>> +
>> +    w2 = xive_tctx_word2(&tctx->regs[ring]);
>> +    switch (ring) {
>> +    case TM_QW1_OS:
>> +        if (!(be32_to_cpu(w2) & TM2_QW1W2_VO)) {
>> +            return -1;
>> +        }
>> +        cam = xive_get_field32(TM2_QW1W2_OS_CAM, w2);
>> +        break;
>> +    case TM_QW2_HV_POOL:
>> +        if (!(be32_to_cpu(w2) & TM2_QW2W2_VP)) {
>> +            return -1;
>> +        }
>> +        cam = xive_get_field32(TM2_QW2W2_POOL_CAM, w2);
>> +        break;
>> +    case TM_QW3_HV_PHYS:
>> +        if (!(be32_to_cpu(w2) & TM2_QW3W2_VT)) {
>> +            return -1;
>> +        }
>> +        cam = xive2_tctx_hw_cam_line(tctx->xptr, tctx);
>> +        break;
>> +    default:
>> +        return -1;
>> +    }
>> +    *nvp_blk = xive2_nvp_blk(cam);
>> +    *nvp_idx = xive2_nvp_idx(cam);
>> +    return 0;
>> +}
>> +
>> +static void xive2_tctx_set_cppr(XiveTCTX *tctx, uint8_t ring, uint8_t cppr)
> Some of the xive1 code kind of has placeholder for group code or routes
> group stuff through to xive2 code, so I wonder if this duplication is
> really necessary or it can just be added to xive1?
>
> I kind of hoped we could unify xive1 and 2 more, but maybe it's too late
> without a lot more work, and all new development is going to go into
> xive2...


I think that ship sailed long before I got involved.  Our other sim 
models are totally independent, gen 1 or gen2.  Nor did we support gen 2 
running n gen 1 mode.  Trying to move much of this function back into 
xive 1 would be difficult and possible break existing  platforms and models.


>
> Okay for now I guess, we could look at unification one day maybe.


You can have Caleb add it to the plan if you still think it would be 
beneficial.  I think it would be a very low priority item. Contact me 
directly to discuss.


>
>> +{
>> +    uint8_t *regs = &tctx->regs[ring];
>> +    Xive2Router *xrtr = XIVE2_ROUTER(tctx->xptr);
>> +    uint8_t old_cppr, backlog_prio, first_group, group_level = 0;
>> +    uint8_t pipr_min, lsmfb_min, ring_min;
>> +    bool group_enabled;
>> +    uint32_t nvp_blk, nvp_idx;
>> +    Xive2Nvp nvp;
>> +    int rc;
>> +
>> +    trace_xive_tctx_set_cppr(tctx->cs->cpu_index, ring,
>> +                             regs[TM_IPB], regs[TM_PIPR],
>> +                             cppr, regs[TM_NSR]);
>> +
>> +    if (cppr > XIVE_PRIORITY_MAX) {
>> +        cppr = 0xff;
>> +    }
>> +
>> +    old_cppr = regs[TM_CPPR];
>> +    regs[TM_CPPR] = cppr;
> If CPPR remains the same, can return early.
>
> If CPPR is being increased, this scanning is not required (a
> redistribution of group interrupt if it became precluded is
> required as noted in the TODO, but no scanning should be needed
> so that TODO should be moved up here).
>
> If there is an interrupt already presented and CPPR is being
> lowered, nothing needs to be done either (because the presented
> interrupt should already be the most favoured).

xive2_tctx_set_cppr() has gone though  a couple of iterations since this 
patch set was done on Oct 2023.   Some of your points above have already 
been addressed and will be included in group5.   For specifics, see the 
following in ponq-4:

     ppc/xive2: PIPR not updated correctly with CPPR updates.
     4de83cd1a9fab774b1ab95aba804afa3c0159ebf


>> +
>> +    /*
>> +     * Recompute the PIPR based on local pending interrupts. It will
>> +     * be adjusted below if needed in case of pending group interrupts.
>> +     */
>> +    pipr_min = xive_ipb_to_pipr(regs[TM_IPB]);
>> +    group_enabled = !!regs[TM_LGS];
>> +    lsmfb_min = (group_enabled) ? regs[TM_LSMFB] : 0xff;
>> +    ring_min = ring;
>> +
>> +    /* PHYS updates also depend on POOL values */
>> +    if (ring == TM_QW3_HV_PHYS) {
>> +        uint8_t *pregs = &tctx->regs[TM_QW2_HV_POOL];
>> +
>> +        /* POOL values only matter if POOL ctx is valid */
>> +        if (pregs[TM_WORD2] & 0x80) {
>> +
>> +            uint8_t pool_pipr = xive_ipb_to_pipr(pregs[TM_IPB]);
>> +            uint8_t pool_lsmfb = pregs[TM_LSMFB];
>> +
>> +            /*
>> +             * Determine highest priority interrupt and
>> +             * remember which ring has it.
>> +             */
>> +            if (pool_pipr < pipr_min) {
>> +                pipr_min = pool_pipr;
>> +                if (pool_pipr < lsmfb_min) {
>> +                    ring_min = TM_QW2_HV_POOL;
>> +                }
>> +            }
>> +
>> +            /* Values needed for group priority calculation */
>> +            if (pregs[TM_LGS] && (pool_lsmfb < lsmfb_min)) {
>> +                group_enabled = true;
>> +                lsmfb_min = pool_lsmfb;
>> +                if (lsmfb_min < pipr_min) {
>> +                    ring_min = TM_QW2_HV_POOL;
>> +                }
>> +            }
>> +        }
>> +    }
>> +    regs[TM_PIPR] = pipr_min;
>> +
>> +    rc = xive2_tctx_get_nvp_indexes(tctx, ring_min, &nvp_blk, &nvp_idx);
>> +    if (rc) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: set CPPR on invalid context\n");
>> +        return;
>> +    }
>> +
>> +    if (cppr < old_cppr) {
>> +        /*
>> +         * FIXME: check if there's a group interrupt being presented
>> +         * and if the new cppr prevents it. If so, then the group
>> +         * interrupt needs to be re-added to the backlog and
>> +         * re-triggered (see re-trigger END info in the NVGC
>> +         * structure)
>> +         */
>> +    }
>> +
>> +    if (group_enabled &&
>> +        lsmfb_min < cppr &&
>> +        lsmfb_min < regs[TM_PIPR]) {
>> +        /*
>> +         * Thread has seen a group interrupt with a higher priority
>> +         * than the new cppr or pending local interrupt. Check the
>> +         * backlog
>> +         */
>> +        if (xive2_router_get_nvp(xrtr, nvp_blk, nvp_idx, &nvp)) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "XIVE: No NVP %x/%x\n",
>> +                          nvp_blk, nvp_idx);
>> +            return;
>> +        }
>> +
>> +        if (!xive2_nvp_is_valid(&nvp)) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid NVP %x/%x\n",
>> +                          nvp_blk, nvp_idx);
>> +            return;
>> +        }
>> +
>> +        first_group = xive_get_field32(NVP2_W0_PGOFIRST, nvp.w0);
>> +        if (!first_group) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid NVP %x/%x\n",
>> +                          nvp_blk, nvp_idx);
>> +            return;
>> +        }
>> +
>> +        backlog_prio = xive2_presenter_backlog_check(tctx->xptr,
>> +                                                     nvp_blk, nvp_idx,
>> +                                                     first_group, &group_level);
>> +        tctx->regs[ring_min + TM_LSMFB] = backlog_prio;
> LSMFB may not be the same as lsmfb_min, so you can't present
> unconditionally.
>
> I think after updating, it should test
>
>    if (lsmfb_min != backlog_prio) {
>        goto scan_again;
>    }
>
> Where scan_again: goes back to recomputing min priorities and scanning.


Ditto from above.  I think...


>
> Thanks,
> Nick
>
>> +        if (backlog_prio != 0xFF) {
>> +            xive2_presenter_backlog_decr(tctx->xptr, nvp_blk, nvp_idx,
>> +                                         backlog_prio, group_level);
>> +            regs[TM_PIPR] = backlog_prio;
>> +        }
>> +    }
>> +    /* CPPR has changed, check if we need to raise a pending exception */
>> +    xive_tctx_notify(tctx, ring_min, group_level);
>> +}
>> +
>> +void xive2_tm_set_hv_cppr(XivePresenter *xptr, XiveTCTX *tctx,
>> +                          hwaddr offset, uint64_t value, unsigned size)
>> +{
>> +    xive2_tctx_set_cppr(tctx, TM_QW3_HV_PHYS, value & 0xff);
>> +}
>> +
>> +void xive2_tm_set_os_cppr(XivePresenter *xptr, XiveTCTX *tctx,
>> +                          hwaddr offset, uint64_t value, unsigned size)
>> +{
>> +    xive2_tctx_set_cppr(tctx, TM_QW1_OS, value & 0xff);
>> +}
>> +
>>   static void xive2_tctx_set_target(XiveTCTX *tctx, uint8_t ring, uint8_t target)
>>   {
>>       uint8_t *regs = &tctx->regs[ring];
>> @@ -934,7 +1101,9 @@ int xive2_presenter_tctx_match(XivePresenter *xptr, XiveTCTX *tctx,
>>   
>>   bool xive2_tm_irq_precluded(XiveTCTX *tctx, int ring, uint8_t priority)
>>   {
>> -    uint8_t *regs = &tctx->regs[ring];
>> +    /* HV_POOL ring uses HV_PHYS NSR, CPPR and PIPR registers */
>> +    uint8_t alt_ring = (ring == TM_QW2_HV_POOL) ? TM_QW3_HV_PHYS : ring;
>> +    uint8_t *alt_regs = &tctx->regs[alt_ring];
>>   
>>       /*
>>        * The xive2_presenter_tctx_match() above tells if there's a match
>> @@ -942,7 +1111,7 @@ bool xive2_tm_irq_precluded(XiveTCTX *tctx, int ring, uint8_t priority)
>>        * priority to know if the thread can take the interrupt now or if
>>        * it is precluded.
>>        */
>> -    if (priority < regs[TM_CPPR]) {
>> +    if (priority < alt_regs[TM_CPPR]) {
>>           return false;
>>       }
>>       return true;
> These last two are logically separate patch for enabling group for POOL?
>

[-- Attachment #2: Type: text/html, Size: 13985 bytes --]