Re: [PATCH v2 09/14] ppc/xive2: Add support for MMIO operations on the NVPG/NVC BAR

["Nicholas Piggin" <npiggin@gmail.com>]

On Tue Dec 10, 2024 at 10:05 AM AEST, Michael Kowal wrote:
> From: Frederic Barrat <fbarrat@linux.ibm.com>
>
> Add support for the NVPG and NVC BARs.  Access to the BAR pages will
> cause backlog counter operations to either increment or decriment
> the counter.
>
> Also added qtests for the same.
>
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> Signed-off-by: Michael Kowal <kowal@linux.ibm.com>
> ---
>  include/hw/ppc/xive2.h           |   9 ++
>  include/hw/ppc/xive2_regs.h      |   3 +
>  tests/qtest/pnv-xive2-common.h   |   1 +
>  hw/intc/pnv_xive2.c              |  80 +++++++++++++---
>  hw/intc/xive2.c                  |  87 +++++++++++++++++
>  tests/qtest/pnv-xive2-nvpg_bar.c | 154 +++++++++++++++++++++++++++++++
>  tests/qtest/pnv-xive2-test.c     |   3 +
>  hw/intc/trace-events             |   4 +
>  tests/qtest/meson.build          |   3 +-
>  9 files changed, 329 insertions(+), 15 deletions(-)
>  create mode 100644 tests/qtest/pnv-xive2-nvpg_bar.c
>
> diff --git a/include/hw/ppc/xive2.h b/include/hw/ppc/xive2.h
> index fc7422fea7..c07e23e1d3 100644
> --- a/include/hw/ppc/xive2.h
> +++ b/include/hw/ppc/xive2.h
> @@ -90,6 +90,15 @@ int xive2_presenter_tctx_match(XivePresenter *xptr, XiveTCTX *tctx,
>                                 uint8_t nvt_blk, uint32_t nvt_idx,
>                                 bool cam_ignore, uint32_t logic_serv);
>  
> +uint64_t xive2_presenter_nvp_backlog_op(XivePresenter *xptr,
> +                                        uint8_t blk, uint32_t idx,
> +                                        uint16_t offset);
> +
> +uint64_t xive2_presenter_nvgc_backlog_op(XivePresenter *xptr,
> +                                         bool crowd,
> +                                         uint8_t blk, uint32_t idx,
> +                                         uint16_t offset, uint16_t val);
> +
>  /*
>   * XIVE2 END ESBs  (POWER10)
>   */
> diff --git a/include/hw/ppc/xive2_regs.h b/include/hw/ppc/xive2_regs.h
> index e88d6eab1e..9bcf7a8a6f 100644
> --- a/include/hw/ppc/xive2_regs.h
> +++ b/include/hw/ppc/xive2_regs.h
> @@ -233,4 +233,7 @@ typedef struct Xive2Nvgc {
>  void xive2_nvgc_pic_print_info(Xive2Nvgc *nvgc, uint32_t nvgc_idx,
>                                 GString *buf);
>  
> +#define NVx_BACKLOG_OP            PPC_BITMASK(52, 53)
> +#define NVx_BACKLOG_PRIO          PPC_BITMASK(57, 59)
> +
>  #endif /* PPC_XIVE2_REGS_H */
> diff --git a/tests/qtest/pnv-xive2-common.h b/tests/qtest/pnv-xive2-common.h
> index 9ae34771aa..2077c05ebc 100644
> --- a/tests/qtest/pnv-xive2-common.h
> +++ b/tests/qtest/pnv-xive2-common.h
> @@ -107,5 +107,6 @@ extern void set_end(QTestState *qts, uint32_t index, uint32_t nvp_index,
>  
>  
>  void test_flush_sync_inject(QTestState *qts);
> +void test_nvpg_bar(QTestState *qts);
>  
>  #endif /* TEST_PNV_XIVE2_COMMON_H */
> diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
> index 41b727d1fb..54abfe3947 100644
> --- a/hw/intc/pnv_xive2.c
> +++ b/hw/intc/pnv_xive2.c
> @@ -2202,21 +2202,40 @@ static const MemoryRegionOps pnv_xive2_tm_ops = {
>      },
>  };
>  
> -static uint64_t pnv_xive2_nvc_read(void *opaque, hwaddr offset,
> +static uint64_t pnv_xive2_nvc_read(void *opaque, hwaddr addr,
>                                     unsigned size)
>  {
>      PnvXive2 *xive = PNV_XIVE2(opaque);
> +    XivePresenter *xptr = XIVE_PRESENTER(xive);
> +    uint32_t page = addr >> xive->nvpg_shift;
> +    uint16_t op = addr & 0xFFF;
> +    uint8_t blk = pnv_xive2_block_id(xive);
>  
> -    xive2_error(xive, "NVC: invalid read @%"HWADDR_PRIx, offset);
> -    return -1;
> +    if (size != 2) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid nvc load size %d\n",
> +                      size);
> +        return -1;
> +    }
> +
> +    return xive2_presenter_nvgc_backlog_op(xptr, true, blk, page, op, 1);
>  }
>  
> -static void pnv_xive2_nvc_write(void *opaque, hwaddr offset,
> +static void pnv_xive2_nvc_write(void *opaque, hwaddr addr,
>                                  uint64_t val, unsigned size)
>  {
>      PnvXive2 *xive = PNV_XIVE2(opaque);
> +    XivePresenter *xptr = XIVE_PRESENTER(xive);
> +    uint32_t page = addr >> xive->nvc_shift;
> +    uint16_t op = addr & 0xFFF;
> +    uint8_t blk = pnv_xive2_block_id(xive);
>  
> -    xive2_error(xive, "NVC: invalid write @%"HWADDR_PRIx, offset);
> +    if (size != 1) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid nvc write size %d\n",
> +                      size);
> +        return;
> +    }

It would be nice to convert these accessors to _with_attrs() variants
that can report access errors like this back through the memory
subsystem. I guess that's something for the todo list rather than an
issue with this patch in particular.

> +    /*
> +     * op:
> +     * 0b00 => increment
> +     * 0b01 => decrement
> +     * 0b1- => read
> +     */

Could use define or enum for these like the qtest has...


> +    if (op == 0b00 || op == 0b01) {
> +        if (op == 0b00) {
> +            count += val;
> +        } else {
> +            if (count > val) {
> +                count -= val;
> +            } else {
> +                count = 0;
> +            }
> +        }
> +        xive2_nvgc_set_backlog(&nvgc, priority, count);
> +        xive2_router_write_nvgc(xrtr, crowd, blk, idx, &nvgc);
> +    }
> +    trace_xive_nvgc_backlog_op(crowd, blk, idx, op, priority, old_count);
> +    return old_count;
> +}
> +
> +uint64_t xive2_presenter_nvp_backlog_op(XivePresenter *xptr,
> +                                        uint8_t blk, uint32_t idx,
> +                                        uint16_t offset)
> +{
> +    Xive2Router *xrtr = XIVE2_ROUTER(xptr);
> +    uint8_t priority = GETFIELD(NVx_BACKLOG_PRIO, offset);
> +    uint8_t op = GETFIELD(NVx_BACKLOG_OP, offset);
> +    Xive2Nvp nvp;
> +    uint8_t ipb, old_ipb, rc;
> +
> +    if (xive2_router_get_nvp(xrtr, blk, idx, &nvp)) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: No NVP %x/%x\n", blk, idx);
> +        return -1;
> +    }
> +    if (!xive2_nvp_is_valid(&nvp)) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: Invalid NVP %x/%x\n", blk, idx);
> +        return -1;
> +    }
> +
> +    old_ipb = xive_get_field32(NVP2_W2_IPB, nvp.w2);
> +    ipb = old_ipb;
> +    /*
> +     * op:
> +     * 0b00 => set priority bit
> +     * 0b01 => reset priority bit
> +     * 0b1- => read
> +     */
> +    if (op == 0b00 || op == 0b01) {
> +        if (op == 0b00) {
> +            ipb |= xive_priority_to_ipb(priority);
> +        } else {
> +            ipb &= ~xive_priority_to_ipb(priority);
> +        }
> +        nvp.w2 = xive_set_field32(NVP2_W2_IPB, nvp.w2, ipb);
> +        xive2_router_write_nvp(xrtr, blk, idx, &nvp, 2);
> +    }
> +    rc = !!(old_ipb & xive_priority_to_ipb(priority));
> +    trace_xive_nvp_backlog_op(blk, idx, op, priority, rc);
> +    return rc;
> +}
> +
>  void xive2_eas_pic_print_info(Xive2Eas *eas, uint32_t lisn, GString *buf)
>  {
>      if (!xive2_eas_is_valid(eas)) {
> diff --git a/tests/qtest/pnv-xive2-nvpg_bar.c b/tests/qtest/pnv-xive2-nvpg_bar.c
> new file mode 100644
> index 0000000000..10d4962d1e
> --- /dev/null
> +++ b/tests/qtest/pnv-xive2-nvpg_bar.c
> @@ -0,0 +1,154 @@
> +/*
> + * QTest testcase for PowerNV 10 interrupt controller (xive2)
> + *  - Test NVPG BAR MMIO operations
> + *
> + * Copyright (c) 2024, IBM Corporation.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * later. See the COPYING file in the top-level directory.
> + */
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +
> +#include "pnv-xive2-common.h"
> +
> +#define NVPG_BACKLOG_OP_SHIFT   10
> +#define NVPG_BACKLOG_PRIO_SHIFT 4
> +
> +#define XIVE_PRIORITY_MAX       7
> +
> +enum NVx {
> +    NVP,
> +    NVG,
> +    NVC
> +};
> +
> +typedef enum {
> +    INCR_STORE = 0b100,
> +    INCR_LOAD  = 0b000,
> +    DECR_STORE = 0b101,
> +    DECR_LOAD  = 0b001,
> +    READ_x     = 0b010,
> +    READ_y     = 0b011,
> +} backlog_op;
> +
> +static uint32_t nvpg_backlog_op(QTestState *qts, backlog_op op,
> +                                enum NVx type, uint64_t index,
> +                                uint8_t priority, uint8_t delta)
> +{
> +    uint64_t addr, offset;
> +    uint32_t count = 0;
> +
> +    switch (type) {
> +    case NVP:
> +        addr = XIVE_NVPG_ADDR + (index << (XIVE_PAGE_SHIFT + 1));
> +        break;
> +    case NVG:
> +        addr = XIVE_NVPG_ADDR + (index << (XIVE_PAGE_SHIFT + 1)) +
> +            (1 << XIVE_PAGE_SHIFT);
> +        break;
> +    case NVC:
> +        addr = XIVE_NVC_ADDR + (index << XIVE_PAGE_SHIFT);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    offset = (op & 0b11) << NVPG_BACKLOG_OP_SHIFT;
> +    offset |= priority << NVPG_BACKLOG_PRIO_SHIFT;
> +    if (op >> 2) {
> +        qtest_writeb(qts, addr + offset, delta);
> +    } else {
> +        count = qtest_readw(qts, addr + offset);
> +    }
> +    return count;
> +}
> +
> +void test_nvpg_bar(QTestState *qts)
> +{
> +    uint32_t nvp_target = 0x11;
> +    uint32_t group_target = 0x17; /* size 16 */
> +    uint32_t vp_irq = 33, group_irq = 47;
> +    uint32_t vp_end = 3, group_end = 97;
> +    uint32_t vp_irq_data = 0x33333333;
> +    uint32_t group_irq_data = 0x66666666;
> +    uint8_t vp_priority = 0, group_priority = 5;
> +    uint32_t vp_count[XIVE_PRIORITY_MAX + 1] = { 0 };
> +    uint32_t group_count[XIVE_PRIORITY_MAX + 1] = { 0 };
> +    uint32_t count, delta;
> +    uint8_t i;
> +
> +    printf("# ============================================================\n");
> +    printf("# Testing NVPG BAR operations\n");
> +
> +    set_nvg(qts, group_target, 0);
> +    set_nvp(qts, nvp_target, 0x04);
> +    set_nvp(qts, group_target, 0x04);
> +
> +    /*
> +     * Setup: trigger a VP-specific interrupt and a group interrupt
> +     * so that the backlog counters are initialized to something else
> +     * than 0 for at least one priority level
> +     */
> +    set_eas(qts, vp_irq, vp_end, vp_irq_data);
> +    set_end(qts, vp_end, nvp_target, vp_priority, false /* group */);
> +
> +    set_eas(qts, group_irq, group_end, group_irq_data);
> +    set_end(qts, group_end, group_target, group_priority, true /* group */);
> +
> +    get_esb(qts, vp_irq, XIVE_EOI_PAGE, XIVE_ESB_SET_PQ_00);
> +    set_esb(qts, vp_irq, XIVE_TRIGGER_PAGE, 0, 0);
> +    vp_count[vp_priority]++;
> +
> +    get_esb(qts, group_irq, XIVE_EOI_PAGE, XIVE_ESB_SET_PQ_00);
> +    set_esb(qts, group_irq, XIVE_TRIGGER_PAGE, 0, 0);
> +    group_count[group_priority]++;
> +
> +    /* check the initial counters */
> +    for (i = 0; i <= XIVE_PRIORITY_MAX; i++) {
> +        count = nvpg_backlog_op(qts, READ_x, NVP, nvp_target, i, 0);
> +        g_assert_cmpuint(count, ==, vp_count[i]);
> +
> +        count = nvpg_backlog_op(qts, READ_y, NVG, group_target, i, 0);
> +        g_assert_cmpuint(count, ==, group_count[i]);
> +    }
> +
> +    /* do a few ops on the VP. Counter can only be 0 and 1 */
> +    vp_priority = 2;
> +    delta = 7;
> +    nvpg_backlog_op(qts, INCR_STORE, NVP, nvp_target, vp_priority, delta);
> +    vp_count[vp_priority] = 1;
> +    count = nvpg_backlog_op(qts, INCR_LOAD, NVP, nvp_target, vp_priority, 0);
> +    g_assert_cmpuint(count, ==, vp_count[vp_priority]);
> +    count = nvpg_backlog_op(qts, READ_y, NVP, nvp_target, vp_priority, 0);
> +    g_assert_cmpuint(count, ==, vp_count[vp_priority]);
> +
> +    count = nvpg_backlog_op(qts, DECR_LOAD, NVP, nvp_target, vp_priority, 0);
> +    g_assert_cmpuint(count, ==, vp_count[vp_priority]);
> +    vp_count[vp_priority] = 0;
> +    nvpg_backlog_op(qts, DECR_STORE, NVP, nvp_target, vp_priority, delta);
> +    count = nvpg_backlog_op(qts, READ_x, NVP, nvp_target, vp_priority, 0);
> +    g_assert_cmpuint(count, ==, vp_count[vp_priority]);

It is a bit confusing because NVP ops AFAIKS are set/clear of a priority
bit. But this has inc/dec. The comment is there and set/clear is basically
incrementing/decrementing a saturating 1-bit counter so it makes sense
but might be good to make this terminology match what the model uses.

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

> +
> +    /* do a few ops on the group */
> +    group_priority = 2;
> +    delta = 9;
> +    /* can't go negative */
> +    nvpg_backlog_op(qts, DECR_STORE, NVG, group_target, group_priority, delta);
> +    count = nvpg_backlog_op(qts, READ_y, NVG, group_target, group_priority, 0);
> +    g_assert_cmpuint(count, ==, 0);
> +    nvpg_backlog_op(qts, INCR_STORE, NVG, group_target, group_priority, delta);
> +    group_count[group_priority] += delta;
> +    count = nvpg_backlog_op(qts, INCR_LOAD, NVG, group_target,
> +                            group_priority, delta);
> +    g_assert_cmpuint(count, ==, group_count[group_priority]);
> +    group_count[group_priority]++;
> +
> +    count = nvpg_backlog_op(qts, DECR_LOAD, NVG, group_target,
> +                            group_priority, delta);
> +    g_assert_cmpuint(count, ==,  group_count[group_priority]);
> +    group_count[group_priority]--;
> +    count = nvpg_backlog_op(qts, READ_x, NVG, group_target, group_priority, 0);
> +    g_assert_cmpuint(count, ==, group_count[group_priority]);
> +}
> +
> diff --git a/tests/qtest/pnv-xive2-test.c b/tests/qtest/pnv-xive2-test.c
> index a4d06550ee..a0e9f19313 100644
> --- a/tests/qtest/pnv-xive2-test.c
> +++ b/tests/qtest/pnv-xive2-test.c
> @@ -493,6 +493,9 @@ static void test_xive(void)
>      reset_state(qts);
>      test_flush_sync_inject(qts);
>  
> +    reset_state(qts);
> +    test_nvpg_bar(qts);
> +
>      qtest_quit(qts);
>  }
>  
> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> index 7435728c51..7f362c38b0 100644
> --- a/hw/intc/trace-events
> +++ b/hw/intc/trace-events
> @@ -285,6 +285,10 @@ xive_tctx_tm_read(uint32_t index, uint64_t offset, unsigned int size, uint64_t v
>  xive_presenter_notify(uint8_t nvt_blk, uint32_t nvt_idx, uint8_t ring, uint8_t group_level) "found NVT 0x%x/0x%x ring=0x%x group_level=%d"
>  xive_end_source_read(uint8_t end_blk, uint32_t end_idx, uint64_t addr) "END 0x%x/0x%x @0x%"PRIx64
>  
> +# xive2.c
> +xive_nvp_backlog_op(uint8_t blk, uint32_t idx, uint8_t op, uint8_t priority, uint8_t rc) "NVP 0x%x/0x%x operation=%d priority=%d rc=%d"
> +xive_nvgc_backlog_op(bool c, uint8_t blk, uint32_t idx, uint8_t op, uint8_t priority, uint32_t rc) "NVGC crowd=%d 0x%x/0x%x operation=%d priority=%d rc=%d"
> +
>  # pnv_xive.c
>  pnv_xive_ic_hw_trigger(uint64_t addr, uint64_t val) "@0x%"PRIx64" val=0x%"PRIx64
>  
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index bd41c9da5f..f7da3df24b 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -348,7 +348,8 @@ qtests = {
>    'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
>    'migration-test': migration_files,
>    'pxe-test': files('boot-sector.c'),
> -  'pnv-xive2-test': files('pnv-xive2-common.c', 'pnv-xive2-flush-sync.c'),
> +  'pnv-xive2-test': files('pnv-xive2-common.c', 'pnv-xive2-flush-sync.c',
> +                          'pnv-xive2-nvpg_bar.c'),
>    'qos-test': [chardev, io, qos_test_ss.apply({}).sources()],
>    'tpm-crb-swtpm-test': [io, tpmemu_files],
>    'tpm-crb-test': [io, tpmemu_files],