[kvm-unit-tests PATCH 1/3] riscv: sbi: Improve gen_report
Andrew Jones
andrew.jones at linux.dev
Mon Feb 3 03:26:47 PST 2025
On Mon, Feb 03, 2025 at 09:57:49AM +0100, Clément Léger wrote:
> Hey Andrew,
>
> While using it, I wondered if it would be helpful to display the
> expected return value even in the case it was successful and only
> display the return value if it was different. I feel like it avoid the
> need to specify it in the reported string, for instance,
>
> I have a few "test foo no error" or "test invalid event id 0x0". So that
> displaying them like:
>
> "test foo: SBI_SUCCESS"
> "test event id 0x0: SBI_INVALID_PARAM"
This looks nice and is easy to do now that we use a macro. Of course, in the
end, a PASSing line is almost never descriptive enough for people to truly
know what was tested anyway. If somebody wants to know what was actually
passing then they'll need to read the code. So as long as the strings are
unique, even if only by prefix, then we're pretty much good to go.
Thanks,
drew
>
> could be better. But that's only a personal taste.
> Anyway, I used these macros and it works has expected:
>
> Tested-by: Clément Léger <cleger at rivosinc.com>
> Reviewed-by: Clément Léger <cleger at rivosinc.com>
>
> Thanks,
>
> Clément
>
>
> On 29/01/2025 18:01, Andrew Jones wrote:
> > Make several improvements to gen_report(), starting by renaming it
> > to something less generic (sbiret_report) and then, instead of
> > relying on report prefix to link the report_info with the unexpected
> > return values to the failed test, use the test string itself, by
> > taking it as a parameter. And, reporting sbiret.error and
> > sbiret.value separately was more verbose than necessary since the
> > report_info can be used to see which one failed, so combine them.
> > Finally, return the pass/fail result of the test in case a caller
> > wants to use it.
> >
> > Signed-off-by: Andrew Jones <andrew.jones at linux.dev>
> > ---
> > riscv/sbi.c | 45 ++++++++++++++++++++++++---------------------
> > 1 file changed, 24 insertions(+), 21 deletions(-)
> >
> > diff --git a/riscv/sbi.c b/riscv/sbi.c
> > index 6f4ddaf13df3..9f591f8ff76a 100644
> > --- a/riscv/sbi.c
> > +++ b/riscv/sbi.c
> > @@ -156,19 +156,22 @@ static bool get_invalid_addr(phys_addr_t *paddr, bool allow_default)
> > return false;
> > }
> >
> > -static void gen_report(struct sbiret *ret,
> > - long expected_error, long expected_value)
> > -{
> > - bool check_error = ret->error == expected_error;
> > - bool check_value = ret->value == expected_value;
> > -
> > - if (!check_error || !check_value)
> > - report_info("expected (error: %ld, value: %ld), received: (error: %ld, value %ld)",
> > - expected_error, expected_value, ret->error, ret->value);
> > -
> > - report(check_error, "expected sbi.error");
> > - report(check_value, "expected sbi.value");
> > -}
> > +#define sbiret_report(ret, expected_error, expected_value, fmt, ...) ({ \
> > + long ex_err = expected_error; \
> > + long ex_val = expected_value; \
> > + bool ch_err = (ret)->error == ex_err; \
> > + bool ch_val = (ret)->value == ex_val; \
> > + bool pass = report(ch_err && ch_val, fmt, ##__VA_ARGS__); \
> > + \
> > + if (!pass) \
> > + report_info(fmt ": expected (error: %ld, value: %ld), received: (error: %ld, value %ld)", \
> > + ##__VA_ARGS__, ex_err, ex_val, (ret)->error, (ret)->value); \
> > + \
> > + pass; \
> > +})
> > +
> > +#define sbiret_check(ret, expected_error, expected_value) \
> > + sbiret_report(ret, expected_error, expected_value, "check sbi.error and sbi.value")
> >
> > static void check_base(void)
> > {
> > @@ -184,7 +187,7 @@ static void check_base(void)
> > expected = (long)strtoul(getenv("SBI_SPEC_VERSION"), NULL, 0);
> > assert_msg(!(expected & BIT(31)), "SBI spec version bit 31 must be zero");
> > assert_msg(__riscv_xlen == 32 || !(expected >> 32), "SBI spec version bits greater than 31 must be zero");
> > - gen_report(&ret, 0, expected);
> > + sbiret_check(&ret, 0, expected);
> > }
> > report_prefix_pop();
> >
> > @@ -199,7 +202,7 @@ static void check_base(void)
> > if (env_or_skip("SBI_IMPL_ID")) {
> > expected = (long)strtoul(getenv("SBI_IMPL_ID"), NULL, 0);
> > ret = sbi_base(SBI_EXT_BASE_GET_IMP_ID, 0);
> > - gen_report(&ret, 0, expected);
> > + sbiret_check(&ret, 0, expected);
> > }
> > report_prefix_pop();
> >
> > @@ -207,17 +210,17 @@ static void check_base(void)
> > if (env_or_skip("SBI_IMPL_VERSION")) {
> > expected = (long)strtoul(getenv("SBI_IMPL_VERSION"), NULL, 0);
> > ret = sbi_base(SBI_EXT_BASE_GET_IMP_VERSION, 0);
> > - gen_report(&ret, 0, expected);
> > + sbiret_check(&ret, 0, expected);
> > }
> > report_prefix_pop();
> >
> > report_prefix_push("probe_ext");
> > expected = getenv("SBI_PROBE_EXT") ? (long)strtoul(getenv("SBI_PROBE_EXT"), NULL, 0) : 1;
> > ret = sbi_base(SBI_EXT_BASE_PROBE_EXT, SBI_EXT_BASE);
> > - gen_report(&ret, 0, expected);
> > + sbiret_check(&ret, 0, expected);
> > report_prefix_push("unavailable");
> > ret = sbi_base(SBI_EXT_BASE_PROBE_EXT, 0xb000000);
> > - gen_report(&ret, 0, 0);
> > + sbiret_check(&ret, 0, 0);
> > report_prefix_popn(2);
> >
> > report_prefix_push("mvendorid");
> > @@ -225,7 +228,7 @@ static void check_base(void)
> > expected = (long)strtoul(getenv("MVENDORID"), NULL, 0);
> > assert(__riscv_xlen == 32 || !(expected >> 32));
> > ret = sbi_base(SBI_EXT_BASE_GET_MVENDORID, 0);
> > - gen_report(&ret, 0, expected);
> > + sbiret_check(&ret, 0, expected);
> > }
> > report_prefix_pop();
> >
> > @@ -233,7 +236,7 @@ static void check_base(void)
> > if (env_or_skip("MARCHID")) {
> > expected = (long)strtoul(getenv("MARCHID"), NULL, 0);
> > ret = sbi_base(SBI_EXT_BASE_GET_MARCHID, 0);
> > - gen_report(&ret, 0, expected);
> > + sbiret_check(&ret, 0, expected);
> > }
> > report_prefix_pop();
> >
> > @@ -241,7 +244,7 @@ static void check_base(void)
> > if (env_or_skip("MIMPID")) {
> > expected = (long)strtoul(getenv("MIMPID"), NULL, 0);
> > ret = sbi_base(SBI_EXT_BASE_GET_MIMPID, 0);
> > - gen_report(&ret, 0, expected);
> > + sbiret_check(&ret, 0, expected);
> > }
> > report_prefix_popn(2);
> > }
>
>
> --
> kvm-riscv mailing list
> kvm-riscv at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv
More information about the kvm-riscv
mailing list