On Thu, Feb 27, 2025 at 12:08 AM Andrew Jones <ajones@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxx>
