[kvm-unit-tests PATCH v6] riscv: sbi: Add SBI Debug Triggers Extension tests

[Andrew Jones]

On Fri, Jun 27, 2025 at 12:06:02PM -0700, Jesse Taube wrote:
> On Fri, Jun 27, 2025 at 6:02 AM Andrew Jones <andrew.jones at linux.dev> wrote:
> >
> > On Wed, Jun 18, 2025 at 08:44:52AM -0700, Jesse Taube wrote:
...
> > > +     report(shmem->data.tdata1 == tdata1, "tdata1 expected: 0x%016lx", tdata1);
> > > +     report_info("tdata1 found: 0x%016lx", shmem->data.tdata1);
> > > +     report(shmem->data.tdata2 == ((unsigned long)&test), "tdata2 expected: 0x%016lx",
> > > +            (unsigned long)&test);
> > > +     report_info("tdata2 found: 0x%016lx", shmem->data.tdata2);
> > > +     report(shmem->data.tstate == tstatus_expected, "tstate expected: 0x%016lx", tstatus_expected);
> > > +     report_info("tstate found: 0x%016lx", shmem->data.tstate);
> >
> > These don't need to be split into report/report_info pairs because the
> > report is checking for a specific value. We only split when report is
> > checking for some nonzero value and we also want to output what that
> > specific value was for informational purposes.
> 
> I'm a bit confused. Should it only report_info when the test fails?
> 

Usually, and that's what we do in sbiret_report().

I haven't been very good at describing my idea about this nor really
implementing it consistently throughout the framework. The idea is
that when platform specific or SBI implementation specific values are
involved that we should avoid putting them in report() strings in order
to ensure we can capture output from a successful run, where INFO lines
have been filtered out, and then subsequent runs can have their filtered
output directly diffed with that originally captured output, even if we
run on another platform / SBI implementation. I don't think we can achieve
that right now, but I try to keep it in mind when reviewing to avoid
making things worse. Besides the sbiret_report() approach we can also
always output values with report_info if that information may be useful,
i.e.

  report(check(value), "no values output here");
  report_info("value is %d");

or, with an expected value output

  expected = getenv("SOME_VAR");
  if (!report(value == expected, "value matches expectation (%d)", expected))
     report_info("value is %d");

The second approach won't allow diffing output captures arbitrarily, but
should still allow diffing for targets with the same configurations,
described by the environment variables, allowing the report lines to
contain more information.

Looking at your use above again, then, assuming the expected value is
something we want to always output, like in the environment variable
example, I guess there's nothing wrong with it. We could just avoid
the info lines on success with the if (!report(... guarding them.

...
> > > +void check_dbtr(void)
> > > +{
> > > +     static struct sbi_dbtr_shmem_entry shmem[RV_MAX_TRIGGERS] = {};
> > > +     unsigned long num_trigs;
> > > +     enum McontrolType trig_type;
> > > +     struct sbiret ret;
> > > +
> > > +     report_prefix_push("dbtr");
> > > +
> > > +     if (!sbi_probe(SBI_EXT_DBTR)) {
> > > +             report_skip("extension not available");
> > > +             report_prefix_pop();
> > > +             return;
> >
> > Could rename the 'error' label to something else and goto it from here
> > too.
> 
> Is `dtbr_exit_test` ok?

Sure, or even just exit_test, since this is a dbtr-only function.

Thanks,
drew