| Message ID | 20250226-kvm_pmu_improve-v1-3-74c058c2bf6d@rivosinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V KVM PMU fix and selftest improvement | expand |
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>
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>
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"); }
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(-)