Thanks for the review! I have updated the patch accordingly and verified the v3 version passes the test. Will send it out shortly.
Best regards, Jiakai Xu > -----Original Messages----- > From: "Andrew Jones" <[email protected]> > Sent Time: 2026-03-10 05:08:22 (Tuesday) > To: "Jiakai Xu" <[email protected]> > Cc: [email protected], [email protected], "Shuah > Khan" <[email protected]>, "Paolo Bonzini" <[email protected]>, "Andrew > Jones" <[email protected]>, "Alexandre Ghiti" <[email protected]>, "Albert > Ou" <[email protected]>, "Palmer Dabbelt" <[email protected]>, "Paul > Walmsley" <[email protected]>, "Atish Patra" <[email protected]>, "Anup > Patel" <[email protected]>, "Jiakai Xu" <[email protected]> > Subject: Re: [PATCH v2 2/2] RISC-V: KVM: selftests: Fix firmware counter read > in sbi_pmu_test > > On Sun, Mar 08, 2026 at 02:38:03AM +0000, Jiakai Xu wrote: > > The current sbi_pmu_test attempts to read firmware counters without > > configuring them first with SBI_EXT_PMU_COUNTER_CFG_MATCH. > > > > Previously this did not fail because KVM incorrectly allowed the read > > and accessed fw_event[] with an out-of-bounds index when the counter > > was unconfigured. After fixing that bug, the read now correctly returns > > SBI_ERR_INVALID_PARAM, causing the selftest to fail. > > > > Update the test to configure a firmware event before reading the > > counter. Also add a negative test to ensure that attempting to read an > > unconfigured firmware counter fails gracefully. > > > > Signed-off-by: Jiakai Xu <[email protected]> > > Signed-off-by: Jiakai Xu <[email protected]> > > --- > > .../testing/selftests/kvm/include/riscv/sbi.h | 28 +++++++++++++++++++ > > .../selftests/kvm/riscv/sbi_pmu_test.c | 16 ++++++++++- > > 2 files changed, 43 insertions(+), 1 deletion(-) > > > > diff --git a/tools/testing/selftests/kvm/include/riscv/sbi.h > > b/tools/testing/selftests/kvm/include/riscv/sbi.h > > index 046b432ae896..8c172422f386 100644 > > --- a/tools/testing/selftests/kvm/include/riscv/sbi.h > > +++ b/tools/testing/selftests/kvm/include/riscv/sbi.h > > @@ -97,6 +97,34 @@ enum sbi_pmu_hw_generic_events_t { > > SBI_PMU_HW_GENERAL_MAX, > > }; > > > > +enum sbi_pmu_fw_generic_events_t { > > + SBI_PMU_FW_MISALIGNED_LOAD = 0, > > + SBI_PMU_FW_MISALIGNED_STORE = 1, > > + SBI_PMU_FW_ACCESS_LOAD = 2, > > + SBI_PMU_FW_ACCESS_STORE = 3, > > + SBI_PMU_FW_ILLEGAL_INSN = 4, > > + SBI_PMU_FW_SET_TIMER = 5, > > + SBI_PMU_FW_IPI_SENT = 6, > > + SBI_PMU_FW_IPI_RCVD = 7, > > + SBI_PMU_FW_FENCE_I_SENT = 8, > > + SBI_PMU_FW_FENCE_I_RCVD = 9, > > + SBI_PMU_FW_SFENCE_VMA_SENT = 10, > > + SBI_PMU_FW_SFENCE_VMA_RCVD = 11, > > + SBI_PMU_FW_SFENCE_VMA_ASID_SENT = 12, > > + SBI_PMU_FW_SFENCE_VMA_ASID_RCVD = 13, > > + > > + SBI_PMU_FW_HFENCE_GVMA_SENT = 14, > > + SBI_PMU_FW_HFENCE_GVMA_RCVD = 15, > > + SBI_PMU_FW_HFENCE_GVMA_VMID_SENT = 16, > > + SBI_PMU_FW_HFENCE_GVMA_VMID_RCVD = 17, > > + > > + SBI_PMU_FW_HFENCE_VVMA_SENT = 18, > > + SBI_PMU_FW_HFENCE_VVMA_RCVD = 19, > > + SBI_PMU_FW_HFENCE_VVMA_ASID_SENT = 20, > > + SBI_PMU_FW_HFENCE_VVMA_ASID_RCVD = 21, > > + SBI_PMU_FW_MAX, > > +}; > > + > > /* SBI PMU counter types */ > > enum sbi_pmu_ctr_type { > > SBI_PMU_CTR_TYPE_HW = 0x0, > > diff --git a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c > > b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c > > index 924a335d2262..9b7e28bec435 100644 > > --- a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c > > +++ b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c > > @@ -461,7 +461,21 @@ static void test_pmu_basic_sanity(void) > > pmu_csr_read_num(ctrinfo.csr); > > GUEST_ASSERT(illegal_handler_invoked); > > } else if (ctrinfo.type == SBI_PMU_CTR_TYPE_FW) { > > - read_fw_counter(i, ctrinfo); > > + /* Read without configure should fail */ > > + ret = sbi_ecall(SBI_EXT_PMU, > > SBI_EXT_PMU_COUNTER_FW_READ, > > + i, 0, 0, 0, 0, 0); > > + GUEST_ASSERT(ret.error == SBI_ERR_INVALID_PARAM); > > + > > + /* > > + * Try to configure with a common firmware event. > > + * If configuration succeeds, verify we can read it. > > + */ > > + ret = sbi_ecall(SBI_EXT_PMU, > > SBI_EXT_PMU_COUNTER_CFG_MATCH, > > + i, 1, 0, SBI_PMU_FW_ACCESS_LOAD, 0, 0); > > + if (ret.error == 0 && > > + ret.value < RISCV_MAX_PMU_COUNTERS && > > + (BIT(ret.value) & counter_mask_available)) > > + read_fw_counter(i, ctrinfo); > > We don't need the 'BIT(ret.value) & counter_mask_available' test. Instead > we should assert that ret.value == i. > > if (ret.error == 0) { > GUEST_ASSERT(ret.value == i); > read_fw_counter(i, ctrinfo); > } > > Thanks, > drew
