Re: Re: [PATCH v3 2/2] RISC-V: KVM: selftests: Fix firmware counter read in sbi_pmu_test

[Jiakai Xu]

Hi Nutty, 

Thanks for the review!
I'll fix the indentation and send a v5.

Best Regards,
Jiakai

"Nutty.Liu" <nutty.liu@xxxxxxxxxxx>wrote:
> 
> On 3/10/2026 1:55 PM, 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 <xujiakai2025@xxxxxxxxxxx>
> > Signed-off-by: Jiakai Xu <jiakaiPeanut@xxxxxxxxx>
> > ---
> > V2 -> V3:
> > - Removed unnecessary BIT(ret.value) & counter_mask_available check.
> > - Asserted ret.value == i after successful CFG_MATCH.
> > - Fixed eidx construction in SBI_EXT_PMU_COUNTER_CFG_MATCH.
> > ---
> >   .../testing/selftests/kvm/include/riscv/sbi.h | 37 +++++++++++++++++++
> >   .../selftests/kvm/riscv/sbi_pmu_test.c        | 20 +++++++++-
> >   2 files changed, 56 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..16f1815ac48f 100644
> > --- a/tools/testing/selftests/kvm/include/riscv/sbi.h
> > +++ b/tools/testing/selftests/kvm/include/riscv/sbi.h
> > @@ -97,6 +97,43 @@ 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 event types */
> > +enum sbi_pmu_event_type {
> > +	SBI_PMU_EVENT_TYPE_HW = 0x0,
> > +	SBI_PMU_EVENT_TYPE_CACHE = 0x1,
> > +	SBI_PMU_EVENT_TYPE_RAW = 0x2,
> > +	SBI_PMU_EVENT_TYPE_RAW_V2 = 0x3,
> > +	SBI_PMU_EVENT_TYPE_FW = 0xf,
> > +};
> > +
> >   /* 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..9404577e4ad5 100644
> > --- a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> > +++ b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> > @@ -436,6 +436,7 @@ static void test_pmu_basic_sanity(void)
> >   	struct sbiret ret;
> >   	int num_counters = 0, i;
> >   	union sbi_pmu_ctr_info ctrinfo;
> > +	unsigned long fw_eidx;
> >   
> >   	probe = guest_sbi_probe_extension(SBI_EXT_PMU, &out_val);
> >   	GUEST_ASSERT(probe && out_val == 1);
> > @@ -461,7 +462,24 @@ 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.
> > +			 */
> > +			fw_eidx = ((unsigned long)SBI_PMU_EVENT_TYPE_FW << 16) |
> > +					  SBI_PMU_FW_ACCESS_LOAD;
> Nit: Also need to align with previous line ?
> 
> Otherwise,
> Reviewed-by: Nutty Liu <nutty.liu@xxxxxxxxxxx>
> 
> Thanks,
> Nutty
> > +
> > +			ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_CFG_MATCH,
> > +					i, 1, 0, fw_eidx, 0, 0);
> > +			if (ret.error == 0) {
> > +				GUEST_ASSERT(ret.value == i);
> > +				read_fw_counter(i, ctrinfo);
> > +			}
> >   		}
> >   	}
> >