Re: [PATCH v4 1/9] qtest/phb4: Add testbench for PHB4

[Harsh Prateek Bora <harshpb@linux.ibm.com>]

<Adding folks working in this area or who might be interested in it>

On 05/03/26 11:39 am, Saif Abrar wrote:
> New qtest testbench added for PHB[345].
> Testbench reads PHB Version register and asserts that
> bits[24:31] have value 0xA3, 0xA4 and 0xA5 respectively.
> 
> Signed-off-by: Saif Abrar <saif.abrar@linux.vnet.ibm.com>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> v3: Updates for coding guidelines.
> v2: Added version check for PHB3 and PHB4 also.
> 
>   tests/qtest/meson.build     |  1 +
>   tests/qtest/pnv-phb4-test.c | 98 +++++++++++++++++++++++++++++++++++++
>   2 files changed, 99 insertions(+)
>   create mode 100644 tests/qtest/pnv-phb4-test.c
> 
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index ba9f59d2f8..030f3f4fc6 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -182,6 +182,7 @@ qtests_ppc64 = \
>     (config_all_devices.has_key('CONFIG_POWERNV') ? ['pnv-xive2-test'] : []) +                 \
>     (config_all_devices.has_key('CONFIG_POWERNV') ? ['pnv-spi-seeprom-test'] : []) +           \
>     (config_all_devices.has_key('CONFIG_POWERNV') ? ['pnv-host-i2c-test'] : []) +              \
> +  (config_all_devices.has_key('CONFIG_POWERNV') ? ['pnv-phb4-test'] : []) +                  \

Should we just call it pnv-phb-test as it takes care of all PHB 3/4/5.

Also, need to audit all the patches whether to keep the naming as just 
phb or phb{3,4,5} as applicable.


>     (config_all_devices.has_key('CONFIG_PSERIES') ? ['numa-test'] : []) +                      \
>     (config_all_devices.has_key('CONFIG_PSERIES') ? ['rtas-test'] : []) +                      \
>     (slirp.found() ? ['pxe-test'] : []) +              \
> diff --git a/tests/qtest/pnv-phb4-test.c b/tests/qtest/pnv-phb4-test.c
> new file mode 100644
> index 0000000000..797f6b6c87
> --- /dev/null
> +++ b/tests/qtest/pnv-phb4-test.c
> @@ -0,0 +1,98 @@
> +/*
> + * QTest testcase for PowerNV PHB4
> + *
> + * Copyright (c) 2025, IBM Corporation.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "hw/pci-host/pnv_phb4_regs.h"
> +#include "pnv-xscom.h"
> +
> +#define PPC_BIT(bit)            (0x8000000000000000ULL >> (bit))
> +#define PPC_BITMASK(bs, be)     ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))

Looks like we already have 2 files in tests/qtest folder which contains 
these macros. Time to cleanup common code into a common header ?
> +
> +#define PHB3_PBCQ_SPCI_ASB_ADDR      0x0
> +#define PHB3_PBCQ_SPCI_ASB_DATA      0x2
> +
> +/* Index of PNV_CHIP_POWER10 in pnv_chips[] within "pnv-xscom.h" */
> +#define PNV_P10_CHIP_INDEX      3

Where is this being used and do we really need it?

> +#define PHB4_XSCOM              0x40084800ull
> +
> +/*
> + * Indirect XSCOM read::
> + * - Write 'Indirect Address Register' with register-offset to read.
> + * - Read 'Indirect Data Register' to get the value.
> + */
> +static uint64_t pnv_phb_xscom_read(QTestState *qts, const PnvChip *chip,
> +        uint64_t scom, uint32_t indirect_addr, uint32_t indirect_data,
> +        uint64_t reg)
> +{
> +    qtest_writeq(qts, pnv_xscom_addr(chip, (scom >> 3) + indirect_addr), reg);

What does bit shift 3 signify here, can we have a self explanatory macro 
for this?

> +    return qtest_readq(qts, pnv_xscom_addr(chip, (scom >> 3) + indirect_data));
> +}
> +
> +/* Assert that 'PHB - Version Register' bits[24:31] are as expected */
> +static void phb_version_test(const void *data)
> +{
> +    const PnvChip *chip = (PnvChip *)data;
> +    QTestState *qts;
> +    const char *machine = "powernv8";
> +    uint64_t phb_xscom = 0x4809e000;
> +    uint64_t reg_phb_version = PHB_VERSION;
> +    uint32_t indirect_addr = PHB3_PBCQ_SPCI_ASB_ADDR;
> +    uint32_t indirect_data = PHB3_PBCQ_SPCI_ASB_DATA;
> +    uint32_t expected_ver = 0xA3;
> +    uint64_t ver;
> +
> +    if (chip->chip_type == PNV_CHIP_POWER9) {
> +        machine = "powernv9";
> +        phb_xscom = 0x68084800;
> +        indirect_addr = PHB_SCOM_HV_IND_ADDR;
> +        indirect_data = PHB_SCOM_HV_IND_DATA;
> +        reg_phb_version |= PPC_BIT(0);
> +        expected_ver = 0xA4;
> +    } else if (chip->chip_type == PNV_CHIP_POWER10) {

What about POWER11? Also, could the check be based on PHB type supported
(need new attribute?) so that we dont need to add checks for every next
PnvChip->chip_type which supports the existing PHB type?

> +        machine = "powernv10";
> +        phb_xscom = PHB4_XSCOM;
> +        indirect_addr = PHB_SCOM_HV_IND_ADDR;
> +        indirect_data = PHB_SCOM_HV_IND_DATA;
> +        reg_phb_version |= PPC_BIT(0);
> +        expected_ver = 0xA5;
> +    }
> +
> +    qts = qtest_initf("-M %s -accel tcg -cpu %s", machine, chip->cpu_model);
> +
> +    ver = pnv_phb_xscom_read(qts, chip, phb_xscom,
> +                                indirect_addr, indirect_data, reg_phb_version);
> +
> +    /* PHB Version register bits [24:31] */
> +    ver = ver >> (63 - 31);

It is always preferable to mask the remaining bits before extracting.

> +    g_assert_cmpuint(ver, ==, expected_ver);
> +
> +    qtest_quit(qts);
> +}
> +
> +/* Verify versions of all supported PHB's */
> +static void add_phbX_version_test(void)
> +{
> +    for (int i = 0; i < ARRAY_SIZE(pnv_chips); i++) {
> +        char *tname = g_strdup_printf("pnv-phb/%s",
> +                                      pnv_chips[i].cpu_model);
> +        qtest_add_data_func(tname, &pnv_chips[i], phb_version_test);
> +        g_free(tname);
> +    }
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    g_test_init(&argc, &argv, NULL);
> +
> +    /* PHB[345] tests */
> +    add_phbX_version_test();
> +
> +    return g_test_run();
> +}