[kvm-unit-tests PATCH 1/3] riscv: sbi: Improve gen_report

[Clément Léger]

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"

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);
>  }