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

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