[PATCH 0/4] RISC-V KVM PMU fix and selftest improvement

[PATCH 0/4] RISC-V KVM PMU fix and selftest improvement

[Atish Patra]

This series adds a fix for KVM PMU code and improves the pmu selftest
by allowing generating precise number of interrupts. It also provided
another additional option to the overflow test that allows user to
generate custom number of LCOFI interrupts.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
Atish Patra (4):
      RISC-V: KVM: Disable the kernel perf counter during configure
      KVM: riscv: selftests: Do not start the counter in the overflow handler
      KVM: riscv: selftests: Change command line option
      KVM: riscv: selftests: Allow number of interrupts to be configurable

 arch/riscv/kvm/vcpu_pmu.c                        |  1 +
 tools/testing/selftests/kvm/riscv/sbi_pmu_test.c | 81 ++++++++++++++++--------
 2 files changed, 57 insertions(+), 25 deletions(-)
---
base-commit: 0ad2507d5d93f39619fc42372c347d6006b64319
change-id: 20250225-kvm_pmu_improve-fffd038b2404
--
Regards,
Atish patra

[PATCH 1/4] RISC-V: KVM: Disable the kernel perf counter during configure

[Atish Patra]

The perf event should be marked disabled during the creation as
it is not ready to be scheduled until there is SBI PMU start call
or config matching is called with auto start. Otherwise, event add/start
gets called during perf_event_create_kernel_counter function.
It will be enabled and scheduled to run via perf_event_enable during
either the above mentioned scenario.

Fixes: 0cb74b65d2e5 ("RISC-V: KVM: Implement perf support without sampling")

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 arch/riscv/kvm/vcpu_pmu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
index 2707a51b082c..78ac3216a54d 100644
--- a/arch/riscv/kvm/vcpu_pmu.c
+++ b/arch/riscv/kvm/vcpu_pmu.c
@@ -666,6 +666,7 @@ int kvm_riscv_vcpu_pmu_ctr_cfg_match(struct kvm_vcpu *vcpu, unsigned long ctr_ba
 		.type = etype,
 		.size = sizeof(struct perf_event_attr),
 		.pinned = true,
+		.disabled = true,
 		/*
 		 * It should never reach here if the platform doesn't support the sscofpmf
 		 * extension as mode filtering won't work without it.

-- 
2.43.0

[PATCH 2/4] KVM: riscv: selftests: Do not start the counter in the overflow handler

[Atish Patra]

There is no need to start the counter in the overflow handler as we
intend to trigger precise number of LCOFI interrupts through these
tests. The overflow irq handler has already stopped the counter. As
a result, the stop call from the test function may return already
supported error which is fine as well.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 tools/testing/selftests/kvm/riscv/sbi_pmu_test.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
index f45c0ecc902d..284bc80193bd 100644
--- a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
+++ b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
@@ -118,8 +118,8 @@ static void stop_counter(unsigned long counter, unsigned long stop_flags)
 
 	ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_STOP, counter, 1, stop_flags,
 			0, 0, 0);
-	__GUEST_ASSERT(ret.error == 0, "Unable to stop counter %ld error %ld\n",
-			       counter, ret.error);
+	__GUEST_ASSERT(ret.error == 0 || ret.error == SBI_ERR_ALREADY_STOPPED,
+		       "Unable to stop counter %ld error %ld\n", counter, ret.error);
 }
 
 static void guest_illegal_exception_handler(struct ex_regs *regs)
@@ -137,7 +137,6 @@ static void guest_irq_handler(struct ex_regs *regs)
 	unsigned int irq_num = regs->cause & ~CAUSE_IRQ_FLAG;
 	struct riscv_pmu_snapshot_data *snapshot_data = snapshot_gva;
 	unsigned long overflown_mask;
-	unsigned long counter_val = 0;
 
 	/* Validate that we are in the correct irq handler */
 	GUEST_ASSERT_EQ(irq_num, IRQ_PMU_OVF);
@@ -151,10 +150,6 @@ static void guest_irq_handler(struct ex_regs *regs)
 	GUEST_ASSERT(overflown_mask & 0x01);
 
 	WRITE_ONCE(vcpu_shared_irq_count, vcpu_shared_irq_count+1);
-
-	counter_val = READ_ONCE(snapshot_data->ctr_values[0]);
-	/* Now start the counter to mimick the real driver behavior */
-	start_counter(counter_in_use, SBI_PMU_START_FLAG_SET_INIT_VALUE, counter_val);
 }
 
 static unsigned long get_counter_index(unsigned long cbase, unsigned long cmask,

-- 
2.43.0

[PATCH 3/4] KVM: riscv: selftests: Change command line option

[Atish Patra]

The PMU test commandline option takes an argument to disable a
certain test. The initial assumption behind this was a common use case
is just to run all the test most of the time. However, running a single
test seems more useful instead. Especially, the overflow test has been
helpful to validate PMU virtualizaiton interrupt changes.

Switching the command line option to run a single test instead
of disabling a single test also allows to provide additional
test specific arguments to the test. The default without any options
remains unchanged which continues to run all the tests.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 tools/testing/selftests/kvm/riscv/sbi_pmu_test.c | 40 +++++++++++++++---------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
index 284bc80193bd..533b76d0de82 100644
--- a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
+++ b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
@@ -39,7 +39,11 @@ static bool illegal_handler_invoked;
 #define SBI_PMU_TEST_SNAPSHOT	BIT(2)
 #define SBI_PMU_TEST_OVERFLOW	BIT(3)
 
-static int disabled_tests;
+struct test_args {
+	int disabled_tests;
+};
+
+static struct test_args targs;
 
 unsigned long pmu_csr_read_num(int csr_num)
 {
@@ -604,7 +608,11 @@ static void test_vm_events_overflow(void *guest_code)
 	vcpu_init_vector_tables(vcpu);
 	/* Initialize guest timer frequency. */
 	timer_freq = vcpu_get_reg(vcpu, RISCV_TIMER_REG(frequency));
+
+	/* Export the shared variables to the guest */
 	sync_global_to_guest(vm, timer_freq);
+	sync_global_to_guest(vm, vcpu_shared_irq_count);
+	sync_global_to_guest(vm, targs);
 
 	run_vcpu(vcpu);
 
@@ -613,28 +621,30 @@ static void test_vm_events_overflow(void *guest_code)
 
 static void test_print_help(char *name)
 {
-	pr_info("Usage: %s [-h] [-d <test name>]\n", name);
-	pr_info("\t-d: Test to disable. Available tests are 'basic', 'events', 'snapshot', 'overflow'\n");
+	pr_info("Usage: %s [-h] [-t <test name>]\n", name);
+	pr_info("\t-t: Test to run (default all). Available tests are 'basic', 'events', 'snapshot', 'overflow'\n");
 	pr_info("\t-h: print this help screen\n");
 }
 
 static bool parse_args(int argc, char *argv[])
 {
 	int opt;
-
-	while ((opt = getopt(argc, argv, "hd:")) != -1) {
+	int temp_disabled_tests = SBI_PMU_TEST_BASIC | SBI_PMU_TEST_EVENTS | SBI_PMU_TEST_SNAPSHOT |
+				  SBI_PMU_TEST_OVERFLOW;
+	while ((opt = getopt(argc, argv, "h:t:n:")) != -1) {
 		switch (opt) {
-		case 'd':
+		case 't':
 			if (!strncmp("basic", optarg, 5))
-				disabled_tests |= SBI_PMU_TEST_BASIC;
+				temp_disabled_tests &= ~SBI_PMU_TEST_BASIC;
 			else if (!strncmp("events", optarg, 6))
-				disabled_tests |= SBI_PMU_TEST_EVENTS;
+				temp_disabled_tests &= ~SBI_PMU_TEST_EVENTS;
 			else if (!strncmp("snapshot", optarg, 8))
-				disabled_tests |= SBI_PMU_TEST_SNAPSHOT;
+				temp_disabled_tests &= ~SBI_PMU_TEST_SNAPSHOT;
 			else if (!strncmp("overflow", optarg, 8))
-				disabled_tests |= SBI_PMU_TEST_OVERFLOW;
+				temp_disabled_tests &= ~SBI_PMU_TEST_OVERFLOW;
 			else
 				goto done;
+			targs.disabled_tests = temp_disabled_tests;
 			break;
 		case 'h':
 		default:
@@ -650,25 +660,27 @@ static bool parse_args(int argc, char *argv[])
 
 int main(int argc, char *argv[])
 {
+	targs.disabled_tests = 0;
+
 	if (!parse_args(argc, argv))
 		exit(KSFT_SKIP);
 
-	if (!(disabled_tests & SBI_PMU_TEST_BASIC)) {
+	if (!(targs.disabled_tests & SBI_PMU_TEST_BASIC)) {
 		test_vm_basic_test(test_pmu_basic_sanity);
 		pr_info("SBI PMU basic test : PASS\n");
 	}
 
-	if (!(disabled_tests & SBI_PMU_TEST_EVENTS)) {
+	if (!(targs.disabled_tests & SBI_PMU_TEST_EVENTS)) {
 		test_vm_events_test(test_pmu_events);
 		pr_info("SBI PMU event verification test : PASS\n");
 	}
 
-	if (!(disabled_tests & SBI_PMU_TEST_SNAPSHOT)) {
+	if (!(targs.disabled_tests & SBI_PMU_TEST_SNAPSHOT)) {
 		test_vm_events_snapshot_test(test_pmu_events_snaphost);
 		pr_info("SBI PMU event verification with snapshot test : PASS\n");
 	}
 
-	if (!(disabled_tests & SBI_PMU_TEST_OVERFLOW)) {
+	if (!(targs.disabled_tests & SBI_PMU_TEST_OVERFLOW)) {
 		test_vm_events_overflow(test_pmu_events_overflow);
 		pr_info("SBI PMU event verification with overflow test : PASS\n");
 	}

-- 
2.43.0

[PATCH 4/4] KVM: riscv: selftests: Allow number of interrupts to be configurable

[Atish Patra]

It is helpful to vary the number of the LCOFI interrupts generated
by the overflow test. Allow additional argument for overflow test
to accommodate that. It can be easily cross-validated with
/proc/interrupts output in the host.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 tools/testing/selftests/kvm/riscv/sbi_pmu_test.c | 36 ++++++++++++++++++++----
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
index 533b76d0de82..7c273a1adb17 100644
--- a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
+++ b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
@@ -39,8 +39,10 @@ static bool illegal_handler_invoked;
 #define SBI_PMU_TEST_SNAPSHOT	BIT(2)
 #define SBI_PMU_TEST_OVERFLOW	BIT(3)
 
+#define SBI_PMU_OVERFLOW_IRQNUM_DEFAULT 5
 struct test_args {
 	int disabled_tests;
+	int overflow_irqnum;
 };
 
 static struct test_args targs;
@@ -478,7 +480,7 @@ static void test_pmu_events_snaphost(void)
 
 static void test_pmu_events_overflow(void)
 {
-	int num_counters = 0;
+	int num_counters = 0, i = 0;
 
 	/* Verify presence of SBI PMU and minimum requrired SBI version */
 	verify_sbi_requirement_assert();
@@ -495,11 +497,15 @@ static void test_pmu_events_overflow(void)
 	 * Qemu supports overflow for cycle/instruction.
 	 * This test may fail on any platform that do not support overflow for these two events.
 	 */
-	test_pmu_event_overflow(SBI_PMU_HW_CPU_CYCLES);
-	GUEST_ASSERT_EQ(vcpu_shared_irq_count, 1);
+	for (i = 0; i < targs.overflow_irqnum; i++)
+		test_pmu_event_overflow(SBI_PMU_HW_CPU_CYCLES);
+	GUEST_ASSERT_EQ(vcpu_shared_irq_count, targs.overflow_irqnum);
+
+	vcpu_shared_irq_count = 0;
 
-	test_pmu_event_overflow(SBI_PMU_HW_INSTRUCTIONS);
-	GUEST_ASSERT_EQ(vcpu_shared_irq_count, 2);
+	for (i = 0; i < targs.overflow_irqnum; i++)
+		test_pmu_event_overflow(SBI_PMU_HW_INSTRUCTIONS);
+	GUEST_ASSERT_EQ(vcpu_shared_irq_count, targs.overflow_irqnum);
 
 	GUEST_DONE();
 }
@@ -621,8 +627,11 @@ static void test_vm_events_overflow(void *guest_code)
 
 static void test_print_help(char *name)
 {
-	pr_info("Usage: %s [-h] [-t <test name>]\n", name);
+	pr_info("Usage: %s [-h] [-t <test name>] [-n <number of LCOFI interrupt for overflow test>]\n",
+		name);
 	pr_info("\t-t: Test to run (default all). Available tests are 'basic', 'events', 'snapshot', 'overflow'\n");
+	pr_info("\t-n: Number of LCOFI interrupt to trigger for each event in overflow test (default: %d)\n",
+		SBI_PMU_OVERFLOW_IRQNUM_DEFAULT);
 	pr_info("\t-h: print this help screen\n");
 }
 
@@ -631,6 +640,8 @@ static bool parse_args(int argc, char *argv[])
 	int opt;
 	int temp_disabled_tests = SBI_PMU_TEST_BASIC | SBI_PMU_TEST_EVENTS | SBI_PMU_TEST_SNAPSHOT |
 				  SBI_PMU_TEST_OVERFLOW;
+	int overflow_interrupts = -1;
+
 	while ((opt = getopt(argc, argv, "h:t:n:")) != -1) {
 		switch (opt) {
 		case 't':
@@ -646,12 +657,24 @@ static bool parse_args(int argc, char *argv[])
 				goto done;
 			targs.disabled_tests = temp_disabled_tests;
 			break;
+		case 'n':
+			overflow_interrupts = atoi_positive("Number of LCOFI", optarg);
+			break;
 		case 'h':
 		default:
 			goto done;
 		}
 	}
 
+	if (overflow_interrupts > 0) {
+		if (targs.disabled_tests & SBI_PMU_TEST_OVERFLOW) {
+			pr_info("-n option is only available for overflow test\n");
+			goto done;
+		} else {
+			targs.overflow_irqnum = overflow_interrupts;
+		}
+	}
+
 	return true;
 done:
 	test_print_help(argv[0]);
@@ -661,6 +684,7 @@ static bool parse_args(int argc, char *argv[])
 int main(int argc, char *argv[])
 {
 	targs.disabled_tests = 0;
+	targs.overflow_irqnum = SBI_PMU_OVERFLOW_IRQNUM_DEFAULT;
 
 	if (!parse_args(argc, argv))
 		exit(KSFT_SKIP);

-- 
2.43.0

Re: [PATCH 3/4] KVM: riscv: selftests: Change command line option

[Andrew Jones]

On Wed, Feb 26, 2025 at 12:25:05PM -0800, Atish Patra wrote:
> The PMU test commandline option takes an argument to disable a
> certain test. The initial assumption behind this was a common use case
> is just to run all the test most of the time. However, running a single
> test seems more useful instead. Especially, the overflow test has been
> helpful to validate PMU virtualizaiton interrupt changes.
> 
> Switching the command line option to run a single test instead
> of disabling a single test also allows to provide additional
> test specific arguments to the test. The default without any options
> remains unchanged which continues to run all the tests.
> 
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  tools/testing/selftests/kvm/riscv/sbi_pmu_test.c | 40 +++++++++++++++---------
>  1 file changed, 26 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> index 284bc80193bd..533b76d0de82 100644
> --- a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> +++ b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> @@ -39,7 +39,11 @@ static bool illegal_handler_invoked;
>  #define SBI_PMU_TEST_SNAPSHOT	BIT(2)
>  #define SBI_PMU_TEST_OVERFLOW	BIT(3)
>  
> -static int disabled_tests;
> +struct test_args {
> +	int disabled_tests;
> +};
> +
> +static struct test_args targs;
>  
>  unsigned long pmu_csr_read_num(int csr_num)
>  {
> @@ -604,7 +608,11 @@ static void test_vm_events_overflow(void *guest_code)
>  	vcpu_init_vector_tables(vcpu);
>  	/* Initialize guest timer frequency. */
>  	timer_freq = vcpu_get_reg(vcpu, RISCV_TIMER_REG(frequency));
> +
> +	/* Export the shared variables to the guest */
>  	sync_global_to_guest(vm, timer_freq);
> +	sync_global_to_guest(vm, vcpu_shared_irq_count);
> +	sync_global_to_guest(vm, targs);
>  
>  	run_vcpu(vcpu);
>  
> @@ -613,28 +621,30 @@ static void test_vm_events_overflow(void *guest_code)
>  
>  static void test_print_help(char *name)
>  {
> -	pr_info("Usage: %s [-h] [-d <test name>]\n", name);
> -	pr_info("\t-d: Test to disable. Available tests are 'basic', 'events', 'snapshot', 'overflow'\n");
> +	pr_info("Usage: %s [-h] [-t <test name>]\n", name);
> +	pr_info("\t-t: Test to run (default all). Available tests are 'basic', 'events', 'snapshot', 'overflow'\n");

It's probably fine to drop '-d', since we don't make any claims about
support, but doing so does risk breaking some CI somewhere. If that
potential breakage is a concern, then we could keep '-d', since nothing
stops us from having both.

>  	pr_info("\t-h: print this help screen\n");
>  }
>  
>  static bool parse_args(int argc, char *argv[])
>  {
>  	int opt;
> -
> -	while ((opt = getopt(argc, argv, "hd:")) != -1) {
> +	int temp_disabled_tests = SBI_PMU_TEST_BASIC | SBI_PMU_TEST_EVENTS | SBI_PMU_TEST_SNAPSHOT |
> +				  SBI_PMU_TEST_OVERFLOW;
> +	while ((opt = getopt(argc, argv, "h:t:n:")) != -1) {

'-h' doesn't need an argument and '-n' should be introduced with the next
patch.

>  		switch (opt) {
> -		case 'd':
> +		case 't':
>  			if (!strncmp("basic", optarg, 5))
> -				disabled_tests |= SBI_PMU_TEST_BASIC;
> +				temp_disabled_tests &= ~SBI_PMU_TEST_BASIC;
>  			else if (!strncmp("events", optarg, 6))
> -				disabled_tests |= SBI_PMU_TEST_EVENTS;
> +				temp_disabled_tests &= ~SBI_PMU_TEST_EVENTS;
>  			else if (!strncmp("snapshot", optarg, 8))
> -				disabled_tests |= SBI_PMU_TEST_SNAPSHOT;
> +				temp_disabled_tests &= ~SBI_PMU_TEST_SNAPSHOT;
>  			else if (!strncmp("overflow", optarg, 8))
> -				disabled_tests |= SBI_PMU_TEST_OVERFLOW;
> +				temp_disabled_tests &= ~SBI_PMU_TEST_OVERFLOW;
>  			else
>  				goto done;
> +			targs.disabled_tests = temp_disabled_tests;
>  			break;
>  		case 'h':
>  		default:
> @@ -650,25 +660,27 @@ static bool parse_args(int argc, char *argv[])
>  
>  int main(int argc, char *argv[])
>  {
> +	targs.disabled_tests = 0;
> +
>  	if (!parse_args(argc, argv))
>  		exit(KSFT_SKIP);
>  
> -	if (!(disabled_tests & SBI_PMU_TEST_BASIC)) {
> +	if (!(targs.disabled_tests & SBI_PMU_TEST_BASIC)) {
>  		test_vm_basic_test(test_pmu_basic_sanity);
>  		pr_info("SBI PMU basic test : PASS\n");
>  	}
>  
> -	if (!(disabled_tests & SBI_PMU_TEST_EVENTS)) {
> +	if (!(targs.disabled_tests & SBI_PMU_TEST_EVENTS)) {
>  		test_vm_events_test(test_pmu_events);
>  		pr_info("SBI PMU event verification test : PASS\n");
>  	}
>  
> -	if (!(disabled_tests & SBI_PMU_TEST_SNAPSHOT)) {
> +	if (!(targs.disabled_tests & SBI_PMU_TEST_SNAPSHOT)) {
>  		test_vm_events_snapshot_test(test_pmu_events_snaphost);
>  		pr_info("SBI PMU event verification with snapshot test : PASS\n");
>  	}
>  
> -	if (!(disabled_tests & SBI_PMU_TEST_OVERFLOW)) {
> +	if (!(targs.disabled_tests & SBI_PMU_TEST_OVERFLOW)) {
>  		test_vm_events_overflow(test_pmu_events_overflow);
>  		pr_info("SBI PMU event verification with overflow test : PASS\n");
>  	}
> 
> -- 
> 2.43.0
>

Otherwise,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Re: [PATCH 4/4] KVM: riscv: selftests: Allow number of interrupts to be configurable

[Andrew Jones]

On Wed, Feb 26, 2025 at 12:25:06PM -0800, Atish Patra wrote:
> It is helpful to vary the number of the LCOFI interrupts generated
> by the overflow test. Allow additional argument for overflow test
> to accommodate that. It can be easily cross-validated with
> /proc/interrupts output in the host.
> 
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  tools/testing/selftests/kvm/riscv/sbi_pmu_test.c | 36 ++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> index 533b76d0de82..7c273a1adb17 100644
> --- a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> +++ b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> @@ -39,8 +39,10 @@ static bool illegal_handler_invoked;
>  #define SBI_PMU_TEST_SNAPSHOT	BIT(2)
>  #define SBI_PMU_TEST_OVERFLOW	BIT(3)
>  
> +#define SBI_PMU_OVERFLOW_IRQNUM_DEFAULT 5
>  struct test_args {
>  	int disabled_tests;
> +	int overflow_irqnum;
>  };
>  
>  static struct test_args targs;
> @@ -478,7 +480,7 @@ static void test_pmu_events_snaphost(void)
>  
>  static void test_pmu_events_overflow(void)
>  {
> -	int num_counters = 0;
> +	int num_counters = 0, i = 0;
>  
>  	/* Verify presence of SBI PMU and minimum requrired SBI version */
>  	verify_sbi_requirement_assert();
> @@ -495,11 +497,15 @@ static void test_pmu_events_overflow(void)
>  	 * Qemu supports overflow for cycle/instruction.
>  	 * This test may fail on any platform that do not support overflow for these two events.
>  	 */
> -	test_pmu_event_overflow(SBI_PMU_HW_CPU_CYCLES);
> -	GUEST_ASSERT_EQ(vcpu_shared_irq_count, 1);
> +	for (i = 0; i < targs.overflow_irqnum; i++)
> +		test_pmu_event_overflow(SBI_PMU_HW_CPU_CYCLES);
> +	GUEST_ASSERT_EQ(vcpu_shared_irq_count, targs.overflow_irqnum);
> +
> +	vcpu_shared_irq_count = 0;
>  
> -	test_pmu_event_overflow(SBI_PMU_HW_INSTRUCTIONS);
> -	GUEST_ASSERT_EQ(vcpu_shared_irq_count, 2);
> +	for (i = 0; i < targs.overflow_irqnum; i++)
> +		test_pmu_event_overflow(SBI_PMU_HW_INSTRUCTIONS);
> +	GUEST_ASSERT_EQ(vcpu_shared_irq_count, targs.overflow_irqnum);
>  
>  	GUEST_DONE();
>  }
> @@ -621,8 +627,11 @@ static void test_vm_events_overflow(void *guest_code)
>  
>  static void test_print_help(char *name)
>  {
> -	pr_info("Usage: %s [-h] [-t <test name>]\n", name);
> +	pr_info("Usage: %s [-h] [-t <test name>] [-n <number of LCOFI interrupt for overflow test>]\n",
> +		name);
>  	pr_info("\t-t: Test to run (default all). Available tests are 'basic', 'events', 'snapshot', 'overflow'\n");
> +	pr_info("\t-n: Number of LCOFI interrupt to trigger for each event in overflow test (default: %d)\n",
> +		SBI_PMU_OVERFLOW_IRQNUM_DEFAULT);
>  	pr_info("\t-h: print this help screen\n");
>  }
>  
> @@ -631,6 +640,8 @@ static bool parse_args(int argc, char *argv[])
>  	int opt;
>  	int temp_disabled_tests = SBI_PMU_TEST_BASIC | SBI_PMU_TEST_EVENTS | SBI_PMU_TEST_SNAPSHOT |
>  				  SBI_PMU_TEST_OVERFLOW;
> +	int overflow_interrupts = -1;

Initializing to -1 made me think that '-n 0' would be valid and a way to
disable the overflow test, but...

> +
>  	while ((opt = getopt(argc, argv, "h:t:n:")) != -1) {
>  		switch (opt) {
>  		case 't':
> @@ -646,12 +657,24 @@ static bool parse_args(int argc, char *argv[])
>  				goto done;
>  			targs.disabled_tests = temp_disabled_tests;
>  			break;
> +		case 'n':
> +			overflow_interrupts = atoi_positive("Number of LCOFI", optarg);

...here we use atoi_positive() and...

> +			break;
>  		case 'h':
>  		default:
>  			goto done;
>  		}
>  	}
>  
> +	if (overflow_interrupts > 0) {

...here we only change from the default of 5 for nonzero.

Should we allow '-n 0'? Otherwise overflow_interrupts can be initialized
to zero (not that it matters).

> +		if (targs.disabled_tests & SBI_PMU_TEST_OVERFLOW) {
> +			pr_info("-n option is only available for overflow test\n");
> +			goto done;
> +		} else {
> +			targs.overflow_irqnum = overflow_interrupts;
> +		}
> +	}
> +
>  	return true;
>  done:
>  	test_print_help(argv[0]);
> @@ -661,6 +684,7 @@ static bool parse_args(int argc, char *argv[])
>  int main(int argc, char *argv[])
>  {
>  	targs.disabled_tests = 0;
> +	targs.overflow_irqnum = SBI_PMU_OVERFLOW_IRQNUM_DEFAULT;
>  
>  	if (!parse_args(argc, argv))
>  		exit(KSFT_SKIP);
> 
> -- 
> 2.43.0
>

Thanks,
drew

Re: [PATCH 2/4] KVM: riscv: selftests: Do not start the counter in the overflow handler

[Andrew Jones]

On Wed, Feb 26, 2025 at 12:25:04PM -0800, Atish Patra wrote:
> There is no need to start the counter in the overflow handler as we
> intend to trigger precise number of LCOFI interrupts through these
> tests. The overflow irq handler has already stopped the counter. As
> a result, the stop call from the test function may return already
> supported error which is fine as well.
  ^ stopped

> 
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  tools/testing/selftests/kvm/riscv/sbi_pmu_test.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> index f45c0ecc902d..284bc80193bd 100644
> --- a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> +++ b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> @@ -118,8 +118,8 @@ static void stop_counter(unsigned long counter, unsigned long stop_flags)
>  
>  	ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_STOP, counter, 1, stop_flags,
>  			0, 0, 0);
> -	__GUEST_ASSERT(ret.error == 0, "Unable to stop counter %ld error %ld\n",
> -			       counter, ret.error);
> +	__GUEST_ASSERT(ret.error == 0 || ret.error == SBI_ERR_ALREADY_STOPPED,
> +		       "Unable to stop counter %ld error %ld\n", counter, ret.error);
>  }
>  
>  static void guest_illegal_exception_handler(struct ex_regs *regs)
> @@ -137,7 +137,6 @@ static void guest_irq_handler(struct ex_regs *regs)
>  	unsigned int irq_num = regs->cause & ~CAUSE_IRQ_FLAG;
>  	struct riscv_pmu_snapshot_data *snapshot_data = snapshot_gva;
>  	unsigned long overflown_mask;
> -	unsigned long counter_val = 0;
>  
>  	/* Validate that we are in the correct irq handler */
>  	GUEST_ASSERT_EQ(irq_num, IRQ_PMU_OVF);
> @@ -151,10 +150,6 @@ static void guest_irq_handler(struct ex_regs *regs)
>  	GUEST_ASSERT(overflown_mask & 0x01);
>  
>  	WRITE_ONCE(vcpu_shared_irq_count, vcpu_shared_irq_count+1);
> -
> -	counter_val = READ_ONCE(snapshot_data->ctr_values[0]);
> -	/* Now start the counter to mimick the real driver behavior */
> -	start_counter(counter_in_use, SBI_PMU_START_FLAG_SET_INIT_VALUE, counter_val);
>  }
>  
>  static unsigned long get_counter_index(unsigned long cbase, unsigned long cmask,
> 
> -- 
> 2.43.0
>

Other than the commit message,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Re: [PATCH 1/4] RISC-V: KVM: Disable the kernel perf counter during configure

[Andrew Jones]

On Wed, Feb 26, 2025 at 12:25:03PM -0800, Atish Patra wrote:
> The perf event should be marked disabled during the creation as
> it is not ready to be scheduled until there is SBI PMU start call
> or config matching is called with auto start. Otherwise, event add/start
> gets called during perf_event_create_kernel_counter function.
> It will be enabled and scheduled to run via perf_event_enable during
> either the above mentioned scenario.
> 
> Fixes: 0cb74b65d2e5 ("RISC-V: KVM: Implement perf support without sampling")
> 
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  arch/riscv/kvm/vcpu_pmu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
> index 2707a51b082c..78ac3216a54d 100644
> --- a/arch/riscv/kvm/vcpu_pmu.c
> +++ b/arch/riscv/kvm/vcpu_pmu.c
> @@ -666,6 +666,7 @@ int kvm_riscv_vcpu_pmu_ctr_cfg_match(struct kvm_vcpu *vcpu, unsigned long ctr_ba
>  		.type = etype,
>  		.size = sizeof(struct perf_event_attr),
>  		.pinned = true,
> +		.disabled = true,
>  		/*
>  		 * It should never reach here if the platform doesn't support the sscofpmf
>  		 * extension as mode filtering won't work without it.
> 
> -- 
> 2.43.0
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Re: [PATCH 3/4] KVM: riscv: selftests: Change command line option

[Atish Kumar Patra]

On Thu, Feb 27, 2025 at 12:08 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Wed, Feb 26, 2025 at 12:25:05PM -0800, Atish Patra wrote:
> > The PMU test commandline option takes an argument to disable a
> > certain test. The initial assumption behind this was a common use case
> > is just to run all the test most of the time. However, running a single
> > test seems more useful instead. Especially, the overflow test has been
> > helpful to validate PMU virtualizaiton interrupt changes.
> >
> > Switching the command line option to run a single test instead
> > of disabling a single test also allows to provide additional
> > test specific arguments to the test. The default without any options
> > remains unchanged which continues to run all the tests.
> >
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> >  tools/testing/selftests/kvm/riscv/sbi_pmu_test.c | 40 +++++++++++++++---------
> >  1 file changed, 26 insertions(+), 14 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> > index 284bc80193bd..533b76d0de82 100644
> > --- a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> > +++ b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> > @@ -39,7 +39,11 @@ static bool illegal_handler_invoked;
> >  #define SBI_PMU_TEST_SNAPSHOT        BIT(2)
> >  #define SBI_PMU_TEST_OVERFLOW        BIT(3)
> >
> > -static int disabled_tests;
> > +struct test_args {
> > +     int disabled_tests;
> > +};
> > +
> > +static struct test_args targs;
> >
> >  unsigned long pmu_csr_read_num(int csr_num)
> >  {
> > @@ -604,7 +608,11 @@ static void test_vm_events_overflow(void *guest_code)
> >       vcpu_init_vector_tables(vcpu);
> >       /* Initialize guest timer frequency. */
> >       timer_freq = vcpu_get_reg(vcpu, RISCV_TIMER_REG(frequency));
> > +
> > +     /* Export the shared variables to the guest */
> >       sync_global_to_guest(vm, timer_freq);
> > +     sync_global_to_guest(vm, vcpu_shared_irq_count);
> > +     sync_global_to_guest(vm, targs);
> >
> >       run_vcpu(vcpu);
> >
> > @@ -613,28 +621,30 @@ static void test_vm_events_overflow(void *guest_code)
> >
> >  static void test_print_help(char *name)
> >  {
> > -     pr_info("Usage: %s [-h] [-d <test name>]\n", name);
> > -     pr_info("\t-d: Test to disable. Available tests are 'basic', 'events', 'snapshot', 'overflow'\n");
> > +     pr_info("Usage: %s [-h] [-t <test name>]\n", name);
> > +     pr_info("\t-t: Test to run (default all). Available tests are 'basic', 'events', 'snapshot', 'overflow'\n");
>
> It's probably fine to drop '-d', since we don't make any claims about
> support, but doing so does risk breaking some CI somewhere. If that
> potential breakage is a concern, then we could keep '-d', since nothing
> stops us from having both.

I don't think we have so much legacy usage with this test that we need
to maintain both options.
Since this was merged only a few cycles ago, I assume that it's not
available in many CI to cause breakage.
If somebody running CI actually shouts that it breaks their setup,
sure. Otherwise, I feel it will be just confusing to the users.

>
> >       pr_info("\t-h: print this help screen\n");
> >  }
> >
> >  static bool parse_args(int argc, char *argv[])
> >  {
> >       int opt;
> > -
> > -     while ((opt = getopt(argc, argv, "hd:")) != -1) {
> > +     int temp_disabled_tests = SBI_PMU_TEST_BASIC | SBI_PMU_TEST_EVENTS | SBI_PMU_TEST_SNAPSHOT |
> > +                               SBI_PMU_TEST_OVERFLOW;
> > +     while ((opt = getopt(argc, argv, "h:t:n:")) != -1) {
>
> '-h' doesn't need an argument and '-n' should be introduced with the next
> patch.
>

Yes. Thanks for catching it. I will fix it in v2.

> >               switch (opt) {
> > -             case 'd':
> > +             case 't':
> >                       if (!strncmp("basic", optarg, 5))
> > -                             disabled_tests |= SBI_PMU_TEST_BASIC;
> > +                             temp_disabled_tests &= ~SBI_PMU_TEST_BASIC;
> >                       else if (!strncmp("events", optarg, 6))
> > -                             disabled_tests |= SBI_PMU_TEST_EVENTS;
> > +                             temp_disabled_tests &= ~SBI_PMU_TEST_EVENTS;
> >                       else if (!strncmp("snapshot", optarg, 8))
> > -                             disabled_tests |= SBI_PMU_TEST_SNAPSHOT;
> > +                             temp_disabled_tests &= ~SBI_PMU_TEST_SNAPSHOT;
> >                       else if (!strncmp("overflow", optarg, 8))
> > -                             disabled_tests |= SBI_PMU_TEST_OVERFLOW;
> > +                             temp_disabled_tests &= ~SBI_PMU_TEST_OVERFLOW;
> >                       else
> >                               goto done;
> > +                     targs.disabled_tests = temp_disabled_tests;
> >                       break;
> >               case 'h':
> >               default:
> > @@ -650,25 +660,27 @@ static bool parse_args(int argc, char *argv[])
> >
> >  int main(int argc, char *argv[])
> >  {
> > +     targs.disabled_tests = 0;
> > +
> >       if (!parse_args(argc, argv))
> >               exit(KSFT_SKIP);
> >
> > -     if (!(disabled_tests & SBI_PMU_TEST_BASIC)) {
> > +     if (!(targs.disabled_tests & SBI_PMU_TEST_BASIC)) {
> >               test_vm_basic_test(test_pmu_basic_sanity);
> >               pr_info("SBI PMU basic test : PASS\n");
> >       }
> >
> > -     if (!(disabled_tests & SBI_PMU_TEST_EVENTS)) {
> > +     if (!(targs.disabled_tests & SBI_PMU_TEST_EVENTS)) {
> >               test_vm_events_test(test_pmu_events);
> >               pr_info("SBI PMU event verification test : PASS\n");
> >       }
> >
> > -     if (!(disabled_tests & SBI_PMU_TEST_SNAPSHOT)) {
> > +     if (!(targs.disabled_tests & SBI_PMU_TEST_SNAPSHOT)) {
> >               test_vm_events_snapshot_test(test_pmu_events_snaphost);
> >               pr_info("SBI PMU event verification with snapshot test : PASS\n");
> >       }
> >
> > -     if (!(disabled_tests & SBI_PMU_TEST_OVERFLOW)) {
> > +     if (!(targs.disabled_tests & SBI_PMU_TEST_OVERFLOW)) {
> >               test_vm_events_overflow(test_pmu_events_overflow);
> >               pr_info("SBI PMU event verification with overflow test : PASS\n");
> >       }
> >
> > --
> > 2.43.0
> >
>
> Otherwise,
>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Re: [PATCH 4/4] KVM: riscv: selftests: Allow number of interrupts to be configurable

[Atish Kumar Patra]

On Thu, Feb 27, 2025 at 12:16 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Wed, Feb 26, 2025 at 12:25:06PM -0800, Atish Patra wrote:
> > It is helpful to vary the number of the LCOFI interrupts generated
> > by the overflow test. Allow additional argument for overflow test
> > to accommodate that. It can be easily cross-validated with
> > /proc/interrupts output in the host.
> >
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> >  tools/testing/selftests/kvm/riscv/sbi_pmu_test.c | 36 ++++++++++++++++++++----
> >  1 file changed, 30 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> > index 533b76d0de82..7c273a1adb17 100644
> > --- a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> > +++ b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> > @@ -39,8 +39,10 @@ static bool illegal_handler_invoked;
> >  #define SBI_PMU_TEST_SNAPSHOT        BIT(2)
> >  #define SBI_PMU_TEST_OVERFLOW        BIT(3)
> >
> > +#define SBI_PMU_OVERFLOW_IRQNUM_DEFAULT 5
> >  struct test_args {
> >       int disabled_tests;
> > +     int overflow_irqnum;
> >  };
> >
> >  static struct test_args targs;
> > @@ -478,7 +480,7 @@ static void test_pmu_events_snaphost(void)
> >
> >  static void test_pmu_events_overflow(void)
> >  {
> > -     int num_counters = 0;
> > +     int num_counters = 0, i = 0;
> >
> >       /* Verify presence of SBI PMU and minimum requrired SBI version */
> >       verify_sbi_requirement_assert();
> > @@ -495,11 +497,15 @@ static void test_pmu_events_overflow(void)
> >        * Qemu supports overflow for cycle/instruction.
> >        * This test may fail on any platform that do not support overflow for these two events.
> >        */
> > -     test_pmu_event_overflow(SBI_PMU_HW_CPU_CYCLES);
> > -     GUEST_ASSERT_EQ(vcpu_shared_irq_count, 1);
> > +     for (i = 0; i < targs.overflow_irqnum; i++)
> > +             test_pmu_event_overflow(SBI_PMU_HW_CPU_CYCLES);
> > +     GUEST_ASSERT_EQ(vcpu_shared_irq_count, targs.overflow_irqnum);
> > +
> > +     vcpu_shared_irq_count = 0;
> >
> > -     test_pmu_event_overflow(SBI_PMU_HW_INSTRUCTIONS);
> > -     GUEST_ASSERT_EQ(vcpu_shared_irq_count, 2);
> > +     for (i = 0; i < targs.overflow_irqnum; i++)
> > +             test_pmu_event_overflow(SBI_PMU_HW_INSTRUCTIONS);
> > +     GUEST_ASSERT_EQ(vcpu_shared_irq_count, targs.overflow_irqnum);
> >
> >       GUEST_DONE();
> >  }
> > @@ -621,8 +627,11 @@ static void test_vm_events_overflow(void *guest_code)
> >
> >  static void test_print_help(char *name)
> >  {
> > -     pr_info("Usage: %s [-h] [-t <test name>]\n", name);
> > +     pr_info("Usage: %s [-h] [-t <test name>] [-n <number of LCOFI interrupt for overflow test>]\n",
> > +             name);
> >       pr_info("\t-t: Test to run (default all). Available tests are 'basic', 'events', 'snapshot', 'overflow'\n");
> > +     pr_info("\t-n: Number of LCOFI interrupt to trigger for each event in overflow test (default: %d)\n",
> > +             SBI_PMU_OVERFLOW_IRQNUM_DEFAULT);
> >       pr_info("\t-h: print this help screen\n");
> >  }
> >
> > @@ -631,6 +640,8 @@ static bool parse_args(int argc, char *argv[])
> >       int opt;
> >       int temp_disabled_tests = SBI_PMU_TEST_BASIC | SBI_PMU_TEST_EVENTS | SBI_PMU_TEST_SNAPSHOT |
> >                                 SBI_PMU_TEST_OVERFLOW;
> > +     int overflow_interrupts = -1;
>
> Initializing to -1 made me think that '-n 0' would be valid and a way to
> disable the overflow test, but...
>

Is there any benefit ? I found it much more convenient to select a
single test and run instead of disabling
a single test.

Once you single or a set of tests, all other tests are disabled anyways.

> > +
> >       while ((opt = getopt(argc, argv, "h:t:n:")) != -1) {
> >               switch (opt) {
> >               case 't':
> > @@ -646,12 +657,24 @@ static bool parse_args(int argc, char *argv[])
> >                               goto done;
> >                       targs.disabled_tests = temp_disabled_tests;
> >                       break;
> > +             case 'n':
> > +                     overflow_interrupts = atoi_positive("Number of LCOFI", optarg);
>
> ...here we use atoi_positive() and...
>
> > +                     break;
> >               case 'h':
> >               default:
> >                       goto done;
> >               }
> >       }
> >
> > +     if (overflow_interrupts > 0) {
>
> ...here we only change from the default of 5 for nonzero.
>
> Should we allow '-n 0'? Otherwise overflow_interrupts can be initialized
> to zero (not that it matters).
>

I will change the default value to 0 to avoid ambiguity for now.
Please let me know if you strongly think we should support -n 0.
We can always support it. I just don't see the point of specifying the
test with options to disable it anymore.

> > +             if (targs.disabled_tests & SBI_PMU_TEST_OVERFLOW) {
> > +                     pr_info("-n option is only available for overflow test\n");
> > +                     goto done;
> > +             } else {
> > +                     targs.overflow_irqnum = overflow_interrupts;
> > +             }
> > +     }
> > +
> >       return true;
> >  done:
> >       test_print_help(argv[0]);
> > @@ -661,6 +684,7 @@ static bool parse_args(int argc, char *argv[])
> >  int main(int argc, char *argv[])
> >  {
> >       targs.disabled_tests = 0;
> > +     targs.overflow_irqnum = SBI_PMU_OVERFLOW_IRQNUM_DEFAULT;
> >
> >       if (!parse_args(argc, argv))
> >               exit(KSFT_SKIP);
> >
> > --
> > 2.43.0
> >
>
> Thanks,
> drew

Re: [PATCH 4/4] KVM: riscv: selftests: Allow number of interrupts to be configurable

[Andrew Jones]

On Mon, Mar 03, 2025 at 01:27:47PM -0800, Atish Kumar Patra wrote:
> On Thu, Feb 27, 2025 at 12:16 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Wed, Feb 26, 2025 at 12:25:06PM -0800, Atish Patra wrote:
...
> I will change the default value to 0 to avoid ambiguity for now.
> Please let me know if you strongly think we should support -n 0.
> We can always support it. I just don't see the point of specifying the
> test with options to disable it anymore.
>

I don't mind not supporting '-n 0'.

Thanks,
drew